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

Add /metrics and profiling handlers to kube-proxy #45889

Merged
merged 2 commits into from
May 17, 2017
Merged

Add /metrics and profiling handlers to kube-proxy #45889

merged 2 commits into from
May 17, 2017

Conversation

wojtek-t
Copy link
Member

Also expose "syncProxyRules latency" as a prometheus metrics.

Fix #45876

@wojtek-t wojtek-t added area/kube-proxy release-note-none Denotes a PR that doesn't merit a release note. labels May 16, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 16, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2017
@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 16, 2017
@wojtek-t wojtek-t force-pushed the kube_proxy_handlers branch 2 times, most recently from 5ff14ad to 1157d1a Compare May 16, 2017 14:45
MrHohn
MrHohn reviewed May 16, 2017
View reviewed changes
cmd/kube-proxy/app/server.go
@@ -146,6 +146,7 @@ func AddFlags(options *Options, fs *pflag.FlagSet) {
&options.config.Conntrack.TCPCloseWaitTimeout.Duration, "conntrack-tcp-timeout-close-wait",
options.config.Conntrack.TCPCloseWaitTimeout.Duration,
"NAT timeout for TCP connections in the CLOSE_WAIT state")
fs.BoolVar(&options.config.EnableProfiling, "profiling", options.config.EnableProfiling, "If true enables profiling via web interface on /debug/pprof handler.")
Copy link
Member

Choose a reason for hiding this comment

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

As the comment in codes: All flags below here are deprecated and will eventually be removed, do we really need to add a new flag? Maybe letting user configure this through componentconfig :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that all our setups are still using flags (test clusters, kubemark). I wouldn't like to block using this flag on migration to component config.

Copy link
Member

Choose a reason for hiding this comment

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

I thnk it's appropriate to have a flag like this - I am assuming it is not generally going to be on, and it's not a "knob" but a very fundamental operating mode, not something users would change except to debug?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - that was also my way of thinking.

MrHohn
MrHohn reviewed May 17, 2017
View reviewed changes
pkg/proxy/iptables/proxier.go Outdated
@@ -518,6 +518,7 @@ func (proxier *Proxier) SyncLoop() {
defer t.Stop()
// Update healthz timestamp at beginning in case Sync() never succeeds.
if proxier.healthzServer != nil {
registerMetrics()
Copy link
Member

Choose a reason for hiding this comment

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

Because the metrics handleFuncs are added to metrics server but not healthz server, I think we shouldn't put it in this condition block.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the approach in the meantime, so obviously you're right.
Done.

@wojtek-t
Copy link
Member Author

@MrHohn - thanks for review.

PTAL

thockin
thockin reviewed May 17, 2017
View reviewed changes
pkg/proxy/iptables/proxier.go Outdated
@@ -516,6 +516,7 @@ func (proxier *Proxier) Sync() {
func (proxier *Proxier) SyncLoop() {
t := time.NewTicker(proxier.syncPeriod)
defer t.Stop()
registerMetrics()
Copy link
Member

Choose a reason for hiding this comment

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

Why is this in SyncLoop, rather than in app-level init (cmd) or NewProxier or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to register those if not in iptables mode. But we have if there anyway.

Will move.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

cmd/kube-proxy/app/server.go
@@ -146,6 +146,7 @@ func AddFlags(options *Options, fs *pflag.FlagSet) {
&options.config.Conntrack.TCPCloseWaitTimeout.Duration, "conntrack-tcp-timeout-close-wait",
options.config.Conntrack.TCPCloseWaitTimeout.Duration,
"NAT timeout for TCP connections in the CLOSE_WAIT state")
fs.BoolVar(&options.config.EnableProfiling, "profiling", options.config.EnableProfiling, "If true enables profiling via web interface on /debug/pprof handler.")
Copy link
Member

Choose a reason for hiding this comment

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

I thnk it's appropriate to have a flag like this - I am assuming it is not generally going to be on, and it's not a "knob" but a very fundamental operating mode, not something users would change except to debug?

MrHohn
MrHohn reviewed May 17, 2017
View reviewed changes
pkg/apis/componentconfig/types.go
@@ -101,6 +101,8 @@ type KubeProxyConfiguration struct {
// metricsBindAddress is the IP address and port for the metrics server to serve on,
// defaulting to 127.0.0.1:10249 (set to 0.0.0.0 for all interfaces)
MetricsBindAddress string
// enableProfiling enables profiling via web interface on /debug/pprof handler.
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to indicate the profiling will be served by the metrics server, as it may not be obvious to user.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

wojtek-t
wojtek-t commented May 17, 2017
View reviewed changes
Copy link
Member Author

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

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

@MrHohn - PTAL

pkg/proxy/iptables/proxier.go Outdated
@@ -516,6 +516,7 @@ func (proxier *Proxier) Sync() {
func (proxier *Proxier) SyncLoop() {
t := time.NewTicker(proxier.syncPeriod)
defer t.Stop()
registerMetrics()
Copy link
Member Author

Choose a reason for hiding this comment

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

done

pkg/apis/componentconfig/types.go
@@ -101,6 +101,8 @@ type KubeProxyConfiguration struct {
// metricsBindAddress is the IP address and port for the metrics server to serve on,
// defaulting to 127.0.0.1:10249 (set to 0.0.0.0 for all interfaces)
MetricsBindAddress string
// enableProfiling enables profiling via web interface on /debug/pprof handler.
Copy link
Member Author

Choose a reason for hiding this comment

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

done

MrHohn
MrHohn approved these changes May 17, 2017
View reviewed changes
Copy link
Member

@MrHohn MrHohn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@freehan
Copy link
Contributor

freehan commented May 17, 2017

Cool!
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45953, 45889)

@k8s-github-robot k8s-github-robot merged commit 9a9a296 into kubernetes:master May 17, 2017
@wojtek-t wojtek-t deleted the kube_proxy_handlers branch May 23, 2017 10:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thockin thockin thockin left review comments

@MrHohn MrHohn MrHohn approved these changes

Assignees

@freehan freehan

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kube-proxy cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@wojtek-t @freehan @k8s-github-robot @thockin @MrHohn @k8s-ci-robot