[cinder-csi-plugin] Add finalizer to VolumeSnapshot when create PVC with the VolumeSnapshot #2369

jeffyjf · 2023-09-26T11:48:22Z

What this PR does / why we need it:

This PR implement a PVC watcher. When the watcher receive a PVC ADDED event and PVC with VolumeSnapshot kind dataSource, the VolumeSnapshot will be added a finalizer. And the finalizer will be removed when the watcher receive the PVC's DELETED event. It can used to protect VolumeSnapshot, avoid that the VS be deleted while there are any PersistentVolume on it.

Which issue this PR fixes(if applicable):
fixes #2294

Special notes for reviewers:

Asynchronously add finalizer by watcher seems like unsafety, might result in some potential issues. After the PR [1] merged and released, we need to refactor it and implement a synchronous way to add finalizer.

[1] kubernetes-csi/external-provisioner#1070

Release note:

Add  a command flag `--enable-vs-deletion-protection`. If set, The VS will be added a finalizer while a PVC created base on it, ensure the VS couldn't be deleted before the PVC be deleted.

k8s-ci-robot · 2023-09-26T11:48:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign jichenjc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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 · 2023-09-26T11:48:32Z

Hi @jeffyjf. Thanks for your PR.

I'm waiting for a kubernetes 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.

mdbooth

As mentioned in the issue, I do like the concept of adding a finalizer to the snapshot for each volume which requires it. However, I can't think of another instance where finalizers are used this way, so it might be worth some effort to discover if there are any problems with potentially very big lists of finalizers. Imagine, for example, a some CI system which uses a 'golden image' extremely heavily and has 1,000 volumes using it. Would that be ok?

However, I don't think we can safely add the finalizer asynchronously. A finalizer would typically be added synchronously before resource creation. It's also worth noting that the resource in this case would be the PV rather than the PVC.

Could we add code which adds and persists the snapshot finalizer immediately before creating the PV?

We would also need to add code to remove the finalizer in the deletion flow of the PV strictly before removing the PV finalizer.

mdbooth · 2023-09-27T13:25:47Z

Looks very WIP right now so probably not worth actually running tests, but

/ok-to-test

Note that tests won't run automatically while the PR is marked WIP, but you can run them manually on demand when you're ready with /test all.

pkg/csi/cinder/controllerserver.go

jeffyjf · 2023-10-02T10:04:59Z

Hi @mdbooth, thanks very much for your comments. But I have a bit question.

However, I don't think we can safely add the finalizer asynchronously. A finalizer would typically be added synchronously before resource creation. It's also worth noting that the resource in this case would be the PV rather than the PVC.

I am a newbie. In my opinion, this cannot be implemented just by watcher. We need to add a webhook to do this. right? If it's not right, do you have any other suggestions?

jeffyjf · 2023-10-09T02:18:02Z

As mentioned in the issue, I do like the concept of adding a finalizer to the snapshot for each volume which requires it. However, I can't think of another instance where finalizers are used this way, so it might be worth some effort to discover if there are any problems with potentially very big lists of finalizers. Imagine, for example, a some CI system which uses a 'golden image' extremely heavily and has 1,000 volumes using it. Would that be ok?

However, I don't think we can safely add the finalizer asynchronously. A finalizer would typically be added synchronously before resource creation. It's also worth noting that the resource in this case would be the PV rather than the PVC.

Could we add code which adds and persists the snapshot finalizer immediately before creating the PV?

We would also need to add code to remove the finalizer in the deletion flow of the PV strictly before removing the PV finalizer.

In the past few days, I tried a few methods to implement add finalizer before creating the PV synchronously:

I tried implement this in CreateVolume function, but I found I can't get enough context informations by graceful way. Such as, I can't obtain the k8s VolumeSnapshot's name and namespace, this result in I can't add finalizer to it.
I tried add a webhook to intercept the PV's creation, this can add finalizer before creating the PV. But, this will increase the difficulty of deployment and maintenance. Moreover, If a cluster have multiple webhook about PV and another webhook reject the PV's creation after this webhook, I don't konw how to resolve this situation. So, I don't like this way.

I think the NO.1 method is more graceful and appropriate. But, the first methon require external-provisioner sidecar provide more necessary informations. So I think we should submit a KEP to CSI spec in order to we can get enough necessary informations about VolumeSnatshot in CreateVolume function to resolve this issue appropriately.

jeffyjf · 2023-11-02T12:25:04Z

As mentioned in the issue, I do like the concept of adding a finalizer to the snapshot for each volume which requires it. However, I can't think of another instance where finalizers are used this way, so it might be worth some effort to discover if there are any problems with potentially very big lists of finalizers. Imagine, for example, a some CI system which uses a 'golden image' extremely heavily and has 1,000 volumes using it. Would that be ok?
However, I don't think we can safely add the finalizer asynchronously. A finalizer would typically be added synchronously before resource creation. It's also worth noting that the resource in this case would be the PV rather than the PVC.
Could we add code which adds and persists the snapshot finalizer immediately before creating the PV?
We would also need to add code to remove the finalizer in the deletion flow of the PV strictly before removing the PV finalizer.

In the past few days, I tried a few methods to implement add finalizer before creating the PV synchronously:

I tried implement this in CreateVolume function, but I found I can't get enough context informations by graceful way. Such as, I can't obtain the k8s VolumeSnapshot's name and namespace, this result in I can't add finalizer to it.

I tried add a webhook to intercept the PV's creation, this can add finalizer before creating the PV. But, this will increase the difficulty of deployment and maintenance. Moreover, If a cluster have multiple webhook about PV and another webhook reject the PV's creation after this webhook, I don't konw how to resolve this situation. So, I don't like this way.

I think the NO.1 method is more graceful and appropriate. But, the first methon require external-provisioner sidecar provide more necessary informations. So I think we should submit a KEP to CSI spec in order to we can get enough necessary informations about VolumeSnatshot in CreateVolume function to resolve this issue appropriately.

I've already commit a PR to external-provisioner, in order to we can add finalizer to VolumeSnapshot synchronously in future. But now, let's continue to complete this imperfect solution to fix the issue #2294 and hold back resource leak in production environment asap.

dulek · 2023-11-08T16:51:37Z

Hi @mdbooth, thanks very much for your comments. But I have a bit question.

However, I don't think we can safely add the finalizer asynchronously. A finalizer would typically be added synchronously before resource creation. It's also worth noting that the resource in this case would be the PV rather than the PVC.

I am a newbie. In my opinion, this cannot be implemented just by watcher. We need to add a webhook to do this. right? If it's not right, do you have any other suggestions?

Hey, what is the problem with adding a finalizer asynchronously? We've did that all the time in Kuryr and it worked totally well when we made sure we're doing it by patching the resource properly.

jeffyjf · 2023-11-09T11:52:26Z

/hold cancel

jeffyjf · 2023-11-13T07:59:44Z

Hi CPO Co-Leads @dulek @jichenjc @kayrus @zetaab

This PR is ready for review. Please take a look at this PR in your spare time. I'm eager to get your advices.

jichenjc · 2023-11-14T07:27:13Z

Asynchronously add finalizer by watcher seems like unsafety, might result in some potential issues. After the PR [1] merged and released, we need to refactor it and implement a synchronous way to add finalizer.

thanks for the PR, I thought our PR rely on kubernetes-csi/external-provisioner#1070
but read again seems you are proposing a long term solution in external-provisioner then after that's done
we will do another round of fix to current one ? so this PR is a temp solution as stop gap ?

jeffyjf · 2023-11-14T08:00:07Z

Asynchronously add finalizer by watcher seems like unsafety, might result in some potential issues. After the PR [1] merged and released, we need to refactor it and implement a synchronous way to add finalizer.

thanks for the PR, I thought our PR rely on kubernetes-csi/external-provisioner#1070 but read again seems you are proposing a long term solution in external-provisioner then after that's done we will do another round of fix to current one ? so this PR is a temp solution as stop gap ?

Yep, I am not sure when the kubernetes-csi/external-provisioner#1070 can be merged and released. This PR can be temporarily used to hold back resources leak.

jeffyjf · 2023-11-14T08:13:38Z

Hi @mdbooth, thanks very much for your comments. But I have a bit question.

However, I don't think we can safely add the finalizer asynchronously. A finalizer would typically be added synchronously before resource creation. It's also worth noting that the resource in this case would be the PV rather than the PVC.

I am a newbie. In my opinion, this cannot be implemented just by watcher. We need to add a webhook to do this. right? If it's not right, do you have any other suggestions?

Hey, what is the problem with adding a finalizer asynchronously? We've did that all the time in Kuryr and it worked totally well when we made sure we're doing it by patching the resource properly.

There may just be some potential problems, but I have no concrete example. Maybe in an extremely busy environment, the resource may be deleted before the finalizer be successfully added.

k8s-ci-robot · 2023-12-12T04:03:39Z

PR needs rebase.

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.

kayrus · 2023-12-12T09:54:31Z

@jeffyjf can you rebase on latest master?

jeffyjf · 2023-12-13T00:31:32Z

@jeffyjf can you rebase on latest master?

Of course I will rebase it late on.

k8s-ci-robot · 2023-12-13T01:06:26Z

@jeffyjf: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
openstack-cloud-keystone-authentication-authorization-test	`ee04af8`	link	true	`/test openstack-cloud-keystone-authentication-authorization-test`
openstack-cloud-controller-manager-e2e-test	`ee04af8`	link	true	`/test openstack-cloud-controller-manager-e2e-test`
openstack-cloud-csi-manila-e2e-test	`ee04af8`	link	true	`/test openstack-cloud-csi-manila-e2e-test`
openstack-cloud-csi-manila-sanity-test	`ee04af8`	link	true	`/test openstack-cloud-csi-manila-sanity-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

kayrus · 2023-12-13T08:00:57Z

@jeffyjf can you fix unit tests?

jeffyjf · 2023-12-13T08:13:27Z

@jeffyjf can you fix unit tests?

Of course, I'll fix it asap.

This patch implement a PVC watcher. When the watcher receive a PVC ADDED event and PVC with VolumeSnapshot kind dataSource, the VolumeSnapshot will be added a finalizer. And the finalizer will be removed when the watcher receive the PVC's DELETED event. NOTE: Asynchronously add finalizer by watcher seems like unsafety, might result in some potential issues. After the PR [1] merged and released, we need to refactor it and implement a synchronous way to add finalizer. [1] kubernetes-csi/external-provisioner#1070

k8s-ci-robot · 2024-01-03T18:43:07Z

PR needs rebase.

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.

dulek

This feels like it should be a proper K8s controller and not just a simplified version of a watcher.

dulek · 2024-01-10T18:21:05Z

pkg/csi/cinder/watcher.go

+	snap "github.com/kubernetes-csi/external-snapshotter/client/v6/clientset/versioned"
+	"golang.org/x/net/context"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/watch"
+	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/rest"
+	"k8s.io/klog/v2"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"


Looks like this is mixing K8s and non-K8s imports. No biggie.

dulek · 2024-01-10T18:24:25Z

pkg/csi/cinder/watcher.go

+		if event.Type == watch.Added {
+			klog.V(5).InfoS("Receive ADDED event try to add finalizer to VolumeSnapshot", "PersistentVolumeClaim", pvc.Name, "VolumeSnapshot", snapshotObj.Name)
+			update = controllerutil.AddFinalizer(snapshotObj, finalizer)
+		}


Uhm, what if we missed an ADDED event somehow, but then get the MODIFIED one? I believe we should rather look for ADDED/MODIFIED and check if finalizer is there and if it's not add it.

dulek · 2024-01-10T18:27:04Z

pkg/csi/cinder/watcher.go

+		if event.Type == watch.Deleted {
+			klog.V(5).InfoS("Receive DELETED event try to remove finalizer from VolumeSnapshot", "PersistentVolumeClaim", pvc.Name, "VolumeSnapshot", snapshotObj.Name)
+			update = controllerutil.RemoveFinalizer(snapshotObj, finalizer)
+		}


What if you've missed a deletion event here? Are we stuck with a snapshot?

dulek · 2024-01-10T18:27:57Z

pkg/csi/cinder/watcher.go

+			update = controllerutil.RemoveFinalizer(snapshotObj, finalizer)
+		}
+		if update {
+			_, err := cw.snapClient.SnapshotV1().VolumeSnapshots(pvc.Namespace).Update(context.Background(), snapshotObj, metav1.UpdateOptions{})


This should be a Patch, otherwise we might end up replacing changes made by some other controller.

jichenjc · 2024-01-11T06:36:55Z

cmd/cinder-csi-plugin/main.go

@@ -65,6 +69,7 @@ func main() {

 	cmd.PersistentFlags().StringVar(&cluster, "cluster", "", "The identifier of the cluster that the plugin is running in.")
 	cmd.PersistentFlags().StringVar(&httpEndpoint, "http-endpoint", "", "The TCP network address where the HTTP server for providing metrics for diagnostics, will listen (example: `:8080`). The default is empty string, which means the server is disabled.")
+	cmd.PersistentFlags().BoolVar(&vsDeletionProtectionEnabled, "enable-vs-deletion-protection", false, "If set, The VS will be added a finalizer while a PVC created base on it, ensure the VS couldn't be deleted before the PVC be deleted.")


VS is not widely used I think you mean virtual server ?

jichenjc · 2024-01-11T06:40:29Z

pkg/csi/cinder/watcher.go

+	watcher    watch.Interface
+}
+
+func (cw *controllerWatcher) run() {


I am not sure but this to me is general feature instead of CPO ? e.g every cloud has this problem?
should this be part of snapshot general thing e.g external snapshot controller to handle ?

k8s-triage-robot · 2024-04-10T06:41:03Z

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2024-05-10T07:26:34Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 26, 2023

jeffyjf marked this pull request as draft September 26, 2023 11:48

k8s-ci-robot requested review from dulek and zetaab September 26, 2023 11:48

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Sep 26, 2023

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 26, 2023

mdbooth reviewed Sep 27, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 27, 2023

kayrus reviewed Sep 27, 2023

View reviewed changes

pkg/csi/cinder/controllerserver.go Outdated Show resolved Hide resolved

pkg/csi/cinder/controllerserver.go Outdated Show resolved Hide resolved

jeffyjf changed the title ~~[cinder-csi-plugin] Add finalizer to volumeshot when create PVC with the volumeshot~~ [cinder-csi-plugin] Add finalizer to VolumeSnapshot when create PVC with the VolumeSnapshot Oct 9, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 20, 2023

jeffyjf mentioned this pull request Oct 21, 2023

Add volumeSnapshotRef to VolumeContentSource for CreateVolumeRequest in csi.proto kubernetes/enhancements#4310

Closed

4 tasks

jeffyjf force-pushed the add-finalizer-to-snapshot branch from ee04af8 to 22e3bf6 Compare November 2, 2023 12:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2023

jeffyjf force-pushed the add-finalizer-to-snapshot branch from 22e3bf6 to 2ff7f36 Compare November 2, 2023 12:13

jeffyjf force-pushed the add-finalizer-to-snapshot branch from 2ff7f36 to d49caa6 Compare November 8, 2023 09:38

jeffyjf force-pushed the add-finalizer-to-snapshot branch from d49caa6 to 9c2d979 Compare November 9, 2023 11:36

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 Nov 9, 2023

k8s-ci-robot requested a review from kayrus November 9, 2023 11:55

jeffyjf force-pushed the add-finalizer-to-snapshot branch from 9c2d979 to e1d4650 Compare November 9, 2023 11:57

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023

jeffyjf force-pushed the add-finalizer-to-snapshot branch from e1d4650 to 00d7a31 Compare November 23, 2023 10:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 12, 2023

jeffyjf force-pushed the add-finalizer-to-snapshot branch from 00d7a31 to ebc7baf Compare December 13, 2023 00:56

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 13, 2023

jeffyjf force-pushed the add-finalizer-to-snapshot branch from ebc7baf to 4c412ca Compare December 13, 2023 08:17

jeffyjf force-pushed the add-finalizer-to-snapshot branch from 4c412ca to 616d1b0 Compare December 13, 2023 08:18

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 3, 2024

dulek requested changes Jan 10, 2024

View reviewed changes

jichenjc reviewed Jan 11, 2024

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 10, 2024

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cinder-csi-plugin] Add finalizer to VolumeSnapshot when create PVC with the VolumeSnapshot #2369

[cinder-csi-plugin] Add finalizer to VolumeSnapshot when create PVC with the VolumeSnapshot #2369

jeffyjf commented Sep 26, 2023 •

edited

k8s-ci-robot commented Sep 26, 2023

k8s-ci-robot commented Sep 26, 2023

mdbooth left a comment

mdbooth commented Sep 27, 2023

jeffyjf commented Oct 2, 2023 •

edited

jeffyjf commented Oct 9, 2023 •

edited

jeffyjf commented Nov 2, 2023

dulek commented Nov 8, 2023

jeffyjf commented Nov 9, 2023

jeffyjf commented Nov 13, 2023 •

edited

jichenjc commented Nov 14, 2023

jeffyjf commented Nov 14, 2023

jeffyjf commented Nov 14, 2023

k8s-ci-robot commented Dec 12, 2023

kayrus commented Dec 12, 2023

jeffyjf commented Dec 13, 2023

k8s-ci-robot commented Dec 13, 2023 •

edited

kayrus commented Dec 13, 2023

jeffyjf commented Dec 13, 2023

k8s-ci-robot commented Jan 3, 2024

dulek left a comment

dulek Jan 10, 2024

dulek Jan 10, 2024

dulek Jan 10, 2024

dulek Jan 10, 2024

jichenjc Jan 11, 2024

jichenjc Jan 11, 2024

k8s-triage-robot commented Apr 10, 2024

k8s-triage-robot commented May 10, 2024

[cinder-csi-plugin] Add finalizer to VolumeSnapshot when create PVC with the VolumeSnapshot #2369

Are you sure you want to change the base?

[cinder-csi-plugin] Add finalizer to VolumeSnapshot when create PVC with the VolumeSnapshot #2369

Conversation

jeffyjf commented Sep 26, 2023 • edited

k8s-ci-robot commented Sep 26, 2023

k8s-ci-robot commented Sep 26, 2023

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth commented Sep 27, 2023

jeffyjf commented Oct 2, 2023 • edited

jeffyjf commented Oct 9, 2023 • edited

jeffyjf commented Nov 2, 2023

dulek commented Nov 8, 2023

jeffyjf commented Nov 9, 2023

jeffyjf commented Nov 13, 2023 • edited

jichenjc commented Nov 14, 2023

jeffyjf commented Nov 14, 2023

jeffyjf commented Nov 14, 2023

k8s-ci-robot commented Dec 12, 2023

kayrus commented Dec 12, 2023

jeffyjf commented Dec 13, 2023

k8s-ci-robot commented Dec 13, 2023 • edited

kayrus commented Dec 13, 2023

jeffyjf commented Dec 13, 2023

k8s-ci-robot commented Jan 3, 2024

dulek left a comment

Choose a reason for hiding this comment

dulek Jan 10, 2024

Choose a reason for hiding this comment

dulek Jan 10, 2024

Choose a reason for hiding this comment

dulek Jan 10, 2024

Choose a reason for hiding this comment

dulek Jan 10, 2024

Choose a reason for hiding this comment

jichenjc Jan 11, 2024

Choose a reason for hiding this comment

jichenjc Jan 11, 2024

Choose a reason for hiding this comment

k8s-triage-robot commented Apr 10, 2024

k8s-triage-robot commented May 10, 2024

jeffyjf commented Sep 26, 2023 •

edited

jeffyjf commented Oct 2, 2023 •

edited

jeffyjf commented Oct 9, 2023 •

edited

jeffyjf commented Nov 13, 2023 •

edited

k8s-ci-robot commented Dec 13, 2023 •

edited