Improve Requeue durations of LoadBalancer Reconciler

controllers/yawol-controller/loadbalancer/loadbalancer_controller.go

@@ -4,17 +4,16 @@ import (
 	"context"
 	"time"
 
+	"github.com/stackitcloud/yawol/internal/helper"
+	"github.com/stackitcloud/yawol/internal/helper/kubernetes"
+	"github.com/stackitcloud/yawol/internal/openstack"
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/utils/ptr"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
 
-	"github.com/stackitcloud/yawol/internal/helper"
-	"github.com/stackitcloud/yawol/internal/helper/kubernetes"
-	"github.com/stackitcloud/yawol/internal/openstack"
-
 	"github.com/go-logr/logr"
 	"github.com/gophercloud/gophercloud"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/servergroups"
@@ -108,14 +107,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, err
 	}
 
+	emptyResult := ctrl.Result{}
+
 	// run openstack reconcile if needed
-	if res, err = r.reconcileOpenStackIfNeeded(ctx, &lb, req, osClient); err != nil || res.Requeue || res.RequeueAfter != 0 {
+	if res, err = r.reconcileOpenStackIfNeeded(ctx, &lb, req, osClient); err != nil || res != emptyResult {
 		return res, err
 	}
 
 	// lbs reconcile is not affected by lastOpenstackReconcile
-	if err := r.reconcileLoadBalancerSet(ctx, log, &lb); err != nil {
-		return ctrl.Result{}, err
+	if res, err := r.reconcileLoadBalancerSet(ctx, log, &lb); err != nil || res != emptyResult {
+		return res, err
 	}
 
 	return ctrl.Result{RequeueAfter: 5 * time.Minute}, nil
@@ -915,61 +916,62 @@ func (r *Reconciler) reconcileLoadBalancerSet(
 	ctx context.Context,
 	log logr.Logger,
 	lb *yawolv1beta1.LoadBalancer,
-) error {
+) (ctrl.Result, error) {
+	emptyResult := ctrl.Result{}
 	// Make sure LoadBalancer has revision number
 	currentRevision, err := helper.ReadCurrentRevisionFromLB(lb)
 	if err != nil {
-		return err
+		return emptyResult, err
 	}
 	log = log.WithValues("revision", currentRevision)
 
 	if currentRevision == 0 {
-		return helper.PatchLoadBalancerRevision(ctx, r.Client, lb, 1)
+		return emptyResult, helper.PatchLoadBalancerRevision(ctx, r.Client, lb, 1)
 	}
 
 	// Get the labels LBS will receive on creation from this lb
 	lbsLabels := helper.GetLoadBalancerSetLabelsFromLoadBalancer(lb)
 
 	loadBalancerSetList := &yawolv1beta1.LoadBalancerSetList{}
 	if err := r.Client.List(ctx, loadBalancerSetList, client.MatchingLabels(lbsLabels)); err != nil {
-		return err
+		return emptyResult, err
 	}
 
 	// Get Hash for current LoadBalancerMachineSpec
 	var hash string
 	hash, err = helper.GetHashForLoadBalancerMachineSet(lb)
 	if err != nil {
-		return err
+		return emptyResult, err
 	}
 	log = log.WithValues("hash", hash)
 
 	// Get LoadBalancerSet for current hash
 	var currentSet *yawolv1beta1.LoadBalancerSet
 	if currentSet, err = helper.GetLoadBalancerSetForHash(loadBalancerSetList, hash); err != nil {
-		return err
+		return emptyResult, err
 	}
 
 	// create new LBS if hash changed or LB was newly created
 	if currentSet == nil {
 		nextRevision, err := helper.GetNextRevisionForLoadBalancer(loadBalancerSetList)
 		if err != nil {
-			return err
+			return emptyResult, err
 		}
 
 		if lb.Status.PortID == nil {
-			return helper.ErrLBPortNotSet
+			return emptyResult, helper.ErrLBPortNotSet
 		}
 
 		if err := helper.PatchLoadBalancerRevision(ctx, r.Client, lb, nextRevision); err != nil {
-			return err
+			return emptyResult, err
 		}
 
 		currentSet = helper.NewLoadBalancerSetForLoadBalancer(lb, hash, nextRevision)
 		log = log.WithValues("loadBalancerSet", currentSet.Name)
 		log.Info("Creating new LoadBalancerSet")
 
 		if err := r.Client.Create(ctx, currentSet); err != nil {
-			return err
+			return emptyResult, err
 		}
 	}
 	log = log.WithValues("loadBalancerSet", currentSet.Name)
@@ -979,15 +981,15 @@ func (r *Reconciler) reconcileLoadBalancerSet(
 	statusReplicas, statusReadyReplicas := helper.StatusReplicasFromSetList(loadBalancerSetList)
 	lb.Status.Replicas, lb.Status.ReadyReplicas = &statusReplicas, &statusReadyReplicas
 	if err := r.Client.Status().Patch(ctx, lb, patch); err != nil {
-		return err
+		return emptyResult, err
 	}
 
 	// update LoadBalancerSet replicas if required
 	desiredReplicas := lb.Spec.Replicas
 	if currentSet.Spec.Replicas != desiredReplicas {
 		log.Info("Setting replicas of current LoadBalancerSet", "replicas", desiredReplicas)
 		if err := helper.PatchLoadBalancerSetReplicas(ctx, r.Client, currentSet, desiredReplicas); err != nil {
-			return err
+			return emptyResult, err
 		}
 	}
 
@@ -1000,26 +1002,31 @@ func (r *Reconciler) reconcileLoadBalancerSet(
 	// check if all replicas from current LBS are ready and requeue if not ready
 	if currentReplicas != desiredReplicas || currentReadyReplicas != desiredReplicas {
 		log.V(1).Info("Current LoadBalancerSet is not ready yet")
-		return nil
+		return emptyResult, nil
 	}
 
 	log.Info("Current LoadBalancerSet is ready")
 
 	// check if in current loadbalancerset is a keepalived master
-	if !helper.LBSetHasKeepalivedMaster(currentSet) {
-		log.V(1).Info("Current LoadBalancerSet has no keepalived master yet")
-		return nil
+	if ok, requeueAfter := helper.LBSetHasKeepalivedMaster(currentSet); !ok {
+		// the helper only returns true if the LBSet has the master for some
+		// time, so we want to requeue more frequently to catch that.
+		if requeueAfter > 5*time.Minute {
+			requeueAfter = 5 * time.Minute
+		}
+		log.V(1).Info("Current LoadBalancerSet has no keepalived master yet", "forceCheckAfter", requeueAfter)
+		return ctrl.Result{RequeueAfter: requeueAfter, Requeue: true}, nil
 	}
 
 	log.Info("Current LoadBalancerSet has keepalived master")
 
 	// scale down all other existing LoadBalancerSets for the LoadBalancer after upscale to ensure no downtime
 	log.Info("Scale down old LoadBalancerSets")
 	if err := helper.ScaleDownOldLoadBalancerSets(ctx, r.Client, loadBalancerSetList, currentSet.Name); err != nil {
-		return nil
+		return emptyResult, nil
 	}
 
-	return nil
+	return emptyResult, nil
 }
 
 func (r *Reconciler) deletionRoutine(

internal/helper/loadbalancerset.go

@@ -7,7 +7,7 @@ import (
 
 	yawolv1beta1 "github.com/stackitcloud/yawol/api/v1beta1"
 	helpermetrics "github.com/stackitcloud/yawol/internal/metrics"
-	metaV1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/utils/ptr"
 
 	"k8s.io/apimachinery/pkg/labels"
@@ -20,10 +20,10 @@ func NewLoadBalancerSetForLoadBalancer(lb *yawolv1beta1.LoadBalancer, hash strin
 	setLabels[HashLabel] = hash
 
 	return &yawolv1beta1.LoadBalancerSet{
-		ObjectMeta: metaV1.ObjectMeta{
+		ObjectMeta: metav1.ObjectMeta{
 			Name:      lb.Name + "-" + hash,
 			Namespace: lb.Namespace,
-			OwnerReferences: []metaV1.OwnerReference{
+			OwnerReferences: []metav1.OwnerReference{
 				GetOwnersReferenceForLB(lb),
 			},
 			Labels: setLabels,
@@ -32,7 +32,7 @@ func NewLoadBalancerSetForLoadBalancer(lb *yawolv1beta1.LoadBalancer, hash strin
 			},
 		},
 		Spec: yawolv1beta1.LoadBalancerSetSpec{
-			Selector: metaV1.LabelSelector{
+			Selector: metav1.LabelSelector{
 				MatchLabels: setLabels,
 			},
 			Template: yawolv1beta1.LoadBalancerMachineTemplateSpec{
@@ -76,25 +76,27 @@ func ScaleDownOldLoadBalancerSets(
 	return nil
 }
 
-// LBSetHasKeepalivedMaster returns true one of the following conditions are met:
+// LBSetHasKeepalivedMaster returns true if one of the following conditions are met:
 // - if the keepalived condition on set is ready for more than 2 min
 // - keepalived condition is not ready for more than 10 min (to make sure this does not block updates)
 // - no keepalived condition is in lbs but lbs is older than 15 min (to make sure this does not block updates)
-func LBSetHasKeepalivedMaster(set *yawolv1beta1.LoadBalancerSet) bool {
-	before2Minutes := metaV1.Time{Time: time.Now().Add(-2 * time.Minute)}
-	before10Minutes := metaV1.Time{Time: time.Now().Add(-10 * time.Minute)}
-	before15Minutes := metaV1.Time{Time: time.Now().Add(-15 * time.Minute)}
+// When `false` is returned, the second return parameter can be used to requeue again,
+// once the condition that caused the "false" state to be ignored.
+func LBSetHasKeepalivedMaster(set *yawolv1beta1.LoadBalancerSet) (bool, time.Duration) {
+	now := time.Now()
 
+	considerTrueAfter := set.CreationTimestamp.Add(15 * time.Minute)
 	for _, condition := range set.Status.Conditions {
 		if condition.Type != HasKeepalivedMaster {
 			continue
 		}
-		if condition.Status == metaV1.ConditionTrue {
-			return condition.LastTransitionTime.Before(&before2Minutes)
+		if condition.Status == metav1.ConditionTrue {
+			considerTrueAfter = condition.LastTransitionTime.Add(2 * time.Minute)
+			break
 		}
-		return condition.LastTransitionTime.Before(&before10Minutes)
+		considerTrueAfter = condition.LastTransitionTime.Add(10 * time.Minute)
 	}
-	return set.CreationTimestamp.Before(&before15Minutes)
+	return now.After(considerTrueAfter), considerTrueAfter.Sub(now)
 }
 
 // StatusReplicasFromSetList returns the total replicas and ready replicas based on the given list of LoadBalancerSets.

internal/helper/loadbalancerset_test.go

@@ -0,0 +1,71 @@
+package helper
+
+import (
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	yawolv1beta1 "github.com/stackitcloud/yawol/api/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+var _ = Describe("LBSetHasKeepalivedMaster", func() {
+	It("should return true if everything is ok", func() {
+		set := &yawolv1beta1.LoadBalancerSet{}
+		setKeepalivedCond(set, metav1.ConditionTrue, time.Now().Add(-3*time.Minute))
+
+		ok, _ := LBSetHasKeepalivedMaster(set)
+		Expect(ok).To(BeTrue())
+	})
+
+	It("should return false if the condition is true for < 2 Minutes", func() {
+		set := &yawolv1beta1.LoadBalancerSet{}
+		setKeepalivedCond(set, metav1.ConditionTrue, time.Now().Add(-1*time.Minute))
+
+		ok, req := LBSetHasKeepalivedMaster(set)
+		Expect(ok).To(BeFalse())
+		Expect(req).To(BeNumerically("~", 1*time.Minute, 1*time.Second))
+	})
+
+	It("should return false if the condition is false for < 10 Minutes", func() {
+		set := &yawolv1beta1.LoadBalancerSet{}
+		setKeepalivedCond(set, metav1.ConditionFalse, time.Now().Add(-1*time.Minute))
+
+		ok, req := LBSetHasKeepalivedMaster(set)
+		Expect(ok).To(BeFalse())
+		Expect(req).To(BeNumerically("~", 9*time.Minute, 1*time.Second))
+	})
+	It("should return true if the condition is false for > 10 Minutes", func() {
+		set := &yawolv1beta1.LoadBalancerSet{}
+		setKeepalivedCond(set, metav1.ConditionFalse, time.Now().Add(-11*time.Minute))
+
+		ok, _ := LBSetHasKeepalivedMaster(set)
+		Expect(ok).To(BeTrue())
+	})
+
+	It("should return false if the condition is absent for < 15 Minutes", func() {
+		set := &yawolv1beta1.LoadBalancerSet{ObjectMeta: metav1.ObjectMeta{
+			CreationTimestamp: metav1.NewTime(time.Now().Add(-1 * time.Minute)),
+		}}
+
+		ok, req := LBSetHasKeepalivedMaster(set)
+		Expect(ok).To(BeFalse())
+		Expect(req).To(BeNumerically("~", 14*time.Minute, 1*time.Second))
+	})
+	It("should return true if the condition is absent for > 15 Minutes", func() {
+		set := &yawolv1beta1.LoadBalancerSet{ObjectMeta: metav1.ObjectMeta{
+			CreationTimestamp: metav1.NewTime(time.Now().Add(-16 * time.Minute)),
+		}}
+
+		ok, _ := LBSetHasKeepalivedMaster(set)
+		Expect(ok).To(BeTrue())
+	})
+})
+
+func setKeepalivedCond(set *yawolv1beta1.LoadBalancerSet, status metav1.ConditionStatus, ltt time.Time) {
+	set.Status.Conditions = []metav1.Condition{{
+		Type:               HasKeepalivedMaster,
+		Status:             status,
+		LastTransitionTime: metav1.NewTime(ltt),
+	}}
+}