Conversation
fcrisciani
force-pushed
the
ubuntu-dns
branch
6 times, most recently
from
8c60cca to
bddac4c
Compare
|
Hey. Definitely a change that Docker engine needs. But not sure on the details, fwiw. First: I'm 50/50 on splitting out the utility libraries in "common/" to separate sub-packages. Of course I'm not really clear on why "common/" is split out as it is. What makes "caller", "setmatrix" and now "procfs" special? Maybe they should all just go to the top level? Or maybe other stand-alones should go in "internal/"? Hrmm... Second: you have a procfs_linux.go but no procfs_unimplemented.go. Probably need that. Third: is checking for a particular binary running the right way to do this? K8s is mainly doing it to issue a warning so that's "safe" in a sense. But transparently substituting in libnetwork doesn't seem robust. Maybe we should do what k8s is doing even more so: require the user to provide an option for a default resolv.conf file when starting up the engine. Libnetwork does have (sandbox) options to specify the source of the resolv.conf file already, so this wouldn't necessarily mean a libnetwork change at all. It could just be a moby change. (We could still do the warning if systemd-resolverd is detected and the resolv.conf option isn't overridden.) |
|
replying to the concerns:
|
|
but I definitely see how undesirable is doing it for each sandbox creation |
internal/procfs/procfs_linux.go
Outdated
| package procfs | ||
|
|
||
| /* | ||
| Copyright 2015 The Kubernetes Authors. |
There was a problem hiding this comment.
Is the copyright needed?
cc @thaJeztah for advice
Internal directory is designed to contain libraries that are exclusively used by this project Signed-off-by: Flavio Crisciani <[email protected]>
fcrisciani
force-pushed
the
ubuntu-dns
branch
3 times, most recently
from
e718e88 to
6950ccf
Compare
| // This is for the host mode networking | ||
| if sb.config.originHostsPath != "" { | ||
| if sb.config.useDefaultSandBox && len(sb.config.extraHosts) == 0 { | ||
| if err := copyFile(sb.config.originHostsPath, sb.config.hostsPath); err != nil && !os.IsNotExist(err) { |
There was a problem hiding this comment.
nit -- assumes that if the condition holds true that originHostsPath has also been set. This is true in the moby patch of course, but is unclear from just reading the libnetwork code. Suggest at least putting a comment on OptionUseDefaultSandbox() to say that one must also add OptionOriginHostsPath() or one or more OptionExtraHost() options or expect an error. Same for OptionOriginResolvConfPath() and OptionDNS()/OptionDNSSearch()/OptionDNSOptions()...
Leverage what is it passed from the daemon Fix check about the host networking Signed-off-by: Flavio Crisciani <[email protected]>
|
@ctelfer added comments requested, feel free to merge if they look ok |
3 participants
Addresses: #2187
Moby side: moby/moby#37485