configDB enforcer for interface by jipanyang · Pull Request #357 · sonic-net/sonic-swss

jipanyang · 2017-10-21T06:03:30Z

Support VLAN interface only for now

Signed-off-by: Jipan Yang [email protected]

What I did
Configuration enforcer for VLAN interface

Why I did it
Support interface 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

Support VLAN interface only for now Signed-off-by: Jipan Yang <[email protected]>

jipanyang · 2017-10-21T06:03:44Z

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

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

@taoyl-ms , @stcheng to review

qiluo-msft · 2017-10-23T19:06:37Z

intfconfd/Makefile.am

+DBGFLAGS = -g
+endif
+
+intfconfd_SOURCES = intfconfd.cpp intfconf.cpp $(top_srcdir)/cfgorch/cfgorch.cpp


cfgorch [](start = 61, length = 7)

suggest merge "cfgorch", "intfconfd" into one directory #Closed

Yes, cfgagent might be a good name for the directory. Will move intfconfd, vlanconfd and debug cli implmentation cfgmgrtest there. Actually that was the initial arrangement.

qiluo-msft · 2017-10-23T19:07:08Z

intfconfd/intfconf.cpp

+#define VLAN_PREFIX         "Vlan"
+#define LAG_PREFIX          "PortChannel"
+
+IntfConf::IntfConf(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, vector<string> tableNames) :


vector [](start = 81, length = 6)

Do you want to copy a vector? #Closed

The table names for different DB are different. a vector might not be a good fit here.

I am not sure you are talking about the same thing. I mean that use a copied instance may cost much than a reference. #Closed

qiluo-msft · 2017-10-23T19:09:24Z

intfconfd/intfconf.cpp

+
+    cmd = "ip address " + opCmd + " ";
+    cmd += ipPrefixStr + " dev " + alias;
+    swss::exec(cmd, res);


exec [](start = 10, length = 4)

check the return value? #Closed

Error has been logged in exec if there is any. In this case, error is not really expected.
That is common question for many functions in sonic, end 2 end feedback loop is missing.

I don't agree. Locally if there is any error happens, you should check the return code all the levels up the stack, and quit as soon as possible, or recover depending on the situation. #Closed

fine. But I don't see the same criteria applied in orchagent, syncd, those python code and etc.
We could start the endeavor from this.

qiluo-msft · 2017-10-23T19:13:23Z

intfconfd/intfconf.cpp

+    CfgOrch::syncCfgDB(CFG_VLAN_INTF_TABLE_NAME, m_cfgVlanIntfTable);
+}
+
+bool IntfConf::setIntfIp(string &alias, string &opCmd, string &ipPrefixStr)


string & [](start = 25, length = 8)

If using a reference, consider using a const reference if possible. #Closed

qiluo-msft · 2017-10-23T19:18:12Z

intfconfd/intfconf.cpp

+    return true;
+}
+
+bool IntfConf::isIntfStateOk(string &alias)


isIntfStateOk [](start = 15, length = 13)

suggest name: readyIntf

Honestly I don't see readyIntf a better name than isIntfStateOk, we do put state ok in stateDB though it is not checked for now. If you insist on changing the name, I could do it.

Just checking the code, there is nothing about "Ok" state. So I guess the original name is not perfect.

Ok, will add code to check the state:

127.0.0.1:6379[6]> hgetall "PORT_TABLE|Ethernet47"

"state"

"ok"

I don't see that you check "ok"

qiluo-msft · 2017-10-23T19:21:13Z

intfconfd/intfconf.cpp

+            continue;
+        }
+
+        IpPrefix ip_prefix(kfvKey(t).substr(kfvKey(t).find(CONFIGDB_KEY_SEPARATOR)+1));


IpPrefix ip_prefix(kfvKey(t).substr(kfvKey(t).find(CONFIGDB_KEY_SEPARATOR)+1)); [](start = 8, length = 79)

Split into multiple statement. It is too complex to read. #Closed

qiluo-msft · 2017-10-23T20:32:22Z

intfconfd/intfconfd.cpp

+    }
+    catch(const std::exception &e)
+    {
+        SWSS_LOG_ERROR("Runtime error: %s", e.what());


return a non zero error code? #Closed

Signed-off-by: Jipan Yang <[email protected]>

lguohan · 2017-10-25T10:23:15Z

.gitignore

 teamsyncd/teamsyncd
 fpmsyncd/fpmsyncd
 intfsyncd/intfsyncd
+cfgagent/intfconfd


can you rename to intfmgr?

The role of confd, orchagent, syncd are blurred, it is hard to have a clear cuts of different roles they are playing, so mgr can include all functions, which manages both asic and linux setup. therefore I prefer mgr name

Ok, will rename them to intfmgr and vlanmgr. For switch table level configuration, besides switchmgr process itself, the configuration may be used by multiple function modules like VLAN.

I plan to keep the class name of VlanConf, IntfConf and SwitchConf, let me know if you have other suggestions.

lguohan · 2017-10-25T10:23:49Z

cfgagent/intfconf.cpp

+#define VLAN_PREFIX         "Vlan"
+#define LAG_PREFIX          "PortChannel"
+
+IntfConf::IntfConf(DBConnector *cfgDb, DBConnector *appDb, DBConnector *stateDb, vector<string> tableNames) :


as requested above, can you rename to IntfMgr

So process and binary name will be changed to VlanMgr/IntfMgr/SwitchMgr, you want to change the internal class name to xxxMgr too? IntfConfd/VlanConfd were chosen per the offline meeting discussion. Yes, class name may be changed.

lguohan · 2017-10-25T10:25:59Z

can you add swssloglevel support for this agent so that we can control the log level of this agent on the fly.

qiluo-msft · 2017-10-25T17:58:55Z

cfgagent/Makefile.am

+endif
+
+
+intfconfd_SOURCES = intfconfd.cpp intfconf.cpp $(top_srcdir)/orchagent/orchbase.cpp


orchbase [](start = 71, length = 8)

Is this file missing in the PR? Why not added it in the same directory?

It is in PR #360

lguohan · 2017-10-25T18:26:06Z

retest this please

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

cfgagent/intfconf.cpp

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


Vlan is ready.

ok, will fix it.

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

cfgagent/intfconf.cpp

+        string op = kfvOp(t);
+        if (op == SET_COMMAND)
+        {
+            /* Don't proceed if port/lag/VLAN is not ready yet */


add more comments here, retry. Later, we need to add subscriber for the state db to get notification when the port/lag/vlan is created.

jipanyang · 2017-10-25T19:50:09Z

@lguohan lguohan swssloglevel has been supported with "Logger::linkToDbNative("intfconfd");"

root@sonic:/etc/sonic# swssloglevel -p
SAI_API_ACL SAI_LOG_LEVEL_NOTICE
........
intfconfd NOTICE
intfsyncd NOTICE
..........
vlanconfd DEBUG

Signed-off-by: Jipan Yang <[email protected]>

lguohan · 2017-11-01T05:01:25Z

cfgagent/intfmgr.cpp

+{
+}
+
+void IntfMgr::syncCfgDB()


where does this function get called?

lguohan · 2017-11-01T21:48:16Z

configure.ac

    teamsyncd/Makefile
    swssconfig/Makefile
    tests/Makefile
+    cfgagent/Makefile


cfgagent [](start = 4, length = 8)

cfgmgr

lguohan

per our discussion, can you change to use orch class for this PR?

Signed-off-by: Jipan Yang <[email protected]>

jipanyang · 2017-11-02T10:59:23Z

Switched to orch class.
Has dependency on #356

qiluo-msft · 2017-11-02T18:00:48Z

cfgmgr/intfmgr.cpp

+    {
+        return true;
+    }
+    return false;


return ret == 0;

lguohan

once vlanmgr is merged.

lguohan · 2017-11-05T07:40:00Z

@JipanYanga , can you resolve the conflict?

Signed-off-by: Jipan Yang <[email protected]>

jipanyang · 2017-11-05T12:14:46Z

Conflicts resolved.

* Fix optimized remove in comparison logic * Add missing brackets * Address comments

* configDB enforcer for interface Support VLAN interface only for now Signed-off-by: Jipan Yang <[email protected]> * Use OrchBase for configDB intf configuration enforcer Signed-off-by: Jipan Yang <[email protected]> * Rename intfconf to intfmgr Signed-off-by: Jipan Yang <[email protected]> * Use shell full path command macros Signed-off-by: Jipan Yang <[email protected]> * Optimize shell command execution error handling Signed-off-by: Jipan Yang <[email protected]> * Use Orch class for intfmgrd orchestration Signed-off-by: Jipan Yang <[email protected]>

configDB enforcer for interface

3ed4cc8

Support VLAN interface only for now Signed-off-by: Jipan Yang <[email protected]>

jipanyang mentioned this pull request Oct 21, 2017

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

Merged

lguohan requested a review from qiluo-msft October 23, 2017 17:44

qiluo-msft reviewed Oct 23, 2017

View reviewed changes

jipanyang mentioned this pull request Oct 24, 2017

configDB enforcer for VLAN #356

Merged

Use OrchBase for configDB intf configuration enforcer

7c7764c

Signed-off-by: Jipan Yang <[email protected]>

lguohan reviewed Oct 25, 2017

View reviewed changes

qiluo-msft reviewed Oct 25, 2017

View reviewed changes

lguohan reviewed Oct 25, 2017

View reviewed changes

jipanyang added 3 commits October 25, 2017 15:31

Rename intfconf to intfmgr

ac1b6ba

Signed-off-by: Jipan Yang <[email protected]>

Use shell full path command macros

416f4f4

Signed-off-by: Jipan Yang <[email protected]>

Optimize shell command execution error handling

16bfac1

Signed-off-by: Jipan Yang <[email protected]>

lguohan reviewed Nov 1, 2017

View reviewed changes

cfgagent/intfmgr.cpp Outdated

{

}

void IntfMgr::syncCfgDB()

Copy link

Contributor

lguohan Nov 1, 2017

Choose a reason for hiding this comment

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

where does this function get called?

lguohan reviewed Nov 1, 2017

View reviewed changes

lguohan requested changes Nov 2, 2017

View reviewed changes

Use Orch class for intfmgrd orchestration

c24f0c0

Signed-off-by: Jipan Yang <[email protected]>

qiluo-msft reviewed Nov 2, 2017

View reviewed changes

lguohan approved these changes Nov 3, 2017

View reviewed changes

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

0b0b686

Signed-off-by: Jipan Yang <[email protected]>

lguohan merged commit 6e84504 into sonic-net:master Nov 5, 2017

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

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

Fix optimized remove in comparison logic (sonic-net#357)

85a579b

* Fix optimized remove in comparison logic * Add missing brackets * Address comments

		endif


		intfconfd_SOURCES = intfconfd.cpp intfconf.cpp $(top_srcdir)/orchagent/orchbase.cpp

Conversation

jipanyang commented Oct 21, 2017

Uh oh!

jipanyang commented Oct 21, 2017

Uh oh!

lguohan commented Oct 23, 2017

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jipanyang Oct 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiluo-msft Oct 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lguohan commented Oct 25, 2017

qiluo-msft Oct 23, 2017 •

edited

Loading

qiluo-msft Oct 23, 2017 •

edited

Loading

qiluo-msft Oct 25, 2017 •

edited

Loading

qiluo-msft Oct 23, 2017 •

edited

Loading

qiluo-msft Oct 26, 2017 •

edited

Loading

qiluo-msft Oct 23, 2017 •

edited

Loading

qiluo-msft Oct 23, 2017 •

edited

Loading

jipanyang Oct 26, 2017 •

edited

Loading

qiluo-msft Oct 23, 2017 •

edited

Loading

qiluo-msft Oct 23, 2017 •

edited

Loading

jipanyang commented Oct 25, 2017 •

edited

Loading