From 6111c6d886ef51629b1d4d86e5cb16b5f536d4a1 Mon Sep 17 00:00:00 2001 From: Joao Morais Date: Thu, 20 Aug 2026 21:24:43 -0300 Subject: [PATCH] OCPBUGS-104856: clear unmanaged-route metric when the route is removed The unmanaged_owner gauge is a GaugeVec keyed by route name/namespace/ host; Collect() only iterates routes that currently exist in the lister cache, so a route that's deleted left its last-known value stuck in the gauge forever, keeping any alert based on it firing even after the unmanaged route itself was gone. This update tracks the set of routes flagged unmanaged on each Collect and zero out any that drop out of that set on the next pass. Guard the tracked set with a mutex since concurrent scrapes can call Collect() at the same time. https://redhat.atlassian.net/browse/OCPBUGS-104856 Co-Authored-By: Claude Sonnet 5 --- pkg/route/ingress/ingress.go | 17 +++- pkg/route/ingress/metrics.go | 27 +++++- pkg/route/ingress/metrics_test.go | 147 +++++++++++++++++++++++------- 3 files changed, 153 insertions(+), 38 deletions(-) diff --git a/pkg/route/ingress/ingress.go b/pkg/route/ingress/ingress.go index 4177fd116..d85464a7f 100644 --- a/pkg/route/ingress/ingress.go +++ b/pkg/route/ingress/ingress.go @@ -103,6 +103,12 @@ type Controller struct { metricsCreated bool metricsCreateOnce sync.Once metricsCreateLock sync.RWMutex + + // flaggedUnmanagedRoutes is the set of route metrics flagged as unmanaged. + // This set is used to clear the metric when the route is removed from the + // apiserver. + flaggedUnmanagedRoutes sets.Set[routeMetricLabels] + flaggedUnmanagedRoutesLock sync.Mutex } // expectations track an upcoming change to a named resource related @@ -113,6 +119,13 @@ type expectations struct { expect map[queueKey]sets.String } +// routeMetricLabels defines the route metric labels as a comparable type +type routeMetricLabels struct { + name string + namespace string + host string +} + // newExpectations returns a tracking object for upcoming events // that the controller may expect to happen. func newExpectations() *expectations { @@ -195,6 +208,8 @@ func NewController(eventsClient kv1core.EventsGetter, routeClient routeclient.Ro routeLister: routes.Lister(), serviceLister: services.Lister(), + flaggedUnmanagedRoutes: sets.New[routeMetricLabels](), + syncs: []cache.InformerSynced{ ingresses.Informer().HasSynced, secrets.Informer().HasSynced, @@ -387,7 +402,7 @@ func (c *Controller) sync(key queueKey) error { ingress, err := c.ingressLister.Ingresses(key.namespace).Get(key.name) if kerrors.IsNotFound(err) { - c.ResetIngressMetrics(key.namespace, key.name) + c.resetIngressMetrics(key.namespace, key.name) return nil } if err != nil { diff --git a/pkg/route/ingress/metrics.go b/pkg/route/ingress/metrics.go index 7c383c846..ec6eb7ecb 100644 --- a/pkg/route/ingress/metrics.go +++ b/pkg/route/ingress/metrics.go @@ -5,6 +5,7 @@ import ( "github.com/prometheus/client_golang/prometheus" "k8s.io/apimachinery/pkg/labels" utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/sets" "github.com/openshift/route-controller-manager/pkg/routecontroller" ) @@ -76,6 +77,7 @@ func (c *Controller) Collect(ch chan<- prometheus.Metric) { return } + currentUnmanagedRoutes := sets.New[routeMetricLabels]() for _, routeInstance := range routeInstances { labelVal := 0 if owner, have := hasIngressOwnerRef(routeInstance.OwnerReferences); have { @@ -97,16 +99,37 @@ func (c *Controller) Collect(ch chan<- prometheus.Metric) { } } unmanagedRoutes.WithLabelValues(routeInstance.Name, routeInstance.Namespace, routeInstance.Spec.Host).Set(float64(labelVal)) + if labelVal > 0 { + currentUnmanagedRoutes.Insert(routeMetricLabels{ + name: routeInstance.Name, + namespace: routeInstance.Namespace, + host: routeInstance.Spec.Host, + }) + } } + c.resetUnusedUnmanagedRoutesMetrics(currentUnmanagedRoutes) unmanagedRoutes.Collect(ch) } -// ResetIngressMetrics clears metrics for the specified ingress by setting its +// resetIngressMetrics clears metrics for the specified ingress by setting its // series data to 0. This is appropriate to do when an ingress object is // deleted to prevent stale metrics from triggering alerts. As Collect only // updates metrics for ingresses that exist at the time when Collect is called, // it does not clear metrics for deleted routes. -func (c *Controller) ResetIngressMetrics(namespace, ingressName string) { +func (c *Controller) resetIngressMetrics(namespace, ingressName string) { ingressesWithoutClassName.WithLabelValues(ingressName, namespace).Set(0.0) } + +// resetUnusedUnmanagedRoutesMetrics zeroes the metric for routes that were flagged +// unmanaged in a previous Collect call but aren't in the current flaggedRoutes set, +// e.g. because the route was deleted or its owner became managed again. +func (c *Controller) resetUnusedUnmanagedRoutesMetrics(currentUnmanagedRoutes sets.Set[routeMetricLabels]) { + c.flaggedUnmanagedRoutesLock.Lock() + defer c.flaggedUnmanagedRoutesLock.Unlock() + + for route := range c.flaggedUnmanagedRoutes.Difference(currentUnmanagedRoutes) { + unmanagedRoutes.WithLabelValues(route.name, route.namespace, route.host).Set(0.0) + } + c.flaggedUnmanagedRoutes = currentUnmanagedRoutes +} diff --git a/pkg/route/ingress/metrics_test.go b/pkg/route/ingress/metrics_test.go index 8632fa5f0..d314da65b 100644 --- a/pkg/route/ingress/metrics_test.go +++ b/pkg/route/ingress/metrics_test.go @@ -34,12 +34,54 @@ func TestMetrics(t *testing.T) { customIngressClassName := "custom" openshiftDefaultIngressClassName := "openshift-default" + unmanagedIngress := networkingv1.Ingress{ + ObjectMeta: metav1.ObjectMeta{ + Name: "not-managed", + Namespace: "test", + }, + Spec: networkingv1.IngressSpec{ + IngressClassName: &customIngressClassName, + }, + } + + unmanagedIngressClass := networkingv1.IngressClass{ + // IngressClass specifying "acme.io/ingress-controller" controller + ObjectMeta: metav1.ObjectMeta{ + Name: customIngressClassName, + }, + Spec: networkingv1.IngressClassSpec{ + Controller: "acme.io/ingress-controller", + }, + } + unmanagedRoute := func(host string) *routev1.Route { + return &routev1.Route{ + ObjectMeta: metav1.ObjectMeta{ + Name: "owned-by-unmanaged", + Namespace: "test", + OwnerReferences: []metav1.OwnerReference{{APIVersion: "networking.k8s.io/v1", Kind: "Ingress", Name: "not-managed", Controller: &boolTrue}}, + }, + Spec: routev1.RouteSpec{ + Host: host, + }, + } + } + testCases := []struct { - name string + name string + + // listers for the first scrape ingressLister *ingressLister ingressclassLister *ingressclassLister routeLister *routeLister + + // listers for the second scrape, optional + ingressLister2 *ingressLister + ingressclassLister2 *ingressclassLister + routeLister2 *routeLister + + // response of the first and second scrapes expectedResponse string + expectedResponses2 []string }{ { name: "Ingress with nil IngressClassName should return 1", @@ -118,43 +160,13 @@ func TestMetrics(t *testing.T) { { name: "Route with an unmanaged Ingress owner should return 1", ingressLister: &ingressLister{ - Items: []*networkingv1.Ingress{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "not-managed", - Namespace: "test", - }, - Spec: networkingv1.IngressSpec{ - IngressClassName: &customIngressClassName, - }, - }, - }, + Items: []*networkingv1.Ingress{&unmanagedIngress}, }, ingressclassLister: &ingressclassLister{ - Items: []*networkingv1.IngressClass{ - { // IngressClass specifying "acme.io/ingress-controller" controller - ObjectMeta: metav1.ObjectMeta{ - Name: customIngressClassName, - }, - Spec: networkingv1.IngressClassSpec{ - Controller: "acme.io/ingress-controller", - }, - }, - }, + Items: []*networkingv1.IngressClass{&unmanagedIngressClass}, }, routeLister: &routeLister{ - Items: []*routev1.Route{ - { - ObjectMeta: metav1.ObjectMeta{ - Name: "owned-by-unmanaged", - Namespace: "test", - OwnerReferences: []metav1.OwnerReference{{APIVersion: "networking.k8s.io/v1", Kind: "Ingress", Name: "not-managed", Controller: &boolTrue}}, - }, - Spec: routev1.RouteSpec{ - Host: "test.com", - }, - }, - }, + Items: []*routev1.Route{unmanagedRoute("test.com")}, }, expectedResponse: "openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"test.com\",name=\"owned-by-unmanaged\",namespace=\"test\"} 1", }, @@ -201,6 +213,55 @@ func TestMetrics(t *testing.T) { }, expectedResponse: "openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"test.com\",name=\"owned-by-managed\",namespace=\"test\"} 0", }, + { + name: "Route removed after being flagged unmanaged resets its metric to 0", + ingressLister: &ingressLister{ + Items: []*networkingv1.Ingress{&unmanagedIngress}, + }, + ingressclassLister: &ingressclassLister{ + Items: []*networkingv1.IngressClass{&unmanagedIngressClass}, + }, + routeLister: &routeLister{ + Items: []*routev1.Route{unmanagedRoute("test.com")}, + }, + expectedResponse: "openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"test.com\",name=\"owned-by-unmanaged\",namespace=\"test\"} 1", + ingressLister2: &ingressLister{ + Items: []*networkingv1.Ingress{&unmanagedIngress}, + }, + ingressclassLister2: &ingressclassLister{ + Items: []*networkingv1.IngressClass{&unmanagedIngressClass}, + }, + routeLister2: &routeLister{ + Items: []*routev1.Route{}, + }, + expectedResponses2: []string{"openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"test.com\",name=\"owned-by-unmanaged\",namespace=\"test\"} 0"}, + }, + { + name: "Route host change resets the old host's metric and flags the new host", + ingressLister: &ingressLister{ + Items: []*networkingv1.Ingress{&unmanagedIngress}, + }, + ingressclassLister: &ingressclassLister{ + Items: []*networkingv1.IngressClass{&unmanagedIngressClass}, + }, + routeLister: &routeLister{ + Items: []*routev1.Route{unmanagedRoute("test.com")}, + }, + expectedResponse: "openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"test.com\",name=\"owned-by-unmanaged\",namespace=\"test\"} 1", + ingressLister2: &ingressLister{ + Items: []*networkingv1.Ingress{&unmanagedIngress}, + }, + ingressclassLister2: &ingressclassLister{ + Items: []*networkingv1.IngressClass{&unmanagedIngressClass}, + }, + routeLister2: &routeLister{ + Items: []*routev1.Route{unmanagedRoute("acme.local")}, + }, + expectedResponses2: []string{ + "openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"test.com\",name=\"owned-by-unmanaged\",namespace=\"test\"} 0", + "openshift_ingress_to_route_controller_route_with_unmanaged_owner{host=\"acme.local\",name=\"owned-by-unmanaged\",namespace=\"test\"} 1", + }, + }, } i := ingressLister{} @@ -233,6 +294,22 @@ func TestMetrics(t *testing.T) { if !strings.Contains(respStr, tc.expectedResponse) { t.Errorf("expected string %s did not appear in %s", tc.expectedResponse, respStr) } + + if tc.ingressLister2 != nil || tc.ingressclassLister2 != nil || tc.routeLister2 != nil { + i.Items = tc.ingressLister2.Items + ic.Items = tc.ingressclassLister2.Items + r.Items = tc.routeLister2.Items + + rw := &fakeResponseWriter{header: http.Header{}} + h.ServeHTTP(rw, &http.Request{}) + + respStr2 := rw.String() + for _, response := range tc.expectedResponses2 { + if !strings.Contains(respStr2, response) { + t.Errorf("second scrape - expected string %s did not appear in %s", tc.expectedResponses2, respStr2) + } + } + } }) } } @@ -291,7 +368,7 @@ func Test_ResetIngressMetrics(t *testing.T) { t.Log("Simulate deletion of ingress2 and update of ingress3.") c.ingressLister = &ingressLister{Items: []*networkingv1.Ingress{i1, i3}} - c.ResetIngressMetrics(i2.Namespace, i2.Name) + c.resetIngressMetrics(i2.Namespace, i2.Name) i3.Spec.IngressClassName = &defaultIngressClassName assertMetrics(t,