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.

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

Deprecate kubelet flag for cadvisor port #59827

Merged

Conversation

dashpole
Copy link
Contributor

@dashpole dashpole commented Feb 13, 2018

Which issue(s) this PR fixes:
Issue: #56523
TL;DR the Kubelet's stats/summary API is the preferred way of monitoring the node. If you need additional metrics from cAdvisor, it can be run as a daemonset.

Release note:

action required: Deprecate the kubelet's cadvisor port. The default will change to 0 (disabled) in 1.12, and the cadvisor port will be removed entirely in 1.13.

/assign @mtaufen @tallclair
cc @kubernetes/sig-node-pr-reviews

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 13, 2018
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 13, 2018
@dashpole
Copy link
Contributor Author

/kind cleanup
/priority important-soon

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Feb 13, 2018
@dashpole
Copy link
Contributor Author

/assign @derekwaynecarr
for approval

@dashpole dashpole added this to the v1.10 milestone Feb 13, 2018
@@ -363,6 +362,8 @@ func (f *KubeletFlags) AddFlags(fs *pflag.FlagSet) {
fs.StringVar(&f.BootstrapCheckpointPath, "bootstrap-checkpoint-path", f.BootstrapCheckpointPath, "<Warning: Alpha feature> Path to to the directory where the checkpoints are stored")

// DEPRECATED FLAGS
fs.Int32Var(&f.CAdvisorPort, "cadvisor-port", f.CAdvisorPort, "The port of the localhost cAdvisor endpoint (set to 0 to disable)")
fs.MarkDeprecated("cadvisor-port", "The default will change to 0 (disabled) in 1.11, and the cadvisor port will be removed entirely in 1.13")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we give 6 months of warning (v1.12) before disabling? This would fit more with the deprecation policy, which specifies the longer of 6 months or 1 release, given our current quarterly release cadence. Since disabling has the same effect as deprecation (the server disappears) for anyone who doesn't explicitly set this, it seems like v1.12 might be better than v1.11.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the timeline didn't apply to defaults, just the presence of the flag. Although I don't think its a big deal to move it back a release. Does this mean all changes to flag defaults need 6 mo warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Changes to defaults are API changes, and should follow the deprecation policy's warning timelines. If this were a declarative API, rather than a CLI, changes to defaults would require incrementing the API version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to 1.12

@mtaufen
Copy link
Contributor

mtaufen commented Feb 13, 2018

One question otherwise LGTM

@mtaufen
Copy link
Contributor

mtaufen commented Feb 14, 2018

/retest

@mtaufen
Copy link
Contributor

mtaufen commented Feb 14, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 14, 2018
@derekwaynecarr
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, derekwaynecarr, mtaufen

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 16, 2018
@derekwaynecarr
Copy link
Member

@sjenning we should make sure we have a tracking card for this in openshift.

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit cfa6d35 into kubernetes:master Feb 16, 2018
@k8s-ci-robot k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 16, 2018
@tallclair
Copy link
Member

Thanks David! I updated the release note to call out the schedule, and I think it's action required (although the the action doesn't need to happen in this release).

@smoya
Copy link

smoya commented Mar 26, 2018

Regarding deprecating cadvisor API, are you aware of the fact that pods created before the kube api server start time, are being reported as Pending in the Kubelet /pods endpoint forever? This makes hidding the ContainerStatuses from those pods. Then, the only way of getting info about the containers, as container id, container image id, etc, and apart from going to the kubernetes API (which is not desired in my case) is accessing /metrics/cadvisor.

Do you have an alternative to that?

@dashpole dashpole deleted the depreciate_cadvisor_port branch March 26, 2018 16:47
@dashpole
Copy link
Contributor Author

@smoya that sounds like a bug we should fix. Feel free to open a bug with an example and cc me.

@smoya
Copy link

smoya commented Mar 26, 2018

@dashpole #59889 <- This issue was fixed but reverted because somehow tests were broken. Then the author of the fix reopened as an issue.

@mtaufen
Copy link
Contributor

mtaufen commented Mar 26, 2018

are you aware of the fact that pods created before the kube api server start time, are being reported as Pending in the Kubelet /pods endpoint forever?

I believe this is expected behavior for static pods. /pods treats the API server as the source of truth for pod lifecycle information, and IIRC it does not look at mirror pods... @Random-Liu am I remembering correctly?

@yujuhong
Copy link
Contributor

This is working as intended for static pods, see also #61717 (comment)

@smoya
Copy link

smoya commented Mar 26, 2018

@mtaufen @yujuhong Thanks for your messages.

What I don't understand is why showing them as Pending anyway when the API shows them as Running. Having them as Pending means losing some critical information about their associated containers.

@mtaufen
Copy link
Contributor

mtaufen commented Mar 27, 2018

Out of curiosity, what are you using /pods for? I don't think this endpoint is really intended for production use, just debugging.

@yujuhong
Copy link
Contributor

/pods currently shows the desired pod specs that kubelet syncs to, and (I just checked) it doesn't have mirror pods (since they are pseudo pods created by kubelet). It's possible to let kubelet return both static and mirror pods in /pods, but as @mtaufen said, we'd need to understand the use cases.

@smoya
Copy link

smoya commented Mar 30, 2018

I'm currently developing a tool that collects metrics and sends it to a saas agent. I can't use the API directly since I need data related to the nodes and only available through Kubelet. There is 1 agent residing in each Node, so calling the API is not an option (imagine a big cluster with hundreds of nodes making calls every few secs to the API).

@dashpole
Copy link
Contributor Author

dashpole commented Apr 3, 2018

@smoya There are some discussions ongoing about the kubelet providing metadata for monitoring and logging agents, which your work may fit into. I don't think using the /pods endpoint, which isn't intended to support production use-cases, is the right answer right now.

k8s-github-robot pushed a commit that referenced this pull request Jul 7, 2018
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Remove --cadvisor-port - has been deprecated since v1.10

**What this PR does / why we need it**:

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #56523

**Special notes for your reviewer**:
- Deprecated in #59827 (v1.10)
- Disabled in #63881 (v1.11)

**Release note**:

```release-note
[action required] The formerly publicly-available cAdvisor web UI that the kubelet started using `--cadvisor-port` is now entirely removed in 1.12. The recommended way to run cAdvisor if you still need it, is via a DaemonSet.
```
@stevebail
Copy link

@dashpole Hi Dave. I am using the latest kubelet versions (1.15, 1.16, 1.17). What are the URLs to retrieve the cAdvisor metrics directly from each node? Are they always available on TCP port 10255? The following two URLs work me bit I don't know the difference: curl :10255/stats/summary and curl :10255/metrics. It looks like /stats/summary are the cAdvisor metrics and /metrics are the Kubelet metrics. Is this correct? Lastly, is :10255/stats/summary used by Prometheus server when scraping the node cAdvisor at regular interval? I often see a reference to port 8080 for cAdvisor and I know don't where it is coming from. Thank you for your help.

@dashpole
Copy link
Contributor Author

@stevebail you can use :10255/metrics/cadvisor for what used to be at the cAdvisor port

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants