build: Add per-step CPU and memory resource limits#3876
Conversation
crazy-max
left a comment
There was a problem hiding this comment.
Thanks but I don't think Buildx should expose this as seven new top-level build flags and seven independent bake fields.
Can we make the Buildx API a single extensible resource collection instead? For CLI, I think --resource fits better with existing repeatable buildx flags:
--resource=memory=2g --resource=memory-swap=4g --resource=cpu-quota=50000
For bake, I think this should mirror that as one target field, like:
resources = ["memory=2g", "memory-swap=4g", "cpu-quota=50000"]That keeps the user-facing API smaller, matches the existing repeatable key/value style, and avoids adding a new flag and bake field every time BuildKit grows another resource knob.
I also don't think we should add these to Compose x-bake. New Compose build fields should be added upstream in https://github.com/compose-spec/compose-spec first and implemented in https://github.com/compose-spec/compose-go, then Buildx can consume them from the normal Compose build model. Expanding x-bake for new fields moves the schema in the wrong place and we plan to deprecate it: #3025
Thanks for the review. So the existing: flags.StringVarP(&ignore, "memory", "m", "", "Memory limit")
flags.MarkHidden("memory")
flags.StringVar(&ignore, "memory-swap", "", `Swap limit equal to memory plus swap: "-1" to enable unlimited swap`)
flags.MarkHidden("memory-swap")
flags.Int64VarP(&ignoreInt, "cpu-shares", "c", 0, "CPU shares (relative weight)")
flags.MarkHidden("cpu-shares")
flags.Int64Var(&ignoreInt, "cpu-period", 0, "Limit the CPU CFS (Completely Fair Scheduler) period")
flags.MarkHidden("cpu-period")
flags.Int64Var(&ignoreInt, "cpu-quota", 0, "Limit the CPU CFS (Completely Fair Scheduler) quota")
flags.MarkHidden("cpu-quota")
flags.StringVar(&ignore, "cpuset-cpus", "", `CPUs in which to allow execution ("0-3", "0,1")`)
flags.MarkHidden("cpuset-cpus")
flags.StringVar(&ignore, "cpuset-mems", "", `MEMs in which to allow execution ("0-3", "0,1")`)
flags.MarkHidden("cpuset-mems")should stay there, right? or do we want to remove it as well? |
7be227d to
f3d3173
Compare
|
I think we want to keep the existing flags (and connect them with For the bake format, I guess it should be a struct (or double format like the cache and output options). |
|
@jirimoravcik Sorry if it was not clear but yes we can keep the existing flags as @tonistiigi explained above. |
e92893f to
2f56a6b
Compare
| CPUSetMems *string `json:"cpuset-mems,omitempty"` | ||
| } | ||
|
|
||
| func (r *ResourcesConfig) Merge(other *ResourcesConfig) *ResourcesConfig { |
There was a problem hiding this comment.
This should avoid mutating either input and shouldn't return other directly.
Target.Merge uses this while resolving inherited targets. If a base target has resources, the current Merge can share the base target's *ResourcesConfig pointer with the resolved child. If the child also sets one resource field, r.Merge(other) mutates that shared config, so the child value can leak into the base target or into another target inheriting the same base.
Can we make this return a fresh ResourcesConfig (or clone before merging), and add a test with two targets inheriting the same base where only one child overrides a resource field?
There was a problem hiding this comment.
It now returns a fresh ResourcesConfig. Added a test (fails with the old logic, passes with the new one)
Port of moby/buildkit#6569 to buildx. Adds --memory, --memory-swap, --cpu-shares, --cpu-period, --cpu-quota, --cpuset-cpus, and --cpuset-mems flags to build, plus the equivalent bake target attributes and compose x-bake fields. Signed-off-by: Jiří Moravčík <jiri.moravcik@gmail.com>
2f56a6b to
80b2293
Compare
| } else { | ||
| t.Ulimits = o.ArrValue | ||
| } | ||
| case "resources": |
There was a problem hiding this comment.
The override keys seem to be undocumented in docs/reference/bake
| fmt.Fprintf(tw, "Resource Limits:\t%s\n", out.Config.Ulimit) | ||
| } | ||
| if out.Config.Memory != "" { | ||
| fmt.Fprintf(tw, "Memory:\t%s\n", out.Config.Memory) |
There was a problem hiding this comment.
These fields should have friendlier printing than just big integer.
| if len(keys) != 3 { | ||
| return nil, errors.Errorf("invalid key %s, resources requires name", parts[0]) | ||
| } | ||
| if len(parts) == 2 { |
There was a problem hiding this comment.
nit: this is already guaranteed by the len check in L582
Port of moby/buildkit#6569 to buildx. Adds
--resourcerow with things like--memory,--memory-swap,--cpu-shares,--cpu-period,--cpu-quota,--cpuset-cpus, and--cpuset-memsto build, plus the equivalent bake target attributes