refactor: service lifecycle into optional Init Run Shutdown #2021

sthaha · 2025-04-19T05:48:56Z

This commit modifies the start up behavior. It refactors the service lifecycle management, replacing the Start and Stop methods with

Init: used to initiliaze a service and can assume it isn't called from go routines and must be called before Run.
Run: Runs the service and blocks until context is cancelled or an error is encountered
Shutdown: method is the cleanup and indicates that you can't call Run after Shutdown.

This change aims to separate initialization which is run serially and allows services to initialize without worrying about thread-safety. I.E. Its okay for an Init() to call another Init().

Thus, this change separates initialization, runtime, and cleanup phases for better clarity and control. All Methods are optional and main calls the methods if they are defined.

Tests are updated keep up with the change.

codecov · 2025-04-19T05:50:20Z

Codecov Report

Attention: Patch coverage is 76.66667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 90.59%. Comparing base (1341ea9) to head (6c8bfd5).
Report is 4 commits behind head on reboot.

Files with missing lines	Patch %	Lines
internal/monitor/monitor.go	60.00%	3 Missing and 1 partial ⚠️
internal/device/fake_cpu_power_meter.go	50.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           reboot    #2021      +/-   ##
==========================================
- Coverage   91.50%   90.59%   -0.91%     
==========================================
  Files          17       17              
  Lines        1000      999       -1     
==========================================
- Hits          915      905      -10     
- Misses         66       75       +9     
  Partials       19       19

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This commit modifies the start up behavior. It refactors the service lifecycle management, replacing the Start and Stop methods with * Init: used to initiliaze a service and can assume it isn't called from go routines * Run: Runs the service and blocks until context is cancelled or an error is encountered * `Shutdown`: methods is the cleanup and indicates that you can't call `Run` after `Shutdown`. Instead of mandating a service to have all Init, Run, Shutdown, services can now have any of these methods. Main calls Init if it exits, followed by Run (if it exits) and then Shutdown This change also aims to separate initialization which is run serially and allows services to initialize without worrying about thread-safety. I.E. Its okay for an Init() to call another Init(). Thus this change separates initialization, runtime, and cleanup phases for better clarity and control. Tests are updated keep up with the change. Signed-off-by: Sunil Thaha <[email protected]>

vimalk78 · 2025-04-23T05:30:50Z

cmd/kepler/main.go

+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()


pls check if we need a context for initialization. no Init is using it, and it is getting cancelled immediately after all services initialized

I agree, we can add context to Init if there is a need.
Can I address that in a separate patch so that the rest of the PRs can be merged?

vimalk78 · 2025-04-23T05:42:41Z

cmd/kepler/main.go

+func runServices(logger *slog.Logger, services []service.Service) error {
+	logger.Info("Running all services")
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()


no cancel() function call in func(error) of g.Add ? as was case before this refactor?

cancel gets called in waitForInterrupt and is also deferred.

vimalk78 · 2025-04-23T06:07:28Z

cmd/kepler/main.go

+			},
+		)
+	}
+	g.Add(waitForInterrupt(ctx, cancel, logger, os.Interrupt))


add handling for SIGTERM . this is used in kubernetes to gracefully shutdown a pod.

vimalk78 · 2025-04-23T06:16:08Z

internal/device/fake_cpu_power_meter_test.go

+	err := <-errCh
+	duration := time.Since(start)
+	assert.NoError(t, err, "Run() should not return an error")
+	assert.GreaterOrEqual(t, duration, 50*time.Millisecond, "Run() should run until the context is cancelled")

 	err = meter.Stop()


Stop can be removed since fake do need to implement service.Shutdown

Fake implements CPUPowerMeter and isn't a service so to satisfy that interface, we need a Stop method.

vimalk78 · 2025-04-23T06:22:17Z

internal/device/power_meter.go

+	Init(ctx context.Context) error
+
+	// Run() power meter for reading energy or power
+	Run(ctx context.Context) error

 	// Stop() stops the power meter and releases any resources held
 	Stop() error


change to Shutdown?

The idea behind shutdown was to indicate that you can't start after shutdown where as for devices, I am not sure (at this point in time) if those should be treated as a Service. Any thoughts?

vimalk78 · 2025-04-23T06:23:57Z

internal/device/rapl_sysfs_power_meter.go

+func (r *raplPowerMeter) Run(ctx context.Context) error {
+	<-ctx.Done()
+	return nil
+}
+


since real and fake power meter do not actually Run and Stop/Shutdown , no need to implement Runner and Shutdown. the runServices checks for interface implementation anyway.

PowerMeters aren't services. It implements PowerMeter so these are specific to that interface.

vimalk78 · 2025-04-23T06:29:15Z

internal/exporter/prometheus/prometheus.go

prometheus service does not implement Shutdown. it can unregister the prometheus handler from server

perometheus service does not implement Runner 👍🏽

+1 to unregister idea but with shutdown being called, there is no point in unregister since all services are being shutdown -right?

vimalk78 · 2025-04-23T06:31:51Z

internal/service/service.go

+}
+
+// Shutdown is the interface that all services must implement that are to be shutdown / cleaned up
+type Shutdown interface {


change name to Shutdowner

:) Happy to do that. Separate PR?

sthaha · 2025-04-23T07:33:45Z

internal/device/power_meter.go

+	// Init() initializes the power meter and makes it ready for use. This method
+	// is not required to be thread-safe
+	Init(ctx context.Context) error
+
+	// Run() power meter for reading energy or power
+	Run(ctx context.Context) error


Lets make use of service.Runner and service.Initializer to replace the Init in CPUPowerMeter.
Power Meter will only return Zones()

vimalk78

/lgtm
with few changes to be done in a followup PR.

This commit addresses comments on sustainable-computing-io#2021 Simplify service lifecycle management by: - Removing context.Context from Initializer.Init() method - Renaming Shutdown to Shutdowner for clarity - Removing Init, Run, and Stop methods from powerMeter interface - Removing Shutdown from PowerMonitor and Stop from power meters - Refactoring createPowerMonitor into createServices Enhance signal handling by adding syscall.SIGTERM support in waitForInterrupt. Add debug logging for skipped service initialization and shutdown. Update tests to reflect simplified lifecycle management. Signed-off-by: Sunil Thaha <[email protected]>

Changes include: * Introduce service.NewSignalHandler for SIGINT and SIGTERM, replacing custom signal handling. * Replace runServices and initServices with service.Run and service.Init for standardized lifecycle management. * Simplify powerMeter interface by removing Init, Run, and Stop methods. * Rename Shutdown to Shutdowner and remove context from Initializer.Init. * Inline createPowerMonitor into createServices and reorder services. * Add debug logging for skipped initializations and successful shutdowns. * Update tests to reflect simplified lifecycle and context removal. * Addresses comments on: sustainable-computing-io#2021 Signed-off-by: Sunil Thaha <[email protected]>

sthaha added the refactor Code refactoring without changing functionality label Apr 19, 2025

sthaha requested review from vimalk78, vprashar2929 and KaiyiLiu1234 April 19, 2025 05:49

sthaha force-pushed the refactor-service-init branch from 87e9295 to bb9452f Compare April 23, 2025 02:22

sthaha force-pushed the refactor-service-init branch from bb9452f to 6c8bfd5 Compare April 23, 2025 02:56

sthaha changed the title ~~refactor: allow services to be initialized serially~~ refactor: service lifecycle into optional Init Run Shutdown Apr 23, 2025

vimalk78 reviewed Apr 23, 2025

View reviewed changes

sthaha commented Apr 23, 2025

View reviewed changes

vimalk78 approved these changes Apr 23, 2025

View reviewed changes

vimalk78 merged commit 2ad09ba into sustainable-computing-io:reboot Apr 23, 2025
6 of 8 checks passed

sthaha mentioned this pull request Apr 23, 2025

refactor(service): simplify service lifecycle #2026

Merged

		ctx, cancel := context.WithCancel(context.Background())
		defer cancel()

refactor: service lifecycle into optional Init Run Shutdown #2021

refactor: service lifecycle into optional Init Run Shutdown #2021

Uh oh!

Conversation

sthaha commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

vimalk78 Apr 23, 2025 • 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!

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!

sthaha Apr 23, 2025 • 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!

vimalk78 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sthaha commented Apr 19, 2025 •

edited

Loading

codecov bot commented Apr 19, 2025 •

edited

Loading

vimalk78 Apr 23, 2025 •

edited

Loading

sthaha Apr 23, 2025 •

edited

Loading