Skip to content

Commit 9d8100d

Browse files
tonyzhctonyzhc
committed
Do not set access mode for HdT and HdE, and disallow multi-attachment
access mode for these types of disks.
1 parent 242f002 commit 9d8100d

File tree

5 files changed

+100
-17
lines changed

5 files changed

+100
-17
lines changed

Diff for: ‎pkg/common/parameters.go

+8-4
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:

Diff for: ‎pkg/gce-pd-csi-driver/controller.go

+22-3
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,10 @@ var (
233233
}
234234
listDisksFieldsWithUsers = append(listDisksFieldsWithoutUsers, "items/users")
235235
disksWithModifiableAccessMode = []string{"hyperdisk-ml"}
236+
disksWithUnsettableAccessMode = map[string]bool{
237+
common.DiskTypeHdE: true,
238+
common.DiskTypeHdT: true,
239+
}
236240
)
237241

238242
func isDiskReady(disk *gce.CloudDisk) (bool, error) {
@@ -374,8 +378,17 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req
374378
return nil, status.Error(codes.InvalidArgument, "VolumeContentSource must be provided when AccessMode is set to read only")
375379
}
376380

377-
if readonly && params.DiskType == common.ParameterHdHADiskType {
378-
return nil, status.Errorf(codes.InvalidArgument, "Invalid access mode for disk type %s", common.ParameterHdHADiskType)
381+
if readonly && params.DiskType == common.DiskTypeHdHA {
382+
return nil, status.Errorf(codes.InvalidArgument, "Invalid access mode for disk type %s", common.DiskTypeHdHA)
383+
}
384+
385+
// Hyperdisk-throughput and hyperdisk-extreme do not support attaching to multiple VMs.
386+
isMultiAttach, err := getMultiAttachementFromCapabilities(volumeCapabilities)
387+
if err != nil {
388+
return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to parse volume capabilities: %v", err)
389+
}
390+
if isMultiAttach && disksWithUnsettableAccessMode[params.DiskType] {
391+
return nil, status.Errorf(codes.InvalidArgument, "Invalid access mode for disk type %s", params.DiskType)
379392
}
380393

381394
// Validate multi-zone provisioning configuration
@@ -602,6 +615,12 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi
602615
if common.IsHyperdisk(params.DiskType) {
603616
if am, err := getHyperdiskAccessModeFromCapabilities(req.GetVolumeCapabilities()); err != nil {
604617
return nil, err
618+
} else if disksWithUnsettableAccessMode[params.DiskType] {
619+
// Disallow multi-attach for HdT and HdE. These checks were done in `createVolumeInternal`,
620+
// but repeating them here future-proves us from possible refactors.
621+
if am != common.GCEReadWriteOnceAccessMode {
622+
return nil, status.Errorf(codes.Internal, "")
623+
}
605624
} else {
606625
accessMode = am
607626
}
@@ -1570,7 +1589,7 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C
15701589
return nil, common.LoggedError("CreateSnapshot, failed to getDisk: ", err)
15711590
}
15721591
if common.IsHyperdisk(disk.GetPDType()) && disk.GetAccessMode() == common.GCEReadWriteManyAccessMode {
1573-
return nil, status.Errorf(codes.InvalidArgument, "Cannot create snapshot for disk type %s with access mode %s", common.ParameterHdHADiskType, common.GCEReadWriteManyAccessMode)
1592+
return nil, status.Errorf(codes.InvalidArgument, "Cannot create snapshot for disk type %s with access mode %s", common.DiskTypeHdHA, common.GCEReadWriteManyAccessMode)
15741593
}
15751594

15761595
snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraTags)

Diff for: ‎pkg/gce-pd-csi-driver/controller_test.go

+45-8
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ func TestCreateSnapshotArguments(t *testing.T) {
236236
gce.CloudDiskFromV1(&compute.Disk{
237237
Name: name,
238238
SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name),
239-
Type: common.ParameterHdHADiskType,
239+
Type: common.DiskTypeHdHA,
240240
Region: "country-region",
241241
}),
242242
},
@@ -253,7 +253,7 @@ func TestCreateSnapshotArguments(t *testing.T) {
253253
gce.CloudDiskFromV1(&compute.Disk{
254254
Name: name,
255255
SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name),
256-
Type: common.ParameterHdHADiskType,
256+
Type: common.DiskTypeHdHA,
257257
Region: "country-region",
258258
}),
259259
},
@@ -276,7 +276,7 @@ func TestCreateSnapshotArguments(t *testing.T) {
276276
gce.CloudDiskFromV1(&compute.Disk{
277277
Name: name,
278278
SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name),
279-
Type: common.ParameterHdHADiskType,
279+
Type: common.DiskTypeHdHA,
280280
AccessMode: common.GCEReadWriteManyAccessMode,
281281
Region: "country-region",
282282
}),
@@ -921,7 +921,7 @@ func TestCreateVolumeArguments(t *testing.T) {
921921
Name: name,
922922
CapacityRange: stdCapRange,
923923
VolumeCapabilities: stdVolCaps,
924-
Parameters: map[string]string{common.ParameterKeyType: common.ParameterHdHADiskType},
924+
Parameters: map[string]string{common.ParameterKeyType: common.DiskTypeHdHA},
925925
AccessibilityRequirements: &csi.TopologyRequirement{
926926
Preferred: []*csi.Topology{
927927
{
@@ -954,7 +954,7 @@ func TestCreateVolumeArguments(t *testing.T) {
954954
CapacityRange: stdCapRange,
955955
VolumeCapabilities: stdVolCaps,
956956
Parameters: map[string]string{
957-
common.ParameterKeyType: common.ParameterHdHADiskType,
957+
common.ParameterKeyType: common.DiskTypeHdHA,
958958
},
959959
AccessibilityRequirements: &csi.TopologyRequirement{
960960
Requisite: []*csi.Topology{
@@ -978,7 +978,7 @@ func TestCreateVolumeArguments(t *testing.T) {
978978
CapacityRange: stdCapRange,
979979
VolumeCapabilities: stdVolCaps,
980980
Parameters: map[string]string{
981-
common.ParameterKeyType: common.ParameterHdHADiskType,
981+
common.ParameterKeyType: common.DiskTypeHdHA,
982982
},
983983
},
984984
expVol: &csi.Volume{
@@ -1262,6 +1262,42 @@ func TestCreateVolumeArguments(t *testing.T) {
12621262
CapacityBytes: MinimumVolumeSizeInBytes,
12631263
},
12641264
},
1265+
{
1266+
name: "fail with hyperdisk-throughput access mode ROX",
1267+
req: &csi.CreateVolumeRequest{
1268+
Name: name,
1269+
Parameters: map[string]string{"type": "hyperdisk-throughput"},
1270+
VolumeCapabilities: []*csi.VolumeCapability{
1271+
{
1272+
AccessType: &csi.VolumeCapability_Block{
1273+
Block: &csi.VolumeCapability_BlockVolume{},
1274+
},
1275+
AccessMode: &csi.VolumeCapability_AccessMode{
1276+
Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY,
1277+
},
1278+
},
1279+
},
1280+
},
1281+
expErrCode: codes.InvalidArgument,
1282+
},
1283+
{
1284+
name: "fail with hyperdisk-extreme access mode RWX",
1285+
req: &csi.CreateVolumeRequest{
1286+
Name: name,
1287+
Parameters: map[string]string{"type": "hyperdisk-extreme"},
1288+
VolumeCapabilities: []*csi.VolumeCapability{
1289+
{
1290+
AccessType: &csi.VolumeCapability_Block{
1291+
Block: &csi.VolumeCapability_BlockVolume{},
1292+
},
1293+
AccessMode: &csi.VolumeCapability_AccessMode{
1294+
Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER,
1295+
},
1296+
},
1297+
},
1298+
},
1299+
expErrCode: codes.InvalidArgument,
1300+
},
12651301
}
12661302

12671303
// Run test cases
@@ -2454,8 +2490,9 @@ func TestVolumeModifyErrorHandling(t *testing.T) {
24542490
resp, err := gceDriver.cs.CreateVolume(context.Background(), tc.createReq)
24552491
if err != nil {
24562492
t.Errorf("Expected no error, got %v", err)
2493+
} else {
2494+
volId = resp.GetVolume().VolumeId
24572495
}
2458-
volId = resp.GetVolume().VolumeId
24592496
}
24602497

24612498
tc.modifyReq.VolumeId = volId
@@ -3047,7 +3084,7 @@ func TestCloningLocationRequirements(t *testing.T) {
30473084
sourceVolumeID: testZonalVolumeSourceID,
30483085
requestCapacityRange: stdCapRange,
30493086
reqParameters: map[string]string{
3050-
common.ParameterKeyType: common.ParameterHdHADiskType,
3087+
common.ParameterKeyType: common.DiskTypeHdHA,
30513088
},
30523089
cloneIsRegional: true,
30533090
expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcIsRegional: false, cloneIsRegional: true},

Diff for: ‎pkg/gce-pd-csi-driver/utils.go

+23
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ var (
4141
csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY: common.GCEReadOnlyManyAccessMode,
4242
csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER: common.GCEReadWriteManyAccessMode,
4343
}
44+
45+
supportedMultiAttachAccessModes = map[csi.VolumeCapability_AccessMode_Mode]bool{
46+
csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY: true,
47+
csi.VolumeCapability_AccessMode_MULTI_NODE_MULTI_WRITER: true,
48+
}
4449
)
4550

4651
func NewVolumeCapabilityAccessMode(mode csi.VolumeCapability_AccessMode_Mode) *csi.VolumeCapability_AccessMode {
@@ -261,6 +266,24 @@ func getReadOnlyFromCapabilities(vcs []*csi.VolumeCapability) (bool, error) {
261266
return false, nil
262267
}
263268

269+
func getMultiAttachementFromCapabilities(vcs []*csi.VolumeCapability) (bool, error) {
270+
if vcs == nil {
271+
return false, errors.New("volume capabilities is nil")
272+
}
273+
for _, vc := range vcs {
274+
if vc.GetAccessMode() == nil {
275+
return false, errors.New("access mode is nil")
276+
}
277+
mode := vc.GetAccessMode().GetMode()
278+
if isMultiAttach, ok := supportedMultiAttachAccessModes[mode]; !ok {
279+
return false, nil
280+
} else {
281+
return isMultiAttach, nil
282+
}
283+
}
284+
return false, nil
285+
}
286+
264287
func getHyperdiskAccessModeFromCapabilities(vcs []*csi.VolumeCapability) (string, error) {
265288
if vcs == nil {
266289
return "", errors.New("volume capabilities is nil")

Diff for: ‎test/e2e/tests/multi_zone_e2e_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -1065,7 +1065,7 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() {
10651065
// Create Disk
10661066
volName := testNamePrefix + string(uuid.NewUUID())
10671067
volume, err := controllerClient.CreateVolume(volName, map[string]string{
1068-
common.ParameterKeyType: common.ParameterHdHADiskType,
1068+
common.ParameterKeyType: common.DiskTypeHdHA,
10691069
}, defaultRepdSizeGb, &csi.TopologyRequirement{
10701070
Requisite: []*csi.Topology{
10711071
{
@@ -1144,7 +1144,7 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() {
11441144
// Create Disk
11451145
volName := testNamePrefix + string(uuid.NewUUID())
11461146
volume, err := controllerClient.CreateVolume(volName, map[string]string{
1147-
common.ParameterKeyType: common.ParameterHdHADiskType,
1147+
common.ParameterKeyType: common.DiskTypeHdHA,
11481148
common.ParameterAvailabilityClass: common.ParameterRegionalHardFailoverClass,
11491149
}, defaultRepdSizeGb, &csi.TopologyRequirement{
11501150
Requisite: []*csi.Topology{

0 commit comments

Comments
 (0)