Ensure OVS port is valid before setting NO_FLOOD (#4674)

There could be some cases that OVS ports are left invalid. Setting NO_FLOOD for these ports will fail for sure and restarting agents would just meet the same error. Later we should enhance the port cleanup logic, either when they are firstly identified, or when their owners do the initialization. For now, as there could be invalid ports in interface cache, we should ensure a port is valid before setting NO_FLOOD. Signed-off-by: Quan Tian <[email protected]>
antrea-io · Mar 31, 2023 · ea9d87e · ea9d87e
1 parent 166b6d1
commit ea9d87e
Show file tree

Hide file tree

Showing 6 changed files with 82 additions and 19 deletions.
diff --git a/cmd/antrea-agent/agent.go b/cmd/antrea-agent/agent.go
@@ -232,6 +232,7 @@ func run(o *Options) error {
 		k8sClient,
 		crdClient,
 		ovsBridgeClient,
+		ovsctl.NewClient(o.config.OVSBridge),
 		ofClient,
 		routeClient,
 		ifaceStore,

diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go
@@ -95,6 +95,7 @@ type Initializer struct {
 	client                clientset.Interface
 	crdClient             versioned.Interface
 	ovsBridgeClient       ovsconfig.OVSBridgeClient
+	ovsCtlClient          ovsctl.OVSCtlClient
 	ofClient              openflow.Client
 	routeClient           route.Interface
 	wireGuardClient       wireguard.Interface
@@ -122,6 +123,7 @@ func NewInitializer(
 	k8sClient clientset.Interface,
 	crdClient versioned.Interface,
 	ovsBridgeClient ovsconfig.OVSBridgeClient,
+	ovsCtlClient ovsctl.OVSCtlClient,
 	ofClient openflow.Client,
 	routeClient route.Interface,
 	ifaceStore interfacestore.InterfaceStore,
@@ -142,6 +144,7 @@ func NewInitializer(
 ) *Initializer {
 	return &Initializer{
 		ovsBridgeClient:       ovsBridgeClient,
+		ovsCtlClient:          ovsCtlClient,
 		client:                k8sClient,
 		crdClient:             crdClient,
 		ifaceStore:            ifaceStore,
@@ -371,17 +374,20 @@ func (i *Initializer) initInterfaceStore() error {
 }
 
 func (i *Initializer) restorePortConfigs() error {
-	ovsCtlClient := ovsctl.NewClient(i.ovsBridge)
 	interfaces := i.ifaceStore.ListInterfaces()
 	for _, intf := range interfaces {
 		switch intf.Type {
 		case interfacestore.IPSecTunnelInterface:
 			fallthrough
 		case interfacestore.TrafficControlInterface:
-			if err := ovsCtlClient.SetPortNoFlood(int(intf.OFPort)); err != nil {
-				return fmt.Errorf("failed to set port %s with no-flood: %w", intf.InterfaceName, err)
+			if intf.OFPort < 0 {
+				klog.InfoS("Skipped setting no-flood for port due to invalid ofPort", "port", intf.InterfaceName, "ofport", intf.OFPort)
+				continue
+			}
+			if err := i.ovsCtlClient.SetPortNoFlood(int(intf.OFPort)); err != nil {
+				return fmt.Errorf("failed to set no-flood for port %s: %w", intf.InterfaceName, err)
 			}
-			klog.InfoS("Set port no-flood successfully", "PortName", intf.InterfaceName)
+			klog.InfoS("Set no-flood for port", "port", intf.InterfaceName)
 		}
 	}
 	return nil

diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go
@@ -37,6 +37,7 @@ import (
 	"antrea.io/antrea/pkg/agent/types"
 	"antrea.io/antrea/pkg/ovs/ovsconfig"
 	ovsconfigtest "antrea.io/antrea/pkg/ovs/ovsconfig/testing"
+	ovsctltest "antrea.io/antrea/pkg/ovs/ovsctl/testing"
 	"antrea.io/antrea/pkg/util/env"
 	"antrea.io/antrea/pkg/util/ip"
 	"antrea.io/antrea/pkg/util/runtime"
@@ -647,3 +648,70 @@ func mockConfigureLinkAddress(returnedErr error) func() {
 		configureLinkAddresses = originalConfigureLinkAddresses
 	}
 }
+
+func TestRestorePortConfigs(t *testing.T) {
+	tests := []struct {
+		name                string
+		existingInterfaces  []*interfacestore.InterfaceConfig
+		expectedOVSCtlCalls func(client *ovsctltest.MockOVSCtlClientMockRecorder)
+		expectedErr         string
+	}{
+		{
+			name: "success",
+			existingInterfaces: []*interfacestore.InterfaceConfig{
+				interfacestore.NewIPSecTunnelInterface("antrea-ipsec1",
+					ovsconfig.GeneveTunnel,
+					"node1",
+					net.ParseIP("1.1.1.1"),
+					"abcdefg",
+					"node1",
+					&interfacestore.OVSPortConfig{OFPort: 11, PortUUID: "uuid1"}),
+				interfacestore.NewTunnelInterface(defaultTunInterfaceName,
+					ovsconfig.GeneveTunnel,
+					0,
+					net.ParseIP("1.1.1.10"),
+					true,
+					&interfacestore.OVSPortConfig{OFPort: 12}),
+				interfacestore.NewTrafficControlInterface("antrea-tap1",
+					&interfacestore.OVSPortConfig{OFPort: 13, PortUUID: "uuid3"}),
+				interfacestore.NewTrafficControlInterface("antrea-tap2",
+					&interfacestore.OVSPortConfig{OFPort: -1, PortUUID: "uuid3"}),
+			},
+			expectedOVSCtlCalls: func(client *ovsctltest.MockOVSCtlClientMockRecorder) {
+				client.SetPortNoFlood(11).Return(nil)
+				client.SetPortNoFlood(13).Return(nil)
+			},
+		},
+		{
+			name: "fail",
+			existingInterfaces: []*interfacestore.InterfaceConfig{
+				interfacestore.NewTrafficControlInterface("antrea-tap1",
+					&interfacestore.OVSPortConfig{OFPort: 10, PortUUID: "uuid3"}),
+			},
+			expectedOVSCtlCalls: func(client *ovsctltest.MockOVSCtlClientMockRecorder) {
+				client.SetPortNoFlood(10).Return(fmt.Errorf("server unavailable"))
+			},
+			expectedErr: "failed to set no-flood for port antrea-tap1: server unavailable",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := mock.NewController(t)
+			defer controller.Finish()
+			mockOVSCtlClient := ovsctltest.NewMockOVSCtlClient(controller)
+			ifaceStore := interfacestore.NewInterfaceStore()
+			initializer := &Initializer{
+				ifaceStore:   ifaceStore,
+				ovsCtlClient: mockOVSCtlClient,
+			}
+			ifaceStore.Initialize(tt.existingInterfaces)
+			tt.expectedOVSCtlCalls(mockOVSCtlClient.EXPECT())
+			err := initializer.restorePortConfigs()
+			if tt.expectedErr == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.ErrorContains(t, err, tt.expectedErr)
+			}
+		})
+	}
+}
diff --git a/pkg/agent/controller/trafficcontrol/controller.go b/pkg/agent/controller/trafficcontrol/controller.go
@@ -762,8 +762,7 @@ func (c *Controller) getOrCreateTrafficControlPort(port *v1alpha2.TrafficControl
 			return 0, err
 		}
 	}
-	itf := interfacestore.NewTrafficControlInterface(portName)
-	itf.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: portUUID, OFPort: ofPort}
+	itf := interfacestore.NewTrafficControlInterface(portName, &interfacestore.OVSPortConfig{PortUUID: portUUID, OFPort: ofPort})
 	c.interfaceStore.AddInterface(itf)
 	// Create binding for the newly created port.
 	c.portToTCBindings[portName] = &portToTCBinding{

diff --git a/pkg/agent/interfacestore/interface_cache_test.go b/pkg/agent/interfacestore/interface_cache_test.go
@@ -40,7 +40,6 @@ func TestNewInterfaceStore(t *testing.T) {
 	t.Run("testGatewayInterface", testGatewayInterface)
 	t.Run("testTunnelInterface", testTunnelInterface)
 	t.Run("testUplinkInterface", testUplinkInterface)
-	t.Run("testTrafficControlInterface", testTrafficControlInterface)
 	t.Run("testExternalEntityInterface", testEntityInterface)
 }
 
@@ -164,16 +163,6 @@ func testUplinkInterface(t *testing.T) {
 	testGeneralInterface(t, uplinkInterface, UplinkInterface)
 }
 
-func testTrafficControlInterface(t *testing.T) {
-	tcInterface := NewTrafficControlInterface("tc0")
-	tcInterface.IPs = []net.IP{hostIP}
-	tcInterface.OVSPortConfig = &OVSPortConfig{
-		OFPort:   17,
-		PortUUID: "1234567890",
-	}
-	testGeneralInterface(t, tcInterface, TrafficControlInterface)
-}
-
 func testEntityInterface(t *testing.T) {
 	store := NewInterfaceStore()
 	portConfig := &OVSPortConfig{OFPort: 18, PortUUID: "123456789"}

diff --git a/pkg/agent/interfacestore/types.go b/pkg/agent/interfacestore/types.go
@@ -175,8 +175,8 @@ func NewUplinkInterface(uplinkName string) *InterfaceConfig {
 	return uplinkConfig
 }
 
-func NewTrafficControlInterface(interfaceName string) *InterfaceConfig {
-	trafficControlConfig := &InterfaceConfig{InterfaceName: interfaceName, Type: TrafficControlInterface}
+func NewTrafficControlInterface(interfaceName string, ovsPortConfig *OVSPortConfig) *InterfaceConfig {
+	trafficControlConfig := &InterfaceConfig{InterfaceName: interfaceName, Type: TrafficControlInterface, OVSPortConfig: ovsPortConfig}
 	return trafficControlConfig
 }