On 06/09/2021 11:46 AM, Miroslav Zagorac wrote:
On 06/09/2021 09:10 AM, Willy Tarreau wrote:
Hi Miroslav,
On Mon, Jun 07, 2021 at 04:55:21PM +0200, Miroslav Zagorac wrote:
...
In fact, the only access to these files is achieved only once at the
beginning of the HAProxy process, in the initialization of threads.
After this initialization, no access to the file system is performed.
This resolves GitHub issue #1274.
...
Please try again to have a real initialization phase in the post_check
or wherever suits you (we can even add another hook if you need a very
special place, it's not a problem), but this thing needs to be
initialized
and to have its files loaded before chrooting. And if you still face any
issue doing that, we can discuss it to figure how to address it, but I
don't want us to paper over problems using methods that have short-term
nor long-term implications on the users. And this one definitely has.
Hello Willy,
yes, I agree with your suggestion to determine exactly where the problem
is, i.e. why the thread started by the opentracing library is killed
after chroot().
I hope it's something trivial about what I missed, because otherwise
someone needs to bury themselves in the depths of several connected
libraries written in a pretty complex C++ code.
Hello Willy,
the next two patches solve the problem satisfactorily, no more file
access after initializing the daemon mode (and/or chroot mode).
This is solved so that the initialization of the OpenTracing plugin is
split into two operations, first the plugin (dynamic library) is loaded
before switching the HAProxy to daemon mode (or chroot) and then the
tracer thread is started.
This resolves GitHub issues #1204 and #1274.
Best regards,
--
Zaga <miros...@zagorac.name>
What can change the nature of a man?
>From fd06ebf4cab6dcef649e1ca5a6f38db33ae68b26 Mon Sep 17 00:00:00 2001
From: Miroslav Zagorac <mzago...@haproxy.com>
Date: Thu, 10 Jun 2021 01:19:13 +0200
Subject: [PATCH 1/2] Revert "BUG/MINOR: opentracing: initialization after
establishing daemon mode"
This reverts commit f2263435d71964d1aa3eb80df6464500696c0515.
This commit is unnecessary because although it solves the problem of using
the OpenTracing filter in daemon mode, it does not solve the same problem
if chroot is used.
The following commit related to the OpenTracing filter solves both problems
efficiently.
---
addons/ot/src/filter.c | 48 ++++++++++++++++++------------------------
1 file changed, 20 insertions(+), 28 deletions(-)
diff --git a/addons/ot/src/filter.c b/addons/ot/src/filter.c
index d5fc2c72f..6699d4613 100644
--- a/addons/ot/src/filter.c
+++ b/addons/ot/src/filter.c
@@ -156,15 +156,29 @@ static void flt_ot_return_void(const struct filter *f, char **err)
static int flt_ot_init(struct proxy *p, struct flt_conf *fconf)
{
struct flt_ot_conf *conf = FLT_OT_DEREF(fconf, conf, NULL);
- int retval = FLT_OT_RET_OK;
+ char *err = NULL;
+ int retval = FLT_OT_RET_ERROR;
FLT_OT_FUNC("%p, %p", p, fconf);
if (conf == NULL)
- FLT_OT_RETURN(FLT_OT_RET_ERROR);
+ FLT_OT_RETURN(retval);
flt_ot_cli_init();
+ /*
+ * Initialize the OpenTracing library.
+ * Enable HTX streams filtering.
+ */
+ retval = ot_init(&(conf->tracer->tracer), conf->tracer->config, conf->tracer->plugin, &err);
+ if (retval != FLT_OT_RET_ERROR)
+ fconf->flags |= FLT_CFG_FL_HTX;
+ else if (err != NULL) {
+ FLT_OT_ALERT("%s", err);
+
+ FLT_OT_ERR_FREE(err);
+ }
+
FLT_OT_RETURN(retval);
}
@@ -412,6 +426,8 @@ static int flt_ot_check(struct proxy *p, struct flt_conf *fconf)
}
+#ifdef DEBUG_OT
+
/***
* NAME
* flt_ot_init_per_thread -
@@ -430,38 +446,14 @@ static int flt_ot_check(struct proxy *p, struct flt_conf *fconf)
*/
static int flt_ot_init_per_thread(struct proxy *p, struct flt_conf *fconf)
{
- struct flt_ot_conf *conf = FLT_OT_DEREF(fconf, conf, NULL);
- char *err = NULL;
- int retval = FLT_OT_RET_ERROR;
+ int retval = FLT_OT_RET_OK;
FLT_OT_FUNC("%p, %p", p, fconf);
- if (conf == NULL)
- FLT_OT_RETURN(retval);
-
- /*
- * Initialize the OpenTracing library.
- * Enable HTX streams filtering.
- */
- if (conf->tracer->tracer == NULL) {
- retval = ot_init(&(conf->tracer->tracer), conf->tracer->config, conf->tracer->plugin, &err);
- if (retval != FLT_OT_RET_ERROR)
- fconf->flags |= FLT_CFG_FL_HTX;
- else if (err != NULL) {
- FLT_OT_ALERT("%s", err);
-
- FLT_OT_ERR_FREE(err);
- }
- } else {
- retval = FLT_OT_RET_OK;
- }
-
FLT_OT_RETURN(retval);
}
-#ifdef DEBUG_OT
-
/***
* NAME
* flt_ot_deinit_per_thread -
@@ -1120,7 +1112,7 @@ struct flt_ops flt_ot_ops = {
.init = flt_ot_init,
.deinit = flt_ot_deinit,
.check = flt_ot_check,
- .init_per_thread = flt_ot_init_per_thread,
+ .init_per_thread = FLT_OT_DBG_IFDEF(flt_ot_init_per_thread, NULL),
.deinit_per_thread = FLT_OT_DBG_IFDEF(flt_ot_deinit_per_thread, NULL),
/* Stream callbacks. */
--
2.30.1
>From 58c20892899e832c177b39f83ff17aed37d254f7 Mon Sep 17 00:00:00 2001
From: Miroslav Zagorac <mzago...@haproxy.com>
Date: Thu, 10 Jun 2021 01:23:15 +0200
Subject: [PATCH 2/2] MINOR: opentracing: initialization before establishing
daemon and/or chroot mode
This patch solves the problem reported in github issue #1204, where the
OpenTracing filter cannot communicate with the selected tracer if HAProxy
is run in daemon mode.
This commit also solves github issue #1274, where the problem manifests
itself when using the 'chroot' keyword in the HAProxy configuration.
This is solved so that the initialization of the OpenTracing plugin is
split into two operations, first the plugin (dynamic library) is loaded
before switching the HAProxy to daemon mode (or chroot) and then the
tracer thread is started.
This means that nothing is retrieved from the file system in runtime.
This resolves GitHub issues #1204 and #1274.
---
addons/ot/include/conf.h | 9 +++++----
addons/ot/include/opentracing.h | 1 +
addons/ot/src/conf.c | 1 +
addons/ot/src/filter.c | 33 +++++++++++++++++++++++++------
addons/ot/src/opentracing.c | 35 ++++++++++++++++++++++++++++++++-
addons/ot/src/parser.c | 10 ++++++++--
6 files changed, 76 insertions(+), 13 deletions(-)
diff --git a/addons/ot/include/conf.h b/addons/ot/include/conf.h
index 97df12e05..a4bb7fcc0 100644
--- a/addons/ot/include/conf.h
+++ b/addons/ot/include/conf.h
@@ -57,10 +57,10 @@
#define FLT_OT_DBG_CONF_PH(f,a) \
FLT_OT_DBG(3, f FLT_OT_CONF_HDR_FMT "%p }", FLT_OT_CONF_HDR_ARGS(a, id), (a)->ptr)
-#define FLT_OT_DBG_CONF_TRACER(f,a) \
- FLT_OT_DBG(3, f FLT_OT_CONF_HDR_FMT "'%s' '%s' %p %u %hhu %hhu 0x%02hhx %p:%s 0x%08x %s %s %s }", \
- FLT_OT_CONF_HDR_ARGS(a, id), (a)->config, (a)->plugin, (a)->tracer, (a)->rate_limit, (a)->flag_harderr, \
- (a)->flag_disabled, (a)->logging, &((a)->proxy_log), flt_ot_list_debug(&((a)->proxy_log.logsrvs)), (a)->analyzers, \
+#define FLT_OT_DBG_CONF_TRACER(f,a) \
+ FLT_OT_DBG(3, f FLT_OT_CONF_HDR_FMT "'%s' %p '%s' %p %u %hhu %hhu 0x%02hhx %p:%s 0x%08x %s %s %s }", \
+ FLT_OT_CONF_HDR_ARGS(a, id), (a)->config, (a)->cfgbuf, (a)->plugin, (a)->tracer, (a)->rate_limit, (a)->flag_harderr, \
+ (a)->flag_disabled, (a)->logging, &((a)->proxy_log), flt_ot_list_debug(&((a)->proxy_log.logsrvs)), (a)->analyzers, \
flt_ot_list_debug(&((a)->acls)), flt_ot_list_debug(&((a)->ph_groups)), flt_ot_list_debug(&((a)->ph_scopes)))
#define FLT_OT_DBG_CONF(f,a) \
@@ -155,6 +155,7 @@ struct flt_ot_conf_ph {
struct flt_ot_conf_tracer {
FLT_OT_CONF_HDR(id); /* The tracer name. */
char *config; /* The OpenTracing configuration file name. */
+ char *cfgbuf; /* The OpenTracing configuration. */
char *plugin; /* The OpenTracing plugin library file name. */
struct otc_tracer *tracer; /* The OpenTracing tracer handle. */
uint32_t rate_limit; /* [0 2^32-1] <-> [0.0 100.0] */
diff --git a/addons/ot/include/opentracing.h b/addons/ot/include/opentracing.h
index 2dbf5321a..ec565fa26 100644
--- a/addons/ot/include/opentracing.h
+++ b/addons/ot/include/opentracing.h
@@ -52,6 +52,7 @@ void ot_text_map_show(const struct otc_text_map *text_map);
void ot_debug(void);
#endif
int ot_init(struct otc_tracer **tracer, const char *config, const char *plugin, char **err);
+int ot_start(struct otc_tracer *tracer, const char *cfgbuf, char **err);
struct otc_span *ot_span_init(struct otc_tracer *tracer, const char *operation_name, const struct timespec *ts_steady, const struct timespec *ts_system, int ref_type, int ref_ctx_idx, const struct otc_span *ref_span, const struct otc_tag *tags, int num_tags, char **err);
int ot_span_tag(struct otc_span *span, const struct otc_tag *tags, int num_tags);
int ot_span_log(struct otc_span *span, const struct otc_log_field *log_fields, int num_fields);
diff --git a/addons/ot/src/conf.c b/addons/ot/src/conf.c
index 71db96775..d07be2c66 100644
--- a/addons/ot/src/conf.c
+++ b/addons/ot/src/conf.c
@@ -671,6 +671,7 @@ void flt_ot_conf_tracer_free(struct flt_ot_conf_tracer **ptr)
FLT_OT_FREE((*ptr)->id);
FLT_OT_FREE((*ptr)->config);
+ FLT_OT_FREE((*ptr)->cfgbuf);
FLT_OT_FREE((*ptr)->plugin);
FLT_OT_DBG(2, "- deleting acls list %s", flt_ot_list_debug(&((*ptr)->acls)));
list_for_each_entry_safe(acl, aclback, &((*ptr)->acls), list) {
diff --git a/addons/ot/src/filter.c b/addons/ot/src/filter.c
index 6699d4613..a7708fd74 100644
--- a/addons/ot/src/filter.c
+++ b/addons/ot/src/filter.c
@@ -168,11 +168,10 @@ static int flt_ot_init(struct proxy *p, struct flt_conf *fconf)
/*
* Initialize the OpenTracing library.
- * Enable HTX streams filtering.
*/
retval = ot_init(&(conf->tracer->tracer), conf->tracer->config, conf->tracer->plugin, &err);
if (retval != FLT_OT_RET_ERROR)
- fconf->flags |= FLT_CFG_FL_HTX;
+ /* Do nothing. */;
else if (err != NULL) {
FLT_OT_ALERT("%s", err);
@@ -426,8 +425,6 @@ static int flt_ot_check(struct proxy *p, struct flt_conf *fconf)
}
-#ifdef DEBUG_OT
-
/***
* NAME
* flt_ot_init_per_thread -
@@ -446,14 +443,38 @@ static int flt_ot_check(struct proxy *p, struct flt_conf *fconf)
*/
static int flt_ot_init_per_thread(struct proxy *p, struct flt_conf *fconf)
{
- int retval = FLT_OT_RET_OK;
+ struct flt_ot_conf *conf = FLT_OT_DEREF(fconf, conf, NULL);
+ char *err = NULL;
+ int retval = FLT_OT_RET_ERROR;
FLT_OT_FUNC("%p, %p", p, fconf);
+ if (conf == NULL)
+ FLT_OT_RETURN(retval);
+
+ /*
+ * Start the OpenTracing library tracer thread.
+ * Enable HTX streams filtering.
+ */
+ if (!(fconf->flags & FLT_CFG_FL_HTX)) {
+ retval = ot_start(conf->tracer->tracer, conf->tracer->cfgbuf, &err);
+ if (retval != FLT_OT_RET_ERROR)
+ fconf->flags |= FLT_CFG_FL_HTX;
+ else if (err != NULL) {
+ FLT_OT_ALERT("%s", err);
+
+ FLT_OT_ERR_FREE(err);
+ }
+ } else {
+ retval = FLT_OT_RET_OK;
+ }
+
FLT_OT_RETURN(retval);
}
+#ifdef DEBUG_OT
+
/***
* NAME
* flt_ot_deinit_per_thread -
@@ -1112,7 +1133,7 @@ struct flt_ops flt_ot_ops = {
.init = flt_ot_init,
.deinit = flt_ot_deinit,
.check = flt_ot_check,
- .init_per_thread = FLT_OT_DBG_IFDEF(flt_ot_init_per_thread, NULL),
+ .init_per_thread = flt_ot_init_per_thread,
.deinit_per_thread = FLT_OT_DBG_IFDEF(flt_ot_deinit_per_thread, NULL),
/* Stream callbacks. */
diff --git a/addons/ot/src/opentracing.c b/addons/ot/src/opentracing.c
index 58936d122..eda6d3932 100644
--- a/addons/ot/src/opentracing.c
+++ b/addons/ot/src/opentracing.c
@@ -172,7 +172,7 @@ int ot_init(struct otc_tracer **tracer, const char *config, const char *plugin,
FLT_OT_RETURN(retval);
}
- *tracer = otc_tracer_init(path, config, NULL, errbuf, sizeof(errbuf));
+ *tracer = otc_tracer_load(path, errbuf, sizeof(errbuf));
if (*tracer == NULL) {
FLT_OT_ERR("%s", (*errbuf == '\0') ? "failed to initialize tracing library" : errbuf);
} else {
@@ -185,6 +185,39 @@ int ot_init(struct otc_tracer **tracer, const char *config, const char *plugin,
}
+/***
+ * NAME
+ * ot_start -
+ *
+ * ARGUMENTS
+ * tracer -
+ * cfgbuf -
+ * err -
+ *
+ * DESCRIPTION
+ * -
+ *
+ * RETURN VALUE
+ * This function does not return a value.
+ */
+int ot_start(struct otc_tracer *tracer, const char *cfgbuf, char **err)
+{
+ char errbuf[BUFSIZ] = "";
+ int retval = -1;
+
+ FLT_OT_FUNC("%p, %p, %p:%p", tracer, cfgbuf, FLT_OT_DPTR_ARGS(err));
+
+ if (cfgbuf == NULL)
+ FLT_OT_RETURN(retval);
+
+ retval = otc_tracer_start(NULL, cfgbuf, errbuf, sizeof(errbuf));
+ if (retval == -1)
+ FLT_OT_ERR("%s", (*errbuf == '\0') ? "failed to start tracer" : errbuf);
+
+ FLT_OT_RETURN(retval);
+}
+
+
/***
* NAME
* ot_close -
diff --git a/addons/ot/src/parser.c b/addons/ot/src/parser.c
index 5dec8629d..e26ca18ee 100644
--- a/addons/ot/src/parser.c
+++ b/addons/ot/src/parser.c
@@ -574,7 +574,8 @@ static int flt_ot_parse_cfg_tracer(const char *file, int linenum, char **args, i
*/
static int flt_ot_post_parse_cfg_tracer(void)
{
- int retval = ERR_NONE;
+ char errbuf[BUFSIZ] = "";
+ int retval = ERR_NONE;
FLT_OT_FUNC("");
@@ -586,8 +587,13 @@ static int flt_ot_post_parse_cfg_tracer(void)
if (flt_ot_current_tracer->id == NULL)
FLT_OT_RETURN(retval);
- if (flt_ot_current_tracer->config == NULL)
+ if (flt_ot_current_tracer->config == NULL) {
FLT_OT_POST_PARSE_ALERT("tracer '%s' has no configuration file specified", flt_ot_current_tracer->cfg_line, flt_ot_current_tracer->id);
+ } else {
+ flt_ot_current_tracer->cfgbuf = otc_file_read(flt_ot_current_tracer->config, "#", errbuf, sizeof(errbuf));
+ if (flt_ot_current_tracer->cfgbuf == NULL)
+ FLT_OT_POST_PARSE_ALERT("tracer '%s' %s", flt_ot_current_tracer->cfg_line, flt_ot_current_tracer->id, (*errbuf == '\0') ? "cannot load configuration file" : errbuf);
+ }
if (flt_ot_current_tracer->plugin == NULL)
FLT_OT_POST_PARSE_ALERT("tracer '%s' has no plugin library specified", flt_ot_current_tracer->cfg_line, flt_ot_current_tracer->id);
--
2.30.1
