🌱 Reduce reconcilements (#1359)

- Filter HCloudMachine events so that status updates of the resource
  don't trigger another reconcilement of HCloudMachine
- Filter HetznerCluster events so that status updates of the resource
  don't trigger another reconcilement of HetznerCluster
- Filter HetznerCluster events so that some status updates that are not
  important don't trigger reconcilement of HCloudMachine
- Filter Machine events so that status updates of the resource don't
  trigger another reconcilement of HCloudMachine a
- Filter Cluster events so that status updates of the resource don't
  trigger another reconcilement of HetznerCluster
- Increase the time period to requeue if a server is starting
- Increase the time period to requeue if a server has not status yet
- Increase the time period to requeue if the kubeapi server is not yet
  reachable before adding it as target to the load balancer
- Don't trigger multiple shutdown calls for one server and increase the
  timeout to requeue after a shutdown is triggered

controllers/hcloudmachine_controller.go

@@ -220,14 +220,16 @@ func (r *HCloudMachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl
 		WithOptions(options).
 		For(&infrav1.HCloudMachine{}).
 		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(log, r.WatchFilterValue)).
+		WithEventFilter(IgnoreInsignificantHCloudMachineStatusUpdates(log)).
 		Watches(
 			&clusterv1.Machine{},
 			handler.EnqueueRequestsFromMapFunc(util.MachineToInfrastructureMapFunc(infrav1.GroupVersion.WithKind("HCloudMachine"))),
+			builder.WithPredicates(IgnoreInsignificantMachineStatusUpdates(log)),
 		).
 		Watches(
 			&infrav1.HetznerCluster{},
 			handler.EnqueueRequestsFromMapFunc(r.HetznerClusterToHCloudMachines(ctx)),
-			builder.WithPredicates(IgnoreHetznerClusterConditionUpdates(log)),
+			builder.WithPredicates(IgnoreInsignificantHetznerClusterUpdates(log)),
 		).
 		Build(r)
 	if err != nil {
@@ -303,8 +305,8 @@ func (r *HCloudMachineReconciler) HetznerClusterToHCloudMachines(_ context.Conte
 	}
 }
 
-// IgnoreHetznerClusterConditionUpdates is a predicate used for ignoring HetznerCluster condition updates.
-func IgnoreHetznerClusterConditionUpdates(logger logr.Logger) predicate.Funcs {
+// IgnoreInsignificantHetznerClusterUpdates is a predicate used for ignoring insignificant HetznerCluster.Status updates.
+func IgnoreInsignificantHetznerClusterUpdates(logger logr.Logger) predicate.Funcs {
 	return predicate.Funcs{
 		UpdateFunc: func(e event.UpdateEvent) bool {
 			log := logger.WithValues(
@@ -341,6 +343,23 @@ func IgnoreHetznerClusterConditionUpdates(logger logr.Logger) predicate.Funcs {
 			oldCluster.Status.Conditions = nil
 			newCluster.Status.Conditions = nil
 
+			if oldCluster.Status.ControlPlaneLoadBalancer != nil {
+				oldCluster.Status.ControlPlaneLoadBalancer.Target = nil
+			}
+			if newCluster.Status.ControlPlaneLoadBalancer != nil {
+				newCluster.Status.ControlPlaneLoadBalancer.Target = nil
+			}
+
+			oldCluster.Status.HCloudPlacementGroups = nil
+			newCluster.Status.HCloudPlacementGroups = nil
+
+			if oldCluster.Status.Network != nil {
+				oldCluster.Status.Network.AttachedServers = nil
+			}
+			if newCluster.Status.Network != nil {
+				newCluster.Status.Network.AttachedServers = nil
+			}
+
 			if reflect.DeepEqual(oldCluster, newCluster) {
 				// Only insignificant fields changed, no need to reconcile
 				return false
@@ -355,3 +374,109 @@ func IgnoreHetznerClusterConditionUpdates(logger logr.Logger) predicate.Funcs {
 		GenericFunc: func(_ event.GenericEvent) bool { return true },
 	}
 }
+
+// IgnoreInsignificantMachineStatusUpdates is a predicate used for ignoring insignificant Machine.Status updates.
+func IgnoreInsignificantMachineStatusUpdates(logger logr.Logger) predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			log := logger.WithValues(
+				"predicate", "IgnoreInsignificantMachineStatusUpdates",
+				"type", "update",
+				"namespace", e.ObjectNew.GetNamespace(),
+				"kind", strings.ToLower(e.ObjectNew.GetObjectKind().GroupVersionKind().Kind),
+				"name", e.ObjectNew.GetName(),
+			)
+
+			var oldMachine, newMachine *clusterv1.Machine
+			var ok bool
+			// This predicate only looks at Machine objects
+			if oldMachine, ok = e.ObjectOld.(*clusterv1.Machine); !ok {
+				return true
+			}
+			if newMachine, ok = e.ObjectNew.(*clusterv1.Machine); !ok {
+				// Something weird happened, and we received two different kinds of objects
+				return true
+			}
+
+			// We should not modify the original objects, this causes issues with code that relies on the original object.
+			oldMachine = oldMachine.DeepCopy()
+			newMachine = newMachine.DeepCopy()
+
+			oldMachine.ManagedFields = nil
+			newMachine.ManagedFields = nil
+
+			oldMachine.ResourceVersion = ""
+			newMachine.ResourceVersion = ""
+
+			oldMachine.Status = clusterv1.MachineStatus{}
+			newMachine.Status = clusterv1.MachineStatus{}
+
+			if reflect.DeepEqual(oldMachine, newMachine) {
+				// Only insignificant fields changed, no need to reconcile
+				return false
+			}
+
+			log.V(1).Info("Machine -> HCloudMachine event")
+			return true
+		},
+		CreateFunc:  func(_ event.CreateEvent) bool { return true },
+		DeleteFunc:  func(_ event.DeleteEvent) bool { return true },
+		GenericFunc: func(_ event.GenericEvent) bool { return true },
+	}
+}
+
+// IgnoreInsignificantHCloudMachineStatusUpdates is a predicate used for ignoring insignificant HCloudMachine.Status updates.
+func IgnoreInsignificantHCloudMachineStatusUpdates(logger logr.Logger) predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			log := logger.WithValues(
+				"predicate", "IgnoreInsignificantHCloudMachineStatusUpdates",
+				"type", "update",
+				"namespace", e.ObjectNew.GetNamespace(),
+				"kind", strings.ToLower(e.ObjectNew.GetObjectKind().GroupVersionKind().Kind),
+				"name", e.ObjectNew.GetName(),
+			)
+
+			var oldHCloudMachine, newHCloudMachine *infrav1.HCloudMachine
+			var ok bool
+			// This predicate only looks at HCloudMachine objects
+			if oldHCloudMachine, ok = e.ObjectOld.(*infrav1.HCloudMachine); !ok {
+				return true
+			}
+			if newHCloudMachine, ok = e.ObjectNew.(*infrav1.HCloudMachine); !ok {
+				// Something weird happened, and we received two different kinds of objects
+				return true
+			}
+
+			// We should not modify the original objects, this causes issues with code that relies on the original object.
+			oldHCloudMachine = oldHCloudMachine.DeepCopy()
+			newHCloudMachine = newHCloudMachine.DeepCopy()
+
+			// check if status is empty - if so, it should be restored
+			emptyStatus := infrav1.HCloudMachineStatus{}
+			if reflect.DeepEqual(newHCloudMachine.Status, emptyStatus) {
+				return true
+			}
+
+			oldHCloudMachine.ManagedFields = nil
+			newHCloudMachine.ManagedFields = nil
+
+			oldHCloudMachine.ResourceVersion = ""
+			newHCloudMachine.ResourceVersion = ""
+
+			oldHCloudMachine.Status = infrav1.HCloudMachineStatus{}
+			newHCloudMachine.Status = infrav1.HCloudMachineStatus{}
+
+			if reflect.DeepEqual(oldHCloudMachine, newHCloudMachine) {
+				// Only insignificant fields changed, no need to reconcile
+				return false
+			}
+
+			log.V(1).Info("HCloudMachine event")
+			return true
+		},
+		CreateFunc:  func(_ event.CreateEvent) bool { return true },
+		DeleteFunc:  func(_ event.DeleteEvent) bool { return true },
+		GenericFunc: func(_ event.GenericEvent) bool { return true },
+	}
+}

controllers/hcloudmachine_controller_test.go

@@ -627,7 +627,7 @@ var _ = Describe("HCloudMachine validation", func() {
 	})
 })
 
-var _ = Describe("IgnoreHetznerClusterConditionUpdates Predicate", func() {
+var _ = Describe("IgnoreInsignificantHetznerClusterUpdates Predicate", func() {
 	var (
 		predicate predicate.Predicate
 
@@ -636,7 +636,7 @@ var _ = Describe("IgnoreHetznerClusterConditionUpdates Predicate", func() {
 	)
 
 	BeforeEach(func() {
-		predicate = IgnoreHetznerClusterConditionUpdates(klog.Background())
+		predicate = IgnoreInsignificantHetznerClusterUpdates(klog.Background())
 
 		oldCluster = &infrav1.HetznerCluster{
 			ObjectMeta: metav1.ObjectMeta{Name: "test-predicate", ResourceVersion: "1"},

controllers/hetznercluster_controller.go

@@ -21,11 +21,13 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"reflect"
 	"strconv"
 	"strings"
 	"sync"
 	"time"
 
+	"github.com/go-logr/logr"
 	certificatesv1 "k8s.io/api/certificates/v1"
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -46,9 +48,11 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
+	"sigs.k8s.io/controller-runtime/pkg/predicate"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
@@ -732,6 +736,7 @@ func (r *HetznerClusterReconciler) SetupWithManager(ctx context.Context, mgr ctr
 		For(&infrav1.HetznerCluster{}).
 		WithEventFilter(predicates.ResourceNotPausedAndHasFilterLabel(log, r.WatchFilterValue)).
 		WithEventFilter(predicates.ResourceIsNotExternallyManaged(log)).
+		WithEventFilter(IgnoreInsignificantHetznerClusterStatusUpdates(log)).
 		Owns(&corev1.Secret{}).
 		Build(r)
 	if err != nil {
@@ -777,5 +782,116 @@ func (r *HetznerClusterReconciler) SetupWithManager(ctx context.Context, mgr ctr
 				},
 			}
 		}),
+		IgnoreInsignificantClusterStatusUpdates(log),
 	)
 }
+
+// IgnoreInsignificantClusterStatusUpdates is a predicate used for ignoring insignificant HetznerCluster.Status updates.
+func IgnoreInsignificantClusterStatusUpdates(logger logr.Logger) predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			log := logger.WithValues(
+				"predicate", "IgnoreInsignificantClusterStatusUpdates",
+				"type", "update",
+				"namespace", e.ObjectNew.GetNamespace(),
+				"kind", strings.ToLower(e.ObjectNew.GetObjectKind().GroupVersionKind().Kind),
+				"name", e.ObjectNew.GetName(),
+			)
+
+			var oldCluster, newCluster *clusterv1.Cluster
+			var ok bool
+			// This predicate only looks at Cluster objects
+			if oldCluster, ok = e.ObjectOld.(*clusterv1.Cluster); !ok {
+				return true
+			}
+			if newCluster, ok = e.ObjectNew.(*clusterv1.Cluster); !ok {
+				// Something weird happened, and we received two different kinds of objects
+				return true
+			}
+
+			// We should not modify the original objects, this causes issues with code that relies on the original object.
+			oldCluster = oldCluster.DeepCopy()
+			newCluster = newCluster.DeepCopy()
+
+			// Set fields we do not care about to nil
+
+			oldCluster.ManagedFields = nil
+			newCluster.ManagedFields = nil
+
+			oldCluster.ResourceVersion = ""
+			newCluster.ResourceVersion = ""
+
+			oldCluster.Status = clusterv1.ClusterStatus{}
+			newCluster.Status = clusterv1.ClusterStatus{}
+
+			if reflect.DeepEqual(oldCluster, newCluster) {
+				// Only insignificant fields changed, no need to reconcile
+				return false
+			}
+			// There is a noteworthy diff, so we should reconcile
+			log.V(1).Info("Cluster -> HetznerCluster")
+			return true
+		},
+		CreateFunc:  func(_ event.CreateEvent) bool { return true },
+		DeleteFunc:  func(_ event.DeleteEvent) bool { return true },
+		GenericFunc: func(_ event.GenericEvent) bool { return true },
+	}
+}
+
+// IgnoreInsignificantHetznerClusterStatusUpdates is a predicate used for ignoring insignificant HetznerHetznerCluster.Status updates.
+func IgnoreInsignificantHetznerClusterStatusUpdates(logger logr.Logger) predicate.Funcs {
+	return predicate.Funcs{
+		UpdateFunc: func(e event.UpdateEvent) bool {
+			log := logger.WithValues(
+				"predicate", "IgnoreInsignificantHetznerClusterStatusUpdates",
+				"namespace", e.ObjectNew.GetNamespace(),
+				"kind", strings.ToLower(e.ObjectNew.GetObjectKind().GroupVersionKind().Kind),
+				"name", e.ObjectNew.GetName(),
+			)
+
+			var oldHetznerCluster, newHetznerCluster *infrav1.HetznerCluster
+			var ok bool
+			// This predicate only looks at HetznerCluster objects
+			if oldHetznerCluster, ok = e.ObjectOld.(*infrav1.HetznerCluster); !ok {
+				return true
+			}
+			if newHetznerCluster, ok = e.ObjectNew.(*infrav1.HetznerCluster); !ok {
+				// Something weird happened, and we received two different kinds of objects
+				return true
+			}
+
+			// We should not modify the original objects, this causes issues with code that relies on the original object.
+			oldHetznerCluster = oldHetznerCluster.DeepCopy()
+			newHetznerCluster = newHetznerCluster.DeepCopy()
+
+			// check if status is empty - if so, it should be restored
+			emptyStatus := infrav1.HetznerClusterStatus{}
+			if reflect.DeepEqual(newHetznerCluster.Status, emptyStatus) {
+				return true
+			}
+
+			// Set fields we do not care about to nil
+
+			oldHetznerCluster.ManagedFields = nil
+			newHetznerCluster.ManagedFields = nil
+
+			oldHetznerCluster.ResourceVersion = ""
+			newHetznerCluster.ResourceVersion = ""
+
+			oldHetznerCluster.Status = infrav1.HetznerClusterStatus{}
+			newHetznerCluster.Status = infrav1.HetznerClusterStatus{}
+
+			if reflect.DeepEqual(oldHetznerCluster, newHetznerCluster) {
+				// Only insignificant fields changed, no need to reconcile
+				return false
+			}
+			// There is a noteworthy diff, so we should reconcile
+			log.V(1).Info("HetznerCluster Update")
+			return true
+		},
+		// We only care about Update events, anything else should be reconciled
+		CreateFunc:  func(_ event.CreateEvent) bool { return true },
+		DeleteFunc:  func(_ event.DeleteEvent) bool { return true },
+		GenericFunc: func(_ event.GenericEvent) bool { return true },
+	}
+}