OCPBUGS-3541: Don't create route metrics for ingress controllers that are not admitted #869

gcs278 · 2022-12-13T23:59:33Z

To fix OCPBUGS-3541, don't create metrics for ingress controller that are not admitted. The Ingress Operator won't add the finalizer if the Ingress Controller was not admitted. Since there is no finalizer, we can't delete any metrics that we created for the Ingress Controller upon deletion. Only adding metrics for Ingress Controllers that are admitted will correct this bug.

Furthermore, if an ingress controller is not admitted, it can't have any routes, so there isn't a large point in creating the metrics for it yet.

openshift-ci-robot · 2022-12-13T23:59:37Z

@gcs278: This pull request references Jira Issue OCPBUGS-3541, which is invalid:

expected the bug to target the "4.13.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

To fix OCPBUGS-3541, don't create metrics for ingress controller that are not admitted. The Ingress Operator won't add the finalizer if the Ingress Controller was not admitted. Since there is no finalizer, we can't delete any metrics that we created for the Ingress Controller upon deletion. Only adding metrics for Ingress Controllers that are admitted will correct this bug.

Furthermore, if an ingress controller is not admitted, it can't have any routes, so there isn't a large point in creating the metrics for it yet.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gcs278 · 2022-12-14T00:03:04Z

/jira refresh

openshift-ci-robot · 2022-12-14T00:03:10Z

@gcs278: This pull request references Jira Issue OCPBUGS-3541, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.13.0) matches configured target version for branch (4.13.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @lihongan

In response to this:

/jira refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Miciah · 2022-12-14T00:11:11Z

pkg/operator/controller/route-metrics/controller.go

@@ -3,6 +3,7 @@ package routemetrics
 import (
 	"context"
 	"fmt"
+	"github.com/openshift/cluster-ingress-operator/pkg/util/ingresscontroller"


This import can be grouped with the logf import.

Now the "golang.org/x/time/rate" got moved. * grin *. Can you put "golang.org/x/time/rate" back where it was?

Sorry I moved it cause it didn't seem related to openshift packages, but moved it back. Not sure what the organization is supposed to be.

Fair enough. I don't know where this is clearly documented, and we are inconsistent with the ordering of imports across the files in this repository (and other repositories). We usually group imports into the following groups and ordering:

Standard packages (e.g. "context", "fmt", "time").

General non-standard packages (e.g. "github.com/davecgh/go-spew/spew", "github.com/google/go-cmp/cmp", "golang.org/x/time/whatever").

OpenShift packages (e.g. "github.com/openshift/api/operator/v1", "github.com/openshift/library-go/pkg/crypto", "github.com/openshift/cluster-ingress-operator/whatever"), except ofttimes we put github.com/openshift/cluster-ingress-operator/* in its own group, usually before or after other OpenShift packages and in a couple cases as the last group of imports.

Kubernetes packages (e.g. "k8s.io/api/core/v1", "k8s.io/apimachinery/pkg/api/errors"), except sometimes we put k8s.io/api/* and k8s.io/apimachinery/* in separate groups, and we usually put sigs.k8s.io/controller-runtime/* in another group after that.

So the basic pattern is typically this: standard packages, non-standard, github.com/openshift/*, k8s.io/*, sigs.k8s.io/*. However, we have a lot of exceptions, and we have a lot of imports that just don't follow any rules (I often see people do weird things with their imports, some editors screw with import ordering, and I often let it go during reviews).

This closest thing I have found to an upstream guideline is this obscure GitHub comment: kubernetes/kubernetes#103981 (comment). One interesting thing about that comment is that it puts imports for the local repository last. We put github.com/openshift/cluster-ingress-operator/* imports last in only a few files in this repository.

Anyway, if I'm being picky, "golang.org/x/time/rate" should be moved to a new group. However, that import was already grouped with the github.com/openshift/* imports, so I prefer the commit that fixes the bug just leave it where it was originally so as not to distract from the important changes in the commit.

Miciah · 2022-12-14T16:57:01Z

Why did you regenerate bindata.go? You might need to change Go versions.

Miciah · 2022-12-14T16:58:52Z

/assign

gcs278 · 2022-12-14T19:52:00Z

Why did you regenerate bindata.go? You might need to change Go versions.

Sorry, I fixed it. I'm using go 1.18.6, but this is the update it keeps trying to make a spacing update, but I just ignore and don't check it in.

diff --git a/pkg/manifests/bindata.go b/pkg/manifests/bindata.go
index 326b5b40..d36f38c7 100644
--- a/pkg/manifests/bindata.go
+++ b/pkg/manifests/bindata.go
@@ -946,13 +946,11 @@ var _bindata = map[string]func() (*asset, error){
 // directory embedded in the file by go-bindata.
 // For example if you run go-bindata on data/... and data contains the
 // following hierarchy:
-//
-//     data/
-//       foo.txt
-//       img/
-//         a.png
-//         b.png
-//
+//     data/
+//       foo.txt
+//       img/
+//         a.png
+//         b.png
 // then AssetDir("data") would return []string{"foo.txt", "img"},
 // AssetDir("data/img") would return []string{"a.png", "b.png"},
 // AssetDir("foo.txt") and AssetDir("notexist") would return an error, and

gcs278 · 2022-12-14T21:45:08Z

/hold
writing unit tests

gcs278 · 2022-12-15T15:20:59Z

/unhold
Unit tests are written. @arkadeepsen FYSA, I learnt you can mock K8S clients fairly easy, so added unit tests to route-metrics controller.

arkadeepsen · 2022-12-19T14:47:21Z

Unit tests are written. @arkadeepsen FYSA, I learnt you can mock K8S clients fairly easy, so added unit tests to route-metrics controller.

@gcs278 This is pretty easy and very useful. Thanks for adding the unit tests 👍

gcs278 · 2022-12-21T20:16:35Z

all appear to be known issues
/retest

gcs278 · 2022-12-22T14:11:24Z

/retest
unrelated failures

gcs278 · 2023-01-03T14:35:51Z

/retest

candita · 2023-01-06T23:28:53Z

/test e2e-gcp-ovn-serial

pkg/operator/controller/route-metrics/controller_test.go

alebedev87 · 2023-04-24T19:56:28Z

/lgtm

gcs278 · 2023-04-25T11:13:07Z

Mostly TestClientTLS Failures
/retest-required

gcs278 · 2023-05-01T13:46:49Z

Known flakes except TestRouteMetricsControllerOnlyNamespaceSelector flaked. I see it happened one other time with https://search.ci.openshift.org/?search=error+waiting+for+route+metrics&maxAge=336h&context=1&type=build-log&name=&excludeName=&maxMatches=5&maxBytes=20971520&groupBy=job so I'll keep an eye on it, but I think it's a seperate issue.
/retest-required

gcs278 · 2023-05-01T17:15:34Z

Cluster install fail
/test e2e-azure-operator

gcs278 · 2023-05-26T17:18:24Z

/retest e2e-hypershift

openshift-ci · 2023-05-26T17:18:31Z

@gcs278: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test e2e-aws-operator
/test e2e-aws-ovn
/test e2e-aws-ovn-serial
/test e2e-aws-ovn-upgrade
/test e2e-azure-operator
/test e2e-gcp-operator
/test e2e-hypershift
/test images
/test unit
/test verify

The following commands are available to trigger optional jobs:

/test e2e-aws-ovn-single-node
/test e2e-azure-ovn
/test e2e-gcp-ovn

Use /test all to run all jobs.

In response to this:

/retest e2e-hypershift

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

gcs278 · 2023-05-26T17:57:08Z

/test e2e-hypershift

gcs278 · 2023-06-21T13:47:37Z

/retest-required

gcs278 · 2023-07-05T18:51:46Z

/retest

… metrics for ingress controllers that are not admitted

…e function. Add builder functions for building routes, namespaces, and ingress controllers.

gcs278 · 2023-09-25T17:20:43Z

@alebedev87 minor changes in https://github.com/openshift/cluster-ingress-operator/compare/004ebf75059b4db3834e0e4502bbaaceedfd9ab9..7ee37ab92e459af483c3c2b1989fdb9de0864e4b

import ordering, fixing Build() methods to refer to struct as a pointer, and some other minor clean up.

openshift-ci · 2023-09-25T19:48:28Z

@gcs278: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-ovn-serial	`8c34806`	link	true	`/test e2e-gcp-ovn-serial`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

gcs278 · 2023-09-25T19:52:46Z

https://issues.redhat.com/browse/OCPBUGS-13106
/test e2e-gcp-operator

disruption
/test e2e-gcp-ovn

weird error: the server doesn't have a resource type \"volumesnapshotclass\"\n"}
/test e2e-hypershift

alebedev87 · 2023-09-26T13:06:18Z

/lgtm

alebedev87 · 2023-09-26T13:40:08Z

/approve

openshift-ci · 2023-09-26T13:47:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alebedev87

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [alebedev87]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2023-09-26T15:44:46Z

/retest-required

Remaining retests: 0 against base HEAD ed6306f and 2 for PR HEAD 7ee37ab in total

openshift-ci-robot · 2023-09-26T17:37:58Z

@gcs278: Jira Issue OCPBUGS-3541: All pull requests linked via external trackers have merged:

openshift/cluster-ingress-operator#869

Jira Issue OCPBUGS-3541 has been moved to the MODIFIED state.

In response to this:

To fix OCPBUGS-3541, don't create metrics for ingress controller that are not admitted. The Ingress Operator won't add the finalizer if the Ingress Controller was not admitted. Since there is no finalizer, we can't delete any metrics that we created for the Ingress Controller upon deletion. Only adding metrics for Ingress Controllers that are admitted will correct this bug.

Furthermore, if an ingress controller is not admitted, it can't have any routes, so there isn't a large point in creating the metrics for it yet.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-merge-robot · 2023-09-27T15:50:20Z

Fix included in accepted release 4.15.0-0.nightly-2023-09-27-073353

openshift-ci-robot added the jira/invalid-bug Indicates that a referenced Jira bug is invalid for the branch this PR is targeting. label Dec 13, 2022

openshift-ci bot requested review from knobunc and rfredette December 14, 2022 00:00

openshift-ci bot requested a review from lihongan December 14, 2022 00:03

Miciah reviewed Dec 14, 2022

View reviewed changes

gcs278 force-pushed the route-metrics-ic-admitted branch from 0e89b1a to d2d7e7a Compare December 14, 2022 14:15

openshift-ci bot assigned Miciah Dec 14, 2022

gcs278 force-pushed the route-metrics-ic-admitted branch from d2d7e7a to 01178ee Compare December 14, 2022 17:26

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 14, 2022

gcs278 force-pushed the route-metrics-ic-admitted branch 3 times, most recently from e3db693 to 22be71c Compare December 15, 2022 15:18

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 15, 2022

gcs278 force-pushed the route-metrics-ic-admitted branch from 22be71c to 8c34806 Compare December 15, 2022 20:37

alebedev87 reviewed Apr 19, 2023

View reviewed changes

pkg/operator/controller/route-metrics/controller_test.go Outdated Show resolved Hide resolved

gcs278 force-pushed the route-metrics-ic-admitted branch from b771b86 to 6fc40ab Compare April 24, 2023 19:23

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2023

gcs278 mentioned this pull request Apr 27, 2023

[release-4.11] OCPBUGS-3560: Allow PROXY protocol for "Private" strategy #914

Merged

OCPBUGS-3541: Don't create route_metrics_controiller_routes_per_shard…

1311288

… metrics for ingress controllers that are not admitted

gcs278 force-pushed the route-metrics-ic-admitted branch from 6fc40ab to 004ebf7 Compare September 25, 2023 16:35

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 25, 2023

OCPBUGS-1689: Add unit test for the route-metrics controller reconcil…

7ee37ab

…e function. Add builder functions for building routes, namespaces, and ingress controllers.

gcs278 force-pushed the route-metrics-ic-admitted branch from 004ebf7 to 7ee37ab Compare September 25, 2023 17:13

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2023

openshift-merge-robot merged commit a10d93e into openshift:master Sep 26, 2023
14 checks passed

OCPBUGS-3541: Don't create route metrics for ingress controllers that are not admitted #869

OCPBUGS-3541: Don't create route metrics for ingress controllers that are not admitted #869

Conversation

gcs278 commented Dec 13, 2022

openshift-ci-robot commented Dec 13, 2022

gcs278 commented Dec 14, 2022

openshift-ci-robot commented Dec 14, 2022

Miciah Dec 14, 2022

Choose a reason for hiding this comment

gcs278 Dec 14, 2022

Choose a reason for hiding this comment

Miciah Dec 14, 2022

Choose a reason for hiding this comment

gcs278 Dec 14, 2022

Choose a reason for hiding this comment

Miciah Dec 15, 2022

Choose a reason for hiding this comment

Miciah commented Dec 14, 2022

Miciah commented Dec 14, 2022

gcs278 commented Dec 14, 2022 • edited

gcs278 commented Dec 14, 2022

gcs278 commented Dec 15, 2022

arkadeepsen commented Dec 19, 2022

gcs278 commented Dec 21, 2022

gcs278 commented Dec 22, 2022

gcs278 commented Jan 3, 2023

candita commented Jan 6, 2023

alebedev87 commented Apr 24, 2023

gcs278 commented Apr 25, 2023

gcs278 commented May 1, 2023

gcs278 commented May 1, 2023

gcs278 commented May 26, 2023

openshift-ci bot commented May 26, 2023

gcs278 commented May 26, 2023

gcs278 commented Jun 21, 2023

gcs278 commented Jul 5, 2023

gcs278 commented Sep 25, 2023

openshift-ci bot commented Sep 25, 2023 • edited

gcs278 commented Sep 25, 2023

alebedev87 commented Sep 26, 2023

alebedev87 commented Sep 26, 2023

openshift-ci bot commented Sep 26, 2023

openshift-ci-robot commented Sep 26, 2023

openshift-ci-robot commented Sep 26, 2023

openshift-merge-robot commented Sep 27, 2023

gcs278 commented Dec 14, 2022 •

edited

openshift-ci bot commented Sep 25, 2023 •

edited