spec: Use ABORTED instead of UNIMPLEMENTED

[jieyu]

For the case where the CO is not supposed to call some RPCs, the plugin
should return ABORTED to indicate that the CO may want to retry at a
higher level (e.g., by calling ControllerGetCapabilities first).

xref #115

[cpuguy83]

Unimplemented looks like the correct code to me:

Unimplemented indicates operation is not implemented or not supported/enabled in this service.

Aborted indicates the operation was aborted, typically due to a concurrency issue like sequencer check failures, transaction aborts, etc.

Aborted would indicate that given a change in state or the request the call could succeed.

[jdef]

@cpuguy83 it's more a matter of recovery semantics.

Unimplemented (as per grpc) has no recovery mechanism. Consider that a CO attempts to invoke a gRPC service that some plugin doesn't register/bind to it's server mechanism. The "unimplemented" code returned by the gRPC indicates that the requested service or RPC is nowhere to be found, it's completely unregistered, and there's no caller recovery for this.

With Aborted there's specific recovery logic in the case of CSI for RPCs that aren't supposed to be invoked by the CO: the CO should invoke a xxxGetCapabilities RPC to determine how to properly operate the plugin in question. This qualifies as "re-trying at a higher level" of the protocol - the CO MAY restart its process of discovering what the plugin is capable of.

[jdef]

LGTM

[saad-ali]

With Aborted there's specific recovery logic in the case of CSI for RPCs that aren't supposed to be invoked by the CO: the CO should invoke a xxxGetCapabilities RPC to determine how to properly operate the plugin in question. This qualifies as "re-trying at a higher level" of the protocol - the CO MAY restart its process of discovering what the plugin is capable of.

I can buy that argument.

LGTM

[jieyu]

@cpuguy83 @julian-hj, do you have any further thoughts on this?

[cpuguy83]

I don't agree with the case.
It is an error for the CO to call this endpoint when not specified in the capabilities. The CO should never try to call this function again because it shouldn't have to begin with.

[julian-hj]

I agree with @cpuguy83. UNIMPLEMENTED seems like the correct return code to me. Calling a bunch of different RPCs instead of this one does not seem to me like a recovery, and as Brian points out--the only correct recovery is to go back and fix your CO code.

[akutz]

It should be unimplemented if the SP doesn’t implement something. Aborted should be used when RPCs in a known workflow are invoked out of order. For example, if NodePublishVolume is called before ControllerPublishVolume then the former could return aborted to indicate the device is not yet available on the node host and to attempt ControllerPublishVolume.

[jdef]

Does anyone see value in avoiding overlap between error codes generated by the grpc libs and the application-level error codes listed in the CSI spec?

…

On Mon, Nov 20, 2017 at 1:24 PM, Schley Andrew Kutz < ***@***.***> wrote: It should be unimplemented if the SP doesn’t implement something. Aborted should be used when RPCs in a known workflow are invoked out of order. For example, if NodePublishVolume is called before ControllerPublishVolume then the former could return aborted to indicate the device is not yet available on the node host and to attempt ControllerPublishVolume. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#151 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLO89kWKp0gE1Qbnmwq-0G8D5t17qks5s4cPJgaJpZM4QimA_> .

[akutz]

Absolutely, and I thought that was the purpose of the gRPC error model change. I wasn’t arguing to use an app specific code. I thought we were debating when it makes sense to use unimplemented vs aborted.

[julian-hj]

@jdef : in this case, my feeling is that there isn't a great deal of benefit in making that distinction. Basically, if the CO is coded correctly, and the plugin is operating as advertised, we should never see UNIMPLEMENTED. So, if we see it, that means that we need to go digging in the logs to figure out whether SP or the CO is to blame. That would be true regardless of whether the error is generated by the SP or the gRPC layer.

[akutz]

I agree with Julian. Take GetNodeID for example - while it’s optional, that state is tied to the ControllerCapability PublishUnpublish.

IMO there are too many concerns regarding examining capabilities to ascertain the workflows supported by an SP. If there should only be one or two supported models then let’s abandon capabilities and replace it with something simpler. Otherwise let’s acknowledge that a COs responsibility is to query the SP’s capabilities to figure out the names and order of RPCs to invoke. If an SP returns UNIMPLEMETED at that point then the SP is invalid anyway.

[jdef]

this should be rebased and take into account #152