Merge pull request #950 from akrejcir/fix-template-metric

fix: Increase reconcile metric only if SSP CR was not changed
kubevirt · Apr 15, 2024 · c1637a1 · c1637a1
2 parents 04a71b6 + 281c552
commit c1637a1
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
@@ -149,18 +149,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) {
@@ -231,11 +233,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
@@ -73,7 +73,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)
 	}
 
@@ -90,7 +90,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
@@ -72,6 +72,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{
@@ -84,8 +88,9 @@ var _ = Describe("Common-Templates operand", func() {
 					},
 				},
 			},
-			Logger:       log,
-			VersionCache: common.VersionCache{},
+			InstanceChanged: false,
+			Logger:          log,
+			VersionCache:    common.VersionCache{},
 		}
 	})
 
@@ -325,6 +330,26 @@ var _ = Describe("Common-Templates operand", func() {
 			Expect(err).ToNot(HaveOccurred())
 			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))
+
+			value, err := metrics.GetCommonTemplatesRestored()
+			Expect(err).ToNot(HaveOccurred())
+			Expect(value).To(Equal(initialMetricValue))
+		})
 	})
 })