Merge pull request #760 from trozet/bz2005480

Bug 2005480: [4.8z] Remove waiting for namespace and namespace lock contention

go-controller/pkg/ovn/address_set/fake_address_set.go

@@ -104,12 +104,6 @@ func (f *FakeAddressSetFactory) removeAddressSet(name string) {
 	delete(f.sets, name)
 }
 
-// ExpectNoAddressSet ensures the named address set does not exist
-func (f *FakeAddressSetFactory) ExpectNoAddressSet(name string) {
-	_, ok := f.sets[name]
-	gomega.Expect(ok).To(gomega.BeFalse())
-}
-
 // ExpectAddressSetWithIPs ensures the named address set exists with the given set of IPs
 func (f *FakeAddressSetFactory) ExpectAddressSetWithIPs(name string, ips []string) {
 	var lenAddressSet int

go-controller/pkg/ovn/egressfirewall.go

@@ -179,12 +179,12 @@ func (oc *Controller) syncEgressFirewall(egressFirwalls []interface{}) {
 
 func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.EgressFirewall) error {
 	klog.Infof("Adding egressFirewall %s in namespace %s", egressFirewall.Name, egressFirewall.Namespace)
-	nsInfo, err := oc.waitForNamespaceLocked(egressFirewall.Namespace)
+	nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(egressFirewall.Namespace, false)
 	if err != nil {
-		return fmt.Errorf("failed to wait for namespace %s event (%v)",
+		return fmt.Errorf("failed to ensure namespace locked for egress firewall: %s, error: %v",
 			egressFirewall.Namespace, err)
 	}
-	defer nsInfo.Unlock()
+	defer nsUnlock()
 
 	if nsInfo.egressFirewall != nil {
 		return fmt.Errorf("error attempting to add egressFirewall %s to namespace %s when it already has an egressFirewall",
@@ -240,12 +240,13 @@ func (oc *Controller) addEgressFirewall(egressFirewall *egressfirewallapi.Egress
 
 func (oc *Controller) updateEgressFirewall(oldEgressFirewall, newEgressFirewall *egressfirewallapi.EgressFirewall) error {
 	// block all external traffic in this namespace
-	nsInfo, err := oc.waitForNamespaceLocked(newEgressFirewall.Namespace)
+	nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(newEgressFirewall.Namespace, true)
 	if err != nil {
-		return fmt.Errorf("cannot update egressfirewall in %s:%v", newEgressFirewall.Namespace, err)
+		return fmt.Errorf("cannot update egressfirewall %s, failed to ensure namespace: %v",
+			newEgressFirewall.Namespace, err)
 	}
 	addressSet := nsInfo.addressSet
-	nsInfo.Unlock()
+	nsUnlock()
 	priority, err := strconv.Atoi(types.EgressFirewallStartPriority)
 	if err != nil {
 		return fmt.Errorf("cannot update egressfirewall in %s:%v", newEgressFirewall.Namespace, err)
@@ -280,7 +281,7 @@ func (oc *Controller) deleteEgressFirewall(egressFirewall *egressfirewallapi.Egr
 	klog.Infof("Deleting egress Firewall %s in namespace %s", egressFirewall.Name, egressFirewall.Namespace)
 	deleteDNS := false
 
-	nsInfo := oc.getNamespaceLocked(egressFirewall.Namespace)
+	nsInfo, nsUnlock := oc.getNamespaceLocked(egressFirewall.Namespace, false)
 	if nsInfo != nil {
 		// clear it so an error does not prevent future egressFirewalls
 		for _, rule := range nsInfo.egressFirewall.egressRules {
@@ -290,7 +291,7 @@ func (oc *Controller) deleteEgressFirewall(egressFirewall *egressfirewallapi.Egr
 			}
 		}
 		nsInfo.egressFirewall = nil
-		nsInfo.Unlock()
+		nsUnlock()
 	}
 	if deleteDNS {
 		oc.egressFirewallDNS.Delete(egressFirewall.Namespace)

go-controller/pkg/ovn/egressgw.go

@@ -82,11 +82,11 @@ func (oc *Controller) addPodExternalGWForNamespace(namespace string, pod *kapi.P
 	}
 	klog.Infof("Adding routes for external gateway pod: %s, next hops: %q, namespace: %s, bfd-enabled: %t",
 		pod.Name, gws, namespace, egress.bfdEnabled)
-	nsInfo, err := oc.waitForNamespaceLocked(namespace)
+	nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(namespace, false)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to ensure namespace locked: %v", err)
 	}
-	defer nsInfo.Unlock()
+	defer nsUnlock()
 	nsInfo.routingExternalPodGWs[pod.Name] = egress
 	return oc.addGWRoutesForNamespace(namespace, egress, nsInfo)
 }
@@ -117,6 +117,11 @@ func (oc *Controller) addGWRoutesForNamespace(namespace string, egress gatewayIn
 					continue
 				}
 
+				// if route was already programmed, skip it
+				if foundGR, ok := nsInfo.podExternalRoutes[podIP.IP][gw.String()]; ok && foundGR == gr {
+					continue
+				}
+
 				mask := GetIPFullMask(podIP.IP)
 				nbctlArgs := []string{"--may-exist", "--policy=src-ip", "--ecmp-symmetric-reply",
 					"lr-route-add", gr, podIP.IP + mask, gw.String()}
@@ -159,11 +164,11 @@ func (oc *Controller) deletePodExternalGW(pod *kapi.Pod) {
 
 // deletePodGwRoutesForNamespace handles deleting all routes in a namespace for a specific pod GW
 func (oc *Controller) deletePodGWRoutesForNamespace(pod, namespace string) {
-	nsInfo := oc.getNamespaceLocked(namespace)
+	nsInfo, nsUnlock := oc.getNamespaceLocked(namespace, false)
 	if nsInfo == nil {
 		return
 	}
-	defer nsInfo.Unlock()
+	defer nsUnlock()
 	// check if any gateways were stored for this pod
 	foundGws, ok := nsInfo.routingExternalPodGWs[pod]
 	if !ok {
@@ -243,11 +248,12 @@ func (oc *Controller) deleteGWRoutesForNamespace(nsInfo *namespaceInfo) {
 // deleteGwRoutesForPod handles deleting all routes to gateways for a pod IP on a specific GR
 func (oc *Controller) deleteGWRoutesForPod(namespace string, podIPNets []*net.IPNet) {
 	// delete src-ip cached route to GR
-	nsInfo := oc.getNamespaceLocked(namespace)
+	nsInfo, nsUnlock := oc.getNamespaceLocked(namespace, false)
 	if nsInfo == nil {
 		return
 	}
-	defer nsInfo.Unlock()
+	defer nsUnlock()
+
 	for _, podIPNet := range podIPNets {
 		pod := podIPNet.IP.String()
 		if gwToGr, ok := nsInfo.podExternalRoutes[pod]; ok {
@@ -276,11 +282,11 @@ func (oc *Controller) deleteGWRoutesForPod(namespace string, podIPNets []*net.IP
 
 // addEgressGwRoutesForPod handles adding all routes to gateways for a pod on a specific GR
 func (oc *Controller) addGWRoutesForPod(gateways []gatewayInfo, podIfAddrs []*net.IPNet, namespace, node string) error {
-	nsInfo, err := oc.waitForNamespaceLocked(namespace)
-	if err != nil {
-		return err
+	nsInfo, nsUnlock := oc.getNamespaceLocked(namespace, false)
+	if nsInfo == nil {
+		return fmt.Errorf("unable to get namespace: %s", namespace)
 	}
-	defer nsInfo.Unlock()
+	defer nsUnlock()
 	gr := util.GetGatewayRouterFromNode(node)
 	for _, podIPNet := range podIfAddrs {
 		for _, gateway := range gateways {
@@ -292,6 +298,10 @@ func (oc *Controller) addGWRoutesForPod(gateways []gatewayInfo, podIfAddrs []*ne
 				podIP := podIPNet.IP.String()
 				for _, gw := range gws {
 					gwStr := gw.String()
+					// if route was already programmed, skip it
+					if foundGR, ok := nsInfo.podExternalRoutes[podIP][gwStr]; ok && foundGR == gr {
+						continue
+					}
 					mask := GetIPFullMask(podIP)
 					nbctlArgs := []string{"--may-exist", "--policy=src-ip", "--ecmp-symmetric-reply",
 						"lr-route-add", gr, podIP + mask, gw.String()}

go-controller/pkg/ovn/egressgw_test.go

@@ -97,6 +97,54 @@ var _ = ginkgo.Describe("OVN Egress Gateway Operations", func() {
 		}, table.Entry("No BFD", false, "ovn-nbctl --timeout=15 --may-exist --policy=src-ip --ecmp-symmetric-reply lr-route-add GR_node1 10.128.1.3/32 9.0.0.1"),
 			table.Entry("BFD Enabled", true, "ovn-nbctl --timeout=15 --may-exist --bfd --policy=src-ip --ecmp-symmetric-reply lr-route-add GR_node1 10.128.1.3/32 9.0.0.1 rtoe-GR_node1"))
 
+		table.DescribeTable("reconciles an new pod with namespace single exgw annotation already set with pod event first", func(bfd bool, expectedNbctl string) {
+			app.Action = func(ctx *cli.Context) error {
+
+				namespaceT := *newNamespace("namespace1")
+				namespaceT.Annotations = map[string]string{"k8s.ovn.org/routing-external-gws": "9.0.0.1"}
+				if bfd {
+					namespaceT.Annotations["k8s.ovn.org/bfd-enabled"] = ""
+				}
+				t := newTPod(
+					"node1",
+					"10.128.1.0/24",
+					"10.128.1.2",
+					"10.128.1.1",
+					"myPod",
+					"10.128.1.3",
+					"0a:58:0a:80:01:03",
+					namespaceT.Name,
+				)
+
+				t.baseCmds(fExec)
+				fakeOvn.start(ctx,
+					&v1.PodList{
+						Items: []v1.Pod{
+							*newPod(t.namespace, t.podName, t.nodeName, t.podIP),
+						},
+					},
+				)
+				t.populateLogicalSwitchCache(fakeOvn)
+				fExec.AddFakeCmd(&ovntest.ExpectedCmd{
+					Cmd:    expectedNbctl,
+					Output: "\n",
+				})
+				fakeOvn.controller.WatchNamespaces()
+				fakeOvn.controller.WatchPods()
+
+				_, err := fakeOvn.fakeClient.KubeClient.CoreV1().Namespaces().Create(context.TODO(), &namespaceT, metav1.CreateOptions{})
+				gomega.Expect(err).NotTo(gomega.HaveOccurred())
+
+				gomega.Eventually(func() string { return getPodAnnotations(fakeOvn.fakeClient.KubeClient, t.namespace, t.podName) }, 2).Should(gomega.MatchJSON(`{"default": {"ip_addresses":["` + t.podIP + `/24"], "mac_address":"` + t.podMAC + `", "gateway_ips": ["` + t.nodeGWIP + `"], "ip_address":"` + t.podIP + `/24", "gateway_ip": "` + t.nodeGWIP + `"}}`))
+				gomega.Eventually(fExec.CalledMatchesExpected).Should(gomega.BeTrue(), fExec.ErrorDesc)
+				return nil
+			}
+
+			err := app.Run([]string{app.Name})
+			gomega.Expect(err).NotTo(gomega.HaveOccurred())
+		}, table.Entry("No BFD", false, "ovn-nbctl --timeout=15 --may-exist --policy=src-ip --ecmp-symmetric-reply lr-route-add GR_node1 10.128.1.3/32 9.0.0.1"),
+			table.Entry("BFD Enabled", true, "ovn-nbctl --timeout=15 --may-exist --bfd --policy=src-ip --ecmp-symmetric-reply lr-route-add GR_node1 10.128.1.3/32 9.0.0.1 rtoe-GR_node1"))
+
 		table.DescribeTable("reconciles an new pod with namespace double exgw annotation already set", func(bfd bool, expectedNbctl []string) {
 
 			app.Action = func(ctx *cli.Context) error {

go-controller/pkg/ovn/master.go

@@ -807,20 +807,11 @@ func (oc *Controller) ensureNodeLogicalNetwork(node *kapi.Node, hostSubnets []*n
 	if err = func() error {
 		hostNetworkNamespace := config.Kubernetes.HostNetworkNamespace
 		if hostNetworkNamespace != "" {
-			nsInfo, err := oc.waitForNamespaceLocked(hostNetworkNamespace)
+			nsInfo, nsUnlock, err := oc.ensureNamespaceLocked(hostNetworkNamespace, true)
 			if err != nil {
-				klog.Errorf("Failed to get namespace %s (%v)",
-					hostNetworkNamespace, err)
-				return err
-			}
-			defer nsInfo.Unlock()
-			if nsInfo.addressSet == nil {
-				nsInfo.addressSet, err = oc.createNamespaceAddrSetAllPods(hostNetworkNamespace)
-				if err != nil {
-					return fmt.Errorf("cannot create address set for namespace: %s,"+
-						"error: %v", hostNetworkNamespace, err)
-				}
+				return fmt.Errorf("failed to ensure namespace locked: %v", err)
 			}
+			defer nsUnlock()
 			if err = nsInfo.addressSet.AddIPs(hostNetworkPolicyIPs); err != nil {
 				return err
 			}