fix: Increase reconcile metric only if SSP CR was not changed

The metric "kubevirt_ssp_common_templates_restored_total" should be increased if user changes a template and the operator restores it back to what it should be. If the SSP CR is changed, for example one of the labels that are propagated to the templates are changed, then the reconciliation should not increase the metric. Signed-off-by: Andrej Krejcir <[email protected]>
kubevirt · Apr 15, 2024 · fbbcbca · fbbcbca
1 parent df3777a
commit fbbcbca
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 22 deletions.
diff --git a/controllers/ssp_controller.go b/controllers/ssp_controller.go
@@ -148,18 +148,20 @@ func (r *sspReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ct
 		// Error reading the object - requeue the request.
 		return ctrl.Result{}, err
 	}
-	r.clearCacheIfNeeded(instance)
+
+	sspChanged := r.clearCacheIfNeeded(instance)
 
 	sspRequest := &common.Request{
-		Request:        req,
-		Client:         r.client,
-		UncachedReader: r.uncachedReader,
-		Context:        ctx,
-		Instance:       instance,
-		Logger:         reqLogger,
-		VersionCache:   r.subresourceCache,
-		TopologyMode:   r.topologyMode,
-		CrdList:        r.crdList,
+		Request:         req,
+		Client:          r.client,
+		UncachedReader:  r.uncachedReader,
+		Context:         ctx,
+		Instance:        instance,
+		InstanceChanged: sspChanged,
+		Logger:          reqLogger,
+		VersionCache:    r.subresourceCache,
+		TopologyMode:    r.topologyMode,
+		CrdList:         r.crdList,
 	}
 
 	if !isInitialized(sspRequest.Instance) {
@@ -230,11 +232,13 @@ func (r *sspReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ct
 	return ctrl.Result{}, nil
 }
 
-func (r *sspReconciler) clearCacheIfNeeded(sspObj *ssp.SSP) {
+func (r *sspReconciler) clearCacheIfNeeded(sspObj *ssp.SSP) bool {
 	if !reflect.DeepEqual(r.lastSspSpec, sspObj.Spec) {
 		r.subresourceCache = common.VersionCache{}
 		r.lastSspSpec = sspObj.Spec
+		return true
 	}
+	return false
 }
 
 func (r *sspReconciler) clearCache() {

diff --git a/internal/common/request.go b/internal/common/request.go
@@ -14,13 +14,14 @@ import (
 
 type Request struct {
 	reconcile.Request
-	Client         client.Client
-	UncachedReader client.Reader
-	Context        context.Context
-	Instance       *ssp.SSP
-	Logger         logr.Logger
-	VersionCache   VersionCache
-	TopologyMode   osconfv1.TopologyMode
+	Client          client.Client
+	UncachedReader  client.Reader
+	Context         context.Context
+	Instance        *ssp.SSP
+	InstanceChanged bool
+	Logger          logr.Logger
+	VersionCache    VersionCache
+	TopologyMode    osconfv1.TopologyMode
 
 	CrdList crd_watch.CrdList
 }

diff --git a/internal/operands/common-templates/reconcile.go b/internal/operands/common-templates/reconcile.go
@@ -80,7 +80,7 @@ func (c *commonTemplates) Reconcile(request *common.Request) ([]common.Reconcile
 		return nil, err
 	}
 
-	if !isUpgradingNow(request) {
+	if !operatorIsUpgrading(request) && !request.InstanceChanged {
 		incrementTemplatesRestoredMetric(reconcileTemplatesResults, request.Logger)
 	}
 
@@ -97,7 +97,7 @@ func (c *commonTemplates) Reconcile(request *common.Request) ([]common.Reconcile
 	return append(reconcileTemplatesResults, oldTemplatesResults...), nil
 }
 
-func isUpgradingNow(request *common.Request) bool {
+func operatorIsUpgrading(request *common.Request) bool {
 	return request.Instance.Status.ObservedVersion != common.GetOperatorVersion()
 }
 

diff --git a/internal/operands/common-templates/reconcile_test.go b/internal/operands/common-templates/reconcile_test.go
@@ -73,6 +73,10 @@ var _ = Describe("Common-Templates operand", func() {
 				ObjectMeta: metav1.ObjectMeta{
 					Name:      name,
 					Namespace: namespace,
+					Labels: map[string]string{
+						common.AppKubernetesPartOfLabel:  "template-unit-tests",
+						common.AppKubernetesVersionLabel: "v1.0.0",
+					},
 				},
 				Spec: ssp.SSPSpec{
 					CommonTemplates: ssp.CommonTemplates{
@@ -85,8 +89,9 @@ var _ = Describe("Common-Templates operand", func() {
 					},
 				},
 			},
-			Logger:       log,
-			VersionCache: common.VersionCache{},
+			InstanceChanged: false,
+			Logger:          log,
+			VersionCache:    common.VersionCache{},
 		}
 	})
 
@@ -326,6 +331,26 @@ var _ = Describe("Common-Templates operand", func() {
 			Expect(desc).To(ContainSubstring("kubevirt_ssp_common_templates_restored_total"))
 			Expect(value).To(Equal(initialMetricValue))
 		})
+
+		It("should not increase when SSP CR is changed", func() {
+			const updatedPartOf = "updated-part-of"
+			const updatedVersion = "v2.0.0"
+
+			request.Instance.Labels[common.AppKubernetesPartOfLabel] = updatedPartOf
+			request.Instance.Labels[common.AppKubernetesVersionLabel] = updatedVersion
+			request.InstanceChanged = true
+
+			_, err := operand.Reconcile(&request)
+			Expect(err).ToNot(HaveOccurred())
+
+			updatedTemplate := getTemplate(request, template)
+			Expect(updatedTemplate.Labels).To(HaveKeyWithValue(common.AppKubernetesPartOfLabel, updatedPartOf))
+			Expect(updatedTemplate.Labels).To(HaveKeyWithValue(common.AppKubernetesVersionLabel, updatedVersion))
+
+			desc, value := getCommonTemplatesRestoredMetric()
+			Expect(desc).To(ContainSubstring("kubevirt_ssp_common_templates_restored_total"))
+			Expect(value).To(Equal(initialMetricValue))
+		})
 	})
 })