Add pprof to all VPA components #7153

adrianmoisey · 2024-08-10T12:40:18Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add proof to all 3 VPA components.

Which issue(s) this PR fixes:

Special notes for your reviewer:

The change was inspired by the cluster-autoscaler implementation of pprof. I figured it made sense to move a lot of the logic out of the metrics package, as that code handled metrics, health check and pprof.

EDIT: I don't know if I handled the dependencies and vendoring correctly, so I left them in their own commit in case that needs to be changed.

Does this PR introduce a user-facing change?

Add --profiling parameter to all 3 VPA components to enable pprof

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2024-08-10T12:40:28Z

Hi @adrianmoisey. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

adrianmoisey · 2024-08-19T18:48:03Z

@kwiesmueller any chance you can look at this?

kwiesmueller · 2024-08-19T18:53:17Z

/assign @voelzmo @raywainman

raywainman · 2024-09-05T02:17:50Z

vertical-pod-autoscaler/go.mod

@@ -13,6 +13,7 @@ require (
 	golang.org/x/time v0.4.0
 	k8s.io/api v0.28.3
 	k8s.io/apimachinery v0.28.3
+	k8s.io/apiserver v0.0.0-00010101000000-000000000000


It's weird that it is using a pseudo-version number... Do you know why it is importing this?

What happens if you try to pull in a specific version? Maybe we can use the same as cluster autoscaler?

autoscaler/cluster-autoscaler/go.mod

Line 47 in 386f0f7

k8s.io/apiserver v0.31.0-beta.0

(I'm not an expert in this so sorry if this is totally off - this just looks weird to me)

I think 679d6bd fixes it
I used 0.28.3, since that's what the VPA currently uses

raywainman · 2024-09-05T02:27:56Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

 	metrics_admission.Register()

+	go func() {
+		pathRecorderMux := mux.NewPathRecorderMux("vertical-pod-autoscaler")


reading the documentation and it looks like this name is used for logging, should we make this unique across each binary?

eg. here we would make it vertical-pod-autoscaler-admission-controller

Fixed in e4fc614

raywainman · 2024-09-05T17:23:47Z

/lgtm

raywainman · 2024-09-05T17:23:54Z

/ok-to-test

adrianmoisey · 2024-09-14T18:43:40Z

/assign voelzmo

adrianmoisey · 2024-09-24T08:17:07Z

Hey @voelzmo can you approve this?

kwiesmueller · 2024-09-24T17:08:15Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

 	metrics_admission.Register()

+	go func() {


Would it make sense to put this block into some shared util package?

Done in 7ec04b3

kwiesmueller · 2024-09-24T17:09:56Z

vertical-pod-autoscaler/pkg/admission-controller/main.go

+		pathRecorderMux := mux.NewPathRecorderMux("vertical-pod-autoscaler-admission-controller")
+
+		if *enableProfiling {
+			routes.Profiling{}.Install(pathRecorderMux)


Is depending on the apiserver package the best practice here? I'm not sure if that's entirely safe as they may add profiling things that don't fully match.
Might there be something similar focused on api clients (as we're not really apiservers, even though at least the admission controller is serving).

Yeah, valid point.
I can't any client side library that includes a pprof function that can be reused.
Since your other comment is suggesting that this chunk of code gets moved to a central place to be re-used, I'll just manually setup the pprof paths there, rather than bringing in a new dependancy

Done in 7ec04b3

Something I realised, is that I copied the cluster-autoscaler way of adding pprof, see:

autoscaler/cluster-autoscaler/main.go

Lines 689 to 691 in 77bf580

if *enableProfiling {

routes.Profiling{}.Install(pathRecorderMux)

}

I do agree that for adding pprof it doesn't make sense to add an entire new dependency, so may be that's an opportunity to go clean up cluster-autoscaler too.

kwiesmueller · 2024-09-24T17:10:47Z

Thanks for sending this! Very nice to get profiling added.

Fixes kubernetes#6946

kwiesmueller

Thanks!
/lgtm
/approve

k8s-ci-robot · 2024-09-25T13:51:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, kwiesmueller

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:

~~vertical-pod-autoscaler/OWNERS~~ [kwiesmueller]

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 added kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 10, 2024

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 10, 2024

k8s-ci-robot requested a review from kgolab August 10, 2024 12:40

k8s-ci-robot added the area/vertical-pod-autoscaler label Aug 10, 2024

k8s-ci-robot requested a review from kwiesmueller August 10, 2024 12:40

k8s-ci-robot assigned raywainman and voelzmo Aug 19, 2024

raywainman reviewed Sep 5, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2024

adrianmoisey mentioned this pull request Sep 6, 2024

REQUEST: New membership for adrianmoisey kubernetes/org#5133

Closed

11 tasks

kwiesmueller requested changes Sep 24, 2024

View reviewed changes

Enable pprof for all VPA components

7ec04b3

Fixes kubernetes#6946

adrianmoisey force-pushed the add-pprofile branch from e4fc614 to 7ec04b3 Compare September 25, 2024 06:45

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 25, 2024

kwiesmueller approved these changes Sep 25, 2024

View reviewed changes

k8s-ci-robot assigned kwiesmueller Sep 25, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 25, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2024

k8s-ci-robot merged commit 1c35e11 into kubernetes:master Sep 25, 2024
7 checks passed

ialidzhikov mentioned this pull request Jan 21, 2025

VPA 1.3.0 release gardener/gardener#11218

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pprof to all VPA components #7153

Add pprof to all VPA components #7153

adrianmoisey commented Aug 10, 2024 •

edited

Loading

k8s-ci-robot commented Aug 10, 2024

adrianmoisey commented Aug 19, 2024

kwiesmueller commented Aug 19, 2024

raywainman Sep 5, 2024

adrianmoisey Sep 5, 2024

raywainman Sep 5, 2024

adrianmoisey Sep 5, 2024

raywainman commented Sep 5, 2024

raywainman commented Sep 5, 2024

adrianmoisey commented Sep 14, 2024

adrianmoisey commented Sep 24, 2024

kwiesmueller Sep 24, 2024

adrianmoisey Sep 25, 2024

kwiesmueller Sep 24, 2024

adrianmoisey Sep 24, 2024

adrianmoisey Sep 25, 2024

kwiesmueller commented Sep 24, 2024

kwiesmueller left a comment

k8s-ci-robot commented Sep 25, 2024

	if *enableProfiling {
	routes.Profiling{}.Install(pathRecorderMux)
	}

Add pprof to all VPA components #7153

Add pprof to all VPA components #7153

Conversation

adrianmoisey commented Aug 10, 2024 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 10, 2024

adrianmoisey commented Aug 19, 2024

kwiesmueller commented Aug 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raywainman commented Sep 5, 2024

raywainman commented Sep 5, 2024

adrianmoisey commented Sep 14, 2024

adrianmoisey commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwiesmueller commented Sep 24, 2024

kwiesmueller left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 25, 2024

adrianmoisey commented Aug 10, 2024 •

edited

Loading