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

Fix adoption of orphan DataVolumes #5595

Merged
merged 1 commit into from May 6, 2021

Conversation

zcahana
Copy link
Contributor

@zcahana zcahana commented May 5, 2021

What this PR does / why we need it:

Fixes a bug in the DataVolumes adoption logic, which tries to patch a VMI (add an ownerReference) instead of patching the DV.

The observed behavior is that the DV is left unclaimed, which manifests itself in several ways:

  • If the VMI is stopped, then it can't be started
  • If the VMI is running, then it can't be stopped
  • If the VM is deleted, then the deletion doesn't cascade to the DV.

Steps to reproduce (using this example VM):

# create any VM with dataVolumeTemplates
kubectl apply -f alpine.yaml

# delete the VM with with --cascade=orphan,
# i.e. release owned objects without deleting them
kubectl delete vm alpine --cascade=orphan

# recreate the VM (or another) referencing the same DV
kubectl apply -f alpine.yaml

# try to start the VM. It won't.
virtctl start alpine

Special notes for your reviewer:

Also added a unit test to cover this behavior.

Release note:

Fixes adoption of orphan DataVolumes

@kubevirt-bot 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. labels May 5, 2021
@ezrasilvera
Copy link
Contributor

Thanks @zcahana . Can you please add to the description what is the observed error and how to produce it ?

@zcahana
Copy link
Contributor Author

zcahana commented May 5, 2021

Can you please add to the description what is the observed error and how to produce it ?

Added.

@mhenriks
Copy link
Member

mhenriks commented May 5, 2021

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2021
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mhenriks

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:

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2021
Signed-off-by: Zvi Cahana <zvic@il.ibm.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label May 5, 2021
@zcahana
Copy link
Contributor Author

zcahana commented May 5, 2021

@mhenriks Hi Michael, I've made a tiny change to the test as per @ezrasilvera's comment, can you please reapprove?

@ezrasilvera
Copy link
Contributor

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2021
@kubevirt-bot kubevirt-bot merged commit e9f9756 into kubevirt:master May 6, 2021
@zcahana zcahana deleted the fix_adopt_dv branch May 6, 2021 08:09
@zcahana
Copy link
Contributor Author

zcahana commented May 11, 2021

@mhenriks do we need to backport this bugfix to previous releases? Based on the backporting policy it seems to be eligible for backporting, but is there any criteria to determine what eligible bugfixes are actually backported?

@davidvossel
Copy link
Member

is there any criteria to determine what eligible bugfixes are actually backported?

There's no criteria. The policy is intentionally vague there. As it is today, anyone who wants to backport a bugfix can do that to any stable branch

@zcahana
Copy link
Contributor Author

zcahana commented May 13, 2021

Very well. So based on the unformal criteria of "is anyone likely to hit this bug", I think we can skip backporting it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants