feat(image): publish converted OCI layers over P2P - #226
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 _guard = self.object_update_lock.lock().await; | ||
| let mut p2p_keys = p2p_keys; | ||
| if let Some(existing) = self.get_hard_commit_object(&digest).await? { | ||
| p2p_keys.extend(existing.p2p_keys); | ||
| } | ||
| self.store | ||
| .put( |
There was a problem hiding this comment.
The async mutex is held while performing the read and write to the KV store, and the reconciliation callers hold it across multiple get_hard_commit_object calls plus a batch write. This serializes unrelated metadata operations and can make large config reconciliation block P2P ownership updates for the entire duration. If the store cannot provide an atomic read-modify-write, consider narrowing the lock to per-object updates or using a transactional/CAS operation.
| let Some(mut record) = self.get_hard_commit_object(digest).await? else { | ||
| bail!("hard commit object {digest} does not exist while recording P2P key '{key}'"); | ||
| }; | ||
| if !record.p2p_keys.insert(key.clone()) { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
This ownership update can race with GC deletion: add_hard_commit_p2p_key is serialized by object_update_lock, but delete_collectable_hard_commit ultimately calls remove_hard_commit_object without that lock. If the add reads the record while GC removes it, the add can then write the stale record back after the file and P2P publication have been deleted, leaving a dangling hard-commit metadata entry that can be repeatedly reconsidered by GC. Make removal participate in the same atomic update/lock protocol (and re-check the record while holding it).
| async fn publish_converted_layer( | ||
| &self, | ||
| key: &LayerConversionKey, | ||
| layer: &LocalLayer, | ||
| ) -> Result<bool> { | ||
| let Some(transport) = self.p2p_transport.get().cloned() else { | ||
| return Ok(false); | ||
| }; |
There was a problem hiding this comment.
This early return is dead code when P2P is disabled, which is the default. Startup calls initialize_image_cache_p2p_transport no matter what, and in the disabled case that installs a DisabledP2pTransport whose publish just returns Ok(()). So the guard never triggers.
That has a couple of annoying consequences. On a default deployment, every local conversion records the key via add_hard_commit_p2p_key (the next line) even though nothing will ever be published, so the metadata slowly fills with ownership entries for commits nobody owns. Worse, cleanup of those keys is gated on the transport being initialized at all — delete_collectable_hard_commit refuses to delete them from a process that never initialized. So the entries are written unconditionally but can only be removed under a condition that has nothing to do with whether they exist.
I'd suggest checking whether the transport is actually enabled instead of whether it exists — e.g. return Ok(false) early when local_endpoint().is_none().
| return Ok(None); | ||
| return self.lookup_remote_converted_layer(key).await; |
There was a problem hiding this comment.
This path does the same failed lookup twice. cached_standard_oci_lowers returns None on the first miss, then convert_standard_oci_layers_pipeline_inner starts over from layer 0 and calls lookup_converted_layer for that same layer again. ImageCacheOperationHold doesn't remember per-operation results, so the first-missing layer's P2P lookup and fetch run twice in full inside a single resolve.
That gets expensive with a stale catalog entry. If the provider is dead, the fetch sits on the default 30s timeout, and every cold resolve of that image pays the stall twice before falling back to the registry. One bad catalog entry doubles the worst-case latency for everyone.
I'd cache failed P2P results by key in the hold, scoped to the operation's lifetime, so the second lookup gives up immediately instead of retrying the fetch.
2 participants
What
Publish converted standard OCI image layers through the existing P2P transport and reuse them across nodes. Track P2P artifact ownership in the image-cache metadata so eviction can remove remote references together with local commit files.
Why
Standard OCI layers currently need to be downloaded and converted independently on each node. Sharing completed OverlayBD conversions avoids repeated registry downloads and conversion work while preserving the existing local-cache and registry fallback paths.
Related issue
N/A. No issue or prior design discussion is referenced by the commits in this branch.
Scope and non-goals
This PR includes:
oci-layer/v1/{context-hash}P2P key derived fromLayerConversionKey.iroh1.0.3 andiroh-blobs0.103.0 dependency update.This PR does not change overlaybd-native image handling, snapshot artifact publication, public API routes, or introduce new configuration knobs.
Design and behavior changes
The server initializes the image cache with the same process-wide P2P transport used by the other P2P consumers. For a standard OCI layer, the conversion context is serialized and hashed into the catalog key. Once the converted commit is durable locally, the image cache records ownership and publishes the file by reference.
On a cache-index miss or stale entry, the image cache first attempts the corresponding P2P lookup. Fetched bytes must match the advertised digest and size and contain a sealed OverlayBD layer with the expected UUID before they are persisted and indexed locally. Lookup, fetch, metadata, digest, or structural validation failures are treated as acceleration misses and fall back to registry download and conversion; invalid fetched artifacts are unpublished when possible.
Image-cache GC carries the recorded P2P keys through its collectability checks and unpublishes them before removing the local commit and metadata. Metadata updates are serialized so ownership is preserved across concurrent record/reconcile operations.
Compatibility and operations
p2p_keysfield, defaulting to empty for existing records.irohto 1.0.3 andiroh-blobsto 0.103.0. Nodes without usable P2P publication or lookup continue through the existing local/registry paths; cache metadata remains readable when no P2P keys are present.Validation
make fmtmake clippymake test-unitmake -C services test(required whenservices/changes)maketargetCommands and results:
Skipped checks and reasons:
Risks and reviewer notes
src/image/oci_image.rsandsrc/image/cache/service.rs.Checklist