From 6ecdd9b6f6dea6494f079abe7e92f985067dc465 Mon Sep 17 00:00:00 2001 From: Hanchi Zhang Date: Wed, 29 Jan 2025 01:51:27 +0000 Subject: [PATCH] Support provisioing of HdHA disks and add E2E tests. --- cmd/gce-pd-csi-driver/main.go | 3 +- pkg/common/parameters.go | 9 + pkg/common/parameters_test.go | 19 ++ pkg/common/utils_test.go | 2 +- pkg/gce-cloud-provider/compute/cloud-disk.go | 11 + pkg/gce-cloud-provider/compute/fake-gce.go | 4 + pkg/gce-cloud-provider/compute/gce-compute.go | 4 +- pkg/gce-cloud-provider/compute/gce.go | 2 + pkg/gce-pd-csi-driver/controller.go | 116 +++++----- pkg/gce-pd-csi-driver/controller_test.go | 205 +++++++++++++++--- pkg/gce-pd-csi-driver/gce-pd-driver.go | 3 +- pkg/gce-pd-csi-driver/gce-pd-driver_test.go | 2 +- pkg/gce-pd-csi-driver/utils.go | 4 +- pkg/gce-pd-csi-driver/utils_test.go | 64 +++++- test/e2e/tests/multi_zone_e2e_test.go | 194 +++++++++++++++++ test/e2e/tests/setup_e2e_test.go | 38 +++- test/e2e/tests/single_zone_e2e_test.go | 1 + test/e2e/utils/utils.go | 1 + test/run-e2e.sh | 2 +- test/sanity/sanity_test.go | 2 +- 20 files changed, 571 insertions(+), 115 deletions(-) diff --git a/cmd/gce-pd-csi-driver/main.go b/cmd/gce-pd-csi-driver/main.go index 5c6e5a83a..3d3a2e5a1 100644 --- a/cmd/gce-pd-csi-driver/main.go +++ b/cmd/gce-pd-csi-driver/main.go @@ -72,6 +72,7 @@ var ( formatAndMountTimeout = flag.Duration("format-and-mount-timeout", 1*time.Minute, "The maximum duration of a format and mount operation before another such operation will be started. Used only if --serialize-format-and-mount") fallbackRequisiteZonesFlag = flag.String("fallback-requisite-zones", "", "Comma separated list of requisite zones that will be used if there are not sufficient zones present in requisite topologies when provisioning a disk") enableStoragePoolsFlag = flag.Bool("enable-storage-pools", false, "If set to true, the CSI Driver will allow volumes to be provisioned in Storage Pools") + enableHdHAFlag = flag.Bool("allow-hdha-provisioning", false, "If set to true, will allow the driver to provision Hyperdisk-balanced High Availability disks") multiZoneVolumeHandleDiskTypesFlag = flag.String("multi-zone-volume-handle-disk-types", "", "Comma separated list of allowed disk types that can use the multi-zone volumeHandle. Used only if --multi-zone-volume-handle-enable") multiZoneVolumeHandleEnableFlag = flag.Bool("multi-zone-volume-handle-enable", false, "If set to true, the multi-zone volumeHandle feature will be enabled") @@ -222,7 +223,7 @@ func handle() { } initialBackoffDuration := time.Duration(*errorBackoffInitialDurationMs) * time.Millisecond maxBackoffDuration := time.Duration(*errorBackoffMaxDurationMs) * time.Millisecond - controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones, *enableStoragePoolsFlag, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig) + controllerServer = driver.NewControllerServer(gceDriver, cloudProvider, initialBackoffDuration, maxBackoffDuration, fallbackRequisiteZones, *enableStoragePoolsFlag, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig, *enableHdHAFlag) } else if *cloudConfigFilePath != "" { klog.Warningf("controller service is disabled but cloud config given - it has no effect") } diff --git a/pkg/common/parameters.go b/pkg/common/parameters.go index f76c0aaa3..f41c32336 100644 --- a/pkg/common/parameters.go +++ b/pkg/common/parameters.go @@ -34,6 +34,7 @@ const ( ParameterKeyStoragePools = "storage-pools" ParameterKeyResourceTags = "resource-tags" ParameterKeyEnableMultiZoneProvisioning = "enable-multi-zone-provisioning" + ParameterHdHADiskType = "hyperdisk-balanced-high-availability" // Parameters for VolumeSnapshotClass ParameterKeyStorageLocations = "storage-locations" @@ -108,6 +109,10 @@ type DiskParameters struct { MultiZoneProvisioning bool } +func (dp *DiskParameters) IsRegional() bool { + return dp.ReplicationType == "regional-pd" || dp.DiskType == ParameterHdHADiskType +} + // SnapshotParameters contains normalized and defaulted parameters for snapshots type SnapshotParameters struct { StorageLocations []string @@ -129,6 +134,7 @@ type ParameterProcessor struct { DriverName string EnableStoragePools bool EnableMultiZone bool + EnableHdHA bool } type ModifyVolumeParameters struct { @@ -167,6 +173,9 @@ func (pp *ParameterProcessor) ExtractAndDefaultParameters(parameters map[string] case ParameterKeyType: if v != "" { p.DiskType = strings.ToLower(v) + if !pp.EnableHdHA && p.DiskType == ParameterHdHADiskType { + return p, fmt.Errorf("parameters contain invalid disk type %s", ParameterHdHADiskType) + } } case ParameterKeyReplicationType: if v != "" { diff --git a/pkg/common/parameters_test.go b/pkg/common/parameters_test.go index 83a823535..e83e59cfb 100644 --- a/pkg/common/parameters_test.go +++ b/pkg/common/parameters_test.go @@ -30,6 +30,7 @@ func TestExtractAndDefaultParameters(t *testing.T) { labels map[string]string enableStoragePools bool enableMultiZone bool + enableHdHA bool extraTags map[string]string expectParams DiskParameters expectErr bool @@ -395,6 +396,23 @@ func TestExtractAndDefaultParameters(t *testing.T) { parameters: map[string]string{ParameterKeyType: "hyperdisk-ml", ParameterKeyEnableMultiZoneProvisioning: "true"}, expectErr: true, }, + { + name: "disk parameters, hdha disabled", + parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced-high-availability"}, + expectErr: true, + }, + { + name: "disk parameters, hdha enabled", + parameters: map[string]string{ParameterKeyType: "hyperdisk-balanced-high-availability"}, + enableHdHA: true, + expectParams: DiskParameters{ + DiskType: "hyperdisk-balanced-high-availability", + ReplicationType: "none", + Tags: map[string]string{}, + ResourceTags: map[string]string{}, + Labels: map[string]string{}, + }, + }, } for _, tc := range tests { @@ -403,6 +421,7 @@ func TestExtractAndDefaultParameters(t *testing.T) { DriverName: "testDriver", EnableStoragePools: tc.enableStoragePools, EnableMultiZone: tc.enableMultiZone, + EnableHdHA: tc.enableHdHA, } p, err := pp.ExtractAndDefaultParameters(tc.parameters, tc.labels, tc.extraTags) if gotErr := err != nil; gotErr != tc.expectErr { diff --git a/pkg/common/utils_test.go b/pkg/common/utils_test.go index 0df0ed1d1..c1307e734 100644 --- a/pkg/common/utils_test.go +++ b/pkg/common/utils_test.go @@ -1409,7 +1409,7 @@ func TestIsUserMultiAttachError(t *testing.T) { }, } for _, test := range cases { - code, err := isUserMultiAttachError(fmt.Errorf(test.errorString)) + code, err := isUserMultiAttachError(fmt.Errorf("%v", test.errorString)) if test.expectCode { if err != nil || code != test.expectedCode { t.Errorf("Failed with non-nil error %v or bad code %v: %s", err, code, test.errorString) diff --git a/pkg/gce-cloud-provider/compute/cloud-disk.go b/pkg/gce-cloud-provider/compute/cloud-disk.go index 6790992c4..f4cd3bec6 100644 --- a/pkg/gce-cloud-provider/compute/cloud-disk.go +++ b/pkg/gce-cloud-provider/compute/cloud-disk.go @@ -166,6 +166,17 @@ func (d *CloudDisk) GetZone() string { } } +func (d *CloudDisk) GetRegion() string { + switch { + case d.disk != nil: + return d.disk.Region + case d.betaDisk != nil: + return d.betaDisk.Region + default: + return "" + } +} + func (d *CloudDisk) GetSnapshotId() string { switch { case d.disk != nil: diff --git a/pkg/gce-cloud-provider/compute/fake-gce.go b/pkg/gce-cloud-provider/compute/fake-gce.go index f2289a398..2f00ea6e2 100644 --- a/pkg/gce-cloud-provider/compute/fake-gce.go +++ b/pkg/gce-cloud-provider/compute/fake-gce.go @@ -79,6 +79,10 @@ func CreateFakeCloudProvider(project, zone string, cloudDisks []*CloudDisk) (*Fa mockDiskStatus: "READY", } for _, d := range cloudDisks { + if d.LocationType() == meta.Regional { + fcp.disks[meta.RegionalKey(d.GetName(), d.GetRegion()).String()] = d + continue + } diskZone := d.GetZone() if diskZone == "" { diskZone = zone diff --git a/pkg/gce-cloud-provider/compute/gce-compute.go b/pkg/gce-cloud-provider/compute/gce-compute.go index c2c2b7603..e2a753b22 100644 --- a/pkg/gce-cloud-provider/compute/gce-compute.go +++ b/pkg/gce-cloud-provider/compute/gce-compute.go @@ -423,8 +423,8 @@ func ValidateDiskParameters(disk *CloudDisk, params common.DiskParameters) error } locationType := disk.LocationType() - if (params.ReplicationType == "none" && locationType != meta.Zonal) || (params.ReplicationType == "regional-pd" && locationType != meta.Regional) { - return fmt.Errorf("actual disk replication type %v did not match expected param %s", locationType, params.ReplicationType) + if (params.ReplicationType == "none" && locationType != meta.Zonal) || (params.IsRegional() && locationType != meta.Regional) { + return fmt.Errorf("actual replication type %v did not match expected param %s and %s", locationType, params.ReplicationType, params.DiskType) } if !KmsKeyEqual( diff --git a/pkg/gce-cloud-provider/compute/gce.go b/pkg/gce-cloud-provider/compute/gce.go index f25519cf4..ff0fbd63c 100644 --- a/pkg/gce-cloud-provider/compute/gce.go +++ b/pkg/gce-cloud-provider/compute/gce.go @@ -101,6 +101,8 @@ type CloudProvider struct { tagsRateLimiter *rate.Limiter listInstancesConfig ListInstancesConfig + + enableHdHA bool } var _ GCECompute = &CloudProvider{} diff --git a/pkg/gce-pd-csi-driver/controller.go b/pkg/gce-pd-csi-driver/controller.go index decac55a0..117df6413 100644 --- a/pkg/gce-pd-csi-driver/controller.go +++ b/pkg/gce-pd-csi-driver/controller.go @@ -104,6 +104,10 @@ type GCEControllerServer struct { // If set to true, the CSI Driver will allow volumes to be provisioned in Storage Pools. enableStoragePools bool + // If set to true, the CSI Driver will allow Hyperdisk-balanced High Availability disks + // to be provisioned. + enableHdHA bool + multiZoneVolumeHandleConfig MultiZoneVolumeHandleConfig listVolumesConfig ListVolumesConfig @@ -167,10 +171,10 @@ type workItem struct { // locationRequirements are additional location topology requirements that must be respected when creating a volume. type locationRequirements struct { - srcVolRegion string - srcVolZone string - srcReplicationType string - cloneReplicationType string + srcVolRegion string + srcVolZone string + srcIsRegional bool + cloneIsRegional bool } // PDCSIContext is the extracted VolumeContext from controller requests. @@ -206,7 +210,8 @@ const ( listDisksUsersField = googleapi.Field("items/users") - readOnlyManyAccessMode = "READ_ONLY_MANY" + gceReadOnlyManyAccessMode = "READ_ONLY_MANY" + gceReadWriteManyAccessMode = "READ_WRITE_MANY" ) var ( @@ -256,7 +261,7 @@ func isDiskReady(disk *gce.CloudDisk) (bool, error) { // cloningLocationRequirements returns additional location requirements to be applied to the given create volume requests topology. // If the CreateVolumeRequest will use volume cloning, location requirements in compliance with the volume cloning limitations // will be returned: https://cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/volume-cloning#limitations. -func cloningLocationRequirements(req *csi.CreateVolumeRequest, cloneReplicationType string) (*locationRequirements, error) { +func cloningLocationRequirements(req *csi.CreateVolumeRequest, cloneIsRegional bool) (*locationRequirements, error) { if !useVolumeCloning(req) { return nil, nil } @@ -278,12 +283,7 @@ func cloningLocationRequirements(req *csi.CreateVolumeRequest, cloneReplicationT sourceVolKey.Region = region } - srcReplicationType := replicationTypeNone - if !isZonalSrcVol { - srcReplicationType = replicationTypeRegionalPD - } - - return &locationRequirements{srcVolZone: sourceVolKey.Zone, srcVolRegion: sourceVolKey.Region, srcReplicationType: srcReplicationType, cloneReplicationType: cloneReplicationType}, nil + return &locationRequirements{srcVolZone: sourceVolKey.Zone, srcVolRegion: sourceVolKey.Region, srcIsRegional: !isZonalSrcVol, cloneIsRegional: cloneIsRegional}, nil } // useVolumeCloning returns true if the create volume request should be created with volume cloning. @@ -374,6 +374,10 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req return nil, status.Error(codes.InvalidArgument, "VolumeContentSource must be provided when AccessMode is set to read only") } + if readonly && params.DiskType == common.ParameterHdHADiskType { + return nil, status.Errorf(codes.InvalidArgument, "Invalid access mode for disk type %s", common.ParameterHdHADiskType) + } + // Validate multi-zone provisioning configuration err = gceCS.validateMultiZoneProvisioning(req, params) if err != nil { @@ -381,7 +385,7 @@ func (gceCS *GCEControllerServer) createVolumeInternal(ctx context.Context, req } // Verify that the regional availability class is only used on regional disks. - if params.ForceAttach && params.ReplicationType != replicationTypeRegionalPD { + if params.ForceAttach && !params.IsRegional() { return nil, status.Errorf(codes.InvalidArgument, "invalid availabilty class for zonal disk") } @@ -528,19 +532,19 @@ func (gceCS *GCEControllerServer) updateAccessModeIfNecessary(ctx context.Contex return nil } project := gceCS.CloudProvider.GetDefaultProject() - if disk.GetAccessMode() == readOnlyManyAccessMode { + if disk.GetAccessMode() == gceReadOnlyManyAccessMode { // If the access mode is already readonly, return return nil } - return gceCS.CloudProvider.SetDiskAccessMode(ctx, project, volKey, readOnlyManyAccessMode) + return gceCS.CloudProvider.SetDiskAccessMode(ctx, project, volKey, gceReadOnlyManyAccessMode) } func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, req *csi.CreateVolumeRequest, params common.DiskParameters) (*csi.CreateVolumeResponse, error) { var err error var locationTopReq *locationRequirements if useVolumeCloning(req) { - locationTopReq, err = cloningLocationRequirements(req, params.ReplicationType) + locationTopReq, err = cloningLocationRequirements(req, params.IsRegional()) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "failed to get location requirements: %v", err.Error()) } @@ -549,18 +553,7 @@ func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, re // Determine the zone or zones+region of the disk var zones []string var volKey *meta.Key - switch params.ReplicationType { - case replicationTypeNone: - zones, err = gceCS.pickZones(ctx, req.GetAccessibilityRequirements(), 1, locationTopReq) - if err != nil { - return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error()) - } - if len(zones) != 1 { - return nil, status.Errorf(codes.Internal, "Failed to pick exactly 1 zone for zonal disk, got %v instead", len(zones)) - } - volKey = meta.ZonalKey(req.GetName(), zones[0]) - - case replicationTypeRegionalPD: + if params.IsRegional() { zones, err = gceCS.pickZones(ctx, req.GetAccessibilityRequirements(), 2, locationTopReq) if err != nil { return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error()) @@ -570,7 +563,16 @@ func (gceCS *GCEControllerServer) createSingleDeviceDisk(ctx context.Context, re return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to get region from zones: %v", err.Error()) } volKey = meta.RegionalKey(req.GetName(), region) - default: + } else if params.ReplicationType == replicationTypeNone { + zones, err = gceCS.pickZones(ctx, req.GetAccessibilityRequirements(), 1, locationTopReq) + if err != nil { + return nil, status.Errorf(codes.InvalidArgument, "CreateVolume failed to pick zones for disk: %v", err.Error()) + } + if len(zones) != 1 { + return nil, status.Errorf(codes.Internal, "Failed to pick exactly 1 zone for zonal disk, got %v instead", len(zones)) + } + volKey = meta.ZonalKey(req.GetName(), zones[0]) + } else { return nil, status.Errorf(codes.InvalidArgument, "CreateVolume replication type '%s' is not supported", params.ReplicationType) } @@ -598,7 +600,7 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi readonly, _ := getReadOnlyFromCapabilities(req.GetVolumeCapabilities()) accessMode := "" if readonly && slices.Contains(disksWithModifiableAccessMode, params.DiskType) { - accessMode = readOnlyManyAccessMode + accessMode = gceReadOnlyManyAccessMode } // Validate if disk already exists @@ -711,16 +713,7 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi var disk *gce.CloudDisk name := req.GetName() - switch params.ReplicationType { - case replicationTypeNone: - if len(zones) != 1 { - return nil, status.Errorf(codes.Internal, "CreateVolume failed to get a single zone for creating zonal disk, instead got: %v", zones) - } - disk, err = createSingleZoneDisk(ctx, gceCS.CloudProvider, name, zones, params, capacityRange, capBytes, snapshotID, volumeContentSourceVolumeID, multiWriter, accessMode) - if err != nil { - return nil, common.LoggedError("CreateVolume failed to create single zonal disk "+name+": ", err) - } - case replicationTypeRegionalPD: + if params.IsRegional() { if len(zones) != 2 { return nil, status.Errorf(codes.Internal, "CreateVolume failed to get a 2 zones for creating regional disk, instead got: %v", zones) } @@ -728,7 +721,15 @@ func (gceCS *GCEControllerServer) createSingleDisk(ctx context.Context, req *csi if err != nil { return nil, common.LoggedError("CreateVolume failed to create regional disk "+name+": ", err) } - default: + } else if params.ReplicationType == replicationTypeNone { + if len(zones) != 1 { + return nil, status.Errorf(codes.Internal, "CreateVolume failed to get a single zone for creating zonal disk, instead got: %v", zones) + } + disk, err = createSingleZoneDisk(ctx, gceCS.CloudProvider, name, zones, params, capacityRange, capBytes, snapshotID, volumeContentSourceVolumeID, multiWriter, accessMode) + if err != nil { + return nil, common.LoggedError("CreateVolume failed to create single zonal disk "+name+": ", err) + } + } else { return nil, status.Errorf(codes.InvalidArgument, "CreateVolume replication type '%s' is not supported", params.ReplicationType) } @@ -1268,6 +1269,7 @@ func (gceCS *GCEControllerServer) parameterProcessor() *common.ParameterProcesso DriverName: gceCS.Driver.name, EnableStoragePools: gceCS.enableStoragePools, EnableMultiZone: gceCS.multiZoneVolumeHandleConfig.Enable, + EnableHdHA: gceCS.enableHdHA, } } @@ -1547,6 +1549,10 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C } return nil, common.LoggedError("CreateSnapshot, failed to getDisk: ", err) } + isHyperdisk := strings.HasPrefix(disk.GetPDType(), "hyperdisk-") + if isHyperdisk && disk.GetAccessMode() == gceReadWriteManyAccessMode { + return nil, status.Errorf(codes.InvalidArgument, "Cannot create snapshot for disk type %s with access mode %s", common.ParameterHdHADiskType, gceReadWriteManyAccessMode) + } snapshotParams, err := common.ExtractAndDefaultSnapshotParameters(req.GetParameters(), gceCS.Driver.name, gceCS.Driver.extraTags) if err != nil { @@ -1561,6 +1567,9 @@ func (gceCS *GCEControllerServer) CreateSnapshot(ctx context.Context, req *csi.C return nil, err } case common.DiskImageType: + if disk.LocationType() == meta.Regional { + return nil, status.Errorf(codes.InvalidArgument, "Cannot create backup type %s for regional disk %s", common.DiskImageType, disk.GetName()) + } snapshot, err = gceCS.createImage(ctx, project, volKey, req.Name, snapshotParams) if err != nil { return nil, err @@ -2144,17 +2153,8 @@ func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int, locationT if locationTopReq != nil { srcVolZone := locationTopReq.srcVolZone - switch locationTopReq.cloneReplicationType { - // For zonal -> zonal cloning, the source disk zone must match the destination disk zone. - case replicationTypeNone: - // If the source volume zone is not in the topology requirement, we return an error. - if !slices.Contains(prefZones, srcVolZone) && !slices.Contains(reqZones, srcVolZone) { - volumeCloningReq := fmt.Sprintf("clone zone must match source disk zone: %s", srcVolZone) - return nil, fmt.Errorf("failed to find zone from topology %v: %s", top, volumeCloningReq) - } - return []string{srcVolZone}, nil - // For zonal or regional -> regional disk cloning, the source disk region must match the destination disk region. - case replicationTypeRegionalPD: + if locationTopReq.cloneIsRegional { + // For zonal or regional -> regional disk cloning, the source disk region must match the destination disk region. srcVolRegion := locationTopReq.srcVolRegion prefZones = pickZonesInRegion(srcVolRegion, prefZones) reqZones = pickZonesInRegion(srcVolRegion, reqZones) @@ -2165,13 +2165,21 @@ func pickZonesFromTopology(top *csi.TopologyRequirement, numZones int, locationT } // For zonal -> regional disk cloning, one of the replicated zones must match the source zone. - if locationTopReq.srcReplicationType == replicationTypeNone { + if !locationTopReq.srcIsRegional { if !slices.Contains(prefZones, srcVolZone) && !slices.Contains(reqZones, srcVolZone) { volumeCloningReq := fmt.Sprintf("one of the replica zones of the clone must match the source disk zone: %s", srcVolZone) return nil, fmt.Errorf("failed to find zone from topology %v: %s", top, volumeCloningReq) } prefZones = prependZone(srcVolZone, prefZones) } + } else { + // For zonal -> zonal cloning, the source disk zone must match the destination disk zone. + if !slices.Contains(prefZones, srcVolZone) && !slices.Contains(reqZones, srcVolZone) { + // If the source volume zone is not in the topology requirement, we return an error. + volumeCloningReq := fmt.Sprintf("clone zone must match source disk zone: %s", srcVolZone) + return nil, fmt.Errorf("failed to find zone from topology %v: %s", top, volumeCloningReq) + } + return []string{srcVolZone}, nil } } @@ -2259,7 +2267,7 @@ func (gceCS *GCEControllerServer) pickZones(ctx context.Context, top *csi.Topolo // in the same zone as the source volume, and for zonal -> regional, one of the replicated zones will always // be the zone of the source volume. For regional -> regional cloning, the srcVolZone will not be set, so we // just use the default zone. - if locationTopReq != nil && locationTopReq.srcReplicationType == replicationTypeNone { + if locationTopReq != nil && !locationTopReq.srcIsRegional { existingZones = []string{locationTopReq.srcVolZone} } // If topology is nil, then the Immediate binding mode was used without setting allowedTopologies in the storageclass. diff --git a/pkg/gce-pd-csi-driver/controller_test.go b/pkg/gce-pd-csi-driver/controller_test.go index 06db7222e..165fc7f9b 100644 --- a/pkg/gce-pd-csi-driver/controller_test.go +++ b/pkg/gce-pd-csi-driver/controller_test.go @@ -224,6 +224,64 @@ func TestCreateSnapshotArguments(t *testing.T) { }, expErrCode: codes.InvalidArgument, }, + { + name: "fail to create image for HdHA", + req: &csi.CreateSnapshotRequest{ + Name: name, + SourceVolumeId: testRegionalID, + Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2", common.ParameterKeySnapshotType: "images"}, + }, + seedDisks: []*gce.CloudDisk{ + gce.CloudDiskFromV1(&compute.Disk{ + Name: name, + SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name), + Type: common.ParameterHdHADiskType, + Region: "country-region", + }), + }, + expErrCode: codes.InvalidArgument, + }, + { + name: "success with creating snapshot for HdHA", + req: &csi.CreateSnapshotRequest{ + Name: name, + SourceVolumeId: testRegionalID, + Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2"}, + }, + seedDisks: []*gce.CloudDisk{ + gce.CloudDiskFromV1(&compute.Disk{ + Name: name, + SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name), + Type: common.ParameterHdHADiskType, + Region: "country-region", + }), + }, + expSnapshot: &csi.Snapshot{ + SnapshotId: testSnapshotID, + SourceVolumeId: testRegionalID, + CreationTime: tp, + SizeBytes: common.GbToBytes(gce.DiskSizeGb), + ReadyToUse: false, + }, + }, + { + name: "fail to create snapshot for HdHA multi-writer", + req: &csi.CreateSnapshotRequest{ + Name: name, + SourceVolumeId: testRegionalID, + Parameters: map[string]string{common.ParameterKeyStorageLocations: " US-WEST2"}, + }, + seedDisks: []*gce.CloudDisk{ + gce.CloudDiskFromV1(&compute.Disk{ + Name: name, + SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project/regions/country-region/name/%s", name), + Type: common.ParameterHdHADiskType, + AccessMode: gceReadWriteManyAccessMode, + Region: "country-region", + }), + }, + expErrCode: codes.InvalidArgument, + }, } for _, tc := range testCases { @@ -255,8 +313,7 @@ func TestCreateSnapshotArguments(t *testing.T) { } if !reflect.DeepEqual(snapshot, tc.expSnapshot) { - errStr := fmt.Sprintf("Expected snapshot: %#v\n to equal snapshot: %#v\n", snapshot, tc.expSnapshot) - t.Errorf(errStr) + t.Errorf("Expected snapshot: %#v\n to equal snapshot: %#v\n", snapshot, tc.expSnapshot) } } } @@ -546,8 +603,7 @@ func TestListSnapshotsArguments(t *testing.T) { t.Fatalf("Expected snapshots number %v, got no snapshot", tc.numSnapshots) } if len(snapshots) != tc.expectedCount { - errStr := fmt.Sprintf("Expected snapshot number to equal: %v", tc.numSnapshots) - t.Errorf(errStr) + t.Errorf("Expected snapshot number to equal: %v", tc.numSnapshots) } } } @@ -857,6 +913,87 @@ func TestCreateVolumeArguments(t *testing.T) { }, }, }, + // HdHA tests + { + name: "success with topology with HdHA", + req: &csi.CreateVolumeRequest{ + Name: name, + CapacityRange: stdCapRange, + VolumeCapabilities: stdVolCaps, + Parameters: map[string]string{common.ParameterKeyType: common.ParameterHdHADiskType}, + AccessibilityRequirements: &csi.TopologyRequirement{ + Preferred: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: region + "-c"}, + }, + { + Segments: map[string]string{common.TopologyKeyZone: region + "-b"}, + }, + }, + }, + }, + expVol: &csi.Volume{ + CapacityBytes: common.GbToBytes(20), + VolumeId: testRegionalID, + VolumeContext: nil, + AccessibleTopology: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: region + "-c"}, + }, + { + Segments: map[string]string{common.TopologyKeyZone: region + "-b"}, + }, + }, + }, + }, + { + name: "fail not enough topology with HdHA", + req: &csi.CreateVolumeRequest{ + Name: name, + CapacityRange: stdCapRange, + VolumeCapabilities: stdVolCaps, + Parameters: map[string]string{ + common.ParameterKeyType: common.ParameterHdHADiskType, + }, + AccessibilityRequirements: &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: region + "-c"}, + }, + }, + Preferred: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: region + "-c"}, + }, + }, + }, + }, + expErrCode: codes.InvalidArgument, + }, + { + name: "success with no toplogy specified with HdHA", + req: &csi.CreateVolumeRequest{ + Name: name, + CapacityRange: stdCapRange, + VolumeCapabilities: stdVolCaps, + Parameters: map[string]string{ + common.ParameterKeyType: common.ParameterHdHADiskType, + }, + }, + expVol: &csi.Volume{ + CapacityBytes: common.GbToBytes(20), + VolumeId: testRegionalID, + VolumeContext: nil, + AccessibleTopology: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: zone}, + }, + { + Segments: map[string]string{common.TopologyKeyZone: secondZone}, + }, + }, + }, + }, { name: "success with block volume capability", req: &csi.CreateVolumeRequest{ @@ -1130,9 +1267,9 @@ func TestCreateVolumeArguments(t *testing.T) { t.Errorf("Accessible topologies are not the same length, got %v, expected %v", len(vol.GetAccessibleTopology()), len(tc.expVol.GetAccessibleTopology())) } for i := 0; i < len(vol.GetAccessibleTopology()); i++ { - errStr = errStr + fmt.Sprintf("Got topology %#v\nExpected toplogy %#v\n\n", vol.GetAccessibleTopology()[i], tc.expVol.GetAccessibleTopology()[i]) + errStr += fmt.Sprintf("Got topology %#v\nExpected toplogy %#v\n\n", vol.GetAccessibleTopology()[i], tc.expVol.GetAccessibleTopology()[i]) } - t.Errorf(errStr) + t.Error(errStr) } } } @@ -2679,7 +2816,7 @@ func TestCloningLocationRequirements(t *testing.T) { nilVolumeContentSource bool reqParameters map[string]string requestCapacityRange *csi.CapacityRange - replicationType string + cloneIsRegional bool expectedLocationRequirements *locationRequirements expectedErr bool }{ @@ -2690,8 +2827,8 @@ func TestCloningLocationRequirements(t *testing.T) { reqParameters: map[string]string{ common.ParameterKeyReplicationType: replicationTypeNone, }, - replicationType: replicationTypeNone, - expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeNone}, + cloneIsRegional: false, + expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcIsRegional: false, cloneIsRegional: false}, expectedErr: false, }, { @@ -2701,8 +2838,8 @@ func TestCloningLocationRequirements(t *testing.T) { reqParameters: map[string]string{ common.ParameterKeyReplicationType: replicationTypeRegionalPD, }, - replicationType: replicationTypeRegionalPD, - expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: "", srcReplicationType: replicationTypeRegionalPD, cloneReplicationType: replicationTypeRegionalPD}, + cloneIsRegional: true, + expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: "", srcIsRegional: true, cloneIsRegional: true}, expectedErr: false, }, { @@ -2710,10 +2847,10 @@ func TestCloningLocationRequirements(t *testing.T) { sourceVolumeID: testZonalVolumeSourceID, requestCapacityRange: stdCapRange, reqParameters: map[string]string{ - common.ParameterKeyReplicationType: replicationTypeRegionalPD, + common.ParameterKeyType: common.ParameterHdHADiskType, }, - replicationType: replicationTypeRegionalPD, - expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + cloneIsRegional: true, + expectedLocationRequirements: &locationRequirements{srcVolRegion: region, srcVolZone: zone, srcIsRegional: false, cloneIsRegional: true}, expectedErr: false, }, { @@ -2723,7 +2860,7 @@ func TestCloningLocationRequirements(t *testing.T) { reqParameters: map[string]string{ common.ParameterKeyReplicationType: replicationTypeRegionalPD, }, - replicationType: replicationTypeRegionalPD, + cloneIsRegional: true, expectedLocationRequirements: nil, expectedErr: false, }, @@ -2734,7 +2871,7 @@ func TestCloningLocationRequirements(t *testing.T) { reqParameters: map[string]string{ common.ParameterKeyReplicationType: replicationTypeNone, }, - replicationType: replicationTypeNone, + cloneIsRegional: false, expectedLocationRequirements: nil, expectedErr: true, }, @@ -2759,12 +2896,12 @@ func TestCloningLocationRequirements(t *testing.T) { req.VolumeContentSource = nil } - locationRequirements, err := cloningLocationRequirements(req, tc.replicationType) + locationRequirements, err := cloningLocationRequirements(req, tc.cloneIsRegional) if err != nil != tc.expectedErr { t.Fatalf("Got error %v, expected error %t", err, tc.expectedErr) } - input := fmt.Sprintf("cloningLocationRequirements(%v, %s", req, tc.replicationType) + input := fmt.Sprintf("cloningLocationRequirements(%v, %v)", req, tc.cloneIsRegional) if fmt.Sprintf("%v", tc.expectedLocationRequirements) != fmt.Sprintf("%v", locationRequirements) { t.Fatalf("%s returned unexpected diff got: %v, want %v", input, locationRequirements, tc.expectedLocationRequirements) } @@ -2917,7 +3054,7 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) { sourceCapacityRange: stdCapRange, enableStoragePools: true, reqParameters: map[string]string{ - common.ParameterKeyType: "test-type", + common.ParameterKeyType: "hyperdisk-balanced", common.ParameterKeyReplicationType: replicationTypeNone, common.ParameterKeyDiskEncryptionKmsKey: "encryption-key", common.ParameterKeyStoragePools: "projects/test-project/zones/country-region-zone/storagePools/storagePool-1", @@ -3381,14 +3518,14 @@ func TestCreateVolumeWithVolumeSourceFromVolume(t *testing.T) { if serverError.Code() != tc.expErrCode { t.Fatalf("Expected error code: %v, got: %v. err : %v", tc.expErrCode, serverError.Code(), err) } + if tc.expErrMsg != "" && !strings.Contains(err.Error(), tc.expErrMsg) { + t.Fatalf("Got error: %v, expected error: %v", err.Error(), tc.expErrMsg) + } continue } if tc.expErrCode != codes.OK { t.Fatalf("Expected error: %v, got no error", tc.expErrCode) } - if tc.expErrMsg != "" && tc.expErrMsg != err.Error() { - t.Fatalf("Got error: %v, expected error: %v", err.Error(), tc.expErrMsg) - } // Make sure the response has the source volume. respVolume := resp.GetVolume() @@ -4067,7 +4204,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeNone}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcIsRegional: false, cloneIsRegional: false}, numZones: 1, expZones: []string{"us-central1-a"}, }, @@ -4090,7 +4227,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, Preferred: []*csi.Topology{}, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-c", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-c", srcIsRegional: false, cloneIsRegional: true}, numZones: 2, expZones: []string{"us-central1-c", "us-central1-f"}, }, @@ -4158,7 +4295,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcReplicationType: replicationTypeRegionalPD, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcIsRegional: true, cloneIsRegional: true}, numZones: 5, expZones: []string{"us-central1-b", "us-central1-a", "us-central1-c", "us-central1-d", "us-central1-f"}, }, @@ -4191,7 +4328,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcIsRegional: false, cloneIsRegional: true}, numZones: 5, expZones: []string{"us-central1-a", "us-central1-b", "us-central1-c", "us-central1-d", "us-central1-f"}, }, @@ -4221,7 +4358,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcIsRegional: false, cloneIsRegional: true}, numZones: 3, expZones: []string{"us-central1-a", "us-central1-c", "us-central1-b"}, }, @@ -4251,7 +4388,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-b", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-b", srcIsRegional: false, cloneIsRegional: true}, numZones: 3, expZones: []string{"us-central1-b", "us-central1-c", "us-central1-f"}, }, @@ -4276,7 +4413,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, fallbackRequisiteZones: []string{"us-central1-a", "us-central1-f", "us-central1-g"}, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", srcIsRegional: false, cloneIsRegional: true}, numZones: 2, expZones: []string{"us-central1-a", "us-central1-b"}, }, @@ -4298,7 +4435,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, fallbackRequisiteZones: []string{"us-central1-a", "us-central1-b", "us-central1-c"}, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-b", srcReplicationType: replicationTypeNone, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-b", srcIsRegional: false, cloneIsRegional: true}, numZones: 2, expZones: []string{"us-central1-b", "us-central1-c"}, }, @@ -4316,7 +4453,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, fallbackRequisiteZones: []string{"us-central1-a", "us-central1-b", "us-central1-c", "us-central1-f"}, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-b", srcReplicationType: replicationTypeRegionalPD, cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-b", srcIsRegional: true, cloneIsRegional: true}, numZones: 2, expZones: []string{"us-central1-b", "us-central1-c"}, }, @@ -4411,7 +4548,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-east1", srcVolZone: "us-east1-a", cloneReplicationType: replicationTypeNone}, + locReq: &locationRequirements{srcVolRegion: "us-east1", srcVolZone: "us-east1-a", cloneIsRegional: false}, numZones: 1, expErr: true, }, @@ -4441,7 +4578,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-east1", srcVolZone: "us-east1-a", cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-east1", srcVolZone: "us-east1-a", cloneIsRegional: true}, numZones: 2, expErr: true, }, @@ -4471,7 +4608,7 @@ func TestPickZonesFromTopology(t *testing.T) { }, }, }, - locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", cloneReplicationType: replicationTypeRegionalPD}, + locReq: &locationRequirements{srcVolRegion: "us-central1", srcVolZone: "us-central1-a", cloneIsRegional: true}, numZones: 4, expErr: true, }, diff --git a/pkg/gce-pd-csi-driver/gce-pd-driver.go b/pkg/gce-pd-csi-driver/gce-pd-driver.go index 8bc26ae74..494f03899 100644 --- a/pkg/gce-pd-csi-driver/gce-pd-driver.go +++ b/pkg/gce-pd-csi-driver/gce-pd-driver.go @@ -156,7 +156,7 @@ func NewNodeServer(gceDriver *GCEDriver, mounter *mount.SafeFormatAndMount, devi } } -func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, errorBackoffInitialDuration, errorBackoffMaxDuration time.Duration, fallbackRequisiteZones []string, enableStoragePools bool, multiZoneVolumeHandleConfig MultiZoneVolumeHandleConfig, listVolumesConfig ListVolumesConfig, provisionableDisksConfig ProvisionableDisksConfig) *GCEControllerServer { +func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, errorBackoffInitialDuration, errorBackoffMaxDuration time.Duration, fallbackRequisiteZones []string, enableStoragePools bool, multiZoneVolumeHandleConfig MultiZoneVolumeHandleConfig, listVolumesConfig ListVolumesConfig, provisionableDisksConfig ProvisionableDisksConfig, enableHdHA bool) *GCEControllerServer { return &GCEControllerServer{ Driver: gceDriver, CloudProvider: cloudProvider, @@ -168,6 +168,7 @@ func NewControllerServer(gceDriver *GCEDriver, cloudProvider gce.GCECompute, err multiZoneVolumeHandleConfig: multiZoneVolumeHandleConfig, listVolumesConfig: listVolumesConfig, provisionableDisksConfig: provisionableDisksConfig, + enableHdHA: enableHdHA, } } diff --git a/pkg/gce-pd-csi-driver/gce-pd-driver_test.go b/pkg/gce-pd-csi-driver/gce-pd-driver_test.go index 628419526..fb73c33fb 100644 --- a/pkg/gce-pd-csi-driver/gce-pd-driver_test.go +++ b/pkg/gce-pd-csi-driver/gce-pd-driver_test.go @@ -54,7 +54,7 @@ func controllerServerForTest(cloudProvider gce.GCECompute) *GCEControllerServer SupportsThroughputChange: []string{"hyperdisk-balanced", "hyperdisk-throughput", "hyperdisk-ml"}, } - return NewControllerServer(gceDriver, cloudProvider, errorBackoffInitialDuration, errorBackoffMaxDuration, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig) + return NewControllerServer(gceDriver, cloudProvider, errorBackoffInitialDuration, errorBackoffMaxDuration, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig, true /* enableHdHA */) } func initGCEDriverWithCloudProvider(t *testing.T, cloudProvider gce.GCECompute) *GCEDriver { diff --git a/pkg/gce-pd-csi-driver/utils.go b/pkg/gce-pd-csi-driver/utils.go index 8e14b272a..77ac640cb 100644 --- a/pkg/gce-pd-csi-driver/utils.go +++ b/pkg/gce-pd-csi-driver/utils.go @@ -156,8 +156,8 @@ func validateStoragePools(req *csi.CreateVolumeRequest, params common.DiskParame return fmt.Errorf("invalid disk-type: %q. storage pools only support hyperdisk-balanced or hyperdisk-throughput", params.DiskType) } - if params.ReplicationType == replicationTypeRegionalPD { - return fmt.Errorf("storage pools do not support regional PD") + if params.IsRegional() { + return fmt.Errorf("storage pools do not support regional disks") } if useVolumeCloning(req) { diff --git a/pkg/gce-pd-csi-driver/utils_test.go b/pkg/gce-pd-csi-driver/utils_test.go index fd114fc86..9d65c355b 100644 --- a/pkg/gce-pd-csi-driver/utils_test.go +++ b/pkg/gce-pd-csi-driver/utils_test.go @@ -297,11 +297,12 @@ func TestGetReadOnlyFromCapabilities(t *testing.T) { func TestValidateStoragePools(t *testing.T) { testCases := []struct { - name string - req *csi.CreateVolumeRequest - params common.DiskParameters - project string - expErr error + name string + req *csi.CreateVolumeRequest + params common.DiskParameters + project string + expErr error + enableHdHA bool }{ { name: "success with storage pools not enabled", @@ -396,7 +397,58 @@ func TestValidateStoragePools(t *testing.T) { }, }, project: "test-project", - expErr: fmt.Errorf("storage pools do not support regional PD"), + expErr: fmt.Errorf("storage pools do not support regional disks"), + }, + { + name: "fail storage pools with HdHA, even when HdHA is allowed", + req: &csi.CreateVolumeRequest{ + Name: "test-name", + }, + params: common.DiskParameters{ + DiskType: "hyperdisk-balanced-high-availability", + StoragePools: []common.StoragePool{ + { + Project: "test-project", + Zone: "us-central1-a", + Name: "storagePool-1", + ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", + }, + { + Project: "test-project", + Zone: "us-central1-b", + Name: "storagePool-2", + ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", + }, + }, + }, + project: "test-project", + expErr: fmt.Errorf("invalid disk-type: \"hyperdisk-balanced-high-availability\". storage pools only support hyperdisk-balanced or hyperdisk-throughput"), + enableHdHA: true, + }, + { + name: "fail storage pools with HdHA when HdHA is not allowed", + req: &csi.CreateVolumeRequest{ + Name: "test-name", + }, + params: common.DiskParameters{ + DiskType: "hyperdisk-balanced-high-availability", + StoragePools: []common.StoragePool{ + { + Project: "test-project", + Zone: "us-central1-a", + Name: "storagePool-1", + ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", + }, + { + Project: "test-project", + Zone: "us-central1-b", + Name: "storagePool-2", + ResourceName: "projects/test-project/zones/us-central1-a/storagePools/storagePool-1", + }, + }, + }, + project: "test-project", + expErr: fmt.Errorf("invalid disk-type: \"hyperdisk-balanced-high-availability\". storage pools only support hyperdisk-balanced or hyperdisk-throughput"), }, { name: "fail storage pools with disk clones", diff --git a/test/e2e/tests/multi_zone_e2e_test.go b/test/e2e/tests/multi_zone_e2e_test.go index fb1ea32dd..492cb7cac 100644 --- a/test/e2e/tests/multi_zone_e2e_test.go +++ b/test/e2e/tests/multi_zone_e2e_test.go @@ -1028,6 +1028,200 @@ var _ = Describe("GCE PD CSI Driver Multi-Zone", func() { Expect(err).To(BeNil(), "failed read in zone 1") Expect(strings.TrimSpace(string(readContents))).To(BeIdenticalTo(testFileContents), "content mismatch in zone 1") }) + + It("Should successfully run through entire lifecycle of a HdHA volume on instances in 2 zones", func() { + // Create new driver and client + + Expect(hyperdiskTestContexts).NotTo(BeEmpty()) + + zoneToContext := map[string]*remote.TestContext{} + zones := []string{} + + for _, tc := range hyperdiskTestContexts { + _, z, _ := tc.Instance.GetIdentity() + // Zone hasn't been seen before + if _, ok := zoneToContext[z]; !ok { + zoneToContext[z] = tc + zones = append(zones, z) + } + if len(zoneToContext) == 2 { + break + } + } + + Expect(len(zoneToContext)).To(Equal(2), "Must have instances in 2 zones") + + controllerContext := zoneToContext[zones[0]] + controllerClient := controllerContext.Client + controllerInstance := controllerContext.Instance + + p, _, _ := controllerInstance.GetIdentity() + + region, err := common.GetRegionFromZones(zones) + Expect(err).To(BeNil(), "Failed to get region from zones") + + // Create Disk + volName := testNamePrefix + string(uuid.NewUUID()) + volume, err := controllerClient.CreateVolume(volName, map[string]string{ + common.ParameterKeyType: common.ParameterHdHADiskType, + }, defaultRepdSizeGb, &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: zones[0]}, + }, + { + Segments: map[string]string{common.TopologyKeyZone: zones[1]}, + }, + }, + }, nil) + Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err) + + // Validate Disk Created + cloudDisk, err := computeService.RegionDisks.Get(p, region, volName).Do() + Expect(err).To(BeNil(), "Could not get disk from cloud directly") + Expect(cloudDisk.Type).To(ContainSubstring(hdhaDiskType)) + Expect(cloudDisk.Status).To(Equal(readyState)) + Expect(cloudDisk.SizeGb).To(Equal(defaultRepdSizeGb)) + Expect(cloudDisk.Name).To(Equal(volName)) + Expect(len(cloudDisk.ReplicaZones)).To(Equal(2)) + zonesSet := sets.NewString(zones...) + for _, replicaZone := range cloudDisk.ReplicaZones { + tokens := strings.Split(replicaZone, "/") + actualZone := tokens[len(tokens)-1] + Expect(zonesSet.Has(actualZone)).To(BeTrue(), "Expected zone %v to exist in zone set %v", actualZone, zones) + } + + defer func() { + // Delete Disk + controllerClient.DeleteVolume(volume.VolumeId) + Expect(err).To(BeNil(), "DeleteVolume failed") + + // Validate Disk Deleted + _, err = computeService.RegionDisks.Get(p, region, volName).Do() + Expect(gce.IsGCEError(err, "notFound")).To(BeTrue(), "Expected disk to not be found") + }() + + // For each of the two instances + i := 0 + for _, testContext := range zoneToContext { + err = testAttachWriteReadDetach(volume.VolumeId, volName, testContext.Instance, testContext.Client, false) + Expect(err).To(BeNil(), "failed volume lifecycle checks") + i = i + 1 + } + }) + + It("Should create a HdHA instance, write to it, force-attach it to another instance, and read the same data", func() { + Expect(hyperdiskTestContexts).NotTo(BeEmpty()) + + zoneToContext := map[string]*remote.TestContext{} + zones := []string{} + + for _, tc := range hyperdiskTestContexts { + _, z, _ := tc.Instance.GetIdentity() + // Zone hasn't been seen before + if _, ok := zoneToContext[z]; !ok { + zoneToContext[z] = tc + zones = append(zones, z) + } + if len(zoneToContext) == 2 { + break + } + } + + Expect(len(zoneToContext)).To(Equal(2), "Must have instances in 2 zones") + + controllerContext := zoneToContext[zones[0]] + controllerClient := controllerContext.Client + controllerInstance := controllerContext.Instance + + p, _, _ := controllerInstance.GetIdentity() + + region, err := common.GetRegionFromZones(zones) + Expect(err).To(BeNil(), "Failed to get region from zones") + + // Create Disk + volName := testNamePrefix + string(uuid.NewUUID()) + volume, err := controllerClient.CreateVolume(volName, map[string]string{ + common.ParameterKeyType: common.ParameterHdHADiskType, + common.ParameterAvailabilityClass: common.ParameterRegionalHardFailoverClass, + }, defaultRepdSizeGb, &csi.TopologyRequirement{ + Requisite: []*csi.Topology{ + { + Segments: map[string]string{common.TopologyKeyZone: zones[0]}, + }, + { + Segments: map[string]string{common.TopologyKeyZone: zones[1]}, + }, + }, + }, nil) + Expect(err).To(BeNil(), "CreateVolume failed with error: %v", err) + + // Validate Disk Created + cloudDisk, err := computeService.RegionDisks.Get(p, region, volName).Do() + Expect(err).To(BeNil(), "Could not get disk from cloud directly") + Expect(cloudDisk.Type).To(ContainSubstring(hdhaDiskType)) + Expect(cloudDisk.Status).To(Equal(readyState)) + Expect(cloudDisk.SizeGb).To(Equal(defaultRepdSizeGb)) + Expect(cloudDisk.Name).To(Equal(volName)) + Expect(len(cloudDisk.ReplicaZones)).To(Equal(2)) + zonesSet := sets.NewString(zones...) + for _, replicaZone := range cloudDisk.ReplicaZones { + tokens := strings.Split(replicaZone, "/") + actualZone := tokens[len(tokens)-1] + Expect(zonesSet.Has(actualZone)).To(BeTrue(), "Expected zone %v to exist in zone set %v", actualZone, zones) + } + Expect(volume.VolumeContext).To(HaveKeyWithValue("force-attach", "true")) + + detachers := []detacherFunc{} + + defer func() { + // Perform any detaches + for _, fn := range detachers { + fn() + } + + // Delete Disk + controllerClient.DeleteVolume(volume.VolumeId) + Expect(err).To(BeNil(), "DeleteVolume failed") + + // Validate Disk Deleted + _, err = computeService.RegionDisks.Get(p, region, volName).Do() + Expect(gce.IsGCEError(err, "notFound")).To(BeTrue(), "Expected disk to not be found") + }() + + // Attach disk to instance in the first zone. + tc0 := zoneToContext[zones[0]] + err, detacher, args := testAttachAndMount(volume.VolumeId, volName, tc0.Instance, tc0.Client, attachAndMountArgs{ + readOnly: false, + useBlock: false, + forceAttach: false, + }) + detachers = append(detachers, detacher) + Expect(err).To(BeNil(), "failed attach in zone 0") + testFileName := filepath.Join(args.publishDir, "force-attach-test") + testFileContents := "force attach test" + err = testutils.WriteFile(tc0.Instance, testFileName, testFileContents) + Expect(err).To(BeNil(), "failed write in zone 0") + _, err = tc0.Instance.SSH("sync") // Sync so force detach doesn't lose data. + Expect(err).To(BeNil(), "failed sync") + + readContents, err := testutils.ReadFile(tc0.Instance, testFileName) + Expect(err).To(BeNil(), "failed read in zone 0") + Expect(strings.TrimSpace(string(readContents))).To(BeIdenticalTo(testFileContents), "content mismatch in zone 0") + + // Now force attach to the second instance without detaching. + tc1 := zoneToContext[zones[1]] + err, detacher, _ = testAttachAndMount(volume.VolumeId, volName, tc1.Instance, tc1.Client, attachAndMountArgs{ + readOnly: false, + useBlock: false, + forceAttach: true, + }) + detachers = append(detachers, detacher) + Expect(err).To(BeNil(), "failed force attach in zone 1") + readContents, err = testutils.ReadFile(tc1.Instance, testFileName) + Expect(err).To(BeNil(), "failed read in zone 1") + Expect(strings.TrimSpace(string(readContents))).To(BeIdenticalTo(testFileContents), "content mismatch in zone 1") + }) }) func deleteDisk(controllerClient *remote.CsiClient, p, zone, volID, volName string) { diff --git a/test/e2e/tests/setup_e2e_test.go b/test/e2e/tests/setup_e2e_test.go index 2e1c8cf51..ebe1a920a 100644 --- a/test/e2e/tests/setup_e2e_test.go +++ b/test/e2e/tests/setup_e2e_test.go @@ -49,12 +49,15 @@ var ( cloudtopHost = flag.Bool("cloudtop-host", false, "The local host is cloudtop, a kind of googler machine with special requirements to access GCP") extraDriverFlags = flag.String("extra-driver-flags", "", "Extra flags to pass to the driver") enableConfidentialCompute = flag.Bool("enable-confidential-compute", false, "Create VMs with confidential compute mode. This uses NVMe devices") - - testContexts = []*remote.TestContext{} - computeService *compute.Service - computeAlphaService *computealpha.Service - computeBetaService *computebeta.Service - kmsClient *cloudkms.KeyManagementClient + hdMachineType = flag.String("hyperdisk-machine-type", "c3-standard-4", "Type of machine to provision instance on") + hdMinCpuPlatform = flag.String("hyperdisk-min-cpu-platform", "sapphirerapids", "Minimum CPU architecture") + + testContexts = []*remote.TestContext{} + hyperdiskTestContexts = []*remote.TestContext{} + computeService *compute.Service + computeAlphaService *computealpha.Service + computeBetaService *computebeta.Service + kmsClient *cloudkms.KeyManagementClient ) func init() { @@ -70,7 +73,9 @@ func TestE2E(t *testing.T) { var _ = BeforeSuite(func() { var err error tcc := make(chan *remote.TestContext) + hdtcc := make(chan *remote.TestContext) defer close(tcc) + defer close(hdtcc) zones := strings.Split(*zones, ",") @@ -101,7 +106,11 @@ var _ = BeforeSuite(func() { for _, zone := range zones { go func(curZone string) { defer GinkgoRecover() - tcc <- NewTestContext(curZone) + tcc <- NewDefaultTestContext(curZone) + }(zone) + go func(curZone string) { + defer GinkgoRecover() + hdtcc <- NewTestContext(curZone, *hdMinCpuPlatform, *hdMachineType) }(zone) } @@ -109,6 +118,9 @@ var _ = BeforeSuite(func() { tc := <-tcc testContexts = append(testContexts, tc) klog.Infof("Added TestContext for node %s", tc.Instance.GetName()) + tc = <-hdtcc + hyperdiskTestContexts = append(hyperdiskTestContexts, tc) + klog.Infof("Added TestContext for node %s", tc.Instance.GetName()) } }) @@ -133,17 +145,21 @@ func getDriverConfig() testutils.DriverConfig { } } -func NewTestContext(zone string) *remote.TestContext { - nodeID := fmt.Sprintf("%s-%s", *vmNamePrefix, zone) +func NewDefaultTestContext(zone string) *remote.TestContext { + return NewTestContext(zone, *minCpuPlatform, *machineType) +} + +func NewTestContext(zone, minCpuPlatform, machineType string) *remote.TestContext { + nodeID := fmt.Sprintf("%s-%s-%s", *vmNamePrefix, zone, machineType) klog.Infof("Setting up node %s", nodeID) instanceConfig := remote.InstanceConfig{ Project: *project, Architecture: *architecture, - MinCpuPlatform: *minCpuPlatform, + MinCpuPlatform: minCpuPlatform, Zone: zone, Name: nodeID, - MachineType: *machineType, + MachineType: machineType, ServiceAccount: *serviceAccount, ImageURL: *imageURL, CloudtopHost: *cloudtopHost, diff --git a/test/e2e/tests/single_zone_e2e_test.go b/test/e2e/tests/single_zone_e2e_test.go index c77c7b911..da83ad0b5 100644 --- a/test/e2e/tests/single_zone_e2e_test.go +++ b/test/e2e/tests/single_zone_e2e_test.go @@ -64,6 +64,7 @@ const ( hdxDiskType = "hyperdisk-extreme" hdtDiskType = "hyperdisk-throughput" hdmlDiskType = "hyperdisk-ml" + hdhaDiskType = "hyperdisk-balanced-high-availability" provisionedIOPSOnCreate = "12345" provisionedIOPSOnCreateInt = int64(12345) provisionedIOPSOnCreateDefaultInt = int64(100000) diff --git a/test/e2e/utils/utils.go b/test/e2e/utils/utils.go index ec905c890..bc040b152 100644 --- a/test/e2e/utils/utils.go +++ b/test/e2e/utils/utils.go @@ -68,6 +68,7 @@ func GCEClientAndDriverSetup(instance *remote.InstanceInfo, driverConfig DriverC "--use-instance-api-to-list-volumes-published-nodes", "--supports-dynamic-iops-provisioning=hyperdisk-balanced,hyperdisk-extreme", "--supports-dynamic-throughput-provisioning=hyperdisk-balanced,hyperdisk-throughput,hyperdisk-ml", + "--allow-hdha-provisioning", fmt.Sprintf("--fallback-requisite-zones=%s", strings.Join(driverConfig.Zones, ",")), } extra_flags = append(extra_flags, fmt.Sprintf("--compute-endpoint=%s", driverConfig.ComputeEndpoint)) diff --git a/test/run-e2e.sh b/test/run-e2e.sh index 0775ac695..3f42f9d05 100755 --- a/test/run-e2e.sh +++ b/test/run-e2e.sh @@ -5,7 +5,7 @@ set -x readonly PKGDIR=sigs.k8s.io/gcp-compute-persistent-disk-csi-driver -TIMEOUT=30m +TIMEOUT=40m if [ "$RUN_CONTROLLER_MODIFY_VOLUME_TESTS" = true ]; then TIMEOUT=45m fi diff --git a/test/sanity/sanity_test.go b/test/sanity/sanity_test.go index c6a2dad22..47df0712f 100644 --- a/test/sanity/sanity_test.go +++ b/test/sanity/sanity_test.go @@ -75,7 +75,7 @@ func TestSanity(t *testing.T) { //Initialize GCE Driver identityServer := driver.NewIdentityServer(gceDriver) - controllerServer := driver.NewControllerServer(gceDriver, cloudProvider, 0, 5*time.Minute, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig) + controllerServer := driver.NewControllerServer(gceDriver, cloudProvider, 0, 5*time.Minute, fallbackRequisiteZones, enableStoragePools, multiZoneVolumeHandleConfig, listVolumesConfig, provisionableDisksConfig, true) fakeStatter := mountmanager.NewFakeStatterWithOptions(mounter, mountmanager.FakeStatterOptions{IsBlock: false}) nodeServer := driver.NewNodeServer(gceDriver, mounter, deviceUtils, metadataservice.NewFakeService(), fakeStatter) err = gceDriver.SetupGCEDriver(driverName, vendorVersion, extraLabels, nil, identityServer, controllerServer, nodeServer)