[v0.13] create --append with missing builder changed from warn to error

dixneuf19 commented 3 months ago

Contributing guidelines

[X] I've read the contributing guidelines and wholeheartedly agree

I've found a bug and checked that ...

[X] ... the documentation does not mention anything about my problem
[X] ... there are no open or closed issues that are related to my problem

Description

Hi, We are extensively using buildx with remote buildkit nodes, notably for its speed and scheduling of build on native CPU architectures (arm64 build on arm64 VMs, same for amd64)

Before the version 0.13.0, we used the following syntax in our CI/CD pipelines

docker buildx create \
  --append \
  --name buildkit \
  --driver remote \
  --driver-opt cacert=/certs/ca.pem,cert=/certs/cert.pem,key=/certs/key.pem \
  --platform arm64 \
  tcp://buildkit-arm64.buildkit.svc.cluster.local:1234 ;

  docker buildx create \ 
  --append \
  --name buildkit \
  --driver remote \
  --driver-opt cacert=/certs/ca.pem,cert=/certs/cert.pem,key=/certs/key.pem \
  --platform amd64 \
  tcp://buildkit-amd64.buildkit.svc.cluster.local:1234 ;

 docker buildx build \ 
  --platform amd64,arm64 \
...

Note that we use the same syntax docker buildx create --append both time. This is not well documentend but worked nicely, with a warning for the first command

WARNING: failed to find "buildkit" for append, creating a new instance instead buildkit

With the 0.13.0 release, this behavious was broken, with the following error

ERROR: failed to find instance "buildkit" for append

It seems that this is linked to a refactoring of the create command on this commit (@crazy-max )

Since buildx is automatically shipped with docker, the 0.13.0 release was unknowingly shipped with a daily rebuild of docker:24-cli and broke our CI/CD.

While this is an undocumented feature, and we might want to explicitely fail on this usage, I find the usage of an idempotent command to add node to a builder very useful.

Indeed, in our case, we dynamically generate the CI depending on how many CPU archs we want to build, either 1 or more. So we would be forced to implement a logic like "if this is the first command, do not put --append, otherwise do. While this is doable, idempotent command are truly useful. Imagine a world without kubectl apply but only kubectl create and kubectl patch !

If you agree with my proposal, I might draft a PR, seems it looks not that complicated to reintroduce this logic. What do you think ?

Expected behaviour

docker buildx create \
  --append \
  --name buildkit \
  --driver remote \
  --driver-opt cacert=/certs/ca.pem,cert=/certs/cert.pem,key=/certs/key.pem \
  --platform arm64 \
  tcp://buildkit-arm64.buildkit.svc.cluster.local:1234 ;
  WARNING: failed to find "buildkit" for append, creating a new instance instead buildkit

  docker buildx create \ 
  --append \
  --name buildkit \
  --driver remote \
  --driver-opt cacert=/certs/ca.pem,cert=/certs/cert.pem,key=/certs/key.pem \
  --platform amd64 \
  tcp://buildkit-amd64.buildkit.svc.cluster.local:1234 ;

Actual behaviour

docker buildx create \
  --append \
  --name buildkit \
  --driver remote \
  --driver-opt cacert=/certs/ca.pem,cert=/certs/cert.pem,key=/certs/key.pem \
  --platform arm64 \
  tcp://buildkit-arm64.buildkit.svc.cluster.local:1234 ;
ERROR: failed to find instance "buildkit" for append

Buildx version

v0.13.0

Docker info

No response

Builders list

N/A

Configuration

FROM alpine

Build logs

No response

Additional info

No response

dixneuf19 commented 3 months ago

https://github.com/docker/buildx/issues/2346 seems to be linked

crazy-max commented 3 months ago

Yes we bail out early since v0.13 as the instance should already exists when appending. This was changed specifically for programmatic invocations of builder creation.

tonistiigi commented 3 months ago

I think as a default, erroring behavior makes sense. The user is asking to append a node to a named builder that does not exist. We might consider adding some way to signal the "append-or-create" mode, but not sure what way exactly this could be passed.

dixneuf19 commented 3 months ago

Ok thanks at least the behaviour is now explicit. However having an idempotent command would still be useful.

So maybe something like

docker buildx create \
  --append-or-create \
  --name buildkit \

or

docker buildx create \
  --append --create-if-not-exists \
  --name buildkit \

I am not very good at naming things, sorry :sweat_smile:

crazy-max commented 4 weeks ago

Ok thanks at least the behaviour is now explicit. However having an idempotent command would still be useful.

So maybe something like
docker buildx create \
  --append-or-create \
  --name buildkit \
or
docker buildx create \
  --append --create-if-not-exists \
  --name buildkit \
I am not very good at naming things, sorry 😅

I think we could add a new flag --create-if-missing that communicates the intent to create the builder if it doesn't exist or short one --ensure indicating that the builder should be ensured to exist.

docker / buildx