Hello Willy,

So I'm fighting with dba97077 made by Frédéric Lécaille - it broke many
things.
E.g. you can't use "source", because that patch broke it. I'm curious how
many other stuff got broken with those patches around default-server.

We need some CI (even if they will only build haproxy) and IMHO people with
@haproxy.com mails should test their code before posting and merging :(

In attachment there is patch fixed after your (Willy) review. Sorry for
loop,
I was focused on fixing all those problems with Frédérics patch I just
didn't
think how to replace do..while (which obviously works) with this cleaner
version - thanks for this. The patch got even simpler.

Thanks again,
Michał

2017-03-27 15:34 GMT+02:00 Willy Tarreau <[email protected]>:

> Hi Michal,
>
> On Mon, Mar 27, 2017 at 03:18:10PM +0200, Michal wrote:
> > Hello,
> > Thank you for your response. I agree with part about backward
> compatibility
> > and of course I don't want to break working things
> >
> > I prepared patch with described functionality and with two notes in doc
> to
> > let users know about this behaviour.
>
> Thanks. Just a few points :
>
> 1) cosmetic cleanups below :
>
> > diff --git a/doc/configuration.txt b/doc/configuration.txt
> > index fb3e691..7f22782 100644
> > --- a/doc/configuration.txt
> > +++ b/doc/configuration.txt
> > @@ -11370,6 +11370,10 @@ track [<proxy>/]<server>
> >    enabled. If <proxy> is omitted the current one is used. If
> disable-on-404 is
> >    used, it has to be enabled on both proxies.
> >
> > +  Note:
> > +    Relative weight changes are propagated to all tracking servers. Each
> > +    tracking server will have it's weight recalculated separately.
>
> s/it's/its/
>
> > +
> >    Supported in default-server: No
> >
> >  verify [none|required]
> > diff --git a/doc/management.txt b/doc/management.txt
> > index 1d34f84..3f3730a 100644
> > --- a/doc/management.txt
> > +++ b/doc/management.txt
> > @@ -1694,6 +1694,10 @@ set weight <backend>/<server> <weight>[%]
> >    "admin". Both the backend and the server may be specified either by
> their
> >    name or by their numeric ID, prefixed with a sharp ('#').
> >
> > +  Note:
> > +    Relative weight changes are propagated to all tracking servers. Each
> > +    tracking server will have it's weight recalculated separately.
>
> s/it's/its/
>
> > +
> >  show cli sockets
> >    List CLI sockets. The output format is composed of 3 fields separated
> by
> >    spaces. The first field is the socket address, it can be a unix
> socket, a
> > diff --git a/src/server.c b/src/server.c
> > index 5589723..9462a16 100644
> > --- a/src/server.c
> > +++ b/src/server.c
> > @@ -45,6 +45,9 @@
> >  static void srv_update_state(struct server *srv, int version, char
> **params);
> >  static int srv_apply_lastaddr(struct server *srv, int *err_code);
> >
> > +const char *server_propagate_weight_change_request(struct server *sv,
> > +     const char *weight_str);
> > +
> >  /* List head of all known server keywords */
> >  static struct srv_kw_list srv_keywords = {
> >       .list = LIST_HEAD_INIT(srv_keywords.list)
> > @@ -912,6 +915,8 @@ const char *server_parse_weight_change_request(struct
> server *sv,
> >       struct proxy *px;
> >       long int w;
> >       char *end;
> > +     const char *msg;
> > +     int relative = 0;
> >
> >       px = sv->proxy;
> >
> > @@ -933,6 +938,8 @@ const char *server_parse_weight_change_request(struct
> server *sv,
> >               w = sv->iweight * w / 100;
> >               if (w > 256)
> >                       w = 256;
> > +
> > +             relative = 1;
> >       }
> >       else if (w < 0 || w > 256)
> >               return "Absolute weight can only be between 0 and 256
> inclusive.\n";
> > @@ -945,6 +952,34 @@ const char *server_parse_weight_change_request(struct
> server *sv,
> >       sv->uweight = w;
> >       server_recalc_eweight(sv);
> >
> > +     if (relative) {
> > +             msg = server_propagate_weight_change_request(sv,
> weight_str);
> > +             if (msg != NULL) {
> > +                     return msg;
> > +             }
> > +     }
> > +
> > +     return NULL;
> > +}
> > +
> > +const char *server_propagate_weight_change_request(struct server *sv,
> > +     const char *weight_str)
> > +{
> > +     if (sv->trackers == NULL)
> > +             return NULL;
> > +
> > +     struct server *_propagated_server = sv->trackers;
> > +     const char *msg;
>
> Please don't mix code and declarations, building this emits a warning.
> Also please avoid starting your variable name with an underscore, it
> makes it look "special".
>
> > +
> > +     do {
> > +             msg = server_parse_weight_change_req
> uest(_propagated_server,
> > +                     weight_str);
> > +
> > +             if (msg != NULL) {
> > +                     return msg;
> > +             }
> > +     } while ((_propagated_server = _propagated_server->tracknext) !=
> NULL);
>
> The test at the beginning of the function and the loop combined into
> this quite simplified form :
>
>         struct server *tracker;
>         const char *msg;
>
>         for (tracker = sv->trackers; tracker; tracker =
> tracker->tracknext) {
>                 msg = server_parse_weight_change_request(_propagated_server,
> weight_str);
>                 if (msg)
>                         return msg;
>         }
>
> And 2) please send the resulting patch to the mailing list so that others
> can participate and share their opinion. Some may say "wow cool" and others
> might say "hey please no" (though I suspect that nobody will possibly
> complain
> about the change on relative weights only). And you'll get more testers.
> Just for your information you don't need to be subscribed to the mailing
> list, it's open and you can freely post.
>

I'm sorry. I tend to click "Reply" instead of "Reply to all".


>
> Thanks,
> Willy
>

Attachment: 0001-MINOR-server-Inherit-CLI-weight-changes-and-agent-ch.patch
Description: Binary data

Reply via email to