fix: clean up metrics for deleted mvtx (#2338)

Signed-off-by: Derek Wang <[email protected]>

pkg/reconciler/isbsvc/controller.go

@@ -116,9 +116,7 @@ func (r *interStepBufferServiceReconciler) reconcile(ctx context.Context, isbSvc
 	if controllerutil.ContainsFinalizer(isbSvc, deprecatedFinalizerName) { // Remove deprecated finalizer if exists
 		controllerutil.RemoveFinalizer(isbSvc, deprecatedFinalizerName)
 	}
-	if needsFinalizer(isbSvc) {
-		controllerutil.AddFinalizer(isbSvc, finalizerName)
-	}
+	controllerutil.AddFinalizer(isbSvc, finalizerName)
 
 	defer func() {
 		if isbSvc.Status.IsHealthy() {
@@ -139,13 +137,3 @@ func (r *interStepBufferServiceReconciler) reconcile(ctx context.Context, isbSvc
 	}
 	return installer.Install(ctx, isbSvc, r.client, r.kubeClient, r.config, log, r.recorder)
 }
-
-func needsFinalizer(isbSvc *dfv1.InterStepBufferService) bool {
-	if isbSvc.Spec.Redis != nil && isbSvc.Spec.Redis.Native != nil && isbSvc.Spec.Redis.Native.Persistence != nil {
-		return true
-	}
-	if isbSvc.Spec.JetStream != nil && isbSvc.Spec.JetStream.Persistence != nil {
-		return true
-	}
-	return false
-}

pkg/reconciler/isbsvc/controller_test.go

@@ -217,26 +217,6 @@ func TestNeedsUpdate(t *testing.T) {
 	})
 }
 
-func TestNeedsFinalizer(t *testing.T) {
-	t.Run("needs finalizer redis", func(t *testing.T) {
-		testStorageClass := "test"
-		testIsbs := nativeRedisIsbs.DeepCopy()
-		testIsbs.Spec.Redis.Native.Persistence = &dfv1.PersistenceStrategy{
-			StorageClassName: &testStorageClass,
-		}
-		assert.True(t, needsFinalizer(testIsbs))
-	})
-
-	t.Run("needs finalizer jetstream", func(t *testing.T) {
-		testStorageClass := "test"
-		testIsbs := jetStreamIsbs.DeepCopy()
-		testIsbs.Spec.JetStream.Persistence = &dfv1.PersistenceStrategy{
-			StorageClassName: &testStorageClass,
-		}
-		assert.True(t, needsFinalizer(testIsbs))
-	})
-}
-
 func contains(arr []string, str string) bool {
 	for _, a := range arr {
 		if a == str {

pkg/reconciler/monovertex/controller.go

@@ -68,6 +68,8 @@ func (mr *monoVertexReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 	monoVtx := &dfv1.MonoVertex{}
 	if err := mr.client.Get(ctx, req.NamespacedName, monoVtx); err != nil {
 		if apierrors.IsNotFound(err) {
+			// Clean up metrics here, since there's no finalizer defined for MonoVertex objects, best effort
+			cleanupMetrics(req.NamespacedName.Namespace, req.NamespacedName.Name)
 			return reconcile.Result{}, nil
 		}
 		mr.logger.Errorw("Unable to get MonoVertex", zap.Any("request", req), zap.Error(err))
@@ -100,12 +102,8 @@ func (mr *monoVertexReconciler) reconcile(ctx context.Context, monoVtx *dfv1.Mon
 	if !monoVtx.DeletionTimestamp.IsZero() {
 		log.Info("Deleting mono vertex")
 		mr.scaler.StopWatching(mVtxKey)
-		// Clean up metrics
-		_ = reconciler.MonoVertexHealth.DeleteLabelValues(monoVtx.Namespace, monoVtx.Name)
-		_ = reconciler.MonoVertexDesiredReplicas.DeleteLabelValues(monoVtx.Namespace, monoVtx.Name)
-		_ = reconciler.MonoVertexCurrentReplicas.DeleteLabelValues(monoVtx.Namespace, monoVtx.Name)
-		_ = reconciler.MonoVertexMaxReplicas.DeleteLabelValues(monoVtx.Namespace, monoVtx.Name)
-		_ = reconciler.MonoVertexMinReplicas.DeleteLabelValues(monoVtx.Namespace, monoVtx.Name)
+		// Clean up metrics, best effort
+		cleanupMetrics(monoVtx.Namespace, monoVtx.Name)
 		return ctrl.Result{}, nil
 	}
 
@@ -622,3 +620,12 @@ func (mr *monoVertexReconciler) checkChildrenResourceStatus(ctx context.Context,
 
 	return nil
 }
+
+// Clean up metrics, should be called when corresponding mvtx is deleted
+func cleanupMetrics(namespace, mvtx string) {
+	_ = reconciler.MonoVertexHealth.DeleteLabelValues(namespace, mvtx)
+	_ = reconciler.MonoVertexDesiredReplicas.DeleteLabelValues(namespace, mvtx)
+	_ = reconciler.MonoVertexCurrentReplicas.DeleteLabelValues(namespace, mvtx)
+	_ = reconciler.MonoVertexMaxReplicas.DeleteLabelValues(namespace, mvtx)
+	_ = reconciler.MonoVertexMinReplicas.DeleteLabelValues(namespace, mvtx)
+}