From 797abc8d497ede37bd0c69ccc557c836f8791ce2 Mon Sep 17 00:00:00 2001 From: Shirly Radco Date: Thu, 12 Mar 2026 20:42:50 +0200 Subject: [PATCH 1/2] k8s: add orphan AlertRelabelConfig GC Detect and remove orphan AlertRelabelConfig resources that no longer have a matching PrometheusRule, preventing stale relabel configs from accumulating. Cover orphan deletion and keeper cases in e2e (live rule, GitOps, unannotated). Signed-off-by: Shirly Radco Co-authored-by: AI Assistant --- pkg/k8s/alert_relabel_config_gc.go | 57 ++++++ pkg/k8s/alert_relabel_config_gc_test.go | 188 +++++++++++++++++++ pkg/k8s/relabeled_rules.go | 35 ++-- test/e2e/orphan_arc_gc_test.go | 232 ++++++++++++++++++++++++ 4 files changed, 500 insertions(+), 12 deletions(-) create mode 100644 pkg/k8s/alert_relabel_config_gc.go create mode 100644 pkg/k8s/alert_relabel_config_gc_test.go create mode 100644 test/e2e/orphan_arc_gc_test.go diff --git a/pkg/k8s/alert_relabel_config_gc.go b/pkg/k8s/alert_relabel_config_gc.go new file mode 100644 index 000000000..f7d5bac50 --- /dev/null +++ b/pkg/k8s/alert_relabel_config_gc.go @@ -0,0 +1,57 @@ +package k8s + +import ( + "context" + + "github.com/openshift/monitoring-plugin/pkg/managementlabels" +) + +// gcOrphanedARCs deletes AlertRelabelConfigs whose associated alert rule no +// longer exists. This handles the case where an operator (or manual action) +// removes rules from a PrometheusRule or deletes the CR entirely — the ARCs +// that were created by the plugin for classification/drop/stamp become orphans. +// +// Only ARCs carrying the plugin's alertRuleId annotation are considered. +// GitOps-managed ARCs are never deleted automatically; a warning is logged +// so that operators can clean them up manually. +// +// liveRuleIDs must include every alerting-rule ID still present on a +// PrometheusRule, including platform rules dropped by relabel configs. +// IDs are recorded in collectAlerts before the drop continue, so a Drop +// ARC for a disabled rule is not treated as an orphan. +func (rrm *relabeledRulesManager) gcOrphanedARCs(ctx context.Context, liveRuleIDs map[string]struct{}) { + if rrm.alertRelabelConfigs == nil { + return + } + + arcs, err := rrm.alertRelabelConfigs.List(ctx, "") + if err != nil { + log.Errorf("orphan AlertRelabelConfig cleanup: failed to list AlertRelabelConfigs: %v", err) + return + } + + for i := range arcs { + arc := &arcs[i] + + ruleID, ok := arc.Annotations[managementlabels.ARCAnnotationAlertRuleIDKey] + if !ok || ruleID == "" { + continue + } + + if _, alive := liveRuleIDs[ruleID]; alive { + continue + } + + if IsManagedByGitOps(arc.Annotations, arc.Labels) { + log.Warnf("orphan AlertRelabelConfig cleanup: AlertRelabelConfig %s/%s (ruleId=%s) is orphaned but GitOps-managed — skipping deletion, manual cleanup required", arc.Namespace, arc.Name, ruleID) + continue + } + + if err := rrm.alertRelabelConfigs.Delete(ctx, arc.Namespace, arc.Name); err != nil { + log.Errorf("orphan AlertRelabelConfig cleanup: failed to delete AlertRelabelConfig %s/%s: %v", arc.Namespace, arc.Name, err) + continue + } + + log.Infof("orphan AlertRelabelConfig cleanup: deleted orphaned AlertRelabelConfig %s/%s (ruleId=%s)", arc.Namespace, arc.Name, ruleID) + } +} diff --git a/pkg/k8s/alert_relabel_config_gc_test.go b/pkg/k8s/alert_relabel_config_gc_test.go new file mode 100644 index 000000000..21acc246f --- /dev/null +++ b/pkg/k8s/alert_relabel_config_gc_test.go @@ -0,0 +1,188 @@ +package k8s + +import ( + "context" + "testing" + + osmv1 "github.com/openshift/api/monitoring/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/openshift/monitoring-plugin/pkg/managementlabels" +) + +type mockARCInterface struct { + arcs map[string]*osmv1.AlertRelabelConfig + deleted []string +} + +func (m *mockARCInterface) List(_ context.Context, _ string) ([]osmv1.AlertRelabelConfig, error) { + var result []osmv1.AlertRelabelConfig + for _, arc := range m.arcs { + result = append(result, *arc) + } + return result, nil +} + +func (m *mockARCInterface) Get(_ context.Context, ns, name string) (*osmv1.AlertRelabelConfig, bool, error) { + if arc, ok := m.arcs[ns+"/"+name]; ok { + return arc, true, nil + } + return nil, false, nil +} + +func (m *mockARCInterface) Create(_ context.Context, arc osmv1.AlertRelabelConfig) (*osmv1.AlertRelabelConfig, error) { + return &arc, nil +} + +func (m *mockARCInterface) Update(_ context.Context, _ osmv1.AlertRelabelConfig) error { return nil } + +func (m *mockARCInterface) Delete(_ context.Context, ns, name string) error { + m.deleted = append(m.deleted, ns+"/"+name) + delete(m.arcs, ns+"/"+name) + return nil +} + +func newARC(ns, name, ruleID string, annotations, labels map[string]string) *osmv1.AlertRelabelConfig { + if annotations == nil { + annotations = map[string]string{} + } + if ruleID != "" { + annotations[managementlabels.ARCAnnotationAlertRuleIDKey] = ruleID + } + return &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: ns, + Annotations: annotations, + Labels: labels, + }, + } +} + +func TestGCOrphanedARCs_DeletesOrphan(t *testing.T) { + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-orphan": newARC("openshift-monitoring", "arc-orphan", "rule-gone", nil, nil), + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) + + if len(mock.deleted) != 1 || mock.deleted[0] != "openshift-monitoring/arc-orphan" { + t.Fatalf("expected orphan ARC to be deleted, got deleted=%v", mock.deleted) + } +} + +func TestGCOrphanedARCs_KeepsLiveRule(t *testing.T) { + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-live": newARC("openshift-monitoring", "arc-live", "rule-alive", nil, nil), + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{"rule-alive": {}}) + + if len(mock.deleted) != 0 { + t.Fatalf("expected no deletions, got deleted=%v", mock.deleted) + } +} + +func TestGCOrphanedARCs_SkipsGitOpsManaged(t *testing.T) { + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-gitops": newARC("openshift-monitoring", "arc-gitops", "rule-gone", + map[string]string{"argocd.argoproj.io/tracking-id": "some-id"}, nil), + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) + + if len(mock.deleted) != 0 { + t.Fatalf("expected GitOps-managed ARC to be preserved, got deleted=%v", mock.deleted) + } +} + +func TestGCOrphanedARCs_SkipsARCWithoutAnnotation(t *testing.T) { + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-manual": newARC("openshift-monitoring", "arc-manual", "", nil, nil), + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) + + if len(mock.deleted) != 0 { + t.Fatalf("expected ARC without annotation to be preserved, got deleted=%v", mock.deleted) + } +} + +func TestGCOrphanedARCs_MixedScenario(t *testing.T) { + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-live": newARC("openshift-monitoring", "arc-live", "rule-1", nil, nil), + "openshift-monitoring/arc-orphan1": newARC("openshift-monitoring", "arc-orphan1", "rule-deleted-1", nil, nil), + "openshift-monitoring/arc-orphan2": newARC("openshift-monitoring", "arc-orphan2", "rule-deleted-2", nil, nil), + "openshift-monitoring/arc-gitops": newARC("openshift-monitoring", "arc-gitops", "rule-deleted-3", + map[string]string{"argocd.argoproj.io/tracking-id": "t"}, nil), + "openshift-monitoring/arc-manual": newARC("openshift-monitoring", "arc-manual", "", nil, nil), + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock} + + liveIDs := map[string]struct{}{"rule-1": {}} + rrm.gcOrphanedARCs(context.Background(), liveIDs) + + deletedSet := map[string]bool{} + for _, d := range mock.deleted { + deletedSet[d] = true + } + + if len(mock.deleted) != 2 { + t.Fatalf("expected 2 deletions, got %d: %v", len(mock.deleted), mock.deleted) + } + if !deletedSet["openshift-monitoring/arc-orphan1"] { + t.Error("expected arc-orphan1 to be deleted") + } + if !deletedSet["openshift-monitoring/arc-orphan2"] { + t.Error("expected arc-orphan2 to be deleted") + } + if deletedSet["openshift-monitoring/arc-live"] { + t.Error("arc-live should not have been deleted") + } + if deletedSet["openshift-monitoring/arc-gitops"] { + t.Error("arc-gitops should not have been deleted (GitOps-managed)") + } + if deletedSet["openshift-monitoring/arc-manual"] { + t.Error("arc-manual should not have been deleted (no annotation)") + } +} + +func TestGCOrphanedARCs_NilInterface(t *testing.T) { + rrm := &relabeledRulesManager{alertRelabelConfigs: nil} + // Should not panic + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) +} + +func TestGCOrphanedARCs_NilAnnotations(t *testing.T) { + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-nil": { + ObjectMeta: metav1.ObjectMeta{ + Name: "arc-nil", + Namespace: "openshift-monitoring", + }, + }, + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) + + if len(mock.deleted) != 0 { + t.Fatalf("expected ARC with nil annotations to be preserved, got deleted=%v", mock.deleted) + } +} diff --git a/pkg/k8s/relabeled_rules.go b/pkg/k8s/relabeled_rules.go index 02452c385..c1104cf84 100644 --- a/pkg/k8s/relabeled_rules.go +++ b/pkg/k8s/relabeled_rules.go @@ -46,6 +46,10 @@ const ( AppKubernetesIoComponent = "app.kubernetes.io/component" AppKubernetesIoComponentAlertManagementApi = "alert-management-api" AppKubernetesIoComponentMonitoringPlugin = "monitoring-plugin" + + relabeledRulesSyncKeyInitial = "initial-sync" + relabeledRulesSyncKeyPrometheusRule = "prometheus-rule-sync" + relabeledRulesSyncKeySecret = "secret-sync" ) type relabeledRulesManager struct { @@ -97,7 +101,7 @@ func newRelabeledRulesManager(ctx context.Context, namespaceManager NamespaceInt return } log.Debugf("prometheus rule added: %s/%s", promRule.Namespace, promRule.Name) - rrm.queue.Add("prometheus-rule-sync") + rrm.queue.Add(relabeledRulesSyncKeyPrometheusRule) }, UpdateFunc: func(oldObj interface{}, newObj interface{}) { promRule, ok := newObj.(*monitoringv1.PrometheusRule) @@ -105,7 +109,7 @@ func newRelabeledRulesManager(ctx context.Context, namespaceManager NamespaceInt return } log.Debugf("prometheus rule updated: %s/%s", promRule.Namespace, promRule.Name) - rrm.queue.Add("prometheus-rule-sync") + rrm.queue.Add(relabeledRulesSyncKeyPrometheusRule) }, DeleteFunc: func(obj interface{}) { if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok { @@ -117,7 +121,7 @@ func newRelabeledRulesManager(ctx context.Context, namespaceManager NamespaceInt return } log.Debugf("prometheus rule deleted: %s/%s", promRule.Namespace, promRule.Name) - rrm.queue.Add("prometheus-rule-sync") + rrm.queue.Add(relabeledRulesSyncKeyPrometheusRule) }, }) if err != nil { @@ -126,13 +130,13 @@ func newRelabeledRulesManager(ctx context.Context, namespaceManager NamespaceInt _, err = rrm.secretInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ AddFunc: func(obj interface{}) { - rrm.queue.Add("secret-sync") + rrm.queue.Add(relabeledRulesSyncKeySecret) }, UpdateFunc: func(oldObj interface{}, newObj interface{}) { - rrm.queue.Add("secret-sync") + rrm.queue.Add(relabeledRulesSyncKeySecret) }, DeleteFunc: func(obj interface{}) { - rrm.queue.Add("secret-sync") + rrm.queue.Add(relabeledRulesSyncKeySecret) }, }) if err != nil { @@ -149,7 +153,7 @@ func newRelabeledRulesManager(ctx context.Context, namespaceManager NamespaceInt return nil, fmt.Errorf("failed to sync RelabeledRulesConfig informer") } - if err := rrm.sync(ctx); err != nil { + if err := rrm.sync(ctx, relabeledRulesSyncKeyInitial); err != nil { return nil, fmt.Errorf("initial relabeled rules sync failed: %w", err) } @@ -180,7 +184,7 @@ func (rrm *relabeledRulesManager) processNextWorkItem(ctx context.Context) bool defer rrm.queue.Done(key) - if err := rrm.sync(ctx); err != nil { + if err := rrm.sync(ctx, key); err != nil { log.Errorf("error syncing relabeled rules: %v", err) rrm.queue.AddRateLimited(key) return true @@ -191,7 +195,7 @@ func (rrm *relabeledRulesManager) processNextWorkItem(ctx context.Context) bool return true } -func (rrm *relabeledRulesManager) sync(ctx context.Context) error { +func (rrm *relabeledRulesManager) sync(ctx context.Context, key string) error { relabelConfigs, err := rrm.loadRelabelConfigs() if err != nil { return fmt.Errorf("failed to load relabel configs: %w", err) @@ -201,13 +205,20 @@ func (rrm *relabeledRulesManager) sync(ctx context.Context) error { rrm.relabelConfigs = relabelConfigs rrm.mu.Unlock() - alerts := rrm.collectAlerts(ctx, relabelConfigs) + alerts, allRuleIDs := rrm.collectAlerts(ctx, relabelConfigs) rrm.mu.Lock() rrm.relabeledRules = alerts rrm.mu.Unlock() log.Infof("Synced %d relabeled rules in memory", len(alerts)) + + // GC orphaned ARCs only when triggered by PrometheusRule events or + // initial sync — secret-only changes cannot create orphans. + if key == relabeledRulesSyncKeyPrometheusRule || key == relabeledRulesSyncKeyInitial { + rrm.gcOrphanedARCs(ctx, allRuleIDs) + } + return nil } @@ -256,7 +267,7 @@ func (rrm *relabeledRulesManager) loadRelabelConfigs() ([]*relabel.Config, error return configs, nil } -func (rrm *relabeledRulesManager) collectAlerts(ctx context.Context, relabelConfigs []*relabel.Config) map[string]monitoringv1.Rule { +func (rrm *relabeledRulesManager) collectAlerts(ctx context.Context, relabelConfigs []*relabel.Config) (map[string]monitoringv1.Rule, map[string]struct{}) { alerts := make(map[string]monitoringv1.Rule) seenIDs := make(map[string]struct{}) @@ -336,7 +347,7 @@ func (rrm *relabeledRulesManager) collectAlerts(ctx context.Context, relabelConf } log.Debugf("Collected %d alerts", len(alerts)) - return alerts + return alerts, seenIDs } // alertingRuleOwner returns the name of the AlertingRule CR that generated diff --git a/test/e2e/orphan_arc_gc_test.go b/test/e2e/orphan_arc_gc_test.go new file mode 100644 index 000000000..6213de28a --- /dev/null +++ b/test/e2e/orphan_arc_gc_test.go @@ -0,0 +1,232 @@ +//go:build e2e + +package e2e + +import ( + "context" + "fmt" + "testing" + "time" + + osmv1 "github.com/openshift/api/monitoring/v1" + monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/intstr" + + "github.com/openshift/monitoring-plugin/pkg/k8s" + "github.com/openshift/monitoring-plugin/pkg/managementlabels" + "github.com/openshift/monitoring-plugin/test/e2e/framework" +) + +const ( + orphanARCGCPollInterval = time.Second + orphanARCGCPollTimeout = 2 * time.Minute +) + +// TestOrphanAlertRelabelConfigGC creates plugin-owned AlertRelabelConfigs +// and a PrometheusRule, then waits for a PrometheusRule-driven sync to +// delete the orphan while keeping live, GitOps-managed, and unannotated ARCs. +func TestOrphanAlertRelabelConfigGC(t *testing.T) { + f, err := framework.New() + if err != nil { + t.Fatalf("Failed to create framework: %v", err) + } + + ctx := context.Background() + + // Cluster-monitoring namespace so GET /rules can see the live rule + // (e2e-management-api does not enable user-workload monitoring). + testNamespace, cleanup, err := f.CreatePlatformNamespace(ctx, "test-orphan-arc-gc") + if err != nil { + t.Fatalf("Failed to create test namespace: %v", err) + } + defer func() { + if err := cleanup(); err != nil { + t.Logf("cleanup failed: %v", err) + } + }() + + alertName := "E2EOrphanARCGCLive" + forDuration := monitoringv1.Duration("5m") + liveRule := monitoringv1.Rule{ + Alert: alertName, + Expr: intstr.FromString(`absent(nonexistent{e2e_test="orphan_arc_gc_live"})`), + For: &forDuration, + Labels: map[string]string{ + "severity": "none", + "e2e_test": "orphan_arc_gc", + }, + } + + promRule, err := createPrometheusRule(ctx, f, testNamespace, liveRule) + if err != nil { + t.Fatalf("Failed to create PrometheusRule: %v", err) + } + + var liveRuleID string + err = framework.Poll(orphanARCGCPollInterval, orphanARCGCPollTimeout, func() error { + rules, listErr := listRules(ctx, f) + if listErr != nil { + return fmt.Errorf("list rules: %w", listErr) + } + id, found := alertRuleIDByName(rules, alertName) + if !found { + return fmt.Errorf("alert %s not in GET /rules yet", alertName) + } + liveRuleID = id + return nil + }) + if err != nil { + t.Fatalf("Timeout waiting for live rule to appear: %v", err) + } + + idSuffix := fmt.Sprintf("%d", time.Now().UnixNano()) + orphanName := "e2e-ogc-orphan-" + idSuffix + liveName := "e2e-ogc-live-" + idSuffix + gitopsName := "e2e-ogc-gitops-" + idSuffix + manualName := "e2e-ogc-manual-" + idSuffix + orphanRuleID := "e2e-orphan-gc-missing-" + idSuffix + gitopsRuleID := "e2e-orphan-gc-gitops-" + idSuffix + + t.Cleanup(func() { + for _, name := range []string{orphanName, liveName, gitopsName, manualName} { + if delErr := deleteAlertRelabelConfig(ctx, f, name); delErr != nil { + t.Logf("cleanup ARC %s: %v", name, delErr) + } + } + }) + + if err := createAlertRelabelConfig(ctx, f, orphanName, map[string]string{ + managementlabels.ARCAnnotationAlertRuleIDKey: orphanRuleID, + }, nil); err != nil { + t.Fatalf("Failed to create orphan ARC: %v", err) + } + if err := createAlertRelabelConfig(ctx, f, liveName, map[string]string{ + managementlabels.ARCAnnotationAlertRuleIDKey: liveRuleID, + }, nil); err != nil { + t.Fatalf("Failed to create live ARC: %v", err) + } + if err := createAlertRelabelConfig(ctx, f, gitopsName, map[string]string{ + managementlabels.ARCAnnotationAlertRuleIDKey: gitopsRuleID, + "argocd.argoproj.io/tracking-id": "e2e-orphan-arc-gc", + }, nil); err != nil { + t.Fatalf("Failed to create GitOps ARC: %v", err) + } + if err := createAlertRelabelConfig(ctx, f, manualName, nil, nil); err != nil { + t.Fatalf("Failed to create unannotated ARC: %v", err) + } + + err = framework.Poll(orphanARCGCPollInterval, 20*time.Second, func() error { + current, getErr := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get( + ctx, promRule.Name, metav1.GetOptions{}, + ) + if getErr != nil { + return getErr + } + if current.Annotations == nil { + current.Annotations = map[string]string{} + } + current.Annotations["e2e.monitoring.openshift.io/gc-sync"] = idSuffix + _, updateErr := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Update( + ctx, current, metav1.UpdateOptions{}, + ) + return updateErr + }) + if err != nil { + t.Fatalf("Failed to update PrometheusRule to trigger GC: %v", err) + } + + err = framework.Poll(orphanARCGCPollInterval, orphanARCGCPollTimeout, func() error { + exists, existsErr := alertRelabelConfigExists(ctx, f, orphanName) + if existsErr != nil { + return existsErr + } + if exists { + return fmt.Errorf("orphan ARC %s still present", orphanName) + } + return nil + }) + if err != nil { + t.Fatalf("Timeout waiting for orphan ARC GC: %v", err) + } + + for _, keeper := range []string{liveName, gitopsName, manualName} { + exists, existsErr := alertRelabelConfigExists(ctx, f, keeper) + if existsErr != nil { + t.Fatalf("Failed to get keeper ARC %s: %v", keeper, existsErr) + } + if !exists { + t.Errorf("keeper ARC %s was deleted", keeper) + } + } +} + +func alertRuleIDByName(rules []k8s.PrometheusRule, alertName string) (string, bool) { + for _, rule := range rules { + if rule.Name != alertName { + continue + } + id := rule.Labels[k8s.AlertRuleLabelId] + if id == "" { + return "", false + } + return id, true + } + return "", false +} + +func createAlertRelabelConfig(ctx context.Context, f *framework.Framework, name string, annotations, labels map[string]string) error { + arc := &osmv1.AlertRelabelConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: k8s.ClusterMonitoringNamespace, + Annotations: annotations, + Labels: labels, + }, + Spec: osmv1.AlertRelabelConfigSpec{ + Configs: []osmv1.RelabelConfig{ + { + SourceLabels: []osmv1.LabelName{"alertname"}, + Regex: "E2EOrphanARCGCNeverMatch", + TargetLabel: "e2e_orphan_gc", + Replacement: "test", + Action: "Replace", + }, + }, + }, + } + + return framework.Poll(time.Second, 20*time.Second, func() error { + _, err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).Create( + ctx, arc, metav1.CreateOptions{}, + ) + if err == nil || apierrors.IsAlreadyExists(err) { + return nil + } + return err + }) +} + +func deleteAlertRelabelConfig(ctx context.Context, f *framework.Framework, name string) error { + err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).Delete( + ctx, name, metav1.DeleteOptions{}, + ) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil +} + +func alertRelabelConfigExists(ctx context.Context, f *framework.Framework, name string) (bool, error) { + _, err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).Get( + ctx, name, metav1.GetOptions{}, + ) + if apierrors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, err + } + return true, nil +} From c3c5ca074d332ba14c74a44da46b1c5f337559ce Mon Sep 17 00:00:00 2001 From: Shirly Radco Date: Thu, 17 Sep 2026 16:10:25 +0300 Subject: [PATCH 2/2] k8s: add AlertRelabelConfig GC metrics Expose list and delete error counters and a GitOps-orphan gauge on /metrics. Signed-off-by: Shirly Radco Co-authored-by: AI Assistant --- go.mod | 3 +- pkg/k8s/alert_relabel_config_gc.go | 19 ++-- pkg/k8s/alert_relabel_config_gc_metrics.go | 89 +++++++++++++++++ .../alert_relabel_config_gc_metrics_test.go | 99 +++++++++++++++++++ pkg/k8s/alert_relabel_config_gc_test.go | 12 ++- pkg/k8s/relabeled_rules.go | 3 +- pkg/server/server.go | 3 + pkg/server/server_test.go | 10 ++ test/e2e/framework/framework.go | 6 +- test/e2e/framework/poll.go | 13 ++- test/e2e/helpers_test.go | 27 +++++ test/e2e/orphan_arc_gc_test.go | 45 ++++++--- test/e2e/prometheus_text.go | 44 +++++++++ test/e2e/prometheus_text_test.go | 40 ++++++++ 14 files changed, 389 insertions(+), 24 deletions(-) create mode 100644 pkg/k8s/alert_relabel_config_gc_metrics.go create mode 100644 pkg/k8s/alert_relabel_config_gc_metrics_test.go create mode 100644 test/e2e/prometheus_text.go create mode 100644 test/e2e/prometheus_text_test.go diff --git a/go.mod b/go.mod index bcc7afc8a..ffb3e0f0a 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,7 @@ require ( github.com/openshift/library-go v0.0.0-20240905123346-5bdbfe35a6f5 github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.87.0 github.com/prometheus-operator/prometheus-operator/pkg/client v0.87.0 + github.com/prometheus/client_golang v1.23.2 github.com/prometheus/common v0.67.4 github.com/prometheus/prometheus v0.308.0 github.com/sirupsen/logrus v1.9.3 @@ -52,12 +53,12 @@ require ( github.com/google/uuid v1.6.0 // indirect github.com/grafana/regexp v0.0.0-20250905093917-f7b3be9d1853 // indirect github.com/json-iterator/go v1.1.12 // indirect + github.com/kylelemons/godebug v1.1.0 // indirect github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/pkg/errors v0.9.1 // indirect github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2 // indirect - github.com/prometheus/client_golang v1.23.2 // indirect github.com/prometheus/client_model v0.6.2 // indirect github.com/prometheus/procfs v0.16.1 // indirect github.com/spf13/pflag v1.0.6 // indirect diff --git a/pkg/k8s/alert_relabel_config_gc.go b/pkg/k8s/alert_relabel_config_gc.go index f7d5bac50..6ce9df533 100644 --- a/pkg/k8s/alert_relabel_config_gc.go +++ b/pkg/k8s/alert_relabel_config_gc.go @@ -8,28 +8,32 @@ import ( // gcOrphanedARCs deletes AlertRelabelConfigs whose associated alert rule no // longer exists. This handles the case where an operator (or manual action) -// removes rules from a PrometheusRule or deletes the CR entirely — the ARCs -// that were created by the plugin for classification/drop/stamp become orphans. +// removes rules from a PrometheusRule or deletes the CR entirely — the +// AlertRelabelConfigs that were created by the plugin for +// classification/drop/stamp become orphans. // -// Only ARCs carrying the plugin's alertRuleId annotation are considered. -// GitOps-managed ARCs are never deleted automatically; a warning is logged -// so that operators can clean them up manually. +// Only AlertRelabelConfigs carrying the plugin's alertRuleId annotation +// are considered. GitOps-managed configs are never deleted automatically; +// scrapeable metrics surface them so cluster-monitoring-operator can alert. // // liveRuleIDs must include every alerting-rule ID still present on a // PrometheusRule, including platform rules dropped by relabel configs. // IDs are recorded in collectAlerts before the drop continue, so a Drop -// ARC for a disabled rule is not treated as an orphan. +// AlertRelabelConfig for a disabled rule is not treated as an orphan. func (rrm *relabeledRulesManager) gcOrphanedARCs(ctx context.Context, liveRuleIDs map[string]struct{}) { if rrm.alertRelabelConfigs == nil { return } + metrics := rrm.gcMetrics arcs, err := rrm.alertRelabelConfigs.List(ctx, "") if err != nil { + metrics.observeListError() log.Errorf("orphan AlertRelabelConfig cleanup: failed to list AlertRelabelConfigs: %v", err) return } + gitOpsOrphans := 0 for i := range arcs { arc := &arcs[i] @@ -43,15 +47,18 @@ func (rrm *relabeledRulesManager) gcOrphanedARCs(ctx context.Context, liveRuleID } if IsManagedByGitOps(arc.Annotations, arc.Labels) { + gitOpsOrphans++ log.Warnf("orphan AlertRelabelConfig cleanup: AlertRelabelConfig %s/%s (ruleId=%s) is orphaned but GitOps-managed — skipping deletion, manual cleanup required", arc.Namespace, arc.Name, ruleID) continue } if err := rrm.alertRelabelConfigs.Delete(ctx, arc.Namespace, arc.Name); err != nil { + metrics.observeDeleteError() log.Errorf("orphan AlertRelabelConfig cleanup: failed to delete AlertRelabelConfig %s/%s: %v", arc.Namespace, arc.Name, err) continue } log.Infof("orphan AlertRelabelConfig cleanup: deleted orphaned AlertRelabelConfig %s/%s (ruleId=%s)", arc.Namespace, arc.Name, ruleID) } + metrics.setGitOpsOrphans(float64(gitOpsOrphans)) } diff --git a/pkg/k8s/alert_relabel_config_gc_metrics.go b/pkg/k8s/alert_relabel_config_gc_metrics.go new file mode 100644 index 000000000..2254f7527 --- /dev/null +++ b/pkg/k8s/alert_relabel_config_gc_metrics.go @@ -0,0 +1,89 @@ +package k8s + +import ( + "net/http" + + "github.com/prometheus/client_golang/prometheus" + "github.com/prometheus/client_golang/prometheus/promhttp" +) + +const ( + MetricAlertRelabelConfigGCListErrorsTotal = "monitoring_plugin_alert_relabel_config_gc_list_errors_total" + MetricAlertRelabelConfigGCDeleteErrorsTotal = "monitoring_plugin_alert_relabel_config_gc_delete_errors_total" + MetricAlertRelabelConfigGitOpsOrphans = "monitoring_plugin_alert_relabel_config_gitops_orphans" +) + +type alertRelabelConfigGCMetrics struct { + listErrors prometheus.Counter + deleteErrors prometheus.Counter + gitopsOrphans prometheus.Gauge +} + +func newAlertRelabelConfigGCMetrics() *alertRelabelConfigGCMetrics { + return &alertRelabelConfigGCMetrics{ + listErrors: prometheus.NewCounter(prometheus.CounterOpts{ + Name: MetricAlertRelabelConfigGCListErrorsTotal, + Help: "Count of failed List calls while cleaning up orphaned AlertRelabelConfigs.", + }), + deleteErrors: prometheus.NewCounter(prometheus.CounterOpts{ + Name: MetricAlertRelabelConfigGCDeleteErrorsTotal, + Help: "Count of failed Delete calls while cleaning up orphaned AlertRelabelConfigs.", + }), + gitopsOrphans: prometheus.NewGauge(prometheus.GaugeOpts{ + Name: MetricAlertRelabelConfigGitOpsOrphans, + Help: "Number of GitOps-managed AlertRelabelConfigs that are orphaned and were not deleted.", + }), + } +} + +func (m *alertRelabelConfigGCMetrics) mustRegister(reg prometheus.Registerer) { + reg.MustRegister(m.listErrors, m.deleteErrors, m.gitopsOrphans) +} + +func (m *alertRelabelConfigGCMetrics) observeListError() { + if m == nil { + return + } + m.listErrors.Inc() +} + +func (m *alertRelabelConfigGCMetrics) observeDeleteError() { + if m == nil { + return + } + m.deleteErrors.Inc() +} + +func (m *alertRelabelConfigGCMetrics) setGitOpsOrphans(n float64) { + if m == nil { + return + } + m.gitopsOrphans.Set(n) +} + +var ( + alertRelabelConfigGCMetricsRegistry = prometheus.NewRegistry() + defaultAlertRelabelConfigGCMetrics = newAlertRelabelConfigGCMetrics() +) + +func init() { + defaultAlertRelabelConfigGCMetrics.mustRegister(alertRelabelConfigGCMetricsRegistry) +} + +// AlertRelabelConfigGCMetricsRegistry is the registry served at /metrics +// when alert-management-api is enabled. Additional collectors should +// register here so a single scrape endpoint exposes all series. +func AlertRelabelConfigGCMetricsRegistry() *prometheus.Registry { + return alertRelabelConfigGCMetricsRegistry +} + +// AlertRelabelConfigGCMetricsHandler serves the GC metrics registry. +func AlertRelabelConfigGCMetricsHandler() http.Handler { + return promhttp.HandlerFor(alertRelabelConfigGCMetricsRegistry, promhttp.HandlerOpts{}) +} + +// EmptyMetricsHandler serves an empty Prometheus registry so /metrics +// still returns 200 when alert-management-api is off. +func EmptyMetricsHandler() http.Handler { + return promhttp.HandlerFor(prometheus.NewRegistry(), promhttp.HandlerOpts{}) +} diff --git a/pkg/k8s/alert_relabel_config_gc_metrics_test.go b/pkg/k8s/alert_relabel_config_gc_metrics_test.go new file mode 100644 index 000000000..14f64f6ee --- /dev/null +++ b/pkg/k8s/alert_relabel_config_gc_metrics_test.go @@ -0,0 +1,99 @@ +package k8s + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/prometheus/client_golang/prometheus/testutil" + + osmv1 "github.com/openshift/api/monitoring/v1" +) + +func TestGCOrphanedARCs_ListErrorIncrementsMetric(t *testing.T) { + metrics := newAlertRelabelConfigGCMetrics() + mock := &mockARCInterface{listErr: errors.New("list failed")} + rrm := &relabeledRulesManager{alertRelabelConfigs: mock, gcMetrics: metrics} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) + + if got := testutil.ToFloat64(metrics.listErrors); got != 1 { + t.Fatalf("list errors = %v, want 1", got) + } + if got := testutil.ToFloat64(metrics.deleteErrors); got != 0 { + t.Fatalf("delete errors = %v, want 0", got) + } +} + +func TestGCOrphanedARCs_DeleteErrorIncrementsMetric(t *testing.T) { + metrics := newAlertRelabelConfigGCMetrics() + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-orphan": newARC("openshift-monitoring", "arc-orphan", "rule-gone", nil, nil), + }, + deleteErr: errors.New("delete failed"), + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock, gcMetrics: metrics} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{}) + + if got := testutil.ToFloat64(metrics.deleteErrors); got != 1 { + t.Fatalf("delete errors = %v, want 1", got) + } + if len(mock.deleted) != 0 { + t.Fatalf("expected no deletions, got %v", mock.deleted) + } +} + +func TestGCOrphanedARCs_GitOpsOrphanSetsGauge(t *testing.T) { + metrics := newAlertRelabelConfigGCMetrics() + mock := &mockARCInterface{ + arcs: map[string]*osmv1.AlertRelabelConfig{ + "openshift-monitoring/arc-gitops": newARC("openshift-monitoring", "arc-gitops", "rule-gone", + map[string]string{"argocd.argoproj.io/tracking-id": "some-id"}, nil), + "openshift-monitoring/arc-live": newARC("openshift-monitoring", "arc-live", "rule-alive", nil, nil), + }, + } + rrm := &relabeledRulesManager{alertRelabelConfigs: mock, gcMetrics: metrics} + + rrm.gcOrphanedARCs(context.Background(), map[string]struct{}{"rule-alive": {}}) + + if got := testutil.ToFloat64(metrics.gitopsOrphans); got != 1 { + t.Fatalf("gitops orphans = %v, want 1", got) + } +} + +func TestAlertRelabelConfigGCMetricsHandlerExposesSeries(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/metrics", nil) + rec := httptest.NewRecorder() + AlertRelabelConfigGCMetricsHandler().ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status %d", rec.Code) + } + body := rec.Body.String() + for _, name := range []string{ + MetricAlertRelabelConfigGCListErrorsTotal, + MetricAlertRelabelConfigGCDeleteErrorsTotal, + MetricAlertRelabelConfigGitOpsOrphans, + } { + if !strings.Contains(body, name) { + t.Errorf("handler body missing metric %s:\n%s", name, body) + } + } +} + +func TestEmptyMetricsHandlerHasNoGCSeries(t *testing.T) { + req := httptest.NewRequest(http.MethodGet, "/metrics", nil) + rec := httptest.NewRecorder() + EmptyMetricsHandler().ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("status %d", rec.Code) + } + body := rec.Body.String() + if strings.Contains(body, MetricAlertRelabelConfigGCListErrorsTotal) { + t.Fatalf("empty handler unexpectedly exposed GC metrics: %s", body) + } +} diff --git a/pkg/k8s/alert_relabel_config_gc_test.go b/pkg/k8s/alert_relabel_config_gc_test.go index 21acc246f..aea41f104 100644 --- a/pkg/k8s/alert_relabel_config_gc_test.go +++ b/pkg/k8s/alert_relabel_config_gc_test.go @@ -11,11 +11,16 @@ import ( ) type mockARCInterface struct { - arcs map[string]*osmv1.AlertRelabelConfig - deleted []string + arcs map[string]*osmv1.AlertRelabelConfig + deleted []string + listErr error + deleteErr error } func (m *mockARCInterface) List(_ context.Context, _ string) ([]osmv1.AlertRelabelConfig, error) { + if m.listErr != nil { + return nil, m.listErr + } var result []osmv1.AlertRelabelConfig for _, arc := range m.arcs { result = append(result, *arc) @@ -37,6 +42,9 @@ func (m *mockARCInterface) Create(_ context.Context, arc osmv1.AlertRelabelConfi func (m *mockARCInterface) Update(_ context.Context, _ osmv1.AlertRelabelConfig) error { return nil } func (m *mockARCInterface) Delete(_ context.Context, ns, name string) error { + if m.deleteErr != nil { + return m.deleteErr + } m.deleted = append(m.deleted, ns+"/"+name) delete(m.arcs, ns+"/"+name) return nil diff --git a/pkg/k8s/relabeled_rules.go b/pkg/k8s/relabeled_rules.go index c1104cf84..f117f31fa 100644 --- a/pkg/k8s/relabeled_rules.go +++ b/pkg/k8s/relabeled_rules.go @@ -45,7 +45,6 @@ const ( AppKubernetesIoComponent = "app.kubernetes.io/component" AppKubernetesIoComponentAlertManagementApi = "alert-management-api" - AppKubernetesIoComponentMonitoringPlugin = "monitoring-plugin" relabeledRulesSyncKeyInitial = "initial-sync" relabeledRulesSyncKeyPrometheusRule = "prometheus-rule-sync" @@ -59,6 +58,7 @@ type relabeledRulesManager struct { alertRelabelConfigs AlertRelabelConfigInterface prometheusRulesInformer cache.SharedIndexInformer secretInformer cache.SharedIndexInformer + gcMetrics *alertRelabelConfigGCMetrics // relabeledRules stores the relabeled rules in memory relabeledRules map[string]monitoringv1.Rule @@ -92,6 +92,7 @@ func newRelabeledRulesManager(ctx context.Context, namespaceManager NamespaceInt alertRelabelConfigs: alertRelabelConfigs, prometheusRulesInformer: prometheusRulesInformer, secretInformer: secretInformer, + gcMetrics: defaultAlertRelabelConfigGCMetrics, } _, err := rrm.prometheusRulesInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ diff --git a/pkg/server/server.go b/pkg/server/server.go index 5c5c49800..b429657fb 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -290,6 +290,9 @@ func setupRoutes(cfg *Config, managementClient management.Client) (*mux.Router, if managementClient != nil { managementRouter := managementrouter.New(managementClient) router.PathPrefix("/api/v1/alerting").Handler(managementRouter) + router.Path("/metrics").Handler(k8s.AlertRelabelConfigGCMetricsHandler()) + } else { + router.Path("/metrics").Handler(k8s.EmptyMetricsHandler()) } router.PathPrefix("/").Handler(filesHandler(http.Dir(cfg.StaticPath))) diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index b2a415dc7..15b9e4b97 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -21,6 +21,8 @@ import ( "time" "github.com/stretchr/testify/require" + + "github.com/openshift/monitoring-plugin/pkg/k8s" ) type httpClientConfig struct { @@ -152,6 +154,14 @@ func TestServerRunning(t *testing.T) { t.Fatalf("Failed: could not fetch features endpoint: %v", err) } + metricsBody, err := getRequestResults(t, httpClient, serverURL+"/metrics") + if err != nil { + t.Fatalf("Failed: could not fetch /metrics: %v", err) + } + if strings.Contains(metricsBody, k8s.MetricAlertRelabelConfigGCListErrorsTotal) { + t.Fatalf("expected no GC metrics without alert-management-api, got %q", metricsBody) + } + // sanity check - make sure we cannot get to a bogus context path if _, err = getRequestResults(t, httpClient, serverURL+"/badroot"); err == nil { t.Fatalf("Failed: Should have failed going to /badroot") diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index d08716ee9..5e64ffda0 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -40,6 +40,8 @@ type Framework struct { type CleanupFunc func() error +const namespaceCleanupTimeout = 20 * time.Second + // New creates a Framework backed by a real Kubernetes cluster. It reads // KUBECONFIG and PLUGIN_URL from the environment and returns a singleton // so that expensive client setup happens only once per test binary. @@ -142,7 +144,9 @@ func (f *Framework) createNamespace(ctx context.Context, name string, isClusterM } return testNamespace, func() error { - return f.Clientset.CoreV1().Namespaces().Delete(ctx, testNamespace, metav1.DeleteOptions{}) + cleanupCtx, cancel := context.WithTimeout(context.Background(), namespaceCleanupTimeout) + defer cancel() + return f.Clientset.CoreV1().Namespaces().Delete(cleanupCtx, testNamespace, metav1.DeleteOptions{}) }, nil } diff --git a/test/e2e/framework/poll.go b/test/e2e/framework/poll.go index 8163cd183..016eafea3 100644 --- a/test/e2e/framework/poll.go +++ b/test/e2e/framework/poll.go @@ -13,9 +13,18 @@ import ( // Poll calls f every interval until it returns nil or timeout elapses. // On timeout the last observed error is wrapped with wait.ErrWaitTimeout. func Poll(interval, timeout time.Duration, f func() error) error { + return PollWithContext(context.Background(), interval, timeout, func(context.Context) error { + return f() + }) +} + +// PollWithContext is Poll with a parent context. The callback receives the +// poll-bounded context so Kubernetes and HTTP calls cancel when the timeout +// elapses. +func PollWithContext(ctx context.Context, interval, timeout time.Duration, f func(context.Context) error) error { var lastErr error - err := wait.PollUntilContextTimeout(context.Background(), interval, timeout, true, func(context.Context) (bool, error) { - if lastErr = f(); lastErr != nil { + err := wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(pollCtx context.Context) (bool, error) { + if lastErr = f(pollCtx); lastErr != nil { return false, nil } return true, nil diff --git a/test/e2e/helpers_test.go b/test/e2e/helpers_test.go index cbd2e98eb..7885a586d 100644 --- a/test/e2e/helpers_test.go +++ b/test/e2e/helpers_test.go @@ -135,3 +135,30 @@ func mustCreateRule(ctx context.Context, t *testing.T, f *framework.Framework, n } return id } + +func fetchPluginMetrics(ctx context.Context, f *framework.Framework) (body string, err error) { + req, err := f.AuthorizedRequest(ctx, http.MethodGet, f.PluginURL+"/metrics", nil) + if err != nil { + return "", err + } + + resp, err := f.HTTPClient().Do(req) + if err != nil { + return "", err + } + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil && err == nil { + err = fmt.Errorf("closing response body: %w", closeErr) + } + }() + + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("unexpected status code: %d", resp.StatusCode) + } + + raw, err := io.ReadAll(resp.Body) + if err != nil { + return "", err + } + return string(raw), nil +} diff --git a/test/e2e/orphan_arc_gc_test.go b/test/e2e/orphan_arc_gc_test.go index 6213de28a..f57edae5e 100644 --- a/test/e2e/orphan_arc_gc_test.go +++ b/test/e2e/orphan_arc_gc_test.go @@ -22,6 +22,8 @@ import ( const ( orphanARCGCPollInterval = time.Second orphanARCGCPollTimeout = 2 * time.Minute + orphanARCGCOpTimeout = 20 * time.Second + orphanARCGCTestTimeout = 2*orphanARCGCPollTimeout + 2*orphanARCGCOpTimeout ) // TestOrphanAlertRelabelConfigGC creates plugin-owned AlertRelabelConfigs @@ -33,7 +35,8 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { t.Fatalf("Failed to create framework: %v", err) } - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), orphanARCGCTestTimeout) + defer cancel() // Cluster-monitoring namespace so GET /rules can see the live rule // (e2e-management-api does not enable user-workload monitoring). @@ -65,8 +68,8 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { } var liveRuleID string - err = framework.Poll(orphanARCGCPollInterval, orphanARCGCPollTimeout, func() error { - rules, listErr := listRules(ctx, f) + err = framework.PollWithContext(ctx, orphanARCGCPollInterval, orphanARCGCPollTimeout, func(pollCtx context.Context) error { + rules, listErr := listRules(pollCtx, f) if listErr != nil { return fmt.Errorf("list rules: %w", listErr) } @@ -90,8 +93,10 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { gitopsRuleID := "e2e-orphan-gc-gitops-" + idSuffix t.Cleanup(func() { + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), orphanARCGCOpTimeout) + defer cleanupCancel() for _, name := range []string{orphanName, liveName, gitopsName, manualName} { - if delErr := deleteAlertRelabelConfig(ctx, f, name); delErr != nil { + if delErr := deleteAlertRelabelConfig(cleanupCtx, f, name); delErr != nil { t.Logf("cleanup ARC %s: %v", name, delErr) } } @@ -117,9 +122,9 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { t.Fatalf("Failed to create unannotated ARC: %v", err) } - err = framework.Poll(orphanARCGCPollInterval, 20*time.Second, func() error { + err = framework.PollWithContext(ctx, orphanARCGCPollInterval, orphanARCGCOpTimeout, func(pollCtx context.Context) error { current, getErr := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Get( - ctx, promRule.Name, metav1.GetOptions{}, + pollCtx, promRule.Name, metav1.GetOptions{}, ) if getErr != nil { return getErr @@ -129,7 +134,7 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { } current.Annotations["e2e.monitoring.openshift.io/gc-sync"] = idSuffix _, updateErr := f.Monitoringv1clientset.MonitoringV1().PrometheusRules(testNamespace).Update( - ctx, current, metav1.UpdateOptions{}, + pollCtx, current, metav1.UpdateOptions{}, ) return updateErr }) @@ -137,8 +142,8 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { t.Fatalf("Failed to update PrometheusRule to trigger GC: %v", err) } - err = framework.Poll(orphanARCGCPollInterval, orphanARCGCPollTimeout, func() error { - exists, existsErr := alertRelabelConfigExists(ctx, f, orphanName) + err = framework.PollWithContext(ctx, orphanARCGCPollInterval, orphanARCGCPollTimeout, func(pollCtx context.Context) error { + exists, existsErr := alertRelabelConfigExists(pollCtx, f, orphanName) if existsErr != nil { return existsErr } @@ -160,6 +165,24 @@ func TestOrphanAlertRelabelConfigGC(t *testing.T) { t.Errorf("keeper ARC %s was deleted", keeper) } } + + err = framework.PollWithContext(ctx, orphanARCGCPollInterval, orphanARCGCOpTimeout, func(pollCtx context.Context) error { + body, metricsErr := fetchPluginMetrics(pollCtx, f) + if metricsErr != nil { + return metricsErr + } + value, parseErr := metricSampleValue(body, k8s.MetricAlertRelabelConfigGitOpsOrphans) + if parseErr != nil { + return parseErr + } + if value <= 0 { + return fmt.Errorf("%s = %g, want > 0", k8s.MetricAlertRelabelConfigGitOpsOrphans, value) + } + return nil + }) + if err != nil { + t.Fatalf("Timeout waiting for GitOps-orphan metric: %v", err) + } } func alertRuleIDByName(rules []k8s.PrometheusRule, alertName string) (string, bool) { @@ -197,9 +220,9 @@ func createAlertRelabelConfig(ctx context.Context, f *framework.Framework, name }, } - return framework.Poll(time.Second, 20*time.Second, func() error { + return framework.PollWithContext(ctx, time.Second, orphanARCGCOpTimeout, func(pollCtx context.Context) error { _, err := f.Osmv1clientset.MonitoringV1().AlertRelabelConfigs(k8s.ClusterMonitoringNamespace).Create( - ctx, arc, metav1.CreateOptions{}, + pollCtx, arc, metav1.CreateOptions{}, ) if err == nil || apierrors.IsAlreadyExists(err) { return nil diff --git a/test/e2e/prometheus_text.go b/test/e2e/prometheus_text.go new file mode 100644 index 000000000..1da264238 --- /dev/null +++ b/test/e2e/prometheus_text.go @@ -0,0 +1,44 @@ +package e2e + +import ( + "fmt" + "strings" + + dto "github.com/prometheus/client_model/go" + "github.com/prometheus/common/expfmt" + "github.com/prometheus/common/model" +) + +func metricSampleValue(body, name string) (float64, error) { + parser := expfmt.NewTextParser(model.UTF8Validation) + families, err := parser.TextToMetricFamilies(strings.NewReader(body)) + if err != nil { + return 0, fmt.Errorf("parse prometheus text: %w", err) + } + family, ok := families[name] + if !ok { + return 0, fmt.Errorf("metric %s not found", name) + } + var sum float64 + for _, sample := range family.GetMetric() { + value, err := metricPointValue(family.GetType(), sample) + if err != nil { + return 0, err + } + sum += value + } + return sum, nil +} + +func metricPointValue(metricType dto.MetricType, sample *dto.Metric) (float64, error) { + switch metricType { + case dto.MetricType_GAUGE: + return sample.GetGauge().GetValue(), nil + case dto.MetricType_COUNTER: + return sample.GetCounter().GetValue(), nil + case dto.MetricType_UNTYPED: + return sample.GetUntyped().GetValue(), nil + default: + return 0, fmt.Errorf("unsupported metric type %v", metricType) + } +} diff --git a/test/e2e/prometheus_text_test.go b/test/e2e/prometheus_text_test.go new file mode 100644 index 000000000..585cbbe11 --- /dev/null +++ b/test/e2e/prometheus_text_test.go @@ -0,0 +1,40 @@ +package e2e + +import "testing" + +func TestMetricSampleValueGaugePositive(t *testing.T) { + body := `# HELP monitoring_plugin_alert_relabel_config_gitops_orphans GitOps orphans +# TYPE monitoring_plugin_alert_relabel_config_gitops_orphans gauge +monitoring_plugin_alert_relabel_config_gitops_orphans 2 +` + got, err := metricSampleValue(body, "monitoring_plugin_alert_relabel_config_gitops_orphans") + if err != nil { + t.Fatalf("metricSampleValue: %v", err) + } + if got != 2 { + t.Fatalf("got %v, want 2", got) + } +} + +func TestMetricSampleValueZeroIsFound(t *testing.T) { + body := `# TYPE monitoring_plugin_alert_relabel_config_gitops_orphans gauge +monitoring_plugin_alert_relabel_config_gitops_orphans 0 +` + got, err := metricSampleValue(body, "monitoring_plugin_alert_relabel_config_gitops_orphans") + if err != nil { + t.Fatalf("metricSampleValue: %v", err) + } + if got != 0 { + t.Fatalf("got %v, want 0", got) + } +} + +func TestMetricSampleValueMissing(t *testing.T) { + body := `# TYPE other_metric gauge +other_metric 1 +` + _, err := metricSampleValue(body, "monitoring_plugin_alert_relabel_config_gitops_orphans") + if err == nil { + t.Fatal("expected error for missing metric") + } +}