Update spec doc for GroupController service

[carlory]

What type of PR is this?

/kind document

Special notes for your reviewer:

Does this PR introduce an API-breaking change?:

none

[carlory]

/cc @xing-yang @jdef @saad-ali @bswartz

[carlory]

A new change is that the RPC Interactions section is added.

cc @jdef @xing-yang

[carlory]

/cc @xing-yang

[carlory]

@xing-yang updated. please review again.

[bswartz]

It's unclear what a "server-side gRPC error" is. Please be specific about which error indicate that no cleanup is necessary.

[bswartz]

Also rephrase the last part for grammar.

[carlory]

If the implementation of the SP's group snapshot function is flawed, for example, if two data volumes are snapshotted at the same time, one will succeed and the other will always fail, then no matter how many times CO retries the call, it will not get the expected results. It maybe lead to a deadlock in CO. But in the spec, I can't find a suitable status code to tell CO not to retry.

[carlory]

If we can assume that the SP implementation can handle the above situation, then the following description is not needed.

If the `CreateVolumeGroupSnapshot` RPC returns a server-side gRPC error, it means that SP do clean up and make sure no snapshots are leaked.

[bswartz]

If the implementation of the SP's group snapshot function is flawed, for example, if two data volumes are snapshotted at the same time, one will succeed and the other will always fail, then no matter how many times CO retries the call, it will not get the expected results. It maybe lead to a deadlock in CO. But in the spec, I can't find a suitable status code to tell CO not to retry.

This is correct, and because there's nothing the CO can do to address cases like this, we have to put the burden on the SP to detect and fix this situation. I would propose that of the SP fails to snapshot any member of the group and there isn't hope that a retry will succeed, then the SP must clean up any other snapshots related to that group snapshot and return the terminal GRPC error.

The only other solution I can see is to return a "successful" snapshot but mark it as broken or partial with some explicit boolean value in the response message. This would allow the CO to stop retying and then decide whether to keep the broken group snapshot or clean it up the normal way.

[xing-yang]

How do we mark a snapshot as "broken" or "partial"? Currently we don't have a way to do that and communicate back to CO.

[carlory]

@bswartz @xing-yang

I drop this sentence If the CreateVolumeGroupSnapshot RPC returns a server-side gRPC error, it means that SP do clean up and make sure no snapshots are leaked. and open an issue #554 to track it.

[bswartz]

This is bad guidance, and we can't recommend this in the spec. We need to be clear about what the CO must do and what is optional, and for optional things, how the SP can ensure correct behavior under either option.

[carlory]

There is a similar expression in the spec.

The following content is excerpted from PRC interactions of the controller service.

2. The CO takes no further action regarding the timed out RPC, a snapshot is possibly leaked 
and the operator/user is expected to clean up.

[bswartz]

Thanks for pointing this out. We should probably clean up the language for volume snapshots too. It should be fine to duplicate the existing language from volume snapshots to group snapshots. I just want to make sure the language we're copying it up to date and accurate.

[xing-yang]

Agreed that this sentence regarding volume snapshot should be updated. SP should clean up the snapshot in this case.

[xing-yang]

How do we mark a snapshot as "broken" or "partial"? Currently we don't have a way to do that and communicate back to CO.

[xing-yang]

Agreed that this sentence regarding volume snapshot should be updated. SP should clean up the snapshot in this case.

[bswartz]

How do we mark a snapshot as "broken" or "partial"? Currently we don't have a way to do that and communicate back to CO.

I don't necessarily recommend this, but we could add an additional boolean field in the response message to indicate it.

It's a unique problem for group snapshots because if you have multiple volumes and most of the snapshots were okay, but one cannot succeed, there are good arguments both to preserve the partial group snapshot and arguments to delete it and try over again. My personal inclination would to be to delete it -- in which case the SP could do that work with no RPC changes. But if we return success with a "partial" boolean set then the CO could also delete it, while also having the option to not delete it.

[saad-ali]

Confirmed @carlory signed CSI CLA

[saad-ali]

How do we mark a snapshot as "broken" or "partial"? Currently we don't have a way to do that and communicate back to CO.

I don't necessarily recommend this, but we could add an additional boolean field in the response message to indicate it.

It's a unique problem for group snapshots because if you have multiple volumes and most of the snapshots were okay, but one cannot succeed, there are good arguments both to preserve the partial group snapshot and arguments to delete it and try over again. My personal inclination would to be to delete it -- in which case the SP could do that work with no RPC changes. But if we return success with a "partial" boolean set then the CO could also delete it, while also having the option to not delete it.

Per @xing-yang should fail and clean up if even one snapshot in group fails because they have to be consistent.
Per @bswartz if SP gets 9/10 it is responsible for error handling.

@xing-yang will take another look at this PR.

[saad-ali]

Per @bswartz and @xing-yang - It is not possible to tell k8s partial completion. And if partial completion, it is SP responsibility to clean up. It's all or nothing, if it's not all, SP must clean up.