Re: Interest in patch for web interface to enable/disable servers

Hi all,

Le mercredi 30 juin 2010 08:38:32, Willy Tarreau a écrit :
> Since I am new to the code and may have done
> > something stupid I'd like to put it out there for review and/or
> > comment.
>
> no problem, we'll review your patch (I can't right now but will take
> some time to do so).
>
> > http://www.jpilot.org/testing/haproxy/Screenshot.png
> > http://www.jpilot.org/testing/haproxy/haproxy-post-maint-mode-1.4.8.patch

Thanks Judd for sending me the patch (as jpilot.org is currently down). I've played some minutes with the patch and read it very quickly, I'll take more time for this next days.

My first comments :
1. There's a bug when all servers are down : if you re-enable one, the whole backend stays DOWN, resulting in a 503 response.

This is due to the following line :
sv->state ^= SRV_MAINTAIN;

Please remove this line and it will work perfectly : set_server_up(sv) modifies the state itself and needs to know the server was previously in maintenance.

2. There's a small copy/paste issue where the comment was not updated and doesn't describe what the constant is made for ;)

#define STAT_POSTED 0x00000020 /* do not automatically refresh the stats page */

3. I'm not fond of the colored buttons used to enable/disable the servers. I guess that some users will mistakenly click on this mysterious square. I don't know if others agree with that.

4. In the function called "http_process_req_stat_post" and in the post parameters, you should replace all references to "frontend" with "backend" as servers are not in a frontend but in a backend.

That's all for tonight, I'll make some more complete tests later ;)

--
Cyril Bonté