[bfdorch] Orchagent support hardware BFD #1883

shi-su · 2021-08-23T20:55:39Z

What I did
Implement bfdorch to program hardware BFD sessions via bfd SAI.
Add vs test for bfd sessions.

Why I did it
To support hardware BFD.

How I verified it
Configure hardware BFD sessions on virtual switches and physical devices and confirm the BFD session is programmed.

Details if related

shi-su · 2021-08-24T00:06:58Z

Build failure is expected before sonic-net/sonic-swss-common#524 is merged.

shi-su · 2021-08-30T16:43:54Z

/azp run

azure-pipelines · 2021-08-30T16:44:19Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2021-10-08T01:47:54Z

orchagent/bfdorch.cpp

+        auto value = fvValue(i);
+
+        if (fvField(i) == "tx_interval")
+            tx_interval = to_uint<uint32_t>(value);


please add opening/closing braces {}

Added braces.

prsunny · 2021-11-02T00:56:28Z

orchagent/orchdaemon.cpp

@@ -288,6 +289,8 @@ bool OrchDaemon::init()

    gMacsecOrch = new MACsecOrch(m_applDb, m_stateDb, macsec_app_tables, gPortsOrch);

+    gBfdOrch = new BfdOrch(m_applDb, APP_BFD_SESSION_TABLE_NAME);


Pass m_stateDB as well to the BfdOrch

Thanks for this suggestion, updated the code to pass m_stateDB and STATE_BFD_SESSION_TABLE_NAME to BfdOrch.

prsunny · 2021-11-02T00:57:57Z

orchagent/main.cpp

@@ -451,6 +451,10 @@ int main(int argc, char **argv)
    attr.value.ptr = (void *)on_port_state_change;
    attrs.push_back(attr);

+    attr.id = SAI_SWITCH_ATTR_BFD_SESSION_STATE_CHANGE_NOTIFY;


Just thinking if we should do a capability query before setting this. Do you see any issue if we set this on the current flow where vendor SAI don't have BFD support?

This is a good point. I am not sure how it would behave for vendor SAI without BFD support. Will do some research on that.

prsunny · 2021-11-02T01:00:20Z

orchagent/bfdorch.h

+#define SWSS_BFDORCH_H
+
+#include "orch.h"
+#include "intfsorch.h"


Please move these headers to .cpp if not referenced in this header file.

Moved the headers to .cpp.

prsunny · 2021-11-02T01:05:06Z

orchagent/bfdorch.cpp

+                auto key = bfd_session_lookup[id].peer;
+                m_stateBfdSessionTable->hset(key, "state", session_state_loopup.at(state));
+
+                bfd_session_lookup[id].state = state;


Can you please add the code for notifying observers? similar to port oper change?

Added code for notifying observer. For this PR, there will not be anywhere processing these notifications though.

prsunny · 2021-11-02T01:12:31Z

orchagent/bfdorch.cpp

+    attrs.emplace_back(attr);
+
+    attr.id = SAI_BFD_SESSION_ATTR_MIN_TX;
+    attr.value.u32 = tx_interval * 1000;


define this. don't use magic number

Got this defined as BFD_SESSION_MILLISECOND_TO_MICROSECOND

prsunny · 2021-11-02T01:13:13Z

orchagent/bfdorch.cpp

+        {
+            SWSS_LOG_ERROR("Failed to create BFD session %s: destination MAC address required when hardware lookup not valid",
+                            key.c_str());
+            return false;


return true as no need to retry. Suggest check all return codes in this function

Updated return value to true and checked other return codes

prsunny · 2021-11-02T01:15:07Z

orchagent/bfdorch.cpp

+
+        attrs.emplace_back(attr);
+
+        if (dst_mac_provided)


I think this check can be relaxed as its ok to ignore. In future when modifying attributes are supported, we don't want to have some restriction as to remove certain attributes when some other attributes are modified.

This check is per SAI definition that mac address is required when and only when SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID is false. Therefore, I added the check here and the opposite one for SAI_BFD_SESSION_ATTR_HW_LOOKUP_VALID is true.

prsunny · 2021-11-02T01:16:06Z

orchagent/bfdorch.cpp

+        }
+    }
+
+    const string state_db_key = vrf_name + state_db_key_delimiter + alias + state_db_key_delimiter + peer_address.to_string();


Can you move this to a separate function so it can be used from both create and remove functions?

Defined a separate function to generate keys.

prsunny · 2021-11-02T01:18:51Z

orchagent/bfdorch.cpp

+    SWSS_LOG_ENTER();
+
+    m_state_db = shared_ptr<DBConnector>(new DBConnector("STATE_DB", 0));
+    m_stateBfdSessionTable = unique_ptr<Table>(new Table(m_state_db.get(), STATE_BFD_SESSION_TABLE));


As mentioned, please pass this attributes from orch init

Thanks for the comment. Moved the attributes to orch init.

prsunny · 2021-11-03T22:52:09Z

orchagent/bfdorch.cpp

+    {"async_passive",       SAI_BFD_SESSION_TYPE_ASYNC_PASSIVE}
+};
+
+const map<sai_bfd_session_type_t, string> session_type_loopup =


lookup -> typo?. I would suggest to name as session_type_map. Similarly for session_state

It is.. Thanks for catching it.

prsunny · 2021-11-03T22:53:08Z

orchagent/bfdorch.cpp

+    {SAI_BFD_SESSION_TYPE_ASYNC_PASSIVE,    "async_passive"}
+};
+
+const map<sai_bfd_session_state_t, string> session_state_loopup =


lookup -> typo?

Fixed for this and a few other places.

prsunny · 2021-11-03T22:54:44Z

orchagent/bfdorch.cpp

+        if (!gPortsOrch->getPort(alias, port))
+        {
+            SWSS_LOG_ERROR("Failed to locate port %s", alias.c_str());
+            return true;


this should return false as a retry is expected here. port may get created later

Updated as suggested.

prsunny · 2021-11-03T22:59:37Z

orchagent/bfdorch.cpp

+                auto key = bfd_session_lookup[id].peer;
+                m_stateBfdSessionTable.hset(key, "state", session_state_loopup.at(state));
+
+                SWSS_LOG_NOTICE("Updated BFD session state for %s to %s", key.c_str(), session_state_loopup.at(state).c_str());


can you please rephrase this? BFD session state for %s changed from %s to %s. This shall be an important log message

Updated the log message.

shi-su · 2021-11-08T22:38:27Z

/azp run

azure-pipelines · 2021-11-08T22:38:37Z

Azure Pipelines successfully started running 1 pipeline(s).

What I did Implement bfdorch to program hardware BFD sessions via bfd SAI. Add vs test for bfd sessions. Why I did it To support hardware BFD. How I verified it Configure hardware BFD sessions on virtual switches and physical devices and confirm the BFD session is programmed.

This reverts commit 63bc849.

What I did Implement bfdorch to program hardware BFD sessions via bfd SAI. Add vs test for bfd sessions. Why I did it To support hardware BFD. How I verified it Configure hardware BFD sessions on virtual switches and physical devices and confirm the BFD session is programmed.

…oint health monitoring (#2104) What I did Cherry-pick changes in #1960, #1883, #1955, #2058 Changes in #1960: Add functions to create/remove next hop groups for vnet tunnel routes. Count the reference count of next hop groups to create and remove as needed. Share the counter of next hop groups with routeorch. Add vs test Changes in #1883: Implement bfdorch to program hardware BFD sessions via bfd SAI. Add vs test for bfd sessions. Changes in #1955: Add functions to create/remove next hop groups for vnet tunnel routes. Count the reference count of next hop groups to create and remove as needed. Share the counter of next hop groups with routeorch. Adapt route endpoint according to the BFD state of endpoints. Changes in #2058: Advertise active vnet tunnel routes. Why I did it To add support for overlay ECMP with endpoint health monitoring.

* Add CRM CLIs for SRV6 nexthop and my_sid_entry

shi-su marked this pull request as ready for review September 22, 2021 23:40

shi-su requested a review from prsunny as a code owner September 22, 2021 23:40

shi-su requested review from prsunny and removed request for prsunny September 22, 2021 23:41

prsunny mentioned this pull request Oct 8, 2021

[BFD] HW offload and bfdorch HLD sonic-net/SONiC#880

Merged

prsunny reviewed Nov 2, 2021

View reviewed changes

shi-su force-pushed the bfd_master branch from 8ab936a to 0a39fd3 Compare November 2, 2021 20:27

prsunny reviewed Nov 3, 2021

View reviewed changes

prsunny previously approved these changes Nov 3, 2021

View reviewed changes

Shi Su added 15 commits November 9, 2021 22:57

initial implementation of bfdorch

2beae06

Add support for given interface

060e486

make bfd operatio saparated functions

3b3034c

add vs test

d904657

fix typos

e77e35c

Add bfd notification

c6ff0f1

Adjust log level

6338fb3

Improve BFD orch

9087c47

Add test for handling notification and additional test cases

6ac0f57

Fix a typo

d03d5f7

Add vrf name in bfd session key

b64b240

Address review comments

176f05c

Change bfd session table name

917e779

Update log

f5bbbfe

Address review comments 2

2a105e5

shi-su dismissed prsunny’s stale review via 2a105e5 November 9, 2021 22:57

shi-su force-pushed the bfd_master branch from 24435e3 to 2a105e5 Compare November 9, 2021 22:57

shi-su requested a review from prsunny November 9, 2021 22:58

prsunny approved these changes Nov 10, 2021

View reviewed changes

shi-su merged commit 8119ec0 into sonic-net:master Nov 10, 2021

shi-su mentioned this pull request Nov 17, 2021

Overlay ECMP with BFD support sonic-net/SONiC#861

Merged

shi-su mentioned this pull request Jan 6, 2022

[202012] [bfdorch] Orchagent support hardware BFD #2103

Closed

dgsudharsan added a commit to dgsudharsan/sonic-swss that referenced this pull request Jan 11, 2022

Revert "[bfdorch] Orchagent support hardware BFD (sonic-net#1883)"

d25da7c

This reverts commit 63bc849.

shi-su mentioned this pull request Jan 21, 2022

[202012] [vnetorch] Add ECMP support for vnet tunnel routes with endpoint health monitoring #2104

Merged

EdenGri pushed a commit to EdenGri/sonic-swss that referenced this pull request Feb 28, 2022

Add CRM CLIs for SRV6 nexthop and my_sid_entry (sonic-net#1883)

487b3b7

* Add CRM CLIs for SRV6 nexthop and my_sid_entry

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[bfdorch] Orchagent support hardware BFD #1883

[bfdorch] Orchagent support hardware BFD #1883

shi-su commented Aug 23, 2021

shi-su commented Aug 24, 2021

shi-su commented Aug 30, 2021

azure-pipelines bot commented Aug 30, 2021

prsunny Oct 8, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 2, 2021

shi-su Nov 2, 2021

prsunny Nov 3, 2021

shi-su Nov 3, 2021

prsunny Nov 3, 2021

shi-su Nov 3, 2021

prsunny Nov 3, 2021

shi-su Nov 3, 2021

prsunny Nov 3, 2021 •

edited

Loading

shi-su Nov 3, 2021

shi-su commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

		@@ -288,6 +289,8 @@ bool OrchDaemon::init()

		gMacsecOrch = new MACsecOrch(m_applDb, m_stateDb, macsec_app_tables, gPortsOrch);

		gBfdOrch = new BfdOrch(m_applDb, APP_BFD_SESSION_TABLE_NAME);

[bfdorch] Orchagent support hardware BFD #1883

[bfdorch] Orchagent support hardware BFD #1883

Conversation

shi-su commented Aug 23, 2021

shi-su commented Aug 24, 2021

shi-su commented Aug 30, 2021

azure-pipelines bot commented Aug 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prsunny Nov 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shi-su commented Nov 8, 2021

azure-pipelines bot commented Nov 8, 2021

prsunny Nov 3, 2021 •

edited

Loading