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

vendor: update buildkit to master@c4f191410a41 #6169

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Nov 29, 2023

Split out from #6134 (note - all we actually need here is the buildkit update. Buildkit master already uses a more recent version of Docker which should not be affected by https://github.com/dagger/dagger/security/dependabot/386).

There are two significant changes in upstream (both made by me) since we last updated:

  • Git URL parsing has changed to return a custom type, to bridge over the differences between standard URLs and SCP-style URLs. This actually simplifies our parsing code for the command line git directory args, but means we need to update our tests.
  • fsutil has updated to split the FS interface more, so that now we need to manually create a FilterFS based on a standard FS to define filtering operations (like include, exclude, map, etc).

This vendoring update also bumps github.com/docker/docker to v25.0.0-beta.1, which should prevent GHSA-jq35-85cj-fj4p.

@jedevc jedevc requested a review from gerhard November 29, 2023 12:11
@jedevc jedevc force-pushed the vendor-update-docker-buildkit branch from eaf14d1 to 6c2ce3f Compare November 29, 2023 14:58
@jedevc jedevc changed the title vendor: update buildkit to master@f8937901d348 vendor: update buildkit to master@c9ee8491d74f Nov 29, 2023
@jedevc
Copy link
Member Author

jedevc commented Nov 29, 2023

cc @sipsma, this also removes our OTEL hack to avoid panics with conflicting versions of schemas (this was fixed in moby/buildkit#4318).

Copy link
Member

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

All changes look good to me! WDYT @sipsma @vito?

I am suspicious of the failing checks in CI, doing a local run now.

We shouldn't merge until all checks are green.


Current status: dagger run ./hack/make engine:test
image

@gerhard
Copy link
Member

gerhard commented Nov 29, 2023

Yes, 189 tests are failing for me locally too:

image
image

Are they failing for you too @jedevc?

@gerhard gerhard self-requested a review November 29, 2023 15:54
@jedevc
Copy link
Member Author

jedevc commented Nov 29, 2023

Yup, I missed these, will dig into them now :)

@jedevc jedevc force-pushed the vendor-update-docker-buildkit branch from 6c2ce3f to 4f07ef2 Compare November 29, 2023 18:18
@jedevc jedevc changed the title vendor: update buildkit to master@c9ee8491d74f vendor: update buildkit to master@c4f191410a41 Nov 29, 2023
@jedevc
Copy link
Member Author

jedevc commented Nov 29, 2023

Ok, it looks like at some point buildkit has introduced a regression which causes us to deadlock - looks like moby/buildkit#4347 is the possible culprit, so I've taken the commit right before it was merged (which thankfully has the fsutil update, so we can pull in docker 25).

I'll dive into the deadlock later, but we don't need to block this PR on it.

@gerhard
Copy link
Member

gerhard commented Nov 29, 2023

I just merged the other PR which introduced some conflicts in the go.mod & go.sum.

Would you mind rebasing @jedevc? If I do it, I will appear as a co-author on all commits which doesn't seem right.

If all checks pass after the rebase, all good to merge from my POV. Would feel more comfortable if this gets @sipsma 👍 too

Copy link
Contributor

@sipsma sipsma left a comment

Choose a reason for hiding this comment

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

LGTM, but

cc @sipsma, this also removes our OTEL hack to avoid panics with conflicting versions of schemas (this was fixed in moby/buildkit#4318).

@jedevc did we have to back out of this due to the other deadlock issue? Just curious, not blocking

Ok, it looks like at some point buildkit has introduced a regression which causes us to deadlock - looks like moby/buildkit#4347 is the possible culprit, so I've taken the commit right before it was merged (which thankfully has the fsutil update, so we can pull in docker 25).

Good find on that too btw; very curious what we're doing in our integ tests that buildkit isn't (or if this is somehow particular our dagger-engine flavored buildkit, though it's hard to immediately imagine how since that commit changes things several abstraction layers away from anything we diverge from w/ vanilla buildkit)

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc force-pushed the vendor-update-docker-buildkit branch from 4f07ef2 to b15dcf2 Compare November 29, 2023 21:19
@jedevc
Copy link
Member Author

jedevc commented Nov 29, 2023

I just merged the other PR which introduced some conflicts in the go.mod & go.sum.

Would you mind rebasing @jedevc? If I do it, I will appear as a co-author on all commits which doesn't seem right.

Done, should be good to go 🤞

@jedevc did we have to back out of this due to the other deadlock issue? Just curious, not blocking

Yup 😢 I just merged moby/buildkit#4318, so it appears in the history after the deadlock we hit from moby/buildkit#4347. Once we find the fix for the deadlock, we should be able to pick that commit over.

Good find on that too btw; very curious what we're doing in our integ tests that buildkit isn't (or if this is somehow particular our dagger-engine flavored buildkit, though it's hard to immediately imagine how since that commit changes things several abstraction layers away from anything we diverge from w/ vanilla buildkit)

Yeah I'm not actually 100% sure on this 🤔 I wonder if maybe we only hit this when doing tons of builds - I think it's specifically triggered by lots of edge merging. Upstream buildkit tests each create their very own buildkit instance (I don't think there's any optimization or anything to share these), so they're all isolated from each other - this has the benefit of isolating each test in it's own sandbox, but means that we don't really have any large scale tests where they can all run into each other.

Since we use only a single dagger backend for all our tests, I think it makes edge merges a lot more likely, especially if we're running loads of tests in parallel (which would explain why this doesn't reproduce when only running a small subset of tests).

I think it's more likely that this is an upstream buildkit issue instead of something we're doing specifically in dagger, but I think some more investigation is needed with a reproducer before we can report upstream - will look at seeing if I can get this to reliably reproduce.

Copy link
Member

@gerhard gerhard left a comment

Choose a reason for hiding this comment

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

💪 🚀

@jedevc jedevc merged commit 4004866 into dagger:main Nov 30, 2023
43 checks passed
@jedevc jedevc deleted the vendor-update-docker-buildkit branch November 30, 2023 09:34
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.

None yet

3 participants