-
Notifications
You must be signed in to change notification settings - Fork 543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB #2512
[RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB #2512
Conversation
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
a4b7a55
to
e9feb5e
Compare
@@ -630,6 +632,11 @@ void RouteOrch::doTask(Consumer& consumer) | |||
|
|||
if (fvField(i) == "seg_src") | |||
srv6_source = fvValue(i); | |||
|
|||
if (fvField(i) == "protocol") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have to introduce this field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is required to construct RTM_NEWROUTE with correct proto number.
https://github.com/stepanblyschak/SONiC/blob/bgp-suppress-fib-pending/doc/BGP/BGP-supress-fib-pending.md#74-fpmsyncd
orchagent/routeorch.cpp
Outdated
/* If the operation type is "DEL" then ctx.protocol is empty and fvs is left empty. | ||
* An empty fvs makes ResponsePublisher::publish() remove the state entry from APPL_STATE_DB | ||
*/ | ||
if (!ctx.protocol.empty()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why introduce this field for identifying operation? I'm not sure if i understand the logic here. Can you please explaing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not the motivation for introducing this field. The context does not carry operation type, however, if the protocol is left empty that means it is a delete operation, since the delete update does not carry FVS (including protocol)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny Replaced with a flag saved in the context object, please check.
Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
…k/sonic-swss into routeorch-response-channel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm,
This is needed in case DEL/SET consolidation by ConsumerStateTable Signed-off-by: Stepan Blyschak <stepanb@nvidia.com>
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -165,7 +165,7 @@ tests_intfmgrd_INCLUDES = $(tests_INCLUDES) -I$(top_srcdir)/cfgmgr -I$(top_srcdi | |||
tests_intfmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) | |||
tests_intfmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(CFLAGS_SAI) $(tests_intfmgrd_INCLUDES) | |||
tests_intfmgrd_LDADD = $(LDADD_GTEST) $(LDADD_SAI) -lnl-genl-3 -lhiredis -lhiredis \ | |||
-lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread | |||
-lswsscommon -lswsscommon -lgtest -lgtest_main -lzmq -lnl-3 -lnl-route-3 -lpthread -lgmock -lgmock_main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need this change in intfmgrd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@prsunny tests/mock_tests/fake_response_publisher.cpp tests_intfmgrd depends on is using google mock
/azpw run |
/AzurePipelines run |
Azure Pipelines successfully started running 1 pipeline(s). |
Update sonic-swss submodule pointer to include the following: * f66abed Support for tc-dot1p and tc-dscp qosmap ([sonic-net#2559](sonic-net/sonic-swss#2559)) * 35385ad [RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB ([sonic-net#2512](sonic-net/sonic-swss#2512)) * 0704f78 [Workaround] EvpnRemoteVnip2pOrch warmboot check failure ([sonic-net#2626](sonic-net/sonic-swss#2626)) * 4df5cab [ResponsePublisher] add pipeline support ([sonic-net#2511](sonic-net/sonic-swss#2511)) Signed-off-by: AntonHryshchuk <antonh@nvidia.com>
Update sonic-swss submodule pointer to include the following: * baa302e Do not allow to add port to .1Q bridge while router port deletion is not completed ([sonic-net#2669](sonic-net/sonic-swss#2669)) * f66abed Support for tc-dot1p and tc-dscp qosmap ([sonic-net#2559](sonic-net/sonic-swss#2559)) * 35385ad [RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB ([sonic-net#2512](sonic-net/sonic-swss#2512)) * 0704f78 [Workaround] EvpnRemoteVnip2pOrch warmboot check failure ([sonic-net#2626](sonic-net/sonic-swss#2626)) * 4df5cab [ResponsePublisher] add pipeline support ([sonic-net#2511](sonic-net/sonic-swss#2511)) Signed-off-by: dprital <drorp@nvidia.com>
Update sonic-swss submodule pointer to include the following: * baa302e Do not allow to add port to .1Q bridge while router port deletion is not completed ([#2669](sonic-net/sonic-swss#2669)) * f66abed Support for tc-dot1p and tc-dscp qosmap ([#2559](sonic-net/sonic-swss#2559)) * 35385ad [RouteOrch] Record ROUTE_TABLE entry programming status to APPL_STATE_DB ([#2512](sonic-net/sonic-swss#2512)) * 0704f78 [Workaround] EvpnRemoteVnip2pOrch warmboot check failure ([#2626](sonic-net/sonic-swss#2626)) * 4df5cab [ResponsePublisher] add pipeline support ([#2511](sonic-net/sonic-swss#2511))
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.
…PL_STATE_DB (sonic-net#2512)" This reverts commit 35385ad.
…PL_STATE_DB (sonic-net#2512)" This reverts commit 35385ad.
* 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 <stepanb@nvidia.com> --------- Signed-off-by: Stepan Blyschak <stepanb@nvidia.com> Co-authored-by: Ying Xie <yxieca@users.noreply.github.com>
…PL_STATE_DB (sonic-net#2512)" This reverts commit 35385ad.
Signed-off-by: Stepan Blyschak stepanb@nvidia.com
DEPENDS ON #2511
What I did
I added ROUTE_TABLE entry programming status recording to APPL_STATE_DB. I made a ResponsePublisher work in buffered mode to be more optimized for route bulk processing.
I always publish a route entry with return code SAI_STATUS_SUCCESS because orchagent immidiatelly exits on any error.
Why I did it
I did it to support route supression in BGP. Orchagent has to sends route programming feedback back to fpmsyncd which communicates with zebra.
How I verified it
I wrote UT, also on the switch, you can run:
Details if related