Re: Lua: forcing garbage collector after socket i/o
On Thu, Jan 09, 2020 at 05:30:32PM -0800, Sadasiva Gujjarlapudi wrote:
> After applying Willy's patch
>
> requests/sec 7181.9869
> $time haproxy
> real 0m10.304s
> user 0m1.112s
> sys 0m1.184s
>
> After applying Willy's patch and the following patch
>
> @@ -955,10 +956,14 @@ void hlua_ctx_destroy(struct hlua *lua)
> * the garbage collection.
> */
> if (lua->flags & HLUA_MUST_GC) {
> - if (!SET_SAFE_LJMP(gL.T))
> - return;
> - lua_gc(gL.T, LUA_GCCOLLECT, 0);
> - RESET_SAFE_LJMP(gL.T);
> + if (lua->gc_tune_count++ > 1000) {
> + lua->gc_tune_count = 0;
> + if (!SET_SAFE_LJMP(gL.T))
> + return;
> + lua_gc(gL.T, LUA_GCCOLLECT, 0);
> + RESET_SAFE_LJMP(gL.T);
> + }
> +
> }
>
> requests/sec 8635.0402
> real 0m10.969s
> user 0m0.744s
> sys 0m1.116s
OK this is promising, thanks for the tests!
> Most of the time the CPU is the bottleneck and it may be ok to consume
> little bit more memory on dedicated servers running haproxy.
> Our use case/Lua application required to use as little CPU as possible.
Of course, but according to Thierry the problem is not so much about
memory, but rather about connections/file descriptors.
Well, I'm thinking about something. Since this HLUA_MUST_GC flag is used
only for connections, we could probably proceed differently. Instead of
a flag we could have a connection counter, which gets incremented for
each connection we create and decremented for each closed connection.
Then we'd just run the GC in hlua_destroy_ctx() if the connection count
indicates we did not kill all of them. This way, the GC will never run
for normal situations where connections were cleanly closed, and it will
only be run if absolutely needed.
I can propose you to give a try to the attached patch. I have no idea if
it works or does something correct. It's only build-tested.
Willy
diff --git a/include/types/hlua.h b/include/types/hlua.h
index fb1796c..b541e15 100644
--- a/include/types/hlua.h
+++ b/include/types/hlua.h
@@ -37,7 +37,7 @@ struct stream;
#define HLUA_WAKERESWR 0x0004
#define HLUA_WAKEREQWR 0x0008
#define HLUA_EXIT 0x0010
-#define HLUA_MUST_GC 0x0020
+/* unused: 0x0020 */
#define HLUA_STOP 0x0040
#define HLUA_F_AS_STRING0x01
@@ -93,6 +93,7 @@ struct hlua {
struct list com; /* The list head of the signals attached to this task.
*/
struct ebpt_node node;
struct hlua_consistency cons; /* Store data consistency check. */
+ int conn_count; /* outgoing connections count */
};
/* This is a part of the list containing references to functions
diff --git a/src/hlua.c b/src/hlua.c
index 37f7866..b60e8ba 100644
--- a/src/hlua.c
+++ b/src/hlua.c
@@ -905,6 +905,7 @@ int hlua_ctx_init(struct hlua *lua, struct task *task, int
already_safe)
}
lua->Mref = LUA_REFNIL;
lua->flags = 0;
+ lua->conn_count = 0;
LIST_INIT(&lua->com);
lua->T = lua_newthread(gL.T);
if (!lua->T) {
@@ -954,7 +955,7 @@ void hlua_ctx_destroy(struct hlua *lua)
* NOTE: maybe this action locks all the Lua threads untiml the en of
* the garbage collection.
*/
- if (lua->flags & HLUA_MUST_GC) {
+ if (lua->conn_count) {
if (!SET_SAFE_LJMP(gL.T))
return;
lua_gc(gL.T, LUA_GCCOLLECT, 0);
@@ -1194,11 +1195,6 @@ resume_execution:
break;
}
- /* This GC permits to destroy some object when a Lua timeout strikes. */
- if (lua->flags & HLUA_MUST_GC &&
- ret != HLUA_E_AGAIN)
- lua_gc(lua->T, LUA_GCCOLLECT, 0);
-
switch (ret) {
case HLUA_E_AGAIN:
break;
@@ -1699,6 +1695,7 @@ __LJMP static int hlua_socket_close_helper(lua_State *L)
struct hlua_socket *socket;
struct appctx *appctx;
struct xref *peer;
+ struct hlua *hlua = hlua_gethlua(L);
socket = MAY_LJMP(hlua_checksocket(L, 1));
@@ -1711,6 +1708,8 @@ __LJMP static int hlua_socket_close_helper(lua_State *L)
peer = xref_get_peer_and_lock(&socket->xref);
if (!peer)
return 0;
+
+ hlua->conn_count--;
appctx = container_of(peer, struct appctx, ctx.hlua_cosocket.xref);
/* Set the flag which destroy the session. */
@@ -2463,7 +2462,7 @@ __LJMP static int hlua_socket_connect(struct lua_State *L)
si_rx_endp_more(&s->si[0]);
appctx_wakeup(appctx);
- hlua->flags |= HLUA_MUST_GC;
+ hlua->conn_count++;
if (!notification_new(&hlua->com,
&appctx->ctx.hlua_cosocket.wake_on_write, hlua->task)) {
xref_unlock(&socket->xref, peer);
Re: Lua: forcing garbage collector after socket i/o
After applying Willy's patch
requests/sec 7181.9869
$time haproxy
real 0m10.304s
user 0m1.112s
sys 0m1.184s
After applying Willy's patch and the following patch
@@ -955,10 +956,14 @@ void hlua_ctx_destroy(struct hlua *lua)
* the garbage collection.
*/
if (lua->flags & HLUA_MUST_GC) {
- if (!SET_SAFE_LJMP(gL.T))
- return;
- lua_gc(gL.T, LUA_GCCOLLECT, 0);
- RESET_SAFE_LJMP(gL.T);
+ if (lua->gc_tune_count++ > 1000) {
+ lua->gc_tune_count = 0;
+ if (!SET_SAFE_LJMP(gL.T))
+ return;
+ lua_gc(gL.T, LUA_GCCOLLECT, 0);
+ RESET_SAFE_LJMP(gL.T);
+ }
+
}
requests/sec 8635.0402
real 0m10.969s
user 0m0.744s
sys 0m1.116s
Most of the time the CPU is the bottleneck and it may be ok to consume
little bit more memory on dedicated servers running haproxy.
Our use case/Lua application required to use as little CPU as possible.
Thankyou for the great product and your support !
Regards,
sada
On Thu, Jan 9, 2020 at 2:03 AM Willy Tarreau wrote:
>
> On Thu, Jan 09, 2020 at 10:56:59AM +0100, Thierry Fournier wrote:
> > >> Maybe we try to include new option "tune.lua.forced-gc".
> > >
> > > Based on your description if we need an option, I'd rather have the
> > > opposite, like "tune.lua.disable-gc" so that it remains enabled by
> > > default, and with an explanation that one must not disable it.
> >
> >
> > I agree, but it make sense to run some tests:
> >
> > - First, testing your patch to decide if the GC line 1195 is relevant
> > - Second, testing the behavior when GC is disabled and lua create
> > connexion and lua fail without closing connexion.
> >
> > Maybe the second case make the GC mandatory.
>
> Probably but then it should only be needed in hlua_ctx_destroy(), no ?
> My understanding is that the only goal is to be sure to kill such an
> orphan connection in case the Lua application dies, so I tend to think
> it's never needed in resume (I don't see how we'd resume after dying)
> and that hlua_ctx_destroy() should always be called for the cleanup so
> it should possibly be enough.
>
> Willy
Seven Affordable Training Courses For Charities And Public Sector Organisations
NFP WORKSHOPS Seven Affordable Training Courses For Charities And Public Sector Organisations This email has been sent to haproxy@formilux.org To unsubscribe please reply back inserting "unsubscribe" at the top. Unsubscribes will take effect within seven days at the latest. TO BOOK ANY WORKSHOP OR FIND OUT MORE JUST GOOGLE "NFP WORKSHOPS" BID WRITING: THE BASICS Start 09.00 Finish 11.30 Cost £95.00 Do you know the most common reasons for rejection? Are you gathering the right evidence? Are you making the right arguments? Are you using the right terminology? Are your numbers right? Are you learning from rejections? Are you assembling the right documents? Do you know how to create a clear and concise standard funding bid? Are you communicating with people or just excluding them? Do you know your own organisation well enough? Are you thinking through your projects carefully enough? Do you know enough about your competitors? Are you answering the questions funders will ask themselves about your application? Are you submitting applications correctly? BID WRITING ADVANCED Start 12.00 Finish 14.30 Cost £95.00 Are you applying to the right trusts? Are you applying to enough trusts? Are you asking for the right amount of money? Are you applying in the right ways? Are your projects the most fundable projects? Are you carrying out trust fundraising in a professional way? Are you delegating enough work? Are you highly productive or just very busy? Are you looking for trusts in all the right places? How do you compare with your competitors for funding? Is the rest of your fundraising hampering your bids to trusts? Do you understand what trusts are ideally looking for? LOCATIONS & DATES LONDON VAI, 200A Pentonville Road, Kings Cross, London N1 9JP 13 Jan 2020 - 20 Jan 2020 - 16 Mar 2020 - 23 Mar 2020 SOUTHAMPTON Jurys Inn, Charlotte Place, Southampton SO14 0TB 14 Jan 2020 - 17 Mar 2020 BRISTOL The Waterfront Meeting Rooms, Welsh Back, Bristol BS1 4SB 15 Jan 2020 - 18 Mar 2020 EDINBURGH Courtyard, 1-3 Baxter's Place, Edinburgh EH1 3AF 16 Jan 2020 - 19 Mar 2020 NEWCASTLE Jurys Inn, Scotswood Road, Newcastle upon Tyne NE1 4AD 17 Jan 2020 - 20 Mar 2020 NOTTINGHAM Jurys Inn, Station Street, Nottingham NG2 3BJ 21 Jan 2020 - 24 Mar 2020 BIRMINGHAM Jurys Inn, 245 Broad Street, Birmingham B1 2HQ 22 Jan 2020 - 25 Mar 2020 MANCHESTER Jurys Inn, 56 Great Bridgewater Street, Manchester M1 5LE 23 Jan 2020 - 26 Mar 2020 LEEDS Jurys Inn, Brewery Wharf, Brewery Place, Leeds LS10 1NE 24 Jan 2020 - 27 Mar 2020 BID WRITING: INTERACTIVE Here is how the day works. You bring 11 copies of your standard funding bid or a recent bid. Print them out triple spaced in Arial 16 point font. We will provide a big table, pens, a trainer and up to 9 other attendees. We will all review your bid. Word by word. Line by line. We will test out your jargon and your acronyms to see how well understood they really are. We will ask “What are you actually trying to say here?”. Once your bid is revised you will have the chance to review up to 9 other bids to learn from other people’s bids and how they go about things. By the end of the day you should have a better written, clearer, more effective bid than you arrived with. If it is perfect already then at least you will have had that confirmed by up to 10 other people. We will send you a questionnaire in advance about your trust and foundation fundraising statistics and strategy. We will all then review that strategy to see how you are doing now, what you should be aiming to achieve in the future and the practical steps to bring that about. Finally we will review your organisation as a whole. What aspects should you be telling more people more about. What aspects should you be trying to improve in the years ahead to increase your success rate with trusts and foundations. LONDON VAI, 200A Pentonville Road, Kings Cross, London N1 9JP 13 Mar 2020 RECRUITMENT: THE BASICS A step by step introduction to all the most important things to consider when recruiting new staff. Writing job descriptions. Writing candidate specifications. Writing job adverts. Where to advertise. Using a recruitment agency. Selecting candidates to interview. Interview questions. Interview styles. Final selection. Contracts of employment. Inductions for new staff. Payroll and expenses. An overview of employment legislation. Employment record keeping. Attracting the best staff through improving your public profile. LONDON VAI, 200A Pentonville Road, Kings Cross, London N1 9JP 09 Mar 2020 PURCHASING: THE BASICS How to buy cheaper. How to buy better. How to buy smarter. How to shop around. How to negotiate. How to motivate suppliers. How to improve relationships with existing suppliers. How to encourage new suppliers. Reducing IT, broadband and telephone costs. Reducing energy costs. Saving on premises costs. Saving on staff costs. Top
Re: [PATCH] MINOR: cli: use global unix settings for stats/master sockets
On Thu, Jan 09, 2020 at 03:53:40PM +0100, wlallem...@haproxy.com wrote:
> Regarding the master CLI:
>
> On Thu, Jan 09, 2020 at 02:31:19PM +, William Dauchy wrote:
> > that's a good subject to talk about. While looking at this I also
> > wondered why it was not possible to configure it in the config file; to my
> > knowledge there is no explanation about it in the doc, nor in the code,
> > but I might have missed something.
> > I did not know about those concerns around being able to access CLI even
> > if the config file is corrupted, thanks for the clarification.
> >
>
> In fact it's supposed to be configured in your unit file or from your init
> script (/etc/{default,sysconfig} etc.).
>
> The problem is that the configuration for HAProxy and for the processes are in
> the same configuration file, and if something is wrong, it won't work anymore.
> And the master is supposed to survive a reload with a wrong configuration.
Even without a wrong configuration the important point is that the master
must not change its external interfaces (i.e. the socket) if the config
changes during a reload, or it really becomes a mess. The typical issue
will be that it would then need to consider rebinding across reloads,
which means closing and re-opening the socket, and that's terribly bad.
First it would prevent from being able to maintain a connection across
reloads, second it means that if anything goes wrong during this moment,
you definitely lose access to the master's socket with no option to
recover it.
Willy
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
On Thu, Jan 09, 2020 at 08:22:29PM +0100, Tim Düsterhus wrote: > Bob, > > Am 09.01.20 um 19:44 schrieb Zakharychev, Bob: > > I'd say that since the issue can be solved naturally and completely within > > the confines of Lua itself, new parameters should not be introduced in > > HAProxy to give one ability to solve it differently while obscuring the > > solution in configuration of the host program, however tempting this might > > look. > > To put my POV into a different perspective: HAProxy is the host program > of the Lua scripts and thus it is responsible for providing a working > environment to the script. It should not need to know how the file > system layout works. > Configuring this environment within HAProxy just feels natural even if > it might be possible to configure this setting from within a dedicated > "configuration script" as well. The useful point I'm seeing here is that it allows users to use some external Lua programs with no modification and only specify the paths in the global section which centralizes everything specific to the deployment. Also it even allows to use environment variables there, which can be quite useful. However, Bob, your concern confirms mine about the risks involved by adding the ability to execute some Lua code inlined directly from the haproxy config. So I think that if we want to stay reasonable, at least for a while, we should take Tim's patch as it is now to help configure the path, but not put Lua code in the config file. Willy
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
Willy, Am 09.01.20 um 21:57 schrieb Willy Tarreau: > OK so the second path will be looked up before the first one. This is > something I'll probably add to the doc as well then. Yes. Look up works left to right as within the PATH you know and by prepending we are adding to the left. Best regards Tim Düsterhus
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
On Thu, Jan 09, 2020 at 07:35:45PM +0100, Tim Düsterhus wrote: > > > Prepends the given string followed by a semicolon to Lua's package.path > variable. > > Lua's package.path is a semicolon delimited list of patterns that > specify how the `require` function attempts to find the source file of a > library. Question marks (?) within a pattern will be replaced by module > name. > > Example: > > By specifying the following path: > > lua-prepend-path /usr/share/haproxy-lua/?.lua > > Lua will attempt to load the /usr/share/haproxy-lua/example.lua script > when `require "example"` is being called. > > See https://www.lua.org/pil/8.1.html for the details within the Lua > documentation. > > OK, it looks pretty good like this, I was not asking for more. I'll take your patch and paste this inside. > > doing. The link is then provided to have the details. And by the way it > > should also be clear what happens if multiple such directives are specified > > (I have no idea). > > It prepends multiple times (instead of overriding the to-be-prepended > value). So I suppose it works like you expect it to work. In fact that > was part of the example given in my initial mail. I prepended two > different "paths" and both variants appear in the error message in > addition to the default ones. OK so the second path will be looked up before the first one. This is something I'll probably add to the doc as well then. Thanks! Willy
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
Bob, Am 09.01.20 um 19:44 schrieb Zakharychev, Bob: > I'd say that since the issue can be solved naturally and completely within > the confines of Lua itself, new parameters should not be introduced in > HAProxy to give one ability to solve it differently while obscuring the > solution in configuration of the host program, however tempting this might > look. To put my POV into a different perspective: HAProxy is the host program of the Lua scripts and thus it is responsible for providing a working environment to the script. It should not need to know how the file system layout works. Configuring this environment within HAProxy just feels natural even if it might be possible to configure this setting from within a dedicated "configuration script" as well. Best regards Tim Düsterhus
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
Willy, Vincent, Am 09.01.20 um 17:41 schrieb Willy Tarreau: > I'm seeing some merit in this for quite some stuff which annoys us by > requiring a dedicated file. I'm just a bit anxious about how this can > steer from the original intent, with users seeing it as a way to inline > code anywhere, then complaining that we don't have multi-line strings > nor 64kB long lines :-/ > > I'm interested in other opinions on this as well. > In fact I thought about this topic of "HAProxy specific Lua libraries" a bit more and also about making this useful for Linux distributions when we opt to change something about this within HAProxy. Consider that a Linux distribution wants to distribute the haproxy-lua-http library. That library is not useful outside of HAProxy, because it relies on the internal API. To be a useful package it should be "easy" for the administrator to `require` that library within custom Lua scripts (for a certain definition of easy). Vincent, a few questions for you as a maintainer of the distro of my choice (full thread is here if you need context: https://www.mail-archive.com/haproxy@formilux.org/msg35839.html). If you would package the haproxy-lua-http library for Debian (https://github.com/haproxytech/haproxy-lua-http) [1], what would you believe would be most "useful" / "in spirit of Debian packaging" / "your choice"? a) Install the library into the regular Lua library path, even if it would not be useful / broken outside of HAProxy. Using require within HAProxy simply works without doing anything. b) Install the library into a HAProxy specific Lua library path (e.g. /usr/share/haproxy/lua-path/) and add proper lua-prepend-path to HAProxy's default configuration. The user would need to copy that into their own configuration, otherwise things break. c) Install the library into a HAProxy specific Lua library path and add that path to HAProxy at compile time via a dedicated compile option. For the user it would work out of the box, similar to option (a), but it would not clutter the global Lua library path. d) Something entirely different. Best regards Tim Düsterhus [1] I'd like to see it packaged if something useful comes out of this discussion :-)
RE: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
I'd say that since the issue can be solved naturally and completely within the confines of Lua itself, new parameters should not be introduced in HAProxy to give one ability to solve it differently while obscuring the solution in configuration of the host program, however tempting this might look. Just my two cents. Bob -Original Message- From: Willy Tarreau Sent: Thursday, January 9, 2020 11:42 AM To: Tim Düsterhus Cc: Adis Nezirovic ; haproxy@formilux.org; tfourn...@arpalert.org Subject: Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option On Thu, Jan 09, 2020 at 03:00:34PM +0100, Tim Düsterhus wrote: > However this morning I started thinking whether it might make sense to > generalize `lua-prepend-path` to `lua-run-string` that would execute > the Lua script given as the parameter. It might provide better > flexibility and would be as readable as lua-prepend-path: > > lua-run-string "package.path = package.path .. ';/some/path/?.lua'" > > Opinions? I'm seeing some merit in this for quite some stuff which annoys us by requiring a dedicated file. I'm just a bit anxious about how this can steer from the original intent, with users seeing it as a way to inline code anywhere, then complaining that we don't have multi-line strings nor 64kB long lines :-/ I'm interested in other opinions on this as well. Willy
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
Willy, Am 09.01.20 um 17:38 schrieb Willy Tarreau: >> I concede that the HAProxy documentation should mention `package.path` >> to be clear what kind of path it refers to. But I disagree that we >> should explain how this works in detail, because this is not about any >> HAProxy specific stuff, but about the "inner workings" of Lua and thus >> it's best explained by the Lua documentation. We would only be able to >> "re-explain" the existing documentation, but probably worse, because we >> miss out on important details. > > Except that the terminology here is totally misleading. I didn't understand > your proposed example even when reading the commit message and the proposed > doc. This solely because Lua uses the term "path" to designate a pattern, > while in traditional environments a path is more a list of directories. > *This* must be mentioned. It's not acceptable to have a config documentation > saying "hehe I caught you, you thought you would find the info you were > looking for here, but no, please visit this link instead, and too bad for > you if the net access is down due to haproxy not starting because of the > wrong config you're trying to fix, have a good night sir". I'm not suggesting > to paraphrase the Lua's doc, just to mention the few elements that are > sufficient for someone having to edit an entry to understand what he/she's Fair enough. Without creating a completely new patch, what about something like this: Prepends the given string followed by a semicolon to Lua's package.path variable. Lua's package.path is a semicolon delimited list of patterns that specify how the `require` function attempts to find the source file of a library. Question marks (?) within a pattern will be replaced by module name. Example: By specifying the following path: lua-prepend-path /usr/share/haproxy-lua/?.lua Lua will attempt to load the /usr/share/haproxy-lua/example.lua script when `require "example"` is being called. See https://www.lua.org/pil/8.1.html for the details within the Lua documentation. > doing. The link is then provided to have the details. And by the way it > should also be clear what happens if multiple such directives are specified > (I have no idea). It prepends multiple times (instead of overriding the to-be-prepended value). So I suppose it works like you expect it to work. In fact that was part of the example given in my initial mail. I prepended two different "paths" and both variants appear in the error message in addition to the default ones. Best regards Tim Düsterhus
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
On Thu, Jan 09, 2020 at 03:00:34PM +0100, Tim Düsterhus wrote: > However this morning I started thinking whether it might make sense to > generalize `lua-prepend-path` to `lua-run-string` that would execute the > Lua script given as the parameter. It might provide better flexibility > and would be as readable as lua-prepend-path: > > lua-run-string "package.path = package.path .. ';/some/path/?.lua'" > > Opinions? I'm seeing some merit in this for quite some stuff which annoys us by requiring a dedicated file. I'm just a bit anxious about how this can steer from the original intent, with users seeing it as a way to inline code anywhere, then complaining that we don't have multi-line strings nor 64kB long lines :-/ I'm interested in other opinions on this as well. Willy
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
On Thu, Jan 09, 2020 at 03:27:17PM +0100, Tim Düsterhus wrote: > Willy, > > Am 09.01.20 um 04:07 schrieb Willy Tarreau: > > I'm personally not opposed to this at all, however after reading the link > > above and given that Lua's concept of "path" is all but obvious as described > > in the link above, we definitely need a bit more text in the doc to quickly > > explain that the path is rather a pattern and that the question mark is > > replaced by the file in "require", and probably put a short example. Having > > the link for more details is fine but not as a redirect to figure how to > > use the config keyword. > > I concede that the HAProxy documentation should mention `package.path` > to be clear what kind of path it refers to. But I disagree that we > should explain how this works in detail, because this is not about any > HAProxy specific stuff, but about the "inner workings" of Lua and thus > it's best explained by the Lua documentation. We would only be able to > "re-explain" the existing documentation, but probably worse, because we > miss out on important details. Except that the terminology here is totally misleading. I didn't understand your proposed example even when reading the commit message and the proposed doc. This solely because Lua uses the term "path" to designate a pattern, while in traditional environments a path is more a list of directories. *This* must be mentioned. It's not acceptable to have a config documentation saying "hehe I caught you, you thought you would find the info you were looking for here, but no, please visit this link instead, and too bad for you if the net access is down due to haproxy not starting because of the wrong config you're trying to fix, have a good night sir". I'm not suggesting to paraphrase the Lua's doc, just to mention the few elements that are sufficient for someone having to edit an entry to understand what he/she's doing. The link is then provided to have the details. And by the way it should also be clear what happens if multiple such directives are specified (I have no idea). Regards, Willy
Re: [PATCH] MINOR: cli: use global unix settings for stats/master sockets
Regarding the master CLI:
On Thu, Jan 09, 2020 at 02:31:19PM +, William Dauchy wrote:
> that's a good subject to talk about. While looking at this I also
> wondered why it was not possible to configure it in the config file; to my
> knowledge there is no explanation about it in the doc, nor in the code,
> but I might have missed something.
> I did not know about those concerns around being able to access CLI even
> if the config file is corrupted, thanks for the clarification.
>
In fact it's supposed to be configured in your unit file or from your init
script (/etc/{default,sysconfig} etc.).
The problem is that the configuration for HAProxy and for the processes are in
the same configuration file, and if something is wrong, it won't work anymore.
And the master is supposed to survive a reload with a wrong configuration.
> That being said, it makes my patch less likely to be acceptable, but I
> still confirm the user point of view where:
> - you don't get why you cannot configure the master CLI in the
> configuration file; that's probably a point to address in the doc,
> since the configuration doc mentioned the master cli, but you find how
> to set it up in the management doc.
Hm you are right, I think it needs some more doc about the master process in
general.
Thanks!
--
William Lallemand
Re: [PATCH] MINOR: cli: use global unix settings for stats/master sockets
Thanks for your answers. On Thu, Jan 09, 2020 at 06:17:29AM +0100, Willy Tarreau wrote: > I had a look at how this currently works and am embarrassed by both the > patch and the way things currently work. Indeed, the patch only makes > use of the mode, uid and gid from the unix-bind statement and silently > ignores the path. It would be tempting to decide that since it's a unix > socket we should enfore all of unix-bind settings to the stats socket, > but then the problem caused by unix-bind is that the path component is > a mandatory prefix that is prepended before all socket paths. So if we > enforce the path we'll break all configs already using unix-bind. > > I tend to think that at some point we could decide to purposely break > some of this stuff so that the stats socket is not special at all, but > I fear that some configs could not be expressed anymore due to this, > and typically users will place the stats socket into a location outside > of the chroot so that it cannot be accessed by accident by the process. > That's even more true for the master socket where the path is specified > on the command line, regardless of any global setting. > > So maybe in the end your approach is the most reasonable one. However > in this case we should explicitly state in the doc what settings from > the unix-bind directive are reused by the stats/master socket, because > to be transparent, I wasn't aware of the other ones beyond "prefix" and > that's the first thing I tried and was surprized not to see it work as > I imagined it would. > > What do you think ? I was indeed puzzled for the exact same reasons you mentioned but I badly felt lazy expressing them (shame on me). I also think we should consider those sockets as any others, and so take into account the prefix as well. This will make everything clearer from the user point of view, even though this will come with a breaking change. Indeed, when I started using master socket, I was a bit frustrated to have to replicate the rights settings in two different places instead of having one single coherent one. On Thu, Jan 09, 2020 at 12:00:43PM +0100, William Lallemand wrote: > In my opinion that's not a good idea for the master CLI as it shouldn't depend > on the configuration file. As a reminder the master CLI must work when a > configuration file is corrupted, in "waitpid" mode. In this mode there is no > configuration and the master CLI is running only with the configuration used > in > argument. that's a good subject to talk about. While looking at this I also wondered why it was not possible to configure it in the config file; to my knowledge there is no explanation about it in the doc, nor in the code, but I might have missed something. I did not know about those concerns around being able to access CLI even if the config file is corrupted, thanks for the clarification. That being said, it makes my patch less likely to be acceptable, but I still confirm the user point of view where: - you don't get why you cannot configure the master CLI in the configuration file; that's probably a point to address in the doc, since the configuration doc mentioned the master cli, but you find how to set it up in the management doc. - having to replicate same settings is frustrating even though our config is generated - when reading "stats socket" doc, it is mentioned all settings in "bind" are supported, but while reading "unix-bind" it becomes confusing whether this will be taken into account or not. -- William
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
Willy, Am 09.01.20 um 04:07 schrieb Willy Tarreau: > I'm personally not opposed to this at all, however after reading the link > above and given that Lua's concept of "path" is all but obvious as described > in the link above, we definitely need a bit more text in the doc to quickly > explain that the path is rather a pattern and that the question mark is > replaced by the file in "require", and probably put a short example. Having > the link for more details is fine but not as a redirect to figure how to > use the config keyword. I concede that the HAProxy documentation should mention `package.path` to be clear what kind of path it refers to. But I disagree that we should explain how this works in detail, because this is not about any HAProxy specific stuff, but about the "inner workings" of Lua and thus it's best explained by the Lua documentation. We would only be able to "re-explain" the existing documentation, but probably worse, because we miss out on important details. Best regards Tim Düsterhus
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
Adis, Am 09.01.20 um 10:27 schrieb Adis Nezirovic: > global > lua-load /path/to/config.lua > lua-load /path/to/app.lua > […] > And now for the main topic, if you need to set any paths for Lua > scripts/libraries in non standard locations, you can directly use > package.path in config.lua script > > package.path = package.path .. ';/some/path/?.lua' > > > That way, there is no need for users to edit app script, and they can > override configuration or Lua path settings. IMO, we don't need another > directive in HAProxy just to set Lua paths. Yes, I am aware that this would work. I dislike needing an additional configuration file that would contain a single line more often than not. Because then I need to context switch to understand the configuration. It would also make it easier to explain for administrators to set-up. I can just tell them: "put haproxy-lua-http into /etc/haproxy/lua-modules and add that folder to `lua-prepend-path` and lua-load haproxy-auth-request" vs "put haproxy-lua-http somewhere, then create another Lua script containing package.path = package.path .. ';/that/folder/?.lua' and then lua-load that one in addition to haproxy-auth-request". However this morning I started thinking whether it might make sense to generalize `lua-prepend-path` to `lua-run-string` that would execute the Lua script given as the parameter. It might provide better flexibility and would be as readable as lua-prepend-path: lua-run-string "package.path = package.path .. ';/some/path/?.lua'" Opinions? Best regards Tim Düsterhus
Re: [PATCH] MINOR: cli: use global unix settings for stats/master sockets
On Thu, Jan 09, 2020 at 06:17:29AM +0100, Willy Tarreau wrote: > Hi William, > > On Thu, Jan 09, 2020 at 12:26:06AM +0100, William Dauchy wrote: > > allows to use unix-bind settings in config file for both stats and master > > sockets; this will save some double painful config when you can rely on > > the global unix-bind. > > Local settings will still overload the default global. > > I had a look at how this currently works and am embarrassed by both the > patch and the way things currently work. Indeed, the patch only makes > use of the mode, uid and gid from the unix-bind statement and silently > ignores the path. It would be tempting to decide that since it's a unix > socket we should enfore all of unix-bind settings to the stats socket, > but then the problem caused by unix-bind is that the path component is > a mandatory prefix that is prepended before all socket paths. So if we > enforce the path we'll break all configs already using unix-bind. > > I tend to think that at some point we could decide to purposely break > some of this stuff so that the stats socket is not special at all, but > I fear that some configs could not be expressed anymore due to this, > and typically users will place the stats socket into a location outside > of the chroot so that it cannot be accessed by accident by the process. > That's even more true for the master socket where the path is specified > on the command line, regardless of any global setting. > > So maybe in the end your approach is the most reasonable one. However > in this case we should explicitly state in the doc what settings from > the unix-bind directive are reused by the stats/master socket, because > to be transparent, I wasn't aware of the other ones beyond "prefix" and > that's the first thing I tried and was surprized not to see it work as > I imagined it would. > > What do you think ? > > thanks, > Willy > In my opinion that's not a good idea for the master CLI as it shouldn't depend on the configuration file. As a reminder the master CLI must work when a configuration file is corrupted, in "waitpid" mode. In this mode there is no configuration and the master CLI is running only with the configuration used in argument. -- William Lallemand
Re: Lua: forcing garbage collector after socket i/o
On Thu, Jan 09, 2020 at 10:56:59AM +0100, Thierry Fournier wrote: > >> Maybe we try to include new option "tune.lua.forced-gc". > > > > Based on your description if we need an option, I'd rather have the > > opposite, like "tune.lua.disable-gc" so that it remains enabled by > > default, and with an explanation that one must not disable it. > > > I agree, but it make sense to run some tests: > > - First, testing your patch to decide if the GC line 1195 is relevant > - Second, testing the behavior when GC is disabled and lua create connexion > and lua fail without closing connexion. > > Maybe the second case make the GC mandatory. Probably but then it should only be needed in hlua_ctx_destroy(), no ? My understanding is that the only goal is to be sure to kill such an orphan connection in case the Lua application dies, so I tend to think it's never needed in resume (I don't see how we'd resume after dying) and that hlua_ctx_destroy() should always be called for the cleanup so it should possibly be enough. Willy
Re: Lua: forcing garbage collector after socket i/o
> On 9 Jan 2020, at 10:47, Willy Tarreau wrote: > > Hi Thierry, > > On Thu, Jan 09, 2020 at 10:34:35AM +0100, Thierry Fournier wrote: >> I'm remember: the execution of the GC were the only way to close TCP >> connexions >> because in some cases the object provided by core.tcp() is not closed and the >> connexion is not released. Without the GC execution, HAProxy reach the >> maximum >> of available connexion, and the process were blocked. The flag forcing the >> GCC >> is set only is we use a socket. > > Ah OK then it makes sense, thanks for the explanation. However I'm seeing > that the GC is forced for every yield/resume call, and not just on destroy. > Maybe it could be acceptable not to call it on resume ? > > Sada, would you be interested in checking if this solves most of the > performance issue: > > diff --git a/src/hlua.c b/src/hlua.c > index 37f7866874..e5257efb54 100644 > --- a/src/hlua.c > +++ b/src/hlua.c > @@ -1195,7 +1195,7 @@ resume_execution: >} > >/* This GC permits to destroy some object when a Lua timeout strikes. > */ > - if (lua->flags & HLUA_MUST_GC && > + if (0 && lua->flags & HLUA_MUST_GC && >ret ! HLUA_E_AGAIN) >lua_gc(lua->T, LUA_GCCOLLECT, 0); This make sense, but I not remember if the GC at this point is important. Just for information, there are the commit messages: OPTIM/MEDIUM: lua: executes the garbage collector only when using cosocket The garbage collector is a little bit heavy to run, and it was added only for cosockets. This patch prevent useless executions when no cosockets are used. > >> Maybe we try to include new option "tune.lua.forced-gc". > > Based on your description if we need an option, I'd rather have the > opposite, like "tune.lua.disable-gc" so that it remains enabled by > default, and with an explanation that one must not disable it. I agree, but it make sense to run some tests: - First, testing your patch to decide if the GC line 1195 is relevant - Second, testing the behavior when GC is disabled and lua create connexion and lua fail without closing connexion. Maybe the second case make the GC mandatory. Thierry
Re: Lua: forcing garbage collector after socket i/o
Hi Thierry, On Thu, Jan 09, 2020 at 10:34:35AM +0100, Thierry Fournier wrote: > I'm remember: the execution of the GC were the only way to close TCP > connexions > because in some cases the object provided by core.tcp() is not closed and the > connexion is not released. Without the GC execution, HAProxy reach the maximum > of available connexion, and the process were blocked. The flag forcing the GCC > is set only is we use a socket. Ah OK then it makes sense, thanks for the explanation. However I'm seeing that the GC is forced for every yield/resume call, and not just on destroy. Maybe it could be acceptable not to call it on resume ? Sada, would you be interested in checking if this solves most of the performance issue: diff --git a/src/hlua.c b/src/hlua.c index 37f7866874..e5257efb54 100644 --- a/src/hlua.c +++ b/src/hlua.c @@ -1195,7 +1195,7 @@ resume_execution: } /* This GC permits to destroy some object when a Lua timeout strikes. */ - if (lua->flags & HLUA_MUST_GC && + if (0 && lua->flags & HLUA_MUST_GC && ret != HLUA_E_AGAIN) lua_gc(lua->T, LUA_GCCOLLECT, 0); > Maybe we try to include new option "tune.lua.forced-gc". Based on your description if we need an option, I'd rather have the opposite, like "tune.lua.disable-gc" so that it remains enabled by default, and with an explanation that one must not disable it. Thanks, Willy
Re: Lua: forcing garbage collector after socket i/o
Hi, I try this morning to reproduce the behaviour, but I do not have sufficient time to finish the test. I’ml not surprised by the described behaviour because we are talking about GC. GC are program written to save the brain of developper implying a permanent overconsumption of the CPU. So GC are complex algorithms using most CPU and impacting performances. I’m remember: the execution of the GC were the only way to close TCP connexions because in some cases the object provided by core.tcp() is not closed and the connexion is not released. Without the GC execution, HAProxy reach the maximum of available connexion, and the process were blocked. The flag forcing the GCC is set only is we use a socket. Maybe we try to include new option "tune.lua.forced-gc”. br, Thierry — Thierry Fournier Web Performance & Security Expert m: +33 6 68 69 21 85 | e: thierry.fourn...@ozon.io w: http://www.ozon.io/| b: http://blog.ozon.io/ > On 9 Jan 2020, at 06:25, Willy Tarreau wrote: > > Hi, > > On Thu, Jan 02, 2020 at 04:36:58PM -0800, Sadasiva Gujjarlapudi wrote: >> Hi, >> We have observed (significant)request handling rate dropped if the TCP >> sockets were used in `http-request` action. >> Request handling rate recovered with slight increase in memory usage after >> commenting the line in `hlua_socket_connect()`/hlua.c >> `hlua->flags | HLUA_MUST_GC` >> Doc: >> https://antiphishing.ozon.io/4/0@dIg8fWe3Pg0@uv152ypkX71_NyCG5u2ry1B654tGLFq2UKabFItkBLEp7NFIDiBuEUqAAogidiDiOGvMnTSTFK3ClXj9Ozc9-0FWyvq_z7qztGU6-WYKict5U8Wp-m3Gr7K1h-B9elAz@73c1dfb44abdfca30a93012148bd1e7ebfe3dee2 >> >> I want to add a param to `Socket:connect()` or add new method >> `Socket:connect2()`. >> to disable forced GC. >> Any feedback is appreciated. > > Interesting! CCing Thierry who's the one who knows this best. Thierry, > please also have a look at Sada's follow up e-mail with his numbers and > config. It looks like the GC is particularly aggressive here, and I don't > know if this is really needed. Or maybe we could have a counter and only > force it once every 100 times or so ? > > thanks, > Willy
Re: [PATCH] CLEANUP: server: remove unused err section in server_finalize_init
Hello, On Thu, 9 Jan 2020 at 06:08, Илья Шипицин wrote: > > btw, if you add "Fixes: #438", the issue will be closed automatically > > https://help.github.com/en/github/managing-your-work-on-github/closing-issues-using-keywords which we are asking people *NOT* to do, because we are also tracking backports in the github issues. Issues will be closed manually on Github. Thanks, Lukas
Re: [PATCH] MINOR: lua: Add lua-prepend-path configuration option
On 1/8/20 10:54 PM, Tim Duesterhus wrote:
List,
while working on updating my haproxy-auth-request [1] to make use of
haproxy-lua-http [2] instead of the crappy lua-socket [3] I realized
that it would be difficult for the administrator to use the script
afterwards.
The reason is that `require` in Lua attempts to search the modules
either in the current working directory (which is different from
the filename of the script executing the `require` and which most
likely is not anywhere near HAProxy's configuration folder) or within
Lua's library path which is compiled into Lua (in my case this is
something like `/usr/share/lua/5.3/`).
Add a `lua-prepend-path` configuration option that prepend the given
string to Lua's path, allowing scripts loaded with `lua-load` to find
libraries within that path.
Example configuration:
global
lua-prepend-path /etc/haproxy/lua-modules/?/init.lua
lua-prepend-path /etc/haproxy/lua-modules/?.lua
lua-load /etc/haproxy/lua-modules/auth-request.lua
Hey Tim,
For what is worth, we solve situations like this with regular script,
loaded before main script:
global
lua-load /path/to/config.lua
lua-load /path/to/app.lua
Inside config.lua you can override your app configuration options like this
conf = { opt1 = "val" }
Then, app.lua contains the following pattern
if not conf then
conf = { ... } -- defaults
else
if not conf.opt1 then conf.opt1 = "val" end
...
end
And now for the main topic, if you need to set any paths for Lua
scripts/libraries in non standard locations, you can directly use
package.path in config.lua script
package.path = package.path .. ';/some/path/?.lua'
That way, there is no need for users to edit app script, and they can
override configuration or Lua path settings. IMO, we don't need another
directive in HAProxy just to set Lua paths.
Best regards
--
Adis Nezirovic
Software Engineer
HAProxy Technologies - Powering your uptime!
375 Totten Pond Road, Suite 302 | Waltham, MA 02451, US
+1 (844) 222-4340 | https://www.haproxy.com
Re: [PATCH] MINOR: http: Add 410 to http-request deny
On 09 Jan 06:01, Willy Tarreau wrote: > On Wed, Jan 08, 2020 at 01:26:00PM +0100, Julien Pivotto wrote: > > While we are at it, could we add 404 as well? > > > > 404 is frequently used to deny to hide the fact that the access is > > denied, see > > https://developer.github.com/v3/troubleshooting/#why-am-i-getting-a-404-error-on-a-repository-that-exists > > > > I know there are workarounds for this like service an error file but > > getting it out of the box would be nice. > > Agreed, I remember someone proposing it a year or two ago already and it > does make sense. Feel free to propose a patch, Florian's can serve as an > example. > > Willy Florian was very kind and implemented it. Thank you Florian! -- (o-Julien Pivotto //\Open-Source Consultant V_/_ Inuits - https://www.inuits.eu signature.asc Description: PGP signature
