Merge pull request #18720 from danwinship/fix-egress-ip-reassignment

Automatic merge from submit-queue.

 Fix reassignment of egress IP after removal

When dropping an egress IP from eth0, we weren't updating our internal state to reflect that we had done that, so if you added it back again it wouldn't do everything it needed to do. (Introduced in #18121 but not discoverable until after #18547 was fixed.)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1547899

Author: openshift-merge-robot

pkg/network/node/egressip.go

@@ -213,6 +213,7 @@ func (eip *egressIPWatcher) deleteEgressIP(egressIP string) {
 		if err := eip.releaseEgressIP(egressIP, mark); err != nil {
 			utilruntime.HandleError(fmt.Errorf("Error releasing Egress IP %q: %v", egressIP, err))
 		}
+		node.assignedIPs.Delete(egressIP)
 	}
 
 	if ns.assignedIP == egressIP {

pkg/network/node/egressip_test.go

@@ -55,7 +55,7 @@ func TestEgressIP(t *testing.T) {
 	}
 	err = assertFlowChanges(origFlows, flows) // no changes
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 
 	// Assign NetNamespace.EgressIP first, then HostSubnet.EgressIP, with a remote EgressIP
@@ -75,7 +75,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 
 	ns42 := eip.namespacesByVNID[42]
@@ -99,7 +99,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
@@ -120,7 +120,7 @@ func TestEgressIP(t *testing.T) {
 	}
 	err = assertFlowChanges(origFlows, flows)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 
 	if eip.nodesByEgressIP["172.17.0.100"] != node3 || eip.nodesByEgressIP["172.17.0.101"] != node3 {
@@ -143,7 +143,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
@@ -169,7 +169,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 
 	ns44 := eip.namespacesByVNID[44]
@@ -193,7 +193,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
@@ -214,7 +214,7 @@ func TestEgressIP(t *testing.T) {
 	}
 	err = assertFlowChanges(origFlows, flows)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 
 	if eip.nodesByEgressIP["172.17.0.102"] != node4 || eip.nodesByEgressIP["172.17.0.103"] != node4 {
@@ -237,7 +237,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
@@ -263,14 +263,40 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
 	if eip.namespacesByVNID[44] != nil || eip.namespacesByEgressIP["172.17.0.102"] != nil || eip.nodesByEgressIP["172.17.0.102"] != node4 {
 		t.Fatalf("Unexpected eip state: %#v", eip)
 	}
 
+	// Add namespace EgressIP back again after having removed it...
+	eip.updateNamespaceEgress(44, "172.17.0.102")
+	err = assertNetlinkChange(eip, "claim 172.17.0.102")
+	if err != nil {
+		t.Fatalf("%v", err)
+	}
+	flows, err = ovsif.DumpFlows("")
+	if err != nil {
+		t.Fatalf("Unexpected error dumping flows: %v", err)
+	}
+	err = assertFlowChanges(origFlows, flows,
+		flowChange{
+			kind:  flowAdded,
+			match: []string{"table=100", "reg0=44", "0x0000002c->pkt_mark", "goto_table:101"},
+		},
+	)
+	if err != nil {
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
+	}
+	origFlows = flows
+
+	ns44 = eip.namespacesByVNID[44]
+	if ns44 == nil || eip.namespacesByEgressIP["172.17.0.102"] != ns44 || eip.nodesByEgressIP["172.17.0.102"] != node4 {
+		t.Fatalf("Unexpected eip state: %#v", eip)
+	}
+
 	// Drop remote node EgressIP
 	eip.updateNodeEgress("172.17.0.3", []string{"172.17.0.100"})
 	err = assertNoNetlinkChanges(eip)
@@ -292,7 +318,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
@@ -321,7 +347,7 @@ func TestEgressIP(t *testing.T) {
 		},
 	)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 	origFlows = flows
 
@@ -341,7 +367,7 @@ func TestEgressIP(t *testing.T) {
 	}
 	err = assertFlowChanges(origFlows, flows)
 	if err != nil {
-		t.Fatalf("Unexpected flow changes: %v", err)
+		t.Fatalf("Unexpected flow changes: %v\nOrig: %#v\nNew: %#v", err, origFlows, flows)
 	}
 }