Merge pull request #431 from pjbgf/align-output

Aligns output with source-controller on no-ops

Author: Paulo Gomes

controllers/imageupdateautomation_controller.go

@@ -35,12 +35,9 @@ import (
 	"github.com/go-logr/logr"
 	libgit2 "github.com/libgit2/git2go/v33"
 	corev1 "k8s.io/api/core/v1"
-	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
 	kuberecorder "k8s.io/client-go/tools/record"
-	"k8s.io/client-go/tools/reference"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/builder"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -56,9 +53,9 @@ import (
 	apiacl "github.com/fluxcd/pkg/apis/acl"
 	"github.com/fluxcd/pkg/apis/meta"
 	"github.com/fluxcd/pkg/runtime/acl"
+	helper "github.com/fluxcd/pkg/runtime/controller"
 	"github.com/fluxcd/pkg/runtime/events"
 	"github.com/fluxcd/pkg/runtime/logger"
-	"github.com/fluxcd/pkg/runtime/metrics"
 	"github.com/fluxcd/pkg/runtime/predicates"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
 	"github.com/fluxcd/source-controller/pkg/git"
@@ -87,9 +84,9 @@ type TemplateData struct {
 // ImageUpdateAutomationReconciler reconciles a ImageUpdateAutomation object
 type ImageUpdateAutomationReconciler struct {
 	client.Client
-	Scheme              *runtime.Scheme
-	EventRecorder       kuberecorder.EventRecorder
-	MetricsRecorder     *metrics.Recorder
+	EventRecorder kuberecorder.EventRecorder
+	helper.Metrics
+
 	NoCrossNamespaceRef bool
 }
 
@@ -107,7 +104,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 	log := ctrl.LoggerFrom(ctx)
 	debuglog := log.V(logger.DebugLevel)
 	tracelog := log.V(logger.TraceLevel)
-	now := time.Now()
+	start := time.Now()
 	var templateValues TemplateData
 
 	var auto imagev1.ImageUpdateAutomation
@@ -127,7 +124,6 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 
 	// If the object is under deletion, record the readiness, and remove our finalizer.
 	if !auto.ObjectMeta.DeletionTimestamp.IsZero() {
-		r.recordReadinessMetric(ctx, &auto)
 		controllerutil.RemoveFinalizer(&auto, imagev1.ImageUpdateAutomationFinalizer)
 		if err := r.Update(ctx, &auto); err != nil {
 			return ctrl.Result{}, err
@@ -136,7 +132,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 	}
 
 	// record suspension metrics
-	defer r.recordSuspension(ctx, auto)
+	r.RecordSuspend(ctx, &auto, auto.Spec.Suspend)
 
 	if auto.Spec.Suspend {
 		log.Info("ImageUpdateAutomation is suspended, skipping automation run")
@@ -145,18 +141,11 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 
 	templateValues.AutomationObject = req.NamespacedName
 
-	// Record readiness metric when exiting; if there's any points at
-	// which the readiness is updated _without also exiting_, they
-	// should also record the readiness.
-	defer r.recordReadinessMetric(ctx, &auto)
-	// Record reconciliation duration when exiting
-	if r.MetricsRecorder != nil {
-		objRef, err := reference.GetReference(r.Scheme, &auto)
-		if err != nil {
-			return ctrl.Result{}, err
-		}
-		defer r.MetricsRecorder.RecordDuration(*objRef, now)
-	}
+	defer func() {
+		// Always record readiness and duration metrics
+		r.Metrics.RecordReadiness(ctx, &auto)
+		r.Metrics.RecordDuration(ctx, &auto, start)
+	}()
 
 	// whatever else happens, we've now "seen" the reconcile
 	// annotation if it's there
@@ -388,7 +377,7 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 			return failWithError(err)
 		}
 
-		debuglog.Info("no changes made in working directory; no commit")
+		log.Info("no changes made in working directory; no commit")
 		statusMessage = "no updates made"
 
 		if auto.Status.LastPushTime != nil && len(auto.Status.LastPushCommit) >= 7 {
@@ -405,12 +394,12 @@ func (r *ImageUpdateAutomationReconciler) Reconcile(ctx context.Context, req ctr
 		r.event(ctx, auto, events.EventSeverityInfo, fmt.Sprintf("Committed and pushed change %s to %s\n%s", rev, pushBranch, message))
 		log.Info("pushed commit to origin", "revision", rev, "branch", pushBranch)
 		auto.Status.LastPushCommit = rev
-		auto.Status.LastPushTime = &metav1.Time{Time: now}
+		auto.Status.LastPushTime = &metav1.Time{Time: start}
 		statusMessage = "committed and pushed " + rev + " to " + pushBranch
 	}
 
 	// Getting to here is a successful run.
-	auto.Status.LastAutomationRunTime = &metav1.Time{Time: now}
+	auto.Status.LastAutomationRunTime = &metav1.Time{Time: start}
 	imagev1.SetImageUpdateAutomationReadiness(&auto, metav1.ConditionTrue, imagev1.ReconciliationSucceededReason, statusMessage)
 	if err := r.patchStatus(ctx, req, auto.Status); err != nil {
 		return ctrl.Result{Requeue: true}, err
@@ -923,26 +912,6 @@ func (r *ImageUpdateAutomationReconciler) event(ctx context.Context, auto imagev
 	r.EventRecorder.Eventf(&auto, eventtype, severity, msg)
 }
 
-func (r *ImageUpdateAutomationReconciler) recordReadinessMetric(ctx context.Context, auto *imagev1.ImageUpdateAutomation) {
-	if r.MetricsRecorder == nil {
-		return
-	}
-
-	objRef, err := reference.GetReference(r.Scheme, auto)
-	if err != nil {
-		ctrl.LoggerFrom(ctx).Error(err, "unable to record readiness metric")
-		return
-	}
-	if rc := apimeta.FindStatusCondition(auto.Status.Conditions, meta.ReadyCondition); rc != nil {
-		r.MetricsRecorder.RecordCondition(*objRef, *rc, !auto.DeletionTimestamp.IsZero())
-	} else {
-		r.MetricsRecorder.RecordCondition(*objRef, metav1.Condition{
-			Type:   meta.ReadyCondition,
-			Status: metav1.ConditionUnknown,
-		}, !auto.DeletionTimestamp.IsZero())
-	}
-}
-
 // --- updates
 
 // updateAccordingToSetters updates files under the root by treating
@@ -951,25 +920,6 @@ func updateAccordingToSetters(ctx context.Context, tracelog logr.Logger, path st
 	return update.UpdateWithSetters(tracelog, path, path, policies)
 }
 
-func (r *ImageUpdateAutomationReconciler) recordSuspension(ctx context.Context, auto imagev1.ImageUpdateAutomation) {
-	if r.MetricsRecorder == nil {
-		return
-	}
-	log := ctrl.LoggerFrom(ctx)
-
-	objRef, err := reference.GetReference(r.Scheme, &auto)
-	if err != nil {
-		log.Error(err, "unable to record suspended metric")
-		return
-	}
-
-	if !auto.DeletionTimestamp.IsZero() {
-		r.MetricsRecorder.RecordSuspend(*objRef, false)
-	} else {
-		r.MetricsRecorder.RecordSuspend(*objRef, auto.Spec.Suspend)
-	}
-}
-
 // templateMsg renders a msg template, returning the message or an error.
 func templateMsg(messageTemplate string, templateValues *TemplateData) (string, error) {
 	if messageTemplate == "" {

controllers/suite_test.go

@@ -70,7 +70,6 @@ func TestMain(m *testing.M) {
 	controllerName := "image-automation-controller"
 	if err := (&ImageUpdateAutomationReconciler{
 		Client:        testEnv,
-		Scheme:        scheme.Scheme,
 		EventRecorder: testEnv.GetEventRecorderFor(controllerName),
 	}).SetupWithManager(testEnv, ImageUpdateAutomationReconcilerOptions{}); err != nil {
 		panic(fmt.Sprintf("Failed to start ImageUpdateAutomationReconciler: %v", err))

controllers/update_test.go

@@ -43,7 +43,6 @@ import (
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
-	"k8s.io/client-go/kubernetes/scheme"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -250,7 +249,6 @@ func TestImageAutomationReconciler_crossNamespaceRef(t *testing.T) {
 	builder := fakeclient.NewClientBuilder().WithScheme(testEnv.Scheme())
 	r := &ImageUpdateAutomationReconciler{
 		Client:              builder.Build(),
-		Scheme:              scheme.Scheme,
 		EventRecorder:       testEnv.GetEventRecorderFor("image-automation-controller"),
 		NoCrossNamespaceRef: true,
 	}
@@ -731,7 +729,6 @@ func TestImageAutomationReconciler_e2e(t *testing.T) {
 			// explicitly.
 			imageAutoReconciler := &ImageUpdateAutomationReconciler{
 				Client: testEnv,
-				Scheme: scheme.Scheme,
 			}
 
 			// Wait for the suspension to reach the cache

main.go

@@ -26,7 +26,6 @@ import (
 	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
 	_ "k8s.io/client-go/plugin/pkg/client/auth/gcp"
 	ctrl "sigs.k8s.io/controller-runtime"
-	ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/metrics"
 
 	imagev1_reflect "github.com/fluxcd/image-reflector-controller/api/v1beta1"
 	"github.com/fluxcd/pkg/runtime/acl"
@@ -36,7 +35,6 @@ import (
 	feathelper "github.com/fluxcd/pkg/runtime/features"
 	"github.com/fluxcd/pkg/runtime/leaderelection"
 	"github.com/fluxcd/pkg/runtime/logger"
-	"github.com/fluxcd/pkg/runtime/metrics"
 	"github.com/fluxcd/pkg/runtime/pprof"
 	"github.com/fluxcd/pkg/runtime/probes"
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
@@ -115,9 +113,6 @@ func main() {
 		os.Exit(1)
 	}
 
-	metricsRecorder := metrics.NewRecorder()
-	ctrlmetrics.Registry.MustRegister(metricsRecorder.Collectors()...)
-
 	watchNamespace := ""
 	if !watchAllNamespaces {
 		watchNamespace = os.Getenv("RUNTIME_NAMESPACE")
@@ -151,11 +146,12 @@ func main() {
 		os.Exit(1)
 	}
 
+	metricsH := helper.MustMakeMetrics(mgr)
+
 	if err = (&controllers.ImageUpdateAutomationReconciler{
 		Client:              mgr.GetClient(),
-		Scheme:              mgr.GetScheme(),
 		EventRecorder:       eventRecorder,
-		MetricsRecorder:     metricsRecorder,
+		Metrics:             metricsH,
 		NoCrossNamespaceRef: aclOptions.NoCrossNamespaceRefs,
 	}).SetupWithManager(mgr, controllers.ImageUpdateAutomationReconcilerOptions{
 		MaxConcurrentReconciles: concurrent,

tests/fuzz/image_update_fuzzer.go

@@ -178,7 +178,6 @@ func FuzzImageUpdateReconciler(data []byte) int {
 		utilruntime.Must(ensureDependencies(func(m manager.Manager) {
 			utilruntime.Must((&controllers.ImageUpdateAutomationReconciler{
 				Client: m.GetClient(),
-				Scheme: scheme.Scheme,
 			}).SetupWithManager(m, controllers.ImageUpdateAutomationReconcilerOptions{MaxConcurrentReconciles: 4}))
 		}))
 	})