nomad: remove PortsOvercommited in favor of NetworkIndex

nomad/structs/funcs.go

@@ -41,24 +41,11 @@ func FilterTerminalAllocs(allocs []*Allocation) []*Allocation {
 	return allocs[:n]
 }
 
-// PortsOvercommited checks if any ports are over-committed.
-// This does not handle CIDR subsets, and computes for the entire
-// CIDR block currently.
-func PortsOvercommited(r *Resources) bool {
-	for _, net := range r.Networks {
-		ports := make(map[int]struct{})
-		for _, port := range net.ReservedPorts {
-			if _, ok := ports[port]; ok {
-				return true
-			}
-			ports[port] = struct{}{}
-		}
-	}
-	return false
-}
-
-// AllocsFit checks if a given set of allocations will fit on a node
-func AllocsFit(node *Node, allocs []*Allocation) (bool, *Resources, error) {
+// AllocsFit checks if a given set of allocations will fit on a node.
+// The netIdx can optionally be provided if its already been computed.
+// If the netIdx is provided, it is assumed that the client has already
+// ensured there are no collisions.
+func AllocsFit(node *Node, allocs []*Allocation, netIdx *NetworkIndex) (bool, *Resources, error) {
 	// Compute the utilization from zero
 	used := new(Resources)
 
@@ -82,8 +69,16 @@ func AllocsFit(node *Node, allocs []*Allocation) (bool, *Resources, error) {
 		return false, used, nil
 	}
 
-	// Ensure ports are not over commited
-	if PortsOvercommited(used) {
+	// Create the network index if missing
+	if netIdx == nil {
+		netIdx = NewNetworkIndex()
+		if netIdx.SetNode(node) || netIdx.AddAllocs(allocs) {
+			return false, used, nil
+		}
+	}
+
+	// Check if the network is overcommitted
+	if netIdx.Overcommitted() {
 		return false, used, nil
 	}
 

nomad/structs/funcs_test.go

@@ -39,25 +39,48 @@ func TestFilterTerminalALlocs(t *testing.T) {
 	}
 }
 
-func TestPortsOvercommitted(t *testing.T) {
-	r := &Resources{
-		Networks: []*NetworkResource{
-			&NetworkResource{
-				ReservedPorts: []int{22, 80},
-			},
-			&NetworkResource{
-				ReservedPorts: []int{22, 80},
+func TestAllocsFit_PortsOvercommitted(t *testing.T) {
+	n := &Node{
+		Resources: &Resources{
+			Networks: []*NetworkResource{
+				&NetworkResource{
+					CIDR:  "10.0.0.0/8",
+					MBits: 100,
+				},
 			},
 		},
 	}
-	if PortsOvercommited(r) {
-		t.Fatalf("bad")
+
+	a1 := &Allocation{
+		TaskResources: map[string]*Resources{
+			"web": &Resources{
+				Networks: []*NetworkResource{
+					&NetworkResource{
+						IP:            "10.0.0.1",
+						MBits:         50,
+						ReservedPorts: []int{8000},
+					},
+				},
+			},
+		},
 	}
 
-	// Overcommit 22
-	r.Networks[1].ReservedPorts[1] = 22
-	if !PortsOvercommited(r) {
-		t.Fatalf("bad")
+	// Should fit one allocation
+	fit, _, err := AllocsFit(n, []*Allocation{a1}, nil)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if !fit {
+		t.Fatalf("Bad")
+	}
+
+	// Should not fit second allocation
+	fit, _, err = AllocsFit(n, []*Allocation{a1, a1}, nil)
+	if err != nil {
+		t.Fatalf("err: %v", err)
+	}
+	if fit {
+		t.Fatalf("Bad")
 	}
 }
 
@@ -82,7 +105,7 @@ func TestAllocsFit(t *testing.T) {
 			IOPS:     50,
 			Networks: []*NetworkResource{
 				&NetworkResource{
-					CIDR:          "10.0.0.0/8",
+					IP:            "10.0.0.1",
 					MBits:         50,
 					ReservedPorts: []int{80},
 				},
@@ -98,7 +121,7 @@ func TestAllocsFit(t *testing.T) {
 			IOPS:     50,
 			Networks: []*NetworkResource{
 				&NetworkResource{
-					CIDR:          "10.0.0.0/8",
+					IP:            "10.0.0.1",
 					MBits:         50,
 					ReservedPorts: []int{8000},
 				},
@@ -107,7 +130,7 @@ func TestAllocsFit(t *testing.T) {
 	}
 
 	// Should fit one allocation
-	fit, used, err := AllocsFit(n, []*Allocation{a1})
+	fit, used, err := AllocsFit(n, []*Allocation{a1}, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}
@@ -124,7 +147,7 @@ func TestAllocsFit(t *testing.T) {
 	}
 
 	// Should not fit second allocation
-	fit, used, err = AllocsFit(n, []*Allocation{a1, a1})
+	fit, used, err = AllocsFit(n, []*Allocation{a1, a1}, nil)
 	if err != nil {
 		t.Fatalf("err: %v", err)
 	}

nomad/structs/network.go

@@ -35,8 +35,20 @@ func NewNetworkIndex() *NetworkIndex {
 	}
 }
 
-// SetNode is used to setup the available network resources
-func (idx *NetworkIndex) SetNode(node *Node) {
+// Overcommitted checks if the network is overcommitted
+func (idx *NetworkIndex) Overcommitted() bool {
+	for device, used := range idx.UsedBandwidth {
+		avail := idx.AvailBandwidth[device]
+		if used > avail {
+			return true
+		}
+	}
+	return false
+}
+
+// SetNode is used to setup the available network resources. Returns
+// true if there is a collision
+func (idx *NetworkIndex) SetNode(node *Node) (collide bool) {
 	// Add the available CIDR blocks
 	for _, n := range node.Resources.Networks {
 		if n.CIDR != "" {
@@ -48,26 +60,34 @@ func (idx *NetworkIndex) SetNode(node *Node) {
 	// Add the reserved resources
 	if r := node.Reserved; r != nil {
 		for _, n := range r.Networks {
-			idx.AddReserved(n)
+			if idx.AddReserved(n) {
+				collide = true
+			}
 		}
 	}
+	return
 }
 
-// AddAllocs is used to add the used network resources
-func (idx *NetworkIndex) AddAllocs(allocs []*Allocation) {
+// AddAllocs is used to add the used network resources. Returns
+// true if there is a collision
+func (idx *NetworkIndex) AddAllocs(allocs []*Allocation) (collide bool) {
 	for _, alloc := range allocs {
 		for _, task := range alloc.TaskResources {
 			if len(task.Networks) == 0 {
 				continue
 			}
 			n := task.Networks[0]
-			idx.AddReserved(n)
+			if idx.AddReserved(n) {
+				collide = true
+			}
 		}
 	}
+	return
 }
 
-// AddReserved is used to add a reserved network usage
-func (idx *NetworkIndex) AddReserved(n *NetworkResource) {
+// AddReserved is used to add a reserved network usage, returns true
+// if there is a port collision
+func (idx *NetworkIndex) AddReserved(n *NetworkResource) (collide bool) {
 	// Add the port usage
 	used := idx.UsedPorts[n.IP]
 	if used == nil {
@@ -75,11 +95,16 @@ func (idx *NetworkIndex) AddReserved(n *NetworkResource) {
 		idx.UsedPorts[n.IP] = used
 	}
 	for _, port := range n.ReservedPorts {
-		used[port] = struct{}{}
+		if _, ok := used[port]; ok {
+			collide = true
+		} else {
+			used[port] = struct{}{}
+		}
 	}
 
 	// Add the bandwidth
 	idx.UsedBandwidth[n.Device] += n.MBits
+	return
 }
 
 // yieldIP is used to iteratively invoke the callback with
@@ -107,7 +132,8 @@ func (idx *NetworkIndex) yieldIP(cb func(net *NetworkResource, ip net.IP) bool)
 	}
 }
 
-// AssignNetwork is used to assign network resources given an ask
+// AssignNetwork is used to assign network resources given an ask.
+// If the ask cannot be satisfied, returns nil
 func (idx *NetworkIndex) AssignNetwork(ask *NetworkResource) (out *NetworkResource) {
 	idx.yieldIP(func(n *NetworkResource, ip net.IP) (stop bool) {
 		// Convert the IP to a string

nomad/structs/network_test.go

@@ -6,6 +6,48 @@ import (
 	"testing"
 )
 
+func TestNetworkIndex_Overcommitted(t *testing.T) {
+	idx := NewNetworkIndex()
+
+	// Consume some network
+	reserved := &NetworkResource{
+		Device:        "eth0",
+		IP:            "192.168.0.100",
+		MBits:         505,
+		ReservedPorts: []int{8000, 9000},
+	}
+	collide := idx.AddReserved(reserved)
+	if collide {
+		t.Fatalf("bad")
+	}
+	if !idx.Overcommitted() {
+		t.Fatalf("have no resources")
+	}
+
+	// Add resources
+	n := &Node{
+		Resources: &Resources{
+			Networks: []*NetworkResource{
+				&NetworkResource{
+					Device: "eth0",
+					CIDR:   "192.168.0.100/32",
+					MBits:  1000,
+				},
+			},
+		},
+	}
+	idx.SetNode(n)
+	if idx.Overcommitted() {
+		t.Fatalf("have resources")
+	}
+
+	// Double up our ussage
+	idx.AddReserved(reserved)
+	if !idx.Overcommitted() {
+		t.Fatalf("should be overcommitted")
+	}
+}
+
 func TestNetworkIndex_SetNode(t *testing.T) {
 	idx := NewNetworkIndex()
 	n := &Node{
@@ -29,7 +71,10 @@ func TestNetworkIndex_SetNode(t *testing.T) {
 			},
 		},
 	}
-	idx.SetNode(n)
+	collide := idx.SetNode(n)
+	if collide {
+		t.Fatalf("bad")
+	}
 
 	if len(idx.AvailNetworks) != 1 {
 		t.Fatalf("Bad")
@@ -77,7 +122,10 @@ func TestNetworkIndex_AddAllocs(t *testing.T) {
 			},
 		},
 	}
-	idx.AddAllocs(allocs)
+	collide := idx.AddAllocs(allocs)
+	if collide {
+		t.Fatalf("bad")
+	}
 
 	if idx.UsedBandwidth["eth0"] != 70 {
 		t.Fatalf("Bad")
@@ -102,7 +150,10 @@ func TestNetworkIndex_AddReserved(t *testing.T) {
 		MBits:         20,
 		ReservedPorts: []int{8000, 9000},
 	}
-	idx.AddReserved(reserved)
+	collide := idx.AddReserved(reserved)
+	if collide {
+		t.Fatalf("bad")
+	}
 
 	if idx.UsedBandwidth["eth0"] != 20 {
 		t.Fatalf("Bad")
@@ -113,6 +164,12 @@ func TestNetworkIndex_AddReserved(t *testing.T) {
 	if _, ok := idx.UsedPorts["192.168.0.100"][9000]; !ok {
 		t.Fatalf("Bad")
 	}
+
+	// Try to reserve the same network
+	collide = idx.AddReserved(reserved)
+	if !collide {
+		t.Fatalf("bad")
+	}
 }
 
 func TestNetworkIndex_yieldIP(t *testing.T) {