CreateVolume RPC refinements #101
Conversation
…abilities From reading through the Error Scheme section[1] of the spec and comparing that to the list of CreateVolume Errors[2], I came to the conclusion that AlreadyExists does not quite apply as outlined in the RPC-specific error code section, and instead we should be referring to the general Error Scheme. This changes the error codes around to be more appropriate for signaling what the actual error was, and thus better aligning with the error description we pass back. [1]: https://github.com/container-storage-interface/spec/blob/master/spec.md#error-scheme [2]: https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolume-errors
There is no point continuing any further in the volume creation process if the passed in volume capabilities (access mode) can not be supported by the driver
Added handling for: * Ensuring that the requested volume is at least the minimum supported volume size (1 GB) * Ensuring that the requested volume is not larger than maximum supported volume size (16 TB) Changed the returned error code to InvalidArgument
| requiredBytes := capRange.GetRequiredBytes() | ||
| requiredSet := 0 < requiredBytes | ||
| limitBytes := capRange.GetLimitBytes() | ||
| limitSet := 0 < limitBytes |
There was a problem hiding this comment.
I think we don't need requiredSet and limitSet variables as it makes this function complicated. One because it's not a valid value:
// The capacity of the storage space in bytes. To specify an exact size,
// `required_bytes` and `limit_bytes` SHALL be set to the same value. At
// least one of the these fields MUST be specified.
type CapacityRange struct {
// Volume MUST be at least this big. This field is OPTIONAL.
// A value of 0 is equal to an unspecified field value.
// The value of this field MUST NOT be negative.
RequiredBytes int64 `protobuf:"varint,1,opt,name=required_bytes,json=requiredBytes" json:"required_bytes,omitempty"`
// Volume MUST not be bigger than this. This field is OPTIONAL.
// A value of 0 is equal to an unspecified field value.
// The value of this field MUST NOT be negative.
LimitBytes int64 `protobuf:"varint,2,opt,name=limit_bytes,json=limitBytes" json:"limit_bytes,omitempty"`
It can't be negative, if it's negative, we should return an error immediately. Once we return an error if any of them are negative:
if 0 < requiredBytes || 0 < limitBytes {
return 0, fmt.Errorf("limit %v or required %v bytes can't be negative", limitBytes, requiredBytes)
}we can then remove all limitSet and requiredSet variables and it would be simpler.
There was a problem hiding this comment.
Unfortunately that's not what these variables are for, they are meant to be used as a way to check whether RequiredBytes or LimitBytes was provided.
Since both of these values are optional, per the CSI spec, we can't be sure that one, both or either of them have been set, which is why I thought storing them in a variable that you can simply use to check whether one or the other was set would be convenient.
As it stands today, Kubernetes (via external-provisioner) only ever sets RequiredBytes, but this was intended as a way to future proof things should this change.
I'm happy to drop these values if you feel that's better, but as it stands the code wasn't accounting for a few specific cases, which is why I felt that it would be useful to add these and use them to check different cases.
WDYT?
There was a problem hiding this comment.
It's ok, thanks for the explanation Joonas. PR looks fine 👍
|
I made some additional error message formatting improvements, the errors you'll now see from trying to provision either a too small or too large volume will be as follows: |
After looking through the
CreateVolumecode, I thought that there was a few improvements I could make to it, so here they are.. Let me know if you'd prefer me to break this up or drop some of these.