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

Add VolumeSnapshotBeingDeleted annotation to content #249

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

xing-yang
Copy link
Collaborator

@xing-yang xing-yang commented Feb 7, 2020

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR adds VolumeSnapshotBeingDeleted annotation to the VolumeSnapshotContent object before the finalizers on the VolumeSnapshot object is removed. This is to make sure that the annotation is on the Content before VolumeSnapshot object is removed from the API server.

Which issue(s) this PR fixes:
Fixes #248

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Fixes a problem deleting VolumeSnapshotContent with `Retain` policy for pre-provisioned snapshots.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 7, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 7, 2020
@xing-yang xing-yang changed the title WIP: Add VolumeSnapshotBeingDeleted annotation to content Add VolumeSnapshotBeingDeleted annotation to content Feb 7, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2020
@xing-yang
Copy link
Collaborator Author

/assign @yuxiangqian

@xing-yang
Copy link
Collaborator Author

@yuxiangqian can you review this?

@yuxiangqian
Copy link
Contributor

LGTM, can you add a test case?

@jsafrane
Copy link
Contributor

I got lost in snapshot finalizers... With this PR, snapshot-controller adds annotation and exernal-snapshotter, seeing the annotation, removes the finalizer. Can the snapshot-controller remove the finalizer right away?

@xing-yang
Copy link
Collaborator Author

I got lost in snapshot finalizers... With this PR, snapshot-controller adds annotation and exernal-snapshotter, seeing the annotation, removes the finalizer. Can the snapshot-controller remove the finalizer right away?

This is to avoid leaking of the physical snapshot on the storage system. We want to make sure the physical snapshot is deleted from the storage system before removing the finalizer if the deletion policy is Delete. It is possible to remove finalizer from the content in the snapshot-controller if the policy is Retain, but we'd like to keep the content finalizer removal logic in one place which is the sidecar external-snapshotter.

@jsafrane
Copy link
Contributor

Ack

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 28, 2020
@xing-yang
Copy link
Collaborator Author

@yuxiangqian Unit test added. PTAL.

@yuxiangqian
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: xing-yang, yuxiangqian

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 [xing-yang,yuxiangqian]

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

@k8s-ci-robot k8s-ci-robot merged commit 68880a2 into kubernetes-csi:master Feb 28, 2020
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Mar 30, 2020
@leshibily
Copy link

@xing-yang how did I add the annotation? or snapshot-controller adds the annotation, what config should I do to make it happen?

@xing-yang
Copy link
Collaborator Author

@leshibily This annotation is added internally by the snapshot-controller. You should not be adding it manually. Also it will be removed by the snapshot-controller after the snapshot is deleted from the storage system.

@DSO-TB
Copy link

DSO-TB commented Jul 20, 2022

@xing-yang I am currently running on 6.0.1 and still facing issues with volumesnapshotcontents not being deleted. I have verified that the code from the commit is there and when I remove the finalizer they delete fine. Anything I can check in the annotations to verify that everything is correct or is there another issue open I can reference?

@xing-yang
Copy link
Collaborator Author

@DSO-TB Can you open an issue, describe your testing steps and provide logs?

RaunakShah added a commit to RaunakShah/external-snapshotter that referenced this pull request Feb 23, 2024
dc4d0ae Merge pull request kubernetes-csi#249 from jsafrane/use-go-version
e681b17 Use .go-version to get Kubernetes go version

git-subtree-dir: release-tools
git-subtree-split: dc4d0ae
tyuchn added a commit to tyuchn/external-snapshotter that referenced this pull request Mar 20, 2024
dc4d0ae Merge pull request kubernetes-csi#249 from jsafrane/use-go-version
e681b17 Use .go-version to get Kubernetes go version

git-subtree-dir: release-tools
git-subtree-split: dc4d0ae
andyzhangx pushed a commit to andyzhangx/external-snapshotter that referenced this pull request Jun 5, 2024
Use .go-version to get Kubernetes go version
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot delete VolumeSnapshotContent with 'Retain' policy for imported snapshot using external-snapshotter beta
6 participants