Skip to content

Commit 592cb33

Browse files
travisyxtravisyx
committed
Cleanup ControllerModifyVolume logic and add upstream e2e tests
1 parent e58a2b2 commit 592cb33

21 files changed

+288
-93
lines changed

‎deploy/kubernetes/base/controller/cluster_setup.yaml

+1-15
Original file line numberDiff line numberDiff line change
@@ -318,18 +318,4 @@ subjects:
318318
roleRef:
319319
kind: Role
320320
name: csi-gce-pd-leaderelection-role
321-
apiGroup: rbac.authorization.k8s.io
322-
323-
---
324-
apiVersion: rbac.authorization.k8s.io/v1
325-
kind: ClusterRoleBinding
326-
metadata:
327-
name: csi-gce-pd-controller-sa-cluster-admin
328-
subjects:
329-
- kind: ServiceAccount
330-
name: csi-gce-pd-controller-sa
331-
namespace: gce-pd-csi-driver
332-
roleRef:
333-
kind: ClusterRole
334-
name: cluster-admin
335-
apiGroup: rbac.authorization.k8s.io
321+
apiGroup: rbac.authorization.k8s.io

‎deploy/kubernetes/base/controller/controller.yaml

+2-2
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ spec:
3737
- "--leader-election"
3838
- "--default-fstype=ext4"
3939
- "--controller-publish-readonly=true"
40+
- "--feature-gates=VolumeAttributesClass=true"
4041
env:
4142
- name: PDCSI_NAMESPACE
4243
valueFrom:
@@ -95,6 +96,7 @@ spec:
9596
- "--leader-election"
9697
- "--leader-election-namespace=$(PDCSI_NAMESPACE)"
9798
- "--handle-volume-inuse-error=false"
99+
- "--feature-gates=VolumeAttributesClass=true"
98100
env:
99101
- name: PDCSI_NAMESPACE
100102
valueFrom:
@@ -139,8 +141,6 @@ spec:
139141
args:
140142
- "--v=5"
141143
- "--endpoint=unix:/csi/csi.sock"
142-
- "--supports-dynamic-iops-provisioning=hyperdisk-balanced,hyperdisk-extreme"
143-
- "--supports-dynamic-throughput-provisioning=hyperdisk-balanced,hyperdisk-throughput,hyperdisk-ml"
144144
env:
145145
- name: GOOGLE_APPLICATION_CREDENTIALS
146146
value: "/etc/cloud-sa/cloud-sa.json"

‎deploy/kubernetes/images/stable-master/image.yaml

+1-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ metadata:
44
name: imagetag-csi-provisioner
55
imageTag:
66
name: registry.k8s.io/sig-storage/csi-provisioner
7-
newTag: "v3.6.3"
7+
newTag: "v5.1.0"
88

99
---
1010
apiVersion: builtin

‎deploy/kubernetes/overlays/dev/controller_always_pull.yaml

+1-2
Original file line numberDiff line numberDiff line change
@@ -7,5 +7,4 @@ spec:
77
spec:
88
containers:
99
- name: gce-pd-driver
10-
imagePullPolicy: Always
11-
10+
imagePullPolicy: Always

‎deploy/kubernetes/overlays/dev/driver-args.yaml

+7
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
- op: add
2+
path: /spec/template/spec/containers/0/args/-
3+
value: --supports-dynamic-throughput-provisioning=hyperdisk-balanced,hyperdisk-throughput,hyperdisk-ml
4+
5+
- op: add
6+
path: /spec/template/spec/containers/0/args/-
7+
value: --supports-dynamic-iops-provisioning=hyperdisk-balanced,hyperdisk-extreme

‎deploy/kubernetes/overlays/dev/kustomization.yaml

+8
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,14 @@ resources:
99
# Here dev overlay is using the same image as alpha
1010
transformers:
1111
- ../../images/stable-master
12+
# Apply patches to support dynamic provisioning for hyperdisks
13+
patches:
14+
- path: ./driver-args.yaml
15+
target:
16+
group: apps
17+
version: v1
18+
kind: Deployment
19+
name: csi-gce-pd-controller
1220
# To change the dev image, add something like the following.
1321
#images:
1422
#- name: gke.gcr.io/gcp-compute-persistent-disk-csi-driver

‎pkg/common/parameters.go

+2
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,8 @@ func ExtractModifyVolumeParameters(parameters map[string]string) (ModifyVolumePa
348348
return ModifyVolumeParameters{}, fmt.Errorf("parameters contain invalid throughput parameter: %w", err)
349349
}
350350
modifyVolumeParams.Throughput = &throughput
351+
default:
352+
return ModifyVolumeParameters{}, fmt.Errorf("parameters contain unknown parameter: %s", key)
351353
}
352354
}
353355
return modifyVolumeParams, nil

‎pkg/common/parameters_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,6 @@ func TestExtractModifyVolumeParameters(t *testing.T) {
501501
}
502502

503503
if !reflect.DeepEqual(result, expected) {
504-
t.Errorf("Expected: %v, but got: %v", expected, result)
504+
t.Errorf("Got ExtractModifyVolumeParameters(%+v) = %+v; want: %v", parameters, result, expected)
505505
}
506506
}

‎pkg/gce-cloud-provider/compute/fake-gce.go

+16-3
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,12 @@ func (cloud *FakeCloudProvider) ListDisksWithFilter(ctx context.Context, fields
143143
func (cloud *FakeCloudProvider) ListDisks(ctx context.Context, fields []googleapi.Field) ([]*computev1.Disk, string, error) {
144144
d := []*computev1.Disk{}
145145
for _, cd := range cloud.disks {
146-
d = append(d, cd.disk)
146+
if cd.disk != nil {
147+
d = append(d, cd.disk)
148+
} else if cd.betaDisk != nil {
149+
betaDisk := convertBetaDiskToV1Disk(cd.betaDisk)
150+
d = append(d, betaDisk)
151+
}
147152
}
148153
return d, "", nil
149154
}
@@ -292,10 +297,18 @@ func (cloud *FakeCloudProvider) UpdateDisk(ctx context.Context, project string,
292297
}
293298

294299
if params.IOPS != nil {
295-
existingDisk.betaDisk.ProvisionedIops = *params.IOPS
300+
if existingDisk.betaDisk != nil {
301+
existingDisk.betaDisk.ProvisionedIops = *params.IOPS
302+
} else if existingDisk.disk != nil {
303+
existingDisk.disk.ProvisionedIops = *params.IOPS
304+
}
296305
}
297306
if params.Throughput != nil {
298-
existingDisk.betaDisk.ProvisionedThroughput = *params.Throughput
307+
if existingDisk.betaDisk != nil {
308+
existingDisk.betaDisk.ProvisionedThroughput = *params.Throughput
309+
} else if existingDisk.disk != nil {
310+
existingDisk.disk.ProvisionedThroughput = *params.Throughput
311+
}
299312
}
300313
cloud.disks[volKey.String()] = existingDisk
301314
return nil

‎pkg/gce-cloud-provider/compute/gce-compute.go

+69-3
Original file line numberDiff line numberDiff line change
@@ -461,12 +461,12 @@ func (cloud *CloudProvider) InsertDisk(ctx context.Context, project string, volK
461461
}
462462

463463
func (cloud *CloudProvider) UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error {
464+
// hyperdisks are zonal disks
465+
// pd-disks do not support modification of IOPS and Throughput
464466
if volKey.Type() == meta.Regional {
465-
return fmt.Errorf("Cannot update regional disk")
467+
return status.Error(codes.InvalidArgument, "Cannot update regional disk")
466468
}
467469
klog.V(5).Infof("Updating disk %v", volKey)
468-
// hyperdisks are zonal disks
469-
// pd-disks do not support modification of IOPS and Throughput
470470
return cloud.updateZonalDisk(ctx, project, volKey, existingDisk, params)
471471
}
472472

@@ -567,6 +567,72 @@ func convertV1DiskToBetaDisk(v1Disk *computev1.Disk) *computebeta.Disk {
567567
return betaDisk
568568
}
569569

570+
func convertBetaCustomerEncryptionKeyToV1(betaKey *computebeta.CustomerEncryptionKey) *computev1.CustomerEncryptionKey {
571+
return &computev1.CustomerEncryptionKey{
572+
KmsKeyName: betaKey.KmsKeyName,
573+
RawKey: betaKey.RawKey,
574+
Sha256: betaKey.Sha256,
575+
ForceSendFields: betaKey.ForceSendFields,
576+
NullFields: betaKey.NullFields,
577+
}
578+
}
579+
580+
func convertBetaDiskParamsToV1(betaDiskParams *computebeta.DiskParams) *computev1.DiskParams {
581+
resourceManagerTags := make(map[string]string)
582+
for k, v := range betaDiskParams.ResourceManagerTags {
583+
resourceManagerTags[k] = v
584+
}
585+
return &computev1.DiskParams{
586+
ResourceManagerTags: resourceManagerTags,
587+
}
588+
}
589+
func convertBetaDiskToV1Disk(betaDisk *computebeta.Disk) *computev1.Disk {
590+
var dek *computev1.CustomerEncryptionKey = nil
591+
592+
if betaDisk.DiskEncryptionKey != nil {
593+
dek = convertBetaCustomerEncryptionKeyToV1(betaDisk.DiskEncryptionKey)
594+
}
595+
596+
var params *computev1.DiskParams = nil
597+
if betaDisk.Params != nil {
598+
params = convertBetaDiskParamsToV1(betaDisk.Params)
599+
}
600+
601+
// Note: this is an incomplete list. It only includes the fields we use for disk creation.
602+
v1Disk := &computev1.Disk{
603+
Name: betaDisk.Name,
604+
SizeGb: betaDisk.SizeGb,
605+
Description: betaDisk.Description,
606+
Type: betaDisk.Type,
607+
SourceSnapshot: betaDisk.SourceSnapshot,
608+
SourceImage: betaDisk.SourceImage,
609+
SourceImageId: betaDisk.SourceImageId,
610+
SourceSnapshotId: betaDisk.SourceSnapshotId,
611+
SourceDisk: betaDisk.SourceDisk,
612+
ReplicaZones: betaDisk.ReplicaZones,
613+
DiskEncryptionKey: dek,
614+
Zone: betaDisk.Zone,
615+
Region: betaDisk.Region,
616+
Status: betaDisk.Status,
617+
SelfLink: betaDisk.SelfLink,
618+
Params: params,
619+
AccessMode: betaDisk.AccessMode,
620+
}
621+
622+
// Hyperdisk doesn't currently support multiWriter (https://cloud.google.com/compute/docs/disks/hyperdisks#limitations),
623+
// but if multiWriter + hyperdisk is supported in the future, we want the PDCSI driver to support this feature without
624+
// any additional code change.
625+
if betaDisk.ProvisionedIops > 0 {
626+
v1Disk.ProvisionedIops = betaDisk.ProvisionedIops
627+
}
628+
if betaDisk.ProvisionedThroughput > 0 {
629+
v1Disk.ProvisionedThroughput = betaDisk.ProvisionedThroughput
630+
}
631+
v1Disk.StoragePool = betaDisk.StoragePool
632+
633+
return v1Disk
634+
}
635+
570636
func (cloud *CloudProvider) insertRegionalDisk(
571637
ctx context.Context,
572638
project string,

‎pkg/gce-pd-csi-driver/controller.go

+20-13
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,6 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req
324324
return nil, status.Errorf(codes.InvalidArgument, "CreateVolume Request Capacity is invalid: %v", err.Error())
325325
}
326326

327-
// Validate multiwriter
328327
err = validateVolumeCapabilities(volumeCapabilities)
329328
if err != nil {
330329
return nil, status.Errorf(codes.InvalidArgument, "VolumeCapabilities is invalid: %v", err.Error())
@@ -368,6 +367,7 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req
368367
}
369368
}
370369

370+
// Validate multiwriter
371371
if _, err := getMultiWriterFromCapabilities(volumeCapabilities); err != nil {
372372
return nil, status.Errorf(codes.InvalidArgument, "VolumeCapabilities is invalid: %v", err.Error())
373373
}
@@ -778,7 +778,7 @@ func (gceCS *GCEControllerServer) ControllerModifyVolume(ctx context.Context, re
778778
volumeID := req.GetVolumeId()
779779
klog.V(4).Infof("Modifying Volume ID: %s", volumeID)
780780

781-
if volumeID == "" {
781+
if len(volumeID) == 0 {
782782
return nil, status.Error(codes.InvalidArgument, "volume ID must be provided")
783783
}
784784

@@ -787,53 +787,60 @@ func (gceCS *GCEControllerServer) ControllerModifyVolume(ctx context.Context, re
787787
enableStoragePools := metrics.DefaultEnableStoragePools
788788

789789
defer func() {
790-
gceCS.Metrics.RecordOperationErrorMetrics("ModifyVolume", err, diskType, enableConfidentialCompute, enableStoragePools)
790+
gceCS.Metrics.RecordOperationErrorMetrics("ControllerModifyVolume", err, diskType, enableConfidentialCompute, enableStoragePools)
791791
}()
792792

793793
project, volKey, err := common.VolumeIDToKey(volumeID)
794-
795794
if err != nil {
796-
return nil, status.Errorf(codes.InvalidArgument, "volume ID is invalid: %v", err.Error())
795+
// Cannot find volume associated with this ID because VolumeID is not in the correct format
796+
err = status.Errorf(codes.NotFound, "volume ID is invalid: %v", err.Error())
797+
return nil, err
797798
}
798799

799800
volumeModifyParams, err := common.ExtractModifyVolumeParameters(req.GetMutableParameters())
800801
if err != nil {
801802
klog.Errorf("Failed to extract parameters for volume %s: %v", volumeID, err)
802-
return nil, status.Errorf(codes.InvalidArgument, "Invalid parameters: %v", err)
803+
err = status.Errorf(codes.InvalidArgument, "Invalid parameters: %v", err)
804+
return nil, err
803805
}
804806
klog.V(4).Infof("Modify Volume Parameters for %s: %v", volumeID, volumeModifyParams)
805807

806808
existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionBeta)
807809

808810
if err != nil {
809-
return nil, fmt.Errorf("Failed to get volume: %w", err)
811+
err = fmt.Errorf("Failed to get volume: %w", err)
812+
return nil, err
810813
}
811814

812815
if existingDisk == nil || existingDisk.GetSelfLink() == "" {
813-
814-
return nil, status.Errorf(codes.Internal, "failed to get volume : %s", volumeID)
816+
err = status.Errorf(codes.Internal, "failed to get volume : %s", volumeID)
817+
return nil, err
815818
}
816819
diskType = existingDisk.GetPDType()
817820

818821
// Check if the disk supports dynamic IOPS/Throughput provisioning
819822
supportsIopsChange := gceCS.diskSupportsIopsChange(diskType)
820823
supportsThroughputChange := gceCS.diskSupportsThroughputChange(diskType)
821824
if !supportsIopsChange && !supportsThroughputChange {
822-
return nil, status.Errorf(codes.InvalidArgument, "Failed to modify volume: modifications not supported for disk type %s", diskType)
825+
err = status.Errorf(codes.InvalidArgument, "Failed to modify volume: modifications not supported for disk type %s", diskType)
826+
return nil, err
823827
}
824828
if !supportsIopsChange && volumeModifyParams.IOPS != nil {
825-
return nil, status.Errorf(codes.InvalidArgument, "Cannot specify IOPS for disk type %s", diskType)
829+
err = status.Errorf(codes.InvalidArgument, "Cannot specify IOPS for disk type %s", diskType)
830+
return nil, err
826831
}
827832
if !supportsThroughputChange && volumeModifyParams.Throughput != nil {
828-
return nil, status.Errorf(codes.InvalidArgument, "Cannot specify throughput for disk type %s", diskType)
833+
err = status.Errorf(codes.InvalidArgument, "Cannot specify throughput for disk type %s", diskType)
834+
return nil, err
829835
}
830836

831837
enableStoragePools = strconv.FormatBool(existingDisk.GetEnableStoragePools())
832838

833839
err = gceCS.CloudProvider.UpdateDisk(ctx, project, volKey, existingDisk, volumeModifyParams)
834840
if err != nil {
835841
klog.Errorf("Failed to modify volume %s: %v", volumeID, err)
836-
return nil, fmt.Errorf("Failed to modify volume %s: %w", volumeID, err)
842+
err = fmt.Errorf("Failed to modify volume %s: %w", volumeID, err)
843+
return nil, err
837844
}
838845

839846
return &csi.ControllerModifyVolumeResponse{}, nil

‎pkg/gce-pd-csi-driver/controller_test.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -1780,7 +1780,7 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) {
17801780
Parameters: map[string]string{
17811781
common.ParameterKeyType: "hyperdisk-balanced",
17821782
common.ParameterKeyProvisionedIOPSOnCreate: "10000",
1783-
common.ParameterKeyProvisionedThroughputOnCreate: "500",
1783+
common.ParameterKeyProvisionedThroughputOnCreate: "500Mi",
17841784
},
17851785
AccessibilityRequirements: &csi.TopologyRequirement{
17861786
Preferred: []*csi.Topology{
@@ -1813,7 +1813,7 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) {
18131813
Parameters: map[string]string{
18141814
common.ParameterKeyType: "pd-ssd",
18151815
common.ParameterKeyProvisionedIOPSOnCreate: "10000",
1816-
common.ParameterKeyProvisionedThroughputOnCreate: "500",
1816+
common.ParameterKeyProvisionedThroughputOnCreate: "500Mi",
18171817
},
18181818
AccessibilityRequirements: &csi.TopologyRequirement{
18191819
Preferred: []*csi.Topology{
@@ -2036,7 +2036,7 @@ func TestVolumeModifyErrorHandling(t *testing.T) {
20362036
Parameters: map[string]string{
20372037
common.ParameterKeyType: "hyperdisk-balanced",
20382038
common.ParameterKeyProvisionedIOPSOnCreate: "3000",
2039-
common.ParameterKeyProvisionedThroughputOnCreate: "150",
2039+
common.ParameterKeyProvisionedThroughputOnCreate: "150Mi",
20402040
},
20412041
VolumeCapabilities: stdVolCaps,
20422042
AccessibilityRequirements: &csi.TopologyRequirement{
@@ -2072,7 +2072,7 @@ func TestVolumeModifyErrorHandling(t *testing.T) {
20722072
Parameters: map[string]string{
20732073
common.ParameterKeyType: "hyperdisk-balanced",
20742074
common.ParameterKeyProvisionedIOPSOnCreate: "3000",
2075-
common.ParameterKeyProvisionedThroughputOnCreate: "150",
2075+
common.ParameterKeyProvisionedThroughputOnCreate: "150Mi",
20762076
},
20772077
VolumeCapabilities: stdVolCaps,
20782078
AccessibilityRequirements: &csi.TopologyRequirement{

‎test/e2e/tests/single_zone_e2e_test.go

-4
Original file line numberDiff line numberDiff line change
@@ -1775,7 +1775,3 @@ func merge(a, b map[string]string) map[string]string {
17751775
}
17761776
return res
17771777
}
1778-
1779-
func stringPtr(str string) *string {
1780-
return &str
1781-
}

‎test/k8s-integration/cluster.go

+18
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,24 @@ func clusterUpGCE(k8sDir, gceZone string, numNodes int, numWindowsNodes int, ima
9090
klog.V(4).Infof("Set Kubernetes feature gates: %v", *kubeFeatureGates)
9191
}
9292

93+
if len(*kubeRuntimeConfig) != 0 {
94+
err = os.Setenv("KUBE_RUNTIME_CONFIG", *kubeRuntimeConfig)
95+
if err != nil {
96+
return fmt.Errorf("failed to set kubernetes runtime config: %w", err)
97+
}
98+
klog.V(4).Infof("Set Kubernetes runtime config: %v", *kubeRuntimeConfig)
99+
// If runtime config is set, we will update the machine type to support hyperdisks
100+
err = os.Setenv("NODE_SIZE", "c3-standard-4")
101+
if err != nil {
102+
return fmt.Errorf("failed to set NODE_SIZE: %w", err)
103+
}
104+
// The node disk type also needs to be updated
105+
err = os.Setenv("NODE_DISK_TYPE", "pd-ssd")
106+
if err != nil {
107+
return fmt.Errorf("failed to set NODE_DISK_TYPE: %w", err)
108+
}
109+
}
110+
93111
err = setImageTypeEnvs(imageType)
94112
if err != nil {
95113
return fmt.Errorf("failed to set image type environment variables: %w", err)

0 commit comments

Comments
 (0)