Skip to content

daemon: propagate exec-root to libnetwork-setkey#37850

Merged
thaJeztah merged 1 commit intomoby:masterfrom
AkihiroSuda:propagate-exec-root-to-libnetwork
Sep 28, 2018
Merged

daemon: propagate exec-root to libnetwork-setkey#37850
thaJeztah merged 1 commit intomoby:masterfrom
AkihiroSuda:propagate-exec-root-to-libnetwork

Conversation

@AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Sep 14, 2018

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

- What I did

propagate exec-root to libnetwork-setkey

- How I did it

moby/libnetwork#2248

- How to verify it

$ mount -t tmpfs -r none /run/docker/libnetwork
$ touch /run/docker/libnetwork/masked-for-testing
touch: cannot touch '/run/docker/libnetwork/masked-for-testing`: Read-only file system
$ dockerd --exec-root /tmp/foo

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

🐧

@AkihiroSuda AkihiroSuda requested a review from tianon as a code owner September 14, 2018 15:44
@AkihiroSuda AkihiroSuda mentioned this pull request Sep 14, 2018
Merged
@AkihiroSuda AkihiroSuda force-pushed the propagate-exec-root-to-libnetwork branch from 957448a to f35e839 Compare September 14, 2018 15:55
@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Sep 15, 2018 •
edited
Loading

CI failing: failed to start daemon: Error initializing network controller: error obtaining controller instance: listen unix /tmp/docker-exe croot/d033a311eea36/libnetwork/5f42e518a177abd218b58799edfe1104b78ae0e57dceb3bab8afe925d5d68743.sock: bind: invalid argument

Seems hitting unix(7) path length limitation:
len("/tmp/docker-execroot/d033a311eea36/libnetwork/5f42e518a177abd218b58799edfe1104b78ae0e57dceb3bab8afe925d5d68743.sock") = 115 > 108

😢

@AkihiroSuda
daemon: propagate exec-root to libnetwork-setkey
4038520 
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda AkihiroSuda force-pushed the propagate-exec-root-to-libnetwork branch from f35e839 to 4038520 Compare September 15, 2018 04:49
@codecov
Copy link

codecov bot commented Sep 15, 2018 •
edited
Loading

Codecov Report

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

@@            Coverage Diff            @@
##             master   #37850   +/-   ##
=========================================
  Coverage          ?   36.14%           
=========================================
  Files             ?      610           
  Lines             ?    45085           
  Branches          ?        0           
=========================================
  Hits              ?    16296           
  Misses            ?    26551           
  Partials          ?     2238

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Sep 15, 2018

renamed /tmp/docker-execroot to /tmp/dxr

@AkihiroSuda
Copy link
Member Author

AkihiroSuda commented Sep 18, 2018

@selansen @ctelfer @thaJeztah @vdemeester PTAL?

vdemeester
vdemeester approved these changes Sep 18, 2018
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

selansen
selansen approved these changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@selansen selansen left a comment

Choose a reason for hiding this comment

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

Thanks @AkihiroSuda . LGTM

ctelfer
ctelfer approved these changes Sep 18, 2018
View reviewed changes
Copy link
Contributor

@ctelfer ctelfer left a comment

Choose a reason for hiding this comment

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

LGTM

thaJeztah
thaJeztah approved these changes Sep 20, 2018
View reviewed changes
Copy link
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.

generally LGTM, but left one question 😅

daemon/oci_linux.go
Prestart: []specs.Hook{{
Path: target,
Args: []string{"libnetwork-setkey", c.ID, daemon.netController.ID()},
Args: []string{"libnetwork-setkey", "-exec-root=" + daemon.configStore.GetExecRoot(), c.ID, daemon.netController.ID()},
Copy link
Member

@thaJeztah thaJeztah Sep 20, 2018

Choose a reason for hiding this comment

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

Ah, just realized this uses a single dash for the flag (-exec-root, not --exec-root) probably because it's using the standard flag library..

Should we only pass this option if it's not the default, or pass it in all situations? (not sure I have a real preference, just putting it here so that we can discuss)

Copy link
Member Author

@AkihiroSuda AkihiroSuda Sep 28, 2018

Choose a reason for hiding this comment

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

I prefer passing it in all situations for better code readability

Copy link
Member

@thaJeztah thaJeztah Sep 28, 2018

Choose a reason for hiding this comment

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

Alright; it was not a blocker, just a question, so let's get this merged 👍 thanks!

@thaJeztah thaJeztah added the area/networking Networking label Sep 20, 2018
@thaJeztah thaJeztah merged commit deac65c into moby:master Sep 28, 2018
@Rid Rid mentioned this pull request Jul 25, 2019
Closed
thaJeztah referenced this pull request in docker-archive/docker-ce Nov 6, 2019
@Rid
daemon: Use short libnetwork ID in exec-root & update libnetwork
861aaf0 
Signed-off-by: Grant Millar <rid@cylo.io>
Upstream-commit: df7b8f458aec29400c76b8dc87b3ce087d3fa76c
Component: engine
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

@tianon tianon Awaiting requested review from tianon tianon is a code owner

@cpuguy83 cpuguy83 Awaiting requested review from cpuguy83

+3 more reviewers

@vdemeester vdemeester vdemeester approved these changes

@selansen selansen selansen approved these changes

@ctelfer ctelfer ctelfer approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/networking Networking status/2-code-review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@AkihiroSuda @vdemeester @thaJeztah @selansen @ctelfer @yongtang @GordonTheTurtle