[healthcheck] optimize: reduce unnecessary function calls - #5048
[healthcheck] optimize: reduce unnecessary function calls#5048ningmingxiao wants to merge 1 commit into
Conversation
32becea to
4058db1
Compare
68d6a1e to
897c2b2
Compare
ningmingxiao
changed the title
There was a problem hiding this comment.
Pull request overview
This PR targets the startup-time regression reported in #5047 by reducing repeated container label lookups during healthcheck setup and avoiding unnecessary task.Wait() work in detach mode.
Changes:
- Extend healthcheck timer APIs (
CreateTimer/StartTimer) to accept an already-fetched labels map, and update extraction logic to reuse it. - Update container start/run/unpause paths to pass cached labels and skip healthcheck setup when no healthcheck label is present.
- Adjust
nerdctl runflow to avoid callingtask.Wait()in detach mode (but currently introduces a wait/start ordering risk for non-detach).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/healthcheck/healthcheck_manager_linux.go | Reuses caller-provided labels to avoid repeated container.Labels() calls during healthcheck timer setup. |
| pkg/healthcheck/healthcheck_manager_windows.go | Updates stub signatures to match new healthcheck API. |
| pkg/healthcheck/healthcheck_manager_freebsd.go | Updates stub signatures to match new healthcheck API. |
| pkg/healthcheck/healthcheck_manager_darwin.go | Updates stub signatures to match new healthcheck API. |
| pkg/containerutil/containerutil.go | Passes cached labels into healthcheck setup and skips healthcheck work when not configured. |
| cmd/nerdctl/container/container_run.go | Skips task.Wait() for detach runs and passes cached labels into healthcheck setup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hcStr, ok := lab[labels.HealthCheck]; ok && hcStr != "" { | ||
| // If container has health checks configured, create and start systemd timer/service files. | ||
| if err := healthcheck.CreateTimer(ctx, container, cfg, nerdctlCmd, nerdctlArgs, lab); err != nil { | ||
| return fmt.Errorf("failed to create healthcheck timer: %w", err) | ||
| } |
| statusC, err := task.Wait(ctx) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
you can see
https://github.com/containerd/containerd/blob/v2.3.3/cmd/ctr/commands/run/run.go#L249
@haytok
here we don't need to wait container exited
There was a problem hiding this comment.
Maybe we should move back the code before task.Start
There was a problem hiding this comment.
it's not necessary to move back, it's not a bug if user use -d we don't need to wait it. @ChengyuZhu6
There was a problem hiding this comment.
you can see
https://github.com/containerd/containerd/blob/v2.3.3/cmd/ctr/commands/run/run.go#L249
In ctr, task.Wait(ctx) is called before task.Start(ctx) when detach is false.
In this PR, task.Wait(ctx) is called after task.Start(ctx), which differs from ctr.
So, as ChengyuZhu6 mentioned, we should move back the code before task.Start. Wait still needs to be called at least when !createOpt.Detach.
AkihiroSuda
changed the title
Signed-off-by: ningmingxiao <ning.mingxiao@zte.com.cn>
reduce about 100ms
ref:#5047