From 79fb8cc056e43c045d797a011349d8e54f3d5e0a Mon Sep 17 00:00:00 2001 From: Vivek Singh Date: Fri, 8 Mar 2024 18:34:05 +0100 Subject: [PATCH] Fix a bug in `zoneToRegion` which didn't work if multiple zones where passed separated by `__` (#2731) * Fix the bug in `zoneToRegion` There was a bug in `zoneToRegion` function because of which the PV resources were not being created with correct `nodeAffinity` set. `zoneToRegion` tried to get regions from passed zone just by removing `-[onechar]` from the zone. For example if we passed the zone `us-west1-b` this function would return `us-west1` by removing `-b`. This would work properly in the cases where just one zone is given to the function. But if more than one zones are given to the function (separated by `__`), this function didn't work properly. Because for the input `us-west1-a__us-west2-a` it returned `us-west1-a__us-west2` (using existing logic), which is incorrect. As we can see the `-a` was not reomved from first zone. This commit doesn't introduce the separator `__`, it was already part of code and zones were being passed to this funciton separated by `__`. This commit fixes above problem by making sure that we split the zones and then remove `-char` from the zone and add the regions together again using `__` sep. * Add a const for regions and zone separator * Correct function name --- pkg/blockstorage/blockstorage_test.go | 3 +- pkg/blockstorage/gcepd/gcepd.go | 7 ++-- pkg/kube/volume/volume.go | 24 ++++++++++- pkg/kube/volume/volume_test.go | 60 +++++++++++++++++++++++++++ 4 files changed, 88 insertions(+), 6 deletions(-) diff --git a/pkg/blockstorage/blockstorage_test.go b/pkg/blockstorage/blockstorage_test.go index 9cf78ec80e..a98790d5b1 100644 --- a/pkg/blockstorage/blockstorage_test.go +++ b/pkg/blockstorage/blockstorage_test.go @@ -28,6 +28,7 @@ import ( ktags "github.com/kanisterio/kanister/pkg/blockstorage/tags" envconfig "github.com/kanisterio/kanister/pkg/config" "github.com/kanisterio/kanister/pkg/field" + "github.com/kanisterio/kanister/pkg/kube/volume" "github.com/kanisterio/kanister/pkg/log" ) @@ -306,7 +307,7 @@ func (s *BlockStorageProviderSuite) getConfig(c *C, region string) map[string]st } func (b *BlockStorageProviderSuite) isRegional(az string) bool { - return strings.Contains(az, "__") + return strings.Contains(az, volume.RegionZoneSeparator) } func (b *BlockStorageProviderSuite) TestFilterSnasphotWithTags(c *C) { diff --git a/pkg/blockstorage/gcepd/gcepd.go b/pkg/blockstorage/gcepd/gcepd.go index c84aadba27..3defb021a4 100644 --- a/pkg/blockstorage/gcepd/gcepd.go +++ b/pkg/blockstorage/gcepd/gcepd.go @@ -36,6 +36,7 @@ import ( "github.com/kanisterio/kanister/pkg/blockstorage/zone" "github.com/kanisterio/kanister/pkg/field" "github.com/kanisterio/kanister/pkg/kube" + "github.com/kanisterio/kanister/pkg/kube/volume" "github.com/kanisterio/kanister/pkg/log" "github.com/kanisterio/kanister/pkg/poll" ) @@ -409,7 +410,7 @@ func (s *GpdStorage) VolumeCreateFromSnapshot(ctx context.Context, snapshot bloc if err != nil { return nil, err } - volZone := strings.Join(zones, "__") + volZone := strings.Join(zones, volume.RegionZoneSeparator) // Validates new Zones region, err = getRegionFromZones(volZone) if err != nil { @@ -627,7 +628,7 @@ func (s *GpdStorage) dynamicRegionToZoneMap(ctx context.Context) (map[string][]s } func isMultiZone(az string) bool { - return strings.Contains(az, "__") + return strings.Contains(az, volume.RegionZoneSeparator) } // getRegionFromZones function is used from the link below @@ -666,5 +667,5 @@ func (s *GpdStorage) getSelfLinks(ctx context.Context, zones []string) ([]string } func splitZones(az string) []string { - return strings.Split(az, "__") + return strings.Split(az, volume.RegionZoneSeparator) } diff --git a/pkg/kube/volume/volume.go b/pkg/kube/volume/volume.go index 021a7b0b79..2818ccbe55 100644 --- a/pkg/kube/volume/volume.go +++ b/pkg/kube/volume/volume.go @@ -43,6 +43,8 @@ const ( // NoPVCNameSpecified is used by the caller to indicate that the PVC name // should be auto-generated NoPVCNameSpecified = "" + + RegionZoneSeparator = "__" ) // CreatePVC creates a PersistentVolumeClaim and returns its name @@ -336,11 +338,29 @@ func labelSelector(labels map[string]string) string { return strings.Join(ls, ",") } -// zoneToRegion removes -latter or just last latter from provided zone. +// zoneToRegion figures out region from a zone and to do that it +// just removes `-[onchar]` from the end of zone. func zoneToRegion(zone string) string { + // zone can have multiple zone separate by `__` that's why first call + // zonesToRegions to get region for every zone and then return back + // by appending every region with `__` separator + return strings.Join(zonesToRegions(zone), RegionZoneSeparator) +} + +func zonesToRegions(zone string) []string { + reg := map[string]struct{}{} // TODO: gocritic rule below suggests to use regexp.MustCompile but it // panics if regex cannot be compiled. We should add proper test before // enabling this below so that no change to this regex results in a panic r, _ := regexp.Compile("-?[a-z]$") //nolint:gocritic - return r.ReplaceAllString(zone, "") + for _, z := range strings.Split(zone, RegionZoneSeparator) { + zone = r.ReplaceAllString(z, "") + reg[zone] = struct{}{} + } + + var regions []string + for k := range reg { + regions = append(regions, k) + } + return regions } diff --git a/pkg/kube/volume/volume_test.go b/pkg/kube/volume/volume_test.go index cfadac7485..1000251fd6 100644 --- a/pkg/kube/volume/volume_test.go +++ b/pkg/kube/volume/volume_test.go @@ -201,3 +201,63 @@ func (s *TestVolSuite) fakeUnstructuredSnasphotWSize(vsName, namespace, size str } return &unstructured.Unstructured{Object: Object} } + +func (s *TestVolSuite) TestZoneToRegion(c *C) { + for _, tc := range []struct { + zone string + expectedRegion []string + }{ + { + zone: "us-west1-b", + expectedRegion: []string{"us-west1"}, + }, + { + zone: "us-west1-a", + expectedRegion: []string{"us-west1"}, + }, + { + zone: "us-west2-c", + expectedRegion: []string{"us-west2"}, + }, + { + zone: "us-west1-a__us-west2-b", + expectedRegion: []string{"us-west1", "us-west2"}, + }, + { + zone: "us-west1-a__us-west2-b__us-west2-c", + expectedRegion: []string{"us-west1", "us-west2"}, + }, + { + zone: "us-west1-a__us-west1-b__us-west2-b__us-west2-c", + expectedRegion: []string{"us-west1", "us-west2"}, + }, + { + zone: "us-west1-a__us-west1-b__us-west2-b__us-west2-c__us-west2-d", + expectedRegion: []string{"us-west1", "us-west2"}, + }, + } { + reg := zonesToRegions(tc.zone) + c.Assert(slicesEqual(reg, tc.expectedRegion), Equals, true) + } +} + +// slicesEqual compares two unordered slices and returns true if +// both of them have same elements +func slicesEqual(one, two []string) bool { + if len(one) != len(two) { + return false + } + + for _, o := range one { + var found bool + for _, t := range two { + if o == t { + found = true + } + } + if !found { + return false + } + } + return true +}