Skip to content

fix(manager): quarantine damaged daemon records instead of aborting recovery - #780

Open
iaroslav-reflection wants to merge 2 commits into
containerd:mainfrom
iaroslav-reflection:fix/resilient-daemon-recovery
Open

fix(manager): quarantine damaged daemon records instead of aborting recovery#780
iaroslav-reflection wants to merge 2 commits into
containerd:mainfrom
iaroslav-reflection:fix/resilient-daemon-recovery

Conversation

@iaroslav-reflection

@iaroslav-reflection iaroslav-reflection commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

A persisted fusedev daemon record whose config.json cannot be reloaded made recoverDaemons return an error, aborting the whole WalkDaemons pass and failing NewSnapshotter. Since daemon records are never pruned, one damaged record crash-loops the snapshotter on every restart until the boltdb is repaired by hand.

Such records are easy to acquire: createDaemon persists the record before the configuration is dumped to disk, so a crash or a full disk (the likeliest cause, since the blob cache shares the volume) in that window leaves a record without a config file; a power loss can truncate an existing config the same way.

flowchart LR
    A["record persisted,<br/>config.json missing"] --> B["restart → recovery walk"]
    B --> C["config reload fails →<br/>whole walk aborts"]
    C --> D["NewSnapshotter fails →<br/>snapshotter exits"]
    D --> B
    C --> E["💥 crash loop until manual<br/>boltdb surgery"]
Loading

Make recovery fault-isolated per record: skip records whose configuration cannot be reloaded — log and continue recovering everything else. The record stays in the store, so restoring the config file lets a later restart re-adopt the daemon.

Persisting the record only after its configuration is on disk is a worthwhile follow-up hardening; this change makes recovery robust to such damage regardless of how it was produced.

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 26.33%. Comparing base (18e7e6a) to head (1252e2a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #780      +/-   ##
==========================================
+ Coverage   26.01%   26.33%   +0.32%     
==========================================
  Files         133      133              
  Lines       12711    12712       +1     
==========================================
+ Hits         3307     3348      +41     
+ Misses       8998     8951      -47     
- Partials      406      413       +7     
Files with missing lines Coverage Δ
pkg/manager/manager.go 21.73% <100.00%> (+21.73%) ⬆️

... and 1 file with indirect coverage changes

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

@iaroslav-reflection

Copy link
Copy Markdown
Contributor Author

@imeoer @Zephyrcf PTAL

@iaroslav-reflection

Copy link
Copy Markdown
Contributor Author

The failing run-e2e-for-cgroups jobs are pre-existing data races, fix proposed in #781.

Comment thread pkg/manager/manager.go Outdated
Comment on lines +343 to +346
log.L.Errorf("Quarantining daemon %s: failed to reload configuration %s, %s", d.ID(), d.ConfigFile(""), err)
quarantined = append(quarantined, d)
//nolint:nilerr
return nil

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.

Suggested change
log.L.Errorf("Quarantining daemon %s: failed to reload configuration %s, %s", d.ID(), d.ConfigFile(""), err)
quarantined = append(quarantined, d)
//nolint:nilerr
return nil
//nolint:nilerr
return nil

I think it's best to skip this part; the existence of these bad records won't actually cause any problems. Another scenario is accidentally deleting these configuration file directories, which means that the existing daemon will be deleted during the subsequent recovery operation. In reality, I only need to restore the corresponding configuration files. Therefore, skipping this part instead of deleting is a conservative and effective approach.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, changed to skip without deleting. The daemon is still dropped from the in-memory cache since it's registered before the config load.

…ecovery

recoverDaemons returned an error when a persisted fusedev daemon record's
configuration file could not be reloaded, which aborts the whole
WalkDaemons pass, fails Recover and therefore NewSnapshotter: the
snapshotter exits on startup. Since nothing ever prunes daemon records, a
single damaged record crash-loops the snapshotter on every subsequent
restart until the database is repaired by hand.

Such records are easy to acquire: the daemon record is persisted by
createDaemon before the configuration is dumped to disk, so a crash or a
full disk (the likeliest cause, since the blob cache shares the volume)
in that window leaves a record without a config.json. A power loss can
truncate an existing config the same way.

Recovery must be fault-isolated per record: quarantine records whose
configuration cannot be reloaded — log, skip, and prune them after the
walk — and continue recovering everything else. A daemon without its
configuration cannot be recovered or managed anyway; dropping the record
keeps the next restart from tripping over it, and orphan process cleanup
is already handled by the regular vestige clearing.

Persisting the record only after its configuration is on disk is a
worthwhile follow-up hardening; this change makes recovery robust to any
such damage regardless of how it was produced.

Signed-off-by: Iaroslav Geraskin <iaroslav@reflection.ai>
A missing config file does not prove the daemon behind the record is
dead. Keep the record so a later restart can re-adopt the daemon once
the config is restored; only drop the in-memory registration.

Signed-off-by: Iaroslav Geraskin <iaroslav@reflection.ai>
@iaroslav-reflection
iaroslav-reflection force-pushed the fix/resilient-daemon-recovery branch from c5f5f7b to 1252e2a Compare July 21, 2026 18:41

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

@iaroslav-reflection

Copy link
Copy Markdown
Contributor Author

@imeoer approved by @Zephyrcf, good to merge?

@iaroslav-reflection

Copy link
Copy Markdown
Contributor Author

@imeoer ping — approved since July, good to merge?

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.

2 participants