[fpmsyncd] Fpmsyncd Next Hop Table Enhancement #2919

ntt-omw · 2023-10-02T10:50:44Z

What I did
Implementing code changes for sonic-net/SONiC#1425

Why I did it
add nexthop group feature to fpmsyncd.

How I verified it

enable/disable nexthop group feature

Klish will call REST API to configure feature next-hop-group enable.
FEATURE|nexthop_group will be created in CONFIG_DB
- template zebra.conf.j2 will generate zebra.conf with fpm use-next-hop-groups if FEATURE|nexthop_group exists in CONFIG_DB. Else, it will generate zebra.conf with no fpm use-next-hop-groups (default behavior)
Do config save comman and write to /etc/sonic/config_db.json
restart SONiC: virsh reboot sonic-nhg
/etc/frr/zebra.conf has fpm use-next-hop-groups instead of no fpm use-next-hop-groups

Klish CLI for feature nexthop_group

Enable: sonic(config)# feature next-hop-group enable
Disable: sonic(config)# no feature next-hop-group

Enable

admin@sonic:~$ sonic-cli

sonic# configure terminal

sonic(config)#
  end        Exit to EXEC mode
  exit       Exit from current mode
  feature    Configure additional feature
  interface  Select an interface
  ip         Global IP configuration subcommands
  mclag      domain
  no         To delete / disable commands in config mode

sonic(config)# feature
  next-hop-group  Next-hop Groups feature

sonic(config)# feature next-hop-group
  enable  Enable Next-hop Groups feature

sonic(config)# feature next-hop-group enable
  <cr>

Disable

sonic(config)# no
  feature  Disable additional feature
  ip       Global IP configuration subcommands
  mclag    domain

sonic(config)# no feature
  next-hop-group  Disable Next-hop Groups feature

sonic(config)# no feature next-hop-group
  <cr>

Details if related

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

zice312963205 · 2023-10-12T09:13:40Z

fpmsyncd/routesync.cpp

+            FieldValueTuple nhg("nexthop_group", nhg_id_key.c_str());
+            fvVector.push_back(nhg);
+            updateNextHopGroup(nhg_id);
+            use_nhg = false;


use_nhg=true?

This code was removed after the change to not use NHG for route with single nexthop.

shuaishang · 2023-10-12T09:42:40Z

fpmsyncd/routesync.h

+    /* nexthop group table */
+    ProducerStateTable  m_nexthop_groupTable;
+    map<uint32_t,NextHopGroup> m_nh_groups;
+    map<string,NextHopGroupRoute> m_nh_routes;


The fpmsyncd should NOT cache the routes, it will consume too much memory in large scale routes scenario.
For one route entry, it already exists in orchagent, sai meta, syncd(sai/sdk). We cannot afford another copying :(

Removed map<string,NextHopGroupRoute> m_nh_routes; based on the discussion in the Routing WG.
Please kindly check if this has resolved your concern.

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

nakano-omw · 2023-11-15T04:07:04Z

/AzurePipelines run Azure.sonic-swss

azure-pipelines · 2023-11-15T04:07:11Z

Commenter does not have sufficient privileges for PR 2919 in repo sonic-net/sonic-swss

nakano-omw · 2023-11-15T04:11:36Z

@zice312963205 @shuaishang
Please help in restarting the /AzurePipelines run Azure.sonic-swss. Would you be able to grant me the privilege?

ridahanif96 · 2023-12-15T04:27:54Z

@nakano-omw your branch needs to be updated and you need to repush your code first. @prsunny Hi Prince, can you please help with this PR to merge. Thanks.

nakano-omw · 2023-12-15T11:49:18Z

@ridahanif96 I have repush. Thanks.

zice312963205 · 2024-01-18T09:12:48Z

Looks good

zhangyanzhao · 2024-02-05T08:38:33Z

reviewers, can you please help to review and approve this PR? Thanks.

dgsudharsan

Please add UT to the changes. There is a requirement for 80% coverage.

dgsudharsan · 2024-05-21T02:31:09Z

fpmsyncd/routesync.h


 #include <netlink/route/route.h>

+#if (LINUX_VERSION_CODE > KERNEL_VERSION(5,3,0))
+#define HAVE_NEXTHOP_GROUP


Why do we need this macro HAVE_NEXTHOP_GROUP? Since we are submitting the code to master, the linux version is expected to be above 5,3,0. Please remove unnecessary ifdefs

Thank you. I have removed the code.

dgsudharsan · 2024-05-21T02:37:10Z

fpmsyncd/routesync.cpp

+    else
+#endif
+    {
+        onEvpnRouteMsg(h, len);


Have you tested if nexthop group works with EVPN in case of overlay nexthop?

dgsudharsan · 2024-05-21T02:50:57Z

fpmsyncd/routesync.cpp

+    char ifname_unknown[IFNAMSIZ] = "unknown";
+
+    SWSS_LOG_INFO("type %d len %d", nlmsg_type, len);
+    if ((nlmsg_type != RTM_NEWNEXTHOP)


This is already checked in the calling function and hence redundant. Please remove here
https://github.com/sonic-net/sonic-swss/pull/2919/files#diff-0555c0a4f1e207c410ac8ab7d4a44f48a0925da2ed14c57499a4e9175223be57R625

Thank you. I have removed the code.

dgsudharsan · 2024-05-21T02:55:09Z

fpmsyncd/routesync.cpp

@@ -45,6 +49,8 @@ using namespace swss;

 #define ETHER_ADDR_STRLEN (3*ETH_ALEN)

+#define MULTIPATH_NUM 256 //Same value used for FRR in SONiC


Can we rename as MAX_MULTIPATH_NUM for better readability?

I have modified it to MAX_MULTIPATH_NUM.

dgsudharsan · 2024-05-21T03:12:29Z

fpmsyncd/routesync.cpp

+            auto itr = m_nh_groups.find(id);
+            if(itr == m_nh_groups.end())
+            {
+                SWSS_LOG_INFO("NextHop group is incomplete: %d", nhg.id);


Shouldn't this be a warn or error log?

I have corrected it to SWSS_LOG_ERROR.

dgsudharsan · 2024-05-21T03:17:22Z

fpmsyncd/routesync.cpp

+    auto git = m_nh_groups.find(nh_id);
+    if(git == m_nh_groups.end())
+    {
+        SWSS_LOG_INFO("Nexthop not found: %d", nh_id);


Shouldn't this be a warn or error message?

I have corrected it to SWSS_LOG_ERROR.

eddieruan-alibaba

Cherry picked into Phoenix Wing folk and validate it.

http://phoenixwing.com.cn/codes

kperumalbfn · 2024-09-19T22:02:22Z

fpmsyncd/fpmlink.cpp

@@ -276,6 +276,13 @@ void FpmLink::processFpmMessage(fpm_msg_hdr_t* hdr)
            /* EVPN Type5 Add route processing */
            processRawMsg(nl_hdr);
        }
+#ifdef HAVE_NEXTHOP_GROUP


Why do we need this macro check? Can we do a dynamic check like

DEVICE_METADATA['localhost']['nexthop_group']

This is done as part of PR - https://github.com/sonic-net/sonic-buildimage/pull/16762/files

Could you pls check this?

kperumalbfn · 2024-09-19T22:03:35Z

fpmsyncd/routesync.cpp


-    vector<string> alsv = tokenize(intf_list, NHG_DELIMITER);
-    for (auto alias : alsv)
+#ifdef HAVE_NEXTHOP_GROUP


Please check the previous comment and we could use device_metadata dynamic check.

kperumalbfn · 2024-09-19T22:04:48Z

fpmsyncd/routesync.cpp

-         * up/down events. Skipping routes to eth0 or docker0 to avoid such behavior
-         */
-        if (alias == "eth0" || alias == "docker0")
+        const auto itg = m_nh_groups.find(nhg_id);


Could you add couple of sonic-swss tests with NHGs?

kperumalbfn · 2024-09-19T22:07:07Z

@ntt-omw Please check the compiler failures

routesync.cpp:800:23: error: 'rtnl_route_get_nh_id' was not declared in this scope; did you mean 'rtnl_route_get_iif'?
800 | uint32_t nhg_id = rtnl_route_get_nh_id(route_obj);
| ^~~~~~~~~~~~~~~~~~~~
| rtnl_route_get_iif

eddieruan-alibaba · 2024-09-29T03:19:27Z

@ntt-omw can you rebase your branch and trigger recompile? You need #3105 's changes to fix the compile issue @kperumalbfn pointed out.

VladimirKuk · 2024-09-29T07:36:41Z

fpmsyncd/routesync.cpp

+                // In this case since we do not want the route with next hop on eth0/docker0, we return.
+                // But still we need to clear the route from the APPL_DB. Otherwise the APPL_DB and data
+                // path will be left with stale route entry
+                if(alsv.size() == 1)


Can't this test be moved outside the loop ? If the list is single entry then there is no reason for the loop itself.

The purpose of this loop is not to show skipped routes, but to skip routes to specific interfaces (eth0 or docker0) and do the associated processing.

VladimirKuk · 2024-09-29T07:38:24Z

fpmsyncd/routesync.cpp

+        string weights = getNextHopWt(route_obj);
+
+        vector<string> alsv = tokenize(intf_list, NHG_DELIMITER);
+        for (auto alias : alsv)


What is the purpose of this loop? Is it to print the skipped routes ? Because the only logic there is when the list is of size 1.

Thanks, I moved if(alsv.size() == 1)) outside the loop.

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

nakano-omw · 2024-10-21T06:40:10Z

@dgsudharsan @kperumalbfn
The test will be implemented in Phoenix Wing. @eddieruan-alibaba , let me know if you have any additional information.

eddieruan-alibaba · 2024-10-28T00:45:34Z

@dgsudharsan @kperumalbfn The test will be implemented in Phoenix Wing. @eddieruan-alibaba , let me know if you have any additional information.

@ntt-omw can you help to get swss sanity check passed?

Failed: 3 (0.35%)

test_rebind_eni_route_group
AssertionError: Field SAI_ENI_ATTR_OUTBOUND_ROUTING_GROUP_ID in ASIC_DB table ASIC_STATE:SAI_OBJECT_TYPE_ENI not equal to oid:0x14008000000617
https://github.com/sonic-net/sonic-swss/pull/2919/checks?check_run_id=31324424340

Might be related to your changes.

saiarcot895 · 2024-10-29T23:01:00Z

@nakano-omw libnl 3.10 will have support for getting/setting the nexthop ID attribute, but the API is a little bit different. See thom311/libnl@3e08063 for details. It looks like in the version of code that has been committed, it's nhid instead of nh_id.

For ease of upgrades, it would be good if the same API syntax is used. Would you be able to rework this PR to use that new API instead?

eddieruan-alibaba · 2024-11-03T23:15:34Z

@ntt-omw @nakano-omw can you rebase your branch to latest master? You have "This branch is out-of-date with the base branch"

sugisono-omw · 2024-11-06T01:51:35Z

@eddieruan-alibaba

We are fixing this issue.

@dgsudharsan @kperumalbfn The test will be implemented in Phoenix Wing. @eddieruan-alibaba , let me know if you have any additional information.

@ntt-omw can you help to get swss sanity check passed?

Failed: 3 (0.35%)

test_rebind_eni_route_group AssertionError: Field SAI_ENI_ATTR_OUTBOUND_ROUTING_GROUP_ID in ASIC_DB table ASIC_STATE:SAI_OBJECT_TYPE_ENI not equal to oid:0x14008000000617 https://github.com/sonic-net/sonic-swss/pull/2919/checks?check_run_id=31324424340

Might be related to your changes.

sugisono-omw · 2024-11-06T01:53:33Z

@eddieruan-alibaba

We are rebasing our branch now.

@ntt-omw @nakano-omw can you rebase your branch to latest master? You have "This branch is out-of-date with the base branch"

eddieruan-alibaba · 2024-11-06T23:59:15Z

Following lines are missing test coverage.. Coverage Threshold is 80%.

prvattem · 2024-11-08T02:13:31Z

fpmsyncd/routesync.cpp

+{
+    if(nhg.group.size() == 0)
+    {
+        if(!nhg.nexthop.empty())


This can be replaced with the following:
nexthops = nhg.nexthop.empty() ? (af == AF_INET ? "0.0.0.0" : "::") : nhg.nexthop;

Similar logic is implemented in the non-empty nhg.

prvattem · 2024-11-08T02:19:56Z

fpmsyncd/routesync.cpp

+            //Using route-table only for single next-hop
+            string nexthops, ifnames, weights;
+
+            getNextHopGroupFields(nhg, nexthops, ifnames, weights, rtnl_route_get_family(route_obj));


Do you want to handle a case where the nhg is not based on the ID?
If there is a failure in getNextHopGroupFields(), you would be pushing empty strings.

ntt-omw requested a review from prsunny as a code owner October 2, 2023 10:50

ntt-omw mentioned this pull request Oct 2, 2023

Fpmsyncd Next Hop Table Enhancement HLD sonic-net/SONiC#1425

Merged

nexthop group support

554d835

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

zice312963205 reviewed Oct 12, 2023

View reviewed changes

shuaishang reviewed Oct 12, 2023

View reviewed changes

nakano-omw and others added 2 commits November 9, 2023 19:59

update to not cache in fpmsyncd

4986db2

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

Merge branch 'master' into ntt_fpmsyncd_enhanced

81f32b9

Merge branch 'master' into ntt_fpmsyncd_enhanced

1df53c9

Merge branch 'sonic-net:master' into ntt_fpmsyncd_enhanced

f72ca22

Merge branch 'sonic-net:master' into ntt_fpmsyncd_enhanced

51c3688

zice312963205 approved these changes Feb 26, 2024

View reviewed changes

dgsudharsan requested changes May 21, 2024

View reviewed changes

Merge branch 'master' into ntt_fpmsyncd_enhanced

1c700a4

eddieruan-alibaba approved these changes Sep 11, 2024

View reviewed changes

Merge branch 'master' into ntt_fpmsyncd_enhanced

122d914

kperumalbfn reviewed Sep 19, 2024

View reviewed changes

VladimirKuk reviewed Sep 29, 2024

View reviewed changes

update nexthop group support

76f24ab

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

update nexthop group support

83778eb

Signed-off-by: Kanji Nakano <kanji.nakano@ntt.com>

Merge branch 'sonic-net:master' into ntt_fpmsyncd_enhanced

7f9a9f7

prvattem reviewed Nov 8, 2024

View reviewed changes

		@@ -45,6 +49,8 @@ using namespace swss;

		#define ETHER_ADDR_STRLEN (3*ETH_ALEN)

		#define MULTIPATH_NUM 256 //Same value used for FRR in SONiC

[fpmsyncd] Fpmsyncd Next Hop Table Enhancement #2919

Are you sure you want to change the base?

[fpmsyncd] Fpmsyncd Next Hop Table Enhancement #2919

Conversation

ntt-omw commented Oct 2, 2023

enable/disable nexthop group feature

Klish CLI for feature nexthop_group

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

nakano-omw commented Nov 15, 2023

azure-pipelines bot commented Nov 15, 2023

nakano-omw commented Nov 15, 2023

ridahanif96 commented Dec 15, 2023

nakano-omw commented Dec 15, 2023

zice312963205 commented Jan 18, 2024

zhangyanzhao commented Feb 5, 2024

dgsudharsan left a 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

eddieruan-alibaba left a 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

kperumalbfn commented Sep 19, 2024

eddieruan-alibaba commented Sep 29, 2024

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

nakano-omw commented Oct 21, 2024

eddieruan-alibaba commented Oct 28, 2024

saiarcot895 commented Oct 29, 2024

eddieruan-alibaba commented Nov 3, 2024

sugisono-omw commented Nov 6, 2024

sugisono-omw commented Nov 6, 2024

eddieruan-alibaba commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment