Skip to content

fix: serialize daemon restart and termination - #774

Open
qinfustu wants to merge 1 commit into
containerd:mainfrom
qinfustu:fix/daemon-restart-terminate-serialize
Open

fix: serialize daemon restart and termination#774
qinfustu wants to merge 1 commit into
containerd:mainfrom
qinfustu:fix/daemon-restart-terminate-serialize

Conversation

@qinfustu

Copy link
Copy Markdown

Overview

When a nydusd daemon death event is received, the manager now terminates the
old daemon process before starting a new one, and serializes recovery so that
only one recovery routine runs per daemon at a time. This prevents zombie/
orphan nydusd accumulation and avoids duplicate concurrent restarts.

Related Issues

Related #771

Change Details

Previously, on a daemon death event doDaemonRestart / doDaemonFailover only
called d.Wait() and then started a new nydusd. When the death event was a
false positive (the API socket closed while the process was hung), the old
process kept running while a new one was spawned, and duplicate death events
could trigger multiple concurrent recoveries. Over time this leaked orphan
nydusd processes that kept hitting the registry.

This PR makes recovery robust:

  • Terminate the old process before restart/failover (terminateDaemonProcess):
    • Grabs the os.Process handle first (uses a pidfd on Linux) to pin the
      handle to the specific process before any identity check, eliminating a
      PID-reuse TOCTOU.
    • Verifies process identity via /proc/<pid>/cmdline (matching --apisock)
      and classifies it as alive / zombie / gone / reused.
    • For a live process: sends SIGTERM, waits with a timeout, then escalates
      to SIGKILL; for a zombie: reaps it via Wait() with a timeout; for
      gone/reused: does nothing.
    • Returns an error on failure so recovery aborts instead of leaving an
      unmonitored live process behind. Unsubscribe now happens only after
      successful termination.
  • Serialize recovery per daemon (recoveryInFlight sync.Map on Manager):
    • beginDaemonRecovery uses LoadOrStore to dedup concurrent death events;
      the flag is cleared when the recovery goroutine finishes.
  • Guard against stale death events: the liveness monitor now carries the
    subscribed process PID in deathEvent, and handleDaemonDeathEvent ignores
    events whose PID no longer matches the daemon's current PID (generation
    check), preventing a delayed event for an old process from killing its
    replacement.
  • Removed the // TODO: ratelimit and reads d.Pid() under the daemon lock
    (daemonProcessID) to avoid a data race.

Test Results

Added unit tests in pkg/manager/daemon_event_test.go and updated
monitor_test.go:

  • TestBeginDaemonRecoverySerializesConcurrentCalls – dedup guard.
  • TestInspectDaemonProcess – alive / zombie / gone / reused classification.
  • TestTerminateDaemonProcessEscalatesToKill – SIGTERM → SIGKILL escalation.
  • TestTerminateDaemonProcessReapsZombie – zombie reap path.
  • TestTerminateDaemonProcessReturnsSignalErrorWithoutWaiting – abort on
    signal failure.
  • TestHandleDaemonDeathEvent_* – removed-daemon skip, dedup skip, flag clear,
    stale-PID skip.
  • TestLivenessMonitor – verifies the PID is propagated in the death event.

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.

@qinfustu
qinfustu force-pushed the fix/daemon-restart-terminate-serialize branch from 29b3632 to 68d90b3 Compare July 15, 2026 08:57
@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 74.50980% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 26.28%. Comparing base (d16caad) to head (05577da).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
pkg/manager/daemon_event.go 72.91% 20 Missing and 6 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #774      +/-   ##
==========================================
+ Coverage   25.42%   26.28%   +0.86%     
==========================================
  Files         133      134       +1     
  Lines       12639    12917     +278     
==========================================
+ Hits         3213     3395     +182     
- Misses       9036     9110      +74     
- Partials      390      412      +22     
Files with missing lines Coverage Δ
pkg/manager/manager.go 1.09% <ø> (+1.09%) ⬆️
pkg/manager/monitor.go 63.88% <100.00%> (+1.03%) ⬆️
pkg/manager/daemon_event.go 38.07% <72.91%> (+38.07%) ⬆️

... and 5 files with indirect coverage changes

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

@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.

Thanks for this pr. Here are a few questions:

Comment thread pkg/manager/daemon_event.go Outdated
Comment thread pkg/manager/daemon_event.go Outdated
Comment thread pkg/manager/daemon_event.go Outdated
Comment thread pkg/manager/daemon_event.go Outdated
@qinfustu

Copy link
Copy Markdown
Author

Thanks for this pr. Here are a few questions:

@Zephyrcf Thanks for the review. I’ve addressed all the comments and updated the code accordingly. Please take another look when convenient.

@Zephyrcf

Copy link
Copy Markdown
Contributor

@qinfustu plz squash the commits.

@qinfustu
qinfustu force-pushed the fix/daemon-restart-terminate-serialize branch from 51a5374 to 05577da Compare July 23, 2026 07:09

@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, thanks!

@qinfustu

Copy link
Copy Markdown
Author

LGTM, thanks!

@Zephyrcf Thanks! Security scan is still failing with GO-2026-5970. How should we handle it?

@Zephyrcf

Copy link
Copy Markdown
Contributor

LGTM, thanks!

@Zephyrcf Thanks! Security scan is still failing with GO-2026-5970. How should we handle it?

I submitted #788 to solve this problem.

fix: serialize daemon restart and termination
@qinfustu
qinfustu force-pushed the fix/daemon-restart-terminate-serialize branch 2 times, most recently from 2540dbd to 5579b1c Compare July 29, 2026 07:52
@imeoer

imeoer commented Aug 7, 2026

Copy link
Copy Markdown
Collaborator

This collides with #773: both add defaultDaemonTerminationTimeout to package manager, and both add their own process-termination helper. I tried merging them locally and the second one fails to build. Worth merging the two PRs into one.

On inspectDaemonProcess: identifying the process by grepping --apisock <path> out of /proc/<pid>/cmdline only works because BuildCommand currently emits the flag and its value as two separate argv entries. If nydusd ever accepts --apisock=<path>, the check silently reports daemonProcessReused and we skip termination. #789 is solving the same "is this still our process" question using the start time from /proc/<pid>/stat, which doesn't depend on the CLI shape. Can we settle on one approach across both PRs?

Also still open: @Zephyrcf asked for a squash.

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