Conformance tests for vm_test.go #12682

fossedihelm · 2024-08-27T14:04:37Z

What this PR does

A new label (conformance) is introduced and will be used to tag those tests that will be part of the conformance tests.
This PR aims to identify and label the tests in the vm_tests.go file.
Also, a cleanup and refactoring of the tests is performed:

convert e2e test into a unit test if possible;
remove tests that do not add anything to an already present unit test;
use patchset;
remove duplicates;
merge similar tests.

This is a starting point for several PRs, whose goal is to cover all the conformance tests.

For the number lovers:
~~Previous number of e2e tests: 84~~
~~Current number of e2e tests: 77~~
~~Conformance e2e tests: 27~~

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

Release note

NONE

fossedihelm · 2024-08-27T14:04:49Z

/cc @xpivarc

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

tests/vm_test.go

xpivarc · 2024-08-29T07:18:52Z

tests/vm_test.go

-				vmi.Spec.Domain.Resources.Requests = k8sv1.ResourceList{
+			Entry("[test_id:6867] when VM scheduling error occurs with unsatisfiable resource requirements",
+				libvmi.New(
+					libvmi.WithContainerDisk("disk0", "no-such-image"),


Do we actually need the CD?

I don't think so

xpivarc · 2024-08-29T07:22:06Z

tests/vm_test.go

@@ -246,7 +245,7 @@ var _ = Describe("[rfe_id:1177][crit:medium][vendor:cnv-qe@redhat.com][level:com
 			validateGenerationState(vm, 6, 6, 6, 6)
 		})

-		DescribeTable("[test_id:1520]should update VirtualMachine once VMIs are up", func(createTemplate vmiBuilder) {
+		DescribeTable("[test_id:1520]should update VirtualMachine once VMIs are up", decorators.Conformance, func(createTemplate vmiBuilder) {


Where is the assert here?

The assertion in the libvmops.StartVirtualMachine function itself

But what is the update? I am not sure I understand what this test does.

The update is that the vm.Status.Ready becomes true. Maybe we can drop this?

Seems like duplicate then...

Yeah! Already dropped in a new commit

tests/vm_test.go

xpivarc · 2024-08-29T07:40:12Z

tests/vm_test.go

@@ -359,6 +358,7 @@ var _ = Describe("[rfe_id:1177][crit:medium][vendor:cnv-qe@redhat.com][level:com
 			Expect(pod.Name).ToNot(Equal(firstPod.Name))
 		})

+		//TODO does it make sense?


What are your thoughts?

I was wondering if this makes sense since we have test_id:1526 that checks stop and start multiple times. WDYT?

Let's drop the comment for now and track it offline

xpivarc · 2024-08-29T14:32:42Z

tests/vm_test.go

@@ -575,7 +573,7 @@ var _ = Describe("[rfe_id:1177][crit:medium][vendor:cnv-qe@redhat.com][level:com
 			}
 		})

-		It("[test_id:7679]should report an error status when data volume error occurs", func() {
+		It("[test_id:7679]should report an error status when data volume error occurs", decorators.Conformance, func() {


We should not use Skip inside Conformance tests

Removed from the conformance set

fossedihelm · 2024-09-03T08:45:26Z

Thank you for the review @xpivarc.
PTAL

dominikholler · 2024-09-05T07:28:31Z

A new label (conformance) is introduced and will be used to tag those tests that will be part of the conformance tests.
This PR aims to identify and label the tests in the vm_tests.go file.

Exciting idea!

Also, a cleanup and refactoring of the tests is performed:

No doubts that the cleanup and refactoring is required, but I wonder in which way is this is related to the new label?

fossedihelm · 2024-09-05T07:34:29Z

No doubts that the cleanup and refactoring is required, but I wonder in which way is this is related to the new label?

Thank you! I have to analyze every test to decide if they are or are not part of the conformance.
While doing this, if I find something that can be improved I think it is correct to it.
If we can stabilize/remove/convert_to_unittest why shouldn't we do it?
I mean, from my POV this is part of the stabilization/lowering flakes process.

dominikholler · 2024-09-05T07:38:11Z

Thank you! I have to analyze every test to decide if they are or are not part of the conformance.
While doing this, if I find something that can be improved I think it is correct to it.
If we can stabilize/remove/convert_to_unittest why shouldn't we do it?

No doubts that we should do it and that it is great, I just wonder if it is clean to introduce the change in the PR which is about adding the new label.

I mean, from my POV this is part of the stabilization/lowering flakes process.

No doubts that the changes are highly welcome.

fossedihelm · 2024-09-05T07:43:45Z

No doubts that we should do it and that it is great, I just wonder if it is clean to introduce the change in the PR which is about adding the new label.

By nature, Conformance tests should be as stable as possible. So, I think it is correct to do them all together :)

xpivarc

/approve

kubevirt-bot · 2024-09-10T14:37:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xpivarc

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

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

fossedihelm · 2024-09-11T11:51:15Z

@EdDev can you please have a look at the libvmi WithNodeSelectorFor signature change? Thank you

vladikr

Looks great, just some nits

vladikr · 2024-09-11T13:15:54Z

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

-		vmi.Spec.Domain.Devices.Disks = append(vmi.Spec.Domain.Devices.Disks, v1.Disk{
-			Name: "testdisk",
+	Context("with an invalid VM", func() {
+		It("should reject the request with unrecognized field", func() {


@fossedihelm @xpivarc Did we previously ignored unrecognized fields?
Are we sure we need to reject these?
Are we sure that it's not going to affect upgrades?

@vladikr I did not change any behavior here. I just converted the e2e test to unit test :)
So yes, we were rejecting unrecognized fields even before.
It is something that we do for every resource and looks like we have always done it:
93eecf3#diff-37e7f54f2a03b510af54f5d608e432bb0447d09ea6066a9970e0be8beab66649R1214

AFAIK k8s does not allow unrecognized fields as well.

Also, maybe we can perform this instructing openapi, instead of manually doing this check.

Yes, the fields were rejected as previously the Kube would only ignore the fields. In case of typo the behavior could be different..

vladikr · 2024-09-11T13:17:54Z

pkg/virt-api/webhooks/validating-webhook/admitters/vms-admitter_test.go

-				Template: &v1.VirtualMachineInstanceTemplateSpec{
-					Spec: vmi.Spec,
+
+		It("reject syntax valid VM, but with invalid spec", func() {


What is invalid in this test?
Is it that a matching volume doesn't exist?

Exactly!
Don't know how but looks like I accidentally deleted this comment:

// Add a disk that doesn't map to a volume. // This should get rejected which tells us the webhook validator is working.

I will reintroduce it. Thanks

The invalid vm spec e2e test can easily be a unit test. We can lower the e2e test time by moving it. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Currently, `WithNodeSelectorFor` takes the pointer to a node to create the nodeSelector field of a vmi. In most of the cases we just have the node name. Indeed, we can see that a dummy node obj is passed to respect the function signature. The node name is enough to achieve what we want. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Signed-off-by: fossedihelm <ffossemo@redhat.com>

Annotations carrying was not covering all the cases. The annotation carrying mechanism should ensure that annotations listed in the vm.spec.template object were propagated, while those listed in the vm objects are discarded. In other words, the vm annotations is not propagated to the vmi. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Currently we have two e2e tests: the first one that checks that a revision is created when a vm starts and the second one that checks that the revision is updated when a vm is restarted. The second one includes all the steps of the first one, so there is no reason to have both. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Signed-off-by: fossedihelm <ffossemo@redhat.com>

- Unwrap Delete out of Eventually block - Use `GetPodByVirtualMachineInstance` utility function Signed-off-by: fossedihelm <ffossemo@redhat.com>

Check for events rather than logs, which is well-defined API for users compared to logs that is mostly available only to admins. Signed-off-by: fossedihelm <ffossemo@redhat.com>

The test just checks that a vm becomes ready once the vmi is started. This is performed almost in every other tests, so we can drop it. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Signed-off-by: fossedihelm <ffossemo@redhat.com>

fossedihelm · 2024-09-12T05:29:34Z

Change Addressed @vladikr comment.
Followed by Rebase

acardace · 2024-09-13T14:42:43Z

/lgtm

kubevirt-commenter-bot · 2024-09-13T14:42:51Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.30-sig-network
/test pull-kubevirt-e2e-k8s-1.30-sig-storage
/test pull-kubevirt-e2e-k8s-1.30-sig-compute
/test pull-kubevirt-e2e-k8s-1.30-sig-operator

kubevirt-bot · 2024-09-13T21:52:53Z

@fossedihelm: 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-e2e-arm64	`dde8a54`	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-sigs/prow repository. I understand the commands that are listed here.

kubevirt-commenter-bot · 2024-09-14T02:25:46Z

/retest-required
This bot automatically retries required jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

EdDev · 2024-09-16T03:56:22Z

@EdDev can you please have a look at the libvmi WithNodeSelectorFor signature change? Thank you

Sorry for the late reply, it looks good to me.

kubevirt-bot added release-note-none dco-signoff: yes size/L labels Aug 27, 2024

kubevirt-bot requested review from xpivarc, kbidarkar, ShellyKa13 and vladikr August 27, 2024 14:04

kubevirt-bot added sig/storage needs-rebase labels Aug 27, 2024

fossedihelm force-pushed the conformance-vm branch from 390fffc to 92f436f Compare August 28, 2024 05:29

kubevirt-bot removed the needs-rebase label Aug 28, 2024

xpivarc reviewed Aug 29, 2024

View reviewed changes

kubevirt-bot added the needs-rebase label Sep 3, 2024

fossedihelm force-pushed the conformance-vm branch from 92f436f to 6e041d4 Compare September 3, 2024 08:36

kubevirt-bot added size/XL and removed size/L needs-rebase labels Sep 3, 2024

fossedihelm force-pushed the conformance-vm branch 2 times, most recently from a4cbde7 to de5b441 Compare September 3, 2024 12:26

kubevirt-bot added the sig/network label Sep 3, 2024

fossedihelm force-pushed the conformance-vm branch from de5b441 to a7bad41 Compare September 5, 2024 08:17

kubevirt-bot added the needs-rebase label Sep 7, 2024

fossedihelm force-pushed the conformance-vm branch from a7bad41 to 68e1a05 Compare September 9, 2024 06:09

kubevirt-bot removed the needs-rebase label Sep 9, 2024

kubevirt-bot added size/L and removed needs-rebase labels Sep 10, 2024

xpivarc reviewed Sep 10, 2024

View reviewed changes

kubevirt-bot added the approved label Sep 10, 2024

vladikr reviewed Sep 12, 2024

View reviewed changes

fossedihelm force-pushed the conformance-vm branch from a412545 to 34a6947 Compare September 12, 2024 05:27

fossedihelm added 12 commits September 12, 2024 07:27

Convert vm rejection e2e test to unit test

0308be2

The invalid vm spec e2e test can easily be a unit test. We can lower the e2e test time by moving it. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Use pointer pkg in vm_test.go

a66b49c

Signed-off-by: fossedihelm <ffossemo@redhat.com>

Merge error reporting e2e tests

7aa51e8

Signed-off-by: fossedihelm <ffossemo@redhat.com>

Use PatchSet in vm_test.go

41a19ef

Signed-off-by: fossedihelm <ffossemo@redhat.com>

vm_test: Refactor test_id:1524

a7283b4

- Unwrap Delete out of Eventually block - Use `GetPodByVirtualMachineInstance` utility function Signed-off-by: fossedihelm <ffossemo@redhat.com>

vm_test: Refactor test_id:1521

5fcdff9

Check for events rather than logs, which is well-defined API for users compared to logs that is mostly available only to admins. Signed-off-by: fossedihelm <ffossemo@redhat.com>

vm_test: Drop test_id:1520

39aad44

The test just checks that a vm becomes ready once the vmi is started. This is performed almost in every other tests, so we can drop it. Signed-off-by: fossedihelm <ffossemo@redhat.com>

Label conformance tests in vm_test.go

b0aecf9

Signed-off-by: fossedihelm <ffossemo@redhat.com>

Redeclare labels as single label instead of slice

dde8a54

Signed-off-by: fossedihelm <ffossemo@redhat.com>

fossedihelm force-pushed the conformance-vm branch from 34a6947 to dde8a54 Compare September 12, 2024 05:27

kubevirt-bot assigned acardace Sep 13, 2024

kubevirt-bot added the lgtm label Sep 13, 2024

kubevirt-bot merged commit 6aba1bc into kubevirt:main Sep 14, 2024
39 of 40 checks passed

Conformance tests for vm_test.go #12682

Conformance tests for vm_test.go #12682

Uh oh!

Conversation

fossedihelm commented Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

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

Special notes for your reviewer

Release note

Uh oh!

fossedihelm commented Aug 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fossedihelm commented Sep 3, 2024

Uh oh!

dominikholler commented Sep 5, 2024

Uh oh!

fossedihelm commented Sep 5, 2024

Uh oh!

dominikholler commented Sep 5, 2024

Uh oh!

fossedihelm commented Sep 5, 2024

Uh oh!

xpivarc left a comment

Choose a reason for hiding this comment

Uh oh!

kubevirt-bot commented Sep 10, 2024

Uh oh!

fossedihelm commented Sep 11, 2024

Uh oh!

vladikr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fossedihelm commented Sep 12, 2024

Uh oh!

acardace commented Sep 13, 2024

Uh oh!

fossedihelm commented Aug 27, 2024 •

edited

Loading

kubevirt-bot commented Sep 13, 2024 •

edited

Loading