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.

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

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

Merged
merged 4 commits into from Aug 28, 2014

Conversation

Projects
None yet
10 participants
@erikh
Copy link
Contributor

erikh commented Aug 22, 2014

Carry of #7092

closes #7092
fixes #6350

/cc @vieux

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

This comment has been minimized.

@LK4D4

LK4D4 Aug 22, 2014

Contributor

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

This comment has been minimized.

@erikh

erikh Aug 22, 2014

Contributor

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.

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 22, 2014

Contributor

Agree with @erikh

@LK4D4

This comment has been minimized.

Copy link
Contributor

LK4D4 commented Aug 22, 2014

TestHostsLinkedContainerUpdate fails every time for me.

}
defer os.RemoveAll(tmpdir)

_, err = runCommand(exec.Command(dockerBinary, "run", "-d", "--name", "c1", "busybox", "sleep", "5"))

This comment has been minimized.

@LK4D4

LK4D4 Aug 22, 2014

Contributor

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

This comment has been minimized.

@tiborvass

tiborvass Aug 25, 2014

Collaborator

Agreed

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

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 22, 2014

Contributor

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.

This comment has been minimized.

@LK4D4

LK4D4 Aug 23, 2014

Contributor

You meant continue instead or return nil I think.

This comment has been minimized.

@tiborvass

tiborvass Aug 25, 2014

Collaborator

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

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Aug 25, 2014

[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.

This comment has been minimized.

@ostezer

ostezer Aug 25, 2014

Contributor

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.

This comment has been minimized.

@erikh

erikh Aug 25, 2014

Contributor

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

@@ -432,6 +432,9 @@ mechanism to communicate with a linked container by its alias:
$ docker run -d --name servicename busybox sleep 30
$ docker run -i -t --link servicename:servicealias busybox ping -c 1 servicealias

If you restart the parent container (`servicename` in this case), the child

This comment has been minimized.

@ostezer

ostezer Aug 25, 2014

Contributor

parent > source

Please see #7250 where transition from "parent / child" to "source / recipient" began.

This comment has been minimized.

@ostezer

ostezer Aug 25, 2014

Contributor

I'd reword:

When you restart the source container (e.g., servicename), Docker
will update the recipient container's /etc/hosts entry.

This comment has been minimized.

@fredlf

fredlf Aug 25, 2014

Contributor

Agree with Sonat, and would ask you to please go a step further and simply remove any reference of "parent" or "child" in the docs (I realize it's in the code, c'est la vie). In #7250 we determined it simply is not the right metaphor.

If you need to get the PR in asap, LMK and I'll make a reminder to go back and edit it myself.

This comment has been minimized.

@erikh

erikh Aug 25, 2014

Contributor

I’ve made the changes (will be pushed in a minute) but I think a more global docs edit should be in a different PR.

On Aug 25, 2014, at 9:33 AM, Fred Lifton notifications@github.com wrote:

In docs/sources/reference/run.md:

@@ -432,6 +432,9 @@ mechanism to communicate with a linked container by its alias:
$ docker run -d --name servicename busybox sleep 30
$ docker run -i -t --link servicename:servicealias busybox ping -c 1 servicealias

+If you restart the parent container (servicename in this case), the child
Agree with Sonat, and would ask you to please go a step further and simply remove any reference of "parent" or "child" in the docs (I realize it's in the code, c'est la vie). In #7250 we determined it simply is not the right metaphor.

If you need to get the PR in asap, LMK and I'll make a reminder to go back and edit it myself.


Reply to this email directly or view it on GitHub.

@@ -241,6 +241,16 @@ to make use of your `db` container.
> example, you could have multiple (differently named) web containers attached to your
>`db` container.
If you restart the parent container, the linked child containers `/etc/hosts`

This comment has been minimized.

@ostezer

ostezer Aug 25, 2014

Contributor

I'd reword to something like:

If you restart the source container, Docker will update the /etc/hosts file
of recipient containers with the source's new IP address in order to keep
the linkage.

if err != nil {
return err
}
var re = regexp.MustCompile(fmt.Sprintf("(\\S*)(\\t%s)", hostname))

This comment has been minimized.

@tiborvass

tiborvass Aug 25, 2014

Collaborator

You should regexp.QuoteMeta(hostname).

@erikh erikh force-pushed the erikh:update_hosts_linked_containers branch from 1a7cff5 to c5111d4 Aug 25, 2014

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Aug 25, 2014

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

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

@LK4D4

This comment has been minimized.

Copy link
Contributor

LK4D4 commented Aug 25, 2014

I'll try in a few minutes.

@LK4D4

This comment has been minimized.

Copy link
Contributor

LK4D4 commented Aug 25, 2014

@erikh all working OK now. Thank you.
LGTM

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)

This comment has been minimized.

@tiborvass

tiborvass Aug 25, 2014

Collaborator

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

This comment has been minimized.

@erikh

erikh Aug 25, 2014

Contributor

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.

@erikh erikh force-pushed the erikh:update_hosts_linked_containers branch from c5111d4 to ec3b15a Aug 25, 2014

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Aug 25, 2014

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

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Aug 25, 2014

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

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Aug 25, 2014

@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 erikh force-pushed the erikh:update_hosts_linked_containers branch from ec3b15a to 0ea321f Aug 25, 2014

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Aug 25, 2014

Should be fixed now @tiborvass. Thanks.

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Aug 25, 2014

LGTM

[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

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 25, 2014

Contributor

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

This comment has been minimized.

@erikh

erikh Aug 25, 2014

Contributor

fixed.

@@ -241,6 +241,16 @@ to make use of your `db` container.
> example, you could have multiple (differently named) web containers attached to your
>`db` container.
If you restart the source container, the linked child containers `/etc/hosts`

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 25, 2014

Contributor

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

@erikh erikh force-pushed the erikh:update_hosts_linked_containers branch from 0ea321f to e8a60d3 Aug 25, 2014

@@ -281,6 +281,18 @@ func (db *Database) Children(name string, depth int) ([]WalkMeta, error) {
return db.children(e, name, depth, nil)
}

// Return the parents of a specified entity
func (db *Database) Parents(name string) ([]string, error) {

This comment has been minimized.

@crosbymichael

crosbymichael Aug 25, 2014

Member

graphdb has good test coverage. we should add tests for Parents

This comment has been minimized.

@crosbymichael

crosbymichael Aug 25, 2014

Member

Please add these tests in a separate commit so we don't have to rereview everything else

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Aug 25, 2014

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

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Aug 26, 2014

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.

[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.

This comment has been minimized.

@fredlf

fredlf Aug 26, 2014

Contributor

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

This comment has been minimized.

Copy link
Contributor

fredlf commented Aug 26, 2014

Docs LGTM once my minor comment is addressed.

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Aug 26, 2014

@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

This comment has been minimized.

Copy link
Member

crosbymichael commented Aug 26, 2014

@erikh needs rebased

@erikh erikh force-pushed the erikh:update_hosts_linked_containers branch 3 times, most recently from 64c0384 to b88c054 Aug 27, 2014

vieux and others added some commits Jul 14, 2014

Update /etc/hosts when linked container is restarted
Docker-DCO-1.1-Signed-off-by: Victor Vieux <vieux@docker.com> (github: vieux)
pkg/graphdb: tests for Parents and Children
Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)
pkg/networkfs/etchosts: tests for Update method
Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)
Update networking doc with clarified text regarding links usage
Docker-DCO-1.1-Signed-off-by: Erik Hollensbe <github@hollensbe.org> (github: erikh)

This was referenced Aug 28, 2014

@erikh erikh force-pushed the erikh:update_hosts_linked_containers branch from b88c054 to f6a2fc4 Aug 28, 2014

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Aug 28, 2014

@erikh, @crosbymichael said it needs rebased

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Aug 28, 2014

👿

@erikh

This comment has been minimized.

Copy link
Contributor

erikh commented Aug 28, 2014

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

This comment has been minimized.

Copy link
Collaborator

vieux commented Aug 28, 2014

LGTM

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Aug 28, 2014

@crosbymichael please merge

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Aug 28, 2014

LGTM

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

Merge pull request #7677 from erikh/update_hosts_linked_containers
Update /etc/hosts when linked container is restarted

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

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Aug 29, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment