-
Notifications
You must be signed in to change notification settings - Fork 549
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.
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
Layer 2 Forwarding Enhancements #885
Changes from 12 commits
9afa981
9f9935d
14f36f4
23dc382
d5370f4
80a91a5
0398cc8
a53a461
e02a7e3
fb1d01b
93b50a5
a61485a
3269a27
30dfa58
81b5f54
0df7f35
cd6af32
7cb0d37
7c29491
753acb4
0d0cb03
869937e
379c2d7
da3f975
c852011
a9fb507
a9c4b5d
bcaeb98
602bf2b
61c0f75
a5c33d2
757cf70
56e330e
33f513f
94bbb50
cac0096
47bcce9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ using namespace swss; | |
|
||
#define DOT1Q_BRIDGE_NAME "Bridge" | ||
#define VLAN_PREFIX "Vlan" | ||
#define SWITCH_STR "switch" | ||
#define LAG_PREFIX "PortChannel" | ||
#define DEFAULT_VLAN_ID "1" | ||
#define DEFAULT_MTU_STR "9100" | ||
|
@@ -28,6 +29,8 @@ VlanMgr::VlanMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c | |
m_stateLagTable(stateDb, STATE_LAG_TABLE_NAME), | ||
m_stateVlanTable(stateDb, STATE_VLAN_TABLE_NAME), | ||
m_stateVlanMemberTable(stateDb, STATE_VLAN_MEMBER_TABLE_NAME), | ||
m_appFdbTableProducer(appDb, APP_FDB_TABLE_NAME), | ||
m_appSwitchTableProducer(appDb, APP_SWITCH_TABLE_NAME), | ||
m_appVlanTableProducer(appDb, APP_VLAN_TABLE_NAME), | ||
m_appVlanMemberTableProducer(appDb, APP_VLAN_MEMBER_TABLE_NAME) | ||
{ | ||
|
@@ -86,6 +89,10 @@ VlanMgr::VlanMgr(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, c | |
|
||
EXEC_WITH_ERROR_THROW(echo_cmd_backup, res); | ||
} | ||
/* vlan state notification from portsorch */ | ||
m_VlanStateNotificationConsumer = new swss::NotificationConsumer(appDb, "VLANSTATE"); | ||
auto vlanStatusNotificatier = new Notifier(m_VlanStateNotificationConsumer, this, "VLANSTATE"); | ||
Orch::addExecutor(vlanStatusNotificatier); | ||
} | ||
|
||
bool VlanMgr::addHostVlan(int vlan_id) | ||
|
@@ -99,7 +106,7 @@ bool VlanMgr::addHostVlan(int vlan_id) | |
+ BASH_CMD + " -c \"" | ||
+ BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + DOT1Q_BRIDGE_NAME + " self && " | ||
+ IP_CMD + " link add link " + DOT1Q_BRIDGE_NAME | ||
+ " up" | ||
+ " down" | ||
+ " name " + VLAN_PREFIX + std::to_string(vlan_id) | ||
+ " address " + gMacAddress.to_string() | ||
+ " type vlan id " + std::to_string(vlan_id) + "\""; | ||
|
@@ -177,14 +184,27 @@ bool VlanMgr::addHostVlanMember(int vlan_id, const string &port_alias, const str | |
// /bin/bash -c "/sbin/ip link set {{port_alias}} master Bridge && | ||
// /sbin/bridge vlan del vid 1 dev {{ port_alias }} && | ||
// /sbin/bridge vlan add vid {{vlan_id}} dev {{port_alias}} {{tagging_mode}}" | ||
const std::string cmds = std::string("") | ||
+ BASH_CMD + " -c \"" | ||
+ IP_CMD + " link set " + port_alias + " master " + DOT1Q_BRIDGE_NAME + " && " | ||
+ BRIDGE_CMD + " vlan del vid " + DEFAULT_VLAN_ID + " dev " + port_alias + " && " | ||
+ BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + port_alias + " " + tagging_cmd + "\""; | ||
|
||
const std::string key = std::string("") + "Vlan1|" + port_alias; | ||
|
||
std::string res; | ||
EXEC_WITH_ERROR_THROW(cmds, res); | ||
if (isVlanMemberStateOk(key)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, update the comments above to reflect the changes here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added explanation in comments section: |
||
const std::string cmds = std::string("") | ||
+ BASH_CMD + " -c \"" | ||
+ IP_CMD + " link set " + port_alias + " master " + DOT1Q_BRIDGE_NAME + " && " | ||
+ BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + port_alias + " " + tagging_cmd + "\""; | ||
std::string res; | ||
EXEC_WITH_ERROR_THROW(cmds, res); | ||
} | ||
else | ||
{ | ||
const std::string cmds = std::string("") | ||
+ BASH_CMD + " -c \"" | ||
+ IP_CMD + " link set " + port_alias + " master " + DOT1Q_BRIDGE_NAME + " && " | ||
+ BRIDGE_CMD + " vlan del vid " + DEFAULT_VLAN_ID + " dev " + port_alias + " && " | ||
+ BRIDGE_CMD + " vlan add vid " + std::to_string(vlan_id) + " dev " + port_alias + " " + tagging_cmd + "\""; | ||
std::string res; | ||
EXEC_WITH_ERROR_THROW(cmds, res); | ||
} | ||
|
||
return true; | ||
} | ||
|
@@ -222,6 +242,144 @@ bool VlanMgr::isVlanMacOk() | |
return !!gMacAddress; | ||
} | ||
|
||
void VlanMgr::doSwitchTask(Consumer &consumer) | ||
{ | ||
SWSS_LOG_ENTER(); | ||
auto it = consumer.m_toSync.begin(); | ||
|
||
while (it != consumer.m_toSync.end()) | ||
{ | ||
KeyOpFieldsValuesTuple t = it->second; | ||
|
||
string key = kfvKey(t); | ||
string op = kfvOp(t); | ||
|
||
/* Ensure the key starts with "switch" otherwise ignore */ | ||
if (key != SWITCH_STR) | ||
{ | ||
SWSS_LOG_INFO("Ignoring SWITCH key %s", key.c_str()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not happen, I would increase the log level to NOTICE or WARNING. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
it = consumer.m_toSync.erase(it); | ||
continue; | ||
} | ||
|
||
SWSS_LOG_DEBUG("key:switch"); | ||
|
||
for (auto i : kfvFieldsValues(t)) | ||
{ | ||
if (fvField(i) == "fdb_aging_time") | ||
{ | ||
int agingTime = 0; | ||
SWSS_LOG_DEBUG("attribute:fdb_aging_time"); | ||
if (op == SET_COMMAND) | ||
{ | ||
SWSS_LOG_DEBUG("operation:set"); | ||
agingTime = atoi(fvValue(i).c_str()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should use more robust way to convert string to int to catch any errors. like strtoul There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
if(agingTime < 0) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if (agingTime < 0) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
{ | ||
SWSS_LOG_ERROR("Invalid fdb_aging_time %s", fvValue(i).c_str()); | ||
break; | ||
} | ||
SWSS_LOG_DEBUG("value:%s",fvValue(i).c_str()); | ||
} | ||
else if (op == DEL_COMMAND) | ||
{ | ||
SWSS_LOG_DEBUG("operation:del"); | ||
agingTime=0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agingTime = 0; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("Unknown operation type %s", op.c_str()); | ||
break; | ||
} | ||
|
||
vector<FieldValueTuple> fvVector; | ||
FieldValueTuple aging_time("fdb_aging_time", to_string(agingTime)); | ||
fvVector.push_back(aging_time); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we have range for aging Time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Range is 0-1000000. It is validated during config. |
||
m_appSwitchTableProducer.set(key, fvVector); | ||
break; | ||
} | ||
} | ||
|
||
it = consumer.m_toSync.erase(it); | ||
} | ||
} | ||
|
||
void VlanMgr::doFdbTask(Consumer &consumer) | ||
{ | ||
if (!isVlanMacOk()) | ||
{ | ||
SWSS_LOG_DEBUG("VLAN mac not ready, delaying FDB task"); | ||
SWSS_LOG_DEBUG("VLAN mac not ready, delaying VLAN task"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
return; | ||
} | ||
|
||
auto it = consumer.m_toSync.begin(); | ||
|
||
while (it != consumer.m_toSync.end()) | ||
{ | ||
KeyOpFieldsValuesTuple t = it->second; | ||
|
||
/* format: <VLAN_name>|<MAC_address> */ | ||
vector<string> keys = tokenize(kfvKey(t), config_db_key_delimiter, 1); | ||
/* keys[0] is vlan as (Vlan10) and keys[1] is mac as (00-00-00-00-00-00) */ | ||
string op = kfvOp(t); | ||
|
||
/* Ensure the key starts with "Vlan" otherwise ignore */ | ||
if (strncmp(keys[0].c_str(), VLAN_PREFIX, 4)) | ||
{ | ||
SWSS_LOG_ERROR("Invalid key format. No 'Vlan' prefix: %s", keys[0].c_str()); | ||
it = consumer.m_toSync.erase(it); | ||
continue; | ||
} | ||
|
||
int vlan_id; | ||
vlan_id = stoi(keys[0].substr(4)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
if ((vlan_id <= 0) || (vlan_id > 4095)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Define 4095 as macro. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
{ | ||
SWSS_LOG_ERROR("Invalid key format. Vlan is out of range: %s", keys[0].c_str()); | ||
it = consumer.m_toSync.erase(it); | ||
continue; | ||
} | ||
|
||
MacAddress mac = MacAddress(keys[1]); | ||
|
||
string key = VLAN_PREFIX + to_string(vlan_id); | ||
key += DEFAULT_KEY_SEPARATOR; | ||
key += mac.to_string(); | ||
|
||
if (op == SET_COMMAND) | ||
{ | ||
string port; | ||
for (auto i : kfvFieldsValues(t)) | ||
{ | ||
if (fvField(i) == "port") | ||
{ | ||
port = fvValue(i); | ||
break; | ||
} | ||
} | ||
|
||
vector<FieldValueTuple> fvVector; | ||
FieldValueTuple p("port", port); | ||
fvVector.push_back(p); | ||
FieldValueTuple t("type", "static"); | ||
fvVector.push_back(t); | ||
|
||
m_appFdbTableProducer.set(key, fvVector); | ||
} | ||
else if (op == DEL_COMMAND) | ||
{ | ||
m_appFdbTableProducer.del(key); | ||
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("Unknown operation type %s", op.c_str()); | ||
} | ||
it = consumer.m_toSync.erase(it); | ||
} | ||
} | ||
|
||
void VlanMgr::doVlanTask(Consumer &consumer) | ||
{ | ||
if (!isVlanMacOk()) | ||
|
@@ -292,15 +450,7 @@ void VlanMgr::doVlanTask(Consumer &consumer) | |
/* set up host env .... */ | ||
for (auto i : kfvFieldsValues(t)) | ||
{ | ||
/* Set vlan admin status */ | ||
if (fvField(i) == "admin_status") | ||
{ | ||
admin_status = fvValue(i); | ||
setHostVlanAdminState(vlan_id, admin_status); | ||
fvVector.push_back(i); | ||
} | ||
/* Set vlan mtu */ | ||
else if (fvField(i) == "mtu") | ||
if (fvField(i) == "mtu") | ||
{ | ||
mtu = fvValue(i); | ||
/* | ||
|
@@ -590,9 +740,45 @@ void VlanMgr::doTask(Consumer &consumer) | |
{ | ||
doVlanMemberTask(consumer); | ||
} | ||
else if (table_name == CFG_FDB_TABLE_NAME) | ||
{ | ||
doFdbTask(consumer); | ||
} | ||
else if (table_name == CFG_SWITCH_TABLE_NAME) | ||
{ | ||
SWSS_LOG_DEBUG("Table:SWITCH"); | ||
doSwitchTask(consumer); | ||
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("Unknown config table %s ", table_name.c_str()); | ||
throw runtime_error("VlanMgr doTask failure."); | ||
} | ||
} | ||
|
||
void VlanMgr::doTask(NotificationConsumer &consumer) | ||
{ | ||
std::string op; | ||
std::string data; | ||
std::vector<swss::FieldValueTuple> values; | ||
|
||
if (&consumer != m_VlanStateNotificationConsumer) | ||
{ | ||
return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Log a message There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
} | ||
|
||
consumer.pop(op, data, values); | ||
|
||
int vlan_id = stoi(data.substr(4)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same comment as before, use more robust way, don't hardcode the "4" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
SWSS_LOG_NOTICE("vlanmgr received port status notification state %s vlan %s id %d", | ||
op.c_str(), data.c_str(), vlan_id); | ||
|
||
if (isVlanStateOk(data)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "{" in the next line to have same style. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change code accordingly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
setHostVlanAdminState(vlan_id, op); | ||
} | ||
else | ||
{ | ||
SWSS_LOG_ERROR("received state update for vlan %s not existing", data.c_str()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change the above comments to reflect this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already explained in the comments above:
"Changed default vlan state in kernel to Down and make it Up only when first port/lag in the vlan is oper up"