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

feat: Allow copa to use buildkit from dockerd #233

Merged
merged 3 commits into from
Aug 15, 2023

Conversation

cpuguy83
Copy link
Contributor

@cpuguy83 cpuguy83 commented Aug 2, 2023

This adds support for connection drivers for connecting to buildkit.

  • unix:// addrs are for connecting directly to buildkitd as before
  • docker:// will connect to docker's built-in buildkit instance
  • docker-container:// takes a container name and proxies the buildkit socket using buildctl in the container (similar to buildx)
  • buildx:// will read a specific buildx instance config and try to connect to that (defaults to the currently selected buildx instance) - currently only supports docker-container backed buildx instances
  • An empty addr will attempt the default docker address, then try buildx, and then fallback to the default buildkit addr.

Fixes #177

@cpuguy83 cpuguy83 changed the title feat: Allow copy to use buildkit from dockerd feat: Allow copa to use buildkit from dockerd Aug 2, 2023
@cpuguy83 cpuguy83 force-pushed the try_docker_buildkit branch 2 times, most recently from 0354f5d to 74a89d7 Compare August 2, 2023 22:39
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Patch coverage: 13.98% and project coverage change: -6.22% ⚠️

Comparison is base (6e08cb6) 34.72% compared to head (e307f53) 28.51%.

❗ Current head e307f53 differs from pull request most recent head 57c66e8. Consider uploading reports for the commit 57c66e8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
- Coverage   34.72%   28.51%   -6.22%     
==========================================
  Files          12       16       +4     
  Lines        1146     1487     +341     
==========================================
+ Hits          398      424      +26     
- Misses        727     1037     +310     
- Partials       21       26       +5     
Files Changed Coverage Δ
pkg/buildkit/buildkit.go 0.00% <0.00%> (ø)
pkg/patch/patch.go 7.44% <0.00%> (-0.51%) ⬇️
pkg/buildkit/connhelpers/buildx.go 7.07% <7.07%> (ø)
pkg/buildkit/connhelpers/docker.go 22.22% <22.22%> (ø)
pkg/buildkit/drivers.go 22.72% <22.72%> (ø)
pkg/patch/cmd.go 53.57% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

go.mod Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
@sozercan
Copy link
Member

sozercan commented Aug 2, 2023

@cpuguy83 should we remove the buildkit instance for some of the e2e tests or have a matrix of them (one using docker builtin and another one using standalone buildkit)?

docker run --detach --rm --privileged -p 0.0.0.0:$BUILDKIT_PORT:$BUILDKIT_PORT/tcp --name buildkitd --entrypoint buildkitd "moby/buildkit:v$BUILDKIT_VERSION" --addr tcp://0.0.0.0:$BUILDKIT_PORT

pkg/buildkit/buildkit.go Outdated Show resolved Hide resolved
@cpuguy83 cpuguy83 force-pushed the try_docker_buildkit branch 3 times, most recently from f4e8404 to f47fa29 Compare August 3, 2023 01:40
@cpuguy83
Copy link
Contributor Author

cpuguy83 commented Aug 3, 2023

I updated this with to fixes issues.
Also added a 2nd commit which adds some extra drivers (details in the commit and PR description).

@cpuguy83
Copy link
Contributor Author

cpuguy83 commented Aug 3, 2023

should we remove the buildkit instance for some of the e2e tests or have a matrix of them (one using docker builtin and another one using standalone buildkit)?

@sozercan Sounds good. I'll have to take a look at how e2e is setup tomorrow.

@salaxander
Copy link
Contributor

No review comments this is just super cool that's all

@cpuguy83 cpuguy83 force-pushed the try_docker_buildkit branch 3 times, most recently from 0bdc8b7 to dbb0c56 Compare August 8, 2023 17:47
@cpuguy83
Copy link
Contributor Author

cpuguy83 commented Aug 8, 2023

Pushed updates for adding a test matrix.
I do have problems getting this to work on a devcontainer locally, but that seems to be true even with just a normal buildkit tcp connection.
Mainly the issue is after copa patches everything it tries to export an image.
Unfortunately there are no messages... we'll see how this does in CI after some approves the run.

@cpuguy83 cpuguy83 force-pushed the try_docker_buildkit branch 8 times, most recently from 4126ebb to 8d1ad0f Compare August 10, 2023 21:25
@cpuguy83 cpuguy83 force-pushed the try_docker_buildkit branch 15 times, most recently from 3167bc9 to da12dca Compare August 11, 2023 22:34
@cpuguy83
Copy link
Contributor Author

Finally have CI working the way I'd like it to.

Moved the patch tests into go integration tests.
Tests run in parallel and the test matrix is used to setup different buildkit envs.
So we can add different envs without blowing up the test matrix for a run.

codecov is not happy.
I'm not sure I can really make it happy enough to turn it green.
Some of this code is interacting with the system so it's not really feasible to write unit tests for it.

@sozercan sozercan self-requested a review August 14, 2023 23:21
integration/patch_test.go Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
#!/usr/bin/env sh
Copy link
Member

Choose a reason for hiding this comment

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

do we want to align on sh or bash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's nothing bashy-in here, really the line is to make my editor load the right toolset.
Otherwise totally fine to throw either sh or bash in there.

- Adds all the built-in connection helpers from upstream buildkit (e.g.
  `docker-container://`, `ssh://`, `kubepod://`
- Add connection helpers for:
  - dockerd's built-in buildkit (`docker://`)
  - buildx configured buildkit instances (`buildx://`)
- When addr is unset try to find a suitable buildkit instance
  - Priority: docker -> buildx -> default buildkit addr

For the buildx support, currently only buildx instances over
docker-containers are supported. Support other buildx drivers is
possible (e.g. the kubernetes driver), but this is a lot more effort
that can come later.

Signed-off-by: Brian Goff <[email protected]>
This is just a way to test all the connection helpers using the existing
test harness.
Ideally the patch tests are refactored into use go and we can more
easily manage mutliple parallel environments from one runner.

Signed-off-by: Brian Goff <[email protected]>
Co-authored-by: Sertaç Özercan <[email protected]>
Signed-off-by: Brian Goff <[email protected]>
Copy link
Member

@sozercan sozercan left a comment

Choose a reason for hiding this comment

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

Thank you! LGTM

@sozercan sozercan merged commit 3dec6b7 into project-copacetic:main Aug 15, 2023
14 of 15 checks passed
@cpuguy83 cpuguy83 deleted the try_docker_buildkit branch August 15, 2023 16:07
ashnamehrotra pushed a commit to ashnamehrotra/copacetic that referenced this pull request Aug 25, 2023
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.

[REQ] Skip request to container registry when image already exists locally
3 participants