Skip to content

Commit

Permalink
Merge pull request #2064 from mmamczur/cherry-pick#2063-to-1.22
Browse files Browse the repository at this point in the history 
[Cherry-pick #2063, #2067 to 1.22] Fix a case when regional instance group l…
  • Loading branch information
@k8s-ci-robot
k8s-ci-robot authored Apr 13, 2023
2 parents a567694 + d1db438 commit 874c52c
Show file tree
Hide file tree
Showing 4 changed files with 156 additions and 40 deletions.
13 changes: 7 additions & 6 deletions pkg/backends/ig_linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (igl *instanceGroupLinker) Link(sp utils.ServicePort, groups []GroupKey) er
return err
}

addIGs, err := getInstanceGroupsToAdd(be, igLinks)
addIGs, _, err := getInstanceGroupsToAddAndRemove(be, igLinks)
if err != nil {
return err
}
Expand Down Expand Up @@ -167,12 +167,12 @@ func backendsForIGs(igLinks []string, bm BalancingMode) []*composite.Backend {
return backends
}

func getInstanceGroupsToAdd(be *composite.BackendService, igLinks []string) ([]string, error) {
func getInstanceGroupsToAddAndRemove(be *composite.BackendService, igLinks []string) ([]string, sets.String, error) {
existingIGs := sets.String{}
for _, existingBe := range be.Backends {
path, err := utils.RelativeResourceName(existingBe.Group)
if err != nil {
return nil, fmt.Errorf("failed to parse instance group: %w", err)
return nil, nil, fmt.Errorf("failed to parse instance group: %w", err)
}
existingIGs.Insert(path)
}
Expand All @@ -181,15 +181,16 @@ func getInstanceGroupsToAdd(be *composite.BackendService, igLinks []string) ([]s
for _, igLink := range igLinks {
path, err := utils.RelativeResourceName(igLink)
if err != nil {
return nil, fmt.Errorf("failed to parse instance group: %w", err)
return nil, nil, fmt.Errorf("failed to parse instance group: %w", err)
}
wantIGs.Insert(path)
}

missingIGs := wantIGs.Difference(existingIGs)
if missingIGs.Len() > 0 {
removeIGs := existingIGs.Difference(wantIGs)
if missingIGs.Len() > 0 || removeIGs.Len() > 0 {
klog.V(2).Infof("Backend service %q has instance groups %+v, want %+v",
be.Name, existingIGs.List(), wantIGs.List())
}
return missingIGs.List(), nil
return missingIGs.List(), removeIGs, nil
}
46 changes: 29 additions & 17 deletions pkg/backends/ig_linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,62 +183,71 @@ func TestBackendsForIG(t *testing.T) {
}
}

func TestGetIGLinksToAdd(t *testing.T) {
func TestGetIGLinksToAddAndRemove(t *testing.T) {
url := "https://googleapis.com/v1/compute/projects/my-project/global/backendServices/%s"
link := "projects/my-project/global/backendServices/%s"
for _, tc := range []struct {
name string
igLinks []string
currentIGLinks []string
want []string
wantAdd []string
wantRemove []string
}{
{
name: "empty",
igLinks: []string{},
currentIGLinks: []string{},
want: []string{},
wantAdd: []string{},
wantRemove: []string{},
},
{
name: "No IGs to add",
igLinks: []string{fmt.Sprintf(url, "same-backend")},
currentIGLinks: []string{fmt.Sprintf(url, "same-backend")},
want: []string{},
wantAdd: []string{},
wantRemove: []string{},
},
{
name: "same IGs in wrong order",
igLinks: []string{fmt.Sprintf(url, "same-backend2"), fmt.Sprintf(url, "same-backend")},
currentIGLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
want: []string{},
wantAdd: []string{},
wantRemove: []string{},
},
{
name: "one IG to add",
igLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2"), fmt.Sprintf(url, "same-backend3")},
currentIGLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
want: []string{fmt.Sprintf(link, "same-backend3")},
wantAdd: []string{fmt.Sprintf(link, "same-backend3")},
wantRemove: []string{},
},
{
name: "IGs in wrong order",
igLinks: []string{fmt.Sprintf(url, "same-backend2"), fmt.Sprintf(url, "same-backend")},
currentIGLinks: []string{fmt.Sprintf(url, "same-backend3"), fmt.Sprintf(url, "same-backend2")},
want: []string{fmt.Sprintf(link, "same-backend")},
wantAdd: []string{fmt.Sprintf(link, "same-backend")},
wantRemove: []string{fmt.Sprintf(link, "same-backend3")},
},
{
name: "different IGs",
igLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
currentIGLinks: []string{fmt.Sprintf(url, "same-backend3"), fmt.Sprintf(url, "same-backend4")},
want: []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
wantAdd: []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
wantRemove: []string{fmt.Sprintf(link, "same-backend3"), fmt.Sprintf(link, "same-backend4")},
},
{
name: "empty current",
igLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
currentIGLinks: []string{},
want: []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
wantAdd: []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
wantRemove: []string{},
},
{
name: "empty IGs and non-empty current",
igLinks: []string{},
currentIGLinks: []string{fmt.Sprintf(url, "same-backend"), fmt.Sprintf(url, "same-backend2")},
want: []string{},
wantAdd: []string{},
wantRemove: []string{fmt.Sprintf(link, "same-backend"), fmt.Sprintf(link, "same-backend2")},
},
} {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -251,15 +260,18 @@ func TestGetIGLinksToAdd(t *testing.T) {
newBackends = append(newBackends, b)
}
be.Backends = newBackends
toAdd, err := getInstanceGroupsToAdd(&be, tc.igLinks)
toAdd, toRemove, err := getInstanceGroupsToAddAndRemove(&be, tc.igLinks)
if err != nil {
t.Fatalf("getInstanceGroupsToAdd(_,_): err:%v ", err)
t.Fatalf("getInstanceGroupsToAddAndRemove(_,_): err:%v ", err)
}
sort.Slice(toAdd, func(i, j int) bool {
return toAdd[i] <= toAdd[j]
})
if !reflect.DeepEqual(toAdd, tc.want) {
t.Fatalf("getInstanceGroupsToAdd(_,_) error. Got:%v, Want:%v", toAdd, tc.want)
sort.Strings(toAdd)
if !reflect.DeepEqual(toAdd, tc.wantAdd) {
t.Fatalf("getInstanceGroupsToAddAndRemove(_,_) toAdd error. Got:%v, Want:%v", toAdd, tc.wantAdd)
}
toRemoveList := toRemove.List()
sort.Strings(tc.wantRemove)
if !reflect.DeepEqual(toRemoveList, tc.wantRemove) {
t.Fatalf("getInstanceGroupsToAddAndRemove(_,_) toRemove error. Got:%v, Want:%v", toRemoveList, tc.wantRemove)
}
})
}
Expand Down
26 changes: 19 additions & 7 deletions pkg/backends/regional_ig_linker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,25 +51,37 @@ func (linker *RegionalInstanceGroupLinker) Link(sp utils.ServicePort, projectID
if err != nil {
return err
}
addIGs, err := getInstanceGroupsToAdd(bs, igLinks)
addIGs, removeIGs, err := getInstanceGroupsToAddAndRemove(bs, igLinks)
if err != nil {
return err
}
if len(addIGs) == 0 {
klog.V(3).Infof("No backends to add for %s, skipping update.", sp.BackendName())
if len(addIGs) == 0 && len(removeIGs) == 0 {
klog.V(3).Infof("No backends to add or remove for %s, skipping update.", sp.BackendName())
return nil
}

var newBackends []*composite.Backend
if len(removeIGs) != 0 {
var backendsWithoutRemoved []*composite.Backend
for _, b := range bs.Backends {
path, err := utils.RelativeResourceName(b.Group)
if err != nil {
return err
}
if !removeIGs.Has(path) {
backendsWithoutRemoved = append(backendsWithoutRemoved, b)
}
}
bs.Backends = backendsWithoutRemoved
}

for _, igLink := range addIGs {
b := &composite.Backend{
Group: igLink,
}
newBackends = append(newBackends, b)
bs.Backends = append(bs.Backends, b)
}

bs.Backends = newBackends
klog.V(3).Infof("Update Backend %s, with %d backends.", sp.BackendName(), len(addIGs))
klog.V(3).Infof("Update Backend %s, with %d added backends (total %d).", sp.BackendName(), len(addIGs), len(bs.Backends))
if err := linker.backendPool.Update(bs); err != nil {
return fmt.Errorf("updating backend service %s for IG failed, err:%w", sp.BackendName(), err)
}
Expand Down
111 changes: 101 additions & 10 deletions pkg/backends/regional_ig_linker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
package backends

import (
"strings"
"testing"

"github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud"
Expand All @@ -25,12 +26,14 @@ import (
"k8s.io/ingress-gce/pkg/test"
"k8s.io/ingress-gce/pkg/utils"
"k8s.io/ingress-gce/pkg/utils/namer"
"k8s.io/ingress-gce/pkg/utils/slice"
"k8s.io/legacy-cloud-providers/gce"
)

const (
uscentralzone = "us-central1-a"
hcLink = "some_hc_link"
usCentral1AZone = "us-central1-a"
usCentral1CZone = "us-central1-c"
hcLink = "some_hc_link"
)

func linkerTestClusterValues() gce.TestClusterValues {
Expand All @@ -45,7 +48,7 @@ func linkerTestClusterValues() gce.TestClusterValues {

func newTestRegionalIgLinker(fakeGCE *gce.Cloud, backendPool *Backends, l4Namer *namer.L4Namer) *RegionalInstanceGroupLinker {
fakeIGs := instancegroups.NewEmptyFakeInstanceGroups()
fakeZL := &instancegroups.FakeZoneLister{Zones: []string{uscentralzone}}
fakeZL := &instancegroups.FakeZoneLister{Zones: []string{usCentral1AZone, "us-central1-c"}}
fakeInstancePool := instancegroups.NewManager(&instancegroups.ManagerConfig{
Cloud: fakeIGs,
Namer: l4Namer,
Expand All @@ -69,18 +72,18 @@ func TestRegionalLink(t *testing.T) {
fakeBackendPool := NewPool(fakeGCE, l4Namer)
linker := newTestRegionalIgLinker(fakeGCE, fakeBackendPool, l4Namer)

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err == nil {
if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err == nil {
t.Fatalf("Linking when instances does not exist should return error")
}
if _, err := linker.instancePool.EnsureInstanceGroupsAndPorts(l4Namer.InstanceGroup(), []int64{sp.NodePort}); err != nil {
t.Fatalf("Unexpected error when ensuring IG for ServicePort %+v: %v", sp, err)
}
if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err == nil {
if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err == nil {
t.Fatalf("Linking when backend service does not exist should return error")
}
createBackendService(t, sp, fakeBackendPool)

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err != nil {
if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err != nil {
t.Fatalf("Unexpected error in Link. Error: %v", err)
}

Expand All @@ -91,7 +94,7 @@ func TestRegionalLink(t *testing.T) {
if len(be.Backends) == 0 {
t.Fatalf("Expected Backends to be created")
}
ig, _ := linker.instancePool.Get(sp.IGName(), uscentralzone)
ig, _ := linker.instancePool.Get(sp.IGName(), usCentral1AZone)
expectedLink, _ := utils.RelativeResourceName(ig.SelfLink)
if be.Backends[0].Group != expectedLink {
t.Fatalf("Expected Backend Group: %s received: %s", expectedLink, be.Backends[0].Group)
Expand All @@ -111,13 +114,13 @@ func TestRegionalUpdateLink(t *testing.T) {
}
createBackendService(t, sp, fakeBackendPool)

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err != nil {
if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err != nil {
t.Fatalf("Unexpected error in Link(_). Error: %v", err)
}
// Add error hook to check that Link function will not call Backend Service Update when no IG was added
(fakeGCE.Compute().(*cloud.MockGCE)).MockRegionBackendServices.UpdateHook = test.UpdateRegionBackendServiceWithErrorHookUpdate

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{uscentralzone}); err != nil {
if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err != nil {
t.Fatalf("Unexpected error in Link(_). Error: %v", err)
}
be, err := fakeGCE.GetRegionBackendService(sp.BackendName(), fakeGCE.Region())
Expand All @@ -127,13 +130,101 @@ func TestRegionalUpdateLink(t *testing.T) {
if len(be.Backends) == 0 {
t.Fatalf("Expected Backends to be created")
}
ig, _ := linker.instancePool.Get(sp.IGName(), uscentralzone)
ig, _ := linker.instancePool.Get(sp.IGName(), usCentral1AZone)
expectedLink, _ := utils.RelativeResourceName(ig.SelfLink)
if be.Backends[0].Group != expectedLink {
t.Fatalf("Expected Backend Group: %s received: %s", expectedLink, be.Backends[0].Group)
}
}

func TestRegionalUpdateLinkWithNewBackends(t *testing.T) {
t.Parallel()
fakeGCE := gce.NewFakeGCECloud(linkerTestClusterValues())
clusterID, _ := fakeGCE.ClusterID.GetID()
l4Namer := namer.NewL4Namer("uid1", namer.NewNamer(clusterID, ""))
sp := utils.ServicePort{NodePort: 8080, BackendNamer: l4Namer}
fakeBackendPool := NewPool(fakeGCE, l4Namer)
linker := newTestRegionalIgLinker(fakeGCE, fakeBackendPool, l4Namer)
if _, err := linker.instancePool.EnsureInstanceGroupsAndPorts(l4Namer.InstanceGroup(), []int64{sp.NodePort}); err != nil {
t.Fatalf("Unexpected error when ensuring IG for ServicePort %+v: %v", sp, err)
}
createBackendService(t, sp, fakeBackendPool)

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err != nil {
t.Fatalf("Unexpected error in Link(_). Error: %v", err)
}

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone, usCentral1CZone}); err != nil {
t.Fatalf("Unexpected error in Link(_). Error: %v", err)
}
be, err := fakeGCE.GetRegionBackendService(sp.BackendName(), fakeGCE.Region())
if err != nil {
t.Fatalf("Get Regional Backend Service failed %v", err)
}
var backendGroups []string
for _, b := range be.Backends {
backendGroups = append(backendGroups, b.Group)
}
if len(be.Backends) != 2 {
t.Errorf("Expected that Backends are updated with the added zone with len=2 but got=%+v", strings.Join(backendGroups, ", "))
}

igA, _ := linker.instancePool.Get(sp.IGName(), usCentral1AZone)
expectedLinkA, _ := utils.RelativeResourceName(igA.SelfLink)
igC, _ := linker.instancePool.Get(sp.IGName(), usCentral1CZone)
expectedLinkC, _ := utils.RelativeResourceName(igC.SelfLink)
if !slice.ContainsString(backendGroups, expectedLinkA, nil) {
t.Errorf("expected that BackendService backend contains %v, got=%v", expectedLinkA, backendGroups)
}
if !slice.ContainsString(backendGroups, expectedLinkC, nil) {
t.Errorf("expected that BackendService backend contains %v, got=%v", expectedLinkC, backendGroups)
}
}

func TestRegionalUpdateLinkWithRemovedBackends(t *testing.T) {
t.Parallel()
fakeGCE := gce.NewFakeGCECloud(linkerTestClusterValues())
clusterID, _ := fakeGCE.ClusterID.GetID()
l4Namer := namer.NewL4Namer("uid1", namer.NewNamer(clusterID, ""))
sp := utils.ServicePort{NodePort: 8080, BackendNamer: l4Namer}
fakeBackendPool := NewPool(fakeGCE, l4Namer)
linker := newTestRegionalIgLinker(fakeGCE, fakeBackendPool, l4Namer)
if _, err := linker.instancePool.EnsureInstanceGroupsAndPorts(l4Namer.InstanceGroup(), []int64{sp.NodePort}); err != nil {
t.Fatalf("Unexpected error when ensuring IG for ServicePort %+v: %v", sp, err)
}
createBackendService(t, sp, fakeBackendPool)

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone, usCentral1CZone}); err != nil {
t.Fatalf("Unexpected error in Link(_). Error: %v", err)
}

if err := linker.Link(sp, fakeGCE.ProjectID(), []string{usCentral1AZone}); err != nil {
t.Fatalf("Unexpected error in Link(_). Error: %v", err)
}
be, err := fakeGCE.GetRegionBackendService(sp.BackendName(), fakeGCE.Region())
if err != nil {
t.Fatalf("Get Regional Backend Service failed %v", err)
}
var backendGroups []string
for _, b := range be.Backends {
backendGroups = append(backendGroups, b.Group)
}
if len(be.Backends) != 1 {
t.Errorf("Expected that Backends are updated with the added zone with len=1 but got=%+v", strings.Join(backendGroups, ", "))
}

igA, _ := linker.instancePool.Get(sp.IGName(), usCentral1AZone)
expectedLinkA, _ := utils.RelativeResourceName(igA.SelfLink)
igC, _ := linker.instancePool.Get(sp.IGName(), usCentral1CZone)
expectedLinkC, _ := utils.RelativeResourceName(igC.SelfLink)
if !slice.ContainsString(backendGroups, expectedLinkA, nil) {
t.Errorf("expected that BackendService backend contains %v, got=%v", expectedLinkA, backendGroups)
}
if slice.ContainsString(backendGroups, expectedLinkC, nil) {
t.Errorf("expected that BackendService backend does not contain %v, got=%v", expectedLinkC, backendGroups)
}
}

func createBackendService(t *testing.T, sp utils.ServicePort, backendPool *Backends) {
t.Helper()
namespacedName := types.NamespacedName{Name: "service.Name", Namespace: "service.Namespace"}
Expand Down

0 comments on commit 874c52c

Please sign in to comment.