-
Notifications
You must be signed in to change notification settings - Fork 16
Add hw inventory capability #135
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| message OptionalCapabilities { | ||
| // Virtualization type Kubevirt | ||
| bool hv_type_kubevirt = 1; | ||
| bool hw_inventory_support = 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure if it should be OptionalCapability or APICapability...
I understand APICapability as that from now on every EVE version is supporting this capability, while OptionalCapability is used for capabilities that not necessarily is supported in all EVE versions from now on.
As the hw inventory is supported by all future EVE versions, I would guess it should be an APICapability?
Or should be just abandon APICapability because of it's shortcomings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The OptionalCapabilities makes more sense if only a subset of EVE nodes (for any given version) will support it, or if we are certain we will remove it in future versions.
But otherwise the more compact APICapability makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I understood those fields from reading the docs:
- -
Lines 698 to 703 in b9d7403
// Capabilities indicates features in the EdgeDevConfig where there is // no easy way to otherwise determine whether or not they are parsed and // supported by EVE-OS // A larger number indicates all lower numbers are also supported thus // this works similar to a version field for the EdgeDevConfig support. enum APICapability { APICapabilitydescribes some feature of EVE that can be turned on / configured throughEdgeDevConfig. So the controller needs to know that if it turns it on inEdgeDevConfigit will indeed have effect on EVE - -
Lines 689 to 691 in b9d7403
// OptionalCapabilities indicates any additional capabilities device wants // to publish to controller. For example Kubevirt hypervisor is not supported by // all eve flavors. OptionalCapabilitiesis something that's not available in every EVE version (you're right) but also something that EVE wants to tell to the controller about itself - not related toEdgeDevConfig- more like a passive feature, like what hypervisor it uses. That sounds to me more like "I will send HW inventory, you don't need to do anything, just be aware".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eriknordmark what do you think?
Signal to the controller that it should expect a hardware inventory report from the device. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
Add generate go and python code for info.proto. Signed-off-by: Paul Gaiduk <paulg@zededa.com>
b9d7403 to
f690df4
Compare
eriknordmark
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're correct that this doesn't affect what needs to be configured so let's not put it in api capability. Just check with @zed-rishabh that he doesn't see issues with the controller accessing this field to decide how to proceed.
Signal to the controller that it should expect a hardware inventory report from the device.