Repository navigation
Conversation
Signed-off-by: Max Proske <max@mproske.com>
ndeloof
left a comment
ndeloof
left a comment
There was a problem hiding this comment.
A few comments on the tests, the documentation and the wording around linked targets. No blocking issue from my side on the approach.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| bake.jobs = bakeJobs(bake.cfg, s.maxConcurrency, buildx.Version) |
There was a problem hiding this comment.
we indeed can reuse compose's --parallel flag in this context, whenever it's original goal is not to restrict the build but concurrent API calls.
| if options.Builder != "" { | ||
| args = append(args, "--builder", options.Builder) | ||
| } | ||
| if bake.jobs > 0 { |
There was a problem hiding this comment.
Nothing pins that bakeArgs emits --jobs N: TestBakeJobs only covers bakeJobs. A small assertion in build_bake_test.go (a bakeBuild{jobs: 2} yields --jobs 2, and jobs: 0 yields no --jobs) would cover the wiring. The e2e suite would only exercise it if CI's buildx is >= 0.38.
| {name: "limited, link not built", cfg: group("base", "db", "web"), maxConcurrency: 2, buildx: "v0.38.0", want: 2}, | ||
| {name: "limit holds no target back", cfg: group("base", "db"), maxConcurrency: 2, buildx: "v0.38.0"}, | ||
| {name: "buildx too old", cfg: group("base", "db", "web"), maxConcurrency: 2, buildx: "v0.37.2"}, | ||
| {name: "linked targets", cfg: group("app", "base", "db"), maxConcurrency: 2, buildx: "v0.38.0"}, |
There was a problem hiding this comment.
The linked case is only covered for a limit lower than the number of targets. Worth adding the cases where a target: link exists and the limit is not lower than the participating targets, including a link to a target outside the default group (like app → base here, with app the only one built next to db): they must never produce --jobs, and that is currently only guaranteed indirectly by the early maxConcurrency >= len(targets) return.
| for _, target := range targets { | ||
| for _, source := range cfg.Targets[target].Contexts { | ||
| if strings.HasPrefix(source, "target:") { | ||
| logrus.Warn("limiting build concurrency is not supported with service additional_contexts, ignoring parallel limit") |
There was a problem hiding this comment.
For projects using additional_contexts: service:… this makes the limit a no-op (with a warning), and those multi-image builds are a typical case for throttling to avoid OOM, so #14313 is only partly addressed. That is a reasonable first step given buildx's constraint, but the PR description and the docs should say so explicitly.
| return args | ||
| } | ||
|
|
||
| // bakeJobs returns how many targets bake can build concurrently |
There was a problem hiding this comment.
Docs: the "Configuring parallelism" section of docs/reference/compose.md describes --parallel / COMPOSE_PARALLEL_LIMIT as limiting "concurrent engine calls" and says nothing about builds. Since builds now honor it too, with conditions (buildx >= 0.38, and the limit is ignored with a warning when it can't be applied to linked targets), that section should mention it. It is the generated docker_compose.yaml source that has to be updated along with it.
| logrus.Warnf("limiting build concurrency requires buildx %s or later, ignoring parallel limit", buildxJobsMinVersion) | ||
| return 0 | ||
| } | ||
| // bake rejects parallel limit when targets linked by `target:` contexts |
There was a problem hiding this comment.
I think this comment (and the warning below) conflates two things. Buildx doesn't refuse linked targets as such: it refuses a limit that is lower than the number of participating targets, implicit ones included, once targets are linked. Checked with buildx v0.38.0 on a bake file with 4 targets, one of them linked with target:: --jobs 1, 2 and 3 fail with "limited parallelism is not supported with linked targets", --jobs 4 works. Same with two grouped targets plus one implicit linked target (3 in total): --jobs 1/2 fail, --jobs 3 works.
So what compose has to check is that the limit is sufficient for the linked targets. The early maxConcurrency >= len(targets) return does it, but only for the targets of the default group, and indirectly. Could the comment and the warning state the real condition (e.g. "ignoring a parallel limit lower than the number of linked build targets"), or could the participating count (group + implicit linked targets) be computed explicitly rather than relying on that early return?
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This branch has not been deployed
2 participants
Closes #14313
Pass
--parallel/COMPOSE_PARALLEL_LIMITthrough to bake as--jobs, so builds respect the limit again. Helps throttle builds that normally OOM low memory CI runners.The docs still say it can be used, so no change needed there.