-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
controller: Apply preferences to hot plugged interfaces #11902
base: main
Are you sure you want to change the base?
controller: Apply preferences to hot plugged interfaces #11902
Conversation
Signed-off-by: Lee Yarwood <lyarwood@redhat.com>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
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.
pkg/virt-controller/watch/vm.go
Outdated
@@ -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 { |
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.
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.
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.
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.
@@ -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() { |
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.
For a follow up, consider moving all preferences tests to its own test file. The file size is too big.
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.
#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.
@@ -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() { |
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.
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).
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.
- 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.
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 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:
DescribeTable("spec interfaces", |
Thanks @EdDev, always a pleasure getting your reviews regardless of your awareness of the code :)
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 |
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>
5445006
to
a71e7af
Compare
Latest patchset switches over to a more targeted FWIW I'll clean up |
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.
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.
@@ -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() { |
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 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:
DescribeTable("spec interfaces", |
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. |
/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.
Release note