mirrors/velero
1
0
Fork 0
mirror of https://github.com/vmware-tanzu/velero.git synced 2026-07-26 01:53:09 +00:00
Code Issues Actions Packages Projects Releases Wiki Activity

Address review feedback: use DeleteLabelValues, extract resync method

Browse Source 
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 <spampatt@redhat.com>
This commit is contained in:
Shubham Pampattiwar
2026-07-22 09:17:59 -07:00
parent b9d9dcfc38
commit 4ea38216f5
4 changed files with 74 additions and 96 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/controller/backup_controller.go
+36 -22
View File
@@ -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
pkg/controller/backup_controller_test.go
+21 -63
View File
@@ -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.
pkg/metrics/metrics.go
+4 -3
View File
@@ -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)
}
}
pkg/metrics/metrics_test.go
+13 -8
View File
@@ -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.