feat(snapshot): compress layers at publish time when uploading to OSS/ACR - #256
Conversation
|
🔍 OpenCodeReview found 6 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| let rootfs_digests = committed_layer_digests(&committed.rootfs_layers); | ||
| artifacts.extend(SnapshotP2pArtifact::local_overlaybd_layers( | ||
| &manifest.rootfs.image_config_path, | ||
| &rootfs_digests, | ||
| &rootfs_uuids, | ||
| )); |
There was a problem hiding this comment.
When publish compression is enabled, each raw local layer has neither a digest matching rootfs_digests nor a UUID in the committed layer (the recontainerized ZFile reports uuid = None). Consequently local_overlaybd_layers filters out both publication modes, so no rootfs layer is advertised to P2P; the same applies to memory and attached drives below. This makes [snapshot.publish_compression] effectively disable snapshot-layer P2P and forces every cross-node resume through OSS/ACR. Preserve the prepared compressed artifact until P2P publication (or recontainerize again for the committed digest) so the enabled options continue to work together.
| // TODO: propagate the uploaded (compressed) layer | ||
| // paths out of repository publish so digest-keyed P2P | ||
| // publication can advertise the same bytes the | ||
| // committed manifest records. |
There was a problem hiding this comment.
With publish compression enabled, this drops every newly recontainerized local layer from digest-keyed P2P publication. Consumers resolve the committed compressed digest, so they can only fall back to OSS/ACR; a snapshot can no longer be restored from P2P when the origin is unavailable, and normal restores lose P2P acceleration for these layers. This should not be merged as a TODO: retain/return the prepared compressed artifact from repository publication (at least through this P2P upload) and advertise it under the committed digest before deleting the temporary file.
| let temp = NamedTempFile::new().map_err(|e| { | ||
| RepositoryError::backend( | ||
| format!( | ||
| "create temp zfile layer for recontainerizing '{}'", | ||
| source.display() | ||
| ), | ||
| e, | ||
| ) | ||
| })?; |
There was a problem hiding this comment.
The NamedTempFile only owns temp.path(), while compact_layers writes to a sibling temp.path().with_extension("commit.<uuid>.tmp") and renames it only on completion. If this async preparation future is cancelled during compaction, compact_layers is dropped before its post-error cleanup runs: the named temp is removed, but the potentially large sibling remains in /tmp. Repeated cancelled publishes can exhaust disk space. Use a cancellation-safe guard that also owns/removes the compactor's staging path, or make compact_layers cancellation-safe internally.
| let Some(recontainerized_path) = | ||
| compact_layers(&[layer], temp.path(), mode) | ||
| .await |
There was a problem hiding this comment.
This runs full-layer compression directly in the caller's Tokio task. With the default workers = 1, ZFileBuilder compresses blocks inline; with multiple workers, its result collection uses blocking channel receives. A large snapshot can therefore monopolize/block a Tokio worker and delay unrelated operations. Move the compaction onto a dedicated blocking worker/runtime, or make the compression/result-collection internals nonblocking.
| size: descriptor.size, | ||
| uuid: overlaybd_layer_uuid(&canonical), | ||
| }) | ||
| let upload = prepare_layer_upload(&canonical, self.publish_compression, None).await?; |
There was a problem hiding this comment.
A cancellation during this new publish-time preparation can leak a potentially large *.tmp layer. prepare_layer_upload calls compact_layers, whose cleanup runs only after merge_files_ro(...).await returns; if the enclosing publish future is dropped at this await, the cleanup branch is never reached and the sibling temporary file remains. Please make compaction cleanup drop-based/cancellation-safe (for example, retain a temp-file guard that removes the in-progress output on Drop) before invoking it from request paths. This affects all three new OSS preparation calls.
f85151e to
e2c1fb8
Compare
| pub struct SnapshotPublishCompressionConfig { | ||
| #[config(default = true)] | ||
| pub enabled: bool, | ||
| #[config(default = "lz4")] | ||
| pub algorithm: OverlaybdCompressionAlgorithm, |
There was a problem hiding this comment.
There is no migration path for the removed [memory_snapshot].compression_* / [template_build].compression_* settings. An existing deployment that explicitly disabled compression will now either reject those obsolete keys or ignore them and silently enable publish-time compression, changing CPU usage and artifact format on upgrade. Consider accepting/deprecating the legacy fields with an explicit precedence rule, or fail with an actionable migration diagnostic; add an old-config parsing test.
| #[config(default = true)] | ||
| pub enabled: bool, |
There was a problem hiding this comment.
This default also affects SnapshotPublishCompressionConfig::default(). OssBackend::new() passes that value while explicitly documenting that its convenience path keeps publish compression disabled, so direct callers will actually compress every uploaded layer. Pass an explicit disabled config from that constructor (or provide a separate disabled helper/default) so its behavior matches the API contract.
| algorithm: config.algorithm, | ||
| workers: config.workers.clamp(1, Self::MAX_COMPRESSION_WORKERS), |
There was a problem hiding this comment.
This bounds workers per layer, not process-wide compression concurrency. Repository publishing can run concurrently for multiple snapshots, and each upload may therefore create up to 64 compression workers; with compression now enabled by default, concurrent publishes can exhaust CPU/threads despite this stated safeguard. Gate recontainerization with a shared semaphore/global worker budget, or otherwise cap aggregate concurrent compression.
| } else { | ||
| // The committed record references different bytes for | ||
| // this layer, which means publish-time compression | ||
| // recontainerized the local raw layer as zfile during | ||
| // upload. Publishing the raw file under its raw digest | ||
| // would create a key no consumer ever looks up. | ||
| // TODO: propagate the uploaded (compressed) layer | ||
| // paths out of repository publish so digest-keyed P2P | ||
| // publication can advertise the same bytes the | ||
| // committed manifest records. |
There was a problem hiding this comment.
With the new defaults (snapshot.p2p_enabled = true and publish compression enabled), every newly captured raw delta takes this branch. The committed compressed layer has no UUID either, so neither digest-keyed nor UUID-keyed layer bytes are published to P2P. Consequently, P2P layer distribution is effectively disabled for default OSS publishes and cross-node resumes always fall back to OSS. This should not be left as a TODO in the enabled path: retain/return the prepared compressed artifact from repository publication, or deterministically recontainerize it for P2P and publish it under the committed digest.
| // TODO: propagate the uploaded (compressed) layer | ||
| // paths out of repository publish so digest-keyed P2P | ||
| // publication can advertise the same bytes the | ||
| // committed manifest records. |
There was a problem hiding this comment.
This leaves P2P layer publication incomplete for the default configuration: snapshot.publish_compression.enabled defaults to true, so each newly captured raw layer is uploaded under a new compressed digest and then skipped here. The compressed copy is a NamedTempFile dropped when the repository upload completes, meaning no artifact is ever advertised under the digest consumers request. Backend fallback exists, but normal compression-enabled snapshots now get no P2P delivery for their new layers (and cannot launch through P2P when backend access is unavailable). Keep/return the prepared compressed artifact until P2P publication completes, or prepare the same compressed bytes for P2P before discarding them.
| Self::new_with_publish_compression( | ||
| config, | ||
| cache_root, | ||
| &SnapshotPublishCompressionConfig::default(), |
There was a problem hiding this comment.
SnapshotPublishCompressionConfig::default() has enabled = true, so this convenience constructor enables compression despite the new API contract immediately above saying it remains disabled. This also changes behavior for existing direct callers that cannot supply the new setting. Pass an explicitly disabled config here (or change the documentation/API intentionally).
Suggestion:
| &SnapshotPublishCompressionConfig::default(), | |
| &SnapshotPublishCompressionConfig { | |
| enabled: false, | |
| ..Default::default() | |
| }, |
| # [snapshot.publish_compression] | ||
| # enabled = true | ||
| # Supported algorithms are "lz4" and "zstd". | ||
| # algorithm = "lz4" | ||
| # Workers compressing 4 KiB blocks. One is sequential; higher values run in | ||
| # parallel without changing the output layout. | ||
| # workers = 1 |
There was a problem hiding this comment.
Please enable compression by default.
There was a problem hiding this comment.
OK. I think we should uncomment the configuration. Otherwise the configuration may seem confused to users.
…/ACR Capture-time compression pessimised local resume: the guest working set is unchanged by compression, and while layer files are page-cache-warm the saved bytes never materialize, yet decompression CPU is paid on every fault (cold resume: raw 63.8ms vs lz4 +18% vs zstd +106%; hot path identical). The bytes compression saves only matter on the network path. Local layers now always stay raw. A new [snapshot.publish_compression] switch (enabled/algorithm/workers, on by default) compresses memory layers and incremental read-write layers once as they are uploaded to OSS/ACR, cutting network bytes for cross-node resume. The legacy capture-time switches ([memory_snapshot].compression_* and [template_build]) are removed from the configuration schema. - shared prepare_layer_upload helper recontainerizes raw local layers via compact_layers, then hashes and uploads the compressed bytes; manifest digest/size, OSS object keys and ACR blob annotations all reference the uploaded bytes; remote lowers untouched; zfile inputs skipped (idempotent) - capture path: memory/rootfs/drive layers always sealed raw; template build compression override pipeline removed - P2P digest-keyed layer publication is skipped when the committed record names different bytes than the local descriptor (TODO to propagate compressed paths) - read path unchanged: consumers auto-detect zfile by content magic
e2c1fb8 to
d03b5a6
Compare
| /// enabled, memory layers and incremental read-write layers are compressed | ||
| /// once as they are uploaded, cutting network bytes for cross-node resume. |
There was a problem hiding this comment.
This cross-node claim does not hold for the default P2P path: after recontainerization, the committed record contains the compressed digest, but SnapshotP2pArtifact::local_overlaybd_layers only has the raw local file and explicitly skips publishing it when the digest differs. Since compression is now enabled by default (and P2P is also enabled by default), newly published layers cannot be served by P2P and every remote node must fall back to OSS/ACR. Preserve the prepared compressed artifact through P2P publication, or otherwise publish bytes under the committed digest.
| #[config(default = true)] | ||
| pub enabled: bool, |
There was a problem hiding this comment.
The old [memory_snapshot].compression_* and [template_build].compression_* settings were removed, while this replacement defaults to enabled. Existing deployments that explicitly disabled the old switches will therefore have those keys ignored/rejected by the loader and unexpectedly publish with compression after upgrading. Please retain deprecated compatibility fields or add explicit migration/validation so old configurations do not silently change behavior (and add a legacy-config loading test).
| Self::ZFile { | ||
| algorithm, | ||
| workers: workers.clamp(1, Self::MAX_COMPRESSION_WORKERS), | ||
| algorithm: config.algorithm, | ||
| workers: config.workers.clamp(1, Self::MAX_COMPRESSION_WORKERS), | ||
| } |
There was a problem hiding this comment.
This only caps workers per layer operation. Concurrent snapshot publications can each create up to 64 compression workers, so aggregate CPU/thread usage remains unbounded and can starve unrelated async work. Gate publish-time compactions with a process-wide semaphore/worker budget (or use a shared bounded compression pool), rather than applying only a per-operation clamp.
| if committed_digests.contains(&layer.digest) { | ||
| artifacts.push(Self::content_addressed_overlaybd_layer( | ||
| layer.file.clone(), | ||
| layer.digest, | ||
| layer.size, | ||
| )); | ||
| } else { |
There was a problem hiding this comment.
With publish compression enabled (now the default), every newly captured raw layer takes this branch, while the temporary compressed upload is deleted when repository publication returns. Consequently no artifact is advertised under the committed digest, so cross-node reads always miss P2P and fall back to OSS/ACR; for UUID-addressed reads there is no origin fallback at all. This effectively disables P2P layer distribution for the default publish path. Preserve/return the prepared compressed artifact from repository publication (or publish it before dropping the temporary file) so it can be registered under the committed digest.
| Self::new_with_publish_compression( | ||
| config, | ||
| cache_root, | ||
| &SnapshotPublishCompressionConfig::default(), | ||
| ) |
There was a problem hiding this comment.
This does not disable publish compression as documented: SnapshotPublishCompressionConfig::default() resolves enabled to true. Consequently, every direct/test caller of OssBackend::new now silently enables recontainerization. Pass an explicit config with enabled: false (or change the constructor documentation and intended compatibility behavior).
Suggestion:
| Self::new_with_publish_compression( | |
| config, | |
| cache_root, | |
| &SnapshotPublishCompressionConfig::default(), | |
| ) | |
| Self::new_with_publish_compression( | |
| config, | |
| cache_root, | |
| &SnapshotPublishCompressionConfig { | |
| enabled: false, | |
| ..Default::default() | |
| }, | |
| ) |
| /// independent processes (raw/lz4/zstd temp configs) to cover all modes. | ||
| /// without an intermediate raw memory file, and are always written raw. | ||
| #[tokio::test] | ||
| async fn memory_snapshot_format_matches_config_and_resumes() -> Result<()> { |
There was a problem hiding this comment.
This test no longer compares the format with configuration; it now unconditionally verifies raw capture. Rename it (for example, memory_snapshot_is_raw_and_resumes) so the test name reflects the contract.
2 participants
What
Redesign snapshot layer compression: local layers always stay raw, and compression happens once when layers are uploaded to OSS/ACR. A single new switch,
[snapshot.publish_compression](enabled/algorithm(lz4|zstd) /workers, on by default), replaces the legacy capture-time knobs —[memory_snapshot].compression_*and[template_build]are removed from the configuration schema.Why
Benchmarks show capture-time compression is a local pessimization: the guest working set is unchanged by compression (~62 MiB in the test setup), and while layer files are page-cache-warm the saved bytes never materialize — yet decompression CPU is paid on every fault (cold resume: raw 63.8 ms vs lz4 +18% vs zstd +106%; hot path identical at ~15 ms). The bytes compression actually saves only matter on the network path: OSS/ACR upload and cross-node resume.
Related issue
Closes # (no tracking issue; motivation is the measurement analysis above)
Scope and non-goals
Repositorytrait changes; POSIX backend unchanged (local stays raw); zero read-path changes (ZFile is already transparent via switch-file magic detection); full P2P alignment for compressed digests (left as a TODO).Design and behavior changes
close_sealunchanged).prepare_layer_upload(common/recontainerize.rs) probesis_zfile, recontainerizes raw local layers withcompact_layersinto a temporary ZFile, then hashes and uploads the compressed bytes.ManagedLayer.digest/size, OSS object keys, and ACR blob annotations all reference the actually-uploaded bytes. Remote lowers are re-referenced untouched; already-ZFile inputs are passed through (idempotent).delete_prefix); no partial uploads; temp files are cleaned up by guard.uuid = None, so P2P uuid-keyed acceleration does not apply to them (the daemon virtual-size probe falls back to a fullImageFileopen, one-time).Compatibility and operations
[snapshot.publish_compression]section (enabled by default,lz4) — OSS/ACR uploads start compressing on upgrade; setenabled = falseto keep raw uploads. Legacy keys removed from the schema; unknown TOML keys are ignored by the config parser, so stale configs keep starting.Validation
make fmt(cargo fmt --all -- --check)make clippy(cargo clippy -p agentenv -p agentenv-e2e-tests --all-targets -- -D warnings)make test-unit(cargo test -p agentenv --lib: 817/818; the single failure is a pre-existing flakyapi::proxytest that passes in isolation, unrelated)fc::memory_snapshot_format_matches_config_and_resumes,fc::backend_pause_state_round_trips_through_encoded_artifacts)reference.md,default.toml,p2p-design.md)Commands and results:
Performance comparison (real OSS, release build; template-build scenario, memory layer 87.8 → 31.9 MiB, 2.75×):
Evidence for removing capture-time compression: cold resume raw 63.8 ms vs lz4 75.1 ms (+18%) vs zstd 131.4 ms (+106%); hot path identical at ~15 ms; guest working set (~62 MiB) is codec-independent.
Skipped checks and reasons:
make -C services test(services/untouched); no generated-code changes.Risks and reviewer notes
recontainerize.rsand the minio e2e. Please focus review oncommon/recontainerize.rs(new), the three upload branches inoss/repository.rs, andacr/publisher.rs::upload_local_delta.p2p.rsabout propagating compressed paths to restore acceleration).uuid = None: P2P uuid-keyed acceleration is unavailable for them (pre-existing graceful degradation, not a regression).Checklist