From f3e8c3b30daedf78bd2d97fbc7650cc62f2312d4 Mon Sep 17 00:00:00 2001 From: Cezary Zawdka Date: Thu, 30 Dec 2021 16:00:28 +0100 Subject: [PATCH 1/5] GCE load balancer: Stop managing instance groups for internal load balancer if rbs alpha feature gate enable use multiple instance groups if present --- .../legacy-cloud-providers/gce/gce_alpha.go | 4 ++++ .../gce/gce_instancegroup.go | 10 ++++++++++ .../gce/gce_loadbalancer_internal.go | 18 ++++++++++++++---- 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go index bbc419f23be0d..5c58d0a4cbb30 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go @@ -23,6 +23,10 @@ const ( // AlphaFeatureILBSubsets allows InternalLoadBalancer services to include a subset // of cluster nodes as backends instead of all nodes. AlphaFeatureILBSubsets = "ILBSubsets" + + // AlphaFeatureNetLBRbs enabled L4 Regional Backend Services and + // disables instance group management in service controller + AlphaFeatureNetLBRbs = "NetLB_RBS" ) // AlphaFeatureGate contains a mapping of alpha features to whether they are enabled diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go index d1e929a86798a..f7a12e2cdc58b 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go @@ -50,6 +50,16 @@ func (g *Cloud) DeleteInstanceGroup(name string, zone string) error { return mc.Observe(g.c.InstanceGroups().Delete(ctx, meta.ZonalKey(name, zone))) } +// FilterInstanceGroupsByName lists all InstanceGroups in the project and +// zone that match the name regexp. +func (g *Cloud) FilterInstanceGroupsByName(name, zone string) ([]*compute.InstanceGroup, error) { + ctx, cancel := cloud.ContextWithCallTimeout() + defer cancel() + mc := newInstanceGroupMetricContext("list", zone) + v, err := g.c.InstanceGroups().List(ctx, zone, filter.Regexp("name", name)) + return v, mc.Observe(err) +} + // ListInstanceGroups lists all InstanceGroups in the project and // zone. func (g *Cloud) ListInstanceGroups(zone string) ([]*compute.InstanceGroup, error) { diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index 473b27f0ced7b..5690a8504cc4f 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -625,11 +625,21 @@ func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]s klog.V(2).Infof("ensureInternalInstanceGroups(%v): %d nodes over %d zones in region %v", name, len(nodes), len(zonedNodes), g.region) var igLinks []string for zone, nodes := range zonedNodes { - igLink, err := g.ensureInternalInstanceGroup(name, zone, nodes) - if err != nil { - return []string{}, err + if g.AlphaFeatureGate.Enabled(AlphaFeatureNetLBRbs) { + igs, err := g.FilterInstanceGroupsByName(name, zone) + if err != nil { + return []string{}, err + } + for _, ig := range igs { + igLinks = append(igLinks, ig.SelfLink) + } + } else { + igLink, err := g.ensureInternalInstanceGroup(name, zone, nodes) + if err != nil { + return []string{}, err + } + igLinks = append(igLinks, igLink) } - igLinks = append(igLinks, igLink) } return igLinks, nil From 187ad4695efda75d40f539ea3971684bf4b4c368 Mon Sep 17 00:00:00 2001 From: Cezary Zawdka Date: Fri, 31 Dec 2021 14:25:57 +0100 Subject: [PATCH 2/5] GCE load balancer: unit test for ensuring instance groups while using multiple IGs with rbs alpha feature gate enabled --- .../gce/gce_instancegroup.go | 2 +- .../gce/gce_loadbalancer_internal_test.go | 30 ++++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go index f7a12e2cdc58b..e8b37526221e3 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go @@ -55,7 +55,7 @@ func (g *Cloud) DeleteInstanceGroup(name string, zone string) error { func (g *Cloud) FilterInstanceGroupsByName(name, zone string) ([]*compute.InstanceGroup, error) { ctx, cancel := cloud.ContextWithCallTimeout() defer cancel() - mc := newInstanceGroupMetricContext("list", zone) + mc := newInstanceGroupMetricContext("filter", zone) v, err := g.c.InstanceGroups().List(ctx, zone, filter.Regexp("name", name)) return v, mc.Observe(err) } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index 5102c0c99732a..e0930ebf688aa 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go @@ -23,6 +23,7 @@ import ( "context" "fmt" "reflect" + "sort" "strings" "testing" @@ -165,12 +166,39 @@ func TestEnsureInternalInstanceGroupsLimit(t *testing.T) { igName := makeInstanceGroupName(vals.ClusterID) _, err = gce.ensureInternalInstanceGroups(igName, nodes) require.NoError(t, err) - instances, err := gce.ListInstancesInInstanceGroup(igName, vals.ZoneName, allInstances) require.NoError(t, err) assert.Equal(t, maxInstancesPerInstanceGroup, len(instances)) } +func TestEnsureMultipleInstanceGroups(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureNetLBRbs}) + + nodes, err := createAndInsertNodes(gce, []string{"n1"}, vals.ZoneName) + require.NoError(t, err) + + baseName := makeInstanceGroupName(vals.ClusterID) + clusterIGs := []string{baseName, baseName + "-1", baseName + "-2", baseName + "-3"} + for _, igName := range append(clusterIGs, "zz-another-ig", "k8s-ig--cluster2-id") { + ig := &compute.InstanceGroup{Name: igName} + err := gce.CreateInstanceGroup(ig, vals.ZoneName) + require.NoError(t, err) + } + + igsFromCloud, err := gce.ensureInternalInstanceGroups(baseName, nodes) + require.NoError(t, err) + assert.Len(t, igsFromCloud, len(clusterIGs), "Incorrect number of Instance Groups") + sort.Strings(igsFromCloud) + for i, igName := range clusterIGs { + assert.True(t, strings.HasSuffix(igsFromCloud[i], igName)) + } +} + func TestEnsureInternalLoadBalancer(t *testing.T) { t.Parallel() From f2e29bf4f0e33b2d85f46ff69ce2ae3814b562c3 Mon Sep 17 00:00:00 2001 From: Cezary Zawdka Date: Wed, 5 Jan 2022 13:59:52 +0100 Subject: [PATCH 3/5] GCE load balancer: skip instance group deletion while using multiple IGs with rbs alpha feature gate enabled --- .../gce/gce_loadbalancer_internal.go | 6 +++-- .../gce/gce_loadbalancer_internal_test.go | 24 +++++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index 5690a8504cc4f..c7c0c91a32480 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -654,8 +654,10 @@ func (g *Cloud) ensureInternalInstanceGroupsDeleted(name string) error { klog.V(2).Infof("ensureInternalInstanceGroupsDeleted(%v): attempting delete instance group in all %d zones", name, len(zones)) for _, z := range zones { - if err := g.DeleteInstanceGroup(name, z.Name); err != nil && !isNotFoundOrInUse(err) { - return err + if !g.AlphaFeatureGate.Enabled(AlphaFeatureNetLBRbs) { + if err := g.DeleteInstanceGroup(name, z.Name); err != nil && !isNotFoundOrInUse(err) { + return err + } } } return nil diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index e0930ebf688aa..4ab4062857674 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go @@ -553,6 +553,30 @@ func TestEnsureInternalLoadBalancerDeleted(t *testing.T) { assertInternalLbResourcesDeleted(t, gce, svc, vals, true) } +func TestSkipInstanceGroupDeletion(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + + + svc := fakeLoadbalancerService(string(LBTypeInternal)) + svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) + require.NoError(t, err) + _, err = createInternalLoadBalancer(gce, svc, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) + assert.NoError(t, err) + + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureNetLBRbs}) + err = gce.ensureInternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, svc) + assert.NoError(t, err) + + igName := makeInstanceGroupName(vals.ClusterID) + ig, err := gce.GetInstanceGroup(igName, vals.ZoneName) + assert.NoError(t, err) + assert.NotNil(t, ig, "Instance group should not be deleted when flag 'NetLB_RBS' is present") +} + func TestEnsureInternalLoadBalancerDeletedTwiceDoesNotError(t *testing.T) { t.Parallel() From c92b9227988bd67fded3797f4fd102048e0ee10b Mon Sep 17 00:00:00 2001 From: Cezary Zawdka Date: Tue, 11 Jan 2022 15:40:12 +0100 Subject: [PATCH 4/5] Applied review comments --- .../src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go | 4 ++-- .../gce/gce_loadbalancer_internal.go | 9 +++++---- .../gce/gce_loadbalancer_internal_test.go | 7 +++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go index 5c58d0a4cbb30..64a4f1b236157 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go @@ -24,9 +24,9 @@ const ( // of cluster nodes as backends instead of all nodes. AlphaFeatureILBSubsets = "ILBSubsets" - // AlphaFeatureNetLBRbs enabled L4 Regional Backend Services and + // AlphaFeatureSkipIGsManagement enabled L4 Regional Backend Services and // disables instance group management in service controller - AlphaFeatureNetLBRbs = "NetLB_RBS" + AlphaFeatureSkipIGsManagement = "SkipIGsManagement" ) // AlphaFeatureGate contains a mapping of alpha features to whether they are enabled diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index c7c0c91a32480..3a40d5cba16dd 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -625,7 +625,7 @@ func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]s klog.V(2).Infof("ensureInternalInstanceGroups(%v): %d nodes over %d zones in region %v", name, len(nodes), len(zonedNodes), g.region) var igLinks []string for zone, nodes := range zonedNodes { - if g.AlphaFeatureGate.Enabled(AlphaFeatureNetLBRbs) { + if g.AlphaFeatureGate.Enabled(AlphaFeatureSkipIGsManagement) { igs, err := g.FilterInstanceGroupsByName(name, zone) if err != nil { return []string{}, err @@ -652,9 +652,10 @@ func (g *Cloud) ensureInternalInstanceGroupsDeleted(name string) error { return err } - klog.V(2).Infof("ensureInternalInstanceGroupsDeleted(%v): attempting delete instance group in all %d zones", name, len(zones)) - for _, z := range zones { - if !g.AlphaFeatureGate.Enabled(AlphaFeatureNetLBRbs) { + // Skip Instance Group deletion if IG management was moved out of k/k code + if !g.AlphaFeatureGate.Enabled(AlphaFeatureSkipIGsManagement) { + klog.V(2).Infof("ensureInternalInstanceGroupsDeleted(%v): attempting delete instance group in all %d zones", name, len(zones)) + for _, z := range zones { if err := g.DeleteInstanceGroup(name, z.Name); err != nil && !isNotFoundOrInUse(err) { return err } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go index 4ab4062857674..15cb6acbbd88e 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal_test.go @@ -177,9 +177,9 @@ func TestEnsureMultipleInstanceGroups(t *testing.T) { vals := DefaultTestClusterValues() gce, err := fakeGCECloud(vals) require.NoError(t, err) - gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureNetLBRbs}) + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureSkipIGsManagement}) - nodes, err := createAndInsertNodes(gce, []string{"n1"}, vals.ZoneName) + nodes, err := createAndInsertNodes(gce, []string{"n1"}, vals.ZoneName) require.NoError(t, err) baseName := makeInstanceGroupName(vals.ClusterID) @@ -560,14 +560,13 @@ func TestSkipInstanceGroupDeletion(t *testing.T) { gce, err := fakeGCECloud(vals) require.NoError(t, err) - svc := fakeLoadbalancerService(string(LBTypeInternal)) svc, err = gce.client.CoreV1().Services(svc.Namespace).Create(context.TODO(), svc, metav1.CreateOptions{}) require.NoError(t, err) _, err = createInternalLoadBalancer(gce, svc, nil, []string{"test-node-1"}, vals.ClusterName, vals.ClusterID, vals.ZoneName) assert.NoError(t, err) - gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureNetLBRbs}) + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureSkipIGsManagement}) err = gce.ensureInternalLoadBalancerDeleted(vals.ClusterName, vals.ClusterID, svc) assert.NoError(t, err) From 3f5a8754d0b67af3c50cb63d4aa2a4c946af1657 Mon Sep 17 00:00:00 2001 From: Cezary Zawadka Date: Thu, 27 Jan 2022 10:01:42 +0100 Subject: [PATCH 5/5] Applied review comments --- .../k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go | 4 ++-- .../legacy-cloud-providers/gce/gce_loadbalancer_internal.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go index e8b37526221e3..46c58792b6832 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_instancegroup.go @@ -52,11 +52,11 @@ func (g *Cloud) DeleteInstanceGroup(name string, zone string) error { // FilterInstanceGroupsByName lists all InstanceGroups in the project and // zone that match the name regexp. -func (g *Cloud) FilterInstanceGroupsByName(name, zone string) ([]*compute.InstanceGroup, error) { +func (g *Cloud) FilterInstanceGroupsByNamePrefix(namePrefix, zone string) ([]*compute.InstanceGroup, error) { ctx, cancel := cloud.ContextWithCallTimeout() defer cancel() mc := newInstanceGroupMetricContext("filter", zone) - v, err := g.c.InstanceGroups().List(ctx, zone, filter.Regexp("name", name)) + v, err := g.c.InstanceGroups().List(ctx, zone, filter.Regexp("name", namePrefix+".*")) return v, mc.Observe(err) } diff --git a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go index 3a40d5cba16dd..3fca2d76397a5 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go +++ b/staging/src/k8s.io/legacy-cloud-providers/gce/gce_loadbalancer_internal.go @@ -626,9 +626,9 @@ func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]s var igLinks []string for zone, nodes := range zonedNodes { if g.AlphaFeatureGate.Enabled(AlphaFeatureSkipIGsManagement) { - igs, err := g.FilterInstanceGroupsByName(name, zone) + igs, err := g.FilterInstanceGroupsByNamePrefix(name, zone) if err != nil { - return []string{}, err + return nil, err } for _, ig := range igs { igLinks = append(igLinks, ig.SelfLink) @@ -636,7 +636,7 @@ func (g *Cloud) ensureInternalInstanceGroups(name string, nodes []*v1.Node) ([]s } else { igLink, err := g.ensureInternalInstanceGroup(name, zone, nodes) if err != nil { - return []string{}, err + return nil, err } igLinks = append(igLinks, igLink) }