Hi Joe,
On Thu, Jun 22, 2017 at 12:42:57AM -0700, Joseph Lynch wrote:
> I've been working on a change to Synapse[1] to allow it to use the
> "set server addr" directive to reserve a large(ish) pool of servers in
> a backend and dynamically set server host/ports into that pool as they
> come and go, which would significantly reduce the number of restarts
> we have to do during code deployments. While implementing this I came
> across a problem where I can't figure out how to dynamically update
> the server name (server->id internally), which is important because
> the only host/port information on the stats page is the name afaik.
If it's about the stats page, I think we'd rather emit the FQDN there,
what do you think ?
> The stats page is frequently used in my org to debug which servers are
> up or down so having nice names that say where a server is (basically
> the address) would be useful.
Everyone uses the stats page to debug ;-) That's why I'm fine with adding
further info there. We could even have a description field on servers and
listeners like we have for frontends and backends ("desc") that could be
used to pass extra info and which would not even require unicity (eg:
"3rd rack in DC2").
> Also, historically Synapse and other
> tooling has used the server name to figure out which hosts and ports
> already have server lines but I can work around this by using "show
> stats" and actually using the addr field instead of the name field.
> But, having a nice human readable name is still good I think.
I'm totally fine with adding a human readable field as it will really
help.
> Towards this end I've written up a patch (mostly just cribbed from set
> server fqdn) that updates the name for a server that enables:
>
> set server <b>/<s> name <name>
>
> Would that patch be generally useful or is there a better way to
> update the server name? Or maybe this is already possible and I just
> don't know how? I'm happy to make any needed changes.
I'm having a few problems with changing the server's name :
- it's a unique identifier, so changing it after unicity has been
verified can be a problem and can become confusing in logs, stats
or anything else.
- the name is used for lookups at various places (eg: use_server)
and we really don't want to change it after the config is processed;
- you may even accidently set a name that you'll have a hard time
entering again just to change it, forcing you to restart after a
small mistake or an error in a script.
Given your intent is to provide human-readable information and that the
server name is a config primary key (not always the most readable one
by the way), better add a "desc <name>" config option that you can change
at will on the CLI (set server <b>/<s> desc <desc>), and add it to the
stats page (only when stats-show-desc is set), and let's add a sample
fetch function to return it (logs, headers, anything). What do you think ?
BTW, just a few minor comments on your patch below :
> From 2423ac26f56ecbf5dabe4624b124e84ccbe9f49a Mon Sep 17 00:00:00 2001
> From: Joseph Lynch <[email protected]>
> Date: Wed, 21 Jun 2017 18:44:51 -0700
> Subject: [PATCH] MINOR: cli: ability to change a server's name
>
> This extends the "set server" socket command to also allow setting the
> name for a server. This allows tools to introspect those names and
> allows users of the stats page to have human readable names even when
> they are dynamically updating backends.
> ---
> doc/management.txt | 4 ++++
> include/proto/server.h | 1 +
> src/server.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/src/server.c b/src/server.c
> index 008bafa..8ff804d 100644
> --- a/src/server.c
> +++ b/src/server.c
> @@ -4190,6 +4190,39 @@ int srv_init_addr(void)
> return return_code;
> }
>
> +const char *update_server_name(struct server *server, const char *name,
> const char *updater)
> +{
> + struct chunk *msg;
> +
> + msg = get_trash_chunk();
> + chunk_reset(msg);
> +
> + if (!strcmp(name, server->id)) {
> + chunk_appendf(msg, "no need to change the name");
> + goto out;
> + }
> +
> + char* old_name = server->id;
This will not build, please avoid inline declarations.
> + server->id = strdup(name);
> + if (!server->id) {
> + server->id = old_name;
> + chunk_reset(msg);
> + chunk_appendf(msg, "could not update %s/%s name",
> + server->proxy->id, server->id);
> + } else {
> + chunk_appendf(msg, "%s/%s changed its name from %s to %s",
> + server->proxy->id, old_name, old_name, name);
> + free(old_name);
> + }
I think you'll have less operations to perform if you first allocate the
new name then free the old one and switch the pointers in case of success.
It may also require less locking later for multithreading.
Thanks!
Willy
