From c21fe9d64d0325a75257642354f5e0483149dd21 Mon Sep 17 00:00:00 2001 From: karkunpavan Date: Fri, 10 Jan 2025 12:56:38 +0000 Subject: [PATCH 01/12] changes to support hyperdisk multi-writer mode --- pkg/common/parameters.go | 4 ++++ pkg/gce-cloud-provider/compute/cloud-disk.go | 2 ++ pkg/gce-cloud-provider/compute/gce-compute.go | 20 +++++-------------- pkg/gce-pd-csi-driver/controller.go | 2 +- test/e2e/tests/single_zone_e2e_test.go | 12 +++++------ 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index f41c32336..731e73421 100644 --- a/pkg/common/parameters.go +++ b/pkg/common/parameters.go @@ -107,6 +107,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 } func (dp *DiskParameters) IsRegional() bool { @@ -154,6 +157,7 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] Tags: make(map[string]string), // Default Labels: make(map[string]string), // Default ResourceTags: make(map[string]string), // Default + AccessMode: "READ_WRITE_SINGLE", // Default } for k, v := range extraVolumeLabels { diff --git a/pkg/gce-cloud-provider/compute/cloud-disk.go b/pkg/gce-cloud-provider/compute/cloud-disk.go index f4cd3bec6..69c37a52f 100644 --- a/pkg/gce-cloud-provider/compute/cloud-disk.go +++ b/pkg/gce-cloud-provider/compute/cloud-disk.go @@ -228,6 +228,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: diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index e2a753b22..ad14eaa10 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/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 } diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 117df6413..98479961c 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -598,7 +598,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 = gceReadOnlyManyAccessMode } diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index fc0109c4b..a3f2fc87d 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -905,8 +905,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() @@ -917,7 +916,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 @@ -930,8 +929,7 @@ 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() @@ -939,7 +937,7 @@ var _ = Describe("GCE PD CSI Driver", func() { 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 @@ -1787,6 +1785,8 @@ 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.AccessMode = "READ_WRITE_MANY" + volName := testNamePrefix + string(uuid.NewUUID()) volume, err := client.CreateVolumeWithCaps(volName, disk.params, defaultMwSizeGb, &csi.TopologyRequirement{ From d9f760ee72f15d419592c377ecce6a24d2ec10c3 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Fri, 10 Jan 2025 15:29:49 +0000 Subject: [PATCH 02/12] Added fixes for test key handling. --- pkg/common/parameters.go | 8 +++++++- test/e2e/tests/single_zone_e2e_test.go | 3 ++- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index 731e73421..b55e70589 100644 --- a/pkg/common/parameters.go +++ b/pkg/common/parameters.go @@ -22,6 +22,9 @@ import ( ) const ( + // Disk Params + ParameterAccessMode = "access-mode" + // Parameters for StorageClass ParameterKeyType = "type" ParameterKeyReplicationType = "replication-type" @@ -157,7 +160,6 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] Tags: make(map[string]string), // Default Labels: make(map[string]string), // Default ResourceTags: make(map[string]string), // Default - AccessMode: "READ_WRITE_SINGLE", // Default } for k, v := range extraVolumeLabels { @@ -266,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) } diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index a3f2fc87d..00be5faa4 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -1785,7 +1785,8 @@ 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.AccessMode = "READ_WRITE_MANY" + + disk.params[common.ParameterAccessMode] = "READ_WRITE_MANY" volName := testNamePrefix + string(uuid.NewUUID()) volume, err := client.CreateVolumeWithCaps(volName, disk.params, defaultMwSizeGb, From 7890527b0484aed828ae1defd465e516a656c862 Mon Sep 17 00:00:00 2001 From: sjswerdlow <109298351+sjswerdlow@users.noreply.github.com> Date: Mon, 13 Jan 2025 15:29:50 -0500 Subject: [PATCH 03/12] Multiwriter Test Update (#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. --- test/e2e/tests/setup_e2e_test.go | 32 ++++++++++++++++++++------ test/e2e/tests/single_zone_e2e_test.go | 22 ++++++------------ 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/test/e2e/tests/setup_e2e_test.go b/test/e2e/tests/setup_e2e_test.go index ebe1a920a..769390c56 100644 --- a/test/e2e/tests/setup_e2e_test.go +++ b/test/e2e/tests/setup_e2e_test.go @@ -36,13 +36,17 @@ import ( ) var ( - project = flag.String("project", "", "Project to run tests in") - serviceAccount = flag.String("service-account", "", "Service account to bring up instance with") - vmNamePrefix = flag.String("vm-name-prefix", "gce-pd-csi-e2e", "VM name prefix") - architecture = flag.String("arch", "amd64", "Architecture pd csi driver build on") - minCpuPlatform = flag.String("min-cpu-platform", "AMD Milan", "Minimum CPU architecture") - zones = flag.String("zones", "us-east4-a,us-east4-c", "Zones to run tests in. If there are multiple zones, separate each by comma") - machineType = flag.String("machine-type", "n2d-standard-2", "Type of machine to provision instance on") + project = flag.String("project", "", "Project to run tests in") + serviceAccount = flag.String("service-account", "", "Service account to bring up instance with") + vmNamePrefix = flag.String("vm-name-prefix", "gce-pd-csi-e2e", "VM name prefix") + architecture = flag.String("arch", "amd64", "Architecture pd csi driver build on") + minCpuPlatform = flag.String("min-cpu-platform", "rome", "Minimum CPU architecture") + mwMinCpuPlatform = flag.String("min-cpu-platform-mw", "sapphirerapids", "Minimum CPU architecture for multiwriter tests") + zones = flag.String("zones", "us-east4-a,us-east4-c", "Zones to run tests in. If there are multiple zones, separate each by comma") + machineType = flag.String("machine-type", "n2d-standard-4", "Type of machine to provision instance on") + // Multi-writer is only supported on M3, C3, and N4 + // https://cloud.google.com/compute/docs/disks/sharing-disks-between-vms#hd-multi-writer + mwMachineType = flag.String("mw-machine-type", "c3-standard-4", "Type of machine to provision instance for multiwriter tests") imageURL = flag.String("image-url", "projects/ubuntu-os-cloud/global/images/family/ubuntu-minimal-2404-lts-amd64", "OS image url to get image from") runInProw = flag.Bool("run-in-prow", false, "If true, use a Boskos loaned project and special CI service accounts and ssh keys") deleteInstances = flag.Bool("delete-instances", false, "Delete the instances after tests run") @@ -117,6 +121,8 @@ var _ = BeforeSuite(func() { for i := 0; i < len(zones); i++ { tc := <-tcc testContexts = append(testContexts, tc) + mwTc := <-mwTcc + multiWriterTestContexts = append(multiWriterTestContexts, mwTc) klog.Infof("Added TestContext for node %s", tc.Instance.GetName()) tc = <-hdtcc hyperdiskTestContexts = append(hyperdiskTestContexts, tc) @@ -132,6 +138,13 @@ var _ = AfterSuite(func() { tc.Instance.DeleteInstance() } } + for _, mwTc := range multiWriterTestContexts { + err := remote.TeardownDriverAndClient(mwTc) + Expect(err).To(BeNil(), "Multiwriter Teardown Driver and Client failed with error") + if *deleteInstances { + mwTc.Instance.DeleteInstance() + } + } }) func notEmpty(v string) bool { @@ -201,3 +214,8 @@ func getRandomTestContext() *remote.TestContext { rn := rand.Intn(len(testContexts)) return testContexts[rn] } +func getRandomMwTestContext() *remote.TestContext { + Expect(multiWriterTestContexts).ToNot(BeEmpty()) + rn := rand.Intn(len(multiWriterTestContexts)) + return multiWriterTestContexts[rn] +} diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index 00be5faa4..4ee5b9d37 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -80,7 +80,6 @@ const ( ) var _ = Describe("GCE PD CSI Driver", func() { - It("Should get reasonable volume limits from nodes with NodeGetInfo", func() { testContext := getRandomTestContext() resp, err := testContext.Client.NodeGetInfo() @@ -284,6 +283,7 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "Could not find disk in correct zone") } }) + // TODO(hime): Enable this test once all release branches contain the fix from PR#1708. // It("Should return InvalidArgument when disk size exceeds limit", func() { // // If this returns a different error code (like Unknown), the error wrapping logic in #1708 has regressed. @@ -907,16 +907,12 @@ var _ = Describe("GCE PD CSI Driver", func() { It("Should create and delete multi-writer disk", func() { Expect(testContexts).ToNot(BeEmpty()) - testContext := getRandomTestContext() + testContext := getRandomMwTestContext() - p, _, _ := testContext.Instance.GetIdentity() + p, z, _ := testContext.Instance.GetIdentity() client := testContext.Client - - // Hardcode to us-east1-a while feature is in alpha - zone := "us-east1-a" - // Create and Validate Disk - volName, volID := createAndValidateUniqueZonalMultiWriterDisk(client, p, zone, hdbDiskType) + volName, volID := createAndValidateUniqueZonalMultiWriterDisk(client, p, z, hdbDiskType) defer func() { // Delete Disk @@ -924,13 +920,13 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "DeleteVolume failed") // Validate Disk Deleted - _, err = computeAlphaService.Disks.Get(p, zone, volName).Do() + _, err = computeService.Disks.Get(p, z, volName).Do() Expect(gce.IsGCEError(err, "notFound")).To(BeTrue(), "Expected disk to not be found") }() }) It("Should complete entire disk lifecycle with multi-writer disk", func() { - testContext := getRandomTestContext() + testContext := getRandomMwTestContext() p, z, _ := testContext.Instance.GetIdentity() client := testContext.Client @@ -1787,7 +1783,6 @@ func createAndValidateUniqueZonalMultiWriterDisk(client *remote.CsiClient, proje 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{ @@ -1815,12 +1810,9 @@ func createAndValidateUniqueZonalMultiWriterDisk(client *remote.CsiClient, proje Expect(cloudDisk.Status).To(Equal(readyState)) Expect(cloudDisk.SizeGb).To(Equal(defaultMwSizeGb)) Expect(cloudDisk.Name).To(Equal(volName)) + Expect(cloudDisk.AccessMode).To(Equal("READ_WRITE_MANY")) disk.validate(cloudDisk) - alphaDisk, err := computeAlphaService.Disks.Get(project, zone, volName).Do() - Expect(err).To(BeNil(), "Failed to get cloud disk using alpha API") - Expect(alphaDisk.MultiWriter).To(Equal(true)) - return volName, volume.VolumeId } From 8597e7866763e7dc0b0643280c701eebff7bd362 Mon Sep 17 00:00:00 2001 From: sjswerdlow <109298351+sjswerdlow@users.noreply.github.com> Date: Mon, 13 Jan 2025 18:21:30 -0500 Subject: [PATCH 04/12] Update parameters.go Fixing a linting issue. --- pkg/common/parameters.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index b55e70589..e88d876b8 100644 --- a/pkg/common/parameters.go +++ b/pkg/common/parameters.go @@ -23,7 +23,7 @@ import ( const ( // Disk Params - ParameterAccessMode = "access-mode" + ParameterAccessMode = "access-mode" // Parameters for StorageClass ParameterKeyType = "type" From 2504414d86f49d6094c7703555613b448194de7c Mon Sep 17 00:00:00 2001 From: sjswerdlow <109298351+sjswerdlow@users.noreply.github.com> Date: Wed, 22 Jan 2025 16:34:56 -0500 Subject: [PATCH 05/12] Karkunpavan (#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. --- pkg/gce-cloud-provider/compute/cloud-disk.go | 13 ------------- pkg/gce-cloud-provider/compute/fake-gce.go | 2 +- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/pkg/gce-cloud-provider/compute/cloud-disk.go b/pkg/gce-cloud-provider/compute/cloud-disk.go index 69c37a52f..b381b7782 100644 --- a/pkg/gce-cloud-provider/compute/cloud-disk.go +++ b/pkg/gce-cloud-provider/compute/cloud-disk.go @@ -224,19 +224,6 @@ func (d *CloudDisk) GetKMSKeyName() string { return "" } -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: - return false - } -} - func (d *CloudDisk) GetEnableConfidentialCompute() bool { switch { case d.disk != nil: diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index 2f00ea6e2..43b91a022 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -217,7 +217,7 @@ func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp * } // We are assuming here that a multiWriter disk could be used as non-multiWriter - if multiWriter && !resp.GetMultiWriter() { + if multiWriter { return fmt.Errorf("disk already exists with incompatible capability. Need MultiWriter. Got non-MultiWriter") } From 110d04cfda4559f56bb2df4af674f68376aed75f Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Thu, 23 Jan 2025 17:03:58 +0000 Subject: [PATCH 06/12] Completley removing the multiwriter check from fake-gce. --- pkg/gce-cloud-provider/compute/fake-gce.go | 5 ----- 1 file changed, 5 deletions(-) diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index 43b91a022..a9be5f78f 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -216,11 +216,6 @@ func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp * params.DiskType, respType[len(respType)-1]) } - // We are assuming here that a multiWriter disk could be used as non-multiWriter - if multiWriter { - 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) } From 1b8504cb966ce2db22b502c6375b2f9331b250c3 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Wed, 5 Feb 2025 17:54:35 +0000 Subject: [PATCH 07/12] Changes remove the API version concept from GetDisk and InsertDisk, now just defaults to using the Beta API. --- pkg/gce-cloud-provider/compute/fake-gce.go | 2 +- pkg/gce-cloud-provider/compute/gce-compute.go | 107 ++++++------------ pkg/gce-pd-csi-driver/controller.go | 24 ++-- pkg/gce-pd-csi-driver/controller_test.go | 8 +- 4 files changed, 51 insertions(+), 90 deletions(-) diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index a9be5f78f..6cead5c67 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -188,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() diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index ad14eaa10..1e3750b4a 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -99,7 +99,7 @@ type GCECompute interface { GetDefaultProject() string GetDefaultZone() string // Disk Methods - GetDisk(ctx context.Context, project string, volumeKey *meta.Key, gceAPIVersion GCEAPIVersion) (*CloudDisk, error) + GetDisk(ctx context.Context, project string, volumeKey *meta.Key) (*CloudDisk, error) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error) 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 @@ -321,26 +321,16 @@ func (cloud *CloudProvider) ListSnapshots(ctx context.Context, filter string) ([ return items, "", nil } -func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *meta.Key, gceAPIVersion GCEAPIVersion) (*CloudDisk, error) { +func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *meta.Key) (*CloudDisk, error) { klog.V(5).Infof("Getting disk %v", key) switch key.Type() { case meta.Zonal: - if gceAPIVersion == GCEAPIVersionBeta { - disk, err := cloud.getZonalBetaDiskOrError(ctx, project, key.Zone, key.Name) - return CloudDiskFromBeta(disk), err - } else { - disk, err := cloud.getZonalDiskOrError(ctx, project, key.Zone, key.Name) - return CloudDiskFromV1(disk), err - } + disk, err := cloud.getZonalBetaDiskOrError(ctx, project, key.Zone, key.Name) + return CloudDiskFromBeta(disk), err case meta.Regional: - if gceAPIVersion == GCEAPIVersionBeta { - disk, err := cloud.getRegionalBetaDiskOrError(ctx, project, key.Region, key.Name) - return CloudDiskFromBeta(disk), err - } else { - disk, err := cloud.getRegionalDiskOrError(ctx, project, key.Region, key.Name) - return CloudDiskFromV1(disk), err - } + disk, err := cloud.getRegionalBetaDiskOrError(ctx, project, key.Region, key.Name) + return CloudDiskFromBeta(disk), err default: return nil, fmt.Errorf("key was neither zonal nor regional, got: %v", key.String()) } @@ -633,17 +623,11 @@ func (cloud *CloudProvider) insertRegionalDisk( description string, multiWriter bool) error { var ( - err error - opName string - gceAPIVersion = GCEAPIVersionV1 + err error + opName string ) - // Use beta API for non-hyperdisk types in multi-writer mode. - if multiWriter && !strings.Contains(params.DiskType, "hyperdisk") { - gceAPIVersion = GCEAPIVersionBeta - } - - diskToCreate := &computev1.Disk{ + diskToCreate := &computebeta.Disk{ Name: volKey.Name, SizeGb: common.BytesToGbRoundUp(capBytes), Description: description, @@ -672,7 +656,7 @@ func (cloud *CloudProvider) insertRegionalDisk( diskToCreate.ReplicaZones = replicaZones } if params.DiskEncryptionKMSKey != "" { - diskToCreate.DiskEncryptionKey = &computev1.CustomerEncryptionKey{ + diskToCreate.DiskEncryptionKey = &computebeta.CustomerEncryptionKey{ KmsKeyName: params.DiskEncryptionKMSKey, } } @@ -682,29 +666,21 @@ func (cloud *CloudProvider) insertRegionalDisk( } if len(resourceTags) > 0 { - diskToCreate.Params = &computev1.DiskParams{ + diskToCreate.Params = &computebeta.DiskParams{ ResourceManagerTags: resourceTags, } } - if gceAPIVersion == GCEAPIVersionBeta { - var insertOp *computebeta.Operation - betaDiskToCreate := convertV1DiskToBetaDisk(diskToCreate) - betaDiskToCreate.MultiWriter = multiWriter - insertOp, err = cloud.betaService.RegionDisks.Insert(project, volKey.Region, betaDiskToCreate).Context(ctx).Do() - if insertOp != nil { - opName = insertOp.Name - } - } else { - var insertOp *computev1.Operation - insertOp, err = cloud.service.RegionDisks.Insert(project, volKey.Region, diskToCreate).Context(ctx).Do() - if insertOp != nil { - opName = insertOp.Name - } + var insertOp *computebeta.Operation + diskToCreate.MultiWriter = multiWriter + insertOp, err = cloud.betaService.RegionDisks.Insert(project, volKey.Region, diskToCreate).Context(ctx).Do() + if insertOp != nil { + opName = insertOp.Name } + if err != nil { if IsGCEError(err, "alreadyExists") { - disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion) + disk, err := cloud.GetDisk(ctx, project, volKey) if err != nil { // failed to GetDisk, however the Disk may already exist // the error code should be non-Final @@ -730,7 +706,7 @@ func (cloud *CloudProvider) insertRegionalDisk( // the error code returned should be non-final if err != nil { if IsGCEError(err, "alreadyExists") { - disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion) + disk, err := cloud.GetDisk(ctx, project, volKey) if err != nil { return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err)) } @@ -762,17 +738,11 @@ func (cloud *CloudProvider) insertZonalDisk( multiWriter bool, accessMode string) error { var ( - err error - opName string - gceAPIVersion = GCEAPIVersionV1 + err error + opName string ) - // Use beta API for non-hyperdisk types in multi-writer mode. - if multiWriter && !strings.Contains(params.DiskType, "hyperdisk") { - gceAPIVersion = GCEAPIVersionBeta - } - - diskToCreate := &computev1.Disk{ + diskToCreate := &computebeta.Disk{ Name: volKey.Name, SizeGb: common.BytesToGbRoundUp(capBytes), Description: description, @@ -814,7 +784,7 @@ func (cloud *CloudProvider) insertZonalDisk( } if params.DiskEncryptionKMSKey != "" { - diskToCreate.DiskEncryptionKey = &computev1.CustomerEncryptionKey{ + diskToCreate.DiskEncryptionKey = &computebeta.CustomerEncryptionKey{ KmsKeyName: params.DiskEncryptionKMSKey, } } @@ -826,31 +796,22 @@ func (cloud *CloudProvider) insertZonalDisk( } if len(resourceTags) > 0 { - diskToCreate.Params = &computev1.DiskParams{ + diskToCreate.Params = &computebeta.DiskParams{ ResourceManagerTags: resourceTags, } } - diskToCreate.AccessMode = accessMode - if gceAPIVersion == GCEAPIVersionBeta { - var insertOp *computebeta.Operation - betaDiskToCreate := convertV1DiskToBetaDisk(diskToCreate) - betaDiskToCreate.MultiWriter = multiWriter - insertOp, err = cloud.betaService.Disks.Insert(project, volKey.Zone, betaDiskToCreate).Context(ctx).Do() - if insertOp != nil { - opName = insertOp.Name - } - } else { - var insertOp *computev1.Operation - insertOp, err = cloud.service.Disks.Insert(project, volKey.Zone, diskToCreate).Context(ctx).Do() - if insertOp != nil { - opName = insertOp.Name - } + diskToCreate.AccessMode = accessMode + var insertOp *computebeta.Operation + diskToCreate.MultiWriter = multiWriter + insertOp, err = cloud.betaService.Disks.Insert(project, volKey.Zone, diskToCreate).Context(ctx).Do() + if insertOp != nil { + opName = insertOp.Name } if err != nil { if IsGCEError(err, "alreadyExists") { - disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion) + disk, err := cloud.GetDisk(ctx, project, volKey) if err != nil { // failed to GetDisk, however the Disk may already exist // the error code should be non-Final @@ -877,7 +838,7 @@ func (cloud *CloudProvider) insertZonalDisk( // failed to wait for Op to finish, however, the Op possibly is still running as expected // the error code returned should be non-final if IsGCEError(err, "alreadyExists") { - disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion) + disk, err := cloud.GetDisk(ctx, project, volKey) if err != nil { return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err)) } @@ -1176,7 +1137,7 @@ func (cloud *CloudProvider) waitForAttachOnDisk(ctx context.Context, project str start := time.Now() return wait.ExponentialBackoff(AttachDiskBackoff, func() (bool, error) { klog.V(6).Infof("Polling disks.get for attach of disk %v to instance %v to complete for %v", volKey.Name, instanceName, time.Since(start)) - disk, err := cloud.GetDisk(ctx, project, volKey, GCEAPIVersionV1) + disk, err := cloud.GetDisk(ctx, project, volKey) if err != nil { return false, fmt.Errorf("GetDisk failed to get disk: %w", err) } @@ -1426,7 +1387,7 @@ func (cloud *CloudProvider) DeleteImage(ctx context.Context, project, imageName // k8s.io/apimachinery/quantity package for better size handling func (cloud *CloudProvider) ResizeDisk(ctx context.Context, project string, volKey *meta.Key, requestBytes int64) (int64, error) { klog.V(5).Infof("Resizing disk %v to size %v", volKey, requestBytes) - cloudDisk, err := cloud.GetDisk(ctx, project, volKey, GCEAPIVersionV1) + cloudDisk, err := cloud.GetDisk(ctx, project, volKey) if err != nil { return -1, fmt.Errorf("failed to get disk: %w", err) } diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 98479961c..43949526d 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -604,7 +604,7 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi } // Validate if disk already exists - existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, gceCS.CloudProvider.GetDefaultProject(), volKey, getGCEApiVersion(multiWriter)) + existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, gceCS.CloudProvider.GetDefaultProject(), volKey) if err != nil { if !gce.IsGCEError(err, "notFound") { // failed to GetDisk, however the Disk may already be created, the error code should be non-Final @@ -659,7 +659,7 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi } // Verify that the volume in VolumeContentSource exists. - diskFromSourceVolume, err := gceCS.CloudProvider.GetDisk(ctx, project, sourceVolKey, getGCEApiVersion(multiWriter)) + diskFromSourceVolume, err := gceCS.CloudProvider.GetDisk(ctx, project, sourceVolKey) if err != nil { if gce.IsGCEError(err, "notFound") { return nil, status.Errorf(codes.NotFound, "CreateVolume source volume %s does not exist", volumeContentSourceVolumeID) @@ -788,7 +788,7 @@ func (gceCS *GCEControllerServer) ControllerModifyVolume(ctx context.Context, re } klog.V(4).Infof("Modify Volume Parameters for %s: %v", volumeID, volumeModifyParams) - existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionBeta) + existingDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey) metrics.UpdateRequestMetadataFromDisk(ctx, existingDisk) if err != nil { @@ -884,7 +884,7 @@ func (gceCS *GCEControllerServer) deleteMultiZoneDisk(ctx context.Context, req * Region: volKey.Region, Zone: zone, } - disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, zonalVolKey, gce.GCEAPIVersionV1) + disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, zonalVolKey) // TODO: Consolidate the parameters here, rather than taking the last. metrics.UpdateRequestMetadataFromDisk(ctx, disk) err := gceCS.CloudProvider.DeleteDisk(ctx, project, zonalVolKey) @@ -917,7 +917,7 @@ func (gceCS *GCEControllerServer) deleteSingleDeviceDisk(ctx context.Context, re return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, volumeID) } defer gceCS.volumeLocks.Release(volumeID) - disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1) + disk, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey) metrics.UpdateRequestMetadataFromDisk(ctx, disk) err = gceCS.CloudProvider.DeleteDisk(ctx, project, volKey) if err != nil { @@ -1086,7 +1086,7 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, lockingVolumeID), nil } defer gceCS.volumeLocks.Release(lockingVolumeID) - disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1) + disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey) if err != nil { if gce.IsGCENotFoundError(err) { return nil, status.Errorf(codes.NotFound, "Could not find disk %v: %v", volKey.String(), err.Error()), disk @@ -1232,7 +1232,7 @@ func (gceCS *GCEControllerServer) executeControllerUnpublishVolume(ctx context.C return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, lockingVolumeID), nil } defer gceCS.volumeLocks.Release(lockingVolumeID) - diskToUnpublish, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1) + diskToUnpublish, _ := gceCS.CloudProvider.GetDisk(ctx, project, volKey) instance, err := gceCS.CloudProvider.GetInstanceOrError(ctx, instanceZone, instanceName) if err != nil { if gce.IsGCENotFoundError(err) { @@ -1300,7 +1300,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context } defer gceCS.volumeLocks.Release(volumeID) - disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1) + disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey) metrics.UpdateRequestMetadataFromDisk(ctx, disk) if err != nil { if gce.IsGCENotFoundError(err) { @@ -1541,7 +1541,7 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C defer gceCS.volumeLocks.Release(volumeID) // Check if volume exists - disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1) + disk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey) metrics.UpdateRequestMetadataFromDisk(ctx, disk) if err != nil { if gce.IsGCENotFoundError(err) { @@ -1890,7 +1890,7 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re return nil, status.Errorf(codes.InvalidArgument, "ControllerExpandVolume is not supported with the multi-zone PVC volumeHandle feature. Please re-create the volume %v from source if you want a larger size", volumeID) } - sourceDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey, gce.GCEAPIVersionV1) + sourceDisk, err := gceCS.CloudProvider.GetDisk(ctx, project, volKey) metrics.UpdateRequestMetadataFromDisk(ctx, sourceDisk) resizedGb, err := gceCS.CloudProvider.ResizeDisk(ctx, project, volKey, reqBytes) @@ -2445,7 +2445,7 @@ func createRegionalDisk(ctx context.Context, cloudProvider gce.GCECompute, name gceAPIVersion = gce.GCEAPIVersionBeta } // failed to GetDisk, however the Disk may already be created, the error code should be non-Final - disk, err := cloudProvider.GetDisk(ctx, project, meta.RegionalKey(name, region), gceAPIVersion) + disk, err := cloudProvider.GetDisk(ctx, project, meta.RegionalKey(name, region)) if err != nil { return nil, common.NewTemporaryError(codes.Unavailable, fmt.Errorf("failed to get disk after creating regional disk: %w", err)) } @@ -2468,7 +2468,7 @@ func createSingleZoneDisk(ctx context.Context, cloudProvider gce.GCECompute, nam gceAPIVersion = gce.GCEAPIVersionBeta } // failed to GetDisk, however the Disk may already be created, the error code should be non-Final - disk, err := cloudProvider.GetDisk(ctx, project, meta.ZonalKey(name, diskZone), gceAPIVersion) + disk, err := cloudProvider.GetDisk(ctx, project, meta.ZonalKey(name, diskZone)) if err != nil { return nil, common.NewTemporaryError(codes.Unavailable, fmt.Errorf("failed to get disk after creating zonal disk: %w", err)) } diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index 165fc7f9b..f14528b32 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -1702,7 +1702,7 @@ func TestMultiZoneVolumeCreation(t *testing.T) { for _, zone := range tc.expZones { volumeKey := meta.ZonalKey(name, zone) - disk, err := fcp.GetDisk(context.Background(), project, volumeKey, gce.GCEAPIVersionBeta) + disk, err := fcp.GetDisk(context.Background(), project, volumeKey) if err != nil { t.Fatalf("Get Disk failed for created disk with error: %v", err) } @@ -1996,7 +1996,7 @@ func TestCreateVolumeWithVolumeAttributeClassParameters(t *testing.T) { t.Fatalf("Failed to convert volume id to key: %v", err) } - disk, err := fcp.GetDisk(context.Background(), project, volumeKey, gce.GCEAPIVersionBeta) + disk, err := fcp.GetDisk(context.Background(), project, volumeKey) if err != nil { t.Fatalf("Failed to get disk: %v", err) @@ -2101,7 +2101,7 @@ func TestVolumeModifyOperation(t *testing.T) { } } - modifiedVol, err := fcp.GetDisk(context.Background(), project, volKey, gce.GCEAPIVersionBeta) + modifiedVol, err := fcp.GetDisk(context.Background(), project, volKey) if err != nil { t.Errorf("Failed to get volume: %v", err) @@ -5378,7 +5378,7 @@ func TestCreateConfidentialVolume(t *testing.T) { volumeId := resp.GetVolume().VolumeId project, volumeKey, err := common.VolumeIDToKey(volumeId) - createdDisk, err := fcp.GetDisk(context.Background(), project, volumeKey, gce.GCEAPIVersionBeta) + createdDisk, err := fcp.GetDisk(context.Background(), project, volumeKey) if err != nil { t.Fatalf("Get Disk failed for created disk with error: %v", err) } From bd455b5e0e1ac48ebfa94f471b9a9baa4d4df521 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Thu, 6 Feb 2025 21:29:56 +0000 Subject: [PATCH 08/12] Cleaning up un-used variables. --- pkg/gce-pd-csi-driver/controller.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index 43949526d..3bcee864a 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -2440,10 +2440,6 @@ func createRegionalDisk(ctx context.Context, cloudProvider gce.GCECompute, name return nil, fmt.Errorf("failed to insert regional disk: %w", err) } - gceAPIVersion := gce.GCEAPIVersionV1 - if multiWriter { - gceAPIVersion = gce.GCEAPIVersionBeta - } // failed to GetDisk, however the Disk may already be created, the error code should be non-Final disk, err := cloudProvider.GetDisk(ctx, project, meta.RegionalKey(name, region)) if err != nil { @@ -2463,10 +2459,6 @@ func createSingleZoneDisk(ctx context.Context, cloudProvider gce.GCECompute, nam return nil, fmt.Errorf("failed to insert zonal disk: %w", err) } - gceAPIVersion := gce.GCEAPIVersionV1 - if multiWriter { - gceAPIVersion = gce.GCEAPIVersionBeta - } // failed to GetDisk, however the Disk may already be created, the error code should be non-Final disk, err := cloudProvider.GetDisk(ctx, project, meta.ZonalKey(name, diskZone)) if err != nil { From 5d5d19e9e34a1af46b190e934a1217fe8dc8f4b1 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Thu, 6 Feb 2025 21:52:49 +0000 Subject: [PATCH 09/12] pkg/gce-cloud-provider/compute/fake-gce.go --- pkg/common/utils_test.go | 2 +- pkg/gce-pd-csi-driver/controller_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index c1307e734..554494e2e 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -1409,7 +1409,7 @@ func TestIsUserMultiAttachError(t *testing.T) { }, } for _, test := range cases { - code, err := isUserMultiAttachError(fmt.Errorf("%v", test.errorString)) + code, err := isUserMultiAttachError(fmt.Errorf("%s", 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) diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index f14528b32..80871c77e 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -1878,7 +1878,7 @@ func TestMultiZoneVolumeCreationErrHandling(t *testing.T) { } for _, volKey := range tc.wantDisks { - disk, err := fcp.GetDisk(context.Background(), project, volKey, gce.GCEAPIVersionV1) + disk, err := fcp.GetDisk(context.Background(), project, volKey) if err != nil { t.Errorf("Unexpected err fetching disk %v: %v", volKey, err) } From 80468c50e0f1e9f68ad4e5f5062931ac1b847373 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Mon, 10 Feb 2025 14:32:29 +0000 Subject: [PATCH 10/12] Addressing comments around GetMultiWriter checks. --- pkg/gce-cloud-provider/compute/cloud-disk.go | 11 +++++++++++ pkg/gce-cloud-provider/compute/fake-gce.go | 4 ++++ pkg/gce-cloud-provider/compute/gce-compute.go | 5 +++++ 3 files changed, 20 insertions(+) diff --git a/pkg/gce-cloud-provider/compute/cloud-disk.go b/pkg/gce-cloud-provider/compute/cloud-disk.go index b381b7782..f4cd3bec6 100644 --- a/pkg/gce-cloud-provider/compute/cloud-disk.go +++ b/pkg/gce-cloud-provider/compute/cloud-disk.go @@ -224,6 +224,17 @@ func (d *CloudDisk) GetKMSKeyName() string { return "" } +func (d *CloudDisk) GetMultiWriter() bool { + switch { + case d.disk != nil: + return false + case d.betaDisk != nil: + return d.betaDisk.MultiWriter + default: + return false + } +} + func (d *CloudDisk) GetEnableConfidentialCompute() bool { switch { case d.disk != nil: diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index 6cead5c67..ad517fca0 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -216,6 +216,10 @@ func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp * params.DiskType, respType[len(respType)-1]) } + // 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") + } klog.V(4).Infof("Compatible disk already exists") return ValidateDiskParameters(resp, params) } diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index 1e3750b4a..9344e5f14 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -395,6 +395,11 @@ 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) } From 26a11d10aea777b89693302453f99a80f942fd13 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Mon, 10 Feb 2025 15:28:34 +0000 Subject: [PATCH 11/12] Removing multi-writer as a disk paramter. --- pkg/gce-cloud-provider/compute/gce-compute.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index 9344e5f14..191da8940 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -808,7 +808,6 @@ func (cloud *CloudProvider) insertZonalDisk( diskToCreate.AccessMode = accessMode var insertOp *computebeta.Operation - diskToCreate.MultiWriter = multiWriter insertOp, err = cloud.betaService.Disks.Insert(project, volKey.Zone, diskToCreate).Context(ctx).Do() if insertOp != nil { opName = insertOp.Name From 5bc06cba005ed623ad25ab9ae765178158a5d1f8 Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Tue, 11 Feb 2025 17:15:04 +0000 Subject: [PATCH 12/12] Fixing rebasing issues. --- test/e2e/tests/setup_e2e_test.go | 35 ++++++++++++-------------- test/e2e/tests/single_zone_e2e_test.go | 1 - 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/test/e2e/tests/setup_e2e_test.go b/test/e2e/tests/setup_e2e_test.go index 769390c56..3bc738a2b 100644 --- a/test/e2e/tests/setup_e2e_test.go +++ b/test/e2e/tests/setup_e2e_test.go @@ -36,25 +36,24 @@ import ( ) var ( - project = flag.String("project", "", "Project to run tests in") - serviceAccount = flag.String("service-account", "", "Service account to bring up instance with") - vmNamePrefix = flag.String("vm-name-prefix", "gce-pd-csi-e2e", "VM name prefix") - architecture = flag.String("arch", "amd64", "Architecture pd csi driver build on") - minCpuPlatform = flag.String("min-cpu-platform", "rome", "Minimum CPU architecture") - mwMinCpuPlatform = flag.String("min-cpu-platform-mw", "sapphirerapids", "Minimum CPU architecture for multiwriter tests") - zones = flag.String("zones", "us-east4-a,us-east4-c", "Zones to run tests in. If there are multiple zones, separate each by comma") - machineType = flag.String("machine-type", "n2d-standard-4", "Type of machine to provision instance on") - // Multi-writer is only supported on M3, C3, and N4 - // https://cloud.google.com/compute/docs/disks/sharing-disks-between-vms#hd-multi-writer - mwMachineType = flag.String("mw-machine-type", "c3-standard-4", "Type of machine to provision instance for multiwriter tests") + project = flag.String("project", "", "Project to run tests in") + serviceAccount = flag.String("service-account", "", "Service account to bring up instance with") + vmNamePrefix = flag.String("vm-name-prefix", "gce-pd-csi-e2e", "VM name prefix") + architecture = flag.String("arch", "amd64", "Architecture pd csi driver build on") + minCpuPlatform = flag.String("min-cpu-platform", "rome", "Minimum CPU architecture") + mwMinCpuPlatform = flag.String("min-cpu-platform-mw", "sapphirerapids", "Minimum CPU architecture for multiwriter tests") + zones = flag.String("zones", "us-east4-a,us-east4-c", "Zones to run tests in. If there are multiple zones, separate each by comma") + machineType = flag.String("machine-type", "n2d-standard-4", "Type of machine to provision instance on") imageURL = flag.String("image-url", "projects/ubuntu-os-cloud/global/images/family/ubuntu-minimal-2404-lts-amd64", "OS image url to get image from") runInProw = flag.Bool("run-in-prow", false, "If true, use a Boskos loaned project and special CI service accounts and ssh keys") deleteInstances = flag.Bool("delete-instances", false, "Delete the instances after tests run") cloudtopHost = flag.Bool("cloudtop-host", false, "The local host is cloudtop, a kind of googler machine with special requirements to access GCP") extraDriverFlags = flag.String("extra-driver-flags", "", "Extra flags to pass to the driver") enableConfidentialCompute = flag.Bool("enable-confidential-compute", false, "Create VMs with confidential compute mode. This uses NVMe devices") - hdMachineType = flag.String("hyperdisk-machine-type", "c3-standard-4", "Type of machine to provision instance on") - hdMinCpuPlatform = flag.String("hyperdisk-min-cpu-platform", "sapphirerapids", "Minimum CPU architecture") + // Multi-writer is only supported on M3, C3, and N4 + // https://cloud.google.com/compute/docs/disks/sharing-disks-between-vms#hd-multi-writer + hdMachineType = flag.String("hyperdisk-machine-type", "c3-standard-4", "Type of machine to provision instance on") + hdMinCpuPlatform = flag.String("hyperdisk-min-cpu-platform", "sapphirerapids", "Minimum CPU architecture") testContexts = []*remote.TestContext{} hyperdiskTestContexts = []*remote.TestContext{} @@ -121,8 +120,6 @@ var _ = BeforeSuite(func() { for i := 0; i < len(zones); i++ { tc := <-tcc testContexts = append(testContexts, tc) - mwTc := <-mwTcc - multiWriterTestContexts = append(multiWriterTestContexts, mwTc) klog.Infof("Added TestContext for node %s", tc.Instance.GetName()) tc = <-hdtcc hyperdiskTestContexts = append(hyperdiskTestContexts, tc) @@ -138,7 +135,7 @@ var _ = AfterSuite(func() { tc.Instance.DeleteInstance() } } - for _, mwTc := range multiWriterTestContexts { + for _, mwTc := range hyperdiskTestContexts { err := remote.TeardownDriverAndClient(mwTc) Expect(err).To(BeNil(), "Multiwriter Teardown Driver and Client failed with error") if *deleteInstances { @@ -215,7 +212,7 @@ func getRandomTestContext() *remote.TestContext { return testContexts[rn] } func getRandomMwTestContext() *remote.TestContext { - Expect(multiWriterTestContexts).ToNot(BeEmpty()) - rn := rand.Intn(len(multiWriterTestContexts)) - return multiWriterTestContexts[rn] + Expect(hyperdiskTestContexts).ToNot(BeEmpty()) + rn := rand.Intn(len(hyperdiskTestContexts)) + return hyperdiskTestContexts[rn] } diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index 4ee5b9d37..42041afb4 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -283,7 +283,6 @@ var _ = Describe("GCE PD CSI Driver", func() { Expect(err).To(BeNil(), "Could not find disk in correct zone") } }) - // TODO(hime): Enable this test once all release branches contain the fix from PR#1708. // It("Should return InvalidArgument when disk size exceeds limit", func() { // // If this returns a different error code (like Unknown), the error wrapping logic in #1708 has regressed.