Skip to content

fix: startdaemon reclaim on startup failure - #773

Open
qinfustu wants to merge 1 commit into
containerd:mainfrom
qinfustu:fix/startdaemon-reclaim-on-startup-failure
Open

fix: startdaemon reclaim on startup failure#773
qinfustu wants to merge 1 commit into
containerd:mainfrom
qinfustu:fix/startdaemon-reclaim-on-startup-failure

Conversation

@qinfustu

Copy link
Copy Markdown

Overview

Terminate nydusd processes that fail startup verification instead of leaving
them running as orphans that keep pulling from the registry.

Related Issues

Related #771

Change Details

When StartDaemon's background goroutine detects that a newly spawned nydusd
failed to come up (API socket not ready within ~10s, event subscription failed,
or daemon not RUNNING within ~2s), it previously just logged and returned,
leaving the process alive. Under registry pressure these timeouts are easily
hit, and upper-layer retries (Prepare, doDaemonRestart) spawn yet another
daemon — each round leaks one live, still-pulling orphan.
pkg/manager/daemon_adaptor.go

  • On any startup-verification failure, call the new terminateFailedDaemon
    helper which does SIGTERM → 5s timeout → SIGKILLWait (reap).
    It operates on the captured *os.Process handle (not pid) so it is safe
    against pid reuse and concurrent teardown.
  • Before killing, unsubscribe the daemon from the liveness monitor so our own
    kill is not misinterpreted as a death event that triggers another restart.
  • Two guards prevent mis-kills:
    • Failover daemons (d.Supervisor != nil): a failover-managed daemon
      legitimately stays in INIT/READY while the takeover flow runs
      (TakeOverStart). Killing it would wreck the takeover, so the helper
      skips it and leaves cleanup to the failover flow.
    • Already-retained shared daemons: if a shared daemon has already been
      retained via TryRetainSharedDaemon (e.g. spurious timeout race), killing
      it would drop the active shared daemon. A new IsSharedDaemonRetained
      callback lets the helper detect and skip this case.
      pkg/manager/manager.go
  • Added IsSharedDaemonRetained callback field to the Manager struct.
    pkg/filesystem/fs.go
  • initSharedDaemon now waits for DaemonStateRunning before calling
    TryRetainSharedDaemon, so a failed shared daemon is never retained with a
    dead process — and a truly-failed one is cleaned up by the helper above.
  • Wired IsSharedDaemonRetained callbacks for both fscache and fusedev managers.

Test Results

New unit tests in pkg/manager/daemon_adaptor_test.go:

  • TestTerminateFailedDaemon/nil_process_is_a_no-op
  • TestTerminateFailedDaemon/SIGTERM_stops_a_well-behaved_process — asserts
    the process is reaped and finishes well before the SIGKILL escalation timeout.
  • TestTerminateFailedDaemon/escalates_to_SIGKILL_when_SIGTERM_is_ignored
    spawns a process that traps SIGTERM, asserts it is still killed via SIGKILL.

Change Type

Please select the type of change your pull request relates to:

  • Bug Fix
  • Feature Addition
  • Documentation Update
  • Code Refactoring
  • Performance Improvement
  • Other (please describe)

Self-Checklist

Before submitting a pull request, please ensure you have completed the following:

  • I have run a code style check and addressed any warnings/errors.
  • I have added appropriate comments to my code (if applicable).
  • I have updated the documentation (if applicable).
  • I have written appropriate unit tests.

@codecov

codecov Bot commented Jul 14, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 22.91667% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.11%. Comparing base (d16caad) to head (fb2d098).
⚠️ Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
pkg/manager/daemon_adaptor.go 27.50% 26 Missing and 3 partials ⚠️
pkg/filesystem/fs.go 0.00% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #773      +/-   ##
==========================================
+ Coverage   25.42%   26.11%   +0.68%     
==========================================
  Files         133      134       +1     
  Lines       12639    12914     +275     
==========================================
+ Hits         3213     3372     +159     
- Misses       9036     9128      +92     
- Partials      390      414      +24     
Files with missing lines Coverage Δ
pkg/manager/manager.go 0.00% <ø> (ø)
pkg/filesystem/fs.go 6.87% <0.00%> (+6.87%) ⬆️
pkg/manager/daemon_adaptor.go 7.28% <27.50%> (+7.28%) ⬆️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@qinfustu
qinfustu force-pushed the fix/startdaemon-reclaim-on-startup-failure branch from 92cd72c to 6a75b2f Compare July 14, 2026 09:48
@qinfustu qinfustu changed the title Fix/startdaemon reclaim on startup failure fix: startdaemon reclaim on startup failure Jul 14, 2026

@Zephyrcf Zephyrcf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qinfustu
qinfustu force-pushed the fix/startdaemon-reclaim-on-startup-failure branch from 6a75b2f to fb2d098 Compare July 29, 2026 08:11
@imeoer

imeoer commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

Three things:

  • defaultDaemonTerminationTimeout is added here and in fix: serialize daemon restart and termination #774, same package, so the two can't both merge — I tried it locally and the build breaks on the duplicate declaration. Both PRs also grow their own SIGTERM→SIGKILL helper (terminateFailedDaemon vs terminateDaemonProcess) with opposite policies for failover daemons. Could this and fix: serialize daemon restart and termination #774 be folded into a single change?
  • Under recover_policy=failover, SupervisorSet is always created, so every daemon has d.Supervisor != nil and terminateFailedDaemon returns immediately. [Bug] nydusd process storm / orphan accumulation under registry pressure #771 stays unfixed for those users. That limitation should at least be spelled out in the PR description.
  • The new WaitUntilState(Running) in initSharedDaemon only retries 20 × 100ms, so 2s total, and the error propagates out of NewFileSystem. On a cold or busy node nydusd won't always reach RUNNING that fast and the snapshotter just refuses to start. Either give this wait its own longer budget or downgrade it to a warning.

Smaller ones: fscacheManager.IsSharedDaemonRetained = ... is a plain field write on an already-live Manager while StartDaemon goroutines read it — please pass it through manager.Opt instead. The failover comment is also duplicated verbatim in StartDaemon and terminateFailedDaemon. And the old // FIXME: Should clean the daemon record in DB if the nydusd fails starting was dropped, but the record still isn't cleaned up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants