Skip to content

Commit

Permalink
[fdborch]: Fix FdbOrch code to work upon SAI v1.0 (sonic-net#284)
Browse files Browse the repository at this point in the history 
- FDB entry creation code update:
  Add switch_id, bridge_type, bridge_id to FDB entry
  Use bridge port ID as an attribute

- Simplify key tokenization

- Comment table modification code due to race condition issue
  • Loading branch information
Shuotian Cheng authored Aug 11, 2017
1 parent db6b3b5 commit 12b86e7
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 107 deletions.
149 changes: 43 additions & 106 deletions orchagent/fdborch.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#include "tokenize.h"
#include "fdborch.h"

extern sai_object_id_t gSwitchId;

extern sai_fdb_api_t *sai_fdb_api;

void FdbOrch::update(sai_fdb_event_t type, const sai_fdb_entry_t* entry, sai_object_id_t bridge_port_id)
Expand Down Expand Up @@ -83,69 +85,55 @@ void FdbOrch::doTask(Consumer& consumer)
{
KeyOpFieldsValuesTuple t = it->second;

string key = kfvKey(t);
/* format: <VLAN_name>:<MAC_address> */
vector<string> keys = tokenize(kfvKey(t), ':', 1);
string op = kfvOp(t);

FdbEntry entry;
if (!splitKey(key, entry))
Port vlan;
if (!m_portsOrch->getPort(keys[0], vlan))
{
it = consumer.m_toSync.erase(it);
SWSS_LOG_INFO("Failed to locate %s", keys[0].c_str());
it++;
continue;
}

FdbEntry entry;
entry.mac = MacAddress(keys[1]);
entry.vlan = vlan.m_vlan_id;

if (op == SET_COMMAND)
{
string port;
string type;
bool port_defined = false;
bool type_defined = false;

for (auto i : kfvFieldsValues(t))
{
if (fvField(i) == "port")
{
port = fvValue(i);
port_defined = true;
}

if (fvField(i) == "type")
{
type = fvValue(i);
type_defined = true;
}
}

if (!port_defined)
{
SWSS_LOG_ERROR("FDB entry with key:'%s' must have 'port' attribute", key.c_str());
it = consumer.m_toSync.erase(it);
continue;
}

if (!type_defined)
{
SWSS_LOG_ERROR("FDB entry with key:'%s' must have 'type' attribute", key.c_str());
it = consumer.m_toSync.erase(it);
continue;
}

// check that type either static or dynamic
if (type != "static" && type != "dynamic")
{
SWSS_LOG_ERROR("FDB entry with key: '%s' has type '%s'. But allowed only types: 'static' or 'dynamic'",
key.c_str(), type.c_str());
it = consumer.m_toSync.erase(it);
continue;
}
/* FDB type is either dynamic or static */
assert(type == "dynamic" || type == "static");

if (addFdbEntry(entry, port, type))
it = consumer.m_toSync.erase(it);
else
it++;

// Remove AppDb entry if FdbEntry type == 'dynamic'
if (type == "dynamic")
m_table.del(key);
/* Remove corresponding APP_DB entry if type is 'dynamic' */
// FIXME: The modification of table is not thread safe.
// Uncomment this after this issue is fixed.
// if (type == "dynamic")
// {
// m_table.del(kfvKey(t));
// }
}
else if (op == DEL_COMMAND)
{
Expand All @@ -167,9 +155,6 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const
{
SWSS_LOG_ENTER();

SWSS_LOG_DEBUG("mac=%s, vlan=%d. port_name %s. type %s",
entry.mac.to_string().c_str(), entry.vlan, port_name.c_str(), type.c_str());

if (m_entries.count(entry) != 0) // we already have such entries
{
// FIXME: should we check that the entry are moving to another port?
Expand All @@ -178,17 +163,27 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const
return true;
}

sai_status_t status;

sai_fdb_entry_t fdb_entry;

fdb_entry.switch_id = gSwitchId;
memcpy(fdb_entry.mac_address, entry.mac.getMac(), sizeof(sai_mac_t));
fdb_entry.bridge_type = SAI_FDB_ENTRY_BRIDGE_TYPE_1Q;
fdb_entry.vlan_id = entry.vlan;
fdb_entry.bridge_id = SAI_NULL_OBJECT_ID;

Port port;
/* Retry until port is created */
if (!m_portsOrch->getPort(port_name, port))
{
SWSS_LOG_ERROR("Failed to get port id for %s", port_name.c_str());
return true;
SWSS_LOG_INFO("Failed to locate port %s", port_name.c_str());
return false;
}

/* Retry until port is added to the VLAN */
if (!port.m_bridge_port_id)
{
SWSS_LOG_INFO("Port %s does not have a bridge port ID", port_name.c_str());
return false;
}

sai_attribute_t attr;
Expand All @@ -199,23 +194,24 @@ bool FdbOrch::addFdbEntry(const FdbEntry& entry, const string& port_name, const
attrs.push_back(attr);

attr.id = SAI_FDB_ENTRY_ATTR_BRIDGE_PORT_ID;
// FIXME: use bridge port
attr.value.oid = port.m_port_id;
attr.value.oid = port.m_bridge_port_id;
attrs.push_back(attr);

attr.id = SAI_FDB_ENTRY_ATTR_PACKET_ACTION;
attr.value.s32 = SAI_PACKET_ACTION_FORWARD;
attrs.push_back(attr);

status = sai_fdb_api->create_fdb_entry(&fdb_entry, (uint32_t)attrs.size(), attrs.data());
sai_status_t status = sai_fdb_api->create_fdb_entry(&fdb_entry, (uint32_t)attrs.size(), attrs.data());
if (status != SAI_STATUS_SUCCESS)
{
SWSS_LOG_ERROR("Failed to add FDB entry. mac=%s, vlan=%d. port_name %s. type %s",
entry.mac.to_string().c_str(), entry.vlan, port_name.c_str(), type.c_str());
return true;
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);
return false;
}

(void)m_entries.insert(entry);
SWSS_LOG_NOTICE("Create %s FDB %s on %s", type.c_str(), entry.mac.to_string().c_str(), port_name.c_str());

(void) m_entries.insert(entry);

return true;
}
Expand All @@ -224,8 +220,6 @@ bool FdbOrch::removeFdbEntry(const FdbEntry& entry)
{
SWSS_LOG_ENTER();

SWSS_LOG_DEBUG("mac=%s, vlan=%d", entry.mac.to_string().c_str(), entry.vlan);

if (m_entries.count(entry) == 0)
{
SWSS_LOG_ERROR("FDB entry isn't found. mac=%s vlan=%d", entry.mac.to_string().c_str(), entry.vlan);
Expand All @@ -249,60 +243,3 @@ bool FdbOrch::removeFdbEntry(const FdbEntry& entry)

return true;
}

bool FdbOrch::splitKey(const string& key, FdbEntry& entry)
{
SWSS_LOG_ENTER();

string mac_address_str;
string vlan_str;

auto fields = tokenize(key, ':');

if (fields.size() < 2 || fields.size() > 2)
{
SWSS_LOG_ERROR("Failed to parse key: %s", key.c_str());
return false;
}

vlan_str = fields[0];
mac_address_str = fields[1];

if (vlan_str.length() <= 4) // "Vlan"
{
SWSS_LOG_ERROR("Failed to extract vlan interface name from the key: %s", key.c_str());
return false;
}

uint8_t mac_array[6];
if (!MacAddress::parseMacString(mac_address_str, mac_array))
{
SWSS_LOG_ERROR("Failed to parse mac address: %s in key: %s", mac_address_str.c_str(), key.c_str());
return false;
}

if (mac_array[0] & 0x01)
{
SWSS_LOG_ERROR("Mac address %s in key %s should be unicast", mac_address_str.c_str(), key.c_str());
return false;
}

entry.mac = MacAddress(mac_array);

Port port;
if (!m_portsOrch->getPort(vlan_str, port))
{
SWSS_LOG_ERROR("Failed to get port for %s", vlan_str.c_str());
return false;
}

if (port.m_type != Port::VLAN)
{
SWSS_LOG_ERROR("Port %s from key %s must be a vlan port", vlan_str.c_str(), key.c_str());
return false;
}

entry.vlan = port.m_vlan_id;

return true;
}
1 change: 0 additions & 1 deletion orchagent/fdborch.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ class FdbOrch: public Orch, public Subject

bool addFdbEntry(const FdbEntry&, const string&, const string&);
bool removeFdbEntry(const FdbEntry&);
bool splitKey(const string&, FdbEntry&);
};

#endif /* SWSS_FDBORCH_H */

0 comments on commit 12b86e7

Please sign in to comment.