Skip to content

Commit 39a5910

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1945 from tonyzhc/hdha
Derive access mode from the CreateVolume request instead of parameters
2 parents 84e1b15 + 9d8100d commit 39a5910

File tree

16 files changed

+2244
-121
lines changed

16 files changed

+2244
-121
lines changed

‎pkg/common/constants.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ const (
3333
// Label that is set on a disk when it is used by a 'multi-zone' VolumeHandle
3434
MultiZoneLabel = "goog-gke-multi-zone"
3535

36+
// GCE Access Modes that are valid for hyperdisks only.
37+
GCEReadOnlyManyAccessMode = "READ_ONLY_MANY"
38+
GCEReadWriteManyAccessMode = "READ_WRITE_MANY"
39+
GCEReadWriteOnceAccessMode = "READ_WRITE_SINGLE"
40+
3641
// Data cache mode
3742
DataCacheModeWriteBack = "writeback"
3843
DataCacheModeWriteThrough = "writethrough"

‎pkg/common/parameters.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,6 @@ const (
4242
ParameterKeyDataCacheMode = "data-cache-mode"
4343
ParameterKeyResourceTags = "resource-tags"
4444
ParameterKeyEnableMultiZoneProvisioning = "enable-multi-zone-provisioning"
45-
ParameterHdHADiskType = "hyperdisk-balanced-high-availability"
4645

4746
// Parameters for VolumeSnapshotClass
4847
ParameterKeyStorageLocations = "storage-locations"
@@ -76,6 +75,11 @@ const (
7675
tagKeyCreatedForSnapshotName = "kubernetes.io/created-for/volumesnapshot/name"
7776
tagKeyCreatedForSnapshotNamespace = "kubernetes.io/created-for/volumesnapshot/namespace"
7877
tagKeyCreatedForSnapshotContentName = "kubernetes.io/created-for/volumesnapshotcontent/name"
78+
79+
// Hyperdisk disk types
80+
DiskTypeHdHA = "hyperdisk-balanced-high-availability"
81+
DiskTypeHdT = "hyperdisk-throughput"
82+
DiskTypeHdE = "hyperdisk-extreme"
7983
)
8084

8185
type DataCacheParameters struct {
@@ -130,7 +134,7 @@ type DiskParameters struct {
130134
}
131135

132136
func (dp *DiskParameters) IsRegional() bool {
133-
return dp.ReplicationType == "regional-pd" || dp.DiskType == ParameterHdHADiskType
137+
return dp.ReplicationType == "regional-pd" || dp.DiskType == DiskTypeHdHA
134138
}
135139

136140
// SnapshotParameters contains normalized and defaulted parameters for snapshots
@@ -200,8 +204,8 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string]
200204
case ParameterKeyType:
201205
if v != "" {
202206
p.DiskType = strings.ToLower(v)
203-
if !pp.EnableHdHA && p.DiskType == ParameterHdHADiskType {
204-
return p, d, fmt.Errorf("parameters contain invalid disk type %s", ParameterHdHADiskType)
207+
if !pp.EnableHdHA && p.DiskType == DiskTypeHdHA {
208+
return p, d, fmt.Errorf("parameters contain invalid disk type %s", DiskTypeHdHA)
205209
}
206210
}
207211
case ParameterKeyReplicationType:

‎pkg/common/utils.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -726,6 +726,10 @@ func NewLimiter(limit, burst int, emptyBucket bool) *rate.Limiter {
726726
return limiter
727727
}
728728

729+
func IsHyperdisk(diskType string) bool {
730+
return strings.HasPrefix(diskType, "hyperdisk-")
731+
}
732+
729733
// shortString is inspired by k8s.io/apimachinery/pkg/util/rand.SafeEncodeString, but takes data from a hash.
730734
func ShortString(s string) string {
731735
hasher := fnv.New128a()

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

Lines changed: 18 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323

2424
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
2525
csi "github.com/container-storage-interface/spec/lib/go/csi"
26+
computebeta "google.golang.org/api/compute/v0.beta"
2627
computev1 "google.golang.org/api/compute/v1"
2728
"google.golang.org/api/googleapi"
2829
"google.golang.org/grpc/codes"
@@ -196,55 +197,30 @@ func (cloud *FakeCloudProvider) GetDisk(ctx context.Context, project string, vol
196197
return disk, nil
197198
}
198199

199-
func (cloud *FakeCloudProvider) ValidateExistingDisk(ctx context.Context, resp *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool) error {
200-
if resp == nil {
201-
return fmt.Errorf("disk does not exist")
202-
}
203-
requestValid := common.GbToBytes(resp.GetSizeGb()) >= reqBytes || reqBytes == 0
204-
responseValid := common.GbToBytes(resp.GetSizeGb()) <= limBytes || limBytes == 0
205-
if !requestValid || !responseValid {
206-
return fmt.Errorf(
207-
"disk already exists with incompatible capacity. Need %v (Required) < %v (Existing) < %v (Limit)",
208-
reqBytes, common.GbToBytes(resp.GetSizeGb()), limBytes)
209-
}
210-
211-
respType := strings.Split(resp.GetPDType(), "/")
212-
typeMatch := strings.TrimSpace(respType[len(respType)-1]) == strings.TrimSpace(params.DiskType)
213-
typeDefault := params.DiskType == "" && strings.TrimSpace(respType[len(respType)-1]) == "pd-standard"
214-
if !typeMatch && !typeDefault {
215-
return fmt.Errorf("disk already exists with incompatible type. Need %v. Got %v",
216-
params.DiskType, respType[len(respType)-1])
217-
}
218-
219-
// We are assuming here that a multiWriter disk could be used as non-multiWriter
220-
if multiWriter && !resp.GetMultiWriter() {
221-
return fmt.Errorf("disk already exists with incompatible capability. Need MultiWriter. Got non-MultiWriter")
222-
}
223-
klog.V(4).Infof("Compatible disk already exists")
224-
return ValidateDiskParameters(resp, params)
225-
}
226-
227200
func (cloud *FakeCloudProvider) 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 {
228201
if disk, ok := cloud.disks[volKey.String()]; ok {
229-
err := cloud.ValidateExistingDisk(ctx, disk, params,
202+
err := ValidateExistingDisk(ctx, disk, params,
230203
int64(capacityRange.GetRequiredBytes()),
231204
int64(capacityRange.GetLimitBytes()),
232-
multiWriter)
205+
multiWriter, accessMode)
233206
if err != nil {
234207
return err
235208
}
236209
}
237210

238-
computeDisk := &computev1.Disk{
239-
Name: volKey.Name,
240-
SizeGb: common.BytesToGbRoundUp(capBytes),
241-
Description: "Disk created by GCE-PD CSI Driver",
242-
Type: cloud.GetDiskTypeURI(project, volKey, params.DiskType),
243-
SourceDiskId: volumeContentSourceVolumeID,
244-
Status: cloud.mockDiskStatus,
245-
Labels: params.Labels,
246-
ProvisionedIops: params.ProvisionedIOPSOnCreate,
247-
ProvisionedThroughput: params.ProvisionedThroughputOnCreate,
211+
computeDisk := &computebeta.Disk{
212+
Name: volKey.Name,
213+
SizeGb: common.BytesToGbRoundUp(capBytes),
214+
Description: "Disk created by GCE-PD CSI Driver",
215+
Type: cloud.GetDiskTypeURI(project, volKey, params.DiskType),
216+
SourceDiskId: volumeContentSourceVolumeID,
217+
Status: cloud.mockDiskStatus,
218+
Labels: params.Labels,
219+
ProvisionedIops: params.ProvisionedIOPSOnCreate,
220+
ProvisionedThroughput: params.ProvisionedThroughputOnCreate,
221+
AccessMode: accessMode,
222+
MultiWriter: multiWriter,
223+
EnableConfidentialCompute: params.EnableConfidentialCompute,
248224
}
249225

250226
if snapshotID != "" {
@@ -263,7 +239,7 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string,
263239
}
264240

265241
if params.DiskEncryptionKMSKey != "" {
266-
computeDisk.DiskEncryptionKey = &computev1.CustomerEncryptionKey{
242+
computeDisk.DiskEncryptionKey = &computebeta.CustomerEncryptionKey{
267243
KmsKeyName: params.DiskEncryptionKMSKey,
268244
}
269245
}
@@ -278,13 +254,7 @@ func (cloud *FakeCloudProvider) InsertDisk(ctx context.Context, project string,
278254
return fmt.Errorf("could not create disk, key was neither zonal nor regional, instead got: %v", volKey.String())
279255
}
280256

281-
if containsBetaDiskType(hyperdiskTypes, params.DiskType) {
282-
betaDisk := convertV1DiskToBetaDisk(computeDisk)
283-
betaDisk.EnableConfidentialCompute = params.EnableConfidentialCompute
284-
cloud.disks[volKey.String()] = CloudDiskFromBeta(betaDisk)
285-
} else {
286-
cloud.disks[volKey.String()] = CloudDiskFromV1(computeDisk)
287-
}
257+
cloud.disks[volKey.String()] = CloudDiskFromBeta(computeDisk)
288258
return nil
289259
}
290260

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

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ const (
5353
pdDiskTypeUnsupportedPattern = `\[([a-z-]+)\] features are not compatible for creating instance`
5454
)
5555

56-
var hyperdiskTypes = []string{"hyperdisk-extreme", "hyperdisk-throughput", "hyperdisk-balanced"}
5756
var pdDiskTypeUnsupportedRegex = regexp.MustCompile(pdDiskTypeUnsupportedPattern)
5857

5958
type GCEAPIVersion string
@@ -101,7 +100,6 @@ type GCECompute interface {
101100
// Disk Methods
102101
GetDisk(ctx context.Context, project string, volumeKey *meta.Key) (*CloudDisk, error)
103102
RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error)
104-
ValidateExistingDisk(ctx context.Context, disk *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool) error
105103
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
106104
DeleteDisk(ctx context.Context, project string, volumeKey *meta.Key) error
107105
UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error
@@ -382,7 +380,7 @@ func (cloud *CloudProvider) getRegionURI(project, region string) string {
382380
region)
383381
}
384382

385-
func (cloud *CloudProvider) ValidateExistingDisk(ctx context.Context, resp *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool) error {
383+
func ValidateExistingDisk(ctx context.Context, resp *CloudDisk, params common.DiskParameters, reqBytes, limBytes int64, multiWriter bool, accessMode string) error {
386384
klog.V(5).Infof("Validating existing disk %v with diskType: %s, reqested bytes: %v, limit bytes: %v", resp, params.DiskType, reqBytes, limBytes)
387385
if resp == nil {
388386
return fmt.Errorf("disk does not exist")
@@ -395,14 +393,31 @@ func (cloud *CloudProvider) ValidateExistingDisk(ctx context.Context, resp *Clou
395393
reqBytes, common.GbToBytes(resp.GetSizeGb()), limBytes)
396394
}
397395

398-
// We are assuming here that a multiWriter disk could be used as non-multiWriter
399-
if multiWriter && !resp.GetMultiWriter() {
396+
if common.IsHyperdisk(params.DiskType) {
397+
if !validAccessMode(accessMode, resp.GetAccessMode()) {
398+
return fmt.Errorf("disk already exists with incompatible capability. Need %s. Got %s", accessMode, resp.GetAccessMode())
399+
}
400+
} else if multiWriter && !resp.GetMultiWriter() {
401+
// We are assuming here that a multiWriter PD could be used as non-multiWriter
400402
return fmt.Errorf("disk already exists with incompatible capability. Need MultiWriter. Got non-MultiWriter")
401403
}
402404

403405
return ValidateDiskParameters(resp, params)
404406
}
405407

408+
func validAccessMode(want, got string) bool {
409+
if want == got {
410+
return true
411+
}
412+
switch want {
413+
case common.GCEReadOnlyManyAccessMode, common.GCEReadWriteOnceAccessMode:
414+
return got == common.GCEReadWriteManyAccessMode
415+
// For RWX, no other access mode is valid.
416+
default:
417+
return false
418+
}
419+
}
420+
406421
// ValidateDiskParameters takes a CloudDisk and returns true if the parameters
407422
// specified validly describe the disk provided, and false otherwise.
408423
func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error {
@@ -442,7 +457,7 @@ func (cloud *CloudProvider) InsertDisk(ctx context.Context, project string, volK
442457
if description == "" {
443458
description = "Regional disk created by GCE-PD CSI Driver"
444459
}
445-
return cloud.insertRegionalDisk(ctx, project, volKey, params, capBytes, capacityRange, replicaZones, snapshotID, volumeContentSourceVolumeID, description, multiWriter)
460+
return cloud.insertRegionalDisk(ctx, project, volKey, params, capBytes, capacityRange, replicaZones, snapshotID, volumeContentSourceVolumeID, description, multiWriter, accessMode)
446461
default:
447462
return fmt.Errorf("could not insert disk, key was neither zonal nor regional, instead got: %v", volKey.String())
448463
}
@@ -626,7 +641,8 @@ func (cloud *CloudProvider) insertRegionalDisk(
626641
snapshotID string,
627642
volumeContentSourceVolumeID string,
628643
description string,
629-
multiWriter bool) error {
644+
multiWriter bool,
645+
accessMode string) error {
630646
var (
631647
err error
632648
opName string
@@ -676,8 +692,13 @@ func (cloud *CloudProvider) insertRegionalDisk(
676692
}
677693
}
678694

695+
if common.IsHyperdisk(params.DiskType) {
696+
diskToCreate.AccessMode = accessMode
697+
} else {
698+
diskToCreate.MultiWriter = multiWriter
699+
}
700+
679701
var insertOp *computebeta.Operation
680-
diskToCreate.MultiWriter = multiWriter
681702
insertOp, err = cloud.betaService.RegionDisks.Insert(project, volKey.Region, diskToCreate).Context(ctx).Do()
682703
if insertOp != nil {
683704
opName = insertOp.Name
@@ -691,10 +712,10 @@ func (cloud *CloudProvider) insertRegionalDisk(
691712
// the error code should be non-Final
692713
return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err))
693714
}
694-
err = cloud.ValidateExistingDisk(ctx, disk, params,
715+
err = ValidateExistingDisk(ctx, disk, params,
695716
int64(capacityRange.GetRequiredBytes()),
696717
int64(capacityRange.GetLimitBytes()),
697-
multiWriter)
718+
multiWriter, accessMode)
698719
if err != nil {
699720
return err
700721
}
@@ -715,10 +736,10 @@ func (cloud *CloudProvider) insertRegionalDisk(
715736
if err != nil {
716737
return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err))
717738
}
718-
err = cloud.ValidateExistingDisk(ctx, disk, params,
739+
err = ValidateExistingDisk(ctx, disk, params,
719740
int64(capacityRange.GetRequiredBytes()),
720741
int64(capacityRange.GetLimitBytes()),
721-
multiWriter)
742+
multiWriter, accessMode)
722743
if err != nil {
723744
return err
724745
}
@@ -806,7 +827,12 @@ func (cloud *CloudProvider) insertZonalDisk(
806827
}
807828
}
808829

809-
diskToCreate.AccessMode = accessMode
830+
if common.IsHyperdisk(params.DiskType) {
831+
diskToCreate.AccessMode = accessMode
832+
} else {
833+
diskToCreate.MultiWriter = multiWriter
834+
}
835+
810836
var insertOp *computebeta.Operation
811837
insertOp, err = cloud.betaService.Disks.Insert(project, volKey.Zone, diskToCreate).Context(ctx).Do()
812838
if insertOp != nil {
@@ -821,10 +847,10 @@ func (cloud *CloudProvider) insertZonalDisk(
821847
// the error code should be non-Final
822848
return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err))
823849
}
824-
err = cloud.ValidateExistingDisk(ctx, disk, params,
850+
err = ValidateExistingDisk(ctx, disk, params,
825851
int64(capacityRange.GetRequiredBytes()),
826852
int64(capacityRange.GetLimitBytes()),
827-
multiWriter)
853+
multiWriter, accessMode)
828854
if err != nil {
829855
return err
830856
}
@@ -846,10 +872,10 @@ func (cloud *CloudProvider) insertZonalDisk(
846872
if err != nil {
847873
return common.NewTemporaryError(codes.Unavailable, fmt.Errorf("error when getting disk: %w", err))
848874
}
849-
err = cloud.ValidateExistingDisk(ctx, disk, params,
875+
err = ValidateExistingDisk(ctx, disk, params,
850876
int64(capacityRange.GetRequiredBytes()),
851877
int64(capacityRange.GetLimitBytes()),
852-
multiWriter)
878+
multiWriter, accessMode)
853879
if err != nil {
854880
return err
855881
}
@@ -1687,13 +1713,3 @@ func encodeTags(tags map[string]string) (string, error) {
16871713
}
16881714
return string(enc), nil
16891715
}
1690-
1691-
func containsBetaDiskType(betaDiskTypes []string, diskType string) bool {
1692-
for _, betaDiskType := range betaDiskTypes {
1693-
if betaDiskType == diskType {
1694-
return true
1695-
}
1696-
}
1697-
1698-
return false
1699-
}

0 commit comments

Comments
 (0)