Mux/IPTunnel orchagent changes #1497

prsunny · 2020-11-06T00:34:01Z

What I did

Introduce TunnelMgr Daemon
Introduce Mux orchagent
Added support to enable/disable neighbors via NeighOrch
Added support to create/remove nexthop tunnels via decapOrch
Added support for ACL handling

Why I did it
Support Mux state transitions in orchagent

How I verified it
Manual test/verification in-progress

Details if related

prsunny · 2020-11-30T17:05:27Z

retest this please

gechiang · 2020-12-07T20:44:18Z

cfgmgr/tunnelmgr.cpp

+        }
+        else if (op == DEL_COMMAND)
+        {
+            /* TODO: Handle Tunnel delete for other tunnel types */


Can we add a check for value == "IPINIP" before calling doIpInIpTunnelTask(t)? This way it is in parity with how the "SET_COMMAND" is handled that do check for the tunnel type.

In the del operation we will not get the value

tahmed-dev · 2020-12-01T04:16:50Z

cfgmgr/Makefile.am

+tunnelmgrd_SOURCES = tunnelmgrd.cpp tunnelmgr.cpp $(top_srcdir)/orchagent/orch.cpp $(top_srcdir)/orchagent/request_parser.cpp shellcmd.h
+tunnelmgrd_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
+tunnelmgrd_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_SAI)
+tunnelmgrd_LDADD = -lswsscommon


nit: missing EOL

tahmed-dev · 2020-12-01T04:26:34Z

cfgmgr/tunnelmgrd.cpp

+using namespace swss;
+
+/* select() function timeout retry time, in millisecond */
+#define SELECT_TIMEOUT 1000


What about the time unit to the name SELECT_TIMEOUT_MSEC

This is the default value for all mgrs, keeping it for consistency

tahmed-dev · 2020-12-01T23:00:46Z

cfgmgr/tunnelmgrd.cpp

+
+        TunnelMgr tunnelmgr(&cfgDb, &appDb, cfg_tunnel_tables);
+
+        std::vector<Orch *> cfgOrchList = {&tunnelmgr};


curious cfg_tunnel_tables is a list of one and cfgOrchList is a list of one. Why not use variable?

nit: style mismatch camelCase vs hyphened_variables.

tahmed-dev · 2020-12-01T23:13:21Z

orchagent/muxorch.cpp

+        return false;
+    }
+
+    if (!aclHandler(port.m_port_id))


Why are we not removing the routes here as per other failure cases above?

gechiang · 2020-12-07T23:19:37Z

cfgmgr/tunnelmgrd.cpp

+            ret = s.select(&sel, SELECT_TIMEOUT);
+            if (ret == Select::ERROR)
+            {
+                SWSS_LOG_NOTICE("Error: %s!", strerror(errno));


Any reason not to use SWSS_LOG_ERROR instead of NOTICE here for the error handling?

Same for all managers, keeping for consistency

gechiang · 2020-12-07T23:31:53Z

orchagent/muxorch.cpp

+    sai_status_t status = sai_route_api->create_route_entry(&route_entry, (uint32_t)attrs.size(), attrs.data());
+    if (status != SAI_STATUS_SUCCESS)
+    {
+        SWSS_LOG_ERROR("Failed to create tunnel route %s, rv:%d",


To ease debug you may want to consider also capture nh oid in the syslog when create route failed.

tahmed-dev · 2020-12-08T17:13:33Z

orchagent/muxorch.h

+const request_description_t mux_cfg_request_description = {
+            { REQ_T_STRING },
+            {
+                { "server_ipv4", REQ_T_IP_PREFIX },


missing state field.

tahmed-dev · 2020-12-08T17:29:18Z

orchagent/muxorch.h

+const request_description_t mux_cable_request_description = {
+            { REQ_T_STRING },
+            {
+                { "state",  REQ_T_STRING },


missing read_side and active_side fields

Done for the State entry

tahmed-dev · 2020-12-08T17:30:30Z

orchagent/muxorch.cpp

+extern sai_router_interface_api_t* sai_router_intfs_api;
+
+/* Constants */
+#define MUX_TUNNEL "MUX_TUNNEL0"


Tunnel name: MuxTunnel0

daall

ACL changes LGTM

tahmed-dev · 2020-12-22T00:28:31Z

cfgmgr/tunnelmgr.cpp

+    if (op == SET_COMMAND)
+    {
+        m_appIpInIpTunnelTable.set(TunnelName, kfvFieldsValues(t));
+


nit: extra line

tahmed-dev · 2020-12-22T00:30:52Z

cfgmgr/tunnelmgr.h

+class TunnelMgr : public Orch
+{
+public:
+    TunnelMgr(DBConnector *cfgDb, DBConnector *appDb, std::string tableName);


What about explicitly deleting the default and copy constructor. Also, calling destructor as default

tahmed-dev · 2020-12-22T00:39:53Z

orchagent/muxorch.cpp

+    mux_cb_orch_ = gDirectory.get<MuxCableOrch*>();
+    mux_state_orch_ = gDirectory.get<MuxStateOrch*>();
+
+    nbr_handler_ = unique_ptr<MuxNbrHandler> (new MuxNbrHandler());


std::make_unique<>() ?

tahmed-dev · 2020-12-22T00:49:34Z

orchagent/muxorch.cpp

+            return false;
+        }
+
+        mux_cable_tb_[port_name] = std::unique_ptr<MuxCable>


make_unique<MuxCable>(...)

tahmed-dev · 2020-12-22T00:52:27Z

orchagent/muxorch.cpp

+}
+
+MuxOrch::MuxOrch(DBConnector *db, const std::vector<std::string> &tables, TunnelDecapOrch* decapOrch, NeighOrch* neighOrch)
+          : Orch2(db, tables, request_), decap_orch_(decapOrch), neigh_orch_(neighOrch)


initializer list element on separate lines to match other files.

tahmed-dev · 2020-12-22T01:28:45Z

orchagent/muxorch.h

+// Mux ACL Handler for adding/removing ACLs
+class MuxAclHandler
+{
+    public:


nit: indentation is off compared to other header files.

tahmed-dev · 2020-12-22T01:30:23Z

orchagent/muxorch.h

+        return (state_ == MuxState::MUX_STATE_ACTIVE);
+    }
+
+    typedef pair<MuxStateChange, bool (MuxCable::*)()> handler_pair;


nit: using?

gechiang · 2020-12-22T03:32:58Z

@prsunny VS test has 27 failures. Can you address them? Also some branch conflicts needs to be resolved...

prsunny · 2020-12-22T22:26:16Z

retest this please

prsunny · 2020-12-23T01:51:56Z

@gechiang , resolved conflicts..

…r Extensions (sonic-net#1497) Changes when reading system EEPROM values from State DB (`decode-syseeprom -d`): - Display CRC-32 - Display all present vendor extensions - Display hex TLV code with lower-case `x` to match the output when reading directly from EEPROM - If a TLV code is not available, omit it from the output rather than displaying 'N/A' for all fields

MuxTunnel orchagent changes

59c2df0

prsunny mentioned this pull request Nov 6, 2020

Tunnel Manager changes sonic-net/sonic-buildimage#5843

Merged

3 tasks

Create P2P tunnel, Renaming, IPv6 nbr handling

0dc07ef

prsunny marked this pull request as ready for review November 26, 2020 00:13

gechiang requested review from gechiang, jleveque, lguohan and pavel-shirshov November 30, 2020 21:14

prsunny added 2 commits December 3, 2020 01:31

Update statedb state for mux hw, fix logs

7b527fd

Merge remote-tracking branch 'upstream1/master' into iptunnel

6ff07ac

prsunny force-pushed the iptunnel branch from 46dd2a6 to 6ff07ac Compare December 3, 2020 01:42

Fix compilation

6d545df

gechiang reviewed Dec 7, 2020

View reviewed changes

tahmed-dev reviewed Dec 7, 2020

View reviewed changes

gechiang reviewed Dec 7, 2020

View reviewed changes

tahmed-dev reviewed Dec 8, 2020

View reviewed changes

prsunny added 3 commits December 8, 2020 20:22

Address review comments

4fa72c4

Merge remote-tracking branch 'upstream1/master' into iptunnel

0390536

Revert statedb hw attribute update

fd20f0e

daall previously approved these changes Dec 14, 2020

View reviewed changes

Address review comments, add pytest for tunnel handling

3adbe90

prsunny dismissed daall’s stale review via 3adbe90 December 18, 2020 01:59

tahmed-dev previously approved these changes Dec 22, 2020

View reviewed changes

prsunny added 2 commits December 22, 2020 17:44

Merge remote-tracking branch 'upstream1/master' into iptunnel

5b21dfa

Address review comments

cb52c21

prsunny dismissed tahmed-dev’s stale review via cb52c21 December 22, 2020 19:41

tahmed-dev approved these changes Dec 23, 2020

View reviewed changes

gechiang approved these changes Dec 23, 2020

View reviewed changes

prsunny merged commit c39a4b1 into sonic-net:master Dec 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mux/IPTunnel orchagent changes #1497

Mux/IPTunnel orchagent changes #1497

prsunny commented Nov 6, 2020

prsunny commented Nov 30, 2020

gechiang Dec 7, 2020

prsunny Dec 18, 2020

tahmed-dev Dec 1, 2020

tahmed-dev Dec 1, 2020

prsunny Dec 18, 2020

tahmed-dev Dec 1, 2020

prsunny Dec 18, 2020

tahmed-dev Dec 1, 2020

prsunny Dec 18, 2020

gechiang Dec 7, 2020

prsunny Dec 18, 2020

gechiang Dec 7, 2020

prsunny Dec 18, 2020

tahmed-dev Dec 8, 2020

prsunny Dec 18, 2020

tahmed-dev Dec 8, 2020

prsunny Dec 18, 2020

tahmed-dev Dec 8, 2020

daall left a comment

tahmed-dev Dec 22, 2020

tahmed-dev Dec 22, 2020

tahmed-dev Dec 22, 2020

tahmed-dev Dec 22, 2020

tahmed-dev Dec 22, 2020

tahmed-dev Dec 22, 2020

tahmed-dev Dec 22, 2020

gechiang commented Dec 22, 2020

prsunny commented Dec 22, 2020

prsunny commented Dec 23, 2020


		TunnelMgr tunnelmgr(&cfgDb, &appDb, cfg_tunnel_tables);

		std::vector<Orch *> cfgOrchList = {&tunnelmgr};

Mux/IPTunnel orchagent changes #1497

Mux/IPTunnel orchagent changes #1497

Conversation

prsunny commented Nov 6, 2020

prsunny commented Nov 30, 2020

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

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

daall 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gechiang commented Dec 22, 2020

prsunny commented Dec 22, 2020

prsunny commented Dec 23, 2020