From 5623006f984661e957e92f6f02b256169bfccfaf Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Tue, 30 Jul 2019 12:36:32 -0700 Subject: [PATCH 01/12] unit test --- .../gce/gce_loadbalancer_internal_test.go | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) 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 f2aaf029f7efe..2e56bb65d178e 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 @@ -31,6 +31,7 @@ import ( "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + cloudprovider "k8s.io/cloud-provider" servicehelper "k8s.io/cloud-provider/service/helpers" ) @@ -847,3 +848,65 @@ func TestCompareHealthChecks(t *testing.T) { }) } } + +// Test creation of InternalLoadBalancer with ILB Subsets featuregate enabled. +func TestEnsureInternalLoadBalancerSubsetting(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBSubsets}) + + nodeNames := []string{"test-node-1"} + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + require.NoError(t, err) + + svc := fakeLoadbalancerService(string(LBTypeInternal)) + status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName) + assert.EqualError(t, err, cloudprovider.ImplementedElsewhere.Error()) + // No loadbalancer resources will be created due to the ILB Feature Gate + assert.Empty(t, status) + assertInternalLbResourcesDeleted(t, gce, svc, vals, true) + // Invoking delete should be a no-op + err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc) + assert.NoError(t, err) + assertInternalLbResourcesDeleted(t, gce, svc, vals, true) + // Now remove the feature gate so that lb resources are created + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{}) + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + assert.NoError(t, err) + assert.NotEmpty(t, status.Ingress) + assertInternalLbResources(t, gce, svc, vals, nodeNames) +} + +func TestEnsureInternalLoadBalancerDeletedSubsetting(t *testing.T) { + t.Parallel() + + vals := DefaultTestClusterValues() + gce, err := fakeGCECloud(vals) + require.NoError(t, err) + + nodeNames := []string{"test-node-1"} + nodes, err := createAndInsertNodes(gce, nodeNames, vals.ZoneName) + require.NoError(t, err) + svc := fakeLoadbalancerService(string(LBTypeInternal)) + status, err := createInternalLoadBalancer(gce, svc, nil, nodeNames, vals.ClusterName, vals.ClusterID, vals.ZoneName) + + assert.NoError(t, err) + assert.NotEmpty(t, status.Ingress) + // Enable FeatureGatee + gce.AlphaFeatureGate = NewAlphaFeatureGate([]string{AlphaFeatureILBSubsets}) + newLBStatus := v1.LoadBalancerStatus{Ingress: []v1.LoadBalancerIngress{{IP: "1.2.3.4"}}} + // mock scenario where a different controller modifies status. + svc.Status.LoadBalancer = newLBStatus + status, err = gce.EnsureLoadBalancer(context.Background(), vals.ClusterName, svc, nodes) + assert.EqualError(t, err, cloudprovider.ImplementedElsewhere.Error()) + // ensure that the status is empty + assert.Empty(t, status) + assert.Equal(t, svc.Status.LoadBalancer, newLBStatus) + // Invoked when service is deleted. + err = gce.EnsureLoadBalancerDeleted(context.Background(), vals.ClusterName, svc) + assert.NoError(t, err) + assertInternalLbResourcesDeleted(t, gce, svc, vals, true) +} From d79d4e85259eca065a153367940706a55a510c7b Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Mon, 15 Jul 2019 00:01:33 -0700 Subject: [PATCH 02/12] Handle ImplementedElsewhere error in service_controller This is used when the cloudprovider layer is not implementing loadBalancer service. Implementation will be in a different controller running on master. --- pkg/controller/service/service_controller.go | 11 ++++++++++- .../gce/gce_loadbalancer_internal.go | 8 ++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index a3da8b6554aea..ffba3de787318 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -339,6 +339,10 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st } newStatus, err = s.ensureLoadBalancer(service) if err != nil { + if err == cloudprovider.ImplementedElsewhere { + klog.Infof("LoadBalancer for service %s not implemented by alternate controller %s, Ignoring error", key, s.cloud.ProviderName()) + return op, nil + } return op, fmt.Errorf("failed to ensure load balancer: %v", err) } s.eventRecorder.Event(service, v1.EventTypeNormal, "EnsuredLoadBalancer", "Ensured load balancer") @@ -703,7 +707,12 @@ func (s *ServiceController) lockedUpdateLoadBalancerHosts(service *v1.Service, h } return nil } - + if err == cloudprovider.ImplementedElsewhere { + // Skip error since LoadBalancer implementation is in some other controller. In this case, the loadBalancer will likely not + // exist and will be handled in the if block below. Adding this check in case the alternate loadBalancer implementation + // uses the same naming scheme. + return nil + } // It's only an actual error if the load balancer still exists. if _, exists, err := s.balancer.GetLoadBalancer(context.TODO(), s.clusterName, service); err != nil { runtime.HandleError(fmt.Errorf("failed to check if load balancer exists for service %s/%s: %v", service.Namespace, service.Name, 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 881cf9ab21ac1..89182a1716cb4 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 @@ -27,6 +27,7 @@ import ( "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" + cloudprovider "k8s.io/cloud-provider" servicehelpers "k8s.io/cloud-provider/service/helpers" "k8s.io/klog" ) @@ -36,6 +37,10 @@ const ( ) func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) { + if g.AlphaFeatureGate.Enabled("ILBSubsets") { + return nil, cloudprovider.NotImplemented + } + nm := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} ports, protocol := getPortsAndProtocol(svc.Spec.Ports) if protocol != v1.ProtocolTCP && protocol != v1.ProtocolUDP { @@ -201,6 +206,9 @@ func (g *Cloud) clearPreviousInternalResources(svc *v1.Service, loadBalancerName // updateInternalLoadBalancer is called when the list of nodes has changed. Therefore, only the instance groups // and possibly the backend service need to be updated. func (g *Cloud) updateInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, nodes []*v1.Node) error { + if g.AlphaFeatureGate.Enabled("ILBSubsets") { + return cloudprovider.NotImplemented + } g.sharedResourceLock.Lock() defer g.sharedResourceLock.Unlock() From acaa16c2729d48aca181ffd5d3c06ba826febac5 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Mon, 15 Jul 2019 22:04:47 -0700 Subject: [PATCH 03/12] skip ilb creation if subsetting is enabled. --- .../k8s.io/legacy-cloud-providers/gce/gce_alpha.go | 1 + .../gce/gce_loadbalancer_internal.go | 12 ++++++++---- 2 files changed, 9 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 598f1e336d3f3..db31a9e74c452 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 @@ -26,6 +26,7 @@ const ( // // alpha: v1.8 (for Services) AlphaFeatureNetworkTiers = "NetworkTiers" + AlphaFeatureILBSubsets = "ILBSubsets" ) // 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 89182a1716cb4..f6e1b1642ab83 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 @@ -36,9 +36,13 @@ const ( allInstances = "ALL" ) +func (g *Cloud) usesSubsets() bool { + return g.AlphaFeatureGate != nil && g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) + } + func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) { - if g.AlphaFeatureGate.Enabled("ILBSubsets") { - return nil, cloudprovider.NotImplemented + if g.usesSubsets() { + return nil, cloudprovider.ImplementedElsewhere } nm := types.NamespacedName{Name: svc.Name, Namespace: svc.Namespace} @@ -206,8 +210,8 @@ func (g *Cloud) clearPreviousInternalResources(svc *v1.Service, loadBalancerName // updateInternalLoadBalancer is called when the list of nodes has changed. Therefore, only the instance groups // and possibly the backend service need to be updated. func (g *Cloud) updateInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, nodes []*v1.Node) error { - if g.AlphaFeatureGate.Enabled("ILBSubsets") { - return cloudprovider.NotImplemented + if g.usesSubsets() { + return cloudprovider.ImplementedElsewhere } g.sharedResourceLock.Lock() defer g.sharedResourceLock.Unlock() From 8382906c9d346549ea72bacfdc9b004887c620d1 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Mon, 29 Jul 2019 15:37:03 -0700 Subject: [PATCH 04/12] Add a new error type in cloud.go Adding a new error "ImplementedElsewhere" to cloud.go. This error indicates that implementation for a particular service/loadbalancer spec is handled by a different controller. The caller can ignore the error and skip modifying service status upon receiving this error. --- staging/src/k8s.io/cloud-provider/cloud.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/cloud.go b/staging/src/k8s.io/cloud-provider/cloud.go index 6db0219520bfd..5da25c1f27dd5 100644 --- a/staging/src/k8s.io/cloud-provider/cloud.go +++ b/staging/src/k8s.io/cloud-provider/cloud.go @@ -199,9 +199,10 @@ type Routes interface { } var ( - InstanceNotFound = errors.New("instance not found") - DiskNotFound = errors.New("disk is not found") - NotImplemented = errors.New("unimplemented") + InstanceNotFound = errors.New("instance not found") + DiskNotFound = errors.New("disk is not found") + NotImplemented = errors.New("unimplemented") + ImplementedElsewhere = errors.New("Implemented by a different controller") ) // Zone represents the location of a particular machine. From d4210b94e25cc6b06f5b1072a52f854efbc0a8b6 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Tue, 30 Jul 2019 15:21:52 -0700 Subject: [PATCH 05/12] addressed review comments --- pkg/controller/service/service_controller.go | 9 +++++---- staging/src/k8s.io/cloud-provider/cloud.go | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index ffba3de787318..91204bad6d665 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -340,7 +340,8 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st newStatus, err = s.ensureLoadBalancer(service) if err != nil { if err == cloudprovider.ImplementedElsewhere { - klog.Infof("LoadBalancer for service %s not implemented by alternate controller %s, Ignoring error", key, s.cloud.ProviderName()) + klog.Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", + key, s.cloud.ProviderName()) return op, nil } return op, fmt.Errorf("failed to ensure load balancer: %v", err) @@ -708,9 +709,9 @@ func (s *ServiceController) lockedUpdateLoadBalancerHosts(service *v1.Service, h return nil } if err == cloudprovider.ImplementedElsewhere { - // Skip error since LoadBalancer implementation is in some other controller. In this case, the loadBalancer will likely not - // exist and will be handled in the if block below. Adding this check in case the alternate loadBalancer implementation - // uses the same naming scheme. + // ImplementedElsewhere indicates that the UpdateLoadBalancer is a nop and the + // functionality is implemented by a different controller. In this case, we + // return immediately without doing anything. return nil } // It's only an actual error if the load balancer still exists. diff --git a/staging/src/k8s.io/cloud-provider/cloud.go b/staging/src/k8s.io/cloud-provider/cloud.go index 5da25c1f27dd5..164d2f4fd71de 100644 --- a/staging/src/k8s.io/cloud-provider/cloud.go +++ b/staging/src/k8s.io/cloud-provider/cloud.go @@ -199,10 +199,10 @@ type Routes interface { } var ( - InstanceNotFound = errors.New("instance not found") DiskNotFound = errors.New("disk is not found") - NotImplemented = errors.New("unimplemented") ImplementedElsewhere = errors.New("Implemented by a different controller") + InstanceNotFound = errors.New("instance not found") + NotImplemented = errors.New("unimplemented") ) // Zone represents the location of a particular machine. From ffb450423060f87edcb1066564694dfd0f39914d Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Tue, 30 Jul 2019 22:49:16 -0700 Subject: [PATCH 06/12] fixed error message --- staging/src/k8s.io/cloud-provider/cloud.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/cloud-provider/cloud.go b/staging/src/k8s.io/cloud-provider/cloud.go index 164d2f4fd71de..0131acc7fb688 100644 --- a/staging/src/k8s.io/cloud-provider/cloud.go +++ b/staging/src/k8s.io/cloud-provider/cloud.go @@ -22,7 +22,7 @@ import ( "fmt" "strings" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" @@ -200,7 +200,7 @@ type Routes interface { var ( DiskNotFound = errors.New("disk is not found") - ImplementedElsewhere = errors.New("Implemented by a different controller") + ImplementedElsewhere = errors.New("implemented by a different controller") InstanceNotFound = errors.New("instance not found") NotImplemented = errors.New("unimplemented") ) From dbfc876e831f7c965227574e165e66a5bdaf3a77 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Wed, 31 Jul 2019 13:05:51 -0700 Subject: [PATCH 07/12] Fixed review comments, lint. Added error check to EnsureLoadBalancerDeleted. --- pkg/controller/service/service_controller.go | 7 ++++++- staging/src/k8s.io/cloud-provider/cloud.go | 2 +- staging/src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go | 4 +++- .../gce/gce_loadbalancer_internal.go | 4 ++-- 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index 91204bad6d665..a0009126ce27b 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -311,6 +311,11 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st klog.V(2).Infof("Deleting existing load balancer for service %s", key) s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletingLoadBalancer", "Deleting load balancer") if err := s.balancer.EnsureLoadBalancerDeleted(context.TODO(), s.clusterName, service); err != nil { + if err == cloudprovider.ImplementedElsewhere { + klog.V(3).Infof("LoadBalancer for service %s implemented by a different controller %s," + + "Ignoring error upon deletion", key, s.cloud.ProviderName()) + return op, nil + } return op, fmt.Errorf("failed to delete load balancer: %v", err) } } @@ -340,7 +345,7 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st newStatus, err = s.ensureLoadBalancer(service) if err != nil { if err == cloudprovider.ImplementedElsewhere { - klog.Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", + klog.V(3).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", key, s.cloud.ProviderName()) return op, nil } diff --git a/staging/src/k8s.io/cloud-provider/cloud.go b/staging/src/k8s.io/cloud-provider/cloud.go index 0131acc7fb688..c42160e25d62a 100644 --- a/staging/src/k8s.io/cloud-provider/cloud.go +++ b/staging/src/k8s.io/cloud-provider/cloud.go @@ -200,7 +200,7 @@ type Routes interface { var ( DiskNotFound = errors.New("disk is not found") - ImplementedElsewhere = errors.New("implemented by a different controller") + ImplementedElsewhere = errors.New("implemented by alternate to cloud provider") InstanceNotFound = errors.New("instance not found") NotImplemented = errors.New("unimplemented") ) 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 db31a9e74c452..d3e70b9153e08 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 @@ -26,7 +26,9 @@ const ( // // alpha: v1.8 (for Services) AlphaFeatureNetworkTiers = "NetworkTiers" - AlphaFeatureILBSubsets = "ILBSubsets" + // AlphaFeatureILBSubsets allows InternalLoadBalancer services to include a subset + // of cluster nodes as backends instead of all nodes. + AlphaFeatureILBSubsets = "ILBSubsets" ) // 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 f6e1b1642ab83..27b02ce2a2a6f 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 @@ -37,8 +37,8 @@ const ( ) func (g *Cloud) usesSubsets() bool { - return g.AlphaFeatureGate != nil && g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) - } + return g.AlphaFeatureGate != nil && g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) +} func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) { if g.usesSubsets() { From 3244e5da5a391446ca932002663f2ceaaf91473f Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Wed, 31 Jul 2019 16:15:20 -0700 Subject: [PATCH 08/12] Added comment on how to use ImplementedElsewhere --- staging/src/k8s.io/cloud-provider/cloud.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/staging/src/k8s.io/cloud-provider/cloud.go b/staging/src/k8s.io/cloud-provider/cloud.go index c42160e25d62a..f7bb761a57f2f 100644 --- a/staging/src/k8s.io/cloud-provider/cloud.go +++ b/staging/src/k8s.io/cloud-provider/cloud.go @@ -104,6 +104,12 @@ func GetInstanceProviderID(ctx context.Context, cloud Interface, nodeName types. } // LoadBalancer is an abstract, pluggable interface for load balancers. +// PR https://github.com/kubernetes/kubernetes/pull/80660 added support for cloud providers to return an error +// ImplementedElsewhere. This error is used to indicate that the loadbalancer implementation is handled outside of +// cloud provider, maybe in a different controller. +// In order to use this correctly, the cloud-provider implementation needs to return "NotFound" +// for GetLoadBalancer and empty string for GetLoadBalancerName. EnsureLoadBalancer and UpdateLoadBalancer need to +// return ImplementedElsewhere error. EnsureLoadBalancerDeleted can return ImplementedElsewhere as well. type LoadBalancer interface { // TODO: Break this up into different interfaces (LB, etc) when we have more than one type of service // GetLoadBalancer returns whether the specified load balancer exists, and From 987677c2011b1edca5d6ab17865f31d2158679de Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Wed, 31 Jul 2019 16:35:22 -0700 Subject: [PATCH 09/12] Setting log level to 4 since default is 2 or 3. Depending on how the cluster is created. Test clusters set a default level of 4. --- pkg/controller/service/service_controller.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index a0009126ce27b..81b1afe43174a 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -312,7 +312,7 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletingLoadBalancer", "Deleting load balancer") if err := s.balancer.EnsureLoadBalancerDeleted(context.TODO(), s.clusterName, service); err != nil { if err == cloudprovider.ImplementedElsewhere { - klog.V(3).Infof("LoadBalancer for service %s implemented by a different controller %s," + + klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s," + "Ignoring error upon deletion", key, s.cloud.ProviderName()) return op, nil } @@ -345,7 +345,7 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st newStatus, err = s.ensureLoadBalancer(service) if err != nil { if err == cloudprovider.ImplementedElsewhere { - klog.V(3).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", + klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", key, s.cloud.ProviderName()) return op, nil } From da887e85e98adabb4526e84d661e8ff34d369694 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Thu, 1 Aug 2019 00:48:56 -0700 Subject: [PATCH 10/12] Updated comment about ImplementedElsewhere Removed handling ImplementedElsewhere error in call to EnsureLoadBalancerDeleted. --- pkg/controller/service/service_controller.go | 5 ----- staging/src/k8s.io/cloud-provider/cloud.go | 23 ++++++++++++++------ 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index 81b1afe43174a..95cc877fa7eef 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -311,11 +311,6 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st klog.V(2).Infof("Deleting existing load balancer for service %s", key) s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletingLoadBalancer", "Deleting load balancer") if err := s.balancer.EnsureLoadBalancerDeleted(context.TODO(), s.clusterName, service); err != nil { - if err == cloudprovider.ImplementedElsewhere { - klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s," + - "Ignoring error upon deletion", key, s.cloud.ProviderName()) - return op, nil - } return op, fmt.Errorf("failed to delete load balancer: %v", err) } } diff --git a/staging/src/k8s.io/cloud-provider/cloud.go b/staging/src/k8s.io/cloud-provider/cloud.go index f7bb761a57f2f..38703f10302cf 100644 --- a/staging/src/k8s.io/cloud-provider/cloud.go +++ b/staging/src/k8s.io/cloud-provider/cloud.go @@ -22,7 +22,7 @@ import ( "fmt" "strings" - v1 "k8s.io/api/core/v1" + "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/informers" clientset "k8s.io/client-go/kubernetes" @@ -104,12 +104,21 @@ func GetInstanceProviderID(ctx context.Context, cloud Interface, nodeName types. } // LoadBalancer is an abstract, pluggable interface for load balancers. -// PR https://github.com/kubernetes/kubernetes/pull/80660 added support for cloud providers to return an error -// ImplementedElsewhere. This error is used to indicate that the loadbalancer implementation is handled outside of -// cloud provider, maybe in a different controller. -// In order to use this correctly, the cloud-provider implementation needs to return "NotFound" -// for GetLoadBalancer and empty string for GetLoadBalancerName. EnsureLoadBalancer and UpdateLoadBalancer need to -// return ImplementedElsewhere error. EnsureLoadBalancerDeleted can return ImplementedElsewhere as well. +// +// Cloud provider may chose to implement the logic for +// constructing/destroying specific kinds of load balancers in a +// controller separate from the ServiceController. If this is the case, +// then {Ensure,Update}LoadBalancer must return the ImplementedElsewhere error. +// For the given LB service, the GetLoadBalancer must return "exists=True" if +// there exists a LoadBalancer instance created by ServiceController. +// In all other cases, GetLoadBalancer must return a NotFound error. +// EnsureLoadBalancerDeleted must not return ImplementedElsewhere to ensure +// proper teardown of resources that were allocated by the ServiceController. +// This can happen if a user changes the type of LB via an update to the resource +// or when migrating from ServiceController to alternate implementation. +// The finalizer on the service will be added and removed by ServiceController +// irrespective of the ImplementedElsewhere error. Additional finalizers for +// LB services must be managed in the alternate implementation. type LoadBalancer interface { // TODO: Break this up into different interfaces (LB, etc) when we have more than one type of service // GetLoadBalancer returns whether the specified load balancer exists, and From 501081e64fe56d4542d9315704327343efd51277 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Fri, 2 Aug 2019 10:34:02 -0700 Subject: [PATCH 11/12] Moved nil check inside AlphaFeatureGate.Enabled --- .../src/k8s.io/legacy-cloud-providers/gce/gce_alpha.go | 3 +++ .../gce/gce_loadbalancer_internal.go | 8 ++------ 2 files changed, 5 insertions(+), 6 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 d3e70b9153e08..efb57c0abd5de 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 @@ -38,6 +38,9 @@ type AlphaFeatureGate struct { // Enabled returns true if the provided alpha feature is enabled func (af *AlphaFeatureGate) Enabled(key string) bool { + if af == nil || af.features == nil { + return false + } return af.features[key] } 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 27b02ce2a2a6f..9b0c109da0035 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 @@ -36,12 +36,8 @@ const ( allInstances = "ALL" ) -func (g *Cloud) usesSubsets() bool { - return g.AlphaFeatureGate != nil && g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) -} - func (g *Cloud) ensureInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, existingFwdRule *compute.ForwardingRule, nodes []*v1.Node) (*v1.LoadBalancerStatus, error) { - if g.usesSubsets() { + if g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) { return nil, cloudprovider.ImplementedElsewhere } @@ -210,7 +206,7 @@ func (g *Cloud) clearPreviousInternalResources(svc *v1.Service, loadBalancerName // updateInternalLoadBalancer is called when the list of nodes has changed. Therefore, only the instance groups // and possibly the backend service need to be updated. func (g *Cloud) updateInternalLoadBalancer(clusterName, clusterID string, svc *v1.Service, nodes []*v1.Node) error { - if g.usesSubsets() { + if g.AlphaFeatureGate.Enabled(AlphaFeatureILBSubsets) { return cloudprovider.ImplementedElsewhere } g.sharedResourceLock.Lock() From ecad65a3f8622fdf1be556a6c1cd8e19007f3949 Mon Sep 17 00:00:00 2001 From: Pavithra Ramesh Date: Fri, 2 Aug 2019 11:24:24 -0700 Subject: [PATCH 12/12] Added comment after invoking ensureLoadBalancer --- pkg/controller/service/service_controller.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/controller/service/service_controller.go b/pkg/controller/service/service_controller.go index 95cc877fa7eef..8fd896bfcc22e 100644 --- a/pkg/controller/service/service_controller.go +++ b/pkg/controller/service/service_controller.go @@ -340,8 +340,10 @@ func (s *ServiceController) syncLoadBalancerIfNeeded(service *v1.Service, key st newStatus, err = s.ensureLoadBalancer(service) if err != nil { if err == cloudprovider.ImplementedElsewhere { - klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", - key, s.cloud.ProviderName()) + // ImplementedElsewhere indicates that the ensureLoadBalancer is a nop and the + // functionality is implemented by a different controller. In this case, we + // return immediately without doing anything. + klog.V(4).Infof("LoadBalancer for service %s implemented by a different controller %s, Ignoring error", key, s.cloud.ProviderName()) return op, nil } return op, fmt.Errorf("failed to ensure load balancer: %v", err)