Skip to content

[202012][portsorch] fix errors when moving port from one lag to another. #1819

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

Merged
qiluo-msft merged 4 commits into from
Jul 20, 2021
Merged

[202012][portsorch] fix errors when moving port from one lag to another. #1819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6f84acf
[portsorch] fix errors when moving port from one lag to another. (#1797)
stepanblyschak Jul 8, 2021
91a85d9
fix compilation
stepanblyschak Jul 14, 2021
e64329a
fix test for 202012
stepanblyschak Jul 15, 2021
3d8e1e3
fix test for 202012
stepanblyschak Jul 15, 2021
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
25 changes: 22 additions & 3 deletions orchagent/portsorch.cpp
View file
Open in desktop
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
Open in desktop
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
}

}