From 9d4561f79c7fcc2ef73fb8b170be1bbb42b8a6c6 Mon Sep 17 00:00:00 2001 From: ionutboangiu Date: Sun, 21 Jul 2024 02:56:08 +0300 Subject: [PATCH] Revise CPU Profiling implementation cgr-engine.go: - use filepath.Join instead of path.Join - handle *CoreService.StopCPUProfiling error inside deferred function - same with the error from *os.File.Close() cores/core.go: - StartCPUProfile now returns an io.Closer (as opposed to an io.WriteCloser) - handle pprof.StartCPUProfile error and ensure file is closed before returning - log file close error as a warning if it occurs - return missing mandatory error with correct path field name ('DirPath') - no need to check if fileCPU is nil for profiling status - pprof.StartCPUProfiling will return an error if profiling is already started - os.File.Close() will return ErrClosed if profiling is already stopped - differentiate between calling StopCPUProfiling when profiling hasn't started and when it was already stopped by returning appropriate errors - improved comments and error messages --- cmd/cgr-engine/cgr-engine.go | 22 +++++++----- cores/core.go | 65 +++++++++++++++++++++--------------- 2 files changed, 51 insertions(+), 36 deletions(-) diff --git a/cmd/cgr-engine/cgr-engine.go b/cmd/cgr-engine/cgr-engine.go index d81bf7edd..a9f9427c8 100644 --- a/cmd/cgr-engine/cgr-engine.go +++ b/cmd/cgr-engine/cgr-engine.go @@ -26,6 +26,7 @@ import ( "os" "os/signal" "path" + "path/filepath" "runtime" "runtime/pprof" "strconv" @@ -368,21 +369,24 @@ func main() { }() } - var cpuProfileFile io.Closer + var cpuProf io.Closer if *cpuProfDir != utils.EmptyString { - cpuPath := path.Join(*cpuProfDir, utils.CpuPathCgr) - cpuProfileFile, err = cores.StartCPUProfiling(cpuPath) + cpuPath := filepath.Join(*cpuProfDir, utils.CpuPathCgr) + cpuProf, err = cores.StartCPUProfiling(cpuPath) if err != nil { - log.Fatalf("<%s> error received: <%s>, exiting!", utils.InitS, err.Error()) + log.Fatal(err) } defer func() { if cS != nil { - cS.StopCPUProfiling() + // Use CoreService's StopCPUProfiling method if it has been initialized. + if err := cS.StopCPUProfiling(); err != nil { + log.Print(err) + } return } - if cpuProfileFile != nil { - pprof.StopCPUProfile() - cpuProfileFile.Close() + pprof.StopCPUProfile() + if err := cpuProf.Close(); err != nil { + log.Printf("could not close file %q: %v", cpuProf.(*os.File).Name(), err) } }() } @@ -576,7 +580,7 @@ func main() { // init CoreSv1 - coreS := services.NewCoreService(cfg, caps, server, internalCoreSv1Chan, anz, cpuProfileFile, memPrfDirForCores, shdWg, stopMemProf, shdChan, srvDep) + coreS := services.NewCoreService(cfg, caps, server, internalCoreSv1Chan, anz, cpuProf, *memProfDir, shdWg, stopMemProf, shdChan, srvDep) shdWg.Add(1) if err := coreS.Start(); err != nil { log.Fatalf("<%s> error received: <%s>, exiting!", utils.InitS, err.Error()) diff --git a/cores/core.go b/cores/core.go index 43c396208..de61d076e 100644 --- a/cores/core.go +++ b/cores/core.go @@ -60,9 +60,11 @@ type CoreService struct { stopMemPrf chan struct{} shdChan *utils.SyncedChan fileMEM string - fileCPU io.Closer - fileMx sync.Mutex - caps *engine.Caps + + fileMux sync.Mutex + fileCPU io.Closer + + caps *engine.Caps } // Shutdown is called to shutdown the service @@ -82,13 +84,21 @@ func (cS *CoreService) StopChanMemProf() { } } -func StartCPUProfiling(path string) (file io.WriteCloser, err error) { - file, err = os.Create(path) +// StartCPUProfiling creates a file and passes it to pprof.StartCPUProfile. It returns the file +// as an io.Closer to be able to close it later when stopping the CPU profiling. +func StartCPUProfiling(path string) (io.Closer, error) { + f, err := os.Create(path) if err != nil { return nil, fmt.Errorf("could not create CPU profile: %v", err) } - err = pprof.StartCPUProfile(file) - return + if err := pprof.StartCPUProfile(f); err != nil { + if err := f.Close(); err != nil { + utils.Logger.Warning(fmt.Sprintf( + "<%s> could not close file %q: %v", utils.CoreS, f.Name(), err)) + } + return nil, fmt.Errorf("could not start CPU profile: %v", err) + } + return f, nil } func MemProfFile(memProfPath string) bool { @@ -153,31 +163,32 @@ func (cS *CoreService) V1Status(_ *context.Context, _ *utils.TenantWithAPIOpts, return } -// StartCPUProfiling is used to start CPUProfiling in the given path -func (cS *CoreService) StartCPUProfiling(argPath string) (err error) { - cS.fileMx.Lock() - defer cS.fileMx.Unlock() - if cS.fileCPU != nil { - return fmt.Errorf("CPU profiling already started") +// StartCPUProfiling starts CPU profiling and saves the profile to the specified path. +func (cS *CoreService) StartCPUProfiling(path string) (err error) { + if path == utils.EmptyString { + return utils.NewErrMandatoryIeMissing("DirPath") } - if argPath == utils.EmptyString { - return utils.NewErrMandatoryIeMissing("Path") - } - cS.fileCPU, err = StartCPUProfiling(argPath) + cS.fileMux.Lock() + defer cS.fileMux.Unlock() + cS.fileCPU, err = StartCPUProfiling(path) return } -// StopCPUProfiling is used to stop CPUProfiling in the given path -func (cS *CoreService) StopCPUProfiling() (err error) { - cS.fileMx.Lock() - defer cS.fileMx.Unlock() - if cS.fileCPU != nil { - pprof.StopCPUProfile() - err = cS.fileCPU.Close() - cS.fileCPU = nil - return +// StopCPUProfiling stops CPU profiling and closes the profile file. +func (cS *CoreService) StopCPUProfiling() error { + cS.fileMux.Lock() + defer cS.fileMux.Unlock() + pprof.StopCPUProfile() + if cS.fileCPU == nil { + return errors.New("CPU profiling has not been started") } - return fmt.Errorf(" cannot stop because CPUProfiling is not active") + if err := cS.fileCPU.Close(); err != nil { + if errors.Is(err, os.ErrClosed) { + return errors.New("CPU profiling has already been stopped") + } + return fmt.Errorf("could not close profile file: %v", err) + } + return nil } // StartMemoryProfiling is used to start MemoryProfiling in the given path