Skip to content

Commit

Permalink
[202012][portsorch] fix errors when moving port from one lag to anoth…
Browse files Browse the repository at this point in the history 
…er. (#1819)

In scenario that is executed in sonic-mgmt in test_po_update.py a portchannel member is deleted from one portchannel and added to another portchannel.
It is possible that requests from teamsynd will arrive in different order
This reordering happens because teamsyncd has single event handler/selectable TeamSync::TeamPortSync::onChange() per team device so when two of them are ready it is swss::Select implementation detail in which order they are going to be returned.
This is a fundamental issue of Producer/ConsumerStateTable, thus orchagent must be aware of this and treat it as normal situation and figure out the right order and not crash or print an errors.

Original PR #1797

**What I did**
Check if port is already a lag member beforehand.
Added an UT to cover this scenario, this UT verifies that SAI API is not called in this case.
Refactored portsorch_ut.cpp by moving out Orchs creation/deletion into SetUp()/TearDown()

**Why I did it**
To fix errors in log.

**How I verified it**
Ran test_po_update.py test.
  • Loading branch information
@stepanblyschak
stepanblyschak authored Jul 20, 2021
1 parent b0fb392 commit a67d8af
Show file tree
Hide file tree
Showing 2 changed files with 163 additions and 101 deletions.
25 changes: 22 additions & 3 deletions orchagent/portsorch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ static char* hostif_vlan_tag[] = {
[SAI_HOSTIF_VLAN_TAG_KEEP] = "SAI_HOSTIF_VLAN_TAG_KEEP",
[SAI_HOSTIF_VLAN_TAG_ORIGINAL] = "SAI_HOSTIF_VLAN_TAG_ORIGINAL"
};

static bool isValidPortTypeForLagMember(const Port& port)
{
return port.m_type == Port::Type::PHY;
}

/*
* Initialize PortsOrch
* 0) If Gearbox is enabled, then initialize the external PHYs as defined in
Expand Down Expand Up @@ -1938,7 +1944,7 @@ sai_status_t PortsOrch::removePort(sai_object_id_t port_id)

Port port;

/*
/*
* Make sure to bring down admin state.
* SET would have replaced with DEL
*/
Expand Down Expand Up @@ -3189,6 +3195,15 @@ void PortsOrch::doLagMemberTask(Consumer &consumer)
continue;
}

/* Fail if a port type is not a valid type for being a LAG member port.
* Erase invalid entry, no need to retry in this case. */
if (!isValidPortTypeForLagMember(port))
{
SWSS_LOG_ERROR("LAG member port has to be of type PHY or SYSTEM");
it = consumer.m_toSync.erase(it);
continue;
}

/* Update a LAG member */
if (op == SET_COMMAND)
{
Expand All @@ -3201,8 +3216,12 @@ void PortsOrch::doLagMemberTask(Consumer &consumer)

if (lag.m_members.find(port_alias) == lag.m_members.end())
{
/* Assert the port doesn't belong to any LAG already */
assert(!port.m_lag_id && !port.m_lag_member_id);
if (port.m_lag_member_id != SAI_NULL_OBJECT_ID)
{
SWSS_LOG_INFO("Port %s is already a LAG member", port.m_alias.c_str());
it++;
continue;
}

if (!addLagMember(lag, port, (status == "enabled")))
{
Expand Down
239 changes: 141 additions & 98 deletions tests/mock_tests/portsorch_ut.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
#define private public // make Directory::m_values available to clean it.
#include "directory.h"
#undef private

#include "ut_helper.h"
#include "mock_orchagent_main.h"
#include "mock_table.h"
Expand Down Expand Up @@ -33,17 +37,51 @@ namespace portsorch_test
virtual void SetUp() override
{
::testing_db::reset();

// Create dependencies ...

const int portsorch_base_pri = 40;

vector<table_name_with_pri_t> ports_tables = {
{ APP_PORT_TABLE_NAME, portsorch_base_pri + 5 },
{ APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 },
{ APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri },
{ APP_LAG_TABLE_NAME, portsorch_base_pri + 4 },
{ APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri }
};

ASSERT_EQ(gPortsOrch, nullptr);

vector<string> flex_counter_tables = {
CFG_FLEX_COUNTER_TABLE_NAME
};
auto* flexCounterOrch = new FlexCounterOrch(m_config_db.get(), flex_counter_tables);
gDirectory.set(flexCounterOrch);

gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables);
vector<string> buffer_tables = { APP_BUFFER_POOL_TABLE_NAME,
APP_BUFFER_PROFILE_TABLE_NAME,
APP_BUFFER_QUEUE_TABLE_NAME,
APP_BUFFER_PG_TABLE_NAME,
APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME };

ASSERT_EQ(gBufferOrch, nullptr);
gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables);
}

virtual void TearDown() override
{
::testing_db::reset();

delete gPortsOrch;
gPortsOrch = nullptr;
delete gBufferOrch;
gBufferOrch = nullptr;
}

// clear orchs saved in directory
gDirectory.m_values.clear();
}
static void SetUpTestCase()
{
// Init switch and create dependencies
Expand Down Expand Up @@ -89,6 +127,7 @@ namespace portsorch_test

ut_helper::uninitSaiApi();
}

};

TEST_F(PortsOrchTest, PortReadinessColdBoot)
Expand Down Expand Up @@ -129,36 +168,14 @@ namespace portsorch_test
pgTableCfg.set(ossCfg.str(), { { "profile", "[BUFFER_PROFILE|test_profile]" } });
}

// Create dependencies ...

const int portsorch_base_pri = 40;

vector<table_name_with_pri_t> ports_tables = {
{ APP_PORT_TABLE_NAME, portsorch_base_pri + 5 },
{ APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 },
{ APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri },
{ APP_LAG_TABLE_NAME, portsorch_base_pri + 4 },
{ APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri }
};

ASSERT_EQ(gPortsOrch, nullptr);

vector<string> flex_counter_tables = {
CFG_FLEX_COUNTER_TABLE_NAME
};
auto* flexCounterOrch = new FlexCounterOrch(m_config_db.get(), flex_counter_tables);

gDirectory.set(flexCounterOrch);

gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables);
// Recreate buffer orch to read populated data
vector<string> buffer_tables = { APP_BUFFER_POOL_TABLE_NAME,
APP_BUFFER_PROFILE_TABLE_NAME,
APP_BUFFER_QUEUE_TABLE_NAME,
APP_BUFFER_PG_TABLE_NAME,
APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME };

ASSERT_EQ(gBufferOrch, nullptr);
gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables);

// Populate pot table with SAI ports
Expand Down Expand Up @@ -221,7 +238,6 @@ namespace portsorch_test

TEST_F(PortsOrchTest, PortReadinessWarmBoot)
{

Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
Table pgTable = Table(m_app_db.get(), APP_BUFFER_PG_TABLE_NAME);
Table profileTable = Table(m_app_db.get(), APP_BUFFER_PROFILE_TABLE_NAME);
Expand Down Expand Up @@ -266,30 +282,6 @@ namespace portsorch_test
portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } });
portTable.set("PortInitDone", { { "lanes", "0" } });

// Create dependencies ...

const int portsorch_base_pri = 40;

vector<table_name_with_pri_t> ports_tables = {
{ APP_PORT_TABLE_NAME, portsorch_base_pri + 5 },
{ APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 },
{ APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri },
{ APP_LAG_TABLE_NAME, portsorch_base_pri + 4 },
{ APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri }
};

ASSERT_EQ(gPortsOrch, nullptr);
gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables);
vector<string> buffer_tables = { APP_BUFFER_POOL_TABLE_NAME,
APP_BUFFER_PROFILE_TABLE_NAME,
APP_BUFFER_QUEUE_TABLE_NAME,
APP_BUFFER_PG_TABLE_NAME,
APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME };

ASSERT_EQ(gBufferOrch, nullptr);
gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables);

// warm start, bake fill refill consumer

gBufferOrch->bake();
Expand Down Expand Up @@ -336,30 +328,6 @@ namespace portsorch_test
// Get SAI default ports to populate DB
auto ports = ut_helper::getInitialSaiPorts();

// Create dependencies ...

const int portsorch_base_pri = 40;

vector<table_name_with_pri_t> ports_tables = {
{ APP_PORT_TABLE_NAME, portsorch_base_pri + 5 },
{ APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 },
{ APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri },
{ APP_LAG_TABLE_NAME, portsorch_base_pri + 4 },
{ APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri }
};

ASSERT_EQ(gPortsOrch, nullptr);
gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables);
vector<string> buffer_tables = { APP_BUFFER_POOL_TABLE_NAME,
APP_BUFFER_PROFILE_TABLE_NAME,
APP_BUFFER_QUEUE_TABLE_NAME,
APP_BUFFER_PG_TABLE_NAME,
APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME };

ASSERT_EQ(gBufferOrch, nullptr);
gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables);

// Populate port table with SAI ports
for (const auto &it : ports)
{
Expand Down Expand Up @@ -458,6 +426,106 @@ namespace portsorch_test
ts.clear();
}

/* This test checks that a LAG member validation happens on orchagent level
* and no SAI call is executed in case a port requested to be a LAG member
* is already a LAG member.
*/
TEST_F(PortsOrchTest, LagMemberDoesNotCallSAIApiWhenPortIsAlreadyALagMember)
{
Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
Table lagTable = Table(m_app_db.get(), APP_LAG_TABLE_NAME);
Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME);

// Get SAI default ports to populate DB
auto ports = ut_helper::getInitialSaiPorts();

/*
* Next we will prepare some configuration data to be consumed by PortsOrch
* 32 Ports, 2 LAGs, 1 port is LAG member.
*/

// Populate pot table with SAI ports
for (const auto &it : ports)
{
portTable.set(it.first, it.second);
}

// Set PortConfigDone
portTable.set("PortConfigDone", { { "count", to_string(ports.size()) } });
portTable.set("PortInitDone", { { } });

lagTable.set("PortChannel999",
{
{"admin_status", "up"},
{"mtu", "9100"}
}
);
lagTable.set("PortChannel0001",
{
{"admin_status", "up"},
{"mtu", "9100"}
}
);
lagMemberTable.set(
std::string("PortChannel999") + lagMemberTable.getTableNameSeparator() + ports.begin()->first,
{ {"status", "enabled"} });

// refill consumer
gPortsOrch->addExistingData(&portTable);
gPortsOrch->addExistingData(&lagTable);
gPortsOrch->addExistingData(&lagMemberTable);

static_cast<Orch *>(gPortsOrch)->doTask();

// check LAG, VLAN tasks were processed
// port table may require one more doTask iteration
for (auto tableName: {APP_LAG_TABLE_NAME, APP_LAG_MEMBER_TABLE_NAME})
{
vector<string> ts;
auto exec = gPortsOrch->getExecutor(tableName);
auto consumer = static_cast<Consumer*>(exec);
ts.clear();
consumer->dumpPendingTasks(ts);
ASSERT_TRUE(ts.empty());
}

// Set first port as a LAG member while this port is still a member of different LAG.
lagMemberTable.set(
std::string("PortChannel0001") + lagMemberTable.getTableNameSeparator() + ports.begin()->first,
{ {"status", "enabled"} });

// save original api since we will spy
auto orig_lag_api = sai_lag_api;
sai_lag_api = new sai_lag_api_t();
memcpy(sai_lag_api, orig_lag_api, sizeof(*sai_lag_api));

bool lagMemberCreateCalled = false;

auto lagSpy = SpyOn<SAI_API_LAG, SAI_OBJECT_TYPE_LAG_MEMBER>(&sai_lag_api->create_lag_member);
lagSpy->callFake([&](sai_object_id_t *oid, sai_object_id_t swoid, uint32_t count, const sai_attribute_t * attrs) -> sai_status_t
{
lagMemberCreateCalled = true;
return orig_lag_api->create_lag_member(oid, swoid, count, attrs);
}
);

gPortsOrch->addExistingData(&lagMemberTable);

static_cast<Orch *>(gPortsOrch)->doTask();
sai_lag_api = orig_lag_api;

// verify there is a pending task to do.
vector<string> ts;
auto exec = gPortsOrch->getExecutor(APP_LAG_MEMBER_TABLE_NAME);
auto consumer = static_cast<Consumer*>(exec);
ts.clear();
consumer->dumpPendingTasks(ts);
ASSERT_FALSE(ts.empty());

// verify there was no SAI call executed.
ASSERT_FALSE(lagMemberCreateCalled);
}

/*
* The scope of this test is to verify that LAG member is
* added to a LAG before any other object on LAG is created, like RIF, bridge port in warm mode.
Expand All @@ -472,7 +540,6 @@ namespace portsorch_test
*/
TEST_F(PortsOrchTest, LagMemberIsCreatedBeforeOtherObjectsAreCreatedOnLag)
{

Table portTable = Table(m_app_db.get(), APP_PORT_TABLE_NAME);
Table lagTable = Table(m_app_db.get(), APP_LAG_TABLE_NAME);
Table lagMemberTable = Table(m_app_db.get(), APP_LAG_MEMBER_TABLE_NAME);
Expand All @@ -482,29 +549,6 @@ namespace portsorch_test
// Get SAI default ports to populate DB
auto ports = ut_helper::getInitialSaiPorts();

// Create dependencies ...
const int portsorch_base_pri = 40;

vector<table_name_with_pri_t> ports_tables = {
{ APP_PORT_TABLE_NAME, portsorch_base_pri + 5 },
{ APP_VLAN_TABLE_NAME, portsorch_base_pri + 2 },
{ APP_VLAN_MEMBER_TABLE_NAME, portsorch_base_pri },
{ APP_LAG_TABLE_NAME, portsorch_base_pri + 4 },
{ APP_LAG_MEMBER_TABLE_NAME, portsorch_base_pri }
};

ASSERT_EQ(gPortsOrch, nullptr);
gPortsOrch = new PortsOrch(m_app_db.get(), ports_tables);
vector<string> buffer_tables = { APP_BUFFER_POOL_TABLE_NAME,
APP_BUFFER_PROFILE_TABLE_NAME,
APP_BUFFER_QUEUE_TABLE_NAME,
APP_BUFFER_PG_TABLE_NAME,
APP_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
APP_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME };

ASSERT_EQ(gBufferOrch, nullptr);
gBufferOrch = new BufferOrch(m_app_db.get(), m_config_db.get(), m_state_db.get(), buffer_tables);

/*
* Next we will prepare some configuration data to be consumed by PortsOrch
* 32 Ports, 1 LAG, 1 port is LAG member and LAG is in Vlan.
Expand Down Expand Up @@ -596,5 +640,4 @@ namespace portsorch_test

ASSERT_FALSE(bridgePortCalledBeforeLagMember); // bridge port created on lag before lag member was created
}

}

0 comments on commit a67d8af

Please sign in to comment.