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

Turn of metrics for reflectors created by watch managers #73624

Closed
wants to merge 1 commit into from
Closed

Turn of metrics for reflectors created by watch managers #73624

wants to merge 1 commit into from

Conversation

wojtek-t
Copy link
Member

@wojtek-t wojtek-t commented Feb 1, 2019

Fix #73587

@k8s-ci-robot
Copy link
Contributor

@wojtek-t: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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.

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 1, 2019
@wojtek-t wojtek-t mentioned this pull request Feb 1, 2019
Closed
@wojtek-t wojtek-t added sig/node Categorizes an issue or PR as relevant to SIG Node. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 1, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wojtek-t

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019
@wojtek-t wojtek-t force-pushed the disable_manager_reflector_metrics branch from bb1aa97 to 033c89c Compare February 1, 2019 08:34
@wojtek-t
Copy link
Member Author

wojtek-t commented Feb 1, 2019

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 1, 2019
wojtek-t
wojtek-t commented Feb 1, 2019
View reviewed changes
pkg/kubelet/kubelet.go
// after this point, so that watch-based managers (if used) will not
// blow the Kubelet memory usage if there are huge churn of secrets and/or
// configmaps attached to pods running on this node.
cache.SetReflectorMetricsProvider(manager.NoopMetricsProvider{})
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it won't work, because SetReflectorMetricsProvider is using sync.Once underneath (so once initialized, we can't change it).

That said, I realized that I don't understand why this is happening, because the:

  • metrics factory is defaulted to noop
  • the prometheus metrics need to be explicitly linked and initialized (as they are in apiserver e.g.:

    kubernetes/cmd/kube-apiserver/app/server.go

    Line 81 in d1031b1

    _ "k8s.io/kubernetes/pkg/util/reflector/prometheus" // for reflector metric registration
    )

So I'm not sure what the exact problem actually is...

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Feb 1, 2019 •
edited

@wojtek-t: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e-containerized 033c89c link /test pull-kubernetes-local-e2e-containerized
pull-kubernetes-verify 033c89c link /test pull-kubernetes-verify

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.

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.

@wojtek-t wojtek-t closed this Feb 12, 2019
@wojtek-t wojtek-t deleted the disable_manager_reflector_metrics branch July 5, 2019 06:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@feiskyer feiskyer Awaiting requested review from feiskyer

Assignees

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/node Categorizes an issue or PR as relevant to SIG Node.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wojtek-t @k8s-ci-robot @yujuhong