Skip to content

Commit 4c5bb0b

Browse files
mattcarymattcary
authored
Merge pull request #2207 from mattcary/key-zone
Unpublish legacy volume even if it appears in multiple zones
2 parents 68bb62d + 42459e5 commit 4c5bb0b

File tree

4 files changed

+148
-64
lines changed

4 files changed

+148
-64
lines changed

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

Lines changed: 2 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@ import (
3030
"google.golang.org/grpc/status"
3131
"k8s.io/klog/v2"
3232
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
33-
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants"
3433
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters"
3534

3635
"k8s.io/apimachinery/pkg/util/sets"
@@ -103,35 +102,8 @@ func (cloud *FakeCloudProvider) GetDefaultZone() string {
103102
return cloud.zone
104103
}
105104

106-
func (cloud *FakeCloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error) {
107-
if project == constants.UnspecifiedValue {
108-
project = cloud.project
109-
}
110-
switch volumeKey.Type() {
111-
case meta.Zonal:
112-
if volumeKey.Zone != constants.UnspecifiedValue {
113-
return project, volumeKey, nil
114-
}
115-
for diskVolKey, d := range cloud.disks {
116-
if diskVolKey == volumeKey.String() {
117-
volumeKey.Zone = d.GetZone()
118-
return project, volumeKey, nil
119-
}
120-
}
121-
return "", nil, notFoundError()
122-
case meta.Regional:
123-
if volumeKey.Region != constants.UnspecifiedValue {
124-
return project, volumeKey, nil
125-
}
126-
r, err := common.GetRegionFromZones([]string{cloud.zone})
127-
if err != nil {
128-
return "", nil, fmt.Errorf("failed to get region from zones: %w", err)
129-
}
130-
volumeKey.Region = r
131-
return project, volumeKey, nil
132-
default:
133-
return "", nil, fmt.Errorf("Volume key %v not zonal nor regional", volumeKey.Name)
134-
}
105+
func (cloud *FakeCloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error) {
106+
return repairUnderspecifiedVolumeKeyWithProvider(ctx, cloud, project, volumeKey, fallbackZone)
135107
}
136108

137109
func (cloud *FakeCloudProvider) ListZones(ctx context.Context, region string) ([]string, error) {

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

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121
"fmt"
2222
"net/http"
2323
"regexp"
24+
"slices"
2425
"strings"
2526
"time"
2627

@@ -39,7 +40,6 @@ import (
3940
"google.golang.org/grpc/status"
4041
"k8s.io/apimachinery/pkg/util/wait"
4142
"k8s.io/klog/v2"
42-
"k8s.io/utils/strings/slices"
4343
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
4444
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/constants"
4545
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/parameters"
@@ -101,7 +101,7 @@ type GCECompute interface {
101101
GetDefaultZone() string
102102
// Disk Methods
103103
GetDisk(ctx context.Context, project string, volumeKey *meta.Key) (*CloudDisk, error)
104-
RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error)
104+
RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error)
105105
InsertDisk(ctx context.Context, project string, volKey *meta.Key, params parameters.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error
106106
DeleteDisk(ctx context.Context, project string, volumeKey *meta.Key) error
107107
UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params parameters.ModifyVolumeParameters) error
@@ -292,26 +292,30 @@ func (cloud *CloudProvider) listInstancesForProject(service *computev1.Service,
292292

293293
// RepairUnderspecifiedVolumeKey will query the cloud provider and check each zone for the disk specified
294294
// by the volume key and return a volume key with a correct zone
295-
func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error) {
295+
func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error) {
296+
return repairUnderspecifiedVolumeKeyWithProvider(ctx, cloud, project, volumeKey, fallbackZone)
297+
}
298+
299+
func repairUnderspecifiedVolumeKeyWithProvider(ctx context.Context, cloud GCECompute, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error) {
296300
klog.V(5).Infof("Repairing potentially underspecified volume key %v", volumeKey)
297301
if project == constants.UnspecifiedValue {
298-
project = cloud.project
302+
project = cloud.GetDefaultProject()
299303
}
300-
region, err := common.GetRegionFromZones([]string{cloud.zone})
304+
region, err := common.GetRegionFromZones([]string{cloud.GetDefaultZone()})
301305
if err != nil {
302306
return "", nil, fmt.Errorf("failed to get region from zones: %w", err)
303307
}
304308
switch volumeKey.Type() {
305309
case meta.Zonal:
306-
foundZone := ""
307310
if volumeKey.Zone == constants.UnspecifiedValue {
308311
// list all zones, try to get disk in each zone
309312
zones, err := cloud.ListZones(ctx, region)
310313
if err != nil {
311314
return "", nil, err
312315
}
316+
diskZones := []string{}
313317
for _, zone := range zones {
314-
_, err := cloud.getZonalDiskOrError(ctx, project, zone, volumeKey.Name)
318+
_, err := cloud.GetDisk(ctx, project, &meta.Key{Name: volumeKey.Name, Zone: zone})
315319
if err != nil {
316320
if IsGCENotFoundError(err) {
317321
// Couldn't find the disk in this zone so we keep
@@ -322,16 +326,22 @@ func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, p
322326
// so we return error immediately
323327
return "", nil, err
324328
}
325-
if len(foundZone) > 0 {
326-
return "", nil, fmt.Errorf("found disk %s in more than one zone: %s and %s", volumeKey.Name, foundZone, zone)
327-
}
328-
foundZone = zone
329+
diskZones = append(diskZones, zone)
329330
}
330331

331-
if len(foundZone) == 0 {
332+
if len(diskZones) == 0 {
332333
return "", nil, notFoundError()
334+
} else if len(diskZones) > 1 && fallbackZone == "" {
335+
return "", nil, fmt.Errorf("found disk %s in more than one zone and no fallback: %v", volumeKey.Name, diskZones)
336+
} else if len(diskZones) > 1 && fallbackZone != "" {
337+
if !slices.Contains(diskZones, fallbackZone) {
338+
return "", nil, fmt.Errorf("found disk %s in more than one zone (%v) but none match fallback zone %s", volumeKey.Name, diskZones, fallbackZone)
339+
}
340+
volumeKey.Zone = fallbackZone
341+
} else {
342+
volumeKey.Zone = diskZones[0]
333343
}
334-
volumeKey.Zone = foundZone
344+
335345
return project, volumeKey, nil
336346
}
337347
return project, volumeKey, nil
@@ -394,22 +404,6 @@ func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *me
394404
}
395405
}
396406

397-
func (cloud *CloudProvider) getZonalDiskOrError(ctx context.Context, project, volumeZone, volumeName string) (*computev1.Disk, error) {
398-
disk, err := cloud.service.Disks.Get(project, volumeZone, volumeName).Context(ctx).Do()
399-
if err != nil {
400-
return nil, err
401-
}
402-
return disk, nil
403-
}
404-
405-
func (cloud *CloudProvider) getRegionalDiskOrError(ctx context.Context, project, volumeRegion, volumeName string) (*computev1.Disk, error) {
406-
disk, err := cloud.service.RegionDisks.Get(project, volumeRegion, volumeName).Context(ctx).Do()
407-
if err != nil {
408-
return nil, err
409-
}
410-
return disk, nil
411-
}
412-
413407
func (cloud *CloudProvider) getZonalBetaDiskOrError(ctx context.Context, project, volumeZone, volumeName string) (*computebeta.Disk, error) {
414408
disk, err := cloud.betaService.Disks.Get(project, volumeZone, volumeName).Context(ctx).Do()
415409
if err != nil {

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

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,12 @@ package gcecloudprovider
1616

1717
import (
1818
"context"
19+
"fmt"
1920
"testing"
2021

22+
"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta"
2123
computebeta "google.golang.org/api/compute/v0.beta"
24+
"google.golang.org/api/compute/v1"
2225
computev1 "google.golang.org/api/compute/v1"
2326
"google.golang.org/grpc/codes"
2427
"sigs.k8s.io/gcp-compute-persistent-disk-csi-driver/pkg/common"
@@ -331,3 +334,118 @@ func TestCodeForGCEOpError(t *testing.T) {
331334
}
332335
}
333336
}
337+
338+
func TestRepairUnderspecifiedVolumeKey(t *testing.T) {
339+
cloudProvider, err := CreateFakeCloudProvider("project-id", "country-region-fakefirstzone", []*CloudDisk{
340+
CloudDiskFromV1(&compute.Disk{
341+
Name: "disk-a",
342+
Zone: "country-region-fakefirstzone",
343+
SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project-id/zones/country-region-fakefirstzone/disks/disk-a"),
344+
}),
345+
CloudDiskFromV1(&compute.Disk{
346+
Name: "disk-ab",
347+
Zone: "country-region-fakefirstzone",
348+
SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project-id/zones/country-region-fakefirstzone/disks/disk-ab"),
349+
}),
350+
CloudDiskFromV1(&compute.Disk{
351+
Name: "disk-ab",
352+
Zone: "country-region-fakesecondzone",
353+
SelfLink: fmt.Sprintf("https://www.googleapis.com/compute/v1/projects/project-id/zones/country-region-fakesecondzone/disks/disk-ab"),
354+
}),
355+
})
356+
if err != nil {
357+
t.Fatalf("can't create fake cloud provider: %v", err)
358+
}
359+
360+
for _, tc := range []struct {
361+
testName string
362+
project string
363+
key meta.Key
364+
fallback string
365+
expectedProject string
366+
expectedKey meta.Key
367+
expectError bool
368+
}{
369+
{
370+
testName: "fully specified",
371+
project: "my-project",
372+
key: meta.Key{Name: "disk", Zone: "zone-1"},
373+
expectedProject: "my-project",
374+
expectedKey: meta.Key{Name: "disk", Zone: "zone-1"},
375+
},
376+
{
377+
testName: "fully specified, fallback ignored",
378+
project: "my-project",
379+
key: meta.Key{Name: "disk", Zone: "zone-1"},
380+
fallback: "zone-2",
381+
expectedProject: "my-project",
382+
expectedKey: meta.Key{Name: "disk", Zone: "zone-1"},
383+
},
384+
{
385+
testName: "unspecified zonal",
386+
project: "UNSPECIFIED",
387+
key: meta.Key{Name: "disk-a", Zone: "UNSPECIFIED"},
388+
expectedProject: "project-id",
389+
expectedKey: meta.Key{Name: "disk-a", Zone: "country-region-fakefirstzone"},
390+
},
391+
{
392+
testName: "unspecified regional",
393+
project: "UNSPECIFIED",
394+
key: meta.Key{Name: "disk-a", Region: "UNSPECIFIED"},
395+
expectedProject: "project-id",
396+
expectedKey: meta.Key{Name: "disk-a", Region: "country-region"},
397+
},
398+
{
399+
testName: "multizone regional",
400+
project: "UNSPECIFIED",
401+
key: meta.Key{Name: "disk-ab", Region: "UNSPECIFIED"},
402+
expectedProject: "project-id",
403+
expectedKey: meta.Key{Name: "disk-ab", Region: "country-region"},
404+
},
405+
{
406+
testName: "multi-zone, no fallback",
407+
project: "project-id",
408+
key: meta.Key{Name: "disk-ab", Zone: "UNSPECIFIED"},
409+
expectError: true,
410+
},
411+
{
412+
testName: "multi-zone, no matching fallback",
413+
project: "project-id",
414+
key: meta.Key{Name: "disk-ab", Zone: "UNSPECIFIED"},
415+
fallback: "unknown-zone",
416+
expectError: true,
417+
},
418+
{
419+
testName: "multi-zone, fallback",
420+
project: "my-project",
421+
key: meta.Key{Name: "disk-ab", Zone: "UNSPECIFIED"},
422+
fallback: "country-region-fakesecondzone",
423+
expectedProject: "my-project",
424+
expectedKey: meta.Key{Name: "disk-ab", Zone: "country-region-fakesecondzone"},
425+
},
426+
} {
427+
t.Run(tc.testName, func(t *testing.T) {
428+
// RepairUnderspecifiedVolumeKey mutates the argument as well as returning it, sigh. We verify those semantics here
429+
key := tc.key
430+
prj, retKey, err := cloudProvider.RepairUnderspecifiedVolumeKey(context.Background(), tc.project, &key, tc.fallback)
431+
if tc.expectError {
432+
if err == nil {
433+
t.Error("Expected error but got none")
434+
}
435+
} else {
436+
if err != nil {
437+
t.Errorf("Expected no error but got %v", err)
438+
}
439+
if retKey != &key {
440+
t.Error("Did not return argument key")
441+
}
442+
if prj != tc.expectedProject {
443+
t.Errorf("Got project %s, expected %s", prj, tc.expectedProject)
444+
}
445+
if key.Name != tc.expectedKey.Name || key.Zone != tc.expectedKey.Zone || key.Region != tc.expectedKey.Region {
446+
t.Errorf("Got key %+v, expected %+v", key, tc.expectedKey)
447+
}
448+
}
449+
})
450+
}
451+
}

‎pkg/gce-pd-csi-driver/controller.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -970,7 +970,7 @@ func (gceCS *GCEControllerServer) deleteMultiZoneDisk(ctx context.Context, req *
970970
func (gceCS *GCEControllerServer) deleteSingleDeviceDisk(ctx context.Context, req *csi.DeleteVolumeRequest, project string, volKey *meta.Key) (*csi.DeleteVolumeResponse, error) {
971971
var err error
972972
volumeID := req.GetVolumeId()
973-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
973+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
974974
if err != nil {
975975
if gce.IsGCENotFoundError(err) {
976976
klog.Warningf("DeleteVolume treating volume as deleted because cannot find volume %v: %v", volumeID, err.Error())
@@ -1146,7 +1146,7 @@ func (gceCS *GCEControllerServer) executeControllerPublishVolume(ctx context.Con
11461146
volKey = convertMultiZoneVolKeyToZoned(volKey, instanceZone)
11471147
}
11481148

1149-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1149+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
11501150
if err != nil {
11511151
if gce.IsGCENotFoundError(err) {
11521152
return nil, status.Errorf(codes.NotFound, "ControllerPublishVolume could not find volume with ID %v: %v", volumeID, err.Error()), nil
@@ -1294,7 +1294,7 @@ func (gceCS *GCEControllerServer) executeControllerUnpublishVolume(ctx context.C
12941294
volKey = convertMultiZoneVolKeyToZoned(volKey, instanceZone)
12951295
}
12961296

1297-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1297+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, instanceZone)
12981298
if err != nil {
12991299
if gce.IsGCENotFoundError(err) {
13001300
klog.Warningf("Treating volume %v as unpublished because it could not be found", volumeID)
@@ -1366,7 +1366,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
13661366
return nil, status.Errorf(codes.InvalidArgument, "Volume ID is invalid: %v", err.Error())
13671367
}
13681368

1369-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1369+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
13701370
if err != nil {
13711371
if gce.IsGCENotFoundError(err) {
13721372
return nil, status.Errorf(codes.NotFound, "ValidateVolumeCapabilities could not find volume with ID %v: %v", volumeID, err.Error())
@@ -1962,7 +1962,7 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
19621962
if err != nil {
19631963
return nil, status.Errorf(codes.InvalidArgument, "ControllerExpandVolume Volume ID is invalid: %v", err.Error())
19641964
}
1965-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1965+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
19661966

19671967
if err != nil {
19681968
if gce.IsGCENotFoundError(err) {

0 commit comments

Comments
 (0)