Clarify that plugin may return OK for ControllerUnpublish if node or volume not found

[davidz627]

This clarifies the plugin is allowed to return OK if because of a missing node or volume the plugin is sure that the volume is detached from that node.

Context: kubernetes-csi/external-attacher#165

Fixes #373

Discussion at CSI Community Meeting Notes here: https://docs.google.com/document/d/1-oiNg5V_GtS_JBAEViVBhZ3BYVFlbSz70hreyaD7c5Y/edit#heading=h.z24362cngqjs

/assign @saad-ali @jdef @julian-hj @jieyu
/cc @gnufied @jsafrane @msau42

Clarifies the plugin is allowed to return `OK` for `ControllerUnpublishResponse` if the specified node is or volume no longer exist and the plugin is sure that the volume is detached from that node.

[saad-ali]

CC @julian-hj @ddebroy

[davidz627]

@saad-ali @jieyu @julian-hj resolved all comments, PTAL

[davidz627]

@jdef added a release note to the PR description

[saad-ali]

Outside of the comment below (which can be addressed in follow up PR), LGTM.

[saad-ali]

Part of the reason for this change is that the current Kubernetes handling of this is wrong to accommodate that. However, even after this change, as specified,the plugin SHOULD return '0 OK' means SP could return NOT_FOUND and per the error code Recovery Behavior the Caller MUST verify that the '..._id' is correct... before retrying with exponential backoff -- which caller can not do because it has no way to be able to verify if a given node or volume still exist. Therefore, we should either loosen the Recovery Behavior to Caller SHOULD verify..., but really this is something that we will need to fix in CSI 2.0 -- NOT_FOUND should not be allowed as an error code, and the SP MUST return OK if volume or node are gone and effectively detached.

[saad-ali]

Merging

[jdef]

Should we file an issue to track the suggested 2.0 change?

…

On Wed, Aug 14, 2019, 6:48 PM Saad Ali ***@***.***> wrote: ***@***.**** approved this pull request. Outside of the comment below (which can be addressed in follow up PR), LGTM. ------------------------------ In spec.md <#375 (comment)> : > @@ -1292,8 +1293,8 @@ The CO MUST implement the specified error recovery behavior when it encounters t | Condition | gRPC Code | Description | Recovery Behavior | |-----------|-----------|-------------|-------------------| -| Volume does not exist | 5 NOT_FOUND | Indicates that a volume corresponding to the specified `volume_id` does not exist. | Caller MUST verify that the `volume_id` is correct and that the volume is accessible and has not been deleted before retrying with exponential back off. | -| Node does not exist | 5 NOT_FOUND | Indicates that a node corresponding to the specified `node_id` does not exist. | Caller MUST verify that the `node_id` is correct and that the node is available and has not been terminated or deleted before retrying with exponential backoff. | +| Volume does not exist and volume not assumed ControllerUnpublished from node | 5 NOT_FOUND | Indicates that a volume corresponding to the specified `volume_id` does not exist and is not assumed to be ControllerUnpublished from node corresponding to the specified `node_id`. | Caller MUST verify that the `volume_id` is correct and that the volume is accessible and has not been deleted before retrying with exponential back off. | +| Node does not exist and volume not assumed ControllerUnpublished from node | 5 NOT_FOUND | Indicates that a node corresponding to the specified `node_id` does not exist and the volume corresponding to the specified `volume_id` is not assumed to be ControllerUnpublished from node. | Caller MUST verify that the `node_id` is correct and that the node is available and has not been terminated or deleted before retrying with exponential backoff. | Part of the reason for this change is that the current Kubernetes handling of this is wrong to accommodate that. However, even after this change, as specified,the plugin SHOULD return '0 OK' means SP could return NOT_FOUND and per the error code Recovery Behavior the Caller MUST verify that the '..._id' is correct... before retrying with exponential backoff -- which caller can not do because it has no way to be able to verify if a given node or volume still exist. Therefore, we should either loosen the Recovery Behavior to Caller SHOULD verify..., but really this is something that we will need to fix in CSI 2.0 -- NOT_FOUND should not be allowed as an error code, and the SP MUST return OK if volume or node are gone and effectively detached. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#375?email_source=notifications&email_token=AAR5KLDQAWOBIPDXNXX6FFDQESDVPA5CNFSM4IKEQER2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBTRIOA#pullrequestreview-275190840>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAR5KLBWRCAONBFVP34ELVDQESDVPANCNFSM4IKEQERQ> .

[davidz627]

@jdef #382