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 diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 74b857fd2..167fb7eaf 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -84,33 +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 + 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( @@ -204,28 +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) - 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 bab98efb6..b86434796 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/testutil" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -2041,6 +2042,48 @@ func Test_getLastSuccessBySchedule(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"] + + activeBackup := builder.ForBackup("velero", "b1"). + ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "active-schedule")). + Phase(velerov1api.BackupPhaseCompleted). + CompletionTimestamp(baseTime). + Result() + + deletedBackup := builder.ForBackup("velero", "b2"). + ObjectMeta(builder.WithLabels(velerov1api.ScheduleNameLabel, "deleted-schedule")). + Phase(velerov1api.BackupPhaseCompleted). + CompletionTimestamp(baseTime). + Result() + + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, activeBackup, deletedBackup) + + c := &backupReconciler{ + kbClient: fakeClient, + logger: logrus.StandardLogger(), + metrics: m, + } + + // First resync: sets metrics for both schedules + c.resyncBackupMetrics() + assert.Equal(t, 2, testutil.CollectAndCount(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. // 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 86d78028c..4661eaec8 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -758,6 +758,14 @@ func (m *ServerMetrics) RegisterPodVolumeOpLatencyGauge(node, pvbName, opName, b } } +// 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.DeleteLabelValues(scheduleName) + } +} + // 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..d7f070298 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,6 +458,32 @@ func getHistogramCount(t *testing.T, vec *prometheus.HistogramVec, scheduleLabel return 0 } +// TestDeleteBackupLastSuccessfulTimestamp verifies that DeleteBackupLastSuccessfulTimestamp +// removes only the specified schedule's metric. +func TestDeleteBackupLastSuccessfulTimestamp(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)) + + g := m.metrics[backupLastSuccessfulTimestamp].(*prometheus.GaugeVec) + assert.Equal(t, 3, testutil.CollectAndCount(g)) + + 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)) +} + // TestRepoMaintenanceMetrics verifies that repo maintenance metrics are properly recorded. func TestRepoMaintenanceMetrics(t *testing.T) { tests := []struct {