Merge ~bryce/ubuntu/+source/apache2:apache2-fix-lp1832182-hirsute into ubuntu/+source/apache2:ubuntu/devel

Proposed by Bryce Harrington on 2020-11-06
Status: Merged
Approved by: Bryce Harrington on 2020-11-19
Approved revision: 2fea15554926ad8a677ad04b3720d73a7cca08d7
Merge reported by: Christian Ehrhardt 
Merged at revision: 2fea15554926ad8a677ad04b3720d73a7cca08d7
Proposed branch: ~bryce/ubuntu/+source/apache2:apache2-fix-lp1832182-hirsute
Merge into: ubuntu/+source/apache2:ubuntu/devel
Diff against target: 98 lines (+40/-18)
2 files modified
debian/apache2ctl (+30/-18)
debian/changelog (+10/-0)
Reviewer Review Type Date Requested Status
Bryce Harrington Approve on 2020-11-19
Lucas Kanashiro 2020-11-06 Needs Fixing on 2020-11-12
Canonical Server packageset reviewers 2020-11-06 Pending
Canonical Server Team 2020-11-06 Pending
Review via email: mp+393426@code.launchpad.net

Description of the change

This is a re-send of a server-next fix for Apache2, which supersedes an earlier MP targeting groovy: https://code.launchpad.net/~bryce/ubuntu/+source/apache2/+git/apache2/+merge/391856

This fixes some errors in the earlier code and incorporates Christian's review comments. I've tested the code via the test case in the bug with some variations, and it seems good. Autopkgtest checks out too, although the test cases don't exercise the 'graceful' command.

The issue is applicable to all past supported releases, so once this is accepted for hirsute I plan to also apply it to groovy, focal, bionic, and xenial. Thus please also review the SRU text in the bug report itself.

To post a comment you must log in.
Lucas Kanashiro (lucaskanashiro) wrote :

I am grabbing this for review.

Lucas Kanashiro (lucaskanashiro) wrote :

The package changes and the SRU bug description LGMT in general, just some comments below:

- In the changelog your target is groovy but this MP is targeting hirsute.

- This is me being nitpick: I would change this commit message:

commit a4a20160eefd3cd0a3cb07d2a95fb9665c1f2b16
Author: Bryce Harrington <email address hidden>
Date: Mon Oct 5 16:03:00 2020 -0700

    * d/apache2ctl: Also use systemd for graceful if it is in use. This extends an earlier fix for the start command to behave similarly for restart / graceful. Fixes service failures on unattended upgrade. (LP: #1832182)

to something like:

    * d/apache2ctl: Also use systemd for graceful if it is in use (LP: #1832182).

    This extends an earlier fix for the start command to behave similarly for
    restart / graceful. Fixes service failures on unattended upgrade.

It is a better reading IMO.

- I see this same bug was reported to Debian here:

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927302

Could we forward these changes to Debian as well?

Apart from that the package built fine and I was able to confirm that the fix is working as expected (following the instructions in the SRU bug). I also confirmed that autopkgtest did not identify any regression (despite it does not exercise the path of proposed changes):

autopkgtest [10:51:10]: @@@@@@@@@@@@@@@@@@@@ summary
run-test-suite PASS
duplicate-module-load PASS
htcacheclean PASS
default-mods PASS
ssl-passphrase PASS
check-http2 PASS
chroot PASS

review: Needs Fixing
Bryce Harrington (bryce) wrote :

Thanks for the review. I've incorporated the suggested changes.

I've forwarded the debdiff to debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927302

Branch and PPA are updated.

Bryce Harrington (bryce) wrote :

Since I incorporated all the review feedback, and since Christian also reviewed and approved the same changes for the SRUs I've gone ahead and uploaded this one too.

review: Approve
Christian Ehrhardt  (paelzer) wrote :

This is fix released in hirsute -> merged

Update scan failed

At least one of the branches involved have failed to scan. You can manually schedule a rescan if required.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
diff --git a/debian/apache2ctl b/debian/apache2ctl
index 404b9f9..1358f2a 100755
--- a/debian/apache2ctl
+++ b/debian/apache2ctl
@@ -143,6 +143,18 @@ mkdir_chown () {
143 fi143 fi
144}144}
145145
146need_systemd () {
147 # Detect if systemd is in use and should be used for managing
148 # the Apache2 httpd service. Returns 0 if so, 1 otherwise.
149 if [ -z "${APACHE_STARTED_BY_SYSTEMD}" ]; then
150 case "$(readlink -f /proc/1/exe)" in
151 *systemd*)
152 return 0
153 ;;
154 esac
155 fi
156 return 1
157}
146158
147[ ! -d ${APACHE_RUN_DIR:-/var/run/apache2} ] && mkdir -p ${APACHE_RUN_DIR:-/var/run/apache2}159[ ! -d ${APACHE_RUN_DIR:-/var/run/apache2} ] && mkdir -p ${APACHE_RUN_DIR:-/var/run/apache2}
148[ ! -d ${APACHE_LOCK_DIR:-/var/lock/apache2} ] && mkdir_chown ${APACHE_RUN_USER:-www-data} ${APACHE_LOCK_DIR:-/var/lock/apache2}160[ ! -d ${APACHE_LOCK_DIR:-/var/lock/apache2} ] && mkdir_chown ${APACHE_RUN_USER:-www-data} ${APACHE_LOCK_DIR:-/var/lock/apache2}
@@ -153,38 +165,38 @@ start)
153 # (this is bad if there are several apache2 instances running)165 # (this is bad if there are several apache2 instances running)
154 rm -f ${APACHE_RUN_DIR:-/var/run/apache2}/*ssl_scache*166 rm -f ${APACHE_RUN_DIR:-/var/run/apache2}/*ssl_scache*
155167
156 need_systemd=false168 if need_systemd; then
157 if [ -z "$APACHE_STARTED_BY_SYSTEMD" ] ; then
158 case "$(readlink -f /proc/1/exe)" in
159 *systemd*)
160 need_systemd=true
161 ;;
162 *)
163 ;;
164 esac
165 fi
166 if $need_systemd ; then
167 # If running on systemd we should not start httpd without systemd169 # If running on systemd we should not start httpd without systemd
168 # or systemd will get confused about the status of httpd.170 # or systemd will get confused about the status of httpd.
169 echo "Invoking 'systemctl start $APACHE_SYSTEMD_SERVICE'."171 echo "Invoking 'systemctl start ${APACHE_SYSTEMD_SERVICE}'."
170 echo "Use 'systemctl status $APACHE_SYSTEMD_SERVICE' for more info."172 echo "Use 'systemctl status ${APACHE_SYSTEMD_SERVICE}' for more info."
171 systemctl start "$APACHE_SYSTEMD_SERVICE"173 systemctl start "${APACHE_SYSTEMD_SERVICE}"
172 else174 else
173 unset APACHE_STARTED_BY_SYSTEMD175 unset APACHE_STARTED_BY_SYSTEMD
174 $HTTPD ${APACHE_ARGUMENTS} -k "$ARGV"176 ${HTTPD} ${APACHE_ARGUMENTS} -k "${ARGV}"
175 fi177 fi
176178
177 ERROR=$?179 ERROR=$?
178 ;;180 ;;
179stop|graceful-stop)181stop|graceful-stop)
180 $HTTPD ${APACHE_ARGUMENTS} -k "$ARGV"182 ${HTTPD} ${APACHE_ARGUMENTS} -k "$ARGV"
181 ERROR=$?183 ERROR=$?
182 ;;184 ;;
183restart|graceful)185restart|graceful)
184 if $HTTPD ${APACHE_ARGUMENTS} -t 2> /dev/null ; then186 if $HTTPD ${APACHE_ARGUMENTS} -t 2> /dev/null ; then
185 $HTTPD ${APACHE_ARGUMENTS} -k "$ARGV"187 if need_systemd; then
188 # If running on systemd we should not directly restart httpd since
189 # systemd would be confused about httpd's status.
190 # (See LP: #1832182)
191 echo "Invoking 'systemctl restart ${APACHE_SYSTEMD_SERVICE}'."
192 echo "Use 'systemctl status ${APACHE_SYSTEMD_SERVICE}' for more info."
193 systemctl restart "${APACHE_SYSTEMD_SERVICE}"
194 else
195 unset APACHE_STARTED_BY_SYSTEMD
196 ${HTTPD} ${APACHE_ARGUMENTS} -k "${ARGV}"
197 fi
186 else198 else
187 $HTTPD ${APACHE_ARGUMENTS} -t199 ${HTTPD} ${APACHE_ARGUMENTS} -t
188 fi200 fi
189 ERROR=$?201 ERROR=$?
190 ;;202 ;;
diff --git a/debian/changelog b/debian/changelog
index 6295e4e..9cdcc74 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
1apache2 (2.4.46-1ubuntu2) hirsute; urgency=medium
2
3 * d/apache2ctl: Also use systemd for graceful if it is in use.
4 (LP: #1832182)
5 - This extends an earlier fix for the start command to behave
6 similarly for restart / graceful. Fixes service failures on
7 unattended upgrade.
8
9 -- Bryce Harrington <bryce@canonical.com> Mon, 05 Oct 2020 16:06:32 -0700
10
1apache2 (2.4.46-1ubuntu1) groovy; urgency=medium11apache2 (2.4.46-1ubuntu1) groovy; urgency=medium
212
3 * Merge with Debian unstable. Remaining changes:13 * Merge with Debian unstable. Remaining changes:

Subscribers

People subscribed via source and target branches