Skip to content

Try accelerator before registry resolution - #469

Open
vpatelsj wants to merge 3 commits into
containerd:mainfrom
vpatelsj:fix/accelerator-before-registry-resolution
Open

vpatelsj wants to merge 3 commits into
containerd:mainfrom
vpatelsj:fix/accelerator-before-registry-resolution

Conversation

@vpatelsj

Copy link
Copy Markdown

What this PR does / why we need it:

When an accelerator is configured, RegistryFS v2 currently resolves the canonical registry blob URL before sending the data request through the accelerator. That makes the accelerator path depend on registry authentication or redirect resolution even when the accelerator can serve the canonical URL directly.

This change tries the configured accelerator first with the canonical URL and requested byte range. A 200 or 206 response is returned immediately. Any other response continues through the existing registry resolution and authenticated request path.

Validation:

  • Exercised this OverlayBD build in an AKS ACR Artifact Streaming run with 1,000 successful pull pods on 1,000 unique nodes.
  • Physical host-store verification found all 41 manifest layers on all 1,000 nodes: 41,000 layer files and 43,325,418,496,000 bytes.
  • git diff --check passes.
  • A local CMake build was not run because CMake is not installed in the available environment.

Which issue(s) this PR fixes (optional):

None.

Please check the following list:

  • Does the affected code have corresponding tests, e.g. unit test, E2E test? (Validated by the integration run described above; no focused RegistryFS unit test was added.)
  • Does this change require a documentation update? (No.)
  • Does this introduce breaking changes that would require an announcement or bumping the major version? (No.)
  • Do all new files have an appropriate license header? (No new files.)

Signed-off-by: vpatelsj <vpatelsj@gmail.com>

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

The probe can exhaust the fallback deadline and repeatedly add failed requests to authenticated reads.

2 open findings
What changed in this PR

This PR lets RegistryFS v2 try the accelerator before resolving the canonical registry URL.

Changes:

  • Sends the requested byte range directly through the configured accelerator.
  • Returns successful responses immediately; otherwise continues through existing registry resolution and authentication.
File Description
src/​overlaybd/​registryfs/​registryfs_v2.cpp Adds the accelerator-first request path.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/overlaybd/registryfs/registryfs_v2.cpp Outdated
Comment thread src/overlaybd/registryfs/registryfs_v2.cpp Outdated
Signed-off-by: vpatelsj <vpatelsj@gmail.com>

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

Setting an unchanged accelerator address clears shared failure decisions, causing avoidable failed probes during image creation.

1 open issue
2 resolved since last review

Review effort: Balanced

Comment thread src/overlaybd/registryfs/registryfs_v2.cpp Outdated
Signed-off-by: vpatelsj <vpatelsj@gmail.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