Remove NewRandomVMIWithEphemeralDisk from tests/utils.go #11923

nunnatsa · 2024-05-15T12:20:34Z

What this PR does

Before this PR

The NewRandomVMIWithEphemeralDisk is deprecated. After merging several PRs to stop using it, it's now only called from 3 other deprecated tests/utils.go's deprecated functions:

NewRandomVMIWithEphemeralDiskAndUserdata
NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata
NewRandomVMIWithEphemeralDiskAndConfigDriveUserdataNetworkData

Now it's the time to finally remove these 4 functions by first replacing all the calls for the utils functions with libvmi functions.

After this PR

4 deprecated util functions removed, and replaced with libvmi functionms.

For the implementation of this change, new factory function added to the libvmifact package:

NewCirrosNoUserData - this function creates a Cirros vmi, with no user-data, in order to user-defined user-data. The reason for that is that the existing NewCirros already adds CloudInitNoCloud UserData volume.
This makes it hard to add other user-data volume, like CloudInitConfigDrive.

In addition, several user-data related libvmi options were added:

WithCloudInitNoCloudUserDataSecretName
WithCloudInitConfigDriveEncodedUserData
WithCloudInitConfigDriveUserDataSecretName
WithCloudInitConfigDriveNetworkData
WithCloudInitConfigDriveEncodedNetworkData
WithCloudInitConfigDriveNetworkDataSecretName

Now we can use the NewCirrosNoUserData factory, together with one of the new (or existing) user-data options; e.g.

vmi := libvmifact.NewCirrosNoUserData(libvmi.WithCloudInitNoCloudUserDataSecretName(secretName))

Release note

None

kubevirt-bot · 2024-05-15T12:20:41Z

[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 dhiller, eddev 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:

pkg/libvmi/OWNERS
tests/OWNERS

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

The existing `NewCirros` already adds CloudInitNoCloud UserData volume. This makes it hard to add other user-data volume, like CloudInitConfigDrive. This commit add the new `NewCirrosNoUserData` to create cirros vmi with no user data at all, so the lib user would be able to safly add thier own volume. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Added the following libvmi options, to allow adding different types of user data: * `WithCloudInitNoCloudUserDataSecretName` * `WithCloudInitConfigDriveEncodedUserData` * `WithCloudInitConfigDriveUserDataSecretName` * `WithCloudInitConfigDriveNetworkData` * `WithCloudInitConfigDriveEncodedNetworkData` * `WithCloudInitConfigDriveNetworkDataSecretName` Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Stop using the deprecated `NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata` function. Replaced with libvmi. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

…rdata function Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Stop using the deprecated `NewRandomVMIWithEphemeralDiskAndConfigDriveUserdataNetworkData` function. Replaced with libvmi. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

…dataNetworkData function Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

kubevirt-bot · 2024-05-16T06:04:01Z

@nunnatsa: 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-check-tests-for-flakes	`f76d114`	link	false	`/test pull-kubevirt-check-tests-for-flakes`

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.

nunnatsa · 2024-05-16T12:20:59Z

/sig code-quality

ormergi

Thanks for the PR, its really nice seeing deprecated helper finally removed.

Just wanted to point out feedback I got on my take of getting rid of NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata #11304.
The new CirrosNoCloudInit factory produce awkward VMI spec where it could hang on reboot for long period of time.
You documented it well, but having the factory available only where it should be use would prevent misuse 🙂
Since the new CirrosNoCloudInit factory is used only at vmi_cloudinit_test.go it would be easy change, please consider it.

Regarding the PR structure, please consider changing it in a way where removal or addition of code would be in the same commit with the changes that replace it (where possible).
For me it would make it easier to review 🙂

ormergi · 2024-05-20T12:50:04Z

tests/libvmifact/factory.go

-	withNonEmptyUserData := libvmi.WithCloudInitNoCloudEncodedUserData("#!/bin/bash\necho hello\n")
+	cirrosOpts := []libvmi.Option{
+		libvmi.WithCloudInitNoCloudEncodedUserData("#!/bin/bash\necho hello\n"),
+	}


I would not change the current state because withNonEmptyUserData makes the code more explanatory

ormergi · 2024-05-20T13:27:35Z

tests/vmi_cloudinit_test.go

+					libvmi.WithInterface(v1.Interface{
+						Name: "default",
+						Tag:  "specialNet",
+						InterfaceBindingMethod: v1.InterfaceBindingMethod{
+							Masquerade: &v1.InterfaceMasquerade{},
+						},
+					}),


nit: I would keep it inline or emphesize the test subject field tag:

Suggested change

libvmi.WithInterface(v1.Interface{

Name: "default",

Tag: "specialNet",

InterfaceBindingMethod: v1.InterfaceBindingMethod{

Masquerade: &v1.InterfaceMasquerade{},

},

}),

libvmi.WithInterface(v1.Interface{Name: "default", InterfaceBindingMethod: v1.InterfaceBindingMethod{Masquerade: &v1.InterfaceMasquerade{},

Tag: "specialNet",

}}),

ormergi · 2024-05-20T13:35:51Z

tests/vmi_cloudinit_test.go

+				// Store cloudinit data as k8s secret
+				By("Creating a secret with userdata")
+				uSecret := libsecret.New(uSecretID, libsecret.DataString{userDataLabel: testUserData})

-					_, err := virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), uSecret, metav1.CreateOptions{})
-					Expect(err).ToNot(HaveOccurred())
+				By("Creating a secret with network data")
+				nSecret := libsecret.New(nSecretID, libsecret.DataString{networkDataLabel: testNetworkData})

-					_, err = virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), nSecret, metav1.CreateOptions{})
-					Expect(err).ToNot(HaveOccurred())
+				_, err := virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), uSecret, metav1.CreateOptions{})
+				Expect(err).ToNot(HaveOccurred())

-					break
-				}
+				_, err = virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), nSecret, metav1.CreateOptions{})
+				Expect(err).ToNot(HaveOccurred())


With this change the By statements indicate when the secret object is instantiated instead of actually be created by the client, in case of an issue it would be confusing to debug.

Could you please move the client create operations closer to the secret instantiation? (move line 572 right after 567)

ormergi · 2024-05-20T13:36:43Z

tests/vmi_cloudinit_test.go


-					By("Creating a secret with network data")
-					nSecret := libsecret.New(nSecretID, libsecret.DataString{networkDataLabel: testNetworkData})
+				// Store cloudinit data as k8s secret


This comment doesnt add much value IMO, please consider removing it.

ormergi · 2024-05-20T13:40:04Z

tests/dryrun_test.go

-
+			vmi := libvmifact.NewCirros()
+			vm = libvmi.NewVirtualMachine(vmi)
+			vm.Namespace = testsuite.GetTestNamespace(vmi)


In general I think its not effective to set namesapce for an object "manually", its better to spesify the namespace on creation instead, it makes the object state true to reality (as it have no namespace before actually created in the cluster), and reduce potential for unexpected behavior (since its specifying once).

The vmi is created with empty namespace, which make the result of testsuite.GetTestNamespace(vmi) similar to calling it with nil - testsuite.GetTestNamespace(nil).

I think we can simplify it by calling the VMI client create with testsuite.GetTestNamespace(nil) instead.
(I dont like the fact we call a function with 'nil', its all over the tests code though)

Right. But it failed on e2e, since in this test, we're using some other functions. I added it to make the test running.

ormergi · 2024-05-20T13:40:42Z

tests/vmi_cloudinit_test.go

 			}

+			// Store userdata as k8s secret


Same here, I think this comment add no value, please consider removing it.

ormergi · 2024-05-20T13:42:55Z

tests/vmi_cloudinit_test.go

@@ -525,7 +518,7 @@ var _ = Describe("[rfe_id:151][crit:high][vendor:cnv-qe@redhat.com][level:compon
 				By("Creating a secret with user and network data")
 				secret := libsecret.New(secretID, libsecret.DataString{"networkdata": testNetworkData})

-				_, err := virtClient.CoreV1().Secrets(vmi.Namespace).Create(context.Background(), secret, metav1.CreateOptions{})
+				_, err := virtClient.CoreV1().Secrets(testsuite.GetTestNamespace(vmi)).Create(context.Background(), secret, metav1.CreateOptions{})


Same here, calling the VMI client create with testsuite.GetTestNamespace(nil) gives the same result and it will be consistent with the tests code.

please notice that this is not vmi creation, but secret creation. the vmi.NS is not populated at this point.

nunnatsa · 2024-05-20T14:25:07Z

The new CirrosNoCloudInit factory produce awkward VMI spec where it could hang on reboot for long period of time. You documented it well, but having the factory available only where it should be use would prevent misuse 🙂 Since the new CirrosNoCloudInit factory is used only at vmi_cloudinit_test.go it would be easy change, please consider it.

Agree, but then I'll have to export or copy the logic around the memory resources. So I prefer to keep it like that.

EdDev · 2024-05-20T15:59:39Z

pkg/libvmi/cloudinit.go

The need to double the builder functions (from 6 to 12) is a smell.

The original builders created for cloudinit probably did not expect to have this matrix of combinations.
But as it is clearly needed now, it is time to re-examine the existing to fit the need of the new.

Given we see now all these combinations, how could we make it better? Assume we can change/refactor the existing.

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 May 15, 2024

kubevirt-bot requested review from enp0s3 and kbidarkar May 15, 2024 12:20

kubevirt-bot added the size/L label May 15, 2024

nunnatsa force-pushed the rm-NewRandomVMIWithEphemeralDisk branch 3 times, most recently from be20e22 to 80e75eb Compare May 15, 2024 19:02

nunnatsa added 12 commits May 16, 2024 08:07

Use libvmi instead of the deprecated utils func

f2ba153

Stop using the deprecated `NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata` function. Replaced with libvmi. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

removed the depracated NewRandomVMIWithEphemeralDiskAndConfigDriveUse…

0548dd9

…rdata function Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

Use libvmi instead of the deprecated utils func

30d788b

Stop using the deprecated `NewRandomVMIWithEphemeralDiskAndConfigDriveUserdataNetworkData` function. Replaced with libvmi. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

remove the deprecated NewRandomVMIWithEphemeralDiskAndConfigDriveUser…

ed482cc

…dataNetworkData function Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

stop using deprecated function in container_disk_test.go

a691097

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

stop using deprecated function in dryrun_test.go

9281979

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

stop using deprecated function in vmi_cloudinit_test.go

a359021

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

stop using deprecated function in vmi_configuration_test.go

dd83e05

NewRandomVMIWithEphemeralDiskAndUserdata is deprecated; use libvmi instead. Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

remove the deprecated NewRandomVMIWithEphemeralDiskAndUserdata function

2b538b3

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

remove the deprecated NewRandomVMIWithEphemeralDisk function

f76d114

Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>

nunnatsa force-pushed the rm-NewRandomVMIWithEphemeralDisk branch from 80e75eb to f76d114 Compare May 16, 2024 05:07

kubevirt-bot added the sig/code-quality label May 16, 2024

ormergi reviewed May 20, 2024

View reviewed changes

ormergi mentioned this pull request May 20, 2024

tests: Use libvmi instead of 'NewRandomVMIWithEphemeralDiskAndConfigDriveUserdata #11304

Open

8 tasks

EdDev reviewed May 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove NewRandomVMIWithEphemeralDisk from tests/utils.go #11923

Remove NewRandomVMIWithEphemeralDisk from tests/utils.go #11923

nunnatsa commented May 15, 2024

kubevirt-bot commented May 15, 2024

kubevirt-bot commented May 16, 2024 •

edited

nunnatsa commented May 16, 2024

ormergi left a comment •

edited

ormergi May 20, 2024

ormergi May 20, 2024

ormergi May 20, 2024

ormergi May 20, 2024

ormergi May 20, 2024

nunnatsa May 20, 2024

ormergi May 20, 2024

ormergi May 20, 2024

nunnatsa May 20, 2024

nunnatsa commented May 20, 2024

EdDev May 20, 2024

Remove NewRandomVMIWithEphemeralDisk from tests/utils.go #11923

Are you sure you want to change the base?

Remove NewRandomVMIWithEphemeralDisk from tests/utils.go #11923

Conversation

nunnatsa commented May 15, 2024

What this PR does

Before this PR

After this PR

Release note

kubevirt-bot commented May 15, 2024

kubevirt-bot commented May 16, 2024 • edited

nunnatsa commented May 16, 2024

ormergi left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunnatsa commented May 20, 2024

Choose a reason for hiding this comment

kubevirt-bot commented May 16, 2024 •

edited

ormergi left a comment •

edited