flavors: Apply preferences to any added default network interfaces or missing volume disks #7919

lyarwood · 2022-06-14T15:41:41Z

What this PR does / why we need it:

With the introduction of flavors and preferences we now need to ensure some of the default logic fired by the VirtualMachineInstance mutation webhook is called before any flavor and/or preference application within the VirtualMachine controller. Doing so should ensure that any device preferences (such as preferred disk bus or interface model) are applied to any default network interfaces or missing volume disks that are added to the VirtualMachineInstance at runtime.

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 #

Special notes for your reviewer:

Release note:

Device preferences are now applied to any default network interfaces or missing volume disks added to a `VirtualMachineInstance` at runtime.

lyarwood · 2022-06-14T16:03:10Z

/cc @rmohr @davidvossel @akrejcir This is the part of #7806 that I wanted to save, ensuring we apply preferences to any devices previously added in by the VMI mutation webhook.

lyarwood · 2022-06-15T07:49:50Z

/retest

akrejcir · 2022-06-15T13:08:40Z

I've only looked at the first commit for now.

My original reasoning for using mocked flavor methods in virt-controller/watch/vm_test.go was to only test that the flavor methods are called and error handling is handled properly. Tests for various behaviors of FlavorMethods can be put into flovor/flavor_test.go, so the tests are not duplicated in different files.

What do you think?

lyarwood · 2022-06-15T13:28:08Z

I've only looked at the first commit for now.

My original reasoning for using mocked flavor methods in virt-controller/watch/vm_test.go was to only test that the flavor methods are called and error handling is handled properly. Tests for various behaviors of FlavorMethods can be put into flovor/flavor_test.go, so the tests are not duplicated in different files.

What do you think?

Yeah I understand but I wanted more complete coverage of these cases in the context of the VM controller using actual flavor module code. The overhead of adding in test informers etc seemed low enough to make it worthwhile IMHO and makes the tests later in this series to assert that preferences are being applied to new devices added by the controller much easier and complete. As you say, most of the specific flavor application testing will still happen outside of these tests in the module unit tests.

pkg/util/util.go

The original tests used a mock copy of FlavorMethods and didn't really exercise much outside of the basic error handling within the VM controller. This change reworks the tests to use the actual FlavorMethods, increases coverage and cleans up some nits from now defunct PRs. Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

This is required for the following change introducing support for the VM controller to add a default network and associated interface to a VMI during a VM start. ClusterConfig being required for that code to determine which type of interface needs to be added. Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

With the introduction of flavors and preferences we now need to ensure that the default network and associated interface are added to the VMI *before* applying any preferences. This should ensure any network interface specific preferences are also applied to the default network interface ahead of the VirtualMachineInstance being submitted. To allow this the original `SetDefaultNetworkInterface` code from the VMI mutation webhook is moved into the ClusterConfig. Allowing the code to now be shared between the VM controller and the original VMI mutation webhook caller. Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

As with the default network interface we also need to ensure any missing disks for volumes are also added before we apply preferences to the VirtualMachineInstance within the VirtualMachine controller. This time the code is moved to utils as it requires now access to the ClusterConfig. Signed-off-by: Lee Yarwood <lyarwood@redhat.com>

davidvossel

/approve

kubevirt-bot · 2022-06-16T18:15:51Z

[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 · 2022-06-17T08:15:10Z

/retest

maiqueb · 2022-06-17T08:45:49Z

pkg/util/util.go

+func SetDefaultVolumeDisk(obj *v1.VirtualMachineInstance) {
+
+	diskAndFilesystemNames := make(map[string]struct{})
+
+	for _, disk := range obj.Spec.Domain.Devices.Disks {
+		diskAndFilesystemNames[disk.Name] = struct{}{}
+	}
+
+	for _, fs := range obj.Spec.Domain.Devices.Filesystems {
+		diskAndFilesystemNames[fs.Name] = struct{}{}
+	}
+
+	for _, volume := range obj.Spec.Volumes {
+		if _, foundDisk := diskAndFilesystemNames[volume.Name]; !foundDisk {
+			obj.Spec.Domain.Devices.Disks = append(
+				obj.Spec.Domain.Devices.Disks,
+				v1.Disk{
+					Name: volume.Name,
+				},
+			)
+		}
+	}
+}


Would this one also fit the same file ?

I wouldn't think so as it's not using anything from ClusterConfig. If anything it should be in a storage specific package but I couldn't find anything obvious when I looked. Happy to look again and move in a follow up if it's important.

Yeah, you're right. I just looked for the word volume in that file and got no matches.

I was just trying to avoid adding code to anything whose name is utils ...

Is there any place related to storage that would fit @brybacki ? ...

If not, the code looks good, and you already have the lgtm tag.

Thanks for listening :)

there is no such place yet, at least I can not find it

akrejcir · 2022-06-17T09:28:20Z

/lgtm

kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Jun 14, 2022

kubevirt-bot requested review from AlonaKaplan and kbidarkar June 14, 2022 15:42

lyarwood mentioned this pull request Jun 14, 2022

Refactor, rename and extend the Instancetype API #7897

Closed

19 tasks

lyarwood changed the title ~~Flavor: Apply preferences to any added default network interfaces or missing volume disks~~ flavors: Apply preferences to any added default network interfaces or missing volume disks Jun 14, 2022

maiqueb reviewed Jun 15, 2022

View reviewed changes

pkg/util/util.go Outdated Show resolved Hide resolved

lyarwood added 4 commits June 16, 2022 09:33

lyarwood force-pushed the flavor-apply-preferences-to-default-network-and-disks branch from c69ba75 to e91cc08 Compare June 16, 2022 09:32

davidvossel reviewed Jun 16, 2022

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 16, 2022

maiqueb reviewed Jun 17, 2022

View reviewed changes

kubevirt-bot assigned akrejcir Jun 17, 2022

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2022

lyarwood mentioned this pull request Jun 17, 2022

flavors: Remove VirtualMachineFlavor and VirtualMachinePreference SharedInformers #7935

Merged

kubevirt-bot merged commit 8fb9a44 into kubevirt:main Jun 17, 2022

lyarwood deleted the flavor-apply-preferences-to-default-network-and-disks branch June 18, 2022 14:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

flavors: Apply preferences to any added default network interfaces or missing volume disks #7919

flavors: Apply preferences to any added default network interfaces or missing volume disks #7919

lyarwood commented Jun 14, 2022

lyarwood commented Jun 14, 2022

lyarwood commented Jun 15, 2022 •

edited

Loading

akrejcir commented Jun 15, 2022

lyarwood commented Jun 15, 2022 •

edited

Loading

davidvossel left a comment

kubevirt-bot commented Jun 16, 2022

lyarwood commented Jun 17, 2022

maiqueb Jun 17, 2022

lyarwood Jun 17, 2022

maiqueb Jun 17, 2022

brybacki Jun 20, 2022

akrejcir commented Jun 17, 2022

flavors: Apply preferences to any added default network interfaces or missing volume disks #7919

flavors: Apply preferences to any added default network interfaces or missing volume disks #7919

Conversation

lyarwood commented Jun 14, 2022

lyarwood commented Jun 14, 2022

lyarwood commented Jun 15, 2022 • edited Loading

akrejcir commented Jun 15, 2022

lyarwood commented Jun 15, 2022 • edited Loading

davidvossel left a comment

Choose a reason for hiding this comment

kubevirt-bot commented Jun 16, 2022

lyarwood commented Jun 17, 2022

maiqueb Jun 17, 2022

Choose a reason for hiding this comment

lyarwood Jun 17, 2022

Choose a reason for hiding this comment

maiqueb Jun 17, 2022

Choose a reason for hiding this comment

brybacki Jun 20, 2022

Choose a reason for hiding this comment

akrejcir commented Jun 17, 2022

lyarwood commented Jun 15, 2022 •

edited

Loading

lyarwood commented Jun 15, 2022 •

edited

Loading