Skip to content

[swss]:Skip APPL_STATE_DB and pub/sub writes when FIB suppression is disabled #4333

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.

Sign up for GitHub

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

Open
mike-dubrovsky wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[swss]:Skip APPL_STATE_DB and pub/sub writes when FIB suppression is disabled #4333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 1 addition & 59 deletions fpmsyncd/fpmsyncd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
#include "netdispatcher.h"
#include "netlink.h"
#include "notificationconsumer.h"
#include "subscriberstatetable.h"
#include "warmRestartHelper.h"
#include "fpmsyncd/fpmlink.h"
#include "fpmsyncd/fpmsyncd.h"
Expand Down Expand Up @@ -79,7 +78,6 @@ int main(int argc, char **argv)

DBConnector db("APPL_DB", 0);
DBConnector cfgDb("CONFIG_DB", 0);
SubscriberStateTable deviceMetadataTableSubscriber(&cfgDb, CFG_DEVICE_METADATA_TABLE_NAME);
Table deviceMetadataTable(&cfgDb, CFG_DEVICE_METADATA_TABLE_NAME);
DBConnector applStateDb("APPL_STATE_DB", 0);
std::unique_ptr<NotificationConsumer> routeResponseChannel;
Expand Down Expand Up @@ -116,6 +114,7 @@ int main(int argc, char **argv)
routeResponseChannel = std::make_unique<NotificationConsumer>(&applStateDb, routeResponseChannelName);
sync.setSuppressionEnabled(true);
}
SWSS_LOG_NOTICE("FIB suppression state: %s", suppressionEnabledStr.c_str());

while (true)
{
Expand All @@ -142,8 +141,6 @@ int main(int argc, char **argv)

s.addSelectable(&fpm);
s.addSelectable(&netlink);
s.addSelectable(&deviceMetadataTableSubscriber);

if (sync.isSuppressionEnabled())
{
s.addSelectable(routeResponseChannel.get());
Expand Down Expand Up @@ -249,61 +246,6 @@ int main(int argc, char **argv)
s.removeSelectable(&eoiuCheckTimer);
}
}
else if (temps == &deviceMetadataTableSubscriber)
Copy link
Copy Markdown
Collaborator

@prsunny prsunny Mar 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this removed? seems unrelated.

Copy link
Copy Markdown
Author

@mike-dubrovsky mike-dubrovsky Mar 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fpmsyncd code was subscribing to CONFIG_DB changes to dynamically toggle FIB suppression at runtime. After this fix, the suppress-fib-pending configuration becomes operational only on swss container restart (cold/fast reboot or systemctl restart swss), so there is no longer a need to listen for CONFIG_DB changes.

{
std::deque<KeyOpFieldsValuesTuple> keyOpFvsQueue;
deviceMetadataTableSubscriber.pops(keyOpFvsQueue);

for (const auto& keyOpFvs: keyOpFvsQueue)
{
const auto& key = kfvKey(keyOpFvs);
const auto& op = kfvOp(keyOpFvs);
const auto& fvs = kfvFieldsValues(keyOpFvs);

if (op != SET_COMMAND)
{
continue;
}

if (key != "localhost")
{
continue;
}

for (const auto& fv: fvs)
{
const auto& field = fvField(fv);
const auto& value = fvValue(fv);

if (field != "suppress-fib-pending")
{
continue;
}

bool shouldEnable = (value == "enabled");

if (shouldEnable && !sync.isSuppressionEnabled())
{
routeResponseChannel = std::make_unique<NotificationConsumer>(&applStateDb, routeResponseChannelName);
sync.setSuppressionEnabled(true);
s.addSelectable(routeResponseChannel.get());
}
else if (!shouldEnable && sync.isSuppressionEnabled())
{
/* When disabling suppression we mark all existing routes offloaded in zebra
* as there could be some transient routes which are pending response from
* orchagent, thus such updates might be missing. Since we are disabling suppression
* we no longer care about real HW offload status and can mark all routes as offloaded
* to avoid routes stuck in suppressed state after transition. */
sync.markRoutesOffloaded(db);

sync.setSuppressionEnabled(false);
s.removeSelectable(routeResponseChannel.get());
routeResponseChannel.reset();
}
} // end for fvs
} // end for keyOpFvsQueue
}
else if (routeResponseChannel && (temps == routeResponseChannel.get()))
{
std::deque<KeyOpFieldsValuesTuple> notifications;
Expand Down
9 changes: 7 additions & 2 deletions orchagent/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ extern int gBatchSize;

bool gRingMode = false;
bool gSyncMode = false;
bool gEnableFibSuppress = false;
sai_redis_communication_mode_t gRedisCommunicationMode = SAI_REDIS_COMMUNICATION_MODE_REDIS_ASYNC;
string gAsicInstance;

Expand Down Expand Up @@ -92,7 +93,7 @@ bool isChassisDbInUse()

void usage()
{
cout << "usage: orchagent [-h] [-r record_type] [-A] [-d record_location] [-f swss_rec_filename] [-j sairedis_rec_filename] [-b batch_size] [-m MAC] [-i INST_ID] [-s] [-z mode] [-k bulk_size] [-q zmq_server_address] [-c mode] [-t create_switch_timeout] [-v VRF] [-I heart_beat_interval] [-R] [-M]" << endl;
cout << "usage: orchagent [-h] [-r record_type] [-A] [-d record_location] [-f swss_rec_filename] [-j sairedis_rec_filename] [-b batch_size] [-m MAC] [-i INST_ID] [-s] [-z mode] [-k bulk_size] [-q zmq_server_address] [-c mode] [-t create_switch_timeout] [-v VRF] [-I heart_beat_interval] [-R] [-M] [-F]" << endl;
cout << " -h: display this message" << endl;
cout << " -r record_type: record orchagent logs with type (default 3)" << endl;
cout << " Bit 0: sairedis.rec, Bit 1: swss.rec, Bit 2: responsepublisher.rec. For example:" << endl;
Expand All @@ -118,6 +119,7 @@ void usage()
cout << " -I heart_beat_interval: Heart beat interval in millisecond (default 10)" << endl;
cout << " -R enable the ring thread feature" << endl;
cout << " -M enable SAI MACSec POST" << endl;
cout << " -F enable BGP FIB suppression" << endl;
}

void sighup_handler(int signo)
Expand Down Expand Up @@ -470,7 +472,7 @@ int main(int argc, char **argv)
// Disable SAI MACSec POST by default. Use option -M to enable it.
bool macsec_post_enabled = false;

while ((opt = getopt(argc, argv, "b:m:r:Af:j:d:i:hsz:k:q:c:t:v:I:R:M")) != -1)
while ((opt = getopt(argc, argv, "b:m:r:Af:j:d:i:hsz:k:q:c:t:v:I:R:MF")) != -1)
{
switch (opt)
{
Expand Down Expand Up @@ -595,6 +597,9 @@ int main(int argc, char **argv)
case 'M':
macsec_post_enabled = true;
break;
case 'F':
gEnableFibSuppress = true;
break;
Comment thread
mike-dubrovsky marked this conversation as resolved.
default: /* '?' */
exit(EXIT_FAILURE);
}
Expand Down
6 changes: 6 additions & 0 deletions orchagent/routeorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ extern TunnelDecapOrch *gTunneldecapOrch;

extern size_t gMaxBulkSize;
extern string gMySwitchType;
extern bool gEnableFibSuppress;

/* Default maximum number of next hop groups */
#define DEFAULT_NUMBER_OF_ECMP_GROUPS 128
Expand Down Expand Up @@ -3186,6 +3187,11 @@ void RouteOrch::publishRouteState(const RouteBulkContext& ctx, const ReturnCode&
{
SWSS_LOG_ENTER();

if (!gEnableFibSuppress)
{
return;
}
Comment thread
mike-dubrovsky marked this conversation as resolved.

std::vector<FieldValueTuple> fvs;

/* Leave the fvs empty if the operation type is "DEL".
Expand Down
1 change: 1 addition & 0 deletions tests/mock_tests/mock_orchagent_main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ string gMyHostName = "Linecard1";
string gMyAsicName = "Asic0";
bool gTraditionalFlexCounter = false;
bool gSyncMode = false;
bool gEnableFibSuppress = false;
sai_redis_communication_mode_t gRedisCommunicationMode = SAI_REDIS_COMMUNICATION_MODE_REDIS_ASYNC;
bool gOrchUnhealthy = false;
string gSaiErrorString;
Expand Down
6 changes: 6 additions & 0 deletions tests/mock_tests/routeorch_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "bulker.h"

extern string gMySwitchType;
extern bool gEnableFibSuppress;
Comment thread
mike-dubrovsky marked this conversation as resolved.

extern std::unique_ptr<MockResponsePublisher> gMockResponsePublisher;

Expand Down Expand Up @@ -388,6 +389,8 @@ namespace routeorch_test

void TearDown() override
{
gEnableFibSuppress = false;

RestoreSaiApis();
DEINIT_SAI_API_MOCK(route);

Expand Down Expand Up @@ -718,6 +721,7 @@ namespace routeorch_test

TEST_F(RouteOrchTest, RouteOrchTestSetDelResponse)
{
gEnableFibSuppress = true;
gMockResponsePublisher = std::make_unique<MockResponsePublisher>();

std::deque<KeyOpFieldsValuesTuple> entries;
Expand Down Expand Up @@ -754,6 +758,7 @@ namespace routeorch_test

TEST_F(RouteOrchTest, RouteOrchSetFullMaskSubnetPrefix)
{
gEnableFibSuppress = true;
gMockResponsePublisher = std::make_unique<MockResponsePublisher>();

std::deque<KeyOpFieldsValuesTuple> entries;
Expand All @@ -772,6 +777,7 @@ namespace routeorch_test

TEST_F(RouteOrchTest, RouteOrchLoopbackRoute)
{
gEnableFibSuppress = true;
gMockResponsePublisher = std::make_unique<MockResponsePublisher>();

std::deque<KeyOpFieldsValuesTuple> entries;
Expand Down
15 changes: 13 additions & 2 deletions tests/test_route.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1077,15 +1077,22 @@ def is_offloaded(self, dvs, route):
route_entry = json.loads(output)
return bool(route_entry[route][0].get('offloaded'))

@pytest.mark.xfail(reason="BGP suppress FIB disabled on master/202405 - https://github.com/sonic-net/sonic-buildimage/issues/19092")
@pytest.mark.parametrize("suppress_state", ["enabled", "disabled"])
def test_offload(self, suppress_state, setup, dvs):
route = "1.1.1.0/24"

# enable route suppression
# configure suppress-fib-pending
rc, _ = dvs.runcmd(f"config suppress-fib-pending {suppress_state}")
assert rc == 0, "Failed to configure suppress-fib-pending"

# Container restart is used here as a quick substitute for a full config
# reload or device reboot, which is the only supported way to apply
# suppress-fib-pending config changes. This is intentionally unsupported
# in production but sufficient for DVS unit test purposes.
dvs.stop_fpmsyncd()
dvs.stop_swss()
dvs.start_swss()
dvs.start_fpmsyncd()
time.sleep(5)

try:
Expand Down Expand Up @@ -1116,6 +1123,10 @@ def check_offloaded():

# make sure route suppression is disabled
dvs.runcmd("config suppress-fib-pending disabled")
dvs.stop_fpmsyncd()
dvs.stop_swss()
dvs.start_swss()
dvs.start_fpmsyncd()
Comment thread
mike-dubrovsky marked this conversation as resolved.


class TestSubnetDecapVipRoute(TestRouteBase):
Expand Down
Loading