Update /etc/hosts when linked container is restarted #7677

erikh · 2014-08-22T06:27:58Z

Carry of #7092

LK4D4 · 2014-08-22T08:48:01Z

daemon/container.go

@@ -297,6 +297,9 @@ func (container *Container) Start() (err error) {
 	if err := container.initializeNetworking(); err != nil {
 		return err
 	}
+	if err := container.updateParentsHosts(); err != nil {


I wonder if we need to remove records from hosts on container stop.

I don’t think that’s necessary; the next time the container starts the file will be rewritten anyhow.

-Erik

On Aug 22, 2014, at 1:48 AM, Alexandr Morozov notifications@github.com wrote:

In daemon/container.go:

@@ -297,6 +297,9 @@ func (container *Container) Start() (err error) {
if err := container.initializeNetworking(); err != nil {
return err
}

if err := container.updateParentsHosts(); err != nil {
I wonder if we need to remove records from hosts on container stop.

—
Reply to this email directly or view it on GitHub https://github.com/docker/docker/pull/7677/files#r16587815.

Agree with @erikh

LK4D4 · 2014-08-22T10:33:09Z

TestHostsLinkedContainerUpdate fails every time for me.

LK4D4 · 2014-08-22T10:51:17Z

integration-cli/docker_cli_run_test.go

+	}
+	defer os.RemoveAll(tmpdir)
+
+	_, err = runCommand(exec.Command(dockerBinary, "run", "-d", "--name", "c1", "busybox", "sleep", "5"))


I think we should use runCommandWithOutput and print output in Fatal. Because now test failing for me and I have no idea why.

cpuguy83 · 2014-08-22T20:26:45Z

daemon/container.go

+		return err
+	}
+	for _, cid := range parents {
+		if cid != "0" {


This is some super deep nesting.
Could we change these ifs to be the other way

if cid == "0" { return nil }

etc, on down the line.

You meant continue instead or return nil I think.

+1 for if cid == "0" { continue }

SvenDowideit · 2014-08-25T02:08:39Z

DocsLGTM - @jamtur01 @fredlf @ostezer

ghost · 2014-08-25T13:33:45Z

docs/sources/articles/networking.md

+    [Communication between containers](#between-containers). If the linked
+    container is restarted, it is likely to get a different IP address; to
+    cater for this, Docker then updates the child container's `/etc/hosts` file
+    `ALIAS` entry.


I'd reword [this] to something like this:

Communication between containers.
On restart, Docker may assign a different IP address to the linked containers.
To keep the link, Docker then updates the ALIAS entry in the recipient
(i.e., child) container's /etc/hosts file.

Or:

Communication between containers.
Docker updates the ALIAS entry in the /etc/hosts file of the recipient
containers (i.e., children) in order to keep the link since Docker may
assign a different IP address to the linked containers on restart.

Please note that "to keep the link", however, might not be the correct term to use here.

I like the second one better, I'll be using that.

erikh · 2014-08-25T16:45:43Z

@LK4D4, can you verify the errors are still happening?

Otherwise, I believe I have addressed all issues docs and otherwise.

LK4D4 · 2014-08-25T16:47:36Z

I'll try in a few minutes.

LK4D4 · 2014-08-25T16:54:22Z

@erikh all working OK now. Thank you.
LGTM

tiborvass · 2014-08-25T18:32:28Z

daemon/container.go

+		c := container.daemon.Get(cid)
+		if c != nil && !container.daemon.config.DisableNetwork && !container.hostConfig.NetworkMode.IsContainer() && !container.hostConfig.NetworkMode.IsHost() {
+			if err := etchosts.Update(c.HostsPath, container.NetworkSettings.IPAddress, container.Name[1:]); err != nil {
+				return fmt.Errorf("Failed to update parent: %v", err)


Error message could be more descriptive: Failed to update /etc/hosts of parent container.

Should be fixed now. Thanks!

-Erik

On Aug 25, 2014, at 11:32 AM, Tibor Vass notifications@github.com wrote:

In daemon/container.go:

@@ -878,6 +886,26 @@ func (container *Container) setupContainerDns() error {
return ioutil.WriteFile(container.ResolvConfPath, resolvConf, 0644)
}

+func (container *Container) updateParentsHosts() error {

parents, err := container.daemon.Parents(container.Name)

if err != nil {

return err

}

for _, cid := range parents {

if cid == "0" {

continue

}

c := container.daemon.Get(cid)

if c != nil && !container.daemon.config.DisableNetwork && !container.hostConfig.NetworkMode.IsContainer() && !container.hostConfig.NetworkMode.IsHost() {

if err := etchosts.Update(c.HostsPath, container.NetworkSettings.IPAddress, container.Name[1:]); err != nil {

return fmt.Errorf("Failed to update parent: %v", err)
Error message could be more descriptive: Failed to update /etc/hosts of parent container.

—
Reply to this email directly or view it on GitHub.

tiborvass · 2014-08-25T19:41:02Z

@erikh Your DCO has vieux's name and email :)

erikh · 2014-08-25T19:44:40Z

It's also committed by @vieux :) It's his patch, I tried to preserve it.

tiborvass · 2014-08-25T19:55:35Z

@erikh Sorry, I meant that, there are 2 DCOs (1 for vieux and 1 for you), and the 2nd one (yours) has still vieux's name and email.

erikh · 2014-08-25T20:01:00Z

Should be fixed now @tiborvass. Thanks.

tiborvass · 2014-08-25T20:18:50Z

LGTM

cpuguy83 · 2014-08-25T20:46:43Z

docs/sources/articles/networking.md

-    [Communication between containers](#between-containers).
+    [Communication between containers](#between-containers).  Docker updates
+    the ALIAS entry in the /etc/hosts file of the recipient containers (i.e.,
+    children) in order to keep the link since Docker may assign a different IP


I thought we want to remove references to parent/child in docs?

crosbymichael · 2014-08-25T23:23:09Z

Functionally looks good. Just need another commit with some unit tests for the Update and Parents functions in graphd and the network code

erikh · 2014-08-26T05:24:43Z

Updated, PTAL.

On Aug 25, 2014, at 4:23 PM, Michael Crosby notifications@github.com wrote:

Functionally looks good. Just need another commit with some unit tests for the Update and Parents functions in graphd and the network code

—
Reply to this email directly or view it on GitHub.

fredlf · 2014-08-26T17:04:33Z

docs/sources/articles/networking.md

+    [Communication between containers](#between-containers).  Docker updates
+    the ALIAS entry in the /etc/hosts file of the recipient containers
+    in order to keep the link since Docker may assign a different IP
+    address to the linked containers on restart.


We should flip this sentence so the context is at the beginning where it can be more helpful. So,

Because Docker may assign a different IP address to the linked containers on restart, Docker updates the ALIAS entry in the /etc/hosts file of the recipient containers.

fredlf · 2014-08-26T17:06:18Z

Docs LGTM once my minor comment is addressed.

erikh · 2014-08-26T18:00:00Z

@fredlf, @crosbymichael, PTAL

On Aug 26, 2014, at 10:06 AM, Fred Lifton notifications@github.com wrote:

Docs LGTM once my minor comment is addressed.

—
Reply to this email directly or view it on GitHub.

crosbymichael · 2014-08-26T18:00:48Z

@erikh needs rebased

Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

vieux · 2014-08-28T02:37:26Z

@erikh, @crosbymichael said it needs rebased

vieux · 2014-08-28T02:37:31Z

👿

erikh · 2014-08-28T02:48:24Z

rebased
On Aug 27, 2014, at 7:37 PM, Victor Vieux notifications@github.com wrote:

—
Reply to this email directly or view it on GitHub.

vieux · 2014-08-28T02:53:33Z

LGTM

vieux · 2014-08-28T02:53:43Z

@crosbymichael please merge

crosbymichael · 2014-08-28T02:54:06Z

LGTM

Update /etc/hosts when linked container is restarted

vieux · 2014-08-29T17:39:57Z

In our next release, 1.3

On Fri, Aug 29, 2014 at 12:44 AM, omeid notifications@github.com wrote:

When is this going to be tagged and released?

—
Reply to this email directly or view it on GitHub
#7677 (comment).

Victor VIEUX
http://vvieux.com

erikh mentioned this pull request Aug 22, 2014

Update /etc/hosts when linked container is restarted #7092

Closed

LK4D4 reviewed Aug 22, 2014
View reviewed changes

jamtur01 added the /project/doc label Aug 22, 2014

cpuguy83 reviewed Aug 22, 2014
View reviewed changes

ghost reviewed Aug 25, 2014
View reviewed changes

tiborvass reviewed Aug 25, 2014
View reviewed changes

cpuguy83 reviewed Aug 25, 2014
View reviewed changes

fredlf reviewed Aug 26, 2014
View reviewed changes

Update /etc/hosts when linked container is restarted

450740c

Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)

Erik Hollensbe added 3 commits August 27, 2014 18:23

pkg/graphdb: tests for Parents and Children

925a325

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

pkg/networkfs/etchosts: tests for Update method

9f09ef1

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

Update networking doc with clarified text regarding links usage

f6a2fc4

Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

This was referenced Aug 28, 2014

Cleanup: Link and Names manager #7770

Closed

Link Jobs #7771

Closed

crosbymichael added a commit that referenced this pull request Aug 28, 2014

Merge pull request #7677 from erikh/update_hosts_linked_containers

2a5e29a

Update /etc/hosts when linked container is restarted

crosbymichael merged commit 2a5e29a into moby:master Aug 28, 2014

vieux added this to the 1.3.0 milestone Aug 29, 2014

mgood mentioned this pull request Oct 23, 2014

/etc/hosts not updated for aliased links #8721

Closed

kayiwa mentioned this pull request Jan 16, 2015

nginx returns 502 Bad Gateway after restart trevormunoz/docker-nginx-es#5

Closed

tianon mentioned this pull request Jan 16, 2015

mysql IP increasing automatically? docker-library/wordpress#41

Closed

Update /etc/hosts when linked container is restarted #7677

Update /etc/hosts when linked container is restarted #7677

Conversation

erikh commented Aug 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LK4D4 commented Aug 22, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SvenDowideit commented Aug 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikh commented Aug 25, 2014

LK4D4 commented Aug 25, 2014

LK4D4 commented Aug 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiborvass commented Aug 25, 2014

erikh commented Aug 25, 2014

tiborvass commented Aug 25, 2014

erikh commented Aug 25, 2014

tiborvass commented Aug 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crosbymichael commented Aug 25, 2014

erikh commented Aug 26, 2014

Choose a reason for hiding this comment

fredlf commented Aug 26, 2014

erikh commented Aug 26, 2014

crosbymichael commented Aug 26, 2014

vieux commented Aug 28, 2014

vieux commented Aug 28, 2014

erikh commented Aug 28, 2014

vieux commented Aug 28, 2014

vieux commented Aug 28, 2014

crosbymichael commented Aug 28, 2014

vieux commented Aug 29, 2014