Implementing discussed 0.2.0 CSI spec changes #182

sbezverk · 2018-02-01T16:48:40Z

Don't use unsigned integers in proto
Change “ID” to “Id” in Method names
Every “noun” should have a message, so Volume should be a message
These are breaking changes from the 0.1 release.

sbezverk · 2018-02-01T16:52:02Z

@saad-ali @jdef @jieyu Appologies for bugging you, previous PR stopped accepting any chages I tried to push and I had to open a new one. This PR addresses the same from PR #176

jieyu · 2018-02-01T17:48:25Z

@sbezverk can you add some text saying that negative value MUST not be returned by the SP for each int type?

sbezverk · 2018-02-01T18:41:20Z

@jieyu sure, do you want me to add this comment to each occurence of int or one common comment would be sufficient?

akutz · 2018-02-01T18:43:00Z

Hi @sbezverk,

Would you mind please titling the PR with a short, succinct description instead of just the issue numbers? Thanks!

sbezverk · 2018-02-01T18:49:41Z

@akutz There are 4 lines summarizing all the changes done by this PR. Do you need more details?

sbezverk · 2018-02-01T18:50:30Z

@akutz oops, misread your comment. Changing title.

jieyu · 2018-02-01T19:47:32Z

@sbezverk I'd prefer each occurence of int to be more explicit.

sbezverk · 2018-02-01T20:07:01Z

@jieyu done, please let me know if added comments are sufficient.

saad-ali

Minor suggestion on wording of negative, otherwise LGTM

saad-ali · 2018-02-01T20:39:55Z

csi.proto

-  uint32 minor = 2;  // This field is REQUIRED.
-  uint32 patch = 3;  // This field is REQUIRED.
+  // NOTE: Plugin must ensure in returning valid **int** values,
+  // **negative** values must **not** be returned.


How about: The value of these fields must not be negative. for this one.

what you suggest is to replace second line in the NOTE, right?

Correct, I would suggest replace both lines with this.

saad-ali · 2018-02-01T20:41:04Z

csi.proto

@@ -228,19 +230,25 @@ message VolumeCapability {
 message CapacityRange {
  // Volume must be at least this big. This field is OPTIONAL.
  // A value of 0 is equal to an unspecified field value.
-  uint64 required_bytes = 1;
+  // NOTE: Plugin must ensure in returning valid **int** values,
+  // **negative** values must **not** be returned.


How about: The value of this field must not be negative. for this one. And same below?

same question as above..

jieyu · 2018-02-02T00:44:03Z

csi.proto

@@ -228,19 +229,22 @@ message VolumeCapability {
 message CapacityRange {
  // Volume must be at least this big. This field is OPTIONAL.
  // A value of 0 is equal to an unspecified field value.
-  uint64 required_bytes = 1;
+  // The value of this field must not be negative. 


s/must not/MUST NOT/

jieyu · 2018-02-02T00:44:25Z

csi.proto


  // Volume must not be bigger than this. This field is OPTIONAL.
  // A value of 0 is equal to an unspecified field value.
-  uint64 limit_bytes = 2;
+  // The value of this field must not be negative. 


jieyu · 2018-02-02T00:44:36Z

csi.proto

  // The capacity of the volume in bytes. This field is OPTIONAL. If not
  // set (value of 0), it indicates that the capacity of the volume is
  // unknown (e.g., NFS share).
-  uint64 capacity_bytes = 1;
+  // The value of this field must not be negative. 


jieyu · 2018-02-02T00:44:46Z

csi.proto

@@ -402,7 +406,8 @@ message ListVolumesRequest {
  // in the subsequent `ListVolumes` call. This field is OPTIONAL. If
  // not specified (zero value), it means there is no restriction on the
  // number of entries that can be returned.
-  uint32 max_entries = 2;
+  // The value of this field must not be negative. 


jieyu · 2018-02-02T00:44:57Z

csi.proto

@@ -452,7 +457,8 @@ message GetCapacityResponse {
  // specified in the request, the Plugin SHALL take those into
  // consideration when calculating the available capacity of the
  // storage. This field is REQUIRED.
-  uint64 available_capacity = 1;
+  // The value of this field must not be negative. 


jieyu · 2018-02-02T00:45:49Z

spec.md

-  uint32 major = 1;  // This field is REQUIRED.
-  uint32 minor = 2;  // This field is REQUIRED.
-  uint32 patch = 3;  // This field is REQUIRED.
+  // The value of these fields must not be negative.


I'd suggest have one such line per field (in case we add more fields in the future)

akutz · 2018-02-02T00:48:07Z

I think this PR should either contain multiple commits or be split into multiple PRs. I don’t like how many changes are being performed in a single commit. There are clear issues, and this PR doesn’t directly relate to any parictlaur one, which makes tracking things later more difficult.

Closes: #172 #173 #174

sbezverk · 2018-02-02T05:04:07Z

@akutz This change is fairly small it is just 27 lines in spec.md, does it really requires separate commits?

akutz · 2018-02-02T05:19:08Z

I don’t consider 27 lines that small, and separate commits isn’t related to size but the purpose of the change. If changes are not related, why make them part of the same commit? Commits are cheap, and it’s good practice to make reverting changes easier by isolating work into smaller chunks. For example, if this PR is reverted, would you want it pulled out in its entirety or does it make sense to pull out smaller units of work?

…

-- -a

On Feb 1, 2018, at 11:04 PM, Serguei Bezverkhi ***@***.***> wrote: @akutz This change is fairly small it is just 27 lines in spec.md, does it really requires separate commits? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

j-griffith · 2018-02-02T15:26:10Z

csi.proto

@@ -228,19 +231,22 @@ message VolumeCapability {
 message CapacityRange {
  // Volume must be at least this big. This field is OPTIONAL.
  // A value of 0 is equal to an unspecified field value.
-  uint64 required_bytes = 1;
+  // The value of this field MUST NOT be negative. 
+  int64 required_bytes = 1;


Do we want to get rid of the use of underscores while we're at it? Granted that's mostly my own pedantic annoyance :) and it would be a separate patch but just asking.

Hi @j-griffith,

Please leave the underscores. The tell language generators how to camel case things.

j-griffith

I'm good with these clean ups and tightening up the conventions a bit. I'd like to gauge interest in going all in on additional "effective go" recommendations.

sbezverk · 2018-02-02T17:00:13Z

@j-griffith csi.proto is autogenerated file, the actual change was applied ONLY to spec.md

akutz · 2018-02-02T17:43:45Z

I'm confused as to why we wouldn't used unsigned integers in the protobuf. It makes sense for versions and many other places it is used.

akutz · 2018-02-02T17:46:04Z

I don't mind a lot of these changes, but aside from GetNodeID -> NodeGetID, many of them seem to be simply based on preference, and that's not a good reason IMO to break the existing spec. The VolumeInfo to Volume one, for example, will have an especially large impact.

akutz · 2018-02-02T17:46:47Z

I answered my own question about the unsigned integers after reading the original issue:

Recommendation from a gRPC author after review of CSI API: We should not use unsigned integers, and should change them to signed integers because not all languages allow unsigned integers (e.g. Java), and this can cause issues.

sbezverk · 2018-02-02T17:47:05Z

@akutz Based on my understanding, since version info gets send/recevied in gRPC messages, the recommendation from gRPC team was to use int instead of uint as not all gRPC clients support uint type.

akutz · 2018-02-02T17:48:23Z

I still remain adamant that there should be a separate commit, perhaps even PR, for each specific issue. Commits aren't about adding to a project, but about making it easy to remove things later if necessary. As it is now, any issue with any of these changes will require the removal of all of these changes.

saad-ali · 2018-02-05T17:59:58Z

LGTM

I agree it would be nice to have had this PR broken in to multiple commits. But if that's too much of a pain to do at this point, I'm fine accepting it as is.

I don't mind a lot of these changes, but aside from GetNodeID -> NodeGetID, many of them seem to be simply based on preference, and that's not a good reason IMO to break the existing spec. The VolumeInfo to Volume one, for example, will have an especially large impact.

If we're going to make breaking changes, might as well tighten the spec up. The reasoning behind VolumeInfo vs Volume is to align with the operation names CreateVolume, PublishVolume, etc.

julian-hj

👍

sbezverk · 2018-02-05T20:49:43Z

@jdef appreciate if you could review this revision of the spec. thanks a lot.

saad-ali · 2018-02-08T00:25:27Z

Merging. Please ensure future PRs are broken in to logical commits.

akutz · 2018-02-08T01:08:55Z

Just logging my objection to this being accepted as a single commit. It’s bad form, bad precedent, and bad practice. Reverting any of these now means reverting them all.

sbezverk · 2018-02-08T01:11:03Z

@akutz all 3 changes are a part of 0.2.0, so it is ALL or nothing. You cannot have version 0.1.0.3/4. That is the only reason why these three changes are bundled.

akutz · 2018-02-08T01:25:07Z

That is a blunt approach to a process usually handled with more finesse and care.

All commits after the previous tag are not generally squashed upon a release, just because a release can be considered a sum of its parts. That’s a bad process. Features and changes are atomic so they can be reverted later, if necessary, even after a release.

This PR and its changes should have been multiple commits to ensure that changes corresponding to each issue could be reverted separately.

This PR was merged against all best practices, hence my objection.

sbezverk mentioned this pull request Feb 1, 2018

Clean up proto naming #176

Closed

jieyu approved these changes Feb 1, 2018

View reviewed changes

sbezverk changed the title ~~Issues_172_173_174~~ Implementing discussed 0.2.0 CSI spec changes Feb 1, 2018

saad-ali reviewed Feb 1, 2018

View reviewed changes

jieyu reviewed Feb 2, 2018

View reviewed changes

Issues_172_173_174

fb27ffc

Closes: #172 #173 #174

j-griffith reviewed Feb 2, 2018

View reviewed changes

j-griffith approved these changes Feb 2, 2018

View reviewed changes

julian-hj approved these changes Feb 5, 2018

View reviewed changes

saad-ali merged commit 7ab01a9 into container-storage-interface:master Feb 8, 2018

This was referenced Feb 20, 2018

Every “noun” should have a message, so Volume should be a message #174

Closed

Change “ID” to “Id” in Method names #173

Closed

Implementing discussed 0.2.0 CSI spec changes #182

Implementing discussed 0.2.0 CSI spec changes #182

Conversation

sbezverk commented Feb 1, 2018

sbezverk commented Feb 1, 2018

jieyu commented Feb 1, 2018

sbezverk commented Feb 1, 2018

akutz commented Feb 1, 2018

sbezverk commented Feb 1, 2018

sbezverk commented Feb 1, 2018

jieyu commented Feb 1, 2018

sbezverk commented Feb 1, 2018

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz commented Feb 2, 2018

sbezverk commented Feb 2, 2018

akutz commented Feb 2, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith left a comment

Choose a reason for hiding this comment

sbezverk commented Feb 2, 2018

akutz commented Feb 2, 2018 • edited Loading

akutz commented Feb 2, 2018

akutz commented Feb 2, 2018

sbezverk commented Feb 2, 2018

akutz commented Feb 2, 2018

saad-ali commented Feb 5, 2018

julian-hj left a comment

Choose a reason for hiding this comment

sbezverk commented Feb 5, 2018

saad-ali commented Feb 8, 2018

akutz commented Feb 8, 2018

sbezverk commented Feb 8, 2018

akutz commented Feb 8, 2018

akutz commented Feb 2, 2018 •

edited

Loading