Re: BUG/MEDIUM: spoe: engine-id is necessary if not health check

[Christopher Faulet]

Le 17/09/2019 à 11:11, Christopher Faulet a écrit :

Le 16/09/2019 à 03:40, Kevin Zhu a écrit :

Sorry Crhistopher, have you look at this mail ?

On Mon, 2 Sep 2019 at 16:11, Kevin Zhu mailto:ip0...@gmail.com>> wrote:

 Hi Christopher

 SPOE engine-id is all same when nbproc is more than 1, the clients all
 group under same engine, and same stream-id and frame-id frames may come at
 same time. So we alway need engine-id to identify client's and frames.
 Best Regards



Hi Kevin,

Don't be sorry. I was busy when received your mail and after I forgot to review
it. So, thanks for the reminder.

There are 2 parts in your patch. First, about the SPOE engine-id when there is
more than 1 process, you're right. The engine-id is the same for all instances
and it's a problem. But if we are fixing this part, we might as well fix it for
the threads too. For now, the Async mode does not work with several threads just
because of the engine-id. It is pretty easy to fix, thus I will do it too.

Then, about the mandatory nature of the engine-id, I'm disagree. For 2 reasons.
First, because health-checks does not provide it. Second, because it is only
required for the async mode. We can imagine to not send it when this mode is
explicitly disabled. From the protocol point of view, I prefer to keep it 
optional.

I can amend your patch to get only part to generate a different engine-id per
process if you are ok. Then I'll do a fix to have a different engine-id
per-thread too and always allow the async mode.



Hi Kevin,

I've finally found some time to push the patches. I will backport it soon. 
Thanks !

--
Christopher Faulet

Re: BUG/MEDIUM: spoe: engine-id is necessary if not health check

[Christopher Faulet]

Le 16/09/2019 à 03:40, Kevin Zhu a écrit :

Sorry Crhistopher, have you look at this mail ?

On Mon, 2 Sep 2019 at 16:11, Kevin Zhu > wrote:


Hi Christopher

SPOE engine-id is all same when nbproc is more than 1, the clients all
group under same engine, and same stream-id and frame-id frames may come at
same time. So we alway need engine-id to identify client's and frames.
Best Regards



Hi Kevin,

Don't be sorry. I was busy when received your mail and after I forgot to review 
it. So, thanks for the reminder.


There are 2 parts in your patch. First, about the SPOE engine-id when there is 
more than 1 process, you're right. The engine-id is the same for all instances 
and it's a problem. But if we are fixing this part, we might as well fix it for 
the threads too. For now, the Async mode does not work with several threads just 
because of the engine-id. It is pretty easy to fix, thus I will do it too.


Then, about the mandatory nature of the engine-id, I'm disagree. For 2 reasons. 
First, because health-checks does not provide it. Second, because it is only 
required for the async mode. We can imagine to not send it when this mode is 
explicitly disabled. From the protocol point of view, I prefer to keep it optional.


I can amend your patch to get only part to generate a different engine-id per 
process if you are ok. Then I'll do a fix to have a different engine-id 
per-thread too and always allow the async mode.


--
Christopher Faulet

Re: BUG/MEDIUM: spoe: engine-id is necessary if not health check

[Kevin Zhu]

Sorry Crhistopher, have you look at this mail ?

On Mon, 2 Sep 2019 at 16:11, Kevin Zhu  wrote:

> Hi Christopher
>
> SPOE engine-id is all same when nbproc is more than 1, the clients all
> group under same engine, and same stream-id and frame-id frames may come
> at
> same time. So we alway need engine-id to identify client's and frames.
> Best Regards
>

BUG/MEDIUM: spoe: engine-id is necessary if not health check

[Kevin Zhu]

Hi Christopher

SPOE engine-id is all same when nbproc is more than 1, the clients all
group under same engine, and same stream-id and frame-id frames may come at
same time. So we alway need engine-id to identify client's and frames.
Best Regards
From caedabe336897704b4da39818ac7d226c7b8a9db Mon Sep 17 00:00:00 2001
From: Kevin Zhu 
Date: Mon, 2 Sep 2019 13:45:34 +0800
Subject: [PATCH] BUG/MEDIUM: spoe: engine-id is necessary if not health check

SPOE engine-id is all same when nbproc is more than 1, the clients all
group under same engine, and same stream-id and frame-id frames may come at
same time. So we alway need engine-id to identify client's and frames.
---
 doc/SPOE.txt   |  8 
 src/flt_spoe.c | 35 +--
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/doc/SPOE.txt b/doc/SPOE.txt
index 19f00ad..dd8f920 100644
--- a/doc/SPOE.txt
+++ b/doc/SPOE.txt
@@ -932,6 +932,10 @@ Following items are mandatory in the KV-LIST:
 This a comma-separated list of capabilities supported by HAProxy. Spaces
 must be ignored, if any.
 
+  * "engine-id"
+
+This is a uniq string that identify a SPOE engine.
+
 Following optional items can be added in the KV-LIST:
 
   * "healthcheck"
@@ -939,10 +943,6 @@ Following optional items can be added in the KV-LIST:
 If this item is set to TRUE, then the HAPROXY-HELLO frame is sent during a
 SPOE health check. When set to FALSE, this item can be ignored.
 
-  * "engine-id"
-
-This is a uniq string that identify a SPOE engine.
-
 To finish the HELLO handshake, the agent must return an AGENT-HELLO frame with
 its supported SPOP version, the lower value between its maximum size allowed
 for a frame and the HAProxy one and capabilities it supports. If an error
diff --git a/src/flt_spoe.c b/src/flt_spoe.c
index b8323f6..06ceeae 100644
--- a/src/flt_spoe.c
+++ b/src/flt_spoe.c
@@ -267,7 +267,7 @@ generate_pseudo_uuid()
 		return NULL;
 
 	if (!init) {
-		srand(now_ms);
+		srand(now_ms + pid);
 		init = 1;
 	}
 
@@ -457,17 +457,18 @@ spoe_prepare_hahello_frame(struct appctx *appctx, char *frame, size_t size)
 	if (spoe_encode_buffer(chk->area, chk->data, , end) == -1)
 		goto too_big;
 
-	/* (optionnal) "engine-id" K/V item, if present */
-	if (agent != NULL && agent->engine_id != NULL) {
-		sz = SLEN(ENGINE_ID_KEY);
-		if (spoe_encode_buffer(ENGINE_ID_KEY, sz, , end) == -1)
-			goto too_big;
+	/* "engine-id" K/V item */
+	sz = SLEN(ENGINE_ID_KEY);
+	if (spoe_encode_buffer(ENGINE_ID_KEY, sz, , end) == -1)
+		goto too_big;
 
-		*p++ = SPOE_DATA_T_STR;
+	*p++ = SPOE_DATA_T_STR;
+	if (agent != NULL) {
 		sz = strlen(agent->engine_id);
 		if (spoe_encode_buffer(agent->engine_id, sz, , end) == -1)
 			goto too_big;
-	}
+	} else if (spoe_encode_buffer(NULL, 0, , end) == -1)
+		goto too_big;
 
 	return (p - frame);
 
@@ -3111,6 +3112,21 @@ spoe_check(struct proxy *px, struct flt_conf *fconf)
 	return 0;
 }
 
+/* Initializes the SPOE filter for a proxy for a specific thread.
+ * Returns a negative value if an error occurs. */
+static int
+spoe_init_per_thread(struct proxy *p, struct flt_conf *fconf)
+{
+	struct spoe_agent *agent = ((struct spoe_config*)fconf->conf)->agent;
+	if (agent->engine_id == NULL)
+		agent->engine_id = generate_pseudo_uuid();
+
+	if (agent->engine_id == NULL)
+		return -1;
+
+	return 0;
+}
+
 /**
  * Hooks attached to a stream
  */
@@ -3309,6 +3325,7 @@ struct flt_ops spoe_ops = {
 	.init   = spoe_init,
 	.deinit = spoe_deinit,
 	.check  = spoe_check,
+	.init_per_thread = spoe_init_per_thread,
 
 	/* Handle start/stop of SPOE */
 	.attach = spoe_start,
@@ -4177,8 +4194,6 @@ parse_spoe_flt(char **args, int *cur_arg, struct proxy *px,
 		}
 		curagent->var_pfx = strdup(curagent->id);
 	}
-	if (curagent->engine_id == NULL)
-		curagent->engine_id = generate_pseudo_uuid();
 
 	if (curagent->var_on_error) {
 		struct arg arg;
-- 
2.7.4