From 5f568cd606d649d100e99436ace2dd524c8f364b Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Tue, 8 Oct 2024 14:07:26 -0600 Subject: [PATCH 1/4] Defer deletion of reloadable remap plugins --- cmake/layout.cmake | 2 +- include/proxy/http/remap/PluginDso.h | 8 +--- include/proxy/http/remap/PluginFactory.h | 4 +- src/proxy/http/remap/PluginDso.cc | 6 +-- src/proxy/http/remap/PluginFactory.cc | 39 ++++++++++++++----- .../http/remap/unit-tests/test_PluginDso.cc | 2 - .../remap/unit-tests/test_PluginFactory.cc | 6 +-- src/traffic_server/traffic_server.cc | 4 +- 8 files changed, 39 insertions(+), 32 deletions(-) diff --git a/cmake/layout.cmake b/cmake/layout.cmake index 69a7b239705..72dcbf0fd7c 100644 --- a/cmake/layout.cmake +++ b/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 diff --git a/include/proxy/http/remap/PluginDso.h b/include/proxy/http/remap/PluginDso.h index e7c84e97c9e..d3ea8087a2b 100644 --- a/include/proxy/http/remap/PluginDso.h +++ b/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 - _plugins; /** @brief a global list of plugins, usually maintained by a plugin factory or plugin instance itself */ - std::atomic _instanceCount{ - 0}; /** @brief used for properly calling "done" and "indicate config reload" methods by the factory */ + static Ptr _plugins; /** @brief a global list of plugins */ + std::atomic _instanceCount{0}; /** @brief used for properly calling "done" and "indicate config reload" */ }; inline const DbgCtl & diff --git a/include/proxy/http/remap/PluginFactory.h b/include/proxy/http/remap/PluginFactory.h index 9007c80c9cf..fd08c00000f 100644 --- a/include/proxy/http/remap/PluginFactory.h +++ b/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(); diff --git a/src/proxy/http/remap/PluginDso.cc b/src/proxy/http/remap/PluginDso.cc index 6c23823687d..cd0499d8e60 100644 --- a/src/proxy/http/remap/PluginDso.cc +++ b/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()); diff --git a/src/proxy/http/remap/PluginFactory.cc b/src/proxy/http/remap/PluginFactory.cc index 3f13543a71d..c1d0c726562 100644 --- a/src/proxy/http/remap/PluginFactory.cc +++ b/src/proxy/http/remap/PluginFactory.cc @@ -36,6 +36,7 @@ #include "../../../iocore/eventsystem/P_EventSystem.h" #include /* std::swap */ +#include RemapPluginInst::RemapPluginInst(RemapPluginInfo &plugin) : _plugin(plugin) { @@ -102,7 +103,14 @@ PluginFactory::~PluginFactory() _instList.apply([](RemapPluginInst *pluginInst) -> void { delete pluginInst; }); _instList.clear(); - fs::remove_all(_runtimeDir, _ec); + { + 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); + } PluginDbg(_dbg_ctl(), "destroyed plugin factory %s", getUuid()); delete _uuid; @@ -226,9 +234,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 +281,26 @@ PluginFactory::getEffectivePath(const fs::path &configPath) return path; } +void +PluginFactory::cleanup() +{ + std::error_code ec; + auto path = RecConfigReadRuntimeDir(); + + 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); + } + } + } + } +} + /** * @brief Find a plugin by path from our linked plugin list by using plugin effective (canonical) path * @@ -326,9 +351,3 @@ PluginFactory::indicatePostReload(bool reloadSuccessful) PluginDso::loadedPlugins()->indicatePostReload(reloadSuccessful, pluginUsed, getUuid()); } - -void -PluginFactory::clean(std::string & /* error ATS_UNUSED */) -{ - fs::remove_all(_runtimeDir, _ec); -} diff --git a/src/proxy/http/remap/unit-tests/test_PluginDso.cc b/src/proxy/http/remap/unit-tests/test_PluginDso.cc index 72eb31f33ef..e3d40714942 100644 --- a/src/proxy/http/remap/unit-tests/test_PluginDso.cc +++ b/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 diff --git a/src/proxy/http/remap/unit-tests/test_PluginFactory.cc b/src/proxy/http/remap/unit-tests/test_PluginFactory.cc index 9053d9668f0..1d233245335 100644 --- a/src/proxy/http/remap/unit-tests/test_PluginFactory.cc +++ b/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 * diff --git a/src/traffic_server/traffic_server.cc b/src/traffic_server/traffic_server.cc index 31a4493c852..ad4492c60d8 100644 --- a/src/traffic_server/traffic_server.cc +++ b/src/traffic_server/traffic_server.cc @@ -1767,7 +1767,6 @@ configure_io_uring() // // Main // - int main(int /* argc ATS_UNUSED */, const char **argv) { @@ -1907,6 +1906,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 From 3147d3455c8e9babb0eb285400a1fbb30fc29037 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Tue, 8 Oct 2024 14:21:07 -0600 Subject: [PATCH 2/4] Fix tests, since plugins now remains --- src/proxy/http/remap/unit-tests/test_PluginFactory.cc | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/proxy/http/remap/unit-tests/test_PluginFactory.cc b/src/proxy/http/remap/unit-tests/test_PluginFactory.cc index 1d233245335..6997da4d348 100644 --- a/src/proxy/http/remap/unit-tests/test_PluginFactory.cc +++ b/src/proxy/http/remap/unit-tests/test_PluginFactory.cc @@ -301,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); @@ -602,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 From eff0ac9796a359865a965ea1f24d110b4a5b6296 Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Tue, 8 Oct 2024 22:10:35 -0600 Subject: [PATCH 3/4] Put the startup cleanup into a try-catch --- src/proxy/http/remap/PluginFactory.cc | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/src/proxy/http/remap/PluginFactory.cc b/src/proxy/http/remap/PluginFactory.cc index c1d0c726562..3dc2c0355c9 100644 --- a/src/proxy/http/remap/PluginFactory.cc +++ b/src/proxy/http/remap/PluginFactory.cc @@ -287,17 +287,21 @@ PluginFactory::cleanup() std::error_code ec; auto path = RecConfigReadRuntimeDir(); - 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); + 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()); } } From f4030ff1c3d6a22fa069a067272ca81e53a66f5f Mon Sep 17 00:00:00 2001 From: Leif Hedstrom Date: Wed, 9 Oct 2024 10:37:16 -0600 Subject: [PATCH 4/4] Make autest happier during shutdown --- src/proxy/http/remap/PluginFactory.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/proxy/http/remap/PluginFactory.cc b/src/proxy/http/remap/PluginFactory.cc index 3dc2c0355c9..c5217cee1aa 100644 --- a/src/proxy/http/remap/PluginFactory.cc +++ b/src/proxy/http/remap/PluginFactory.cc @@ -103,13 +103,15 @@ PluginFactory::~PluginFactory() _instList.apply([](RemapPluginInst *pluginInst) -> void { delete pluginInst; }); _instList.clear(); - { + 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()); @@ -285,7 +287,7 @@ void PluginFactory::cleanup() { std::error_code ec; - auto path = RecConfigReadRuntimeDir(); + std::string path(RecConfigReadRuntimeDir()); try { if (path.starts_with("/") && std::filesystem::is_directory(path)) {