@dankenigsberg i linked a bug for Libvirt in the PR description:
https://gitlab.com/libvirt/libvirt/-/issues/608

/hold

To avoid the validation error mentioned above we will disable
some of the features in the /usr/share/libvirt/cpu_map/[CPU Model].xml files:

disable svm for Opteron_G2 to avoid:
https://bugzilla.redhat.com/show_bug.cgi?id=2122283
disable mpx for Skylake, Cascadelake, and Icelake to avoid:
https://gitlab.com/libvirt/libvirt/-/issues/304

Same for

In addition, for future issues we allow setting
a config map called "cpu-model-to-features-to-disable"

The PR description does not explain why KubeVirt has to do this expensive task of manually tracking certain CPU features.
We should request 2 things

1. Why does libvirt not do this?
2. Why should KubeVirt do this?

Going forward this will be an ever growing list of cpu fetaures which need special treatment - why shoulda high level mgmt layer (KubeVirt) manage thos, and not something like a VMM runtime daemon (libvirt)?

/hold

To avoid the validation error mentioned above we will disable
some of the features in the /usr/share/libvirt/cpu_map/[CPU Model].xml files:

disable svm for Opteron_G2 to avoid:
https://bugzilla.redhat.com/show_bug.cgi?id=2122283
disable mpx for Skylake, Cascadelake, and Icelake to avoid:
https://gitlab.com/libvirt/libvirt/-/issues/304

I didn't open these bugs these are old bugs describing why we had to read the /usr/share/libvirt/cpu_map/[CPU Model].xml files.

Same for

In addition, for future issues we allow setting
a config map called "cpu-model-to-features-to-disable"

This is meant for fixing similar issues in the future or when we encounter problematic features. It's not part of the kubevirt API, and only administrators can use it since it is in the kubevirt installation namespace.

The PR description does not explain why KubeVirt has to do this expensive task of manually tracking certain CPU features. We should request 2 things

1. Why does libvirt not do this?
2. Why should KubeVirt do this?

Going forward this will be an ever growing list of cpu fetaures which need special treatment - why shoulda high level mgmt layer (KubeVirt) manage thos, and not something like a VMM runtime daemon (libvirt)?

Hey Fabian.
This is a temporary workaround to avoid failures until the bug is fixed.
We also included a way for a quick fix whever we hit the problem again.

I modifed the pr description to be clear on that and also added a bug i created for libvirt: https://gitlab.com/libvirt/libvirt/-/issues/608

Thanks for the notes

Can't we then wait for libvirt to fix the bug instead of working around it?

Can't we then wait for libvirt to fix the bug instead of working around it?

It might take a while for libvirt to fix the issue. From what I understand, there's a compatibility problem with QEMU, but I'm not sure about the details.

The new workaround replaces the old one, which is removed in the first commit of this PR. However, using the old workaround is not recommended by libvirt.

These are the advantages of the new approach:

• allowing broader CPU model(Cascadelake,Icelake,Skylake and more) support when svm or mpx features are absent.
• Included a way for a quick fix whever we hit the problem again.

It might take a while for libvirt to fix the issue. From what I understand, there's a compatibility problem with QEMU, but I'm not sure about the details.

The new workaround replaces the old one, which is removed in the first commit of this PR. However, using the old workaround is not recommended by libvirt.

These are the advantages of the new approach:

• allowing broader CPU model(Cascadelake,Icelake,Skylake and more) support when svm or mpx features are absent.
• Included a way for a quick fix whever we hit the problem again.

@fabiand I'll also add this to the PR description to make it more clear

Can't we then wait for libvirt to fix the bug instead of working around it?

@fabiand As I answered here

This is not directly related to the bug. This PR should allow KubeVirt to react and mitigate issues with CPU models quickly instead of waiting for fixes from libvirt and QEMU.
There can be all kinds of CPU vulnerabilities that can theoretically be mitigated on the cluster level quickly (Specter / Meltdown).
That's in addition to a known inconsistency between libvirt/QEMU wrt svm in a certain model.
mpx is just another case that could have been mitigated cluster-wide until a relevant fix arrives.

By default, no action is required from the cluster admin.

There can be all kinds of CPU vulnerabilities that can theoretically be mitigated on the cluster level quickly (Specter / Meltdown).

@vladikr is the root approver so he makes the call.

I take a different approach: these sorts of issues should be fixed in the relevant component (libvirt, qemu) and then consumed by KubeVirt via a rebuild. Exposing this in our API surface makes KubeVirt complicated, adding a burden on our users, tests and documentation. @Barakmor1 I'd appreciation if you keep cpu-model-to-features-to-disable in its separate commit, and include a documentation for it somewhere.

I take a different approach

That#s fine, but please recognize that this is a joint effort, @dankenigsberg

Thanks for the link @vladikr , it would just be good (and now it is) to have this in this pr description as well

@Barakmor1 if this is a mitigation - What is the complete workflow for an admin in order to safely use this feature?

The timeline

1. There is a bug detected in a CPU model (qemu/libvirt)
2. This feature is used to work around it
3. qemu/libvirt have fixed the issue
4. KubeVirt adopts the fix

I think it's clear how an admin would use the feature in order to workaround the issue, now some questions:

• How would VMs look that go created after step 2
• How would VMs work after step 3
• How would the admin remove the workaround again? What happens with the VMs impacted by this workaround?
• How high or low is the uncertainty that this workaround will or will not work?

I'm trying to widen the view in order to get us to understand what burden we are putting onto the admin.

@Barakmor1 can you help to find some answres to the questions above?
And what other questions should we look at?

@Barakmor1 do we know when https://gitlab.com/libvirt/libvirt/-/issues/608 will see some attention?
What are we doing to get this resolved by libvirt/qemu?

@Barakmor1 @vladikr FWIW wouldn't an alernative be to be able to define custom CPU models?
I.e. a user would specify a base model, and then add or remove flags and give it a custom name.

The diff to this proposal (IIUIC) would be: A custom CPU is independent of what libvirt/qemu provides. Aka the upgrade path would be clearer.

There can be all kinds of CPU vulnerabilities that can theoretically be mitigated on the cluster level quickly (Specter / Meltdown).

@vladikr is the root approver so he makes the call.

I take a different approach: these sorts of issues should be fixed in the relevant component (libvirt, qemu) and then consumed by KubeVirt via a rebuild. Exposing this in our API surface makes KubeVirt complicated, adding a burden on our users, tests and documentation. @Barakmor1 I'd appreciation if you keep cpu-model-to-features-to-disable in its separate commit, and include a documentation for it somewhere.

Hi @dankenigsberg ,

I appreciate the important notes.

I've added a link to Libvirt Issue in the comments.
The addition of the config map is now in a separate commit.

I'll also include a clear explanation in the documentation at https://github.com/kubevirt/user-guide. I'm currently working on a PR and will notify you once it's published.

Thanks for the link @vladikr , it would just be good (and now it is) to have this in this pr description as well

@Barakmor1 if this is a mitigation - What is the complete workflow for an admin in order to safely use this feature?

The timeline

1. There is a bug detected in a CPU model (qemu/libvirt)
2. This feature is used to work around it
3. qemu/libvirt have fixed the issue
4. KubeVirt adopts the fix

Yes, that's the process. Just a heads up: during the second stage, some existing VMs may need to be restarted.

This process is simpler compared to the current one, which involves disabling Node Labeller, adding labels to nodes, modifying VMs to disable the feature, restarting them, and manually disabling the feature for new VMs.

I think it's clear how an admin would use the feature in order to workaround the issue, now some questions:

• How would VMs look that go created after step 2
• How would VMs work after step 3

With this new workaround, the VMs would appear the same in all the steps. The only thing that needs to be done is to create a config map called "cpu-model-to-features-to-disable" in the installation namespace with the problematic feature and relevant models.

With the current(without that PR changes) workaround:

• The VMs of the second step would have to manually disable the problematic feature.
• The VMs of the third step would look the same as before the issue (the problematic feature would be disabled implicitly by default).

• How would the admin remove the workaround again? What happens with the VMs impacted by this workaround?
• How high or low is the uncertainty that this workaround will or will not work?

With the workaround, we will list features that are deprecated and should not be enabled, such as mpx in a ConfigMap.
If the VM explicitly requires mpx in the VM spec, like in this example:

spec:
  domain:
    cpu:
      model: Skylake-Client-noTSX-IBRS
      features:
      - name: "mpx"
        policy: "require"

The workaround will not affect anything, and the VM will enable mpx even though it exists in the map. The workaround will only help admins disable deprecated features that should not be used anyway. The libvirt/qemu/kubevirt fix will ensure that these features are disabled by default, even without the workaround.

So there is no risk if an admin forgets to remove the ConfigMap, as this won't affect anything.

@Barakmor1 do we know when https://gitlab.com/libvirt/libvirt/-/issues/608 will see some attention? What are we doing to get this resolved by libvirt/qemu?

There is a discussion regarding this issue with the Libvirt team in a mailing list. Hopefully, this will be addressed soon :) .

@Barakmor1 @vladikr FWIW wouldn't an alernative be to be able to define custom CPU models? I.e. a user would specify a base model, and then add or remove flags and give it a custom name.

The diff to this proposal (IIUIC) would be: A custom CPU is independent of what libvirt/qemu provides. Aka the upgrade path would be clearer.

This is similar to what we are doing here: the features in the map will be disabled automatically for some CPU models. There is no need to add features if the user didn't request additional features in the VMI's spec.

@dankenigsberg here is the documentation PR:
kubevirt/user-guide#771

@fabiand can we remove the hold label?

@Barakmor1 @vladikr FWIW wouldn't an alernative be to be able to define custom CPU models? I.e. a user would specify a base model, and then add or remove flags and give it a custom name.

@fabiand partially this is possible today already. https://kubevirt.io/user-guide/virtual_machines/virtual_hardware/#features
We're only missing a way to disable a specific CPU feature, which can be added.

I don't see how this is an alternative to a cluster-wide approach. This approach puts a burden on the guest user who may not be away for CVEs and cluster wide issues.

This PR is about allowing the admin to react quickly to known issues until fixes from lower layered projects will come.

The diff to this proposal (IIUIC) would be: A custom CPU is independent of what libvirt/qemu provides. Aka the upgrade path would be clearer.

/unhold

Hi.

@Barakmor1 @vladikr the topic got started because of the mpx flag, iiiuic.
Ignoring this work, and Barak's fix in the other PR: Could we have also mitigated the issue by setting

features:
- name: mpx
  policy: disable

?

Hi.

@Barakmor1 @vladikr the topic got started because of the mpx flag, iiiuic. Ignoring this work, and Barak's fix in the other PR: Could we have also mitigated the issue by setting

features:
- name: mpx
  policy: disable

?

Yes, for each new VM, we'll need to disable mpx through the vmi.spec . Additionally, we'll need to update the existing ones to disable mpx and then restart them.

Suggestions have been raised and they are still coming. For now, I would like to understand why this cannot be resolved by instancetypes [1].

I suppose for the same reason why device plugins are not managed via instance-types.

Sorry, I do not see the relation. IIUC we can use instancetypes to share a common configuration over a group of VMs. The only missing peace is to have a "default" one that we do not need to specify per VM but per cluster (or per a group of them). If we could express that we want an instancetype to be added for all VMs through Kubevirt CR, then we could set whatever fields we want, including these CPU features settings.

The instancetypes's goal is different and they come with limitations, you can't use them in all cases.

Is this something doable?

[1] #11407 (comment)
The notion of creating a new API to try and resolve an intermediate problem until formal release is surprising to me. Do we have examples from other projects that do stuff like this?

As Barak has repeatedly explained, this PR was not created to address a bug. This PR allows KubeVirt to manage the availability/enablement of CPU features cluster-side to mitigate either security issues or bugs.

"mitigate either security issues or bugs" sounds exactly like treating something intermediate to me.

I am not sure what you mean here. Is an API designated to help with intermediate API not allowed? Is Kubevirt not allowed to provide better user/admin experience? I would argue that faster security patches or bug fixes are worth the additional (low) maintenance.

We need to take into account that Kubevirt is just one layer on top of many and each layer can take considerable time to provide fixes.

In general, CPU features and models are managed by QEMU. libvirt has a layer to verify its usage and rationalize these. In most cases, KubeVirt relies on libvirt for that matter, however, in some cases, KubeVirt needs to have a way to deviate from this approach temporarily. (re: description)

That is what I understood as well. We need this for a temporary period of time until it is treated correctly. I guess my problem here is that IMO introducing an API just to solve an intermediate need like this is not worth it. The solution is to release a fix quickly and until then to find a mitigating solution that is ugly but works. Several such solutions have been given.

My main issue with the suggested API is its too specific without an option to leverage it for other reasonable uses. It is down to creating another method to "template" parameters over the whole cluster VMs. But this time it is focused on CPU features being disabled.

@Barakmor1 thanks.

Now, if we divide our small KubeVirt world into two phases: Env with a problematic CPU model , and an Env with a fixed CPU model, then we are saying

1. This PR is needed to target the "BEFORE" environment
2. We assume that eventually qemu libvirt us will deliver a fix which will take us to the "AFTER" environment

Thus we look for a temporary measure to fix one or more (possibly not all) VMs on a cluster to get through the time before "AFTER" appears.

I have my concerns - raised above - with the mangling of CPU models (now sharpening my concerns) on our KubeVirt level. I really wonder - and eventually worry - how we keep track of it, and how to mitigate or rather foresee the impact of this.
This being said we now know, that the manual reconfiguration of a VM is addressing the "BEFORE" case and can be applied to one or more VMs. And we know that a specific fix like #11671 would allow KubeVirt to quickly target a specific CPU issue.

For multiple VMs a webhook, a tool like Kyverno, or something like the patch-operator) could help to do the job.
Having it in the API makes this also very explicit, and we do not need to have any additional measures to protect ourselfs, or guide admins how to uprgade/remove these flags again.

TLDR We have two strategies to address critical situations (kubevirt fix and CPU features on VMI API), automation tools could be used to address multiple VMs. This implementation in this PR willr equire a mainteinance burden, additional knowledge by admins, and possibly other things we don't know of yet.
Due to this I wonder if we should simply settle with what we have (VMI CPU API, kubevirt fixes as needed) and close this topic and this pr.

@fabiand For me, relying on external tools to set a cluster wide policy that only affects VMs is not good enough.
This way, we could say that managing device plugins should also be done outside of KubeVirt.

This API is specific because it should enforce a temporary policy for all VMs without affecting the VM/VMI spec.

Also, it could be that admins would want to disable certain CPU features and make these unavailable. For explain, SVM was part of the QEMU CPU models.

TLDR We have two strategies to address critical situations (kubevirt fix and CPU features on VMI API), automation tools could be used to address multiple VMs. This implementation in this PR willr equire a mainteinance burden, additional knowledge by admins, and possibly other things we don't know of yet.

Managing CPU flags on a VM level cannot be considered a cluster-wide strategy or an admins policy.

In addition, I'm not sure whether CPU

features:
- name: mpx
  policy: disable

can work with instance types.

@fabiand @vladikr BTW another intresting usecase:
harvester/harvester#3015 (comment)

The instancetypes's goal is different and they come with limitations, you can't use them in all cases.

My understanding is that instancetypes are a kind of template, allowing a user to share a configuration snippet across a group of VMs. In my view, this is exactly what you are trying to do here with the CPU feature disablement.
In any case, this is an example of how one can share a central configuration with many VM/VMIs.

"mitigate either security issues or bugs" sounds exactly like treating something intermediate to me.

I am not sure what you mean here. Is an API designated to help with intermediate API not allowed?

It is allowed, but it is not recommended.
Creating an API to solve intermediate glitches is a sign of a problem.

Is Kubevirt not allowed to provide better user/admin experience? I would argue that faster security patches or bug fixes are worth the additional (low) maintenance.

I guess this is exactly where we see things differently.
As an Admin, I would not want you to fix things using configuration, but using a release. Quickly and efficiently without me needing to manage any configuration.

We need to take into account that Kubevirt is just one layer on top of many and each layer can take considerable time to provide fixes.

My expectation is for Kubevirt to ideally provide the mitigation through a hotfix release and not through configuration. Then, it is up to Kubevirt to pressure the layered drivers it uses to deliver a proper fix.
The management is done internally and the Admin is not bothered with configuration burden.

I was unable to understand why a configuration API is better than a quick, hotfix release with the same logic embedded into Kubevirt and managed by it until a proper resolution is found.

Deferring the problem to the Admin using a configuration workaround sounds problematic to me.

My expectation is for Kubevirt to ideally provide the mitigation through a hotfix release and not through configuration. Then, it is up to Kubevirt to pressure the layered drivers it uses to deliver a proper fix. The management is done internally and the Admin is not bothered with configuration burden.

I was unable to understand why a configuration API is better than a quick, hotfix release with the same logic embedded into Kubevirt and managed by it until a proper resolution is found.

Deferring the problem to the Admin using a configuration workaround sounds problematic to me.

I find it had to agree with this. As an admin who is dealing with a security issue or an inconsistency that affects the cluster, I'd prefer to have a simple configuration rather than wait for an update and be exposed while the hotfix arrives.

Issues that are described in this PR have been waiting for a solution for many months. Even though some of these issues are fixed in some layers, we still cannot get a sufficient release.
KubeVirt can control its layer. And this API aims to let KubeVirt provide the flexibility needed to address issues as they appear.
Other platforms have this API and even more extensive (per node) configuration - which solved issues in the past.

My understanding is that instancetypes are a kind of template, allowing a user to share a configuration snippet across a group of VMs. In my view, this is exactly what you are trying to do here with the CPU feature disablement.
In any case, this is an example of how one can share a central configuration with many VM/VMIs.

Not all VMIs are started from a VM that references an instance type.

My understanding is that instancetypes are a kind of template, allowing a user to share a configuration snippet across a group of VMs. In my view, this is exactly what you are trying to do here with the CPU feature disablement.
In any case, this is an example of how one can share a central configuration with many VM/VMIs.

Not all VMIs are started from a VM that references an instance type.

I understand it is not perfect.
However we have many features that depend on a VM and not VMI, so it is not that terrible IMO.

Personally, if VMIs need to be treated as a must, I would prefer templating to be extended to VMIs over adding a new specific CPU-feature disabling API. But I do not think it is desired.

tl;dr instance types (well preferences) are not a valid solution to this issue

Chiming in late with my $0.05 on instance types (well preferences) somehow being another way to fix this. IMHO they are not. AFAICT we would end up in the middle ground between the admin driven workflow proposed by this PR and the current purely VM owner driven workflow.

Instance types are resource containers and do not model CPU features [1]. Preferences can provide preferred CPU Features but these can be overridden within the VM definition [2]. Both only apply to the VM and have a 1:1 point in time relationship with the VM making it awkward to apply cluster-wide updates to already deployed VMs.

Introducing preferences into this mix means that an admin would need to mutate all VirtualMachineClusterPreferences to disable certain CPUFeatures and then move all existing VMs to use the new version of the preference. That in itself might move the VM from a much older generation of the resource to something totally new that itself might be undesirable. VM owners might also have their own namespaced VirtualMachinePreferences where they also need to repeat these steps.

Hopefully this brings the idea of using instance types and preferences to solve this particular use cases to a close.

I've not been following this PR closely enough to provide a complete review but at a high level providing an admin driven workflow over something involving a VM owner is IMHO the way to go.

[1] https://kubevirt.io/api-reference/main/definitions.html#_v1beta1_cpuinstancetype
[2] https://kubevirt.io/api-reference/main/definitions.html#_v1beta1_cpupreferences

/cc

@Barakmor1: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

/hold

Simply due to the long discussion

PR needs rebase.

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

/lifecycle stale

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

/lifecycle rotten

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

/close

@kubevirt-bot: Closed this PR.