refactor: Replace deprecated NewRandomVirtualMachine* functions by using libvmi directly #11621

TheRealSibasishBehera · 2024-03-29T04:46:14Z

Before this PR: The codebase was using deprecated NewRandomVirtualMachineInstanceWithBlockDisk , NewRandomVirtualMachineInstanceWithFileDisk , and NewRandomVirtualMachineInstanceWithDisk functions for testing purposes.

After this PR: Replaced all occurrences of deprecated NewRandomVirtualMachine* functions with equivalent functions from the libvmi library.

Fixes #11565

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

The NewRandomVirtualMachine* functions were deprecated and needed to be replaced to ensure compatibility with future releases. This PR updates the codebase to use the recommended libvmi functions for improved maintainability.

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

kubevirt-bot · 2024-03-29T04:46:23Z

Hi @TheRealSibasishBehera. Thanks for your PR.

PRs from untrusted users cannot be marked as trusted with /ok-to-test in this repo meaning untrusted PR authors can never trigger tests themselves. Collaborators can still trigger tests on the PR using /test all.

I understand the commands that are listed here.

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 · 2024-03-29T04:46:24Z

[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 alonakaplan 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:

tests/OWNERS

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

dankenigsberg · 2024-03-30T11:36:16Z

tests/migration/migration.go

@@ -1017,7 +1017,36 @@ var _ = SIGMigrationDescribe("VM Live Migration", func() {
 				Expect(err.Error()).To(ContainSubstring("DisksNotLiveMigratable"))
 			})
 			It("[test_id:1479][storage-req] should migrate a vmi with a shared block disk", decorators.StorageReq, func() {
-				vmi, _ := tests.NewRandomVirtualMachineInstanceWithBlockDisk(cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskAlpine), testsuite.GetTestNamespace(nil), k8sv1.ReadWriteMany)
+				if !libstorage.HasCDI() {
+					Skip("Skip DataVolume tests when CDI is not present")


Thanks for your PR. We should work hard to eliminate the catch-all tests/utils file.

These Skips were here before your PR, but now they are more spread out. Skip should not be used in tests, because they may cause us to ignore a test silently and unintentionally. Besides, I bet (but not sure) that the [storage-req] label means that these tests are executed only when storage requirements (CDI, block storage) are available. Please avoid them.

makes sense , i will change that

dankenigsberg · 2024-03-30T11:39:12Z

tests/migration/migration.go

+
+				dataVolume, err = virtClient.CdiClient().CdiV1beta1().DataVolumes(testsuite.GetTestNamespace(nil)).Create(context.Background(), dataVolume, metav1.CreateOptions{})
+				Expect(err).ToNot(HaveOccurred())
+				libstorage.EventuallyDV(dataVolume, 240, Or(HaveSucceeded(), WaitForFirstConsumer()))


I don't think that waiting for the DV here is necessary or helpful.

dankenigsberg · 2024-03-30T11:47:41Z

tests/migration/migration.go

@@ -1077,7 +1106,37 @@ var _ = SIGMigrationDescribe("VM Live Migration", func() {

 			It("[test_id:1854]should migrate a VMI with shared and non-shared disks", func() {
 				// Start the VirtualMachineInstance with PVC and Ephemeral Disks
-				vmi, _ := tests.NewRandomVirtualMachineInstanceWithBlockDisk(cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskAlpine), testsuite.GetTestNamespace(nil), k8sv1.ReadWriteMany)
+				if !libstorage.HasCDI() {


tests/migration/migration.go is already too long. I would prefer it that you define a private function such as vmi, err := newAlpineWithDV() and reuse it in this file.

dankenigsberg · 2024-03-30T11:48:47Z

/sig code-quality

alicefr · 2024-04-03T14:34:32Z

tests/migration/migration.go

@@ -1017,7 +1017,36 @@ var _ = SIGMigrationDescribe("VM Live Migration", func() {
 				Expect(err.Error()).To(ContainSubstring("DisksNotLiveMigratable"))
 			})
 			It("[test_id:1479][storage-req] should migrate a vmi with a shared block disk", decorators.StorageReq, func() {
-				vmi, _ := tests.NewRandomVirtualMachineInstanceWithBlockDisk(cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskAlpine), testsuite.GetTestNamespace(nil), k8sv1.ReadWriteMany)
+				if !libstorage.HasCDI() {


@TheRealSibasishBehera you can refer to the document that Felix is writing as part of #11456 . It is still in progress but there is some guidelines to avoid Skip and use the decorators.

Hence you should assume that the feature and components are there and failing as consequence. Hence, this becomes:

Expect(libstorage.HasCDI()).To(BeTrue())

yes ,i will make the changes . Thanks

alicefr · 2024-04-03T14:35:00Z

tests/migration/migration.go

+					Skip("Skip DataVolume tests when CDI is not present")
+				}
+				sc, exists := libstorage.GetRWXBlockStorageClass()
+				if !exists {


Same here with the skip

alicefr · 2024-04-03T15:08:54Z

@TheRealSibasishBehera I think we need to create another function that wraps all those code duplications. You could create a new template in the factory.go.
I know that this might seem similar to the deprecated function. However, the goal is that you still need to add the options to make this extensible. Something like:

func NewAlpineWithDataVolume(sc string, accessMode  k8sv1.AccessMode, opts ...libvmi.Option) *kvirtv1.VirtualMachineInstance {}

You can also include there the CDI and storage class checks and the dv creation there. In this way you can call the function with different option

libvmifact.NewAlpineWithDataVolume(
   sc, k8sv1.ReadWriteMany,
   libvmi.WithNamespace(testsuite.GetTestNamespace(nil)),
  ....
)

…libvmi equivalents Signed-off-by: TheRealSibasishBehera <fangedhamster3114@gmail.com>

dankenigsberg · 2024-04-09T15:33:00Z

tests/libvmifact/factory.go

 	cd "kubevirt.io/kubevirt/tests/containerdisk"
+	"kubevirt.io/kubevirt/tests/framework/kubevirt"


this PR adds a lot of dependencies to this file. Must the new functions be introduced here? Did you consider keeping them closer to their callers?

I think that @akalenyu is working on a similar thing.

Yep. However, different approach #11670
I believe the DV should be fully passed in, and we could expose certain presets from libdv if needed

cc @alicefr

@akalenyu we can, it is just more repetition of the dv specifications, but I guess we want to avoid too many variation of the functions

@akalenyu we can, it is just more repetition of the dv specifications, but I guess we want to avoid too many variation of the functions

From my investigation a "one-fits-all" DV would also not work unfortunately,
but yes, I definitely agree we should settle on one of them

@TheRealSibasishBehera do you mind passing an entire DV as part of the function NewAlpineWithDataVolume and adjust the function calls?

@alicefr would you prefer I handle this in my other PR so this PR doesn't have to diverge much?

@akalenyu mh maybe it is better. However, this function should also be called outside the storage code, see the changes of this PR in migration.go

yeah I'll move it

@akalenyu mh maybe it is better. However, this function should also be called outside the storage code, see the changes of this PR in migration.go

yeah @alicefr i agree on that part , this function would be called outside storage so it better to keep it in libvmifact .
an alternative can be keeping it in something like libvmifact/storage.go which will have functions involving storage related operations

dankenigsberg · 2024-04-09T15:34:45Z

tests/libvmifact/factory.go

+
+func getVolumeModeForAccessMode(accessMode k8sv1.PersistentVolumeAccessMode) k8sv1.PersistentVolumeMode {
+	if accessMode == k8sv1.ReadWriteMany {
+		return k8sv1.PersistentVolumeBlock


I suspect that ReadWriteMany implies PersistentVolumeBlock only in specific tests or a specific test framework. I think it would be better if the DV requests exactly what it wants to get,

Yeah those two are not coupled

i will change that

dankenigsberg · 2024-04-09T15:35:18Z

tests/migration/migration.go

@@ -1017,7 +1017,11 @@ var _ = SIGMigrationDescribe("VM Live Migration", func() {
 				Expect(err.Error()).To(ContainSubstring("DisksNotLiveMigratable"))
 			})
 			It("[test_id:1479][storage-req] should migrate a vmi with a shared block disk", decorators.StorageReq, func() {
-				vmi, _ := tests.NewRandomVirtualMachineInstanceWithBlockDisk(cd.DataVolumeImportUrlForContainerDisk(cd.ContainerDiskAlpine), testsuite.GetTestNamespace(nil), k8sv1.ReadWriteMany)
+				Expect(libstorage.HasCDI()).To(BeTrue())


Can you add a trailing reason string, so that if this fails, we'd see a clear failure reason?

kubevirt-bot · 2024-04-10T16:24:31Z

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/test-infra repository.

avlitman · 2024-05-08T18:04:08Z

@TheRealSibasishBehera thank you so much for contributing, this is great!
Are you planing on continue with this pr? I'm asking since we talked about fixing this as well, so want to plan.

RoniKishner · 2024-05-09T09:26:58Z

Hey this will need to be rebase and changed a bit because of #11624

TheRealSibasishBehera · 2024-05-09T10:15:29Z

@TheRealSibasishBehera thank you so much for contributing, this is great! Are you planing on continue with this pr? I'm asking since we talked about fixing this as well, so want to plan.

Yes , I plan to complete this .
#11670 is handling something similar . I am planning match its pattern .
Its still under work . We can wait till the PR is complete there or go with whats done till now regarding how DV creation is handled there.
Can you suggest which one would be the better option here ?

cc @alicefr @akalenyu

TheRealSibasishBehera · 2024-05-09T10:17:33Z

Hey this will need to be rebase and changed a bit because of #11624

Thanks @RoniKishner I will change that

akalenyu · 2024-05-15T14:39:30Z

@TheRealSibasishBehera thank you so much for contributing, this is great! Are you planing on continue with this pr? I'm asking since we talked about fixing this as well, so want to plan.

Yes , I plan to complete this . #11670 is handling something similar . I am planning match its pattern . Its still under work . We can wait till the PR is complete there or go with whats done till now regarding how DV creation is handled there. Can you suggest which one would be the better option here ?

cc @alicefr @akalenyu

Right so the whole approach of having this type of factory is being challenged,
and thus the PR you mention is now on hold:
#11670 (comment)

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. sig/storage labels Mar 29, 2024

kubevirt-bot added the size/L label Mar 29, 2024

kubevirt-bot requested review from awels and kbidarkar March 29, 2024 04:46

dankenigsberg suggested changes Mar 30, 2024

View reviewed changes

kubevirt-bot assigned dankenigsberg Mar 30, 2024

kubevirt-bot added the sig/code-quality label Mar 30, 2024

fossedihelm mentioned this pull request Apr 1, 2024

Replace NewRandomVirtualMachineInstanceWithFileDisk inside vm_test.go #11624

Merged

alicefr reviewed Apr 3, 2024

View reviewed changes

refactor: Replace deprecated NewRandomVirtualMachine* functions with …

290772a

…libvmi equivalents Signed-off-by: TheRealSibasishBehera <fangedhamster3114@gmail.com>

TheRealSibasishBehera force-pushed the lib-vmi branch from a589256 to 290772a Compare April 8, 2024 09:20

TheRealSibasishBehera requested review from alicefr and dankenigsberg April 8, 2024 09:38

dankenigsberg suggested changes Apr 9, 2024

View reviewed changes

kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2024

akalenyu mentioned this pull request Apr 30, 2024

tests: extract common VM with DV template creation #11670

Draft

8 tasks

avlitman mentioned this pull request May 21, 2024

Move functions from utils #11423

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Replace deprecated NewRandomVirtualMachine* functions by using libvmi directly #11621

refactor: Replace deprecated NewRandomVirtualMachine* functions by using libvmi directly #11621

TheRealSibasishBehera commented Mar 29, 2024

kubevirt-bot commented Mar 29, 2024

kubevirt-bot commented Mar 29, 2024

dankenigsberg Mar 30, 2024

TheRealSibasishBehera Apr 2, 2024

dankenigsberg Mar 30, 2024

dankenigsberg Mar 30, 2024

dankenigsberg commented Mar 30, 2024

alicefr Apr 3, 2024

TheRealSibasishBehera Apr 4, 2024

alicefr Apr 3, 2024

alicefr commented Apr 3, 2024 •

edited

Loading

dankenigsberg Apr 9, 2024

akalenyu Apr 9, 2024 •

edited

Loading

akalenyu Apr 9, 2024

alicefr Apr 10, 2024

akalenyu Apr 10, 2024

alicefr Apr 10, 2024 •

edited

Loading

akalenyu Apr 10, 2024

alicefr Apr 10, 2024 •

edited

Loading

akalenyu Apr 10, 2024

TheRealSibasishBehera Apr 10, 2024

dankenigsberg Apr 9, 2024

akalenyu Apr 9, 2024

TheRealSibasishBehera Apr 10, 2024

dankenigsberg Apr 9, 2024

kubevirt-bot commented Apr 10, 2024

avlitman commented May 8, 2024 •

edited

Loading

RoniKishner commented May 9, 2024

TheRealSibasishBehera commented May 9, 2024

TheRealSibasishBehera commented May 9, 2024

akalenyu commented May 15, 2024

		cd "kubevirt.io/kubevirt/tests/containerdisk"
		"kubevirt.io/kubevirt/tests/framework/kubevirt"

refactor: Replace deprecated NewRandomVirtualMachine* functions by using libvmi directly #11621

Are you sure you want to change the base?

refactor: Replace deprecated NewRandomVirtualMachine* functions by using libvmi directly #11621

Conversation

TheRealSibasishBehera commented Mar 29, 2024

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

Special notes for your reviewer

Checklist

Release note

kubevirt-bot commented Mar 29, 2024

kubevirt-bot commented Mar 29, 2024

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

dankenigsberg commented Mar 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr commented Apr 3, 2024 • edited Loading

Choose a reason for hiding this comment

akalenyu Apr 9, 2024 • edited Loading

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

alicefr Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alicefr Apr 10, 2024 • edited Loading

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

kubevirt-bot commented Apr 10, 2024

avlitman commented May 8, 2024 • edited Loading

RoniKishner commented May 9, 2024

TheRealSibasishBehera commented May 9, 2024

TheRealSibasishBehera commented May 9, 2024

akalenyu commented May 15, 2024

alicefr commented Apr 3, 2024 •

edited

Loading

akalenyu Apr 9, 2024 •

edited

Loading

alicefr Apr 10, 2024 •

edited

Loading

alicefr Apr 10, 2024 •

edited

Loading

avlitman commented May 8, 2024 •

edited

Loading