fix: clear outdated_helm_chart for charts no longer referenced

controllers/helmchart_outdated_check.go

@@ -90,24 +90,34 @@ var (
 )
 
 // outdatedHelmChartPassState accumulates, across the concurrent workers of one
-// checkOutdatedHelmCharts pass, which metric label combinations were actually re-checked this
-// pass and which of those came back outdated. Both are needed to reconcile the gauge correctly:
-// a combination that was outdated last pass but wasn't re-checked this pass (its fetch failed)
-// must be left alone, not cleared — clearing it would be reporting "confirmed fixed" when
-// really nothing was learned.
+// checkOutdatedHelmCharts pass, which metric label combinations a ClusterSummary still references,
+// which were actually re-checked this pass and which of those came back outdated. All are needed
+// to reconcile the gauge correctly: a combination that was outdated last pass but wasn't
+// re-checked this pass (its fetch failed) must be left alone, not cleared — clearing it would be
+// reporting "confirmed fixed" when really nothing was learned. A combination no ClusterSummary
+// references anymore will never be re-checked, so it is cleared instead.
 type outdatedHelmChartPassState struct {
 	mu                sync.Mutex
+	referenced        map[outdatedHelmChartMetricKey]struct{}
 	currentlyOutdated map[outdatedHelmChartMetricKey]struct{}
 	checkedThisPass   map[outdatedHelmChartMetricKey]struct{}
 }
 
 func newOutdatedHelmChartPassState() *outdatedHelmChartPassState {
 	return &outdatedHelmChartPassState{
+		referenced:        map[outdatedHelmChartMetricKey]struct{}{},
 		currentlyOutdated: map[outdatedHelmChartMetricKey]struct{}{},
 		checkedThisPass:   map[outdatedHelmChartMetricKey]struct{}{},
 	}
 }
 
+func (s *outdatedHelmChartPassState) markReferenced(key *outdatedHelmChartMetricKey) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	s.referenced[*key] = struct{}{}
+}
+
 func (s *outdatedHelmChartPassState) markChecked(key *outdatedHelmChartMetricKey, outdated bool) {
 	s.mu.Lock()
 	defer s.mu.Unlock()
@@ -150,6 +160,12 @@ func checkOutdatedHelmCharts(ctx context.Context, c client.Client, logger logr.L
 	}
 
 	passState := newOutdatedHelmChartPassState()
+	for key, keyRefs := range refs {
+		for i := range keyRefs {
+			metricKey := metricKeyForRef(&keyRefs[i], key.chartName)
+			passState.markReferenced(&metricKey)
+		}
+	}
 
 	var wg sync.WaitGroup
 	for range numWorkers {
@@ -168,7 +184,8 @@ func checkOutdatedHelmCharts(ctx context.Context, c client.Client, logger logr.L
 }
 
 // reconcileOutdatedHelmChartMetric updates previouslyOutdatedHelmCharts for the next pass and
-// clears the gauge series for every combination confirmed, this pass, to no longer be outdated.
+// clears the gauge series for every combination confirmed, this pass, to no longer be outdated,
+// or no longer referenced by any ClusterSummary.
 // Deliberately never calls outdatedHelmChartGauge.Reset(): doing so up front would make every
 // still-outdated chart briefly vanish from /metrics while later workers in this same pass are
 // still catching up to it, producing a false "not outdated" reading for the gap. Combinations
@@ -184,12 +201,15 @@ func reconcileOutdatedHelmChartMetric(passState *outdatedHelmChartPassState) {
 		if _, stillOutdated := passState.currentlyOutdated[key]; stillOutdated {
 			continue // already carried forward above
 		}
-		if _, checked := passState.checkedThisPass[key]; !checked {
+		_, referenced := passState.referenced[key]
+		if _, checked := passState.checkedThisPass[key]; !checked && referenced {
 			// Could not re-verify this pass (its chart key's fetch failed) -- leave state
 			// unchanged rather than guessing, and retry on the next pass.
 			next[key] = struct{}{}
 			continue
 		}
+		// Either confirmed no longer outdated, or no ClusterSummary references the release
+		// anymore (it was deleted, or the chart was removed from its profile).
 		clearOutdatedHelmChart(key.profileKind, key.profileNamespace, key.profileName,
 			key.clusterType, key.clusterNamespace, key.clusterName,
 			key.chartName, key.releaseNamespace, key.releaseName)

controllers/helmchart_outdated_check_test.go

@@ -22,10 +22,16 @@ package controllers
 
 import (
 	"context"
+	"net/http"
+	"net/http/httptest"
 	"testing"
 
 	"github.com/go-logr/logr"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/metrics"
 
 	configv1beta1 "github.com/projectsveltos/addon-controller/api/v1beta1"
 )
@@ -234,3 +240,179 @@ func TestSetResolvedHelmChartIdentity_KeepsOutdatedVersionInfoWhenVersionUnchang
 		t.Errorf("expected LastCheckedTime to be left untouched, got nil")
 	}
 }
+
+// outdatedChartKey is the outdated_helm_chart series of the release clusterSummaryManagingChart manages.
+func outdatedChartKey() outdatedHelmChartMetricKey {
+	return outdatedHelmChartMetricKey{
+		profileKind: configv1beta1.ClusterProfileKind, profileName: testProfileName,
+		clusterType: testClusterType, clusterNamespace: testClusterNamespace, clusterName: testClusterName,
+		chartName: testChartName, releaseNamespace: testReleaseNamespace, releaseName: testReleaseName,
+	}
+}
+
+// clusterSummaryManagingChart returns the ClusterSummary of the ClusterProfile testProfileName that
+// manages version testChartVersion of testChartName, from repoURL.
+func clusterSummaryManagingChart(repoURL string) *configv1beta1.ClusterSummary {
+	return &configv1beta1.ClusterSummary{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      testProfileName + "-" + testClusterName,
+			Namespace: testClusterNamespace,
+			OwnerReferences: []metav1.OwnerReference{
+				{APIVersion: configv1beta1.GroupVersion.String(), Kind: configv1beta1.ClusterProfileKind, Name: testProfileName},
+			},
+		},
+		Spec: configv1beta1.ClusterSummarySpec{
+			ClusterNamespace: testClusterNamespace, ClusterName: testClusterName, ClusterType: testClusterType,
+		},
+		Status: configv1beta1.ClusterSummaryStatus{
+			HelmReleaseSummaries: []configv1beta1.HelmChartSummary{
+				{
+					ReleaseName:      testReleaseName,
+					ReleaseNamespace: testReleaseNamespace,
+					Status:           configv1beta1.HelmChartStatusManaging,
+					ChartName:        testChartName,
+					RepoURL:          repoURL,
+					ChartVersion:     testChartVersion,
+				},
+			},
+		},
+	}
+}
+
+// markOutdatedByPreviousPass leaves the checker as a previous pass does when it finds key outdated,
+// and restores the checker when t ends.
+func markOutdatedByPreviousPass(t *testing.T, key *outdatedHelmChartMetricKey) {
+	t.Helper()
+
+	saved := previouslyOutdatedHelmCharts
+	t.Cleanup(func() {
+		previouslyOutdatedHelmCharts = saved
+		clearOutdatedHelmChart(key.profileKind, key.profileNamespace, key.profileName, key.clusterType,
+			key.clusterNamespace, key.clusterName, key.chartName, key.releaseNamespace, key.releaseName)
+	})
+
+	previouslyOutdatedHelmCharts = map[outdatedHelmChartMetricKey]struct{}{*key: {}}
+	trackOutdatedHelmChart(key.profileKind, key.profileNamespace, key.profileName, key.clusterType,
+		key.clusterNamespace, key.clusterName, key.chartName, key.releaseNamespace, key.releaseName, logr.Discard())
+}
+
+// runCheckerPass runs one checkOutdatedHelmCharts pass against a fake client holding objs.
+func runCheckerPass(t *testing.T, objs ...client.Object) {
+	t.Helper()
+
+	scheme := runtime.NewScheme()
+	if err := configv1beta1.AddToScheme(scheme); err != nil {
+		t.Fatalf("AddToScheme() = %v", err)
+	}
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(objs...).WithStatusSubresource(objs...).Build()
+
+	checkOutdatedHelmCharts(context.Background(), c, logr.Discard())
+}
+
+// outdatedHelmChartState returns whether the checker still tracks key as outdated, and whether its
+// outdated_helm_chart series is still exposed.
+func outdatedHelmChartState(t *testing.T, key *outdatedHelmChartMetricKey) (tracked, exposed bool) {
+	t.Helper()
+
+	_, tracked = previouslyOutdatedHelmCharts[*key]
+
+	want := map[string]string{
+		metricProfileKindLabel: key.profileKind, metricProfileNamespaceLabel: key.profileNamespace,
+		metricProfileNameLabel: key.profileName, metricClusterTypeLabel: key.clusterType,
+		metricClusterNamespaceLabel: key.clusterNamespace, metricClusterNameLabel: key.clusterName,
+		metricChartNameLabel: key.chartName, metricReleaseNamespaceLabel: key.releaseNamespace,
+		metricReleaseNameLabel: key.releaseName,
+	}
+
+	families, err := metrics.Registry.Gather()
+	if err != nil {
+		t.Fatalf("Gather() = %v", err)
+	}
+	for _, family := range families {
+		if family.GetName() != metricNamespace+"_outdated_helm_chart" {
+			continue
+		}
+		for _, metric := range family.GetMetric() {
+			matched := 0
+			for _, label := range metric.GetLabel() {
+				if value, ok := want[label.GetName()]; ok && value == label.GetValue() {
+					matched++
+				}
+			}
+			if matched == len(want) {
+				return tracked, true
+			}
+		}
+	}
+
+	return tracked, false
+}
+
+// A deleted ClusterSummary is never checked again, so its series must not be carried forward the
+// way the series of a chart that could not be fetched is.
+func TestCheckOutdatedHelmCharts_ClearsChartOfDeletedClusterSummary(t *testing.T) {
+	key := outdatedChartKey()
+	markOutdatedByPreviousPass(t, &key)
+
+	runCheckerPass(t)
+
+	if tracked, exposed := outdatedHelmChartState(t, &key); tracked || exposed {
+		t.Errorf("chart of a deleted ClusterSummary: tracked = %v, exposed = %v, want false, false", tracked, exposed)
+	}
+}
+
+func TestCheckOutdatedHelmCharts_ClearsChartRemovedFromProfile(t *testing.T) {
+	key := outdatedChartKey()
+	markOutdatedByPreviousPass(t, &key)
+
+	clusterSummary := clusterSummaryManagingChart(testRepoURL)
+	clusterSummary.Status.HelmReleaseSummaries = nil
+
+	runCheckerPass(t, clusterSummary)
+
+	if tracked, exposed := outdatedHelmChartState(t, &key); tracked || exposed {
+		t.Errorf("chart removed from its profile: tracked = %v, exposed = %v, want false, false", tracked, exposed)
+	}
+}
+
+// A failed fetch says nothing about the chart, so its series stays until a pass can check it.
+func TestCheckOutdatedHelmCharts_KeepsChartItCouldNotCheck(t *testing.T) {
+	key := outdatedChartKey()
+	markOutdatedByPreviousPass(t, &key)
+
+	// Nothing listens on port 1, so fetching the index fails right away.
+	runCheckerPass(t, clusterSummaryManagingChart("http://127.0.0.1:1/charts"))
+
+	if tracked, exposed := outdatedHelmChartState(t, &key); !tracked || !exposed {
+		t.Errorf("chart that could not be checked: tracked = %v, exposed = %v, want true, true", tracked, exposed)
+	}
+}
+
+func TestCheckOutdatedHelmCharts_ClearsChartThatIsUpToDate(t *testing.T) {
+	key := outdatedChartKey()
+	markOutdatedByPreviousPass(t, &key)
+
+	index := []byte(`apiVersion: v1
+entries:
+  nginx:
+  - apiVersion: v2
+    name: nginx
+    version: 1.0.0
+    urls:
+    - nginx-1.0.0.tgz
+`)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/index.yaml" {
+			http.NotFound(w, r)
+			return
+		}
+		_, _ = w.Write(index)
+	}))
+	defer server.Close()
+
+	runCheckerPass(t, clusterSummaryManagingChart(server.URL))
+
+	if tracked, exposed := outdatedHelmChartState(t, &key); tracked || exposed {
+		t.Errorf("chart that is up to date: tracked = %v, exposed = %v, want false, false", tracked, exposed)
+	}
+}

controllers/metrics.go

@@ -151,9 +151,9 @@ var (
 	)
 
 	// outdatedHelmChartGauge is 1 when a ClusterSummary's managed HelmChart release is behind
-	// the latest version published upstream. Reset() once per checkOutdatedHelmCharts pass and
-	// re-Set(1) only for entries found outdated that pass, so stale series (charts that became
-	// up to date, or are no longer referenced) don't linger. Version strings are deliberately
+	// the latest version published upstream. Set(1) for entries found outdated, and deleted by
+	// reconcileOutdatedHelmChartMetric once they are up to date or no longer referenced, so
+	// stale series don't linger. Version strings are deliberately
 	// not labels here (high cardinality/churn) — they live only on HelmChartSummary.
 	outdatedHelmChartGauge = prometheus.NewGaugeVec(
 		prometheus.GaugeOpts{