controller: Apply preferences to hot plugged interfaces #11902

lyarwood · 2024-05-13T12:48:44Z

/area instancetype
/cc @EdDev
/cc @orelmisan

What this PR does

Before this PR:

Previously any preferences referenced by the VirtualMachine would not be applied to hot plugged interfaces.

After this PR:

This change adds a crude check for new interfaces after ApplyDynamicIfaceRequestOnVMI has been called before then calling applyDevicePreferences to lookup and apply any associated preferences to a copy of the VMI that is later used to patch the VMI object itself.

Fixes #

Why we need it and why it was done in this way

The following tradeoffs were made:

The following alternatives were considered:

Links to places where the discussion took place:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Design: A design document was considered and is present (link) or not required
PR: The PR description is expressive enough and will help future contributors
Code: Write code that humans can understand and Keep it simple
Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
Upgrade: Impact of this change on upgrade flows was considered and addressed if required
Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
Community: Announcement to kubevirt-dev was considered

Release note

NONE

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

kubevirt-bot · 2024-05-13T12:48:57Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign fabiand for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

EdDev

Thank you.
Please note that I'm new to preferences :)

A follow up question: How does this play with the storage hotplug and the other rollout changes (I think CPU and memory for the moment, while there be others added in the future). I'm asking because I see applyDevicePreferences called only at the VM start (i.e. VMI creation) and here.

EdDev · 2024-05-22T03:28:42Z

pkg/virt-controller/watch/vm.go

@@ -3121,6 +3121,12 @@ func (c *VMController) sync(vm *virtv1.VirtualMachine, vmi *virtv1.VirtualMachin
 				syncErr = &syncErrorImpl{fmt.Errorf("Error encountered when trying to check if VMI has interface with ordinal names (e.g.: eth1, eth2..): %v", err), HotPlugNetworkInterfaceErrorReason}
 			} else {
 				updatedVmiSpec := network.ApplyDynamicIfaceRequestOnVMI(vmCopy, vmiCopy, hasOrdinalIfaces)
+				// If a new interface is now present apply any device preferences we might have
+				if len(updatedVmiSpec.Domain.Devices.Interfaces) > len(vmiCopy.Spec.Domain.Devices.Interfaces) {
+					if _, err := c.applyDevicePreferences(vmCopy, vmiCopy); err != nil {


It looks small and harmless, but I have a feeling it is a significant change compared to what existed so far.
If I understand correctly, before this addition, the VMI was applied the preferences only on creation.
Now, it is applied the preferences also on partial updates.

E.g. if the VM was updated while running, no preferences kicked in because the VMI was not updated.
However, if at some stage a new interface has been hot-plugged, this preference will be triggered, covering all changes so far to the VM.

Preferences only provide preferred values for parts of the VirtualMachineInstanceSpec and do not overwrite anything already provided by a user within the VirtualMachine.

You are however correct that this starts to specifically apply the device related preferences during an interface hot plug on the VMI.

pkg/virt-controller/watch/vm_test.go

EdDev · 2024-05-22T03:35:40Z

pkg/virt-controller/watch/vm_test.go

@@ -5492,6 +5492,68 @@ var _ = Describe("VirtualMachine", func() {
 					Expect(err).To(Succeed())
 					Expect(vm.Spec.Preference.RevisionName).To(Equal(expectedPreferenceRevision.Name))
 				})
+
+				It("should apply preferences to hot plugged interface in VMI", func() {


For a follow up, consider moving all preferences tests to its own test file. The file size is too big.

#10175 - Yeah I've been trying to sign up folks to this good-first-issue for ages now to help with this but everyone eventually gives up.

FWIW refactoring a large part of pkg/instancetype (the inherited interface, methods approach, single files etc) is currently queued up in my backlog for the 1.4 cycle.

EdDev · 2024-05-22T03:43:38Z

pkg/virt-controller/watch/vm_test.go

@@ -5492,6 +5492,68 @@ var _ = Describe("VirtualMachine", func() {
 					Expect(err).To(Succeed())
 					Expect(vm.Spec.Preference.RevisionName).To(Equal(expectedPreferenceRevision.Name))
 				})
+
+				It("should apply preferences to hot plugged interface in VMI", func() {


In continuation to the concern raised in the production code feedback, it may be useful to cover these scenarios as well:

An interface is plugged but at the same time another is unplugged (and targeted for being removed completely from the spec). Note that an unplug flow has two steps: 1) mark the interface as absent and 2) when all conditions are met, remove the record completely.

Simulate that changes have been done to the VM that are not hotplug (i.e. passed to the VMI) which may effect the preferences (e.g. removing or adding a field value from the spec that may now trigger the preference).

An interface is plugged but at the same time another is unplugged (and targeted for being removed completely from the spec). Note that an unplug flow has two steps: 1) mark the interface as absent and 2) when all conditions are met, remove the record completely.

I can add a test case but my assumption was that the current conditional would still catch the new interface while the interface being removed is still present and marked as absent.

Simulate that changes have been done to the VM that are not hotplug (i.e. passed to the VMI) which may effect the preferences (e.g. removing or adding a field value from the spec that may now trigger the preference).

I think this generic preference behavioural test falls under #10175 but I can add something in a follow up after this.

I can add a test case but my assumption was that the current conditional would still catch the new interface while the interface being removed is still present and marked as absent.

That covers the scenario where the interface is present and marked for unplug.
But there is another scenario where the interface is in practice removed completely (it will no longer appear on the interface/network list).

In practice, the condition if len(updatedVmiSpec.Domain.Devices.Interfaces) > len(vmiCopy.Spec.Domain.Devices.Interfaces) is currently assuming that vmiCopy has these interfaces already removed. That is indeed the case, but this can also change after a code refactor and then nothing will validate it broke something.
These tests explain it well:

kubevirt/pkg/virt-controller/network/hotplug_test.go

Line 268 in 6ece900

DescribeTable("spec interfaces",

lyarwood · 2024-05-22T09:26:39Z

Thank you. Please note that I'm new to preferences :)

Thanks @EdDev, always a pleasure getting your reviews regardless of your awareness of the code :)

A follow up question: How does this play with the storage hotplug and the other rollout changes (I think CPU and memory for the moment, while there be others added in the future). I'm asking because I see applyDevicePreferences called only at the VM start (i.e. VMI creation) and here.

Storage is something I'm looking at in parallel and will likely need a similar fix.

CPU and memory hot plug via instance types is being enabled via #11455 but it's currently being held up by some recent changes and regressions with the LiveUpdates feature.

Previously any preferences referenced by the VirtualMachine would not be applied to hot plugged interfaces. This change adds a crude check for new interfaces after ApplyDynamicIfaceRequestOnVMI has been called before then calling applyDevicePreferences to lookup and apply any associated preferences to a copy of the VMI that is later used to patch the VMI object itself. Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

lyarwood · 2024-05-22T09:47:16Z

Latest patchset switches over to a more targeted applyInferfacePreferences call so we only reapply preferences related to an interface during hot plug and removes the use of context.TODO() in the tests. I did start writing another test to cover the absent interfaces use case but I think the existing test essentially covers this with an old interface already present.

FWIW I'll clean up applyDevicePreferences in a follow up to return early and avoid the massive conditional block.

EdDev

Thanks.

I think we are left with a small risk that with some code refactoring, the condition for executing the preference apply may break. The only safe way to protect it is to add a scenario in which an interface which was prev marked for unplug (i.e. set with absent) is also removed.
The scenario is: One interface is added, another is removed.

But I leave it for you to decide if the risk is worth addressing or not.

EdDev · 2024-05-22T12:25:02Z

pkg/virt-controller/watch/vm_test.go

@@ -5492,6 +5492,68 @@ var _ = Describe("VirtualMachine", func() {
 					Expect(err).To(Succeed())
 					Expect(vm.Spec.Preference.RevisionName).To(Equal(expectedPreferenceRevision.Name))
 				})
+
+				It("should apply preferences to hot plugged interface in VMI", func() {


I can add a test case but my assumption was that the current conditional would still catch the new interface while the interface being removed is still present and marked as absent.

That covers the scenario where the interface is present and marked for unplug.
But there is another scenario where the interface is in practice removed completely (it will no longer appear on the interface/network list).

In practice, the condition if len(updatedVmiSpec.Domain.Devices.Interfaces) > len(vmiCopy.Spec.Domain.Devices.Interfaces) is currently assuming that vmiCopy has these interfaces already removed. That is indeed the case, but this can also change after a code refactor and then nothing will validate it broke something.
These tests explain it well:

kubevirt/pkg/virt-controller/network/hotplug_test.go

Line 268 in 6ece900

DescribeTable("spec interfaces",

kubevirt-bot · 2024-05-24T06:15:35Z

PR needs rebase.

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.

controller: Use PatchReactor for VMIs during VM tests

8720a11

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

kubevirt-bot requested review from EdDev and orelmisan May 13, 2024 12:48

kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. area/instancetype dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/M labels May 13, 2024

EdDev reviewed May 22, 2024

View reviewed changes

lyarwood force-pushed the instancetypes-apply-preferences-to-hot-plug-interfaces branch from 5445006 to a71e7af Compare May 22, 2024 09:42

kubevirt-bot added size/L and removed size/M labels May 22, 2024

EdDev approved these changes May 22, 2024

View reviewed changes

kubevirt-bot assigned EdDev May 22, 2024

kubevirt-bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controller: Apply preferences to hot plugged interfaces #11902

controller: Apply preferences to hot plugged interfaces #11902

lyarwood commented May 13, 2024

kubevirt-bot commented May 13, 2024

EdDev left a comment

EdDev May 22, 2024

lyarwood May 22, 2024

EdDev May 22, 2024

lyarwood May 22, 2024

EdDev May 22, 2024

lyarwood May 22, 2024

EdDev May 22, 2024

lyarwood commented May 22, 2024

lyarwood commented May 22, 2024

EdDev left a comment

EdDev May 22, 2024

kubevirt-bot commented May 24, 2024

controller: Apply preferences to hot plugged interfaces #11902

Are you sure you want to change the base?

controller: Apply preferences to hot plugged interfaces #11902

Conversation

lyarwood commented May 13, 2024

What this PR does

Why we need it and why it was done in this way

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented May 13, 2024

EdDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyarwood commented May 22, 2024

lyarwood commented May 22, 2024

EdDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-bot commented May 24, 2024