Merge pull request #8743 from sbueringer/pr-remove-requeues

🌱 Remove unnecessary requeues

Author: k8s-ci-robot

controllers/external/tracker.go

@@ -17,6 +17,7 @@ limitations under the License.
 package external
 
 import (
+	"fmt"
 	"sync"
 
 	"github.com/go-logr/logr"
@@ -56,15 +57,15 @@ func (o *ObjectTracker) Watch(log logr.Logger, obj runtime.Object, handler handl
 	u := &unstructured.Unstructured{}
 	u.SetGroupVersionKind(gvk)
 
-	log.Info("Adding watcher on external object", "groupVersionKind", gvk.String())
+	log.Info(fmt.Sprintf("Adding watch on external object %q", gvk.String()))
 	err := o.Controller.Watch(
 		source.Kind(o.Cache, u),
 		handler,
 		append(p, predicates.ResourceNotPaused(log))...,
 	)
 	if err != nil {
 		o.m.Delete(key)
-		return errors.Wrapf(err, "failed to add watcher on external object %q", gvk.String())
+		return errors.Wrapf(err, "failed to add watch on external object %q", gvk.String())
 	}
 	return nil
 }

exp/internal/controllers/machinepool_controller.go

@@ -19,7 +19,6 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"sync"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
@@ -31,7 +30,6 @@ import (
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
-	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -70,10 +68,9 @@ type MachinePoolReconciler struct {
 	// WatchFilterValue is the label value used to filter events prior to reconciliation.
 	WatchFilterValue string
 
-	controller       controller.Controller
-	recorder         record.EventRecorder
-	externalWatchers sync.Map
-	cache            cache.Cache
+	controller      controller.Controller
+	recorder        record.EventRecorder
+	externalTracker external.ObjectTracker
 }
 
 func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, options controller.Options) error {
@@ -104,7 +101,10 @@ func (r *MachinePoolReconciler) SetupWithManager(ctx context.Context, mgr ctrl.M
 
 	r.controller = c
 	r.recorder = mgr.GetEventRecorderFor("machinepool-controller")
-	r.cache = mgr.GetCache()
+	r.externalTracker = external.ObjectTracker{
+		Controller: c,
+		Cache:      mgr.GetCache(),
+	}
 	return nil
 }
 

exp/internal/controllers/machinepool_controller_phases.go

@@ -20,17 +20,16 @@ import (
 	"context"
 	"fmt"
 	"reflect"
-	"time"
 
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/klog/v2"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
-	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/controllers/external"
@@ -43,10 +42,6 @@ import (
 	"sigs.k8s.io/cluster-api/util/patch"
 )
 
-var (
-	externalReadyWait = 30 * time.Second
-)
-
 func (r *MachinePoolReconciler) reconcilePhase(mp *expv1.MachinePool) {
 	// Set the phase to "pending" if nil.
 	if mp.Status.Phase == "" {
@@ -118,6 +113,11 @@ func (r *MachinePoolReconciler) reconcileExternal(ctx context.Context, cluster *
 		return external.ReconcileOutput{}, err
 	}
 
+	// Ensure we add a watch to the external object, if there isn't one already.
+	if err := r.externalTracker.Watch(log, obj, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), &expv1.MachinePool{})); err != nil {
+		return external.ReconcileOutput{}, err
+	}
+
 	// if external ref is paused, return error.
 	if annotations.IsPaused(cluster, obj) {
 		log.V(3).Info("External object referenced is paused")
@@ -148,20 +148,6 @@ func (r *MachinePoolReconciler) reconcileExternal(ctx context.Context, cluster *
 		return external.ReconcileOutput{}, err
 	}
 
-	// Add watcher for external object, if there isn't one already.
-	_, loaded := r.externalWatchers.LoadOrStore(obj.GroupVersionKind().String(), struct{}{})
-	if !loaded && r.controller != nil {
-		log.Info("Adding watcher on external object", "groupVersionKind", obj.GroupVersionKind())
-		err := r.controller.Watch(
-			source.Kind(r.cache, obj),
-			handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), &expv1.MachinePool{}),
-		)
-		if err != nil {
-			r.externalWatchers.Delete(obj.GroupVersionKind().String())
-			return external.ReconcileOutput{}, errors.Wrapf(err, "failed to add watcher on external object %q", obj.GroupVersionKind())
-		}
-	}
-
 	// Set failure reason and message, if any.
 	failureReason, failureMessage, err := external.FailuresFrom(obj)
 	if err != nil {
@@ -216,9 +202,9 @@ func (r *MachinePoolReconciler) reconcileBootstrap(ctx context.Context, cluster
 		)
 
 		if !ready {
-			log.V(2).Info("Bootstrap provider is not ready, requeuing")
+			log.Info("Waiting for bootstrap provider to generate data secret and report status.ready", bootstrapConfig.GetKind(), klog.KObj(bootstrapConfig))
 			m.Status.BootstrapReady = ready
-			return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+			return ctrl.Result{}, nil
 		}
 
 		// Get and set the name of the secret containing the bootstrap data.
@@ -289,8 +275,8 @@ func (r *MachinePoolReconciler) reconcileInfrastructure(ctx context.Context, clu
 	)
 
 	if !mp.Status.InfrastructureReady {
-		log.Info("Infrastructure provider is not ready, requeuing")
-		return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+		log.Info("Infrastructure provider is not yet ready", infraConfig.GetKind(), klog.KObj(infraConfig))
+		return ctrl.Result{}, nil
 	}
 
 	var providerIDList []string
@@ -308,8 +294,8 @@ func (r *MachinePoolReconciler) reconcileInfrastructure(ctx context.Context, clu
 	}
 
 	if len(providerIDList) == 0 && mp.Status.Replicas != 0 {
-		log.Info("Retrieved empty Spec.ProviderIDList from infrastructure provider but Status.Replicas is not zero.", "replicas", mp.Status.Replicas)
-		return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+		log.Info("Retrieved empty spec.providerIDList from infrastructure provider but status.replicas is not zero.", "replicas", mp.Status.Replicas)
+		return ctrl.Result{}, nil
 	}
 
 	if !reflect.DeepEqual(mp.Spec.ProviderIDList, providerIDList) {

exp/internal/controllers/machinepool_controller_phases_test.go

@@ -43,10 +43,6 @@ const (
 	wrongNamespace = "wrong-namespace"
 )
 
-func init() {
-	externalReadyWait = 1 * time.Second
-}
-
 func TestReconcileMachinePoolPhases(t *testing.T) {
 	deletionTimestamp := metav1.Now()
 
@@ -569,7 +565,7 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
 				"status": map[string]interface{}{},
 			},
 			expectError:  false,
-			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectResult: ctrl.Result{},
 			expected: func(g *WithT, m *expv1.MachinePool) {
 				g.Expect(m.Status.BootstrapReady).To(BeFalse())
 			},
@@ -727,7 +723,7 @@ func TestReconcileMachinePoolBootstrap(t *testing.T) {
 				},
 			},
 			expectError:  false,
-			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectResult: ctrl.Result{},
 			expected: func(g *WithT, m *expv1.MachinePool) {
 				g.Expect(m.Status.BootstrapReady).To(BeFalse())
 			},

internal/controllers/cluster/cluster_controller_phases.go

@@ -92,6 +92,11 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 		return external.ReconcileOutput{}, err
 	}
 
+	// Ensure we add a watcher to the external object.
+	if err := r.externalTracker.Watch(log, obj, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), &clusterv1.Cluster{})); err != nil {
+		return external.ReconcileOutput{}, err
+	}
+
 	// if external ref is paused, return error.
 	if annotations.IsPaused(cluster, obj) {
 		log.V(3).Info("External object referenced is paused")
@@ -122,11 +127,6 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 		return external.ReconcileOutput{}, err
 	}
 
-	// Ensure we add a watcher to the external object.
-	if err := r.externalTracker.Watch(log, obj, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), &clusterv1.Cluster{})); err != nil {
-		return external.ReconcileOutput{}, err
-	}
-
 	// Set failure reason and message, if any.
 	failureReason, failureMessage, err := external.FailuresFrom(obj)
 	if err != nil {

internal/controllers/machine/machine_controller.go

@@ -128,7 +128,6 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
 	}
 
 	r.controller = c
-
 	r.recorder = mgr.GetEventRecorderFor("machine-controller")
 	r.externalTracker = external.ObjectTracker{
 		Controller: c,

internal/controllers/machine/machine_controller_phases.go

@@ -98,12 +98,17 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 	obj, err := external.Get(ctx, r.Client, ref, m.Namespace)
 	if err != nil {
 		if apierrors.IsNotFound(errors.Cause(err)) {
-			log.Info("could not find external ref, requeuing", ref.Kind, klog.KRef(m.Namespace, ref.Name))
+			log.Info("could not find external ref, requeuing", ref.Kind, klog.KRef(ref.Namespace, ref.Name))
 			return external.ReconcileOutput{RequeueAfter: externalReadyWait}, nil
 		}
 		return external.ReconcileOutput{}, err
 	}
 
+	// Ensure we add a watch to the external object, if there isn't one already.
+	if err := r.externalTracker.Watch(log, obj, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), &clusterv1.Machine{})); err != nil {
+		return external.ReconcileOutput{}, err
+	}
+
 	// if external ref is paused, return error.
 	if annotations.IsPaused(cluster, obj) {
 		log.V(3).Info("External object referenced is paused")
@@ -141,11 +146,6 @@ func (r *Reconciler) reconcileExternal(ctx context.Context, cluster *clusterv1.C
 		return external.ReconcileOutput{}, err
 	}
 
-	// Ensure we add a watcher to the external object.
-	if err := r.externalTracker.Watch(log, obj, handler.EnqueueRequestForOwner(r.Client.Scheme(), r.Client.RESTMapper(), &clusterv1.Machine{})); err != nil {
-		return external.ReconcileOutput{}, err
-	}
-
 	// Set failure reason and message, if any.
 	failureReason, failureMessage, err := external.FailuresFrom(obj)
 	if err != nil {
@@ -217,7 +217,7 @@ func (r *Reconciler) reconcileBootstrap(ctx context.Context, cluster *clusterv1.
 	// If the bootstrap provider is not ready, requeue.
 	if !ready {
 		log.Info("Waiting for bootstrap provider to generate data secret and report status.ready", bootstrapConfig.GetKind(), klog.KObj(bootstrapConfig))
-		return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+		return ctrl.Result{}, nil
 	}
 
 	// Get and set the name of the secret containing the bootstrap data.
@@ -284,7 +284,7 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, cluster *clust
 	// If the infrastructure provider is not ready, return early.
 	if !ready {
 		log.Info("Waiting for infrastructure provider to create machine infrastructure and report status.ready", infraConfig.GetKind(), klog.KObj(infraConfig))
-		return ctrl.Result{RequeueAfter: externalReadyWait}, nil
+		return ctrl.Result{}, nil
 	}
 
 	// Get Spec.ProviderID from the infrastructure provider.

internal/controllers/machine/machine_controller_phases_test.go

@@ -715,7 +715,7 @@ func TestReconcileBootstrap(t *testing.T) {
 				"spec":   map[string]interface{}{},
 				"status": map[string]interface{}{},
 			},
-			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectResult: ctrl.Result{},
 			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.Status.BootstrapReady).To(BeFalse())
@@ -836,7 +836,7 @@ func TestReconcileBootstrap(t *testing.T) {
 					BootstrapReady: true,
 				},
 			},
-			expectResult: ctrl.Result{RequeueAfter: externalReadyWait},
+			expectResult: ctrl.Result{},
 			expectError:  false,
 			expected: func(g *WithT, m *clusterv1.Machine) {
 				g.Expect(m.GetOwnerReferences()).NotTo(ContainRefOfGroupKind("cluster.x-k8s.io", "MachineSet"))