Revert "build: rework node resolution"

[tonistiigi]

This reverts commit 616fb3e.

#2115 #1966

Regression report:

cameron@Cameron-M2 ~/test/docker-buildx $ docker buildx bake "https://github.com/c-ameron/docker-remote-test.git" --file docker-bake.hcl --progress plain --no-cache --load
#0 building with "nice_sammet" instance using docker-container driver

#1 [internal] load git source https://github.com/c-ameron/docker-remote-test.git
#1 0.349 ref: refs/heads/main	HEAD
#1 0.357 816fb52ee3868adb43b68729250451f76d41c0d7	HEAD
#1 0.748 816fb52ee3868adb43b68729250451f76d41c0d7	refs/heads/main
#1 CACHED

#1 [internal] load git source https://github.com/c-ameron/docker-remote-test.git
#1 0.348 ref: refs/heads/main	HEAD
#1 0.354 816fb52ee3868adb43b68729250451f76d41c0d7	HEAD
#1 0.738 816fb52ee3868adb43b68729250451f76d41c0d7	refs/heads/main
#1 CACHED

#2 [context local] load .dockerignore
#2 transferring local: 2B 0.0s done
#2 DONE 0.0s

#3 [auth] library/ubuntu:pull token for registry-1.docker.io
#3 DONE 0.0s

#4 [internal] load metadata for docker.io/library/ubuntu:latest
#4 ...

#5 [context alpine] load metadata for ruby:3.1-alpine
#5 ERROR: no match for platform in manifest sha256:893c539660aaee75daaa47f4412356d3e9850d0fdfc49084fb98348317b3a0ce: not found

#4 [internal] load metadata for docker.io/library/ubuntu:latest
#4 CANCELED
------
 > [context alpine] load metadata for ruby:3.1-alpine:
------
Dockerfile:4
--------------------
   2 |     COPY --from=local bar /bar
   3 |
   4 | >>> FROM alpine as runtime
   5 |     COPY --from=base /bar /bar
   6 |     COPY --from=local foo /foo
--------------------
ERROR: failed to solve: no match for platform in manifest sha256:893c539660aaee75daaa47f4412356d3e9850d0fdfc49084fb98348317b3a0ce: not found
cameron@Cameron-M2 ~/test/docker-buildx $ docker buildx version
github.com/docker/buildx v0.12.0-rc2 d537b9e418b909b65274a902e25b3d1757553a3b

The PR adds an invalid platform in this case, when user specified none. In SolveOpt:

2023/11/16 13:11:32 solveopt: client.SolveOpt{Exports:[]client.ExportEntry{client.ExportEntry{Type:"moby", Attrs:map[string]string{"name":"docker.io/username/webapp:latest"}, Output:(func(map[string]string) (io.WriteCloser, error))(nil), OutputDir:""}}, LocalDirs:map[string]string{"local":"."}, LocalMounts:map[string]fsutil.FS(nil), OCIStores:map[string]content.Store(nil), SharedKey:"", Frontend:"", FrontendAttrs:map[string]string{"context":"https://github.com/c-ameron/docker-remote-test.git", "context:alpine":"docker-image://ruby:3.1-alpine", "context:local":"local:local", "filename":"Dockerfile", "frontend.caps":"moby.buildkit.frontend.contexts+forward", "image-resolve-mode":"local", "no-cache":"", "platform":"darwin/arm64/v8"}, FrontendInputs:map[string]llb.State(nil), CacheExports:[]client.CacheOptionsEntry{}, CacheImports:[]client.CacheOptionsEntry{}, Session:[]session.Attachable{(*authprovider.authProvider)(0x1400080dec0), (*secretsprovider.secretProvider)(0x14000477170), (*sshprovider.socketProvider)(0x14000888008)}, AllowedEntitlements:[]entitlements.Entitlement(nil), SharedSession:(*session.Session)(nil), SessionPreInitialized:false, Internal:false, SourcePolicy:(*moby_buildkit_v1_sourcepolicy.Policy)(nil), Ref:"wbn9zqi6crwga3h64ra91m4wx"}


2023/11/16 13:14:01 solveopt client.SolveOpt{Exports:[]client.ExportEntry{client.ExportEntry{Type:"moby", Attrs:map[string]string{"name":"docker.io/username/webapp:latest"}, Output:(func(map[string]string) (io.WriteCloser, error))(nil), OutputDir:""}}, LocalDirs:map[string]string{"local":"."}, OCIStores:map[string]content.Store(nil), SharedKey:"", Frontend:"", FrontendAttrs:map[string]string{"context":"https://github.com/c-ameron/docker-remote-test.git", "context:alpine":"docker-image://ruby:3.1-alpine", "context:local":"local:local", "filename":"Dockerfile", "frontend.caps":"moby.buildkit.frontend.contexts+forward", "image-resolve-mode":"local", "no-cache":""}, FrontendInputs:map[string]llb.State(nil), CacheExports:[]client.CacheOptionsEntry{}, CacheImports:[]client.CacheOptionsEntry{}, Session:[]session.Attachable{(*authprovider.authProvider)(0x14000750e00), (*secretsprovider.secretProvider)(0x140003c60b0), (*sshprovider.socketProvider)(0x1400034e730)}, AllowedEntitlements:[]entitlements.Entitlement(nil), SharedSession:(*session.Session)(nil), SessionPreInitialized:false, Internal:false, SourcePolicy:(*moby_buildkit_v1_sourcepolicy.Policy)(nil), Ref:"pjo1tz1cfjrp6jjyyg6vxjlqa"}

Note ^ "platform":"darwin/arm64/v8".

We also should look at this patch critically from readability. It isn't entirely obvious to me why there is a second map with same keys driversSolveOpts making it hard to track what is actually going on.

[crazy-max]

Note ^ "platform":"darwin/arm64/v8".

Hum platforms.DefaultSpec() looks to be passed where it should be inferred from the node where the build occurs.

We also should look at this patch critically from readability. It isn't entirely obvious to me why there is a second map with same keys driversSolveOpts making it hard to track what is actually going on.

👍

[jedevc]

Hum platforms.DefaultSpec() looks to be passed where it should be inferred from the node where the build occurs.

Instead of reverting, can this not just be removed?

It isn't entirely obvious to me why there is a second map with same keys driversSolveOpts making it hard to track what is actually going on

I think this is significantly easier to understand than the previous iteration - this was part of the driverPair.so, and was being set as part of the call to Build (and wasn't actually being accessed anywhere else outside of that), which mixed concerns into a single strict whose general purpose was unclear.

I'd personally prefer a fix instead of a full revert, since I'd like to keep the changes to the ordered resolution (and the tests which verify how the platform resolution actually works), but since I'm on holiday I'm not actually able to write a fix for that right now, so won't block on a revert.

[tonistiigi]

can this not just be removed?

I don't think @crazy-max is referring to a specific line. This is a side-effect of the new driver resolver where it seems to set platforms (for the build request) in cases where they should not be set (because the user did not set them). I don't know where the darwin part is coming from but in that example no platform should be set at all.

this was part of the driverPair.so, and was being set as part of the call to Build (and wasn't actually being accessed anywhere else outside of that), which mixed concerns into a single strict whose general purpose was unclear.

Maybe driverPair is not a best name but holding the pointer to specific implementation and specific request that have strict 1-1 mapping together in the same struct and passing them around together looks much safer than indexing them in two separate maps and passing multiple maps around where implementations need to do multiple string based lookups and hope that everything is synchronized and there isn't any mismatch.

[jedevc]

I think this is the line causing the DefaultSpec issue. It should not be set here if the default case with no specified platforms from above was bit.

[jedevc]

passing them around together

I don't think they were being passed around together. They were only being set and accessed in BuildWithResultHandler, which made it confusing to have an extra property on this struct, when it was only filled in for that case. Would 100% agree if we were actually passing it around, but it's only within this function, so I think it's better to keep it separate (but I'd be happy to refactor to put it back, if we decide to go keep the patch in).

[tonistiigi]

Double lookups:

https://github.com/docker/buildx/pull/2124/files#diff-7fdd319c657fbde54b83a5dfa233c5c2c417b291739ff75c05dc5d83ee7a9d24L1306
https://github.com/docker/buildx/pull/2124/files#diff-7fdd319c657fbde54b83a5dfa233c5c2c417b291739ff75c05dc5d83ee7a9d24L583-L585

[jedevc]

Double lookups:

https://github.com/docker/buildx/pull/2124/files#diff-7fdd319c657fbde54b83a5dfa233c5c2c417b291739ff75c05dc5d83ee7a9d24L1306 https://github.com/docker/buildx/pull/2124/files#diff-7fdd319c657fbde54b83a5dfa233c5c2c417b291739ff75c05dc5d83ee7a9d24L583-L585

Fair enough, I can see the argument for putting the solve opt back into the struct - the original PR already does the work to rename driverPair to resolvedNode, so it would just be a matter of updating that.

[tonistiigi]

We need to do something with this. The master branch is completely broken in darwin atm for example.