buildx icon indicating copy to clipboard operation
buildx copied to clipboard

proposal: Remove buildx build resource limits arguments

Open Aposhian opened this issue 4 years ago • 9 comments

Problem

buildx build accepts resource limit arguments for --memory, --cpu-shares, etc., but these are not currently being used in the code as noted in #292 #483. Furthermore, it may not make sense to have these options for the generic buildx build command, since buildx can use builders that work in parallel and across multiple machines, which muddies the semantics of a simple option like --memory. This proposal is not intended to outline the future of resource limits in buildx/buildkit, only to clarify the current arguments and documentation to better match the current capabilities and behavior of buildx.

Proposal

  • [ ] Add a warning message whenever resource limit arguments are used on buildx build so that it is clear that they are being ignored. This preserves backward compatibility for people who use buildx install, but also does not mislead.
  • [ ] Remove resource limit arguments from the documentation

Aposhian avatar Jun 24 '21 16:06 Aposhian

I agree that the current approach is VERY misleading.

felipecrs avatar Feb 16 '22 23:02 felipecrs

Can we get some feedback on this issue?

ayk33 avatar May 26 '22 17:05 ayk33

These flags do not appear in build --help. The only way to hit them is with old build scripts and as they do not change the outcome of the build there is no point to provide error for them.

Showing a warning when flag is set to custom value should be ok, but when we do decide to support the flag it would show an incorrect warning although the flag worked. Since this issue was created, we have already brought back a bunch of these flags.

tonistiigi avatar May 27 '22 15:05 tonistiigi

@tonistiigi what do you mean they do not change the outcome? They for sure do depending on whatever happens during docker build:

https://github.com/felipecrs/oracle-docker-images/blob/main/OracleDatabase/SingleInstance/extensions/prebuiltdb/README.md#:~:text=If%20you%20know,memory%20option.

So, still, I restate that the current approach is very misleading as it produces a different outcome without the user knowing. A warning is better than nothing, but I still believe an error should be the correct.

felipecrs avatar May 27 '22 15:05 felipecrs

do not change the outcome

I meant the build result.

If you run commands that only work with certain configuration passed into them, then you shouldn't rely on the user passing in the correct value. Your build step could detect if there are not enough resources or use ulimit to limit itself. From the link you specified, doesn't that case actually use the 4G as a configuration variable that could be passed in with a --build-arg instead?

Regarding this specific case, setting memory limits for a single container is not a problem for buildkit. But this is not how most users would understand that flag. Instead they would think that this flag means limits for the whole build. In that case instead of passing a value with build request it makes more sense to add limits to the buildkit container or use the --cgroup-parent with limits that apply to all containers as a whole.

tonistiigi avatar May 27 '22 15:05 tonistiigi

From the link you specified, doesn't that case actually use the 4G as a configuration variable that could be passed in with a --build-arg instead?

No, because it's automatically calculated based on the amount of memory allocated for the container.

felipecrs avatar May 27 '22 15:05 felipecrs

No, because it's automatically calculated based on the amount of memory allocated for the container.

That's what I meant. Instead of setting the value directly it expects the value to be set as a container limit and then inside the container, it goes in and reads what the limit was set to("automatic calculation").

tonistiigi avatar May 27 '22 16:05 tonistiigi