Skip to content

Commit

Permalink
[portsorch] process PortsOrch tables in specific order (#1108)
Browse files Browse the repository at this point in the history
* [portsorch] process PortsOrch tables in specific order
* [portsorch] fix other tables not drained and make tableOrder constexpr
* [portsorch_ut] fix test according to fix in portsorch
Don't care if PORT_TABLE was not processed fully,
we care about LAG, LAG_TABLE, VLAN and VLAN_TABLE.
* [tests] fix ut after unsuccessful conflict resolution
  • Loading branch information
stepanblyschak authored and qiluo-msft committed Jan 15, 2020
1 parent 80e01c0 commit 1eac91e
Show file tree
Hide file tree
Showing 4 changed files with 180 additions and 15 deletions.
23 changes: 10 additions & 13 deletions orchagent/orchdaemon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -212,11 +212,11 @@ bool OrchDaemon::init()
* The order of the orch list is important for state restore of warm start and
* the queued processing in m_toSync map after gPortsOrch->allPortsReady() is set.
*
* For the multiple consumers in ports_tables, tasks for LAG_TABLE is processed before VLAN_TABLE
* when iterating ConsumerMap.
* That is ensured implicitly by the order of map key, "LAG_TABLE" is smaller than "VLAN_TABLE" in lexicographic order.
* For the multiple consumers in Orchs, tasks in a table which name is smaller in lexicographic order are processed first
* when iterating ConsumerMap. This is ensured implicitly by the order of keys in ordered map.
* For cases when Orch has to process tables in specific order, like PortsOrch during warm start, it has to override Orch::doTask()
*/
m_orchList = { gSwitchOrch, gCrmOrch, gBufferOrch, gPortsOrch, gIntfsOrch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, wm_orch, policer_orch, sflow_orch, debug_counter_orch};
m_orchList = { gSwitchOrch, gCrmOrch, gPortsOrch, gBufferOrch, gIntfsOrch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, wm_orch, policer_orch, sflow_orch, debug_counter_orch};

bool initialize_dtel = false;
if (platform == BFN_PLATFORM_SUBSTRING || platform == VS_PLATFORM_SUBSTRING)
Expand Down Expand Up @@ -510,20 +510,17 @@ bool OrchDaemon::warmRestoreAndSyncUp()
}

/*
* Four iterations are needed.
* Three iterations are needed.
*
* First iteration: switchorch, Port init/hostif create part of portorch.
* First iteration: switchorch, Port init/hostif create part of portorch, buffers configuration
*
* Second iteratoin: gBufferOrch which requires port created,
* then port speed/mtu/fec_mode/pfc_asym/admin_status config.
* Second iteratoin: port speed/mtu/fec_mode/pfc_asym/admin_status config,
* other orch(s) which wait for port to become ready.
*
* Third iteration: other orch(s) which wait for port init done.
*
* Fourth iteration: Drain remaining data that are out of order like LAG_MEMBER_TABLE and
* VLAN_MEMBER_TABLE since they were checked before LAG_TABLE and VLAN_TABLE within gPortsOrch.
* Third iteration: Drain remaining data that are out of order.
*/

for (auto it = 0; it < 4; it++)
for (auto it = 0; it < 3; it++)
{
SWSS_LOG_DEBUG("The current iteration is %d", it);

Expand Down
28 changes: 28 additions & 0 deletions orchagent/portsorch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2556,6 +2556,34 @@ void PortsOrch::doLagMemberTask(Consumer &consumer)
}
}

void PortsOrch::doTask()
{
constexpr auto tableOrder = {
APP_PORT_TABLE_NAME,
APP_LAG_TABLE_NAME,
APP_LAG_MEMBER_TABLE_NAME,
APP_VLAN_TABLE_NAME,
APP_VLAN_MEMBER_TABLE_NAME,
};

for (auto tableName: tableOrder)
{
auto consumer = getExecutor(tableName);
consumer->drain();
}

// drain remaining tables
for (auto& it: m_consumerMap)
{
auto tableName = it.first;
auto consumer = it.second.get();
if (find(tableOrder.begin(), tableOrder.end(), tableName) == tableOrder.end())
{
consumer->drain();
}
}
}

void PortsOrch::doTask(Consumer &consumer)
{
SWSS_LOG_ENTER();
Expand Down
2 changes: 1 addition & 1 deletion orchagent/portsorch.h
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,9 @@ class PortsOrch : public Orch, public Subject
map<string, uint32_t> m_port_ref_count;
unordered_set<string> m_pendingPortSet;


NotificationConsumer* m_portStatusNotificationConsumer;

void doTask() override;
void doTask(Consumer &consumer);
void doPortTask(Consumer &consumer);
void doVlanTask(Consumer &consumer);
Expand Down
142 changes: 141 additions & 1 deletion tests/mock_tests/portsorch_ut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,7 @@ namespace portsorch_test
Port port;
gPortsOrch->getPort("Ethernet0", port);

auto countersTable = make_shared<Table>(m_counters_db.get(), COUNTERS_TABLE);
auto countersTable = make_shared<Table>(m_counters_db.get(), COUNTERS_TABLE);
auto dropHandler = make_unique<PfcWdZeroBufferHandler>(port.m_port_id, port.m_queue_ids[3], 3, countersTable);

// Create test buffer pool
Expand Down Expand Up @@ -432,4 +432,144 @@ namespace portsorch_test
ASSERT_TRUE(ts.empty()); // PG should be proceesed now
ts.clear();
}

/*
* 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.
* For objects like RIF which are created by a different Orch we know that they will wait until
* allPortsReady(), so we can guaranty they won't be created if PortsOrch can process ports, lags,
* vlans in single doTask().
* If objects are created in PortsOrch, like bridge port, we will spy on SAI API to verify they are
* not called before create_lag_member.
* This is done like this because of limitation on Mellanox platform that does not allow to create objects
* on LAG before at least one LAG members is added in warm reboot. Later this will be fixed.
*
*/
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);
Table vlanTable = Table(m_app_db.get(), APP_VLAN_TABLE_NAME);
Table vlanMemberTable = Table(m_app_db.get(), APP_VLAN_MEMBER_TABLE_NAME);

// 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 = { CFG_BUFFER_POOL_TABLE_NAME,
CFG_BUFFER_PROFILE_TABLE_NAME,
CFG_BUFFER_QUEUE_TABLE_NAME,
CFG_BUFFER_PG_TABLE_NAME,
CFG_BUFFER_PORT_INGRESS_PROFILE_LIST_NAME,
CFG_BUFFER_PORT_EGRESS_PROFILE_LIST_NAME };

ASSERT_EQ(gBufferOrch, nullptr);
gBufferOrch = new BufferOrch(m_config_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.
*/

// 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("PortChannel0001",
{
{"admin_status", "up"},
{"mtu", "9100"}
}
);
lagMemberTable.set(
std::string("PortChannel0001") + lagMemberTable.getTableNameSeparator() + ports.begin()->first,
{ {"status", "enabled"} });
vlanTable.set("Vlan5",
{
{"admin_status", "up"},
{"mtu", "9100"}
}
);
vlanMemberTable.set(
std::string("Vlan5") + vlanMemberTable.getTableNameSeparator() + std::string("PortChannel0001"),
{ {"tagging_mode", "untagged"} }
);

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

// 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));

auto orig_bridge_api = sai_bridge_api;
sai_bridge_api = new sai_bridge_api_t();
memcpy(sai_bridge_api, orig_bridge_api, sizeof(*sai_bridge_api));

bool bridgePortCalled = false;
bool bridgePortCalledBeforeLagMember = 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 {
if (bridgePortCalled) {
bridgePortCalledBeforeLagMember = true;
}
return orig_lag_api->create_lag_member(oid, swoid, count, attrs);
}
);

auto bridgeSpy = SpyOn<SAI_API_BRIDGE, SAI_OBJECT_TYPE_BRIDGE_PORT>(&sai_bridge_api->create_bridge_port);
bridgeSpy->callFake([&](sai_object_id_t *oid, sai_object_id_t swoid, uint32_t count, const sai_attribute_t * attrs) -> sai_status_t {
bridgePortCalled = true;
return orig_bridge_api->create_bridge_port(oid, swoid, count, attrs);
}
);

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

vector<string> ts;

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

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

}

0 comments on commit 1eac91e

Please sign in to comment.