configDB enforcer for VLAN #356

jipanyang · 2017-10-21T05:30:58Z

Signed-off-by: Jipan Yang jipan.yang@alibaba-inc.com

What I did
Configuration enforcer for VLAN

Why I did it
Support vlan configuration via configDB for both initialization and incremental update
How I verified it
VLAN trunk test plan: https://github.com/Azure/SONiC/wiki/VLAN-trunk-test-plan
Details if related

jipanyang · 2017-10-21T05:34:08Z

Has dependency on:
sonic-net/sonic-swss-common#146
#355
#336
#344

lguohan · 2017-10-23T17:44:35Z

@taoyl-ms , @stcheng to review

lguohan · 2017-10-23T17:45:15Z

Makefile.am

@@ -1,4 +1,4 @@
-SUBDIRS = fpmsyncd neighsyncd intfsyncd portsyncd orchagent swssconfig
+SUBDIRS = fpmsyncd neighsyncd intfsyncd portsyncd orchagent swssconfig vlanconfd


i prefer vlanmgr than vlanconfd

There is already bgpconfd. We may change to vlanmgr from vlanconfd, then it introduces name inconsistency.

As suggested by @qiluo-msft in #357, probably we may merge the files of vlan configuration enforcer and intf configuration enforcer into one flat folder while keeping them in separate processes, other future configuration enforcer (in c++) could follow the same practice.

I don't have strong opinion about the naming of configuration enforcer folder and configuration enforcers themselves, but to avoid multiple iterations of name change and folder rearrangement, could @lguohan @taoyl-ms @qiluo-msft @stcheng agree on the following names:

folder name: cfgagent
vlan configuration enforcer: vlanconfd
intf configuration enforcer: intfconfd

Or please help give names (and folder structure) you all agree on, thanks!

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan · 2017-10-25T18:45:39Z

can you seperate the vlanmgr and switchmgr into two PR?

lguohan · 2017-10-25T18:48:43Z

cfgagent/vlanconf.cpp

+    cmd += DOT1Q_BRIDGE_NAME;
+    cmd += " name ";
+    cmd += VLAN_PREFIX + to_string(vlan_id)
+            + " type vlan id " + to_string(vlan_id);


can we contruct the command in one line, easier to ready.

I had problem compiling it if put them in one line. Will check again to see how to around the problem.

I think "stringstream" would be a better option here and it takes different types of input so you wudn't have to convert integers to_string

Using stringstream looks like a good idea.

lguohan · 2017-10-25T18:49:31Z

cfgagent/vlanconf.cpp

+    gSwtichConfVlan->updateHostFloodControl(port_alias);
+
+    // Bring up vlan member port and set MTU to 9100 by default
+    cmd = "ip link set " + port_alias + " up mtu 9100";


9100 hard coded? do we have that value in config db?

It is hard coded in interfaces.j2, not in config DB. Probably port schema in configDB should be updated to have MTU field.

127.0.0.1:6379[4]> hgetall "PORT|Ethernet20"

"alias"

"tenGigE20"
127.0.0.1:6379[4]> hgetall "VLAN_MEMBER|Vlan1000|Ethernet20"

"tagging_mode"

"untagged"

jipanyang · 2017-10-25T19:26:58Z

@lguohan Ok, will take switch related configuration to a separate PR

lguohan · 2017-10-25T21:26:08Z

cfgagent/vlanconf.h

+    VlanConf(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, vector<string> tableNames);
+    void syncCfgDB();
+private:
+	ProducerStateTable m_appVlanTableProducer, m_appVlanMemberTableProducer;


P [](start = 0, length = 2)

can you change all tab to spaces?

lguohan · 2017-10-25T21:27:27Z

cfgagent/vlanconfd.cpp

+
+    string mac_str;
+    swss::exec("ip link show eth0 | grep ether | awk '{print $2}'", mac_str);
+    gMacAddress = mac_str;


can we make this as an parameter instead of hard code the vlan mac address into the code logic, see -m option in orchagent?

SWITCH_ATTR in SWITCH table has switch_mac field, to implement mac fetching from configDB SWITCH. mac could be be taken from there if configured, otherwise eth0 mac is used by default.
mac address here is to set on VLAN.

lguohan · 2017-10-25T21:28:07Z

cfgagent/vlanconf.cpp

+    {
+        if (m_stateLagTable.get(alias, temp))
+        {
+            SWSS_LOG_DEBUG("%s is ready\n", alias.c_str());


\n [](start = 39, length = 2)

remove this '\n', does dot look like we need it.

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan · 2017-10-26T00:04:53Z

cfgagent/vlanconf.cpp

+    swss::exec(cmd, res);
+
+    // When port is not member of any VLAN, it shall be detached from Dot1Q bridge!
+    cmd = "bridge vlan show dev " + port_alias + " | grep None";


we need to use the full path for all the commands, otherwise those commands could be override by users.

right. I'm thinking of having a common header file with all the commands macro defined, full path is specified in the macro definition.

lguohan · 2017-10-26T00:29:35Z

cfgagent/vlanmgr.cpp

+            for (auto i : kfvFieldsValues(t))
+            {
+                if (fvField(i) == "tagging_mode")
+                    tagging_mode = fvValue(i);


always have {}

lguohan · 2017-10-26T00:29:53Z

cfgagent/vlanmgr.cpp

+    if (table_name == CFG_VLAN_TABLE_NAME)
+        doVlanTask(consumer);
+    else if (table_name == CFG_VLAN_MEMBER_TABLE_NAME)
+        doVlanMemberTask(consumer);


always have {}

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan · 2017-10-27T04:45:42Z

cfgagent/vlanmgr.cpp

+            SWSS_LOG_DEBUG("%s", (dumpTuple(consumer, consumer.m_toSync[member_key])).c_str());
+        }
+        /*
+         * There is pending task from consumber pipe, in this case just skip it.


Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan · 2017-10-31T21:02:34Z

cfgagent/vlanmgr.cpp

+    EXEC_WITH_ERROR_THROW(cmd.str(), res);
+}
+
+void VlanMgr::syncCfgDB()


syncCfgDB [](start = 14, length = 9)

where is function gets called?

jipanyang · 2017-11-02T10:58:19Z

has dependency on sonic-net/sonic-swss-common#156

lguohan · 2017-11-02T18:25:00Z

retest this please

lguohan · 2017-11-02T18:43:32Z

retest this please

lguohan · 2017-11-02T18:52:03Z

sonic-net/sonic-swss-common#156 is merged, but build still failed.

jipanyang · 2017-11-02T22:56:54Z

Sorry, too many iterations of name change and folder restructuring, I missed OrchBase to Orch name change in few places for last commit. Please check it again.

lguohan · 2017-11-02T22:59:43Z

const string VLAN_PREFIX = "Vlan";

this can be removed. minor.

Refers to: portsyncd/linksync.cpp:26 in f67e288. [](commit_id = f67e288, deletion_comment = False)

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang · 2017-11-02T23:09:24Z

Removed const string VLAN_PREFIX = "Vlan"; from portsyncd/linksync.cpp
Also the unused g_vlanMap variable form linksync.cpp and portsyncd.cpp

lguohan · 2017-11-02T23:46:42Z

cfgmgr/vlanmgrd.cpp

+
+    string mac_str;
+    stringstream cmd;
+    cmd << IP_CMD << " link show eth0 | " << GREP_CMD << " ether | " << AWK_CMD << " '{print $2}'";


we need to use the -m option to get the mac address since it is complicated.

You can check here. https://github.com/Azure/sonic-buildimage/blob/37dc7bd4786c793796530e53da304b5e68b7ed16/files/image_config/interfaces/interfaces-config.sh#L7

For some asic, the switch mac must match certain pattern.
Here for the VLAN mac, you will want it to be the same as the switch mac. Therefore, we need to follow the same procedure.

Ok, I'm think of using the switch table in configDB which has switch mac field

127.0.0.1:6379[4]> hgetall "SWITCH|SWITCH_ATTR"

"switch_mac"

"00:05:64:30:73:c0"

In interfaces-config.sh, we set the switch_mac to configDB if it is not there already, then use the configDB data in vlanMgr. This way it gives people the option to specify switch mac other than that of eth0.

sonic-net/sonic-buildimage#1108

that is fine, but use eth0 mac directly is not an option. need to remove that part from your current code.

lguohan · 2017-11-02T23:53:55Z

cfgmgr/vlanmgr.cpp

+                    /*
+                     * TODO: support host VLAN mtu setting.
+                     * Host VLAN mtu should be set only after member configured
+                     * and VLAN state is not UNKOWN.


UNKOWN-> UNKNOWN

lguohan · 2017-11-03T00:30:14Z

cfgmgr/vlanmgr.cpp

+                    SWSS_LOG_DEBUG("%s mtu %u: Host VLAN mtu setting to be supported.", key.c_str(), mtu);
+                    fvVector.push_back(i);
+                }
+                else if (fvField(i) == "members@") {


in the new configdb schema, we have VLAN_MEMBER table, we should remove the members@ attribute in VLAN table, otherwise, there cloud potentially be two sources of truth.

We do not need to do this change now. But, please do it in later PR.

Ok, will remove it once minigraph.py update for the member processing is committed.

https://github.com/Azure/sonic-buildimage/pull/1063/files#diff-18b32e4dc045fa3d1c5941a732124c89

lguohan · 2017-11-03T00:31:49Z

cfgmgr/vlanmgr.cpp

+                it++;
+                continue;
+            }
+            string tagging_mode = "untagged";


can you also update the VLAN_MEMBER scheme in https://github.com/Azure/SONiC/wiki/Configuration ?

lguohan

-m need to be added to specify the mac address of the bridge.

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

zhenggen-xu · 2017-11-05T06:30:05Z

cfgmgr/vlanmgr.cpp

+                FieldValueTuple a("admin_status",  "up");
+                fvVector.push_back(a);
+            }
+            m_appVlanTableProducer.set(key, fvVector);


Is there any plan to deal with the operational state of Vlan interface if not yet done? We may need that to disable routing if vlan interface is down.
If we are doing that, I assume some other entity (like netlink message in linksync.cpp) will update the app-db? In that case, do we have a timing issue here? (e,g, netlink messages came before we put the table to app-db)?

We can address this in future PR if needed.

we need to define the operational state of vlan first. if it mean all members are down, then we have way to know the member oper status already, we do not need to rely on the netlink to get the vlan op status, then. This is going to be another feature request and thus a separate PR.

What I did Admin-disable port before applying media-based NPU serdes attributes from media_settings.json. Why I did it This fix is needed along with xcvrd changes #377. Maintain deterministic behavior of interface bring-up, by toggling host_tx_ready flag, which will trigger CMIS reinit for the module once NPU serdes params have been applied. How I verified it Validated media_settings being notified and applied on Cisco 8111 with subject changes combined with diffs from #360, #377 and #15453. Will update final results once #377 is frozen. Details if related Proposal for xcvrd changes: #356 Signed-off-by: Aman Singhal <amans@cisco.com>

…c-net#2831) What I did Admin-disable port before applying media-based NPU serdes attributes from media_settings.json. Why I did it This fix is needed along with xcvrd changes sonic-net#377. Maintain deterministic behavior of interface bring-up, by toggling host_tx_ready flag, which will trigger CMIS reinit for the module once NPU serdes params have been applied. How I verified it Validated media_settings being notified and applied on Cisco 8111 with subject changes combined with diffs from sonic-net#360, sonic-net#377 and #15453. Will update final results once sonic-net#377 is frozen. Details if related Proposal for xcvrd changes: sonic-net#356 Signed-off-by: Aman Singhal <amans@cisco.com>

jipanyang force-pushed the vlanconfd branch from c8a1fd7 to 362f61d Compare October 21, 2017 05:54

jipanyang mentioned this pull request Oct 21, 2017

Add support for vlanconfd and intfconfd sonic-net/sonic-buildimage#1063

Merged

lguohan reviewed Oct 23, 2017

View reviewed changes

jipanyang added 2 commits October 24, 2017 22:32

configDB enforcer for VLAN

9d68329

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Use OrchBase for configDB vlan configuration enforcer

35dc6a4

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang force-pushed the vlanconfd branch from 362f61d to 35dc6a4 Compare October 25, 2017 05:38

lguohan reviewed Oct 25, 2017

View reviewed changes

Rename vlanconf to vlanmgr, and spin off switchmgr

ccba8ee

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan reviewed Oct 26, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into vlanconfd

bb2ec3f

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang force-pushed the vlanconfd branch from 20d33bb to bb2ec3f Compare October 26, 2017 01:47

lguohan reviewed Oct 27, 2017

View reviewed changes

jipanyang added 2 commits October 29, 2017 18:12

Use shell full path command macros

686b86f

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

Optimize shell command execution error handling

0f2e587

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

lguohan reviewed Oct 31, 2017

View reviewed changes

jipanyang force-pushed the vlanconfd branch 2 times, most recently from 28d5e91 to d8b6fb8 Compare November 2, 2017 10:47

jipanyang mentioned this pull request Nov 2, 2017

configDB enforcer for interface #357

Merged

jipanyang force-pushed the vlanconfd branch from d8b6fb8 to f67e288 Compare November 2, 2017 22:53

Use Orch class for vlanmgrd orchestration

5b27bfa

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang force-pushed the vlanconfd branch from f67e288 to 5b27bfa Compare November 2, 2017 23:07

lguohan reviewed Nov 2, 2017

View reviewed changes

lguohan reviewed Nov 3, 2017

View reviewed changes

lguohan requested changes Nov 3, 2017

View reviewed changes

Support getting switch mac from configDB

827e97a

Signed-off-by: Jipan Yang <jipan.yang@alibaba-inc.com>

jipanyang force-pushed the vlanconfd branch from 0f0a308 to 827e97a Compare November 4, 2017 20:22

zhenggen-xu reviewed Nov 5, 2017

View reviewed changes

lguohan approved these changes Nov 5, 2017

View reviewed changes

lguohan merged commit 021ef4f into sonic-net:master Nov 5, 2017

jipanyang deleted the vlanconfd branch June 5, 2018 00:29

oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-swss that referenced this pull request Mar 1, 2023

Submodule update to SAI v1.3.4 (sonic-net#356)

9756e98

		@@ -1,4 +1,4 @@
		SUBDIRS = fpmsyncd neighsyncd intfsyncd portsyncd orchagent swssconfig
		SUBDIRS = fpmsyncd neighsyncd intfsyncd portsyncd orchagent swssconfig vlanconfd

configDB enforcer for VLAN #356

configDB enforcer for VLAN #356

Conversation

jipanyang commented Oct 21, 2017

jipanyang commented Oct 21, 2017

lguohan commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan commented Oct 25, 2017

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 commented Oct 25, 2017

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

Choose a reason for hiding this comment

jipanyang commented Nov 2, 2017

lguohan commented Nov 2, 2017

lguohan commented Nov 2, 2017

lguohan commented Nov 2, 2017

jipanyang commented Nov 2, 2017

lguohan commented Nov 2, 2017

jipanyang commented Nov 2, 2017

Choose a reason for hiding this comment

jipanyang Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

lguohan Nov 3, 2017 • edited Loading

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

lguohan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jipanyang Nov 3, 2017 •

edited

Loading

lguohan Nov 3, 2017 •

edited

Loading