Skip to content

Add periodic snapshot file cleanup#7747

Open
Copilot wants to merge 30 commits intomainfrom
copilot/add-max-retained-snapshot-files
Open

Add periodic snapshot file cleanup#7747
Copilot wants to merge 30 commits intomainfrom
copilot/add-max-retained-snapshot-files

Conversation

Copy link
Contributor

Copilot AI commented Mar 17, 2026

Add optional files_cleanup section to node configuration, with options to periodically clean up snapshots.

If there are snapshot files in excess of files_cleanup.max_snapshots in the snapshots.directory, they are deleted, starting from the oldest one. The cleanup task runs every files_cleanup.interval.

Also updates trigger_snapshot in test infrastructure to use the new /node/snapshot:create operator endpoint (from #7767) instead of the governance proposal mechanism.


📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

Copilot AI and others added 2 commits March 17, 2026 20:00
…ic, docs, tests, and changelog

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
@achamayou
Copy link
Member

@copilot when the test checks that one or more snapshot is deleted, it must ensure that the oldest snapshot(s) were deleted, not just any snapshot. The test should not try to read from the log, that's unreliable.

…ding

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot when the test checks that one or more snapshot is deleted, it must ensure that the oldest snapshot(s) were deleted, not just any snapshot. The test should not try to read from the log, that's...

Fixed in df7d1db. The test now tracks all snapshot seqnos as they're created, then asserts the remaining files are exactly the newest max_retained ones by comparing seqno sets. Removed the log-reading assertions.

Copilot AI requested a review from achamayou March 17, 2026 20:26
@achamayou
Copy link
Member

@copilot please fix the formatting!

… in CHANGELOG.md

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 17, 2026

@copilot please fix the formatting!

Fixed in f3ded94. Two issues: clang-format wanted a single-line declaration in snapshot_manager.h, and CHANGELOG.md used RST double-backticks instead of Markdown single-backticks.

@achamayou achamayou marked this pull request as ready for review March 18, 2026 14:48
@achamayou achamayou requested a review from a team as a code owner March 18, 2026 14:48
Copilot AI review requested due to automatic review settings March 18, 2026 14:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new host configuration option to cap retained committed snapshot files and wires it through config/schema/tests, including an end-to-end retention verification.

Changes:

  • Introduces snapshots.max_retained_snapshot_files in startup config + JSON serialization + schema + docs/changelog.
  • Passes the new option into SnapshotManager and implements on-commit cleanup of old committed snapshot files.
  • Updates test infra and adds an E2E test to verify oldest snapshots are deleted when exceeding the retention limit.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
include/ccf/node/startup_config.h Adds the new optional snapshots retention config field.
src/common/configuration.h Registers the field for JSON (de)serialization.
doc/host_config_schema/cchost_config.json Documents and constrains the new config option in schema.
src/host/run.cpp Passes the new config value into SnapshotManager.
src/snapshots/snapshot_manager.h Enforces minimum value and deletes oldest committed snapshots after commit.
tests/infra/network.py Forwards the new arg into node config generation.
tests/infra/node.py Adds a reusable trigger_snapshot() helper for tests.
tests/config.jinja Adds the new field to the generated test config template.
tests/e2e_operations.py Refactors snapshot triggering and adds a retention E2E test; integrates into test runner.
doc/operations/ledger_snapshot.rst Documents operational behavior of snapshot retention.
CHANGELOG.md Notes the newly added configuration option.

You can also share your feedback on Copilot code review. Take the survey.

Co-authored-by: Amaury Chamayou <amaury@xargs.fr>
@achamayou achamayou added this to the 7.0.0-rc0 milestone Mar 26, 2026
@achamayou
Copy link
Member

@copilot update trigger_snapshot to use the new endpoint added in #7767 rather than the governance.

… of governance

Agent-Logs-Url: https://github.com/microsoft/CCF/sessions/1e685bf8-047a-4456-a883-2bdcc327e280

Co-authored-by: achamayou <4016369+achamayou@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Mar 26, 2026

@copilot update trigger_snapshot to use the new endpoint added in #7767 rather than the governance.

Fixed in cd1ea3b. Updated trigger_snapshot in node.py to use POST /node/snapshot:create on the FILE_SERVING_RPC_INTERFACE instead of the governance proposal, and updated all 5 call sites in e2e_operations.py.

Copilot AI requested a review from achamayou March 26, 2026 16:04
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds an optional host configuration section to periodically delete older committed snapshot files, and updates test infrastructure to trigger snapshots via the operator endpoint rather than governance proposals.

Changes:

  • Introduce files_cleanup host config (schema + C++ config types) and wire a periodic cleanup timer into the host.
  • Add end-to-end coverage for snapshot retention behavior on primary and backup nodes.
  • Update test infra to trigger snapshots via POST /node/snapshot:create.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
src/host/snapshot_cleanup_timer.h New libuv-timer-backed implementation to delete oldest committed snapshots beyond a retention limit.
src/host/run.cpp Instantiates the cleanup timer when files_cleanup.max_snapshots is set and interval > 0.
include/ccf/node/startup_config.h Adds CCFConfig::FilesCleanup to startup config structures.
src/common/configuration.h Adds JSON (de)serialization for CCFConfig::FilesCleanup and hooks it into CCFConfig.
doc/host_config_schema/cchost_config.json Documents/validates new files_cleanup schema fields.
doc/operations/ledger_snapshot.rst Documents retention behavior for committed snapshots.
CHANGELOG.md Records new configuration options.
tests/config.jinja Emits files_cleanup into generated node configs when provided by test args.
tests/infra/network.py Forwards new test args into node config rendering.
tests/infra/node.py Adds Node.trigger_snapshot() using the /node/snapshot:create operator endpoint.
tests/e2e_operations.py Updates snapshot tests to use the operator endpoint and adds new cleanup-focused tests.
src/snapshots/snapshot_manager.h Disables copy/assignment for SnapshotManager.

achamayou and others added 2 commits March 26, 2026 22:14
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

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.

5 participants