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

Perform solve request in the main gateway call #1804

Merged
merged 7 commits into from
Jun 6, 2023

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented May 16, 2023

🛠️ Replaces #1750. This is a lot neater than the original implementation and doesn't require that we go and change the entire API.

Now, we always perform the full solve request in the main gateway call. This ensures that progress works properly (and doesn't stagger), and makes the lifetime semantics much clearer.

NewResultContext abstracts the details of a successful/failed build, to always return a single ResultContext, even though the details of how a gateway is created is different:

  • For a failed build, we can just keep the gateway open.
  • For a successful build, we immediately open another gateway and re-evaluate the build definition in that gateway. This should give an instant cache hit (since the build was just successful).

@jedevc jedevc requested review from tonistiigi and ktock May 16, 2023 16:35
build/build.go Outdated Show resolved Hide resolved
@jedevc jedevc marked this pull request as ready for review May 17, 2023 14:54
@jedevc jedevc added this to the v0.11.0 milestone May 18, 2023
Copy link
Collaborator

@ktock ktock left a comment

Choose a reason for hiding this comment

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

Thanks!

build/result.go Outdated Show resolved Hide resolved
build/build.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
@jedevc jedevc requested a review from ktock May 18, 2023 13:23
@jedevc jedevc added the kind/bug Something isn't working label May 18, 2023
build/build.go Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
jedevc added 2 commits May 26, 2023 10:38
Signed-off-by: Justin Chadwell <me@jedevc.com>
Subrequests have been included in docker/dockerfile:1.5 labs, so we can
update the fallback to point to this release.

Signed-off-by: Justin Chadwell <me@jedevc.com>
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
build/result.go Outdated Show resolved Hide resolved
jedevc added 2 commits May 31, 2023 09:46
Now, we always perform the full solve request in the main gateway call.
This ensures that progress works properly, and makes the lifetime
semantics much clearer.

NewResultContext abstracts the details of a successful/failed build, to
always return a single ResultContext, even though the details of how a
gateway is created is different:
- For a failed build, we can just keep the gateway open.
- For a successful build, we immediately open another gateway and
  re-evaluate the build definition in that gateway. This should give an
  instant cache hit (since the build was just successful).

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc
Copy link
Collaborator Author

jedevc commented May 31, 2023

A bit more tidying, I've removed one of the nested goroutines which wasn't necessary, and tidied up the calls to cancel and close(baseCh) by using a defer.

I think it might be possible to try and merge the done and solve status baseCh, but I've held off doing that, I think that might get a little confusing.

jedevc added 2 commits May 31, 2023 15:13
In practice, this shouldn't happen, but the check is good to include
anyways.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
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.

Let's get this in for the release 🤞 , but this area needs a follow-up refactoring. We need to take a step back, create a design issue that describes all the cases this code needs to solve and current code paths. Then put together a graph of components and function signatures with their scope description. If needed, we need to wrap or refactor the buildkit client.Build() signature if it is too complicated to use in these new cases.

build/result.go Outdated Show resolved Hide resolved
Signed-off-by: Justin Chadwell <me@jedevc.com>
@jedevc jedevc merged commit 7cef021 into docker:master Jun 6, 2023
@jedevc jedevc deleted the fixup-solve branch June 6, 2023 07:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/debug kind/bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants