Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

xing-yang · 2018-10-10T16:55:08Z

This PR adds a Finalizer for VolumeSnapshotContent.
If the VolumeSnapshotContent is bound to a VolumeSnapshot,
the VolumeSnapshotContent is being used and cannot be
deleted.
This PR also adds a Finalizer for VolumeSnapshot.
If a volume is being created from the snapshot,
the VolumeSnapshot is being used and cannot be deleted.

k8s-ci-robot · 2018-10-10T16:55:10Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

xing-yang · 2018-11-20T20:51:10Z

@jingxu97 @wackxu PTAL

wackxu

The code overall LGTM, only some nits.

wackxu · 2018-11-21T09:22:58Z

pkg/controller/snapshot_controller.go

+
+// isVolumeBeingCreatedFromSnapshot checks if an volume is being created from the snapshot.
+func (ctrl *csiSnapshotController) isVolumeBeingCreatedFromSnapshot(snapshot *crdv1.VolumeSnapshot) bool {
+	pvcList, err := ctrl.client.CoreV1().PersistentVolumeClaims(snapshot.Namespace).List(metav1.ListOptions{})


we should use pvcLister to reduce the server pressure instead of using client

Good suggestion. I think a separate PR should be submitted to add pvcLister to the snapshot controller first, and after that this can be updated to use that.

could you create an issue for it so that we won't forget?

Sure. Opened issue #70

wackxu · 2018-11-21T09:28:50Z

pkg/controller/snapshot_controller.go

+	if needToAddContentFinalizer(content) {
+		// Content is not being deleted -> it should have the finalizer. The
+		// finalizer should be added by admission plugin, this is just to add
+		// the finalizer to old volume snapshot contents that were created before


for now, we do not have admission plugin to add the finalizer for snapshot content.

Will remove it for now.

wackxu · 2018-11-21T09:32:13Z

pkg/controller/snapshot_controller.go

+		// Snapshot is not being deleted -> it should have the finalizer. The
+		// finalizer should be added by admission plugin, this is just to add
+		// the finalizer to old volume snapshots that were created before
+		// the admission plugin was enabled.


for now, we do not have admission plugin to add the finalizer for snapshot.

Will remove it for now.

wackxu · 2018-11-21T09:56:11Z

1.Since we are adding admission webhook #36, we can also add a MutatingWebhook to add finalizer for snapshot and snapshot content when they create.

2.Can we make the protection feature in a separate controller, like k8s pv_protection_controller, it is convenient for us to test and we should obey single responsibility principle and make the snapshot controller to do the specialized task.

xing-yang · 2018-11-25T14:58:43Z

We'll add a webhook to add finalizer for snapshot after #36 is merged.

Jing and I discussed about whether to implement finalizer in a separate controller, similar to pv_protection_controller, but decided to keep it simple and add it to the same controller. There's a trade-off between separating logic into different controllers and the overhead of maintaining multiple controllers. PV controller is very big and adding more to it will make it more complicated, while the snapshot controller is much simpler and putting the logic in the same controller makes it easier to maintain.

wackxu

LGTM except some nits. And we need add unit test for these changes, maybe other pr.

wackxu · 2018-11-26T10:52:12Z

pkg/controller/snapshot_controller.go

+		// Check if a volume is being created from snapshot.
+		isUsed := ctrl.isVolumeBeingCreatedFromSnapshot(snapshot)
+		if !isUsed {
+			glog.V(5).Infof("syncSnapshot: Remove Finalizer for VolumeSnapshot[%s]", snapshot.Name)


snapshotKey(snapshot) maybe better for logging

wackxu · 2018-11-26T10:52:24Z

pkg/controller/snapshot_controller.go

+
+	if needToAddSnapshotFinalizer(snapshot) {
+		// Snapshot is not being deleted -> it should have the finalizer.
+		glog.V(5).Infof("syncSnapshot: Add Finalizer for VolumeSnapshot[%s]", snapshot.Name)


wackxu · 2018-11-26T10:53:56Z

pkg/controller/snapshot_controller.go

+func (ctrl *csiSnapshotController) isVolumeBeingCreatedFromSnapshot(snapshot *crdv1.VolumeSnapshot) bool {
+	pvcList, err := ctrl.client.CoreV1().PersistentVolumeClaims(snapshot.Namespace).List(metav1.ListOptions{})
+	if err != nil {
+		glog.Errorf("Failed to retrieve PVCs from the API server to check if volume snapshot %s is being used by a volume: %q", snapshot.Name, err)


wackxu · 2018-11-26T10:54:09Z

pkg/controller/snapshot_controller.go

+			}
+		}
+	}
+	glog.V(5).Infof("isVolumeBeingCreatedFromSnapshot: no volume is being created from snapshot %s", snapshot.Name)


jingxu97 · 2018-11-26T23:20:14Z

lgtm

This PR adds a Finalizer for VolumeSnapshotContent. If the VolumeSnapshotContent is bound to a VolumeSnapshot, the VolumeSnapshotContent is being used and cannot be deleted. This PR also adds a Finalizer for VolumeSnapshot. If a volume is being created from the snapshot, the VolumeSnapshot is being used and cannot be deleted.

jingxu97 · 2018-11-27T18:58:27Z

/lgtm

…cy-openshift-4.7-ose-csi-snapshot-controller Updating ose-csi-snapshot-controller builder & base images to be consistent with ART

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 10, 2018

k8s-ci-robot requested review from jingxu97 and lpabon October 10, 2018 16:55

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 10, 2018

xing-yang force-pushed the content_protect branch 3 times, most recently from e6cc336 to 6ed0fef Compare October 15, 2018 02:39

xing-yang changed the title ~~WIP: Add Finalizer for VolumeSnapshotContent~~ WIP: Add Finalizer for VolumeSnapshot/VolumeSnapshotContent Oct 15, 2018

xing-yang changed the title ~~WIP: Add Finalizer for VolumeSnapshot/VolumeSnapshotContent~~ WIP: Add Finalizer for VolumeSnapshot and VolumeSnapshotContent Oct 15, 2018

xing-yang force-pushed the content_protect branch 3 times, most recently from 70877b5 to dd6f314 Compare November 1, 2018 17:15

xing-yang changed the title ~~WIP: Add Finalizer for VolumeSnapshot and VolumeSnapshotContent~~ Add Finalizer for VolumeSnapshot and VolumeSnapshotContent Nov 1, 2018

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 Nov 1, 2018

xing-yang force-pushed the content_protect branch 2 times, most recently from 5de6d93 to 891f51f Compare November 20, 2018 20:48

wackxu reviewed Nov 21, 2018

View reviewed changes

xing-yang force-pushed the content_protect branch from 891f51f to c1078f1 Compare November 25, 2018 15:11

wackxu reviewed Nov 26, 2018

View reviewed changes

xing-yang mentioned this pull request Nov 27, 2018

Add pvcLister to snapshot controller #70

Closed

xing-yang force-pushed the content_protect branch from c1078f1 to 588e911 Compare November 27, 2018 03:07

xing-yang force-pushed the content_protect branch from 588e911 to ea17039 Compare November 27, 2018 18:36

k8s-ci-robot assigned jingxu97 Nov 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 27, 2018

k8s-ci-robot merged commit 840095d into kubernetes-csi:master Nov 27, 2018

xing-yang mentioned this pull request Jan 21, 2019

REQUEST: New membership for xing-yang kubernetes/org#392

Closed

6 tasks

xing-yang deleted the content_protect branch April 8, 2019 02:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

xing-yang commented Oct 10, 2018 •

edited

Loading

k8s-ci-robot commented Oct 10, 2018

xing-yang commented Nov 20, 2018

wackxu left a comment

wackxu Nov 21, 2018

xing-yang Nov 25, 2018

jingxu97 Nov 26, 2018

xing-yang Nov 27, 2018

wackxu Nov 21, 2018

xing-yang Nov 25, 2018

xing-yang Nov 25, 2018

wackxu Nov 21, 2018

xing-yang Nov 25, 2018

xing-yang Nov 25, 2018

wackxu commented Nov 21, 2018

xing-yang commented Nov 25, 2018

wackxu left a comment

wackxu Nov 26, 2018

xing-yang Nov 27, 2018

wackxu Nov 26, 2018

xing-yang Nov 27, 2018

wackxu Nov 26, 2018

xing-yang Nov 27, 2018

wackxu Nov 26, 2018

xing-yang Nov 27, 2018

jingxu97 commented Nov 26, 2018

jingxu97 commented Nov 27, 2018

Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

Add Finalizer for VolumeSnapshot and VolumeSnapshotContent #39

Conversation

xing-yang commented Oct 10, 2018 • edited Loading

k8s-ci-robot commented Oct 10, 2018

xing-yang commented Nov 20, 2018

wackxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wackxu commented Nov 21, 2018

xing-yang commented Nov 25, 2018

wackxu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jingxu97 commented Nov 26, 2018

jingxu97 commented Nov 27, 2018

xing-yang commented Oct 10, 2018 •

edited

Loading