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: merge build sources for deps #2684

Merged
merged 2 commits into from
Mar 9, 2022

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Feb 27, 2022

follow-up #2654

fixes an issue where sources for buildinfo dependencies were not merged. this also deduplicates sources from main buildinfo that only belong to dependencies.

@tonistiigi
Copy link
Member

tonistiigi commented Feb 28, 2022

Not sure I understand this. We shouldn't be processing the buildinfo for deps as that was not part of the current build request. Whatever was sent with the request should also be in output. If the output is wrong then in implies the request already sent wrong data.

@crazy-max
Copy link
Member Author

If the output is wrong then in implies the request already sent wrong data.

Yes that's it, will fix that instead and see how it goes.

@crazy-max
Copy link
Member Author

not related but gha cache not happy: https://github.com/moby/buildkit/runs/5378541102?check_suite_focus=true#step:6:548

ERROR: failed to copy: invalid status response 416 The range specified is invalid for the current size of the resource. for https://ki6cacprodeus1file5.blob.core.windows.net/2ff2ce0399e5464697755b110c91788d/ea76a6820795ec11a22a501ac56bd046?sv=2019-07-07&sr=b&sig=2bzskof2OaGZZRo0M2Y7QgrQLrl7rTHKHubQBXTQt9c%3D&se=2022-03-01T17%3A31%3A45Z&sp=r&rscl=x-e2eid-4efdc9ae-1e284eb0-85918400-88b1dd2b

@crazy-max
Copy link
Member Author

I tried to avoid using mergeSources func in the bridge solver for the deps by adding buildinfo encoding in the llb bridge forwarded like it's done in the solver but then buildinfo added by the frontend are overwritten so the base cannot resolve the user input ref. I think we should use another key in the exporter. WDYT?

@crazy-max
Copy link
Member Author

crazy-max commented Mar 1, 2022

@tonistiigi Here is the repro for the current behavior with the unprocessed sources for deps: https://github.com/crazy-max/buildx-buildkit-tests/tree/main/buildkit-2684

Metadata output: https://github.com/crazy-max/buildx-buildkit-tests/runs/5382243776?check_suite_focus=true#step:4:256

  {
    "app": {
      "containerimage.buildinfo": {
        "frontend": "dockerfile.v0",
        "attrs": {
          "build-arg:bar": "foo",
          "build-arg:foo": "bar",
          "context:baseapp": "input:0-base",
          "filename": "Dockerfile"
        },
        "sources": [
          {
            "type": "docker-image",
            "ref": "docker.io/library/alpine@sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300",
            "pin": "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300"
          },
          {
            "type": "docker-image",
            "ref": "docker.io/library/busybox:latest",
            "pin": "sha256:afcc7f1ac1b49db317a7196c902e61c6c3c4607d63599ee1a82d702d249a0ccb"
          }
        ],
        "deps": {
          "0-base": {
            "frontend": "dockerfile.v0",
            "attrs": {
              "build-arg:bar": "foo",
              "build-arg:basefoo": "bar",
              "build-arg:foo": "bar",
              "filename": "baseapp.Dockerfile"
            },
            "sources": [
              {
                "type": "docker-image",
                "ref": "alpine",
                "alias": "docker.io/library/alpine@sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300",
                "pin": "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300"
              }
            ]
          }
        }
      }
    },
    "base": {
      "containerimage.buildinfo": {
        "frontend": "dockerfile.v0",
        "attrs": {
          "build-arg:bar": "foo",
          "build-arg:basefoo": "bar",
          "build-arg:foo": "bar",
          "filename": "baseapp.Dockerfile"
        },
        "sources": [
          {
            "type": "docker-image",
            "ref": "docker.io/library/alpine:latest",
            "pin": "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300"
          }
        ]
      }
    }
  }

As we discussed it should look like this:

  {
    "app": {
      "containerimage.buildinfo": {
        "frontend": "dockerfile.v0",
        "attrs": {
          "build-arg:bar": "foo",
          "build-arg:foo": "bar",
          "context:baseapp": "input:0-base",
          "filename": "Dockerfile"
        },
        "sources": [
          {
            "type": "docker-image",
            "ref": "docker.io/library/busybox:latest",
            "pin": "sha256:afcc7f1ac1b49db317a7196c902e61c6c3c4607d63599ee1a82d702d249a0ccb"
          }
        ],
        "deps": {
          "0-base": {
            "frontend": "dockerfile.v0",
            "attrs": {
              "build-arg:bar": "foo",
              "build-arg:basefoo": "bar",
              "build-arg:foo": "bar",
              "filename": "baseapp.Dockerfile"
            },
            "sources": [
              {
                "type": "docker-image",
                "ref": "docker.io/library/alpine:latest",
                "pin": "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300"
              }
            ]
          }
        }
      }
    },
    "base": {
      "containerimage.buildinfo": {
        "frontend": "dockerfile.v0",
        "attrs": {
          "build-arg:bar": "foo",
          "build-arg:basefoo": "bar",
          "build-arg:foo": "bar",
          "filename": "baseapp.Dockerfile"
        },
        "sources": [
          {
            "type": "docker-image",
            "ref": "docker.io/library/alpine:latest",
            "pin": "sha256:21a3deaa0d32a8057914f36584b5288d2e5ecc984380bc0118285c70fa8c9300"
          }
        ]
      }
    }
  }

@crazy-max crazy-max marked this pull request as draft March 1, 2022 21:45
@crazy-max crazy-max marked this pull request as ready for review March 2, 2022 20:31
util/buildinfo/buildinfo.go Outdated Show resolved Hide resolved
util/buildinfo/buildinfo.go Outdated Show resolved Hide resolved
@tonistiigi tonistiigi added this to the v0.10.0 milestone Mar 4, 2022
@crazy-max crazy-max force-pushed the fix-buildinfo-deps-sources branch 4 times, most recently from 5dc5a1a to 9e35a4d Compare March 4, 2022 11:53
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.

The description of the PR needs updating as what it does has changed. I'm not sure about the first commit. The sources in deps should not be "merged", the deps should always be immutable and left as it was sent with the request.

util/buildinfo/buildinfo.go Show resolved Hide resolved
util/buildinfo/buildinfo.go Outdated Show resolved Hide resolved
util/buildinfo/buildinfo.go Outdated Show resolved Hide resolved
@crazy-max
Copy link
Member Author

The sources in deps should not be "merged", the deps should always be immutable and left as it was sent with the request.

I think so but named context never pass to the solver unfortunately but the frontend gateway which we didn't handle atm. Do we? I think that's because the waitContextDeps in buildx does not solve any of them.

@crazy-max crazy-max force-pushed the fix-buildinfo-deps-sources branch 6 times, most recently from 13165bd to 6325ee3 Compare March 5, 2022 03:06
@crazy-max crazy-max force-pushed the fix-buildinfo-deps-sources branch 3 times, most recently from 3af798a to ccb279c Compare March 8, 2022 01:18
@crazy-max crazy-max marked this pull request as draft March 8, 2022 12:05
@crazy-max crazy-max marked this pull request as ready for review March 8, 2022 12:56
@crazy-max crazy-max force-pushed the fix-buildinfo-deps-sources branch 3 times, most recently from 4158034 to 8342efc Compare March 8, 2022 15:22
@crazy-max crazy-max force-pushed the fix-buildinfo-deps-sources branch 10 times, most recently from 620d4c9 to 9f4d4b8 Compare March 9, 2022 15:21
@tonistiigi tonistiigi merged commit 068cf68 into moby:master Mar 9, 2022
@crazy-max crazy-max deleted the fix-buildinfo-deps-sources branch March 9, 2022 19:13
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