From baf8bffc124c49bab69ad6f6462a66e951210fdf Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 25 Nov 2024 11:20:22 +0000 Subject: [PATCH 01/11] feat: Add flags to enable CPU and memory profiling Add capability to profile cli command using https://go.dev/blog/pprof Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 14 +++++++- cmd/helm/profiling.go | 76 +++++++++++++++++++++++++++++++++++++++++++ cmd/helm/root.go | 23 +++++++++++++ 3 files changed, 112 insertions(+), 1 deletion(-) create mode 100644 cmd/helm/profiling.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0c2f88453..aa67b5aef 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -276,12 +276,24 @@ Like any good open source project, we use Pull Requests (PRs) to track code chan or explicitly request another OWNER do that for them. - If the owner of a PR is _not_ listed in `OWNERS`, any core maintainer may merge the PR. -#### Documentation PRs +### Documentation PRs Documentation PRs should be made on the docs repo: . Keeping Helm's documentation up to date is highly desirable, and is recommended for all user facing changes. Accurate and helpful documentation is critical for effectively communicating Helm's behavior to a wide audience. Small, ad-hoc changes/PRs to Helm which introduce user facing changes, which would benefit from documentation changes, should apply the `docs needed` label. Larger changes associated with a HIP should track docs via that HIP. The `docs needed` label doesn't block PRs, and maintainers/PR reviewers should apply discretion judging in whether the `docs needed` label should be applied. +### Testing PRs + +During development, you need to add automated tests where possible. There are a few `make test*` targets that you can use to execute your unit or integration tests. If your contribution requires profiling to check memory and/or CPU usage, you can use `--cpuprofile` and/or `--memprofile` global cli flags to run collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the profiling data. + +Example invocation that collects profiling data +``` +helm show all bitnami/nginx --memprofile mem.prof --cpuprofile cpu.prof + +# Visualize graphs. You need to have installed graphviz in you system +go tool pprof -http=":8000" cpu.prof +``` + ## The Triager Each week, one of the core maintainers will serve as the designated "triager" starting after the diff --git a/cmd/helm/profiling.go b/cmd/helm/profiling.go new file mode 100644 index 000000000..090532416 --- /dev/null +++ b/cmd/helm/profiling.go @@ -0,0 +1,76 @@ +// Profile CPU and memory usage of Helm commands + +package main + +import ( + "fmt" + "os" + "runtime" + "runtime/pprof" + "strings" + + "github.com/spf13/cobra" +) + +var ( + cpuProfileFile *os.File +) + +// startProfiling starts profiling CPU usage +func startProfiling(cpuprofile string) error { + if cpuprofile != "" { + var err error + cpuProfileFile, err = os.Create(cpuprofile) + if err != nil { + return fmt.Errorf("could not create CPU profile: %w", err) + } + if err := pprof.StartCPUProfile(cpuProfileFile); err != nil { + cpuProfileFile.Close() + cpuProfileFile = nil + return fmt.Errorf("could not start CPU profile: %w", err) + } + } + return nil +} + +// stopProfiling stops profiling CPU and memory usage and writes the results to +// the files specified by --cpuprofile and --memprofile flags respectively. +func stopProfiling(memprofile string) error { + errs := []string{} + + // Stop CPU profiling if it was started + if cpuProfileFile != nil { + pprof.StopCPUProfile() + err := cpuProfileFile.Close() + if err != nil { + errs = append(errs, err.Error()) + } + cpuProfileFile = nil + } + + if memprofile != "" { + f, err := os.Create(memprofile) + if err != nil { + errs = append(errs, err.Error()) + } + defer f.Close() + + runtime.GC() // get up-to-date statistics + if err := pprof.WriteHeapProfile(f); err != nil { + errs = append(errs, err.Error()) + } + } + + if len(errs) > 0 { + return fmt.Errorf("errors while stopping profiling: [%s]", strings.Join(errs, ", ")) + } + + return nil +} + +// addProfilingFlags adds the --cpuprofile and --memprofile flags to the given command. +func addProfilingFlags(cmd *cobra.Command) { + // Persistent flags to make available to subcommands + cmd.PersistentFlags().String("cpuprofile", "", "File path to write cpu profiling data") + cmd.PersistentFlags().String("memprofile", "", "File path to write memory profiling data") +} diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 2ba8a882e..fa1e6c6d0 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -95,6 +95,26 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string Short: "The Helm package manager for Kubernetes.", Long: globalUsage, SilenceUsage: true, + PersistentPreRun: func(cmd *cobra.Command, args []string) { + cpuprof, err := cmd.Flags().GetString("cpuprofile") + if err != nil { + log.Printf("Warning: Failed to get cpuprofile flag: %v", err) + } + + if err := startProfiling(cpuprof); err != nil { + log.Printf("Warning: Failed to start profiling: %v", err) + } + }, + PersistentPostRun: func(cmd *cobra.Command, args []string) { + memprof, err := cmd.Flags().GetString("memprofile") + if err != nil { + log.Printf("Warning: Failed to get memprofile flag: %v", err) + } + + if err := stopProfiling(memprof); err != nil { + log.Printf("Warning: Failed to stop profiling: %v", err) + } + }, } flags := cmd.PersistentFlags() @@ -206,6 +226,9 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string // Check for expired repositories checkForExpiredRepos(settings.RepositoryConfig) + // CPU and memory profiling flags that are available to all commands + addProfilingFlags(cmd) + return cmd, nil } From af622e8887fc104e020df6ed8deb6464653b34b6 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 25 Nov 2024 12:01:18 +0000 Subject: [PATCH 02/11] Additional review fixes from PR Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 6 +++--- cmd/helm/profiling.go | 16 +++++++++++++++- cmd/helm/root.go | 4 ++-- 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aa67b5aef..796c661bc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,13 +284,13 @@ Small, ad-hoc changes/PRs to Helm which introduce user facing changes, which wou ### Testing PRs -During development, you need to add automated tests where possible. There are a few `make test*` targets that you can use to execute your unit or integration tests. If your contribution requires profiling to check memory and/or CPU usage, you can use `--cpuprofile` and/or `--memprofile` global cli flags to run collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the profiling data. +During development, you need to add automated tests where possible. There are a few `make test*` targets that you can use to execute your unit or integration tests. If your contribution requires profiling to check memory and/or CPU usage, you can use `--cpuprofile` and/or `--memprofile` global cli flags to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. -Example invocation that collects profiling data +Example analysing collected profiling data ``` helm show all bitnami/nginx --memprofile mem.prof --cpuprofile cpu.prof -# Visualize graphs. You need to have installed graphviz in you system +# Visualize graphs. You need to have installed graphviz package in your system go tool pprof -http=":8000" cpu.prof ``` diff --git a/cmd/helm/profiling.go b/cmd/helm/profiling.go index 090532416..e1ca62853 100644 --- a/cmd/helm/profiling.go +++ b/cmd/helm/profiling.go @@ -1,4 +1,18 @@ -// Profile CPU and memory usage of Helm commands +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ package main diff --git a/cmd/helm/root.go b/cmd/helm/root.go index fa1e6c6d0..50cb87e37 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -95,7 +95,7 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string Short: "The Helm package manager for Kubernetes.", Long: globalUsage, SilenceUsage: true, - PersistentPreRun: func(cmd *cobra.Command, args []string) { + PersistentPreRun: func(cmd *cobra.Command, _ []string) { cpuprof, err := cmd.Flags().GetString("cpuprofile") if err != nil { log.Printf("Warning: Failed to get cpuprofile flag: %v", err) @@ -105,7 +105,7 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string log.Printf("Warning: Failed to start profiling: %v", err) } }, - PersistentPostRun: func(cmd *cobra.Command, args []string) { + PersistentPostRun: func(cmd *cobra.Command, _ []string) { memprof, err := cmd.Flags().GetString("memprofile") if err != nil { log.Printf("Warning: Failed to get memprofile flag: %v", err) From e6f829e6b61a2052adf35b35acef2543674f9a44 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 11 Dec 2024 14:15:58 +0000 Subject: [PATCH 03/11] Update CONTRIBUTING.md Co-authored-by: Terry Howe Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 796c661bc..fb875c2ea 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -282,7 +282,7 @@ Documentation PRs should be made on the docs repo: Date: Wed, 11 Dec 2024 14:20:54 +0000 Subject: [PATCH 04/11] Update CONTRIBUTING.md Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fb875c2ea..7b4254592 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,7 +284,7 @@ Small, ad-hoc changes/PRs to Helm which introduce user facing changes, which wou ### Profiling PRs -During development, you need to add automated tests where possible. There are a few `make test*` targets that you can use to execute your unit or integration tests. If your contribution requires profiling to check memory and/or CPU usage, you can use `--cpuprofile` and/or `--memprofile` global cli flags to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. +If your contribution requires profiling to check memory and/or CPU usage, you can use `--cpuprofile` and/or `--memprofile` global cli flags to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. Example analysing collected profiling data ``` From d4175cfcff087420675451b177ef610a760802e4 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 11 Dec 2024 15:17:30 +0000 Subject: [PATCH 05/11] Move pprof paths to HELM_PPROF env variable Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 4 +- cmd/helm/profiling.go | 44 ++++++++++++++++++--- cmd/helm/profiling_test.go | 79 ++++++++++++++++++++++++++++++++++++++ cmd/helm/root.go | 14 +------ 4 files changed, 122 insertions(+), 19 deletions(-) create mode 100644 cmd/helm/profiling_test.go diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7b4254592..0a90054d9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,11 +284,11 @@ Small, ad-hoc changes/PRs to Helm which introduce user facing changes, which wou ### Profiling PRs -If your contribution requires profiling to check memory and/or CPU usage, you can use `--cpuprofile` and/or `--memprofile` global cli flags to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. +If your contribution requires profiling to check memory and/or CPU usage, you can set `HELM_PPROF=cpu=/path/to/cpu.prof,mem=/path/to/mem.prof` environment variable to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. Example analysing collected profiling data ``` -helm show all bitnami/nginx --memprofile mem.prof --cpuprofile cpu.prof +HELM_PPROF=cpu=/path/to/cpu.prof,mem=/path/to/mem.prof helm show all bitnami/nginx # Visualize graphs. You need to have installed graphviz package in your system go tool pprof -http=":8000" cpu.prof diff --git a/cmd/helm/profiling.go b/cmd/helm/profiling.go index e1ca62853..e231a3e0a 100644 --- a/cmd/helm/profiling.go +++ b/cmd/helm/profiling.go @@ -19,6 +19,8 @@ package main import ( "fmt" "os" + "path" + "path/filepath" "runtime" "runtime/pprof" "strings" @@ -28,11 +30,17 @@ import ( var ( cpuProfileFile *os.File + pprofPaths map[string]string ) +func init() { + pprofPaths = parsePProfPaths(os.Getenv("HELM_PPROF")) +} + // startProfiling starts profiling CPU usage -func startProfiling(cpuprofile string) error { - if cpuprofile != "" { +func startProfiling() error { + cpuprofile, ok := pprofPaths["cpu"] + if ok && cpuprofile != "" { var err error cpuProfileFile, err = os.Create(cpuprofile) if err != nil { @@ -48,8 +56,8 @@ func startProfiling(cpuprofile string) error { } // stopProfiling stops profiling CPU and memory usage and writes the results to -// the files specified by --cpuprofile and --memprofile flags respectively. -func stopProfiling(memprofile string) error { +// the files specified by HELM_PPROF=cpu=/path/to/cpu.prof,mem=/path/to/mem.prof +func stopProfiling() error { errs := []string{} // Stop CPU profiling if it was started @@ -62,7 +70,8 @@ func stopProfiling(memprofile string) error { cpuProfileFile = nil } - if memprofile != "" { + memprofile, ok := pprofPaths["mem"] + if ok && memprofile != "" { f, err := os.Create(memprofile) if err != nil { errs = append(errs, err.Error()) @@ -88,3 +97,28 @@ func addProfilingFlags(cmd *cobra.Command) { cmd.PersistentFlags().String("cpuprofile", "", "File path to write cpu profiling data") cmd.PersistentFlags().String("memprofile", "", "File path to write memory profiling data") } + +func parsePProfPaths(env string) map[string]string { + // Initial empty paths + m := map[string]string{} + for _, pprofs := range strings.Split(env, ",") { + // Is of the format mem=/path/to/memprof + tuple := strings.Split(pprofs, "=") + if len(tuple) != 2 { + continue + } + if tuple[0] != "cpu" && tuple[0] != "mem" { + continue + } + + s, err := filepath.Abs(path.Clean(tuple[1])) + if err != nil { + continue + } + if !strings.HasSuffix(s, string(filepath.Separator)) { + // Ensure its not a directory + m[tuple[0]] = s + } + } + return m +} diff --git a/cmd/helm/profiling_test.go b/cmd/helm/profiling_test.go new file mode 100644 index 000000000..65928edbb --- /dev/null +++ b/cmd/helm/profiling_test.go @@ -0,0 +1,79 @@ +/* +Copyright The Helm Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package main + +import ( + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func Test_parsePProfPaths(t *testing.T) { + cwd, err := os.Getwd() + require.NoError(t, err) + + tests := []struct { + name string + env string + want map[string]string + }{ + { + name: "no env", + env: "", + want: map[string]string{}, + }, + { + name: "single path", + env: "cpu=cpu.pprof", + want: map[string]string{ + "cpu": cwd + "/cpu.pprof", + }, + }, + { + name: "mem and cpu paths", + env: "cpu=cpu.pprof,mem=mem.pprof", + want: map[string]string{ + "cpu": cwd + "/cpu.pprof", + "mem": cwd + "/mem.pprof", + }, + }, + { + name: "extra commas", + env: "cpu=cpu.pprof,mem=mem.pprof,", + want: map[string]string{ + "cpu": cwd + "/cpu.pprof", + "mem": cwd + "/mem.pprof", + }, + }, + { + name: "unknown keys", + env: "cpu=cpu.pprof,mem=mem.pprof,foo=bar", + want: map[string]string{ + "cpu": cwd + "/cpu.pprof", + "mem": cwd + "/mem.pprof", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := parsePProfPaths(tt.env) + assert.Equalf(t, tt.want, got, "parsePProfPaths() = %v, want %v", got, tt.want) + }) + } +} diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 50cb87e37..5f739d248 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -96,22 +96,12 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string Long: globalUsage, SilenceUsage: true, PersistentPreRun: func(cmd *cobra.Command, _ []string) { - cpuprof, err := cmd.Flags().GetString("cpuprofile") - if err != nil { - log.Printf("Warning: Failed to get cpuprofile flag: %v", err) - } - - if err := startProfiling(cpuprof); err != nil { + if err := startProfiling(); err != nil { log.Printf("Warning: Failed to start profiling: %v", err) } }, PersistentPostRun: func(cmd *cobra.Command, _ []string) { - memprof, err := cmd.Flags().GetString("memprofile") - if err != nil { - log.Printf("Warning: Failed to get memprofile flag: %v", err) - } - - if err := stopProfiling(memprof); err != nil { + if err := stopProfiling(); err != nil { log.Printf("Warning: Failed to stop profiling: %v", err) } }, From 5b1eb784cb2a1a90ecf4710b3088d2312c3875d4 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Wed, 11 Dec 2024 15:20:45 +0000 Subject: [PATCH 06/11] Fix linter warning Signed-off-by: Evans Mungai --- cmd/helm/root.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/helm/root.go b/cmd/helm/root.go index 5f739d248..edc6376ae 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -95,12 +95,12 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string Short: "The Helm package manager for Kubernetes.", Long: globalUsage, SilenceUsage: true, - PersistentPreRun: func(cmd *cobra.Command, _ []string) { + PersistentPreRun: func(_ *cobra.Command, _ []string) { if err := startProfiling(); err != nil { log.Printf("Warning: Failed to start profiling: %v", err) } }, - PersistentPostRun: func(cmd *cobra.Command, _ []string) { + PersistentPostRun: func(_ *cobra.Command, _ []string) { if err := stopProfiling(); err != nil { log.Printf("Warning: Failed to stop profiling: %v", err) } From 33aa8b53973b9a65e14ffbb6f5f9c99456fca0db Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 27 Jan 2025 13:30:08 +0000 Subject: [PATCH 07/11] Prefer environment variables to CLI flags Signed-off-by: Evans Mungai --- cmd/helm/profiling.go | 61 +++++++---------------------- cmd/helm/profiling_test.go | 79 -------------------------------------- cmd/helm/root.go | 3 -- 3 files changed, 14 insertions(+), 129 deletions(-) delete mode 100644 cmd/helm/profiling_test.go diff --git a/cmd/helm/profiling.go b/cmd/helm/profiling.go index e231a3e0a..e48441512 100644 --- a/cmd/helm/profiling.go +++ b/cmd/helm/profiling.go @@ -19,30 +19,29 @@ package main import ( "fmt" "os" - "path" - "path/filepath" "runtime" "runtime/pprof" "strings" - - "github.com/spf13/cobra" ) var ( cpuProfileFile *os.File - pprofPaths map[string]string + cpuProfilePath string + memProfilePath string ) func init() { - pprofPaths = parsePProfPaths(os.Getenv("HELM_PPROF")) + cpuProfilePath = os.Getenv("HELM_PPROF_CPU_PROFILE") + memProfilePath = os.Getenv("HELM_PPROF_MEM_PROFILE") } -// startProfiling starts profiling CPU usage +// startProfiling starts profiling CPU usage if HELM_PPROF_CPU_PROFILE is set +// to a file path. It returns an error if the file could not be created or +// CPU profiling could not be started. func startProfiling() error { - cpuprofile, ok := pprofPaths["cpu"] - if ok && cpuprofile != "" { + if cpuProfilePath != "" { var err error - cpuProfileFile, err = os.Create(cpuprofile) + cpuProfileFile, err = os.Create(cpuProfilePath) if err != nil { return fmt.Errorf("could not create CPU profile: %w", err) } @@ -55,8 +54,9 @@ func startProfiling() error { return nil } -// stopProfiling stops profiling CPU and memory usage and writes the results to -// the files specified by HELM_PPROF=cpu=/path/to/cpu.prof,mem=/path/to/mem.prof +// stopProfiling stops profiling CPU and memory usage. +// It writes memory profile to the file path specified in HELM_PPROF_MEM_PROFILE +// environment variable. func stopProfiling() error { errs := []string{} @@ -70,9 +70,8 @@ func stopProfiling() error { cpuProfileFile = nil } - memprofile, ok := pprofPaths["mem"] - if ok && memprofile != "" { - f, err := os.Create(memprofile) + if memProfilePath != "" { + f, err := os.Create(memProfilePath) if err != nil { errs = append(errs, err.Error()) } @@ -90,35 +89,3 @@ func stopProfiling() error { return nil } - -// addProfilingFlags adds the --cpuprofile and --memprofile flags to the given command. -func addProfilingFlags(cmd *cobra.Command) { - // Persistent flags to make available to subcommands - cmd.PersistentFlags().String("cpuprofile", "", "File path to write cpu profiling data") - cmd.PersistentFlags().String("memprofile", "", "File path to write memory profiling data") -} - -func parsePProfPaths(env string) map[string]string { - // Initial empty paths - m := map[string]string{} - for _, pprofs := range strings.Split(env, ",") { - // Is of the format mem=/path/to/memprof - tuple := strings.Split(pprofs, "=") - if len(tuple) != 2 { - continue - } - if tuple[0] != "cpu" && tuple[0] != "mem" { - continue - } - - s, err := filepath.Abs(path.Clean(tuple[1])) - if err != nil { - continue - } - if !strings.HasSuffix(s, string(filepath.Separator)) { - // Ensure its not a directory - m[tuple[0]] = s - } - } - return m -} diff --git a/cmd/helm/profiling_test.go b/cmd/helm/profiling_test.go deleted file mode 100644 index 65928edbb..000000000 --- a/cmd/helm/profiling_test.go +++ /dev/null @@ -1,79 +0,0 @@ -/* -Copyright The Helm Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package main - -import ( - "os" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func Test_parsePProfPaths(t *testing.T) { - cwd, err := os.Getwd() - require.NoError(t, err) - - tests := []struct { - name string - env string - want map[string]string - }{ - { - name: "no env", - env: "", - want: map[string]string{}, - }, - { - name: "single path", - env: "cpu=cpu.pprof", - want: map[string]string{ - "cpu": cwd + "/cpu.pprof", - }, - }, - { - name: "mem and cpu paths", - env: "cpu=cpu.pprof,mem=mem.pprof", - want: map[string]string{ - "cpu": cwd + "/cpu.pprof", - "mem": cwd + "/mem.pprof", - }, - }, - { - name: "extra commas", - env: "cpu=cpu.pprof,mem=mem.pprof,", - want: map[string]string{ - "cpu": cwd + "/cpu.pprof", - "mem": cwd + "/mem.pprof", - }, - }, - { - name: "unknown keys", - env: "cpu=cpu.pprof,mem=mem.pprof,foo=bar", - want: map[string]string{ - "cpu": cwd + "/cpu.pprof", - "mem": cwd + "/mem.pprof", - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := parsePProfPaths(tt.env) - assert.Equalf(t, tt.want, got, "parsePProfPaths() = %v, want %v", got, tt.want) - }) - } -} diff --git a/cmd/helm/root.go b/cmd/helm/root.go index edc6376ae..f8ed82a60 100644 --- a/cmd/helm/root.go +++ b/cmd/helm/root.go @@ -216,9 +216,6 @@ func newRootCmd(actionConfig *action.Configuration, out io.Writer, args []string // Check for expired repositories checkForExpiredRepos(settings.RepositoryConfig) - // CPU and memory profiling flags that are available to all commands - addProfilingFlags(cmd) - return cmd, nil } From 58748b06c79f37cb79a816324ef1ab878e303a5b Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 27 Jan 2025 13:37:28 +0000 Subject: [PATCH 08/11] Update CONTRIBUTING guide Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0a90054d9..bc225d218 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,14 +284,16 @@ Small, ad-hoc changes/PRs to Helm which introduce user facing changes, which wou ### Profiling PRs -If your contribution requires profiling to check memory and/or CPU usage, you can set `HELM_PPROF=cpu=/path/to/cpu.prof,mem=/path/to/mem.prof` environment variable to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. +If your contribution requires profiling to check memory and/or CPU usage, you can set `HELM_PPROF_CPU_PROFILE=/path/to/cpu.prof HELM_PPROF_MEM_PROFILE=/path/to/mem.prof helm show all bitnami/nginx` environment variable to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. Example analysing collected profiling data ``` -HELM_PPROF=cpu=/path/to/cpu.prof,mem=/path/to/mem.prof helm show all bitnami/nginx +HELM_PPROF_CPU_PROFILE=cpu.prof HELM_PPROF_MEM_PROFILE=mem.prof helm show all bitnami/nginx # Visualize graphs. You need to have installed graphviz package in your system go tool pprof -http=":8000" cpu.prof + +go tool pprof -http=":8001" mem.prof ``` ## The Triager From 165654426d9edeaadc5352e54e2ac4c72f408c1d Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 17 Feb 2025 10:27:11 +0000 Subject: [PATCH 09/11] chore: update profiling doc in CONTRIBUTING.md Signed-off-by: Evans Mungai --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bc225d218..8ab93403d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -284,7 +284,7 @@ Small, ad-hoc changes/PRs to Helm which introduce user facing changes, which wou ### Profiling PRs -If your contribution requires profiling to check memory and/or CPU usage, you can set `HELM_PPROF_CPU_PROFILE=/path/to/cpu.prof HELM_PPROF_MEM_PROFILE=/path/to/mem.prof helm show all bitnami/nginx` environment variable to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. +If your contribution requires profiling to check memory and/or CPU usage, you can set `HELM_PPROF_CPU_PROFILE=/path/to/cpu.prof` and/or `HELM_PPROF_MEM_PROFILE=/path/to/mem.prof` environment variables to collect runtime profiling data for analysis. You can use Golang's [pprof](https://github.com/google/pprof/blob/main/doc/README.md) tool to inspect the results. Example analysing collected profiling data ``` From fdf448497137d8c9358d5ccc481e535cfbc6523b Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 17 Feb 2025 18:31:17 +0000 Subject: [PATCH 10/11] Update cmd/helm/profiling.go Co-authored-by: George Jenkins Signed-off-by: Evans Mungai --- cmd/helm/profiling.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/helm/profiling.go b/cmd/helm/profiling.go index e48441512..9bcdb850e 100644 --- a/cmd/helm/profiling.go +++ b/cmd/helm/profiling.go @@ -83,8 +83,8 @@ func stopProfiling() error { } } - if len(errs) > 0 { - return fmt.Errorf("errors while stopping profiling: [%s]", strings.Join(errs, ", ")) + if err := errors.Join(errs...); err != nil { + return fmt.Errorf("error(s) while stopping profiling: %w", err) } return nil From 62576db2fcabd40bb9801f81a0de9e8fcc36d154 Mon Sep 17 00:00:00 2001 From: Evans Mungai Date: Mon, 17 Feb 2025 18:36:04 +0000 Subject: [PATCH 11/11] chore: use []error instead of []string Signed-off-by: Evans Mungai --- cmd/helm/profiling.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cmd/helm/profiling.go b/cmd/helm/profiling.go index 9bcdb850e..950ad15da 100644 --- a/cmd/helm/profiling.go +++ b/cmd/helm/profiling.go @@ -17,11 +17,11 @@ limitations under the License. package main import ( + "errors" "fmt" "os" "runtime" "runtime/pprof" - "strings" ) var ( @@ -58,14 +58,14 @@ func startProfiling() error { // It writes memory profile to the file path specified in HELM_PPROF_MEM_PROFILE // environment variable. func stopProfiling() error { - errs := []string{} + errs := []error{} // Stop CPU profiling if it was started if cpuProfileFile != nil { pprof.StopCPUProfile() err := cpuProfileFile.Close() if err != nil { - errs = append(errs, err.Error()) + errs = append(errs, err) } cpuProfileFile = nil } @@ -73,13 +73,13 @@ func stopProfiling() error { if memProfilePath != "" { f, err := os.Create(memProfilePath) if err != nil { - errs = append(errs, err.Error()) + errs = append(errs, err) } defer f.Close() runtime.GC() // get up-to-date statistics if err := pprof.WriteHeapProfile(f); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, err) } }