From 0450dded45f8d7c7c20094fe05c424759b7e442c Mon Sep 17 00:00:00 2001 From: Gianluca Mardente Date: Fri, 30 Oct 2026 21:22:13 +0100 Subject: [PATCH] fix: Count a drift once when the ClusterSummary update conflicts processResourceSummary called trackDrifts inside the RetryOnConflict closure, before the ClusterSummary status update. Every Conflict ran the closure again, so projectsveltos_total_drifts was incremented again for the same drift. Collect the drifted features in the closure and count them once the update has succeeded. Fixes #2075 --- controllers/controllers_suite_test.go | 3 + controllers/export_test.go | 8 ++ controllers/handlers_helm_test.go | 2 +- controllers/resourcesummary_collection.go | 37 ++++++++-- .../resourcesummary_collection_test.go | 73 +++++++++++++++++++ controllers/suite_helpers_test.go | 30 ++++++++ 6 files changed, 145 insertions(+), 8 deletions(-) diff --git a/controllers/controllers_suite_test.go b/controllers/controllers_suite_test.go index e7155b4d..af0ee56d 100644 --- a/controllers/controllers_suite_test.go +++ b/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 diff --git a/controllers/export_test.go b/controllers/export_test.go index 3fea891e..e75c36f5 100644 --- a/controllers/export_test.go +++ b/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, diff --git a/controllers/handlers_helm_test.go b/controllers/handlers_helm_test.go index d109a3ef..782d0521 100644 --- a/controllers/handlers_helm_test.go +++ b/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")) } diff --git a/controllers/resourcesummary_collection.go b/controllers/resourcesummary_collection.go index 168ae548..58df65a7 100644 --- a/controllers/resourcesummary_collection.go +++ b/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,6 +629,7 @@ 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 }) @@ -632,9 +637,27 @@ func processResourceSummary(ctx context.Context, clusterClient client.Client, 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 diff --git a/controllers/resourcesummary_collection_test.go b/controllers/resourcesummary_collection_test.go index 8e832311..c1e08a21 100644 --- a/controllers/resourcesummary_collection_test.go +++ b/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() diff --git a/controllers/suite_helpers_test.go b/controllers/suite_helpers_test.go index d86c1499..2b152ea1 100644 --- a/controllers/suite_helpers_test.go +++ b/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