From b03e894385053ac2a17a5eb67d74bf5916bc0995 Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Tue, 25 Jun 2024 14:28:42 -0700 Subject: [PATCH] Wait for OVS bridge datapath ID to be available after creating br-int (#6472) We wait (for a maximum of 5s) for the datapath_id of the br-int OVS bridge to be reported in OVSDB, after creating the bridge and before checking supported datapath features. This prevents errors when querying the supported features before the ofproto-dpif provider has been initialized. Fixes #6471 Signed-off-by: Antonin Bas --- pkg/agent/agent.go | 6 +++ pkg/ovs/ovsconfig/interfaces.go | 2 + pkg/ovs/ovsconfig/ovs_client.go | 41 ++++++++++++++++++++- pkg/ovs/ovsconfig/testing/mock_ovsconfig.go | 16 ++++++++ test/integration/ovs/ovs_client_test.go | 33 ++++++++++------- 5 files changed, 83 insertions(+), 15 deletions(-) diff --git a/pkg/agent/agent.go b/pkg/agent/agent.go index 647fe451179..7faa6dd90e2 100644 --- a/pkg/agent/agent.go +++ b/pkg/agent/agent.go @@ -212,6 +212,12 @@ func (i *Initializer) setupOVSBridge() error { return err } + // Wait for the datapath ID for the bridge to be available, as it indicates that the bridge + // has been configured and that we should be able to query supported datapath features. + if _, err := i.ovsBridgeClient.WaitForDatapathID(5 * time.Second); err != nil { + return fmt.Errorf("error when waiting for OVS bridge datapath ID: %w", err) + } + if err := i.validateSupportedDPFeatures(); err != nil { return err } diff --git a/pkg/ovs/ovsconfig/interfaces.go b/pkg/ovs/ovsconfig/interfaces.go index bfa698f7d25..740631ec22f 100644 --- a/pkg/ovs/ovsconfig/interfaces.go +++ b/pkg/ovs/ovsconfig/interfaces.go @@ -16,6 +16,7 @@ package ovsconfig import ( "net" + "time" ) type TunnelType string @@ -54,6 +55,7 @@ type OVSBridgeClient interface { GetExternalIDs() (map[string]string, Error) SetExternalIDs(externalIDs map[string]interface{}) Error GetDatapathID() (string, Error) + WaitForDatapathID(timeout time.Duration) (string, Error) SetDatapathID(datapathID string) Error GetInterfaceOptions(name string) (map[string]string, Error) SetInterfaceOptions(name string, options map[string]interface{}) Error diff --git a/pkg/ovs/ovsconfig/ovs_client.go b/pkg/ovs/ovsconfig/ovs_client.go index 51e3b031cf4..2799639d95f 100644 --- a/pkg/ovs/ovsconfig/ovs_client.go +++ b/pkg/ovs/ovsconfig/ovs_client.go @@ -317,6 +317,45 @@ func (br *OVSBridge) GetDatapathID() (string, Error) { } } +func (br *OVSBridge) WaitForDatapathID(timeout time.Duration) (string, Error) { + tx := br.ovsdb.Transaction(openvSwitchSchema) + tx.Wait(dbtransaction.Wait{ + Table: "Bridge", + Timeout: uint64(timeout.Milliseconds()), + Columns: []string{"datapath_id"}, + Until: "!=", + Rows: []interface{}{ + map[string]interface{}{ + "datapath_id": helpers.MakeOVSDBSet(map[string]interface{}{}), + }, + }, + Where: [][]interface{}{{"name", "==", br.name}}, + }) + tx.Select(dbtransaction.Select{ + Table: "Bridge", + Columns: []string{"datapath_id"}, + Where: [][]interface{}{{"name", "==", br.name}}, + }) + + res, err, temporary := tx.Commit() + if err != nil { + klog.Error("Transaction failed: ", err) + return "", NewTransactionError(err, temporary) + } + + if len(res) < 2 || len(res[1].Rows) == 0 { + return "", NewTransactionError(fmt.Errorf("bridge %s not found", br.name), false) + } + + datapathID := res[1].Rows[0].(map[string]interface{})["datapath_id"] + switch datapathID.(type) { + case string: + return datapathID.(string), nil + default: + return "", nil + } +} + // GetPortUUIDList returns UUIDs of all ports on the bridge. func (br *OVSBridge) GetPortUUIDList() ([]string, Error) { tx := br.ovsdb.Transaction(openvSwitchSchema) @@ -658,7 +697,7 @@ func (br *OVSBridge) GetOFPort(ifName string, waitUntilValid bool) (int32, Error } tx.Wait(dbtransaction.Wait{ Table: "Interface", - Timeout: uint64(defaultGetPortTimeout / time.Millisecond), // The unit of timeout is millisecond + Timeout: uint64(defaultGetPortTimeout.Milliseconds()), Columns: []string{"ofport"}, Until: "!=", Rows: []interface{}{invalidRow}, diff --git a/pkg/ovs/ovsconfig/testing/mock_ovsconfig.go b/pkg/ovs/ovsconfig/testing/mock_ovsconfig.go index 431df64515d..a545e307d20 100644 --- a/pkg/ovs/ovsconfig/testing/mock_ovsconfig.go +++ b/pkg/ovs/ovsconfig/testing/mock_ovsconfig.go @@ -26,6 +26,7 @@ package testing import ( net "net" reflect "reflect" + time "time" ovsconfig "antrea.io/antrea/pkg/ovs/ovsconfig" gomock "go.uber.org/mock/gomock" @@ -530,3 +531,18 @@ func (mr *MockOVSBridgeClientMockRecorder) UpdateOVSOtherConfig(arg0 any) *gomoc mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateOVSOtherConfig", reflect.TypeOf((*MockOVSBridgeClient)(nil).UpdateOVSOtherConfig), arg0) } + +// WaitForDatapathID mocks base method. +func (m *MockOVSBridgeClient) WaitForDatapathID(arg0 time.Duration) (string, ovsconfig.Error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WaitForDatapathID", arg0) + ret0, _ := ret[0].(string) + ret1, _ := ret[1].(ovsconfig.Error) + return ret0, ret1 +} + +// WaitForDatapathID indicates an expected call of WaitForDatapathID. +func (mr *MockOVSBridgeClientMockRecorder) WaitForDatapathID(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WaitForDatapathID", reflect.TypeOf((*MockOVSBridgeClient)(nil).WaitForDatapathID), arg0) +} diff --git a/test/integration/ovs/ovs_client_test.go b/test/integration/ovs/ovs_client_test.go index a51daa49f7e..688d6112a7b 100644 --- a/test/integration/ovs/ovs_client_test.go +++ b/test/integration/ovs/ovs_client_test.go @@ -96,28 +96,33 @@ func TestOVSBridge(t *testing.T) { data.setup(t) defer data.teardown(t) - checkPorts := func(expectedCount int) { - portList, err := data.br.GetPortUUIDList() - require.Nil(t, err, "Error when retrieving port list") - assert.Equal(t, expectedCount, len(portList)) - } + var err error + + start := time.Now() + datapathID, err := data.br.WaitForDatapathID(3 * time.Second) + end := time.Now() + require.NoError(t, err) + require.NotEmpty(t, datapathID) + t.Logf("Waited %v for datapath ID to be assigned", end.Sub(start)) // Test set fixed datapath ID expectedDatapathID, err := randomDatapathID() require.Nilf(t, err, "Failed to generate datapath ID: %s", err) err = data.br.SetDatapathID(expectedDatapathID) require.Nilf(t, err, "Set datapath id failed: %s", err) - var datapathID string - for retry := 0; retry < 3; retry++ { - datapathID, _ = data.br.GetDatapathID() - if datapathID == expectedDatapathID { - break - } - time.Sleep(time.Second) - } - assert.Equal(t, expectedDatapathID, datapathID) + assert.Eventually(t, func() bool { + datapathID, _ := data.br.GetDatapathID() + return datapathID == expectedDatapathID + }, 5*time.Second, 1*time.Second) + vlanID := uint16(100) + checkPorts := func(expectedCount int) { + portList, err := data.br.GetPortUUIDList() + require.Nil(t, err, "Error when retrieving port list") + assert.Equal(t, expectedCount, len(portList)) + } + deleteAllPorts(t, data.br) checkPorts(0)