-
Notifications
You must be signed in to change notification settings - Fork 149
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
Add the AlignCPUs enabler #2695
Conversation
PR kubevirt/kubevirt#10872 introduced a cluster-wide setting to enable KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. Introduce a new enabler to HCO's API in order to toggle this behavior in KubeVirt. The new API field is optional and defaults to false. The logic behind it will be introduced in the following commits. Signed-off-by: Orel Misan <omisan@redhat.com> Signed-off-by: Ram Lavi <ralavi@redhat.com>
Skipping CI for Draft Pull Request. |
Skipping CI for Draft Pull Request. |
/cc @nunnatsa |
Pull Request Test Coverage Report for Build 7346808755
💛 - Coveralls |
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.
Thanks for the PR @orelmisan!
Added some inline comments.
// Note: this feature is in Developer Preview. | ||
// +optional | ||
// +kubebuilder:default=false | ||
// +default=false |
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.
Don't we want it true by default?
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.
Since its an alpha "feature" in KubeVirt, IMO it should be explicitly enabled.
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.
Furthermore, users of this feature probably also set the defaultRuntimeClass
field, so they already need to edit the HCO manifest.
Both changes could be done in a single patch command.
controllers/operands/kubevirt.go
Outdated
@@ -303,6 +310,30 @@ func NewKubeVirt(hc *hcov1beta1.HyperConverged, opts ...string) (*kubevirtcorev1 | |||
return kv, nil | |||
} | |||
|
|||
func isAnnotationStateMeetingRequirements(hc *hcov1beta1.HyperConverged, annotations map[string]string) bool { |
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.
Not sure about the name of this function. I find the name both a bit too long, and too generic.
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.
Are there alternative names you have in mind?
controllers/operands/kubevirt.go
Outdated
_, found := annotations[kubevirtcorev1.EmulatorThreadCompleteToEvenParity] | ||
|
||
if (hc.Spec.FeatureGates.AlignCPUs == nil || !*hc.Spec.FeatureGates.AlignCPUs) && found { | ||
return false | ||
} else if (hc.Spec.FeatureGates.AlignCPUs != nil && *hc.Spec.FeatureGates.AlignCPUs) && !found { | ||
return false | ||
} | ||
|
||
return true |
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 think what we really want to check here is that (required KV's annotation exists) XOR (found KV's annotation exists)
. I mean - here I don't care if the annotation is reuired or not. This is already done in NewKubeVirt
(bysetAnnotationsToReqState
).
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.
Good idea!
Done.
controllers/operands/kubevirt.go
Outdated
if !reflect.DeepEqual(found.Spec, virt.Spec) || | ||
!reflect.DeepEqual(found.Labels, virt.Labels) { | ||
|
||
isUpdateRequired := !reflect.DeepEqual(found.Spec, virt.Spec) || |
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.
Since we only check the EmulatorThreadCompleteToEvenParity
annotation, and not all the annotations, I think we don't need a new variable here, and we can use a regular if
condition. It is not been used after the if.
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.
Done.
Change: Answered review comments. |
okd-hco-e2e-operator-sdk-gcp lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure, ci/prow/okd-hco-e2e-operator-sdk-aws In response to this:
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/test-infra repository. |
hco-e2e-operator-sdk-sno-azure lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-sno-aws, ci/prow/okd-hco-e2e-upgrade-operator-sdk-aws In response to this:
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/test-infra repository. |
hco-e2e-upgrade-prev-operator-sdk-sno-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-azure In response to this:
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/test-infra repository. |
hco-e2e-kv-smoke-gcp lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure In response to this:
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/test-infra repository. |
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.
Added inline comments
|
||
It("should not add the AlignCPUs feature gate if AlignCPUs is not set in HyperConverged CR", func() { | ||
hco.Spec.FeatureGates = hcov1beta1.HyperConvergedFeatureGates{ | ||
AlignCPUs: nil, //nolint SA1019 |
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.
nit: the nolint
comment here is not needed.
This linter alert is about deprecated fields, as in the NonRoot
field, but here the linter won't trigger any alert.
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.
Done.
It("should add the AlignCPUs feature gate if AlignCPUs is true in HyperConverged CR", func() { | ||
hco.Spec.FeatureGates = hcov1beta1.HyperConvergedFeatureGates{ | ||
AlignCPUs: ptr.To(true), | ||
} | ||
|
||
existingResource, err := NewKubeVirt(hco) | ||
Expect(err).ToNot(HaveOccurred()) | ||
By("KV CR should contain the AutoResourceLimitsGate feature gate", func() { | ||
Expect(existingResource.Spec.Configuration.DeveloperConfiguration).NotTo(BeNil()) | ||
Expect(existingResource.Spec.Configuration.DeveloperConfiguration.FeatureGates).To(ContainElement(kvAlignCPUs)) | ||
}) | ||
}) | ||
|
||
It("should not add the AlignCPUs feature gate if AlignCPUs is false in HyperConverged CR", func() { | ||
hco.Spec.FeatureGates = hcov1beta1.HyperConvergedFeatureGates{ | ||
AlignCPUs: ptr.To(false), | ||
} | ||
|
||
existingResource, err := NewKubeVirt(hco) | ||
Expect(err).ToNot(HaveOccurred()) | ||
By("KV CR should not contain the AlignCPUs feature gate", func() { | ||
Expect(existingResource.Spec.Configuration.DeveloperConfiguration).NotTo(BeNil()) | ||
Expect(existingResource.Spec.Configuration.DeveloperConfiguration.FeatureGates).ToNot(ContainElement(kvAlignCPUs)) | ||
}) | ||
}) | ||
|
||
It("should not add the AlignCPUs feature gate if AlignCPUs is not set in HyperConverged CR", func() { | ||
hco.Spec.FeatureGates = hcov1beta1.HyperConvergedFeatureGates{ | ||
AlignCPUs: nil, //nolint SA1019 |
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.
Please also check that the annotation does/doesn't exist in all 3 new tests.
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.
Done.
Use the `AlignCPUs` enabler in order to control KubeVirt's `AlignCPUs` feature gate. Signed-off-by: Orel Misan <omisan@redhat.com>
When the FG is enabled ensure the Kubevirt CR will have: 1. The `AlignCPUs` feature gate enabled. 2. The `alpha.kubevirt.io/EmulatorThreadCompleteToEvenParity` annotation set with a value of an empty string. Following PR kubevirt/kubevirt#10872, it will enable KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. When the FG is disabled, do the opposite: 1. Make sure the `AlignCPUs` feature gate is disabled. 2. Make sure the above annotation is not set. The logic introduced in this commit is allowing users to freely annotate the Kubevirt CR, but HCO will ensure the required state of the above annotation. Signed-off-by: Orel Misan <omisan@redhat.com>
hco-e2e-upgrade-prev-operator-sdk-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure In response to this:
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/test-infra repository. |
/retest |
1 similar comment
/retest |
@orelmisan: The following tests failed, say
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/test-infra repository. I understand the commands that are listed here. |
@nunnatsa The two remaining Azure tests are still failing after multiple retries. |
/override-bot |
hco-e2e-operator-sdk-sno-aws lane passed. |
@nunnatsa: Overrode contexts on behalf of nunnatsa: ci/prow/hco-e2e-operator-sdk-sno-azure In response to this:
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/test-infra repository. |
hco-e2e-upgrade-operator-sdk-sno-aws lane passed /override |
/override ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure |
@nunnatsa: Overrode contexts on behalf of nunnatsa: ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure In response to this:
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/test-infra repository. |
Thank you @nunnatsa |
Following the changes made on HCO [0], the SMT alignment fix is configurable via HCO, thus making the fix enablelment more user freindly. [0] kubevirt/hyperconverged-cluster-operator#2695 Signed-off-by: Ram Lavi <ralavi@redhat.com>
Following the changes made on HCO [0], the SMT alignment fix is configurable via HCO, thus making the fix enablelment more user freindly. [0] kubevirt/hyperconverged-cluster-operator#2695 Signed-off-by: Ram Lavi <ralavi@redhat.com>
Following the changes made on HCO [0], the SMT alignment fix is configurable via HCO, thus making the fix enablelment more user freindly. [0] kubevirt/hyperconverged-cluster-operator#2695 Signed-off-by: Ram Lavi <ralavi@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
In case the user's worker nodes have SMT [1] enabled, they will probably encounter CNV-31584 [2]. Add an instruction to enable the `AlignCPUs` enabler in HCO [3], so it will enable [4] KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation. [1] https://en.wikipedia.org/wiki/Simultaneous_multithreading [2] https://issues.redhat.com/browse/CNV-31584 [3] kubevirt/hyperconverged-cluster-operator#2695 [4] kubevirt/kubevirt#10872 Signed-off-by: Orel Misan <omisan@redhat.com>
What this PR does / why we need it:
PR kubevirt/kubevirt#10872 introduced a cluster-wide setting to enable KubeVirt to request up to two additional dedicated CPUs in order to complete the total CPU count to an even parity when using emulator thread isolation.
In order to enable this functionality, in the Kubevirt CR:
AlignCPUs
FG should be enabled.alpha.kubevirt.io/EmulatorThreadCompleteToEvenParity
annotation should be set with a value of an empty string.Introduce a new enabler to HCO's API in order to toggle this behavior in KubeVirt.
The new API field is optional and defaults to false.
This PR replaces #2685.
Reviewer Checklist
Jira Ticket:
Release note: