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

Port configuration incremental update support #2305

Merged
merged 16 commits into from
Jul 7, 2022
Merged

Port configuration incremental update support #2305

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
0273978
Implement incremental port configuration update
Junchao-Mellanox Mar 8, 2022
623b64a
Improve unit test coverage
Junchao-Mellanox Apr 2, 2022
400ee4b
Merge remote-tracking branch 'origin/master' into incre-port-update
Junchao-Mellanox Apr 11, 2022
24697b2
Merge remote-tracking branch 'origin/master' into incre-port-update
Junchao-Mellanox Apr 18, 2022
c104b77
Fix vs test failure
Junchao-Mellanox Apr 25, 2022
1cca0aa
Merge remote-tracking branch 'origin/master' into incre-port-update
Junchao-Mellanox Apr 27, 2022
97fae06
Do not check configuration value since there could be case that someo…
Junchao-Mellanox Apr 28, 2022
9dce0ca
Merge branch 'master' into incre-port-update
Junchao-Mellanox May 5, 2022
9c5990d
Merge commit '910bfd4' into incre-port-update
Junchao-Mellanox Jun 1, 2022
0b9b2e1
Port incremental configuration support
Junchao-Mellanox Jun 2, 2022
0b18221
Merge remote-tracking branch 'origin/master' into incre-port-update-1
Junchao-Mellanox Jun 2, 2022
bbaadc5
Remove unused code
Junchao-Mellanox Jun 2, 2022
55a18d9
Fix review comment
Junchao-Mellanox Jun 21, 2022
ebeb091
Fix review comment
Junchao-Mellanox Jun 21, 2022
5b99253
Fix review comment
Junchao-Mellanox Jun 28, 2022
25cb579
Merge remote-tracking branch 'origin/master' into incre-port-update-1
Junchao-Mellanox Jul 1, 2022
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
112 changes: 63 additions & 49 deletions cfgmgr/portmgr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,29 +31,9 @@ bool PortMgr::setPortMtu(const string &alias, const string &mtu)

// Set the port MTU in application database to update both
// the port MTU and possibly the port based router interface MTU
vector<FieldValueTuple> fvs;
FieldValueTuple fv("mtu", mtu);
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
}

bool PortMgr::setPortTpid(const string &alias, const string &tpid)
{
stringstream cmd;
string res;

// Set the port TPID in application database to update port TPID
vector<FieldValueTuple> fvs;
FieldValueTuple fv("tpid", tpid);
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
return writeConfigToAppDb(alias, "mtu", mtu);
}


bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
{
stringstream cmd;
Expand All @@ -63,23 +43,7 @@ bool PortMgr::setPortAdminStatus(const string &alias, const bool up)
cmd << IP_CMD << " link set dev " << shellquote(alias) << (up ? " up" : " down");
EXEC_WITH_ERROR_THROW(cmd.str(), res);

vector<FieldValueTuple> fvs;
FieldValueTuple fv("admin_status", (up ? "up" : "down"));
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
}

bool PortMgr::setPortLearnMode(const string &alias, const string &learn_mode)
{
// Set the port MAC learn mode in application database
vector<FieldValueTuple> fvs;
FieldValueTuple fv("learn_mode", learn_mode);
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
return writeConfigToAppDb(alias, "admin_status", (up ? "up" : "down"));
}

bool PortMgr::isPortStateOk(const string &alias)
Expand Down Expand Up @@ -117,14 +81,14 @@ void PortMgr::doTask(Consumer &consumer)

if (op == SET_COMMAND)
{
if (!isPortStateOk(alias))
{
SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str());
it++;
continue;
}
/* portOk=true indicates that the port has been created in kernel.
* We should not call any ip command if portOk=false. However, it is
* valid to put port configuration to APP DB which will trigger port creation in kernel.
*/
bool portOk = isPortStateOk(alias);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think we need to change this? portsyncd does the handlePortConfigFromConfigDB . So portmgrd can still wait and keep the current 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.

Hi @prsunny , handlePortConfigFromConfigDB is only called once at init stage. It cannot handle case like dynamic port breakout. So, I suppose we need keep this change. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok. so the logic here retries for some part of the configuration (kernel), but write to app_db in the first place.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern is, developer has to make sure kernel writes cannot be attempted if portOk is false which is different from the previous logic. Secondly, it keeps re-writing app_db during the wait period. We need to revisit this logic

Copy link
Collaborator

Choose a reason for hiding this comment

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

how about doing this in the if case and keep the original logic.

for (auto &entry : field_values)
            {
                writeConfigToAppDb(alias, fvField(entry), fvValue(entry));

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First let me confirm your solution:

if (!isPortStateOk(alias))
{
    SWSS_LOG_INFO("Port %s is not ready, pending...", alias.c_str());
    for (auto& i : kfvFieldsValues(t))
    {
        writeConfigToAppDb(alias, fvField(i), fvValue(i));
    }
    it++;
    continue;
}

// rest of the code keeps as original one

This solution makes code much clear and simple. But It will repeatedly put the configuration to APP DB until the port state turns "ok".

Copy link
Collaborator

Choose a reason for hiding this comment

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

But its the same with your current implementation as well, as you are looping through all retry attributes and writing to app_db, correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no, for the retry process there are 3 cases:

  1. port turns ok, mtu / admin_status will be configured into kernel, other fields will not be put into APP DB again
  2. port is still not ok and mtu/admin_status were changed by user, it will only put the new mtu / admin_status to APP DB
  3. port is still not ok and mtu / admin_status were not changed, nothing will be put into APP DB

Copy link
Collaborator

Choose a reason for hiding this comment

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

How about this? You can use similar to 'configured' below. Apply it to APP_DB only first time in this else part. I would like to simplify the code. I dont think we need to worry the 2nd case (port is still not ok and mtu/admin_status changed). The new values get anyways applied once the port is ready.


string admin_status, mtu, learn_mode, tpid;
std::vector<FieldValueTuple> field_values;

bool configured = (m_portList.find(alias) != m_portList.end());

Expand Down Expand Up @@ -157,31 +121,57 @@ void PortMgr::doTask(Consumer &consumer)
{
tpid = fvValue(i);
}
else
{
field_values.emplace_back(i);
}
}

if (!mtu.empty())
{
setPortMtu(alias, mtu);
if (portOk)
{
setPortMtu(alias, mtu);
}
else
{
writeConfigToAppDb(alias, "mtu", mtu);
m_retryFields.emplace_back("mtu", mtu);
}
SWSS_LOG_NOTICE("Configure %s MTU to %s", alias.c_str(), mtu.c_str());
}

if (!admin_status.empty())
{
setPortAdminStatus(alias, admin_status == "up");
if (portOk)
{
setPortAdminStatus(alias, admin_status == "up");
}
else
{
writeConfigToAppDb(alias, "admin_status", admin_status);
m_retryFields.emplace_back("admin_status", admin_status);
}
SWSS_LOG_NOTICE("Configure %s admin status to %s", alias.c_str(), admin_status.c_str());
}

if (!learn_mode.empty())
{
setPortLearnMode(alias, learn_mode);
writeConfigToAppDb(alias, "learn_mode", learn_mode);
SWSS_LOG_NOTICE("Configure %s MAC learn mode to %s", alias.c_str(), learn_mode.c_str());
}

if (!tpid.empty())
{
setPortTpid(alias, tpid);
writeConfigToAppDb(alias, "tpid", tpid);
SWSS_LOG_NOTICE("Configure %s TPID to %s", alias.c_str(), tpid.c_str());
}

for (auto &entry : field_values)
{
writeConfigToAppDb(alias, fvField(entry), fvValue(entry));
SWSS_LOG_NOTICE("Configure %s %s to %s", alias.c_str(), fvField(entry).c_str(), fvValue(entry).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.

i'm confused by this. It seems like we are anyways writing all the fields to APP DB, so why individual writes above like mtu, tpid?

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 will fix this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Left a comment above to move this section to top. Also please have this as INFO logs as we don't want to keep logging during retries.

}
}
else if (op == DEL_COMMAND)
{
Expand All @@ -190,6 +180,30 @@ void PortMgr::doTask(Consumer &consumer)
m_portList.erase(alias);
}

it = consumer.m_toSync.erase(it);
if (m_retryFields.empty()) // TODO: test delete & retry
{
it = consumer.m_toSync.erase(it);
}
else
{
/* There are some fields require retry due to set failure. This is usually because
* port has not been created in kernel so that mtu and admin_status configuration
* cannot be synced between kernel and ASIC for now. In this case, we put the retry fields
* back to m_toSync and wait for re-visit later.
*/
it->second = KeyOpFieldsValuesTuple{alias, SET_COMMAND, m_retryFields};
++it;
m_retryFields.clear();
}
}
}

bool PortMgr::writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value)
{
vector<FieldValueTuple> fvs;
FieldValueTuple fv(field, value);
fvs.push_back(fv);
m_appPortTable.set(alias, fvs);

return true;
}
4 changes: 2 additions & 2 deletions cfgmgr/portmgr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ class PortMgr : public Orch
ProducerStateTable m_appPortTable;

std::set<std::string> m_portList;
std::vector<FieldValueTuple> m_retryFields;

void doTask(Consumer &consumer);
bool writeConfigToAppDb(const std::string &alias, const std::string &field, const std::string &value);
bool setPortMtu(const std::string &alias, const std::string &mtu);
bool setPortTpid(const std::string &alias, const std::string &tpid);
bool setPortAdminStatus(const std::string &alias, const bool up);
bool setPortLearnMode(const std::string &alias, const std::string &learn_mode);
bool isPortStateOk(const std::string &alias);
};

Expand Down
10 changes: 8 additions & 2 deletions orchagent/port.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,12 @@ class Port
UNKNOWN
} ;

enum AutoNegMode {
AUTONEG_NOT_SET = -1,
AUTONEG_OFF = 0,
AUTONEG_ON = 1
};

Port() {};
Port(std::string alias, Type type) :
m_alias(alias), m_type(type) {};
Expand All @@ -112,7 +118,7 @@ class Port
uint32_t m_mtu = DEFAULT_MTU;
uint32_t m_speed = 0; // Mbps
std::string m_learn_mode = "hardware";
int m_autoneg = -1; // -1 means not set, 0 = disabled, 1 = enabled
AutoNegMode m_autoneg = Port::AutoNegMode::AUTONEG_NOT_SET;
bool m_admin_state_up = false;
bool m_init = false;
bool m_l3_vni = false;
Expand Down Expand Up @@ -148,7 +154,7 @@ class Port
uint32_t m_up_member_count = 0;
uint32_t m_maximum_headroom = 0;
std::vector<uint32_t> m_adv_speeds;
sai_port_interface_type_t m_interface_type;
sai_port_interface_type_t m_interface_type = SAI_PORT_INTERFACE_TYPE_NONE;
std::vector<uint32_t> m_adv_interface_types;
bool m_mpls = false;

Expand Down
7 changes: 6 additions & 1 deletion orchagent/portsorch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2938,6 +2938,11 @@ void PortsOrch::doPortTask(Consumer &consumer)
}
else
{
if (admin_status.empty())
{
admin_status = p.m_admin_state_up ? "up" : "down";
}

if (!an_str.empty())
{
if (autoneg_mode_map.find(an_str) == autoneg_mode_map.end())
Expand Down Expand Up @@ -2980,7 +2985,7 @@ void PortsOrch::doPortTask(Consumer &consumer)
continue;
}
SWSS_LOG_NOTICE("Set port %s AutoNeg from %d to %d", alias.c_str(), p.m_autoneg, an);
p.m_autoneg = an;
p.m_autoneg = static_cast<swss::Port::AutoNegMode>(an);
m_portList[alias] = p;
}
}
Expand Down
54 changes: 0 additions & 54 deletions portsyncd/portsyncd.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,14 +45,12 @@ void usage()
void handlePortConfigFile(ProducerStateTable &p, string file, bool warm);
void handlePortConfigFromConfigDB(ProducerStateTable &p, DBConnector &cfgDb, bool warm);
void handleVlanIntfFile(string file);
void handlePortConfig(ProducerStateTable &p, map<string, KeyOpFieldsValuesTuple> &port_cfg_map);
void checkPortInitDone(DBConnector *appl_db);

int main(int argc, char **argv)
{
Logger::linkToDbNative("portsyncd");
int opt;
map<string, KeyOpFieldsValuesTuple> port_cfg_map;

while ((opt = getopt(argc, argv, "v:h")) != -1 )
{
Expand All @@ -71,7 +69,6 @@ int main(int argc, char **argv)
DBConnector appl_db("APPL_DB", 0);
DBConnector state_db("STATE_DB", 0);
ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME);
SubscriberStateTable portCfg(&cfgDb, CFG_PORT_TABLE_NAME);

WarmStart::initialize("portsyncd", "swss");
WarmStart::checkWarmStart("portsyncd", "swss");
Expand All @@ -93,7 +90,6 @@ int main(int argc, char **argv)
NetDispatcher::getInstance().registerMessageHandler(RTM_DELLINK, &sync);

s.addSelectable(&netlink);
s.addSelectable(&portCfg);

while (true)
{
Expand Down Expand Up @@ -135,28 +131,6 @@ int main(int argc, char **argv)

g_init = true;
}
if (!port_cfg_map.empty())
{
handlePortConfig(p, port_cfg_map);
}
}
else if (temps == (Selectable *)&portCfg)
{
std::deque<KeyOpFieldsValuesTuple> entries;
portCfg.pops(entries);

for (auto entry: entries)
{
string key = kfvKey(entry);

if (port_cfg_map.find(key) != port_cfg_map.end())
{
/* For now we simply drop previous pending port config */
port_cfg_map.erase(key);
}
port_cfg_map[key] = entry;
}
handlePortConfig(p, port_cfg_map);
}
else
{
Expand Down Expand Up @@ -225,31 +199,3 @@ void handlePortConfigFromConfigDB(ProducerStateTable &p, DBConnector &cfgDb, boo
}

}

void handlePortConfig(ProducerStateTable &p, map<string, KeyOpFieldsValuesTuple> &port_cfg_map)
{
auto it = port_cfg_map.begin();
while (it != port_cfg_map.end())
{
KeyOpFieldsValuesTuple entry = it->second;
string key = kfvKey(entry);
string op = kfvOp(entry);
auto values = kfvFieldsValues(entry);

/* only push down port config when port is not in hostif create pending state */
if (g_portSet.find(key) == g_portSet.end())
{
/* No support for port delete yet */
if (op == SET_COMMAND)
{
p.set(key, values);
}

it = port_cfg_map.erase(it);
}
else
{
it++;
}
}
}
3 changes: 3 additions & 0 deletions tests/mock_tests/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ tests_SOURCES = aclorch_ut.cpp \
mock_table.cpp \
mock_hiredis.cpp \
mock_redisreply.cpp \
mock_shell_command.cpp \
bulker_ut.cpp \
portmgr_ut.cpp \
fake_response_publisher.cpp \
swssnet_ut.cpp \
flowcounterrouteorch_ut.cpp \
Expand Down Expand Up @@ -97,6 +99,7 @@ tests_SOURCES = aclorch_ut.cpp \
$(top_srcdir)/orchagent/bfdorch.cpp \
$(top_srcdir)/orchagent/srv6orch.cpp \
$(top_srcdir)/orchagent/nvgreorch.cpp \
$(top_srcdir)/cfgmgr/portmgr.cpp \
$(top_srcdir)/cfgmgr/buffermgrdyn.cpp

tests_SOURCES += $(FLEX_CTR_DIR)/flex_counter_manager.cpp $(FLEX_CTR_DIR)/flex_counter_stat_manager.cpp $(FLEX_CTR_DIR)/flow_counter_handler.cpp $(FLEX_CTR_DIR)/flowcounterrouteorch.cpp
Expand Down
15 changes: 15 additions & 0 deletions tests/mock_tests/mock_shell_command.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#include <string>
#include <vector>

int mockCmdReturn = 0;
std::string mockCmdStdcout = "";
std::vector<std::string> mockCallArgs;

namespace swss {
int exec(const std::string &cmd, std::string &stdout)
{
mockCallArgs.push_back(cmd);
stdout = mockCmdStdcout;
return mockCmdReturn;
}
}
8 changes: 8 additions & 0 deletions tests/mock_tests/mock_table.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,4 +114,12 @@ namespace swss
iter->second.swap(new_values);
}
}

void ProducerStateTable::del(const std::string &key,
const std::string &op,
const std::string &prefix)
{
auto &table = gDB[m_pipe->getDbId()][getTableName()];
table.erase(key);
}
}
Loading