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

Jump to bottom

[AclOrch] aclOrch enhancement to handle LAG port not configured case #494

Merged
merged 13 commits into from
May 25, 2018
Merged

[AclOrch] aclOrch enhancement to handle LAG port not configured case #494

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3bf6269
Merge pull request #1 from Azure/master
keboliu Jan 12, 2018
bf93016
Merge branch 'master' of https://github.com/Azure/sonic-swss
keboliu May 4, 2018
f2d3035
enhance acl orch to handle the LAG port not configured case
keboliu May 4, 2018
032e862
rename variable, function; redunce redundant code; fix memory issue
keboliu May 8, 2018
c6dcd31
revise according to comments
keboliu May 9, 2018
de11d51
one more blank line
keboliu May 9, 2018
d32c1c1
Merge branch 'master' of https://github.com/Azure/sonic-swss
keboliu May 20, 2018
c670c3b
Merge branch 'master' into acl-sequence
keboliu May 20, 2018
3e56f49
add new VS test cases for acl on LAG
keboliu May 22, 2018
de198bd
update with PR comments fix
keboliu May 24, 2018
be203be
rename getValidPortId and move it to portOrch
keboliu May 25, 2018
0499f41
fix indent
keboliu May 25, 2018
879d7fe
fix test case comments
keboliu May 25, 2018
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
195 changes: 160 additions & 35 deletions orchagent/aclorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -936,7 +936,7 @@ bool AclTable::validate()
{
// Control plane ACLs are handled by a separate process
if (type == ACL_TABLE_UNKNOWN || type == ACL_TABLE_CTRLPLANE) return false;
if (ports.empty()) return false;
if (portSet.empty()) return false;
return true;
}

Expand Down Expand Up @@ -1365,8 +1365,8 @@ bool AclRange::remove()
return true;
}

AclOrch::AclOrch(DBConnector *db, vector<string> tableNames, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch) :
Orch(db, tableNames),
AclOrch::AclOrch(vector<TableConnector>& connectors, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch) :
Orch(connectors),
m_mirrorOrch(mirrorOrch),
m_neighOrch(neighOrch),
m_routeOrch(routeOrch)
Expand Down Expand Up @@ -1449,6 +1449,11 @@ void AclOrch::doTask(Consumer &consumer)
unique_lock<mutex> lock(m_countersMutex);
doAclRuleTask(consumer);
}
else if (table_name == STATE_LAG_TABLE_NAME)
{
unique_lock<mutex> lock(m_countersMutex);
doAclTablePortUpdateTask(consumer);
}
else
{
SWSS_LOG_ERROR("Invalid table %s", table_name.c_str());
Expand Down Expand Up @@ -1584,7 +1589,7 @@ void AclOrch::doAclTableTask(Consumer &consumer)
}
else if (attr_name == TABLE_PORTS)
{
bool suc = processPorts(attr_value, [&](sai_object_id_t portOid) {
bool suc = processPorts(newTable, attr_value, [&](sai_object_id_t portOid) {
newTable.link(portOid);
});

Expand Down Expand Up @@ -1729,17 +1734,117 @@ void AclOrch::doAclRuleTask(Consumer &consumer)
}
}

bool AclOrch::processPorts(string portsList, std::function<void (sai_object_id_t)> inserter)
void AclOrch::doAclTablePortUpdateTask(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);
size_t found = key.find('|');
Copy link
Contributor

Choose a reason for hiding this comment

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

('|'); [](start = 31, length = 6)

are we still hardcoding this separator? We need to get rid of such hardcoded symbol.

string port_alias = key.substr(0, found);
string op = kfvOp(t);

SWSS_LOG_INFO("doAclTablePortUpdateTask: OP: %s, port_alias: %s", op.c_str(), port_alias.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

SWSS_LOG_INFO("doAclTablePortUpda [](start = 7, length = 34)

this is debug level message.


if (op == SET_COMMAND)
{
for (auto itmap : m_AclTables)
{
auto table = itmap.second;

if (table.pendingPortSet.find(port_alias) != table.pendingPortSet.end())
{
SWSS_LOG_NOTICE("found the port: %s in ACL table: %s pending port list, bind it to ACL table.", port_alias.c_str(), table.description.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

You may want to put this as INFO instead of NOTICEs?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

revised.


bool suc = processPendingPort(table, port_alias, [&](sai_object_id_t portOid) {
table.link(portOid);
});

if (!suc)
{
SWSS_LOG_ERROR("Failed to bind the ACL table: %s to port: %s", table.description.c_str(), port_alias.c_str());
}
else
{
table.pendingPortSet.erase(port_alias);
SWSS_LOG_NOTICE("port: %s bound to ACL table table: %s, remove it from pending list", port_alias.c_str(), table.description.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it message notice level, this debug level information.

}
}
}
}
else if (op == DEL_COMMAND)
{
for (auto itmap : m_AclTables)
{
auto table = itmap.second;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you remove the extra lines. There are few other redundant lines as well in the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

deleted.

if (table.portSet.find(port_alias) != table.portSet.end())
{
table.pendingPortSet.emplace(port_alias);
SWSS_LOG_WARN("Add deleted port: %s to the pending list of ACL table: %s", port_alias.c_str(), table.description.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this warning level?

}
}
Copy link
Contributor

@lguohan lguohan May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

what is the scenario here? when a port is removed, then to update the acl table? In this case, you need to unbind the port from acl table, and then put the port to pending list. You cannot simply put the port into the pending list.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for this one maybe need to marked as TODO here. if one port or LAG deleted, do will still have the chance to unbind it? I mean maybe some DB entries already destroyed and for SDK level all the configuration should already be cleared?

}
else
{
SWSS_LOG_ERROR("Unknown operation type %s", op.c_str());
}

it = consumer.m_toSync.erase(it);
}
}

sai_object_id_t AclOrch::getValidPortId(string alias, Port port)
{
SWSS_LOG_ENTER();

sai_object_id_t port_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

sai_object_id_t port_id == SAI_NULL_OBJECT_ID; do it here. then you do not need to assign later in the function.


switch (port.m_type)
{
case Port::PHY:
if (port.m_lag_member_id != SAI_NULL_OBJECT_ID)
{
SWSS_LOG_ERROR("Failed to process port. Bind table to LAG member %s is not allowed", alias.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

SWSS_LOG_ERROR [](start = 12, length = 14)

this should be WARNING level, this is invalid configuration, we just ignore it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Failed to process port. [](start = 28, length = 24)

use "Invalid configuration" is more clear.

port_id = SAI_NULL_OBJECT_ID;
Copy link
Contributor

Choose a reason for hiding this comment

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

port_id = SAI_NULL_OBJECT_ID; [](start = 12, length = 29)

remove this.

}
else
{
port_id = port.m_port_id;
}
break;
case Port::LAG:
port_id = port.m_lag_id;
break;
case Port::VLAN:
port_id = port.m_vlan_info.vlan_oid;
break;
default:
SWSS_LOG_ERROR("Failed to process port. Incorrect port %s type %d", alias.c_str(), port.m_type);
port_id = SAI_NULL_OBJECT_ID;
Copy link
Contributor

Choose a reason for hiding this comment

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

port_id = SAI_NULL_OBJECT_ID; [](start = 6, length = 29)

remove this

break;
}

return port_id;
}

bool AclOrch::processPorts(AclTable &aclTable, string portsList, std::function<void (sai_object_id_t)> inserter)
{
SWSS_LOG_ENTER();

sai_object_id_t port_id;
Copy link
Contributor

Choose a reason for hiding this comment

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

since port_id is only used in the for loop, let's move the definition into the for loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

the principle here is to define the variable when you use it.

In reply to: 190211288 [](ancestors = 190211288)


vector<string> strList;

SWSS_LOG_INFO("Processing ACL table port list %s", portsList.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

SWSS_LOG_INFO(" [](start = 4, length = 15)

this is debug level message.


split(portsList, strList, ',');

set<string> strSet(strList.begin(), strList.end());
aclTable.portSet = strSet;

if (strList.size() != strSet.size())
{
Expand All @@ -1758,30 +1863,51 @@ bool AclOrch::processPorts(string portsList, std::function<void (sai_object_id_t
Port port;
if (!gPortsOrch->getPort(alias, port))
{
SWSS_LOG_ERROR("Failed to process port. Port %s doesn't exist", alias.c_str());
return false;
SWSS_LOG_WARN("Port %s not configured yet, add it to ACL table %s pending list", alias.c_str(), aclTable.description.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

SWSS_LOG_WARN [](start = 12, length = 13)

why warning level? this should be info level since this is a common situation your code is handling, so this should not be warning level.

aclTable.pendingPortSet.emplace(alias);
continue;
}

switch (port.m_type)
port_id = getValidPortId(alias, port);
if (port_id != SAI_NULL_OBJECT_ID)
{
case Port::PHY:
if (port.m_lag_member_id != SAI_NULL_OBJECT_ID)
{
SWSS_LOG_ERROR("Failed to process port. Bind table to LAG member %s is not allowed", alias.c_str());
return false;
}
inserter(port.m_port_id);
break;
case Port::LAG:
inserter(port.m_lag_id);
break;
case Port::VLAN:
inserter(port.m_vlan_info.vlan_oid);
break;
default:
SWSS_LOG_ERROR("Failed to process port. Incorrect port %s type %d", alias.c_str(), port.m_type);
return false;
}
inserter(port_id);
}
else
{
return false;
}
}

return true;
}

bool AclOrch::processPendingPort(AclTable &aclTable, string portAlias, std::function<void (sai_object_id_t)> inserter)
{
SWSS_LOG_ENTER();

SWSS_LOG_INFO("Processing ACL table port %s", portAlias.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

SWSS_LOG_INFO [](start = 4, length = 13)

debug level.


sai_object_id_t port_id;

Port port;
if (!gPortsOrch->getPort(portAlias, port))
{
SWSS_LOG_WARN("Port %s not configured yet, add it to ACL table %s pending list", portAlias.c_str(), aclTable.description.c_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

SWSS_LOG_WARN [](start = 8, length = 13)

info level.

aclTable.pendingPortSet.insert(portAlias);
return true;
}

port_id = getValidPortId(portAlias, port);

if (port_id != SAI_NULL_OBJECT_ID)
{
inserter(port_id);
aclTable.bind(port_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

aclTable.bind(port_id); [](start = 8, length = 23)

move this out of processPendingPort() so that you can reuse this function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have clarified with Qi before, my view is that these two functions do share some same code, but since processPorts is for "normal" flow and for processPendingPort is dedicated to receiving the notification and handle the pending port, to keep the code more straightforward, I feel like to have these two functions separated though will have some slight redundant code.

}
else
{
return false;
}

return true;
Expand Down Expand Up @@ -1898,18 +2024,17 @@ sai_status_t AclOrch::bindAclTable(sai_object_id_t table_oid, AclTable &aclTable
sai_status_t status = SAI_STATUS_SUCCESS;

SWSS_LOG_INFO("%s table %s to ports", bind ? "Bind" : "Unbind", aclTable.id.c_str());

if (aclTable.portSet.empty())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this condition will be ever true in this flow. Can you check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you are correct, I removed this flow.

{
SWSS_LOG_ERROR("Port list is not configured for %s table", aclTable.id.c_str());
return SAI_STATUS_FAILURE;
}

if (aclTable.ports.empty())
{
if (bind)
{
SWSS_LOG_ERROR("Port list is not configured for %s table", aclTable.id.c_str());
return SAI_STATUS_FAILURE;
}
else
{
return SAI_STATUS_SUCCESS;
}
SWSS_LOG_WARN("Binding port list is empty for %s table", aclTable.id.c_str());
Copy link
Collaborator

Choose a reason for hiding this comment

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

It could be a normal flow to delete aclTable which is not yet bind to any port list. We wouldn't want to log warning if it is in the delete flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

condition check added.

return SAI_STATUS_SUCCESS;
}

if (bind)
Expand Down
11 changes: 9 additions & 2 deletions orchagent/aclorch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,10 @@ class AclTable {
std::map<sai_object_id_t, sai_object_id_t> ports;
// Map rule name to rule data
map<string, shared_ptr<AclRule>> rules;
// Set to store the ACL table port alias
set<string> portSet;
// Set to store the not cofigured ACL table port alias
set<string> pendingPortSet;

AclTable()
: type(ACL_TABLE_UNKNOWN)
Expand Down Expand Up @@ -294,7 +298,7 @@ inline void split(string str, Iterable& out, char delim = ' ')
class AclOrch : public Orch, public Observer
{
public:
AclOrch(DBConnector *db, vector<string> tableNames, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch);
AclOrch(vector<TableConnector>& connectors, PortsOrch *portOrch, MirrorOrch *mirrorOrch, NeighOrch *neighOrch, RouteOrch *routeOrch);
~AclOrch();
void update(SubjectType, void *);

Expand All @@ -319,6 +323,7 @@ class AclOrch : public Orch, public Observer
void doTask(Consumer &consumer);
void doAclTableTask(Consumer &consumer);
void doAclRuleTask(Consumer &consumer);
void doAclTablePortUpdateTask(Consumer &consumer);
void doTask(SelectableTimer &timer);

static void collectCountersThread(AclOrch *pAclOrch);
Expand All @@ -329,8 +334,10 @@ class AclOrch : public Orch, public Observer

bool processAclTableType(string type, acl_table_type_t &table_type);
bool processAclTableStage(string stage, acl_stage_type_t &acl_stage);
bool processPorts(string portsList, std::function<void (sai_object_id_t)> inserter);
bool processPorts(AclTable &aclTable, string portsList, std::function<void (sai_object_id_t)> inserter);
bool processPendingPort(AclTable &aclTable, string portAlias, std::function<void (sai_object_id_t)> inserter);
bool validateAclTable(AclTable &aclTable);
sai_object_id_t getValidPortId(string alias, Port port);
Copy link
Contributor

Choose a reason for hiding this comment

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

getValidPortId [](start = 20, length = 14)

to reflect the true meaning, it is better to rename it to getBindPortId. Besides, it makes more sense to move this into portsyncd since it is retrieving a port property.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

renamed and move it to portOrch.


//vector <AclTable> m_AclTables;
map <sai_object_id_t, AclTable> m_AclTables;
Expand Down
11 changes: 6 additions & 5 deletions orchagent/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,11 +247,12 @@ int main(int argc, char **argv)
SWSS_LOG_NOTICE("Created underlay router interface ID %lx", gUnderlayIfId);

/* Initialize orchestration components */
DBConnector *appl_db = new DBConnector(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
DBConnector *config_db = new DBConnector(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
DBConnector appl_db(APPL_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
DBConnector config_db(CONFIG_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);
DBConnector state_db(STATE_DB, DBConnector::DEFAULT_UNIXSOCKET, 0);

OrchDaemon *orchDaemon = new OrchDaemon(appl_db, config_db);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we would need to have orchDaemon allocate from stack. This could still be retained as it is (allocate via new) so we wouldn't have to worry about the class holding more data in future just in case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

revised.

if (!orchDaemon->init())
OrchDaemon orchDaemon(&appl_db, &config_db, &state_db);
if (!orchDaemon.init())
{
SWSS_LOG_ERROR("Failed to initialize orchstration daemon");
exit(EXIT_FAILURE);
Expand All @@ -271,7 +272,7 @@ int main(int argc, char **argv)
exit(EXIT_FAILURE);
}

orchDaemon->start();
orchDaemon.start();
}
catch (char const *e)
{
Expand Down
2 changes: 1 addition & 1 deletion orchagent/orch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ bool Orch::parseIndexRange(const string &input, sai_uint32_t &range_low, sai_uin

void Orch::addConsumer(DBConnector *db, string tableName, int pri)
{
if (db->getDbId() == CONFIG_DB)
if (db->getDbId() == CONFIG_DB || db->getDbId() == STATE_DB)
{
addExecutor(tableName, new Consumer(new SubscriberStateTable(db, tableName, TableConsumable::DEFAULT_POP_BATCH_SIZE, pri), this));
}
Expand Down
22 changes: 13 additions & 9 deletions orchagent/orchdaemon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,10 @@ RouteOrch *gRouteOrch;
AclOrch *gAclOrch;
CrmOrch *gCrmOrch;

OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb) :
OrchDaemon::OrchDaemon(DBConnector *applDb, DBConnector *configDb, DBConnector *stateDb) :
m_applDb(applDb),
m_configDb(configDb)
m_configDb(configDb),
m_stateDb(stateDb)
{
SWSS_LOG_ENTER();
}
Expand All @@ -39,9 +40,6 @@ OrchDaemon::~OrchDaemon()
SWSS_LOG_ENTER();
for (Orch *o : m_orchList)
delete(o);

delete(m_configDb);
delete(m_applDb);
}

bool OrchDaemon::init()
Expand Down Expand Up @@ -99,11 +97,17 @@ bool OrchDaemon::init()
MirrorOrch *mirror_orch = new MirrorOrch(appDbMirrorSession, confDbMirrorSession, gPortsOrch, gRouteOrch, gNeighOrch, gFdbOrch);
VRFOrch *vrf_orch = new VRFOrch(m_configDb, CFG_VRF_TABLE_NAME);

vector<string> acl_tables = {
CFG_ACL_TABLE_NAME,
CFG_ACL_RULE_TABLE_NAME
TableConnector confDbAclTable(m_configDb, CFG_ACL_TABLE_NAME);
TableConnector confDbAclRuleTable(m_configDb, CFG_ACL_RULE_TABLE_NAME);
TableConnector stateDbLagTable(m_stateDb, STATE_LAG_TABLE_NAME);

vector<TableConnector> acl_table_connectors = {
confDbAclTable,
confDbAclRuleTable,
stateDbLagTable
};
gAclOrch = new AclOrch(m_configDb, acl_tables, gPortsOrch, mirror_orch, gNeighOrch, gRouteOrch);

gAclOrch = new AclOrch(acl_table_connectors, gPortsOrch, mirror_orch, gNeighOrch, gRouteOrch);

m_orchList = { switch_orch, gCrmOrch, gPortsOrch, intfs_orch, gNeighOrch, gRouteOrch, copp_orch, tunnel_decap_orch, qos_orch, buffer_orch, mirror_orch, gAclOrch, gFdbOrch, vrf_orch };
m_select = new Select();
Expand Down
3 changes: 2 additions & 1 deletion orchagent/orchdaemon.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ using namespace swss;
class OrchDaemon
{
public:
OrchDaemon(DBConnector *, DBConnector *);
OrchDaemon(DBConnector *, DBConnector *, DBConnector *);
~OrchDaemon();

bool init();
void start();
private:
DBConnector *m_applDb;
DBConnector *m_configDb;
DBConnector *m_stateDb;

std::vector<Orch *> m_orchList;
Select *m_select;
Expand Down