From 4ea38216f5d1e09a400c30e3e5977ba0dfb76346 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Wed, 22 Jul 2026 09:16:24 -0700 Subject: [PATCH] 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.