UPSTREAM: <carry>: Changes to support hyperdisk multi-writer mode

Upstream PR: kubernetes-sigs#1919

Added fixes for test key handling.

Multiwriter Test Update (openshift#3)

* Changes update the tests to use two contexts, one for multiwriter and one for the existing tests. This was deemed necessary as only some disks can support multi-writer, and only some VM shapes can support said disks.

Update parameters.go

Fixing a linting issue.

Karkunpavan (openshift#4)

* Removing alpha disk from tests.

* Changes setup the test to run with hyperdisk extreme.

* Updates the disk type back to balanced, as HDX doesn't support multi writer.

* Moving over to m1 megamem as thats the only type of machine that can support all needed disk types.

* Changes update the tests to use two contexts, one for multiwriter and one for the existing tests. This was deemed necessary as only some disks can support multi-writer, and only some VM shapes can support said disks.

* Changes update the tests to use two contexts, one for multiwriter and one for the existing tests. This was deemed necessary as only some disks can support multi-writer, and only some VM shapes can support said disks.

* Fixing some git oddness

* Fixing some formatting.

* More formatting fixes.

* Hopefully last changes for formatting.

* Fixing linting issue.

* Cleaning up un-used / un-needed GetMultiWriter test function.

Completley removing the multiwriter check from fake-gce.

Changes remove the API version concept from GetDisk and InsertDisk, now just defaults to using the Beta API.

Cleaning up un-used variables.

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

Addressing comments around GetMultiWriter checks.

Removing multi-writer as a disk paramter.

Fixing rebasing issues.

Merge branch 'master' into followup

Merge branch 'kubernetes-sigs:master' into hd-mw

Merge branch 'kubernetes-sigs:master' into hd-mw

Merge pull request openshift#1 from karkunpavan/karkunpavan

Added fixes for test key handling.

Author: karkunpavan

cmd/gce-pd-csi-driver/main.go

@@ -72,6 +72,7 @@ var (
 	formatAndMountTimeout       = flag.Duration("format-and-mount-timeout", 1*time.Minute, "The maximum duration of a format and mount operation before another such operation will be started. Used only if --serialize-format-and-mount")
 	fallbackRequisiteZonesFlag  = flag.String("fallback-requisite-zones", "", "Comma separated list of requisite zones that will be used if there are not sufficient zones present in requisite topologies when provisioning a disk")
 	enableStoragePoolsFlag      = flag.Bool("enable-storage-pools", false, "If set to true, the CSI Driver will allow volumes to be provisioned in Storage Pools")
+	enableHdHAFlag              = flag.Bool("allow-hdha-provisioning", false, "If set to true, will allow the driver to provision Hyperdisk-balanced High Availability disks")
 
 	multiZoneVolumeHandleDiskTypesFlag = flag.String("multi-zone-volume-handle-disk-types", "", "Comma separated list of allowed disk types that can use the multi-zone volumeHandle. Used only if --multi-zone-volume-handle-enable")
 	multiZoneVolumeHandleEnableFlag    = flag.Bool("multi-zone-volume-handle-enable", false, "If set to true, the multi-zone volumeHandle feature will be enabled")
@@ -220,7 +221,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, provisionableDisksConfig)
+		controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones, *enableStoragePoolsFlag, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig, *enableHdHAFlag)
 	} else if *cloudConfigFilePath != "" {
 		klog.Warningf("controller service is disabled but cloud config given - it has no effect")
 	}

deploy/kubernetes/images/prow-stable-sidecar-rc-master/image.yaml

@@ -48,6 +48,6 @@ metadata:
 imageTag:
   name: registry.k8s.io/cloud-provider-gcp/gcp-compute-persistent-disk-csi-driver
   newName: gcr.io/k8s-staging-cloud-provider-gcp/gcp-compute-persistent-disk-csi-driver
-  newTag: "v1.14.2-rc1"
+  newTag: "v1.15.4-rc1"
 ---
 

pkg/common/parameters.go

@@ -22,6 +22,9 @@ import (
 )
 
 const (
+	// Disk Params
+	ParameterAccessMode						  = "access-mode"
+
 	// Parameters for StorageClass
 	ParameterKeyType                          = "type"
 	ParameterKeyReplicationType               = "replication-type"
@@ -34,6 +37,7 @@ const (
 	ParameterKeyStoragePools                  = "storage-pools"
 	ParameterKeyResourceTags                  = "resource-tags"
 	ParameterKeyEnableMultiZoneProvisioning   = "enable-multi-zone-provisioning"
+	ParameterHdHADiskType                     = "hyperdisk-balanced-high-availability"
 
 	// Parameters for VolumeSnapshotClass
 	ParameterKeyStorageLocations = "storage-locations"
@@ -106,6 +110,13 @@ type DiskParameters struct {
 	// Values: {bool}
 	// Default: false
 	MultiZoneProvisioning bool
+	// Values: READ_WRITE_SINGLE, READ_ONLY_MANY, READ_WRITE_MANY
+	// Default: READ_WRITE_SINGLE
+	AccessMode string
+}
+
+func (dp *DiskParameters) IsRegional() bool {
+	return dp.ReplicationType == "regional-pd" || dp.DiskType == ParameterHdHADiskType
 }
 
 // SnapshotParameters contains normalized and defaulted parameters for snapshots
@@ -129,6 +140,7 @@ type ParameterProcessor struct {
 	DriverName         string
 	EnableStoragePools bool
 	EnableMultiZone    bool
+	EnableHdHA         bool
 }
 
 type ModifyVolumeParameters struct {
@@ -167,6 +179,9 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
 		case ParameterKeyType:
 			if v != "" {
 				p.DiskType = strings.ToLower(v)
+				if !pp.EnableHdHA && p.DiskType == ParameterHdHADiskType {
+					return p, fmt.Errorf("parameters contain invalid disk type %s", ParameterHdHADiskType)
+				}
 			}
 		case ParameterKeyReplicationType:
 			if v != "" {
@@ -201,6 +216,9 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
 			if err != nil {
 				return p, fmt.Errorf("parameters contain invalid provisionedThroughputOnCreate parameter: %w", err)
 			}
+			if paramProvisionedThroughputOnCreate < 0 {
+				return p, fmt.Errorf("parameter provisionedThroughputOnCreate cannot be negative")
+			}
 			p.ProvisionedThroughputOnCreate = paramProvisionedThroughputOnCreate
 		case ParameterAvailabilityClass:
 			paramAvailabilityClass, err := ConvertStringToAvailabilityClass(v)
@@ -250,6 +268,10 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
 			if paramEnableMultiZoneProvisioning {
 				p.Labels[MultiZoneLabel] = "true"
 			}
+		case ParameterAccessMode:
+			if v != "" {
+				p.AccessMode = v
+			}
 		default:
 			return p, fmt.Errorf("parameters contains invalid option %q", k)
 		}

pkg/common/parameters_test.go

@@ -30,6 +30,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 		labels             map[string]string
 		enableStoragePools bool
 		enableMultiZone    bool
+		enableHdHA         bool
 		extraTags          map[string]string
 		expectParams       DiskParameters
 		expectErr          bool
@@ -344,6 +345,13 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 			labels:             map[string]string{},
 			expectErr:          true,
 		},
+		{
+			name:               "invalid storage pool parameters, negative ProvisionedThroughputOnCreate",
+			enableStoragePools: true,
+			parameters:         map[string]string{ParameterKeyType: "hyperdisk-throughput", ParameterKeyReplicationType: "none", ParameterKeyDiskEncryptionKmsKey: "foo/key", ParameterKeyLabels: "key1=value1,key2=value2", ParameterKeyResourceTags: "parent1/key1/value1,parent2/key2/value2", ParameterKeyProvisionedThroughputOnCreate: "-50Mi"},
+			labels:             map[string]string{},
+			expectErr:          true,
+		},
 		{
 			name:               "storage pool parameters, enableStoragePools is false",
 			enableStoragePools: false,
@@ -388,6 +396,23 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 			parameters: map[string]string{ParameterKeyType: "hyperdisk-ml", ParameterKeyEnableMultiZoneProvisioning: "true"},
 			expectErr:  true,
 		},
+		{
+			name:       "disk parameters, hdha disabled",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced-high-availability"},
+			expectErr:  true,
+		},
+		{
+			name:       "disk parameters, hdha enabled",
+			parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced-high-availability"},
+			enableHdHA: true,
+			expectParams: DiskParameters{
+				DiskType:        "hyperdisk-balanced-high-availability",
+				ReplicationType: "none",
+				Tags:            map[string]string{},
+				ResourceTags:    map[string]string{},
+				Labels:          map[string]string{},
+			},
+		},
 	}
 
 	for _, tc := range tests {
@@ -396,6 +421,7 @@ func TestExtractAndDefaultParameters(t *testing.T) {
 				DriverName:         "testDriver",
 				EnableStoragePools: tc.enableStoragePools,
 				EnableMultiZone:    tc.enableMultiZone,
+				EnableHdHA:         tc.enableHdHA,
 			}
 			p, err := pp.ExtractAndDefaultParameters(tc.parameters, tc.labels, tc.extraTags)
 			if gotErr := err != nil; gotErr != tc.expectErr {

pkg/common/utils_test.go

@@ -1409,7 +1409,7 @@ func TestIsUserMultiAttachError(t *testing.T) {
 		},
 	}
 	for _, test := range cases {
-		code, err := isUserMultiAttachError(fmt.Errorf(test.errorString))
+		code, err := isUserMultiAttachError(fmt.Errorf("%v", test.errorString))
 		if test.expectCode {
 			if err != nil || code != test.expectedCode {
 				t.Errorf("Failed with non-nil error %v or bad code %v: %s", err, code, test.errorString)

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

@@ -166,6 +166,17 @@ func (d *CloudDisk) GetZone() string {
 	}
 }
 
+func (d *CloudDisk) GetRegion() string {
+	switch {
+	case d.disk != nil:
+		return d.disk.Region
+	case d.betaDisk != nil:
+		return d.betaDisk.Region
+	default:
+		return ""
+	}
+}
+
 func (d *CloudDisk) GetSnapshotId() string {
 	switch {
 	case d.disk != nil:

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

@@ -79,6 +79,10 @@ func CreateFakeCloudProvider(project, zone string, cloudDisks []*CloudDisk) (*Fa
 		mockDiskStatus: "READY",
 	}
 	for _, d := range cloudDisks {
+		if d.LocationType() == meta.Regional {
+			fcp.disks[meta.RegionalKey(d.GetName(), d.GetRegion()).String()] = d
+			continue
+		}
 		diskZone := d.GetZone()
 		if diskZone == "" {
 			diskZone = zone
@@ -184,7 +188,7 @@ func (cloud *FakeCloudProvider) ListSnapshots(ctx context.Context, filter string
 }
 
 // Disk Methods
-func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, project string, volKey *meta.Key, api GCEAPIVersion) (*CloudDisk, error) {
+func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, project string, volKey *meta.Key) (*CloudDisk, error) {
 	disk, ok := cloud.disks[volKey.String()]
 	if !ok {
 		return nil, notFoundError()
@@ -216,7 +220,6 @@ func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp *
 	if multiWriter && !resp.GetMultiWriter() {
 		return fmt.Errorf("disk already exists with incompatible capability. Need MultiWriter. Got non-MultiWriter")
 	}
-
 	klog.V(4).Infof("Compatible disk already exists")
 	return ValidateDiskParameters(resp, params)
 }