Welcome! Log In Create A New Profile

Advanced

Add agent-host configuration directive and allow changing it and agent-send via socket/CLI

Posted by Michał 
Hello!
It's my first PR to haproxy, so please tell me if anything still wrong.
I've read CONTRIBUTING.

This patches implements possiblity to define different host (agent-host) for
agent checks in config and they also allow changing agent-host and
agent-send
variables via CLI/socket. We wonna use this options to dynamically swap
backends
without reloading haproxy. agent-host will allow us to control weight and
status
of servers from single place, so we can enable server when it's ready
(warmed)
instead just after healthcheck passing. I think this change isn't touching
any
hot paths and will find adopters, because reloading haproxy with all SSL
stuff
and losing statstics for less dynamic backends is pain.

In my opinion changes in code don't require any comments. I documented those
features of course, so everyone can use them.

In attachments there are same patches as below, just in .patch format in
case
mail get corrupted.

MD5 (0001-Add-agent-host-config-directive.patch) =
37a3c156b7a7d213f25b1ea52e37df10
MD5 (0002-Add-possiblity-to-change-agent-config-via-CLI-socket.patch) =
f9919fa73a21bd0a178aa301e1585b73
MD5 (0003-Add-docs-for-agent-host-configuration-variable.patch) =
f7d54b0aaef0ea5034ed80193d6f40b9
MD5 (0004-Add-docs-for-agent-host-and-agent-send-CLI-commands.patch) =
8cb920bbab07d964ae5f6d7c6e1c830b

---
src/server.c | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/src/server.c b/src/server.c
index 2f539c9..0ca372a 100644
--- a/src/server.c
+++ b/src/server.c
@@ -1153,6 +1153,14 @@ int parse_server(const char *file, int linenum, char
**args, struct proxy *curpr
newsrv->agent.inter = val;
cur_arg += 2;
}
+ else if (!strcmp(args[cur_arg], "agent-host")) {
+ if(str2ip(args[cur_arg + 1], &newsrv->agent.addr) == NULL) {
+ Alert("parsing agent-host failed. Check if %s is
correct address.\n", args[cur_arg + 1]);
+ goto out;
+ }
+
+ cur_arg += 2;
+ }
else if (!strcmp(args[cur_arg], "agent-port")) {
global.maxsock++;
newsrv->agent.port = atol(args[cur_arg + 1]);
--

---
doc/configuration.txt | 8 ++++++++
1 file changed, 8 insertions(+)

diff --git a/doc/configuration.txt b/doc/configuration.txt
index ae76ef3..088bba6 100644
--- a/doc/configuration.txt
+++ b/doc/configuration.txt
@@ -10684,6 +10684,14 @@ agent-inter <delay>

Supported in default-server: Yes

+agent-host <host>
+ The "agent-host" parameter sets address for agent check.
+
+ You can offload agent-check to another target, so you can make single
place
+ managing status and weights of servers defined in haproxy in case you
can't
+ make self-aware and self-managing services. You can specify both IP or
+ hostname, it will be resolved.
+
agent-port <port>
The "agent-port" parameter sets the TCP port used for agent checks.

--

---
src/server.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/src/server.c b/src/server.c
index 0ca372a..1e6810c 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3467,6 +3467,33 @@ static int cli_parse_set_server(char **args, struct
appctx *appctx, void *privat
appctx->st0 = CLI_ST_PRINT;
}
}
+ else if (strcmp(args[3], "agent-host") == 0) {
+ if(!(sv->agent.state & CHK_ST_ENABLED)) {
+ appctx->ctx.cli.msg = "agent checks are not enabled on this
server.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ } else {
+ if(str2ip(args[4], &sv->agent.addr) == NULL) {
+ appctx->ctx.cli.msg = "incorrect host address given for
agent.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ }
+ }
+ }
+ else if(strcmp(args[3], "agent-send") == 0) {
+ if(!(sv->agent.state & CHK_ST_ENABLED)) {
+ appctx->ctx.cli.msg = "agent checks are not enabled on this
server.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ } else {
+ char *nss = strdup(args[4]);
+ if(!nss) {
+ appctx->ctx.cli.msg = "cannot allocate memory for new
string.\n";
+ appctx->st0 = CLI_ST_PRINT;
+ } else {
+ free(sv->agent.send_string);
+ sv->agent.send_string = nss;
+ sv->agent.send_string_len = strlen(args[4]);
+ }
+ }
+ }
else if (strcmp(args[3], "check-port") == 0) {
int i = 0;
if (strl2irc(args[4], strlen(args[4]), &i) != 0) {
--

---
doc/management.txt | 9 +++++++++
1 file changed, 9 insertions(+)

diff --git a/doc/management.txt b/doc/management.txt
index dd886de..ec4490f 100644
--- a/doc/management.txt
+++ b/doc/management.txt
@@ -1603,6 +1603,15 @@ set server <backend>/<server> agent [ up | down ]
switch a server's state regardless of some slow agent checks for example.
Note that the change is propagated to tracking servers if any.

+set server <backend>/<server> agent-host <addr>
+ Change addr for servers agent checks. Allows to migrate agent-checks to
+ another host at runtime. You can specify both IP and hostname, it will be
+ resolved.
+
+set server <backend>/<server> agent-send <value>
+ Change agent string sent to agent check target. Allows to update string
while
+ changing server address to keep those two matching.
+
set server <backend>/<server> health [ up | stopping | down ]
Force a server's health to a new state. This can be useful to immediately
switch a server's state regardless of some slow health checks for
example.
--
Attachments:
open | download - 0001-Add-agent-host-config-directive.patch (954 bytes)
open | download - 0002-Add-possiblity-to-change-agent-config-via-CLI-socket.patch (1.6 KB)
open | download - 0003-Add-docs-for-agent-host-configuration-variable.patch (987 bytes)
open | download - 0004-Add-docs-for-agent-host-and-agent-send-CLI-commands.patch (1.3 KB)
Hi Michal,

On Mon, Jan 09, 2017 at 02:00:19PM +0100, Micha?? wrote:
> Hello!
> It's my first PR to haproxy, so please tell me if anything still wrong.
> I've read CONTRIBUTING.
>
> This patches implements possiblity to define different host (agent-host) for
> agent checks in config and they also allow changing agent-host and
> agent-send
> variables via CLI/socket. We wonna use this options to dynamically swap
> backends
> without reloading haproxy. agent-host will allow us to control weight and
> status
> of servers from single place, so we can enable server when it's ready
> (warmed)
> instead just after healthcheck passing.

I remember a recent discussion on this subject, I don't remember if it was
here on the list or with Baptiste or someone else, but that's definitely
welcome!

> I think this change isn't touching any
> hot paths and will find adopters,

I think it will be useful to make it easier to control server statuses from
a central place.

> because reloading haproxy with all SSL stuff
> and losing statstics for less dynamic backends is pain.
>
> In my opinion changes in code don't require any comments. I documented those
> features of course, so everyone can use them.

Indeed, however a short description of the changes in the commit messages
would help. Please keep in mind that merged patches are review again later :
- when backporting fixes and the rare few minor improvements
- when bisecting to find the cause of a bug.

In this case it's really important that all the useful information is
visible in the git log to help take the appropriate decision (eg: backport
yes/no, or this is totally unrelated to my problem).

I'd personally prefer if the "agent-host" was renamed to "agent-addr" to
be more consistent with the "addr" parameter we already have for the
checks and which is used at a few places on the CLI. Also in the HTTP
world, "host" is a bit connoted as the string found in the header field
carrying the same name :-)

Otherwise your changes look fine, I'm willing to merge them, I don't
see any risk there.

Thanks!
Willy
Hello!
Thank you for reviewing. For me agent-addr looks better too, I hope it
won't
be confused with "addr" directive.

So here are patches with "agent-addr" changes and I added extebded commit
messages to code commits.

I wrote "Can be backported", because those are not bugfix'es, because they
don't
have to be backported, but on the other hand upgrading to 1.8 won't be
near-future for many projects and backport to 1.7 will spread this feature
to
more people.


Patches files md5:

MD5 (./0001-PATCH-MINOR-checks-Add-agent-addr-config-directive.patch) =
555a04df5dc56fa44ab94cb321df469b
MD5 (./0002-PATCH-MINOR-cli-Add-possiblity-to-change-agent-confi.patch) =
4b1d732dff72a3349766d74a7f28dcd6
MD5 (./0003-PATCH-MINOR-doc-Add-docs-for-agent-addr-configuratio.patch) =
2421ff01f26936e59461955b4ea5684f
MD5 (./0004-PATCH-MINOR-doc-Add-docs-for-agent-addr-and-agent-se.patch) =
971d6c1d23657130db4045d5b9e8cb73

Michał
Attachments:
open | download - 0001-PATCH-MINOR-checks-Add-agent-addr-config-directive.patch (1.1 KB)
open | download - 0002-PATCH-MINOR-cli-Add-possiblity-to-change-agent-confi.patch (1.9 KB)
open | download - 0003-PATCH-MINOR-doc-Add-docs-for-agent-addr-configuratio.patch (1008 bytes)
open | download - 0004-PATCH-MINOR-doc-Add-docs-for-agent-addr-and-agent-se.patch (1.3 KB)
Hi!

On Mon, Jan 16, 2017 at 10:57:12AM +0100, Micha?? wrote:
> Hello!
> Thank you for reviewing. For me agent-addr looks better too, I hope it
> won't
> be confused with "addr" directive.
>
> So here are patches with "agent-addr" changes and I added extebded commit
> messages to code commits.
>
> I wrote "Can be backported", because those are not bugfix'es, because they
> don't
> have to be backported, but on the other hand upgrading to 1.8 won't be
> near-future for many projects and backport to 1.7 will spread this feature
> to
> more people.

OK, thank you, I've applied all the series now. Regarding the backport,
I prefer not to do it. The reason is that some distros will start to ship
with 1.7 soon, and some people might find some articles on the net suggesting
to do something with certain options that don't exist for them. We've faced
this situation a lot in the past during 1.3 and 1.4 and that's really not
nice for distro maintainers who are bugged with wrong reports. Now we
release much more often and limit the backport to fixes.

However I've kept your comment saying that it can be backported because if
someone wants to add your series into his/her tree, it's useful to know that
it can be done without risks.

Thanks!
Willy
Sorry, only registered users may post in this forum.

Click here to login