virt-api: Refactor, share and use `VirtualMachineInstanceSpec` defaulting code from VM validation webhook #9103

lyarwood · 2023-01-25T15:26:30Z

What this PR does / why we need it:

The code responsible for defaulting the VirtualMachineInstanceSpec of a VirtualMachineInstance is refactored and moved to a shared location for use by other webhooks such as the VirtualMachine validation webhook.

This allows the VirtualMachine validation webhook to temporarily apply the same defaults as the VirtualMachineInstance mutation webhook to a copy of the VirtualMachine being admitted before validating the VirtualMachineInstanceSpec.

This in turn resolves issues #9102 where a valid VirtualMachine was rejected because this defaulting had not occurred leading to an invalid VirtualMachineInstanceSpec.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #9102

Special notes for your reviewer:

Release note:

NONE

0xFelix · 2023-01-26T12:28:51Z

Thanks, looks good!

/lgtm
/retest

lyarwood · 2023-01-26T14:39:15Z

@davidvossel @vladikr - This came up during downstream testing of instancetypes where we can't define resource requests at the moment and so always hit this when using hugepages. I think we can workaround this in the validation code here but please let me know if I've missed something here!

davidvossel · 2023-01-26T22:36:04Z

pkg/virt-api/webhooks/validating-webhook/admitters/vmi-create-admitter.go

+			if vmMemory.IsZero() && spec.Domain.Memory.Guest != nil {
+				vmMemory = spec.Domain.Memory.Guest
+				vmMemoryField = field.Child("domain", "memory", "guest").String()
+			}


i wonder if similar sorts of issues like this will bite us again in the future.

Maybe we should be running our defaulter on the templated spec (the logic in vmi mutate function) in the vms-admitter.go before processing the vmi.spec there. would that potentially make sense?

@davidvossel Yeah very true, I would however like to land the current limited approach for v0.59.0 and backport it to at least fix the hugepages corner case while working on a more complete fix for v0.60.0, I doubt that fix will be backportable tbh. Would that be acceptable in the short term?

Slightly OT but I think this class of issue also highlights the need to drive more devs and testers to use VirtualMachines as their initial starting point in tests etc rather than VirtualMachineInstances. I appreciate some of this is a hangover from how things were previously but packages/tools like libvmi that encourage the use of VirtualMachineInstances and skip VirtualMachine validation are starting to hurt us IMHO. If you agree I can take this up on the ML or some other forum (maybe something at the upcoming summit?) to highlight the need for us to exercise VirtualMachine validation more.

I doubt that fix will be backportable tbh. Would that be acceptable in the short term?

why wouldn't the systematic fix be backportable?

I doubt that fix will be backportable tbh. Would that be acceptable in the short term?

why wouldn't the systematic fix be backportable?

Apologies I misunderstood your suggestion, temporarily applying the defaults in the VM validation webhook would be backportable. I thought you were suggesting we apply them in the VM mutation webhook that while possible would be a pretty big behavioural change and likely result in lots of test fallout.

I'll start working on this now but I'd still like to land this ahead of v0.59.0, I can revert it later as part of the eventual fix and backports if it doesn't make it in this week.

lyarwood · 2023-01-27T09:48:13Z

/area instancetype

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood · 2023-01-30T19:52:25Z

/retest

davidvossel

/approve

kubevirt-bot · 2023-01-30T20:12:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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~~ [davidvossel]

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

lyarwood · 2023-01-31T09:57:59Z

/retest

lyarwood · 2023-01-31T10:07:33Z

/hold

0xFelix · 2023-02-01T08:16:52Z

/retest
/lgtm

Looks good, thanks!

kubevirt-commenter-bot · 2023-02-01T11:08:50Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

0xFelix · 2023-02-01T12:10:15Z

/cherry-pick release-0.58
/cherry-pick release-0.53

kubevirt-bot · 2023-02-01T12:10:17Z

@0xFelix: once the present PR merges, I will cherry-pick it on top of release-0.58 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.58
/cherry-pick release-0.53

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.

0xFelix · 2023-02-01T12:10:33Z

/cherry-pick release-0.53

kubevirt-bot · 2023-02-01T12:10:35Z

@0xFelix: once the present PR merges, I will cherry-pick it on top of release-0.53 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.53

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.

0xFelix · 2023-02-01T12:32:10Z

Commented on the wrong PR... just ignore that.

kubevirt-commenter-bot · 2023-02-01T15:07:51Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

lyarwood · 2023-02-01T16:26:25Z

/retest-required

lyarwood · 2023-02-01T19:07:01Z

/retest

kubevirt-commenter-bot · 2023-02-01T23:08:50Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

lyarwood · 2023-02-02T07:27:27Z

/retest-required

kubevirt-commenter-bot · 2023-02-02T12:08:52Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

xpivarc · 2023-02-02T14:52:49Z

/hold
Helping Tide.
The storage lane is currently broken. #9135 should fix this.
Feel free to remove the hold once this is merged.

lyarwood · 2023-02-03T07:06:07Z

/unhold
/retest-required

kubevirt-bot · 2023-02-03T11:28:32Z

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

Test name	Commit	Details	Required	Rerun command
pull-kubevirt-fossa	`e7898af`	link	false	`/test pull-kubevirt-fossa`

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.

kubevirt-bot · 2023-02-03T15:07:17Z

@0xFelix: new pull request created: #9168

In response to this:

/cherry-pick release-0.58
/cherry-pick release-0.53

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.

kubevirt-bot · 2023-02-03T15:08:09Z

@0xFelix: #9103 failed to apply on top of branch "release-0.53":

Applying: vms-admitter: Add test for bug #9102
Applying: virt-api: Refactor and share VM and VMI defaulting code between webhooks
Using index info to reconstruct a base tree...
M	pkg/util/util.go
M	pkg/virt-api/webhooks/BUILD.bazel
A	pkg/virt-api/webhooks/amd64.go
M	pkg/virt-api/webhooks/arm64.go
M	pkg/virt-api/webhooks/hyperv.go
M	pkg/virt-api/webhooks/mutating-webhook/mutators/BUILD.bazel
M	pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
M	pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
M	pkg/virt-config/virt-config.go
M	pkg/virt-controller/watch/vm.go
M	pkg/virt-launcher/virtwrap/converter/converter_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-launcher/virtwrap/converter/converter_test.go
CONFLICT (content): Merge conflict in pkg/virt-launcher/virtwrap/converter/converter_test.go
Auto-merging pkg/virt-controller/watch/vm.go
CONFLICT (content): Merge conflict in pkg/virt-controller/watch/vm.go
Auto-merging pkg/virt-config/virt-config.go
CONFLICT (content): Merge conflict in pkg/virt-config/virt-config.go
Auto-merging pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
CONFLICT (content): Merge conflict in pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator_test.go
Auto-merging pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
CONFLICT (content): Merge conflict in pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go
Auto-merging pkg/virt-api/webhooks/mutating-webhook/mutators/BUILD.bazel
Auto-merging pkg/virt-api/webhooks/hyperv.go
Auto-merging pkg/virt-api/webhooks/arm64.go
CONFLICT (content): Merge conflict in pkg/virt-api/webhooks/arm64.go
CONFLICT (modify/delete): pkg/virt-api/webhooks/amd64.go deleted in HEAD and modified in virt-api: Refactor and share VM and VMI defaulting code between webhooks. Version virt-api: Refactor and share VM and VMI defaulting code between webhooks of pkg/virt-api/webhooks/amd64.go left in tree.
Auto-merging pkg/virt-api/webhooks/BUILD.bazel
CONFLICT (content): Merge conflict in pkg/virt-api/webhooks/BUILD.bazel
Auto-merging pkg/util/util.go
CONFLICT (content): Merge conflict in pkg/util/util.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 virt-api: Refactor and share VM and VMI defaulting code between webhooks
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-0.53

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.

This reverts commit 48a2ade. Thanks to e7898af [1][2] we now apply defaults to a copy of the VirtualMachineInstanceSpec before running the associated validations in the VirtualMachine validation webhook. This actually resolves the original issue with enabling dedicatedCPUPlacement from instance types [3] as it ensures any guest visible memory is expressed as resources requests before we attempt to validate. As such we can drop the blocking validations from the instance type webhooks and allow `dedicatedCPUPlacement` to be set once again. [1] kubevirt#9103 [2] kubevirt#9102 [3] kubevirt#8867 Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood · 2023-02-05T21:30:08Z

/cherry-pick release-0.59

kubevirt-bot · 2023-02-05T21:30:48Z

@lyarwood: new pull request created: #9172

In response to this:

/cherry-pick release-0.59

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.

This reverts commit 48a2ade. Thanks to e7898af [1][2] we now apply defaults to a copy of the VirtualMachineInstanceSpec before running the associated validations in the VirtualMachine validation webhook. This actually resolves the original issue with enabling dedicatedCPUPlacement from instance types [3] as it ensures any guest visible memory is expressed as resources requests before we attempt to validate. As such we can drop the blocking validations from the instance type webhooks and allow `dedicatedCPUPlacement` to be set once again. [1] kubevirt#9103 [2] kubevirt#9102 [3] kubevirt#8867 Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jan 25, 2023

kubevirt-bot requested review from dhiller and marceloamaral January 25, 2023 15:26

kubevirt-bot added the size/L label Jan 25, 2023

lyarwood force-pushed the bug-9102 branch from e711873 to 0163ccf Compare January 25, 2023 15:45

kubevirt-bot assigned 0xFelix Jan 26, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 26, 2023

lyarwood mentioned this pull request Jan 26, 2023

Add kubevirtci deployment support and functional tests kubevirt/common-instancetypes#27

Merged

davidvossel reviewed Jan 26, 2023

View reviewed changes

kubevirt-bot added the area/instancetype label Jan 27, 2023

vms-admitter: Add test for bug kubevirt#9102

8a1620d

Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood force-pushed the bug-9102 branch from 0163ccf to e575c5f Compare January 30, 2023 19:36

kubevirt-bot added size/XL and removed lgtm Indicates that a PR is ready to be merged. size/L labels Jan 30, 2023

lyarwood changed the title ~~virt-api: Fallback to guest memory when checking hugepages~~ virt-api: Refactor, share and use VirtualMachineInstanceSpec defaulting code from VM validation webhook Jan 30, 2023

davidvossel reviewed Jan 30, 2023

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2023

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 31, 2023

lyarwood force-pushed the bug-9102 branch from e575c5f to 42b375c Compare January 31, 2023 10:11

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2023

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2023

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2023

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2023

kubevirt-bot merged commit d8c3456 into kubevirt:main Feb 3, 2023

kubevirt-bot mentioned this pull request Feb 3, 2023

[release-0.58] virt-api: Refactor, share and use VirtualMachineInstanceSpec defaulting code from VM validation webhook #9168

Closed

lyarwood mentioned this pull request Feb 3, 2023

Revert "Add checks of dedicatedCPUPlacement to validation webhook" #9169

Merged

kubevirt-bot mentioned this pull request Feb 5, 2023

[release-0.59] virt-api: Refactor, share and use VirtualMachineInstanceSpec defaulting code from VM validation webhook #9172

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

virt-api: Refactor, share and use `VirtualMachineInstanceSpec` defaulting code from VM validation webhook #9103

virt-api: Refactor, share and use `VirtualMachineInstanceSpec` defaulting code from VM validation webhook #9103

lyarwood commented Jan 25, 2023 •

edited

0xFelix commented Jan 26, 2023

lyarwood commented Jan 26, 2023

davidvossel Jan 26, 2023

lyarwood Jan 27, 2023

davidvossel Jan 30, 2023

lyarwood Jan 30, 2023

lyarwood commented Jan 27, 2023

lyarwood commented Jan 30, 2023

davidvossel left a comment

kubevirt-bot commented Jan 30, 2023

lyarwood commented Jan 31, 2023

lyarwood commented Jan 31, 2023

0xFelix commented Feb 1, 2023

kubevirt-commenter-bot commented Feb 1, 2023

0xFelix commented Feb 1, 2023

kubevirt-bot commented Feb 1, 2023

0xFelix commented Feb 1, 2023

kubevirt-bot commented Feb 1, 2023

0xFelix commented Feb 1, 2023

kubevirt-commenter-bot commented Feb 1, 2023

lyarwood commented Feb 1, 2023

lyarwood commented Feb 1, 2023

kubevirt-commenter-bot commented Feb 1, 2023

lyarwood commented Feb 2, 2023

kubevirt-commenter-bot commented Feb 2, 2023

xpivarc commented Feb 2, 2023

lyarwood commented Feb 3, 2023

kubevirt-bot commented Feb 3, 2023 •

edited

kubevirt-bot commented Feb 3, 2023

kubevirt-bot commented Feb 3, 2023

lyarwood commented Feb 5, 2023

kubevirt-bot commented Feb 5, 2023

virt-api: Refactor, share and use VirtualMachineInstanceSpec defaulting code from VM validation webhook #9103

virt-api: Refactor, share and use VirtualMachineInstanceSpec defaulting code from VM validation webhook #9103

Conversation

lyarwood commented Jan 25, 2023 • edited

0xFelix commented Jan 26, 2023

lyarwood commented Jan 26, 2023

davidvossel Jan 26, 2023

Choose a reason for hiding this comment

lyarwood Jan 27, 2023

Choose a reason for hiding this comment

davidvossel Jan 30, 2023

Choose a reason for hiding this comment

lyarwood Jan 30, 2023

Choose a reason for hiding this comment

lyarwood commented Jan 27, 2023

lyarwood commented Jan 30, 2023

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jan 30, 2023

lyarwood commented Jan 31, 2023

lyarwood commented Jan 31, 2023

0xFelix commented Feb 1, 2023

kubevirt-commenter-bot commented Feb 1, 2023

0xFelix commented Feb 1, 2023

kubevirt-bot commented Feb 1, 2023

0xFelix commented Feb 1, 2023

kubevirt-bot commented Feb 1, 2023

0xFelix commented Feb 1, 2023

kubevirt-commenter-bot commented Feb 1, 2023

lyarwood commented Feb 1, 2023

lyarwood commented Feb 1, 2023

kubevirt-commenter-bot commented Feb 1, 2023

lyarwood commented Feb 2, 2023

kubevirt-commenter-bot commented Feb 2, 2023

xpivarc commented Feb 2, 2023

lyarwood commented Feb 3, 2023

kubevirt-bot commented Feb 3, 2023 • edited

kubevirt-bot commented Feb 3, 2023

kubevirt-bot commented Feb 3, 2023

lyarwood commented Feb 5, 2023

kubevirt-bot commented Feb 5, 2023

virt-api: Refactor, share and use `VirtualMachineInstanceSpec` defaulting code from VM validation webhook #9103

virt-api: Refactor, share and use `VirtualMachineInstanceSpec` defaulting code from VM validation webhook #9103

lyarwood commented Jan 25, 2023 •

edited

kubevirt-bot commented Feb 3, 2023 •

edited