Skip to content

Add e2e test for metrics registration #651

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

Jump to bottom
Closed
sanchezl wants to merge 2 commits into from
Closed

Add e2e test for metrics registration #651

sanchezl wants to merge 2 commits into from

Conversation

@sanchezl
Copy link
Contributor

@sanchezl sanchezl commented Nov 12, 2019

No description provided.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 12, 2019
@openshift-ci-robot
Copy link

openshift-ci-robot commented Nov 12, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sanchezl
To complete the pull request process, please assign tnozicka
You can assign the PR to them by writing /assign @tnozicka in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@sanchezl
Copy link
Contributor Author

sanchezl commented Nov 13, 2019

/test e2e-aws

sttts
sttts reviewed Nov 20, 2019
View reviewed changes
test/e2e/metrics_test.go Outdated
metrics[entry[0]] = entry[1]
}
require.NoError(t, scanner.Err())
return metrics
Copy link
Contributor

@sttts sttts Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make the test a bit easier to read with empty lines and high-level oneliner comments.

Copy link
Contributor Author

@sanchezl sanchezl Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

sttts
sttts reviewed Nov 20, 2019
View reviewed changes
test/e2e/metrics_test.go Outdated
transport, upgrader, err := spdy.RoundTripperFor(kubeConfig)
require.NoError(t, err)
request := restClient.Post().Resource("pods").Namespace(pod.Namespace).Name(pod.Name).SubResource("portforward")
dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, http.MethodPost, request.URL())
Copy link
Contributor

@sttts sttts Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a port forwarder? This sounds super complex.

Copy link
Contributor

@sttts sttts Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying it is not necessary (maybe it is), but it surprises me as a reader and needs a comments explaining in 2 or 3 lines why.

Copy link
Contributor Author

@sanchezl sanchezl Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some comments. AFAIK, port forwarding is needed in order for test to run outside of the cluster.

sttts
sttts reviewed Nov 20, 2019
View reviewed changes
test/e2e/metrics_test.go
@@ -0,0 +1,123 @@
package e2e
Copy link
Contributor

@sttts sttts Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this go to library-go as an e2e helper and just a oneliner here?

Copy link
Contributor Author

@sanchezl sanchezl Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I intend to move to library-go once finalized.

Copy link
Contributor Author

@sanchezl sanchezl Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've opened openshift/library-go#596 to start.

sttts
sttts reviewed Nov 20, 2019
View reviewed changes
test/e2e/metrics_test.go Outdated
transport, upgrader, err := spdy.RoundTripperFor(kubeConfig)
require.NoError(t, err)
request := restClient.Post().Resource("pods").Namespace(pod.Namespace).Name(pod.Name).SubResource("portforward")
dialer := spdy.NewDialer(upgrader, &http.Client{Transport: transport}, http.MethodPost, request.URL())
Copy link
Contributor

@sttts sttts Nov 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

didn't we move to http/2 by now?

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 20, 2019
@marun
Copy link
Contributor

marun commented Nov 20, 2019

Why is it valuable to check that pods are exposing metric endpoints in the absence of checking that prometheus is correctly configured to consume output from those endpoints (e.g. openshift/origin#24169)?

It appears trivial to expose metric endpoints - any controller started by NewControllerCommandConfig gets /metrics for free. Wiring those endpoints properly to prometheus seems far more prone to misconfiguration and therefore deserving of testing effort, and such testing would implicitly guarantee the validation of the metrics endpoints.

@sanchezl
Copy link
Contributor Author

sanchezl commented Nov 21, 2019

It appears trivial to expose metric endpoints - any controller started by NewControllerCommandConfig gets /metrics for free. ? Wiring those endpoints properly to prometheus seems far more prone to misconfiguration and therefore deserving of testing effort, and such testing would implicitly guarantee the validation of the metrics endpoints.

This test is specifically about the /metrics endpoint being there, but not returning ANY metrics.

@sanchezl
Copy link
Contributor Author

sanchezl commented Nov 21, 2019

/test e2e-aws

@sttts
Copy link
Contributor

sttts commented Nov 21, 2019

but not returning ANY metrics.

I thought that was the regression we had: we had an empty /metrics endpoint, but it was always served. cc @mfojtik

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 7, 2020

@sanchezl: PR needs rebase.

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2020
@marun
Copy link
Contributor

marun commented Jan 7, 2020 •
edited
Loading

@sanchezl I have a WIP test that trivially checks that prometheus can scrape the endpoint for a namespace. Once the related PRs merge I intend to add it as a helper to library-go that will enable a one-line check that can be called to check metrics collection for this operator:

checkMetricsCollection(t, kubeConfig, operatorNamespace, [prometheus job name])

Will that be sufficient to fix the but you're targeting, or will it be necessary to check the actual data that is being collected?

@openshift-ci-robot
Copy link

openshift-ci-robot commented Jan 27, 2020

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

Test name Commit Details Rerun command
ci/prow/verify-deps 1b14899 link /test verify-deps
ci/prow/e2e-aws-serial 1b14899 link /test e2e-aws-serial

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.

@sanchezl sanchezl closed this Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfojtik mfojtik Awaiting requested review from mfojtik

@soltysh soltysh Awaiting requested review from soltysh

1 more reviewer

@sttts sttts sttts left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sanchezl @openshift-ci-robot @marun @sttts