Skip to content

Commit 6e43a14

Browse files
mattcarymattcary
authored
Merge pull request #1919 from karkunpavan/followup
Feature: Changes to support Hyperdisk Multi-Writer mode by moving to Beta APIs.
2 parents fb76ee7 + 10375f2 commit 6e43a14

File tree

7 files changed

+92
-129
lines changed

7 files changed

+92
-129
lines changed

‎pkg/common/parameters.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,9 @@ import (
2222
)
2323

2424
const (
25+
// Disk Params
26+
ParameterAccessMode = "access-mode"
27+
2528
// Parameters for StorageClass
2629
ParameterKeyType = "type"
2730
ParameterKeyReplicationType = "replication-type"
@@ -107,6 +110,9 @@ type DiskParameters struct {
107110
// Values: {bool}
108111
// Default: false
109112
MultiZoneProvisioning bool
113+
// Values: READ_WRITE_SINGLE, READ_ONLY_MANY, READ_WRITE_MANY
114+
// Default: READ_WRITE_SINGLE
115+
AccessMode string
110116
}
111117

112118
func (dp *DiskParameters) IsRegional() bool {
@@ -262,6 +268,10 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
262268
if paramEnableMultiZoneProvisioning {
263269
p.Labels[MultiZoneLabel] = "true"
264270
}
271+
case ParameterAccessMode:
272+
if v != "" {
273+
p.AccessMode = v
274+
}
265275
default:
266276
return p, fmt.Errorf("parameters contains invalid option %q", k)
267277
}

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

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ func (cloud *FakeCloudProvider) ListSnapshots(ctx context.Context, filter string
188188
}
189189

190190
// Disk Methods
191-
func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, project string, volKey *meta.Key, api GCEAPIVersion) (*CloudDisk, error) {
191+
func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, project string, volKey *meta.Key) (*CloudDisk, error) {
192192
disk, ok := cloud.disks[volKey.String()]
193193
if !ok {
194194
return nil, notFoundError()
@@ -220,7 +220,6 @@ func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp *
220220
if multiWriter && !resp.GetMultiWriter() {
221221
return fmt.Errorf("disk already exists with incompatible capability. Need MultiWriter. Got non-MultiWriter")
222222
}
223-
224223
klog.V(4).Infof("Compatible disk already exists")
225224
return ValidateDiskParameters(resp, params)
226225
}

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

Lines changed: 33 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ type GCECompute interface {
9999
GetDefaultProject() string
100100
GetDefaultZone() string
101101
// Disk Methods
102-
GetDisk(ctx context.Context, project string, volumeKey *meta.Key, gceAPIVersion GCEAPIVersion) (*CloudDisk, error)
102+
GetDisk(ctx context.Context, project string, volumeKey *meta.Key) (*CloudDisk, error)
103103
RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error)
104104
ValidateExistingDisk(ctx context.Context, disk *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool) error
105105
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,28 +321,16 @@ func (cloud *CloudProvider) ListSnapshots(ctx context.Context, filter string) ([
321321
return items, "", nil
322322
}
323323

324-
func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *meta.Key, gceAPIVersion GCEAPIVersion) (*CloudDisk, error) {
324+
func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *meta.Key) (*CloudDisk, error) {
325325
klog.V(5).Infof("Getting disk %v", key)
326326

327-
// Override GCEAPIVersion as hyperdisk is only available in beta and we cannot get the disk-type with get disk call.
328-
gceAPIVersion = GCEAPIVersionBeta
329327
switch key.Type() {
330328
case meta.Zonal:
331-
if gceAPIVersion == GCEAPIVersionBeta {
332-
disk, err := cloud.getZonalBetaDiskOrError(ctx, project, key.Zone, key.Name)
333-
return CloudDiskFromBeta(disk), err
334-
} else {
335-
disk, err := cloud.getZonalDiskOrError(ctx, project, key.Zone, key.Name)
336-
return CloudDiskFromV1(disk), err
337-
}
329+
disk, err := cloud.getZonalBetaDiskOrError(ctx, project, key.Zone, key.Name)
330+
return CloudDiskFromBeta(disk), err
338331
case meta.Regional:
339-
if gceAPIVersion == GCEAPIVersionBeta {
340-
disk, err := cloud.getRegionalBetaDiskOrError(ctx, project, key.Region, key.Name)
341-
return CloudDiskFromBeta(disk), err
342-
} else {
343-
disk, err := cloud.getRegionalDiskOrError(ctx, project, key.Region, key.Name)
344-
return CloudDiskFromV1(disk), err
345-
}
332+
disk, err := cloud.getRegionalBetaDiskOrError(ctx, project, key.Region, key.Name)
333+
return CloudDiskFromBeta(disk), err
346334
default:
347335
return nil, fmt.Errorf("key was neither zonal nor regional, got: %v", key.String())
348336
}
@@ -553,9 +541,6 @@ func convertV1DiskToBetaDisk(v1Disk *computev1.Disk) *computebeta.Disk {
553541
AccessMode: v1Disk.AccessMode,
554542
}
555543

556-
// Hyperdisk doesn't currently support multiWriter (https://cloud.google.com/compute/docs/disks/hyperdisks#limitations),
557-
// but if multiWriter + hyperdisk is supported in the future, we want the PDCSI driver to support this feature without
558-
// any additional code change.
559544
if v1Disk.ProvisionedIops > 0 {
560545
betaDisk.ProvisionedIops = v1Disk.ProvisionedIops
561546
}
@@ -619,9 +604,6 @@ func convertBetaDiskToV1Disk(betaDisk *computebeta.Disk) *computev1.Disk {
619604
AccessMode: betaDisk.AccessMode,
620605
}
621606

622-
// Hyperdisk doesn't currently support multiWriter (https://cloud.google.com/compute/docs/disks/hyperdisks#limitations),
623-
// but if multiWriter + hyperdisk is supported in the future, we want the PDCSI driver to support this feature without
624-
// any additional code change.
625607
if betaDisk.ProvisionedIops > 0 {
626608
v1Disk.ProvisionedIops = betaDisk.ProvisionedIops
627609
}
@@ -646,16 +628,11 @@ func (cloud *CloudProvider) insertRegionalDisk(
646628
description string,
647629
multiWriter bool) error {
648630
var (
649-
err error
650-
opName string
651-
gceAPIVersion = GCEAPIVersionV1
631+
err error
632+
opName string
652633
)
653634

654-
if multiWriter {
655-
gceAPIVersion = GCEAPIVersionBeta
656-
}
657-
658-
diskToCreate := &computev1.Disk{
635+
diskToCreate := &computebeta.Disk{
659636
Name: volKey.Name,
660637
SizeGb: common.BytesToGbRoundUp(capBytes),
661638
Description: description,
@@ -684,7 +661,7 @@ func (cloud *CloudProvider) insertRegionalDisk(
684661
diskToCreate.ReplicaZones = replicaZones
685662
}
686663
if params.DiskEncryptionKMSKey != "" {
687-
diskToCreate.DiskEncryptionKey = &computev1.CustomerEncryptionKey{
664+
diskToCreate.DiskEncryptionKey = &computebeta.CustomerEncryptionKey{
688665
KmsKeyName: params.DiskEncryptionKMSKey,
689666
}
690667
}
@@ -694,29 +671,21 @@ func (cloud *CloudProvider) insertRegionalDisk(
694671
}
695672

696673
if len(resourceTags) > 0 {
697-
diskToCreate.Params = &computev1.DiskParams{
674+
diskToCreate.Params = &computebeta.DiskParams{
698675
ResourceManagerTags: resourceTags,
699676
}
700677
}
701678

702-
if gceAPIVersion == GCEAPIVersionBeta {
703-
var insertOp *computebeta.Operation
704-
betaDiskToCreate := convertV1DiskToBetaDisk(diskToCreate)
705-
betaDiskToCreate.MultiWriter = multiWriter
706-
insertOp, err = cloud.betaService.RegionDisks.Insert(project, volKey.Region, betaDiskToCreate).Context(ctx).Do()
707-
if insertOp != nil {
708-
opName = insertOp.Name
709-
}
710-
} else {
711-
var insertOp *computev1.Operation
712-
insertOp, err = cloud.service.RegionDisks.Insert(project, volKey.Region, diskToCreate).Context(ctx).Do()
713-
if insertOp != nil {
714-
opName = insertOp.Name
715-
}
679+
var insertOp *computebeta.Operation
680+
diskToCreate.MultiWriter = multiWriter
681+
insertOp, err = cloud.betaService.RegionDisks.Insert(project, volKey.Region, diskToCreate).Context(ctx).Do()
682+
if insertOp != nil {
683+
opName = insertOp.Name
716684
}
685+
717686
if err != nil {
718687
if IsGCEError(err, "alreadyExists") {
719-
disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion)
688+
disk, err := cloud.GetDisk(ctx, project, volKey)
720689
if err != nil {
721690
// failed to GetDisk, however the Disk may already exist
722691
// the error code should be non-Final
@@ -742,7 +711,7 @@ func (cloud *CloudProvider) insertRegionalDisk(
742711
// the error code returned should be non-final
743712
if err != nil {
744713
if IsGCEError(err, "alreadyExists") {
745-
disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion)
714+
disk, err := cloud.GetDisk(ctx, project, volKey)
746715
if err != nil {
747716
return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err))
748717
}
@@ -774,15 +743,11 @@ func (cloud *CloudProvider) insertZonalDisk(
774743
multiWriter bool,
775744
accessMode string) error {
776745
var (
777-
err error
778-
opName string
779-
gceAPIVersion = GCEAPIVersionV1
746+
err error
747+
opName string
780748
)
781-
if multiWriter {
782-
gceAPIVersion = GCEAPIVersionBeta
783-
}
784749

785-
diskToCreate := &computev1.Disk{
750+
diskToCreate := &computebeta.Disk{
786751
Name: volKey.Name,
787752
SizeGb: common.BytesToGbRoundUp(capBytes),
788753
Description: description,
@@ -824,7 +789,7 @@ func (cloud *CloudProvider) insertZonalDisk(
824789
}
825790

826791
if params.DiskEncryptionKMSKey != "" {
827-
diskToCreate.DiskEncryptionKey = &computev1.CustomerEncryptionKey{
792+
diskToCreate.DiskEncryptionKey = &computebeta.CustomerEncryptionKey{
828793
KmsKeyName: params.DiskEncryptionKMSKey,
829794
}
830795
}
@@ -836,31 +801,21 @@ func (cloud *CloudProvider) insertZonalDisk(
836801
}
837802

838803
if len(resourceTags) > 0 {
839-
diskToCreate.Params = &computev1.DiskParams{
804+
diskToCreate.Params = &computebeta.DiskParams{
840805
ResourceManagerTags: resourceTags,
841806
}
842807
}
843-
diskToCreate.AccessMode = accessMode
844808

845-
if gceAPIVersion == GCEAPIVersionBeta {
846-
var insertOp *computebeta.Operation
847-
betaDiskToCreate := convertV1DiskToBetaDisk(diskToCreate)
848-
betaDiskToCreate.MultiWriter = multiWriter
849-
insertOp, err = cloud.betaService.Disks.Insert(project, volKey.Zone, betaDiskToCreate).Context(ctx).Do()
850-
if insertOp != nil {
851-
opName = insertOp.Name
852-
}
853-
} else {
854-
var insertOp *computev1.Operation
855-
insertOp, err = cloud.service.Disks.Insert(project, volKey.Zone, diskToCreate).Context(ctx).Do()
856-
if insertOp != nil {
857-
opName = insertOp.Name
858-
}
809+
diskToCreate.AccessMode = accessMode
810+
var insertOp *computebeta.Operation
811+
insertOp, err = cloud.betaService.Disks.Insert(project, volKey.Zone, diskToCreate).Context(ctx).Do()
812+
if insertOp != nil {
813+
opName = insertOp.Name
859814
}
860815

861816
if err != nil {
862817
if IsGCEError(err, "alreadyExists") {
863-
disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion)
818+
disk, err := cloud.GetDisk(ctx, project, volKey)
864819
if err != nil {
865820
// failed to GetDisk, however the Disk may already exist
866821
// the error code should be non-Final
@@ -887,7 +842,7 @@ func (cloud *CloudProvider) insertZonalDisk(
887842
// failed to wait for Op to finish, however, the Op possibly is still running as expected
888843
// the error code returned should be non-final
889844
if IsGCEError(err, "alreadyExists") {
890-
disk, err := cloud.GetDisk(ctx, project, volKey, gceAPIVersion)
845+
disk, err := cloud.GetDisk(ctx, project, volKey)
891846
if err != nil {
892847
return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err))
893848
}
@@ -1186,7 +1141,7 @@ func (cloud *CloudProvider) waitForAttachOnDisk(ctx context.Context, project str
11861141
start := time.Now()
11871142
return wait.ExponentialBackoff(AttachDiskBackoff, func() (bool, error) {
11881143
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))
1189-
disk, err := cloud.GetDisk(ctx, project, volKey, GCEAPIVersionV1)
1144+
disk, err := cloud.GetDisk(ctx, project, volKey)
11901145
if err != nil {
11911146
return false, fmt.Errorf("GetDisk failed to get disk: %w", err)
11921147
}
@@ -1436,7 +1391,7 @@ func (cloud *CloudProvider) DeleteImage(ctx context.Context, project, imageName
14361391
// k8s.io/apimachinery/quantity package for better size handling
14371392
func (cloud *CloudProvider) ResizeDisk(ctx context.Context, project string, volKey *meta.Key, requestBytes int64) (int64, error) {
14381393
klog.V(5).Infof("Resizing disk %v to size %v", volKey, requestBytes)
1439-
cloudDisk, err := cloud.GetDisk(ctx, project, volKey, GCEAPIVersionV1)
1394+
cloudDisk, err := cloud.GetDisk(ctx, project, volKey)
14401395
if err != nil {
14411396
return -1, fmt.Errorf("failed to get disk: %w", err)
14421397
}

0 commit comments

Comments
 (0)