pkg/compose: use negotiated API version for network setup#13690
pkg/compose: use negotiated API version for network setup#13690
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates Compose’s API-version plumbing to distinguish between the daemon’s raw API version and the client’s negotiated/effective API version, and applies the negotiated version to request-shaping in the container create/network setup path.
Changes:
- Add per-
composeServicecaching for both raw daemon API version (RuntimeVersion) and negotiated client API version (CurrentAPIVersion). - Switch container create networking config to use
CurrentAPIVersion()(negotiated) instead ofRuntimeVersion()(raw server). - Update internal helpers to use pointer receivers (avoids copying newly-added per-service caches) and extend tests to cover negotiated API version caching.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/compose/compose.go | Introduces per-service version caches and adds CurrentAPIVersion() alongside RuntimeVersion(). |
| pkg/compose/create.go | Uses negotiated API version for network request shaping; clarifies when raw daemon version is required for capability checks. |
| pkg/compose/images.go | Clarifies use of raw daemon version for capability checks (ImageInspect platform handling). |
| pkg/compose/hook.go | Switches helpers to pointer receivers to avoid copying service state/caches. |
| pkg/compose/build_bake.go | Switches dry-run bake helpers to pointer receivers to avoid copying service state/caches. |
| pkg/compose/convergence_test.go | Updates create-path mocks to use API negotiation and adds a test validating per-service negotiation caching. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1eedd3e to
cd04968
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
6a527d4 to
ac0d46d
Compare
ac0d46d to
232b8f2
Compare
|
Damn you, tests! 😂 #18 54.92 === RUN TestImages
#18 54.92 compose.go:536: Unexpected call to *mocks.MockAPIClient.Ping([context.Background.WithCancel {true false}]) at /src/pkg/compose/compose.go:536 because: there are no expected calls of the method "Ping" for that receiver
#18 54.92 controller.go:251: missing call(s) to *mocks.MockAPIClient.ImageInspect(is assignable to , is equal to bar:2 (string)) /src/pkg/compose/images_test.go:52
#18 54.92 controller.go:251: aborting test due to missing call(s)
#18 54.92 --- FAIL: TestImages (0.00s) |
232b8f2 to
3174600
Compare
pkg/compose/compose.go
Outdated
| // RuntimeVersion returns the raw API version reported by the daemon. | ||
| // Callers that need the negotiated/effective client API version should use | ||
| // CurrentAPIVersion instead. | ||
| func (s *composeService) RuntimeVersion(ctx context.Context) (string, error) { |
There was a problem hiding this comment.
We should only rely on CurrentAPIVersion, as after API has been negociated, if for some reason uses a version < runtime version, compose should never rely on features or request attributes not defined by this API version.
I suggest you merge both functions into RuntimeAPIVersion and use it everywhere we depend on Engine's features
There was a problem hiding this comment.
Indeed, I'll do it right now
Move runtimeVersionCache from a package-level var to per-instance fields on composeService and add CurrentAPIVersion() that negotiates via Ping before returning the client version. Switch getCreateConfigs and buildContainerVolumes to use CurrentAPIVersion so that version-gated request shaping matches what the daemon actually validates against (the negotiated API version from the request path, not the server's max capability). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Guillaume Lours <[email protected]>
…fields Moving runtimeVersionCache from a package-level var to instance fields on composeService caused copylocks violations in methods using value receivers, since sync.Once contains sync.noCopy. Switch the 4 affected methods to pointer receivers. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Guillaume Lours <[email protected]>
Replace sync.Once with sync.Mutex so that only successful version lookups are cached. Errors (context cancellation, network blips) are returned without caching, allowing subsequent calls to retry. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Guillaume Lours <[email protected]>
…ersion After API negotiation, Compose should only rely on the negotiated version and never use the daemon's raw max version for request shaping. Merge both functions into a single RuntimeAPIVersion that negotiates via Ping and returns ClientVersion, erroring if the client reports an empty version instead of silently falling back to ServerVersion. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> Signed-off-by: Guillaume Lours <[email protected]>
3174600 to
1059a7c
Compare
What I did
This branch changes the API-version plumbing more generally, even if the immediate consumer is the container create path. A better short PR description would be:
This PR makes Compose distinguish between the raw daemon API version and the negotiated client API version.
It adds per-service caching for both values on composeService, introduces CurrentAPIVersion() for callers that need the effective negotiated version, and keeps RuntimeVersion() for raw daemon capability checks.
This avoids using ServerVersion().APIVersion in places where request shaping should follow the client’s negotiated API level.
Targeted tests were added to cover the create path and to verify negotiated API lookup is cached per service instance.
Related issue
N/A
(not mandatory) A picture of a cute animal, if possible in relation to what you did
