Vm controller unit tests: fix return args #11322

xpivarc · 2024-02-21T11:17:49Z

What this PR does

This PR fixes unit tests that were returning the wrong VM in Update operation. This reveals that some assumptions were wrong around the volume requests and so they are fixed as well.

Fixes #

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

Special notes for your reviewer

Release note

NONE

kubevirt-bot · 2024-02-21T11:17:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

xpivarc · 2024-02-21T11:18:14Z

/retest pull-kubevirt-unit-test

kubevirt-bot · 2024-02-21T11:18:18Z

@xpivarc: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test pull-kubevirt-apidocs
/test pull-kubevirt-build
/test pull-kubevirt-build-arm64
/test pull-kubevirt-check-unassigned-tests
/test pull-kubevirt-client-python
/test pull-kubevirt-code-lint
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.27-sig-performance
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-compute
/test pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations
/test pull-kubevirt-e2e-k8s-1.29-sig-network
/test pull-kubevirt-e2e-k8s-1.29-sig-operator
/test pull-kubevirt-e2e-k8s-1.29-sig-storage
/test pull-kubevirt-e2e-kind-1.27-vgpu
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-fossa
/test pull-kubevirt-generate
/test pull-kubevirt-manifests
/test pull-kubevirt-prom-rules-verify
/test pull-kubevirt-unit-test
/test pull-kubevirt-verify-go-mod

The following commands are available to trigger optional jobs:

/test build-kubevirt-builder
/test pull-kubevirt-build-s390x
/test pull-kubevirt-check-tests-for-flakes
/test pull-kubevirt-conformance-arm64
/test pull-kubevirt-e2e-arm64
/test pull-kubevirt-e2e-k8s-1.29-sig-compute-realtime
/test pull-kubevirt-e2e-k8s-1.29-sig-compute-root
/test pull-kubevirt-e2e-k8s-1.29-sig-monitoring
/test pull-kubevirt-e2e-k8s-1.29-sig-network-multus-v4
/test pull-kubevirt-e2e-k8s-1.29-sig-storage-root
/test pull-kubevirt-e2e-k8s-1.29-single-node
/test pull-kubevirt-e2e-k8s-1.29-swap-enabled
/test pull-kubevirt-gosec
/test pull-kubevirt-goveralls
/test pull-kubevirt-metrics-lint
/test pull-kubevirt-unit-test-arm64
/test pull-kubevirt-verify-rpms

Use /test all to run the following jobs that were automatically triggered:

pull-kubevirt-apidocs
pull-kubevirt-build
pull-kubevirt-build-arm64
pull-kubevirt-check-tests-for-flakes
pull-kubevirt-check-unassigned-tests
pull-kubevirt-client-python
pull-kubevirt-code-lint
pull-kubevirt-conformance-arm64
pull-kubevirt-e2e-arm64
pull-kubevirt-e2e-k8s-1.27-sig-performance
pull-kubevirt-e2e-k8s-1.29-sig-compute
pull-kubevirt-e2e-k8s-1.29-sig-compute-migrations
pull-kubevirt-e2e-k8s-1.29-sig-network
pull-kubevirt-e2e-k8s-1.29-sig-operator
pull-kubevirt-e2e-k8s-1.29-sig-storage
pull-kubevirt-fossa
pull-kubevirt-generate
pull-kubevirt-goveralls
pull-kubevirt-manifests
pull-kubevirt-prom-rules-verify
pull-kubevirt-unit-test
pull-kubevirt-unit-test-arm64
pull-kubevirt-verify-go-mod

In response to this:

/retest pull-kubevirt-unit-test

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.

xpivarc · 2024-02-21T11:18:42Z

/cc @awels @akalenyu

xpivarc · 2024-02-21T12:06:59Z

/test pull-kubevirt-unit-test

xpivarc · 2024-02-21T12:54:53Z

I do wonder why VirtualMachine One valid VirtualMachine controller given should hotplug a vm that is running and friends expect to see the request. @awels @davidvossel would you be able to answer?

awels · 2024-02-21T14:30:10Z

So the way the mechanism works is the following:

The sub resources gets a hot(un)plug request.
It validates the request, and if it passes, it modifies the VM status volume requests.
Virt controller processes this list and modifies the VM spec to indicate the change.
If the VM is active, it then modifies the VMI spec.

So you see the volume request in step 2. That looks like what is failing in the test.

xpivarc · 2024-02-21T16:05:32Z

So the way the mechanism works is the following:
1. The sub resources gets a hot(un)plug request.

2. It validates the request, and if it passes, it modifies the VM status volume requests.

3. Virt controller processes this list and modifies the VM spec to indicate the change.

4. If the VM is active, it then modifies the VMI spec.
So you see the volume request in step 2. That looks like what is failing in the test.

All the steps are performed but there is an assumption that the volume requests are not cleared. I do wonder why. This PR exposes that the assumption was wrong from the beginning. I wonder if I can simply remove it from a unit test. WDYT?

Note: See // vol request shouldn't be cleared until update status observes the new volume change

Barakmor1 · 2024-02-22T07:58:19Z

/cc

A real Kubernetes API returns the modified VM. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

Based on strategy a VMI creation is expected Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc · 2024-02-29T12:22:36Z

/hold
Depends on #11372

jean-edouard

/approve
That's amazing, thank you! Just a couple unimportant nits in-line.
#11372 is included here, on purpose I assume, a rebase will probably be needed once that merges.

jean-edouard · 2024-02-29T13:41:59Z

pkg/virt-controller/watch/vm_test.go

+			vmInterface.EXPECT().Update(context.Background(), gomock.Any()).DoAndReturn(func(ctx context.Context, vm *virtv1.VirtualMachine) (*virtv1.VirtualMachine, error) {
+				Expect(vm.Spec.Template.Spec.Volumes[0].Name).To(Equal("vol1"))
+				return vm, nil
+			})


Nit: no need to use DoAndReturn() when no return value has to be computed. Just fixing the func arg in the Do() above would look better IMO. More instances of that present in the PR.

Do you mean using Do and Return? This doesn't work because I still want to return the updated vm and I can only do it with DoAndReturn. Did I miss anything?

Right, my bad

kubevirt-bot · 2024-02-29T13:45:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jean-edouard

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~~ [jean-edouard]

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

0xFelix

Only nits, looks good to me otherwise

pkg/virt-controller/watch/vm_test.go

0xFelix · 2024-02-29T15:06:41Z

pkg/virt-controller/watch/vm_test.go

+			vmInterface.EXPECT().Update(context.Background(), gomock.Any()).DoAndReturn(func(ctx context.Context, vm *virtv1.VirtualMachine) (*virtv1.VirtualMachine, error) {
+				Expect(vm.Spec.Template.Spec.Volumes[0].Name).To(Equal("vol1"))
+				return vm, nil
+			})


pkg/virt-controller/watch/vm_test.go

xpivarc · 2024-03-01T12:06:26Z

/hold Depends on #11372

/hold cancel

1. Remove a wrong assumption where we expect requests to not be removed in the same sync as we update the spec. 2. Cover more assumption, especially if update or hotplug fails. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc · 2024-03-01T12:13:11Z

Fixed pull-kubevirt-code-lint

vladikr · 2024-03-01T14:26:29Z

pkg/virt-controller/watch/vm_test.go

 		}

 		shouldExpectVMFinalizerRemoval := func(vm *virtv1.VirtualMachine) {
 			patch := fmt.Sprintf(`[{ "op": "test", "path": "/metadata/finalizers", "value": ["%s"] }, { "op": "replace", "path": "/metadata/finalizers", "value": [] }]`, virtv1.VirtualMachineControllerFinalizer)

-			vmInterface.EXPECT().Patch(context.Background(), vm.Name, types.JSONPatchType, []byte(patch), &metav1.PatchOptions{}).Return(vm, nil)
+			vmInterface.EXPECT().Patch(context.Background(), vm.Name, types.JSONPatchType, []byte(patch), &metav1.PatchOptions{}).DoAndReturn(


Is this now needed because of the addition of sanityExecute?
Previously this function was doing the right thing as it was just expecting a specific operation and not an actual object change.

Well, the change should be done by the client and the modified object should be returned. This is what happens if you are running the code against a real Kubernetes API server. Therefore, before it was wrong as we were returning a pointer to some VM, which on top of it was modified by the sync.

Note: once we switch to the fake client, this will be handled for us.

This wasn't doing any modifications. It was only expecting a patch. Yes, it did return a VM object, but the same one it got.
But now this actually modifies the object, so I'm a bit confused. Why is it the right thing to do?
I could expect the actual controller code to modify the object and not the test code.
What am I missing?

With the previous code, not only did the caller get back a pointer to a VM object that's actively used by the test code, but it also didn't include the requested patch.
Now we actually apply the patch (easy since we already know what it is) and return a pointer to an object that we don't rely on and that won't change over time.

@jean-edouard I'm still confused... previous code only had to observe the patch - not to perform it.
and now we are performing the patch while there was something else in the controller that had to apply the patch.

@jean-edouard +100.
The fact that we did nothing in the past is wrong, it does not reflect what happens if you run the code against real API.

Note: This can be still wrong but it is at least slightly better. We need #11297 to be as close as we can to the real behavior.

EDIT: I am happy to wait for #11297

perhaps it's better to wait.

The fact that we did nothing in the past is wrong, it does not reflect what happens if you run the code against real API.

Mainly because it's not a real API.
It's a unit test; All we needed to do was to observe the call being made.
I would expect complex flows to be tested in a functional test.

I would expect complex flows to be tested in a functional test.

If our "unit tests" really were unit tests (they're already much more than that) and if functional test were cheap (they're not) then I would 100% agree with you.
However, in the current state of things, I think we should always prefer unit tests when possible, no matter how complex.
Just my opinion, not really related to this PR.

I agree that controllers are difficult to test. There must be a balance between how much you can mock and the effectiveness of the test.
How do we know that the fake client behaves the same way as a real client?
Do we really need to play a catchup game between the fake client and the real one?

We are responsible for the code in our controller and this is what we should test in the unit tests (whatever they are).

A unit test or integration test will never be a substitute for a functional test that exercises the functionality altogether.
Paying "cheap" for a test will make us pay a much higher price later.

How do we know that the fake client behaves the same way as a real client?
Do we really need to play a catchup game between the fake client and the real one?

This is standard tooling from Kubernetes. We are not going to play any catch up as long as we don't want to test specific scenarios/bugs.

We are responsible for the code in our controller and this is what we should test in the unit tests (whatever they are).

Right but we should make sure we are testing realistic scenarios. For example, right now we don't. The patch should return an object that is only adjusted by the changes expressed in the request but until now we would return an object that contains every change or non (depending if we are looking at the code before or after fixing the cache corruption).

A unit test or integration test will never be a substitute for a functional test that exercises the functionality altogether.
Paying "cheap" for a test will make us pay a much higher price later.

+100, we should not remove functional test unless they are duplicates.

vladikr · 2024-03-07T23:53:32Z

/lgtm

kubevirt-commenter-bot · 2024-03-07T23:53:39Z

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.29-ipv6-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-network
/test pull-kubevirt-e2e-k8s-1.27-sig-storage
/test pull-kubevirt-e2e-k8s-1.27-sig-compute
/test pull-kubevirt-e2e-k8s-1.27-sig-operator
/test pull-kubevirt-e2e-k8s-1.28-sig-network
/test pull-kubevirt-e2e-k8s-1.28-sig-storage
/test pull-kubevirt-e2e-k8s-1.28-sig-compute
/test pull-kubevirt-e2e-k8s-1.28-sig-operator

vladikr · 2024-03-07T23:59:36Z

/hold
let me just go over this again.

kubevirt-bot · 2024-03-08T01:50:39Z

@xpivarc: 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-e2e-k8s-1.29-sig-storage	`6a0bee7`	link	true	`/test pull-kubevirt-e2e-k8s-1.29-sig-storage`
pull-kubevirt-e2e-k8s-1.27-sig-compute	`6a0bee7`	link	true	`/test pull-kubevirt-e2e-k8s-1.27-sig-compute`

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.

xpivarc · 2024-03-25T13:43:56Z

/hold let me just go over this again.

@vladikr I would change this to be based on #11297

kubevirt-bot · 2024-04-09T16:24:34Z

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.

xpivarc · 2024-04-17T12:16:04Z

/close
#11726

kubevirt-bot · 2024-04-17T12:16:07Z

@xpivarc: Closed this PR.

In response to this:

/close
#11726

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Feb 21, 2024

kubevirt-bot requested review from AlonaKaplan and enp0s3 February 21, 2024 11:17

kubevirt-bot added the size/M label Feb 21, 2024

kubevirt-bot requested review from akalenyu and awels February 21, 2024 11:18

kubevirt-bot requested a review from Barakmor1 February 22, 2024 07:58

kubevirt-bot added size/L and removed size/M labels Feb 27, 2024

xpivarc added 2 commits February 29, 2024 13:06

VM unit, Fix return args

21f5182

A real Kubernetes API returns the modified VM. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

VM unit: Fix missing VMI creation

88fe9f8

Based on strategy a VMI creation is expected Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc force-pushed the vm-controller-unit-return-args branch from 4b92721 to 3b2c3e2 Compare February 29, 2024 12:21

xpivarc marked this pull request as ready for review February 29, 2024 12:22

kubevirt-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 29, 2024

kubevirt-bot requested a review from 0xFelix February 29, 2024 12:22

jean-edouard reviewed Feb 29, 2024

View reviewed changes

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

0xFelix reviewed Feb 29, 2024

View reviewed changes

xpivarc force-pushed the vm-controller-unit-return-args branch from 3b2c3e2 to 71f0042 Compare March 1, 2024 08:38

kubevirt-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 1, 2024

Test assumotions about volume requests

6a0bee7

1. Remove a wrong assumption where we expect requests to not be removed in the same sync as we update the spec. 2. Cover more assumption, especially if update or hotplug fails. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc force-pushed the vm-controller-unit-return-args branch from 71f0042 to 6a0bee7 Compare March 1, 2024 12:12

vladikr reviewed Mar 1, 2024

View reviewed changes

xpivarc mentioned this pull request Mar 6, 2024

Vm controller fix update and status update #11457

Closed

kubevirt-bot assigned vladikr Mar 7, 2024

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

kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2024

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

kubevirt-bot closed this Apr 17, 2024

Vm controller unit tests: fix return args #11322

Vm controller unit tests: fix return args #11322

Conversation

xpivarc commented Feb 21, 2024 • edited

What this PR does

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

Special notes for your reviewer

Release note

kubevirt-bot commented Feb 21, 2024

xpivarc commented Feb 21, 2024

kubevirt-bot commented Feb 21, 2024

xpivarc commented Feb 21, 2024

xpivarc commented Feb 21, 2024

xpivarc commented Feb 21, 2024

awels commented Feb 21, 2024

xpivarc commented Feb 21, 2024 • edited

Barakmor1 commented Feb 22, 2024

xpivarc commented Feb 29, 2024

jean-edouard 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

kubevirt-bot commented Feb 29, 2024

0xFelix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xpivarc commented Mar 1, 2024

xpivarc commented Mar 1, 2024

Choose a reason for hiding this comment

xpivarc Mar 4, 2024 • 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

xpivarc Mar 8, 2024 • edited

Choose a reason for hiding this comment

vladikr Mar 8, 2024 • 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

vladikr commented Mar 7, 2024

kubevirt-commenter-bot commented Mar 7, 2024

vladikr commented Mar 7, 2024 • edited

kubevirt-bot commented Mar 8, 2024

xpivarc commented Mar 25, 2024

kubevirt-bot commented Apr 9, 2024

xpivarc commented Apr 17, 2024

kubevirt-bot commented Apr 17, 2024

xpivarc commented Feb 21, 2024 •

edited

xpivarc commented Feb 21, 2024 •

edited

xpivarc Mar 4, 2024 •

edited

xpivarc Mar 8, 2024 •

edited

vladikr Mar 8, 2024 •

edited

vladikr commented Mar 7, 2024 •

edited