Merge ~paelzer/ubuntu/+source/haproxy:bug-1848902-hang-on-exit-143-BIONIC into ubuntu/+source/haproxy:ubuntu/bionic-devel

Proposed by Christian Ehrhardt  on 2019-11-12
Status: Merged
Approved by: Christian Ehrhardt  on 2019-11-18
Approved revision: d0d17a603b126f2c2239d8903a5713b529cc4c81
Merged at revision: d0d17a603b126f2c2239d8903a5713b529cc4c81
Proposed branch: ~paelzer/ubuntu/+source/haproxy:bug-1848902-hang-on-exit-143-BIONIC
Merge into: ubuntu/+source/haproxy:ubuntu/bionic-devel
Diff against target: 91 lines (+69/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/lp-1848902-MINOR-systemd-consider-exit-status-143-as-successful.patch (+61/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Bryce Harrington 2019-11-12 Needs Fixing on 2019-11-16
Canonical Server Team 2019-11-12 Pending
Canonical Server packageset reviewers 2019-11-12 Pending
Review via email: mp+375433@code.launchpad.net
To post a comment you must log in.
Bryce Harrington (bryce) wrote :

- [√] changelog entry correct, targeted to correct codename
- [√] no major upstream changes to consider
- [√] debian changes look safe
- [√] update-maintainer has been run
- [-] changes forwarded upstream/debian (if appropriate)
- [-] nothing else to drop
- [√] patches match what was proposed upstream
- [√] patches correctly included in debian/patches/series?
- [√] patches have correct DEP3 metadata

== Testing ==

$ lxc launch ubuntu:18.04/amd64 bug-1848902-haproxy-hang
$ lxc exec bug-1848902-haproxy-hang -- bash

== Existing (broken) case ==
# apt-get install haproxy=1.8.8-1ubuntu0.7
# systemctl status haproxy | grep Active:
   Active: active (running) since Sat 2019-11-16 20:41:51 UTC; 1min 32s ago

# pkill -TERM -x haproxy
# systemctl status haproxy
   Active: active (running) since Sat 2019-11-16 20:43:52 UTC; 1s ago

# for x in {1..100}; do pkill -TERM -x haproxy ; sleep 0.1 ; done
# systemctl status haproxy | grep Active:
   Active: failed (Result: exit-code) since Sat 2019-11-16 20:44:53 UTC; 18s ago

== PPA Version ==
# add-apt-repository ppa:paelzer/bug-1848902-haproxy-hang
# apt install haproxy
# apt-cache policy haproxy | grep Installed:
  Installed: 1.8.8-1ubuntu0.8~ppa1
# systemctl status haproxy | grep Active:
   Active: active (running) since Sat 2019-11-16 20:46:52 UTC; 29s ago

# pkill -TERM -x haproxy
# systemctl status haproxy | grep Active:
   Active: active (running) since Sat 2019-11-16 20:49:10 UTC; 917ms ago

# for x in {1..100}; do pkill -TERM -x haproxy ; sleep 0.1 ; done
# systemctl status haproxy | grep Active:
   Active: failed (Result: start-limit-hit) since Sat 2019-11-16 20:50:18 UTC; 9s ago

---

I think the test case would be clearer if the end result was haproxy remained actively running, rather than fail just for a different reason, however it is failing for the documented reason (start limit hit) instead of the original problem (exit code), so technically seems to be correct and behaving as designed, so +1.

review: Approve
Bryce Harrington (bryce) wrote :

Figured out a way to hackaround the rate limiter in the test case for the SRU template:

for x in {1..100}; do pkill -TERM -x haproxy ; sleep 0.1 ; systemctl reset-failed haproxy.service; done

With that, the test case should result in success with the fixed case.

Otherwise, once you hit x==6 in the loop systemd just kills off the service and you don't exercise the test coverage for the remaining 95 steps of the counter. I couldn't find a way to configure this per-service (I experimented with StartLimitBurst=101 and StartLimitIntervalSec=<N> but no effect), and didn't spot a way to configure this in systemd globally), but the brute force reset-failed should be sufficient for the test case.

review: Needs Fixing
Christian Ehrhardt  (paelzer) wrote :

Thanks for the modified test bryce, but with it also the old code remains an active service.
I extended the steps in the SRU template.
I admit the test isn't perfect at all, but that is the nature with races.

At least the fix is accepted upstream and in addition fact that active users already run the accepted RC in the field convinces me to go forward.

Christian Ehrhardt  (paelzer) wrote :

To ssh://git.launchpad.net/~usd-import-team/ubuntu/+source/haproxy
 * [new tag] upload/1.8.8-1ubuntu0.8 -> upload/1.8.8-1ubuntu0.8

$ dput ubuntu ../haproxy_1.8.8-1ubuntu0.8_source.changes
Checking signature on .changes
gpg: ../haproxy_1.8.8-1ubuntu0.8_source.changes: Error checking signature from BA3E29338280B242: SignatureVerifyError: 0
Checking signature on .dsc
gpg: ../haproxy_1.8.8-1ubuntu0.8.dsc: Error checking signature from BA3E29338280B242: SignatureVerifyError: 0
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading haproxy_1.8.8-1ubuntu0.8.dsc: done.
  Uploading haproxy_1.8.8-1ubuntu0.8.debian.tar.xz: done.
  Uploading haproxy_1.8.8-1ubuntu0.8_source.buildinfo: done.
  Uploading haproxy_1.8.8-1ubuntu0.8_source.changes: done.
Successfully uploaded packages.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/debian/changelog b/debian/changelog
2index 8470612..a388302 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+haproxy (1.8.8-1ubuntu0.8) bionic; urgency=medium
7+
8+ * d/p/lp-1848902-MINOR-systemd-consider-exit-status-143-as-successful.patch:
9+ fix potential hang in haproxy (LP: #1848902)
10+
11+ -- Christian Ehrhardt <christian.ehrhardt@canonical.com> Tue, 12 Nov 2019 13:16:22 +0100
12+
13 haproxy (1.8.8-1ubuntu0.7) bionic-security; urgency=medium
14
15 * SECURITY UPDATE: Messages with transfer-encoding header missing "chunked"
16diff --git a/debian/patches/lp-1848902-MINOR-systemd-consider-exit-status-143-as-successful.patch b/debian/patches/lp-1848902-MINOR-systemd-consider-exit-status-143-as-successful.patch
17new file mode 100644
18index 0000000..1d782f9
19--- /dev/null
20+++ b/debian/patches/lp-1848902-MINOR-systemd-consider-exit-status-143-as-successful.patch
21@@ -0,0 +1,61 @@
22+From fdc6c62dbebf4b646b4f80c383e3b00f34b0440f Mon Sep 17 00:00:00 2001
23+From: Vincent Bernat <vincent@bernat.im>
24+Date: Fri, 22 Jun 2018 20:57:03 +0200
25+Subject: [PATCH] MINOR: systemd: consider exit status 143 as successful
26+MIME-Version: 1.0
27+Content-Type: text/plain; charset=UTF-8
28+Content-Transfer-Encoding: 8bit
29+
30+The master process will exit with the status of the last worker. When
31+the worker is killed with SIGTERM, it is expected to get 143 as an
32+exit status. Therefore, we consider this exit status as normal from a
33+systemd point of view. If it happens when not stopping, the systemd
34+unit is configured to always restart, so it has no adverse effect.
35+
36+This has mostly a cosmetic effect. Without the patch, stopping HAProxy
37+leads to the following status:
38+
39+ ● haproxy.service - HAProxy Load Balancer
40+ Loaded: loaded (/lib/systemd/system/haproxy.service; disabled; vendor preset: enabled)
41+ Active: failed (Result: exit-code) since Fri 2018-06-22 20:35:42 CEST; 8min ago
42+ Docs: man:haproxy(1)
43+ file:/usr/share/doc/haproxy/configuration.txt.gz
44+ Process: 32715 ExecStart=/usr/sbin/haproxy -Ws -f $CONFIG -p $PIDFILE $EXTRAOPTS (code=exited, status=143)
45+ Process: 32714 ExecStartPre=/usr/sbin/haproxy -f $CONFIG -c -q $EXTRAOPTS (code=exited, status=0/SUCCESS)
46+ Main PID: 32715 (code=exited, status=143)
47+
48+After the patch:
49+
50+ ● haproxy.service - HAProxy Load Balancer
51+ Loaded: loaded (/lib/systemd/system/haproxy.service; disabled; vendor preset: enabled)
52+ Active: inactive (dead)
53+ Docs: man:haproxy(1)
54+ file:/usr/share/doc/haproxy/configuration.txt.gz
55+
56+(cherry picked from commit 3b479bd5f5f50ce91cabed32bb26556313552d23)
57+Signed-off-by: William Lallemand <wlallemand@haproxy.org>
58+
59+Origin: upstream, http://git.haproxy.org/?p=haproxy-1.8.git;a=commit;h=fdc6c62dbebf4b646b4f80c383e3b00f34b0440f
60+Bug-Ubuntu: https://bugs.launchpad.net/bugs/1848902
61+Applied-Upstream: 1.8.13
62+Last-Update: 2019-11-12
63+
64+---
65+ contrib/systemd/haproxy.service.in | 1 +
66+ 1 file changed, 1 insertion(+)
67+
68+diff --git a/contrib/systemd/haproxy.service.in b/contrib/systemd/haproxy.service.in
69+index 7a8b6bead..74e66e302 100644
70+--- a/contrib/systemd/haproxy.service.in
71++++ b/contrib/systemd/haproxy.service.in
72+@@ -10,6 +10,7 @@ ExecReload=@SBINDIR@/haproxy -f $CONFIG -c -q
73+ ExecReload=/bin/kill -USR2 $MAINPID
74+ KillMode=mixed
75+ Restart=always
76++SuccessExitStatus=143
77+ Type=notify
78+
79+ # The following lines leverage SystemD's sandboxing options to provide
80+--
81+2.24.0
82+
83diff --git a/debian/patches/series b/debian/patches/series
84index 295e83d..ac811fe 100644
85--- a/debian/patches/series
86+++ b/debian/patches/series
87@@ -14,3 +14,4 @@ stksess-align.patch
88 lp-1841936-CLEANUP-ssl-make-ssl_sock_load_dh_params-handle-errc.patch
89 lp-1841936-BUG-MEDIUM-ssl-tune.ssl.default-dh-param-value-ignor.patch
90 CVE-2019-18277.patch
91+lp-1848902-MINOR-systemd-consider-exit-status-143-as-successful.patch

Subscribers

People subscribed via source and target branches