Skip to content

builder-next: disable mergeop and diffop#45153

Merged
neersighted merged 2 commits intomoby:masterfrom
neersighted:carry_45112
Mar 16, 2023
Merged

builder-next: disable mergeop and diffop#45153
neersighted merged 2 commits intomoby:masterfrom
neersighted:carry_45112

Conversation

@neersighted
Copy link
Copy Markdown
Member

@neersighted neersighted commented Mar 13, 2023

Temporarily disable Merge/Diff until there is a proper solution to #45111. Dockerfiles will detect the missing capability for COPY --link and fall back to the behavior in 20.10.

Copy link
Copy Markdown
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.

Unless there is some time pressure with this, I'd make a different patch for master.

Merge/diff should be enabled when containerd is enabled. The ugly regexp can be avoided with some buildkit refactoring.

@neersighted
Copy link
Copy Markdown
Member Author

The change is targeted to the graphdrivers as it's in newGraphDriverController -- as I understand it, mergeop and diffop are broken with the graphdriver backend until 0.12.x. If that's the case, there's a strong preference to accept a change on master first, even if it's immediately replaced with a better solution, to better understand the divergence of a previous branch and the development branch, and greatly reduce the chance a fix is ever missed.

@thaJeztah
Copy link
Copy Markdown
Member

Looks like Ci is failing on master (perhaps differences between v0.10 and v0.11 of BuildKit and/or other changes)

@crazy-max
Copy link
Copy Markdown
Member

Merge/diff should be enabled when containerd is enabled. The ugly regexp can be avoided with some buildkit refactoring.

Will be sorted with moby/buildkit#3711 hopefully. Will look like this in the workflow:

      -
        name: Test
        run: |
          ./hack/test ${{ matrix.typ }}
        env:
          CONTEXT: "."
          TEST_DOCKERD: "1"
          TEST_DOCKERD_BINARY: "./build/moby/dockerd"
          TESTPKGS: "./${{ matrix.pkg }}"
          TESTFLAGS: "-v --parallel=1 --timeout=30m --run=//worker=dockerd$"
          BUILDKIT_TEST_FEATURE_MERGEDIFF: "0"
        working-directory: buildkit

@tonistiigi
Copy link
Copy Markdown
Member

Looks like Ci is failing on master (perhaps differences between v0.10 and v0.11 of BuildKit and/or other changes)

Yeah, that's a new test that didn't exist in v0.11.

@thaJeztah
Copy link
Copy Markdown
Member

@neersighted looks like this one can be updated with changes from #45165 to help with CI (to skip tests)

Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
@neersighted
Copy link
Copy Markdown
Member Author

I've rebased on master and taken the solution from #45165; I'm updating #45112 next.

Co-authored-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: Bjorn Neergaard <bneergaard@mirantis.com>
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@neersighted neersighted merged commit 2cf6389 into moby:master Mar 16, 2023
@neersighted neersighted deleted the carry_45112 branch March 16, 2023 18:12
@thaJeztah thaJeztah added this to the v-next milestone Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants