Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buildinfo: add build attributes and frontend #2476

Merged
merged 10 commits into from
Feb 14, 2022

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Nov 22, 2021

follow-up #2311

store additional build dependencies such as frontend and attrs.

# buildkit
$ docker buildx build --push --platform linux/amd64,linux/arm64 \
  --build-arg "BUILDKIT_CONTEXT_KEEP_GIT_DIR=1" \
  --tag "crazymax/buildkit:buildattrs" \
  https://github.com/crazy-max/buildkit.git#buildinfo-attrs

# frontend
$ docker buildx build --push --platform linux/amd64,linux/arm64 \
  --build-arg "CHANNEL=mainline" \
  --build-arg "BUILDKIT_CONTEXT_KEEP_GIT_DIR=1" \
  --tag "crazymax/dockerfile:buildattrs" \
  --file "./frontend/dockerfile/cmd/dockerfile-frontend/Dockerfile" \
  https://github.com/crazy-max/buildkit.git#buildinfo-attrs

# buildx to inline build attrs (opt-in atm)
$ docker buildx bake https://github.com/crazy-max/buildx.git#inline-buildattrs binaries
$ cp ./bin/buildx ~/.docker/cli-plugins/docker-buildx

Pre-built frontend # syntax=crazymax/dockerfile:buildattrs and BuildKit image crazymax/buildkit:buildattrs can be used to test this feature:

$ docker buildx create --name buildattrs --driver-opt image=crazymax/buildkit:buildattrs --use
# buildattrs enabled
$ docker buildx build --metadata-file metadata.json \
  --load --tag buildattrs:latest \
  --build-arg BUILDKIT_INLINE_BUILDINFO_ATTRS=1 \
  https://github.com/crazy-max/buildkit-buildsources-test.git#master

$ cat metadata.json | jq -r '."containerimage.buildinfo"' | base64 --decode | jq
{
  "frontend": "dockerfile.v0",
  "attrs": {
    "cmdline": "crazymax/dockerfile:buildattrs",
    "context": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
    "filename": "Dockerfile",
    "source": "crazymax/dockerfile:buildattrs"
  },
  "sources": [
    {
      "type": "docker-image",
      "ref": "docker.io/docker/buildx-bin:0.6.1@sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0",
      "pin": "sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/library/alpine:3.13",
      "pin": "sha256:026f721af4cf2843e07bba648e158fb35ecc876d822130633cc49f707f0fc88c"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/moby/buildkit:v0.9.0",
      "pin": "sha256:8dc668e7f66db1c044aadbed306020743516a94848793e0f81f94a087ee78cab"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/tonistiigi/xx@sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04",
      "pin": "sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04"
    },
    {
      "type": "git",
      "ref": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
      "pin": "43a8bf9c351abf64b0865a6a0548a1de1ded40a8"
    },
    {
      "type": "http",
      "ref": "https://raw.githubusercontent.com/moby/moby/master/README.md",
      "pin": "sha256:419455202b0ef97e480d7f8199b26a721a417818bc0e2d106975f74323f25e6c"
    }
  ]
}

$ docker image save buildattrs:latest | tar -ax --exclude 'manifest.json' --wildcards '*.json' -O | jq -r '."moby.buildkit.buildinfo.v1"' | base64 --decode | jq
{
  "frontend": "dockerfile.v0",
  "attrs": {
    "cmdline": "crazymax/dockerfile:buildattrs",
    "context": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
    "filename": "Dockerfile",
    "source": "crazymax/dockerfile:buildattrs"
  },
  "sources": [
    {
      "type": "docker-image",
      "ref": "docker.io/docker/buildx-bin:0.6.1@sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0",
      "pin": "sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/library/alpine:3.13",
      "pin": "sha256:026f721af4cf2843e07bba648e158fb35ecc876d822130633cc49f707f0fc88c"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/moby/buildkit:v0.9.0",
      "pin": "sha256:8dc668e7f66db1c044aadbed306020743516a94848793e0f81f94a087ee78cab"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/tonistiigi/xx@sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04",
      "pin": "sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04"
    },
    {
      "type": "git",
      "ref": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
      "pin": "43a8bf9c351abf64b0865a6a0548a1de1ded40a8"
    },
    {
      "type": "http",
      "ref": "https://raw.githubusercontent.com/moby/moby/master/README.md",
      "pin": "sha256:419455202b0ef97e480d7f8199b26a721a417818bc0e2d106975f74323f25e6c"
    }
  ]
}
# buildattrs disabled (default)
$ docker buildx build --metadata-file metadata.json \
  --load --tag buildattrs:latest \
  https://github.com/crazy-max/buildkit-buildsources-test.git#master

$ cat metadata.json | jq -r '."containerimage.buildinfo"' | base64 --decode | jq
{
  "frontend": "dockerfile.v0",
  "attrs": {
    "cmdline": "crazymax/dockerfile:buildattrs",
    "context": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
    "filename": "Dockerfile",
    "source": "crazymax/dockerfile:buildattrs"
  },
  "sources": [
    {
      "type": "docker-image",
      "ref": "docker.io/docker/buildx-bin:0.6.1@sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0",
      "pin": "sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/library/alpine:3.13",
      "pin": "sha256:026f721af4cf2843e07bba648e158fb35ecc876d822130633cc49f707f0fc88c"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/moby/buildkit:v0.9.0",
      "pin": "sha256:8dc668e7f66db1c044aadbed306020743516a94848793e0f81f94a087ee78cab"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/tonistiigi/xx@sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04",
      "pin": "sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04"
    },
    {
      "type": "git",
      "ref": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
      "pin": "43a8bf9c351abf64b0865a6a0548a1de1ded40a8"
    },
    {
      "type": "http",
      "ref": "https://raw.githubusercontent.com/moby/moby/master/README.md",
      "pin": "sha256:419455202b0ef97e480d7f8199b26a721a417818bc0e2d106975f74323f25e6c"
    }
  ]
}

$ docker image save buildattrs:latest | tar -ax --exclude 'manifest.json' --wildcards '*.json' -O | jq -r '."moby.buildkit.buildinfo.v1"' | base64 --decode | jq
{
  "frontend": "dockerfile.v0",
  "sources": [
    {
      "type": "docker-image",
      "ref": "docker.io/docker/buildx-bin:0.6.1@sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0",
      "pin": "sha256:a652ced4a4141977c7daaed0a074dcd9844a78d7d2615465b12f433ae6dd29f0"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/library/alpine:3.13",
      "pin": "sha256:026f721af4cf2843e07bba648e158fb35ecc876d822130633cc49f707f0fc88c"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/moby/buildkit:v0.9.0",
      "pin": "sha256:8dc668e7f66db1c044aadbed306020743516a94848793e0f81f94a087ee78cab"
    },
    {
      "type": "docker-image",
      "ref": "docker.io/tonistiigi/xx@sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04",
      "pin": "sha256:21a61be4744f6531cb5f33b0e6f40ede41fa3a1b8c82d5946178f80cc84bfc04"
    },
    {
      "type": "git",
      "ref": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
      "pin": "43a8bf9c351abf64b0865a6a0548a1de1ded40a8"
    },
    {
      "type": "http",
      "ref": "https://raw.githubusercontent.com/moby/moby/master/README.md",
      "pin": "sha256:419455202b0ef97e480d7f8199b26a721a417818bc0e2d106975f74323f25e6c"
    }
  ]
}

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should pick all build-args + known opts like context, filename etc only. If variable changes build result then it should be picked.

Need to think about what to do with frontend inputs. We should at least mark somehow that they were used. DefinitionOp should carry the buildinfo of the inputs so it could be added to the main one.

We also discussed that a client should be able to send its own data for this. Eg. bake to send its args/target. This can be follow-up as well.

@crazy-max
Copy link
Member Author

@tonistiigi I wonder where we should add the opt-in. New image output key? (e.g., buildinfo-attrs=false) or a frontend opt (e.g., buildinfo-attrs + build-arg:BUILDKIT_KEEP_BUILDINFO_ATTRS)? The later sound better.

@tonistiigi
Copy link
Member

I think opt-in should be just for the image inline. So as we currently have BUILDKIT_INLINE_CACHE we can have BUILDKIT_INLINE_BUILDINFO_ATTRS. We can also do the exporter attribute for the image exporter.

Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the special InlineBuildInfoAttrs handling. It looks out of place compared to the other exporter attrs.

I think we should just use the regular exporter attr for image exporter like we do today for the image exporter. Just the regular map like for other options.

The frontend(dockerfile) shouldn't check if attr exporting is enabled or not, just like for buildinfo. It just always sets it and if there is no opt-in then the image exporter just clears the whole attr section.

So the base opt-in will be with output option. For the build-arg opt-in we can make it in buildx like for the inline cache https://github.com/docker/buildx/blob/4c938c77bab00dbe983597b7d29f2e2eb19e8000/build/build.go#L358 .

Not for this PR but looking at the current buildinfo exporter attr buildinfo=[all,imageconfig,metadata,none], I'm not sure what the point of the buildinfo=metadata was for there. Doesn't it make more sense to have it as bool and metadata is just always enabled. What does image exporter have to do with exporting metadata.json at all? I assume that other than inlining in config this all still works when I'm not exporting an image. In that case we would have 2 exporter attr buildinfo, buildinfo-attrs . Currently by default buildinfo would default to true and buildinfo-attrs to false. We could even add build-arg config for both of them.

@crazy-max
Copy link
Member Author

crazy-max commented Jan 8, 2022

I'm not sure about the special InlineBuildInfoAttrs handling. It looks out of place compared to the other exporter attrs.

I think we should just use the regular exporter attr for image exporter like we do today for the image exporter. Just the regular map like for other options.

The frontend(dockerfile) shouldn't check if attr exporting is enabled or not, just like for buildinfo. It just always sets it and if there is no opt-in then the image exporter just clears the whole attr section.

So the base opt-in will be with output option. For the build-arg opt-in we can make it in buildx like for the inline cache https://github.com/docker/buildx/blob/4c938c77bab00dbe983597b7d29f2e2eb19e8000/build/build.go#L358 .

Yes, it looks better that way, thanks for pointing that out.

Not for this PR but looking at the current buildinfo exporter attr buildinfo=[all,imageconfig,metadata,none], I'm not sure what the point of the buildinfo=metadata was for there. Doesn't it make more sense to have it as bool and metadata is just always enabled. What does image exporter have to do with exporting metadata.json at all?

Yes, initially the idea was to be able to choose the export mode. I agree a bool is enough to enable/disable inline buildinfo in image config. Will work on that in a follow-up.

I assume that other than inlining in config this all still works when I'm not exporting an image.

That's the tricky part. The metadata response is always null if no exporter is specified. We need at least one of them (oci, local, image, tar) otherwise the solver is never called iiuc.

In that case we would have 2 exporter attr buildinfo, buildinfo-attrs . Currently by default buildinfo would default to true and buildinfo-attrs to false. We could even add build-arg config for both of them.

SGTM

@tonistiigi
Copy link
Member

The metadata response is always null if no exporter is specified. We need at least one of them (oci, local, image, tar) otherwise the solver is never called iiuc.

I think we should try to fix this. I don't know exactly what the issue is in here, maybe we need like a exporter post-step. I can see it being useful without exporting an image as well. But let's address this part in a follow-up PR.

@crazy-max crazy-max marked this pull request as draft January 8, 2022 16:44
@crazy-max crazy-max marked this pull request as ready for review January 8, 2022 16:53
"attrs": {
"build-arg:foo": "bar",
"cmdline": "crazymax/dockerfile:master",
"context": "https://github.com/crazy-max/buildkit-buildsources-test.git#master",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something to think about: would be good if we could capture if build used a local context or no context. That is important info for replay.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a local context is being used then context field is not set:

"attrs": {
  "cmdline": "crazymax/dockerfile:buildattrs",
  "filename": "Dockerfile",
  "source": "crazymax/dockerfile:buildattrs"
}

Do you think it's worth adding something like "context": "local"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a local context is being used then context field is not set:

Yes but it might also be that there is no local context at all. Where you just build from Dockerfile with no COPY commands from context. On replay, we should be able to give an error when the local source is not passed by the user and required for replay, but not when it is not required at all.

docs/build-repro.md Outdated Show resolved Hide resolved
util/buildinfo/buildinfo.go Outdated Show resolved Hide resolved
util/buildinfo/buildinfo.go Outdated Show resolved Hide resolved
solver/llbsolver/solver.go Outdated Show resolved Hide resolved
}
return bi, nil
}

var knownAttrs = []string{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this list Dockerfile specific or generic enough to be in here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this is Dockerfile specific atm:

keyTarget = "target"
keyFilename = "filename"
keyCacheFrom = "cache-from" // for registry only. deprecated in favor of keyCacheImports
keyCacheImports = "cache-imports" // JSON representation of []CacheOptionsEntry
keyCgroupParent = "cgroup-parent"
keyContextSubDir = "contextsubdir"
keyForceNetwork = "force-network-mode"
keyGlobalAddHosts = "add-hosts"
keyHostname = "hostname"
keyImageResolveMode = "image-resolve-mode"
keyMultiPlatform = "multi-platform"
keyNameContext = "contextkey"
keyNameDockerfile = "dockerfilekey"
keyNoCache = "no-cache"
keyOverrideCopyImage = "override-copy-image" // remove after CopyOp implemented
keyShmSize = "shm-size"
keyTargetPlatform = "platform"
keyUlimit = "ulimit"

util/buildinfo/buildinfo.go Show resolved Hide resolved
@crazy-max crazy-max force-pushed the buildinfo-attrs branch 4 times, most recently from c190de4 to 4d0a96c Compare January 11, 2022 11:04
@crazy-max crazy-max marked this pull request as draft January 12, 2022 06:22
@tonistiigi tonistiigi added this to the v0.10.0 milestone Jan 18, 2022
@crazy-max crazy-max marked this pull request as ready for review February 3, 2022 23:44
Copy link
Member

@tonistiigi tonistiigi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we have some integration tests for this. Something that tests the opt-in, default behavior, inlining. If we add the case where frontend can also provide state via Metadata then that case as well (with a client.Build you can return the result metadata, look a build-context tests for example).

frontend/result.go Outdated Show resolved Hide resolved
}

// ReduceMap joins map string (union)
func ReduceMap(m1, m2 map[string]string) map[string]string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if m1 is map[string]*string then we can remove the key if it is nil.

@crazy-max crazy-max force-pushed the buildinfo-attrs branch 2 times, most recently from 0440083 to 6089f8a Compare February 14, 2022 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants