From ab0e4749b90e34deee73901e6dc182cd3ad0414f Mon Sep 17 00:00:00 2001 From: Suman Kumar <73022268+sumanbrcm@users.noreply.github.com> Date: Tue, 8 Nov 2022 17:31:13 +0530 Subject: [PATCH] swss: Fixing race condition for rif counters (#2488) The cleanup code for stale rif counters are now moved to syncd . Earlier as part of fix for issue #2193 the cleanup for stale rif counters was added. But it could create a race condition between orchagent removes RIF rate counters from DB and lua script fetching them. So as a fix all such cleanup has been moved to syncd. - What I did Fix for sonic-net/sonic-buildimage#11621 As a past fix which aimed at removing stale rif counters (#2199) , there is a chance of race condition and it leads to lua script reporting error. To handle this , the rif counters cleanup code(handled in cleanUpRifFromCounterDb) is now called from syncd ( removeCounter ) to avoid such race condition. - Why I did it The operations in Orchagent and syncd is not synchronous, so while Orchagent deletes the rif counters from Counters Db, the syncd could still access it. In race conditions the lua script trying to fetch rif counters will have errors syslog for such access as it was already deleted by orchagent. The cleanup code is removed from orchagent is added in syncd - it will make sure no such race condition would get hit. - How I verified it Followed the steps in (sonic-net/sonic-buildimage#11621) : Create RIF in SONiC, wait till RIF rates are populated in COUNTERS DB Remove RIF Repeat the steps multiple times and check if any error syslog is seen (No error syslog is seen) Also checked cleanup for rif counters. After RIF creation derived info of oid for RIF from "COUNTERS_RIF_NAME_MAP" 127) "Vlan100" 128) "oid:0x6000000000aa5" Checked all the tabled in COUNTER_DB which has same OID in keys 127.0.0.1:6379[2]> keys 6000000000aa5 1) "RATES:oid:0x6000000000aa5:RIF" 2) "COUNTERS:oid:0x6000000000aa5" 3) "RATES:oid:0x6000000000aa5" 127.0.0.1:6379[2]> Deleted the RIF by removing the ip on the intf. Checked COUNTER_DB again with same OID if there are stale entries or not. No stale entries exist now. 127.0.0.1:6379[2]> keys 6000000000aa5 (empty array) 127.0.0.1:6379[2]> Signed-off-by: Suman Kumar --- orchagent/intfsorch.cpp | 35 ----------------------------------- orchagent/intfsorch.h | 4 ---- 2 files changed, 39 deletions(-) diff --git a/orchagent/intfsorch.cpp b/orchagent/intfsorch.cpp index 595ec7292371..adc2aac9f480 100644 --- a/orchagent/intfsorch.cpp +++ b/orchagent/intfsorch.cpp @@ -1277,7 +1277,6 @@ bool IntfsOrch::removeRouterIntfs(Port &port) const auto id = sai_serialize_object_id(port.m_rif_id); removeRifFromFlexCounter(id, port.m_alias); - cleanUpRifFromCounterDb(id, port.m_alias); sai_status_t status = sai_router_intfs_api->remove_router_interface(port.m_rif_id); if (status != SAI_STATUS_SUCCESS) @@ -1500,45 +1499,11 @@ void IntfsOrch::removeRifFromFlexCounter(const string &id, const string &name) SWSS_LOG_DEBUG("Unregistered interface %s from Flex counter", name.c_str()); } -/* - TODO A race condition can exist when swss removes the counter from COUNTERS DB - and at the same time syncd is inserting a new entry in COUNTERS DB. Therefore - all the rif counters cleanup code should move to syncd -*/ -void IntfsOrch::cleanUpRifFromCounterDb(const string &id, const string &name) -{ - SWSS_LOG_ENTER(); - string counter_key = getRifCounterTableKey(id); - string rate_key = getRifRateTableKey(id); - string rate_init_key = getRifRateInitTableKey(id); - m_counter_db->del(counter_key); - m_counter_db->del(rate_key); - m_counter_db->del(rate_init_key); - SWSS_LOG_NOTICE("CleanUp interface %s oid %s from counter db", name.c_str(),id.c_str()); -} - string IntfsOrch::getRifFlexCounterTableKey(string key) { return string(RIF_STAT_COUNTER_FLEX_COUNTER_GROUP) + ":" + key; } -string IntfsOrch::getRifCounterTableKey(string key) -{ - return "COUNTERS:" + key; -} - -string IntfsOrch::getRifRateTableKey(string key) -{ - return "RATES:" + key; -} - -string IntfsOrch::getRifRateInitTableKey(string key) -{ - return "RATES:" + key + ":RIF"; -} - - - void IntfsOrch::generateInterfaceMap() { m_updateMapsTimer->start(); diff --git a/orchagent/intfsorch.h b/orchagent/intfsorch.h index ba28c8dde6f5..6b4942897efa 100644 --- a/orchagent/intfsorch.h +++ b/orchagent/intfsorch.h @@ -93,10 +93,6 @@ class IntfsOrch : public Orch unique_ptr m_flexCounterGroupTable; std::string getRifFlexCounterTableKey(std::string s); - std::string getRifCounterTableKey(std::string s); - std::string getRifRateTableKey(std::string s); - std::string getRifRateInitTableKey(std::string s); - void cleanUpRifFromCounterDb(const string &id, const string &name); bool addRouterIntfs(sai_object_id_t vrf_id, Port &port, string loopbackAction); bool removeRouterIntfs(Port &port);