Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restart required correctly propagate #11904

Closed
wants to merge 3 commits into from

Conversation

acardace
Copy link
Member

@acardace acardace commented May 14, 2024

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 and #11836

This PR only adds 8b619e0 on top of #11836 to fix an API regression.

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>
@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/L labels May 14, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from acardace. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

Otherwise if a VM gets created with RunStrategy == Halted|Manual and
then switched to RerunOnFailure the VM will not start.

Signed-off-by: Antonio Cardace <acardace@redhat.com>
@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels May 14, 2024
@acardace acardace requested review from fossedihelm and jean-edouard and removed request for lyarwood and EdDev May 14, 2024 08:55
@acardace
Copy link
Member Author

/hold
Checking the logic

@kubevirt-bot 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
@lyarwood
Copy link
Member

/cc


// only add the condition if the VM has been started at least once
if vmi != nil {
vm.Status.Conditions = append(vm.Status.Conditions, virtv1.VirtualMachineCondition{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please use the conditionmanager and the UpdateCondition method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants