-
Notifications
You must be signed in to change notification settings - Fork 152
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
Kubevirt: Enforce limits & requests by a configurable ratio #2206
Conversation
Skipping CI for Draft Pull Request. |
Skipping CI for Draft Pull Request. |
b6b5a88
to
69fd669
Compare
69fd669
to
28b970b
Compare
Pull Request Test Coverage Report for Build 3981586663
💛 - Coveralls |
28b970b
to
7df714f
Compare
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 three comments about context, but they are all the same one, actually.
} | ||
} | ||
|
||
func (m *VirtLauncherMutator) Handle(_ context.Context, req admission.Request) admission.Response { |
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 use a real parameter for the context. You'll need it below.
} | ||
originalPod := launcherPod.DeepCopy() | ||
|
||
hco, err := getHcoObject(m.cli, m.hcoNamespace) |
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 use the context here.
pkg/webhooks/mutator/utils.go
Outdated
func getHcoObject(cli client.Client, namespace string) (*v1beta1.HyperConverged, error) { | ||
ctx := context.TODO() |
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 define the context as the first function parameter and use it.
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.
FYI, as can be seen here, I was simply using the same logic that already exists and following the current pattern.
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.
If you don't mind to fix it, please do. You're refactoring this code anyway.
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.
👍
3ef06b8
to
24b848c
Compare
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 a few comments
pkg/webhooks/mutator/utils.go
Outdated
hcoutil "github.com/kubevirt/hyperconverged-cluster-operator/pkg/util" | ||
) | ||
|
||
func getHcoObject(cli client.Client, ctx context.Context, namespace string) (*v1beta1.HyperConverged, error) { |
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 golang convention is that the context is always the first parameter in the function. Could you please reorder?
pkg/webhooks/mutator/utils.go
Outdated
if err != nil { | ||
if apierrors.IsNotFound(err) { | ||
logger.Info("HCO CR doesn't not exist, allow hcoNamespace deletion") | ||
return nil, err | ||
} | ||
|
||
logger.Error(err, "failed getting HyperConverged CR") | ||
return nil, err | ||
} |
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.
Is that the right error handling and error message, now that it's a generic function?
|
||
hco, err := getHcoObject(m.cli, ctx, m.hcoNamespace) | ||
if err != nil { | ||
m.logErr(err, "cannot get hco object") |
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.
maybe
m.logErr(err, "cannot get hco object") | |
m.logErr(err, "cannot get the HyperConverged object") |
Expect(resources.Limits[v1.ResourceCPU].Equal(expectedResources.Limits[v1.ResourceCPU])).To(BeTrue()) | ||
Expect(resources.Requests[v1.ResourceCPU].Equal(expectedResources.Requests[v1.ResourceCPU])).To(BeTrue()) | ||
Expect(resources.Limits[v1.ResourceMemory].Equal(expectedResources.Limits[v1.ResourceMemory])).To(BeTrue()) | ||
Expect(resources.Requests[v1.ResourceMemory].Equal(expectedResources.Requests[v1.ResourceMemory])).To(BeTrue()) |
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.
Isn't the gomega's Equals
matcher work here?
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.
Unfortunately not :(
With Gomega's Equals I get errors that are similar to:
[FAILED] Expected
<resource.Quantity>: {
i: {value: 150000000000, scale: -3},
d: {Dec: nil},
s: "150M",
Format: "DecimalSI",
}
to equal
<resource.Quantity>: {
i: {value: 150, scale: 6},
d: {Dec: nil},
s: "150M",
Format: "DecimalSI",
}
Although the two quantities are effectively equal (to 150M
).
podNamespace = "fake-namespace" | ||
) | ||
|
||
var _ = Describe("virt-launcher webhook mutator", func() { |
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 add error cases; e.g. no HyperConverged, negative ratio, zero ratio, wrongly formatted inputs etc.
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!
Added tests for no HCO and invalid ratio. IMHO we don't need to test the library that parses the annotation as it's beyond this "unit's" scope.
pkg/webhooks/mutator/utils.go
Outdated
func getHcoObject(cli client.Client, namespace string) (*v1beta1.HyperConverged, error) { | ||
ctx := context.TODO() |
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.
If you don't mind to fix it, please do. You're refactoring this code anyway.
okd-hco-e2e-image-index-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-image-index-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 |
hco-e2e-upgrade-index-sno-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-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. |
@iholder101: 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. |
hco-e2e-upgrade-index-sno-aws lane succeeded. |
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-index-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. |
since it is a temporary solution for the resource limits problem in VirtualMachines, and it is going to get a permanent fix in kubevirt/kubevirt side, i'm ok with disregarding the coverage decrease. |
@orenc1: Overrode contexts on behalf of orenc1: coverage/coveralls 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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: orenc1 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:
Approvers can indicate their approval by writing |
/cherrypick release-1.6 |
@iholder101: #2206 failed to apply on top of branch "release-1.6":
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. |
@iholder101 wrt to the annotation, can we change it from
to
That's quite long, agreed, but otoh the current name does not really say WHAT cpu and memory values are adjusted, and we have at least two places (.resources and .cpu.count or .memory.guest). At the same time it is not clear on what entity is being operated, vm, vmi, pod, which pod? |
/cherrypick release-1.8 |
@iholder101: #2206 failed to apply on top of branch "release-1.8":
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. |
Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
…evirt#2341) Remove the support of the kubevirt.io/cpu-limit-to-request-ratio and the kubevirt.io/memory-limit-to-request-ratio annotations, as this workaround does now work as expected. Revert PR kubevirt#2206 as it's not needed anymore This is a manual cherry-pick of: kubevirt#2341 Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com> Signed-off-by: Simone Tiraboschi <stirabos@redhat.com> Signed-off-by: Nahshon Unna Tsameret <60659093+nunnatsa@users.noreply.github.com>
Remove the support of the kubevirt.io/cpu-limit-to-request-ratio and the kubevirt.io/memory-limit-to-request-ratio annotations, as this workaround does now work as expected. Revert PR #2206 as it's not needed anymore This is a manual cherry-pick of: #2341 Signed-off-by: Nahshon Unna Tsameret <60659093+nunnatsa@users.noreply.github.com> Co-authored-by: Nahshon Unna Tsameret <60659093+nunnatsa@users.noreply.github.com>
This PR brings a mechanism to align Kubevirt
virt-launcher
Pods with ResourceQuotas that are applied to the namespace.As an example, let's look at the following VMI:
When this VMI is created, the following
virt-launcher
pod is created (some details are omitted for simplicity):As can be seen, this
virt-launcher
has only CPU and memory requests - but not limits. This means that if this VMI is being created in a namespace that has a ResourceQuota defined in it - the virt-launcher Pod won't be able to start. This now can be solved by the mechanism that's presented in this PR.To enable this mechanism, first a ratio between memory/CPU limits to request needs to be defined as an annotation in HCO object:
In addition, a ResourceQuota needs to exist on the relevant namespace. As an example, it's possible to create the following object:
Please take into account that if a ResourceQuota only sets a limit on
limits.cpu
orlimits.cpu
, then CPU/memory limits will be set accordingly. If multiple ResourceQuota exist within the relevant namespace, it takes only one of then to limit CPU/memory limits in order to enforce these limits.When these annotations are enabled along with a ResourceQuota object, a mutating webhook that's targeted to virt-launcher pods will enforce limits on the pod. It would now look like the following:
Bear in mind that the new webhook would be active only if a request is set but a limit is not.
Reviewer Checklist
Release note: