From 841a051febde4d5997f186a6382e0422d8122494 Mon Sep 17 00:00:00 2001 From: Marcel Boehm Date: Tue, 23 Jan 2024 15:18:31 +0100 Subject: [PATCH] Improve Requeue durations of LoadBalancer Reconciler --- .../loadbalancer/loadbalancer_controller.go | 57 ++++++++------- internal/helper/loadbalancerset.go | 28 ++++---- internal/helper/loadbalancerset_test.go | 71 +++++++++++++++++++ 3 files changed, 118 insertions(+), 38 deletions(-) create mode 100644 internal/helper/loadbalancerset_test.go diff --git a/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go b/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go index 0bcf825c..f9ad39a6 100644 --- a/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go +++ b/controllers/yawol-controller/loadbalancer/loadbalancer_controller.go @@ -4,6 +4,9 @@ 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" @@ -11,10 +14,6 @@ import ( 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,16 +916,17 @@ 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 @@ -932,36 +934,36 @@ func (r *Reconciler) reconcileLoadBalancerSet( 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) @@ -969,7 +971,7 @@ func (r *Reconciler) reconcileLoadBalancerSet( 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,7 +981,7 @@ 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 @@ -987,7 +989,7 @@ func (r *Reconciler) reconcileLoadBalancerSet( 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,15 +1002,20 @@ 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") @@ -1016,10 +1023,10 @@ func (r *Reconciler) reconcileLoadBalancerSet( // 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( diff --git a/internal/helper/loadbalancerset.go b/internal/helper/loadbalancerset.go index 206f04e1..a683327f 100644 --- a/internal/helper/loadbalancerset.go +++ b/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. diff --git a/internal/helper/loadbalancerset_test.go b/internal/helper/loadbalancerset_test.go new file mode 100644 index 00000000..7a3c8df7 --- /dev/null +++ b/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), + }} +}