tests/utils.go: Get rid of RunVMI #11033

0xFelix · 2024-01-18T10:07:33Z

What this PR does / why we need it:

This function was giving the false impression of allowing the caller to
wait for the VMI to be running by allowing to supply a timeout.

In reality this function only created a VMI and was using a nonsensical
Eventually loop around the Create call.

Get rid of RunVMI by replacing all its usages with direct calls to the
kubevirt client.

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:

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

0xFelix · 2024-01-18T10:08:22Z

/cc @fossedihelm
/cc @dankenigsberg

0xFelix · 2024-01-18T10:08:32Z

/sig code-quality

fossedihelm

Thanks @0xFelix! It looks good.
I just have a question about one test below.

fossedihelm · 2024-01-18T10:24:59Z

tests/container_disk_test.go

+					for _, vmi := range vmis {
+						verifyContainerDiskVMI(virtClient, vmi)
+					}


Honestly, I don't undestand why we should verify all the vmi at every iteration (O(n²)) instead of create 2 for cycle (O(n))

I don't understand either what is the point here, but I took that from the original test. IMO it should suffice to create all the VMs and then verify them in a second loop, as you said. If five VMIs were verified, less VMIs should be OK too.

fossedihelm · 2024-01-18T10:54:01Z

/lgtm

dankenigsberg

Thanks for catching and fixing this.

dankenigsberg · 2024-01-18T10:44:44Z

tests/utils.go

-
-	var obj *v1.VirtualMachineInstance
-	var err error
-	Eventually(func() error {


Yes, if Create() fails, tests should typically just fail.

dankenigsberg · 2024-01-18T10:46:05Z

tests/container_disk_test.go

@@ -296,3 +263,26 @@ func getContainerDiskContainerOfPod(pod *k8sv1.Pod, volumeName string) *k8sv1.Co
 	diskContainerName := fmt.Sprintf("volume%s", volumeName)
 	return tests.GetContainerOfPod(pod, diskContainerName)
 }
+
+func verifyContainerDiskVMI(virtClient kubecli.KubevirtClient, vmi *v1.VirtualMachineInstance) {


+1 for avoiding the redundant lambda

I went one step further and dropped it alltogether.

tests/container_disk_test.go

EdDev

Very nice, thanks.

I had one inline comment.

EdDev · 2024-01-18T12:37:54Z

tests/container_disk_test.go

-				for idx, vmi := range vmis {
-					verifyContainerDiskVMI(vmi, objs[idx])
+					By("Verifying the containerdisk is online")
+					containerdiskFound := false


Can we avoid this nasty variables and just extract all the code below to a helper functions that returns this? e.g. hasContainerDisk(...)

hasContainerDisk in a sense that it doesn't care how many?

Thank you for the change. It is exactly what I meant.

dankenigsberg · 2024-01-18T13:30:34Z

tests/container_disk_test.go

+					vmi := libvmi.NewCirros(libvmi.WithResourceMemory("1Mi"))
+					vmi, err := virtClient.VirtualMachineInstance(testsuite.GetTestNamespace(vmi)).Create(context.Background(), vmi)
+					Expect(err).ToNot(HaveOccurred())
+					libwait.WaitForSuccessfulVMIStart(vmi)


Do I understand correctly that the old code did not have a wait at all (and thus had a race condition)?

I am all for waiting for the VMIs to start, but I'd do it after all count vmis are created, to save time.

No, the old implementation was waiting for the vmi to start inside the verifyContainerDiskVMI function.
So for the first vmi 5 times, for the second vmi 4 times, for the third vmi 3 times, and so on... waste of time, even if milliseconds.
I agree anyway, to move this under the second for loop

The old code had a wait and since it was using a nested-loop, it was basically progressively waiting for every VMI to become available after it was created. In each iteration of the outer loop a new VMI was added to the list, and during the inner loop the existing VMIs should already have been waited on, while it still needs to wait for the VMI that was added last.

By waiting for all VMs to be created first, we know that once we reach the second loop all the VMIs are already running. We can take the wiat out of the first loop, but then I'd add another loop for it. So IMO since it doesn't matter for the verification I'd leave it as is.

We can take the wiat out of the first loop, but then I'd add another loop for it. So IMO since it doesn't matter for the verification I'd leave it as is.

Ok. This improvement can be done in a follow-up PR.

I agree anyway, to move this under the second for loop

I also think it makes sense.

We can take the wiat out of the first loop, but then I'd add another loop for it. So IMO since it doesn't matter for the verification I'd leave it as is

Why another for-loop is needed?
If the second step depends on all VMIs to be running, then you are right.
But if it is not, then before treating each VMI a check that it is in the right phase and wait on it also makes sense. After waiting for the first, it is likely that the others are already running (parallel execution).

My idea was to keep it in the first loop so the second loop would only test the scenario where 5 vmis are already running. But in the end both choices are valid. WDYT, which one do you prefer?

If you wait on creation, you serialize the processes which prolongs the test significantly.
If you first create all, then wait for each, it should be much faster.

I moved the wait to the second loop.

I moved the wait to the second loop.

Thanks! But since this changes what the test does, I would have preferred if it was in its own commit, explaining why it is more sensible to create all VMIs first and wait for them later. Or at least add a word about it in the second commit.

I'm not really worried about this not-so-important test; but it is a good practice to separate mere cleanup from improvements.

In this way, it should actually be more in line with how the test originally worked.

EdDev

Thanks!

I think https://github.com/kubevirt/kubevirt/pull/11033/files#r1457444148 can be resolved in this PR as an additional commit.
But it can also be optimized in a follow up.

EdDev

Thanks!

0xFelix · 2024-01-19T08:55:26Z

@EdDev Rebased on main, adapted changes in tests/monitoring/vm_monitoring.go.

0xFelix · 2024-01-19T14:23:08Z

/retest

enp0s3 · 2024-01-21T10:00:11Z

@0xFelix Thank you!

/approve

kubevirt-bot · 2024-01-21T10:00:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enp0s3

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~~ [enp0s3]

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

dankenigsberg · 2024-01-22T08:26:54Z

Makefile

@@ -206,7 +206,7 @@ format:
 fmt: format

 lint:
-	if [ $$(wc -l < tests/utils.go) -gt 2350 ]; then echo >&2 "do not make tests/utils longer"; exit 1; fi
+	if [ $$(wc -l < tests/utils.go) -gt 2168 ]; then echo >&2 "do not make tests/utils longer"; exit 1; fi


If not for this modification, the PR would have been merged. Better use a separate PR to bump this...

You are right, next time

Get rid of RunVMI in container_disk_test.go and refactor/rewrite a test to be cleaner and more concise. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

This function was giving the false impression of allowing the caller to wait for the VMI to be running by allowing to supply a timeout. In reality this function only created a VMI and was using a nonsensical Eventually loop around the Create call. Get rid of RunVMI by replacing all its usages with direct calls to the kubevirt client. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

kubevirt-commenter-bot · 2024-01-22T09:14:03Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-k8s-1.27-ipv6-sig-network

kubevirt-bot · 2024-01-22T10:16:40Z

@0xFelix: The following tests 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	`f1e004f`	link	false	`/test pull-kubevirt-check-tests-for-flakes`
pull-kubevirt-e2e-arm64	`f1e004f`	link	false	`/test pull-kubevirt-e2e-arm64`

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.

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/network size/L labels Jan 18, 2024

kubevirt-bot requested review from EdDev, kbidarkar and ormergi January 18, 2024 10:07

kubevirt-bot requested review from dankenigsberg and fossedihelm January 18, 2024 10:08

kubevirt-bot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 18, 2024

0xFelix force-pushed the runvmi-cleanup branch from 4588685 to e7391bc Compare January 18, 2024 10:10

fossedihelm reviewed Jan 18, 2024

View reviewed changes

0xFelix force-pushed the runvmi-cleanup branch from e7391bc to 6e8e992 Compare January 18, 2024 10:50

kubevirt-bot assigned fossedihelm Jan 18, 2024

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

dankenigsberg suggested changes Jan 18, 2024

View reviewed changes

kubevirt-bot assigned dankenigsberg Jan 18, 2024

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

EdDev reviewed Jan 18, 2024

View reviewed changes

0xFelix force-pushed the runvmi-cleanup branch 2 times, most recently from 7fb5264 to 5e24759 Compare January 18, 2024 13:16

dankenigsberg suggested changes Jan 18, 2024

View reviewed changes

dankenigsberg approved these changes Jan 18, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

EdDev approved these changes Jan 18, 2024

View reviewed changes

kubevirt-bot assigned EdDev Jan 18, 2024

0xFelix force-pushed the runvmi-cleanup branch from 5e24759 to 70ef884 Compare January 18, 2024 14:44

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

EdDev approved these changes Jan 18, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 18, 2024

0xFelix force-pushed the runvmi-cleanup branch from 70ef884 to 6d89cf4 Compare January 19, 2024 08:54

kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 19, 2024

0xFelix force-pushed the runvmi-cleanup branch from 6d89cf4 to cc28c2a Compare January 19, 2024 14:45

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

dankenigsberg approved these changes Jan 21, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 21, 2024

EdDev approved these changes Jan 21, 2024

View reviewed changes

kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2024

dankenigsberg reviewed Jan 22, 2024

View reviewed changes

0xFelix added 2 commits January 22, 2024 09:30

tests/container_disk_test.go: Get rid of RunVMI

973e70a

Get rid of RunVMI in container_disk_test.go and refactor/rewrite a test to be cleaner and more concise. Signed-off-by: Felix Matouschek <fmatouschek@redhat.com>

0xFelix force-pushed the runvmi-cleanup branch from cc28c2a to f1e004f Compare January 22, 2024 08:34

kubevirt-bot removed 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 Jan 22, 2024

fossedihelm approved these changes Jan 22, 2024

View reviewed changes

kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2024

kubevirt-bot merged commit 0a3141f into kubevirt:main Jan 22, 2024
35 of 37 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests/utils.go: Get rid of RunVMI #11033

tests/utils.go: Get rid of RunVMI #11033

0xFelix commented Jan 18, 2024

0xFelix commented Jan 18, 2024

0xFelix commented Jan 18, 2024

fossedihelm left a comment

fossedihelm Jan 18, 2024

0xFelix Jan 18, 2024

fossedihelm commented Jan 18, 2024

dankenigsberg left a comment

dankenigsberg Jan 18, 2024 •

edited

Loading

dankenigsberg Jan 18, 2024

0xFelix Jan 18, 2024

EdDev left a comment

EdDev Jan 18, 2024

0xFelix Jan 18, 2024

EdDev Jan 18, 2024

dankenigsberg Jan 18, 2024

fossedihelm Jan 18, 2024

0xFelix Jan 18, 2024 •

edited

Loading

dankenigsberg Jan 18, 2024

EdDev Jan 18, 2024

0xFelix Jan 18, 2024

EdDev Jan 18, 2024

0xFelix Jan 18, 2024

dankenigsberg Jan 19, 2024

0xFelix Jan 19, 2024

EdDev left a comment

EdDev left a comment

0xFelix commented Jan 19, 2024

0xFelix commented Jan 19, 2024

enp0s3 commented Jan 21, 2024

kubevirt-bot commented Jan 21, 2024

dankenigsberg Jan 22, 2024

0xFelix Jan 22, 2024

kubevirt-commenter-bot commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024 •

edited

Loading

tests/utils.go: Get rid of RunVMI #11033

tests/utils.go: Get rid of RunVMI #11033

Conversation

0xFelix commented Jan 18, 2024

0xFelix commented Jan 18, 2024

0xFelix commented Jan 18, 2024

fossedihelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fossedihelm commented Jan 18, 2024

dankenigsberg left a comment

Choose a reason for hiding this comment

dankenigsberg Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

0xFelix Jan 18, 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

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

EdDev left a comment

Choose a reason for hiding this comment

0xFelix commented Jan 19, 2024

0xFelix commented Jan 19, 2024

enp0s3 commented Jan 21, 2024

kubevirt-bot commented Jan 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Jan 22, 2024

kubevirt-bot commented Jan 22, 2024 • edited Loading

dankenigsberg Jan 18, 2024 •

edited

Loading

0xFelix Jan 18, 2024 •

edited

Loading

kubevirt-bot commented Jan 22, 2024 •

edited

Loading