Merge pull request #8297 from aluzzardi/f-stable-ip

Stable Networking: Keep the same network settings during the entire container lifecycle.

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")

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
 }

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 {

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)

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)
 
-	originalContent, err := ioutil.ReadAll(f)
-	f.Close()
+		portCmd := exec.Command(dockerBinary, "port", id, "1234")
+		port, _, err := runCommandWithOutput(portCmd)
+		errorOut(err, t, out)
 
-	if err != nil {
-		t.Fatal(err)
+		for i := range ids {
+			if ip == ips[i] {
+				t.Fatalf("Conflicting IP: %s", ip)
+			}
+			if port == ports[i] {
+				t.Fatalf("Conflicting port: %s", port)
+			}
+		}
 	}
 
-	out, _, err = runCommandWithOutput(exec.Command(dockerBinary, "restart", "-t", "0", "c1"))
-	if err != nil {
-		t.Fatal(err, out)
-	}
+	// 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)
 
-	f, err = os.Open(filepath.Join("/var/lib/docker/containers", contID, "hosts"))
-	if err != nil {
-		t.Fatal(err)
-	}
+		ip, err := inspectField(id, "NetworkSettings.IPAddress")
+		errorOut(err, t, out)
 
-	newContent, err := ioutil.ReadAll(f)
-	f.Close()
+		mac, err := inspectField(id, "NetworkSettings.MacAddress")
+		errorOut(err, t, out)
 
-	if err != nil {
-		t.Fatal(err)
-	}
+		portCmd := exec.Command(dockerBinary, "port", ids[i], "1234")
+		port, _, err := runCommandWithOutput(portCmd)
+		errorOut(err, t, out)
 
-	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