Skip to content

Conversation

@hweawer
Copy link
Collaborator

@hweawer hweawer commented Jan 26, 2026

This PR introduces context-aware, traced DownloadBlob operations in the origin blob client and propagates the new signature throughout the call chain (cluster client, proxy prefetch/preheat, registry transfer, and tag resolution), along with corresponding test and mock updates.

Changes:

  • Extend blobclient.Client and blobclient.ClusterClient DownloadBlob signatures to accept context.Context, and add OpenTelemetry spans, attributes, and logging for origin and cluster-level download operations.
  • Update all production call sites (proxy preheat/prefetch, Docker tag resolution, registry read/write transferer, origin server tests) to pass a context (typically context.Background()), and wire the context through to HTTP requests via httputil.SendTracingContext.
  • Regenerate/update GoMock mocks and unit tests to match the new method signatures, and ensure existing download/error-handling behaviors (e.g., ErrBlobNotFound, 404/202 handling, Poll backoff behavior) remain covered.

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

This PR introduces context-aware, traced DownloadBlob operations in the origin blob client and propagates the new signature throughout the call chain (cluster client, proxy prefetch/preheat, registry transfer, and tag resolution), along with corresponding test and mock updates.

Changes:

  • Extend blobclient.Client and blobclient.ClusterClient DownloadBlob signatures to accept context.Context, and add OpenTelemetry spans, attributes, and logging for origin and cluster-level download operations.
  • Update all production call sites (proxy preheat/prefetch, Docker tag resolution, registry read/write transferer, origin server tests) to pass a context (typically context.Background()), and wire the context through to HTTP requests via httputil.SendTracingContext.
  • Regenerate/update GoMock mocks and unit tests to match the new method signatures, and ensure existing download/error-handling behaviors (e.g., ErrBlobNotFound, 404/202 handling, Poll backoff behavior) remain covered.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
proxy/proxyserver/server_test.go Updates origin client mock expectations for DownloadBlob to include a context.Context argument via gomock.Any(), keeping proxy server tests aligned with the new client interface.
proxy/proxyserver/preheat.go Passes context.Background() into clusterClient.DownloadBlob when fetching manifests for preheat, enabling traced, context-aware origin downloads.
proxy/proxyserver/prefetch.go Uses context.Background() in all clusterClient.DownloadBlob calls (manifest fetch and layer fetches) to participate in the new traced download path while preserving existing metrics and error handling.
origin/blobserver/testutils_test.go Imports context and updates ensureHasBlob to call DownloadBlob with a context, keeping test utilities compatible with the new client signature.
origin/blobserver/server_test.go Adds context import and updates the DownloadBlob call in TestDownloadBlobNotFound to pass context.Background(), preserving the 404 behavior test with the new API.
origin/blobserver/cluster_client_test.go Adjusts cluster client tests to use context-aware DownloadBlob in both direct calls and Poll callbacks, and updates mock expectations to accept an extra context.Context parameter.
origin/blobclient/cluster_client.go Extends the ClusterClient interface to include ctx in DownloadBlob, wraps the operation in an OpenTelemetry span with relevant attributes, and routes the context into underlying clients while preserving ErrBlobNotFound and error-handling semantics.
origin/blobclient/client.go Extends Client.DownloadBlob to accept ctx, instruments the HTTP GET/download with OTel spans (including origin address and bytes transferred), and propagates the tracing context into httputil.Get via SendTracingContext.
mocks/origin/blobclient/clusterclient.go Updates generated GoMock MockClusterClient.DownloadBlob and its recorder to include a context.Context argument, aligning mocks with the new ClusterClient interface.
mocks/origin/blobclient/client.go Updates generated GoMock MockClient.DownloadBlob and its recorder to accept context.Context, matching the updated Client interface.
lib/dockerregistry/transfer/rw_transferer_test.go Updates origin cluster mock expectations for DownloadBlob to include a context parameter for the cache-populating download path.
lib/dockerregistry/transfer/rw_transferer.go Wraps registry-side Download and downloadFromOrigin in OTel spans and passes the tracing context into originCluster.DownloadBlob, adding detailed status and error annotations while keeping existing CAS and error behavior.
build-index/tagtype/map_test.go Adjusts originClient.DownloadBlob expectations in TestMapResolveDocker to account for the new context.Context parameter.
build-index/tagtype/docker_resolver_test.go Adds a context import and updates all origin client DownloadBlob expectations and DoAndReturn signatures to accept a context.Context, ensuring resolver retry semantics are still verified.
build-index/tagtype/docker_resolver.go Imports context and calls originClient.DownloadBlob(context.Background(), ...) in downloadManifest, making Docker resolver downloads use the new context-aware, traced download API.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

return client.DownloadBlob(ctx, namespace, d, dst)
})
if httputil.IsNotFound(err) {
span.SetStatus(codes.Error, "blob not found")
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is sometimes an expected scenario that happens quite a bit in production actually (although im not sure why). At the same time, we have situations where it is not an expected scenario (when the blob shd be in GCS but isn't due to writeback delay). So based on those circumstances, should we be treating this scenario as an error or as normal?

interval = interval * 2
}
if err := ph.clusterClient.DownloadBlob(repo, d, buf); err == nil {
if err := ph.clusterClient.DownloadBlob(context.Background(), repo, d, buf); err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't the context be generated in the handler instead of the helper funcs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants