Restart required correctly propagate #11836

xpivarc · 2024-05-02T08:35:15Z

What this PR does

This PR fixes cases where RestartRequired condition is not propagated to Kubernetes API. The cause is Update call that should not be called when only .Status is changed (note if a change was made the condition will be missing). Also ensure that we detect a .Status change in the updateStatus.

Fixes #

Special notes for your reviewer

Release note

Bug fix: Correctly reflect RestartRequired condition

Based on #11726

kubevirt-bot · 2024-05-02T08:35:17Z

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

Removing the `DeepCopy` from updateStatus allow us to detect changes to the Status inside the "sync" and acaccurately represent changes. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

VM CRD has a status subresource enabled. This means that "Update" ignores changes to the status stanza. Also propagate status changes from "hotplug sync" by returning the cloned vm. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

jean-edouard · 2024-05-10T13:42:13Z

pkg/virt-controller/watch/vm.go

+			} else {
+				vm = vmCopy


That means that if spec/objectmeta changed, we ignore status changes, right?
Example: something above changed both vm.spec and vm.status. We'll call update, which will lose the status changes, and return updatedVm instead of vmCopy.
How about something like that instead?

Suggested change

} else {

vm = vmCopy

}

vm.Status = *vmCopy.Status.DeepCopy()

That means that if spec/objectmeta changed, we ignore status changes, right?

Yes, there are couple of edge cases that are not fixed by this PR but the last 3 PRs are making it much more simple to fix these cases. @acardace should have a PR that will fix at least partially these cases.

Example: something above changed both vm.spec and vm.status. We'll call update, which will lose the status changes, and return updatedVm instead of vmCopy.
How about something like that instead?

WDYT about adding equality.Semantic.DeepEqual(vm.Status, vmCopy.Status) && (!equality.Semantic.DeepEqual(vm.Spec, vmCopy.Spec) || !equality.Semantic.DeepEqual(vm.ObjectMeta, vmCopy.ObjectMeta)) { ? But as I said this should be fixed as follow up.

The vm.Status = *vmCopy.Status should not be needed because if the status have changed then no spec changes should be done as well...

The vm.Status = *vmCopy.Status should not be needed because if the status have changed then no spec changes should be done as well...

If we know we never change both Status and Spec at the same time, then this is all good!
/approve

We do unfortunately but that is bug on its own but the PR doesn't make it worse

kubevirt-bot · 2024-05-10T14:50:01Z

[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

xpivarc · 2024-05-11T22:30:10Z

/cc @fossedihelm

xpivarc · 2024-05-11T22:30:52Z

/cherry-pick release-1.2

kubevirt-bot · 2024-05-11T22:30:53Z

@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

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.

kubevirt-commenter-bot · 2024-05-13T06:19:44Z

Required labels detected, running phase 2 presubmits:
/test pull-kubevirt-e2e-windows2016
/test pull-kubevirt-e2e-kind-sriov
/test pull-kubevirt-e2e-k8s-1.30-ipv6-sig-network
/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
/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

lyarwood · 2024-05-13T16:37:55Z

/retest-required

kubevirt-commenter-bot · 2024-05-13T20:38:33Z

/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.

kubevirt-commenter-bot · 2024-05-14T00:38:32Z

/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.

kubevirt-commenter-bot · 2024-05-14T04:38:33Z

/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.

fossedihelm · 2024-05-14T04:45:02Z

/hold
investigating on the continuous failure

kubevirt-bot · 2024-05-14T06:00:18Z

@xpivarc: 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-k8s-1.30-sig-compute-migrations	`71de121`	link	true	`/test pull-kubevirt-e2e-k8s-1.30-sig-compute-migrations`

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-05-24T06:15:37Z

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-sigs/prow repository.

fossedihelm · 2024-05-24T19:49:41Z

Close in favor of
#11973

kubevirt-bot requested review from 0xFelix and vasiliy-ul May 2, 2024 08:35

kubevirt-bot added size/XXL sig/buildsystem Denotes an issue or PR that relates to changes in the build system. labels May 2, 2024

This was referenced May 2, 2024

Vm controller fix update and status update #11457

Closed

VM controller - move unit tests to fake client (VirtualMachineInterface) #11726

Merged

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

xpivarc added 2 commits May 10, 2024 12:00

VM: Propagate status updates from "sync"

9d62a8b

Removing the `DeepCopy` from updateStatus allow us to detect changes to the Status inside the "sync" and acaccurately represent changes. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>

xpivarc force-pushed the restart-required-correctly-propagate branch from f53d55e to 71de121 Compare May 10, 2024 10:19

kubevirt-bot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL labels May 10, 2024

xpivarc marked this pull request as ready for review May 10, 2024 10:19

kubevirt-bot added size/L and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 10, 2024

kubevirt-bot requested review from enp0s3 and stu-gott May 10, 2024 10:19

jean-edouard reviewed May 10, 2024

View reviewed changes

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

kubevirt-bot requested a review from fossedihelm May 11, 2024 22:30

fossedihelm approved these changes May 13, 2024

View reviewed changes

kubevirt-bot assigned fossedihelm May 13, 2024

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

orelmisan mentioned this pull request May 13, 2024

Ctrl, VM: Require restart when socket count is reduced #11883

Merged

8 tasks

lyarwood mentioned this pull request May 13, 2024

instancetype: Support Live Updates #11455

Open

8 tasks

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

acardace mentioned this pull request May 14, 2024

Restart required correctly propagate #11904

Closed

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

acardace closed this May 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restart required correctly propagate #11836

Restart required correctly propagate #11836

xpivarc commented May 2, 2024

kubevirt-bot commented May 2, 2024

jean-edouard May 10, 2024 •

edited

xpivarc May 10, 2024

jean-edouard May 10, 2024

xpivarc May 10, 2024

kubevirt-bot commented May 10, 2024

xpivarc commented May 11, 2024

xpivarc commented May 11, 2024

kubevirt-bot commented May 11, 2024

kubevirt-commenter-bot commented May 13, 2024

lyarwood commented May 13, 2024

kubevirt-commenter-bot commented May 13, 2024

kubevirt-commenter-bot commented May 14, 2024

kubevirt-commenter-bot commented May 14, 2024

fossedihelm commented May 14, 2024

kubevirt-bot commented May 14, 2024

kubevirt-bot commented May 24, 2024

fossedihelm commented May 24, 2024

Restart required correctly propagate #11836

Restart required correctly propagate #11836

Conversation

xpivarc commented May 2, 2024

What this PR does

Special notes for your reviewer

Release note

kubevirt-bot commented May 2, 2024

jean-edouard May 10, 2024 • edited

Choose a reason for hiding this comment

xpivarc May 10, 2024

Choose a reason for hiding this comment

jean-edouard May 10, 2024

Choose a reason for hiding this comment

xpivarc May 10, 2024

Choose a reason for hiding this comment

kubevirt-bot commented May 10, 2024

xpivarc commented May 11, 2024

xpivarc commented May 11, 2024

kubevirt-bot commented May 11, 2024

kubevirt-commenter-bot commented May 13, 2024

lyarwood commented May 13, 2024

kubevirt-commenter-bot commented May 13, 2024

kubevirt-commenter-bot commented May 14, 2024

kubevirt-commenter-bot commented May 14, 2024

fossedihelm commented May 14, 2024

kubevirt-bot commented May 14, 2024

kubevirt-bot commented May 24, 2024

fossedihelm commented May 24, 2024

jean-edouard May 10, 2024 •

edited