[WIP] KEP 3751 volume attribute class #1
Conversation
maxkimambo
force-pushed
the
3751-volume-attribute-class
branch
from
82e0b25 to
61fe1b9
Compare
| verbs: ["*"] | ||
| - nonResourceURLs: ["*"] | ||
| verbs: ["*"] | ||
| --- |
There was a problem hiding this comment.
Role added temporarily, I need to find correct set of permissions that are required before this is final.
There was a problem hiding this comment.
You need view access to VolumeAttributesClasses similar to how CSI driver gets access to StorageClasses
| return nil, status.Errorf(codes.InvalidArgument, "Failed to modify volume: modifications not supported for disk type %s", diskType) | ||
| } | ||
|
|
||
| // TODO : not sure how this metric should be reported |
| @@ -0,0 +1,33 @@ | |||
| { | |||
| @@ -313,3 +313,28 @@ roleRef: | |||
| kind: Role | |||
| name: csi-gce-pd-leaderelection-role | |||
| apiGroup: rbac.authorization.k8s.io | |||
|
|
|||
There was a problem hiding this comment.
Do you intend to check this in? This in handing over admin privileges to CSI driver. I assume you needed that for testing but we should limit the permissions required by CSI driver as good practice.
| function cleanup { | ||
| rm -rf "$tmpDir" | ||
| } | ||
| # function cleanup { |
| @@ -279,6 +280,22 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string, | |||
| return nil | |||
| } | |||
|
|
|||
| func (cloud *FakeCloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { | |||
|
|
|||
| if params.IOPS == 0 || params.Throughput == 0 { | |||
There was a problem hiding this comment.
We should check IOPS and throughput separately and generate different errors
|
|
||
| func (cloud *CloudProvider) updateZonalDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { | ||
|
|
||
| if params.IOPS == 0 && params.Throughput == 0 { |
There was a problem hiding this comment.
Neither IOPS nor throughput must be 0. Can we distinguish user explicitly setting 0 from zero value used by protobuf?
| "pd-balanced": false, | ||
| "pd-standard": false, | ||
| "pd-ssd": false, |
There was a problem hiding this comment.
I would not include legacy here and only explicitly specify disk types that to do support dynamic performance.
Also IOPS and throughput are not valid for all disk types. pd-throghput only supports throughput for instance.
| return nil, status.Errorf(codes.InvalidArgument, "Failed to modify volume: modifications not supported for disk type %s", diskType) | ||
| } | ||
|
|
||
| // TODO : not sure how this metric should be reported |
There was a problem hiding this comment.
We are reporting it via gceCS.Metrics.RecordOperationErrorMetrics()
| @@ -22,6 +22,8 @@ import ( | |||
| "reflect" | |||
| "sort" | |||
| "strconv" | |||
| "strings" | |||
| "strings" | |||
There was a problem hiding this comment.
How come golang is not complaining about duplicate imports? Just run "go format" before checking in
| expError: "", | ||
| }, | ||
| { | ||
| name: "Volume attribute class parameters should be ignored for incompatible disk types", |
There was a problem hiding this comment.
Shouldn't we rather fail than ignore parameters?
| cmdParams := []string{"container", "clusters", "create", *gkeTestClusterName, | ||
| locationArg, locationVal, "--num-nodes", strconv.Itoa(numNodes), | ||
| "--quiet", "--machine-type", "n1-standard-2", "--image-type", imageType, "--no-enable-autoupgrade"} | ||
| "--quiet", "--machine-type", machineType, "--image-type", imageType, "--no-enable-autoupgrade", "--service-account", "[email protected]"} |
| "pd-balanced": false, | ||
| "pd-standard": false, | ||
| "pd-ssd": false, | ||
| "pd-extreme": true, |
There was a problem hiding this comment.
should be false (but see comment above)
What type of PR is this?
What this PR does / why we need it:
PR adds a feature to modify Iops and throughput parameters when using hyperdisk volumes
This implements
Which issue(s) this PR fixes:
Fixes kubernetes-sigs#1693
Special notes for your reviewer:
This PR is still WIP, looking for early feedback.
Does this PR introduce a user-facing change?: