NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting #493

sgreene570 · 2020-11-18T15:17:43Z

Follow up PR to #476 in support of NE-199.

This PR adds logic to periodically test the canary route. Canary probe results are reported very verbosely via the ingress operator's logs, somewhat verbosely via newly declared Prometheus metrics, and very granularly through the default ingress controllers status conditions.

This PR adds the go-httpstat, which provides boiler plate code for measuring HTTP latencies off of go's native httpstat library.

sgreene570 · 2020-11-18T17:41:02Z

/test e2e-aws-operator so I can verify the e2e tests I drafted

sgreene570 · 2020-11-18T19:11:02Z

Soliciting initial feedback 🚢
/cc @openshift/openshift-team-network-edge

sgreene570 · 2020-11-18T22:58:02Z

need to investigate why the new canary e2e test is not passing

sgreene570 · 2020-11-20T00:47:29Z

/retest

sgreene570 · 2020-11-23T15:10:05Z

0f519d7 updates this PR to account for openshift/origin#25703

sgreene570 · 2020-11-23T16:57:40Z

/retest

sgreene570 · 2020-11-23T19:26:43Z

/retest

sgreene570 · 2020-11-23T22:33:26Z

/retest

sgreene570 · 2020-11-23T22:35:11Z

pkg/operator/controller/canary/metrics.go

+)
+
+var (
+	CanaryRequestTime = prometheus.NewHistogramVec(


@RiRa12621 Hey if you have a second could you glance at these metrics and make sure they look good to you? Asking since we've collaborated on Ingress metrics in the past 😄

sgreene570 · 2020-12-02T00:39:47Z

/retest

pkg/operator/controller/canary/http.go

Miciah · 2020-12-02T06:41:08Z

Looks good, but #498 has caused a conflict.

Add tcnksm/go-httpstat to go.mod and vendor/ The go-httpstat dep wraps the go httptrace library with boiler-plate timing definitions. Add prometheus/client_golang to go.mod and vendor/ This allows the ingress operator to create it's own metrics.

Use the stop channel used to control the operator in cmd/ingress-operator/start.go to control the Canary metrics and canary route polling loop.

sgreene570 · 2020-12-02T14:55:55Z

Looks good, but #498 has caused a conflict.

Fixed! Ready to go 🚢

sgreene570 · 2020-12-02T16:01:01Z

CI is failing on VPC limits
/retest

frobware

Overall LGTM, just one case where we don't handle an err in the tests that needs fixing. The rest observations/comments/suggestions.

test/e2e/canary_test.go

pkg/operator/controller/canary/http.go

pkg/operator/controller/canary: Add basic HTTP checks for the canary route created in NE-199 phase 1. Add canary metrics to the canary controller and populate them with the results of the canary checks. Add a canary check status condition to the default ingress controller to mark the ingress operator as degraded when canary checks are unsucessful for a sustained period of time. pkg/operator/controller/canary/http.go: New file to define the canary route check HTTP funcs using go-httpstat. pkg/operator/controller/canary/metrics.go: New file to define the canary route check metrics to be collected via prometheus.

pkg/operator/controller/ingress/controller.go: Define the canary status condition name. pkg/operator/controller/ingress/status.go: Mark the default ingress controller as degraded when the canary status condition has been set to false for a sustained period of time. pkg/operator/controller/ingress/status_test.go: Add test cases to verify the default ingress controller canary status condition changes.

Add a test to test/e2e/operator_test.go that verifies the ingress clusteroperator's status related objects. Add 2 tests to a new test file, test/e2e/canary_test.go. The first test verifies that the Ingress canary route and echo pod (hello-openshift) work as intended. The second test verifies that the default ingress controller reports the canary check success status condition after a short period of time.

`origin-hello-openshift` is now publicly available on quay.io. Use the publicly available image in `manifests/image-references` (instead of the outdated `hello-openshift` image hosted on DockerHub). Note: This only affects ingress operator development, as the CVO overrides this reference to use the hello-openshift image in the release payload anyways.

Miciah · 2020-12-02T19:00:33Z

#493 (comment) made me realize, test/e2e/canary_test.go should be able to use kclient, right? If so, we can clean that up in a follow-up.

sgreene570 · 2020-12-02T19:03:30Z

#493 (comment) made me realize, test/e2e/canary_test.go should be able to use kclient, right? If so, we can clean that up in a follow-up.

I believe so. I'll make a note to fix that in a follow up.

Miciah · 2020-12-02T19:05:55Z

/lgtm

openshift-ci-robot · 2020-12-02T19:06:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, sgreene570

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~~ [Miciah,sgreene570]

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

sgreene570 · 2020-12-02T19:50:10Z

level=error msg=Error: Error creating VPC: VpcLimitExceeded: The maximum number of VPCs has been reached.
level=error msg=	status code: 400, request id: 2032d3f9-cd40-48be-81d6-a663735959fe

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_cluster-ingress-operator/493/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-operator/1334192150635614208

sgreene570 · 2020-12-02T23:33:44Z

/retest

sgreene570 · 2020-12-03T14:19:24Z

/refresh
/retest

openshift-ci-robot · 2020-12-03T14:19:38Z

@sgreene570: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`91b9929`	link	`/test e2e-aws`
ci/prow/e2e-upgrade	`91b9929`	link	`/test e2e-upgrade`
ci/prow/e2e-aws-operator	`91b9929`	link	`/test e2e-aws-operator`

Full PR test history. Your PR dashboard.

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.

sgreene570 · 2020-12-03T16:35:07Z

One more to go!
/test e2e-upgrade

NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 18, 2020

openshift-ci-robot requested review from Miciah and knobunc November 18, 2020 15:18

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2020

sgreene570 force-pushed the NE-199-phase-2 branch 2 times, most recently from 14a2028 to 1db6ee0 Compare November 18, 2020 15:32

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

sgreene570 force-pushed the NE-199-phase-2 branch from 1db6ee0 to 30f7e61 Compare November 18, 2020 15:32

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

openshift-ci-robot requested a review from a team November 18, 2020 19:11

sgreene570 force-pushed the NE-199-phase-2 branch 2 times, most recently from 4920a28 to fd7eac0 Compare November 18, 2020 21:38

sgreene570 force-pushed the NE-199-phase-2 branch 3 times, most recently from 5af1d60 to c7dfe44 Compare November 19, 2020 21:49

sgreene570 force-pushed the NE-199-phase-2 branch 2 times, most recently from f57b083 to c0085e8 Compare November 20, 2020 17:01

sgreene570 changed the title ~~[WIP] NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting~~ NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting Nov 20, 2020

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 Nov 20, 2020

sgreene570 force-pushed the NE-199-phase-2 branch from c0085e8 to 0f519d7 Compare November 23, 2020 15:06

sgreene570 force-pushed the NE-199-phase-2 branch from 0f519d7 to ae2d98c Compare November 23, 2020 21:09

sgreene570 commented Nov 23, 2020

View reviewed changes

Miciah reviewed Dec 2, 2020

View reviewed changes

pkg/operator/controller/canary/http.go Outdated Show resolved Hide resolved

sgreene570 added 2 commits December 2, 2020 09:53

Use operator stop channel for canary controller

704fec8

Use the stop channel used to control the operator in cmd/ingress-operator/start.go to control the Canary metrics and canary route polling loop.

sgreene570 force-pushed the NE-199-phase-2 branch from ecdc75c to b8cee02 Compare December 2, 2020 14:54

frobware reviewed Dec 2, 2020

View reviewed changes

sgreene570 added 5 commits December 2, 2020 12:45

Update bindata

91b9929

sgreene570 force-pushed the NE-199-phase-2 branch from b8cee02 to 91b9929 Compare December 2, 2020 17:45

openshift-ci-robot assigned Miciah Dec 2, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020

candita mentioned this pull request Dec 3, 2020

Bug 1870373: Ingress Operator reports available when DNS fails to provision #495

Merged

openshift-merge-robot merged commit d323ac2 into openshift:master Dec 3, 2020

candita pushed a commit to candita/cluster-ingress-operator that referenced this pull request Dec 3, 2020

Merge pull request openshift#493 from sgreene570/NE-199-phase-2

36bdc04

NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting

sgreene570 mentioned this pull request Dec 3, 2020

Bug 1903165: NE-199 Follow Up Fixes. #501

Merged

NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting #493

NE-199 Phase 2: Add periodic canary route HTTP checks w/ metrics & basic status reporting #493

Uh oh!

Conversation

sgreene570 commented Nov 18, 2020

Uh oh!

sgreene570 commented Nov 18, 2020

Uh oh!

sgreene570 commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgreene570 commented Nov 18, 2020

Uh oh!

sgreene570 commented Nov 20, 2020

Uh oh!

sgreene570 commented Nov 23, 2020

Uh oh!

sgreene570 commented Nov 23, 2020

Uh oh!

sgreene570 commented Nov 23, 2020

Uh oh!

sgreene570 commented Nov 23, 2020

Uh oh!

sgreene570 Nov 23, 2020

Choose a reason for hiding this comment

Uh oh!

sgreene570 commented Dec 2, 2020

Uh oh!

Uh oh!

Miciah commented Dec 2, 2020

Uh oh!

sgreene570 commented Dec 2, 2020

Uh oh!

sgreene570 commented Dec 2, 2020

Uh oh!

frobware left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Miciah commented Dec 2, 2020

Uh oh!

sgreene570 commented Dec 2, 2020

Uh oh!

Miciah commented Dec 2, 2020

Uh oh!

openshift-ci-robot commented Dec 2, 2020

Uh oh!

sgreene570 commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgreene570 commented Dec 2, 2020

Uh oh!

sgreene570 commented Dec 3, 2020

Uh oh!

openshift-ci-robot commented Dec 3, 2020

Uh oh!

sgreene570 commented Dec 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sgreene570 commented Nov 18, 2020 •

edited

Loading

frobware left a comment •

edited

Loading

sgreene570 commented Dec 2, 2020 •

edited

Loading