Add device capability HVTypeKubevirt to message OptionalCapabilities

[zedi-pramodh]

Add HVTypeKubevirt as a device capability

Also, add the missing capabilities CPUPinning and VHost.
These capabilities should match the Capabilities published by domainmgr in eve.
Even if those missing capabilities are not used by Controller, lets publish those
for consistency.

[eriknordmark]

The message your are extending says "// Information about hardware capabilities of node" and these are software/API capabilities.

There is an api_capability field which is what you want to extend here.
Since that is a cumulative number (see associated comments) there is no utility in filling in things we've already implemented and deployed. So just adding one about kubevirt should be sufficient.

[zedi-pramodh]

We cannot use api_capability IMO. That's a hardcoded value and will be used for kvm based eve too !
Can we compile time generate that ?

[uncleDecart]

@zedi-pramodh I agree with Erik on this. What you describe here is Software capability of specific hypervisor. So it would be confusing to developers if we have software capability in hardware capabilities. Moreover, this field will depend on EVE version which makes backward compatibility trickier.
Perhaps, it might make sense to structure software capabilities in API

[eriknordmark]

We cannot use api_capability IMO. That's a hardcoded value and will be used for kvm based eve too ! Can we compile time generate that ?

We can make it report different api_capabilities based on eve-hytype.

However, when we introduce the next api capability we will end up with holes on the sequence with some implement "next" + kubevirt + current set, and others implementing "next" + current set.
So we need to start planning for reporting a map of eve api capabilities and not just the largest number (something we knew we'd need to do sooner or later). Question is whether we want to do that as an opt-in or opt-out. For kube-virt the opt out would be efficient - can express "next" + current set except kubevirt.

But if we want to have EVE agents which implement a small subset of the API down the road then an opt in might be more clear? In any case, we don't need to solve that until we need to add the next api capability after this.

[zedi-pramodh]

@naiming-zededa @milan-zededa please provide your inputs too.

[naiming-zededa]

@naiming-zededa @milan-zededa please provide your inputs too.

I think Erik's suggestion on temporary solution for kubevirt is fine with me. Basically we add 'kubevirt' to the end of the this api-capability enum. Only kubevirt eve software set this, kvm will just use 'kubevirt' - 1 number. If later we want to add one more capability in the future, we need to redesign this to a map structure for api-capability, but that is for later to handle properly in a separate effort.

current APICapability 'Snapshot' uses 5, maybe Kubevirt can be added as '10', so later on new capability if is only related to EVE version support, can still use number 6-9 (kubevirt also needs to add 11-14 definitions), thus further kicking the can down the road :-)

[milan-zededa]

IMHO this is neither HW nor SW capability, rather a field informing about which EVE OS variant is running on a given device. Therefore, it might make sense to add (more generic) Hypervisor enum into ZInfoDevice, perhaps under a new OSInfo structure (or into existing ZInfoDevSW)?

[eriknordmark]

current APICapability 'Snapshot' uses 5, maybe Kubevirt can be added as '10', so later on new capability if is only related to EVE version support, can still use number 6-9 (kubevirt also needs to add 11-14 definitions), thus further kicking the can down the road :-)

FWIW that wouldn't work since we'd be making the statement that the kubevirt image we ship today would support all the (undefined) capabilities with APICapability < 10.

[eriknordmark]

IMHO this is neither HW nor SW capability, rather a field informing about which EVE OS variant is running on a given device. Therefore, it might make sense to add (more generic) Hypervisor enum into ZInfoDevice, perhaps under a new OSInfo structure (or into existing ZInfoDevSW)?

Good point.

If it is not about particular capabilities but effectively sending /run/eve-hv-type to the controller, then it would make sense to define that in the info message (maybe in the hwinfo since it doesn't change unless the device is rebooted).
But it all depends what decisions we think the controller will make based on this. Do we know what those planned decisions hence implied semantivcs are today?

[zedi-pramodh]

But it all depends what decisions we think the controller will make based on this. Do we know what those planned decisions hence implied semantivcs are today?

Current plan is to use this field to support native container deployments and as well as any storage/compute clustering related features.

[naiming-zededa]

Should we leave this API-capability along since it's basically a software-version, and just adding another 'capability', say message OptionalCapability { bool HvTypeKubevirt = 1 } to start with, it seems cleaner.

[zedi-pramodh]

We can do that @naiming-zededa . Just that controller/UI need new code to process that new message. I am fine with whatever the decision is.

[naiming-zededa]

We can do that @naiming-zededa . Just that controller/UI need new code to process that new message. I am fine with whatever the decision is.

it should be similar for controller/UI to check either something called 'apicapacity' or the 'optionalcapability' items, not much difference in this regard.

[eriknordmark]

Should we leave this API-capability along since it's basically a software-version, and just adding another 'capability', say message OptionalCapability { bool HvTypeKubevirt = 1 } to start with, it seems cleaner.

Might want to name that in plural (OptionalCapabilities) since I assume the intent is to potentially add additional items here over time (even though we don't yet know what they might be.)

[naiming-zededa]

'OptionalCapabilities' sounds good to me. yes, for sure it is meant to be later on adding more optional software capabilities to use

[zedi-pramodh]

We can do that @naiming-zededa . Just that controller/UI need new code to process that new message. I am fine with whatever the decision is.

it should be similar for controller/UI to check either something called 'apicapacity' or the 'optionalcapability' items, not much difference in this regard.

I know. But apicapability code already exists in controller/UI hence adding additional capability does not require code changes at controller side. With this new message optional capabilities some change is required in controller.

But that is fine, I will amend this commit.

[zedi-pramodh]

Amended the commit

[eriknordmark]

Can you do this in lower_snake_case as yetus suggests?
protoc will make hv_type_kubevirt into HvTypeKubevirt in the golang definition.

[zedi-pramodh]

Done, amended the commit

[milan-zededa]

This is OK, although I still think that it might be useful to publish Hypervisor enum as part of device info.
Then in the future we might be able to search for devices using hypervisor XYZ in the controller database etc. I don't know if we can rely on EVE image name suffix for this.

Plus HVTypeKubevirt does not sound like capability, more like a property.
Capabilities could be:

• supports native containers
• supports clustering
• etc.

[eriknordmark]

LGTM

[zedi-pramodh]

This is OK, although I still think that it might be useful to publish Hypervisor enum as part of device info. Then in the future we might be able to search for devices using hypervisor XYZ in the controller database etc. I don't know if we can rely on EVE image name suffix for this.

Plus HVTypeKubevirt does not sound like capability, more like a property. Capabilities could be:

• supports native containers
• supports clustering
• etc.

We are not picking by eve image name. Its actually eve hypervisor type which we generate during build and save in /etc/eve-hv-type file. Kubevirt type of virtualization is also a capability of this device. But I do see your point that we are overriding a lot on just one virtualization type. Ideally we should have hierarchy under Kubevirt type, may instead of bool it should be a map. We should think about it.