From 577ea7ce8b706de79aecccdfc54cad9a313d972d Mon Sep 17 00:00:00 2001 From: Andrew Kroh Date: Thu, 3 Sep 2026 18:12:18 -0400 Subject: [PATCH] [Backups] Count backup attempts by outcome in the operator A Backup object lives two to four hours. A cleaner CronJob deletes terminal and orphaned Backups after that, and the BackupOwnerReference on Cluster collects them sooner. kube-state-metrics reports only objects that exist, so a success ratio computed from kube_customresource_cnpg_backup_info over days silently shows only the last few hours, and it counts objects, not events. Add a Prometheus counter, cnpg_backup_attempts_total, with labels phase, method, and failure_reason. Register it on controller-runtime's global registry, so the operator serves it on the metrics endpoint it already exposes. Increment it at the terminal-phase early return in the Backup reconciler, because a long-lived operator with leader election observes every Backup transition. The instance manager cannot hold this counter: it dies with its pod, so a scale-to-zero wake resets it. Do not label by cluster, namespace, or backup name: cell and region already arrive as resource attributes from the collector, and the label set needs to stay bounded regardless of how many clusters exist. A terminal Backup stays around until garbage collection removes it, so the reconciler visits it many times. Add backupAttemptDeduper, an in-memory set of already-counted Backup UIDs, so each Backup counts once. Age an entry out 24 hours after the reconciler first counts it, not by a field read off the object, so the bound does not depend on CreationTimestamp being set. An operator restart starts the set empty, so a terminal Backup still present at that point counts again once. Add a unit test that reconciles the same terminal Backup three times and asserts the counter increments once. The test needs prometheus/client_golang/prometheus/testutil, so run go mod tidy to add its one new indirect dependency to go.mod. --- go.mod | 1 + internal/controller/backup_controller.go | 2 + internal/controller/backup_controller_test.go | 39 ++++++ internal/controller/backup_metrics.go | 125 ++++++++++++++++++ 4 files changed, 167 insertions(+) create mode 100644 internal/controller/backup_metrics.go diff --git a/go.mod b/go.mod index c7ebc89f7..e33afa4f2 100644 --- a/go.mod +++ b/go.mod @@ -91,6 +91,7 @@ require ( github.com/jackc/pgservicefile v0.0.0-20240606120523-5a60cdf6a761 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect + github.com/kylelemons/godebug v1.1.0 // indirect github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de // indirect github.com/mailru/easyjson v0.9.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect diff --git a/internal/controller/backup_controller.go b/internal/controller/backup_controller.go index df141e0ec..82a0e797d 100644 --- a/internal/controller/backup_controller.go +++ b/internal/controller/backup_controller.go @@ -87,6 +87,7 @@ type BackupReconciler struct { instanceStatusClient remote.InstanceClient vsr *volumesnapshot.Reconciler + backupAttempts backupAttemptDeduper } // NewBackupReconciler properly initializes the BackupReconciler @@ -131,6 +132,7 @@ func (r *BackupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr switch backup.Status.Phase { case apiv1.BackupPhaseFailed, apiv1.BackupPhaseCompleted, apiv1.BackupPhaseCancelled: + r.recordBackupAttempt(&backup) return ctrl.Result{}, nil } diff --git a/internal/controller/backup_controller_test.go b/internal/controller/backup_controller_test.go index 4562b7300..885ebc10d 100644 --- a/internal/controller/backup_controller_test.go +++ b/internal/controller/backup_controller_test.go @@ -24,9 +24,11 @@ import ( "time" volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + "github.com/prometheus/client_golang/prometheus/testutil" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/utils/ptr" + ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -730,3 +732,40 @@ var _ = Describe("waitForBackupPod", func() { Expect(stored.Status.Phase).To(BeEquivalentTo(apiv1.BackupPhaseFailed)) }) }) + +var _ = Describe("backup attempt metric dedupe", func() { + var env *testingEnvironment + BeforeEach(func() { env = buildTestEnvironment() }) + + It("counts a terminal backup once no matter how many times it is reconciled", func(ctx context.Context) { + ns := newFakeNamespace(env.client) + + backup := &apiv1.Backup{ + ObjectMeta: metav1.ObjectMeta{Name: "backup-dedupe-test", Namespace: ns}, + Spec: apiv1.BackupSpec{ + Cluster: apiv1.LocalObjectReference{Name: "cluster-example"}, + Method: apiv1.BackupMethodPgBackRest, + }, + Status: apiv1.BackupStatus{ + Phase: apiv1.BackupPhaseCompleted, + Method: apiv1.BackupMethodPgBackRest, + }, + } + Expect(env.client.Create(ctx, backup)).To(Succeed()) + + counter := backupAttemptsTotal.WithLabelValues( + string(apiv1.BackupPhaseCompleted), string(apiv1.BackupMethodPgBackRest), "") + before := testutil.ToFloat64(counter) + + req := ctrl.Request{NamespacedName: client.ObjectKeyFromObject(backup)} + + _, err := env.backupReconciler.Reconcile(ctx, req) + Expect(err).ToNot(HaveOccurred()) + _, err = env.backupReconciler.Reconcile(ctx, req) + Expect(err).ToNot(HaveOccurred()) + _, err = env.backupReconciler.Reconcile(ctx, req) + Expect(err).ToNot(HaveOccurred()) + + Expect(testutil.ToFloat64(counter) - before).To(Equal(1.0)) + }) +}) diff --git a/internal/controller/backup_metrics.go b/internal/controller/backup_metrics.go new file mode 100644 index 000000000..a4484374c --- /dev/null +++ b/internal/controller/backup_metrics.go @@ -0,0 +1,125 @@ +/* +Copyright © contributors to CloudNativePG, established as +CloudNativePG a Series of LF Projects, LLC. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. + +SPDX-License-Identifier: Apache-2.0 +*/ + +package controller + +import ( + "sync" + "time" + + "github.com/prometheus/client_golang/prometheus" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/metrics" + + apiv1 "github.com/xataio/xata-cnpg/api/v1" +) + +// backupAttemptDedupeTTL bounds how long a Backup UID is remembered for +// dedupe purposes. The cleaner CronJob removes terminal Backups within two +// hours, and BackupOwnerReference often collects them with their Cluster +// sooner than that, so a UID observed longer ago than this TTL belongs to an +// object that is gone (or is about to be) and can safely be forgotten even +// if, improbably, it is still being reconciled. +const backupAttemptDedupeTTL = 24 * time.Hour + +// backupAttemptsTotal counts terminal Backup reconciliations by outcome, so +// an attempt success ratio can be computed over a window much longer than a +// Backup object's own lifetime (it is garbage collected within hours). It is +// registered on controller-runtime's global registry, so it is served on the +// metrics endpoint the operator already exposes. +// +// failure_reason is empty for a completed backup; that is expected, not +// missing data. +var backupAttemptsTotal = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: "cnpg", + Subsystem: "backup", + Name: "attempts_total", + Help: "Total number of Backup objects that reached a terminal phase, labeled by outcome. " + + "Counted once per Backup UID regardless of how many times it is reconciled afterwards.", + }, + []string{"phase", "method", "failure_reason"}, +) + +func init() { + metrics.Registry.MustRegister(backupAttemptsTotal) +} + +// backupAttemptDeduper remembers which terminal Backup UIDs have already +// been counted, so a Backup reconciled repeatedly after reaching a terminal +// phase (which is the normal case: it sits around until the cleaner CronJob +// or its Cluster's garbage collection removes it) is only counted once. +// +// The zero value is ready to use. +// +// Known and accepted imprecision: an operator restart starts this map empty, +// so any terminal Backup still present in the cluster is re-counted once +// after a restart. This is rare relative to the volume of attempts a ratio +// is computed over, and avoids the complexity of persisting dedupe state +// somewhere outside the process. +type backupAttemptDeduper struct { + mu sync.Mutex + counted map[types.UID]time.Time +} + +// observeTerminal records that backup reached a terminal phase and reports +// whether this call is the first to do so for its UID. An entry ages out +// backupAttemptDedupeTTL after it was first counted; that is deliberately +// the observation time, not a field read off the object (e.g. its creation +// timestamp), so the bound does not depend on any timestamp the object +// happens to carry being populated. +func (d *backupAttemptDeduper) observeTerminal(backup *apiv1.Backup) bool { + d.mu.Lock() + defer d.mu.Unlock() + + if d.counted == nil { + d.counted = make(map[types.UID]time.Time) + } + + d.evictLocked() + + if _, ok := d.counted[backup.UID]; ok { + return false + } + d.counted[backup.UID] = time.Now() + return true +} + +func (d *backupAttemptDeduper) evictLocked() { + cutoff := time.Now().Add(-backupAttemptDedupeTTL) + for uid, createdAt := range d.counted { + if createdAt.Before(cutoff) { + delete(d.counted, uid) + } + } +} + +// recordBackupAttempt increments backupAttemptsTotal for backup's terminal +// outcome, once per Backup UID. +func (r *BackupReconciler) recordBackupAttempt(backup *apiv1.Backup) { + if !r.backupAttempts.observeTerminal(backup) { + return + } + + backupAttemptsTotal.WithLabelValues( + string(backup.Status.Phase), + string(backup.Status.Method), + string(backup.Status.FailureReason), + ).Inc() +}