Added controller for PVC deletion #422

Danil-Grigorev · 2020-03-11T10:43:41Z

/kind feature

What this PR does / why we need it:

This PR addresses an attempt to split PVC handling between external-provisioner and sig-storage-lib-external-provisioner, also introducing a way to handle finalizers, related to in-progress PVC cloning.

PVC referenced in a data source is now updated with finalizer provisioner.storage.kubernetes.io/cloning-protectionr at the moment when a ProvisionExt method is called, used to dynamically provision a PV to which the content will be cloned at.

When cloning is finished - all PVC referencing the one as
a data source goes from Pending to Bound state, which allows the finalizer to be removed before deletion.

Which issue(s) this PR fixes:

Fixes #414

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

finalizers:
- provisioner.storage.kubernetes.io/cloning-protection

is now set on PVCs which are the source ones for the cloning,
preventing their removal before the cloning finishes.

k8s-ci-robot · 2020-03-11T10:43:49Z

Welcome @Danil-Grigorev!

It looks like this is your first PR to kubernetes-csi/external-provisioner 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-csi/external-provisioner has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2020-03-11T10:43:49Z

Hi @Danil-Grigorev. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/controller/controller.go

msau42 · 2020-03-11T21:52:38Z

/ok-to-test
/assign @jsafrane

pkg/controller/clone_controller_test.go

jsafrane · 2020-03-16T11:47:04Z

pkg/controller/clone_controller.go

+	"sigs.k8s.io/sig-storage-lib-external-provisioner/controller"
+)
+
+// CloningProtectionController struct


Please add a comment what the controller does - it prevents a PVC from being deleted while it's cloned.

jsafrane · 2020-03-16T13:09:47Z

cmd/csi-provisioner/csi-provisioner.go

 	run := func(context.Context) {
-		stopCh := context.Background().Done()


Why did you remove stopCh?

Accidentally left this change after seeing issues with a similar approach in tests, will undo this.

jsafrane · 2020-03-16T13:26:34Z

pkg/controller/clone_controller_test.go

+				pvcPhase(
+					pvcDataSourceClone(
+						pvcNamed(baseClaim(), dstName),
+						srcName,
+					),
+					v1.ClaimPending)},


It would be more readable if you put claim as the last argument - function name + its arguments would be on the same line:

pvcPhase(v1.ClaimPending, pvcDataSourceClone(srcName, pvcNamed(dstName, baseClaim())

jsafrane · 2020-03-16T13:30:24Z

pkg/controller/controller.go

@@ -134,6 +134,8 @@ const (
 	annStorageProvisioner = "volume.beta.kubernetes.io/storage-provisioner"

 	snapshotNotBound = "snapshot %s not bound"
+
+	pvcCloneFinalizer = "external-provisioner.clone.kubernetes.io/finalizer"


provisioner.storage.kubernetes.io/cloning-protection

- Finalizer rename: 'external-provisioner.clone.kubernetes.io/finalizer' -> 'provisioner.storage.kubernetes.io/cloning-protection'

jsafrane · 2020-03-16T17:21:13Z

Good work and thanks a lot!
/lgtm
/approve

k8s-ci-robot · 2020-03-16T17:21:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev, jsafrane

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~~ [jsafrane]

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

msau42 · 2020-03-16T17:52:42Z

pkg/controller/clone_controller.go

+	}
+	p.claimInformer.AddEventHandlerWithResyncPeriod(claimHandler, controller.DefaultResyncPeriod)
+
+	for i := 0; i < threadiness; i++ {


This is going to double the number of goroutines, and default value of worker-threads is 100 (so 200) total. Do we really need that many threads to process this finalizer? @jsafrane

hm, maybe not...

I'll address this in a follow up PR. Sorry for the issue. Could 5 be sufficient @msau42?

…go_modules/github.com/golang/protobuf-1.5.3 Bump github.com/golang/protobuf from 1.5.2 to 1.5.3

Added controller for PVC deletion

c11f55b

k8s-ci-robot requested review from msau42 and sbezverk March 11, 2020 10:43

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 11, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 11, 2020

Danil-Grigorev added 2 commits March 11, 2020 14:27

On update pvc handling, added deletionTimestamp filtering

e7a7ad7

Added finalizer removal logic

21c1fdf

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 11, 2020

Danil-Grigorev changed the title ~~[WIP] Added controller for PVC deletion~~ Added controller for PVC deletion Mar 11, 2020

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 Mar 11, 2020

jsafrane reviewed Mar 11, 2020

View reviewed changes

k8s-ci-robot assigned jsafrane Mar 11, 2020

Danil-Grigorev added 2 commits March 12, 2020 11:40

Fixed existing tests

dc62b57

Using cashed PVCs, code restructuring, fixed tests

9dda579

Danil-Grigorev force-pushed the pvc-clone-finalizer branch from 4b1dbb9 to 9dda579 Compare March 12, 2020 10:40

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 12, 2020

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Mar 13, 2020

Added tests for cloning protection

5b36954

Danil-Grigorev force-pushed the pvc-clone-finalizer branch from 564c967 to 5b36954 Compare March 13, 2020 15:58

Adjusted tests for finalizer removal, added tests for it's creation

d0c6d55

jsafrane reviewed Mar 16, 2020

View reviewed changes

pkg/controller/clone_controller_test.go Outdated Show resolved Hide resolved

Added tests for claim enqueue, refactor for finalizer removal

bd9a0d2

jsafrane reviewed Mar 16, 2020

View reviewed changes

Danil-Grigorev added 2 commits March 16, 2020 15:44

Added description, refactored tests, undo stopCh removal

4a530dd

- Finalizer rename: 'external-provisioner.clone.kubernetes.io/finalizer' -> 'provisioner.storage.kubernetes.io/cloning-protection'

Better description message

cec1dc3

Danil-Grigorev requested a review from jsafrane March 16, 2020 15:05

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 16, 2020

k8s-ci-robot merged commit 59528e1 into kubernetes-csi:master Mar 16, 2020

Danil-Grigorev deleted the pvc-clone-finalizer branch March 16, 2020 17:29

msau42 reviewed Mar 16, 2020

View reviewed changes

Danil-Grigorev mentioned this pull request Mar 16, 2020

Reduced number of threads for finalizer removal to 1 #424

Merged

kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023

Merge pull request kubernetes-csi#422 from kubernetes-csi/dependabot/…

a2e45b5

…go_modules/github.com/golang/protobuf-1.5.3 Bump github.com/golang/protobuf from 1.5.2 to 1.5.3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added controller for PVC deletion #422

Added controller for PVC deletion #422

Danil-Grigorev commented Mar 11, 2020 •

edited

Loading

k8s-ci-robot commented Mar 11, 2020

k8s-ci-robot commented Mar 11, 2020

msau42 commented Mar 11, 2020

jsafrane Mar 16, 2020

jsafrane Mar 16, 2020

Danil-Grigorev Mar 16, 2020

jsafrane Mar 16, 2020

jsafrane Mar 16, 2020

jsafrane commented Mar 16, 2020

k8s-ci-robot commented Mar 16, 2020

msau42 Mar 16, 2020

jsafrane Mar 16, 2020

Danil-Grigorev Mar 16, 2020

		run := func(context.Context) {
		stopCh := context.Background().Done()

Added controller for PVC deletion #422

Added controller for PVC deletion #422

Conversation

Danil-Grigorev commented Mar 11, 2020 • edited Loading

k8s-ci-robot commented Mar 11, 2020

k8s-ci-robot commented Mar 11, 2020

msau42 commented Mar 11, 2020

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

jsafrane commented Mar 16, 2020

k8s-ci-robot commented Mar 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Danil-Grigorev commented Mar 11, 2020 •

edited

Loading