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.

Sign up for GitHub

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

Jump to bottom

Add remote driver #1078

Merged
merged 2 commits into from
Apr 29, 2022
Merged

Add remote driver #1078

merged 2 commits into from
Apr 29, 2022

Conversation

jedevc
Copy link
Collaborator

@jedevc jedevc commented Apr 25, 2022 •
edited
Loading

As a follow-up to #909, this PR introduces support for the new "remote" driver. This new PR is because the original contributor hasn't responded, so I've done my best at tidying up the review comments and completing the desired feature set.

Fixes #23.

I've also started work on some docs in jedevc@97e79d1 (not part of this PR), and will do a follow-up once the feature is merged.

CC: @developer-guy @Dentrax

@jedevc jedevc changed the title Remote driver Add remote driver Apr 25, 2022
tonistiigi
tonistiigi reviewed Apr 25, 2022
View reviewed changes
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.

Could we add some CI testing for this. Could just start buildkitd manually in a container and then use remote driver to dial into it via port.

driver/remote/driver.go Outdated Show resolved Hide resolved
driver/remote/driver.go Outdated Show resolved Hide resolved
driver/remote/driver.go Outdated Show resolved Hide resolved
driver/remote/factory.go Outdated
}

func (*factory) Priority(ctx context.Context, api dockerclient.APIClient) int {
if api == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Priority of remote driver should not depend on Docker API.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Makes sense, it should just return a fixed priority.

Is there a reason that the kubernetes driver priority depends on the Docker API?

driver/remote/factory.go Outdated Show resolved Hide resolved
driver/remote/factory.go Show resolved Hide resolved
driver/remote/factory.go Show resolved Hide resolved
@jedevc jedevc force-pushed the remote-driver branch 4 times, most recently from 6d48dc9 to 98cfecc Compare April 26, 2022 13:02
@tonistiigi
Copy link
Member

So it looks like Stop is called on remove - returning an error here means that the remove command errors with the message we put here.

Ok, leave it nil atm. As a follow-up would be good to return a typed error in here so we can show an error message for stop in these cases but let rm pass cleanly.

tonistiigi
tonistiigi reviewed Apr 27, 2022
View reviewed changes
driver/remote/driver.go Outdated Show resolved Hide resolved
driver/remote/factory.go Outdated Show resolved Hide resolved
driver/manager.go Outdated Show resolved Hide resolved
driver/remote/driver.go Outdated Show resolved Hide resolved
driver/remote/factory.go Outdated Show resolved Hide resolved
@jedevc jedevc force-pushed the remote-driver branch 3 times, most recently from 05f516d to 764e2f7 Compare April 27, 2022 11:39
@jedevc
Copy link
Collaborator Author

jedevc commented Apr 27, 2022

So, I've changed a few more things (as another commit on top, will squash when code is good).

  • Validate the endpoint argument at point of creation, requiring that it exists, and is a valid url with a valid scheme.
  • Use the value of BUILDKIT_HOST as fallback if the endpoint arg is not provided
  • Fix priorities for the remote (so if the url matches a given pattern, we prioritize it more)
    • For now, I've done unix:// (which might not be the right call) and tcp:// - no tls://, since those args are handled at a different layer, and I think conceptually you could also do TLS over unix sockets?
  • Refactoring around the runCreate function, to get it to play nicely with the remote driver.

@jedevc jedevc requested a review from tonistiigi April 27, 2022 12:32
@jedevc jedevc mentioned this pull request Apr 28, 2022
Closed
@jedevc
Copy link
Collaborator Author

jedevc commented Apr 28, 2022

Squashed, should be good to go 🎉

developer-guy and others added 2 commits April 28, 2022 11:34
@developer-guy @Dentrax @jedevc
feat: env driver
3dc83e5 
Co-authored-by: Furkan Türkal <[email protected]>
Signed-off-by: Batuhan Apaydın <[email protected]>
@jedevc
Complete remote driver
d7e4aff 
This patch completes the work started in creating a remote driver:

- Renames the env driver to the remote driver (an alternative suggestion
  that should be more user-friendly)
- Adds support for TLS to encrypt connections with buildkitd
- Fixes outstanding review comments
- Reworks the buildx create command endpoint construction to be clearer
  and include better support for this new driver.

Signed-off-by: Justin Chadwell <[email protected]>
@tonistiigi tonistiigi merged commit 38f1138 into docker:master Apr 29, 2022
@jedevc jedevc mentioned this pull request Apr 29, 2022
Merged
@jedevc jedevc deleted the remote-driver branch May 10, 2022 15:23
@jedevc jedevc restored the remote-driver branch May 10, 2022 15:23
@jedevc jedevc deleted the remote-driver branch May 10, 2022 15:23
@crazy-max crazy-max added this to the v0.9.0 milestone Jun 5, 2022
@developer-guy developer-guy mentioned this pull request Jun 6, 2022
Open
@remidebette remidebette mentioned this pull request Jun 24, 2022
Closed
@developer-guy developer-guy mentioned this pull request Jun 28, 2022
Open
@jaihwan104
Copy link
Contributor

I want to use the remote driver.
When will this feature be released? 👀

@jedevc jedevc mentioned this pull request Jul 18, 2022
Closed
@kingdonb kingdonb mentioned this pull request Jul 27, 2022
Open
@tonistiigi tonistiigi mentioned this pull request Aug 2, 2022
Merged
@crazy-max crazy-max mentioned this pull request Feb 26, 2023
Merged
@jedevc jedevc mentioned this pull request Mar 23, 2023
Merged
@jedevc jedevc mentioned this pull request May 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tonistiigi tonistiigi tonistiigi approved these changes

@AkihiroSuda AkihiroSuda Awaiting requested review from AkihiroSuda

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.9.0
Development

Successfully merging this pull request may close these issues.

add "env" driver that connects to $BUILDKIT_HOST
5 participants
@jedevc @tonistiigi @jaihwan104 @crazy-max @developer-guy