Infoblox, heavy load fixed

controllers/gslb_controller.go

@@ -19,8 +19,8 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"time"
 
+	"github.com/AbsaOSS/k8gb/controllers/internal/utils"
 	"github.com/AbsaOSS/k8gb/controllers/providers/dns"
 
 	"github.com/AbsaOSS/k8gb/controllers/providers/metrics"
@@ -72,7 +72,7 @@ const (
 func (r *GslbReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	ctx := context.Background()
 	log := r.Log.WithValues("gslb", req.NamespacedName)
-
+	result := utils.NewReconcileResultHandler(r.Config.ReconcileRequeueSeconds, log)
 	// Fetch the Gslb instance
 	gslb := &k8gbv1beta1.Gslb{}
 	err := r.Get(ctx, req.NamespacedName, gslb)
@@ -81,18 +81,14 @@ func (r *GslbReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 			// Request object not found, could have been deleted after reconcile request.
 			// Owned objects are automatically garbage collected. For additional cleanup logic use finalizers.
 			// Return and don't requeue
-			return ctrl.Result{}, nil
+			return result.Stop()
 		}
-		// Error reading the object - requeue the request.
-		return ctrl.Result{}, err
+		return result.RequeueError(fmt.Errorf("error reading the object (%s)", err))
 	}
 
-	var result *ctrl.Result
-
 	err = r.DepResolver.ResolveGslbSpec(ctx, gslb)
 	if err != nil {
-		log.Error(err, "resolving spec.strategy")
-		return ctrl.Result{}, err
+		return result.RequeueError(fmt.Errorf("resolving spec (%s)", err))
 	}
 	// == Finalizer business ==
 
@@ -105,70 +101,66 @@ func (r *GslbReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 			// finalization logic fails, don't remove the finalizer so
 			// that we can retry during the next reconciliation.
 			if err := r.finalizeGslb(gslb); err != nil {
-				return ctrl.Result{}, err
+				return result.RequeueError(err)
 			}
 
 			// Remove gslbFinalizer. Once all finalizers have been
 			// removed, the object will be deleted.
 			gslb.SetFinalizers(remove(gslb.GetFinalizers(), gslbFinalizer))
 			err := r.Update(ctx, gslb)
 			if err != nil {
-				return ctrl.Result{}, err
+				return result.RequeueError(err)
 			}
 		}
-		return ctrl.Result{}, nil
+		return result.Stop()
 	}
 
 	// Add finalizer for this CR
 	if !contains(gslb.GetFinalizers(), gslbFinalizer) {
 		if err := r.addFinalizer(gslb); err != nil {
-			return ctrl.Result{}, err
+			return result.RequeueError(err)
 		}
 	}
 
 	// == Ingress ==========
 	ingress, err := r.gslbIngress(gslb)
 	if err != nil {
-		// Requeue the request
-		return ctrl.Result{}, err
+		return result.RequeueError(err)
 	}
 
-	result, err = r.ensureIngress(gslb, ingress)
-	if result != nil {
-		return *result, err
+	err = r.saveIngress(gslb, ingress)
+	if err != nil {
+		return result.RequeueError(err)
 	}
 
 	// == external-dns dnsendpoints CRs ==
 	dnsEndpoint, err := r.gslbDNSEndpoint(gslb)
 	if err != nil {
-		// Requeue the request
-		return ctrl.Result{}, err
+		return result.RequeueError(err)
 	}
 
-	result, err = r.DNSProvider.SaveDNSEndpoint(gslb, dnsEndpoint)
-	if result != nil {
-		return *result, err
+	err = r.DNSProvider.SaveDNSEndpoint(gslb, dnsEndpoint)
+	if err != nil {
+		return result.RequeueError(err)
 	}
 
 	// == handle delegated zone in Edge DNS
-	result, err = r.DNSProvider.CreateZoneDelegationForExternalDNS(gslb)
-	if result != nil {
-		return *result, err
+	err = r.DNSProvider.CreateZoneDelegationForExternalDNS(gslb)
+	if err != nil {
+		return result.RequeueError(err)
 	}
 
 	// == Status =
 	err = r.updateGslbStatus(gslb)
 	if err != nil {
-		// Requeue the request
-		return ctrl.Result{}, err
+		return result.RequeueError(err)
 	}
 
 	// == Finish ==========
 	// Everything went fine, requeue after some time to catch up
 	// with external Gslb status
 	// TODO: potentially enhance with smarter reaction to external Event
-
-	return ctrl.Result{RequeueAfter: time.Second * time.Duration(r.Config.ReconcileRequeueSeconds)}, nil
+	return result.Requeue()
 }
 
 // SetupWithManager configures controller manager

controllers/ingress.go

@@ -2,14 +2,16 @@ package controllers
 
 import (
 	"context"
+	"reflect"
+
+	"github.com/AbsaOSS/k8gb/controllers/internal/utils"
 
 	k8gbv1beta1 "github.com/AbsaOSS/k8gb/api/v1beta1"
 	v1beta1 "k8s.io/api/extensions/v1beta1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 func (r *GslbReconciler) gslbIngress(gslb *k8gbv1beta1.Gslb) (*v1beta1.Ingress, error) {
@@ -33,7 +35,7 @@ func (r *GslbReconciler) gslbIngress(gslb *k8gbv1beta1.Gslb) (*v1beta1.Ingress,
 	return ingress, err
 }
 
-func (r *GslbReconciler) ensureIngress(instance *k8gbv1beta1.Gslb, i *v1beta1.Ingress) (*reconcile.Result, error) {
+func (r *GslbReconciler) saveIngress(instance *k8gbv1beta1.Gslb, i *v1beta1.Ingress) error {
 	found := &v1beta1.Ingress{}
 	err := r.Get(context.TODO(), types.NamespacedName{
 		Name:      instance.Name,
@@ -48,26 +50,41 @@ func (r *GslbReconciler) ensureIngress(instance *k8gbv1beta1.Gslb, i *v1beta1.In
 		if err != nil {
 			// Creation failed
 			log.Error(err, "Failed to create new Ingress", "Ingress.Namespace", i.Namespace, "Ingress.Name", i.Name)
-			return &reconcile.Result{}, err
+			return err
 		}
 		// Creation was successful
-		return nil, nil
+		return nil
 	} else if err != nil {
 		// Error that isn't due to the service not existing
 		log.Error(err, "Failed to get Ingress")
-		return &reconcile.Result{}, err
+		return err
 	}
 
 	// Update existing object with new spec and annotations
-	found.Spec = i.Spec
-	found.Annotations = i.Annotations
-	err = r.Update(context.TODO(), found)
-
-	if err != nil {
-		// Update failed
-		log.Error(err, "Failed to update Ingress", "Ingress.Namespace", found.Namespace, "Ingress.Name", found.Name)
-		return &reconcile.Result{}, err
+	if !ingressEqual(found, i) {
+		found.Spec = i.Spec
+		found.Annotations = utils.MergeAnnotations(found.Annotations, i.Annotations)
+		err = r.Update(context.TODO(), found)
+		if errors.IsConflict(err) {
+			r.Log.Info("Ingress has been modified outside of controller, retrying reconciliation",
+				"Ingress.Namespace", found.Namespace, "Ingress.Name", found.Name)
+			return nil
+		}
+		if err != nil {
+			// Update failed
+			log.Error(err, "Failed to update Ingress", "Ingress.Namespace", found.Namespace, "Ingress.Name", found.Name)
+			return err
+		}
 	}
 
-	return nil, nil
+	return nil
+}
+
+func ingressEqual(ing1 *v1beta1.Ingress, ing2 *v1beta1.Ingress) bool {
+	for k, v := range ing2.Annotations {
+		if ing1.Annotations[k] != v {
+			return false
+		}
+	}
+	return reflect.DeepEqual(ing1.Spec, ing2.Spec)
 }

controllers/internal/utils/annotations.go

@@ -0,0 +1,17 @@
+package utils
+
+// MergeAnnotations adds or updates annotations from source to target and returns merge
+func MergeAnnotations(target map[string]string, source map[string]string) map[string]string {
+	if target == nil {
+		target = make(map[string]string)
+	}
+	if source == nil {
+		source = make(map[string]string)
+	}
+	for k, v := range source {
+		if target[k] != v {
+			target[k] = v
+		}
+	}
+	return target
+}

controllers/internal/utils/annotations_test.go

@@ -0,0 +1,73 @@
+package utils
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+var a2 = map[string]string{"k8gb.io/primary-geotag": "eu", "k8gb.io/strategy": "failover"}
+var a1 = map[string]string{"field.cattle.io/publicEndpoints": "dummy"}
+
+func TestAddNewAnnotations(t *testing.T) {
+	// arrange
+	// act
+	repaired := MergeAnnotations(a1, a2)
+	// assert
+	assert.Equal(t, 3, len(repaired))
+	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
+	assert.Equal(t, "dummy", repaired["field.cattle.io/publicEndpoints"])
+}
+
+func TestAddExistingAnnotations(t *testing.T) {
+	// arrange
+	for k, v := range a2 {
+		a1[k] = v
+	}
+	// act
+	repaired := MergeAnnotations(a1, a2)
+	// assert
+	assert.Equal(t, 3, len(repaired))
+	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
+	assert.Equal(t, "dummy", repaired["field.cattle.io/publicEndpoints"])
+	assert.Equal(t, "failover", repaired["k8gb.io/strategy"])
+}
+
+func TestUpdateExistingRecords(t *testing.T) {
+	// arrange
+	for k, v := range a2 {
+		a1[k] = v
+	}
+	a1["k8gb.io/primary-geotag"] = "us"
+	// act
+	repaired := MergeAnnotations(a1, a2)
+	// assert
+	assert.Equal(t, 3, len(repaired))
+	assert.Equal(t, "eu", repaired["k8gb.io/primary-geotag"])
+	assert.Equal(t, "dummy", repaired["field.cattle.io/publicEndpoints"])
+	assert.Equal(t, "failover", repaired["k8gb.io/strategy"])
+}
+
+func TestEqualAnnotationsWithNilA1(t *testing.T) {
+	// arrange
+	// act
+	repaired := MergeAnnotations(nil, a2)
+	// assert
+	assert.True(t, assert.ObjectsAreEqual(a2, repaired))
+}
+
+func TestEqualAnnotationsWithNilA2(t *testing.T) {
+	// arrange
+	// act
+	repaired := MergeAnnotations(a1, nil)
+	// assert
+	assert.True(t, assert.ObjectsAreEqual(a1, repaired))
+}
+
+func TestEqualAnnotationsWithNilInput(t *testing.T) {
+	// arrange
+	// act
+	repaired := MergeAnnotations(nil, nil)
+	// assert
+	assert.Equal(t, 0, len(repaired))
+}

controllers/internal/utils/reconciler_result.go

@@ -0,0 +1,45 @@
+package utils
+
+import (
+	"time"
+
+	"github.com/go-logr/logr"
+	ctrl "sigs.k8s.io/controller-runtime"
+)
+
+type ReconcileResultHandler struct {
+	log           logr.Logger
+	delayedResult ctrl.Result
+}
+
+func NewReconcileResultHandler(reconcileAfter int, log logr.Logger) *ReconcileResultHandler {
+	return &ReconcileResultHandler{
+		delayedResult: ctrl.Result{RequeueAfter: time.Second * time.Duration(reconcileAfter)},
+		log:           log,
+	}
+}
+
+// Stop stops reconciliation loop
+func (r *ReconcileResultHandler) Stop() (ctrl.Result, error) {
+	r.log.Info("reconciler exit")
+	return ctrl.Result{}, nil
+}
+
+// RequeueError requeue loop immediately
+// see default controller limiter: https://danielmangum.com/posts/controller-runtime-client-go-rate-limiting/
+func (r *ReconcileResultHandler) RequeueError(err error) (ctrl.Result, error) {
+	// logging error is handled in caller function
+	return ctrl.Result{}, err
+}
+
+// Requeue requeue loop after config.ReconcileRequeueSeconds
+// this apply in case you didn't modify request resources.
+// If so, reconciliation starts immediately
+// see: https://github.com/operator-framework/operator-sdk/issues/1164
+func (r *ReconcileResultHandler) Requeue() (ctrl.Result, error) {
+	return r.delayedResult, nil
+}
+
+func (r *ReconcileResultHandler) RequeueNow() (ctrl.Result, error) {
+	return ctrl.Result{Requeue: true}, nil
+}

controllers/providers/assistant/gslb.go

@@ -10,7 +10,6 @@ import (
 	"github.com/miekg/dns"
 
 	k8gbv1beta1 "github.com/AbsaOSS/k8gb/api/v1beta1"
-	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	externaldns "sigs.k8s.io/external-dns/endpoint"
 
 	"github.com/AbsaOSS/k8gb/controllers/internal/utils"
@@ -108,7 +107,7 @@ func (r *GslbLoggerAssistant) GslbIngressExposedIPs(gslb *k8gbv1beta1.Gslb) ([]s
 }
 
 // SaveDNSEndpoint update DNS endpoint or create new one if doesnt exist
-func (r *GslbLoggerAssistant) SaveDNSEndpoint(namespace string, i *externaldns.DNSEndpoint) (*reconcile.Result, error) {
+func (r *GslbLoggerAssistant) SaveDNSEndpoint(namespace string, i *externaldns.DNSEndpoint) error {
 	found := &externaldns.DNSEndpoint{}
 	err := r.client.Get(context.TODO(), types.NamespacedName{
 		Name:      i.Name,
@@ -124,14 +123,14 @@ func (r *GslbLoggerAssistant) SaveDNSEndpoint(namespace string, i *externaldns.D
 			// Creation failed
 			r.Error(err, "Failed to create new DNSEndpoint DNSEndpoint.Namespace: %s DNSEndpoint.Name %s",
 				i.Namespace, i.Name)
-			return &reconcile.Result{}, err
+			return err
 		}
 		// Creation was successful
-		return nil, nil
+		return nil
 	} else if err != nil {
 		// Error that isn't due to the service not existing
 		r.Error(err, "Failed to get DNSEndpoint")
-		return &reconcile.Result{}, err
+		return err
 	}
 
 	// Update existing object with new spec
@@ -142,9 +141,9 @@ func (r *GslbLoggerAssistant) SaveDNSEndpoint(namespace string, i *externaldns.D
 		// Update failed
 		r.Error(err, "Failed to update DNSEndpoint DNSEndpoint.Namespace %s DNSEndpoint.Name %s",
 			found.Namespace, found.Name)
-		return &reconcile.Result{}, err
+		return err
 	}
-	return nil, nil
+	return nil
 }
 
 // RemoveEndpoint removes endpoint