Skip to content

revamp find content logic to fix 290 291 292#294

Merged
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
yuxiangqian:GetContent
Apr 21, 2020
Merged

revamp find content logic to fix 290 291 292#294
k8s-ci-robot merged 1 commit intokubernetes-csi:masterfrom
yuxiangqian:GetContent

Conversation

@yuxiangqian
Copy link
Contributor

@yuxiangqian yuxiangqian commented Apr 13, 2020 •
edited
Loading

What type of PR is this?
/kind bug

What this PR does / why we need it:
fixes issue 290 291 292

Which issue(s) this PR fixes:
Fixes #290 #291 #292

Special notes for your reviewer:

Does this PR introduce a user-facing change?:
NONE

This PR fixes the following issues in snapshot controller:
Issue #290 : Disallow a pre-provisioned VolumeSnapshot pointing to a dynamically created VolumeSnapshotContent
Issue #291 : Verify VolumeSnapshot and VolumeSnapshotContent are bi-directional bound before initializing a deletion on a VolumeSnapshotContent which the to-be-deleted VolumeSnapshot points to.
Issue #292 : Allow deletion of a VolumeSnapshot/VolumeSnapshotContent when the VolumeSnapshotContent's DeletionPolicy has been updated from Delete to Retain

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. labels Apr 13, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 13, 2020
@yuxiangqian
Copy link
Contributor Author

yuxiangqian commented Apr 13, 2020

/assign @xing-yang

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Apr 13, 2020
@xing-yang
Copy link
Collaborator

xing-yang commented Apr 13, 2020

Can you elaborate what is fixed in this PR in the release notes section as that will go to the changelog.

@xing-yang xing-yang mentioned this pull request Apr 13, 2020
Merged
xing-yang
xing-yang reviewed Apr 14, 2020
View reviewed changes
Copy link
Collaborator

@xing-yang xing-yang left a comment

Choose a reason for hiding this comment

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

Review WIP.

xing-yang
xing-yang reviewed Apr 15, 2020
View reviewed changes
@yuxiangqian
Copy link
Contributor Author

yuxiangqian commented Apr 15, 2020

Can you elaborate what is fixed in this PR in the release notes section as that will go to the changelog.

Done. PTAL

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 15, 2020
@xing-yang
Copy link
Collaborator

xing-yang commented Apr 16, 2020

Issue #292 : Allow deletion of a VolumeSnapshot/VolumeSnapshotContent when the VolumeSnapshotContent's DeletionPolicy has been updated from Delete to Retain

Should just be "Allow deletion of a VolumeSnapshot when ..." as VolumeSnapshotContent should not be deleted with a Retain policy.

xing-yang
xing-yang reviewed Apr 16, 2020
View reviewed changes
@yuxiangqian
Copy link
Contributor Author

yuxiangqian commented Apr 16, 2020

Issue #292 : Allow deletion of a VolumeSnapshot/VolumeSnapshotContent when the VolumeSnapshotContent's DeletionPolicy has been updated from Delete to Retain

Should just be "Allow deletion of a VolumeSnapshot when ..." as VolumeSnapshotContent should not be deleted with a Retain policy.

technically it should be both VolumeSnapshot and VolumeSnapshotContent? Without the fix, there is no way to delete VolumeSnapshot without remove finalizer on it manually. VolumeSnapshotContent deletion is not possible (kubectl delete vsc-name) without this fix as well.

xing-yang
xing-yang reviewed Apr 16, 2020
View reviewed changes
@xing-yang
Copy link
Collaborator

xing-yang commented Apr 17, 2020

technically it should be both VolumeSnapshot and VolumeSnapshotContent? Without the fix, there is no way to delete VolumeSnapshot without remove finalizer on it manually. VolumeSnapshotContent deletion is not possible (kubectl delete vsc-name) without this fix as well.

Technically it is correct but is sounds wrong because it is associated with Retain policy:).

xing-yang
xing-yang reviewed Apr 17, 2020
View reviewed changes
xing-yang
xing-yang reviewed Apr 17, 2020
View reviewed changes
xing-yang
xing-yang reviewed Apr 18, 2020
View reviewed changes
xing-yang
xing-yang reviewed Apr 21, 2020
View reviewed changes
@xing-yang
Copy link
Collaborator

xing-yang commented Apr 21, 2020

@yuxiangqian Just some nits. Please squash all commits after addressing comments.

@yuxiangqian
revamp find content logic to fix 290 291 292
45324b7 
add snapshot source validation in syncSnapshot
add content source validation in syncContent
@yuxiangqian
Copy link
Contributor Author

yuxiangqian commented Apr 21, 2020

addressed comments and squashed. thanks Xing

@xing-yang
Copy link
Collaborator

xing-yang commented Apr 21, 2020

/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 Apr 21, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Apr 21, 2020

[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

Details 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 71beade into kubernetes-csi:master Apr 21, 2020
@yuxiangqian yuxiangqian deleted the GetContent branch April 21, 2020 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xing-yang xing-yang xing-yang left review comments

@jingxu97 jingxu97 Awaiting requested review from jingxu97

@saad-ali saad-ali Awaiting requested review from saad-ali

Assignees

@xing-yang xing-yang

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

findContentFromStore in snapshot controller potentially allows rebinding a dynamically provisioned VSC to a pre-provisioned VS

3 participants

@yuxiangqian @xing-yang @k8s-ci-robot