From fdf439963c2299e6111c9387652f724835055511 Mon Sep 17 00:00:00 2001 From: Shubham Pampattiwar Date: Thu, 13 Nov 2025 14:23:39 -0800 Subject: [PATCH] Add Prometheus metrics for maintenance jobs Adds three new Prometheus metrics to track backup repository maintenance job execution: - velero_maintenance_job_success_total: Counter for successful jobs - velero_maintenance_job_failure_total: Counter for failed jobs - velero_maintenance_job_duration_seconds: Histogram for job duration Metrics use repository_name label to identify specific BackupRepositories. Duration is recorded for both successful and failed jobs (when job runs), but not when job fails to start. Includes comprehensive unit and integration tests. Fixes #9225 Signed-off-by: Shubham Pampattiwar --- .../unreleased/9414-shubham-pampattiwar | 1 + pkg/cmd/server/server.go | 1 + .../backup_repository_controller.go | 29 +++ .../backup_repository_controller_test.go | 196 ++++++++++++++++++ pkg/metrics/metrics.go | 67 ++++++ pkg/metrics/metrics_test.go | 145 +++++++++++++ 6 files changed, 439 insertions(+) create mode 100644 changelogs/unreleased/9414-shubham-pampattiwar diff --git a/changelogs/unreleased/9414-shubham-pampattiwar b/changelogs/unreleased/9414-shubham-pampattiwar new file mode 100644 index 000000000..bf7ccbfec --- /dev/null +++ b/changelogs/unreleased/9414-shubham-pampattiwar @@ -0,0 +1 @@ +Add Prometheus metrics for maintenance jobs \ No newline at end of file diff --git a/pkg/cmd/server/server.go b/pkg/cmd/server/server.go index 15c4afd95..b5af9a65b 100644 --- a/pkg/cmd/server/server.go +++ b/pkg/cmd/server/server.go @@ -756,6 +756,7 @@ func (s *server) runControllers(defaultVolumeSnapshotLocations map[string]string s.config.RepoMaintenanceJobConfig, s.logLevel, s.config.LogFormat, + s.metrics, ).SetupWithManager(s.mgr); err != nil { s.logger.Fatal(err, "unable to create controller", "controller", constant.ControllerBackupRepo) } diff --git a/pkg/controller/backup_repository_controller.go b/pkg/controller/backup_repository_controller.go index d2d32d745..eb622908e 100644 --- a/pkg/controller/backup_repository_controller.go +++ b/pkg/controller/backup_repository_controller.go @@ -42,6 +42,7 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/constant" "github.com/vmware-tanzu/velero/pkg/label" + "github.com/vmware-tanzu/velero/pkg/metrics" repoconfig "github.com/vmware-tanzu/velero/pkg/repository/config" "github.com/vmware-tanzu/velero/pkg/repository/maintenance" repomanager "github.com/vmware-tanzu/velero/pkg/repository/manager" @@ -66,6 +67,7 @@ type BackupRepoReconciler struct { repoMaintenanceConfig string logLevel logrus.Level logFormat *logging.FormatFlag + metrics *metrics.ServerMetrics } func NewBackupRepoReconciler( @@ -78,6 +80,7 @@ func NewBackupRepoReconciler( repoMaintenanceConfig string, logLevel logrus.Level, logFormat *logging.FormatFlag, + metrics *metrics.ServerMetrics, ) *BackupRepoReconciler { c := &BackupRepoReconciler{ client, @@ -90,6 +93,7 @@ func NewBackupRepoReconciler( repoMaintenanceConfig, logLevel, logFormat, + metrics, } return c @@ -491,6 +495,12 @@ func (r *BackupRepoReconciler) runMaintenanceIfDue(ctx context.Context, req *vel job, err := funcStartMaintenanceJob(r.Client, ctx, req, r.repoMaintenanceConfig, r.logLevel, r.logFormat, log) if err != nil { log.WithError(err).Warn("Starting repo maintenance failed") + + // Record failure metric when job fails to start + if r.metrics != nil { + r.metrics.RegisterMaintenanceJobFailure(req.Name) + } + return r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) { updateRepoMaintenanceHistory(rr, velerov1api.BackupRepositoryMaintenanceFailed, &metav1.Time{Time: startTime}, nil, fmt.Sprintf("Failed to start maintenance job, err: %v", err)) }) @@ -505,11 +515,30 @@ func (r *BackupRepoReconciler) runMaintenanceIfDue(ctx context.Context, req *vel if status.Result == velerov1api.BackupRepositoryMaintenanceFailed { log.WithError(err).Warn("Pruning repository failed") + + // Record failure metric + if r.metrics != nil { + r.metrics.RegisterMaintenanceJobFailure(req.Name) + if status.StartTimestamp != nil && status.CompleteTimestamp != nil { + duration := status.CompleteTimestamp.Sub(status.StartTimestamp.Time).Seconds() + r.metrics.ObserveMaintenanceJobDuration(req.Name, duration) + } + } + return r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) { updateRepoMaintenanceHistory(rr, velerov1api.BackupRepositoryMaintenanceFailed, status.StartTimestamp, status.CompleteTimestamp, status.Message) }) } + // Record success metric + if r.metrics != nil { + r.metrics.RegisterMaintenanceJobSuccess(req.Name) + if status.StartTimestamp != nil && status.CompleteTimestamp != nil { + duration := status.CompleteTimestamp.Sub(status.StartTimestamp.Time).Seconds() + r.metrics.ObserveMaintenanceJobDuration(req.Name, duration) + } + } + return r.patchBackupRepository(ctx, req, func(rr *velerov1api.BackupRepository) { rr.Status.LastMaintenanceTime = &metav1.Time{Time: status.CompleteTimestamp.Time} updateRepoMaintenanceHistory(rr, velerov1api.BackupRepositoryMaintenanceSucceeded, status.StartTimestamp, status.CompleteTimestamp, status.Message) diff --git a/pkg/controller/backup_repository_controller_test.go b/pkg/controller/backup_repository_controller_test.go index 0a4a04610..1fc1e9199 100644 --- a/pkg/controller/backup_repository_controller_test.go +++ b/pkg/controller/backup_repository_controller_test.go @@ -19,6 +19,8 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" @@ -32,6 +34,7 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" + "github.com/vmware-tanzu/velero/pkg/metrics" "github.com/vmware-tanzu/velero/pkg/repository" "github.com/vmware-tanzu/velero/pkg/repository/maintenance" repomaintenance "github.com/vmware-tanzu/velero/pkg/repository/maintenance" @@ -65,6 +68,7 @@ func mockBackupRepoReconciler(t *testing.T, mockOn string, arg any, ret ...any) "", logrus.InfoLevel, nil, + nil, ) } @@ -584,6 +588,7 @@ func TestGetRepositoryMaintenanceFrequency(t *testing.T) { "", logrus.InfoLevel, nil, + nil, ) freq := reconciler.getRepositoryMaintenanceFrequency(test.repo) @@ -716,6 +721,7 @@ func TestNeedInvalidBackupRepo(t *testing.T) { "", logrus.InfoLevel, nil, + nil, ) need := reconciler.needInvalidBackupRepo(test.oldBSL, test.newBSL) @@ -1581,6 +1587,7 @@ func TestDeleteOldMaintenanceJobWithConfigMap(t *testing.T) { repoMaintenanceConfigName, logrus.InfoLevel, nil, + nil, ) _, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: test.repo.Namespace, Name: "repo"}}) @@ -1638,6 +1645,7 @@ func TestInitializeRepoWithRepositoryTypes(t *testing.T) { "", logrus.InfoLevel, nil, + nil, ) err := reconciler.initializeRepo(t.Context(), rr, location, reconciler.logger) @@ -1689,6 +1697,7 @@ func TestInitializeRepoWithRepositoryTypes(t *testing.T) { "", logrus.InfoLevel, nil, + nil, ) err := reconciler.initializeRepo(t.Context(), rr, location, reconciler.logger) @@ -1739,6 +1748,7 @@ func TestInitializeRepoWithRepositoryTypes(t *testing.T) { "", logrus.InfoLevel, nil, + nil, ) err := reconciler.initializeRepo(t.Context(), rr, location, reconciler.logger) @@ -1750,3 +1760,189 @@ func TestInitializeRepoWithRepositoryTypes(t *testing.T) { assert.Equal(t, velerov1api.BackupRepositoryPhaseReady, rr.Status.Phase) }) } + +func TestMaintenanceJobMetricsRecording(t *testing.T) { + now := time.Now().Round(time.Second) + + tests := []struct { + name string + repo *velerov1api.BackupRepository + startJobFunc func(client.Client, context.Context, *velerov1api.BackupRepository, string, logrus.Level, *logging.FormatFlag, logrus.FieldLogger) (string, error) + waitJobFunc func(client.Client, context.Context, string, string, logrus.FieldLogger) (velerov1api.BackupRepositoryMaintenanceStatus, error) + expectSuccess bool + expectFailure bool + expectDuration bool + }{ + { + name: "metrics recorded on successful maintenance", + repo: &velerov1api.BackupRepository{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "test-repo-success", + }, + Spec: velerov1api.BackupRepositorySpec{ + MaintenanceFrequency: metav1.Duration{Duration: time.Hour}, + }, + Status: velerov1api.BackupRepositoryStatus{ + LastMaintenanceTime: &metav1.Time{Time: now.Add(-2 * time.Hour)}, + }, + }, + startJobFunc: startMaintenanceJobSucceed, + waitJobFunc: waitMaintenanceJobCompleteFunc(now, velerov1api.BackupRepositoryMaintenanceSucceeded, ""), + expectSuccess: true, + expectFailure: false, + expectDuration: true, + }, + { + name: "metrics recorded on failed maintenance", + repo: &velerov1api.BackupRepository{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "test-repo-failure", + }, + Spec: velerov1api.BackupRepositorySpec{ + MaintenanceFrequency: metav1.Duration{Duration: time.Hour}, + }, + Status: velerov1api.BackupRepositoryStatus{ + LastMaintenanceTime: &metav1.Time{Time: now.Add(-2 * time.Hour)}, + }, + }, + startJobFunc: startMaintenanceJobSucceed, + waitJobFunc: func(client.Client, context.Context, string, string, logrus.FieldLogger) (velerov1api.BackupRepositoryMaintenanceStatus, error) { + return velerov1api.BackupRepositoryMaintenanceStatus{ + StartTimestamp: &metav1.Time{Time: now}, + CompleteTimestamp: &metav1.Time{Time: now.Add(time.Minute)}, // Job ran for 1 minute then failed + Result: velerov1api.BackupRepositoryMaintenanceFailed, + Message: "test error", + }, nil + }, + expectSuccess: false, + expectFailure: true, + expectDuration: true, + }, + { + name: "metrics recorded on job start failure", + repo: &velerov1api.BackupRepository{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: velerov1api.DefaultNamespace, + Name: "test-repo-start-fail", + }, + Spec: velerov1api.BackupRepositorySpec{ + MaintenanceFrequency: metav1.Duration{Duration: time.Hour}, + }, + Status: velerov1api.BackupRepositoryStatus{ + LastMaintenanceTime: &metav1.Time{Time: now.Add(-2 * time.Hour)}, + }, + }, + startJobFunc: startMaintenanceJobFail, + expectSuccess: false, + expectFailure: true, + expectDuration: false, // No duration when job fails to start + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + // Create metrics instance + m := metrics.NewServerMetrics() + + // Create reconciler with metrics + reconciler := mockBackupRepoReconciler(t, "", test.repo, nil) + reconciler.metrics = m + reconciler.clock = &fakeClock{now} + + err := reconciler.Client.Create(t.Context(), test.repo) + require.NoError(t, err) + + // Set up job functions + funcStartMaintenanceJob = test.startJobFunc + funcWaitMaintenanceJobComplete = test.waitJobFunc + + // Run maintenance + _ = reconciler.runMaintenanceIfDue(t.Context(), test.repo, velerotest.NewLogger()) + + // Verify metrics were recorded + successCount := getMaintenanceMetricValue(t, m, "maintenance_job_success_total", test.repo.Name) + failureCount := getMaintenanceMetricValue(t, m, "maintenance_job_failure_total", test.repo.Name) + durationCount := getMaintenanceDurationCount(t, m, test.repo.Name) + + if test.expectSuccess { + assert.Equal(t, float64(1), successCount, "Success metric should be recorded") + } else { + assert.Equal(t, float64(0), successCount, "Success metric should not be recorded") + } + + if test.expectFailure { + assert.Equal(t, float64(1), failureCount, "Failure metric should be recorded") + } else { + assert.Equal(t, float64(0), failureCount, "Failure metric should not be recorded") + } + + if test.expectDuration { + assert.Equal(t, uint64(1), durationCount, "Duration metric should be recorded") + } else { + assert.Equal(t, uint64(0), durationCount, "Duration metric should not be recorded") + } + }) + } +} + +// Helper to get maintenance metric value from ServerMetrics +func getMaintenanceMetricValue(t *testing.T, m *metrics.ServerMetrics, metricName, repoName string) float64 { + t.Helper() + + metricMap := m.Metrics() + collector, ok := metricMap[metricName] + if !ok { + return 0 + } + + ch := make(chan prometheus.Metric, 1) + collector.Collect(ch) + close(ch) + + for metric := range ch { + dto := &dto.Metric{} + err := metric.Write(dto) + require.NoError(t, err) + + for _, label := range dto.Label { + if *label.Name == "repository_name" && *label.Value == repoName { + if dto.Counter != nil { + return *dto.Counter.Value + } + } + } + } + return 0 +} + +// Helper to get maintenance duration histogram count +func getMaintenanceDurationCount(t *testing.T, m *metrics.ServerMetrics, repoName string) uint64 { + t.Helper() + + metricMap := m.Metrics() + collector, ok := metricMap["maintenance_job_duration_seconds"] + if !ok { + return 0 + } + + ch := make(chan prometheus.Metric, 1) + collector.Collect(ch) + close(ch) + + for metric := range ch { + dto := &dto.Metric{} + err := metric.Write(dto) + require.NoError(t, err) + + for _, label := range dto.Label { + if *label.Name == "repository_name" && *label.Value == repoName { + if dto.Histogram != nil { + return *dto.Histogram.SampleCount + } + } + } + } + return 0 +} diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index 557a0a4b8..84508bed0 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -27,6 +27,11 @@ type ServerMetrics struct { metrics map[string]prometheus.Collector } +// Metrics returns the metrics map for testing purposes. +func (m *ServerMetrics) Metrics() map[string]prometheus.Collector { + return m.metrics +} + const ( metricNamespace = "velero" podVolumeMetricsNamespace = "podVolume" @@ -75,6 +80,11 @@ const ( DataDownloadFailureTotal = "data_download_failure_total" DataDownloadCancelTotal = "data_download_cancel_total" + // maintenance job metrics + maintenanceJobSuccessTotal = "maintenance_job_success_total" + maintenanceJobFailureTotal = "maintenance_job_failure_total" + maintenanceJobDurationSeconds = "maintenance_job_duration_seconds" + // Labels nodeMetricLabel = "node" podVolumeOperationLabel = "operation" @@ -82,6 +92,7 @@ const ( pvbNameLabel = "pod_volume_backup" scheduleLabel = "schedule" backupNameLabel = "backupName" + repositoryNameLabel = "repository_name" // metrics values BackupLastStatusSucc int64 = 1 @@ -333,6 +344,41 @@ func NewServerMetrics() *ServerMetrics { }, []string{scheduleLabel, backupNameLabel}, ), + maintenanceJobSuccessTotal: prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: metricNamespace, + Name: maintenanceJobSuccessTotal, + Help: "Total number of successful maintenance jobs", + }, + []string{repositoryNameLabel}, + ), + maintenanceJobFailureTotal: prometheus.NewCounterVec( + prometheus.CounterOpts{ + Namespace: metricNamespace, + Name: maintenanceJobFailureTotal, + Help: "Total number of failed maintenance jobs", + }, + []string{repositoryNameLabel}, + ), + maintenanceJobDurationSeconds: prometheus.NewHistogramVec( + prometheus.HistogramOpts{ + Namespace: metricNamespace, + Name: maintenanceJobDurationSeconds, + Help: "Time taken to complete maintenance jobs, in seconds", + Buckets: []float64{ + toSeconds(1 * time.Minute), + toSeconds(5 * time.Minute), + toSeconds(10 * time.Minute), + toSeconds(15 * time.Minute), + toSeconds(30 * time.Minute), + toSeconds(1 * time.Hour), + toSeconds(2 * time.Hour), + toSeconds(3 * time.Hour), + toSeconds(4 * time.Hour), + }, + }, + []string{repositoryNameLabel}, + ), }, } } @@ -912,3 +958,24 @@ func (m *ServerMetrics) RegisterBackupLocationUnavailable(backupLocationName str g.WithLabelValues(backupLocationName).Set(float64(0)) } } + +// RegisterMaintenanceJobSuccess records a successful maintenance job. +func (m *ServerMetrics) RegisterMaintenanceJobSuccess(repositoryName string) { + if c, ok := m.metrics[maintenanceJobSuccessTotal].(*prometheus.CounterVec); ok { + c.WithLabelValues(repositoryName).Inc() + } +} + +// RegisterMaintenanceJobFailure records a failed maintenance job. +func (m *ServerMetrics) RegisterMaintenanceJobFailure(repositoryName string) { + if c, ok := m.metrics[maintenanceJobFailureTotal].(*prometheus.CounterVec); ok { + c.WithLabelValues(repositoryName).Inc() + } +} + +// ObserveMaintenanceJobDuration records the number of seconds a maintenance job took. +func (m *ServerMetrics) ObserveMaintenanceJobDuration(repositoryName string, seconds float64) { + if h, ok := m.metrics[maintenanceJobDurationSeconds].(*prometheus.HistogramVec); ok { + h.WithLabelValues(repositoryName).Observe(seconds) + } +} diff --git a/pkg/metrics/metrics_test.go b/pkg/metrics/metrics_test.go index 36bf29c7c..005228417 100644 --- a/pkg/metrics/metrics_test.go +++ b/pkg/metrics/metrics_test.go @@ -372,3 +372,148 @@ func getHistogramCount(t *testing.T, vec *prometheus.HistogramVec, scheduleLabel t.Fatalf("Histogram with schedule label '%s' not found", scheduleLabel) return 0 } + +// TestMaintenanceJobMetrics verifies that maintenance job metrics are properly recorded. +func TestMaintenanceJobMetrics(t *testing.T) { + tests := []struct { + name string + repositoryName string + description string + }{ + { + name: "maintenance job metrics for repository", + repositoryName: "default-restic-abcd", + description: "Metrics should be recorded with the repository name label", + }, + { + name: "maintenance job metrics for different repository", + repositoryName: "velero-backup-repo-xyz", + description: "Metrics should be recorded with different repository name", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + m := NewServerMetrics() + + // Test maintenance job success metric + t.Run("RegisterMaintenanceJobSuccess", func(t *testing.T) { + m.RegisterMaintenanceJobSuccess(tc.repositoryName) + + metric := getMaintenanceMetricValue(t, m.metrics[maintenanceJobSuccessTotal].(*prometheus.CounterVec), tc.repositoryName) + assert.Equal(t, float64(1), metric, tc.description) + }) + + // Test maintenance job failure metric + t.Run("RegisterMaintenanceJobFailure", func(t *testing.T) { + m.RegisterMaintenanceJobFailure(tc.repositoryName) + + metric := getMaintenanceMetricValue(t, m.metrics[maintenanceJobFailureTotal].(*prometheus.CounterVec), tc.repositoryName) + assert.Equal(t, float64(1), metric, tc.description) + }) + + // Test maintenance job duration metric + t.Run("ObserveMaintenanceJobDuration", func(t *testing.T) { + m.ObserveMaintenanceJobDuration(tc.repositoryName, 300.5) + + // For histogram, we check the count + metric := getMaintenanceHistogramCount(t, m.metrics[maintenanceJobDurationSeconds].(*prometheus.HistogramVec), tc.repositoryName) + assert.Equal(t, uint64(1), metric, tc.description) + }) + }) + } +} + +// TestMultipleMaintenanceJobsAccumulate verifies that multiple maintenance jobs +// accumulate metrics under the same repository label. +func TestMultipleMaintenanceJobsAccumulate(t *testing.T) { + m := NewServerMetrics() + repoName := "default-restic-test" + + // Simulate multiple maintenance job executions + m.RegisterMaintenanceJobSuccess(repoName) + m.RegisterMaintenanceJobSuccess(repoName) + m.RegisterMaintenanceJobSuccess(repoName) + m.RegisterMaintenanceJobFailure(repoName) + m.RegisterMaintenanceJobFailure(repoName) + + // Record multiple durations + m.ObserveMaintenanceJobDuration(repoName, 120.5) + m.ObserveMaintenanceJobDuration(repoName, 180.3) + m.ObserveMaintenanceJobDuration(repoName, 90.7) + + // Verify accumulated metrics + successMetric := getMaintenanceMetricValue(t, m.metrics[maintenanceJobSuccessTotal].(*prometheus.CounterVec), repoName) + assert.Equal(t, float64(3), successMetric, "All maintenance job successes should be counted") + + failureMetric := getMaintenanceMetricValue(t, m.metrics[maintenanceJobFailureTotal].(*prometheus.CounterVec), repoName) + assert.Equal(t, float64(2), failureMetric, "All maintenance job failures should be counted") + + durationCount := getMaintenanceHistogramCount(t, m.metrics[maintenanceJobDurationSeconds].(*prometheus.HistogramVec), repoName) + assert.Equal(t, uint64(3), durationCount, "All maintenance job durations should be observed") +} + +// Helper function to get metric value from a CounterVec with repository_name label +func getMaintenanceMetricValue(t *testing.T, vec prometheus.Collector, repositoryName string) float64 { + t.Helper() + ch := make(chan prometheus.Metric, 1) + vec.Collect(ch) + close(ch) + + for metric := range ch { + dto := &dto.Metric{} + err := metric.Write(dto) + require.NoError(t, err) + + // Check if this metric has the expected repository_name label + hasCorrectLabel := false + for _, label := range dto.Label { + if *label.Name == "repository_name" && *label.Value == repositoryName { + hasCorrectLabel = true + break + } + } + + if hasCorrectLabel { + if dto.Counter != nil { + return *dto.Counter.Value + } + if dto.Gauge != nil { + return *dto.Gauge.Value + } + } + } + + t.Fatalf("Metric with repository_name label '%s' not found", repositoryName) + return 0 +} + +// Helper function to get histogram count with repository_name label +func getMaintenanceHistogramCount(t *testing.T, vec *prometheus.HistogramVec, repositoryName string) uint64 { + t.Helper() + ch := make(chan prometheus.Metric, 1) + vec.Collect(ch) + close(ch) + + for metric := range ch { + dto := &dto.Metric{} + err := metric.Write(dto) + require.NoError(t, err) + + // Check if this metric has the expected repository_name label + hasCorrectLabel := false + for _, label := range dto.Label { + if *label.Name == "repository_name" && *label.Value == repositoryName { + hasCorrectLabel = true + break + } + } + + if hasCorrectLabel && dto.Histogram != nil { + return *dto.Histogram.SampleCount + } + } + + t.Fatalf("Histogram with repository_name label '%s' not found", repositoryName) + return 0 +}