Bug 1764313: 4.3 console_url is no longer reported on metrics endpoint (fix, with e2e test) by benjaminapetersen · Pull Request #299 · openshift/console-operator

benjaminapetersen · 2019-09-21T19:30:09Z

An e2e test to validate that the /metrics endpoint on the operator responds with an expected output.

jhadvig · 2019-09-24T10:39:22Z

Currently will not run due to a 403 error on the endpoint. The test suite is not allowed to hit the endpoint. Needs to decide the best solution for this.

Is this due to the missing operator/operand implementation ?

benjaminapetersen · 2019-09-27T03:04:17Z

I'm not really sure. I'm assuming we are running the tests as kube:admin since we use our kubeconfig. So I would expect we should be allowed to make a request against the /metrics url, but I get 403 Forbidden.

benjaminapetersen · 2019-10-21T21:03:52Z

/cherrypick release-4.2

openshift-cherrypick-robot · 2019-10-21T21:03:53Z

@benjaminapetersen: once the present PR merges, I will cherry-pick it on top of release-4.2 in a new PR and assign it to you.

Details

In response to this:

/cherrypick release-4.2

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.

benjaminapetersen · 2019-10-21T21:04:32Z

Ah, that is unfortunate as I know the release-4.2 will work. At present it appears our console_url metric died in 4.3. Exploring.

jhadvig · 2019-10-22T06:45:53Z

/retest

benjaminapetersen · 2019-10-22T14:21:46Z

At present, this is expected for 4.3/master:

 --- FAIL: TestMetricsEndpoint (5.28s)
    metrics_test.go:143: route for metrics: https://metrics-openshift-console-operator.apps.ci-op-9pmrs04r-4e531.origin-ci-int-aws.dev.rhcloud.com/metrics
    metrics_test.go:149: route to /metrics: (https://metrics-openshift-console-operator.apps.ci-op-9pmrs04r-4e531.origin-ci-int-aws.dev.rhcloud.com/metrics) 
    metrics_test.go:62: did not find console_url

Working on a fix.

jhadvig

Couple of minor comments. The test itself looks good 👍

I see that its failing on the:

metrics_test.go:62: did not find console_url

can you check if the response is valid and the console_url is missing or there is an issue with the route ?

openshift-ci-robot · 2019-10-22T18:08:32Z

@benjaminapetersen: This pull request references Bugzilla bug 1764313, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Bug 1764313: Add e2e test for /metrics endpoint

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.

benjaminapetersen · 2019-10-22T18:10:28Z

/hold

Now just needs PR to fix the missing console_url (separate, as this test is going to be cherry-picked to 4.2 & 4.1.

openshift-ci-robot · 2019-11-07T18:41:17Z

@benjaminapetersen: This pull request references Bugzilla bug 1764313, which is valid.

Details

In response to this:

Bug 1764313: 4.3 console_url is no longer reported on metrics endpoint (fix, with e2e test)

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.

benjaminapetersen · 2019-11-08T14:37:30Z

/retest

could not wait for build: the build src failed after 35s with reason DockerBuildFailed: Docker build strategy has failed.

benjaminapetersen · 2019-11-08T20:26:02Z

/retest

benjaminapetersen · 2019-11-11T15:33:52Z

/retest

console tests fail.

benjaminapetersen · 2019-11-11T15:35:07Z

+
+	// need to add the CAData, the kubeconfig has router certs
+	// that are missing from the system trust roots
+	rootCAs.AppendCertsFromPEM(config.CAData)


@stlaz thanks for your help here!

jhadvig

@benjaminapetersen tests are passing, lets squash and remove the additional debugging prints 👍 so we can merge

benjaminapetersen · 2019-11-11T19:32:20Z

+1 sounds good!

- Use cert based auth for client in metrics test

benjaminapetersen · 2019-11-11T20:36:35Z

@jhadvig squashed into 3 commits:

Delay registry of console_url metric until starter.go
- this is more of a best practices deal and may be part of what we backport.
Use k8s.io legacyregistry for metrics
- this is the fix for this missing metrics in 4.3 and cannot be part of the backport.
Add e2e test for /metrics endpoint
- this is the e2e test and can be part of the backport.

Backport will simply require a cherry pick of 3 commits:

initial addition of /metrics from Bug 1780170: Expose metrics #270, commit 346399b678
delay registry (probably optional) from this PR Bug 1764313: 4.3 console_url is no longer reported on metrics endpoint (fix, with e2e test) #299 commit d5fe6fedc
e2e test from this PR Bug 1764313: 4.3 console_url is no longer reported on metrics endpoint (fix, with e2e test) #299 commit 656a8b0913

I will create a branch for the backport to 4.1 once this merges and we verify our 4.3 metrics are again happy (backport will then require the backport for telemeter as well)

openshift-ci-robot · 2019-11-11T21:36:42Z

@benjaminapetersen: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-upgrade	`522bc54`	link	`/test e2e-aws-upgrade`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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.

benjaminapetersen · 2019-11-11T21:51:11Z

/retest

almost

benjaminapetersen · 2019-11-11T23:54:22Z

@jhadvig looks like we are good to merge!

benjaminapetersen · 2019-11-12T14:40:27Z

@spadgett care to tag? Then we can (finally) get the 4.1 backport moving.

spadgett

/lgtm

spadgett · 2019-11-12T14:58:51Z

+// alternatively:
+// 	oc exec -it console-operator-<pod-id> /bin/bash
+// 	curl --insecure -H "Authorization: Bearer <token>  https://localhost:8443/metrics | grep console_url
+func TestMetricsEndpoint(t *testing.T) {


openshift-ci-robot · 2019-11-12T14:59:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, spadgett

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [benjaminapetersen,spadgett]

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

spadgett · 2019-11-12T17:51:22Z

/hold cancel

openshift-ci-robot · 2019-11-12T17:52:57Z

@benjaminapetersen: All pull requests linked via external trackers have merged. Bugzilla bug 1764313 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1764313: 4.3 console_url is no longer reported on metrics endpoint (fix, with e2e test)

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-cherrypick-robot · 2019-11-12T17:53:07Z

@benjaminapetersen: #299 failed to apply on top of branch "release-4.2":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	pkg/console/operator/operator.go
M	pkg/console/operator/sync_v400.go
M	pkg/console/starter/starter.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/console/starter/starter.go
CONFLICT (content): Merge conflict in pkg/console/starter/starter.go
Auto-merging pkg/console/operator/sync_v400.go
Auto-merging pkg/console/operator/operator.go
CONFLICT (content): Merge conflict in pkg/console/operator/operator.go
Patch failed at 0001 Delay registry of console_url metric until starter.go

Details

In response to this:

/cherrypick release-4.2

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.

benjaminapetersen · 2019-11-12T19:35:18Z

lol forgot there was a hold on this 😄

benjaminapetersen changed the title ~~Add e2e test for /metrics endpoint~~ [WIP] Add e2e test for /metrics endpoint Sep 21, 2019

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 21, 2019

openshift-ci-robot requested review from jhadvig and spadgett September 21, 2019 19:30

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2019

benjaminapetersen force-pushed the monitoring/metrics-test branch 4 times, most recently from 92c8df4 to 112ee93 Compare October 21, 2019 20:52

benjaminapetersen changed the title ~~[WIP] Add e2e test for /metrics endpoint~~ Add e2e test for /metrics endpoint Oct 21, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 21, 2019

benjaminapetersen commented Oct 21, 2019

View reviewed changes

Comment thread test/e2e/metrics_test.go Outdated

benjaminapetersen commented Oct 21, 2019

View reviewed changes

Comment thread test/e2e/metrics_test.go

benjaminapetersen mentioned this pull request Oct 22, 2019

Add e2e test for /metrics endpoint #320

Closed

jhadvig requested changes Oct 22, 2019

View reviewed changes

benjaminapetersen commented Oct 22, 2019

View reviewed changes

Comment thread test/e2e/metrics_test.go Outdated

benjaminapetersen force-pushed the monitoring/metrics-test branch from 112ee93 to 9b38c92 Compare October 22, 2019 17:33

benjaminapetersen mentioned this pull request Oct 22, 2019

Bug 1764316: 4.2 Backport of e2e test for /metrics #322

Closed

benjaminapetersen changed the title ~~Add e2e test for /metrics endpoint~~ Bug 1764313: Add e2e test for /metrics endpoint Oct 22, 2019

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 22, 2019

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 7, 2019

benjaminapetersen force-pushed the monitoring/metrics-test branch from 9cf0867 to bcc338d Compare November 8, 2019 14:41

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 8, 2019

benjaminapetersen commented Nov 11, 2019

View reviewed changes

jhadvig requested changes Nov 11, 2019

View reviewed changes

benjaminapetersen added 3 commits November 11, 2019 15:30

Delay registry of console_url metric until starter.go

d5fe6fe

Use k8s.io legacyregistry for metrics

0f81a12

Add e2e test for /metrics endpoint

656a8b0

- Use cert based auth for client in metrics test

benjaminapetersen force-pushed the monitoring/metrics-test branch from d1b9ecd to 656a8b0 Compare November 11, 2019 20:31

spadgett approved these changes Nov 12, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Nov 12, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 12, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2019

openshift-merge-robot merged commit 2e066fe into openshift:master Nov 12, 2019

benjaminapetersen deleted the monitoring/metrics-test branch November 12, 2019 19:34

Conversation

benjaminapetersen commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhadvig commented Sep 24, 2019

Uh oh!

benjaminapetersen commented Sep 27, 2019

Uh oh!

Uh oh!

Uh oh!

benjaminapetersen commented Oct 21, 2019

Uh oh!

openshift-cherrypick-robot commented Oct 21, 2019

Uh oh!

benjaminapetersen commented Oct 21, 2019

Uh oh!

jhadvig commented Oct 22, 2019

Uh oh!

benjaminapetersen commented Oct 22, 2019

Uh oh!

jhadvig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 22, 2019

Uh oh!

benjaminapetersen commented Oct 22, 2019

Uh oh!

openshift-ci-robot commented Nov 7, 2019

Uh oh!

benjaminapetersen commented Nov 8, 2019

Uh oh!

benjaminapetersen commented Nov 8, 2019

Uh oh!

benjaminapetersen commented Nov 11, 2019

Uh oh!

benjaminapetersen Nov 11, 2019

Choose a reason for hiding this comment

Uh oh!

jhadvig left a comment

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Nov 11, 2019

Uh oh!

benjaminapetersen commented Nov 11, 2019

Uh oh!

openshift-ci-robot commented Nov 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminapetersen commented Nov 11, 2019

Uh oh!

benjaminapetersen commented Nov 11, 2019

Uh oh!

benjaminapetersen commented Nov 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

spadgett Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Nov 12, 2019

Uh oh!

spadgett commented Nov 12, 2019

Uh oh!

openshift-ci-robot commented Nov 12, 2019

Uh oh!

openshift-cherrypick-robot commented Nov 12, 2019

Uh oh!

benjaminapetersen commented Nov 12, 2019

benjaminapetersen commented Sep 21, 2019 •

edited

Loading

openshift-ci-robot commented Nov 11, 2019 •

edited

Loading

benjaminapetersen commented Nov 12, 2019 •

edited

Loading