From 99f7df3e1c1a222c9dff2524467133114f2f3a38 Mon Sep 17 00:00:00 2001
From: Antonio Ojea <aojea@google.com>
Date: Tue, 14 Nov 2023 15:17:53 +0000
Subject: [PATCH] improve default_servicecidr_controller startup

The default service-cidr controller blocks the apiserver because it
needs to create the default ServiceCIDR so Services can be allocated.

If the apiserver is started without the default ServiceCIDR any attempt
to createa  new Service will fail, and this is a breaking change for
users and installers that does not retry on this operation.

Instead of using a channel to signal the controller is ready, just
implement two loops, a first one that verifies that is ready and that
polls with a shorted interval, and leave the second loop with the
existing interval.

Change-Id: I54303af9faeaa9c5cce2a840b6b7b0320cd2f4ad
---
 .../default_servicecidr_controller.go         | 48 +++++++++----------
 .../default_servicecidr_controller_test.go    |  1 -
 2 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller.go b/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller.go
index 686bf6e061d..831290d5de9 100644
--- a/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller.go
+++ b/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller.go
@@ -82,8 +82,6 @@ func NewController(
 	c.eventBroadcaster = broadcaster
 	c.eventRecorder = recorder
 
-	c.readyCh = make(chan struct{})
-
 	return c
 }
 
@@ -99,8 +97,6 @@ type Controller struct {
 	serviceCIDRLister   networkingv1alpha1listers.ServiceCIDRLister
 	serviceCIDRsSynced  cache.InformerSynced
 
-	readyCh chan struct{} // channel to block until the default ServiceCIDR exists
-
 	interval time.Duration
 }
 
@@ -120,28 +116,40 @@ func (c *Controller) Start(stopCh <-chan struct{}) {
 		return
 	}
 
-	go wait.Until(c.sync, c.interval, stopCh)
+	// derive a context from the stopCh so we can cancel the poll loop
+	ctx := wait.ContextForChannel(stopCh)
+	// wait until first successfully sync
+	// this blocks apiserver startup so poll with a short interval
+	err := wait.PollUntilContextCancel(ctx, 100*time.Millisecond, true, func(ctx context.Context) (bool, error) {
+		syncErr := c.sync()
+		return syncErr == nil, nil
+	})
+	if err != nil {
+		klog.Infof("error initializing the default ServiceCIDR: %v", err)
 
-	select {
-	case <-stopCh:
-	case <-c.readyCh:
 	}
+
+	// run the sync loop in the background with the defined interval
+	go wait.Until(func() {
+		err := c.sync()
+		if err != nil {
+			klog.Infof("error trying to sync the default ServiceCIDR: %v", err)
+		}
+	}, c.interval, stopCh)
 }
 
-func (c *Controller) sync() {
+func (c *Controller) sync() error {
 	// check if the default ServiceCIDR already exist
 	serviceCIDR, err := c.serviceCIDRLister.Get(DefaultServiceCIDRName)
 	// if exists
 	if err == nil {
-		c.setReady()
 		c.syncStatus(serviceCIDR)
-		return
+		return nil
 	}
 
 	// unknown error
 	if !apierrors.IsNotFound(err) {
-		klog.Infof("error trying to obtain the default ServiceCIDR: %v", err)
-		return
+		return err
 	}
 
 	// default ServiceCIDR does not exist
@@ -156,21 +164,11 @@ func (c *Controller) sync() {
 	}
 	serviceCIDR, err = c.client.NetworkingV1alpha1().ServiceCIDRs().Create(context.Background(), serviceCIDR, metav1.CreateOptions{})
 	if err != nil && !apierrors.IsAlreadyExists(err) {
-		klog.Infof("error creating default ServiceCIDR: %v", err)
 		c.eventRecorder.Eventf(serviceCIDR, v1.EventTypeWarning, "KubernetesDefaultServiceCIDRError", "The default ServiceCIDR can not be created")
-		return
+		return err
 	}
-
-	c.setReady()
 	c.syncStatus(serviceCIDR)
-}
-
-func (c *Controller) setReady() {
-	select {
-	case <-c.readyCh:
-	default:
-		close(c.readyCh)
-	}
+	return nil
 }
 
 func (c *Controller) syncStatus(serviceCIDR *networkingapiv1alpha1.ServiceCIDR) {
diff --git a/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller_test.go b/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller_test.go
index b0173b0e3f3..cf94a594703 100644
--- a/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller_test.go
+++ b/pkg/controlplane/controller/defaultservicecidr/default_servicecidr_controller_test.go
@@ -56,7 +56,6 @@ func newController(t *testing.T, objects []*networkingapiv1alpha1.ServiceCIDR) (
 		eventRecorder:      record.NewFakeRecorder(100),
 		serviceCIDRLister:  serviceCIDRInformer.Lister(),
 		serviceCIDRsSynced: func() bool { return true },
-		readyCh:            make(chan struct{}),
 	}
 
 	return client, c