Skip to content

fix: make live snapshot idempotent and crash-safe - #473

Open
pakkah wants to merge 1 commit into
containerd:mainfrom
pakkah:dev/snapshot
Open

pakkah wants to merge 1 commit into
containerd:mainfrom
pakkah:dev/snapshot

Conversation

@pakkah

@pakkah pakkah commented Oct 26, 2026 •

Copy link
Copy Markdown

What this PR does / why we need it:

  • Retries were not repeatable. The request file was renamed onto the config path and therefore
    consumed: a retry failed to parse, and a re-created identical request sealed the upper layer a second
    time instead of returning success.
  • A failure after the seal left no consistent state. The layer stack was switched in memory before
    the config was updated, so a failed update left the running device and the on-disk config describing
    different stacks, with no record from which a restart could recover.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

  • the request config is copied to <config>.snapshot and published by rename(), so the caller's file
    is no longer consumed.
  • conf is updated to the committed config, so an identical request returns success without touching
    anything.
  • the config rename() now runs inside the commit callback, under the write lock and before the
    in-memory switch; a failure discards the prepared stack and both memory and disk stay on the old one.

Recovery covers process crashes only; the snapshot path performs no fsync.

Please check the following list:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test?
  • Does this change require a documentation update?
  • Does this introduce breaking changes that would require an announcement or bumping the major version?
  • Do all new files have an appropriate license header?

Copilot AI 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.

🟡 Changes recommended

Unresolved rollback, journal cleanup, and durability failures can discard acknowledged writes or persist an inconsistent layer stack.

7 open issues
What changed in this PR

This PR reworks live snapshots into a recoverable transaction, aiming to make retries idempotent and preserve a consistent layer stack across failures and restarts.

Changes:

  • Splits restacking into preparation, config publication, and in-memory switching.
  • Adds rollback journals, startup recovery, strict layer validation, and snapshot serialization.
  • Adds regression tests and documents failure and recovery semantics.
File Description
src/​test/​image_service_test.cpp Adds retry, validation, rollback, sequential snapshot, and recovery tests.
src/​overlaybd/​lsmt/​index.h Declares index preparation and application APIs.
src/​overlaybd/​lsmt/​index.cpp Prepares merged indexes without modifying the active backing index.
src/​overlaybd/​lsmt/​file.h Adds transaction callbacks and file-capacity reservation.
src/​overlaybd/​lsmt/​file.cpp Implements staged restacking, seal rollback, and fsync error propagation.
src/​image_file.h Declares recovery helpers and a snapshot mutex.
src/​image_file.cpp Implements journals, recovery, validation, and idempotent snapshot handling.
docs/​live-snapshot.md Documents transaction semantics, recovery, and limitations.
docs/​live-snapshot_zh.md Adds corresponding Chinese documentation.

Review effort: Balanced

Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/overlaybd/lsmt/file.cpp Resolved
@pakkah
pakkah requested a balanced review from Copilot October 26, 2026 06:43

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

@pakkah
pakkah force-pushed the dev/snapshot branch 2 times, most recently from a44d8eb to d12179e Compare October 28, 2026 09:11
@pakkah
pakkah requested a balanced review from Copilot October 28, 2026 09:20

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

Copilot AI 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.

Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/overlaybd/lsmt/file.cpp Resolved
Comment thread src/overlaybd/lsmt/file.cpp Resolved
Comment thread src/image_file.cpp Resolved

Copilot AI 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.

Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved

Copilot AI 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.

🟡 Changes recommended

Journal collision checks miss backing files and path aliases, risking layer-data deletion during startup recovery, while direct use of renameat2() breaks the supported CentOS 7 build.

2 open issues
3 resolved since last review

Review effort: Balanced

Comment thread src/image_file.cpp Resolved
Comment thread src/image_file.cpp Resolved
Signed-off-by: huangzhaoxiang.hzx <huangzhaoxiang.hzx@alibaba-inc.com>

This branch has not been deployed

No deployments
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