[fpmsyncd] Implement pending route suppression feature by stepanblyschak · Pull Request #2551 · sonic-net/sonic-swss

stepanblyschak · 2022-11-29T13:18:51Z

DEPENDS: #2512

What I did

I implemented support to enable pending routes suppression feature. When this feature is enabled, fpmsyncd will wait for reply from orchagent before sending offload status message to zebra.

Why I did it

This is done to not announce routes which aren't yet offloaded in HW.

How I verified it

UT and manual tests.

Details if related

Signed-off-by: Stepan Blyschak <[email protected]>

venkatmahalingam · 2022-12-09T17:49:29Z

fpmsyncd/routesync.cpp

    struct nl_addr *daddr;
    char destaddr[MAX_ADDR_SIZE + 1] = {0};

+    sendOffloadReply(route_obj);


Hope you have a test case to test this MPLS label route.

I don't add additional flows for MPLS route handling. So it should be covered by existing MPLS route tests.

venkatmahalingam · 2022-12-09T17:53:44Z

fpmsyncd/routesync.cpp

+            // If field "protocol" is present in the field values then
+            // it is a SET operation. This field is absent only if we are
+            // processing DEL operation.
+            isSetOperation = true;


Dont we get Del operation in the event? why should we assume this?

If we get a response for a delete operation we don't get original FVs. We use this knowledge to filter out DEL events we are not interested in.

ResponsePublisher for reference: https://github.com/sonic-net/sonic-swss/blob/master/orchagent/response_publisher.cpp#L106

venkatmahalingam · 2022-12-09T17:55:38Z

fpmsyncd/routesync.cpp

+    // Mark route as OFFLOAD
+    rtnl_route_set_flags(routeObject.get(), flags);
+
+    if (!sendOffloadReply(routeObject.get()))


When there are multiple route updates for the given route, how do we make sure that the response to sent for the right route-update?

The constructed RTM_NEWROUTE message contains prefix + vrf + protocol of the route

Signed-off-by: Stepan Blyschak <[email protected]>

This reverts commit b38772d.

… and later creation there might be an old name<->ifindex mapping entry Signed-off-by: Stepan Blyschak <[email protected]>

mssonicbld · 2023-02-16T10:34:50Z

/AzurePipelines run

azure-pipelines · 2023-02-16T10:35:00Z

Azure Pipelines successfully started running 1 pipeline(s).

prsunny · 2023-02-16T17:08:24Z

@StormLiangMS , could you help review?

prsunny · 2023-02-16T17:11:28Z

@stepanblyschak , can you please test for backward compatibility?

stepanblyschak · 2023-02-19T15:52:28Z

@prsunny It can be merged without sonic-net/sonic-buildimage#12853. BGP facts and FIB test passed on T1-64-LAG with this change.

Please note, that I had to mark VS tests with xfail because they require VS changes done in sonic-net/sonic-buildimage#12853. Otherwise, it is not possible to merge one without the other in order to not break tests or image.
Later this mark has to be removed once sonic-buildimage PR is merged.

StormLiangMS · 2023-03-03T13:37:02Z

fpmsyncd/fpmlink.cpp

+
+    if (len > m_bufSize)
+    {
+        SWSS_LOG_THROW("Message length %zu is greater then the send buffer size %d", len, m_bufSize);


wording issue, then -> than

Thanks, fixed

StormLiangMS · 2023-03-04T03:32:20Z

@jcaiMR could you help to review?

StormLiangMS · 2023-03-04T15:04:49Z

fpmsyncd/fpmsyncd.cpp

+
    NetDispatcher::getInstance().registerMessageHandler(RTM_NEWROUTE, &sync);
    NetDispatcher::getInstance().registerMessageHandler(RTM_DELROUTE, &sync);
+    NetDispatcher::getInstance().registerMessageHandler(RTM_NEWLINK, &sync);


@stepanblyschak what's the usage for RTM_NETLINK?

I listen for newlink/dellink events to refill link cache - https://github.com/sonic-net/sonic-swss/pull/2551/files#diff-0555c0a4f1e207c410ac8ab7d4a44f48a0925da2ed14c57499a4e9175223be57R607.

E.g, we need link cache to map VRF linux interface name to VRF if_index to pass to FRR. In case VRF linux interface is removed and then re-created with the same name the link cache will have an invalid entry. So I refill the cache on newlink/dellink events.

fpmsyncd/fpmsyncd.cpp

Signed-off-by: Stepan Blyschak <[email protected]>

…led -> disabled Signed-off-by: Stepan Blyschak <[email protected]>

prsunny · 2023-03-13T18:39:37Z

@StormLiangMS , are we good to merge this? Can you please sign-off if no further comments?

StormLiangMS

LGTM

StormLiangMS · 2023-03-18T01:56:31Z

@prsunny this looks good to me.

StormLiangMS · 2023-03-18T01:58:08Z

@stepanblyschak could you also have the testcase in sonic-mgmt to cover this FIB suppress feature? and pls involve Jcai and me for the code review.

stepanblyschak · 2023-03-19T20:55:00Z

@StormLiangMS @jcaiMR BGP suppress FIB pending test cases are already available in PRs - test plan + implementation:

- What I did I added requires "protocol" field due to fpmsyncd reconcile logic (WarmRestartHelper class) requires old fvs keys to match new fvs. - How I did it Add "protocol" field in db migration. - How to verify it Upgrade from older branch to new image with supported FIB pending. The field was added by sonic-net/sonic-swss#2551. Signed-off-by: Stepan Blyschak <[email protected]>

…#2766) - What I did I added requires "protocol" field due to fpmsyncd reconcile logic (WarmRestartHelper class) requires old fvs keys to match new fvs. - How I did it Add "protocol" field in db migration. - How to verify it Upgrade from older branch to new image with supported FIB pending. The field was added by sonic-net/sonic-swss#2551. Signed-off-by: Stepan Blyschak <[email protected]>

…-net#2551)" This reverts commit 840fe1d.

* Revert "[RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB (#2512)" This reverts commit 35385ad. * Revert "[fpmsyncd] Implement pending route suppression feature (#2551)" This reverts commit 840fe1d. * Get back gMockResponsePublisher as it is used by other tests Signed-off-by: Stepan Blyschak <[email protected]> --------- Signed-off-by: Stepan Blyschak <[email protected]> Co-authored-by: Ying Xie <[email protected]>

…-net#2551)" This reverts commit 840fe1d.

DEPENDS: sonic-net#2512 What I did I implemented support to enable pending routes suppression feature. When this feature is enabled, fpmsyncd will wait for reply from orchagent before sending offload status message to zebra. Why I did it This is done to not announce routes which aren't yet offloaded in HW. How I verified it UT and manual tests.

stepanblyschak added 16 commits November 16, 2022 12:17

[fpmsyncd] record protocol in APPL_DB

ad39a92

Signed-off-by: Stepan Blyschak <[email protected]>

[fpm] add send netlink message API

8521ddf

Signed-off-by: Stepan Blyschak <[email protected]>

[fpmsyncd] send offload reply messages

b7a4a52

Signed-off-by: Stepan Blyschak <[email protected]>

[fpmsyncd] implement feedback mechanism

51c6f56

Signed-off-by: Stepan Blyschak <[email protected]>

[fpmsyncd] support warm restart with responses

728d8f7

Signed-off-by: Stepan Blyschak <[email protected]>

add response channel to select

4c59435

Signed-off-by: Stepan Blyschak <[email protected]>

move files

22abcf0

Signed-off-by: Stepan Blyschak <[email protected]>

add more test cases

4c87474

Signed-off-by: Stepan Blyschak <[email protected]>

add a define

e04a3fb

Signed-off-by: Stepan Blyschak <[email protected]>

modify mock fpm to work with older gmock

435ca0b

Signed-off-by: Stepan Blyschak <[email protected]>

change field name

ae2ec35

Signed-off-by: Stepan Blyschak <[email protected]>

fix some review comments

75fac9a

Signed-off-by: Stepan Blyschak <[email protected]>

remove cyclic depdency from RouteSync

2a67069

Signed-off-by: Stepan Blyschak <[email protected]>

break cycle

5c94391

Signed-off-by: Stepan Blyschak <[email protected]>

style

8926ddd

Signed-off-by: Stepan Blyschak <[email protected]>

add comments

277c6ed

Signed-off-by: Stepan Blyschak <[email protected]>

stepanblyschak requested a review from prsunny as a code owner November 29, 2022 13:18

venkatmahalingam reviewed Dec 9, 2022

View reviewed changes

stepanblyschak added 4 commits December 16, 2022 11:07

set NLM_F_REQUEST flag

41e4747

Signed-off-by: Stepan Blyschak <[email protected]>

fi

7f902d2

Signed-off-by: Stepan Blyschak <[email protected]>

debugh

b38772d

Signed-off-by: Stepan Blyschak <[email protected]>

Revert "debugh"

beedfcb

This reverts commit b38772d.

prsunny requested a review from StormLiangMS January 5, 2023 17:07

stepanblyschak marked this pull request as draft January 16, 2023 14:28

refresh link cache on NEWLINK/DELLINK because in case of Vrf delation…

41165ef

… and later creation there might be an old name<->ifindex mapping entry Signed-off-by: Stepan Blyschak <[email protected]>

Update test_route.py

75f55b8

stepanblyschak closed this Feb 19, 2023

stepanblyschak reopened this Feb 19, 2023

StormLiangMS reviewed Mar 3, 2023

View reviewed changes

StormLiangMS requested a review from jcaiMR March 4, 2023 03:31

StormLiangMS reviewed Mar 4, 2023

View reviewed changes

fpmsyncd/fpmsyncd.cpp Show resolved Hide resolved

stepanblyschak added 3 commits March 6, 2023 10:47

Merge branch 'master' of github.com:azure/sonic-swss into fpm-response

a7db827

fix typo

e9dc153

Signed-off-by: Stepan Blyschak <[email protected]>

send offload flag for all routes when switching from suppression enab…

c8d7d0c

…led -> disabled Signed-off-by: Stepan Blyschak <[email protected]>

StormLiangMS approved these changes Mar 18, 2023

View reviewed changes

StormLiangMS merged commit 840fe1d into sonic-net:master Mar 19, 2023

stepanblyschak mentioned this pull request Jun 12, 2023

[db_migrator] add required "protocol" field in ROUTE_TABLE sonic-net/sonic-utilities#2766

Merged

stepanblyschak added a commit to stepanblyschak/sonic-swss that referenced this pull request Dec 19, 2023

Revert "[fpmsyncd] Implement pending route suppression feature (sonic…

bede106

…-net#2551)" This reverts commit 840fe1d.

stepanblyschak added a commit to stepanblyschak/sonic-swss that referenced this pull request Dec 20, 2023

Revert "[fpmsyncd] Implement pending route suppression feature (sonic…

c0726a6

…-net#2551)" This reverts commit 840fe1d.

stepanblyschak added a commit to stepanblyschak/sonic-swss that referenced this pull request May 17, 2024

Revert "[fpmsyncd] Implement pending route suppression feature (sonic…

a14043e

…-net#2551)" This reverts commit 840fe1d.

Conversation

stepanblyschak commented Nov 29, 2022

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!

stepanblyschak Dec 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

mssonicbld commented Feb 16, 2023

Uh oh!

azure-pipelines bot commented Feb 16, 2023

Uh oh!

prsunny commented Feb 16, 2023

Uh oh!

prsunny commented Feb 16, 2023

Uh oh!

stepanblyschak commented Feb 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StormLiangMS commented Mar 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

prsunny commented Mar 13, 2023

Uh oh!

StormLiangMS left a comment

Choose a reason for hiding this comment

Uh oh!

StormLiangMS commented Mar 18, 2023

Uh oh!

StormLiangMS commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stepanblyschak commented Mar 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

stepanblyschak Dec 19, 2022 •

edited

Loading

StormLiangMS commented Mar 18, 2023 •

edited

Loading