This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 71293ea3dd Defer deletion of reloadable remap plugins (#11813)
71293ea3dd is described below
commit 71293ea3dd5fd6431918f7ea6025ecdcb628c46e
Author: Leif Hedstrom <[email protected]>
AuthorDate: Tue Oct 15 13:56:44 2024 -0600
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
---
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 | 45 +++++++++++++++++-----
src/proxy/http/remap/unit-tests/test_PluginDso.cc | 2 -
.../http/remap/unit-tests/test_PluginFactory.cc | 11 +-----
src/traffic_server/traffic_server.cc | 4 +-
8 files changed, 45 insertions(+), 37 deletions(-)
diff --git a/cmake/layout.cmake b/cmake/layout.cmake
index 69a7b23970..72dcbf0fd7 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 e7c84e97c9..d3ea8087a2 100644
--- a/include/proxy/http/remap/PluginDso.h
+++ b/include/proxy/http/remap/PluginDso.h
@@ -170,12 +170,8 @@ protected:
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 &
diff --git a/include/proxy/http/remap/PluginFactory.h
b/include/proxy/http/remap/PluginFactory.h
index 9007c80c9c..fd08c00000 100644
--- a/include/proxy/http/remap/PluginFactory.h
+++ b/include/proxy/http/remap/PluginFactory.h
@@ -99,12 +99,13 @@ public:
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 @@ protected:
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 6c23823687..cd0499d8e6 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 3f13543a71..c5217cee1a 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 <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);
-}
diff --git a/src/proxy/http/remap/unit-tests/test_PluginDso.cc
b/src/proxy/http/remap/unit-tests/test_PluginDso.cc
index 72eb31f33e..e3d4071494 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 @@ public:
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 9053d9668f..6997da4d34 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 *
@@ -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
diff --git a/src/traffic_server/traffic_server.cc
b/src/traffic_server/traffic_server.cc
index 0a7d8e0634..f00e365cc1 100644
--- a/src/traffic_server/traffic_server.cc
+++ b/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
