Fix tsan error in droplog

Separate delete call. Add some more safeguards. Signed-off-by: Kiran Shastri <shastrinator@gmail.com> (cherry picked from commit 1f5032f)
noironetworks · Dec 21, 2020 · d1e85e0 · d1e85e0
1 parent 0bdfd79
commit d1e85e0
Show file tree

Hide file tree

Showing 6 changed files with 38 additions and 17 deletions.
diff --git a/agent-ovs/lib/ExtraConfigManager.cpp b/agent-ovs/lib/ExtraConfigManager.cpp
@@ -181,16 +181,6 @@ void ExtraConfigManager::packetDropPruneConfigUpdated(std::shared_ptr<PacketDrop
     Mutator mutator(framework, "policyelement");
     optional<shared_ptr<modelgbp::policy::Universe>> polUni =
             modelgbp::policy::Universe::resolve(framework);
-    if(dropPrune->removed) {
-        opflex::modb::URI dropPruneURI = 
-            opflex::modb::URIBuilder(polUni.get()->getURI())
-            .addElement("ObserverDropPruneConfig").addElement(dropPrune->filterName).build();
-        DropPruneConfig::remove(framework, dropPruneURI);
-        mutator.commit();
-        dropPruneMap.erase(dropPrune->filterName);
-        notifyPacketDropPruneConfigListeners(dropPrune->filterName);
-        return;
-    }
     shared_ptr<DropPruneConfig> dropPruneCfg =
         polUni.get()->addObserverDropPruneConfig(dropPrune->filterName);
     dropPruneCfg->unsetSrcAddress();
@@ -238,17 +228,37 @@ void ExtraConfigManager::packetDropPruneConfigUpdated(std::shared_ptr<PacketDrop
         dropPruneCfg->setDstPort(dropPrune->dport.get());
     }
     mutator.commit();
+    unique_lock<mutex> guard(prune_mutex);
     dropPruneMap[dropPrune->filterName] = dropPrune;
+    guard.unlock();
     notifyPacketDropPruneConfigListeners(dropPrune->filterName);
 }
 
+void ExtraConfigManager::packetDropPruneConfigDeleted(const std::string &filterName) {
+    using modelgbp::observer::DropPruneConfig;
+    Mutator mutator(framework, "policyelement");
+    optional<shared_ptr<modelgbp::policy::Universe>> polUni =
+            modelgbp::policy::Universe::resolve(framework);
+    opflex::modb::URI dropPruneURI =
+        opflex::modb::URIBuilder(polUni.get()->getURI())
+        .addElement("ObserverDropPruneConfig").addElement(filterName).build();
+    DropPruneConfig::remove(framework, dropPruneURI);
+    mutator.commit();
+    unique_lock<mutex> guard(prune_mutex);
+    dropPruneMap.erase(filterName);
+    guard.unlock();
+    notifyPacketDropPruneConfigListeners(filterName);
+}
+
 bool ExtraConfigManager::getPacketDropPruneSpec(const std::string &pruneFilter, std::shared_ptr<PacketDropLogPruneSpec> &pruneSpec) {
     bool ret = false;
+    unique_lock<mutex> guard(prune_mutex);
     auto it= dropPruneMap.find(pruneFilter);
     if(it != dropPruneMap.end()) {
         pruneSpec = it->second;
         ret = true;
     }
+    guard.unlock();
     return ret;
 }
 

diff --git a/agent-ovs/lib/FSPacketDropLogConfigSource.cpp b/agent-ovs/lib/FSPacketDropLogConfigSource.cpp
@@ -244,9 +244,7 @@ void FSPacketDropLogConfigSource::updated(const fs::path& filePath) {
             /*Notify removed*/
             if(dropPruneCfgSet) {
                 for(auto itr: *dropPruneCfgSet) {
-                    std::shared_ptr<PacketDropLogPruneSpec> pruneSpec(new PacketDropLogPruneSpec(itr));
-                    pruneSpec->removed = true;
-                    manager->packetDropPruneConfigUpdated(pruneSpec);
+                    manager->packetDropPruneConfigDeleted(itr);
                 }
             }
             dropPruneCfgSet = newPruneSet;
@@ -337,9 +335,7 @@ void FSPacketDropLogConfigSource::deleted(const fs::path& filePath) {
             dropCfg.dropLogMode = DropLogModeEnumT::CONST_UNFILTERED_DROP_LOG;
             manager->packetDropLogConfigUpdated(dropCfg);
             for(auto itr: *dropPruneCfgSet) {
-                std::shared_ptr<PacketDropLogPruneSpec> pruneSpec(new PacketDropLogPruneSpec(itr));
-                pruneSpec->removed = true;
-                manager->packetDropPruneConfigUpdated(pruneSpec);
+                manager->packetDropPruneConfigDeleted(itr);
             }
             dropPruneCfgSet->clear();
             LOG(INFO) << "Removed packet drop log config "

diff --git a/agent-ovs/lib/include/opflexagent/ExtraConfigManager.h b/agent-ovs/lib/include/opflexagent/ExtraConfigManager.h
@@ -147,6 +147,14 @@ class ExtraConfigManager : private boost::noncopyable {
      */
     void packetDropPruneConfigUpdated(std::shared_ptr<PacketDropLogPruneSpec> &pruneCfg);
 
+    /**
+     * Add or update a packet drop log prune config object
+     *
+     * @param filterName Drop log Prune spec filter name
+     */
+    void packetDropPruneConfigDeleted(const std::string &filterName);
+
+    std::mutex prune_mutex;
     drop_prune_map_t dropPruneMap;
     /**
      * The extraConfig listeners that have been registered

diff --git a/agent-ovs/lib/test/ExtraConfigManager_test.cpp b/agent-ovs/lib/test/ExtraConfigManager_test.cpp
@@ -100,7 +100,7 @@ BOOST_FIXTURE_TEST_CASE( droplogpruneconfigsource, FSConfigFixture ) {
     using modelgbp::observer::DropLogConfig;
     using modelgbp::observer::DropLogModeEnumT;
     using modelgbp::observer::DropPruneConfig;
-    fs::path path(temp / "a.droplogcfg");
+    fs::path path(temp / "b.droplogcfg");
     fs::ofstream os(path);
     os << "{"
        << "\"drop-log-enable\": true,\n"
@@ -126,6 +126,7 @@ BOOST_FIXTURE_TEST_CASE( droplogpruneconfigsource, FSConfigFixture ) {
     BOOST_CHECK(dropLogCfg.get()->getDropLogMode().get()== DropLogModeEnumT::CONST_UNFILTERED_DROP_LOG);
     BOOST_CHECK(dropLogCfg.get()->getDropLogEnable().get());
     /*Test create prune filter*/
+    WAIT_FOR(DropPruneConfig::resolve(agent.getFramework(), dropPruneUri), 500);
     boost::optional<shared_ptr<DropPruneConfig>> dropPruneCfg = DropPruneConfig::resolve(agent.getFramework(), dropPruneUri);
     BOOST_CHECK(dropPruneCfg.get()->getFilterName().get() == "flt1");
     BOOST_CHECK(dropPruneCfg.get()->getSrcAddress().get() == "1.2.3.4");

diff --git a/agent-ovs/ovs/AccessFlowManager.cpp b/agent-ovs/ovs/AccessFlowManager.cpp
@@ -964,6 +964,7 @@ void AccessFlowManager::rdConfigUpdated(const opflex::modb::URI& rdURI) {
 }
 
 void AccessFlowManager::packetDropLogConfigUpdated(const opflex::modb::URI& dropLogCfgURI) {
+    if (stopping) return;
     using modelgbp::observer::DropLogConfig;
     using modelgbp::observer::DropLogModeEnumT;
     FlowEntryList dropLogFlows;
@@ -1004,6 +1005,7 @@ void AccessFlowManager::packetDropLogConfigUpdated(const opflex::modb::URI& drop
 }
 
 void AccessFlowManager::packetDropFlowConfigUpdated(const opflex::modb::URI& dropFlowCfgURI) {
+    if (stopping) return;
     using modelgbp::observer::DropFlowConfig;
     optional<shared_ptr<DropFlowConfig>> dropFlowCfg =
             DropFlowConfig::resolve(agent.getFramework(), dropFlowCfgURI);

diff --git a/agent-ovs/ovs/IntFlowManager.cpp b/agent-ovs/ovs/IntFlowManager.cpp
@@ -418,6 +418,8 @@ void IntFlowManager::rdConfigUpdated(const opflex::modb::URI& rdURI) {
 }
 
 void IntFlowManager::packetDropLogConfigUpdated(const opflex::modb::URI& dropLogCfgURI) {
+    if(stopping)
+        return;
     using modelgbp::observer::DropLogConfig;
     using modelgbp::observer::DropLogModeEnumT;
     FlowEntryList dropLogFlows;
@@ -458,6 +460,8 @@ void IntFlowManager::packetDropLogConfigUpdated(const opflex::modb::URI& dropLog
 }
 
 void IntFlowManager::packetDropFlowConfigUpdated(const opflex::modb::URI& dropFlowCfgURI) {
+    if(stopping)
+        return;
     using modelgbp::observer::DropFlowConfig;
     optional<shared_ptr<DropFlowConfig>> dropFlowCfg =
             DropFlowConfig::resolve(agent.getFramework(), dropFlowCfgURI);