fix: Count a drift once when the ClusterSummary update conflicts

controllers/controllers_suite_test.go

@@ -51,6 +51,9 @@ import (
 	libsveltosset "github.com/projectsveltos/libsveltos/lib/set"
 )
 
+// clusterSummariesResource is the resource name of ClusterSummary, used to build API errors in tests
+const clusterSummariesResource = "clustersummaries"
+
 var (
 	testEnv          *helpers.TestEnvironment
 	cancel           context.CancelFunc

controllers/export_test.go

@@ -357,6 +357,14 @@ func SetManagementClusterConfigForTest(config *rest.Config) *rest.Config {
 	return old
 }
 
+// SetManagementClusterClientForTest overrides the management cluster client, leaving the rest of the
+// management cluster access untouched, and returns the previous value so a test can restore it.
+func SetManagementClusterClientForTest(c client.Client) client.Client {
+	old := managementClusterClient
+	managementClusterClient = c
+	return old
+}
+
 // NewDeploymentContext constructs a deploymentContext for use in tests.
 func NewDeploymentContext(
 	clusterSummary *configv1beta1.ClusterSummary,

controllers/handlers_helm_test.go

@@ -1511,7 +1511,7 @@ var _ = Describe("HandlersHelm", func() {
 						if subResourceName == testStatusField && conflictCount == 0 {
 							conflictCount++
 							return apierrors.NewConflict(
-								schema.GroupResource{Group: configv1beta1.GroupVersion.Group, Resource: "clustersummaries"},
+								schema.GroupResource{Group: configv1beta1.GroupVersion.Group, Resource: clusterSummariesResource},
 								obj.GetName(),
 								fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again"))
 						}

controllers/resourcesummary_collection.go

@@ -565,7 +565,15 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client,
 		return nil
 	}
 
+	// The closure below runs again on every Conflict. Drifts are counted only once the ClusterSummary
+	// status is updated, so one drift is never counted more than once.
+	var updatedClusterSummary *configv1beta1.ClusterSummary
+	var driftedFeatures []libsveltosv1beta1.FeatureID
+
 	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		updatedClusterSummary = nil
+		driftedFeatures = nil
+
 		clusterSummary := &configv1beta1.ClusterSummary{}
 		err := getManagementClusterClient().Get(ctx,
 			types.NamespacedName{Namespace: clusterSummaryNamespace, Name: clusterSummaryName}, clusterSummary)
@@ -583,7 +591,6 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client,
 		}
 
 		l := logger.WithValues("clusterSummary", clusterSummary.Name)
-		profileKind, profileNamespace, profileName := getProfileLabels(clusterSummary, l)
 		for i := range clusterSummary.Status.FeatureSummaries {
 			switch clusterSummary.Status.FeatureSummaries[i].FeatureID {
 			case libsveltosv1beta1.FeatureHelm:
@@ -594,8 +601,7 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client,
 					markDriftedHelmCharts(clusterSummary, rs.Status.DriftedHelmCharts, l)
 					clusterSummary.Status.FeatureSummaries[i].DriftHistory = newDriftHistory(
 						clusterSummary.Status.FeatureSummaries[i].DriftHistory, rs, libsveltosv1beta1.FeatureHelm)
-					trackDrifts(clusterSummaryNamespace, clusterSummary.Spec.ClusterName, string(clusterSummary.Status.FeatureSummaries[i].FeatureID),
-						string(clusterSummary.Spec.ClusterType), profileKind, profileNamespace, profileName, logger)
+					driftedFeatures = append(driftedFeatures, clusterSummary.Status.FeatureSummaries[i].FeatureID)
 				}
 			case libsveltosv1beta1.FeatureResources:
 				if rs.Status.ResourcesChanged {
@@ -604,8 +610,7 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client,
 					clusterSummary.Status.FeatureSummaries[i].Status = libsveltosv1beta1.FeatureStatusProvisioning
 					clusterSummary.Status.FeatureSummaries[i].DriftHistory = newDriftHistory(
 						clusterSummary.Status.FeatureSummaries[i].DriftHistory, rs, libsveltosv1beta1.FeatureResources)
-					trackDrifts(clusterSummaryNamespace, clusterSummary.Spec.ClusterName, string(clusterSummary.Status.FeatureSummaries[i].FeatureID),
-						string(clusterSummary.Spec.ClusterType), profileKind, profileNamespace, profileName, logger)
+					driftedFeatures = append(driftedFeatures, clusterSummary.Status.FeatureSummaries[i].FeatureID)
 				}
 			case libsveltosv1beta1.FeatureKustomize:
 				if rs.Status.KustomizeResourcesChanged {
@@ -614,8 +619,7 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client,
 					clusterSummary.Status.FeatureSummaries[i].Status = libsveltosv1beta1.FeatureStatusProvisioning
 					clusterSummary.Status.FeatureSummaries[i].DriftHistory = newDriftHistory(
 						clusterSummary.Status.FeatureSummaries[i].DriftHistory, rs, libsveltosv1beta1.FeatureKustomize)
-					trackDrifts(clusterSummaryNamespace, clusterSummary.Spec.ClusterName, string(clusterSummary.Status.FeatureSummaries[i].FeatureID),
-						string(clusterSummary.Spec.ClusterType), profileKind, profileNamespace, profileName, logger)
+					driftedFeatures = append(driftedFeatures, clusterSummary.Status.FeatureSummaries[i].FeatureID)
 				}
 			}
 		}
@@ -625,16 +629,35 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client,
 			logger.V(logs.LogInfo).Error(err, "failed to update ClusterSummary status")
 			return err
 		}
+		updatedClusterSummary = clusterSummary
 		return nil
 	})
 
 	if err != nil {
 		return err
 	}
 
+	trackDriftedFeatures(updatedClusterSummary, driftedFeatures, logger)
+
 	return resetResourceSummaryStatus(ctx, clusterClient, rs, logger)
 }
 
+// trackDriftedFeatures counts one drift for each of driftedFeatures. clusterSummary is nil when there was
+// nothing to update (ClusterSummary not found or marked for deletion): nothing is counted then.
+func trackDriftedFeatures(clusterSummary *configv1beta1.ClusterSummary, driftedFeatures []libsveltosv1beta1.FeatureID,
+	logger logr.Logger) {
+
+	if clusterSummary == nil {
+		return
+	}
+
+	profileKind, profileNamespace, profileName := getProfileLabels(clusterSummary, logger)
+	for _, featureID := range driftedFeatures {
+		trackDrifts(clusterSummary.Namespace, clusterSummary.Spec.ClusterName, string(featureID),
+			string(clusterSummary.Spec.ClusterType), profileKind, profileNamespace, profileName, logger)
+	}
+}
+
 // newDriftHistory returns the drift history of featureID once the drift reported by rs is added to
 // history. rs.Status.DriftedResources is shared by all features, so only the entries of featureID are
 // used. A resource that drifted before keeps a single entry, with the time of its latest drift, so

controllers/resourcesummary_collection_test.go

@@ -18,6 +18,7 @@ package controllers_test
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
@@ -29,10 +30,13 @@ import (
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/klog/v2/textlogger"
 	clusterv1 "sigs.k8s.io/cluster-api/api/core/v1beta2"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 
 	configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1"
 	"github.com/projectsveltos/addon-controller/controllers"
@@ -348,6 +352,75 @@ var _ = Describe("ResourceSummary Collection", func() {
 		}, 2*time.Second, pollingInterval).Should(BeTrue())
 	})
 
+	It("processResourceSummary counts a drift once when updating the ClusterSummary conflicts", func() {
+		clusterNamespace := randomString()
+		clusterSummary := &configv1beta1.ClusterSummary{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: clusterNamespace,
+				Name:      clusterProfileNamePrefix + randomString(),
+				OwnerReferences: []metav1.OwnerReference{
+					{
+						APIVersion: configv1beta1.GroupVersion.String(),
+						Kind:       configv1beta1.ClusterProfileKind,
+						Name:       randomString(),
+						UID:        types.UID(randomString()),
+					},
+				},
+			},
+			Spec: configv1beta1.ClusterSummarySpec{
+				ClusterNamespace: clusterNamespace,
+				ClusterName:      randomString(),
+				ClusterType:      libsveltosv1beta1.ClusterTypeCapi,
+			},
+			Status: configv1beta1.ClusterSummaryStatus{
+				FeatureSummaries: []configv1beta1.FeatureSummary{
+					{
+						FeatureID: libsveltosv1beta1.FeatureResources,
+						Status:    libsveltosv1beta1.FeatureStatusProvisioned,
+						Hash:      []byte(randomString()),
+					},
+				},
+			},
+		}
+
+		resourceSummary := getResourceSummary()
+		resourceSummary.Annotations = map[string]string{
+			libsveltosv1beta1.ClusterSummaryNameAnnotation:      clusterSummary.Name,
+			libsveltosv1beta1.ClusterSummaryNamespaceAnnotation: clusterSummary.Namespace,
+		}
+		resourceSummary.Status.ResourcesChanged = true
+
+		// The first update of the ClusterSummary status conflicts, the retry succeeds
+		conflicts := 0
+		c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(clusterSummary, resourceSummary).
+			WithStatusSubresource(clusterSummary, resourceSummary).
+			WithInterceptorFuncs(interceptor.Funcs{
+				SubResourceUpdate: func(ctx context.Context, cl client.Client, subResourceName string, obj client.Object,
+					opts ...client.SubResourceUpdateOption) error {
+
+					if _, ok := obj.(*configv1beta1.ClusterSummary); ok && conflicts == 0 {
+						conflicts++
+						return apierrors.NewConflict(schema.GroupResource{Resource: clusterSummariesResource}, obj.GetName(),
+							errors.New("the object has been modified"))
+					}
+					return cl.SubResource(subResourceName).Update(ctx, obj, opts...)
+				},
+			}).Build()
+
+		oldClient := controllers.SetManagementClusterClientForTest(c)
+		defer controllers.SetManagementClusterClientForTest(oldClient)
+
+		labels := clusterSummaryMetricLabels(clusterSummary, configv1beta1.ClusterProfileKind, "",
+			clusterSummary.OwnerReferences[0].Name)
+		Expect(getMetricValue("projectsveltos_total_drifts", labels)).To(BeZero())
+
+		Expect(controllers.ProcessResourceSummary(context.TODO(), c, resourceSummary.DeepCopy(),
+			textlogger.NewLogger(textlogger.NewConfig()))).To(Succeed())
+
+		Expect(conflicts).To(Equal(1))
+		Expect(getMetricValue("projectsveltos_total_drifts", labels)).To(Equal(float64(1)))
+	})
+
 	It("processResourceSummary records the drifted resources of each feature in DriftHistory", func() {
 		cluster := prepareCluster()
 

controllers/suite_helpers_test.go

@@ -269,6 +269,36 @@ func hasMetricSeries(name string, labels map[string]string) bool {
 	return false
 }
 
+// getMetricValue returns the value of the counter or gauge called name, in the controller-runtime registry,
+// for the series whose labels include all of labels. It returns 0 when there is no such series.
+func getMetricValue(name string, labels map[string]string) float64 {
+	families, err := metrics.Registry.Gather()
+	Expect(err).To(BeNil())
+
+	for _, family := range families {
+		if family.GetName() != name {
+			continue
+		}
+		for _, metric := range family.GetMetric() {
+			matched := 0
+			for _, label := range metric.GetLabel() {
+				if value, ok := labels[label.GetName()]; ok && value == label.GetValue() {
+					matched++
+				}
+			}
+			if matched != len(labels) {
+				continue
+			}
+			if metric.GetCounter() != nil {
+				return metric.GetCounter().GetValue()
+			}
+			return metric.GetGauge().GetValue()
+		}
+	}
+
+	return 0
+}
+
 // deleteResources deletes following resources:
 // - clusterProfile
 // - clusterSummary