Refactor VolumeHandle into id and vol attributes #123

jdef · 2017-10-20T12:46:50Z

Incorporates the VolumeHandle changes from spec: Update VolumeHandle & NodeID to simple string values #117 (thanks @akutz!), which eliminates metadata and defines the unique volume identifier as a stringid.
Defines attributes for a VolumeInfo and passes them through to publish and validation requests.

Fixes #116

jdef · 2017-10-20T12:52:16Z

If/when this lands, let's "rebase and merge" to retain the commit stream

jdef · 2017-10-20T12:57:15Z

xref #118

akutz · 2017-10-20T16:05:01Z

Hi @jdef,

I'm not a fan of separating the ID and attributes. I personally like the VolumeHandle you created. If attributes are necessary to perform some type of operation on a Volume, then I think they should be included in the same structure as the ID. The VolumeHandle was perfect for this IMO. The question all along was the necessity of the metadata/attributes. Since they've been deemed necessary I believe they should be part of the VolumeHandle, just not part of the identity. Also, what's wrong with the name Metadata or even Tags? The former is consistent with gRPC naming conventions and the latter is widely used.

jdef · 2017-10-21T03:13:44Z

There was a follow up community meeting where the fate of metadata was decided. Basically, it shouldn't contain sensitive data and wasn't necessary for all calls. I renamed it because I thought it would bring clarity of purpose. Let the bikeshedding on naming begin! :)

…

On Oct 20, 2017 12:07 PM, "Schley Andrew Kutz" ***@***.***> wrote: Hi @jdef <https://github.com/jdef>, I'm not a fan of separating the ID and attributes. I personally like the VolumeHandle you created. If attributes are necessary to perform some type of operation on a Volume, then I think they should be included in the same structure as the ID. The VolumeHandle was perfect for this IMO. The question all along was the necessity of the metadata/attributes. Since they've been deemed necessary I believe they should be part of the VolumeHandle, just not part of the identity. Also, what's wrong with the name Metadata or even Tags? The former is consistent with gRPC naming conventions and the latter is widely used. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACPVLAO6k-5dL46JO6XNyl8jZL2Fpb5Fks5suMSugaJpZM4QAnpQ> .

akutz · 2017-10-21T03:53:37Z

When was the follow up meeting? I wasn’t aware it had occurred yet.

akutz · 2017-10-23T17:51:24Z

csi.proto

+  // to volume validation and publishing calls. Attributes MAY NOT
+  // uniquely identify a volume. A volume uniquely identified by `id`
+  // SHALL always report the same attributes. This field is REQUIRED.
+  Attributes attributes = 3;


Hi @jdef,

It doesn't make sense to me that Attributes should be a discreet message type when it could simply be defined like so:

map<string,string> attributes = 3;

I dislike the trend in this spec to wrap single fields.

+1

I don't have a strong opinion, but given that parameters is in map<string, string>, i'd prefer to use map<string,string> attributes = 3; here.

FWIW, my opinion is strong. I prefer consistency :)

jieyu · 2017-10-23T20:30:25Z

@saad-ali @julian-hj can you please take a look at this PR? It looks good to me, except for the naming part. But I am fine with any naming.

julian-hj · 2017-10-23T20:43:16Z

csi.proto

+  // Attributes reflect static properties of a volume and MUST be passed
+  // to volume validation and publishing calls. Attributes MAY NOT
+  // uniquely identify a volume. A volume uniquely identified by `id`
+  // SHALL always report the same attributes. This field is REQUIRED.


During the meeting, I think we said that attributes was required to be passed by the CO iff it is returned by the SP. We seem to be losing that nuance in this spec and declaring it as required always. Does that mean that we will always require the SP to return a map, and in the case where the SP doesn't care about parameters, it should just return an empty map? I guess I am OK with that requirement, but just want to make sure we are all on the same page.

Hi @julian-hj,

If it's on the SP to indicate if the data is required it can return an error if an empty map is transmitted, right? If you're just wanting to clarify the spec then I agree it needs to have tighter language.

I think in @jdef's PR, it's a struct Attributes. So it's required to set this field, but the map inside the struct can be empty. If we change this to map<string, string>, we need to clarify in the wording. This field is indeed optional, but if set, the CO should pass that to the subsequent calls.

@jieyu Ah, that makes sense. So we didn't lose the nuance, we just codified it as an optional map inside a required struct. Makes sense. I still think we'd be better off with a plain map & some explicit language about the behavior though.

julian-hj · 2017-10-23T20:48:04Z

LGTM, with the caveats that I agree with @akutz about the Attributes type being unnecessary, and I think we need to make it clearer that although attributes is required in the CreateVolume response, it is OK for it to be empty.

jieyu

LGTM once the Attributes type is removed in favor of map<string, string> as @julian-hj and @akutz suggested.

jdef · 2017-10-26T22:22:56Z

Addressed feedback but left it as a separate commit for easier review. If it looks good, I'll squash it with the prior commit (but NOT squash the whole PR). And we can "rebase and merge" when ready.

jdef · 2017-10-26T22:24:04Z

csi.proto

+  // a volume. A volume uniquely identified by `id` SHALL always report
+  // the same attributes. This field is OPTIONAL and when present MUST
+  // be passed to volume validation and publishing calls.
+  map<string,string> attributes = 3;


I guess we should probably have a limit for the encoded size of this field. 1MiB, or something else?

akutz · 2017-10-26T22:24:47Z

Lord I regret ever filing #112. I sure liked VolumeHandle for what it was. Oh well. In lieu of that, this LGTM :) Great work @jdef! Thank you for taking this on!

jdef · 2017-10-26T22:25:12Z

Also, should we have an INVALID_VOLUME_ATTRIBUTES error code for the related RPCs?

akutz · 2017-10-26T22:29:22Z

Hi @jdef,

I'd be hesitant to add any new errors until @saad-ali completes #18.

This patch updates all references to VolumeHandle so that it is now a simple string value. Associated field names and error codes have been updated accordingly.

jdef · 2017-10-31T11:34:04Z

ready to "rebase and merge" once CI completes

jdef added this to the v0.1 milestone Oct 20, 2017

jdef requested review from jieyu and saad-ali October 20, 2017 12:46

jdef mentioned this pull request Oct 20, 2017

Remove Metadata from VolumeHandle #116

Closed

jdef mentioned this pull request Oct 20, 2017

spec: clarify expectations and reduce recommended size of volume ID #124

Closed

akutz reviewed Oct 23, 2017

View reviewed changes

julian-hj reviewed Oct 23, 2017

View reviewed changes

codenrhoden mentioned this pull request Oct 24, 2017

Refactor VolumeHandle into ID rexray/gocsi#25

Closed

jieyu approved these changes Oct 26, 2017

View reviewed changes

jdef force-pushed the jdef_refactor_metadata branch from dc91b0f to efaface Compare October 26, 2017 22:19

jdef commented Oct 26, 2017

View reviewed changes

akutz approved these changes Oct 26, 2017

View reviewed changes

akutz mentioned this pull request Oct 26, 2017

spec: Update VolumeHandle & NodeID to simple string values #117

Closed

jdef mentioned this pull request Oct 26, 2017

NodePublishDevice #122

Closed

VolumeHandle to string

12d7beb

This patch updates all references to VolumeHandle so that it is now a simple string value. Associated field names and error codes have been updated accordingly.

jdef force-pushed the jdef_refactor_metadata branch from efaface to 3e75a0d Compare October 31, 2017 11:31

spec: add Attributes to VolumeInfo, and to publish/validation requests.

7cb087c

jdef force-pushed the jdef_refactor_metadata branch from 3e75a0d to 7cb087c Compare October 31, 2017 11:33

jdef merged commit 00095b2 into container-storage-interface:master Oct 31, 2017

jdef deleted the jdef_refactor_metadata branch October 31, 2017 11:35

jdef mentioned this pull request Dec 14, 2020

Proposal: Allow SP to generate secrets at CreateVolume and ControllerPublish time #466

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor VolumeHandle into id and vol attributes #123

Refactor VolumeHandle into id and vol attributes #123

jdef commented Oct 20, 2017

jdef commented Oct 20, 2017

jdef commented Oct 20, 2017

akutz commented Oct 20, 2017

jdef commented Oct 21, 2017 via email

akutz commented Oct 21, 2017

akutz Oct 23, 2017 •

edited

Loading

jieyu Oct 23, 2017 •

edited

Loading

akutz Oct 23, 2017

julian-hj Oct 23, 2017

jieyu commented Oct 23, 2017

julian-hj Oct 23, 2017

akutz Oct 23, 2017

jieyu Oct 23, 2017

julian-hj Oct 23, 2017

julian-hj commented Oct 23, 2017

jieyu left a comment

jdef commented Oct 26, 2017

jdef Oct 26, 2017

akutz commented Oct 26, 2017

jdef commented Oct 26, 2017

akutz commented Oct 26, 2017

jdef commented Oct 31, 2017

Refactor VolumeHandle into id and vol attributes #123

Refactor VolumeHandle into id and vol attributes #123

Conversation

jdef commented Oct 20, 2017

jdef commented Oct 20, 2017

jdef commented Oct 20, 2017

akutz commented Oct 20, 2017

jdef commented Oct 21, 2017 via email

akutz commented Oct 21, 2017

akutz Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

jieyu Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

akutz Oct 23, 2017

Choose a reason for hiding this comment

julian-hj Oct 23, 2017

Choose a reason for hiding this comment

jieyu commented Oct 23, 2017

julian-hj Oct 23, 2017

Choose a reason for hiding this comment

akutz Oct 23, 2017

Choose a reason for hiding this comment

jieyu Oct 23, 2017

Choose a reason for hiding this comment

julian-hj Oct 23, 2017

Choose a reason for hiding this comment

julian-hj commented Oct 23, 2017

jieyu left a comment

Choose a reason for hiding this comment

jdef commented Oct 26, 2017

jdef Oct 26, 2017

Choose a reason for hiding this comment

akutz commented Oct 26, 2017

jdef commented Oct 26, 2017

akutz commented Oct 26, 2017

jdef commented Oct 31, 2017

akutz Oct 23, 2017 •

edited

Loading

jieyu Oct 23, 2017 •

edited

Loading