Clarify idempotent for ControllerExpandVolume and NodeExpandVolume

[wnxn]

Add explicit documentation for clarity idempotent for
ControllerExpandVolume and NoeExpandVolume

Fixes #396

Signed-off-by: Xin Wang wileywang@yunify.com

[wnxn]

/assign @jdef
PTAL

[jdef]

Please accept the suggested change, and also include a change for NodeExpandVolume that indicates that call is also idempotent (similar to the proposed ControllerExpandVolume change).

[jdef]

Thanks for the PR. If you haven't already signed the CLA for CSI, please do so.

[wnxn]

Thanks for the PR. If you haven't already signed the CLA for CSI, please do so.

Tomorrow, I will ask my colleague about how to sign the CLA for CSI. Please wait a moment.

[jdef]

Thanks. Left last suggestions to fix nits (one sentence per line in .md).

[wnxn]

Thanks for the PR. If you haven't already signed the CLA for CSI, please do so.

Tomorrow, I will ask my colleague about how to sign the CLA for CSI. Please wait a moment.

Hi @jdef, I signed the CLA for CSI and sent email to container-storage-interface-approvers@googlegroups.com.
Please review this PR again. Thanks.

[jdef]

Thanks for the feedback @gnufied. I made some additional suggestions to clarify. PTAL

[wnxn]

Thanks for the feedback @gnufied. I made some additional suggestions to clarify. PTAL

Thanks for @jdef and @gnufied , this PR has been updated.

[gnufied]

lgtm

[jdef]

LGTM

[jdef]

@saad-ali @jieyu @julian-hj @ddebroy PTAL

[julian-hj]

I don't understand the differing usage of MUST and SHOULD here.

If the service returns an error when the volume is already the correct size, wouldn't that make it not idempotent? I guess I am missing the use case where the service ignores the SHOULD but is still idempotent and passes the MUST.

[jdef]

If the service returns an error when the volume is already the correct size, wouldn't that make it not idempotent?

No ... idempotent has to do w/ the resulting state of the system vs. the error codes returned. Ideally, all plugins would return 0 if the target size has already been achieved.
If a plugin doesn't implement this behavior and returns a non-zero error but otherwise leaves the volume untouched then it's still idempotent, it's just returning a non-recommended RPC status (and so may be sacrificing CO compatibility).

[wnxn]

If the service returns an error when the volume is already the correct size, wouldn't that make it not idempotent?

No ... idempotent has to do w/ the resulting state of the system vs. the error codes returned. Ideally, all plugins would return 0 if the target size has already been achieved.
If a plugin doesn't implement this behavior and returns a non-zero error but otherwise leaves the volume untouched then it's still idempotent, it's just returning a non-recommended RPC status (and so may be sacrificing CO compatibility).

What is the definition of idempotent in our CSI spec?
(1) For the same input, the operation can get the same output.
(2) Operations that have no side-effects if executed multiple times.

In my opinion,
For (1), the first keyword, MUST or SHOULD, should be the same as second keyword.

This operation MUST be idempotent. If a volume corresponding to the specified volume ID is larger than or equal to the capacity requirements after expansion, the plugin MUST reply 0 OK.

This operation SHOULD be idempotent. If a volume corresponding to the specified volume ID is larger than or equal to the capacity requirements after expansion, the plugin SHOULD reply 0 OK.

For (2), for the purpose of backwards compatibility, we can clarify the idempotent same as now.

This operation MUST be idempotent. If a volume corresponding to the specified volume ID is larger than or equal to the capacity requirements after expansion, the plugin SHOULD reply 0 OK.

[julian-hj]

What is the definition of idempotent in our CSI spec?
(1) For the same input, the operation can get the same output.
(2) Operations that have no side-effects if executed multiple times.

Elsewhere in the spec, we've normally observed the first definition. For example: https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolume

Having said that, it seems like the rest of the world's definition of idempotent is converging on what @jdef said. So I'd be fine with that, except that it's a bit inconsistent with other RPCs.

Is there any reason that we need to use SHOULD here? If not, maybe we could just switch it to MUST/MUST in order to keep things consistent any tidy.

[jdef]

SHOULD is for backwards compatibility for plugins that might not return 0 OK

…

On Thu, Nov 7, 2019, 5:38 PM Julian Hjortshoj ***@***.***> wrote: What is the definition of idempotent in our CSI spec? (1) For the same input, the operation can get the same output. (2) Operations that have no side-effects if executed multiple times. Elsewhere in the spec, we've normally observed the first definition. For example: https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolume Having said that, it seems like the rest of the world's definition of idempotent is converging on what @jdef <https://github.com/jdef> said. So I'd be fine with that, except that it's a bit inconsistent with other RPCs. Is there any reason that we need to use SHOULD here? If not, maybe we could just switch it to MUST/MUST in order to keep things consistent any tidy. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#397?email_source=notifications&email_token=AAR5KLG5Q7UZE5WW6Z2CYJLQSSKEVA5CNFSM4JIUZASKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDOCMUA#issuecomment-551298640>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLCEPLSQ4Q5XJEQY62TQSSKEVANCNFSM4JIUZASA> .

[julian-hj]

SHOULD is for backwards compatibility for plugins that might not return 0 OK

Fair enough.

[jdef]

Is it worth filing a follow up issue for csi v2 that fixes the wording to be more consistent?

…

On Fri, Nov 8, 2019, 11:56 AM Julian Hjortshoj ***@***.***> wrote: ***@***.**** approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#397?email_source=notifications&email_token=AAR5KLGFGHPIKKPS43XOGJLQSWK5NA5CNFSM4JIUZASKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCK6FF7I#pullrequestreview-314331901>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAR5KLC4TCHPGCDMGARC3D3QSWK5NANCNFSM4JIUZASA> .

[gnufied]

I think so yeah. Do we have a label for csiv2? I can write a github issue.