Update spec doc for GroupController service - #545

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

Also rephrase the last part for grammar.

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.

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.

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.

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.

@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.

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.

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.

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.

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

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.

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

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.