imp: flow controller flows cleanup

using external id on the bridge flow controller will remove flows once pod is deleted
cleanup is done as best effort and will only log an error

Signed-off-by: Michael Filanov <[email protected]>

Author: filanov

internal/controller/flow_controller.go

@@ -67,6 +67,14 @@ func (r *FlowReconciler) Reconcile(ctx context.Context, pod *corev1.Pod) (ctrl.R
 
 	logr.Info("reconcile", "namespace", pod.Namespace, "name", pod.Name)
 
+	if pod.DeletionTimestamp != nil {
+		logr.Info("pod is being deleted, deleting flows if needed")
+		if err := r.Flows.DeletePodRailFlows(GenerateUint64FromString(string(pod.UID)), string(pod.UID)); err != nil {
+			logr.Error(err, "failed to delete flows")
+		}
+		return ctrl.Result{}, nil
+	}
+
 	if pod.Annotations == nil {
 		return reconcile.Result{}, nil
 	}
@@ -100,10 +108,6 @@ func (r *FlowReconciler) handlePodFlows(ctx context.Context, pod *corev1.Pod, re
 
 	cookie := GenerateUint64FromString(string(pod.UID))
 
-	if pod.DeletionTimestamp != nil {
-		logr.Info(fmt.Sprintf("pod %s/%s is being deleted, deleting flows", pod.Namespace, pod.Name))
-	}
-
 	var errs error
 
 	for _, ns := range relevantNetworkStatus {
@@ -121,13 +125,6 @@ func (r *FlowReconciler) handlePodFlows(ctx context.Context, pod *corev1.Pod, re
 			continue
 		}
 
-		if pod.DeletionTimestamp != nil {
-			if err = r.Flows.DeletePodRailFlows(cookie, bridge); err != nil {
-				logr.Error(err, fmt.Sprintf("failed to delete flows for rail %s", bridge))
-			}
-			continue
-		}
-
 		if err = r.Flows.AddPodRailFlows(cookie, rep, bridge, ns.IPs[0], ns.Mac); err != nil {
 			logr.Error(err, fmt.Sprintf("failed to add flows to rail %s", ns.Interface))
 			errs = multierr.Append(errs, err)

internal/controller/flow_controller_test.go

@@ -167,22 +167,10 @@ var _ = Describe("Pod Controller", func() {
 			Expect(k8sClient.Create(ctx, pod)).Should(Succeed())
 
 			pod.DeletionTimestamp = &metav1.Time{Time: time.Now()}
-
-			// rail1
-			execMock.EXPECT().
-				Execute("ovs-vsctl --no-heading --columns=name find Port external_ids:contIface=net1 external_ids:contPodUid="+string(pod.UID)).
-				Return("pod-vf-1", nil).Times(1)
-			execMock.EXPECT().Execute("ovs-vsctl iface-to-br pod-vf-1").Return("br-rail1", nil).Times(1)
-
-			// rail2
-			execMock.EXPECT().
-				Execute("ovs-vsctl --no-heading --columns=name find Port external_ids:contIface=net2 external_ids:contPodUid="+string(pod.UID)).
-				Return("pod-vf-2", nil)
-			execMock.EXPECT().Execute("ovs-vsctl iface-to-br pod-vf-2").Return("br-rail2", nil).Times(1)
 		})
 
 		It("delete flows on pod deletion", func() {
-			flowsMock.EXPECT().DeletePodRailFlows(gomock.Any(), gomock.Any()).Return(nil).Times(2)
+			flowsMock.EXPECT().DeletePodRailFlows(gomock.Any(), gomock.Any()).Return(nil).Times(1)
 			pod.DeletionTimestamp = &metav1.Time{Time: time.Now()}
 			result, err := flowController.Reconcile(ctx, pod)
 			Expect(err).Should(Succeed())
@@ -191,7 +179,6 @@ var _ = Describe("Pod Controller", func() {
 
 		It("failed to delete flows on pod deletion - do not fail reconciliation", func() {
 			flowsMock.EXPECT().DeletePodRailFlows(gomock.Any(), gomock.Any()).Return(fmt.Errorf("failed to delete flows"))
-			flowsMock.EXPECT().DeletePodRailFlows(gomock.Any(), gomock.Any()).Return(nil)
 			pod.DeletionTimestamp = &metav1.Time{Time: time.Now()}
 			result, err := flowController.Reconcile(ctx, pod)
 			Expect(err).Should(Succeed())

internal/controller/flows.go

@@ -17,22 +17,26 @@ package controller
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/Mellanox/spectrum-x-operator/pkg/exec"
 	libnetlink "github.com/Mellanox/spectrum-x-operator/pkg/lib/netlink"
+
+	"go.uber.org/multierr"
 )
 
 const (
 	railPeerIP      = "rail_peer_ip"
 	railUplink      = "rail_uplink"
+	RailPodID       = "rail_pod_id"
 	defaultPriority = 32768
 )
 
 //go:generate ../../bin/mockgen -destination mock_flows.go -source flows.go -package controller
 
 type FlowsAPI interface {
 	AddPodRailFlows(cookie uint64, vf, bridge, podIP, podMAC string) error
-	DeletePodRailFlows(cookie uint64, bridge string) error
+	DeletePodRailFlows(cookie uint64, podID string) error
 }
 
 var _ FlowsAPI = &Flows{}
@@ -102,10 +106,39 @@ func (f *Flows) AddPodRailFlows(cookie uint64, vf, bridge, podIP, podMAC string)
 	return nil
 }
 
-func (f *Flows) DeletePodRailFlows(cookie uint64, bridge string) error {
-	flow := fmt.Sprintf(`ovs-ofctl del-flows %s cookie=0x%x/-1`, bridge, cookie)
-	_, err := f.Exec.Execute(flow)
-	return err
+func (f *Flows) DeletePodRailFlows(cookie uint64, podID string) error {
+	out, err := f.Exec.Execute("ovs-vsctl list-br")
+	if err != nil {
+		return fmt.Errorf("failed to list bridges: %v", err)
+	}
+
+	bridges := strings.Split(out, "\n")
+
+	var errs error
+
+	for _, bridge := range bridges {
+		val, err := f.Exec.Execute(fmt.Sprintf("ovs-vsctl br-get-external-id %s %s", bridge, podID))
+		if err != nil {
+			errs = multierr.Append(errs, fmt.Errorf("failed to get external id for bridge %s: %v", bridge, err))
+			continue
+		}
+
+		if val == RailPodID {
+			flow := fmt.Sprintf(`ovs-ofctl del-flows %s cookie=0x%x/-1`, bridge, cookie)
+			out, err := f.Exec.Execute(flow)
+			if err != nil {
+				errs = multierr.Append(errs, fmt.Errorf("failed to delete flows for bridge %s: %v, output: %s", bridge, err, out))
+				continue
+			}
+			// clear external id
+			_, err = f.Exec.Execute(fmt.Sprintf(`ovs-vsctl br-set-external-id %s %s ""`, bridge, podID))
+			if err != nil {
+				errs = multierr.Append(errs, fmt.Errorf("failed to clear external id for bridge %s: %v", bridge, err))
+				continue
+			}
+		}
+	}
+	return errs
 }
 
 func (f *Flows) getTorMac(torIP string) (string, error) {

internal/controller/flows_test.go

@@ -23,11 +23,11 @@ import (
 
 	"github.com/Mellanox/spectrum-x-operator/pkg/exec"
 	mock_netlink "github.com/Mellanox/spectrum-x-operator/pkg/lib/netlink/mocks"
-	gomock "github.com/golang/mock/gomock"
-	vishvananda_netlink "github.com/vishvananda/netlink"
 
+	gomock "github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	vishvananda_netlink "github.com/vishvananda/netlink"
 )
 
 type containsSubstringMatcher struct {
@@ -223,15 +223,76 @@ var _ = Describe("Flows", func() {
 
 	Context("DeletePodRailFlows", func() {
 		It("should delete flows on pod deletion", func() {
+			execMock.EXPECT().Execute("ovs-vsctl list-br").Return("br-rail1\nbr-rail2", nil)
+			execMock.EXPECT().Execute("ovs-vsctl br-get-external-id br-rail1 test-pod-uid").
+				Return("rail_pod_id", nil)
+			execMock.EXPECT().Execute("ovs-vsctl br-get-external-id br-rail2 test-pod-uid").
+				Return("rail_pod_id", nil)
 			execMock.EXPECT().Execute("ovs-ofctl del-flows br-rail1 cookie=0x5/-1").Return("", nil)
-			err := flows.DeletePodRailFlows(0x5, "br-rail1")
+			execMock.EXPECT().Execute("ovs-ofctl del-flows br-rail2 cookie=0x5/-1").Return("", nil)
+			execMock.EXPECT().Execute(`ovs-vsctl br-set-external-id br-rail1 test-pod-uid ""`).Return("", nil)
+			execMock.EXPECT().Execute(`ovs-vsctl br-set-external-id br-rail2 test-pod-uid ""`).Return("", nil)
+			err := flows.DeletePodRailFlows(0x5, "test-pod-uid")
 			Expect(err).Should(Succeed())
 		})
 
-		It("should return error if ovs-ofctl fails", func() {
-			execMock.EXPECT().Execute("ovs-ofctl del-flows br-rail1 cookie=0x5/-1").Return("", fmt.Errorf("failed to delete flows"))
-			err := flows.DeletePodRailFlows(0x5, "br-rail1")
+		It("should return error if failed to list bridges", func() {
+			execMock.EXPECT().Execute("ovs-vsctl list-br").Return("", fmt.Errorf("failed to list bridges"))
+			err := flows.DeletePodRailFlows(0x5, "test-pod-uid")
+			Expect(err).Should(HaveOccurred())
+		})
+
+		It("should return error if failed to get external id", func() {
+			execMock.EXPECT().Execute("ovs-vsctl list-br").Return("br-rail1", nil)
+			execMock.EXPECT().
+				Execute("ovs-vsctl br-get-external-id br-rail1 test-pod-uid").
+				Return("", fmt.Errorf("failed to get external id"))
+
+			err := flows.DeletePodRailFlows(0x5, "test-pod-uid")
+			Expect(err).Should(HaveOccurred())
+		})
+
+		It("should return error if failed to delete flows", func() {
+			execMock.EXPECT().Execute("ovs-vsctl list-br").Return("br-rail1", nil)
+
+			execMock.EXPECT().
+				Execute("ovs-vsctl br-get-external-id br-rail1 test-pod-uid").
+				Return("rail_pod_id", nil)
+
+			execMock.EXPECT().Execute("ovs-ofctl del-flows br-rail1 cookie=0x5/-1").
+				Return("", fmt.Errorf("failed to delete flows"))
+
+			err := flows.DeletePodRailFlows(0x5, "test-pod-uid")
+			Expect(err).Should(HaveOccurred())
+		})
+
+		It("should return error if failed to clear external id", func() {
+			execMock.EXPECT().Execute("ovs-vsctl list-br").Return("br-rail1", nil)
+			execMock.EXPECT().
+				Execute("ovs-vsctl br-get-external-id br-rail1 test-pod-uid").
+				Return("rail_pod_id", nil)
+			execMock.EXPECT().Execute("ovs-ofctl del-flows br-rail1 cookie=0x5/-1").Return("", nil)
+			execMock.EXPECT().
+				Execute(`ovs-vsctl br-set-external-id br-rail1 test-pod-uid ""`).
+				Return("", fmt.Errorf("failed to clear external id"))
+
+			err := flows.DeletePodRailFlows(0x5, "test-pod-uid")
+			Expect(err).Should(HaveOccurred())
+			Expect(err.Error()).Should(ContainSubstring("failed to clear external id"))
+		})
+
+		It("should try to delete all flows before returning error", func() {
+			execMock.EXPECT().Execute("ovs-vsctl list-br").Return("br-rail1\nbr-rail2", nil)
+			execMock.EXPECT().Execute("ovs-vsctl br-get-external-id br-rail1 test-pod-uid").
+				Return("rail_pod_id", fmt.Errorf("failed to get external id"))
+			execMock.EXPECT().Execute("ovs-vsctl br-get-external-id br-rail2 test-pod-uid").
+				Return("rail_pod_id", nil)
+			execMock.EXPECT().Execute("ovs-ofctl del-flows br-rail2 cookie=0x5/-1").Return("", fmt.Errorf("failed to delete flows"))
+
+			err := flows.DeletePodRailFlows(0x5, "test-pod-uid")
 			Expect(err).Should(HaveOccurred())
+			Expect(err.Error()).Should(ContainSubstring("failed to get external id"))
+			Expect(err.Error()).Should(ContainSubstring("failed to delete flows"))
 		})
 	})
 })