Add Windows VMs vCPU overcommit prevention VAP #2535

jcanocan · 2025-09-26T10:27:47Z

What type of PR is this?

Feature

What this PR does / why we need it?

It is required to enable some sort of protection in Windows based VMs to prevent them to overcommit vCPUs. To achieve this, it is assumed that CPUmanager is enabled on, and only on, Windows Licensed nodes. This is due to the fact that, during the scheduling process, any VM with spec.domain.cpu.dedicatedCpuPlacement will be assigned to nodes with cpumanager feature enabled. Therefore, the scheduler will effectively assign those VMs to Windows Licensed nodes. Moreover, if the VM does not overcommit, vCPUs will be allowed to run.

This approach has the following drawbacks as it is:

All Windows VMs created with supported workflows will be required to no overcommit vCPUs.
It does not prevent users to manually create their own custom Windows VM manifest/preference and overcommit vCPUs.
User will be still allowed to schedule non-windows VMs overcommitting CPUs to Windows Licensed nodes.

Which Jira/Github issue(s) this PR fixes?

Fixes #

Special notes for your reviewer:

Pre-checks (if applicable):

Tested latest changes against a cluster
Included documentation changes with PR
If this is a new object that is not intended for the FedRAMP environment (if unsure, please reach out to team FedRAMP), please exclude it with:
```
matchExpressions:
- key: api.openshift.com/fedramp
  operator: NotIn
  values: ["true"]
```

app-sre-bot · 2025-09-26T10:27:51Z

Can one of the admins verify this patch?

openshift-ci · 2025-09-26T10:28:31Z

Hi @jcanocan. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

dominikholler · 2025-09-26T10:37:35Z

deploy/acm-policies/50-GENERATED-srep-vap-vcpu-overcommit.Policy.yaml

+                                        - virtualmachineinstances
+                            validations:
+                                - expression: has(object.spec.domain.cpu.dedicatedCpuPlacement) && object.spec.domain.cpu.dedicatedCpuPlacement == true
+                                  message: Windows VMIs require dedicated CPU placement. Set spec.domain.cpu.dedicatedCpuPlacement to true.


spec.domain.cpu.dedicatedCpuPlacement is quite low level. Can we point the user to an instancetype to use? If there is no instancetype available yet, let's create one

Yes, we can also follow that path. Great idea. In such case, the drawbacks will still apply. However, we will have a 100% supported way of creating Windows VMs, which is a nice advantage compared with my orginal proposal.

After thinking about this twice, if we want to follow this idea, we should force the user to pick up specific preferences for the supported Windows versions. We can include them in the common-instancetypes repo or here, whatever we find more appropriated. This will allow the user to have different windows sizes by using the different instance types we currently provide.

sounds worth a try!

Added an approximation of what it could look like, it is still required to add further preferences for all supported windows versions.

deploy/srep-vap/vcpu-overcommit/101-windows-10-vcpu-restrict.yaml

deploy/srep-vap/vcpu-overcommit/103-vcpu-overcommit-vapb.yaml

deploy/srep-vap/vcpu-overcommit/102-vcpu-overcommit-vap.yaml

deploy/srep-vap/vcpu-overcommit/103-vcpu-overcommit-vapb.yaml

deploy/acm-policies/50-GENERATED-srep-vap.Policy.yaml

jcanocan · 2025-10-03T08:04:36Z

@ksimon1 many thanks for your reviews! Much appreciated :)

deploy/srep-vap/vcpu-overcommit/102-vcpu-overcommit-vap.yaml

abyrne55 · 2025-11-04T22:05:55Z

/ok-to-test

deploy/srep-vap/config.yaml

deploy/srep-vap/vcpu-overcommit/103-vcpu-overcommit-vapb.yaml

joshbranham · 2025-11-04T22:24:44Z

deploy/srep-vap/vcpu-overcommit/config.yaml

+        - "4.14"
+        - "4.15"
+        - "4.16"
+        - "4.17"
+        - "4.18"


What about 4.19 and 4.20?

I guess this means not applying to those version, or say only apply to 4.19+. I will let @jcanocan to clarify.

Yes, that's the idea. Technically speaking, we can deploy the VAP in versions starting from 4.17 (included), that version is based on Kubernetes 1.30, which is the version VAP were GA'd: https://kubernetes.io/docs/reference/access-authn-authz/validating-admission-policy/ https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/release_notes/ocp-4-17-release-notes

Therefore, I will exclude just 4.14/15/16. Let me know if you find another limitation or reason for excluding other versions.

Thanks for raising this.

The BU has set the minimum version for WinLI to 4.19 (slack post), so there's no need to roll this out on 4.18 and below

All right, so let's stick with 4.19+ then.

joshbranham · 2025-11-04T22:25:04Z

deploy/srep-vap/vcpu-overcommit/config.yaml

+deploymentMode: Policy
+clusterSelectors:
+  matchExpressions:
+    - key: hypershift.open-cluster-management.io/hosted-cluster


Do we want this on every hosted cluster? Do the CRDs exist on every cluster even if kubevirt is unused?

Apologizes for my ignorance here, I'm sure we wanted in hosted-cluster, but I'm not sure if we want it in the rest too. Could @BraeTroutman please clarify?

Do you mean kubevirt CRDs? We need kubevirt to be installed in order to have virtualmachinesinstances resources available.

So yeah we have no selectors as far as I'm aware to match only on hosted clusters that have kubevirt enabled, which means that this just needs to be deployed to every hosted cluster. If kubevirt is unused though, this VAP shouldn't affect KAS performance because no KubeVirt means no use of KubeVirt resources

however, I don't know how the VAP creation will function if the relevant custom resource Kind doesn't exist on the cluster in question... I'll see if I can do a quick verification on another cluster to see how the creation behaves, if it fails upon validation of the VAP

Validating Admission Policies can still be created successfully even if the target CR isn't defined on the cluster.

I do wonder how ACM will behave expecting to create the VirtualMachineClusterPreference. If the Hosted Cluster does not have KubeVirt installed, that CR will fail to create

If the Hosted Cluster does not have KubeVirt installed, that CR will fail to create
I presume this means the policy fails to be compliant.

I presume this means the policy fails to be compliant.

@joshbranham do you know if this is acceptable (failing to be compliant)? Given there's no selector at the moment, any suggestion on the next steps?

I am not an expert on ACM policies, so not sure. I think this could be easily validated by the submitter by testing creating the policy for a cluster with the CRDs and one without, and observe the behavior.

Without KubeVirt installed, VirtualMachineClusterPreference will fail to be created.

deploy/acm-policies/50-GENERATED-srep-vap-vcpu-overcommit.Policy.yaml

feichashao · 2025-11-05T01:45:09Z

deploy/srep-vap/vcpu-overcommit/102-vcpu-overcommit-vap.yaml

+  - expression: (('kubevirt.io/preference-name' in object.metadata.annotations) && 
+        (object.metadata.annotations['kubevirt.io/preference-name'].lowerAscii().contains('windows'))) ||
+        (('kubevirt.io/cluster-preference-name' in object.metadata.annotations) && 
+        (object.metadata.annotations['kubevirt.io/preference-name'].lowerAscii().contains('windows'))) ||


Did you mean kubevirt.io/cluster-preference-name instead of kubevirt.io/preference-name? I am seeing it is checking if it has kubevirt.io/cluster-preference-name so I think you are intent to check the value in kubevirt.io/cluster-preference-name in this condition?

I am not familiar with Virt, just reviewing from the CEL point of review. Appreciate if you can clarify, thanks!

You are absolutely right! Nice catch! Adjusted.

abyrne55 · 2025-11-05T16:13:24Z

Note that this is re. epic https://issues.redhat.com/browse/CNV-56734

abyrne55 · 2025-11-05T16:29:44Z

/approve
/hold

Please seek a final / lgtm and / unhold from #hcm-sre-team-thor, as VAP falls into their domain

openshift-ci · 2025-11-05T16:30:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abyrne55, jcanocan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [abyrne55]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

abyrne55 · 2025-11-05T17:19:07Z

deploy/srep-vap/vcpu-overcommit/README.md

Could we get at least a short blurb here for context? Please include a link to the epic

abyrne55 · 2025-11-05T17:20:36Z

deploy/srep-vap/vcpu-overcommit/config.yaml

+        - "4.14"
+        - "4.15"
+        - "4.16"


See previous comment on 4.19 minimum version

abyrne55 · 2025-11-05T17:24:46Z

hack/00-osd-managed-cluster-config-integration.yaml.tmpl

+                    - expression: (('kubevirt.io/preference-name' in object.metadata.annotations)
+                        && (object.metadata.annotations['kubevirt.io/preference-name'].lowerAscii().contains('windows')))
+                        || (('kubevirt.io/cluster-preference-name' in object.metadata.annotations)
+                        && (object.metadata.annotations['kubevirt.io/cluster-preference-name'].lowerAscii().contains('windows')))
+                        || (('vm.kubevirt.io/os' in object.metadata.annotations) &&
+                        (object.metadata.annotations['vm.kubevirt.io/os'].lowerAscii().contains('windows')))


I might be misunderstancing KubeVirt preferences, but just in case an existing customer is using kubevirt for something that has "windows" in the name but isn't Windows OS, can we make sure some public-facing documentation warns the customer not to put "windows" in their preference names? I want to make sure our customer service agents have something to point to if a customer complains about their unfortunately-named non-windows VMs not scheduling anymore.

That's a good point! @jcanocan Could you follow up on this action, maybe have another card for it? This doesn't need to be a blocker of this PR though.

@abyrne55 which documentation might be suitable?

Co-authored-by: Cursor AI Assistant <[email protected]> Signed-off-by: Javier Cano Cano <[email protected]>

openshift-ci · 2025-11-06T10:16:29Z

@jcanocan: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 26, 2025

dominikholler reviewed Sep 26, 2025

View reviewed changes

jcanocan force-pushed the windows-vcpu-overcommit branch 2 times, most recently from 0ada4dc to 4954ab2 Compare September 26, 2025 11:40

dominikholler suggested changes Sep 26, 2025

View reviewed changes

jcanocan force-pushed the windows-vcpu-overcommit branch 2 times, most recently from 9ac0225 to 704e950 Compare September 26, 2025 11:53

BraeTroutman reviewed Sep 30, 2025

View reviewed changes

deploy/srep-vap/vcpu-overcommit/103-vcpu-overcommit-vapb.yaml Outdated Show resolved Hide resolved

ksimon1 reviewed Oct 3, 2025

View reviewed changes

deploy/srep-vap/vcpu-overcommit/102-vcpu-overcommit-vap.yaml Outdated Show resolved Hide resolved

ksimon1 reviewed Oct 3, 2025

View reviewed changes

deploy/srep-vap/vcpu-overcommit/103-vcpu-overcommit-vapb.yaml Outdated Show resolved Hide resolved

ksimon1 reviewed Oct 3, 2025

View reviewed changes

deploy/acm-policies/50-GENERATED-srep-vap.Policy.yaml Outdated Show resolved Hide resolved

jcanocan force-pushed the windows-vcpu-overcommit branch from 704e950 to 611a5f8 Compare October 3, 2025 08:03

jcanocan marked this pull request as ready for review October 3, 2025 08:04

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2025

openshift-ci bot requested review from ravitri and typeid October 3, 2025 08:06

ksimon1 reviewed Oct 7, 2025

View reviewed changes

deploy/srep-vap/vcpu-overcommit/102-vcpu-overcommit-vap.yaml Outdated Show resolved Hide resolved

jcanocan force-pushed the windows-vcpu-overcommit branch from 611a5f8 to 1241d17 Compare October 7, 2025 10:28

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 4, 2025

joshbranham reviewed Nov 4, 2025

View reviewed changes

xiaoyu74 reviewed Nov 5, 2025

View reviewed changes

deploy/acm-policies/50-GENERATED-srep-vap-vcpu-overcommit.Policy.yaml Show resolved Hide resolved

feichashao reviewed Nov 5, 2025

View reviewed changes

jcanocan force-pushed the windows-vcpu-overcommit branch from 1241d17 to 673d612 Compare November 5, 2025 10:20

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 5, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2025

abyrne55 reviewed Nov 5, 2025

View reviewed changes

Add Windows VMs vCPU overcommit prevention VAP

3579719

Co-authored-by: Cursor AI Assistant <[email protected]> Signed-off-by: Javier Cano Cano <[email protected]>

jcanocan force-pushed the windows-vcpu-overcommit branch from 673d612 to 3579719 Compare November 6, 2025 09:57

Add Windows VMs vCPU overcommit prevention VAP #2535

Are you sure you want to change the base?

Add Windows VMs vCPU overcommit prevention VAP #2535

Uh oh!

Conversation

jcanocan commented Sep 26, 2025

What type of PR is this?

What this PR does / why we need it?

Which Jira/Github issue(s) this PR fixes?

Special notes for your reviewer:

Pre-checks (if applicable):

Uh oh!

app-sre-bot commented Sep 26, 2025

Uh oh!

openshift-ci bot commented Sep 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcanocan commented Oct 3, 2025

Uh oh!

Uh oh!

abyrne55 commented Nov 4, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

feichashao Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abyrne55 commented Nov 5, 2025

Uh oh!

abyrne55 commented Nov 5, 2025

Uh oh!

openshift-ci bot commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

feichashao Nov 6, 2025 •

edited

Loading

abyrne55 Nov 5, 2025 •

edited

Loading