Merge ~bryce/ubuntu/+source/apache2:sru-lp1832182-graceful-groovy into ubuntu/+source/apache2:ubuntu/devel

Proposed by Bryce Harrington
Status: Rejected
Rejected by: Bryce Harrington
Proposed branch: ~bryce/ubuntu/+source/apache2:sru-lp1832182-graceful-groovy
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
Christian Ehrhardt  Needs Fixing
Ubuntu Server Developers Pending
Canonical Server Team Pending
Review via email: mp+391856@code.launchpad.net

Description of the change

Fix for a server-next Apache2 bug. The objective is to SRU this to all past LTS releases, so please review with that intent in mind.

The bug description for LP: #1832182 is prepped for the SRU, so I likewise would appreciate a detailed review of that text.

PPA: https://launchpad.net/~bryce/+archive/ubuntu/apache2-sru-lp1832182/

To post a comment you must log in.
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

SRU Template - LGTM
Changelog - LGTM

Actual change:
- ack to move the systemd check to a function

I needed some time for "APACHE_STARTED_BY_SYSTEMD".
That is set in the environment by the .service files.
So I assume if APACHE_STARTED_BY_SYSTEMD isn't set we probe (the new function).
But if it is set, then we exit 1 which means "no need for systemd"

is that
a) to unbreak loops where we start this from systemd, starting systemd, starting systemd, ...
   Then it makes sense but I wanted to ask ito be sure
b) wrong and should actually "exit 0" if APACHE_STARTED_BY_SYSTEMD is set?

Further in the change there is some whitespace damage with the "exit" calls not using spaces but tabs.

If you could fix the latter and explain the former then this should be fine to go.

review: Needs Fixing
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Any updates on that ? it clutters my "active reviews" page :-P

Revision history for this message
Bryce Harrington (bryce) wrote :

Sorry, yeah I realized from your review that my original implementation was totally borked so have recoded it entirely. I noticed another corner case where the service state could get forgotten, so thought I should do some broader testing before reproposing the MP. Unfortunately, the past couple weeks were heavily side-tracked with other higher urgencies, but this (and php-parser) are back at the top of the stack so hope to have the MP up tomorrow or Monday.

Revision history for this message
Bryce Harrington (bryce) wrote :
5b03b08... by Bryce Harrington

changelog

Unmerged commits

5b03b08... by Bryce Harrington

changelog

37b368f... by Bryce Harrington

* 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.

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..517a355 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
1apache2 (2.4.46-1ubuntu1.1) groovy; 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> Fri, 13 Nov 2020 01:36:38 +0000
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