Add support for dataset,runtime,webhook pprof by wangyanghack · Pull Request #985 · fluid-cloudnative/fluid

wangyanghack · 2021-08-23T15:18:28Z

Ⅰ. Describe what this PR does

add instant live visualization of statistics for performance analysis

Ⅱ. Does this pull request fix one issue?

fixes #780

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

I cannot decide on nodeport number. please give me some advice. thanks.

cheyang · 2021-08-24T02:49:17Z

Thanks very much for your great contributions. If you'd like to add documentation, it will be great!

wangyanghack · 2021-08-24T03:18:49Z

Thanks very much for your great contributions. If you'd like to add documentation, it will be great!
Thanks very much for your response. I will add documentation recently.
And Can you give me some advice to decide which number nodeport use or configurable? thanks

cheyang · 2021-08-26T02:18:10Z

And Can you give me some advice to decide which number nodeport use or configurable? thanks

I think you can make nodeport configurable and the default port could be 50000.

Could you rebase you code so that it can fix the CI. Thanks.

codecov · 2021-08-29T14:58:07Z

Codecov Report

Merging #985 (7fdf4e0) into master (ab54f1e) will increase coverage by 0.01%.
The diff coverage is 79.16%.

@@            Coverage Diff             @@
##           master     #985      +/-   ##
==========================================
+ Coverage   70.48%   70.50%   +0.01%     
==========================================
  Files         221      222       +1     
  Lines       12122    12146      +24     
==========================================
+ Hits         8544     8563      +19     
- Misses       2659     2662       +3     
- Partials      919      921       +2

Impacted Files	Coverage Δ
pkg/utils/kubeclient/exec.go	`25.97% <ø> (ø)`
pkg/utils/pprof.go	`79.16% <79.16%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ab54f1e...7fdf4e0. Read the comment docs.

wangyanghack · 2021-08-30T10:06:47Z

I think you can make nodeport configurable and the default port could be 50000.
Could you rebase you code so that it can fix the CI. Thanks.

I have fixed nodeport and passed the CI.

yangyuliufeng

I have tested this pr and found no problems.
I suggest changing the naming of some files such as the svc name.
Also， I'm worried that it's unsafe to expose program indicators through HTTP.

yangyuliufeng · 2021-09-02T07:35:45Z

charts/fluid/fluid/templates/controller/alluxioruntime_service.yaml

+apiVersion: v1
+kind: Service
+metadata:
+  name: fluid-pod-alluxioruntime-controller


It's not suitable to be 'fluid-pod-alluxioruntime-controller', how about 'fluid-alluxioruntime-controller-ststic'

yangyuliufeng · 2021-09-02T07:41:34Z

charts/fluid/fluid/values.yaml

+    statistics:
+      enabled: false
+      nodePort: 50002


I think it's better to put it in runtime.controllerStatistics, not three pieces.
我觉得统一写一个runtime.controllerStatistics比较好，不要分三块

cmd/dataset/main.go

wangyanghack · 2021-09-13T04:58:18Z

Also， I'm worried that it's unsafe to expose program indicators through HTTP.

@yangyuliufeng @cheyang I cannot decide where tls crt and key should be.
My plan is :

execute a certificate.sh to generate tls crt and key
execute kubectl create secret tls tls-secret --cert=path/to/tls.cert --key=path/to/tls.key to generate a secret
pod of fluid volumemount this secret to use tls crt and key

But my plan has a problem: it cannot execute certificate.sh and create secret automatically when helm install.
Please give me some advice. Thanks very much.

cheyang · 2021-09-14T01:31:32Z

charts/fluid/fluid/templates/controller/dataset_service.yaml

@@ -0,0 +1,16 @@
+{{ if .Values.dataset.controller.statistics.enabled -}}


Maybe the service is not necessary. I think we can connect to ip address of pod directly.

cheyang · 2021-09-14T01:32:32Z

cmd/alluxio/main.go

+	"net/http"
+	"os"
+
+	"github.com/arl/statsviz"


How about using "net/http/pprof" ? Kubernetes is also using this. And we can avoid importing new package.

Thanks very much for your good advice. I will use pprof to implement it.

cheyang · 2021-09-14T01:40:04Z

Also， I'm worried that it's unsafe to expose program indicators through HTTP.

@yangyuliufeng @cheyang I cannot decide where tls crt and key should be.
My plan is :

execute a certificate.sh to generate tls crt and key

execute kubectl create secret tls tls-secret --cert=path/to/tls.cert --key=path/to/tls.key to generate a secret

pod of fluid volumemount this secret to use tls crt and key

But my plan has a problem: it cannot execute certificate.sh and create secret automatically when helm install.
Please give me some advice. Thanks very much.

Thank you for your contribution! My suggestion is that if we can leverage the implementation in Kubernetes. Btw, is it enabled by default? https://github.com/kubernetes/kubernetes/blob/ea0764452222146c47ec826977f49d7001b0ea8c/staging/src/k8s.io/apiserver/pkg/server/routes/profiling.go#L30

cheyang · 2021-09-14T01:43:35Z

I cannot decide on nodeport number. please give me some advice. thanks.

I think service to expose is not necessary for debugging.

cheyang · 2021-10-03T09:19:26Z

@wangyanghack could you refer to https://github.com/banzaicloud/logging-operator/blob/master/main.go#L122-L129? thanks in advance!

wangyanghack · 2021-10-05T02:45:48Z

@wangyanghack could you refer to https://github.com/banzaicloud/logging-operator/blob/master/main.go#L122-L129? thanks in advance!

Thank you very much for your careful guidance and patience.
I have followed your advice to add pprof. Please review it and give me more advice.

wangyanghack · 2021-11-08T09:02:21Z

@cheyang please take a moment to review my code. I will apologize in advance if it is not still satisfied with you again. Thanks a lot.

cheyang · 2021-12-06T05:53:35Z

@wangyanghack Please fix the conflict, thanks.

cheyang · 2022-01-04T12:55:27Z

cmd/alluxio/main.go

 	}))

+	if pprofAddr != "" {
+		glog.Infof("Enabling pprof with address %s", pprofAddr)


Can we use the same log system in the controller. I suggest not to use glog here rahter than github.com/go-logr/logr.

cheyang · 2022-01-04T12:57:55Z

cmd/dataset/main.go

 		}
 	}))

+	if pprofAddr != "" {


How about to create a common function to launch pprof? This piece of code has been repeated several times. Just a suggestion, WDYT?

yes, I will create a common function now

cheyang · 2022-01-05T05:21:18Z

pkg/utils/pprof.go

@@ -0,0 +1,47 @@
+package utils


Please add license header. Thanks.

cheyang · 2022-01-05T05:22:01Z

pkg/utils/pprof.go

+			Addr:    pprofAddr,
+			Handler: mux,
+		}
+		glog.Infof("Starting pprof HTTP server at %s", pprofServer.Addr)


Please use logr instead of glog. Thanks.

xieydd

@wangyanghack We can add test for new func to improve the diff hit of codecov, thanks.

Signed-off-by: 汪洋 <1264017613@qq.com>

wangyanghack force-pushed the master branch from 0ae056c to c5cd5cd Compare August 23, 2021 15:19

cheyang requested a review from yangyuliufeng August 24, 2021 02:43

wangyanghack force-pushed the master branch 4 times, most recently from 3db5999 to 8d9bac0 Compare August 29, 2021 14:50

cheyang requested a review from xieydd September 1, 2021 09:35

yangyuliufeng requested changes Sep 2, 2021

View reviewed changes

wangyanghack commented Sep 2, 2021

View reviewed changes

cmd/dataset/main.go Outdated Show resolved Hide resolved

cheyang requested changes Sep 14, 2021

View reviewed changes

wangyanghack force-pushed the master branch 2 times, most recently from e6315e1 to 209a813 Compare October 5, 2021 02:24

wangyanghack requested a review from cheyang November 8, 2021 06:28

wangyanghack closed this Nov 8, 2021

wangyanghack reopened this Nov 8, 2021

wangyanghack force-pushed the master branch from 861163a to 2e52bb1 Compare November 8, 2021 08:43

TrafalgarZZZ mentioned this pull request Dec 6, 2021

[FEATURES] Instant live visualization of Fluid components statistics (GC, MemStats, etc.). #780

Closed

wangyanghack closed this Dec 18, 2021

wangyanghack reopened this Dec 18, 2021

wangyanghack closed this Dec 29, 2021

wangyanghack force-pushed the master branch from 2e52bb1 to cb082b9 Compare December 29, 2021 01:54

wangyanghack reopened this Dec 29, 2021

wangyanghack changed the title ~~add instant live visualization of statistics for performance analysis~~ Add support for dataset,runtime,webhook pprof Dec 29, 2021

wangyanghack force-pushed the master branch 4 times, most recently from 733278d to 368bdaf Compare December 29, 2021 12:38

wangyanghack closed this Dec 29, 2021

wangyanghack reopened this Dec 29, 2021

wangyanghack force-pushed the master branch from 368bdaf to 76154d2 Compare January 4, 2022 12:26

cheyang requested changes Jan 4, 2022

View reviewed changes

cheyang reviewed Jan 4, 2022

View reviewed changes

cheyang requested review from cheyang and yangyuliufeng January 4, 2022 13:01

wangyanghack force-pushed the master branch from 76154d2 to 8154833 Compare January 4, 2022 13:27

cheyang reviewed Jan 5, 2022

View reviewed changes

pkg/utils/pprof.go

@@ -0,0 +1,47 @@

package utils

Copy link

Collaborator

cheyang Jan 5, 2022

Choose a reason for hiding this comment

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

Please add license header. Thanks.

cheyang reviewed Jan 5, 2022

View reviewed changes

xieydd suggested changes Jan 5, 2022

View reviewed changes

wangyanghack force-pushed the master branch 2 times, most recently from acc6f84 to 6e33863 Compare January 5, 2022 12:46

Add support for dataset,runtime,webhook pprof

7fdf4e0

Signed-off-by: 汪洋 <1264017613@qq.com>

wangyanghack force-pushed the master branch from 6e33863 to 7fdf4e0 Compare January 5, 2022 13:18

cheyang approved these changes Jan 6, 2022

View reviewed changes

cheyang merged commit 97a099c into fluid-cloudnative:master Jan 6, 2022

github-actions bot mentioned this pull request Feb 27, 2026

🌱 CNCF mission generation 2026-02-27 kubestellar/console-kb#6

Closed

		@@ -0,0 +1,16 @@
		{{ if .Values.dataset.controller.statistics.enabled -}}

Conversation

wangyanghack commented Aug 23, 2021

Ⅰ. Describe what this PR does

Ⅱ. Does this pull request fix one issue?

Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

Uh oh!

cheyang commented Aug 24, 2021

Uh oh!

wangyanghack commented Aug 24, 2021

Uh oh!

cheyang commented Aug 26, 2021

Uh oh!

codecov bot commented Aug 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

wangyanghack commented Aug 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yangyuliufeng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wangyanghack commented Sep 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheyang Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheyang commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cheyang commented Sep 14, 2021

Uh oh!

cheyang commented Oct 3, 2021

Uh oh!

wangyanghack commented Oct 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangyanghack commented Nov 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cheyang commented Dec 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xieydd left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Aug 29, 2021 •

edited

Loading

wangyanghack commented Aug 30, 2021 •

edited

Loading

wangyanghack commented Sep 13, 2021 •

edited

Loading

cheyang Sep 14, 2021 •

edited

Loading

cheyang commented Sep 14, 2021 •

edited

Loading

wangyanghack commented Oct 5, 2021 •

edited

Loading

wangyanghack commented Nov 8, 2021 •

edited

Loading