Merge pull request #20500 from danwinship/egress-ip-cleanup

Don't auto-assign egress IPs that are no longer in use
openshift · Aug 13, 2018 · edad963 · edad963
2 parents 875f246 + 9132d51
commit edad963
Show file tree

Hide file tree

Showing 2 changed files with 43 additions and 5 deletions.
diff --git a/pkg/network/common/egressip.go b/pkg/network/common/egressip.go
@@ -230,7 +230,7 @@ func (eit *EgressIPTracker) UpdateHostSubnetEgress(hs *networkapi.HostSubnet) {
 		movedEgressIPs := make([]string, 0, node.requestedIPs.Len())
 		for _, ip := range node.requestedIPs.UnsortedList() {
 			eg := eit.egressIPs[ip]
-			if eg.assignedNodeIP == node.nodeIP {
+			if eg != nil && eg.assignedNodeIP == node.nodeIP {
 				movedEgressIPs = append(movedEgressIPs, ip)
 				eit.deleteNodeEgressIP(node, ip)
 			}
@@ -308,7 +308,9 @@ func (eit *EgressIPTracker) UpdateNetNamespaceEgress(netns *networkapi.NetNamesp
 	// Even IPs that weren't added/removed need to be considered "changed", to
 	// ensure we correctly process reorderings, duplicates added/removed, etc.
 	for _, ip := range newRequestedIPs.Intersection(oldRequestedIPs).UnsortedList() {
-		eit.egressIPChanged(eit.egressIPs[ip])
+		if eg := eit.egressIPs[ip]; eg != nil {
+			eit.egressIPChanged(eg)
+		}
 	}
 
 	eit.syncEgressIPs()
@@ -332,7 +334,7 @@ func (eit *EgressIPTracker) egressIPActive(eg *egressIPInfo) (bool, error) {
 	}
 	for _, ip := range eg.namespaces[0].requestedIPs {
 		eg2 := eit.egressIPs[ip]
-		if eg2 != eg && len(eg2.nodes) == 1 && eg2.nodes[0] == eg.nodes[0] {
+		if eg2 != nil && eg2 != eg && len(eg2.nodes) == 1 && eg2.nodes[0] == eg.nodes[0] {
 			return false, fmt.Errorf("Multiple EgressIPs (%s, %s) for VNID %d on node %s", eg.ip, eg2.ip, eg.namespaces[0].vnid, eg.nodes[0].nodeIP)
 		}
 	}
@@ -358,6 +360,12 @@ func (eit *EgressIPTracker) syncEgressIPs() {
 		eit.syncEgressNamespaceState(ns)
 	}
 
+	for eg := range changedEgressIPs {
+		if len(eg.namespaces) == 0 && len(eg.nodes) == 0 {
+			delete(eit.egressIPs, eg.ip)
+		}
+	}
+
 	if eit.updateEgressCIDRs {
 		eit.updateEgressCIDRs = false
 		if eit.nodesWithCIDRs > 0 {
@@ -539,7 +547,7 @@ func (eit *EgressIPTracker) ReallocateEgressIPs() map[string][]string {
 
 	// Allocate pending egress IPs that can only go to a single node
 	for egressIP, eip := range eit.egressIPs {
-		if alreadyAllocated[egressIP] {
+		if alreadyAllocated[egressIP] || len(eip.namespaces) == 0 {
 			continue
 		}
 		nodeName, otherNodes := eit.findEgressIPAllocation(eip.parsed, allocation)
@@ -551,7 +559,7 @@ func (eit *EgressIPTracker) ReallocateEgressIPs() map[string][]string {
 	}
 	// Allocate any other pending egress IPs that we can
 	for egressIP, eip := range eit.egressIPs {
-		if alreadyAllocated[egressIP] {
+		if alreadyAllocated[egressIP] || len(eip.namespaces) == 0 {
 			continue
 		}
 		nodeName, _ := eit.findEgressIPAllocation(eip.parsed, allocation)

diff --git a/pkg/network/common/egressip_test.go b/pkg/network/common/egressip_test.go
@@ -946,6 +946,36 @@ func TestEgressCIDRAllocation(t *testing.T) {
 	if err != nil {
 		t.Fatalf("%v", err)
 	}
+
+	// Changing the EgressIPs of a namespace should drop the old allocation and create a new one
+	updateNetNamespaceEgress(eit, &networkapi.NetNamespace{
+		NetID:     46,
+		EgressIPs: []string{"172.17.0.202"}, // was 172.17.0.200
+	})
+	err = w.assertChanges(
+		"release 172.17.0.200 on 172.17.0.4",
+		"namespace 46 dropped",
+		"update egress CIDRs",
+	)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
+
+	allocation = eit.ReallocateEgressIPs()
+	for _, ip := range allocation["node-4"] {
+		if ip == "172.17.0.200" {
+			t.Fatalf("reallocation failed to drop unused egress IP 172.17.0.200: %#v", allocation)
+		}
+	}
+	updateAllocations(eit, allocation)
+	err = w.assertChanges(
+		"claim 172.17.0.202 on 172.17.0.4 for namespace 46",
+		"namespace 46 via 172.17.0.202 on 172.17.0.4",
+		"update egress CIDRs",
+	)
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
 }
 
 func TestEgressNodeRenumbering(t *testing.T) {