On Mon, Jun 15, 2020 at 07:00:55PM +0200, Willy Tarreau wrote: > Hi Tim, > > On Sun, Jun 14, 2020 at 06:24:19PM +0200, Tim Düsterhus wrote: > > Hi List, > > Willy, > > Ilya, > > > > I noticed that the reg-tests were unable find the issue reported by > > William here: > > https://www.mail-archive.com/haproxy@formilux.org/msg37637.html > > > > This is because VTest never performs a "soft" shutdown of HAProxy, > > instead it uses SIGINT -> SIGTERM -> SIGKILL. Thus the deinit() will > > never trigger. > > > > Fixing this will require a change to VTest: > > https://github.com/vtest/VTest/blob/b9e9e03fdeebd494783ae1dd8e6008f5c1e3a4bc/src/vtc_haproxy.c#L786 > > needs to be SIGUSR1 (and the switch below adjusted). > > > > Making the change in my local repository and running the tests that > > don't need any additional USE_XXX falgs causes 3 tests to fail with 2 of > > them triggering an assertion within VTest. > > > > It would have caught the bug reported by William, though. > > > > Concluding: It probably makes sense to adjust VTest to use SIGUSR1 > > instead of SIGINT, the latter is handled like SIGTERM in HAProxy anyway, > > so there is no need for this distinction. I did not yet look into the > > details of the failing tests, though. > > It *could* be a solution, I don't know if it may have other impacts. > > Actually we must always remember that while convenient, VTest's > primary goal is to test a proxy by synchronizing the two sides (which > is what basically no other testing tool can reliably do). If we want > to run deeper tests on other process-oriented behaviors, it can make > sense to rely on other tools. For example vtest is not the best suited > to testing command line or process stability. It has no process > management, can leak background processes and needs to wait for a > timeout to detect a crash. > > My point above is that as long as *proxy* tests are compatible with > stopping using SIGUSR1 I think I'd be fine with the change. But if > doing this actually results in more pain to test the proxy features, > I'd rather stay away from this and switch to a distinct test series > for this. That's where I'd draw the line. >
I think that's a good idea but It will probably because it will let us test the deinit() with all the diversity of configuration we have in the reg-tests. But I also agree with Willy and we should be careful about the consequences of this change. If there is too much changes to handle it may be painful to do it before the 2.2 release. -- William Lallemand
