Fix cleanup of LRP for deleted pods

pperiyasamy · pperiyasamy · commit f4e2c1799661 · 2025-11-07T13:01:19.000+01:00
When the EIP controller cleans up a stale EIP assignment for a pod, it also removes the pod object from the podAssignment cache. This is incorrect, as it prevents the EIP controller from processing the subsequent pod delete event. Scenario: 1. pod-1 is served by eip-1, both hosted on node1. 2. node1’s ovnkube-controller restarts. 3. Pod add event is received by the EIP controller — no changes. 4. eip-1 moves from node1 to node0. 5. The EIP controller receives the eip-1 add event. 6. eip-1 cleans up pod-1’s stale assignment (SNAT and LRP) for node1, but removes the pod object from the podAssignment cache when no other assignments found. 7. The EIP controller programs the LRP entry with node0’s transit IP as the next hop, but the pod assignment cache is not updated with new podAssignmentState. 8. The pod delete event is received by the EIP controller but ignored, since the pod object is missing from the assignment cache. So this commit fixes the issue by adding podAssignmentState back into podAssignment cache at step 7. Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com> (cherry picked from commit 16dedd1)
diff --git a/go-controller/pkg/ovn/egressip.go b/go-controller/pkg/ovn/egressip.go
@@ -818,7 +818,6 @@ func (e *EgressIPController) addPodEgressIPAssignments(ni util.NetInfo, name str
 			podIPs:               podIPs,
 			network:              ni,
 		}
-		e.podAssignment.Store(podKey, podState)
 	} else if podState.egressIPName == name || podState.egressIPName == "" {
 		// We do the setup only if this egressIP object is the one serving this pod OR
 		// podState.egressIPName can be empty if no re-routes were found in
@@ -863,6 +862,10 @@ func (e *EgressIPController) addPodEgressIPAssignments(ni util.NetInfo, name str
 		}
 		delete(podState.egressStatuses.statusMap, staleStatus)
 	}
+	// We store podState into podAssignment cache at this place for two reasons.
+	// 1. When podAssignmentState is newly created.
+	// 2. deletePodEgressIPAssignments might clean the podAssignment cache, make sure we add it back.
+	e.podAssignment.Store(podKey, podState)
 	// We need to proceed with add only under two conditions
 	// 1) egressNode present in at least one status is local to this zone
 	// (NOTE: The relation between egressIPName and nodeName is 1:1 i.e in the same object the given node will be present only in one status)
diff --git a/go-controller/pkg/ovn/egressip_test.go b/go-controller/pkg/ovn/egressip_test.go
@@ -11355,6 +11355,7 @@ var _ = ginkgo.Describe("OVN master EgressIP Operations cluster default network"
 				}
 				err = fakeOvn.controller.eIPC.reconcileEgressIP(nil, &eIP)
 				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				gomega.Expect(getPodAssignmentState(&egressPod)).NotTo(gomega.BeNil())
 
 				egressSVCServedPodsASv4, _ := buildEgressServiceAddressSets(nil)
 				egressIPServedPodsASv4, _ := buildEgressIPServedPodsAddressSets([]string{podV4IP}, types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName)
@@ -11495,6 +11496,145 @@ var _ = ginkgo.Describe("OVN master EgressIP Operations cluster default network"
 					egressSVCServedPodsASv4, egressIPServedPodsASv4, egressNodeIPsASv4,
 				}
 				gomega.Eventually(fakeOvn.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
+				// Since we called reconcileEgressIP manually with new eIP status in the above step, so update EIP object with
+				// same status as well.
+				_, err = fakeOvn.fakeClient.EgressIPClient.K8sV1().EgressIPs().Update(context.TODO(), &eIP, metav1.UpdateOptions{})
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				gomega.Eventually(fakeOvn.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
+
+				// Now delete local zone egressPod and check LRP is removed.
+				err = fakeOvn.fakeClient.KubeClient.CoreV1().Pods(egressPod.Namespace).Delete(context.TODO(), egressPod.Name, metav1.DeleteOptions{})
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+				egressIPServedPodsASv4, _ = buildEgressIPServedPodsAddressSets([]string{}, types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName)
+				node1Switch.QOSRules = []string{"default-QoS-UUID"}
+				expectedDatabaseState = []libovsdbtest.TestData{
+					&nbdb.LogicalRouterPolicy{
+						Priority: types.DefaultNoRereoutePriority,
+						Match: fmt.Sprintf("(ip4.src == $%s || ip4.src == $%s) && ip4.dst == $%s",
+							egressIPServedPodsASv4.Name, egressSVCServedPodsASv4.Name, egressNodeIPsASv4.Name),
+						Action:      nbdb.LogicalRouterPolicyActionAllow,
+						UUID:        "default-no-reroute-node-UUID",
+						Options:     map[string]string{"pkt_mark": types.EgressIPNodeConnectionMark},
+						ExternalIDs: getEgressIPLRPNoReRoutePodToNodeDbIDs(IPFamilyValueV4, types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName).GetExternalIDs(),
+					},
+					getNoReRouteReplyTrafficPolicy(types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName),
+					&nbdb.LogicalRouterPolicy{
+						Priority:    types.DefaultNoRereoutePriority,
+						Match:       "ip4.src == 10.128.0.0/14 && ip4.dst == 10.128.0.0/14",
+						Action:      nbdb.LogicalRouterPolicyActionAllow,
+						UUID:        "no-reroute-UUID",
+						ExternalIDs: getEgressIPLRPNoReRoutePodToPodDbIDs(IPFamilyValueV4, types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName).GetExternalIDs(),
+					},
+					&nbdb.LogicalRouterPolicy{
+						Priority:    types.DefaultNoRereoutePriority,
+						Match:       fmt.Sprintf("ip4.src == 10.128.0.0/14 && ip4.dst == %s", config.Gateway.V4JoinSubnet),
+						Action:      nbdb.LogicalRouterPolicyActionAllow,
+						UUID:        "no-reroute-service-UUID",
+						ExternalIDs: getEgressIPLRPNoReRoutePodToJoinDbIDs(IPFamilyValueV4, types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName).GetExternalIDs(),
+					},
+					&nbdb.LogicalRouter{
+						Name:     types.OVNClusterRouter,
+						UUID:     types.OVNClusterRouter + "-UUID",
+						Policies: []string{"no-reroute-UUID", "no-reroute-service-UUID", "default-no-reroute-node-UUID", "default-no-reroute-reply-traffic"},
+					},
+					&nbdb.LogicalRouter{
+						Name:    types.GWRouterPrefix + node1.Name,
+						UUID:    types.GWRouterPrefix + node1.Name + "-UUID",
+						Ports:   []string{types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node1.Name + "-UUID"},
+						Nat:     []string{"pod-node-nat-UUID1"},
+						Options: logicalRouterOptions,
+					},
+					&nbdb.LogicalRouter{
+						Name:    types.GWRouterPrefix + node2.Name,
+						UUID:    types.GWRouterPrefix + node2.Name + "-UUID",
+						Ports:   []string{types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node2.Name + "-UUID"},
+						Options: logicalRouterOptions,
+					},
+					&nbdb.LogicalRouter{
+						Name:    types.GWRouterPrefix + node3.Name,
+						UUID:    types.GWRouterPrefix + node3.Name + "-UUID",
+						Ports:   []string{types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node3.Name + "-UUID"},
+						Options: logicalRouterOptions,
+					},
+					// we still expect the SNAT at the end because we are not watching pod events that would remove the SNAT on pod deletion
+					&nbdb.NAT{
+						UUID:        "pod-node-nat-UUID1",
+						LogicalIP:   podV4IP,
+						ExternalIP:  node1IPv4,
+						ExternalIDs: nil,
+						Type:        nbdb.NATTypeSNAT,
+						Options: map[string]string{
+							"stateless": "false",
+						},
+					},
+					&nbdb.LogicalRouterPort{
+						UUID:     types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node2.Name + "-UUID",
+						Name:     types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node2.Name,
+						Networks: []string{node2LogicalRouterIfAddrV4},
+					},
+					&nbdb.LogicalRouterPort{
+						UUID:     types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node1.Name + "-UUID",
+						Name:     types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node1.Name,
+						Networks: []string{nodeLogicalRouterIfAddrV4},
+					},
+					&nbdb.LogicalRouterPort{
+						UUID:     types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node3.Name + "-UUID",
+						Name:     types.GWRouterToJoinSwitchPrefix + types.GWRouterPrefix + node3.Name,
+						Networks: []string{node3LogicalRouterIfAddrV4},
+					},
+					&nbdb.LogicalSwitchPort{
+						UUID: types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node1Name + "-UUID",
+						Name: types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node1Name,
+						Type: "router",
+						Options: map[string]string{
+							libovsdbops.RouterPort:      types.GWRouterToExtSwitchPrefix + "GR_" + node1Name,
+							"nat-addresses":             "router",
+							"exclude-lb-vips-from-garp": "true",
+						},
+					},
+					&nbdb.LogicalSwitchPort{
+						UUID: types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node2Name + "-UUID",
+						Name: types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node2Name,
+						Type: "router",
+						Options: map[string]string{
+							libovsdbops.RouterPort:      types.GWRouterToExtSwitchPrefix + "GR_" + node2Name,
+							"nat-addresses":             "router",
+							"exclude-lb-vips-from-garp": "true",
+						},
+					},
+					&nbdb.LogicalSwitchPort{
+						UUID: types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node3Name + "-UUID",
+						Name: types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node3Name,
+						Type: "router",
+						Options: map[string]string{
+							libovsdbops.RouterPort:      types.GWRouterToExtSwitchPrefix + "GR_" + node3Name,
+							"nat-addresses":             "router",
+							"exclude-lb-vips-from-garp": "true",
+						},
+					},
+					&nbdb.LogicalSwitch{
+						UUID:  types.ExternalSwitchPrefix + node1Name + "-UUID",
+						Name:  types.ExternalSwitchPrefix + node1Name,
+						Ports: []string{types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node1Name + "-UUID"},
+					},
+					&nbdb.LogicalSwitch{
+						UUID:  types.ExternalSwitchPrefix + node2Name + "-UUID",
+						Name:  types.ExternalSwitchPrefix + node2Name,
+						Ports: []string{types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node2Name + "-UUID"},
+					},
+					&nbdb.LogicalSwitch{
+						UUID:  types.ExternalSwitchPrefix + node3Name + "-UUID",
+						Name:  types.ExternalSwitchPrefix + node3Name,
+						Ports: []string{types.EXTSwitchToGWRouterPrefix + types.GWRouterPrefix + node3Name + "-UUID"},
+					},
+					node1Switch,
+					node2Switch,
+					node3Switch,
+					getDefaultQoSRule(false, types.DefaultNetworkName, fakeOvn.controller.eIPC.controllerName),
+					egressSVCServedPodsASv4, egressIPServedPodsASv4, egressNodeIPsASv4,
+				}
+				gomega.Eventually(fakeOvn.nbClient).Should(libovsdbtest.HaveData(expectedDatabaseState))
+
 				return nil
 			}
 			err := app.Run([]string{app.Name})

Original file line number	Diff line number	Diff line change
`@@ -818,7 +818,6 @@ func (e *EgressIPController) addPodEgressIPAssignments(ni util.NetInfo, name str`
`818`	`818`	`podIPs: podIPs,`
`819`	`819`	`network: ni,`
`820`	`820`	`}`
`821`		`- e.podAssignment.Store(podKey, podState)`
`822`	`821`	`} else if podState.egressIPName == name \|\| podState.egressIPName == "" {`
`823`	`822`	`// We do the setup only if this egressIP object is the one serving this pod OR`
`824`	`823`	`// podState.egressIPName can be empty if no re-routes were found in`
`@@ -863,6 +862,10 @@ func (e *EgressIPController) addPodEgressIPAssignments(ni util.NetInfo, name str`
`863`	`862`	`}`
`864`	`863`	`delete(podState.egressStatuses.statusMap, staleStatus)`
`865`	`864`	`}`
	`865`	`+ // We store podState into podAssignment cache at this place for two reasons.`
	`866`	`+ // 1. When podAssignmentState is newly created.`
	`867`	`+ // 2. deletePodEgressIPAssignments might clean the podAssignment cache, make sure we add it back.`
	`868`	`+ e.podAssignment.Store(podKey, podState)`
`866`	`869`	`// We need to proceed with add only under two conditions`
`867`	`870`	`// 1) egressNode present in at least one status is local to this zone`
`868`	`871`	`// (NOTE: The relation between egressIPName and nodeName is 1:1 i.e in the same object the given node will be present only in one status)`