Skip to content

buildkit: enable net modes and bridge#37620

Merged
tiborvass merged 5 commits into
moby:masterfrom
tonistiigi:buildkit-net-modes
Aug 20, 2018
Merged

buildkit: enable net modes and bridge#37620
tiborvass merged 5 commits into
moby:masterfrom
tonistiigi:buildkit-net-modes

Conversation

@tonistiigi
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi commented Aug 9, 2018 •
edited by tiborvass
Loading

Changes the default network used by buildkit to libnetwork bridge and enables --network=host and --network=none. Frontends have the capability to set networking modes per-command.

depends on moby/buildkit#560

@fcrisciani @abhi

EDIT: Related to moby/buildkit#583 and #37676

tonistiigi
tonistiigi commented Aug 10, 2018
View reviewed changes
Comment thread builder/builder-next/builder.go Outdated
Copy link
Copy Markdown
Member Author

@tonistiigi tonistiigi Aug 10, 2018

Choose a reason for hiding this comment

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

This is a temporary variable that needs to be set globally because buildkit by default forbids host network entitlement on daemon level.

abhi
abhi approved these changes Aug 10, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@abhi abhi left a comment

Choose a reason for hiding this comment

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

LGTM. I will give the patch a try for better understanding.

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 10, 2018 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@9916827). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master   #37620   +/-   ##
=========================================
  Coverage          ?   35.16%           
=========================================
  Files             ?      611           
  Lines             ?    45817           
  Branches          ?        0           
=========================================
  Hits              ?    16112           
  Misses            ?    27485           
  Partials          ?     2220

@tonistiigi
Copy link
Copy Markdown
Member Author

tonistiigi commented Aug 10, 2018

Forgot to add --add-host support as well. Added a commit.

@thaJeztah thaJeztah added the area/builder Build label Aug 17, 2018
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Aug 17, 2018

This test is failing on all CI, so may be related;

09:46:47 --- FAIL: TestContainerStartOnDaemonRestart (2.33s)
09:46:47 	daemon.go:289: [d887a474afedf] waiting for daemon to start
09:46:47 	daemon.go:321: [d887a474afedf] daemon started
09:46:47 	daemon.go:289: [d887a474afedf] waiting for daemon to start
09:46:47 	daemon.go:321: [d887a474afedf] daemon started
09:46:47 	daemon_linux_test.go:65: assertion failed: error is not nil: Error response from daemon: mkdir /var/run/docker/containerd/daemon/io.containerd.runtime.v1.linux/moby/5f3a5f580ed0f8999f27b9a7d9f33f251ec812ced9d0998744f104995c20464d: file exists: already exists: failed to start test container
09:46:47 	daemon.go:279: [d887a474afedf] exiting daemon

thaJeztah
thaJeztah requested changes Aug 17, 2018
View reviewed changes
Comment thread vendor.conf Outdated
Copy link
Copy Markdown
Member

@thaJeztah thaJeztah Aug 17, 2018

Choose a reason for hiding this comment

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

moby/buildkit#560 was merged; so this can likely be "un-forked"

Copy link
Copy Markdown
Contributor

@tiborvass tiborvass Aug 17, 2018

Choose a reason for hiding this comment

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

@thaJeztah yes, I'll update as soon I fixed the failure hopefully with @abhi's help

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Aug 19, 2018
@tiborvass tiborvass mentioned this pull request Aug 20, 2018
Closed
@tonistiigi
vendor: update buildkit to 46f9075a
bc67a78 
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tibor Vass <[email protected]>
thaJeztah
thaJeztah approved these changes Aug 20, 2018
View reviewed changes
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

tiborvass
tiborvass approved these changes Aug 20, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@tiborvass tiborvass left a comment

Choose a reason for hiding this comment

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

LGTM

@tiborvass tiborvass merged commit cf72051 into moby:master Aug 20, 2018
@thaJeztah thaJeztah mentioned this pull request Aug 21, 2018
Merged
@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tiborvass tiborvass tiborvass approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@abhi abhi abhi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/builder Build status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@tonistiigi @thaJeztah @abhi @tiborvass @yongtang @GordonTheTurtle