Add name field in DeleteVolumeRequest

[chakri-nelluri]

Add name field in DeleteVolumeRequest.

Storage subsystems which can implement idempotent volume create & deletes using volume names, can use volume name in delete request.

[akutz]

Hi,

First, please make spec changes to spec.md and use the Makefile to update the generated pronto and Lang bindings.

Second, I do not think this is a necessary change. The VolumeId that is sent to RPCs, including DeleteVolume, is essentially an outer struct for a map[string]string. Simply include the name in the VolumeId.Values map along with the ID value.

[chakri-nelluri]

This required a change in the protobuf numbering. Since we haven't published the versioned spec out yet, I assumed it is ok. If not let me know. I will change the protobuf numbering.

[akutz]

I apologize for the above misspelling. I'm on my phone. I did not mean "pronto," rather "proto."

Also, the process for updating the files I referenced is in the contribution guidelines, so please do provide any feedback on how you think that process can be made more visible. Thanks!

[akutz]

I'm personally wary of field order changes, but as you said, the spec is not yet even tagged as alpha. Still, I'm still of the opinion that you should use the VolumeId Values map for additional identification info about a volume. It's what I'm doing.

[akutz]

It looks like you agree with my suggestion? I say that because your latest commit effectively means this PR has zero additions/deletions. Was that the intent?

[chakri-nelluri]

Thanks @akutz for quick responses. I have some stupid git config issue, trying to fix it. We still need it. Give me few more min. I will explain in detail.

[chakri-nelluri]

Hi @akutz VolumeID details are only available after a successful volume create. CO has no idea whether a volume is created or not when create request fails (say timeout).

In which case, when a delete call comes, CO will try to delete the volume with volume name, as it has no VolumeID details. As delete is idempotent, it should not matter, if the volume was never created in the first place.

[akutz]

I absolutely agree with your intent. I was just proposing that a CO do what I'm currently doing and use the existing VolumeId field, since it's a map and not a simple string value, and use a key of "name" instead of, for example, "id".

I like your proposal too though. It certainly makes it more clear. And as I said I totally agree with the reasoning.

My only concern is that it raises the question I've asked before - why make VolumeId a map then? Is it effectively a truth table? And if so, isn't it the correct place to put the name?

[chakri-nelluri]

Actually, you bring an interesting point.
VolumeID is returned by plugin in response to CreateVolumeRequest. Not sure, if it is the intent of the CO to add extra specific fields to the map. If we agree to add the specific fields to VolumeID then we need to standardize and reserve the names for some fields. I am open to it.
But, it looks more cumbersome where 99% of the deletes just take a volume name.

[akutz]

It's returned by a create request, sure, but there is no reason you cannot instantiate it yourself. Also, I'm not sure standard fields are necessary or even a good idea. Especially if it's a truth table. It may be platform specific and the fields could change depending on the storage platform of the plug-in.

[chakri-nelluri]

Storage Platform & plug-in cannot instantiate the fields when CreateVolumeRequest fails. RPC calls can fail at any point.
If CO instantiates the field unfortunately it has to be a reserved field.

[akutz]

Re: CO. True. I've been using "id" and "name" myself. But I like your idea. It's far cleaner. However, a CO should always know what volumes it has created. It's up to the CO to maintain state, isn't it? At least it maintains the volume names, such as when you do a "docker inspect". That tells you what vols a container has. A CO could then issue a create request against a known volume name to obtain its ID info.

[chakri-nelluri]

When a volume is deleted and when CO is not sure whether the volume is created or not (at which point it has no volume identity info), we still have to issue a delete. It covers that case.

[akutz]

Sure, but I'm saying it could also just issue an idempotent create to get the id...and that would recreate it if it doesn't exist. Duh!

Gotcha.

Yeah, I think I agree with this. Good job!

However, please make the changes to spec.md and then run make. It will generate the csi.proto and update the Lang bindings. Please let me know if you have any questions about this. You'll need to clone this repo into a GOPATH to do this.

[jdef]

Thanks for putting together this PR. I was thinking about this some more and it seems that we should be consistent w/ respect to pairing ID with metadata, as we are for other calls, and that the combination of name+metadata probably doesn't make much sense. I wonder if the following protobuf would be more appropriate (also note the additional docs)?

message DeleteVolumeRequest {
  message Reference {
    // The ID of the volume to be deprovisioned. This field is REQUIRED.
    VolumeID volume_id = 1;

    // The metadata of the volume to be deprovisioned. This field is
    // OPTIONAL.
    VolumeMetadata volume_metadata = 2;
  }

  // The API version assumed by the CO. This field is REQUIRED.
  Version version = 1;

  // Either `name` or `reference` SHALL be specified by the CO. A CO MAY specify
  // the `name` when the volume ID and metadata are unknown. For example, in the
  // case where a `CreateVolume` RPC fails and the CO is unsure of the completion
  // state of the request AND no longer needs the possibly-created volume.
  // Alternatively, a CO MAY specify the `reference` field (ID and metadata) when
  // such information is available to it.
  // This field is REQUIRED.
  oneof handle {

    // The CO-generated name of the volume to delete, same as in 
    // `CreateVolumeRequest`.
    string name = 2;

    // The plugin-generated reference to the volume, as returned in
    // `CreateVolumeResponse`.
    Reference reference = 3;
  }
}

[akutz]

Hi James,

Hmm, I don't follow. A VolumeId is what uniquely identifies a volume, not the CreateVolumeResponse. That's just an ephemeral reference. So you have a permanent ID and the name, which can be used by the plug-in to do a lookup for idempotent ops when the ID is unavailable for an assured, unique reference.

I think this is far simpler than you're making it. I'm not even positive why the metadata is present. A delete request acts on a unique ID, or if not available, the name as proposed in this PR.

It seems to me the above addendum is driving this PR in the wrong direction -- increasing the state that must be preserved by both the CO and the plug-in. It makes total sense that at least a VolumeId should be maintained about a volume, and short of that a name could be provided by a user or discerned from inspecting a container'a attached volumes.

Tying a subsequent volume operation to some "handle" from a previous operation is just pushing the problem of state onto the plug-in and should be avoided at all costs. A volume ID when available and name when not are really the best solutions IMO.

[akutz]

Hi James,

FWIW, today we very much do the something similar with REX-Ray. You can act on volume IDs, or if lacking that, a name will cause a lookup for a volume with that name. The fact is, if we're worried about a false-positive operation occurring against the wrong volume because of a re-used name, then that is when it's completely up to the CO to actually maintain state -- the volume ID. In this case there is nothing to prevent a CO from using persistent storage to maintain the volume IDs of created volumes, using those IDs to issue the delete operations to ensure that the correct volume is deleted. Retries will be handled with idempotency.

Because, and here's the thing, anything in addition to the name to ensure uniqueness and the correct volume would also have to be stored by the CO in some state system -- so why not use said state system to store the actual volume ID?

Or if the argument is that a volume name + metadata could be used to manually construct the delete request, then so could the volume ID since the metadata would be just as specific to the platform as the ID.

And with regards to the operation handle/reference used in a subsequent call, that's even more complicated since it would require both the CO and the plug-in to store state -- the CO to use said handle and the plug-in to match it.

So in conclusion, if the operation has to act on a specific volume, then the volume ID is enough to uniquely identify it and a CO MUST store those IDs in a way that can be recovered upon a crash and thus retried. Only then should an ID be removed from a CO's stateful storage of known volumes.

A name could refer to an unintended volume and any other information used to ensure it's the correct volume requires the same state mechanics as the volume ID, which again, takes us back to why not just use the ID?

[chakri-nelluri]

@akutz VolumeID is not always available

VolumeID is available when the calls succeed as below:
CO -- CreateVolumeRequest--> Plugin -- VolumeInfo(VolumeID + metadata) --> CO

VolumeID is not available when CreateVolumeRequest times out.
CO -- CreateVolumeRequest (Timeout) --> Plugin
In this state, we still have to call DeleteVolumeRequest when CO decides to delete the volume, as we do not know volume is created or not.

@jdef I like the suggestion. Also, on the same note, shall we modify all the calls which include "VolumeID + VolumeMetadata" with VolumeReference?

[akutz]

Hi @chakri-nelluri,

If the CreateVolumeRequest times out then you call it again to see if it was created.

In this state, we still have to call DeleteVolumeRequest when CO decides to delete the volume, as we do not know volume is created or not.

I'm not sure what you mean by We? We don't call anything. We are the plug-in that the CO is calling via the CSI spec; in this case the DeleteVolumeRequest. At least it seems to me as if you're approaching this issue from the point-of-view of a plug-in, and your statement certainly refers to the CO as separate from you, hence it seems you're suggesting you're not the CO in the workflow, thus making you the plug-in?

Guys, I'm not sure anything has been explained sufficiently to me why we need more than a volume ID and the name. A volume ID for when it's known and a name when it's not. Because a volume ID is used to uniquely identify a volume and a name can be used to try and look up a volume. The whole concept of the metadata is odd and perplexing to me as it doesn't seem to do a thing to add to the identity of a volume that the volume ID cannot and SHOULD already do as it is not a simple value, but a map that could represent a marshaled, complex object.

Also @chakri-nelluri, in the case that the request times out and no ID was obtained then just send the name to the DeleteVolumeRequest.

I really do not like the idea of some ephemeral ticket/call reference that places a burden on the plug-in to maintain additional state. Because if the plug-in process gets recycled at the same time a call times out (which may be why the call timed out, in which case no reference could even be provided back to the CO) then the reference could mean jack-squat to the plug-in unless it also maintains state via some persistent mechanism.

A volume name has meaning to the underlying storage platform and can be used by both the CO and the plug-in to try and identify a volume in case of a call timeout or retry.

[chakri-nelluri]

@akutz Please clarify, is the discussion about whether we need only "VolumeID"? or do you agree we need both "VolumeID" & "Name"?

As for as retrying "CreateVolumeRequest", you cannot continue to retry create, when the CO decides to delete the volume.

If you still have more questions, we can talk about it more in Wednesdays call. The thread is going back and forth with too much confusion.

[akutz]

Hi @chakri-nelluri and @jdef,

I asked a colleague to review this issue with me, and he pointed out that I misunderstood the concept of a call reference that @jdef introduced. My colleague realized I was interpreting it as some ephemeral ticket instead of simply a struct to wrap a VolumeId and metadata. I'm fine with that. If I now understand the intent correctly it's that one shouldn't provide the metadata along with the name, hence using a container to group the VolumeId and Metadata atomically such that a oneof is name || (id && metadata). Is that it? If so then I apologize for my confusion.

[jdef]

Yes that was the intent, to group ID+metadata. If the naming is confusing, we should come up with something better.

…

On Mon, Aug 21, 2017 at 2:37 PM, Schley Andrew Kutz < ***@***.***> wrote: Hi @chakri-nelluri <https://github.com/chakri-nelluri> and @jdef <https://github.com/jdef>, I asked a colleague to review this issue with me, and he pointed out that I misunderstood the concept of a call reference that @jdef <https://github.com/jdef> introduced. My colleague realized I was interpreting it as some ephemeral ticket instead of simply a struct to wrap a VolumeId and metadata. I'm fine with that. If I now understand the intent correctly it's that one shouldn't provide the metadata along with the name, hence using a container to group the VolumeId and Metadata atomically such that a oneof is name || (id && metadata). Is that it? If so then I apologize for my confusion. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#88 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLDFHMXpwf_DWCBYTD0WshKsBRef1ks5sac50gaJpZM4O8WJ_> .

[akutz]

Hi @chakri-nelluri and @chakri-nelluri,

Can we clarify the need for the ingress volume metadata at all? Again, the ID can be an object with multiple properties encoded as a map, so why have the addition of the metadata, especially when used to identify a volume when the ID object itself is sufficient.

@jdef, what was confusing was the following:

The plugin-generated reference to the volume

I get that everything from the plug-in is technically plug-in generated, but to be fair, the volume ID (and even metadata) likely originate from the storage platform.

[chakri-nelluri]

@jdef Updated the PR based on your recommendation. PTAL.

Not sure why the make is failing. Anything specific I have to do other than running make? It builds fine in my environment.

[jdef]

thanks @chakri-nelluri

I think the build is failing because you actually need to make changes to the spec.md file and then re-generate the .proto file from the markdown. The .proto file is never to be changed by hand. I'll check our contribution docs, that process might not be clear

[jdef]

xref #94

[akutz]

Yep. I mentioned this multiple times above.

[chakri-nelluri]

Thanks @akutz I missed it between the lines and our conversation. Updated it now.

[akutz]

Ah. Perhaps you can help me so I'm more clear in the future. This was my original response to you:

First, please make spec changes to spec.md and use the Makefile to update the generated proto and Lang bindings.

I probably should have included an example or referenced the contribution document. I apologize for that.

[chakri-nelluri]

@jdef fixed the issue. PTAL when you get a chance.
Thanks @akutz. My bad, I didn't read it properly, my morning coffee didn't kick in by that time :)

[jdef]

this is a pretty ugly type name. does anyone care about this? if so, want to suggest a better field name for reference than what's been proposed here?

[jdef]

VolumeHandle changes have landed. @chakri-nelluri want to take a stab at refactoring this PR to use it?

[chakri-nelluri]

Thanks @jdef. Updated PR.

[jdef]

I'm not crazy about handle as the name for this oneof, but I can't think of anything better. using reference yields a weird golang API code. Naming .. ugh.

[jdef]

xref #103

[chakri-nelluri]

As discussed in the call, if we are not sure create succeeded, we will call listvolumes and get the handle before issuing delete. I am fine with that approach.
Closing the issue.

[jdef]

The implication of the above strategy is that the LIST_VOLUMES capability is tied to the CREATE_DELETE for some cases. It would be nice if the spec clarified expectations around behavior w/ respect to possibly failed create calls and CO intent. Capturing expectations in closed PRs and/or tribal knowledge is not sufficient.