[fpmsyncd] Fpmsyncd Next Hop Table Enhancement by ntt-omw · Pull Request #2919 · sonic-net/sonic-swss

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

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

+            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

+    /* 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.

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

 #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

+    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

+    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


 #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

+            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

+    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

            /* 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


-    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

-         * 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

+                // 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

+        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.

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

+{
+    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.

same logic with one line

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

+            //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.

We need FRR to provide NHG ID. Otherwise, the NHG key would be changed during path changes.

As eddie says, FRR is needed to provide NHG IDs. There is no case where nhg is not based on identity,

azure-pipelines · 2025-01-29T08:30:00Z

Azure Pipelines successfully started running 1 pipeline(s).

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

mssonicbld · 2025-01-29T10:46:08Z

/azp run

azure-pipelines · 2025-01-29T10:46:19Z

Azure Pipelines successfully started running 1 pipeline(s).

nakano-omw · 2025-01-31T08:48:05Z

@dgsudharsan The code has been corrected.

eddieruan-alibaba · 2025-02-06T04:57:45Z

@dgsudharsan can you help to review it again? @nakano-omw has already addressed your comments. @nakano-omw can you change the Sudharsan's review status to request for review? Currently, it is still "requested changes"

dgsudharsan · 2025-02-06T23:07:58Z

+                }
+            }
+        }
+        if (grp_count > 0)


The comment here is still not addressed. Not sure why can't we move this code to loop in line 1874 like below?

mssonicbld · 2025-02-06T23:25:10Z

/azp run

azure-pipelines · 2025-02-06T23:25:20Z

Azure Pipelines successfully started running 1 pipeline(s).

dgsudharsan · 2025-02-06T23:25:44Z

@eddieruan-alibaba Still one of my comments is not addressed. Left a comment on how to address that

eddieruan-alibaba · 2025-02-06T23:42:46Z

Thanks @dgsudharsan . @nakano-omw do you want to take a look again?

mssonicbld · 2025-02-07T03:58:09Z

/azp run

azure-pipelines · 2025-02-07T03:58:19Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-07T04:39:52Z

/azp run

azure-pipelines · 2025-02-07T04:40:04Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2025-02-07T04:43:29Z

/azp run

azure-pipelines · 2025-02-07T04:43:39Z

Azure Pipelines successfully started running 1 pipeline(s).

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

mssonicbld · 2025-02-07T07:56:13Z

/azp run

azure-pipelines · 2025-02-07T07:56:24Z

Azure Pipelines successfully started running 1 pipeline(s).

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 Signed-off-by: Baorong Liu <96146196+baorliu@users.noreply.github.com>

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

zice312963205 reviewed Oct 12, 2023

View reviewed changes

shuaishang reviewed Oct 12, 2023

View reviewed changes

zice312963205 approved these changes Feb 26, 2024

View reviewed changes

dgsudharsan requested changes May 21, 2024

View reviewed changes

eddieruan-alibaba approved these changes Sep 11, 2024

View reviewed changes

kperumalbfn reviewed Sep 19, 2024

View reviewed changes

VladimirKuk reviewed Sep 29, 2024

View reviewed changes

prvattem reviewed Nov 8, 2024

View reviewed changes

update nhg

f017e8c

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

nakano-omw force-pushed the ntt_fpmsyncd_enhanced branch from 15b8a6d to f017e8c Compare January 29, 2025 10:46

hasan-brcm approved these changes Jan 31, 2025

View reviewed changes

dgsudharsan requested changes Feb 6, 2025

View reviewed changes

Merge branch 'master' into ntt_fpmsyncd_enhanced

1be7d53

nakano-omw force-pushed the ntt_fpmsyncd_enhanced branch from efb6b50 to a748926 Compare February 7, 2025 04:39

dgsudharsan previously approved these changes Feb 7, 2025

View reviewed changes

update nhg

1534ffd

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

nakano-omw dismissed dgsudharsan’s stale review via 1534ffd February 7, 2025 07:56

nakano-omw force-pushed the ntt_fpmsyncd_enhanced branch from 55553fb to 1534ffd Compare February 7, 2025 07:56

dgsudharsan approved these changes Feb 7, 2025

View reviewed changes

prsunny merged commit 515af60 into sonic-net:master Feb 7, 2025


		#define ETHER_ADDR_STRLEN (3*ETH_ALEN)

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

Conversation

ntt-omw commented Oct 2, 2023

enable/disable nexthop group feature

Klish CLI for feature nexthop_group

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakano-omw commented Nov 15, 2023

Uh oh!

azure-pipelines Bot commented Nov 15, 2023

Uh oh!

nakano-omw commented Nov 15, 2023

Uh oh!

ridahanif96 commented Dec 15, 2023

Uh oh!

nakano-omw commented Dec 15, 2023

Uh oh!

zice312963205 commented Jan 18, 2024

Uh oh!

zhangyanzhao commented Feb 5, 2024

Uh oh!

dgsudharsan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddieruan-alibaba left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kperumalbfn commented Sep 19, 2024

Uh oh!

eddieruan-alibaba commented Sep 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nakano-omw commented Oct 21, 2024

Uh oh!

eddieruan-alibaba commented Oct 28, 2024

Uh oh!

saiarcot895 commented Oct 29, 2024

Uh oh!

eddieruan-alibaba commented Nov 3, 2024