Skip to content

Changes remove the API version concept from GetDisk and InsertDisk, n… #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 6, 2025
Merged

Changes remove the API version concept from GetDisk and InsertDisk, n… #5

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/gce-cloud-provider/compute/fake-gce.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
107 changes: 34 additions & 73 deletions pkg/gce-cloud-provider/compute/gce-compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
}
}
Expand All @@ -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
Expand All @@ -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))
}
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -814,7 +784,7 @@ func (cloud *CloudProvider) insertZonalDisk(
}

if params.DiskEncryptionKMSKey != "" {
diskToCreate.DiskEncryptionKey = &computev1.CustomerEncryptionKey{
diskToCreate.DiskEncryptionKey = &computebeta.CustomerEncryptionKey{
KmsKeyName: params.DiskEncryptionKMSKey,
}
}
Expand All @@ -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
Expand All @@ -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))
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down
24 changes: 12 additions & 12 deletions pkg/gce-pd-csi-driver/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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))
}
Expand All @@ -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))
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/gce-pd-csi-driver/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
Expand Down