spec: Update VolumeHandle & NodeID to simple string values

[akutz]

This patch contains two commits: one updates all references to VolumeHandle to a simple string value and the other does the same for NodeID.

Related to #116. cc/ @saad-ali @jieyu

[saad-ali]

Thanks for jumping on this @akutz.

LGTM and this would close #116

I want to make sure we discuss with @julian-hj the "persisting state" reason for keeping metadata, before we merge this and #118

[akutz]

Hi @saad-ali,

I've updated the PR with the change to ValidateVolumeCapabilities. It now expects a volume ID string instead of a VolumeInfo.

[akutz]

Hi @saad-ali,

RE: @julian-hj and persisting state - while the spec may eventually support persisting data for plug-ins, for now I think that has to be in the domain of the plug-in and not something involving the CO. Believe me, I'd love the COs to offer some type of persistence service for plug-ins, but it's not likely to happen for 1.0 of this spec. Thus I recommend etcd or some other easy-to-use and well known out-of-process solution for persisting any data required by a plug-in.

[julian-hj]

I disagree that CO persistence of metadata will always be required.

I believe that there are a class of plugins for which the metadata mechanism we had before would be totally sufficient. Giving up on that goal means that it will be quite a bit harder to make simple plugins that "just work" across COs because now installation of the plugin will require us to do a bunch of CO specific stuff to facilitate installation of a plugin that knows about a database. And once the plugin requires a database, then managing backup and restore of that database in a manner that keeps it consistent with all of the other databases in the system also becomes something that the operator will have to manage in order to use the plugin in a production environment.

The other note I would add is that in addition to giving the controller plugin a place to store a little bit of information about a volume without having a backing store, metadata also gives the controller plugin an easy way to communicate information to the node plugin without some sort of backplane for plugin communication. If we take that away, then that means that every node plugin needs to be able to somehow know stuff about a volume as soon as the controller plugin has created it. So the node plugin goes from potentially an extremely simple implementation to one that needs to manage database connections or call back to the controller plugin or something of that nature. As we move to more complicated deployments with things like isolation segments or multiple clouds, that's a layer of complexity I was hoping we might avoid for plug-in authors.

We discussed all this fairly early on in the design process when we added the metadata into the create volume response, and I thought everyone was on board, but maybe not.... Anyway my vote is "no" for this PR.

[jieyu]

How about pulling metadata from VolumeID and put it into VolumeInfo, and still pass metadata to every subsequent call? This is similar to what we do in the initial commit: c3c3f33

Looks like this satisfy both @saad-ali's concern that id should only have identity information, and also addressed @julian-hj's concern about simplifying writing plugins.

I don't seem to recall why we do the VolumeHandle change later (#97). @jdef did you recall?

[akutz]

Hi @jieyu,

One, there is an outstanding PR where I added tags to Vol info.

Two, James created the handle as it is for the now closed but not merged change to the Delete RPC that would have accepted a name as well.

[akutz]

Three, the Vol info tags/metadata change doesn’t solve the problem of persistence as the CO only persists the handle/ID, not the Vol info.

[jieyu]

@akutz this is my suggestion (which is indeed what we started in c3c3f33)

message VolumeInfo {
  string id;
  map<string, string> metadata; // Your 'tags' change, just renamed a bit.
  ...
}

message ControllerPublishVolume {
  string volume_id;
  map<string, string> volume_metadata;
  ...
}
...

[akutz]

One, I like tags and I’m the one that filed the PR ;) Two, the change to volume-related RPCs doesn’t make sense since the CO isn’t persisting the metadata.

Before we have a discussion about names of fields Saad and others need to come to a final decision regarding persistence and whether the CO should persist anything than the ID string for volumes and nodes.

That’s the heart of this issue. There can be no productive discussion until that matter is settled.

[jieyu]

I am OK with persisting on tag or metadata if that simplifies plugin's life as @julian-hj mentioned.

[akutz]

Hi @jieyu,

If that's the goal then I suggest the VolumeHandle stay the way it is, with Metadata, and declare the metadata NOT part of the volume's identity as it makes no sense if it is supposed to be stateful information.

However, @saad-ali seems to have strong opinions regarding persistence and appears to believe it's the job of plug-ins. Personally I agree with him.

[saad-ali]

Let's keep the larger design discussion on the original issue. And only have PR specific discussions on the PRs. I will reply on #116.

[akutz]

Hi @jdef,

I'm closing this in favor of #123, but I will file a new PR specific to the NodeID.

[jdef]

ok, thanks

…

On Thu, Oct 26, 2017 at 6:30 PM, Schley Andrew Kutz < ***@***.***> wrote: Closed #117 <#117> . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#117 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLArnZ1NlV9KXN6x048xEERdmCpA_ks5swQfvgaJpZM4PuWJ-> .