Merge pull request #310 from aluzzardi/random-port-detection

Port filter: Random port assignment conflict detection.

scheduler/filter/port.go

@@ -33,16 +33,41 @@ func (p *PortFilter) Filter(config *dockerclient.ContainerConfig, nodes []*clust
 
 func (p *PortFilter) portAlreadyInUse(node *cluster.Node, requested dockerclient.PortBinding) bool {
 	for _, c := range node.Containers() {
-		for _, port := range c.Info.HostConfig.PortBindings {
-			for _, binding := range port {
-				if binding.HostPort == requested.HostPort {
-					// Another container on the same host is binding on the same
-					// port/protocol.  Verify if they are requesting the same
-					// binding IP, or if the other container is already binding on
-					// every interface.
-					if requested.HostIp == binding.HostIp || bindsAllInterfaces(requested) || bindsAllInterfaces(binding) {
-						return true
-					}
+		// HostConfig.PortBindings contains the requested ports.
+		// NetworkSettings.Ports contains the actual ports.
+		//
+		// We have to check both because:
+		// 1/ If the port was not specifically bound (e.g. -p 80), then
+		//    HostConfig.PortBindings.HostPort will be empty and we have to check
+		//    NetworkSettings.Port.HostPort to find out which port got dynamically
+		//    allocated.
+		// 2/ If the port was bound (e.g. -p 80:80) but the container is stopped,
+		//    NetworkSettings.Port will be null and we have to check
+		//    HostConfig.PortBindings to find out the mapping.
+
+		if p.compare(requested, c.Info.HostConfig.PortBindings) || p.compare(requested, c.Info.NetworkSettings.Ports) {
+			return true
+		}
+	}
+	return false
+}
+
+func (p *PortFilter) compare(requested dockerclient.PortBinding, bindings map[string][]dockerclient.PortBinding) bool {
+	for _, binding := range bindings {
+		for _, b := range binding {
+			if b.HostPort == "" {
+				// Skip undefined HostPorts. This happens in bindings that
+				// didn't explicitely specify an external port.
+				continue
+			}
+
+			if b.HostPort == requested.HostPort {
+				// Another container on the same host is binding on the same
+				// port/protocol.  Verify if they are requesting the same
+				// binding IP, or if the other container is already binding on
+				// every interface.
+				if requested.HostIp == b.HostIp || bindsAllInterfaces(requested) || bindsAllInterfaces(b) {
+					return true
 				}
 			}
 		}

scheduler/filter/port_test.go

@@ -175,3 +175,76 @@ func TestPortFilterDifferentInterfaces(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Contains(t, result, nodes[1])
 }
+
+func TestPortFilterRandomAssignment(t *testing.T) {
+	var (
+		p     = PortFilter{}
+		nodes = []*cluster.Node{
+			cluster.NewNode("node-1", 0),
+			cluster.NewNode("node-2", 0),
+			cluster.NewNode("node-3", 0),
+		}
+		result []*cluster.Node
+		err    error
+	)
+
+	// Simulate a container that requested to map 80 to a random port.
+	// In this case, HostConfig.PortBindings should contain a binding with no
+	// HostPort defined and NetworkSettings.Ports should contain the actual
+	// mapped port.
+	container := &cluster.Container{
+		Container: dockerclient.Container{Id: "c1"},
+		Info: dockerclient.ContainerInfo{
+			HostConfig: &dockerclient.HostConfig{
+				PortBindings: map[string][]dockerclient.PortBinding{
+					"80/tcp": {
+						{
+							HostIp:   "",
+							HostPort: "",
+						},
+					},
+				},
+			},
+			NetworkSettings: struct {
+				IpAddress   string
+				IpPrefixLen int
+				Gateway     string
+				Bridge      string
+				Ports       map[string][]dockerclient.PortBinding
+			}{
+				Ports: map[string][]dockerclient.PortBinding{
+					"80/tcp": {
+						{
+							HostIp:   "127.0.0.1",
+							HostPort: "1234",
+						},
+					},
+				},
+			},
+		},
+	}
+
+	assert.NoError(t, nodes[0].AddContainer(container))
+
+	// Request port 80.
+	config := &dockerclient.ContainerConfig{
+		HostConfig: dockerclient.HostConfig{
+			PortBindings: makeBinding("", "80"),
+		},
+	}
+
+	// Since port "80" has been mapped to "1234", we should be able to request "80".
+	result, err = p.Filter(config, nodes)
+	assert.NoError(t, err)
+	assert.Equal(t, result, nodes)
+
+	// However, we should not be able to request "1234" since it has been used for a random assignment.
+	config = &dockerclient.ContainerConfig{
+		HostConfig: dockerclient.HostConfig{
+			PortBindings: makeBinding("", "1234"),
+		},
+	}
+	result, err = p.Filter(config, nodes)
+	assert.NoError(t, err)
+	assert.NotContains(t, result, nodes[0])
+}