UPSTREAM: <carry>: OCPBUGS-44769: Fix RWX mode for hyperdisks

Based on kubernetes-sigs#1901

Author: cemakd

pkg/common/parameters.go

@@ -22,6 +22,9 @@ import (
 )
 
 const (
+	// Disk Params
+	ParameterAccessMode						  = "access-mode"
+
 	// Parameters for StorageClass
 	ParameterKeyType                          = "type"
 	ParameterKeyReplicationType               = "replication-type"
@@ -106,6 +109,9 @@ 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
 }
 
 // SnapshotParameters contains normalized and defaulted parameters for snapshots
@@ -201,6 +207,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 +259,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

@@ -344,6 +344,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,

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

@@ -217,6 +217,8 @@ func (d *CloudDisk) GetMultiWriter() bool {
 	switch {
 	case d.disk != nil:
 		return false
+	case d.disk != nil && d.disk.AccessMode == "READ_WRITE_MANY":
+		return true
 	case d.betaDisk != nil:
 		return d.betaDisk.MultiWriter
 	default:

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

@@ -324,8 +324,6 @@ func (cloud *CloudProvider) ListSnapshots(ctx context.Context, filter string) ([
 func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *meta.Key, gceAPIVersion GCEAPIVersion) (*CloudDisk, error) {
 	klog.V(5).Infof("Getting disk %v", key)
 
-	// Override GCEAPIVersion as hyperdisk is only available in beta and we cannot get the disk-type with get disk call.
-	gceAPIVersion = GCEAPIVersionBeta
 	switch key.Type() {
 	case meta.Zonal:
 		if gceAPIVersion == GCEAPIVersionBeta {
@@ -407,11 +405,6 @@ func (cloud *CloudProvider) ValidateExistingDisk(ctx context.Context, resp *Clou
 			reqBytes, common.GbToBytes(resp.GetSizeGb()), limBytes)
 	}
 
-	// We are assuming here that a multiWriter disk could be used as non-multiWriter
-	if multiWriter && !resp.GetMultiWriter() {
-		return fmt.Errorf("disk already exists with incompatible capability. Need MultiWriter. Got non-MultiWriter")
-	}
-
 	return ValidateDiskParameters(resp, params)
 }
 
@@ -553,9 +546,6 @@ func convertV1DiskToBetaDisk(v1Disk *computev1.Disk) *computebeta.Disk {
 		AccessMode:        v1Disk.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 v1Disk.ProvisionedIops > 0 {
 		betaDisk.ProvisionedIops = v1Disk.ProvisionedIops
 	}
@@ -619,9 +609,6 @@ func convertBetaDiskToV1Disk(betaDisk *computebeta.Disk) *computev1.Disk {
 		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
 	}
@@ -651,7 +638,8 @@ func (cloud *CloudProvider) insertRegionalDisk(
 		gceAPIVersion = GCEAPIVersionV1
 	)
 
-	if multiWriter {
+	// Use beta API for non-hyperdisk types in multi-writer mode.
+	if multiWriter && !strings.Contains(params.DiskType, "hyperdisk") {
 		gceAPIVersion = GCEAPIVersionBeta
 	}
 
@@ -778,7 +766,9 @@ func (cloud *CloudProvider) insertZonalDisk(
 		opName        string
 		gceAPIVersion = GCEAPIVersionV1
 	)
-	if multiWriter {
+
+	// Use beta API for non-hyperdisk types in multi-writer mode.
+	if multiWriter && !strings.Contains(params.DiskType, "hyperdisk") {
 		gceAPIVersion = GCEAPIVersionBeta
 	}
 

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

@@ -606,7 +606,7 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi
 	capBytes, _ := getRequestCapacity(capacityRange)
 	multiWriter, _ := getMultiWriterFromCapabilities(req.GetVolumeCapabilities())
 	readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities())
-	accessMode := ""
+	accessMode := params.AccessMode
 	if readonly && slices.Contains(disksWithModifiableAccessMode, params.DiskType) {
 		accessMode = readOnlyManyAccessMode
 	}

test/e2e/tests/single_zone_e2e_test.go

@@ -892,8 +892,7 @@ var _ = Describe("GCE PD CSI Driver", func() {
 		Expect(err).To(BeNil(), "Failed to go through volume lifecycle")
 	})
 
-	// Pending while multi-writer feature is in Alpha
-	PIt("Should create and delete multi-writer disk", func() {
+	It("Should create and delete multi-writer disk", func() {
 		Expect(testContexts).ToNot(BeEmpty())
 		testContext := getRandomTestContext()
 
@@ -904,7 +903,7 @@ var _ = Describe("GCE PD CSI Driver", func() {
 		zone := "us-east1-a"
 
 		// Create and Validate Disk
-		volName, volID := createAndValidateUniqueZonalMultiWriterDisk(client, p, zone, standardDiskType)
+		volName, volID := createAndValidateUniqueZonalMultiWriterDisk(client, p, zone, hdbDiskType)
 
 		defer func() {
 			// Delete Disk
@@ -917,16 +916,15 @@ var _ = Describe("GCE PD CSI Driver", func() {
 		}()
 	})
 
-	// Pending while multi-writer feature is in Alpha
-	PIt("Should complete entire disk lifecycle with multi-writer disk", func() {
+	It("Should complete entire disk lifecycle with multi-writer disk", func() {
 		testContext := getRandomTestContext()
 
 		p, z, _ := testContext.Instance.GetIdentity()
 		client := testContext.Client
 		instance := testContext.Instance
 
 		// Create and Validate Disk
-		volName, volID := createAndValidateUniqueZonalMultiWriterDisk(client, p, z, standardDiskType)
+		volName, volID := createAndValidateUniqueZonalMultiWriterDisk(client, p, z, hdbDiskType)
 
 		defer func() {
 			// Delete Disk
@@ -1612,6 +1610,9 @@ func deleteVolumeOrError(client *remote.CsiClient, volID string) {
 func createAndValidateUniqueZonalMultiWriterDisk(client *remote.CsiClient, project, zone string, diskType string) (string, string) {
 	// Create Disk
 	disk := typeToDisk[diskType]
+
+	disk.params[common.ParameterAccessMode] = "READ_WRITE_MANY"
+
 	volName := testNamePrefix + string(uuid.NewUUID())
 	volume, err := client.CreateVolumeWithCaps(volName, disk.params, defaultMwSizeGb,
 		&csi.TopologyRequirement{