Mux/IPTunnel orchagent changes#1497
Conversation
|
retest this please |
| } | ||
| else if (op == DEL_COMMAND) | ||
| { | ||
| /* TODO: Handle Tunnel delete for other tunnel types */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In the del operation we will not get the value
cfgmgr/Makefile.am
Outdated
| 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 No newline at end of file |
| using namespace swss; | ||
|
|
||
| /* select() function timeout retry time, in millisecond */ | ||
| #define SELECT_TIMEOUT 1000 |
There was a problem hiding this comment.
What about the time unit to the name SELECT_TIMEOUT_MSEC
There was a problem hiding this comment.
This is the default value for all mgrs, keeping it for consistency
|
|
||
| TunnelMgr tunnelmgr(&cfgDb, &appDb, cfg_tunnel_tables); | ||
|
|
||
| std::vector<Orch *> cfgOrchList = {&tunnelmgr}; |
There was a problem hiding this comment.
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.
| return false; | ||
| } | ||
|
|
||
| if (!aclHandler(port.m_port_id)) |
There was a problem hiding this comment.
Why are we not removing the routes here as per other failure cases above?
| ret = s.select(&sel, SELECT_TIMEOUT); | ||
| if (ret == Select::ERROR) | ||
| { | ||
| SWSS_LOG_NOTICE("Error: %s!", strerror(errno)); |
There was a problem hiding this comment.
Any reason not to use SWSS_LOG_ERROR instead of NOTICE here for the error handling?
There was a problem hiding this comment.
Same for all managers, keeping for consistency
orchagent/muxorch.cpp
Outdated
| 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", |
There was a problem hiding this comment.
To ease debug you may want to consider also capture nh oid in the syslog when create route failed.
| const request_description_t mux_cfg_request_description = { | ||
| { REQ_T_STRING }, | ||
| { | ||
| { "server_ipv4", REQ_T_IP_PREFIX }, |
| const request_description_t mux_cable_request_description = { | ||
| { REQ_T_STRING }, | ||
| { | ||
| { "state", REQ_T_STRING }, |
There was a problem hiding this comment.
missing read_side and active_side fields
There was a problem hiding this comment.
Done for the State entry
orchagent/muxorch.cpp
Outdated
| extern sai_router_interface_api_t* sai_router_intfs_api; | ||
|
|
||
| /* Constants */ | ||
| #define MUX_TUNNEL "MUX_TUNNEL0" |
cfgmgr/tunnelmgr.cpp
Outdated
| if (op == SET_COMMAND) | ||
| { | ||
| m_appIpInIpTunnelTable.set(TunnelName, kfvFieldsValues(t)); | ||
|
|
| class TunnelMgr : public Orch | ||
| { | ||
| public: | ||
| TunnelMgr(DBConnector *cfgDb, DBConnector *appDb, std::string tableName); |
There was a problem hiding this comment.
What about explicitly deleting the default and copy constructor. Also, calling destructor as default
orchagent/muxorch.cpp
Outdated
| mux_cb_orch_ = gDirectory.get<MuxCableOrch*>(); | ||
| mux_state_orch_ = gDirectory.get<MuxStateOrch*>(); | ||
|
|
||
| nbr_handler_ = unique_ptr<MuxNbrHandler> (new MuxNbrHandler()); |
orchagent/muxorch.cpp
Outdated
| return false; | ||
| } | ||
|
|
||
| mux_cable_tb_[port_name] = std::unique_ptr<MuxCable> |
There was a problem hiding this comment.
make_unique<MuxCable>(...)
orchagent/muxorch.cpp
Outdated
| } | ||
|
|
||
| MuxOrch::MuxOrch(DBConnector *db, const std::vector<std::string> &tables, TunnelDecapOrch* decapOrch, NeighOrch* neighOrch) | ||
| : Orch2(db, tables, request_), decap_orch_(decapOrch), neigh_orch_(neighOrch) |
There was a problem hiding this comment.
initializer list element on separate lines to match other files.
orchagent/muxorch.h
Outdated
| // Mux ACL Handler for adding/removing ACLs | ||
| class MuxAclHandler | ||
| { | ||
| public: |
There was a problem hiding this comment.
nit: indentation is off compared to other header files.
orchagent/muxorch.h
Outdated
| return (state_ == MuxState::MUX_STATE_ACTIVE); | ||
| } | ||
|
|
||
| typedef pair<MuxStateChange, bool (MuxCable::*)()> handler_pair; |
|
@prsunny VS test has 27 failures. Can you address them? Also some branch conflicts needs to be resolved... |
|
retest this please |
|
@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
…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
Introduce TunnelMgr Daemon and Mux orchagent Added support to enable/disable neighbors via NeighOrch Added support to create/remove nexthop tunnels Added support for ACL handling for Mux state
What I did
Why I did it
Support Mux state transitions in orchagent
How I verified it
Manual test/verification in-progress
Details if related