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,