Fix total_restored_common_templates metric update

Signed-off-by: João Vilaça <jvilaca@redhat.com>

internal/common/resource.go

@@ -39,6 +39,7 @@ type ResourceStatus struct {
 
 type ReconcileResult struct {
 	Status          ResourceStatus
+	InitialResource client.Object
 	Resource        client.Object
 	OperationResult OperationResult
 }
@@ -192,7 +193,7 @@ func (r *reconcileBuilder) Reconcile() (ReconcileResult, error) {
 		return nil
 	}
 
-	res, err := r.createOrUpdateWithImmutableSpec(found, mutateFn)
+	res, existing, err := r.createOrUpdateWithImmutableSpec(found, mutateFn)
 	if err != nil {
 		r.request.Logger.Info(fmt.Sprintf("Resource create/update failed: %v", err))
 		return ReconcileResult{}, err
@@ -206,7 +207,7 @@ func (r *reconcileBuilder) Reconcile() (ReconcileResult, error) {
 	logOperation(res, found, r.request.Logger)
 
 	status := r.statusFunc(found)
-	return ReconcileResult{status, r.resource, res}, nil
+	return ReconcileResult{status, existing, r.resource, res}, nil
 }
 
 func CreateOrUpdate(request *Request) ReconcileBuilder {
@@ -285,43 +286,43 @@ func DeleteAll(request *Request, resources ...client.Object) ([]CleanupResult, e
 }
 
 // This function was initially copied from controllerutil.CreateOrUpdate
-func (r *reconcileBuilder) createOrUpdateWithImmutableSpec(obj client.Object, f controllerutil.MutateFn) (OperationResult, error) {
+func (r *reconcileBuilder) createOrUpdateWithImmutableSpec(obj client.Object, f controllerutil.MutateFn) (OperationResult, client.Object, error) {
 	key := client.ObjectKeyFromObject(obj)
 	if err := r.request.Client.Get(r.request.Context, key, obj); err != nil {
 		if !errors.IsNotFound(err) {
-			return OperationResultNone, err
+			return OperationResultNone, nil, err
 		}
 		if err := mutate(f, key, obj); err != nil {
-			return OperationResultNone, err
+			return OperationResultNone, nil, err
 		}
 		if err := r.request.Client.Create(r.request.Context, obj); err != nil {
-			return OperationResultNone, err
+			return OperationResultNone, nil, err
 		}
-		return OperationResultCreated, nil
+		return OperationResultCreated, nil, nil
 	}
 
-	existing := obj.DeepCopyObject()
+	existing := obj.DeepCopyObject().(client.Object)
 	if err := mutate(f, key, obj); err != nil {
-		return OperationResultNone, err
+		return OperationResultNone, existing, err
 	}
 
 	if equality.Semantic.DeepEqual(existing, obj) {
-		return OperationResultNone, nil
+		return OperationResultNone, existing, nil
 	}
 
 	// If the resource is immutable and specs are not equal, delete it.
 	// It will be recreated in the next iteration.
-	if r.immutableSpec && !equality.Semantic.DeepEqual(r.specGetter(existing.(client.Object)), r.specGetter(obj)) {
+	if r.immutableSpec && !equality.Semantic.DeepEqual(r.specGetter(existing), r.specGetter(obj)) {
 		if err := r.request.Client.Delete(r.request.Context, obj); err != nil {
-			return OperationResultNone, err
+			return OperationResultNone, existing, err
 		}
-		return OperationResultDeleted, nil
+		return OperationResultDeleted, existing, nil
 	}
 
 	if err := r.request.Client.Update(r.request.Context, obj); err != nil {
-		return OperationResultNone, err
+		return OperationResultNone, existing, err
 	}
-	return OperationResultUpdated, nil
+	return OperationResultUpdated, existing, nil
 }
 
 // This function is a copy of controllerutil.mutate

internal/operands/common-templates/reconcile.go

@@ -5,16 +5,17 @@ import (
 	"regexp"
 	"strings"
 
-	"github.com/go-logr/logr"
 	templatev1 "github.com/openshift/api/template/v1"
-	"github.com/prometheus/client_golang/prometheus"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/selection"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/blang/semver/v4"
+	"github.com/go-logr/logr"
+	"github.com/prometheus/client_golang/prometheus"
+
 	"kubevirt.io/ssp-operator/internal/common"
 	"kubevirt.io/ssp-operator/internal/operands"
 )
@@ -101,10 +102,15 @@ func isUpgradingNow(request *common.Request) bool {
 }
 
 func incrementTemplatesRestoredMetric(reconcileResults []common.ReconcileResult, logger logr.Logger) {
-	for i := range reconcileResults {
-		if reconcileResults[i].OperationResult == common.OperationResultUpdated {
-			logger.Info(fmt.Sprintf("Changes reverted in common template: %s", reconcileResults[i].Resource.GetName()))
-			CommonTemplatesRestored.Inc()
+	for _, reconcileResult := range reconcileResults {
+		if reconcileResult.InitialResource != nil {
+			oldVersion := reconcileResult.InitialResource.GetLabels()[TemplateVersionLabel]
+			newVersion := reconcileResult.Resource.GetLabels()[TemplateVersionLabel]
+
+			if reconcileResult.OperationResult == common.OperationResultUpdated && oldVersion == newVersion {
+				logger.Info(fmt.Sprintf("Changes reverted in common template: %s", reconcileResult.Resource.GetName()))
+				CommonTemplatesRestored.Inc()
+			}
 		}
 	}
 }

internal/operands/common-templates/reconcile_test.go

@@ -7,10 +7,14 @@ import (
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+
 	templatev1 "github.com/openshift/api/template/v1"
 	libhandler "github.com/operator-framework/operator-lib/handler"
+	"github.com/prometheus/client_golang/prometheus"
+	io_prometheus_client "github.com/prometheus/client_model/go"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	lifecycleapi "kubevirt.io/controller-lifecycle-operator-sdk/api"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
@@ -75,6 +79,11 @@ var _ = Describe("Common-Templates operand", func() {
 						Namespace: namespace,
 					},
 				},
+				Status: ssp.SSPStatus{
+					Status: lifecycleapi.Status{
+						ObservedVersion: common.GetOperatorVersion(),
+					},
+				},
 			},
 			Logger:       log,
 			VersionCache: common.VersionCache{},
@@ -88,9 +97,13 @@ var _ = Describe("Common-Templates operand", func() {
 			template.Namespace = namespace
 			ExpectResourceExists(&template, request)
 		}
+
+		desc, value := getCommonTemplatesRestoredMetric()
+		Expect(desc).To(ContainSubstring("total_restored_common_templates"))
+		Expect(value).To(BeZero())
 	})
 
-	It("should reconcle predefined labels", func() {
+	It("should reconcile predefined labels", func() {
 		const (
 			defaultOsLabel = "template.kubevirt.io/default-os-variant"
 			testLabel      = "some.test.label"
@@ -117,6 +130,10 @@ var _ = Describe("Common-Templates operand", func() {
 			Expect(template.Labels).ToNot(HaveKey(defaultOsLabel))
 			Expect(template.Labels).To(HaveKey(testLabel))
 		}
+
+		desc, value := getCommonTemplatesRestoredMetric()
+		Expect(desc).To(ContainSubstring("total_restored_common_templates"))
+		Expect(value).To(Equal(float64(len(testTemplates))))
 	})
 
 	Context("old templates", func() {
@@ -204,6 +221,7 @@ var _ = Describe("Common-Templates operand", func() {
 			Expect(updatedTpl.GetAnnotations()[libhandler.NamespacedNameAnnotation]).ToNot(Equal(""), "owner name annotation is empty for an older template")
 			Expect(updatedTpl.GetAnnotations()[libhandler.TypeAnnotation]).ToNot(Equal(""), "owner type annotation is empty for an older template")
 		})
+
 		It("should remove labels from old templates but keep future template untouched", func() {
 			_, err := operand.Reconcile(&request)
 			Expect(err).ToNot(HaveOccurred(), "reconciliation in order to update old template failed")
@@ -232,6 +250,7 @@ var _ = Describe("Common-Templates operand", func() {
 			Expect(newerTpl.Labels[TemplateVersionLabel]).To(Equal(futureVersion), TemplateVersionLabel+" should equal "+futureVersion)
 			Expect(newerTpl.Annotations[TemplateDeprecatedAnnotation]).To(Equal(""), TemplateDeprecatedAnnotation+" should be empty")
 		})
+
 		It("should not remove labels from latest templates", func() {
 			_, err := operand.Reconcile(&request)
 			Expect(err).ToNot(HaveOccurred(), "reconciliation in order to update old template failed")
@@ -258,6 +277,63 @@ var _ = Describe("Common-Templates operand", func() {
 			}
 		})
 	})
+
+	Context("total_restored_common_templates metric", func() {
+		var template *templatev1.Template
+		var initialMetricValue float64
+
+		BeforeEach(func() {
+			_, err := operand.Reconcile(&request)
+			Expect(err).ToNot(HaveOccurred())
+
+			template = &testTemplates[0]
+			template.Namespace = namespace
+
+			desc, value := getCommonTemplatesRestoredMetric()
+			Expect(desc).To(ContainSubstring("total_restored_common_templates"))
+			initialMetricValue = value
+		})
+
+		It("should increase by 1 when one template is restored", func() {
+			cloneTemplate := template.DeepCopy()
+			escapedTemplateTypeLabel := strings.ReplaceAll(TemplateTypeLabel, "/", "~1")
+			labelJsonPatch := `[{"op": "add", "path": "/metadata/labels/` + escapedTemplateTypeLabel + `", "value": "rand"}]`
+			err := request.Client.Patch(request.Context, cloneTemplate, client.RawPatch(types.JSONPatchType, []byte(labelJsonPatch)))
+			Expect(err).ToNot(HaveOccurred())
+
+			_, err = operand.Reconcile(&request)
+			Expect(err).ToNot(HaveOccurred())
+
+			updatedTpl := getTemplate(request, cloneTemplate)
+			Expect(updatedTpl.Labels[TemplateTypeLabel]).To(Equal(template.Labels[TemplateTypeLabel]))
+
+			desc, value := getCommonTemplatesRestoredMetric()
+			Expect(desc).To(ContainSubstring("total_restored_common_templates"))
+			Expect(value).To(Equal(initialMetricValue + 1))
+		})
+
+		It("should not increase when template restored is from previous version", func() {
+			cloneTemplate := template.DeepCopy()
+			escapedTemplateTypeLabel := strings.ReplaceAll(TemplateTypeLabel, "/", "~1")
+			escapedTemplateVersionLabel := strings.ReplaceAll(TemplateVersionLabel, "/", "~1")
+			labelJsonPatch := `[
+				{"op": "add", "path": "/metadata/labels/` + escapedTemplateTypeLabel + `", "value": "rand"},
+				{"op": "add", "path": "/metadata/labels/` + escapedTemplateVersionLabel + `", "value": "rand"}
+			]`
+			err := request.Client.Patch(request.Context, cloneTemplate, client.RawPatch(types.JSONPatchType, []byte(labelJsonPatch)))
+			Expect(err).ToNot(HaveOccurred())
+
+			_, err = operand.Reconcile(&request)
+			Expect(err).ToNot(HaveOccurred())
+
+			updatedTpl := getTemplate(request, cloneTemplate)
+			Expect(updatedTpl.Labels[TemplateTypeLabel]).To(Equal(template.Labels[TemplateTypeLabel]))
+
+			desc, value := getCommonTemplatesRestoredMetric()
+			Expect(desc).To(ContainSubstring("total_restored_common_templates"))
+			Expect(value).To(Equal(initialMetricValue))
+		})
+	})
 })
 
 func getTestTemplates() []templatev1.Template {
@@ -285,3 +361,24 @@ func getTestTemplates() []templatev1.Template {
 		},
 	}}
 }
+
+func getCommonTemplatesRestoredMetric() (string, float64) {
+	ch := make(chan prometheus.Metric, 1)
+	CommonTemplatesRestored.Collect(ch)
+	close(ch)
+	m := <-ch
+	dto := &io_prometheus_client.Metric{}
+	err := m.Write(dto)
+	Expect(err).ToNot(HaveOccurred())
+
+	return m.Desc().String(), dto.GetCounter().GetValue()
+}
+
+func getTemplate(req common.Request, template *templatev1.Template) *templatev1.Template {
+	key := client.ObjectKeyFromObject(template)
+	updatedTpl := &templatev1.Template{}
+	err := req.Client.Get(req.Context, key, updatedTpl)
+	Expect(err).ToNot(HaveOccurred())
+
+	return updatedTpl
+}

main.go

@@ -29,7 +29,6 @@ import (
 	_ "k8s.io/client-go/plugin/pkg/client/auth"
 
 	"github.com/prometheus/client_golang/prometheus/promhttp"
-	common_templates "kubevirt.io/ssp-operator/internal/operands/common-templates"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/healthz"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -38,6 +37,7 @@ import (
 
 	"kubevirt.io/ssp-operator/controllers"
 	"kubevirt.io/ssp-operator/internal/common"
+	common_templates "kubevirt.io/ssp-operator/internal/operands/common-templates"
 	"kubevirt.io/ssp-operator/webhooks"
 	// +kubebuilder:scaffold:imports
 )