diff --git a/daemon/container.go b/daemon/container.go index 0d97d726a1..221edaad2c 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -441,7 +441,7 @@ func (container *Container) buildHostnameAndHostsFiles(IP string) error { return container.buildHostsFiles(IP) } -func (container *Container) allocateNetwork() error { +func (container *Container) AllocateNetwork() (err error) { mode := container.hostConfig.NetworkMode if container.Config.NetworkDisabled || !mode.IsPrivate() { return nil @@ -449,7 +449,6 @@ func (container *Container) allocateNetwork() error { var ( env *engine.Env - err error eng = container.daemon.eng ) @@ -461,6 +460,15 @@ func (container *Container) allocateNetwork() error { return err } + // Error handling: At this point, the interface is allocated so we have to + // make sure that it is always released in case of error, otherwise we + // might leak resources. + defer func() { + if err != nil { + eng.Job("release_interface", container.ID).Run() + } + }() + if container.Config.PortSpecs != nil { if err := migratePortMappings(container.Config, container.hostConfig); err != nil { return err @@ -511,7 +519,7 @@ func (container *Container) allocateNetwork() error { return nil } -func (container *Container) releaseNetwork() { +func (container *Container) ReleaseNetwork() { if container.Config.NetworkDisabled { return } @@ -521,11 +529,41 @@ func (container *Container) releaseNetwork() { container.NetworkSettings = &NetworkSettings{} } +func (container *Container) isNetworkAllocated() bool { + return container.NetworkSettings.IPAddress != "" +} + +func (container *Container) RestoreNetwork() error { + mode := container.hostConfig.NetworkMode + // Don't attempt a restore if we previously didn't allocate networking. + // This might be a legacy container with no network allocated, in which case the + // allocation will happen once and for all at start. + if !container.isNetworkAllocated() || container.Config.NetworkDisabled || !mode.IsPrivate() { + return nil + } + + eng := container.daemon.eng + + // Re-allocate the interface with the same IP and MAC address. + job := eng.Job("allocate_interface", container.ID) + job.Setenv("RequestedIP", container.NetworkSettings.IPAddress) + job.Setenv("RequestedMac", container.NetworkSettings.MacAddress) + if err := job.Run(); err != nil { + return err + } + + // Re-allocate any previously allocated ports. + for port := range container.NetworkSettings.Ports { + if err := container.allocatePort(eng, port, container.NetworkSettings.Ports); err != nil { + return err + } + } + return nil +} + // cleanup releases any network resources allocated to the container along with any rules // around how containers are linked together. It also unmounts the container's root filesystem. func (container *Container) cleanup() { - container.releaseNetwork() - // Disable all active links if container.activeLinks != nil { for _, link := range container.activeLinks { @@ -969,8 +1007,14 @@ func (container *Container) initializeNetworking() error { container.Config.NetworkDisabled = true return container.buildHostnameAndHostsFiles("127.0.1.1") } - if err := container.allocateNetwork(); err != nil { - return err + // Backward compatibility: + // Network allocation used to be done when containers started, not when they + // were created, therefore we might be starting a legacy container that + // doesn't have networking. + if !container.isNetworkAllocated() { + if err := container.AllocateNetwork(); err != nil { + return err + } } return container.buildHostnameAndHostsFiles(container.NetworkSettings.IPAddress) } @@ -1149,7 +1193,6 @@ func (container *Container) allocatePort(eng *engine.Engine, port nat.Port, bind return err } if err := job.Run(); err != nil { - eng.Job("release_interface", container.ID).Run() return err } b.HostIp = portEnv.Get("HostIP") diff --git a/daemon/create.go b/daemon/create.go index 3309d034c1..5adbc3a378 100644 --- a/daemon/create.go +++ b/daemon/create.go @@ -83,6 +83,9 @@ func (daemon *Daemon) Create(config *runconfig.Config, hostConfig *runconfig.Hos if container, err = daemon.newContainer(name, config, img); err != nil { return nil, nil, err } + if err := daemon.Register(container); err != nil { + return nil, nil, err + } if err := daemon.createRootfs(container, img); err != nil { return nil, nil, err } @@ -90,12 +93,13 @@ func (daemon *Daemon) Create(config *runconfig.Config, hostConfig *runconfig.Hos if err := daemon.setHostConfig(container, hostConfig); err != nil { return nil, nil, err } + // We may only allocate the network if a host config was passed, otherwise we'll miss port mappings. + if err := container.AllocateNetwork(); err != nil { + return nil, nil, err + } } if err := container.ToDisk(); err != nil { return nil, nil, err } - if err := daemon.Register(container); err != nil { - return nil, nil, err - } return container, warnings, nil } diff --git a/daemon/daemon.go b/daemon/daemon.go index 6ccf8aaf2a..1757b4fea1 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -372,6 +372,16 @@ func (daemon *Daemon) restore() error { registeredContainers = append(registeredContainers, container) } + // Restore networking of registered containers. + // This must be performed prior to any IP allocation, otherwise we might + // end up giving away an already allocated address. + for _, container := range registeredContainers { + if err := container.RestoreNetwork(); err != nil { + log.Errorf("Failed to restore network for %v: %v", container.Name, err) + continue + } + } + // check the restart policy on the containers and restart any container with // the restart policy of "always" if daemon.config.AutoRestart { diff --git a/daemon/delete.go b/daemon/delete.go index 77be926c1c..923cd79368 100644 --- a/daemon/delete.go +++ b/daemon/delete.go @@ -94,6 +94,8 @@ func (daemon *Daemon) Destroy(container *Container) error { return err } + container.ReleaseNetwork() + // Deregister the container before removing its directory, to avoid race conditions daemon.idIndex.Delete(container.ID) daemon.containers.Delete(container.ID) diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 01a3f57638..280a8761da 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -1868,57 +1868,98 @@ func TestRunMutableNetworkFiles(t *testing.T) { } } -func TestRunHostsLinkedContainerUpdate(t *testing.T) { - deleteAllContainers() - out, _, err := runCommandWithOutput(exec.Command(dockerBinary, "run", "-d", "--name", "c1", "busybox", "sh", "-c", "while true; do sleep 1; done")) - if err != nil { - t.Fatal(err, out) +func TestRunStableIPAndPort(t *testing.T) { + const nContainers = 2 + var ids, ips, macs, ports [nContainers]string + + // Setup: Create a couple of containers and collect their IPs and public ports. + for i := 0; i < nContainers; i++ { + runCmd := exec.Command(dockerBinary, "run", "-d", "-p", "1234", "busybox", "top") + out, _, err := runCommandWithOutput(runCmd) + if err != nil { + t.Fatal(err) + } + ids[i] = strings.TrimSpace(out) + + ips[i], err = inspectField(ids[i], "NetworkSettings.IPAddress") + errorOut(err, t, out) + if ips[i] == "" { + t.Fatal("IP allocation failed") + } + + macs[i], err = inspectField(ids[i], "NetworkSettings.MacAddress") + errorOut(err, t, out) + + portCmd := exec.Command(dockerBinary, "port", ids[i], "1234") + ports[i], _, err = runCommandWithOutput(portCmd) + errorOut(err, t, out) + if ports[i] == "" { + t.Fatal("Port allocation failed") + } } - // TODO fix docker cp and /etc/hosts - out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "run", "-d", "--link", "c1:c1", "--name", "c2", "busybox", "sh", "-c", "while true;do sleep 1; done")) - if err != nil { - t.Fatal(err, out) + // Stop them all. + for _, id := range ids { + cmd := exec.Command(dockerBinary, "stop", id) + out, _, err := runCommandWithOutput(cmd) + if err != nil { + t.Fatal(err, out) + } } - contID := strings.TrimSpace(out) + // Create a new container and ensure it's not getting the IP or port of some stopped container. + { + runCmd := exec.Command(dockerBinary, "run", "-d", "-p", "1234", "busybox", "top") + out, _, err := runCommandWithOutput(runCmd) + errorOut(err, t, out) - f, err := os.Open(filepath.Join("/var/lib/docker/containers", contID, "hosts")) - if err != nil { - t.Fatal(err) + id := strings.TrimSpace(out) + ip, err := inspectField(id, "NetworkSettings.IPAddress") + errorOut(err, t, out) + + portCmd := exec.Command(dockerBinary, "port", id, "1234") + port, _, err := runCommandWithOutput(portCmd) + errorOut(err, t, out) + + for i := range ids { + if ip == ips[i] { + t.Fatalf("Conflicting IP: %s", ip) + } + if port == ports[i] { + t.Fatalf("Conflicting port: %s", port) + } + } } - originalContent, err := ioutil.ReadAll(f) - f.Close() + // Start the containers back, and ensure they are getting the same IPs, MACs and ports. + for i, id := range ids { + runCmd := exec.Command(dockerBinary, "start", id) + out, _, err := runCommandWithOutput(runCmd) + errorOut(err, t, out) - if err != nil { - t.Fatal(err) - } + ip, err := inspectField(id, "NetworkSettings.IPAddress") + errorOut(err, t, out) - out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "restart", "-t", "0", "c1")) - if err != nil { - t.Fatal(err, out) - } + mac, err := inspectField(id, "NetworkSettings.MacAddress") + errorOut(err, t, out) - f, err = os.Open(filepath.Join("/var/lib/docker/containers", contID, "hosts")) - if err != nil { - t.Fatal(err) - } + portCmd := exec.Command(dockerBinary, "port", ids[i], "1234") + port, _, err := runCommandWithOutput(portCmd) + errorOut(err, t, out) - newContent, err := ioutil.ReadAll(f) - f.Close() - - if err != nil { - t.Fatal(err) - } - - if strings.TrimSpace(string(originalContent)) == strings.TrimSpace(string(newContent)) { - t.Fatalf("expected /etc/hosts to be updated, but wasn't") + if ips[i] != ip { + t.Fatalf("Container started with a different IP: %s != %s", ip, ips[i]) + } + if macs[i] != mac { + t.Fatalf("Container started with a different MAC: %s != %s", mac, macs[i]) + } + if ports[i] != port { + t.Fatalf("Container started with a different port: %s != %s", port, ports[i]) + } } deleteAllContainers() - - logDone("run - /etc/hosts updated in parent when restart") + logDone("run - ips and ports must not change") } // Ensure that CIDFile gets deleted if it's empty