From d823dd179677c59dc3fc896f99a0b098b588354f Mon Sep 17 00:00:00 2001 From: Shuotian Cheng Date: Fri, 20 Sep 2019 16:31:47 -0700 Subject: [PATCH] [MirrorOrch]: Mirror Session Retention across Warm Reboot (#1054) * [MirrorOrch]: Mirror Session Retention across Warm Reboot After warm reboot, it is expected that the monitor port of the mirror session is retained - no changing on the monitor port withint the ECMP group members and the LAG members. This is due to the general of the sairedis comparison logic and the minimalization of SAI function calls during reconciliation. Changes: 1. Add bake() and postBake() functions in MirrorOrch bake() function retrieves the state database information and get the VLAN + monitor port information. postBake() function leverages the information and recovers the active mirror sessions the same as before warm reboot. 2. state database format change Instead of storing the object ID of the monitor port, store the alias of the monitor port. Instead of storing true/false of VLAN header, store the VLAN ID. Update: Freeze doTask() function instead of update() function With this update, we could fix potential orchagent issues before the warm reboot when the monitor port was wrongly calculated. Signed-off-by: Shu0T1an ChenG --- orchagent/mirrororch.cpp | 196 ++++++++++++++++++++++++++++++++++----- orchagent/mirrororch.h | 6 ++ orchagent/orch.cpp | 19 ++-- orchagent/orch.h | 2 + orchagent/orchdaemon.cpp | 8 ++ orchagent/port.h | 4 +- tests/test_mirror.py | 2 +- 7 files changed, 204 insertions(+), 33 deletions(-) diff --git a/orchagent/mirrororch.cpp b/orchagent/mirrororch.cpp index ac6857ff4aa0..fbb241d1b3a6 100644 --- a/orchagent/mirrororch.cpp +++ b/orchagent/mirrororch.cpp @@ -10,10 +10,12 @@ #include "swssnet.h" #include "converter.h" #include "mirrororch.h" +#include "tokenize.h" #define MIRROR_SESSION_STATUS "status" #define MIRROR_SESSION_STATUS_ACTIVE "active" #define MIRROR_SESSION_STATUS_INACTIVE "inactive" +#define MIRROR_SESSION_NEXT_HOP_IP "next_hop_ip" #define MIRROR_SESSION_SRC_IP "src_ip" #define MIRROR_SESSION_DST_IP "dst_ip" #define MIRROR_SESSION_GRE_TYPE "gre_type" @@ -23,7 +25,7 @@ #define MIRROR_SESSION_DST_MAC_ADDRESS "dst_mac" #define MIRROR_SESSION_MONITOR_PORT "monitor_port" #define MIRROR_SESSION_ROUTE_PREFIX "route_prefix" -#define MIRROR_SESSION_VLAN_HEADER_VALID "vlan_header_valid" +#define MIRROR_SESSION_VLAN_ID "vlan_id" #define MIRROR_SESSION_POLICER "policer" #define MIRROR_SESSION_DEFAULT_VLAN_PRI 0 @@ -58,6 +60,7 @@ MirrorEntry::MirrorEntry(const string& platform) : } nexthopInfo.prefix = IpPrefix("0.0.0.0/0"); + nexthopInfo.nexthop = IpAddress("0.0.0.0"); } MirrorOrch::MirrorOrch(TableConnector stateDbConnector, TableConnector confDbConnector, @@ -75,6 +78,74 @@ MirrorOrch::MirrorOrch(TableConnector stateDbConnector, TableConnector confDbCon m_fdbOrch->attach(this); } +bool MirrorOrch::bake() +{ + SWSS_LOG_ENTER(); + + // Freeze the route update during orchagent restoration + m_freeze = true; + + deque entries; + vector keys; + m_mirrorTable.getKeys(keys); + for (const auto &key : keys) + { + vector tuples; + m_mirrorTable.get(key, tuples); + + bool active = false; + string monitor_port; + string next_hop_ip; + + for (const auto &tuple : tuples) + { + if (fvField(tuple) == MIRROR_SESSION_STATUS) + { + active = fvValue(tuple) == MIRROR_SESSION_STATUS_ACTIVE; + } + else if (fvField(tuple) == MIRROR_SESSION_MONITOR_PORT) + { + monitor_port = fvValue(tuple); + } + else if (fvField(tuple) == MIRROR_SESSION_NEXT_HOP_IP) + { + next_hop_ip = fvValue(tuple); + } + } + + if (!active) + { + continue; + } + + SWSS_LOG_NOTICE("Found mirror session %s active before warm reboot", + key.c_str()); + + // Recover saved active session's monitor port + m_recoverySessionMap.emplace( + key, monitor_port + state_db_key_delimiter + next_hop_ip); + } + + return Orch::bake(); +} + +bool MirrorOrch::postBake() +{ + SWSS_LOG_ENTER(); + + SWSS_LOG_NOTICE("Start MirrorOrch post-baking"); + + // Unfreeze the route update + m_freeze = false; + + Orch::doTask(); + + // Clean up the recovery cache + m_recoverySessionMap.clear(); + + return Orch::postBake(); +} + void MirrorOrch::update(SubjectType type, void *cntx) { SWSS_LOG_ENTER(); @@ -320,10 +391,11 @@ void MirrorOrch::setSessionState(const string& name, const MirrorEntry& session, { SWSS_LOG_ENTER(); - SWSS_LOG_INFO("Setting mirroring sessions %s state\n", name.c_str()); + SWSS_LOG_INFO("Update mirroring sessions %s state", name.c_str()); vector fvVector; string value; + if (attr.empty() || attr == MIRROR_SESSION_STATUS) { value = session.status ? MIRROR_SESSION_STATUS_ACTIVE : MIRROR_SESSION_STATUS_INACTIVE; @@ -332,8 +404,9 @@ void MirrorOrch::setSessionState(const string& name, const MirrorEntry& session, if (attr.empty() || attr == MIRROR_SESSION_MONITOR_PORT) { - value = sai_serialize_object_id(session.neighborInfo.portId); - fvVector.emplace_back(MIRROR_SESSION_MONITOR_PORT, value); + Port port; + m_portsOrch->getPort(session.neighborInfo.portId, port); + fvVector.emplace_back(MIRROR_SESSION_MONITOR_PORT, port.m_alias); } if (attr.empty() || attr == MIRROR_SESSION_DST_MAC_ADDRESS) @@ -348,10 +421,16 @@ void MirrorOrch::setSessionState(const string& name, const MirrorEntry& session, fvVector.emplace_back(MIRROR_SESSION_ROUTE_PREFIX, value); } - if (attr.empty() || attr == MIRROR_SESSION_VLAN_HEADER_VALID) + if (attr.empty() || attr == MIRROR_SESSION_VLAN_ID) + { + value = to_string(session.neighborInfo.port.m_vlan_info.vlan_id); + fvVector.emplace_back(MIRROR_SESSION_VLAN_ID, value); + } + + if (attr.empty() || attr == MIRROR_SESSION_NEXT_HOP_IP) { - value = to_string(session.neighborInfo.port.m_type == Port::VLAN); - fvVector.emplace_back(MIRROR_SESSION_VLAN_HEADER_VALID, value); + value = session.nexthopInfo.nexthop.to_string(); + fvVector.emplace_back(MIRROR_SESSION_NEXT_HOP_IP, value); } m_mirrorTable.set(name, fvVector); @@ -396,32 +475,68 @@ bool MirrorOrch::getNeighborInfo(const string& name, MirrorEntry& session) return false; } - // Get the firt member of the LAG - Port member; - const auto& first_member_alias = *session.neighborInfo.port.m_members.begin(); - m_portsOrch->getPort(first_member_alias, member); + // Recover the LAG member monitor port picked before warm reboot + // to minimalize the data plane changes across warm reboot. + if (m_recoverySessionMap.find(name) != m_recoverySessionMap.end()) + { + string alias = tokenize(m_recoverySessionMap[name], + state_db_key_delimiter, 1)[0]; + Port member; + m_portsOrch->getPort(alias, member); + + SWSS_LOG_NOTICE("Recover mirror session %s with LAG member port %s", + name.c_str(), alias.c_str()); + session.neighborInfo.portId = member.m_port_id; + } + else + { + // Get the firt member of the LAG + Port member; + string first_member_alias = *session.neighborInfo.port.m_members.begin(); + m_portsOrch->getPort(first_member_alias, member); + + session.neighborInfo.portId = member.m_port_id; + } - session.neighborInfo.portId = member.m_port_id; return true; } case Port::VLAN: { SWSS_LOG_NOTICE("Get mirror session destination IP neighbor VLAN %d", session.neighborInfo.port.m_vlan_info.vlan_id); - Port member; - if (!m_fdbOrch->getPort(session.neighborInfo.mac, session.neighborInfo.port.m_vlan_info.vlan_id, member)) + + // Recover the VLAN member monitor port picked before warm reboot + // since the FDB entries are not yet learned on the hardware + if (m_recoverySessionMap.find(name) != m_recoverySessionMap.end()) { - SWSS_LOG_NOTICE("Waiting to get FDB entry MAC %s under VLAN %s", - session.neighborInfo.mac.to_string().c_str(), - session.neighborInfo.port.m_alias.c_str()); - return false; + string alias = tokenize(m_recoverySessionMap[name], + state_db_key_delimiter, 1)[0]; + Port member; + m_portsOrch->getPort(alias, member); + + SWSS_LOG_NOTICE("Recover mirror session %s with VLAN member port %s", + name.c_str(), alias.c_str()); + session.neighborInfo.portId = member.m_port_id; } else { - // Update monitor port - session.neighborInfo.portId = member.m_port_id; - return true; + Port member; + if (!m_fdbOrch->getPort(session.neighborInfo.mac, + session.neighborInfo.port.m_vlan_info.vlan_id, member)) + { + SWSS_LOG_NOTICE("Waiting to get FDB entry MAC %s under VLAN %s", + session.neighborInfo.mac.to_string().c_str(), + session.neighborInfo.port.m_alias.c_str()); + return false; + } + else + { + // Update monitor port + session.neighborInfo.portId = member.m_port_id; + } } + + return true; } default: { @@ -741,7 +856,7 @@ bool MirrorOrch::updateSessionType(const string& name, MirrorEntry& session) SWSS_LOG_NOTICE("Update mirror session %s VLAN to %s", name.c_str(), session.neighborInfo.port.m_alias.c_str()); - setSessionState(name, session, MIRROR_SESSION_VLAN_HEADER_VALID); + setSessionState(name, session, MIRROR_SESSION_VLAN_ID); return true; } @@ -782,7 +897,35 @@ void MirrorOrch::updateNextHop(const NextHopUpdate& update) if (update.nexthopGroup != IpAddresses()) { - session.nexthopInfo.nexthop = *update.nexthopGroup.getIpAddresses().begin(); + SWSS_LOG_NOTICE(" next hop IPs: %s", update.nexthopGroup.to_string().c_str()); + + // Recover the session based on the state database information + if (m_recoverySessionMap.find(name) != m_recoverySessionMap.end()) + { + IpAddress nexthop = IpAddress(tokenize(m_recoverySessionMap[name], + state_db_key_delimiter, 1)[1]); + + // Check if recovered next hop IP is within the update's next hop IPs + if (update.nexthopGroup.getIpAddresses().count(nexthop)) + { + SWSS_LOG_NOTICE("Recover mirror session %s with next hop %s", + name.c_str(), nexthop.to_string().c_str()); + session.nexthopInfo.nexthop = nexthop; + } + else + { + // Correct the next hop IP + SWSS_LOG_NOTICE("Correct mirror session %s next hop from %s to %s", + name.c_str(), session.nexthopInfo.nexthop.to_string().c_str(), + nexthop.to_string().c_str()); + session.nexthopInfo.nexthop = *update.nexthopGroup.getIpAddresses().begin(); + } + } + else + { + // Pick the first one from the next hop group + session.nexthopInfo.nexthop = *update.nexthopGroup.getIpAddresses().begin(); + } } else { @@ -968,6 +1111,11 @@ void MirrorOrch::doTask(Consumer& consumer) { SWSS_LOG_ENTER(); + if (m_freeze) + { + return; + } + if (!gPortsOrch->allPortsReady()) { return; @@ -991,7 +1139,7 @@ void MirrorOrch::doTask(Consumer& consumer) } else { - SWSS_LOG_ERROR("Unknown operation type %s\n", op.c_str()); + SWSS_LOG_ERROR("Unknown operation type %s", op.c_str()); } consumer.m_toSync.erase(it++); diff --git a/orchagent/mirrororch.h b/orchagent/mirrororch.h index 765b24cb5f5d..4a5801df6f45 100644 --- a/orchagent/mirrororch.h +++ b/orchagent/mirrororch.h @@ -69,6 +69,8 @@ class MirrorOrch : public Orch, public Observer, public Subject MirrorOrch(TableConnector appDbConnector, TableConnector confDbConnector, PortsOrch *portOrch, RouteOrch *routeOrch, NeighOrch *neighOrch, FdbOrch *fdbOrch, PolicerOrch *policerOrch); + bool bake() override; + bool postBake() override; void update(SubjectType, void *); bool sessionExists(const string&); bool getSessionStatus(const string&, bool&); @@ -86,6 +88,10 @@ class MirrorOrch : public Orch, public Observer, public Subject Table m_mirrorTable; MirrorTable m_syncdMirrors; + // session_name -> VLAN | monitor_port_alias | next_hop_ip + map m_recoverySessionMap; + + bool m_freeze = false; void createEntry(const string&, const vector&); void deleteEntry(const string&); diff --git a/orchagent/orch.cpp b/orchagent/orch.cpp index 89f924dcccea..a6dce7efbb07 100644 --- a/orchagent/orch.cpp +++ b/orchagent/orch.cpp @@ -27,7 +27,7 @@ Orch::Orch(DBConnector *db, const string tableName, int pri) Orch::Orch(DBConnector *db, const vector &tableNames) { - for(auto it : tableNames) + for (auto it : tableNames) { addConsumer(db, it, default_orch_pri); } @@ -35,7 +35,7 @@ Orch::Orch(DBConnector *db, const vector &tableNames) Orch::Orch(DBConnector *db, const vector &tableNames_with_pri) { - for(const auto& it : tableNames_with_pri) + for (const auto& it : tableNames_with_pri) { addConsumer(db, it.first, it.second); } @@ -60,7 +60,7 @@ Orch::~Orch() vector Orch::getSelectables() { vector selectables; - for(auto& it : m_consumerMap) + for (auto& it : m_consumerMap) { selectables.push_back(it.second.get()); } @@ -240,7 +240,7 @@ bool Orch::bake() { SWSS_LOG_ENTER(); - for(auto &it : m_consumerMap) + for (auto &it : m_consumerMap) { string executorName = it.first; auto executor = it.second; @@ -257,6 +257,13 @@ bool Orch::bake() return true; } +bool Orch::postBake() +{ + SWSS_LOG_ENTER(); + + return true; +} + /* - Validates reference has proper format which is [table_name:object_name] - validates table_name exists @@ -365,7 +372,7 @@ ref_resolve_status Orch::resolveFieldRefValue( void Orch::doTask() { - for(auto &it : m_consumerMap) + for (auto &it : m_consumerMap) { it.second->drain(); } @@ -373,7 +380,7 @@ void Orch::doTask() void Orch::dumpPendingTasks(vector &ts) { - for(auto &it : m_consumerMap) + for (auto &it : m_consumerMap) { Consumer* consumer = dynamic_cast(it.second.get()); if (consumer == NULL) diff --git a/orchagent/orch.h b/orchagent/orch.h index 0298770f2fac..8b2a13082372 100644 --- a/orchagent/orch.h +++ b/orchagent/orch.h @@ -183,6 +183,8 @@ class Orch // Prepare for warm start if Redis contains valid input data // otherwise fallback to cold start virtual bool bake(); + // Clean up the state set in bake() + virtual bool postBake(); /* Iterate all consumers in m_consumerMap and run doTask(Consumer) */ virtual void doTask(); diff --git a/orchagent/orchdaemon.cpp b/orchagent/orchdaemon.cpp index 2c4f2126b268..400d24f85a96 100644 --- a/orchagent/orchdaemon.cpp +++ b/orchagent/orchdaemon.cpp @@ -479,14 +479,22 @@ bool OrchDaemon::warmRestoreAndSyncUp() * Fourth iteration: Drain remaining data that are out of order like LAG_MEMBER_TABLE and * VLAN_MEMBER_TABLE since they were checked before LAG_TABLE and VLAN_TABLE within gPortsOrch. */ + for (auto it = 0; it < 4; it++) { + SWSS_LOG_DEBUG("The current iteration is %d", it); + for (Orch *o : m_orchList) { o->doTask(); } } + for (Orch *o : m_orchList) + { + o->postBake(); + } + /* * At this point, all the pre-existing data should have been processed properly, and * orchagent should be in exact same state of pre-shutdown. diff --git a/orchagent/port.h b/orchagent/port.h index 34b72aa8c2d5..ad264a1db569 100644 --- a/orchagent/port.h +++ b/orchagent/port.h @@ -30,8 +30,8 @@ typedef std::map vlan_members_t; struct VlanInfo { - sai_object_id_t vlan_oid; - sai_vlan_id_t vlan_id; + sai_object_id_t vlan_oid = 0; + sai_vlan_id_t vlan_id = 0; }; class Port diff --git a/tests/test_mirror.py b/tests/test_mirror.py index c3da54d43d5f..80db25075576 100644 --- a/tests/test_mirror.py +++ b/tests/test_mirror.py @@ -140,7 +140,7 @@ def test_MirrorAddRemove(self, dvs, testlog): # add route to mirror destination via 10.0.0.1 self.add_route(dvs, "2.2.2.2", "10.0.0.1") assert self.get_mirror_session_state(session)["status"] == "active" - assert self.get_mirror_session_state(session)["monitor_port"] == dvs.asicdb.portnamemap["Ethernet16"] + assert self.get_mirror_session_state(session)["monitor_port"] == "Ethernet16" assert self.get_mirror_session_state(session)["dst_mac"] == "02:04:06:08:10:12" assert self.get_mirror_session_state(session)["route_prefix"] == "2.2.2.2/32"