cleanup code, add test coverage

Author: Alexander-Kita

pkg/autoscaler/metrics/collector.go

@@ -390,7 +390,7 @@ func (c *collection) setPause(pause bool) {
 	}
 }
 
-// pause the scraper, happens when activator in path
+// function to get if scraper is paused or not
 func (c *collection) getPaused() bool {
 	c.mux.RLock()
 	defer c.mux.RUnlock()

pkg/reconciler/metric/metric.go

@@ -43,6 +43,21 @@ var (
 
 // ReconcileKind implements Interface.ReconcileKind.
 func (r *reconciler) ReconcileKind(ctx context.Context, metric *autoscalingv1alpha1.Metric) pkgreconciler.Event {
+	// Check if autoscaler in proxy or serving mode, if so pause or resume if needed
+	revisionName := metric.Labels[serving.RevisionLabelKey]
+	if revisionName != "" {
+		paName := revisionName
+		pa, err := r.paLister.PodAutoscalers(metric.Namespace).Get(paName)
+		if err == nil && pa.Status.MetricsPaused {
+			r.collector.Pause(metric)
+		} else {
+			r.collector.Resume(metric)
+		}
+	} else {
+		// unpause metrics to be safe
+		r.collector.Resume(metric)
+	}
+
 	if err := r.collector.CreateOrUpdate(metric); err != nil {
 		switch {
 		case errors.Is(err, metrics.ErrFailedGetEndpoints):
@@ -58,25 +73,6 @@ func (r *reconciler) ReconcileKind(ctx context.Context, metric *autoscalingv1alp
 		return nil
 	}
 
-	// Check if autoscaler in proxy or serving mode, if so pause or resume if needed
-	revisionName := metric.Labels[serving.RevisionLabelKey]
-	if revisionName != "" {
-		paName := revisionName
-		pa, err := r.paLister.PodAutoscalers(metric.Namespace).Get(paName)
-		switch {
-		case err != nil:
-			// unpause in error case to be safe
-			r.collector.Resume(metric)
-		case pa.Status.MetricsPaused:
-			r.collector.Pause(metric)
-		default:
-			r.collector.Resume(metric)
-		}
-	} else {
-		// unpause metrics to be safe
-		r.collector.Resume(metric)
-	}
-
 	metric.Status.MarkMetricReady()
 	return nil
 }

pkg/reconciler/metric/metric_test.go

@@ -33,7 +33,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/wait"
 	clientgotesting "k8s.io/client-go/testing"
-	rcl "knative.dev/pkg/reconciler"
 
 	"knative.dev/pkg/configmap"
 	"knative.dev/pkg/controller"
@@ -285,27 +284,30 @@ func TestReconcilePaused(t *testing.T) {
 	scs.AutoscalingV1alpha1().PodAutoscalers(a.Namespace).Create(ctx, a, metav1.CreateOptions{})
 	autoscalerinformer.Get(ctx).Informer().GetIndexer().Add(a)
 
-	if la, ok := ctl.Reconciler.(rcl.LeaderAware); ok {
-		la.Promote(rcl.UniversalBucket(), func(rcl.Bucket, types.NamespacedName) {})
-	}
-
-	err = ctl.Reconciler.Reconcile(ctx, m.Namespace+"/"+m.Name)
-
-	if err != nil {
-		t.Fatal("Error reconciling metric %w", err)
-	} else if collector.paused.Load() {
-		t.Fatal("collector should not be paused")
+	if err := wait.PollUntilContextTimeout(ctx, 10*time.Millisecond, 1*time.Second, false, func(context.Context) (bool, error) {
+		return !collector.paused.Load(), nil
+	}); err != nil {
+		t.Fatal("collector is paused, should not be paused")
 	}
 
 	// pause metrics
 	a.Status.MetricsPaused = true
 	updatePodAutoscaler(t, ctx, a)
 
-	err = ctl.Reconciler.Reconcile(ctx, m.Namespace+"/"+m.Name)
-	if err != nil {
-		t.Fatal("Error reconciling metric")
-	} else if !collector.paused.Load() {
-		t.Fatal("collector should be paused")
+	if err := wait.PollUntilContextTimeout(ctx, 10*time.Millisecond, 1*time.Second, false, func(context.Context) (bool, error) {
+		return collector.paused.Load(), nil
+	}); err != nil {
+		t.Fatal("collector is not paused, should be paused")
+	}
+
+	// test when empty revision label found
+	m.Labels[serving.RevisionLabelKey] = ""
+	updateMetric(t, ctx, m)
+
+	if err := wait.PollUntilContextTimeout(ctx, 10*time.Millisecond, 1*time.Second, false, func(context.Context) (bool, error) {
+		return !collector.paused.Load(), nil
+	}); err != nil {
+		t.Fatal("collector is paused, should not be paused")
 	}
 }
 
@@ -327,6 +329,16 @@ func ready(m *autoscalingv1alpha1.Metric) {
 	m.Status.MarkMetricReady()
 }
 
+func updateMetric(
+	t *testing.T,
+	ctx context.Context,
+	m *autoscalingv1alpha1.Metric,
+) {
+	t.Helper()
+	servingclient.Get(ctx).AutoscalingV1alpha1().Metrics(m.Namespace).Update(ctx, m, metav1.UpdateOptions{})
+	metricinformer.Get(ctx).Informer().GetIndexer().Update(m)
+}
+
 func metric(namespace, name string, opts ...metricOption) *autoscalingv1alpha1.Metric {
 	m := &autoscalingv1alpha1.Metric{
 		ObjectMeta: metav1.ObjectMeta{