Skip to content

Commit 88bb599

Browse files
mattcarymattcary
authored
Merge pull request #2214 from mattcary/zone-121
Unpublish legacy volume even if it appears in multiple zones
2 parents df01f09 + 7fcb19f commit 88bb599

File tree

4 files changed

+148
-63
lines changed

4 files changed

+148
-63
lines changed

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

Lines changed: 2 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -101,35 +101,8 @@ func (cloud *FakeCloudProvider) GetDefaultZone() string {
101101
return cloud.zone
102102
}
103103

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

135108
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
)
4545

@@ -99,7 +99,7 @@ type GCECompute interface {
9999
GetDefaultZone() string
100100
// Disk Methods
101101
GetDisk(ctx context.Context, project string, volumeKey *meta.Key) (*CloudDisk, error)
102-
RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error)
102+
RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error)
103103
InsertDisk(ctx context.Context, project string, volKey *meta.Key, params common.DiskParameters, capBytes int64, capacityRange *csi.CapacityRange, replicaZones []string, snapshotID string, volumeContentSourceVolumeID string, multiWriter bool, accessMode string) error
104104
DeleteDisk(ctx context.Context, project string, volumeKey *meta.Key) error
105105
UpdateDisk(ctx context.Context, project string, volKey *meta.Key, existingDisk *CloudDisk, params common.ModifyVolumeParameters) error
@@ -290,26 +290,30 @@ func (cloud *CloudProvider) listInstancesForProject(service *computev1.Service,
290290

291291
// RepairUnderspecifiedVolumeKey will query the cloud provider and check each zone for the disk specified
292292
// by the volume key and return a volume key with a correct zone
293-
func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key) (string, *meta.Key, error) {
293+
func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error) {
294+
return repairUnderspecifiedVolumeKeyWithProvider(ctx, cloud, project, volumeKey, fallbackZone)
295+
}
296+
297+
func repairUnderspecifiedVolumeKeyWithProvider(ctx context.Context, cloud GCECompute, project string, volumeKey *meta.Key, fallbackZone string) (string, *meta.Key, error) {
294298
klog.V(5).Infof("Repairing potentially underspecified volume key %v", volumeKey)
295299
if project == common.UnspecifiedValue {
296-
project = cloud.project
300+
project = cloud.GetDefaultProject()
297301
}
298-
region, err := common.GetRegionFromZones([]string{cloud.zone})
302+
region, err := common.GetRegionFromZones([]string{cloud.GetDefaultZone()})
299303
if err != nil {
300304
return "", nil, fmt.Errorf("failed to get region from zones: %w", err)
301305
}
302306
switch volumeKey.Type() {
303307
case meta.Zonal:
304-
foundZone := ""
305308
if volumeKey.Zone == common.UnspecifiedValue {
306309
// list all zones, try to get disk in each zone
307310
zones, err := cloud.ListZones(ctx, region)
308311
if err != nil {
309312
return "", nil, err
310313
}
314+
diskZones := []string{}
311315
for _, zone := range zones {
312-
_, err := cloud.getZonalDiskOrError(ctx, project, zone, volumeKey.Name)
316+
_, err := cloud.GetDisk(ctx, project, &meta.Key{Name: volumeKey.Name, Zone: zone})
313317
if err != nil {
314318
if IsGCENotFoundError(err) {
315319
// Couldn't find the disk in this zone so we keep
@@ -320,16 +324,22 @@ func (cloud *CloudProvider) RepairUnderspecifiedVolumeKey(ctx context.Context, p
320324
// so we return error immediately
321325
return "", nil, err
322326
}
323-
if len(foundZone) > 0 {
324-
return "", nil, fmt.Errorf("found disk %s in more than one zone: %s and %s", volumeKey.Name, foundZone, zone)
325-
}
326-
foundZone = zone
327+
diskZones = append(diskZones, zone)
327328
}
328329

329-
if len(foundZone) == 0 {
330+
if len(diskZones) == 0 {
330331
return "", nil, notFoundError()
332+
} else if len(diskZones) > 1 && fallbackZone == "" {
333+
return "", nil, fmt.Errorf("found disk %s in more than one zone and no fallback: %v", volumeKey.Name, diskZones)
334+
} else if len(diskZones) > 1 && fallbackZone != "" {
335+
if !slices.Contains(diskZones, fallbackZone) {
336+
return "", nil, fmt.Errorf("found disk %s in more than one zone (%v) but none match fallback zone %s", volumeKey.Name, diskZones, fallbackZone)
337+
}
338+
volumeKey.Zone = fallbackZone
339+
} else {
340+
volumeKey.Zone = diskZones[0]
331341
}
332-
volumeKey.Zone = foundZone
342+
333343
return project, volumeKey, nil
334344
}
335345
return project, volumeKey, nil
@@ -392,22 +402,6 @@ func (cloud *CloudProvider) GetDisk(ctx context.Context, project string, key *me
392402
}
393403
}
394404

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

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

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

1146-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1146+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
11471147
if err != nil {
11481148
if gce.IsGCENotFoundError(err) {
11491149
return nil, status.Errorf(codes.NotFound, "ControllerPublishVolume could not find volume with ID %v: %v", volumeID, err.Error()), nil
@@ -1291,7 +1291,7 @@ func (gceCS *GCEControllerServer) executeControllerUnpublishVolume(ctx context.C
12911291
volKey = convertMultiZoneVolKeyToZoned(volKey, instanceZone)
12921292
}
12931293

1294-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1294+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, instanceZone)
12951295
if err != nil {
12961296
if gce.IsGCENotFoundError(err) {
12971297
klog.Warningf("Treating volume %v as unpublished because it could not be found", volumeID)
@@ -1363,7 +1363,7 @@ func (gceCS *GCEControllerServer) ValidateVolumeCapabilities(ctx context.Context
13631363
return nil, status.Errorf(codes.InvalidArgument, "Volume ID is invalid: %v", err.Error())
13641364
}
13651365

1366-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1366+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
13671367
if err != nil {
13681368
if gce.IsGCENotFoundError(err) {
13691369
return nil, status.Errorf(codes.NotFound, "ValidateVolumeCapabilities could not find volume with ID %v: %v", volumeID, err.Error())
@@ -1959,7 +1959,7 @@ func (gceCS *GCEControllerServer) ControllerExpandVolume(ctx context.Context, re
19591959
if err != nil {
19601960
return nil, status.Errorf(codes.InvalidArgument, "ControllerExpandVolume Volume ID is invalid: %v", err.Error())
19611961
}
1962-
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey)
1962+
project, volKey, err = gceCS.CloudProvider.RepairUnderspecifiedVolumeKey(ctx, project, volKey, "")
19631963

19641964
if err != nil {
19651965
if gce.IsGCENotFoundError(err) {

0 commit comments

Comments
 (0)