[teammgrd]: Add teammgrd to manage LAG related configurations #626

stcheng · 2018-09-20T22:51:36Z

teammgrd will monitor configuration changes in CFG_LAG_TABLE_NAME
and CFG_LAG_MEMBER_TABLE_NAME. It supports creation/removal of
LAGs as well as membership changes of the LAG members.

Signed-off-by: Shu0T1an ChenG shuche@microsoft.com

lguohan · 2018-09-21T05:12:49Z

retest this please

stcheng · 2018-09-21T06:44:02Z

Sadly, the current virtual switch test will not pass due to the lack of teammgrd.
The test will fail because portmgrd is no longer responsible for the port channel MTU changes, and the failure of the test case leads to the failure of clean up previous states and it causes more tests to fail. I have added the pull request: sonic-net/sonic-buildimage#2064 in buildimage to update the submodule as well as update the virtual switch so that it will pass after both commits are merged.

lguohan · 2018-09-21T07:15:03Z

test failure.

lguohan · 2018-09-21T07:58:29Z

orchagent/intfsorch.cpp

@@ -46,7 +46,7 @@ void IntfsOrch::increaseRouterIntfsRefCount(const string &alias)
    SWSS_LOG_ENTER();

    m_syncdIntfses[alias].ref_count++;
-    SWSS_LOG_DEBUG("Router interface %s ref count is increased to %d",
+    SWSS_LOG_INFO("Router interface %s ref count is increased to %d",


why change to INFO?

DEBUG comes with function ENTER and EXIT. It is better to separate this with DEBUG level messages which is too verbose.

this is still DEBUG level message, does not feel it should be info level.

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

https://stackoverflow.com/questions/2031163/when-to-use-the-different-log-levels

In reply to: 219422213 [](ancestors = 219422213,219416641)

lguohan · 2018-09-21T09:00:49Z

cfgmgr/portmgr.cpp

@@ -145,6 +124,7 @@ void PortMgr::doTask(Consumer &consumer)
                            alias.c_str(), status.c_str());
                }
            }
+            m_portList.insert(alias);


m_portList [](start = 12, length = 10)

is this used anywhere?

lguohan · 2018-09-21T09:05:54Z

cfgmgr/teammgrd.cpp

+
+        TableConnector conf_lag_table(&conf_db, CFG_LAG_TABLE_NAME);
+        TableConnector conf_lag_member_table(&conf_db, CFG_LAG_MEMBER_TABLE_NAME);
+        TableConnector stat_port_table(&stat_db, STATE_PORT_TABLE_NAME);


state? stat usually means stats

lguohan · 2018-09-21T09:08:24Z

cfgmgr/teammgr.cpp

+
+        if (op == SET_COMMAND)
+        {
+                int min_links = 0;


lguohan · 2018-09-21T09:11:03Z

cfgmgr/teammgr.cpp

+
+                for (auto key : keys)
+                {
+                    SWSS_LOG_NOTICE("current key: %s", key.c_str());


SWSS_LOG_NOTICE("current key: %s", key.c_str()); [](start = 19, length = 49)

this cannot be notice level

debug level.

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

lguohan · 2018-09-21T09:17:16Z

orchagent/portsorch.cpp

+    vector<FieldValueTuple> fvVector;
+    FieldValueTuple s("state", "ok");
+    fvVector.push_back(s);
+    m_stateLagTable->set(lag_alias, fvVector);


state ok is always linux kernel state ok, in this case, after you have created lag port in the linux kernel, you should say ok.

should be in teamgr.cpp or if you want to use netlink message to decide whether lag has been created or not.

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

lguohan · 2018-09-21T09:19:28Z

orchagent/portsorch.cpp

@@ -2643,6 +2653,8 @@ bool PortsOrch::removeLag(Port lag)

    m_portList.erase(lag.m_alias);

+    m_stateLagTable->del(lag.m_alias);


m_stateLagTable->del(lag.m_alias); [](start = 4, length = 34)

not here

lguohan · 2018-09-21T09:21:20Z

teamsyncd/teamsync.cpp

@@ -70,11 +70,6 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state,
    auto sync = make_shared<TeamPortSync>(lagName, ifindex, &m_lagMemberTable);
    m_select->addSelectable(sync.get());
    m_teamPorts[lagName] = sync;
-
-    fvVector.clear();
-    FieldValueTuple s("state", "ok");


why move to portsorch.cpp?

jipanyang

One general comment:
are the changes warm restart compatible? It looks to me those lag/lag member add operations will interrupt existing lag status.

jipanyang · 2018-09-21T18:35:51Z

cfgmgr/portmgr.cpp

@@ -119,9 +82,25 @@ void PortMgr::doTask(Consumer &consumer)
        string alias = kfvKey(t);
        string op = kfvOp(t);

+        // Skip port which is a member of a port channel
+        vector<string> keys;
+        m_cfgLagMemberTable.getKeys(keys);


Could port config comes before port channel config?

that is fine, once the port channel configuration comes, the configurations of port will be overwritten

teammgrd will monitor configuration changes in CFG_LAG_TABLE_NAME and CFG_LAG_MEMBER_TABLE_NAME. It supports creation/removal of LAGs as well as membership changes of the LAG members. Signed-off-by: Shu0T1an ChenG <shuche@microsoft.com>

lguohan · 2018-10-19T00:58:44Z

cfgmgr/teammgr.cpp

+    SWSS_LOG_INFO("Port channel %s teamd configuration: %s",
+            alias.c_str(), conf.str().c_str());
+
+    cmd << TEAMD_CMD << " -r -t " << alias << " -c " << conf.str() << " -d";


here, you force team device recreation in case if already exist, we probably need to revisit in the warm restart context.

stcheng · 2018-10-19T01:14:57Z

stcheng · 2018-10-19T01:15:48Z

this is tested with the pull request sonic-net/sonic-buildimage#2064

stcheng added the Enhancement ➕ label Sep 20, 2018

stcheng force-pushed the teammgrd branch from 4732c09 to 3cb567d Compare September 21, 2018 07:54

lguohan reviewed Sep 21, 2018

View reviewed changes

cfgmgr/teammgr.cpp Outdated

if (op == SET_COMMAND)

{

int min_links = 0;

Copy link

Contributor

lguohan Sep 21, 2018

Choose a reason for hiding this comment

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

4 spaces

lguohan reviewed Sep 21, 2018

View reviewed changes

jipanyang reviewed Sep 21, 2018

View reviewed changes

stcheng force-pushed the teammgrd branch from 3cb567d to 76a45a9 Compare September 26, 2018 00:48

lguohan reviewed Oct 19, 2018

View reviewed changes

lguohan approved these changes Oct 19, 2018

View reviewed changes

lguohan merged commit 2115a99 into sonic-net:master Oct 19, 2018

stcheng deleted the teammgrd branch October 19, 2018 07:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[teammgrd]: Add teammgrd to manage LAG related configurations #626

[teammgrd]: Add teammgrd to manage LAG related configurations #626

stcheng commented Sep 20, 2018

lguohan commented Sep 21, 2018

stcheng commented Sep 21, 2018

lguohan commented Sep 21, 2018

lguohan Sep 21, 2018

stcheng Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

lguohan Sep 21, 2018

jipanyang left a comment

jipanyang Sep 21, 2018

stcheng Sep 21, 2018

lguohan Oct 19, 2018

stcheng Oct 19, 2018

stcheng commented Oct 19, 2018

stcheng commented Oct 19, 2018

		@@ -2643,6 +2653,8 @@ bool PortsOrch::removeLag(Port lag)

		m_portList.erase(lag.m_alias);

		m_stateLagTable->del(lag.m_alias);

[teammgrd]: Add teammgrd to manage LAG related configurations #626

[teammgrd]: Add teammgrd to manage LAG related configurations #626

Conversation

stcheng commented Sep 20, 2018

lguohan commented Sep 21, 2018

stcheng commented Sep 21, 2018

lguohan commented Sep 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jipanyang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stcheng commented Oct 19, 2018

stcheng commented Oct 19, 2018