Skip to content
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

Merged
merged 8 commits into from
Feb 14, 2023
38 changes: 37 additions & 1 deletion orchagent/routeorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ RouteOrch::RouteOrch(DBConnector *db, vector<table_name_with_pri_t> &tableNames,
{
SWSS_LOG_ENTER();

m_publisher.setBuffered(true);

sai_attribute_t attr;
attr.id = SAI_SWITCH_ATTR_NUMBER_OF_ECMP_GROUPS;

Expand Down Expand Up @@ -499,7 +501,7 @@ void RouteOrch::doTask(Consumer& consumer)

auto rc = toBulk.emplace(std::piecewise_construct,
std::forward_as_tuple(key, op),
std::forward_as_tuple());
std::forward_as_tuple(key));

bool inserted = rc.second;
auto& ctx = rc.first->second;
Expand Down Expand Up @@ -630,6 +632,11 @@ void RouteOrch::doTask(Consumer& consumer)

if (fvField(i) == "seg_src")
srv6_source = fvValue(i);

if (fvField(i) == "protocol")
Copy link
Collaborator

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

{
ctx.protocol = fvValue(i);
}
}

/*
Expand Down Expand Up @@ -831,6 +838,10 @@ void RouteOrch::doTask(Consumer& consumer)
/* fullmask subnet route is same as ip2me route */
else if (ip_prefix.isFullMask() && m_intfsOrch->isPrefixSubnet(ip_prefix, alsv[0]))
{
/* The prefix is full mask (/32 or /128) and it is an interface subnet route, so IntfOrch has already
* created an IP2ME route for it and we skip programming such route here as it already exists.
* However, to keep APPL_DB and APPL_STATE_DB consistent we have to publish it. */
publishRouteState(ctx, ReturnCode(SAI_STATUS_SUCCESS));
it = consumer.m_toSync.erase(it);
}
/* subnet route, vrf leaked route, etc */
Expand Down Expand Up @@ -2226,6 +2237,9 @@ bool RouteOrch::addRoutePost(const RouteBulkContext& ctx, const NextHopGroupKey

notifyNextHopChangeObservers(vrf_id, ipPrefix, nextHops, true);

/* Publish and update APPL STATE DB route entry programming status */
publishRouteState(ctx, ReturnCode(SAI_STATUS_SUCCESS));

/*
* If the route uses a temporary synced NHG owned by NhgOrch, return false
* in order to keep trying to update the route in case the NHG is updated,
Expand Down Expand Up @@ -2419,6 +2433,9 @@ bool RouteOrch::removeRoutePost(const RouteBulkContext& ctx)

SWSS_LOG_INFO("Remove route %s with next hop(s) %s",
ipPrefix.to_string().c_str(), it_route->second.nhg_key.to_string().c_str());

/* Publish removal status, removes route entry from APPL STATE DB */
publishRouteState(ctx, ReturnCode(SAI_STATUS_SUCCESS));

if (ipPrefix.isDefaultRoute() && vrf_id == gVirtualRouterId)
{
Expand Down Expand Up @@ -2574,3 +2591,22 @@ void RouteOrch::decNhgRefCount(const std::string &nhg_index)
gCbfNhgOrch->decNhgRefCount(nhg_index);
}
}

void RouteOrch::publishRouteState(const RouteBulkContext& ctx, const ReturnCode& status)
{
SWSS_LOG_ENTER();

std::vector<FieldValueTuple> fvs;

/* 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())
Copy link
Collaborator

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?

Copy link
Contributor Author

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)

Copy link
Contributor Author

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.

{
fvs.emplace_back("protocol", ctx.protocol);
}

const bool replace = false;

m_publisher.publish(APP_ROUTE_TABLE_NAME, ctx.key, fvs, status, replace);
}
11 changes: 9 additions & 2 deletions orchagent/routeorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,11 @@ struct RouteBulkContext
// using_temp_nhg will track if the NhgOrch's owned NHG is temporary or not
bool using_temp_nhg;

RouteBulkContext()
: excp_intfs_flag(false), using_temp_nhg(false)
std::string key; // Key in database table
std::string protocol; // Protocol string

RouteBulkContext(const std::string& key)
: key(key), excp_intfs_flag(false), using_temp_nhg(false)
{
}

Expand All @@ -139,6 +142,8 @@ struct RouteBulkContext
excp_intfs_flag = false;
vrf_id = SAI_NULL_OBJECT_ID;
using_temp_nhg = false;
key.clear();
protocol.clear();
}
};

Expand Down Expand Up @@ -269,6 +274,8 @@ class RouteOrch : public Orch, public Subject
const NhgBase &getNhg(const std::string& nhg_index);
void incNhgRefCount(const std::string& nhg_index);
void decNhgRefCount(const std::string& nhg_index);

void publishRouteState(const RouteBulkContext& ctx, const ReturnCode& status);
};

#endif /* SWSS_ROUTEORCH_H */
2 changes: 1 addition & 1 deletion tests/mock_tests/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

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?

Copy link
Contributor Author

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


## fpmsyncd unit tests

Expand Down
21 changes: 19 additions & 2 deletions tests/mock_tests/fake_response_publisher.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,36 @@
#include <vector>

#include "response_publisher.h"
#include "mock_response_publisher.h"

/* This mock plugs into this fake response publisher implementation
* when needed to test code that uses response publisher. */
std::unique_ptr<MockResponsePublisher> gMockResponsePublisher;

ResponsePublisher::ResponsePublisher() : m_db("APPL_STATE_DB", 0) {}

void ResponsePublisher::publish(
const std::string& table, const std::string& key,
const std::vector<swss::FieldValueTuple>& intent_attrs,
const ReturnCode& status,
const std::vector<swss::FieldValueTuple>& state_attrs, bool replace) {}
const std::vector<swss::FieldValueTuple>& state_attrs, bool replace)
{
if (gMockResponsePublisher)
{
gMockResponsePublisher->publish(table, key, intent_attrs, status, state_attrs, replace);
}
}

void ResponsePublisher::publish(
const std::string& table, const std::string& key,
const std::vector<swss::FieldValueTuple>& intent_attrs,
const ReturnCode& status, bool replace) {}
const ReturnCode& status, bool replace)
{
if (gMockResponsePublisher)
{
gMockResponsePublisher->publish(table, key, intent_attrs, status, replace);
}
}

void ResponsePublisher::writeToDB(
const std::string& table, const std::string& key,
Expand Down
56 changes: 56 additions & 0 deletions tests/mock_tests/routeorch_ut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,14 @@
#include "ut_helper.h"
#include "mock_orchagent_main.h"
#include "mock_table.h"
#include "mock_response_publisher.h"
#include "bulker.h"

extern string gMySwitchType;

extern std::unique_ptr<MockResponsePublisher> gMockResponsePublisher;

using ::testing::_;

namespace routeorch_test
{
Expand Down Expand Up @@ -282,6 +286,10 @@ namespace routeorch_test
{"mac_addr", "00:00:00:00:00:00" }});
intfTable.set("Ethernet0:10.0.0.1/24", { { "scope", "global" },
{ "family", "IPv4" }});
intfTable.set("Ethernet4", { {"NULL", "NULL" },
{"mac_addr", "00:00:00:00:00:00" }});
intfTable.set("Ethernet4:11.0.0.1/32", { { "scope", "global" },
{ "family", "IPv4" }});
gIntfsOrch->addExistingData(&intfTable);
static_cast<Orch *>(gIntfsOrch)->doTask();

Expand Down Expand Up @@ -422,4 +430,52 @@ namespace routeorch_test
ASSERT_EQ(current_set_count + 1, set_route_count);
ASSERT_EQ(sai_fail_count, 0);
}

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

std::deque<KeyOpFieldsValuesTuple> entries;
std::string key = "2.2.2.0/24";
std::vector<FieldValueTuple> fvs{{"ifname", "Ethernet0,Ethernet0"}, {"nexthop", "10.0.0.2,10.0.0.3"}, {"protocol", "bgp"}};
entries.push_back({key, "SET", fvs});

auto consumer = dynamic_cast<Consumer *>(gRouteOrch->getExecutor(APP_ROUTE_TABLE_NAME));
consumer->addToSync(entries);

EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector<FieldValueTuple>{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
static_cast<Orch *>(gRouteOrch)->doTask();

entries.clear();

// Route deletion

entries.clear();
entries.push_back({key, "DEL", {}});

consumer->addToSync(entries);

EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector<FieldValueTuple>{}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
static_cast<Orch *>(gRouteOrch)->doTask();

gMockResponsePublisher.reset();
}

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

std::deque<KeyOpFieldsValuesTuple> entries;
std::string key = "11.0.0.1/32";
std::vector<FieldValueTuple> fvs{{"ifname", "Ethernet4"}, {"nexthop", "0.0.0.0"}, {"protocol", "bgp"}};
entries.push_back({key, "SET", fvs});

auto consumer = dynamic_cast<Consumer *>(gRouteOrch->getExecutor(APP_ROUTE_TABLE_NAME));
consumer->addToSync(entries);

EXPECT_CALL(*gMockResponsePublisher, publish(APP_ROUTE_TABLE_NAME, key, std::vector<FieldValueTuple>{{"protocol", "bgp"}}, ReturnCode(SAI_STATUS_SUCCESS), false)).Times(1);
static_cast<Orch *>(gRouteOrch)->doTask();

gMockResponsePublisher.reset();
}
}