Define DeleteVolume behavior with snapshots #347

Akrog · 2019-01-25T12:25:56Z

Spec is not clear on what should happen when on a DeleteVolume call when
the volume has snapshots.

This patch clarifies the situation by explicitly mentioning that the
operation should complete and the snapshots should still be operational.

Closes: #346

jdef · 2019-01-25T16:49:10Z

spec.md

@@ -1131,6 +1131,8 @@ This RPC will be called by the CO to deprovision a volume.
 This operation MUST be idempotent.
 If a volume corresponding to the specified `volume_id` does not exist or the artifacts associated with the volume do not exist anymore, the Plugin MUST reply `0 OK`.

+CSI plugins MUST treat volumes independent from their snapshots.  Controller Plugin MUST support deleting a volume without affecting its existing snapshots, which MUST still be fully operational and acceptable as sources for new volumes as well as appear on `ListSnapshot` calls.


I think this could be too onerous for some plugins to support, especially if they're not maintaining their own state store independent of any internal store provided by the "real" backend.

I'd rather let the plugin decide how to do this:

Fail the volume-delete RPC because it's not allowed there are snapshots.

Honor the volume-delete RPC and maintain the existing snapshots (because they're still usable).

Honor the volume-delete RPC and remove all linked snapshots.

If there's an ongoing snapshot-related operation in progress (e.g. creating a snapshot from said volume, or creating a volume from a snapshot that's linked to the volume-to-be-removed) then we have error codes a plugin can use to indicate a conflict with an ongoing operation (ABORTED).

If (1) is the case then there's already a documented error code for delete's that fail because a resource is in-use (FAILED_PRECONDITION).

If the plugin is able to execute (3) without errors, then it's up to the CO to periodically reconcile its internal state w/ that of the plugin in order to account for snapshots that may have disappeared.

Since a plugin's capabilities w/ respect to (2) or (3) aren't discoverable via some CSI capability, a CO w/ CSI snapshot support should probably be periodically scanning/reconciling snapshots anyway.

If we want to support the 3 options, which sounds great as we'd be more inclusive with all types of backends and plugins, I think we should add some kind of capability reporting to specify each plugin behavior.

Without knowing expected behavior it'll get messy for COs and users, since each plugin would behave differently and the CO wouldn't know what to expect, and users would have to go to the plugin's documentation to know if deleting a volume with snapshots is supported or not.

I agree that we should let the SP decide but this is a good suggestion. So how about /s/MUST/SHOULD/ in this change to make it a recommendation rather then a requirement?

I'm updating the patch to change it to SHOULD and also make it contemplate @jdef 's first scenario. This way plugins can either support deleting volumes independently or fail. We can leave support for cascade deletion of snapshots as a new feature and leave this patch to just clarify the current expected behavior.

saad-ali · 2019-02-26T22:01:04Z

spec.md

@@ -1131,6 +1131,8 @@ This RPC will be called by the CO to deprovision a volume.
 This operation MUST be idempotent.
 If a volume corresponding to the specified `volume_id` does not exist or the artifacts associated with the volume do not exist anymore, the Plugin MUST reply `0 OK`.

+CSI plugins MUST treat volumes independent from their snapshots.  Controller Plugin MUST support deleting a volume without affecting its existing snapshots, which MUST still be fully operational and acceptable as sources for new volumes as well as appear on `ListSnapshot` calls.


I agree that we should let the SP decide but this is a good suggestion. So how about /s/MUST/SHOULD/ in this change to make it a recommendation rather then a requirement?

jdef · 2019-03-02T02:43:49Z

spec.md

@@ -1131,6 +1131,8 @@ This RPC will be called by the CO to deprovision a volume.
 This operation MUST be idempotent.
 If a volume corresponding to the specified `volume_id` does not exist or the artifacts associated with the volume do not exist anymore, the Plugin MUST reply `0 OK`.

+CSI plugins SHOULD treat volumes independent from their snapshots.  If the Controller Plugin supports deleting a volume without affecting its existing snapshots, then these snapshots MUST still be fully operational and acceptable as sources for new volumes as well as appear on `ListSnapshot` calls.


Thanks for the revised language, though I recommend s/MUST/SHOULD/ in the 2nd sentence. We're saying this behavior is optional, but recommended - so the use of MUST only makes it more confusing: is this required or optional behavior?

nit: one sentence per line, please.

I forgot about the one line per sentence, sorry.

The reason for using MUST in the second sentence is that it is a conditional sentence, and I believe it to be correct.

If a driver does support deleting a volume independently of the snapshots then it MUST (not should, this is a hard requirement) allow using those snapshots normally.

If I change the MUST to SHOULD, the specs end up saying that it's OK to have a driver that allows deleting a volume with snapshots and leave its snapshots in a state that they cannot be normally used. Which is not what we want to say.

I will add another sentence explaining the case of the controller not supporting deleting volumes independently of their snapshots to make it explicit that both options are possible, and that's why we had the SHOULD in the first sentence.

Thanks for the review.

Spec is not clear on what should happen when on a DeleteVolume call when the volume has snapshots. This patch clarifies the situation by explicitly mentioning that the operation should complete and the snapshots should still be operational. Closes: container-storage-interface#346

jdef · 2019-03-07T16:27:30Z

Thanks for the clarifying language. I wonder if this should be considered a breaking change?

This could be construed as more restrictive behavior vs. what was called out in the original specification.

Akrog · 2019-03-07T16:49:34Z

I think it's not really a breaking change, it's mostly a clarification with a preferred behavior.

Existing v1.0 plugins will have implemented the delete operation in one of those two ways, and it doesn't matter which one they have, because they will conform to the spec regardless.

julian-hj

LGTM

jdef · 2019-03-07T19:57:26Z

Existing v1.0 plugins will have implemented the delete operation in one of those two ways, and it doesn't matter which one they have, because they will conform to the spec regardless.

ok, sounds good

saad-ali · 2019-03-14T21:00:53Z

/lgtm
/approve

zhucan · 2019-04-25T06:26:26Z

maybe, whether volumes independent from their snapshots， we can set a flag to volumesnapshotclass？

zhucan · 2019-04-25T06:28:49Z

when deleting volumes , we can judge wether volumes independent from their snapshots from the flag?

Akrog · 2019-04-25T10:48:18Z

maybe, whether volumes independent from their snapshots， we can set a flag to volumesnapshotclass？
when deleting volumes , we can judge wether volumes independent from their snapshots from the flag?

From CSI's perspective there's no such thing as the VolumeSnapshotClass. That is a CO (k8s) specific detail.

From the CSI perspective what we discussed was that a CSI plugins could report whether the plugin supports this capability or not. But decided to let drivers decide and be the CO's decision.

It should be possible to use a flag in the VolumeSnapshotClass to tell k8s whether a volume with snapshots can be deleted or not, but adding this feature should be brought up with the Kubernetes community.

zhucan · 2019-04-25T11:00:41Z

But our storage support volumes independent from snapshots and volumes not independent from snapshots发自我的iPhone------------------ Original ------------------From: Gorka Eguileor <notifications@github.com>Date: Thu,Apr 25,2019 6:48 PMTo: container-storage-interface/spec <spec@noreply.github.com>Cc: zhucan <zhucan.k8s@gmail.com>, Comment <comment@noreply.github.com>Subject: Re: [container-storage-interface/spec] Define DeleteVolume behaviorwith snapshots (#347) maybe, whether volumes independent from their snapshots， we can set a flag to volumesnapshotclass？ when deleting volumes , we can judge wether volumes independent from their snapshots from the flag? From CSI's perspective there's no such thing as the VolumeSnapshotClass. That is a CO (k8s) specific detail. From the CSI perspective what we discussed was that a CSI plugins could report whether the plugin supports this capability or not. But decided to let drivers decide and be the CO's decision. It should be possible to use a flag in the VolumeSnapshotClass to tell k8s whether a volume with snapshots can be deleted or not, but adding this feature should be brought up with the Kubernetes community. —You are receiving this because you commented.Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/container-storage-interface/spec","title":"container-storage-interface/spec","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/container-storage-interface/spec"}},"updates":{"snippets":[{"icon":"PERSON","message":"@Akrog in #347: \u003e maybe, whether volumes independent from their snapshots， we can set a flag to volumesnapshotclass？\r\n\u003e when deleting volumes , we can judge wether volumes independent from their snapshots from the flag?\r\n\r\nFrom CSI's perspective there's no such thing as the VolumeSnapshotClass. That is a CO (k8s) specific detail.\r\n\r\nFrom the CSI perspective what we discussed was that a CSI plugins could report whether the plugin supports this capability or not. But decided to let drivers decide and be the CO's decision.\r\n\r\nIt should be possible to use a flag in the VolumeSnapshotClass to tell k8s whether a volume with snapshots can be deleted or not, but adding this feature should be brought up with the Kubernetes community."}],"action":{"name":"View Pull Request","url":"#347 (comment)"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "#347 (comment)", "url": "#347 (comment)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

Akrog · 2019-04-25T11:08:04Z

Then your storage must succeed for independent volumes and return FAILED_PRECONDITION error for the non-independent ones.

zhucan · 2019-04-25T11:10:03Z

Ok, get it发自我的iPhone------------------ Original ------------------From: Gorka Eguileor <notifications@github.com>Date: Thu,Apr 25,2019 7:08 PMTo: container-storage-interface/spec <spec@noreply.github.com>Cc: zhucan <zhucan.k8s@gmail.com>, Comment <comment@noreply.github.com>Subject: Re: [container-storage-interface/spec] Define DeleteVolume behaviorwith snapshots (#347)Then your storage must succeed for independent volumes and return FAILED_PRECONDITION error for the non-independent ones. —You are receiving this because you commented.Reply to this email directly, view it on GitHub, or mute the thread. {"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/container-storage-interface/spec","title":"container-storage-interface/spec","subtitle":"GitHub repository","main_image_url":"https://github.githubassets.com/images/email/message_cards/header.png","avatar_image_url":"https://github.githubassets.com/images/email/message_cards/avatar.png","action":{"name":"Open in GitHub","url":"https://github.com/container-storage-interface/spec"}},"updates":{"snippets":[{"icon":"PERSON","message":"@Akrog in #347: Then your storage must succeed for independent volumes and return `FAILED_PRECONDITION` error for the non-independent ones."}],"action":{"name":"View Pull Request","url":"#347 (comment)"}}} [ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "#347 (comment)", "url": "#347 (comment)", "name": "View Pull Request" }, "description": "View this Pull Request on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

jdef reviewed Jan 25, 2019

View reviewed changes

saad-ali requested changes Feb 26, 2019

View reviewed changes

Akrog force-pushed the clarify-delete-volume branch from 86e779e to b291631 Compare February 27, 2019 12:58

jdef requested changes Mar 2, 2019

View reviewed changes

Akrog force-pushed the clarify-delete-volume branch from b291631 to a249266 Compare March 4, 2019 10:15

jdef approved these changes Mar 7, 2019

View reviewed changes

jieyu approved these changes Mar 7, 2019

View reviewed changes

julian-hj approved these changes Mar 7, 2019

View reviewed changes

saad-ali approved these changes Mar 14, 2019

View reviewed changes

saad-ali merged commit c3707a6 into container-storage-interface:master Mar 14, 2019

saad-ali mentioned this pull request Mar 14, 2019

VolumeDelete may fail if Snapshots for volume not deleted first kubernetes-csi/external-provisioner#252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define DeleteVolume behavior with snapshots #347

Define DeleteVolume behavior with snapshots #347

Akrog commented Jan 25, 2019

jdef Jan 25, 2019

jdef Jan 25, 2019

Akrog Jan 28, 2019

saad-ali Feb 26, 2019

Akrog Feb 27, 2019

saad-ali Feb 26, 2019

jdef Mar 2, 2019

Akrog Mar 4, 2019

jdef commented Mar 7, 2019

Akrog commented Mar 7, 2019

julian-hj left a comment

jdef commented Mar 7, 2019

saad-ali commented Mar 14, 2019

zhucan commented Apr 25, 2019 •

edited

Loading

zhucan commented Apr 25, 2019

Akrog commented Apr 25, 2019

zhucan commented Apr 25, 2019 via email

Akrog commented Apr 25, 2019

zhucan commented Apr 25, 2019 via email

Define DeleteVolume behavior with snapshots #347

Define DeleteVolume behavior with snapshots #347

Conversation

Akrog commented Jan 25, 2019

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

jdef commented Mar 7, 2019

Akrog commented Mar 7, 2019

julian-hj left a comment

Choose a reason for hiding this comment

jdef commented Mar 7, 2019

saad-ali commented Mar 14, 2019

zhucan commented Apr 25, 2019 • edited Loading

zhucan commented Apr 25, 2019

Akrog commented Apr 25, 2019

zhucan commented Apr 25, 2019 via email

Akrog commented Apr 25, 2019

zhucan commented Apr 25, 2019 via email

zhucan commented Apr 25, 2019 •

edited

Loading