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.

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

[DPB/VLAN] Add test VS cases and fix FDB flush issues #1242

Merged
merged 10 commits into from
Jun 10, 2020
Merged

[DPB/VLAN] Add test VS cases and fix FDB flush issues #1242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
66cf33f
Flush fdb entries when port/LAG is operational down (#14)
zhenggen-xu Dec 11, 2019
813c9d8
FIX bugs and add VLAN scale test (#27)
vasant17 Mar 30, 2020
1141437
Refactor DPB-VLAN test cases based on DVSlib
May 7, 2020
4cfe3ff
Fixed VS test case failures
May 9, 2020
518028f
Code-review comments addressed
May 14, 2020
0961590
Move Flush FDB from portsorch to fdborch
May 28, 2020
8e4ab1e
Addressed code-review comments and additional cleanup
Jun 6, 2020
eb2c937
Code-review comments addressed
Jun 8, 2020
a5dedc2
Remove lag related methods from dvs_vlan.py
Jun 10, 2020
d205997
Fix a bug
Jun 10, 2020
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
188 changes: 161 additions & 27 deletions orchagent/fdborch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,13 @@ bool FdbOrch::bake()
bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
{
const FdbEntry& entry = update.entry;
const Port& port = update.port;
const MacAddress& mac = entry.mac;
string portName = port.m_alias;
Port vlan;

if (!m_portsOrch->getPort(entry.bv_id, vlan))
{
SWSS_LOG_NOTICE("FdbOrch notification: Failed to locate vlan port from bv_id 0x%" PRIx64, entry.bv_id);
SWSS_LOG_NOTICE("FdbOrch notification: Failed to locate \
vlan port from bv_id 0x%" PRIx64, entry.bv_id);
return false;
}

Expand All @@ -73,6 +72,9 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)

if (update.add)
{
SWSS_LOG_INFO("Storing FDB entry: [%s, 0x%" PRIx64 "] [ port: %s ]",
entry.mac.to_string().c_str(),
entry.bv_id, entry.port_name.c_str());
auto inserted = m_entries.insert(entry);

SWSS_LOG_DEBUG("FdbOrch notification: mac %s was inserted into bv_id 0x%" PRIx64,
Expand All @@ -86,7 +88,7 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)

// Write to StateDb
std::vector<FieldValueTuple> fvs;
fvs.push_back(FieldValueTuple("port", portName));
fvs.push_back(FieldValueTuple("port", entry.port_name));
fvs.push_back(FieldValueTuple("type", "dynamic"));
m_fdbStateTable.set(key, fvs);

Expand All @@ -111,14 +113,29 @@ bool FdbOrch::storeFdbEntryState(const FdbUpdate& update)
}
}

void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id)
void FdbOrch::update(sai_fdb_event_t type,
const sai_fdb_entry_t* entry,
sai_object_id_t bridge_port_id)
{
SWSS_LOG_ENTER();

FdbUpdate update;
update.entry.mac = entry->mac_address;
update.entry.bv_id = entry->bv_id;

SWSS_LOG_INFO("FDB event:%d, MAC: %s , BVID: 0x%" PRIx64 " , \
bridge port ID: 0x%" PRIx64 ".",
type, update.entry.mac.to_string().c_str(),
entry->bv_id, bridge_port_id);

if (bridge_port_id &&
!m_portsOrch->getPortByBridgePortId(bridge_port_id, update.port))
{
SWSS_LOG_ERROR("Failed to get port by bridge port ID 0x%" PRIx64 ".",
bridge_port_id);
return;
}

switch (type)
{
case SAI_FDB_EVENT_LEARNED:
Expand All @@ -137,8 +154,10 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj
}

update.add = true;
update.entry.port_name = update.port.m_alias;
storeFdbEntryState(update);

SWSS_LOG_INFO("Notifying observers of FDB entry LEARN");
for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
Expand All @@ -151,6 +170,7 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj
update.add = false;
storeFdbEntryState(update);

SWSS_LOG_INFO("Notifying observers of FDB entry removal on AGED/MOVED");
for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
Expand All @@ -159,8 +179,31 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj
break;

case SAI_FDB_EVENT_FLUSHED:
if (bridge_port_id == SAI_NULL_OBJECT_ID && entry->bv_id == SAI_NULL_OBJECT_ID)

SWSS_LOG_INFO("FDB Flush event received: [ %s , 0x%" PRIx64 " ], \
bridge port ID: 0x%" PRIx64 ".",
update.entry.mac.to_string().c_str(), entry->bv_id,
bridge_port_id);

string vlanName = "-";
if (entry->bv_id) {
Port vlan;

if (!m_portsOrch->getPort(entry->bv_id, vlan))
{
SWSS_LOG_ERROR("FdbOrch notification: Failed to locate vlan\
port from bv_id 0x%" PRIx64, entry->bv_id);
return;
}
vlanName = "Vlan" + to_string(vlan.m_vlan_info.vlan_id);
}


if (bridge_port_id == SAI_NULL_OBJECT_ID &&
entry->bv_id == SAI_NULL_OBJECT_ID)
{
SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: - }",
update.entry.mac.to_string().c_str(), vlanName.c_str());
for (auto itr = m_entries.begin(); itr != m_entries.end();)
{
/*
Expand All @@ -176,27 +219,50 @@ void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_obj

storeFdbEntryState(update);

SWSS_LOG_DEBUG("FdbOrch notification: mac %s was removed", update.entry.mac.to_string().c_str());

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
}
}
else if (bridge_port_id && entry->bv_id == SAI_NULL_OBJECT_ID)
else if (entry->bv_id == SAI_NULL_OBJECT_ID)
{
/*this is a placeholder for flush port fdb case, not supported yet.*/
SWSS_LOG_ERROR("FdbOrch notification: not supported flush port fdb action, port_id = 0x%" PRIx64 ", bv_id = 0x%" PRIx64 ".", bridge_port_id, entry->bv_id);
/* FLUSH based on port */
SWSS_LOG_INFO("FDB Flush: [ %s , %s ] = { port: %s }",
update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());

for (const auto& itr : m_entries)
{
if (itr.port_name == update.port.m_alias)
{
update.entry.mac = itr.mac;
update.entry.bv_id = itr.bv_id;
update.add = false;

storeFdbEntryState(update);

for (auto observer: m_observers)
{
observer->update(SUBJECT_TYPE_FDB_CHANGE, &update);
}
}
}
}
else if (bridge_port_id == SAI_NULL_OBJECT_ID && entry->bv_id != SAI_NULL_OBJECT_ID)
else if (bridge_port_id == SAI_NULL_OBJECT_ID)
{
/*this is a placeholder for flush vlan fdb case, not supported yet.*/
SWSS_LOG_ERROR("FdbOrch notification: not supported flush vlan fdb action, port_id = 0x%" PRIx64 ", bv_id = 0x%" PRIx64 ".", bridge_port_id, entry->bv_id);
/* FLUSH based on VLAN - unsupported */
SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: - }",
update.entry.mac.to_string().c_str(),
vlanName.c_str());

}
else
{
SWSS_LOG_ERROR("FdbOrch notification: not supported flush fdb action, port_id = 0x%" PRIx64 ", bv_id = 0x%" PRIx64 ".", bridge_port_id, entry->bv_id);
/* FLUSH based on port and VLAN - unsupported */
SWSS_LOG_ERROR("Unsupported FDB Flush: [ %s , %s ] = { port: %s }",
update.entry.mac.to_string().c_str(),
vlanName.c_str(), update.port.m_alias.c_str());
}
break;
}
Expand All @@ -217,6 +283,12 @@ void FdbOrch::update(SubjectType type, void *cntx)
updateVlanMember(*update);
break;
}
case SUBJECT_TYPE_PORT_OPER_STATE_CHANGE:
{
PortOperStateUpdate *update = reinterpret_cast<PortOperStateUpdate *>(cntx);
updatePortOperState(*update);
break;
}
default:
break;
}
Expand Down Expand Up @@ -307,10 +379,11 @@ void FdbOrch::doTask(Consumer& consumer)
}
}

entry.port_name = port;
/* FDB type is either dynamic or static */
assert(type == "dynamic" || type == "static");

if (addFdbEntry(entry, port, type))
if (addFdbEntry(entry, type))
it = consumer.m_toSync.erase(it);
else
it++;
Expand Down Expand Up @@ -416,13 +489,67 @@ void FdbOrch::doTask(NotificationConsumer& consumer)
}
}

void FdbOrch::flushFDBEntries(sai_object_id_t bridge_port_oid,
sai_object_id_t vlan_oid)
{
vector<sai_attribute_t> attrs;
sai_attribute_t attr;
sai_status_t rv = SAI_STATUS_SUCCESS;

SWSS_LOG_ENTER();

if (SAI_NULL_OBJECT_ID == bridge_port_oid &&
SAI_NULL_OBJECT_ID == vlan_oid)
{
SWSS_LOG_WARN("Couldn't flush FDB. Bridge port OID: 0x%" PRIx64 " bvid:%" PRIx64 ",",
bridge_port_oid, vlan_oid);
return;
}

if (SAI_NULL_OBJECT_ID != bridge_port_oid)
{
attr.id = SAI_FDB_FLUSH_ATTR_BRIDGE_PORT_ID;
attr.value.oid = bridge_port_oid;
attrs.push_back(attr);
}

if (SAI_NULL_OBJECT_ID != vlan_oid)
{
attr.id = SAI_FDB_FLUSH_ATTR_BV_ID;
attr.value.oid = vlan_oid;
attrs.push_back(attr);
}

SWSS_LOG_INFO("Flushing FDB bridge_port_oid: 0x%" PRIx64 ", and bvid_oid:0x%" PRIx64 ".", bridge_port_oid, vlan_oid);

rv = sai_fdb_api->flush_fdb_entries(gSwitchId, (uint32_t)attrs.size(), attrs.data());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need CRM counters update. Can you check?

if (SAI_STATUS_SUCCESS != rv)
{
SWSS_LOG_ERROR("Flushing FDB failed. rv:%d", rv);
}
}

void FdbOrch::updatePortOperState(const PortOperStateUpdate& update)
{
SWSS_LOG_ENTER();
if (update.operStatus == SAI_PORT_OPER_STATUS_DOWN)
Copy link
Contributor

@qiluo-msft qiluo-msft Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

SAI_PORT_OPER_STATUS_DOWN [](start = 29, length = 25)

How about SAI_PORT_OPER_STATUS_UP? #Closed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't do anything. On down to up transition, we update FDB entries on learning. But on up to down we want to flush FDB entries.

{
swss::Port p = update.port;
flushFDBEntries(p.m_bridge_port_id, SAI_NULL_OBJECT_ID);
}
return;
}

void FdbOrch::updateVlanMember(const VlanMemberUpdate& update)
{
SWSS_LOG_ENTER();

if (!update.add)
{
return; // we need additions only
swss::Port vlan = update.vlan;
swss::Port port = update.member;
flushFDBEntries(port.m_bridge_port_id, vlan.m_vlan_info.vlan_oid);
return;
}

string port_name = update.member.m_alias;
Expand All @@ -433,12 +560,12 @@ void FdbOrch::updateVlanMember(const VlanMemberUpdate& update)
{
// try to insert an FDB entry. If the FDB entry is not ready to be inserted yet,
// it would be added back to the saved_fdb_entries structure by addFDBEntry()
(void)addFdbEntry(fdb.entry, port_name, fdb.type);
(void)addFdbEntry(fdb.entry, fdb.type);
}
}
}

bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const string& type)
bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& type)
{
SWSS_LOG_ENTER();

Expand All @@ -450,19 +577,21 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const

Port port;
/* Retry until port is created */
if (!m_portsOrch->getPort(port_name, port))
if (!m_portsOrch->getPort(entry.port_name, port))
{
SWSS_LOG_DEBUG("Saving a fdb entry until port %s becomes active", port_name.c_str());
saved_fdb_entries[port_name].push_back({entry, type});
SWSS_LOG_DEBUG("Saving a fdb entry until port %s becomes active",
entry. port_name.c_str());
saved_fdb_entries[entry.port_name].push_back({entry, type});

return true;
}

/* Retry until port is added to the VLAN */
if (!port.m_bridge_port_id)
{
SWSS_LOG_DEBUG("Saving a fdb entry until port %s has got a bridge port ID", port_name.c_str());
saved_fdb_entries[port_name].push_back({entry, type});
SWSS_LOG_DEBUG("Saving a fdb entry until port %s has got a bridge port ID",
entry.port_name.c_str());
saved_fdb_entries[entry.port_name].push_back({entry, type});

return true;
}
Expand Down Expand Up @@ -491,11 +620,14 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to create %s FDB %s on %s, rv:%d",
type.c_str(), entry.mac.to_string().c_str(), port_name.c_str(), status);
type.c_str(), entry.mac.to_string().c_str(),
entry.port_name.c_str(), status);
return false; //FIXME: it should be based on status. Some could be retried, some not
}

SWSS_LOG_NOTICE("Create %s FDB %s on %s", type.c_str(), entry.mac.to_string().c_str(), port_name.c_str());
SWSS_LOG_NOTICE("Storing FDB entry: [%s, 0x%" PRIx64 "] [ port: %s , type: %s]",
entry.mac.to_string().c_str(),
entry.bv_id, entry.port_name.c_str(), type.c_str());

(void) m_entries.insert(entry);

Expand All @@ -516,7 +648,8 @@ bool FdbOrch::removeFdbEntry(const FdbEntry& entry)

if (m_entries.count(entry) == 0)
{
SWSS_LOG_ERROR("FDB entry isn't found. mac=%s bv_id=0x%" PRIx64, entry.mac.to_string().c_str(), entry.bv_id);
SWSS_LOG_ERROR("FDB entry isn't found. mac=%s bv_id=0x%" PRIx64 ".",
entry.mac.to_string().c_str(), entry.bv_id);
return true;
}

Expand All @@ -541,6 +674,7 @@ bool FdbOrch::removeFdbEntry(const FdbEntry& entry)
Port port;
m_portsOrch->getPortByBridgePortId(entry.bv_id, port);

SWSS_LOG_INFO("Notifying observers of FDB entry removal");
FdbUpdate update = {entry, port, false};
for (auto observer: m_observers)
{
Expand Down
6 changes: 5 additions & 1 deletion orchagent/fdborch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ struct FdbEntry
{
MacAddress mac;
sai_object_id_t bv_id;
std::string port_name;

bool operator<(const FdbEntry& other) const
{
Expand Down Expand Up @@ -60,8 +61,11 @@ class FdbOrch: public Orch, public Subject, public Observer
void doTask(NotificationConsumer& consumer);

void updateVlanMember(const VlanMemberUpdate&);
bool addFdbEntry(const FdbEntry&, const string&, const string&);
void updatePortOperState(const PortOperStateUpdate&);
bool addFdbEntry(const FdbEntry&, const string&);
bool removeFdbEntry(const FdbEntry&);
void flushFDBEntries(sai_object_id_t bridge_port_oid,
sai_object_id_t vlan_oid);

bool storeFdbEntryState(const FdbUpdate& update);
};
Expand Down
1 change: 1 addition & 0 deletions orchagent/observer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ enum SubjectType
SUBJECT_TYPE_MIRROR_SESSION_CHANGE,
SUBJECT_TYPE_INT_SESSION_CHANGE,
SUBJECT_TYPE_PORT_CHANGE,
SUBJECT_TYPE_PORT_OPER_STATE_CHANGE,
};

class Observer
Expand Down
Loading