fix: panic for incomplete metric in eviction controller (#1083)

* test: add tests for eviction complete metric

* add UT for failed reconcile, emit incomplete metric

* fix test name

* remove new line

* lint fix

* delete the incomplete eviction metric

* minor changes

* remove table from UT

* move complete metric UT -> IT

* emit incomplete metric in IT to ensure it gets deleted

* add comment

* address comments

* remove Ordered marker

* use DeletePartial

* address comment

---------

Co-authored-by: Arvind Thirumurugan <arvindth@microsoft.com>

Author: Arvindthiru

pkg/controllers/clusterresourceplacementeviction/controller.go

@@ -46,7 +46,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req runtime.Request) (runtim
 	var internalError bool
 	defer func() {
 		if internalError {
-			metrics.FleetEvictionStatus.WithLabelValues(evictionName, "false").SetToCurrentTime()
+			metrics.FleetEvictionStatus.WithLabelValues(evictionName, "false", "unknown").SetToCurrentTime()
 		}
 		latency := time.Since(startTime).Milliseconds()
 		klog.V(2).InfoS("ClusterResourcePlacementEviction reconciliation ends", "clusterResourcePlacementEviction", evictionName, "latency", latency)
@@ -365,7 +365,7 @@ func markEvictionNotExecuted(eviction *placementv1beta1.ClusterResourcePlacement
 }
 
 func emitEvictionCompleteMetric(eviction *placementv1beta1.ClusterResourcePlacementEviction) {
-	metrics.FleetEvictionStatus.Delete(prometheus.Labels{"name": eviction.Name, "isCompleted": "false"})
+	metrics.FleetEvictionStatus.DeletePartialMatch(prometheus.Labels{"name": eviction.GetName(), "isCompleted": "false"})
 	// check to see if eviction is valid.
 	if condition.IsConditionStatusTrue(eviction.GetCondition(string(placementv1beta1.PlacementEvictionConditionTypeValid)), eviction.GetGeneration()) {
 		metrics.FleetEvictionStatus.WithLabelValues(eviction.Name, "true", "true").SetToCurrentTime()

pkg/controllers/clusterresourceplacementeviction/controller_intergration_test.go

@@ -11,6 +11,8 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	"github.com/prometheus/client_golang/prometheus"
+	prometheusclientmodel "github.com/prometheus/client_model/go"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -21,6 +23,7 @@ import (
 
 	placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
 	"go.goms.io/fleet/pkg/utils/condition"
+	"go.goms.io/fleet/pkg/utils/controller/metrics"
 	testutilseviction "go.goms.io/fleet/test/utils/eviction"
 )
 
@@ -41,15 +44,46 @@ const (
 var _ = Describe("Test ClusterResourcePlacementEviction Controller", func() {
 	crpName := fmt.Sprintf(crpNameTemplate, GinkgoParallelProcess())
 	evictionName := fmt.Sprintf(evictionNameTemplate, GinkgoParallelProcess())
+	var customRegistry *prometheus.Registry
+
+	BeforeEach(func() {
+		// Create a test registry
+		customRegistry = prometheus.NewRegistry()
+		Expect(customRegistry.Register(metrics.FleetEvictionStatus)).Should(Succeed())
+		// Reset metrics before each test
+		metrics.FleetEvictionStatus.Reset()
+		// emit incomplete eviction metric to simulate eviction failed once.
+		metrics.FleetEvictionStatus.WithLabelValues(evictionName, "false", "unknown").SetToCurrentTime()
+	})
 
 	AfterEach(func() {
 		ensureCRPDBRemoved(crpName)
 		ensureAllBindingsAreRemoved(crpName)
 		ensureEvictionRemoved(evictionName)
 		ensureCRPRemoved(crpName)
+		Expect(customRegistry.Unregister(metrics.FleetEvictionStatus)).Should(BeTrue())
 	})
 
-	It("Eviction Blocked - ClusterResourcePlacementDisruptionBudget's maxUnavailable blocks eviction", func() {
+	It("Invalid Eviction Blocked - emit complete metric with isValid=false, isComplete=true", func() {
+		By("Create ClusterResourcePlacementEviction", func() {
+			eviction := buildTestEviction(evictionName, "random-crp", "test-cluster")
+			Expect(k8sClient.Create(ctx, eviction)).Should(Succeed())
+		})
+
+		By("Check eviction status", func() {
+			evictionStatusUpdatedActual := testutilseviction.StatusUpdatedActual(
+				ctx, k8sClient, evictionName,
+				&testutilseviction.IsValidEviction{IsValid: false, Msg: condition.EvictionInvalidMissingCRPMessage},
+				nil)
+			Eventually(evictionStatusUpdatedActual, eventuallyDuration, eventuallyInterval).Should(Succeed())
+		})
+
+		By("Ensure eviction complete metric was emitted", func() {
+			checkEvictionCompleteMetric(customRegistry, "false", "true")
+		})
+	})
+
+	It("Eviction Blocked - ClusterResourcePlacementDisruptionBudget's maxUnavailable blocks eviction, emit complete isValid=true, isComplete=true", func() {
 		crbName := fmt.Sprintf(crbNameTemplate, GinkgoParallelProcess())
 
 		By("Create ClusterResourcePlacement", func() {
@@ -122,9 +156,13 @@ var _ = Describe("Test ClusterResourcePlacementEviction Controller", func() {
 				return !k8serrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: crbName}, &crb))
 			}, consistentlyDuration, consistentlyInterval).Should(BeTrue())
 		})
+
+		By("Ensure eviction complete metric was emitted", func() {
+			checkEvictionCompleteMetric(customRegistry, "true", "true")
+		})
 	})
 
-	It("Eviction Allowed - ClusterResourcePlacementDisruptionBudget's maxUnavailable allows eviction", func() {
+	It("Eviction Allowed - ClusterResourcePlacementDisruptionBudget's maxUnavailable allows eviction, emit complete isValid=true, isComplete=true", func() {
 		crbName := fmt.Sprintf(crbNameTemplate, GinkgoParallelProcess())
 
 		By("Create ClusterResourcePlacement", func() {
@@ -218,9 +256,13 @@ var _ = Describe("Test ClusterResourcePlacementEviction Controller", func() {
 				return k8serrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: crbName}, &crb))
 			}, consistentlyDuration, consistentlyInterval).Should(BeTrue())
 		})
+
+		By("Ensure eviction complete metric was emitted", func() {
+			checkEvictionCompleteMetric(customRegistry, "true", "true")
+		})
 	})
 
-	It("Eviction Blocked - ClusterResourcePlacementDisruptionBudget's minAvailable blocks eviction", func() {
+	It("Eviction Blocked - ClusterResourcePlacementDisruptionBudget's minAvailable blocks eviction, emit complete isValid=true, isComplete=true", func() {
 		crbName := fmt.Sprintf(crbNameTemplate, GinkgoParallelProcess())
 
 		By("Create ClusterResourcePlacement", func() {
@@ -293,9 +335,13 @@ var _ = Describe("Test ClusterResourcePlacementEviction Controller", func() {
 				return !k8serrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: crbName}, &crb))
 			}, consistentlyDuration, consistentlyInterval).Should(BeTrue())
 		})
+
+		By("Ensure eviction complete metric was emitted", func() {
+			checkEvictionCompleteMetric(customRegistry, "true", "true")
+		})
 	})
 
-	It("Eviction Allowed - ClusterResourcePlacementDisruptionBudget's minUnavailable allows eviction", func() {
+	It("Eviction Allowed - ClusterResourcePlacementDisruptionBudget's minUnavailable allows eviction, emit complete isValid=true, isComplete=true", func() {
 		crbName := fmt.Sprintf(crbNameTemplate, GinkgoParallelProcess())
 		anotherCRBName := fmt.Sprintf(anotherCRBNameTemplate, GinkgoParallelProcess())
 
@@ -419,6 +465,10 @@ var _ = Describe("Test ClusterResourcePlacementEviction Controller", func() {
 				return !k8serrors.IsNotFound(k8sClient.Get(ctx, types.NamespacedName{Name: anotherCRBName}, &anotherCRB))
 			}, consistentlyDuration, consistentlyInterval).Should(BeTrue())
 		})
+
+		By("Ensure eviction complete metric was emitted", func() {
+			checkEvictionCompleteMetric(customRegistry, "true", "true")
+		})
 	})
 })
 
@@ -464,7 +514,7 @@ func ensureEvictionRemoved(name string) {
 			Name: name,
 		},
 	}
-	Expect(k8sClient.Delete(ctx, &eviction)).Should(Succeed())
+	Expect(client.IgnoreNotFound(k8sClient.Delete(ctx, &eviction))).Should(Succeed())
 	// Ensure eviction doesn't exist.
 	Eventually(func() error {
 		if err := k8sClient.Get(ctx, types.NamespacedName{Name: name}, &eviction); !k8serrors.IsNotFound(err) {
@@ -536,3 +586,26 @@ func ensureAllBindingsAreRemoved(crpName string) {
 		ensureCRBRemoved(bindingList.Items[i].Name)
 	}
 }
+
+func checkEvictionCompleteMetric(registry *prometheus.Registry, isValid, isComplete string) {
+	metricFamilies, err := registry.Gather()
+	Expect(err).Should(Succeed())
+	var evictionCompleteMetrics []*prometheusclientmodel.Metric
+	for _, mf := range metricFamilies {
+		if mf.GetName() == "fleet_workload_eviction_complete" {
+			evictionCompleteMetrics = mf.GetMetric()
+		}
+	}
+	// we only expect one metric, incomplete eviction metric should be removed.
+	Expect(len(evictionCompleteMetrics)).Should(Equal(1))
+	metricLabels := evictionCompleteMetrics[0].GetLabel()
+	Expect(len(metricLabels)).Should(Equal(3))
+	for _, label := range metricLabels {
+		if label.GetName() == "isValid" {
+			Expect(label.GetValue()).Should(Equal(isValid))
+		}
+		if label.GetName() == "isComplete" {
+			Expect(label.GetValue()).Should(Equal(isComplete))
+		}
+	}
+}

pkg/controllers/clusterresourceplacementeviction/controller_test.go

@@ -6,22 +6,29 @@ Licensed under the MIT license.
 package clusterresourceplacementeviction
 
 import (
+	"context"
 	"errors"
+	"fmt"
 	"strings"
 	"testing"
 	"time"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/prometheus/client_golang/prometheus"
+	prometheusclientmodel "github.com/prometheus/client_model/go"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/utils/ptr"
+	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	placementv1beta1 "go.goms.io/fleet/apis/placement/v1beta1"
 	"go.goms.io/fleet/pkg/utils/condition"
+	"go.goms.io/fleet/pkg/utils/controller/metrics"
 	"go.goms.io/fleet/pkg/utils/defaulter"
 )
 
@@ -1469,6 +1476,69 @@ func TestIsEvictionAllowed(t *testing.T) {
 	}
 }
 
+func TestReconcileForIncompleteEvictionMetric(t *testing.T) {
+	request := controllerruntime.Request{NamespacedName: types.NamespacedName{Name: "test-eviction"}}
+	isValid := "unknown"
+	isComplete := "false"
+
+	// Create a test registry
+	customRegistry := prometheus.NewRegistry()
+	if err := customRegistry.Register(metrics.FleetEvictionStatus); err != nil {
+		t.Errorf("Failed to register metric: %v", err)
+	}
+
+	// Reset metrics before each test
+	metrics.FleetEvictionStatus.Reset()
+
+	scheme := serviceScheme(t)
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		Build()
+	r := Reconciler{
+		Client: mockClient{fakeClient},
+	}
+	_, err := r.Reconcile(ctx, request)
+	if err == nil {
+		t.Errorf("reconcile should have failed")
+	}
+
+	metricFamilies, err := customRegistry.Gather()
+	if err != nil {
+		t.Errorf("error gathering metrics: %v", err)
+	}
+
+	var evictionCompleteMetrics []*prometheusclientmodel.Metric
+	for _, mf := range metricFamilies {
+		if mf.GetName() == "fleet_workload_eviction_complete" {
+			evictionCompleteMetrics = mf.GetMetric()
+		}
+	}
+
+	// we only expect one metric.
+	if len(evictionCompleteMetrics) != 1 {
+		t.Errorf("expected one eviction complete metric, got %d", len(evictionCompleteMetrics))
+	}
+
+	// Check if the metric matches the expected label values
+	labels := evictionCompleteMetrics[0].GetLabel()
+	if len(labels) != 3 {
+		t.Errorf("expected three labels, got %d", len(labels))
+	}
+
+	for _, label := range labels {
+		if label.GetName() == "isValid" {
+			if label.GetValue() != isValid {
+				t.Errorf("isValid label value doesn't match got: %v, want %v", label.GetValue(), isValid)
+			}
+		}
+		if label.GetName() == "isComplete" {
+			if label.GetValue() != isComplete {
+				t.Errorf("isComplete label value doesn't match got: %v, want %v", label.GetValue(), isComplete)
+			}
+		}
+	}
+}
+
 func buildTestPickAllCRP(crpName string) placementv1beta1.ClusterResourcePlacement {
 	return placementv1beta1.ClusterResourcePlacement{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1492,3 +1562,11 @@ func serviceScheme(t *testing.T) *runtime.Scheme {
 	}
 	return scheme
 }
+
+type mockClient struct {
+	client.Client
+}
+
+func (m mockClient) Get(_ context.Context, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error {
+	return fmt.Errorf("internal error")
+}