Conversation
This commit refactores the probe initialization to use a Hive Job instead of a plain lifecycle start hook. This way we can also get rid of the raw Go routine to execute the check if every probe successfully executed at least once before exposing the status. Note: the kvstore "shutdown check" is still part of its own lifecycle stop hook. Probably better to eventually move this to the kvstore module. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the status probe lifecycle management to use Hive's job framework instead of a standalone goroutine. The change improves integration with the application's lifecycle management system by leveraging Hive's job abstraction for better control and observability of the probe execution process.
Changes:
- Migrated probe initialization from a lifecycle hook with a detached goroutine to a Hive
OneShotjob - Removed the
startStatusCollectormethod and integrated its logic directly into the job - Updated health reporting to use the job's health parameter instead of creating a new health scope
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/status/status_collector.go | Removed the startStatusCollector method containing the original probe initialization logic |
| pkg/status/cell.go | Replaced lifecycle-based probe initialization with a Hive OneShot job, added job.Group dependency, and moved probe cleanup to the job's defer statement |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| waitCtx, cancelWait := context.WithTimeout(ctx, params.Config.StatusCollectorProbeCheckTimeout) | ||
| defer cancelWait() | ||
|
|
||
| // Report health whether all probes have been executed at least once. |
There was a problem hiding this comment.
Corrected 'whether' to 'when' for grammatical accuracy. The comment should read 'Report health when all probes have been executed at least once'.
| // Report health whether all probes have been executed at least once. | |
| // Report health when all probes have been executed at least once. |
| statusCollector: newCollector(params.Logger, params.Config), | ||
| } | ||
|
|
||
| params.JobGroup.Add(job.OneShot("probes", func(ctx context.Context, health cell.Health) error { |
There was a problem hiding this comment.
The health parameter is declared but never used in the job function. Either utilize it for health reporting (replacing the removed health scope functionality from the original code) or remove it from the function signature if health reporting is no longer needed.
2 participants
PR_044