Defer deletion of reloadable remap plugins (#11813)

* Defer deletion of reloadable remap plugins

* Fix tests, since plugins now remains

* Put the startup cleanup into a try-catch

* Make autest happier during shutdown

Author: zwoop

cmake/layout.cmake

@@ -43,7 +43,7 @@ set(CMAKE_INSTALL_LOCALSTATEDIR
     CACHE STRING "localstatedir"
 )
 set(CMAKE_INSTALL_RUNSTATEDIR
-    "${CMAKE_INSTALL_LOCALSTATEDIR}/trafficserver"
+    "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LOCALSTATEDIR}/trafficserver"
     CACHE STRING "runstatedir"
 )
 set(CMAKE_INSTALL_DATAROOTDIR

include/proxy/http/remap/PluginDso.h

@@ -170,12 +170,8 @@ class PluginDso : public PluginThreadContext
   static constexpr const char *const _tag = "plugin_dso"; /** @brief log tag used by this class */
   static const DbgCtl               &_dbg_ctl();
   swoc::file::file_time_type _mtime{fs::file_time_type::min()}; /* @brief modification time of the DSO's file, used for checking */
-  bool                       _preventiveCleaning = true;
-
-  static Ptr<LoadedPlugins>
-    _plugins; /** @brief a global list of plugins, usually maintained by a plugin factory or plugin instance itself */
-  std::atomic<int> _instanceCount{
-    0}; /** @brief used for properly calling "done" and "indicate config reload" methods by the factory */
+  static Ptr<LoadedPlugins>  _plugins;                          /** @brief a global list of plugins */
+  std::atomic<int>           _instanceCount{0}; /** @brief used for properly calling "done" and "indicate config reload" */
 };
 
 inline const DbgCtl &

include/proxy/http/remap/PluginFactory.h

@@ -99,12 +99,13 @@ class PluginFactory
   RemapPluginInst *getRemapPlugin(const fs::path &configPath, int argc, char **argv, std::string &error, bool dynamicReloadEnabled);
 
   virtual const char *getUuid();
-  void                clean(std::string &error);
 
   void deactivate();
   void indicatePreReload();
   void indicatePostReload(bool reloadSuccessful);
 
+  static void cleanup(); // For startup, clean out all temporary directory we may have left from before
+
 protected:
   PluginDso *findByEffectivePath(const fs::path &path, bool dynamicReloadEnabled);
   fs::path   getEffectivePath(const fs::path &configPath);
@@ -117,7 +118,6 @@ class PluginFactory
 
   ATSUuid        *_uuid = nullptr;
   std::error_code _ec;
-  bool            _preventiveCleaning = true;
 
   static constexpr const char *const _tag = "plugin_factory"; /** @brief log tag used by this class */
   static const DbgCtl               &_dbg_ctl();

src/proxy/http/remap/PluginDso.cc

@@ -68,6 +68,7 @@ PluginDso::PluginDso(const fs::path &configPath, const fs::path &effectivePath,
 PluginDso::~PluginDso()
 {
   std::string error;
+
   (void)unload(error);
 }
 
@@ -136,11 +137,6 @@ PluginDso::load(std::string &error, const fs::path &compilerPath)
         PluginError("plugin '%s' failed to load: %s", _configPath.c_str(), error.c_str());
       }
     }
-
-    /* Remove the runtime DSO copy even if we succeed loading to avoid leftovers after crashes */
-    if (_preventiveCleaning) {
-      clean(error);
-    }
   }
   PluginDbg(_dbg_ctl(), "plugin '%s' finished loading DSO", _configPath.c_str());
 

src/proxy/http/remap/PluginFactory.cc

@@ -36,6 +36,7 @@
 #include "../../../iocore/eventsystem/P_EventSystem.h"
 
 #include <algorithm> /* std::swap */
+#include <filesystem>
 
 RemapPluginInst::RemapPluginInst(RemapPluginInfo &plugin) : _plugin(plugin)
 {
@@ -102,7 +103,16 @@ PluginFactory::~PluginFactory()
   _instList.apply([](RemapPluginInst *pluginInst) -> void { delete pluginInst; });
   _instList.clear();
 
-  fs::remove_all(_runtimeDir, _ec);
+  if (!TSSystemState::is_event_system_shut_down()) {
+    uint32_t elevate_access = 0;
+
+    REC_ReadConfigInteger(elevate_access, "proxy.config.plugin.load_elevated");
+    ElevateAccess access(elevate_access ? ElevateAccess::FILE_PRIVILEGE : 0);
+
+    fs::remove_all(_runtimeDir, _ec);
+  } else {
+    fs::remove_all(_runtimeDir, _ec); // Try anyways
+  }
 
   PluginDbg(_dbg_ctl(), "destroyed plugin factory %s", getUuid());
   delete _uuid;
@@ -226,9 +236,6 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
           delete plugin;
         }
 
-        if (dynamicReloadEnabled && _preventiveCleaning) {
-          clean(error);
-        }
       } else {
         /* Plugin DSO load failed. */
         PluginDbg(_dbg_ctl(), "plugin '%s' DSO load failed", configPath.c_str());
@@ -276,6 +283,30 @@ PluginFactory::getEffectivePath(const fs::path &configPath)
   return path;
 }
 
+void
+PluginFactory::cleanup()
+{
+  std::error_code ec;
+  std::string     path(RecConfigReadRuntimeDir());
+
+  try {
+    if (path.starts_with("/") && std::filesystem::is_directory(path)) {
+      for (const auto &entry : std::filesystem::directory_iterator(path, ec)) {
+        if (entry.is_directory()) {
+          std::string dir_name   = entry.path().string();
+          int         dash_count = std::count(dir_name.begin(), dir_name.end(), '-'); // All UUIDs have 4 dashes
+
+          if (dash_count == 4) {
+            std::filesystem::remove_all(dir_name, ec);
+          }
+        }
+      }
+    }
+  } catch (std::exception &e) {
+    PluginError("Error cleaning up runtime directory: %s", e.what());
+  }
+}
+
 /**
  * @brief Find a plugin by path from our linked plugin list by using plugin effective (canonical) path
  *
@@ -326,9 +357,3 @@ PluginFactory::indicatePostReload(bool reloadSuccessful)
 
   PluginDso::loadedPlugins()->indicatePostReload(reloadSuccessful, pluginUsed, getUuid());
 }
-
-void
-PluginFactory::clean(std::string & /* error ATS_UNUSED */)
-{
-  fs::remove_all(_runtimeDir, _ec);
-}

src/proxy/http/remap/unit-tests/test_PluginDso.cc

@@ -66,8 +66,6 @@ class PluginDsoUnitTest : public PluginDso
   PluginDsoUnitTest(const fs::path &configPath, const fs::path &effectivePath, const fs::path &runtimePath)
     : PluginDso(configPath, effectivePath, runtimePath)
   {
-    /* don't remove runtime DSO copy preventively so we can check if it was created properly */
-    _preventiveCleaning = false;
   }
 
   virtual void

src/proxy/http/remap/unit-tests/test_PluginFactory.cc

@@ -68,11 +68,7 @@ static fs::path tempComponent = fs::path("c71e2bab-90dc-4770-9535-c9304c3de38e")
 class PluginFactoryUnitTest : public PluginFactory
 {
 public:
-  PluginFactoryUnitTest(const fs::path &tempComponent)
-  {
-    _tempComponent      = tempComponent;
-    _preventiveCleaning = false;
-  }
+  PluginFactoryUnitTest(const fs::path &tempComponent) { _tempComponent = tempComponent; }
 
 protected:
   const char *
@@ -305,9 +301,7 @@ SCENARIO("loading plugins", "[plugin][core]")
         validateSuccessfulConfigPathTest(plugin, error, effectivePath, runtimePath);
         CHECK(nullptr != PluginDso::loadedPlugins()->findByEffectivePath(effectivePath, isPluginDynamicReloadEnabled()));
 
-        // check Dso at effective path still exists while copy at runtime path doesn't
         CHECK(fs::exists(plugin->_plugin.effectivePath()));
-        CHECK(!fs::exists(plugin->_plugin.runtimePath()));
       }
 
       teardownConfigPathTest(factory);
@@ -606,13 +600,10 @@ checkTwoLoadedVersionsDifferent(const RemapPluginInst *plugin_v1, const RemapPlu
   // 2 versions can be different only when dynamic reload enabled
   CHECK(isPluginDynamicReloadEnabled());
 
-  // check Dso at effective path still exists while the copy at runtime path doesn't
   CHECK(plugin_v1->_plugin.effectivePath() != plugin_v1->_plugin.runtimePath());
   CHECK(fs::exists(plugin_v1->_plugin.effectivePath()));
-  CHECK(!fs::exists(plugin_v1->_plugin.runtimePath()));
   CHECK(plugin_v2->_plugin.effectivePath() != plugin_v2->_plugin.runtimePath());
   CHECK(fs::exists(plugin_v2->_plugin.effectivePath()));
-  CHECK(!fs::exists(plugin_v2->_plugin.runtimePath()));
 }
 
 void

src/traffic_server/traffic_server.cc

@@ -1786,7 +1786,6 @@ configure_io_uring()
 //
 // Main
 //
-
 int
 main(int /* argc ATS_UNUSED */, const char **argv)
 {
@@ -1926,6 +1925,9 @@ main(int /* argc ATS_UNUSED */, const char **argv)
     signal_register_crash_handler(crash_logger_invoke);
   }
 
+  // Clean out any remnant temporary plugin (UUID named) directories.
+  PluginFactory::cleanup();
+
 #if TS_USE_POSIX_CAP
   // Change the user of the process.
   // Do this before we start threads so we control the user id of the