Skip to content

OCPBUGS-104856: clear unmanaged-route metric when the route is removed #102

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jcmoraisjr wants to merge 1 commit into
base: main
Choose a base branch
from
+153 −38
Open

OCPBUGS-104856: clear unmanaged-route metric when the route is removed #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion pkg/route/ingress/ingress.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
Expand Down
27 changes: 25 additions & 2 deletions pkg/route/ingress/metrics.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
147 changes: 112 additions & 35 deletions pkg/route/ingress/metrics_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
},
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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)
}
}
}
})
}
}
Expand Down Expand Up @@ -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,
Expand Down