diff --git a/cmd/gce-pd-csi-driver/main.go b/cmd/gce-pd-csi-driver/main.go index 3c7808498..4513a82aa 100644 --- a/cmd/gce-pd-csi-driver/main.go +++ b/cmd/gce-pd-csi-driver/main.go @@ -84,6 +84,9 @@ var ( useInstanceAPIForListVolumesPublishedNodesFlag = flag.Bool("use-instance-api-to-list-volumes-published-nodes", false, "Enables using the instances.list API to determine published_node_ids in ListVolumes. When false (default), the disks.list API is used") instancesListFiltersFlag = flag.String("instances-list-filters", "", "Comma separated list of filters to use when calling the instances.list API. By default instances.list fetches all instances in a region") + diskSupportsIopsChangeFlag = flag.String("supports-dynamic-iops-provisioning", "", "Comma separated list of disk types that support dynamic IOPS provisioning") + diskSupportsThroughputChangeFlag = flag.String("supports-dynamic-throughput-provisioning", "", "Comma separated list of disk types that support dynamic throughput provisioning") + extraTagsStr = flag.String("extra-tags", "", "Extra tags to attach to each Compute Disk, Image, Snapshot created. It is a comma separated list of parent id, key and value like '//,...,//'. parent_id is the Organization or the Project ID or Project name where the tag key and the tag value resources exist. A maximum of 50 tags bindings is allowed for a resource. See https://cloud.google.com/resource-manager/docs/tags/tags-overview, https://cloud.google.com/resource-manager/docs/tags/tags-creating-and-managing for details") version string @@ -200,6 +203,14 @@ func handle() { UseInstancesAPIForPublishedNodes: *useInstanceAPIForListVolumesPublishedNodesFlag, } + // Initialize provisionableDisks config + supportsIopsChange := parseCSVFlag(*diskSupportsIopsChangeFlag) + supportsThroughputChange := parseCSVFlag(*diskSupportsThroughputChangeFlag) + provisionableDisksConfig := driver.ProvisionableDisksConfig{ + SupportsIopsChange: supportsIopsChange, + SupportsThroughputChange: supportsThroughputChange, + } + // Initialize requirements for the controller service var controllerServer *driver.GCEControllerServer if *runControllerService { @@ -209,7 +220,7 @@ func handle() { } initialBackoffDuration := time.Duration(*errorBackoffInitialDurationMs) * time.Millisecond maxBackoffDuration := time.Duration(*errorBackoffMaxDurationMs) * time.Millisecond - controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones, *enableStoragePoolsFlag, multiZoneVolumeHandleConfig, listVolumesConfig) + controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones, *enableStoragePoolsFlag, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig) } else if *cloudConfigFilePath != "" { klog.Warningf("controller service is disabled but cloud config given - it has no effect") } diff --git a/deploy/kubernetes/base/controller/cluster_setup.yaml b/deploy/kubernetes/base/controller/cluster_setup.yaml index e22c46b55..a6941a119 100644 --- a/deploy/kubernetes/base/controller/cluster_setup.yaml +++ b/deploy/kubernetes/base/controller/cluster_setup.yaml @@ -34,6 +34,9 @@ rules: - apiGroups: ["storage.k8s.io"] resources: ["storageclasses"] verbs: ["get", "list", "watch"] + - apiGroups: ["storage.k8s.io"] + resources: ["volumeattributesclasses"] + verbs: ["get", "list", "watch"] - apiGroups: [""] resources: ["events"] verbs: ["list", "watch", "create", "update", "patch"] @@ -69,7 +72,7 @@ roleRef: kind: ClusterRole name: csi-gce-pd-provisioner-role apiGroup: rbac.authorization.k8s.io - + --- # xref: https://github.com/kubernetes-csi/external-attacher/blob/master/deploy/kubernetes/rbac.yaml kind: ClusterRole @@ -143,6 +146,9 @@ rules: - apiGroups: [""] resources: ["persistentvolumeclaims/status"] verbs: ["update", "patch"] + - apiGroups: ["storage.k8s.io"] + resources: ["volumeattributesclasses"] + verbs: ["get", "list", "watch"] - apiGroups: [""] resources: ["events"] verbs: ["list", "watch", "create", "update", "patch"] @@ -312,4 +318,4 @@ subjects: roleRef: kind: Role name: csi-gce-pd-leaderelection-role - apiGroup: rbac.authorization.k8s.io + apiGroup: rbac.authorization.k8s.io \ No newline at end of file diff --git a/deploy/kubernetes/base/controller/controller.yaml b/deploy/kubernetes/base/controller/controller.yaml index e7e5db9d1..a9fa141e0 100644 --- a/deploy/kubernetes/base/controller/controller.yaml +++ b/deploy/kubernetes/base/controller/controller.yaml @@ -37,6 +37,7 @@ spec: - "--leader-election" - "--default-fstype=ext4" - "--controller-publish-readonly=true" + - "--feature-gates=VolumeAttributesClass=true" env: - name: PDCSI_NAMESPACE valueFrom: @@ -95,6 +96,7 @@ spec: - "--leader-election" - "--leader-election-namespace=$(PDCSI_NAMESPACE)" - "--handle-volume-inuse-error=false" + - "--feature-gates=VolumeAttributesClass=true" env: - name: PDCSI_NAMESPACE valueFrom: diff --git a/deploy/kubernetes/images/stable-master/image.yaml b/deploy/kubernetes/images/stable-master/image.yaml index 56a0ac6ae..b342ed813 100644 --- a/deploy/kubernetes/images/stable-master/image.yaml +++ b/deploy/kubernetes/images/stable-master/image.yaml @@ -4,7 +4,7 @@ metadata: name: imagetag-csi-provisioner imageTag: name: registry.k8s.io/sig-storage/csi-provisioner - newTag: "v3.6.3" + newTag: "v5.1.0" --- apiVersion: builtin @@ -22,7 +22,7 @@ metadata: name: imagetag-csi-resizer imageTag: name: registry.k8s.io/sig-storage/csi-resizer - newTag: "v1.9.3" + newTag: "v1.11.1" --- apiVersion: builtin diff --git a/deploy/kubernetes/overlays/dev/controller_always_pull.yaml b/deploy/kubernetes/overlays/dev/controller_always_pull.yaml index 77a0b8f0d..0cbc95b74 100644 --- a/deploy/kubernetes/overlays/dev/controller_always_pull.yaml +++ b/deploy/kubernetes/overlays/dev/controller_always_pull.yaml @@ -7,5 +7,4 @@ spec: spec: containers: - name: gce-pd-driver - imagePullPolicy: Always - + imagePullPolicy: Always \ No newline at end of file diff --git a/deploy/kubernetes/overlays/dev/driver-args.yaml b/deploy/kubernetes/overlays/dev/driver-args.yaml new file mode 100644 index 000000000..c8db2b93c --- /dev/null +++ b/deploy/kubernetes/overlays/dev/driver-args.yaml @@ -0,0 +1,7 @@ +- op: add + path: /spec/template/spec/containers/0/args/- + value: --supports-dynamic-throughput-provisioning=hyperdisk-balanced,hyperdisk-throughput,hyperdisk-ml + +- op: add + path: /spec/template/spec/containers/0/args/- + value: --supports-dynamic-iops-provisioning=hyperdisk-balanced,hyperdisk-extreme \ No newline at end of file diff --git a/deploy/kubernetes/overlays/dev/kustomization.yaml b/deploy/kubernetes/overlays/dev/kustomization.yaml index f6ffff391..03861055b 100644 --- a/deploy/kubernetes/overlays/dev/kustomization.yaml +++ b/deploy/kubernetes/overlays/dev/kustomization.yaml @@ -9,6 +9,14 @@ resources: # Here dev overlay is using the same image as alpha transformers: - ../../images/stable-master +# Apply patches to support dynamic provisioning for hyperdisks +patches: +- path: ./driver-args.yaml + target: + group: apps + version: v1 + kind: Deployment + name: csi-gce-pd-controller # To change the dev image, add something like the following. #images: #- name: gke.gcr.io/gcp-compute-persistent-disk-csi-driver diff --git a/examples/kubernetes/demo-vol-update.yml b/examples/kubernetes/demo-vol-update.yml new file mode 100644 index 000000000..e82aa3e1d --- /dev/null +++ b/examples/kubernetes/demo-vol-update.yml @@ -0,0 +1,99 @@ +apiVersion: storage.k8s.io/v1beta1 +kind: VolumeAttributesClass +metadata: + name: silver +driverName: pd.csi.storage.gke.io +parameters: + throughput: "350" + iops: "6000" +--- +apiVersion: storage.k8s.io/v1beta1 +kind: VolumeAttributesClass +metadata: + name: gold +driverName: pd.csi.storage.gke.io +parameters: + throughput: "550" + iops: "15000" +--- +apiVersion: storage.k8s.io/v1 +kind: StorageClass +metadata: + name: balanced +provisioner: pd.csi.storage.gke.io +allowVolumeExpansion: true +volumeBindingMode: WaitForFirstConsumer +parameters: + type: hyperdisk-balanced + provisioned-throughput-on-create: "300Mi" + provisioned-iops-on-create: "5000" +--- +apiVersion: storage.k8s.io/v1 +kind: StorageClass +metadata: + name: throughput-optimized +provisioner: pd.csi.storage.gke.io +volumeBindingMode: WaitForFirstConsumer +allowVolumeExpansion: true +parameters: + type: hyperdisk-balanced + provisioned-throughput-on-create: "500Mi" + provisioned-iops-on-create: "10000" + +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: balanced-pvc +spec: + volumeAttributesClassName: silver + storageClassName: balanced + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 256Gi +--- +apiVersion: v1 +kind: PersistentVolumeClaim +metadata: + name: throughput-optimized-pvc +spec: + volumeAttributesClassName: silver + storageClassName: throughput-optimized + accessModes: + - ReadWriteOnce + resources: + requests: + storage: 256Gi +--- +kind: Pod +apiVersion: v1 +metadata: + name: pod-demo +spec: + volumes: + - name: pvc-demo-vol + persistentVolumeClaim: + claimName: balanced-pvc + - name: data-vol + persistentVolumeClaim: + claimName: throughput-optimized-pvc + containers: + - name: pod-demo + image: nginx:latest + resources: + limits: + cpu: 10m + memory: 80Mi + requests: + cpu: 10m + memory: 80Mi + ports: + - containerPort: 80 + name: "http-server" + volumeMounts: + - mountPath: "/usr/share/nginx/html" + name: pvc-demo-vol + - mountPath: "/data" + name: data-vol diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index 2a1fa4d13..e80346ed8 100644 --- a/pkg/common/parameters.go +++ b/pkg/common/parameters.go @@ -131,6 +131,11 @@ type ParameterProcessor struct { EnableMultiZone bool } +type ModifyVolumeParameters struct { + IOPS *int64 + Throughput *int64 +} + // ExtractAndDefaultParameters will take the relevant parameters from a map and // put them into a well defined struct making sure to default unspecified fields. // extraVolumeLabels are added as labels; if there are also labels specified in @@ -324,3 +329,28 @@ func extractResourceTagsParameter(tagsString string, resourceTags map[string]str } return nil } + +func ExtractModifyVolumeParameters(parameters map[string]string) (ModifyVolumeParameters, error) { + + modifyVolumeParams := ModifyVolumeParameters{} + + for key, value := range parameters { + switch strings.ToLower(key) { + case "iops": + iops, err := ConvertStringToInt64(value) + if err != nil { + return ModifyVolumeParameters{}, fmt.Errorf("parameters contain invalid iops parameter: %w", err) + } + modifyVolumeParams.IOPS = &iops + case "throughput": + throughput, err := ConvertStringToInt64(value) + if err != nil { + return ModifyVolumeParameters{}, fmt.Errorf("parameters contain invalid throughput parameter: %w", err) + } + modifyVolumeParams.Throughput = &throughput + default: + return ModifyVolumeParameters{}, fmt.Errorf("parameters contain unknown parameter: %s", key) + } + } + return modifyVolumeParams, nil +} diff --git a/pkg/common/parameters_test.go b/pkg/common/parameters_test.go index 75f1c2a16..cb60ecff0 100644 --- a/pkg/common/parameters_test.go +++ b/pkg/common/parameters_test.go @@ -482,3 +482,25 @@ func TestSnapshotParameters(t *testing.T) { }) } } +func TestExtractModifyVolumeParameters(t *testing.T) { + parameters := map[string]string{ + "iops": "1000", + "throughput": "500", + } + + iops := int64(1000) + throughput := int64(500) + expected := ModifyVolumeParameters{ + IOPS: &iops, + Throughput: &throughput, + } + + result, err := ExtractModifyVolumeParameters(parameters) + if err != nil { + t.Errorf("Unexpected error: %v", err) + } + + if !reflect.DeepEqual(result, expected) { + t.Errorf("Got ExtractModifyVolumeParameters(%+v) = %+v; want: %v", parameters, result, expected) + } +} diff --git a/pkg/gce-cloud-provider/compute/cloud-disk.go b/pkg/gce-cloud-provider/compute/cloud-disk.go index 02b70d5df..6790992c4 100644 --- a/pkg/gce-cloud-provider/compute/cloud-disk.go +++ b/pkg/gce-cloud-provider/compute/cloud-disk.go @@ -267,3 +267,25 @@ func (d *CloudDisk) GetAccessMode() string { return "" } } + +func (d *CloudDisk) GetProvisionedIops() int64 { + switch { + case d.disk != nil: + return d.disk.ProvisionedIops + case d.betaDisk != nil: + return d.betaDisk.ProvisionedIops + default: + return 0 + } +} + +func (d *CloudDisk) GetProvisionedThroughput() int64 { + switch { + case d.disk != nil: + return d.disk.ProvisionedThroughput + case d.betaDisk != nil: + return d.betaDisk.ProvisionedThroughput + default: + return 0 + } +} diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index 77dbf0751..f2289a398 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -143,7 +143,12 @@ func (cloud *FakeCloudProvider) ListDisksWithFilter(ctx context.Context, fields func (cloud *FakeCloudProvider) ListDisks(ctx context.Context, fields []googleapi.Field) ([]*computev1.Disk, string, error) { d := []*computev1.Disk{} for _, cd := range cloud.disks { - d = append(d, cd.disk) + if cd.disk != nil { + d = append(d, cd.disk) + } else if cd.betaDisk != nil { + betaDisk := convertBetaDiskToV1Disk(cd.betaDisk) + d = append(d, betaDisk) + } } return d, "", nil } @@ -228,14 +233,15 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string, } computeDisk := &computev1.Disk{ - Name: volKey.Name, - SizeGb: common.BytesToGbRoundUp(capBytes), - Description: "Disk created by GCE-PD CSI Driver", - Type: cloud.GetDiskTypeURI(project, volKey, params.DiskType), - SourceDiskId: volumeContentSourceVolumeID, - Status: cloud.mockDiskStatus, - Labels: params.Labels, - ProvisionedIops: params.ProvisionedIOPSOnCreate, + Name: volKey.Name, + SizeGb: common.BytesToGbRoundUp(capBytes), + Description: "Disk created by GCE-PD CSI Driver", + Type: cloud.GetDiskTypeURI(project, volKey, params.DiskType), + SourceDiskId: volumeContentSourceVolumeID, + Status: cloud.mockDiskStatus, + Labels: params.Labels, + ProvisionedIops: params.ProvisionedIOPSOnCreate, + ProvisionedThroughput: params.ProvisionedThroughputOnCreate, } if snapshotID != "" { @@ -279,6 +285,35 @@ 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 { + _, ok := cloud.disks[volKey.String()] + if !ok { + return notFoundError() + } + specifiedIops := params.IOPS != nil && *params.IOPS != 0 + specifiedThroughput := params.Throughput != nil && *params.Throughput != 0 + if !specifiedIops && !specifiedThroughput { + return fmt.Errorf("no IOPS or Throughput specified for disk %v", existingDisk.GetSelfLink()) + } + + if params.IOPS != nil { + if existingDisk.betaDisk != nil { + existingDisk.betaDisk.ProvisionedIops = *params.IOPS + } else if existingDisk.disk != nil { + existingDisk.disk.ProvisionedIops = *params.IOPS + } + } + if params.Throughput != nil { + if existingDisk.betaDisk != nil { + existingDisk.betaDisk.ProvisionedThroughput = *params.Throughput + } else if existingDisk.disk != nil { + existingDisk.disk.ProvisionedThroughput = *params.Throughput + } + } + cloud.disks[volKey.String()] = existingDisk + return nil +} + func (cloud *FakeCloudProvider) DeleteDisk(ctx context.Context, project string, volKey *meta.Key) error { delete(cloud.disks, volKey.String()) return nil diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index b08a28cf9..dc45b6bed 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -104,6 +104,7 @@ type GCECompute interface { ValidateExistingDisk(ctx context.Context, disk *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool) error InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error DeleteDisk(ctx context.Context, project string, volumeKey *meta.Key) error + UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error AttachDisk(ctx context.Context, project string, volKey *meta.Key, readWrite, diskType, instanceZone, instanceName string, forceAttach bool) error DetachDisk(ctx context.Context, project, deviceName, instanceZone, instanceName string) error SetDiskAccessMode(ctx context.Context, project string, volKey *meta.Key, accessMode string) error @@ -459,6 +460,46 @@ func (cloud *CloudProvider) InsertDisk(ctx context.Context, project string, volK } } +func (cloud *CloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { + // hyperdisks are zonal disks + // pd-disks do not support modification of IOPS and Throughput + if volKey.Type() == meta.Regional { + return status.Error(codes.InvalidArgument, "Cannot update regional disk") + } + klog.V(5).Infof("Updating disk %v", volKey) + return cloud.updateZonalDisk(ctx, project, volKey, existingDisk, params) +} + +func (cloud *CloudProvider) updateZonalDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error { + specifiedIops := params.IOPS != nil && *params.IOPS != 0 + specifiedThroughput := params.Throughput != nil && *params.Throughput != 0 + if !specifiedIops && !specifiedThroughput { + return fmt.Errorf("no IOPS or Throughput specified for disk %v", existingDisk.GetSelfLink()) + } + updatedDisk := &computev1.Disk{ + Name: existingDisk.GetName(), + } + paths := []string{} + if params.IOPS != nil && *params.IOPS != 0 { + updatedDisk.ProvisionedIops = *params.IOPS + paths = append(paths, "provisionedIops") + } + if params.Throughput != nil && *params.Throughput != 0 { + updatedDisk.ProvisionedThroughput = *params.Throughput + paths = append(paths, "provisionedThroughput") + } + + diskUpdateOp := cloud.service.Disks.Update(project, volKey.Zone, volKey.Name, updatedDisk) + diskUpdateOp.Paths(paths...) + _, err := diskUpdateOp.Context(ctx).Do() + + if err != nil { + return fmt.Errorf("error updating disk %v: %w", volKey, err) + } + + return nil +} + func convertV1CustomerEncryptionKeyToBeta(v1Key *computev1.CustomerEncryptionKey) *computebeta.CustomerEncryptionKey { return &computebeta.CustomerEncryptionKey{ KmsKeyName: v1Key.KmsKeyName, @@ -526,6 +567,72 @@ func convertV1DiskToBetaDisk(v1Disk *computev1.Disk) *computebeta.Disk { return betaDisk } +func convertBetaCustomerEncryptionKeyToV1(betaKey *computebeta.CustomerEncryptionKey) *computev1.CustomerEncryptionKey { + return &computev1.CustomerEncryptionKey{ + KmsKeyName: betaKey.KmsKeyName, + RawKey: betaKey.RawKey, + Sha256: betaKey.Sha256, + ForceSendFields: betaKey.ForceSendFields, + NullFields: betaKey.NullFields, + } +} + +func convertBetaDiskParamsToV1(betaDiskParams *computebeta.DiskParams) *computev1.DiskParams { + resourceManagerTags := make(map[string]string) + for k, v := range betaDiskParams.ResourceManagerTags { + resourceManagerTags[k] = v + } + return &computev1.DiskParams{ + ResourceManagerTags: resourceManagerTags, + } +} +func convertBetaDiskToV1Disk(betaDisk *computebeta.Disk) *computev1.Disk { + var dek *computev1.CustomerEncryptionKey = nil + + if betaDisk.DiskEncryptionKey != nil { + dek = convertBetaCustomerEncryptionKeyToV1(betaDisk.DiskEncryptionKey) + } + + var params *computev1.DiskParams = nil + if betaDisk.Params != nil { + params = convertBetaDiskParamsToV1(betaDisk.Params) + } + + // Note: this is an incomplete list. It only includes the fields we use for disk creation. + v1Disk := &computev1.Disk{ + Name: betaDisk.Name, + SizeGb: betaDisk.SizeGb, + Description: betaDisk.Description, + Type: betaDisk.Type, + SourceSnapshot: betaDisk.SourceSnapshot, + SourceImage: betaDisk.SourceImage, + SourceImageId: betaDisk.SourceImageId, + SourceSnapshotId: betaDisk.SourceSnapshotId, + SourceDisk: betaDisk.SourceDisk, + ReplicaZones: betaDisk.ReplicaZones, + DiskEncryptionKey: dek, + Zone: betaDisk.Zone, + Region: betaDisk.Region, + Status: betaDisk.Status, + SelfLink: betaDisk.SelfLink, + Params: params, + AccessMode: betaDisk.AccessMode, + } + + // Hyperdisk doesn't currently support multiWriter (https://cloud.google.com/compute/docs/disks/hyperdisks#limitations), + // but if multiWriter + hyperdisk is supported in the future, we want the PDCSI driver to support this feature without + // any additional code change. + if betaDisk.ProvisionedIops > 0 { + v1Disk.ProvisionedIops = betaDisk.ProvisionedIops + } + if betaDisk.ProvisionedThroughput > 0 { + v1Disk.ProvisionedThroughput = betaDisk.ProvisionedThroughput + } + v1Disk.StoragePool = betaDisk.StoragePool + + return v1Disk +} + func (cloud *CloudProvider) insertRegionalDisk( ctx context.Context, project string, diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 8c1211b6d..5a47e5da9 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -109,6 +109,8 @@ type GCEControllerServer struct { listVolumesConfig ListVolumesConfig + provisionableDisksConfig ProvisionableDisksConfig + // Embed UnimplementedControllerServer to ensure the driver returns Unimplemented for any // new RPC methods that might be introduced in future versions of the spec. csi.UnimplementedControllerServer @@ -135,6 +137,11 @@ type ListVolumesConfig struct { UseInstancesAPIForPublishedNodes bool } +type ProvisionableDisksConfig struct { + SupportsIopsChange []string + SupportsThroughputChange []string +} + func (c ListVolumesConfig) listDisksFields() []googleapi.Field { if c.UseInstancesAPIForPublishedNodes { // If we are using the instances.list API in ListVolumes, @@ -332,11 +339,38 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req if err != nil { return nil, status.Errorf(codes.InvalidArgument, "failed to extract parameters: %v", err.Error()) } + // https://github.com/container-storage-interface/spec/blob/master/spec.md#createvolume + // mutable_parameters MUST take precedence over the values from parameters. + mutableParams := req.GetMutableParameters() + // If the disk type does not support dynamic provisioning, throw an error + supportsIopsChange := gceCS.diskSupportsIopsChange(params.DiskType) + supportsThroughputChange := gceCS.diskSupportsThroughputChange(params.DiskType) + if len(mutableParams) > 0 { + if !supportsIopsChange && !supportsThroughputChange { + return nil, status.Errorf(codes.InvalidArgument, "Disk type %s does not support dynamic provisioning", params.DiskType) + } + p, err := common.ExtractModifyVolumeParameters(mutableParams) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "Invalid mutable parameters: %v", err) + } + if p.IOPS != nil { + if !supportsIopsChange { + return nil, status.Errorf(codes.InvalidArgument, "Cannot specify IOPS for disk type %s", params.DiskType) + } + params.ProvisionedIOPSOnCreate = *p.IOPS + } + if p.Throughput != nil { + if !supportsThroughputChange { + return nil, status.Errorf(codes.InvalidArgument, "Cannot specify throughput for disk type %s", params.DiskType) + } + params.ProvisionedThroughputOnCreate = *p.Throughput + } + } + // Validate multiwriter if _, err := getMultiWriterFromCapabilities(volumeCapabilities); err != nil { return nil, status.Errorf(codes.InvalidArgument, "VolumeCapabilities is invalid: %v", err.Error()) } - err = validateStoragePools(req, params, gceCS.CloudProvider.GetDefaultProject()) if err != nil { // Reassign error so that all errors are reported as InvalidArgument to RecordOperationErrorMetrics. @@ -720,8 +754,96 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi return disk, nil } +func (gceCS *GCEControllerServer) diskSupportsIopsChange(diskType string) bool { + for _, disk := range gceCS.provisionableDisksConfig.SupportsIopsChange { + if disk == diskType { + return true + } + } + return false +} + +func (gceCS *GCEControllerServer) diskSupportsThroughputChange(diskType string) bool { + for _, disk := range gceCS.provisionableDisksConfig.SupportsThroughputChange { + if disk == diskType { + return true + } + } + return false +} + func (gceCS *GCEControllerServer) ControllerModifyVolume(ctx context.Context, req *csi.ControllerModifyVolumeRequest) (*csi.ControllerModifyVolumeResponse, error) { - return nil, status.Error(codes.Unimplemented, "ControllerModifyVolume unsupported") + var err error + + volumeID := req.GetVolumeId() + klog.V(4).Infof("Modifying Volume ID: %s", volumeID) + + if len(volumeID) == 0 { + return nil, status.Error(codes.InvalidArgument, "volume ID must be provided") + } + + diskType := metrics.DefaultDiskTypeForMetric + enableConfidentialCompute := metrics.DefaultEnableConfidentialCompute + enableStoragePools := metrics.DefaultEnableStoragePools + + defer func() { + gceCS.Metrics.RecordOperationErrorMetrics("ControllerModifyVolume", err, diskType, enableConfidentialCompute, enableStoragePools) + }() + + project, volKey, err := common.VolumeIDToKey(volumeID) + if err != nil { + // Cannot find volume associated with this ID because VolumeID is not in the correct format + err = status.Errorf(codes.NotFound, "volume ID is invalid: %v", err.Error()) + return nil, err + } + + volumeModifyParams, err := common.ExtractModifyVolumeParameters(req.GetMutableParameters()) + if err != nil { + klog.Errorf("Failed to extract parameters for volume %s: %v", volumeID, err) + err = status.Errorf(codes.InvalidArgument, "Invalid parameters: %v", err) + return nil, err + } + klog.V(4).Infof("Modify Volume Parameters for %s: %v", volumeID, volumeModifyParams) + + existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionBeta) + + if err != nil { + err = fmt.Errorf("Failed to get volume: %w", err) + return nil, err + } + + if existingDisk == nil || existingDisk.GetSelfLink() == "" { + err = status.Errorf(codes.Internal, "failed to get volume : %s", volumeID) + return nil, err + } + diskType = existingDisk.GetPDType() + + // Check if the disk supports dynamic IOPS/Throughput provisioning + supportsIopsChange := gceCS.diskSupportsIopsChange(diskType) + supportsThroughputChange := gceCS.diskSupportsThroughputChange(diskType) + if !supportsIopsChange && !supportsThroughputChange { + err = status.Errorf(codes.InvalidArgument, "Failed to modify volume: modifications not supported for disk type %s", diskType) + return nil, err + } + if !supportsIopsChange && volumeModifyParams.IOPS != nil { + err = status.Errorf(codes.InvalidArgument, "Cannot specify IOPS for disk type %s", diskType) + return nil, err + } + if !supportsThroughputChange && volumeModifyParams.Throughput != nil { + err = status.Errorf(codes.InvalidArgument, "Cannot specify throughput for disk type %s", diskType) + return nil, err + } + + enableStoragePools = strconv.FormatBool(existingDisk.GetEnableStoragePools()) + + err = gceCS.CloudProvider.UpdateDisk(ctx, project, volKey, existingDisk, volumeModifyParams) + if err != nil { + klog.Errorf("Failed to modify volume %s: %v", volumeID, err) + err = fmt.Errorf("Failed to modify volume %s: %w", volumeID, err) + return nil, err + } + + return &csi.ControllerModifyVolumeResponse{}, nil } func (gceCS *GCEControllerServer) DeleteVolume(ctx context.Context, req *csi.DeleteVolumeRequest) (*csi.DeleteVolumeResponse, error) { diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index 497afe0ef..0e79a3663 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -16,12 +16,14 @@ package gceGCEDriver import ( "context" + "errors" "fmt" "math/rand" "net/http" "reflect" "sort" "strconv" + "strings" "testing" "time" @@ -34,15 +36,16 @@ import ( "google.golang.org/api/googleapi" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" - "k8s.io/apimachinery/pkg/util/clock" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/util/flowcontrol" "k8s.io/klog/v2" + clock "k8s.io/utils/clock/testing" "k8s.io/utils/strings/slices" csi "github.com/container-storage-interface/spec/lib/go/csi" "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common" gce "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" + gcecloudprovider "sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/gce-cloud-provider/compute" ) const ( @@ -1749,6 +1752,395 @@ func TestMultiZoneVolumeCreationErrHandling(t *testing.T) { } } +func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) { + // When volume attribute class specifies iops / throughput they should take precedence over storage class parameters + testCases := []struct { + name string + req *csi.CreateVolumeRequest + expIops int64 + expThroughput int64 + wantErr bool + expErrCode codes.Code + }{ + { + name: "VolumeAttributesClass parameters should take precedence over storage class parameters", + req: &csi.CreateVolumeRequest{ + Name: name, + CapacityRange: stdCapRange, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + }, + Parameters: map[string]string{ + common.ParameterKeyType: "hyperdisk-balanced", + common.ParameterKeyProvisionedIOPSOnCreate: "10000", + common.ParameterKeyProvisionedThroughputOnCreate: "500Mi", + }, + AccessibilityRequirements: &csi.TopologyRequirement{ + Preferred: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"}, + }, + }, + }, + MutableParameters: map[string]string{"iops": "20000", "throughput": "600"}, + }, + expIops: 20000, + expThroughput: 600, + wantErr: false, + }, + { + name: "VolumeAttributesClass parameters should throw an error for incompatible disk types", + req: &csi.CreateVolumeRequest{ + Name: "pd-ssd-vol", + CapacityRange: stdCapRange, + VolumeCapabilities: []*csi.VolumeCapability{ + { + AccessType: &csi.VolumeCapability_Mount{ + Mount: &csi.VolumeCapability_MountVolume{}, + }, + AccessMode: &csi.VolumeCapability_AccessMode{ + Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER, + }, + }, + }, + Parameters: map[string]string{ + common.ParameterKeyType: "pd-ssd", + common.ParameterKeyProvisionedIOPSOnCreate: "10000", + common.ParameterKeyProvisionedThroughputOnCreate: "500Mi", + }, + AccessibilityRequirements: &csi.TopologyRequirement{ + Preferred: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"}, + }, + }, + }, + MutableParameters: map[string]string{"iops": "20000", "throughput": "600"}, + }, + expIops: 0, + expThroughput: 0, + wantErr: true, + expErrCode: codes.InvalidArgument, + }, + } + + for _, tc := range testCases { + var d []*gce.CloudDisk + fcp, err := gce.CreateFakeCloudProvider(project, zone, d) + gceDriver := initGCEDriverWithCloudProvider(t, fcp) + + if err != nil { + t.Fatalf("Failed to create fake cloud provider: %v", err) + } + + createVolReq := tc.req + + resp, err := gceDriver.cs.CreateVolume(context.Background(), createVolReq) + if tc.wantErr { + if status.Code(err) != tc.expErrCode { + t.Fatalf("Expected error code: %v, got: %v. err : %v", tc.expErrCode, status.Code(err), err) + } + continue + } + if err != nil { + t.Fatalf("Failed to create volume: %v", err) + } + + volumeId := resp.GetVolume().VolumeId + project, volumeKey, err := common.VolumeIDToKey(volumeId) + if err != nil { + t.Fatalf("Failed to convert volume id to key: %v", err) + } + + disk, err := fcp.GetDisk(context.Background(), project, volumeKey, gce.GCEAPIVersionBeta) + + if err != nil { + t.Fatalf("Failed to get disk: %v", err) + } + if disk != nil { + if disk.GetProvisionedIops() != tc.expIops { + t.Errorf("Expected IOPS to be %d, got: %v", tc.expIops, disk.GetProvisionedIops()) + } + if disk.GetProvisionedThroughput() != tc.expThroughput { + t.Errorf("Expected Throughput to be %d, got: %v", tc.expThroughput, disk.GetProvisionedThroughput()) + } + } + } + +} + +func TestVolumeModifyOperation(t *testing.T) { + testCases := []struct { + name string + req *csi.ControllerModifyVolumeRequest + diskType string + params *common.DiskParameters + expIops int64 + expThroughput int64 + expErrMessage string + }{ + { + name: "Update volume with valid parameters", + req: &csi.ControllerModifyVolumeRequest{ + VolumeId: testVolumeID, + MutableParameters: map[string]string{"iops": "20000", "throughput": "600"}, + }, + diskType: "hyperdisk-balanced", + params: &common.DiskParameters{ + DiskType: "hyperdisk-balanced", + ProvisionedIOPSOnCreate: 10000, + ProvisionedThroughputOnCreate: 500, + }, + expIops: 20000, + expThroughput: 600, + expErrMessage: "", + }, + { + name: "Update volume with invalid parameters", + req: &csi.ControllerModifyVolumeRequest{ + VolumeId: testVolumeID, + MutableParameters: map[string]string{"iops": "0", "throughput": "0"}, + }, + diskType: "hyperdisk-balanced", + params: &common.DiskParameters{ + DiskType: "hyperdisk-balanced", + ProvisionedIOPSOnCreate: 10000, + ProvisionedThroughputOnCreate: 500, + }, + expIops: 10000, + expThroughput: 500, + expErrMessage: "no IOPS or Throughput specified for disk", + }, + { + name: "Update volume with valid parameters but invalid disk type", + req: &csi.ControllerModifyVolumeRequest{ + VolumeId: testVolumeID, + MutableParameters: map[string]string{"iops": "20000", "throughput": "600"}, + }, + diskType: "pd-ssd", + params: &common.DiskParameters{ + DiskType: "pd-ssd", + }, + expIops: 0, + expThroughput: 0, + expErrMessage: fmt.Sprintf("modifications not supported for disk type %s", "pd-ssd"), + }, + } + + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + // Arrange + fcp, err := gce.CreateFakeCloudProvider(project, zone, nil) + + if err != nil { + t.Fatalf("Failed to create mock cloud provider: %v", err) + } + + gceDriver := initGCEDriverWithCloudProvider(t, fcp) + project, volKey, err := common.VolumeIDToKey(testVolumeID) + if err != nil { + t.Fatalf("Failed convert key: %v", err) + } + + err = fcp.InsertDisk(context.Background(), project, volKey, *tc.params, 200000, nil, nil, "", "", false, "") + if err != nil { + t.Fatalf("Failed to insert disk: %v", err) + } + // Act + _, err = gceDriver.cs.ControllerModifyVolume(context.Background(), tc.req) + + // Assert + if err != nil { + msg := err.Error() + if !strings.ContainsAny(msg, tc.expErrMessage) { + t.Errorf("Failed to modify volume: %v", err) + } + } + + modifiedVol, err := fcp.GetDisk(context.Background(), project, volKey, gce.GCEAPIVersionBeta) + + if err != nil { + t.Errorf("Failed to get volume: %v", err) + } + + diskIops := modifiedVol.GetProvisionedIops() + throughput := modifiedVol.GetProvisionedThroughput() + + if diskIops != tc.expIops && throughput != tc.expThroughput { + t.Errorf("Failed to modify volume: %v", err) + } + } +} + +type FakeCloudProviderUpdateDiskErr struct { + *gce.FakeCloudProvider + updateDiskErrors map[string]error +} + +func NewFakeCloudProviderUpdateDiskErr(project, zone string) (*FakeCloudProviderUpdateDiskErr, error) { + provider, err := gce.CreateFakeCloudProvider(project, zone, nil) + if err != nil { + return nil, err + } + return &FakeCloudProviderUpdateDiskErr{ + FakeCloudProvider: provider, + updateDiskErrors: map[string]error{}, + }, nil +} + +func (cloud *FakeCloudProviderUpdateDiskErr) AddDiskForErr(volKey *meta.Key, err error) { + cloud.updateDiskErrors[volKey.String()] = err +} + +func (cloud *FakeCloudProviderUpdateDiskErr) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *gcecloudprovider.CloudDisk, params common.ModifyVolumeParameters) error { + if err, ok := cloud.updateDiskErrors[volKey.String()]; ok { + return err + } + + return cloud.FakeCloudProvider.UpdateDisk(ctx, project, volKey, existingDisk, params) +} + +type modifyVolumeErrorTest struct { + expErrCode int + wantReason bool + reason string +} + +func TestVolumeModifyErrorHandling(t *testing.T) { + testCases := []struct { + name string + modifyVolumeErrors map[*meta.Key]error + createReq *csi.CreateVolumeRequest + modifyReq *csi.ControllerModifyVolumeRequest + expErr *modifyVolumeErrorTest + }{ + { + name: "disk notFound errors", + modifyReq: &csi.ControllerModifyVolumeRequest{}, + expErr: &modifyVolumeErrorTest{ + wantReason: true, + reason: "notFound", + }, + }, + { + name: "Too Many Requests errors", + createReq: &csi.CreateVolumeRequest{ + Name: name, + Parameters: map[string]string{ + common.ParameterKeyType: "hyperdisk-balanced", + common.ParameterKeyProvisionedIOPSOnCreate: "3000", + common.ParameterKeyProvisionedThroughputOnCreate: "150Mi", + }, + VolumeCapabilities: stdVolCaps, + AccessibilityRequirements: &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"}, + }, + }, + Preferred: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"}, + }, + }, + }, + }, + modifyReq: &csi.ControllerModifyVolumeRequest{ + MutableParameters: map[string]string{"iops": "3001", "throughput": "151"}, + }, + modifyVolumeErrors: map[*meta.Key]error{ + meta.ZonalKey(name, "us-central1-a"): &googleapi.Error{ + Code: http.StatusTooManyRequests, + Message: "too many IOPS/Throughput modifications in a 6 hour window", + }, + }, + expErr: &modifyVolumeErrorTest{ + expErrCode: http.StatusTooManyRequests, + }, + }, + { + name: "InvalidArgument errors", + createReq: &csi.CreateVolumeRequest{ + Name: name, + Parameters: map[string]string{ + common.ParameterKeyType: "hyperdisk-balanced", + common.ParameterKeyProvisionedIOPSOnCreate: "3000", + common.ParameterKeyProvisionedThroughputOnCreate: "150Mi", + }, + VolumeCapabilities: stdVolCaps, + AccessibilityRequirements: &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"}, + }, + }, + Preferred: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"}, + }, + }, + }, + }, + modifyReq: &csi.ControllerModifyVolumeRequest{ + MutableParameters: map[string]string{"iops": "10000", "throughput": "2400"}, + }, + modifyVolumeErrors: map[*meta.Key]error{ + meta.ZonalKey(name, "us-central1-a"): &googleapi.Error{Code: int(codes.InvalidArgument), Message: "InvalidArgument"}, + }, + expErr: &modifyVolumeErrorTest{ + expErrCode: int(codes.InvalidArgument), + }, + }, + } + for _, tc := range testCases { + t.Logf("test case: %s", tc.name) + fcp, err := NewFakeCloudProviderUpdateDiskErr(project, zone) + if err != nil { + t.Fatalf("Failed to create mock cloud provider") + } + gceDriver := initGCEDriverWithCloudProvider(t, fcp) + + for volKey, err := range tc.modifyVolumeErrors { + fcp.AddDiskForErr(volKey, err) + } + + volId := testVolumeID + if tc.createReq != nil { + fmt.Printf("Creating volume") + resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.createReq) + if err != nil { + t.Errorf("Expected no error, got %v", err) + } + volId = resp.GetVolume().VolumeId + } + + tc.modifyReq.VolumeId = volId + _, err = gceDriver.cs.ControllerModifyVolume(context.Background(), tc.modifyReq) + if err == nil { + t.Errorf("Expected err: %v, got no error", tc.expErr.expErrCode) + } + + var e *googleapi.Error + if ok := errors.As(err, &e); ok { + if e.Code != tc.expErr.expErrCode { + t.Errorf("Expected error: %v, got: %v", tc.expErr.expErrCode, e.Code) + } + if tc.expErr.wantReason && !googleapiErrContainsReason(e, tc.expErr.reason) { + t.Errorf("Expected error to contain reason %s", tc.expErr.reason) + } + } else { + t.Errorf("Expected error %v to be a googleapi error", err) + } + } +} + func TestListVolumePagination(t *testing.T) { testCases := []struct { name string @@ -4922,3 +5314,12 @@ func isInternalError(err error) bool { return st.Code().String() == "Internal" } + +func googleapiErrContainsReason(err *googleapi.Error, reason string) bool { + for _, errItem := range err.Errors { + if strings.Contains(errItem.Reason, reason) { + return true + } + } + return false +} diff --git a/pkg/gce-pd-csi-driver/gce-pd-driver.go b/pkg/gce-pd-csi-driver/gce-pd-driver.go index ba1a0c0ba..b41ce6e3d 100644 --- a/pkg/gce-pd-csi-driver/gce-pd-driver.go +++ b/pkg/gce-pd-csi-driver/gce-pd-driver.go @@ -73,6 +73,7 @@ func (gceDriver *GCEDriver) SetupGCEDriver(name, vendorVersion string, extraVolu csi.ControllerServiceCapability_RPC_LIST_VOLUMES, csi.ControllerServiceCapability_RPC_LIST_VOLUMES_PUBLISHED_NODES, csi.ControllerServiceCapability_RPC_CLONE_VOLUME, + csi.ControllerServiceCapability_RPC_MODIFY_VOLUME, } gceDriver.AddControllerServiceCapabilities(csc) ns := []csi.NodeServiceCapability_RPC_Type{ @@ -154,7 +155,7 @@ func NewNodeServer(gceDriver *GCEDriver, mounter *mount.SafeFormatAndMount, devi } } -func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, errorBackoffInitialDuration, errorBackoffMaxDuration time.Duration, fallbackRequisiteZones []string, enableStoragePools bool, multiZoneVolumeHandleConfig MultiZoneVolumeHandleConfig, listVolumesConfig ListVolumesConfig) *GCEControllerServer { +func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, errorBackoffInitialDuration, errorBackoffMaxDuration time.Duration, fallbackRequisiteZones []string, enableStoragePools bool, multiZoneVolumeHandleConfig MultiZoneVolumeHandleConfig, listVolumesConfig ListVolumesConfig, provisionableDisksConfig ProvisionableDisksConfig) *GCEControllerServer { return &GCEControllerServer{ Driver: gceDriver, CloudProvider: cloudProvider, @@ -165,6 +166,7 @@ func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, err enableStoragePools: enableStoragePools, multiZoneVolumeHandleConfig: multiZoneVolumeHandleConfig, listVolumesConfig: listVolumesConfig, + provisionableDisksConfig: provisionableDisksConfig, } } diff --git a/pkg/gce-pd-csi-driver/gce-pd-driver_test.go b/pkg/gce-pd-csi-driver/gce-pd-driver_test.go index 15306c141..7e8138207 100644 --- a/pkg/gce-pd-csi-driver/gce-pd-driver_test.go +++ b/pkg/gce-pd-csi-driver/gce-pd-driver_test.go @@ -50,8 +50,12 @@ func initGCEDriverWithCloudProvider(t *testing.T, cloudProvider gce.GCECompute) enableStoragePools := false multiZoneVolumeHandleConfig := MultiZoneVolumeHandleConfig{} listVolumesConfig := ListVolumesConfig{} + provisionableDisksConfig := ProvisionableDisksConfig{ + SupportsIopsChange: []string{"hyperdisk-balanced", "hyperdisk-extreme"}, + SupportsThroughputChange: []string{"hyperdisk-balanced", "hyperdisk-throughput", "hyperdisk-ml"}, + } - controllerServer := NewControllerServer(gceDriver, cloudProvider, errorBackoffInitialDuration, errorBackoffMaxDuration, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig) + controllerServer := NewControllerServer(gceDriver, cloudProvider, errorBackoffInitialDuration, errorBackoffMaxDuration, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig) err := gceDriver.SetupGCEDriver(driver, vendorVersion, nil, nil, nil, controllerServer, nil) if err != nil { t.Fatalf("Failed to setup GCE Driver: %v", err) diff --git a/test/k8s-integration/cluster.go b/test/k8s-integration/cluster.go index 5f63cbaaf..88343c530 100644 --- a/test/k8s-integration/cluster.go +++ b/test/k8s-integration/cluster.go @@ -90,6 +90,24 @@ func clusterUpGCE(k8sDir, gceZone string, numNodes int, numWindowsNodes int, ima klog.V(4).Infof("Set Kubernetes feature gates: %v", *kubeFeatureGates) } + if len(*kubeRuntimeConfig) != 0 { + err = os.Setenv("KUBE_RUNTIME_CONFIG", *kubeRuntimeConfig) + if err != nil { + return fmt.Errorf("failed to set kubernetes runtime config: %w", err) + } + klog.V(4).Infof("Set Kubernetes runtime config: %v", *kubeRuntimeConfig) + // If runtime config is set, we will update the machine type to support hyperdisks + err = os.Setenv("NODE_SIZE", "c3-standard-4") + if err != nil { + return fmt.Errorf("failed to set NODE_SIZE: %w", err) + } + // The node disk type also needs to be updated + err = os.Setenv("NODE_DISK_TYPE", "pd-ssd") + if err != nil { + return fmt.Errorf("failed to set NODE_DISK_TYPE: %w", err) + } + } + err = setImageTypeEnvs(imageType) if err != nil { return fmt.Errorf("failed to set image type environment variables: %w", err) diff --git a/test/k8s-integration/config/hdb-volumeattributesclass.yaml b/test/k8s-integration/config/hdb-volumeattributesclass.yaml new file mode 100644 index 000000000..f032912d2 --- /dev/null +++ b/test/k8s-integration/config/hdb-volumeattributesclass.yaml @@ -0,0 +1,8 @@ +apiVersion: storage.k8s.io/v1beta1 +kind: VolumeAttributesClass +metadata: + name: csi-gcepd-hdb-vac +driverName: pd.csi.storage.gke.io +parameters: + iops: "3600" + throughput: "290" \ No newline at end of file diff --git a/test/k8s-integration/config/sc-hdb.yaml b/test/k8s-integration/config/sc-hdb.yaml new file mode 100644 index 000000000..94572f3e2 --- /dev/null +++ b/test/k8s-integration/config/sc-hdb.yaml @@ -0,0 +1,8 @@ +apiVersion: storage.k8s.io/v1 +kind: StorageClass +metadata: + name: csi-gcepd-hdb +provisioner: pd.csi.storage.gke.io +parameters: + type: hyperdisk-balanced +volumeBindingMode: WaitForFirstConsumer diff --git a/test/k8s-integration/config/test-config-template.in b/test/k8s-integration/config/test-config-template.in index a70686c59..05338fbed 100644 --- a/test/k8s-integration/config/test-config-template.in +++ b/test/k8s-integration/config/test-config-template.in @@ -4,13 +4,17 @@ StorageClass: SnapshotClass: FromFile: {{ .SnapshotClassFile }} {{end}} +{{if .VolumeAttributesClassFile }} +VolumeAttributesClass: + FromFile: {{ .VolumeAttributesClassFile }} +{{end}} {{if .Timeouts}} Timeouts: {{ range $key, $value := .Timeouts }}{{ $key }}: {{ $value }} {{ end }} {{end}} DriverInfo: - Name: csi-gcepd-{{.StorageClass}}--{{.SnapshotClass}} + Name: csi-gcepd-{{.StorageClass}}--{{.SnapshotClass}}--{{.VolumeAttributesClass}} SupportedFsType: {{range .SupportedFsType}} {{ . }}: {{end}} diff --git a/test/k8s-integration/driver-config.go b/test/k8s-integration/driver-config.go index 25cdef47d..b5e16892b 100644 --- a/test/k8s-integration/driver-config.go +++ b/test/k8s-integration/driver-config.go @@ -10,15 +10,17 @@ import ( ) type driverConfig struct { - StorageClassFile string - StorageClass string - SnapshotClassFile string - SnapshotClass string - Capabilities []string - SupportedFsType []string - MinimumVolumeSize string - NumAllowedTopologies int - Timeouts map[string]string + StorageClassFile string + StorageClass string + SnapshotClassFile string + SnapshotClass string + VolumeAttributesClassFile string + VolumeAttributesClass string + Capabilities []string + SupportedFsType []string + MinimumVolumeSize string + NumAllowedTopologies int + Timeouts map[string]string } const ( @@ -124,17 +126,28 @@ func generateDriverConfigFile(testParams *testParameters) (string, error) { absSnapshotClassFilePath = filepath.Join(testParams.pkgDir, testConfigDir, testParams.snapshotClassFile) snapshotClassName = testParams.snapshotClassFile[:strings.LastIndex(testParams.snapshotClassFile, ".")] } else { - snapshotClassName = "no-volumesnapshotclass" + snapshotClassName = "no-vsc" + } + + var absVacFilePath string + var vacName string + if testParams.volumeAttributesClassFile != "" { + absVacFilePath = filepath.Join(testParams.pkgDir, testConfigDir, testParams.volumeAttributesClassFile) + vacName = testParams.volumeAttributesClassFile[:strings.LastIndex(testParams.volumeAttributesClassFile, ".")] + } else { + vacName = "no-vac" } if !strings.Contains(testParams.storageClassFile, "sc-extreme") { caps = append(caps, "pvcDataSource") } - minimumVolumeSize := "5Gi" + minimumVolumeSize := "10Gi" numAllowedTopologies := 1 if testParams.storageClassFile == regionalPDStorageClass { minimumVolumeSize = "200Gi" numAllowedTopologies = 2 + } else if len(testParams.volumeAttributesClassFile) > 0 { + minimumVolumeSize = "100Gi" } timeouts := map[string]string{ dataSourceProvisionTimeoutKey: dataSourceProvisionTimeout, @@ -142,15 +155,17 @@ func generateDriverConfigFile(testParams *testParameters) (string, error) { extLoc := strings.LastIndex(testParams.storageClassFile, ".") scName := testParams.storageClassFile[:extLoc] params := driverConfig{ - StorageClassFile: filepath.Join(testParams.pkgDir, testConfigDir, testParams.storageClassFile), - StorageClass: scName, - SnapshotClassFile: absSnapshotClassFilePath, - SnapshotClass: snapshotClassName, - SupportedFsType: fsTypes, - Capabilities: caps, - MinimumVolumeSize: minimumVolumeSize, - NumAllowedTopologies: numAllowedTopologies, - Timeouts: timeouts, + StorageClassFile: filepath.Join(testParams.pkgDir, testConfigDir, testParams.storageClassFile), + StorageClass: scName, + SnapshotClassFile: absSnapshotClassFilePath, + SnapshotClass: snapshotClassName, + VolumeAttributesClassFile: absVacFilePath, + VolumeAttributesClass: vacName, + SupportedFsType: fsTypes, + Capabilities: caps, + MinimumVolumeSize: minimumVolumeSize, + NumAllowedTopologies: numAllowedTopologies, + Timeouts: timeouts, } // Write config file diff --git a/test/k8s-integration/main.go b/test/k8s-integration/main.go index b23815e3e..90f954453 100644 --- a/test/k8s-integration/main.go +++ b/test/k8s-integration/main.go @@ -44,6 +44,7 @@ var ( kubeVersion = flag.String("kube-version", "", "version of Kubernetes to download and use for the cluster") testVersion = flag.String("test-version", "", "version of Kubernetes to download and use for tests") kubeFeatureGates = flag.String("kube-feature-gates", "", "feature gates to set on new kubernetes cluster") + kubeRuntimeConfig = flag.String("kube-runtime-config", "", "runtime config to set on new kubernetes cluster") localK8sDir = flag.String("local-k8s-dir", "", "local prebuilt kubernetes/kubernetes directory to use for cluster and test binaries") deploymentStrat = flag.String("deployment-strategy", "gce", "choose between deploying on gce or gke") gkeClusterVer = flag.String("gke-cluster-version", "", "version of Kubernetes master and node for gke") @@ -60,6 +61,7 @@ var ( boskosResourceType = flag.String("boskos-resource-type", "gce-project", "name of the boskos resource type to reserve") storageClassFiles = flag.String("storageclass-files", "", "name of storageclass yaml file to use for test relative to test/k8s-integration/config. This may be a comma-separated list to test multiple storage classes") snapshotClassFiles = flag.String("snapshotclass-files", "", "name of snapshotclass yaml file to use for test relative to test/k8s-integration/config. This may be a comma-separated list to test multiple storage classes") + vacFiles = flag.String("volumeattributesclass-files", "", "name of volumeattributesclass yaml file to use for test relative to test/k8s-integration/config. This may be a comma-separated list to test multiple volumeattributesclasses.") inProw = flag.Bool("run-in-prow", false, "is the test running in PROW") // Driver flags @@ -90,25 +92,26 @@ const ( ) type testParameters struct { - platform string - stagingVersion string - goPath string - pkgDir string - testParentDir string - k8sSourceDir string - testFocus string - testSkip string - storageClassFile string - snapshotClassFile string - cloudProviderArgs []string - deploymentStrategy string - outputDir string - allowedNotReadyNodes int - useGKEManagedDriver bool - clusterVersion string - nodeVersion string - imageType string - parallel int + platform string + stagingVersion string + goPath string + pkgDir string + testParentDir string + k8sSourceDir string + testFocus string + testSkip string + storageClassFile string + snapshotClassFile string + volumeAttributesClassFile string + cloudProviderArgs []string + deploymentStrategy string + outputDir string + allowedNotReadyNodes int + useGKEManagedDriver bool + clusterVersion string + nodeVersion string + imageType string + parallel int } func init() { @@ -530,6 +533,7 @@ func handle() error { if len(*storageClassFiles) != 0 { applicableStorageClassFiles := []string{} applicableSnapshotClassFiles := []string{} + applicableVacFiles := []string{} for _, rawScFile := range strings.Split(*storageClassFiles, ",") { scFile := strings.TrimSpace(rawScFile) if len(scFile) == 0 { @@ -553,6 +557,12 @@ func handle() error { applicableSnapshotClassFiles = append(applicableSnapshotClassFiles, snapshotClassFile) } } + for _, rawVacFile := range strings.Split(*vacFiles, ",") { + vacFile := strings.TrimSpace(rawVacFile) + if len(vacFile) != 0 { + applicableVacFiles = append(applicableVacFiles, vacFile) + } + } var ginkgoErrors []string var testOutputDirs []string @@ -566,6 +576,18 @@ func handle() error { ginkgoErrors = append(ginkgoErrors, err.Error()) } } + // Run volume modify tests + if len(applicableStorageClassFiles) > 0 { + testParams.storageClassFile = applicableStorageClassFiles[0] + for _, vacFile := range applicableVacFiles { + outputDir := strings.TrimSuffix(vacFile, ".yaml") + testOutputDirs = append(testOutputDirs, outputDir) + testParams.volumeAttributesClassFile = vacFile + if err = runCSITests(testParams, outputDir); err != nil { + ginkgoErrors = append(ginkgoErrors, err.Error()) + } + } + } // Run snapshot tests, if there are applicable files, using the first storage class. if len(applicableStorageClassFiles) > 0 { testParams.storageClassFile = applicableStorageClassFiles[0] @@ -619,6 +641,10 @@ func generateGCETestSkip(testParams *testParameters) string { if v.LessThan(apimachineryversion.MustParseSemantic("1.20.0")) { skipString = skipString + "|fsgroupchangepolicy" } + if v.LessThan(apimachineryversion.MustParseSemantic("1.31.0")) { + skipString = skipString + "|VolumeAttributesClass" + } + if testParams.platform == "windows" { skipString = skipString + "|\\[LinuxOnly\\]" } @@ -645,6 +671,11 @@ func generateGKETestSkip(testParams *testParameters) string { skipString = skipString + "|should.provision.correct.filesystem.size.when.restoring.snapshot.to.larger.size.pvc" } + // VolumeAttributesClasses were promoted to beta in 1.31 + if curVer.lessThan(mustParseVersion("1.31.0")) { + skipString = skipString + "|VolumeAttributesClass" + } + // "volumeMode should not mount / map unused volumes in a pod" tests a // (https://github.com/kubernetes/kubernetes/pull/81163) // bug-fix introduced in 1.16 @@ -754,14 +785,30 @@ func runTestsWithConfig(testParams *testParameters, testConfigArg, reportPrefix focus := testParams.testFocus skip := testParams.testSkip + focuses := []string{} + driver := "Driver:\\s*csi-gcepd.*" // If testParams.snapshotClassFile is empty, then snapshot tests will be automatically skipped. Otherwise confirm // the right tests are run. if testParams.snapshotClassFile != "" && strings.Contains(skip, "VolumeSnapshotDataSource") { return fmt.Errorf("Snapshot class file %s specified, but snapshot tests are skipped: %s", testParams.snapshotClassFile, skip) } if testParams.snapshotClassFile != "" { - // Run exactly the snapshot tests, if there is a snapshot class file. - focus = "Driver:\\s*csi-gcepd.*Feature:VolumeSnapshotDataSource" + // If there is a snapshot class file, run snapshot tests + focuses = append(focuses, "VolumeSnapshotDataSource") + } + + // If testParams.volumeAttributesClassFile is empty, then VAC tests will be automatically skipped. Otherwise confirm + // the right tests are run. + if testParams.volumeAttributesClassFile != "" && strings.Contains(skip, "VolumeAttributesClass") { + return fmt.Errorf("VolumeAttributesClass file %s specified, but VolumeAttributesClass tests are skipped: %s", testParams.volumeAttributesClassFile, skip) + } + if testParams.volumeAttributesClassFile != "" { + // If there is a VolumeAttributesClass file, run VAC tests + focuses = append(focuses, "VolumeAttributesClass") + } + + if len(focuses) > 0 { + focus = fmt.Sprintf("%sFeature:(%s)", driver, strings.Join(focuses, "|")) } ginkgoArgs := fmt.Sprintf("--ginkgo.focus=%s --ginkgo.skip=%s", focus, skip) diff --git a/test/remote/client-wrappers.go b/test/remote/client-wrappers.go index 671cbb1df..15c8ba5a6 100644 --- a/test/remote/client-wrappers.go +++ b/test/remote/client-wrappers.go @@ -317,3 +317,12 @@ func (c *CsiClient) DeleteSnapshot(snapshotID string) error { _, err := c.ctrlClient.DeleteSnapshot(context.Background(), dsr) return err } + +func (c *CsiClient) ControllerModifyVolume(volId string, mutableParameters map[string]string) error { + cmvr := &csipb.ControllerModifyVolumeRequest{ + VolumeId: volId, + MutableParameters: mutableParameters, + } + _, err := c.ctrlClient.ControllerModifyVolume(context.Background(), cmvr) + return err +} diff --git a/test/run-k8s-integration.sh b/test/run-k8s-integration.sh index 03813ffb2..160cea662 100755 --- a/test/run-k8s-integration.sh +++ b/test/run-k8s-integration.sh @@ -47,7 +47,9 @@ base_cmd="${PKGDIR}/bin/k8s-integration-test \ --run-in-prow=true --service-account-file=${E2E_GOOGLE_APPLICATION_CREDENTIALS} \ --do-driver-build=${do_driver_build} --teardown-driver=${teardown_driver} \ --do-k8s-build=${do_k8s_build} --boskos-resource-type=${boskos_resource_type} \ - --storageclass-files=sc-standard.yaml --snapshotclass-files=pd-volumesnapshotclass.yaml \ + --storageclass-files=sc-hdb.yaml --snapshotclass-files=pd-volumesnapshotclass.yaml \ + --volumeattributesclass-files=hdb-volumeattributesclass.yaml \ + --kube-runtime-config=api/all=true \ --deployment-strategy=${deployment_strategy} --test-version=${test_version} \ --num-nodes=3 --image-type=${image_type} --use-kubetest2=${use_kubetest2}" @@ -64,7 +66,7 @@ if [ "$deployment_strategy" = "gke" ]; then base_cmd="${base_cmd} --gke-cluster-version=${gke_cluster_version}" fi else - base_cmd="${base_cmd} --kube-version=${kube_version}" + base_cmd="${base_cmd} --kube-version=${kube_version} --kube-feature-gates=VolumeAttributesClass=true" fi if [ -z "$gce_region" ]; then diff --git a/test/sanity/sanity_test.go b/test/sanity/sanity_test.go index 2bf0cf089..8086eb251 100644 --- a/test/sanity/sanity_test.go +++ b/test/sanity/sanity_test.go @@ -65,13 +65,17 @@ func TestSanity(t *testing.T) { enableStoragePools := false multiZoneVolumeHandleConfig := driver.MultiZoneVolumeHandleConfig{} listVolumesConfig := driver.ListVolumesConfig{} + provisionableDisksConfig := driver.ProvisionableDisksConfig{ + SupportsIopsChange: []string{"hyperdisk-balanced", "hyperdisk-extreme"}, + SupportsThroughputChange: []string{"hyperdisk-balanced", "hyperdisk-throughput", "hyperdisk-ml"}, + } mounter := mountmanager.NewFakeSafeMounter() deviceUtils := deviceutils.NewFakeDeviceUtils(true) //Initialize GCE Driver identityServer := driver.NewIdentityServer(gceDriver) - controllerServer := driver.NewControllerServer(gceDriver, cloudProvider, 0, 5*time.Minute, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig) + controllerServer := driver.NewControllerServer(gceDriver, cloudProvider, 0, 5*time.Minute, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig) fakeStatter := mountmanager.NewFakeStatterWithOptions(mounter, mountmanager.FakeStatterOptions{IsBlock: false}) nodeServer := driver.NewNodeServer(gceDriver, mounter, deviceUtils, metadataservice.NewFakeService(), fakeStatter) err = gceDriver.SetupGCEDriver(driverName, vendorVersion, extraLabels, nil, identityServer, controllerServer, nodeServer) @@ -112,6 +116,12 @@ func TestSanity(t *testing.T) { DialOptions: []grpc.DialOption{grpc.WithInsecure()}, IDGen: newPDIDGenerator(project, zone), TestVolumeSize: common.GbToBytes(200), + TestVolumeParameters: map[string]string{ + common.ParameterKeyType: "hyperdisk-balanced", + common.ParameterKeyProvisionedIOPSOnCreate: "3000", + common.ParameterKeyProvisionedThroughputOnCreate: "150Mi", + }, + TestVolumeMutableParameters: map[string]string{"iops": "3013", "throughput": "151"}, } sanity.Test(t, config) } diff --git a/vendor/k8s.io/apimachinery/pkg/util/clock/clock.go b/vendor/k8s.io/apimachinery/pkg/util/clock/clock.go deleted file mode 100644 index ff97612df..000000000 --- a/vendor/k8s.io/apimachinery/pkg/util/clock/clock.go +++ /dev/null @@ -1,86 +0,0 @@ -/* -Copyright 2014 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package clock - -import ( - "time" - - clocks "k8s.io/utils/clock" - testclocks "k8s.io/utils/clock/testing" -) - -// PassiveClock allows for injecting fake or real clocks into code -// that needs to read the current time but does not support scheduling -// activity in the future. -// -// Deprecated: Use k8s.io/utils/clock.PassiveClock instead. -type PassiveClock = clocks.PassiveClock - -// Clock allows for injecting fake or real clocks into code that -// needs to do arbitrary things based on time. -// -// Deprecated: Use k8s.io/utils/clock.WithTickerAndDelayedExecution instead. -type Clock = clocks.WithTickerAndDelayedExecution - -// Deprecated: Use k8s.io/utils/clock.RealClock instead. -type RealClock = clocks.RealClock - -// FakePassiveClock implements PassiveClock, but returns an arbitrary time. -// -// Deprecated: Use k8s.io/utils/clock/testing.FakePassiveClock instead. -type FakePassiveClock = testclocks.FakePassiveClock - -// FakeClock implements Clock, but returns an arbitrary time. -// -// Deprecated: Use k8s.io/utils/clock/testing.FakeClock instead. -type FakeClock = testclocks.FakeClock - -// NewFakePassiveClock returns a new FakePassiveClock. -// -// Deprecated: Use k8s.io/utils/clock/testing.NewFakePassiveClock instead. -func NewFakePassiveClock(t time.Time) *testclocks.FakePassiveClock { - return testclocks.NewFakePassiveClock(t) -} - -// NewFakeClock returns a new FakeClock. -// -// Deprecated: Use k8s.io/utils/clock/testing.NewFakeClock instead. -func NewFakeClock(t time.Time) *testclocks.FakeClock { - return testclocks.NewFakeClock(t) -} - -// IntervalClock implements Clock, but each invocation of Now steps -// the clock forward the specified duration. -// -// WARNING: most of the Clock methods just `panic`; -// only PassiveClock is honestly implemented. -// The alternative, SimpleIntervalClock, has only the -// PassiveClock methods. -// -// Deprecated: Use k8s.io/utils/clock/testing.SimpleIntervalClock instead. -type IntervalClock = testclocks.IntervalClock - -// Timer allows for injecting fake or real timers into code that -// needs to do arbitrary things based on time. -// -// Deprecated: Use k8s.io/utils/clock.Timer instead. -type Timer = clocks.Timer - -// Ticker defines the Ticker interface. -// -// Deprecated: Use k8s.io/utils/clock.Ticker instead. -type Ticker = clocks.Ticker diff --git a/vendor/modules.txt b/vendor/modules.txt index 09f9ef522..1567e34b1 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -717,7 +717,6 @@ k8s.io/apimachinery/pkg/runtime/serializer/streaming k8s.io/apimachinery/pkg/runtime/serializer/versioning k8s.io/apimachinery/pkg/selection k8s.io/apimachinery/pkg/types -k8s.io/apimachinery/pkg/util/clock k8s.io/apimachinery/pkg/util/errors k8s.io/apimachinery/pkg/util/framer k8s.io/apimachinery/pkg/util/intstr