From 21916c05a8f4cb0ef2a238d8b8decec542546c80 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Fri, 3 Mar 2023 12:35:46 +0800 Subject: [PATCH] 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 --- pkg/agent/agent.go | 11 +-- pkg/agent/agent_linux.go | 3 +- pkg/agent/agent_test.go | 68 +++++++++++++++++++ .../controller/trafficcontrol/controller.go | 3 +- .../interfacestore/interface_cache_test.go | 11 --- pkg/agent/interfacestore/types.go | 4 +- 6 files changed, 79 insertions(+), 21 deletions(-) diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 4bfc73d6a5d..2b5eccafaa8 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -379,17 +379,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_linux.go b/pkg/agent/agent_linux.go index 23ea7342c84..cea934785d4 100644 --- a/pkg/agent/agent_linux.go +++ b/pkg/agent/agent_linux.go @@ -348,8 +348,7 @@ func (i *Initializer) prepareL7NetworkPolicyInterfaces() error { return err } - itf := interfacestore.NewTrafficControlInterface(portName) - itf.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: portUUID, OFPort: ofPort} + itf := interfacestore.NewTrafficControlInterface(portName, &interfacestore.OVSPortConfig{PortUUID: portUUID, OFPort: ofPort}) i.ifaceStore.AddInterface(itf) } diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index dd179481ac0..3309a6c897f 100644 --- 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 index 3ed0675184e..3c37909fb23 100644 --- 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 index 1596959fc72..c338fb2fe09 100644 --- 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 index 0387cf1426d..90715c1a2bc 100644 --- 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 }