From e63374bd00006d0b2b1564f8a8f23461c114fe3b Mon Sep 17 00:00:00 2001 From: Sam Serdlow Date: Wed, 5 Feb 2025 17:54:35 +0000 Subject: [PATCH] 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 f9b646b64..0ffcd39e9 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -184,7 +184,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 940d9d86f..b493a3962 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 b096305dc..6f14a4473 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -602,7 +602,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 @@ -657,7 +657,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) @@ -787,7 +787,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 { @@ -883,7 +883,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) @@ -916,7 +916,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 { @@ -1085,7 +1085,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 @@ -1231,7 +1231,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) { @@ -1298,7 +1298,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) { @@ -1539,7 +1539,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) { @@ -1881,7 +1881,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) @@ -2437,7 +2437,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)) } @@ -2460,7 +2460,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 06db7222e..d46c0859d 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -1565,7 +1565,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) } @@ -1859,7 +1859,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) @@ -1964,7 +1964,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) @@ -5241,7 +5241,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) }