From 1968bf44ee90ec9fa6909e65ed5d7121308a8058 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 14 Jul 2026 09:42:55 -0700 Subject: [PATCH 1/9] Add ResetBackupLastSuccessfulTimestamp to ServerMetrics Add a method to reset all backupLastSuccessfulTimestamp gauge values. This will be used by the backup controller's periodic resync to prune stale metrics for deleted schedules. Fixes #9239 Signed-off-by: Shubham Pampattiwar --- pkg/metrics/metrics.go | 7 +++++++ pkg/metrics/metrics_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 86d78028c..d54eb02b5 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -758,6 +758,13 @@ func (m *ServerMetrics) RegisterPodVolumeOpLatencyGauge(node, pvbName, opName, b } } +// ResetBackupLastSuccessfulTimestamp removes all schedule-level backupLastSuccessfulTimestamp values. +func (m *ServerMetrics) ResetBackupLastSuccessfulTimestamp() { + if g, ok := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec); ok { + g.Reset() + } +} + // SetBackupTarballSizeBytesGauge records the size, in bytes, of a backup tarball. func (m *ServerMetrics) SetBackupTarballSizeBytesGauge(backupSchedule string, size int64) { if g, ok := m.metrics[backupTarballSizeBytesGauge].(*prometheus.GaugeVec); ok { diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index a24f2bf33..07004f172 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -457,6 +457,38 @@ func getHistogramCount(t *testing.T, vec *prometheus.HistogramVec, scheduleLabel return 0 } +// TestResetBackupLastSuccessfulTimestamp verifies that ResetBackupLastSuccessfulTimestamp +// removes all schedule-level values from the backupLastSuccessfulTimestamp gauge. +func TestResetBackupLastSuccessfulTimestamp(t *testing.T) { + m := NewServerMetrics() + + now := time.Now() + m.SetBackupLastSuccessfulTimestamp("schedule-1", now) + m.SetBackupLastSuccessfulTimestamp("schedule-2", now.Add(-time.Hour)) + m.SetBackupLastSuccessfulTimestamp("", now.Add(-2*time.Hour)) + + // Verify all three entries exist + g := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec) + assert.Equal(t, 3, collectGaugeCount(t, g)) + + // Reset should remove all entries + m.ResetBackupLastSuccessfulTimestamp() + assert.Equal(t, 0, collectGaugeCount(t, g)) +} + +// collectGaugeCount returns the number of time series in a GaugeVec. +func collectGaugeCount(t *testing.T, g *prometheus.GaugeVec) int { + t.Helper() + ch := make(chan prometheus.Metric, 10) + g.Collect(ch) + close(ch) + count := 0 + for range ch { + count++ + } + return count +} + // TestRepoMaintenanceMetrics verifies that repo maintenance metrics are properly recorded. func TestRepoMaintenanceMetrics(t *testing.T) { tests := []struct { From 8cf03998ddfc643830195128d0c72785ed24cfd0 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 14 Jul 2026 09:47:39 -0700 Subject: [PATCH 2/9] Reset backupLastSuccessfulTimestamp during periodic resync The periodic backup metrics resync in updateTotalBackupMetric only set backupLastSuccessfulTimestamp values but never removed stale entries. When a schedule was deleted and its backups removed, the gauge persisted until the Velero pod was restarted. Reset the gauge before re-setting current values so that deleted schedules are pruned automatically each resync cycle. Fixes #9239 Signed-off-by: Shubham Pampattiwar --- pkg/controller/backup_controller.go | 4 ++- pkg/controller/backup_controller_test.go | 32 ++++++++++++++++++++++++ pkg/metrics/metrics.go | 17 +++++++++++++ 3 files changed, 52 insertions(+), 1 deletion(-) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 74b857fd2..7a58424eb 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -215,7 +215,9 @@ func (b *backupReconciler) updateTotalBackupMetric() { } // recompute backup_last_successful_timestamp metric for each - // schedule (including the empty schedule, i.e. ad-hoc backups) + // schedule (including the empty schedule, i.e. ad-hoc backups). + // Reset first to prune stale entries for deleted schedules. + b.metrics.ResetBackupLastSuccessfulTimestamp() for schedule, timestamp := range getLastSuccessBySchedule(backups.Items) { b.metrics.SetBackupLastSuccessfulTimestamp(schedule, timestamp) } diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index bab98efb6..3a1903e0f 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -2041,6 +2041,38 @@ func Test_getLastSuccessBySchedule(t *testing.T) { } } +// Test_updateTotalBackupMetric_prunesStaleTimestamps verifies that the periodic +// resync removes backupLastSuccessfulTimestamp entries for schedules that no longer +// have any completed backups (e.g. after the schedule and its backups are deleted). +func Test_updateTotalBackupMetric_prunesStaleTimestamps(t *testing.T) { + baseTime, err := time.Parse(time.RFC1123, time.RFC1123) + require.NoError(t, err) + + m := metrics.NewServerMetrics() + + // Simulate a previous resync that set the metric for "deleted-schedule" + m.SetBackupLastSuccessfulTimestamp("deleted-schedule", baseTime) + require.Equal(t, 1, m.BackupLastSuccessfulTimestampCount()) + + // Current backups only contain entries for "active-schedule" + backups := []velerov1api.Backup{ + *builder.ForBackup("velero", "b1"). + ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "active-schedule")). + Phase(velerov1api.BackupPhaseCompleted). + CompletionTimestamp(baseTime). + Result(), + } + + // Replicate the resync logic: reset then set + m.ResetBackupLastSuccessfulTimestamp() + for schedule, timestamp := range getLastSuccessBySchedule(backups) { + m.SetBackupLastSuccessfulTimestamp(schedule, timestamp) + } + + // Only "active-schedule" should remain; "deleted-schedule" should be pruned + assert.Equal(t, 1, m.BackupLastSuccessfulTimestampCount()) +} + // Unit tests to make sure that the backup's status is updated correctly during reconcile. // To clear up confusion whether status can be updated with Patch alone without status writer and not kbClient.Status().Patch() func TestPatchResourceWorksWithStatus(t *testing.T) { diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index d54eb02b5..d95867fd1 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -765,6 +765,23 @@ func (m *ServerMetrics) ResetBackupLastSuccessfulTimestamp() { } } +// BackupLastSuccessfulTimestampCount returns the number of active time series +// in the backupLastSuccessfulTimestamp gauge. +func (m *ServerMetrics) BackupLastSuccessfulTimestampCount() int { + g, ok := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec) + if !ok { + return 0 + } + ch := make(chan prometheus.Metric, 100) + g.Collect(ch) + close(ch) + count := 0 + for range ch { + count++ + } + return count +} + // SetBackupTarballSizeBytesGauge records the size, in bytes, of a backup tarball. func (m *ServerMetrics) SetBackupTarballSizeBytesGauge(backupSchedule string, size int64) { if g, ok := m.metrics[backupTarballSizeBytesGauge].(*prometheus.GaugeVec); ok { From 4357ad89767ca0f968156658617d6f4e81eb6e65 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 14 Jul 2026 09:48:49 -0700 Subject: [PATCH 3/9] Add changelog for PR #10000 Signed-off-by: Shubham Pampattiwar --- changelogs/unreleased/10000-shubham-pampattiwar | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10000-shubham-pampattiwar diff --git a/changelogs/unreleased/10000-shubham-pampattiwar b/changelogs/unreleased/10000-shubham-pampattiwar new file mode 100644 index 000000000..4134b77e2 --- /dev/null +++ b/changelogs/unreleased/10000-shubham-pampattiwar @@ -0,0 +1 @@ +Fix stale backupLastSuccessfulTimestamp metric after schedule deletion From 6ea95548d69e41744467ed69c634ba7ba8f30ad5 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Mon, 20 Jul 2026 06:10:53 -0700 Subject: [PATCH 4/9] Remove BackupLastSuccessfulTimestampCount, use Metrics() in test Remove the exported method that was only used in tests. Use the existing Metrics() getter to access the gauge directly in the backup controller test instead. Signed-off-by: Shubham Pampattiwar --- pkg/controller/backup_controller_test.go | 18 ++++++++++++++++-- pkg/metrics/metrics.go | 17 ----------------- 2 files changed, 16 insertions(+), 19 deletions(-) diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 3a1903e0f..3aafa2c71 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -31,6 +31,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" + "github.com/prometheus/client_golang/prometheus" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -2049,10 +2050,11 @@ func Test_updateTotalBackupMetric_prunesStaleTimestamps(t *testing.T) { require.NoError(t, err) m := metrics.NewServerMetrics() + gauge := m.Metrics()["backup_last_successful_timestamp"].(*prometheus.GaugeVec) // Simulate a previous resync that set the metric for "deleted-schedule" m.SetBackupLastSuccessfulTimestamp("deleted-schedule", baseTime) - require.Equal(t, 1, m.BackupLastSuccessfulTimestampCount()) + require.Equal(t, 1, collectGaugeCount(t, gauge)) // Current backups only contain entries for "active-schedule" backups := []velerov1api.Backup{ @@ -2070,7 +2072,19 @@ func Test_updateTotalBackupMetric_prunesStaleTimestamps(t *testing.T) { } // Only "active-schedule" should remain; "deleted-schedule" should be pruned - assert.Equal(t, 1, m.BackupLastSuccessfulTimestampCount()) + assert.Equal(t, 1, collectGaugeCount(t, gauge)) +} + +func collectGaugeCount(t *testing.T, g *prometheus.GaugeVec) int { + t.Helper() + ch := make(chan prometheus.Metric, 10) + g.Collect(ch) + close(ch) + count := 0 + for range ch { + count++ + } + return count } // Unit tests to make sure that the backup's status is updated correctly during reconcile. diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index d95867fd1..d54eb02b5 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -765,23 +765,6 @@ func (m *ServerMetrics) ResetBackupLastSuccessfulTimestamp() { } } -// BackupLastSuccessfulTimestampCount returns the number of active time series -// in the backupLastSuccessfulTimestamp gauge. -func (m *ServerMetrics) BackupLastSuccessfulTimestampCount() int { - g, ok := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec) - if !ok { - return 0 - } - ch := make(chan prometheus.Metric, 100) - g.Collect(ch) - close(ch) - count := 0 - for range ch { - count++ - } - return count -} - // SetBackupTarballSizeBytesGauge records the size, in bytes, of a backup tarball. func (m *ServerMetrics) SetBackupTarballSizeBytesGauge(backupSchedule string, size int64) { if g, ok := m.metrics[backupTarballSizeBytesGauge].(*prometheus.GaugeVec); ok { From bfeccba0a84958943aae709766c663f73950160d Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Mon, 20 Jul 2026 12:32:02 -0700 Subject: [PATCH 5/9] Move metric reset inside List success block Avoid clearing backupLastSuccessfulTimestamp on transient API errors. The reset and re-set now only run when the backup List call succeeds, so existing metric values remain stable across temporary failures. Signed-off-by: Shubham Pampattiwar --- pkg/controller/backup_controller.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 7a58424eb..0e2fb1384 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -212,14 +212,14 @@ func (b *backupReconciler) updateTotalBackupMetric() { b.logger.Error(err, "Error computing backup_total metric") } else { b.metrics.SetBackupTotal(int64(len(backups.Items))) - } - // recompute backup_last_successful_timestamp metric for each - // schedule (including the empty schedule, i.e. ad-hoc backups). - // Reset first to prune stale entries for deleted schedules. - b.metrics.ResetBackupLastSuccessfulTimestamp() - for schedule, timestamp := range getLastSuccessBySchedule(backups.Items) { - b.metrics.SetBackupLastSuccessfulTimestamp(schedule, timestamp) + // recompute backup_last_successful_timestamp metric for each + // schedule (including the empty schedule, i.e. ad-hoc backups). + // Reset first to prune stale entries for deleted schedules. + b.metrics.ResetBackupLastSuccessfulTimestamp() + for schedule, timestamp := range getLastSuccessBySchedule(backups.Items) { + b.metrics.SetBackupLastSuccessfulTimestamp(schedule, timestamp) + } } }, backupResyncPeriod, From b9d9dcfc385583d98fec0f7c1722344168c25327 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Tue, 21 Jul 2026 10:13:12 -0700 Subject: [PATCH 6/9] Add integration test for updateTotalBackupMetric resync Add a test that exercises the actual updateTotalBackupMetric goroutine with a fake client to verify stale backupLastSuccessfulTimestamp entries are pruned during a real resync cycle. Signed-off-by: Shubham Pampattiwar --- pkg/controller/backup_controller_test.go | 39 ++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 3aafa2c71..9f6bf1a28 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -18,6 +18,7 @@ package controller import ( "bytes" + "context" "fmt" "io" "reflect" @@ -2087,6 +2088,44 @@ func collectGaugeCount(t *testing.T, g *prometheus.GaugeVec) int { return count } +// Test_updateTotalBackupMetric_prunesStaleTimestamps_integration tests the actual +// updateTotalBackupMetric goroutine with a fake client to verify stale metrics are +// pruned during a real resync cycle. +func Test_updateTotalBackupMetric_prunesStaleTimestamps_integration(t *testing.T) { + baseTime, err := time.Parse(time.RFC1123, time.RFC1123) + require.NoError(t, err) + + m := metrics.NewServerMetrics() + gauge := m.Metrics()["backup_last_successful_timestamp"].(*prometheus.GaugeVec) + + activeBackup := builder.ForBackup("velero", "b1"). + ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "active-schedule")). + Phase(velerov1api.BackupPhaseCompleted). + CompletionTimestamp(baseTime). + Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, activeBackup) + + m.SetBackupLastSuccessfulTimestamp("deleted-schedule", baseTime) + require.Equal(t, 1, collectGaugeCount(t, gauge)) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + c := &backupReconciler{ + ctx: ctx, + kbClient: fakeClient, + logger: logrus.StandardLogger(), + metrics: m, + } + + c.updateTotalBackupMetric() + time.Sleep(7 * time.Second) + cancel() + + assert.Equal(t, 1, collectGaugeCount(t, gauge)) +} + // Unit tests to make sure that the backup's status is updated correctly during reconcile. // To clear up confusion whether status can be updated with Patch alone without status writer and not kbClient.Status().Patch() func TestPatchResourceWorksWithStatus(t *testing.T) { From 4ea38216f5d1e09a400c30e3e5977ba0dfb76346 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Wed, 22 Jul 2026 09:16:24 -0700 Subject: [PATCH 7/9] Address review feedback: use DeleteLabelValues, extract resync method Replace blanket Reset() with targeted DeleteLabelValues to avoid briefly wiping metrics for schedules that still exist. Track known schedules in a set and only delete stale entries on each resync. Extract the wait.Until closure into resyncBackupMetrics() so tests can call it directly without goroutine timing. Replace hand-rolled collectGaugeCount helper with testutil.CollectAndCount. Signed-off-by: Shubham Pampattiwar --- pkg/controller/backup_controller.go | 58 +++++++++------- pkg/controller/backup_controller_test.go | 84 ++++++------------------ pkg/metrics/metrics.go | 7 +- pkg/metrics/metrics_test.go | 21 +++--- 4 files changed, 74 insertions(+), 96 deletions(-) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 0e2fb1384..01a660dad 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -107,10 +107,11 @@ type backupReconciler struct { credentialFileStore credentials.FileStore maxConcurrentK8SConnections int defaultSnapshotMoveData bool - globalCRClient kbclient.Client - itemBlockWorkerCount int - concurrentBackups int - globalVolumePoliciesConfigMap string + globalCRClient kbclient.Client + itemBlockWorkerCount int + concurrentBackups int + globalVolumePoliciesConfigMap string + knownSchedulesWithSuccessfulBackup sets.Set[string] } func NewBackupReconciler( @@ -204,30 +205,43 @@ func (b *backupReconciler) updateTotalBackupMetric() { time.Sleep(5 * time.Second) wait.Until( - func() { - // recompute backup_total metric - backups := &velerov1api.BackupList{} - err := b.kbClient.List(context.Background(), backups, &kbclient.ListOptions{LabelSelector: labels.Everything()}) - if err != nil { - b.logger.Error(err, "Error computing backup_total metric") - } else { - b.metrics.SetBackupTotal(int64(len(backups.Items))) - - // recompute backup_last_successful_timestamp metric for each - // schedule (including the empty schedule, i.e. ad-hoc backups). - // Reset first to prune stale entries for deleted schedules. - b.metrics.ResetBackupLastSuccessfulTimestamp() - for schedule, timestamp := range getLastSuccessBySchedule(backups.Items) { - b.metrics.SetBackupLastSuccessfulTimestamp(schedule, timestamp) - } - } - }, + b.resyncBackupMetrics, backupResyncPeriod, b.ctx.Done(), ) }() } +func (b *backupReconciler) resyncBackupMetrics() { + backups := &velerov1api.BackupList{} + err := b.kbClient.List(context.Background(), backups, &kbclient.ListOptions{LabelSelector: labels.Everything()}) + if err != nil { + b.logger.Error(err, "Error computing backup_total metric") + return + } + + b.metrics.SetBackupTotal(int64(len(backups.Items))) + + currentSchedules := getLastSuccessBySchedule(backups.Items) + for schedule, timestamp := range currentSchedules { + b.metrics.SetBackupLastSuccessfulTimestamp(schedule, timestamp) + } + + // Remove metrics for schedules that no longer have successful backups + if b.knownSchedulesWithSuccessfulBackup != nil { + for schedule := range b.knownSchedulesWithSuccessfulBackup { + if _, exists := currentSchedules[schedule]; !exists { + b.metrics.DeleteBackupLastSuccessfulTimestamp(schedule) + } + } + } + + b.knownSchedulesWithSuccessfulBackup = sets.New[string]() + for schedule := range currentSchedules { + b.knownSchedulesWithSuccessfulBackup.Insert(schedule) + } +} + // getLastSuccessBySchedule finds the most recent completed backup for each schedule // and returns a map of schedule name -> completion time of the most recent completed // backup. This map includes an entry for ad-hoc/non-scheduled backups, where the key diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 9f6bf1a28..b86434796 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -18,7 +18,6 @@ package controller import ( "bytes" - "context" "fmt" "io" "reflect" @@ -32,7 +31,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1" - "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -2043,60 +2042,15 @@ func Test_getLastSuccessBySchedule(t *testing.T) { } } -// Test_updateTotalBackupMetric_prunesStaleTimestamps verifies that the periodic -// resync removes backupLastSuccessfulTimestamp entries for schedules that no longer -// have any completed backups (e.g. after the schedule and its backups are deleted). -func Test_updateTotalBackupMetric_prunesStaleTimestamps(t *testing.T) { +// Test_resyncBackupMetrics_prunesStaleTimestamps verifies that resyncBackupMetrics +// removes backupLastSuccessfulTimestamp entries for schedules that no longer have +// any completed backups (e.g. after the schedule and its backups are deleted). +func Test_resyncBackupMetrics_prunesStaleTimestamps(t *testing.T) { baseTime, err := time.Parse(time.RFC1123, time.RFC1123) require.NoError(t, err) m := metrics.NewServerMetrics() - gauge := m.Metrics()["backup_last_successful_timestamp"].(*prometheus.GaugeVec) - - // Simulate a previous resync that set the metric for "deleted-schedule" - m.SetBackupLastSuccessfulTimestamp("deleted-schedule", baseTime) - require.Equal(t, 1, collectGaugeCount(t, gauge)) - - // Current backups only contain entries for "active-schedule" - backups := []velerov1api.Backup{ - *builder.ForBackup("velero", "b1"). - ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "active-schedule")). - Phase(velerov1api.BackupPhaseCompleted). - CompletionTimestamp(baseTime). - Result(), - } - - // Replicate the resync logic: reset then set - m.ResetBackupLastSuccessfulTimestamp() - for schedule, timestamp := range getLastSuccessBySchedule(backups) { - m.SetBackupLastSuccessfulTimestamp(schedule, timestamp) - } - - // Only "active-schedule" should remain; "deleted-schedule" should be pruned - assert.Equal(t, 1, collectGaugeCount(t, gauge)) -} - -func collectGaugeCount(t *testing.T, g *prometheus.GaugeVec) int { - t.Helper() - ch := make(chan prometheus.Metric, 10) - g.Collect(ch) - close(ch) - count := 0 - for range ch { - count++ - } - return count -} - -// Test_updateTotalBackupMetric_prunesStaleTimestamps_integration tests the actual -// updateTotalBackupMetric goroutine with a fake client to verify stale metrics are -// pruned during a real resync cycle. -func Test_updateTotalBackupMetric_prunesStaleTimestamps_integration(t *testing.T) { - baseTime, err := time.Parse(time.RFC1123, time.RFC1123) - require.NoError(t, err) - - m := metrics.NewServerMetrics() - gauge := m.Metrics()["backup_last_successful_timestamp"].(*prometheus.GaugeVec) + gauge := m.Metrics()["backup_last_successful_timestamp"] activeBackup := builder.ForBackup("velero", "b1"). ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "active-schedule")). @@ -2104,26 +2058,30 @@ func Test_updateTotalBackupMetric_prunesStaleTimestamps_integration(t *testing.T CompletionTimestamp(baseTime). Result() - fakeClient := velerotest.NewFakeControllerRuntimeClient(t, activeBackup) + deletedBackup := builder.ForBackup("velero", "b2"). + ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "deleted-schedule")). + Phase(velerov1api.BackupPhaseCompleted). + CompletionTimestamp(baseTime). + Result() - m.SetBackupLastSuccessfulTimestamp("deleted-schedule", baseTime) - require.Equal(t, 1, collectGaugeCount(t, gauge)) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, activeBackup, deletedBackup) c := &backupReconciler{ - ctx: ctx, kbClient: fakeClient, logger: logrus.StandardLogger(), metrics: m, } - c.updateTotalBackupMetric() - time.Sleep(7 * time.Second) - cancel() + // First resync: sets metrics for both schedules + c.resyncBackupMetrics() + assert.Equal(t, 2, testutil.CollectAndCount(gauge)) - assert.Equal(t, 1, collectGaugeCount(t, gauge)) + // Simulate schedule deletion: remove the backup for "deleted-schedule" + require.NoError(t, fakeClient.Delete(t.Context(), deletedBackup)) + + // Second resync: prunes "deleted-schedule" metric, keeps "active-schedule" + c.resyncBackupMetrics() + assert.Equal(t, 1, testutil.CollectAndCount(gauge)) } // Unit tests to make sure that the backup's status is updated correctly during reconcile. diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index d54eb02b5..4661eaec8 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -758,10 +758,11 @@ func (m *ServerMetrics) RegisterPodVolumeOpLatencyGauge(node, pvbName, opName, b } } -// ResetBackupLastSuccessfulTimestamp removes all schedule-level backupLastSuccessfulTimestamp values. -func (m *ServerMetrics) ResetBackupLastSuccessfulTimestamp() { +// DeleteBackupLastSuccessfulTimestamp removes the backupLastSuccessfulTimestamp +// metric for a single schedule. +func (m *ServerMetrics) DeleteBackupLastSuccessfulTimestamp(scheduleName string) { if g, ok := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec); ok { - g.Reset() + g.DeleteLabelValues(scheduleName) } } diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index 07004f172..2f2135ad1 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -21,6 +21,7 @@ import ( "time" "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/testutil" dto "github.com/prometheus/client_model/go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -457,9 +458,9 @@ func getHistogramCount(t *testing.T, vec *prometheus.HistogramVec, scheduleLabel return 0 } -// TestResetBackupLastSuccessfulTimestamp verifies that ResetBackupLastSuccessfulTimestamp -// removes all schedule-level values from the backupLastSuccessfulTimestamp gauge. -func TestResetBackupLastSuccessfulTimestamp(t *testing.T) { +// TestDeleteBackupLastSuccessfulTimestamp verifies that DeleteBackupLastSuccessfulTimestamp +// removes only the specified schedule's metric. +func TestDeleteBackupLastSuccessfulTimestamp(t *testing.T) { m := NewServerMetrics() now := time.Now() @@ -467,13 +468,17 @@ func TestResetBackupLastSuccessfulTimestamp(t *testing.T) { m.SetBackupLastSuccessfulTimestamp("schedule-2", now.Add(-time.Hour)) m.SetBackupLastSuccessfulTimestamp("", now.Add(-2*time.Hour)) - // Verify all three entries exist g := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec) - assert.Equal(t, 3, collectGaugeCount(t, g)) + assert.Equal(t, 3, testutil.CollectAndCount(g)) - // Reset should remove all entries - m.ResetBackupLastSuccessfulTimestamp() - assert.Equal(t, 0, collectGaugeCount(t, g)) + m.DeleteBackupLastSuccessfulTimestamp("schedule-1") + assert.Equal(t, 2, testutil.CollectAndCount(g)) + + m.DeleteBackupLastSuccessfulTimestamp("schedule-2") + assert.Equal(t, 1, testutil.CollectAndCount(g)) + + m.DeleteBackupLastSuccessfulTimestamp("") + assert.Equal(t, 0, testutil.CollectAndCount(g)) } // collectGaugeCount returns the number of time series in a GaugeVec. From 911cc9eb9e8ed5aadbb7a85c6acfd8ca2ff38bd9 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Wed, 22 Jul 2026 09:57:28 -0700 Subject: [PATCH 8/9] Fix gofmt alignment in backupReconciler struct Signed-off-by: Shubham Pampattiwar --- pkg/controller/backup_controller.go | 56 ++++++++++++++--------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 01a660dad..167fb7eaf 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -84,34 +84,34 @@ var autoExcludeClusterScopedResources = []string{ } type backupReconciler struct { - ctx context.Context - logger logrus.FieldLogger - discoveryHelper discovery.Helper - backupper pkgbackup.Backupper - kbClient kbclient.Client - clock clock.WithTickerAndDelayedExecution - backupLogLevel logrus.Level - newPluginManager func(logrus.FieldLogger) clientmgmt.Manager - backupTracker BackupTracker - defaultBackupLocation string - defaultVolumesToFsBackup bool - defaultBackupTTL time.Duration - defaultVGSLabelKey string - defaultCSISnapshotTimeout time.Duration - resourceTimeout time.Duration - defaultItemOperationTimeout time.Duration - defaultSnapshotLocations map[string]string - metrics *metrics.ServerMetrics - backupStoreGetter persistence.ObjectBackupStoreGetter - formatFlag logging.Format - credentialFileStore credentials.FileStore - maxConcurrentK8SConnections int - defaultSnapshotMoveData bool - globalCRClient kbclient.Client - itemBlockWorkerCount int - concurrentBackups int - globalVolumePoliciesConfigMap string - knownSchedulesWithSuccessfulBackup sets.Set[string] + ctx context.Context + logger logrus.FieldLogger + discoveryHelper discovery.Helper + backupper pkgbackup.Backupper + kbClient kbclient.Client + clock clock.WithTickerAndDelayedExecution + backupLogLevel logrus.Level + newPluginManager func(logrus.FieldLogger) clientmgmt.Manager + backupTracker BackupTracker + defaultBackupLocation string + defaultVolumesToFsBackup bool + defaultBackupTTL time.Duration + defaultVGSLabelKey string + defaultCSISnapshotTimeout time.Duration + resourceTimeout time.Duration + defaultItemOperationTimeout time.Duration + defaultSnapshotLocations map[string]string + metrics *metrics.ServerMetrics + backupStoreGetter persistence.ObjectBackupStoreGetter + formatFlag logging.Format + credentialFileStore credentials.FileStore + maxConcurrentK8SConnections int + defaultSnapshotMoveData bool + globalCRClient kbclient.Client + itemBlockWorkerCount int + concurrentBackups int + globalVolumePoliciesConfigMap string + knownSchedulesWithSuccessfulBackup sets.Set[string] } func NewBackupReconciler( From 0f01b534c314dbdd2378df5dfff93c7e3888f8d2 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Wed, 22 Jul 2026 10:43:13 -0700 Subject: [PATCH 9/9] Remove unused collectGaugeCount, assert surviving label values Remove the unused collectGaugeCount helper and assert specific label values survive after each deletion using testutil.ToFloat64. Signed-off-by: Shubham Pampattiwar --- pkg/metrics/metrics_test.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index 2f2135ad1..d7f070298 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -473,27 +473,17 @@ func TestDeleteBackupLastSuccessfulTimestamp(t *testing.T) { m.DeleteBackupLastSuccessfulTimestamp("schedule-1") assert.Equal(t, 2, testutil.CollectAndCount(g)) + assert.Equal(t, float64(now.Add(-time.Hour).Unix()), testutil.ToFloat64(g.WithLabelValues("schedule-2"))) + assert.Equal(t, float64(now.Add(-2*time.Hour).Unix()), testutil.ToFloat64(g.WithLabelValues(""))) m.DeleteBackupLastSuccessfulTimestamp("schedule-2") assert.Equal(t, 1, testutil.CollectAndCount(g)) + assert.Equal(t, float64(now.Add(-2*time.Hour).Unix()), testutil.ToFloat64(g.WithLabelValues(""))) m.DeleteBackupLastSuccessfulTimestamp("") assert.Equal(t, 0, testutil.CollectAndCount(g)) } -// collectGaugeCount returns the number of time series in a GaugeVec. -func collectGaugeCount(t *testing.T, g *prometheus.GaugeVec) int { - t.Helper() - ch := make(chan prometheus.Metric, 10) - g.Collect(ch) - close(ch) - count := 0 - for range ch { - count++ - } - return count -} - // TestRepoMaintenanceMetrics verifies that repo maintenance metrics are properly recorded. func TestRepoMaintenanceMetrics(t *testing.T) { tests := []struct {