This is an automated email from the ASF dual-hosted git repository.
jpeach 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 4b42ada Fix Lua metrics configuration reloading.
4b42ada is described below
commit 4b42adaf94fc7efce8e163989bed4fb32ef58e16
Author: James Peach <[email protected]>
AuthorDate: Wed Jun 7 22:42:26 2017 -0700
Fix Lua metrics configuration reloading.
The Lua metrics evaluator stored the chunk to be evaluated in the
registry. We carefully placed an assertion in the Evaluator destructor to
make sure that we cleaned up the registry, but we never actually did it.
Add an unbind() API to Evaluator and EvaluatorList to clean up the Lua
state registry entries, and add some simple unit tests to verify that
this works corrrectly.
This fixes issue #2091.
---
cmd/Makefile.am | 2 +
cmd/traffic_manager/Makefile.am | 13 ++++++
cmd/traffic_manager/metrics.cc | 58 ++++++++++++++++++-------
cmd/traffic_manager/metrics.h | 8 ++++
cmd/traffic_manager/test_metrics.cc | 78 ++++++++++++++++++++++++++++++++++
cmd/traffic_manager/traffic_manager.cc | 6 +--
6 files changed, 147 insertions(+), 18 deletions(-)
diff --git a/cmd/Makefile.am b/cmd/Makefile.am
index bd66e52..00a63b4 100644
--- a/cmd/Makefile.am
+++ b/cmd/Makefile.am
@@ -24,6 +24,8 @@ SUBDIRS = \
traffic_top \
traffic_via
+TESTS = $(check_PROGRAMS)
+
if BUILD_WCCP
SUBDIRS += traffic_wccp
diff --git a/cmd/traffic_manager/Makefile.am b/cmd/traffic_manager/Makefile.am
index 0aa4823..40342e2 100644
--- a/cmd/traffic_manager/Makefile.am
+++ b/cmd/traffic_manager/Makefile.am
@@ -16,6 +16,9 @@
# limitations under the License.
bin_PROGRAMS = traffic_manager
+check_PROGRAMS = test_metrics
+
+TESTS = $(check_PROGRAMS)
AM_CPPFLAGS += \
$(LUAJIT_CPPFLAGS) \
@@ -85,6 +88,16 @@ traffic_manager_LDADD += \
@OPENSSL_LIBS@
endif
+test_metrics_SOURCES = test_metrics.cc metrics.cc
+test_metrics_LDADD = \
+ $(top_builddir)/mgmt/libmgmt_lm.la \
+ $(top_builddir)/lib/records/librecords_lm.a \
+ $(top_builddir)/lib/bindings/libbindings.la \
+ $(top_builddir)/lib/luajit/src/libluajit.a \
+ $(top_builddir)/lib/ts/libtsutil.la \
+ $(top_builddir)/iocore/eventsystem/libinkevent.a \
+ @LIBTCL@ @LIBPCRE@
+
include $(top_srcdir)/build/tidy.mk
tidy-local: $(DIST_SOURCES)
diff --git a/cmd/traffic_manager/metrics.cc b/cmd/traffic_manager/metrics.cc
index f06d5a5..eda0ae7 100644
--- a/cmd/traffic_manager/metrics.cc
+++ b/cmd/traffic_manager/metrics.cc
@@ -36,12 +36,12 @@
#include "metrics.h"
struct Evaluator {
- Evaluator() : rec_name(nullptr), data_type(RECD_NULL), ref(-1) {}
- ~Evaluator()
- {
- ats_free(this->rec_name);
- ink_release_assert(this->ref == -1);
- }
+ Evaluator() : rec_name(nullptr), data_type(RECD_NULL), ref(LUA_NOREF) {}
+
+ ~Evaluator() { ink_release_assert(this->ref == LUA_NOREF); }
+
+ Evaluator(const Evaluator &) = delete;
+ Evaluator &operator=(const Evaluator &) = delete;
bool
bind(lua_State *L, const char *metric, const char *expression)
@@ -72,6 +72,20 @@ struct Evaluator {
}
void
+ unbind(lua_State *L)
+ {
+ if (this->ref != LUA_NOREF) {
+ luaL_unref(L, LUA_REGISTRYINDEX, this->ref);
+ }
+
+ ats_free(this->rec_name);
+
+ this->ref = LUA_NOREF;
+ this->rec_name = nullptr;
+ this->data_type = RECD_NULL;
+ }
+
+ void
eval(lua_State *L) const
{
// Push the stashed expression chunk onto the stack.
@@ -137,6 +151,9 @@ struct EvaluatorList {
}
}
+ EvaluatorList(const EvaluatorList &) = delete;
+ EvaluatorList &operator=(const EvaluatorList &) = delete;
+
void
push_back(Evaluator *e)
{
@@ -144,6 +161,14 @@ struct EvaluatorList {
}
void
+ unbind(lua_State *L) const
+ {
+ forv_Vec (Evaluator, e, this->evaluators) {
+ e->unbind(L);
+ }
+ }
+
+ void
evaluate(lua_State *L) const
{
ink_hrtime start = ink_get_hrtime_internal();
@@ -278,9 +303,6 @@ metrics_create_float(lua_State *L)
bool
metrics_binding_initialize(BindingInstance &binding)
{
- ats_scoped_str sysconfdir(RecConfigReadConfigDir());
- ats_scoped_str config(Layout::get()->relative_to(sysconfdir,
"metrics.config"));
-
if (!binding.construct()) {
mgmt_fatal(0, "failed to initialize Lua runtime\n");
}
@@ -300,12 +322,7 @@ metrics_binding_initialize(BindingInstance &binding)
// Stash a backpointer to the evaluators.
binding.attach_ptr("evaluators", new EvaluatorList());
- // Finally, execute the config file.
- if (binding.require(config.get())) {
- return true;
- }
-
- return false;
+ return true;
}
void
@@ -315,9 +332,20 @@ metrics_binding_destroy(BindingInstance &binding)
evaluators = (EvaluatorList *)binding.retrieve_ptr("evaluators");
binding.attach_ptr("evaluators", nullptr);
+
+ evaluators->unbind(binding.lua);
delete evaluators;
}
+bool
+metrics_binding_configure(BindingInstance &binding)
+{
+ ats_scoped_str sysconfdir(RecConfigReadConfigDir());
+ ats_scoped_str config(Layout::get()->relative_to(sysconfdir,
"metrics.config"));
+
+ return binding.require(config.get());
+}
+
void
metrics_binding_evaluate(BindingInstance &binding)
{
diff --git a/cmd/traffic_manager/metrics.h b/cmd/traffic_manager/metrics.h
index 4bc97ce..8a41551 100644
--- a/cmd/traffic_manager/metrics.h
+++ b/cmd/traffic_manager/metrics.h
@@ -24,8 +24,16 @@
#ifndef METRICS_H_D289E71B_AAC5_4CF3_9954_D54EDED60D1B
#define METRICS_H_D289E71B_AAC5_4CF3_9954_D54EDED60D1B
+#include "bindings/bindings.h"
+#include "bindings/metrics.h"
+
bool metrics_binding_initialize(BindingInstance &binding);
void metrics_binding_destroy(BindingInstance &binding);
+
+// Configure metrics from the metrics.config configuration file.
+bool metrics_binding_configure(BindingInstance &binding);
+
+// Evaluate the metrics in this binding instance.
void metrics_binding_evaluate(BindingInstance &binding);
#endif /* METRICS_H_D289E71B_AAC5_4CF3_9954_D54EDED60D1B */
diff --git a/cmd/traffic_manager/test_metrics.cc
b/cmd/traffic_manager/test_metrics.cc
new file mode 100644
index 0000000..52ca96e
--- /dev/null
+++ b/cmd/traffic_manager/test_metrics.cc
@@ -0,0 +1,78 @@
+/*
+
+ @section license License
+
+ Licensed to the Apache Software Foundation (ASF) under one
+ or more contributor license agreements. See the NOTICE file
+ distributed with this work for additional information
+ regarding copyright ownership. The ASF licenses this file
+ to you under the Apache License, Version 2.0 (the
+ "License"); you may not use this file except in compliance
+ with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+ Unless required by applicable law or agreed to in writing, software
+ distributed under the License is distributed on an "AS IS" BASIS,
+ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ See the License for the specific language governing permissions and
+ limitations under the License.
+*/
+
+#include "ts/Regression.h"
+#include "ts/TestBox.h"
+#include "ts/I_Layout.h"
+#include "LocalManager.h"
+#include "RecordsConfig.h"
+#include "P_RecLocal.h"
+#include "metrics.h"
+
+LocalManager *lmgmt = nullptr;
+
+// Check that we can load and delete metrics.
+REGRESSION_TEST(LoadMetrics)(RegressionTest *t, int /* atype ATS_UNUSED */,
int *pstatus)
+{
+ TestBox box(t, pstatus);
+
+ box = REGRESSION_TEST_PASSED;
+
+ BindingInstance binding;
+ box.check(metrics_binding_initialize(binding), "initialize metrics");
+ metrics_binding_destroy(binding);
+}
+
+// Check that we can set a value.
+REGRESSION_TEST(EvalMetrics)(RegressionTest *t, int /* atype ATS_UNUSED */,
int *pstatus)
+{
+ TestBox box(t, pstatus);
+
+ box = REGRESSION_TEST_PASSED;
+
+ const char *config = R"(
+integer 'proxy.node.test.value' [[
+ return 5
+]]
+ )";
+
+ BindingInstance binding;
+
+ box.check(metrics_binding_initialize(binding), "initialize metrics");
+ box.check(binding.eval(config), "load metrics config");
+
+ metrics_binding_evaluate(binding);
+
+ RecInt value = 0;
+ box.check(RecGetRecordInt("proxy.node.test.value", &value) == REC_ERR_OKAY,
"read value (5) from proxy.node.test.value");
+ box.check(value == 5, "proxy.node.test.value was %" PRId64 ", wanted 5",
value);
+
+ metrics_binding_destroy(binding);
+}
+
+int
+main(int argc, const char **argv)
+{
+ Layout::create();
+ RecLocalInit();
+ LibRecordsConfigInit();
+ return RegressionTest::main(argc, argv, REGRESSION_TEST_QUICK);
+}
diff --git a/cmd/traffic_manager/traffic_manager.cc
b/cmd/traffic_manager/traffic_manager.cc
index 3f5734c..34f6dd2 100644
--- a/cmd/traffic_manager/traffic_manager.cc
+++ b/cmd/traffic_manager/traffic_manager.cc
@@ -50,9 +50,6 @@
#include "P_RecLocal.h"
-#include "bindings/bindings.h"
-#include "bindings/metrics.h"
-
#include "metrics.h"
#if TS_USE_POSIX_CAP
@@ -679,6 +676,7 @@ main(int argc, const char **argv)
binding = new BindingInstance;
metrics_binding_initialize(*binding);
+ metrics_binding_configure(*binding);
int sleep_time = 0; // sleep_time given in sec
@@ -692,6 +690,8 @@ main(int argc, const char **argv)
binding = new BindingInstance;
metrics_binding_initialize(*binding);
+ metrics_binding_configure(*binding);
+
binding_version = metrics_version;
}
--
To stop receiving notification emails like this one, please contact
['"[email protected]" <[email protected]>'].
