Skip to content
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

⚠️ metrics are now on by default #510

Merged
merged 1 commit into from
Jul 2, 2019
Merged

⚠️ metrics are now on by default #510

merged 1 commit into from
Jul 2, 2019

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Jul 1, 2019 •
edited
Loading

It can be turn off by setting manager.Options{MetricsBindAddress: "0"}. GoDoc.
Note: If upgrading from controller-runtime v0.1.* to v0.2.0-beta.4+ and you have multiple controller tests, you may hit an error that these tests are racing to bind the same default 8080 port. And only one of them can succeed, others may fail. You may turn off the metrics in your tests in this case.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 1, 2019
@k8s-ci-robot k8s-ci-robot requested review from droot and pwittrock July 1, 2019 18:26
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 1, 2019
@mengqiy mengqiy requested a review from DirectXMan12 July 1, 2019 18:26
@droot
Copy link
Contributor

droot commented Jul 1, 2019

A few more points that popped up during the conversation:

  • testenv pkg should disable metrics server by default so that tests can run in parallel by default
  • Make failure to "binding to default port 8080" obvious because people may encounter that in dev environment because 8080 being a popular port.

@mengqiy
Copy link
Member Author

mengqiy commented Jul 1, 2019

testenv pkg should disable metrics server by default so that tests can run in parallel by default

testenv pkg handles the testing control plane, it doesn't touch manager.Options.
IMO it should be when we scaffold controller tests, metrics should be disabled in manager.Options.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 1, 2019
@mengqiy
Copy link
Member Author

mengqiy commented Jul 1, 2019

PTAL

droot
droot reviewed Jul 2, 2019
View reviewed changes
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

some minor nits.

pkg/metrics/listener.go Outdated Show resolved Hide resolved
pkg/metrics/listener.go Outdated Show resolved Hide resolved
pkg/manager/internal.go Outdated Show resolved Hide resolved
@mengqiy
Copy link
Member Author

mengqiy commented Jul 2, 2019

PTAL

@droot droot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 2, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: mengqiy

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:

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

@k8s-ci-robot k8s-ci-robot merged commit 2495fdd into kubernetes-sigs:master Jul 2, 2019
@mengqiy mengqiy deleted the metricsdefault branch July 2, 2019 20:10
@vincepri
Copy link
Member

vincepri commented Jul 3, 2019 •
edited
Loading

@mengqiy We're getting testing failures because multiple processes are trying to bind port 8080.

We replaced the test Manager's default Kubebuilder options with manager.Options{MetricsBindAddress: "0"}, definitely something that should go into the release notes.

PR: kubernetes-sigs/cluster-api#1107
Test logs: https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_cluster-api/1107/pull-cluster-api-test/1146217126005051403/

@mengqiy
Copy link
Member Author

mengqiy commented Jul 3, 2019

I added something in the GoDoc: https://github.com/kubernetes-sigs/controller-runtime/pull/510/files#diff-7d903b129bc883285ffc5068c39c00b0R139
I will update the release notes to make it more helpful.

@mengqiy
Copy link
Member Author

mengqiy commented Jul 3, 2019

Updated release notes and the description of this PR.

@vincepri
Copy link
Member

vincepri commented Jul 3, 2019

Thank you!

sebgl added a commit to sebgl/cloud-on-k8s that referenced this pull request Oct 1, 2019
@sebgl
Disable metrics following changes in controller-runtime
1933510 
Controller-runtime metrics are now enabled by default since
kubernetes-sigs/controller-runtime#510 was
merged.
To disable metrics, we must explicitly set the port to "0".
@sebgl sebgl mentioned this pull request Oct 1, 2019
Merged
sebgl added a commit to elastic/cloud-on-k8s that referenced this pull request Oct 1, 2019
@sebgl
Disable metrics following changes in controller-runtime (#1830)
fdcfd3f 
Controller-runtime metrics are now enabled by default since
kubernetes-sigs/controller-runtime#510 was
merged.
To disable metrics, we must explicitly set the port to "0".
@ixdy ixdy mentioned this pull request Feb 13, 2020
Merged
@2uasimojo 2uasimojo mentioned this pull request Aug 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droot droot droot left review comments

@pwittrock pwittrock Awaiting requested review from pwittrock

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mengqiy @droot @k8s-ci-robot @vincepri