Vm controller copy fix #11265

xpivarc · 2024-02-15T16:23:06Z

What this PR does

Before this PR:
A cache is corrupted.

After this PR:
A cache is not corrupted ;)

Fixes #

Special notes for your reviewer

This is a suboptimal patch to be able to backport this for the latest releases.

Release note

Bug fix: VM controller doesn't corrupt its cache anymore

jean-edouard

/approve
Just one nit. Also you pushed a modified example VM. Thank you!

jean-edouard · 2024-02-15T16:28:45Z

pkg/virt-controller/watch/vm_test.go

@@ -333,6 +334,12 @@ var _ = Describe("VirtualMachine", func() {
 			mockQueue.Wait()
 		}

+		execute := func(vm *virtv1.VirtualMachine) {


Nit: the name execute() is confusing, since vm.go already has one. Maybe sanityExecute() or something like that would be a better name?

Done, thanks!

kubevirt-bot · 2024-02-15T16:30:02Z

[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

This allow us to detect if a given VM struct is being modified by Execute. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

cache.Store is returning a pointer to stored struct. Any modification of the struct is therefore reflected in the underlying store. This can cause issues when a loop changes something but the update call fails. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

These tests are relying on a fact that a cached object is being modified Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc · 2024-02-15T16:37:05Z

/cherry-pick release-1.2
/cherry-pick release-1.1
/cherry-pick release-1.0
/cherry-pick release-0.59

kubevirt-bot · 2024-02-15T16:37:08Z

@xpivarc: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.2
/cherry-pick release-1.1
/cherry-pick release-1.0
/cherry-pick release-0.59

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.

vasiliy-ul · 2024-02-15T18:01:18Z

pkg/virt-controller/watch/vm.go

@@ -297,6 +297,7 @@ func (c *VMController) execute(key string) error {
 		return nil
 	}
 	vm := obj.(*virtv1.VirtualMachine)
+	vm = vm.DeepCopy()


Just curious: where was the actual cache corruption? I tried to track it and the code below seems to do deep copy where needed. But maybe I missed that...

https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L2923-L2940

I think there was one or two more. Anyway, there are additional bugs, will post PRs tomorrow :)

indeed. nice catch 👍

@vasiliy-ul
Here as well.
https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L875
https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L2944
https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L2957

@vasiliy-ul Here as well. https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L875

👍

https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L2944 https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-controller/watch/vm.go#L2957

c.removeVMFinalizer and c.addVMFinalizer do a deep copy internally.

Ah right, should have looked at my notes better... They do it but they do not need to. I will post a patch for it.

kubevirt-commenter-bot · 2024-02-15T18:14:19Z

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

kubevirt-bot · 2024-02-16T04:52:02Z

@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.28-sig-compute	`a3060dd`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.28-sig-compute`
pull-kubevirt-e2e-k8s-1.27-sig-storage	`a3060dd`	link	unknown	`/test pull-kubevirt-e2e-k8s-1.27-sig-storage`

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 · 2024-02-16T07:55:42Z

@xpivarc: #11265 failed to apply on top of branch "release-1.1":

Applying: Unit test: Wrap controller.Execute
Using index info to reconstruct a base tree...
M	pkg/virt-controller/watch/vm_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/watch/vm_test.go
CONFLICT (content): Merge conflict in pkg/virt-controller/watch/vm_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Unit test: Wrap controller.Execute
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2
/cherry-pick release-1.1
/cherry-pick release-1.0
/cherry-pick release-0.59

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-02-16T07:56:18Z

@xpivarc: #11265 failed to apply on top of branch "release-1.0":

Applying: Unit test: Wrap controller.Execute
Using index info to reconstruct a base tree...
M	pkg/virt-controller/watch/vm_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/watch/vm_test.go
CONFLICT (content): Merge conflict in pkg/virt-controller/watch/vm_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Unit test: Wrap controller.Execute
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2
/cherry-pick release-1.1
/cherry-pick release-1.0
/cherry-pick release-0.59

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-02-16T07:57:03Z

@xpivarc: #11265 failed to apply on top of branch "release-0.59":

Applying: Unit test: Wrap controller.Execute
Using index info to reconstruct a base tree...
M	pkg/virt-controller/watch/vm_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/virt-controller/watch/vm_test.go
CONFLICT (content): Merge conflict in pkg/virt-controller/watch/vm_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Unit test: Wrap controller.Execute
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2
/cherry-pick release-1.1
/cherry-pick release-1.0
/cherry-pick release-0.59

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-02-16T07:57:47Z

@xpivarc: new pull request created: #11271

In response to this:

/cherry-pick release-1.2
/cherry-pick release-1.1
/cherry-pick release-1.0
/cherry-pick release-0.59

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 release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Feb 15, 2024

kubevirt-bot requested review from dhiller and vasiliy-ul February 15, 2024 16:23

jean-edouard reviewed Feb 15, 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 15, 2024

xpivarc added 3 commits February 15, 2024 17:33

Unit test: Wrap controller.Execute

709377c

This allow us to detect if a given VM struct is being modified by Execute. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

Fix instance type unit tests

a3060dd

These tests are relying on a fact that a cached object is being modified Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc force-pushed the vm-controller-copy-fix branch from 6e9422f to a3060dd Compare February 15, 2024 16:36

vasiliy-ul reviewed Feb 15, 2024

View reviewed changes

vasiliy-ul approved these changes Feb 15, 2024

View reviewed changes

kubevirt-bot assigned vasiliy-ul Feb 15, 2024

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

kubevirt-bot merged commit 9a79118 into kubevirt:main Feb 16, 2024
35 of 37 checks passed

kubevirt-bot mentioned this pull request Feb 16, 2024

[release-1.2] Vm controller copy fix #11271

Merged

This was referenced Feb 19, 2024

[release-1.0] vm controller copy fix #11291

Merged

[release-1.1] vm controller copy fix #11292

Merged

This was referenced Feb 27, 2024

VM: Remove redundant DeepCopy - cleanupRestartRequired & addRestartRequiredIfNeeded #11339

Merged

Remove redundant DeepCopy calls #11341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vm controller copy fix #11265

Vm controller copy fix #11265

xpivarc commented Feb 15, 2024

jean-edouard left a comment

jean-edouard Feb 15, 2024

xpivarc Feb 15, 2024

kubevirt-bot commented Feb 15, 2024

xpivarc commented Feb 15, 2024

kubevirt-bot commented Feb 15, 2024

vasiliy-ul Feb 15, 2024

xpivarc Feb 15, 2024

vasiliy-ul Feb 15, 2024

xpivarc Feb 15, 2024

vasiliy-ul Feb 16, 2024

xpivarc Feb 16, 2024

kubevirt-commenter-bot commented Feb 15, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

Vm controller copy fix #11265

Vm controller copy fix #11265

Conversation

xpivarc commented Feb 15, 2024

What this PR does

Special notes for your reviewer

Release note

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

kubevirt-bot commented Feb 15, 2024

xpivarc commented Feb 15, 2024

kubevirt-bot commented Feb 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubevirt-commenter-bot commented Feb 15, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024

kubevirt-bot commented Feb 16, 2024