diff --git a/controllers/helmchart_outdated_check.go b/controllers/helmchart_outdated_check.go index 146e9014..cc0a1218 100644 --- a/controllers/helmchart_outdated_check.go +++ b/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) diff --git a/controllers/helmchart_outdated_check_test.go b/controllers/helmchart_outdated_check_test.go index dc3b423b..5fb66d2e 100644 --- a/controllers/helmchart_outdated_check_test.go +++ b/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) + } +} diff --git a/controllers/metrics.go b/controllers/metrics.go index 4914f3a8..41675484 100644 --- a/controllers/metrics.go +++ b/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{