feat(oci): serve a virtual repository's blob route (S32 plan: 16/20)
Summary
This MR fills the blob route of a kind=1 container repository. GET and HEAD on /v2/<name>/blobs/<digest> now resolve through the virtual repository's upstreams and stop answering the interim 501.
- The handler runs the digest gate first, then maps the request's repository to its
container_virtual_repositoriesrow through the finder, then callsResolve. - A warm blob follows the configured delivery mode. A cold full
GETstreams under both modes. A coldHEADor rangedGETdrains the fill, then answers from the committed row. - A committed read is a follower whose leader's fill already committed. It closes its copy unread and answers every request shape from the row, as the standalone
kind=2route does. Under redirect delivery a fullGETfrom such a follower answers307. - The route arms its own write deadline. Without it, net/http's
server.timeouts.write(10sinconfig.example.yaml) cuts every cold layer that takes longer, and no such layer caches. The pre-fetch arm coversresolve_timeoutplus the fill budget. The re-arm afterResolvetakes the larger of the fill tail andserver.timeouts.write, so an operator who raised that key for large layers keeps the raised value. - The stream-failure log now receives the re-armed instant. A copy that the armed deadline cut logs at Error as a deadline kill, not as client churn.
Plan amendment
!2941 (merged) amends Step 16 and is still open. It adds the server.timeouts.write floor and the committed-read branch to the plan. Until !2941 (merged) merges, this code follows !2941 (merged)'s plan text, not the plan on main.
Governing ADRs
I read each ADR against the diff. The local mirror was fresh (scripts/adr-freshness.sh exited 0).
- ADR-005 (delivery mode): a warm read follows the namespace's delivery mode. A
HEADnever redirects. A cold fullGETstreams under both modes, as S16's standalone route does. - ADR-008 (content-addressable storage): the fill carries the expected digest. A mismatch caches nothing and asks no lower-priority upstream.
- ADR-009 (API design):
GETandHEADon the blob route are the verbs ADR-009 lists for every kind. - ADR-013 (storage backend): warm reads go through the storage layer's
OpenBlob. - ADR-021 (authorization): the addressed virtual repository's
read_artifactrole governs the read. The handler adds no check per upstream. - ADR-023 (code structure): the new files stay flat in
internal/format/ociand import no sibling format package. - ADR-001, ADR-004, ADR-007, ADR-012: I found no conflict.
Testing
Spec coverage
| Criterion (plan Step 16) | Test |
|---|---|
| The list read uses the finder's id, on a fixture whose two ids differ | TestVirtualBlobHandler_KeysTheListReadByTheFindersID, TestResolveVirtualRepository_ReturnsTheFindersIDAndWritesNothing, TestWireOCIWithStore_VirtualBlobRouteResolvesTheVirtualRepositorysUpstreams |
A missing container_virtual_repositories row answers 500 |
TestVirtualBlobHandler_AMissingVirtualRowAnswers500BeforeAnyResolution, TestResolveVirtualRepository_AMissingVirtualRowAnswers500, TestWireOCIWithStore_AVirtualBlobRouteWithNoVirtualRowAnswers500 |
A departed client answers 499 with no Error record |
TestResolveVirtualRepository_AFinderErrorForADepartedClientAnswers499, TestVirtualBlobHandler_AnAbandonedResolutionAnswers499 |
| A warm digest pull makes no upstream call | TestWireOCIWithStore_AWarmVirtualBlobPullMakesNoUpstreamRequest |
A malformed digest answers 400 DIGEST_INVALID before resolution |
TestVirtualBlobHandler_DigestGateAnswersBeforeAnyResolution |
A cold full GET fills and streams under both delivery modes |
TestVirtualBlobHandler_Delivery (the two "cold full GET streams" rows) |
A cold HEAD and a cold ranged GET drain, then answer from the row; ranged GET under redirect answers 307 |
TestVirtualBlobHandler_Delivery (the "cold HEAD" and "cold ranged GET" rows) |
A Range on a HEAD answers 200 with the full headers, never 206 |
TestVirtualBlobHandler_Delivery (the warm and cold HEAD with Range rows) |
| A ranged request on a stored blob follows the delivery mode | TestVirtualBlobHandler_Delivery (the two "warm remote range" rows) |
| A committed read closes its copy unread and answers from the row | TestVirtualBlobHandler_Delivery (the four "committed-read" rows), TestWireOCIWithStore_CommittedReadVirtualFollowersAreAnsweredFromTheRow |
A cold fill longer than server.timeouts.write completes and writes its row |
TestWireOCIWithStore_AColdVirtualFillOutlivingTheWriteTimeoutWritesItsCacheRow, TestVirtualBlobHandler_AResolutionOutlivingTheWriteTimeoutStillDeliversTheWholeBody |
A winner's digest mismatch answers 503 with a handler Retry-After, caches nothing, and asks no lower upstream; a streamed GET is cut short |
TestWireOCIWithStore_AVirtualWinnersDigestMismatchServesNothingInItsPlace, TestVirtualBlobHandler_ADrainedDigestMismatchAnswers503UnderAHandlerRetryAfter, TestVirtualBlobHandler_AStreamedDigestMismatchCutsTheBodyShort |
A relayed 429 keeps the upstream's own Retry-After |
TestVirtualBlobHandler_MapsAFailedResolutionThroughTheBlobPolicy |
Distinct cold blobs fill in parallel up to max_concurrent_fills_per_repo |
TestWireOCIWithStore_DistinctColdVirtualBlobsFillInParallelUpToTheCap |
Two virtuals and a direct read of their remote issue one upstream GET and one cache write |
TestWireOCIWithStore_ColdBlobReadsAcrossVirtualsAndTheirRemoteIssueOneUpstreamGET |
Concurrent reads of one cold blob issue one upstream GET |
TestWireOCIWithStore_ConcurrentColdVirtualReadsOfOneBlobIssueOneUpstreamGET |
The route arms the resolution budget, then re-arms from the instant Resolve returns |
TestVirtualBlobHandler_ArmsTheResolutionBudgetThenReArmsToTheTail, TestVirtualBlobHandler_ReArmsFromTheInstantResolveReturns |
A ServerWriteTimeout above the tail sets the re-arm; one below leaves the tail |
TestVirtualBlobHandler_FloorsTheTailAtTheServerWriteTimeout |
| A re-arm that fails keeps the first arm's instant, which the connection still holds | TestVirtualBlobHandler_RearmKeepsTheFirstInstant |
A failed first arm skips the re-arm and logs the virtual route's own message, with no remote_repository_id |
TestVirtualBlobHandler_SkipsTheReArmWhereTheFirstArmFailed |
A stream cut after the re-armed deadline logs msgVirtualStreamDeadline at Error, with a write error that is not a net.Error timeout; copyStored classifies against the same instant |
TestVirtualServe_ClassifiesAFailedStreamAgainstTheArmedDeadline |
| The builder reads each of the five budget terms from its own config field | TestOCIVirtualBlobBudget_ReadsEachTermFromItsOwnField, TestOCIRemoteWiringFrom_CarriesTheClientAndBothBudgetsToTheirOwnFields |
| The download is booked on the supplying position | TestVirtualBlobHandler_RecordsTheDownloadOnTheSupplyingPosition |
TestVirtualBlobHandler_AStreamFailureBetweenTheTwoArmsIsNotADeadlineKill covers the hand-off between the two arms in one direction only. I accepted that gap.
Dropped subtests
Step 15's manifest suite has not landed, so this suite has no Step 15 suite to diff against. When Step 15 lands, it runs that diff against this suite.
Against the standalone remote_blob_test.go suite, this suite leaves out the criteria that Step 12's virtual_serve_test.go already covers on main. Those are range handling (TestVirtualServe_BlobRanges), stored-copy and mid-stream failures (TestVirtualServe_StoredCopyFailures, TestVirtualServe_StreamedBlobFailureInterruptsResponse, TestVirtualServe_RefusedStreamIsNotFramedAsCompleteToTheClient), and client hangup (TestVirtualServe_ClientHangupIsNotAborted, TestVirtualServe_ClientLeavingDuringAFailedCallIsNotInternal). Upstream status relays are covered by TestVirtualBlobHandler_MapsAFailedResolutionThroughTheBlobPolicy.
The MR removes TestWireOCIWithStore_VirtualBlobRouteAnswersItsInterim501, because the route no longer answers 501. TestWireOCIWithStore_AVirtualBlobRouteWithNoUpstreamsAnswersBlobUnknown covers the route in its place.
Local runs
- Build,
go vet(with and without-tags=integration), and the unit tests ininternal/format/oci,cmd/artifact-registry,internal/virtual, andinternal/configpass. - The OCI wiring integration tests and the datastore
ContainerVirtualintegration tests pass. golangci-lintreports 0 issues on the changed packages in plain mode. With--build-tags=integrationit reports 0 issues in this MR's files. The 11 it reports are in npm, maven, and dispatcher integration tests that this MR does not touch.- The run recipe's
driver.sh smokedid not run: this machine gets a quay.io401forminio/minio.
Conformance
The conformance:oci:* jobs run on this pipeline, because the diff touches internal/format/oci. The suite pushes and pulls against a hosted repository. So it guards the shared serve code against a regression, but it does not drive the virtual route.
Size
The diff changes 2,884 lines: source 345, tests 2,433, docs and config 30, run recipe 76. That is past the 500-line guideline.
A split would not help. The write-deadline arming must land with the route, because the route cannot cache a cold layer without it. The tests ship with the code they cover.
The tests are about twice the plan's ~1,120-line estimate. The estimate dates from the plan's first draft. Three criteria arrived later: the finder criteria (Step 4a), the server.timeouts.write floor (!2941 (merged)), and the committed-read follower. The delivery table also crosses cache state, method, Range, delivery mode, and committed read.
| File | Lines |
|---|---|
internal/format/oci/virtual_blob_test.go |
1,448 |
cmd/artifact-registry/wire_oci_virtual_blob_integration_test.go |
693 |
internal/format/oci/virtual_repository_test.go |
129 |
cmd/artifact-registry/wire_oci_virtual_blob_test.go |
125 |
cmd/artifact-registry/wire_oci_virtual_integration_test.go (the interim-501 test removed) |
34 |
cmd/artifact-registry/wire_oci_remote_test.go |
4 |
Notes for reviewers and later steps
- Plan Files entry: it says "Their test files (Create)".
wire_oci_remote_test.goandwire_oci_virtual_integration_test.goare modified instead. - Run recipe: the recipe moves with the route, and the smoke count stays at 80. Three blob checks replace the three interim-
501blob checks. Some edits fall outside Step 16's anchor, and the Step 15 author will meet them:- The smoke summary in
SKILL.mdnow names the manifest route's interim answer on its own. - The seed paragraph, the manifest-route section, and the
501troubleshooting row now name only the manifest route. - In
driver.sh, theseed-container-virtualhint and the comments oncheck_container_virtual_manifestand the smoke block now name only the manifest route.
- The smoke summary in
- The token cache has a window where a cancelled leader affects its followers. #1453 (closed) tracks it.
- e2e catalog: no scenario changes here. Step 18 owns the container virtual e2e catalog, and
docs/testing/e2e/oci.mdkeeps virtual client journeys out of scope until then. - User docs: no change.
docs/user/says a Docker or OCI virtual repository accepts no upstreams yet, and that stays true. No route adds an upstream to one, so no customer can reach this route with an upstream. The end-of-plandocs-authorpass covers S32.
Related to #291 (closed)