From e7d7681b1b8c9be5c22e6746115d783a23d2d196 Mon Sep 17 00:00:00 2001 From: Sunny Date: Fri, 11 Aug 2023 13:56:07 +0000 Subject: [PATCH] Delete stale metrics on object delete Move record suspend metrics next to readiness and duration metrics so that it gets recorded along with others always at the end and the metrics delete, which requires the knowledge of deleted finalizers, applies to suspend too. HelmRepository cache event metrics for a given helmrepo also continues to be exported even after the object is deleted. This change deletes the cache event metrics when the object is deleted. Signed-off-by: Sunny --- go.mod | 2 +- go.sum | 4 ++-- internal/cache/metrics.go | 5 +++++ internal/controller/bucket_controller.go | 6 ++---- internal/controller/gitrepository_controller.go | 6 ++---- internal/controller/helmchart_controller.go | 6 ++---- internal/controller/helmrepository_controller.go | 12 ++++++++---- internal/controller/helmrepository_controller_oci.go | 6 ++---- internal/controller/ocirepository_controller.go | 6 ++---- internal/controller/suite_test.go | 3 ++- main.go | 3 ++- 11 files changed, 30 insertions(+), 29 deletions(-) diff --git a/go.mod b/go.mod index 5065e815..78cac957 100644 --- a/go.mod +++ b/go.mod @@ -34,7 +34,7 @@ require ( github.com/fluxcd/pkg/lockedfile v0.1.0 github.com/fluxcd/pkg/masktoken v0.2.0 github.com/fluxcd/pkg/oci v0.30.1 - github.com/fluxcd/pkg/runtime v0.41.0 + github.com/fluxcd/pkg/runtime v0.42.0 github.com/fluxcd/pkg/sourceignore v0.3.5 github.com/fluxcd/pkg/ssh v0.8.1 github.com/fluxcd/pkg/tar v0.2.0 diff --git a/go.sum b/go.sum index d1e64cd8..9210fcc5 100644 --- a/go.sum +++ b/go.sum @@ -407,8 +407,8 @@ github.com/fluxcd/pkg/masktoken v0.2.0 h1:HoSPTk4l1fz5Fevs2vVRvZGru33blfMwWSZKsH github.com/fluxcd/pkg/masktoken v0.2.0/go.mod h1:EA7GleAHL33kN6kTW06m5R3/Q26IyuGO7Ef/0CtpDI0= github.com/fluxcd/pkg/oci v0.30.1 h1:XRCWzufSRtI6g6TvCH8pJHIqw9qXUf2+9fBH8pOpoU0= github.com/fluxcd/pkg/oci v0.30.1/go.mod h1:HAWYIdzEbCnAT7Me2YGVUlgA5y/CCBdJ0+tFdEOb2nI= -github.com/fluxcd/pkg/runtime v0.41.0 h1:hjWUwVRCKDuGEUhovWrygt/6PRry4p278yKuJNgTfv8= -github.com/fluxcd/pkg/runtime v0.41.0/go.mod h1:1GN+nxoQ7LmSsLJwjH8JW8pA27tBSO+KLH43HpywCDM= +github.com/fluxcd/pkg/runtime v0.42.0 h1:a5DQ/f90YjoHBmiXZUpnp4bDSLORjInbmqP7K11L4uY= +github.com/fluxcd/pkg/runtime v0.42.0/go.mod h1:p6A3xWVV8cKLLQW0N90GehKgGMMmbNYv+OSJ/0qB0vg= github.com/fluxcd/pkg/sourceignore v0.3.5 h1:omcHTH5X5tlPr9w1b9T7WuJTOP+o/KdVdarYb4kgkCU= github.com/fluxcd/pkg/sourceignore v0.3.5/go.mod h1:6Xz3jErz8RsidsdrjUBBUGKes24rbdp/F38MnTGibEw= github.com/fluxcd/pkg/ssh v0.8.1 h1:v35y7Ks/+ABWce8RcnrC7psVIhf3EdCUNFJi5+tYOps= diff --git a/internal/cache/metrics.go b/internal/cache/metrics.go index bf12e73d..09b43ec5 100644 --- a/internal/cache/metrics.go +++ b/internal/cache/metrics.go @@ -67,6 +67,11 @@ func (r *CacheRecorder) IncCacheEvents(event, name, namespace string) { r.cacheEventsCounter.WithLabelValues(event, name, namespace).Inc() } +// DeleteCacheEvent deletes the cache event metric. +func (r *CacheRecorder) DeleteCacheEvent(event, name, namespace string) { + r.cacheEventsCounter.DeleteLabelValues(event, name, namespace) +} + // MustMakeMetrics creates a new CacheRecorder, and registers the metrics collectors in the controller-runtime metrics registry. func MustMakeMetrics() *CacheRecorder { r := NewCacheRecorder() diff --git a/internal/controller/bucket_controller.go b/internal/controller/bucket_controller.go index 8180ebf6..521fb254 100644 --- a/internal/controller/bucket_controller.go +++ b/internal/controller/bucket_controller.go @@ -184,9 +184,6 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res return ctrl.Result{}, client.IgnoreNotFound(err) } - // Record suspended status metric - r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) @@ -213,7 +210,8 @@ func (r *BucketReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res } result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) - // Always record readiness and duration metrics + // Always record suspend, readiness and duration metrics. + r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) r.Metrics.RecordReadiness(ctx, obj) r.Metrics.RecordDuration(ctx, obj, start) }() diff --git a/internal/controller/gitrepository_controller.go b/internal/controller/gitrepository_controller.go index e74ed34a..219663df 100644 --- a/internal/controller/gitrepository_controller.go +++ b/internal/controller/gitrepository_controller.go @@ -177,9 +177,6 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, client.IgnoreNotFound(err) } - // Record suspended status metric - r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) @@ -207,7 +204,8 @@ func (r *GitRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques } result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) - // Always record readiness and duration metrics + // Always record suspend, readiness and duration metrics. + r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) r.Metrics.RecordReadiness(ctx, obj) r.Metrics.RecordDuration(ctx, obj, start) }() diff --git a/internal/controller/helmchart_controller.go b/internal/controller/helmchart_controller.go index 74651ef8..35a896f9 100644 --- a/internal/controller/helmchart_controller.go +++ b/internal/controller/helmchart_controller.go @@ -198,9 +198,6 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( return ctrl.Result{}, client.IgnoreNotFound(err) } - // Record suspended status metric - r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) @@ -228,7 +225,8 @@ func (r *HelmChartReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) - // Always record readiness and duration metrics + // Always record suspend, readiness and duration metrics. + r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) r.Metrics.RecordReadiness(ctx, obj) r.Metrics.RecordDuration(ctx, obj, start) }() diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 91275e73..eb871a1f 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -161,9 +161,6 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque return ctrl.Result{}, client.IgnoreNotFound(err) } - // Record suspended status metric - r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) @@ -190,7 +187,8 @@ func (r *HelmRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reque } result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) - // Always record readiness and duration metrics + // Always record suspend, readiness and duration metrics. + r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) r.Metrics.RecordReadiness(ctx, obj) r.Metrics.RecordDuration(ctx, obj, start) }() @@ -622,6 +620,12 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *hel controllerutil.RemoveFinalizer(obj, sourcev1.SourceFinalizer) } + // Delete cache metrics. + if r.CacheRecorder != nil && r.Metrics.IsDelete(obj) { + r.DeleteCacheEvent(cache.CacheEventTypeHit, obj.Name, obj.Namespace) + r.DeleteCacheEvent(cache.CacheEventTypeMiss, obj.Name, obj.Namespace) + } + // Stop reconciliation as the object is being deleted return sreconcile.ResultEmpty, nil } diff --git a/internal/controller/helmrepository_controller_oci.go b/internal/controller/helmrepository_controller_oci.go index 07a2c837..e25eaf4f 100644 --- a/internal/controller/helmrepository_controller_oci.go +++ b/internal/controller/helmrepository_controller_oci.go @@ -136,9 +136,6 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{RequeueAfter: time.Second}, nil } - // Record suspended status metric - r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) @@ -167,7 +164,8 @@ func (r *HelmRepositoryOCIReconciler) Reconcile(ctx context.Context, req ctrl.Re retErr = kerrors.NewAggregate([]error{retErr, err}) } - // Always record readiness and duration metrics + // Always record suspend, readiness and duration metrics. + r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) r.Metrics.RecordReadiness(ctx, obj) r.Metrics.RecordDuration(ctx, obj, start) }() diff --git a/internal/controller/ocirepository_controller.go b/internal/controller/ocirepository_controller.go index 20663c68..23939ecb 100644 --- a/internal/controller/ocirepository_controller.go +++ b/internal/controller/ocirepository_controller.go @@ -178,9 +178,6 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, client.IgnoreNotFound(err) } - // Record suspended status metric - r.RecordSuspend(ctx, obj, obj.Spec.Suspend) - // Initialize the patch helper with the current version of the object. serialPatcher := patch.NewSerialPatcher(obj, r.Client) @@ -208,7 +205,8 @@ func (r *OCIRepositoryReconciler) Reconcile(ctx context.Context, req ctrl.Reques } result, retErr = summarizeHelper.SummarizeAndPatch(ctx, obj, summarizeOpts...) - // Always record readiness and duration metrics + // Always record suspend, readiness and duration metrics. + r.Metrics.RecordSuspend(ctx, obj, obj.Spec.Suspend) r.Metrics.RecordReadiness(ctx, obj) r.Metrics.RecordDuration(ctx, obj, start) }() diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 6b8e4b99..2429b58a 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -52,6 +52,7 @@ import ( _ "github.com/distribution/distribution/v3/registry/storage/driver/inmemory" "github.com/fluxcd/pkg/runtime/controller" + "github.com/fluxcd/pkg/runtime/metrics" "github.com/fluxcd/pkg/runtime/testenv" "github.com/fluxcd/pkg/testserver" @@ -310,7 +311,7 @@ func TestMain(m *testing.M) { panic(fmt.Sprintf("Failed to create a test storage: %v", err)) } - testMetricsH = controller.MustMakeMetrics(testEnv) + testMetricsH = controller.NewMetrics(testEnv, metrics.MustMakeRecorder(), sourcev1.SourceFinalizer) testWorkspaceDir, err := os.MkdirTemp("", "registry-test-") if err != nil { diff --git a/main.go b/main.go index 762fef71..a7918634 100644 --- a/main.go +++ b/main.go @@ -45,6 +45,7 @@ import ( "github.com/fluxcd/pkg/runtime/jitter" "github.com/fluxcd/pkg/runtime/leaderelection" "github.com/fluxcd/pkg/runtime/logger" + "github.com/fluxcd/pkg/runtime/metrics" "github.com/fluxcd/pkg/runtime/pprof" "github.com/fluxcd/pkg/runtime/probes" @@ -178,7 +179,7 @@ func main() { probes.SetupChecks(mgr, setupLog) pprof.SetupHandlers(mgr, setupLog) - metrics := helper.MustMakeMetrics(mgr) + metrics := helper.NewMetrics(mgr, metrics.MustMakeRecorder(), v1.SourceFinalizer) cacheRecorder := cache.MustMakeMetrics() eventRecorder := mustSetupEventRecorder(mgr, eventsAddr, controllerName) storage := mustInitStorage(storagePath, storageAdvAddr, artifactRetentionTTL, artifactRetentionRecords, artifactDigestAlgo)