From 103a4e0676f0099759e6f9a29ea1c9dc32766806 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Mon, 29 Sep 2014 15:51:36 -0700 Subject: [PATCH 1/6] Network Allocation: Proper rollback in case of failure allocation. Signed-off-by: Andrea Luzzardi --- daemon/container.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/daemon/container.go b/daemon/container.go index 0d97d726a..6bbdb7570 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -463,10 +463,12 @@ func (container *Container) allocateNetwork() error { if container.Config.PortSpecs != nil { if err := migratePortMappings(container.Config, container.hostConfig); err != nil { + eng.Job("release_interface", container.ID).Run() return err } container.Config.PortSpecs = nil if err := container.WriteHostConfig(); err != nil { + eng.Job("release_interface", container.ID).Run() return err } } @@ -496,6 +498,7 @@ func (container *Container) allocateNetwork() error { for port := range portSpecs { if err := container.allocatePort(eng, port, bindings); err != nil { + eng.Job("release_interface", container.ID).Run() return err } } @@ -1149,7 +1152,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") From ab4188c08d9eb63c84c461a9b922f2af479b5a04 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Mon, 29 Sep 2014 16:52:27 -0700 Subject: [PATCH 2/6] Container: Make allocateNetwork and releaseNetwork public. Since we are moving network allocation outside of container scope (it will be managed by create/destroy), those functions need to be accessible from the outside. Signed-off-by: Andrea Luzzardi --- daemon/container.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index 6bbdb7570..069d69189 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() error { mode := container.hostConfig.NetworkMode if container.Config.NetworkDisabled || !mode.IsPrivate() { return nil @@ -514,7 +514,7 @@ func (container *Container) allocateNetwork() error { return nil } -func (container *Container) releaseNetwork() { +func (container *Container) ReleaseNetwork() { if container.Config.NetworkDisabled { return } @@ -527,7 +527,7 @@ func (container *Container) releaseNetwork() { // 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() + container.ReleaseNetwork() // Disable all active links if container.activeLinks != nil { @@ -972,7 +972,7 @@ func (container *Container) initializeNetworking() error { container.Config.NetworkDisabled = true return container.buildHostnameAndHostsFiles("127.0.1.1") } - if err := container.allocateNetwork(); err != nil { + if err := container.AllocateNetwork(); err != nil { return err } return container.buildHostnameAndHostsFiles(container.NetworkSettings.IPAddress) From deffc572ced3909c0ecd77dd21686e0e67c0ea33 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Mon, 29 Sep 2014 16:56:10 -0700 Subject: [PATCH 3/6] Container: Add restore network functionality. RestoreNetwork() allows the container to restore its NetworkSettings (IP and public ports). Signed-off-by: Andrea Luzzardi --- daemon/container.go | 46 ++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 41 insertions(+), 5 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index 069d69189..0aa748727 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,14 +460,21 @@ 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 { - eng.Job("release_interface", container.ID).Run() return err } container.Config.PortSpecs = nil if err := container.WriteHostConfig(); err != nil { - eng.Job("release_interface", container.ID).Run() return err } } @@ -498,7 +504,6 @@ func (container *Container) AllocateNetwork() error { for port := range portSpecs { if err := container.allocatePort(eng, port, bindings); err != nil { - eng.Job("release_interface", container.ID).Run() return err } } @@ -524,6 +529,37 @@ 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 address. + job := eng.Job("allocate_interface", container.ID) + job.Setenv("RequestedIP", container.NetworkSettings.IPAddress) + 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() { From a4875937293f3b0a8ffc569608bbca40a456e9c8 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Mon, 29 Sep 2014 17:06:26 -0700 Subject: [PATCH 4/6] Stable Networking: Keep the same network settings across container restarts. This change will allocate network settings (IP and public ports) at container creation rather than start and keep them throughout the lifetime of the container (i.e. until it gets destroyed) instead of discarding them when the container is stopped. Signed-off-by: Andrea Luzzardi --- daemon/container.go | 14 ++-- daemon/create.go | 10 ++- daemon/delete.go | 2 + integration-cli/docker_cli_run_test.go | 108 ++++++++++++++++--------- 4 files changed, 87 insertions(+), 47 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index 0aa748727..1d38de311 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -552,7 +552,7 @@ func (container *Container) RestoreNetwork() error { } // Re-allocate any previously allocated ports. - for port, _ := range container.NetworkSettings.Ports { + for port := range container.NetworkSettings.Ports { if err := container.allocatePort(eng, port, container.NetworkSettings.Ports); err != nil { return err } @@ -563,8 +563,6 @@ func (container *Container) RestoreNetwork() error { // 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 { @@ -1008,8 +1006,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) } diff --git a/daemon/create.go b/daemon/create.go index 3309d034c..5adbc3a37 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/delete.go b/daemon/delete.go index 4501d91e2..9a74adbfa 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 01a3f5763..813c02fcb 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -1868,57 +1868,87 @@ 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, 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") + } + + 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 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) + portCmd := exec.Command(dockerBinary, "port", ids[i], "1234") + port, _, err := runCommandWithOutput(portCmd) + errorOut(err, t, out) - out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "restart", "-t", "0", "c1")) - if err != nil { - t.Fatal(err, out) - } - - f, err = os.Open(filepath.Join("/var/lib/docker/containers", contID, "hosts")) - if err != nil { - t.Fatal(err) - } - - 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 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 From f1087c5fcf070f151601f643418f3963facfea84 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Mon, 29 Sep 2014 17:10:15 -0700 Subject: [PATCH 5/6] Daemon: Restore network settings at startup. Signed-off-by: Andrea Luzzardi --- daemon/daemon.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/daemon/daemon.go b/daemon/daemon.go index a3c307dda..98457ef1a 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -367,6 +367,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 { From b669025949f1dba1ad3af9bab6711736863d6e24 Mon Sep 17 00:00:00 2001 From: Andrea Luzzardi Date: Fri, 3 Oct 2014 13:29:09 -0700 Subject: [PATCH 6/6] Stable MAC addresses: Add support for MAC address restoring. Signed-off-by: Andrea Luzzardi --- daemon/container.go | 3 ++- integration-cli/docker_cli_run_test.go | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/daemon/container.go b/daemon/container.go index 1d38de311..221edaad2 100644 --- a/daemon/container.go +++ b/daemon/container.go @@ -544,9 +544,10 @@ func (container *Container) RestoreNetwork() error { eng := container.daemon.eng - // Re-allocate the interface with the same IP address. + // 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 } diff --git a/integration-cli/docker_cli_run_test.go b/integration-cli/docker_cli_run_test.go index 813c02fcb..280a8761d 100644 --- a/integration-cli/docker_cli_run_test.go +++ b/integration-cli/docker_cli_run_test.go @@ -1870,7 +1870,7 @@ func TestRunMutableNetworkFiles(t *testing.T) { func TestRunStableIPAndPort(t *testing.T) { const nContainers = 2 - var ids, ips, ports [nContainers]string + 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++ { @@ -1880,12 +1880,16 @@ func TestRunStableIPAndPort(t *testing.T) { 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) @@ -1927,7 +1931,7 @@ func TestRunStableIPAndPort(t *testing.T) { } } - // Start the containers back, and ensure they are getting the same IPs and ports. + // 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) @@ -1935,6 +1939,10 @@ func TestRunStableIPAndPort(t *testing.T) { ip, err := inspectField(id, "NetworkSettings.IPAddress") errorOut(err, t, out) + + mac, err := inspectField(id, "NetworkSettings.MacAddress") + errorOut(err, t, out) + portCmd := exec.Command(dockerBinary, "port", ids[i], "1234") port, _, err := runCommandWithOutput(portCmd) errorOut(err, t, out) @@ -1942,6 +1950,9 @@ func TestRunStableIPAndPort(t *testing.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]) }