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

Add pprof for karmada components #2008

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

Poor12
Copy link
Member

@Poor12 Poor12 commented Jun 16, 2022

Signed-off-by: Poor12 shentiecheng@huawei.com

What type of PR is this?
/kind feature

What this PR does / why we need it:
Since we need to do some performance tests for karmada components, we found that there are still some components which do not support pprof.

Because each components have different implementing logic, some are based on genericAPIServer, some are base on controller-runtime(controller-runtime now do not support pprof kubernetes-sigs/controller-runtime#1943). In this PR, we unify the config to enable pprof.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Karmada-agent, Karmada-aggregated-apiserver, Karmada-controller-manager, Karmada-descheduler, Karmada-search, Karmada-scheduler, Karmada-scheduler-estimator, Karmada-webhook now support --enable-pprof to enable pprof and --profiling-bind-address to config address for pprof web server.

@karmada-bot karmada-bot added the kind/feature Categorizes issue or PR as related to a new feature. label Jun 16, 2022
@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 16, 2022
@RainbowMango
Copy link
Member

Please share the test reports and remember to cc the students who are doing the performance thing.

@Poor12
Copy link
Member Author

Poor12 commented Jun 16, 2022

  1. First enable the flag.
    image
  2. Curl PodIP to get pprof file.
    image

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

@lonelyCZ Can you help to take a look?

I'm thinking if we need to introduce the shared flags, just like what we did at klog-flag.

cmd/agent/app/agent.go Outdated Show resolved Hide resolved
cmd/agent/app/options/options.go Outdated Show resolved Hide resolved
@lonelyCZ
Copy link
Member

I'm thinking if we need to introduce the shared flags, just like what we did at klog-flag.

I think it is a good idea. These flags apply to karmada-agent and karmada-controller-manager. Such as pkg/sharedcli/pprofflag/pprofflag.go

@karmada-bot karmada-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 25, 2022
@Poor12 Poor12 force-pushed the add-pprof branch 2 times, most recently from 9daf78d to 054e81f Compare June 25, 2022 08:12
@RainbowMango
Copy link
Member

Maybe the reason why karmada-schedule enabled pprof is:

  • imports k8s.io/apiserver/pkg/server at here -->
  • k8s.io/apiserver/pkg/server imports k8s.io/apiserver/pkg/server/routes at here
  • k8s.io/apiserver/pkg/server/routes imports net/http/pprof at here

And finally, karmada-scheduler starts the http-server.

@Poor12
Please confirm.
If we are going to introduce flags for profiling, we should ensure the profiling feature could be disabled.
So, I think we need another PR before this.

By the way, the similar issue happens at karmada-descheduler.

@karmada-bot karmada-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 4, 2022
@Poor12 Poor12 force-pushed the add-pprof branch 2 times, most recently from 8657a4b to 4528cad Compare July 4, 2022 12:48
@lonelyCZ
Copy link
Member

lonelyCZ commented Jul 5, 2022

/assign

pkg/sharedcli/profileflag/profileflag.go Outdated Show resolved Hide resolved
cmd/agent/app/agent.go Outdated Show resolved Hide resolved
@Poor12 Poor12 force-pushed the add-pprof branch 2 times, most recently from e33fab8 to c9eb604 Compare July 6, 2022 08:05
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

Generally looks good to me.

pkg/sharedcli/profileflag/profileflag.go Outdated Show resolved Hide resolved
pkg/sharedcli/profileflag/profileflag.go Show resolved Hide resolved
Signed-off-by: Poor12 <shentiecheng@huawei.com>
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 6, 2022
@RainbowMango
Copy link
Member

Please describe how to grab the pprof data from Karmada build by hack/karmada-local-up.sh.

@Poor12
Copy link
Member Author

Poor12 commented Jul 7, 2022

cc @lonelyCZ to review. For further usage, you can base on #2141

@lonelyCZ
Copy link
Member

lonelyCZ commented Jul 7, 2022

I just tested it that worked fine. Thanks~

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2022
@karmada-bot karmada-bot merged commit 5ecb4ba into karmada-io:master Jul 7, 2022
@Poor12 Poor12 deleted the add-pprof branch July 7, 2022 03:36
@RainbowMango RainbowMango added this to the v1.3 milestone Aug 30, 2022
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. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants