You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@trafficserver.apache.org by zw...@apache.org on 2019/10/21 18:09:25 UTC
[trafficserver] 02/03: Rearrange config reload notifications
This is an automated email from the ASF dual-hosted git repository.
zwoop pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
commit a8bcd1c58cdf70e1fb810738a3ea76850e79baa0
Author: Gancho Tenev <ga...@apache.org>
AuthorDate: Tue Sep 3 13:19:40 2019 -0700
Rearrange config reload notifications
Renamed `TSRemapConfigReload` to `TSRemapPreConfigReload` to notify
all already loaded plugins that a configuration (re)load is about to
start.
Added new `TSRemapPostConfigReload(TSReturnCode)` to notify the plugins
that the configuration (re)load is done and provide status of its
success or failure.
(cherry picked from commit 70de21d9c72997564ee9ae549cac5407e0f48a98)
---
build/plugins.mk | 2 +-
doc/developer-guide/api/functions/TSRemap.en.rst | 14 ++-
doc/developer-guide/plugins/remap-plugins.en.rst | 79 ++++++++------
include/ts/remap.h | 22 +++-
proxy/ReverseProxy.cc | 2 +-
proxy/http/remap/Makefile.am | 2 +-
proxy/http/remap/PluginDso.h | 10 +-
proxy/http/remap/PluginFactory.cc | 75 ++++++++-----
proxy/http/remap/PluginFactory.h | 6 +-
proxy/http/remap/RemapConfig.cc | 15 ++-
proxy/http/remap/RemapPluginInfo.cc | 37 +++++--
proxy/http/remap/RemapPluginInfo.h | 27 +++--
.../http/remap/unit-tests/plugin_testing_calls.cc | 11 +-
.../http/remap/unit-tests/plugin_testing_common.h | 28 ++---
proxy/http/remap/unit-tests/test_PluginDso.cc | 6 +-
proxy/http/remap/unit-tests/test_PluginFactory.cc | 119 ++++++++++++++++-----
proxy/http/remap/unit-tests/test_RemapPlugin.cc | 9 +-
17 files changed, 320 insertions(+), 144 deletions(-)
diff --git a/build/plugins.mk b/build/plugins.mk
index 86d0a66..4556fc2 100644
--- a/build/plugins.mk
+++ b/build/plugins.mk
@@ -22,7 +22,7 @@ TS_PLUGIN_LD_FLAGS = \
-module \
-shared \
-avoid-version \
- -export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSPluginInit)$$'
+ -export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSPluginInit|TSRemapPreConfigReload|TSRemapPostConfigReload)$$'
TS_PLUGIN_CPPFLAGS = \
-I$(abs_top_builddir)/proxy/api \
diff --git a/doc/developer-guide/api/functions/TSRemap.en.rst b/doc/developer-guide/api/functions/TSRemap.en.rst
index ddd9917..bbfc220 100644
--- a/doc/developer-guide/api/functions/TSRemap.en.rst
+++ b/doc/developer-guide/api/functions/TSRemap.en.rst
@@ -30,7 +30,8 @@ Synopsis
`#include <ts/remap.h>`
.. function:: TSReturnCode TSRemapInit(TSRemapInterface * api_info, char * errbuff, int errbuff_size)
-.. function:: void TSRemapConfigReload(void)
+.. function:: void TSRemapPreConfigReload(void)
+.. function:: void TSRemapPostConfigReload(TSReturnCode reloadStatus)
.. function:: void TSRemapDone(void)
.. function:: TSRemapStatus TSRemapDoRemap(void * ih, TSHttpTxn rh, TSRemapRequestInfo * rri)
.. function:: TSReturnCode TSRemapNewInstance(int argc, char * argv[], void ** ih, char * errbuff, int errbuff_size)
@@ -65,9 +66,14 @@ any data or continuations associated with that instance.
entry point. In this function, the remap plugin may examine and modify
the HTTP transaction.
-:func:`TSRemapConfigReload` is called once for every remap plugin immediately after a new
-configuration is successfully loaded and immediately before the new remap configuration becomes
-active. This is an optional entry point, which takes no arguments and has no return value.
+:func:`TSRemapPreConfigReload` is called *before* the parsing of a new remap configuration starts
+to notify plugins of the coming configuration reload. It is called on all already loaded plugins,
+invoked by current and all previous still used configurations. This is an optional entry point.
+
+:func:`TSRemapPostConfigReload` is called to indicate the end of the the new remap configuration
+load. It is called on the newly and previously loaded plugins, invoked by the new, current and
+previous still used configurations. It also indicates if the configuration reload was successful
+by passing :macro:`TS_SUCCESS` or :macro:`TS_ERROR`. This is an optional entry point.
Generally speaking, calls to these functions are mutually exclusive. The exception
is for functions which take an HTTP transaction as a parameter. Calls to these
diff --git a/doc/developer-guide/plugins/remap-plugins.en.rst b/doc/developer-guide/plugins/remap-plugins.en.rst
index 7b691c6..ead70b2 100644
--- a/doc/developer-guide/plugins/remap-plugins.en.rst
+++ b/doc/developer-guide/plugins/remap-plugins.en.rst
@@ -80,12 +80,46 @@ multiple times for the rule, once for each invocation. Only the value store in :
available when the rule is actually matched. In particular the plugin arguments will not be
available.
-Calls to :func:`TSRemapNewInstance` are serialized.
+Calls to :func:`TSRemapNewInstance` are serialized. All calls to :func:`TSRemapNewInstance`
+for a given configuration will be called and completed before any calls to :func:`TSRemapDoRemap`.
If there is an error then the callback should return :macro:`TS_ERROR` and fill in the
:arg:`errbuff` with a C-string describing the error. Otherwise the function must return
:macro:`TS_SUCCESS`.
+
+Configuration reload notifications
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+:func:`TSRemapPreConfigReload` is called *before* the parsing of a new remap configuration starts
+to notify plugins of the coming configuration reload. It is called on all already loaded plugins,
+invoked by current and all previous still used configurations. This is an optional entry point.
+
+:func:`TSRemapPostConfigReload` is called to indicate the end of the the new remap configuration
+load. It is called on the newly and previously loaded plugins, invoked by the new, current and
+previous still used configurations. It also indicates if the configuration reload was successful
+by passing :macro:`TS_SUCCESS` or :macro:`TS_ERROR`. This is an optional entry point.
+
+These calls are called per *plugin*, not per invocation of the plugin in :file:`remap.config`
+and only will be called if the plugin was called at least once with :func:`TSRemapNewInstance`
+for any configuration and at least one configuration using it is still loaded.
+
+:func:`TSRemapPreConfigReload` will be called serially for all loaded plugins
+before any call to :func:`TSRemapNewInstance` during parsing of the new configuration.
+
+:func:`TSRemapPostConfigReload` will be called serially for all plugins after
+all calls to :func:`TSRemapNewInstance` during parsing of the new configuration.
+
+The intention of these callbacks can be demonstrated with the following use-case.
+A plugin could use :func:`TSRemapPreConfigReload` as a signal to drop (or allocate) temporary
+per plugin data structures. These structures can be created (or updated) as needed
+when a plugin invocation instance is loaded (:func:`TSRemapNewInstance` is called).
+Then it could be used in subsequent invocation instances loading. After the configuration
+reload is done :func:`TSRemapPostConfigReload` could be used to confirm
+the data structures update if :macro:`TS_SUCCESS` is passed or recover / clean-up
+after a failed reload attempt if :macro:`TS_ERROR` is passed.
+
+
Runtime
=======
@@ -104,8 +138,13 @@ Calls to :func:`TSRemapDoRemap` are not serialized, they can be concurrent, even
invocation instance. However, the callbacks for a single rule for a single transaction are
serialized in the order the plugins are invoked in the rule.
-All calls to :func:`TSRemapNewInstance` for a given configuration will be called and completed
-before any calls to :func:`TSRemapDoRemap`.
+No calls to :func:`TSRemapDoRemap` will occur before :func:`TSRemapPostConfigReload` for
+all plugin instances invoked by the new configuration.
+
+The old configurations, if any, are still active during the calls to :func:`TSRemapPreConfigReload`
+and :func:`TSRemapPreConfigReload` and therefore calls to :func:`TSRemapDoRemap` may occur
+concurrently with those functions.
+
Cleanup
=======
@@ -114,35 +153,9 @@ When a new :file:`remap.config` is loaded successfully, the prior configuration
each call to :func:`TSRemapNewInstance` a corresponding call to :func:`TSRemapDeleteInstance` is
called. The only argument is the invocation instance handle, originally provided by the plugin to
:func:`TSRemapNewInstance`. This is expected to suffice for the plugin to clean up any rule specific
-data.
-
-As part of the old configuration cleanup :func:`TSRemapConfigReload` is called on the plugins in the
-old configuration before any calls to :func:`TSRemapDeleteInstance`. This is an optional entry
-point.
-
-.. note::
-
- This is called per *plugin*, not per invocation of the plugin in :file:`remap.config`, and only
- called if the plugin was called at least once with :func:`TSRemapNewInstance` for that
- configuration.
-
-.. note::
-
- There is no explicit indication or connection between the call to :func:`TSRemapConfigReload` and
- the "old" (existing) configuration. It is guaranteeed that :func:`TSRemapConfigReload` will be
- called on all the plugins before any :func:`TSRemapDeleteInstance` and these calls will be
- serial. Similarly, :func:`TSRemapConfigReload` will be called serially after all calls to
- :func:`TSRemapNewInstance` for a given configuration.
-
-.. note::
-
- The old configuration, if any, is still active during the call to :func:`TSRemapConfigReload` and
- therefore calls to :func:`TSRemapDoRemap` may occur concurrently with that function.
-
-The intention of :func:`TSRemapConfigReload` is to provide for temporary data structures used only
-during configuration loading. These can be created as needed when an invocation instance is loaded
-and used in subsequent invocation instance loading, then cleaned up in :func:`TSRemapConfigReload`.
+data. Calls to :func:`TSRemapDeleteInstance` will be serial for all plugin invocations in a
+remap configuration.
If no rule uses a plugin, it may be unloaded. In that case :func:`TSRemapDone` is called. This is
-an optional entry point, a plugin is not required to provide it. It is called once per plugin, not
-per plugin invocation.
+an optional entry point, a plugin is not required to provide it. It corresponds to :func:`TSRemapInit`.
+It is called once per plugin, not per plugin invocation.
diff --git a/include/ts/remap.h b/include/ts/remap.h
index cc263fd..dd7be38 100644
--- a/include/ts/remap.h
+++ b/include/ts/remap.h
@@ -88,13 +88,27 @@ typedef enum {
*/
tsapi TSReturnCode TSRemapInit(TSRemapInterface *api_info, char *errbuf, int errbuf_size);
-/* This gets called every time remap.config is reloaded. This is complementary
- to TSRemapInit() which gets called when the plugin is first loaded. You can
- not fail, or cause reload to stop here, it's merely a notification.
+/* This gets called every time before remap.config is reloaded. This is complementary
+ to TSRemapInit() which gets called when the plugin is first loaded.
+ It is guaranteed to be called before TSRemapInit() and TSRemapNewInstance().
+ It cannot fail, or cause reload to stop here, it's merely a notification.
Optional function.
+ Params: none
Return: none
*/
-tsapi void TSRemapConfigReload(void);
+tsapi void TSRemapPreConfigReload(void);
+
+/* This gets called every time afterremap.config is reloaded. This is complementary
+ to TSRemapInit() which gets called when the plugin is first loaded.
+ It is guaranteed to be called after TSRemapInit() and TSRemapNewInstance().
+ It cannot fail, or cause reload to stop here, it's merely a notification that
+ the (re)load is done and provide a status of its success or failure..
+ Optional function.
+ Params: reloadStatus - TS_SUCCESS - (re)load was successful,
+ TS_ERROR - (re)load failed.
+ Return: none
+*/
+tsapi void TSRemapPostConfigReload(TSReturnCode reloadStatus);
/* Remap new request
Mandatory interface function.
diff --git a/proxy/ReverseProxy.cc b/proxy/ReverseProxy.cc
index d7d3ae0..2f5de1a 100644
--- a/proxy/ReverseProxy.cc
+++ b/proxy/ReverseProxy.cc
@@ -151,7 +151,7 @@ reloadUrlRewrite()
ink_assert(oldTable != nullptr);
// Release the old one
- oldTable->pluginFactory.indicateReload();
+ oldTable->pluginFactory.deactivate();
oldTable->release();
Debug("url_rewrite", "%s", msg);
diff --git a/proxy/http/remap/Makefile.am b/proxy/http/remap/Makefile.am
index ad705d3..8973d1b 100644
--- a/proxy/http/remap/Makefile.am
+++ b/proxy/http/remap/Makefile.am
@@ -103,7 +103,7 @@ DSO_LDFLAGS = \
-module \
-shared \
-avoid-version \
- -export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSRemapConfigReload|TSPluginInit|pluginDsoVersionTest|getPluginDebugObjectTest)$$'
+ -export-symbols-regex '^(TSRemapInit|TSRemapDone|TSRemapDoRemap|TSRemapNewInstance|TSRemapDeleteInstance|TSRemapOSResponse|TSRemapPreConfigReload|TSRemapPostConfigReload|TSPluginInit|pluginDsoVersionTest|getPluginDebugObjectTest)$$'
# Build plugins for unit testing the plugin (re)load.
pkglib_LTLIBRARIES = \
diff --git a/proxy/http/remap/PluginDso.h b/proxy/http/remap/PluginDso.h
index 4554c6b..23a84b5 100644
--- a/proxy/http/remap/PluginDso.h
+++ b/proxy/http/remap/PluginDso.h
@@ -33,6 +33,7 @@
#include <vector>
#include <ctime>
+#include "ts/apidefs.h"
#include "tscore/ts_file.h"
namespace fs = ts::file;
@@ -73,10 +74,11 @@ public:
using Linkage = ts::IntrusiveLinkage<self_type>;
using PluginList = ts::IntrusiveDList<PluginDso::Linkage>;
- /* Methods to be called when processing a list of plugins, to overloaded by the remap or the global plugins correspondingly */
- virtual void indicateReload() = 0;
- virtual bool init(std::string &error) = 0;
- virtual void done() = 0;
+ /* Methods to be called when processing a list of plugins, to be overloaded by the remap or the global plugins correspondingly */
+ virtual void indicatePreReload() = 0;
+ virtual void indicatePostReload(TSReturnCode reloadStatus) = 0;
+ virtual bool init(std::string &error) = 0;
+ virtual void done() = 0;
void acquire();
void release();
diff --git a/proxy/http/remap/PluginFactory.cc b/proxy/http/remap/PluginFactory.cc
index c6b1c8a..8b045cc 100644
--- a/proxy/http/remap/PluginFactory.cc
+++ b/proxy/http/remap/PluginFactory.cc
@@ -35,28 +35,34 @@
RemapPluginInst::RemapPluginInst(RemapPluginInfo &plugin) : _plugin(plugin)
{
_plugin.acquire();
- _plugin.incInstanceCount();
}
RemapPluginInst::~RemapPluginInst()
{
- _plugin.decInstanceCount();
_plugin.release();
}
-bool
-RemapPluginInst::init(int argc, char **argv, std::string &error)
+RemapPluginInst *
+RemapPluginInst::init(RemapPluginInfo *plugin, int argc, char **argv, std::string &error)
{
- bool result = false;
- result = _plugin.initInstance(argc, argv, &_instance, error);
-
- return result;
+ RemapPluginInst *inst = new RemapPluginInst(*plugin);
+ if (plugin->initInstance(argc, argv, &(inst->_instance), error)) {
+ plugin->incInstanceCount();
+ return inst;
+ }
+ delete inst;
+ return nullptr;
}
void
RemapPluginInst::done()
{
+ _plugin.decInstanceCount();
_plugin.doneInstance(_instance);
+
+ if (0 == _plugin.instanceCount()) {
+ _plugin.done();
+ }
}
TSRemapStatus
@@ -163,9 +169,10 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
_list.append(plugin);
if (plugin->init(error)) {
- inst = new RemapPluginInst(*plugin);
- inst->init(argc, argv, error);
- _instList.append(inst);
+ inst = RemapPluginInst::init(plugin, argc, argv, error);
+ if (nullptr != inst) {
+ _instList.append(inst);
+ }
}
if (_preventiveCleaning) {
@@ -177,9 +184,10 @@ PluginFactory::getRemapPlugin(const fs::path &configPath, int argc, char **argv,
}
} else {
Debug(_tag, "plugin '%s' has already been loaded", configPath.c_str());
- inst = new RemapPluginInst(*plugin);
- inst->init(argc, argv, error);
- _instList.append(inst);
+ inst = RemapPluginInst::init(plugin, argc, argv, error);
+ if (nullptr != inst) {
+ _instList.append(inst);
+ }
}
return inst;
@@ -236,25 +244,44 @@ PluginFactory::findByEffectivePath(const fs::path &path)
}
/**
- * @brief Tell all plugins (that so wish) that remap.config is being reloaded
+ * @brief Tell all plugins instantiated by this factory that the configuration
+ * they are using is no longer the active one.
*
* This method would be useful only in case configs are reloaded independently from
* factory/plugins instantiation and initialization.
*/
void
-PluginFactory::indicateReload()
+PluginFactory::deactivate()
{
- Debug(_tag, "indicated config reload to factory '%s'", getUuid());
+ Debug(_tag, "deactivate configuration used by factory '%s'", getUuid());
_instList.apply([](RemapPluginInst &pluginInst) -> void { pluginInst.done(); });
+}
- _list.apply([](PluginDso &plugin) -> void {
- if (1 == plugin.instanceCount()) {
- plugin.done();
- } else {
- plugin.indicateReload();
- }
- });
+/**
+ * @brief Tell all plugins (that so wish) that remap.config is going to be reloaded
+ */
+void
+PluginFactory::indicatePreReload()
+{
+ Debug(_tag, "indicated config is going to be reloaded by factory '%s' to %zu plugin%s", getUuid(), _list.count(),
+ _list.count() != 1 ? "s" : "");
+
+ _list.apply([](PluginDso &plugin) -> void { plugin.indicatePreReload(); });
+}
+
+/**
+ * @brief Tell all plugins (that so wish) that remap.config is done reloading
+ */
+void
+PluginFactory::indicatePostReload(TSReturnCode reloadStatus)
+{
+ Debug(_tag, "indicated config is done reloading by factory '%s' to %zu plugin%s", getUuid(), _list.count(),
+ _list.count() != 1 ? "s" : "");
+
+ for (auto it = _list.begin(); _list.end() != it; it++) {
+ it->indicatePostReload(reloadStatus);
+ }
}
void
diff --git a/proxy/http/remap/PluginFactory.h b/proxy/http/remap/PluginFactory.h
index 1cb0661..db722bd 100644
--- a/proxy/http/remap/PluginFactory.h
+++ b/proxy/http/remap/PluginFactory.h
@@ -47,7 +47,7 @@ public:
~RemapPluginInst();
/* Used by the PluginFactory */
- bool init(int argc, char **argv, std::string &error);
+ static RemapPluginInst *init(RemapPluginInfo *plugin, int argc, char **argv, std::string &error);
void done();
/* Used by the traffic server core while processing requests */
@@ -100,7 +100,9 @@ public:
virtual const char *getUuid();
void clean(std::string &error);
- void indicateReload();
+ void deactivate();
+ void indicatePreReload();
+ void indicatePostReload(TSReturnCode reloadStatus);
protected:
PluginDso *findByEffectivePath(const fs::path &path);
diff --git a/proxy/http/remap/RemapConfig.cc b/proxy/http/remap/RemapConfig.cc
index e231c09..5774b18 100644
--- a/proxy/http/remap/RemapConfig.cc
+++ b/proxy/http/remap/RemapConfig.cc
@@ -1320,9 +1320,16 @@ remap_parse_config(const char *path, UrlRewrite *rewrite)
{
BUILD_TABLE_INFO bti;
- // If this happens to be a config reload, the list of loaded remap plugins is non-empty, and we
- // can signal all these plugins that a reload has begun.
- rewrite->pluginFactory.indicateReload();
+ /* If this happens to be a config reload, the list of loaded remap plugins is non-empty, and we
+ * can signal all these plugins that a reload has begun. */
+ rewrite->pluginFactory.indicatePreReload();
+
bti.rewrite = rewrite;
- return remap_parse_config_bti(path, &bti);
+ bool status = remap_parse_config_bti(path, &bti);
+
+ /* Now after we parsed the configuration and (re)loaded plugins and plugin instances
+ * accordingly notify all plugins that we are done */
+ rewrite->pluginFactory.indicatePostReload(status ? TS_SUCCESS : TS_ERROR);
+
+ return status;
}
diff --git a/proxy/http/remap/RemapPluginInfo.cc b/proxy/http/remap/RemapPluginInfo.cc
index 8a1c00e..a98c3ee 100644
--- a/proxy/http/remap/RemapPluginInfo.cc
+++ b/proxy/http/remap/RemapPluginInfo.cc
@@ -52,7 +52,9 @@ RemapPluginInfo::getFunctionSymbol(const char *symbol)
{
std::string error; /* ignore the error, return nullptr if symbol not defined */
void *address = nullptr;
- getSymbol(symbol, address, error);
+ if (getSymbol(symbol, address, error)) {
+ Debug(_tag, "plugin '%s' found symbol '%s'", _configPath.c_str(), symbol);
+ }
return reinterpret_cast<T *>(address);
}
@@ -81,13 +83,14 @@ RemapPluginInfo::load(std::string &error)
return false;
}
- init_cb = getFunctionSymbol<Init_F>(TSREMAP_FUNCNAME_INIT);
- config_reload_cb = getFunctionSymbol<Reload_F>(TSREMAP_FUNCNAME_CONFIG_RELOAD);
- done_cb = getFunctionSymbol<Done_F>(TSREMAP_FUNCNAME_DONE);
- new_instance_cb = getFunctionSymbol<New_Instance_F>(TSREMAP_FUNCNAME_NEW_INSTANCE);
- delete_instance_cb = getFunctionSymbol<Delete_Instance_F>(TSREMAP_FUNCNAME_DELETE_INSTANCE);
- do_remap_cb = getFunctionSymbol<Do_Remap_F>(TSREMAP_FUNCNAME_DO_REMAP);
- os_response_cb = getFunctionSymbol<OS_Response_F>(TSREMAP_FUNCNAME_OS_RESPONSE);
+ init_cb = getFunctionSymbol<Init_F>(TSREMAP_FUNCNAME_INIT);
+ pre_config_reload_cb = getFunctionSymbol<PreReload_F>(TSREMAP_FUNCNAME_PRE_CONFIG_RELOAD);
+ post_config_reload_cb = getFunctionSymbol<PostReload_F>(TSREMAP_FUNCNAME_POST_CONFIG_RELOAD);
+ done_cb = getFunctionSymbol<Done_F>(TSREMAP_FUNCNAME_DONE);
+ new_instance_cb = getFunctionSymbol<New_Instance_F>(TSREMAP_FUNCNAME_NEW_INSTANCE);
+ delete_instance_cb = getFunctionSymbol<Delete_Instance_F>(TSREMAP_FUNCNAME_DELETE_INSTANCE);
+ do_remap_cb = getFunctionSymbol<Do_Remap_F>(TSREMAP_FUNCNAME_DO_REMAP);
+ os_response_cb = getFunctionSymbol<OS_Response_F>(TSREMAP_FUNCNAME_OS_RESPONSE);
/* Validate if the callback TSREMAP functions are specified correctly in the plugin. */
bool valid = true;
@@ -243,12 +246,24 @@ RemapPluginInfo::osResponse(void *ih, TSHttpTxn rh, int os_response_type)
RemapPluginInfo::~RemapPluginInfo() {}
void
-RemapPluginInfo::indicateReload()
+RemapPluginInfo::indicatePreReload()
+{
+ setPluginContext();
+
+ if (pre_config_reload_cb) {
+ pre_config_reload_cb();
+ }
+
+ resetPluginContext();
+}
+
+void
+RemapPluginInfo::indicatePostReload(TSReturnCode reloadStatus)
{
setPluginContext();
- if (config_reload_cb) {
- config_reload_cb();
+ if (post_config_reload_cb) {
+ post_config_reload_cb(reloadStatus);
}
resetPluginContext();
diff --git a/proxy/http/remap/RemapPluginInfo.h b/proxy/http/remap/RemapPluginInfo.h
index cc5941d..731196a 100644
--- a/proxy/http/remap/RemapPluginInfo.h
+++ b/proxy/http/remap/RemapPluginInfo.h
@@ -36,13 +36,14 @@ class url_mapping;
extern thread_local PluginThreadContext *pluginThreadContext;
-static constexpr const char *const TSREMAP_FUNCNAME_INIT = "TSRemapInit";
-static constexpr const char *const TSREMAP_FUNCNAME_CONFIG_RELOAD = "TSRemapConfigReload";
-static constexpr const char *const TSREMAP_FUNCNAME_DONE = "TSRemapDone";
-static constexpr const char *const TSREMAP_FUNCNAME_NEW_INSTANCE = "TSRemapNewInstance";
-static constexpr const char *const TSREMAP_FUNCNAME_DELETE_INSTANCE = "TSRemapDeleteInstance";
-static constexpr const char *const TSREMAP_FUNCNAME_DO_REMAP = "TSRemapDoRemap";
-static constexpr const char *const TSREMAP_FUNCNAME_OS_RESPONSE = "TSRemapOSResponse";
+static constexpr const char *const TSREMAP_FUNCNAME_INIT = "TSRemapInit";
+static constexpr const char *const TSREMAP_FUNCNAME_PRE_CONFIG_RELOAD = "TSRemapPreConfigReload";
+static constexpr const char *const TSREMAP_FUNCNAME_POST_CONFIG_RELOAD = "TSRemapPostConfigReload";
+static constexpr const char *const TSREMAP_FUNCNAME_DONE = "TSRemapDone";
+static constexpr const char *const TSREMAP_FUNCNAME_NEW_INSTANCE = "TSRemapNewInstance";
+static constexpr const char *const TSREMAP_FUNCNAME_DELETE_INSTANCE = "TSRemapDeleteInstance";
+static constexpr const char *const TSREMAP_FUNCNAME_DO_REMAP = "TSRemapDoRemap";
+static constexpr const char *const TSREMAP_FUNCNAME_OS_RESPONSE = "TSRemapOSResponse";
/**
* Holds information for a remap plugin, remap specific callback entry points for plugin init/done and instance init/done, do_remap,
@@ -53,8 +54,10 @@ class RemapPluginInfo : public PluginDso
public:
/// Initialization function, called on library load.
using Init_F = TSReturnCode(TSRemapInterface *api_info, char *errbuf, int errbuf_size);
- /// Reload function, called to inform the plugin of a configuration reload.
- using Reload_F = void();
+ /// Reload function, called to inform the plugin that configuration is going to be reloaded.
+ using PreReload_F = void();
+ /// Reload function, called to inform the plugin that configuration is done reloading.
+ using PostReload_F = void(TSReturnCode);
/// Called when remapping for a transaction has finished.
using Done_F = void();
/// Create an rule instance.
@@ -68,7 +71,8 @@ public:
void *dl_handle = nullptr; /* "handle" for the dynamic library */
Init_F *init_cb = nullptr;
- Reload_F *config_reload_cb = nullptr;
+ PreReload_F *pre_config_reload_cb = nullptr;
+ PostReload_F *post_config_reload_cb = nullptr;
Done_F *done_cb = nullptr;
New_Instance_F *new_instance_cb = nullptr;
Delete_Instance_F *delete_instance_cb = nullptr;
@@ -94,7 +98,8 @@ public:
void osResponse(void *ih, TSHttpTxn rh, int os_response_type);
/* Used by traffic server core to indicate configuration reload */
- virtual void indicateReload();
+ virtual void indicatePreReload();
+ virtual void indicatePostReload(TSReturnCode reloadStatus);
protected:
/* Utility to be used only with unit testing */
diff --git a/proxy/http/remap/unit-tests/plugin_testing_calls.cc b/proxy/http/remap/unit-tests/plugin_testing_calls.cc
index 89c8df2..1d57da3 100644
--- a/proxy/http/remap/unit-tests/plugin_testing_calls.cc
+++ b/proxy/http/remap/unit-tests/plugin_testing_calls.cc
@@ -103,9 +103,16 @@ TSRemapOSResponse(void *ih, TSHttpTxn rh, int os_response_type)
}
void
-TSRemapConfigReload(void)
+TSRemapPreConfigReload(void)
{
- debugObject.reloadConfigCalled++;
+ debugObject.preReloadConfigCalled++;
+}
+
+void
+TSRemapPostConfigReload(TSReturnCode reloadStatus)
+{
+ debugObject.postReloadConfigCalled++;
+ debugObject.postReloadConfigSuccess = (TS_SUCCESS == reloadStatus);
}
/* The folowing functions are meant for unit testing */
diff --git a/proxy/http/remap/unit-tests/plugin_testing_common.h b/proxy/http/remap/unit-tests/plugin_testing_common.h
index 66a8dd6..3662d0c 100644
--- a/proxy/http/remap/unit-tests/plugin_testing_common.h
+++ b/proxy/http/remap/unit-tests/plugin_testing_common.h
@@ -51,17 +51,19 @@ public:
void
clear()
{
- contextInit = nullptr;
- contextInitInstance = nullptr;
- doRemapCalled = 0;
- initCalled = 0;
- doneCalled = 0;
- initInstanceCalled = 0;
- deleteInstanceCalled = 0;
- reloadConfigCalled = 0;
- ih = nullptr;
- argc = 0;
- argv = nullptr;
+ contextInit = nullptr;
+ contextInitInstance = nullptr;
+ doRemapCalled = 0;
+ initCalled = 0;
+ doneCalled = 0;
+ initInstanceCalled = 0;
+ deleteInstanceCalled = 0;
+ preReloadConfigCalled = 0;
+ postReloadConfigCalled = 0;
+ postReloadConfigSuccess = 0;
+ ih = nullptr;
+ argc = 0;
+ argv = nullptr;
}
/* Input fields used to set the test behavior of the plugin call-backs */
@@ -76,7 +78,9 @@ public:
int doneCalled = 0; /* mark if done was called */
int initInstanceCalled = 0; /* mark if instance init was called */
int deleteInstanceCalled = 0; /* mark if delete instance was called */
- int reloadConfigCalled = 0; /* mark if reload config was called */
+ int preReloadConfigCalled = 0; /* mark if pre-reload config was called */
+ int postReloadConfigCalled = 0; /* mark if post-reload config was called */
+ bool postReloadConfigSuccess = 0; /* mark if plugin reload status is passed correctly */
void *ih = nullptr; /* instance handler */
int argc = 0; /* number of plugin instance parameters received by the plugin */
char **argv = nullptr; /* plugin instance parameters received by the plugin */
diff --git a/proxy/http/remap/unit-tests/test_PluginDso.cc b/proxy/http/remap/unit-tests/test_PluginDso.cc
index 092261b..1635a65 100644
--- a/proxy/http/remap/unit-tests/test_PluginDso.cc
+++ b/proxy/http/remap/unit-tests/test_PluginDso.cc
@@ -69,7 +69,11 @@ public:
}
virtual void
- indicateReload()
+ indicatePreReload()
+ {
+ }
+ virtual void
+ indicatePostReload(TSReturnCode reloadStatus)
{
}
virtual bool
diff --git a/proxy/http/remap/unit-tests/test_PluginFactory.cc b/proxy/http/remap/unit-tests/test_PluginFactory.cc
index 9c2d819..1c75bbe 100644
--- a/proxy/http/remap/unit-tests/test_PluginFactory.cc
+++ b/proxy/http/remap/unit-tests/test_PluginFactory.cc
@@ -554,7 +554,7 @@ SCENARIO("notifying plugins of config reload", "[plugin][core]")
GIVEN("simple configuration with 1 plugin and 1 factory")
{
- WHEN("indicating config reload")
+ WHEN("(1) signal pre-new-config-load, (2) signal post-new-config-load, (3) old-config deactivate")
{
/* Simulate configuration without plugins - an unused factory */
setupConfigPathTest(configName1, buildPath, uuid_t1, effectivePath1, runtimePath1, 1556825556);
@@ -566,17 +566,43 @@ SCENARIO("notifying plugins of config reload", "[plugin][core]")
/* Prapare the debug object */
PluginDebugObject *debugObject = getDebugObject(plugin1->_plugin);
- debugObject->clear();
- THEN("expect 'done' methods to be called for plugin and the instance but not the 'reload config' methods")
+ THEN("expect pre-, post- and done/delete_instance to be called accordingly ")
{
- /* Simulate reloading the config */
- factory1->indicateReload();
-
- /* was "done" method called? */
- CHECK(1 == debugObject->doneCalled);
+ /* Signal before loading the config */
+ debugObject->clear();
+
+ factory1->indicatePreReload();
+ CHECK(0 == debugObject->deleteInstanceCalled);
+ CHECK(0 == debugObject->doneCalled);
+ CHECK(1 == debugObject->preReloadConfigCalled);
+
+ /* ... parse the new remap config ... */
+
+ /* Assume (re)load was done OK and test */
+ debugObject->clear();
+ factory1->indicatePostReload(TS_SUCCESS);
+ CHECK(0 == debugObject->deleteInstanceCalled);
+ CHECK(0 == debugObject->doneCalled);
+ CHECK(1 == debugObject->postReloadConfigCalled);
+ CHECK(true == debugObject->postReloadConfigSuccess);
+
+ /* Assume (re)load failed and test */
+ debugObject->clear();
+ factory1->indicatePostReload(TS_ERROR);
+ CHECK(0 == debugObject->deleteInstanceCalled);
+ CHECK(0 == debugObject->doneCalled);
+ CHECK(1 == debugObject->postReloadConfigCalled);
+ CHECK(false == debugObject->postReloadConfigSuccess);
+
+ /* ... swap the new and the old config ... */
+
+ /* Signal de-activation of the old config */
+ debugObject->clear();
+ factory1->deactivate();
CHECK(1 == debugObject->deleteInstanceCalled);
- CHECK(0 == debugObject->reloadConfigCalled);
+ CHECK(1 == debugObject->doneCalled);
+ CHECK(0 == debugObject->preReloadConfigCalled);
}
teardownConfigPathTest(factory1);
@@ -585,7 +611,7 @@ SCENARIO("notifying plugins of config reload", "[plugin][core]")
GIVEN("configuration with 2 plugins loaded by 1 factory")
{
- WHEN("indicating config reload")
+ WHEN("(1) signal pre-new-config-load, (2) signal post-new-config-load, (3) old-config deactivate")
{
/* Simulate configuration without plugins - an unused factory */
setupConfigPathTest(configName1, buildPath, uuid_t1, effectivePath1, runtimePath1, 1556825556);
@@ -601,21 +627,60 @@ SCENARIO("notifying plugins of config reload", "[plugin][core]")
/* Prapare the debug objects */
PluginDebugObject *debugObject1 = getDebugObject(plugin1->_plugin);
PluginDebugObject *debugObject2 = getDebugObject(plugin2->_plugin);
- debugObject1->clear();
- debugObject2->clear();
- THEN("expect 'done' methods to be called but not the 'reload config' methods")
+ THEN("expect pre-, post- and done/delete_instance to be called accordingly")
{
- /* Simulate reloading the config */
- factory1->indicateReload();
+ /* Signal before loading the config */
+ debugObject1->clear();
+ debugObject2->clear();
+ factory1->indicatePreReload();
+ CHECK(0 == debugObject1->deleteInstanceCalled);
+ CHECK(0 == debugObject1->doneCalled);
+ CHECK(1 == debugObject1->preReloadConfigCalled);
+ CHECK(0 == debugObject2->doneCalled);
+ CHECK(0 == debugObject2->deleteInstanceCalled);
+ CHECK(1 == debugObject2->preReloadConfigCalled);
- /* Was "done" method called? */
- CHECK(1 == debugObject1->doneCalled);
+ /* ... parse the new remap config ... */
+
+ /* Assume (re)load was done OK */
+ debugObject1->clear();
+ debugObject2->clear();
+ factory1->indicatePostReload(TS_SUCCESS);
+ CHECK(0 == debugObject1->deleteInstanceCalled);
+ CHECK(0 == debugObject1->doneCalled);
+ CHECK(1 == debugObject1->postReloadConfigCalled);
+ CHECK(true == debugObject1->postReloadConfigSuccess);
+ CHECK(0 == debugObject2->deleteInstanceCalled);
+ CHECK(0 == debugObject2->doneCalled);
+ CHECK(1 == debugObject2->postReloadConfigCalled);
+ CHECK(true == debugObject2->postReloadConfigSuccess);
+
+ /* Assume (re)load failed */
+ debugObject1->clear();
+ debugObject2->clear();
+ factory1->indicatePostReload(TS_ERROR);
+ CHECK(0 == debugObject1->deleteInstanceCalled);
+ CHECK(0 == debugObject1->doneCalled);
+ CHECK(1 == debugObject1->postReloadConfigCalled);
+ CHECK(false == debugObject1->postReloadConfigSuccess);
+ CHECK(0 == debugObject2->deleteInstanceCalled);
+ CHECK(0 == debugObject2->doneCalled);
+ CHECK(1 == debugObject2->postReloadConfigCalled);
+ CHECK(false == debugObject2->postReloadConfigSuccess);
+
+ /* ... swap the new and the old config ... */
+
+ /* Signal de-activation of the old config */
+ debugObject1->clear();
+ debugObject2->clear();
+ factory1->deactivate();
CHECK(1 == debugObject1->deleteInstanceCalled);
- CHECK(0 == debugObject1->reloadConfigCalled);
- CHECK(1 == debugObject2->doneCalled);
+ CHECK(1 == debugObject1->doneCalled);
+ CHECK(0 == debugObject1->preReloadConfigCalled);
CHECK(1 == debugObject2->deleteInstanceCalled);
- CHECK(0 == debugObject2->reloadConfigCalled);
+ CHECK(1 == debugObject2->doneCalled);
+ CHECK(0 == debugObject2->preReloadConfigCalled);
}
teardownConfigPathTest(factory1);
@@ -624,7 +689,7 @@ SCENARIO("notifying plugins of config reload", "[plugin][core]")
GIVEN("configuration with 1 plugin loaded by 2 separate factories")
{
- WHEN("indicating config reload")
+ WHEN("indicating de-activation of the factories")
{
/* Simulate configuration without plugins - an unused factory */
setupConfigPathTest(configName1, buildPath, uuid_t1, effectivePath1, runtimePath1, 1556825556);
@@ -633,25 +698,25 @@ SCENARIO("notifying plugins of config reload", "[plugin][core]")
RemapPluginInst *plugin1 = factory1->getRemapPlugin(configName1, 0, nullptr, error);
RemapPluginInst *plugin2 = factory2->getRemapPlugin(configName1, 0, nullptr, error);
- /* Prapare the debug objects */
+ /* Prepare the debug objects */
PluginDebugObject *debugObject1 = getDebugObject(plugin1->_plugin);
PluginDebugObject *debugObject2 = getDebugObject(plugin2->_plugin);
- THEN("expect instance 'done' to be always called, but plugin 'done' called only after destroying one factory")
+ THEN("expect instance 'done' to be always called, but plugin 'done' called only after destroying both factories")
{
debugObject2->clear();
- factory2->indicateReload();
+ factory2->deactivate();
CHECK(0 == debugObject2->doneCalled);
CHECK(1 == debugObject2->deleteInstanceCalled);
- CHECK(1 == debugObject2->reloadConfigCalled);
+ CHECK(0 == debugObject2->preReloadConfigCalled);
delete factory2;
debugObject1->clear();
- factory1->indicateReload();
+ factory1->deactivate();
CHECK(1 == debugObject1->doneCalled);
CHECK(1 == debugObject1->deleteInstanceCalled);
- CHECK(0 == debugObject1->reloadConfigCalled);
+ CHECK(0 == debugObject1->preReloadConfigCalled);
delete factory1;
}
diff --git a/proxy/http/remap/unit-tests/test_RemapPlugin.cc b/proxy/http/remap/unit-tests/test_RemapPlugin.cc
index 1b385cc..49be30f 100644
--- a/proxy/http/remap/unit-tests/test_RemapPlugin.cc
+++ b/proxy/http/remap/unit-tests/test_RemapPlugin.cc
@@ -426,9 +426,14 @@ SCENARIO("config reload", "[plugin][core]")
{
debugObject->clear();
- plugin->indicateReload();
+ plugin->indicatePreReload();
+ plugin->indicatePostReload(TS_SUCCESS);
- THEN("expect it to run") { CHECK(1 == debugObject->reloadConfigCalled); }
+ THEN("expect it to run")
+ {
+ CHECK(1 == debugObject->preReloadConfigCalled);
+ CHECK(1 == debugObject->postReloadConfigCalled);
+ }
cleanupSandBox(plugin);
}
}