Skip to content

Commit 2ec1e10

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1975 from pwschuurman/pdcsi-rox-multi-zone-create
Relax volumeContentSource restriction for ROX multi-zone dynamic volume creation
2 parents 193aa66 + 297c9ee commit 2ec1e10

File tree

4 files changed

+368
-51
lines changed

4 files changed

+368
-51
lines changed

Diff for: ‎pkg/common/parameters.go

+1
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ const (
8080
DiskTypeHdHA = "hyperdisk-balanced-high-availability"
8181
DiskTypeHdT = "hyperdisk-throughput"
8282
DiskTypeHdE = "hyperdisk-extreme"
83+
DiskTypeHdML = "hyperdisk-ml"
8384
)
8485

8586
type DataCacheParameters struct {

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

+41-15
Original file line numberDiff line numberDiff line change
@@ -232,7 +232,7 @@ var (
232232
"nextPageToken",
233233
}
234234
listDisksFieldsWithUsers = append(listDisksFieldsWithoutUsers, "items/users")
235-
disksWithModifiableAccessMode = []string{"hyperdisk-ml"}
235+
disksWithModifiableAccessMode = []string{common.DiskTypeHdML}
236236
disksWithUnsettableAccessMode = map[string]bool{
237237
common.DiskTypeHdE: true,
238238
common.DiskTypeHdT: true,
@@ -374,7 +374,8 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req
374374

375375
// Validate VolumeContentSource is set when access mode is read only
376376
readonly, _ := getReadOnlyFromCapabilities(volumeCapabilities)
377-
if readonly && req.GetVolumeContentSource() == nil {
377+
378+
if readonly && req.GetVolumeContentSource() == nil && params.DiskType != common.DiskTypeHdML {
378379
return nil, status.Error(codes.InvalidArgument, "VolumeContentSource must be provided when AccessMode is set to read only")
379380
}
380381

@@ -485,12 +486,19 @@ func (gceCS *GCEControllerServer) createMultiZoneDisk(ctx context.Context, req *
485486
}
486487
defer gceCS.volumeLocks.Release(volumeID)
487488

489+
// If creating an empty disk (content source nil), always create RWO disks (when supported)
490+
// This allows disks to be created as underlying RWO disks, so they can be hydrated.
491+
accessMode := common.GCEReadWriteOnceAccessMode
492+
if req.GetVolumeContentSource() != nil {
493+
accessMode = common.GCEReadOnlyManyAccessMode
494+
}
495+
488496
createDiskErrs := []error{}
489497
createdDisks := make([]*gce.CloudDisk, 0, len(zones))
490498
for _, zone := range zones {
491499
volKey := meta.ZonalKey(req.GetName(), zone)
492500
klog.V(4).Infof("Creating single zone disk for zone %q and volume: %v", zone, volKey)
493-
disk, err := gceCS.createSingleDisk(ctx, req, params, volKey, []string{zone})
501+
disk, err := gceCS.createSingleDisk(ctx, req, params, volKey, []string{zone}, accessMode)
494502
if err != nil {
495503
createDiskErrs = append(createDiskErrs, err)
496504
continue
@@ -593,11 +601,23 @@ func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, re
593601
if err != nil {
594602
return nil, common.LoggedError("Failed to convert volume key to volume ID: ", err)
595603
}
604+
accessMode, err := getAccessMode(req, params)
605+
if err != nil {
606+
return nil, common.LoggedError("Failed to get access mode: ", err)
607+
}
608+
609+
// If creating an empty disk (content source nil), always create RWO disks (when supported)
610+
// This allows disks to be created as underlying RWO disks, so they can be hydrated.
611+
readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities())
612+
if readonly && req.GetVolumeContentSource() == nil && params.DiskType == common.DiskTypeHdML {
613+
accessMode = common.GCEReadWriteOnceAccessMode
614+
}
615+
596616
if acquired := gceCS.volumeLocks.TryAcquire(volumeID); !acquired {
597617
return nil, status.Errorf(codes.Aborted, common.VolumeOperationAlreadyExistsFmt, volumeID)
598618
}
599619
defer gceCS.volumeLocks.Release(volumeID)
600-
disk, err := gceCS.createSingleDisk(ctx, req, params, volKey, zones)
620+
disk, err := gceCS.createSingleDisk(ctx, req, params, volKey, zones, accessMode)
601621

602622
if err != nil {
603623
return nil, common.LoggedError("CreateVolume failed: %v", err)
@@ -606,30 +626,36 @@ func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, re
606626
return generateCreateVolumeResponseWithVolumeId(disk, zones, params, dataCacheParams, enableDataCache, volumeID), err
607627
}
608628

609-
func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters, volKey *meta.Key, zones []string) (*gce.CloudDisk, error) {
610-
capacityRange := req.GetCapacityRange()
611-
capBytes, _ := getRequestCapacity(capacityRange)
629+
func getAccessMode(req *csi.CreateVolumeRequest, params common.DiskParameters) (string, error) {
612630
readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities())
613-
accessMode := ""
614-
multiWriter := false
615631
if common.IsHyperdisk(params.DiskType) {
616632
if am, err := getHyperdiskAccessModeFromCapabilities(req.GetVolumeCapabilities()); err != nil {
617-
return nil, err
633+
return "", err
618634
} else if disksWithUnsettableAccessMode[params.DiskType] {
619635
// Disallow multi-attach for HdT and HdE. These checks were done in `createVolumeInternal`,
620636
// but repeating them here future-proves us from possible refactors.
621637
if am != common.GCEReadWriteOnceAccessMode {
622-
return nil, status.Errorf(codes.Internal, "")
638+
return "", status.Errorf(codes.Internal, "")
623639
}
624640
} else {
625-
accessMode = am
641+
return am, nil
626642
}
627-
} else {
628-
multiWriter, _ = getMultiWriterFromCapabilities(req.GetVolumeCapabilities())
629643
}
630644

631645
if readonly && slices.Contains(disksWithModifiableAccessMode, params.DiskType) {
632-
accessMode = common.GCEReadOnlyManyAccessMode
646+
return common.GCEReadOnlyManyAccessMode, nil
647+
}
648+
649+
return "", nil
650+
}
651+
652+
func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters, volKey *meta.Key, zones []string, accessMode string) (*gce.CloudDisk, error) {
653+
capacityRange := req.GetCapacityRange()
654+
capBytes, _ := getRequestCapacity(capacityRange)
655+
656+
multiWriter := false
657+
if !common.IsHyperdisk(params.DiskType) {
658+
multiWriter, _ = getMultiWriterFromCapabilities(req.GetVolumeCapabilities())
633659
}
634660

635661
// Validate if disk already exists

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

+106-17
Original file line numberDiff line numberDiff line change
@@ -1298,6 +1298,54 @@ func TestCreateVolumeArguments(t *testing.T) {
12981298
},
12991299
expErrCode: codes.InvalidArgument,
13001300
},
1301+
{
1302+
name: "err empty HdB ROX single-zone disk no content source",
1303+
req: &csi.CreateVolumeRequest{
1304+
Name: name,
1305+
CapacityRange: stdCapRange,
1306+
VolumeCapabilities: []*csi.VolumeCapability{
1307+
{
1308+
AccessType: &csi.VolumeCapability_Mount{
1309+
Mount: &csi.VolumeCapability_MountVolume{},
1310+
},
1311+
AccessMode: &csi.VolumeCapability_AccessMode{
1312+
Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY,
1313+
},
1314+
},
1315+
},
1316+
Parameters: map[string]string{
1317+
common.ParameterKeyType: "hyperdisk-balanced",
1318+
},
1319+
},
1320+
expErrCode: codes.InvalidArgument,
1321+
},
1322+
{
1323+
name: "success empty HdML ROX single-zone disk no content source",
1324+
req: &csi.CreateVolumeRequest{
1325+
Name: name,
1326+
CapacityRange: stdCapRange,
1327+
VolumeCapabilities: []*csi.VolumeCapability{
1328+
{
1329+
AccessType: &csi.VolumeCapability_Mount{
1330+
Mount: &csi.VolumeCapability_MountVolume{},
1331+
},
1332+
AccessMode: &csi.VolumeCapability_AccessMode{
1333+
Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY,
1334+
},
1335+
},
1336+
},
1337+
Parameters: map[string]string{
1338+
common.ParameterKeyType: "hyperdisk-ml",
1339+
},
1340+
},
1341+
expVol: &csi.Volume{
1342+
CapacityBytes: common.GbToBytes(20),
1343+
VolumeId: testVolumeID,
1344+
VolumeContext: nil,
1345+
AccessibleTopology: stdTopology,
1346+
},
1347+
expErrCode: codes.OK,
1348+
},
13011349
}
13021350

13031351
// Run test cases
@@ -1318,6 +1366,7 @@ func TestCreateVolumeArguments(t *testing.T) {
13181366
}
13191367
continue
13201368
}
1369+
t.Logf("ErroCode: %v", err)
13211370
if tc.expErrCode != codes.OK {
13221371
t.Fatalf("Expected error: %v, got no error", tc.expErrCode)
13231372
}
@@ -1504,7 +1553,7 @@ func TestMultiZoneVolumeCreation(t *testing.T) {
15041553
expZones: []string{"us-central1-a", "us-central1-b", "us-central1-c"},
15051554
},
15061555
{
1507-
name: "err single ROX multi-zone no topology",
1556+
name: "success empty HdML ROX multi-zone disk no content source",
15081557
req: &csi.CreateVolumeRequest{
15091558
Name: "test-name",
15101559
CapacityRange: stdCapRange,
@@ -1522,18 +1571,26 @@ func TestMultiZoneVolumeCreation(t *testing.T) {
15221571
common.ParameterKeyType: "hyperdisk-ml",
15231572
common.ParameterKeyEnableMultiZoneProvisioning: "true",
15241573
},
1525-
VolumeContentSource: &csi.VolumeContentSource{
1526-
Type: &csi.VolumeContentSource_Snapshot{
1527-
Snapshot: &csi.VolumeContentSource_SnapshotSource{
1528-
SnapshotId: testSnapshotID,
1574+
AccessibilityRequirements: &csi.TopologyRequirement{
1575+
Requisite: []*csi.Topology{
1576+
{
1577+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
1578+
},
1579+
},
1580+
Preferred: []*csi.Topology{
1581+
{
1582+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
1583+
},
1584+
{
1585+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
15291586
},
15301587
},
15311588
},
15321589
},
1533-
expErrCode: codes.InvalidArgument,
1590+
expZones: []string{"us-central1-a", "us-central1-b"},
15341591
},
15351592
{
1536-
name: "err rwo access mode",
1593+
name: "error empty HdB ROX multi-zone disk no content source",
15371594
req: &csi.CreateVolumeRequest{
15381595
Name: "test-name",
15391596
CapacityRange: stdCapRange,
@@ -1543,21 +1600,14 @@ func TestMultiZoneVolumeCreation(t *testing.T) {
15431600
Mount: &csi.VolumeCapability_MountVolume{},
15441601
},
15451602
AccessMode: &csi.VolumeCapability_AccessMode{
1546-
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
1603+
Mode: csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY,
15471604
},
15481605
},
15491606
},
15501607
Parameters: map[string]string{
1551-
common.ParameterKeyType: "hyperdisk-ml",
1608+
common.ParameterKeyType: "hyperdisk-balanced",
15521609
common.ParameterKeyEnableMultiZoneProvisioning: "true",
15531610
},
1554-
VolumeContentSource: &csi.VolumeContentSource{
1555-
Type: &csi.VolumeContentSource_Snapshot{
1556-
Snapshot: &csi.VolumeContentSource_SnapshotSource{
1557-
SnapshotId: testSnapshotID,
1558-
},
1559-
},
1560-
},
15611611
AccessibilityRequirements: &csi.TopologyRequirement{
15621612
Requisite: []*csi.Topology{
15631613
{
@@ -1568,13 +1618,16 @@ func TestMultiZoneVolumeCreation(t *testing.T) {
15681618
{
15691619
Segments: map[string]string{common.TopologyKeyZone: "us-central1-a"},
15701620
},
1621+
{
1622+
Segments: map[string]string{common.TopologyKeyZone: "us-central1-b"},
1623+
},
15711624
},
15721625
},
15731626
},
15741627
expErrCode: codes.InvalidArgument,
15751628
},
15761629
{
1577-
name: "err no content source",
1630+
name: "err single ROX multi-zone no topology",
15781631
req: &csi.CreateVolumeRequest{
15791632
Name: "test-name",
15801633
CapacityRange: stdCapRange,
@@ -1592,6 +1645,42 @@ func TestMultiZoneVolumeCreation(t *testing.T) {
15921645
common.ParameterKeyType: "hyperdisk-ml",
15931646
common.ParameterKeyEnableMultiZoneProvisioning: "true",
15941647
},
1648+
VolumeContentSource: &csi.VolumeContentSource{
1649+
Type: &csi.VolumeContentSource_Snapshot{
1650+
Snapshot: &csi.VolumeContentSource_SnapshotSource{
1651+
SnapshotId: testSnapshotID,
1652+
},
1653+
},
1654+
},
1655+
},
1656+
expErrCode: codes.InvalidArgument,
1657+
},
1658+
{
1659+
name: "err rwo access mode",
1660+
req: &csi.CreateVolumeRequest{
1661+
Name: "test-name",
1662+
CapacityRange: stdCapRange,
1663+
VolumeCapabilities: []*csi.VolumeCapability{
1664+
{
1665+
AccessType: &csi.VolumeCapability_Mount{
1666+
Mount: &csi.VolumeCapability_MountVolume{},
1667+
},
1668+
AccessMode: &csi.VolumeCapability_AccessMode{
1669+
Mode: csi.VolumeCapability_AccessMode_SINGLE_NODE_WRITER,
1670+
},
1671+
},
1672+
},
1673+
Parameters: map[string]string{
1674+
common.ParameterKeyType: "hyperdisk-ml",
1675+
common.ParameterKeyEnableMultiZoneProvisioning: "true",
1676+
},
1677+
VolumeContentSource: &csi.VolumeContentSource{
1678+
Type: &csi.VolumeContentSource_Snapshot{
1679+
Snapshot: &csi.VolumeContentSource_SnapshotSource{
1680+
SnapshotId: testSnapshotID,
1681+
},
1682+
},
1683+
},
15951684
AccessibilityRequirements: &csi.TopologyRequirement{
15961685
Requisite: []*csi.Topology{
15971686
{

0 commit comments

Comments
 (0)