already_exists and already_published for CreateVolume and xxxPublishVolume RPCs

[jdef]

• Fixes Idempotent functions cannot return both data & errors #157
• Please "rebase and merge" this instead of squashing.

Rationale for using google.protobuf.BoolValue:

• default value for bool is false
• if we used a bool type for this flag then plugins that don't set already_exists at all will end up sending a default false value, even if the volume already existed. this isn't a huge deal from a CO-behavior perspective but it kills the debugging story: what use is a debugging field if you can't rely on it to tell you something meaningful?
• the default value for BoolValue is unset, because it's a wrapper type. this paves the way for a nice debugging story, wherein plugins that don't set already_exists in a response will not being sending a misleading false value; plugins that explicitly set already_exists in a response will be sending the value that they intend a CO/operator to observe.

[jieyu]

I think it should either be a required bool or an optional BoolValue. I am leaning toward making it a required field to force the Plugin to set this field properly, and don't give them the option to not set it.

This is already required by the current spec to return ALREADY_EXISTS if a volume already exists. I don't see a reason why we want to make that optional.

[jdef]

I wonder if there are cases where it'll be awkward for a plugin to understand whether a volume already exists (which will be needed if already_exists is a REQUIRED bool). Consider a plugin that interfaces with a cloudy backend that similarly presents an idempotent interface for creating a volume, only that backend interface doesn't return the equivalent of "already exists" -- in order to comply with a REQUIRED bool already_exists the plugin may need to exec additional queries to the backend prior to executing the create op (and even in this type of workaround there are potential races).

• Skimming the GCE API, it's unclear if the GCE Disks (insert) API behaves this way (https://cloud.google.com/compute/docs/reference/latest/disks/insert). It's possible that this API is idempotent and simply returns status=DONE if the requested disk already exists w/ the requested params (I haven't tested this, just speculating).

• AWS supports idempotent requests for only some of their APIs (I don't see EBS vols listed): http://docs.aws.amazon.com/AWSEC2/latest/APIReference/Run_Instance_Idempotency.html#idempotency-support.

• docker volume create ... is apparently idempotent (or was at one point): Docker volume create should not be idempotent (by default) moby/moby#16068

• DigitalOcean has idempotent "delete" and "update" operations, but not create? https://developers.digitalocean.com/documentation/v2/#requests

• Azure: several blog posts talk about idempotent resource creation, but I don't see it documented in the API: https://docs.microsoft.com/en-us/rest/api/compute/manageddisks/disks/disks-create-or-update. It's possible that this is idempotent and simply returns Accepted if the disk (as specified) already exists.

If one of our overarching goals is to simplify things for plugin writers, then I'd argue that an OPTIONAL .google.protobuf.BoolValue already_exists probably enables simpler plugin code, at the risk of some plugins being lazy and not implementing support for it when their backend API actually DOES provide a convenient mechanism with which to make a distinction.

[jieyu]

LGTM. I am convinced that using BoolValue is better. Thanks @jdef !

[jdef]

ping @julian-hj @cpuguy83

[cpuguy83]

I'm confused, why are we using this instead of a status code?

[jdef]

Was discussed and decided upon at the last community sync. Basically we didn't do enough e2e testing up front and passing response+error back from the server through the golang/grpc client layer doesn't work like we thought it should. Of the options discussed, passing back an additional boolean was the option that we (mostly) agreed on. https://docs.google.com/document/d/1-oiNg5V_GtS_JBAEViVBhZ3BYVFlbSz70hreyaD7c5Y/edit#heading=h.xzorxa44tuy0

…

On Mon, Dec 4, 2017 at 9:33 AM, Brian Goff ***@***.***> wrote: I'm confused, why are we using this instead of a status code? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#158 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLDyHrjbBZzufijnWkUoypKPE8WhPks5s9ALQgaJpZM4QvsJc> .

[cpuguy83]

This seems to muddy the error handling quite a lot.

To get around the various issues with intermediate layers, could we specify that an ALREADY_EXISTS response needs to have encoded in the details the volume ID, and then have a GetVolumeInfo rpc to subsequently have it return the other missing data? I seem to recall we used to have a similar rpc... likely removed when we got rid of the "VolumeHandle"?

[julian-hj]

👍

[akutz]

Hi @cpuguy83,

I agree with you completely. Julian was adamant that the error should not contain any information other than the error due to concerns around logging of sensitive details (despite the fact that no RPC current returns sensitive information as part of the official model).

I was frankly tired of arguing for the idiomatic approach to this issue and just consented to the compromise.

I do not care for it, and this is the reason gRPC errors can marshal protobuf objects into the error details, but I was the only person on the call defending the idiomatic approach.

[cpuguy83]

In this case, volume ID's should not be sensitive and should be expected to log.

So just to be clear, I'm 👎 on this change. It's making some really nasty compromises on the interface before we even do the first pre-release.

[akutz]

Hi @cpuguy83,

I believe that Julian was referring to the possibility that a CreateVolumeResponse might one day include credentials and/or VolumeAttributes may or may not include sensitive information.

However, neither case is specified in the spec today and as such I agree that the current approach is idiosyncratic with respect to how gRPC intends errors and error details to be returned.

[jieyu]

@cpuguy83, can you sync with @julian-hj on this? Honestly, I am fine with either approach, but I don't want this to block the release.

[julian-hj]

I guess I am stuck on the notion that returning an error as a second response to a supposedly idempotent function is idiomatic. I find that hard to believe. Can you guys point to examples of functions that claim to be idempotent, and return data payloads in the error details?

[jieyu]

@akutz @cpuguy83 will you guys be ok if CreateVolume returns an OK if the volume already exists? Sounds like the tradeoff of this approach is debugging. Maybe this is ok given all the issues discussed? cc @saad-ali @jdef @julian-hj

[gpaul]

👎 on adding the field. It is optional and therefore cannot be relied upon by the CO. We lose referential transparency (a precondition for idempotency) making the API awkward to satisfy and consume. The benefit does not outweigh the cost.

I like returning OK iff. all request parameters are met by the existing volume and would like to keep ALREADY_EXISTS as a failure for the case where the named volume exists but does not satisfy the request.

[gpaul]

Let's err on the side of leaving out fields that can always be added for v0.2. Ripping them out for v0.2 is much harder.

[cpuguy83]

I think it's fair to have the SP return an OK response when it already exists assuming the SP has determined that it is the same volume and not just some other volume with the same name.

If we want to aid debugging, we could come up with a proto to deal with this to embed in the status details (even for OK responses... I think this should be fine...), but I'm not sure this should be a requirement for v0.1.

[jieyu]

@akutz @julian-hj @saad-ali, ping? will you be OK if the SP return an OK response when the volume already exists?

[akutz]

Hi @jieyu,

Not really. This is the point of the gRPC error details. :(

I can live with returning a successful result IFF we also declare that an SP may enable debug mode and return ALREADY_EXISTS or NOT_FOUND for CreateVolume and DeleteVolume, indicating to the CO that the object (in the case of the CreateVolume) already exists and is included in the error details.

In other words, returning OK is the default mode, but COs should also still be aware that an SP MAY return an idempotent error with an embedded object.

[julian-hj]

I would argue that “idempotent error” is an oxymoron. If calling the method a second time with the same arguments is an error, then the method is not idempotent.

So I think we have to decide if we want to return an error, or we want to claim idempotency.

For my part, I am fine with Jie’s suggestion. If we really believe that the methods are idempotent, then an ALREADY_EXISTS status is pretty much useless anyway.

I am going to go offline for the rest of the day because I am in jury duty. Apologies for any delayed responses.

[jieyu]

I created #160 based on the conversion above. PTAL

[jieyu]

Close this one as #160 has landed.