This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

[kn-admin] support downloading profiling data for knative components #72

Merged

knative-prow-robot merged 19 commits into knative:master from xliuxu:pprof

Aug 17, 2020

Contributor

xliuxu commented Jul 21, 2020

This is a draft PR for @chaozbj and I to co-work to implement a profiling subcommand for kn-admin plugin.
Details refer to #66

knative-prow-robot added the do-not-merge/work-in-progress label

googlebot added the cla: yes label

knative-prow-robot requested review from maximilien and zhanggbj

July 21, 2020 15:38

knative-prow-robot commented Jul 21, 2020

Hi @lanceliuu. Thanks for your PR.

I'm waiting for a knative 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/test-infra repository.

knative-prow-robot added the needs-ok-to-test label

mattmoor reviewed

View reviewed changes

xliuxu force-pushed the pprof branch from 14987e4 to 2ae278a Compare

July 21, 2020 15:39

knative-prow-robot added the size/XXL label

mattmoor reviewed

View reviewed changes

mattmoor reviewed

View reviewed changes

Member

mattmoor left a comment

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

mattmoor reviewed

View reviewed changes

Member

mattmoor left a comment

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -name '*.pb.go' -prune -o -type f -name '*.go' -print)
goimports -w $(find -name '*.go' | grep -v vendor | grep -v third_party | grep -v .pb.go | grep -v wire_gen.go)

Member

daisy-ycguo commented Jul 22, 2020

/ok-to-test

knative-prow-robot added ok-to-test and removed needs-ok-to-test labels

xliuxu force-pushed the pprof branch from 2ae278a to abd9218 Compare

July 23, 2020 05:55

knative-prow-robot added size/L and removed size/XXL labels

maximilien suggested changes

View reviewed changes

Contributor

maximilien left a comment

Various feedback... I know this is DRAFT but please start adding tests (unit and e2e). At least make sure you have a plan. Perhaps have tests with empty bodies to start with?

plugins/admin/pkg/command/profiling/downloader.go Outdated

+              // ProfileType enums for all supported profiles
+              type ProfileType int
+              const secondsKey = "seconds"

Contributor

maximilien Jul 23, 2020

why is this not in the const block above?

plugins/admin/pkg/command/profiling/downloader.go Outdated

+              type Downloader struct {
+              	podName   string
+              	namespace string
+              	// Close this will trigger closing for the endCh create by our own and then cancel all sub goroutines

Contributor

maximilien Jul 23, 2020

Not sure I understand this comment? "Close ..." you mean closing the stopCh channel?

plugins/admin/pkg/command/profiling/downloader.go Outdated

+              		err := fw.ForwardPorts()
+              		// if the func ForwardPorts() returns, the connection should not be available.
+              		if err != nil {
+              			// TODO: Log for error?

Contributor

maximilien Jul 23, 2020

Yes, definitely add logging support. Perhaps retries IF/WHEN it makes sense.

plugins/admin/pkg/command/profiling/downloader.go Outdated

+              }
+              // ProfilingTime is option to add a seconds param in the http request
+              type ProfilingTime time.Duration

Contributor

maximilien Jul 23, 2020

Does it make sense to separate this (and methods) into its own file? Might make things easier to navigate and manage in the long run...

plugins/admin/pkg/command/profiling/downloader.go

+              // Download specific type of profile with options
+              func (d *Downloader) Download(t ProfileType, output io.Writer, options ...DownloadOptions) error {
+              	if t <= ProfileTypeUnknown || t >= ProfileType(len(ProfileEndpoints)) {

Contributor

maximilien Jul 23, 2020

Hmm, this condition reads odd to me. The value ProfileTypeUnknown as per type above is a string. What would t <= another string result to? Maybe I am reading this wrong...

Contributor Author

xliuxu Jul 24, 2020

ProfileType is enum type of int. So ProfileTypeUnknown is the default value 0. There is an array declared at line 40 to map ProfileType to the coressponding string.

plugins/admin/pkg/command/profiling/main.go Outdated

+              	"k8s.io/client-go/tools/clientcmd"
+              )
+              // DO NOT COMMIT

Contributor

maximilien Jul 23, 2020

"DO NOT COMMIT" and it's in the PR!! :)

Contributor Author

xliuxu Jul 24, 2020

This is just sample code for reference as I need to co-work with @chaozbj. This file will NOT exist in the final PR.

plugins/admin/pkg/command/profiling/main.go Outdated

+              	if err != nil {
+              		log.Fatal(err)
+              	}
+              	f1, _ := ioutil.TempFile("", "")

Contributor

maximilien Jul 23, 2020

Why are we ignoring errors here?

Contributor Author

xliuxu Jul 24, 2020

same as above

Contributor Author

xliuxu commented Jul 24, 2020

@maximilien I will fill up tests and then request for review. Thanks for your comments!

knative-prow-robot added size/XXL and removed size/L labels

Contributor Author

xliuxu commented Jul 26, 2020

/retest

knative-prow-robot commented Jul 26, 2020

@lanceliuu: The /test command needs one or more targets.
The following commands are available to trigger jobs:

/test pull-knative-client-contrib-build-tests
/test pull-knative-client-contrib-unit-tests
/test pull-knative-client-contrib-integration-tests

Use /test all to run all jobs.

In response to this:

/test

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/test-infra repository.

Contributor

maximilien commented Jul 30, 2020 •

edited

Loading

Hi, @lanceliuu, is this ready for new review? Does not seem like PROW ran any tests? Please tag me when ready for another review. Thanks.

xliuxu marked this pull request as ready for review

July 31, 2020 01:58

knative-prow-robot removed the do-not-merge/work-in-progress label

Contributor Author

xliuxu commented Jul 31, 2020

/retest

mattmoor reviewed

View reviewed changes

plugins/admin/README.adoc Outdated Show resolved Hide resolved

plugins/admin/pkg/command/profiling/profiling.go Outdated Show resolved Hide resolved

Contributor Author

xliuxu commented Jul 31, 2020

@maximilien @zhanggbj This PR is ready for review. Thanks!
e2e tests for this command is currently missing since it depends on #63

knative-prow-robot assigned zhanggbj

knative-prow-robot added the lgtm label

zhanggbj commented Aug 16, 2020

Hi @lanceliuu @chaozbj , already LGTM, would you please help to rebase it? Thanks!

knative-prow-robot added the needs-rebase label

Lance Liu and others added 19 commits

August 17, 2020 10:16


          add utils for profile downloader

41f4eab

Signed-off-by: Lance Liu <[email protected]>


          fix typo and package name

1088d03


          remove unnecessary chan in Downloader struct

1dfbda9


          move the example main to seperate folder

db93bc6

Signed-off-by: Lance Liu <[email protected]>


          check response code before saving body

7b1c3b8


          add profiling subcommand

0ea4ca6

Signed-off-by: chaozbj <[email protected]>


          implemented profiling command and ut codes

ea66769

Signed-off-by: chaozbj <[email protected]>


          reworded test function names

a2b2866

Signed-off-by: chaozbj <[email protected]>


          improved expected messages for UT

9214c75

Signed-off-by: chaozbj <[email protected]>


          improved command help

e3b7869

Signed-off-by: chaozbj <[email protected]>


          remove main.go

e193037

add interface to downloader
add tests to downloader
split downloadOptions to seperate file

Signed-off-by: Lance Liu <[email protected]>


          add licence to files

542cf3c

Signed-off-by: Lance Liu <[email protected]>


          change downloader initliazer interface for UT

8cfeaaa

fix some UT for downloader

Signed-off-by: Lance Liu <[email protected]>


          Add more tests for downloader connect function

Signed-off-by: Lance Liu <[email protected]>


          use assert.Error to check error

24be06a

Signed-off-by: Lance Liu <[email protected]>


          fix lint errors

4eb6f74

Signed-off-by: Lance Liu <[email protected]>


          fix typos

d811f70

Signed-off-by: Lance Liu <[email protected]>


          improve wording

12c6882

Signed-off-by: chaozbj <[email protected]>


          add e2e to profiling subcommand

5be890a

Signed-off-by: Lance Liu <[email protected]>

xliuxu force-pushed the pprof branch from 95aa284 to 5be890a Compare

August 17, 2020 02:19

knative-prow-robot removed lgtm needs-rebase labels

Contributor Author

xliuxu commented Aug 17, 2020

@zhanggbj Rebase done. Thank you.

zhanggbj commented Aug 17, 2020

/lgtm
Great! Thanks!

knative-prow-robot added the lgtm label

knative-prow-robot merged commit 88f3a66 into knative:master

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

approved cla: yes lgtm ok-to-test size/XXL