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

[Bryce Harrington]

Backport to groovy of fix from
https://code.launchpad.net/~bryce/ubuntu/+source/apache2/+git/apache2/+merge/393426

The start command was updated to work with systemd earlier; this extends the same fix for the 'graceful' command.

PPA: https://launchpad.net/~bryce/+archive/ubuntu/apache2-sru-lp1832182
SRU: https://bugs.launchpad.net/ubuntu/+source/apache2/+bug/1832182
Forwarded to Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=927302

[Christian Ehrhardt  (paelzer)]

As reference, that old fix for start was at https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=852543 in pkg/import/2.4.25-3

[Christian Ehrhardt  (paelzer)]

I remember reviewing the same fix for another release.
This one still LGTM and makes sense to avoid the direct restart breaking status detection.

Also changelog and version are ok.

The only thing I didn't get to this time is testing, have you run your PPA through upgrade tests already?

review: Approve

[Christian Ehrhardt  (paelzer)]

Actually on the SRU I have one more thought.
A formerly "silent" action like the graceful restart does now emit messages:

  echo "Invoking 'systemctl restart ${APACHE_SYSTEMD_SERVICE}'."
  echo "Use 'systemctl status ${APACHE_SYSTEMD_SERVICE}' for more info."

I agree this is good going forward and also good for "humans" to understand what might happen.
But on an SRU context I'm slightly more concerned people might grep output or such.
I don't have a much better idea yet, maybe pushing the messages to the log (less obvious to be seen, but less potential disruption)?
What do you think?

review: Needs Information

[Bryce Harrington (bryce)]

Thanks for the review.

It's a fair point that in an SRU context that changes to output are effectively behavioral changes. However, I'm not sure it's worth worrying about since a) this affects just the 'graceful' command which had been a bit broken to begin with (thus the bug), and b) the new output is consistent with the output from the 'start' command. People who had been avoiding 'graceful' due to the misbehavior likely will already be dealing with the status output from the 'start' command so if due to this fix they start using graceful they may find the lack of status output weird.

That said, I do find it a bit odd that the start command outputs to stdout rather than stderr, but this seems semi-consistent with similar code. Other places in the code do emit to stderr and I don't know if this is intentional or a discrepancy. In any case, I wanted to make this a minimal fix so am keeping similar to existing code style and not fixing up inconsistencies in the rest of the script, tho I am quite tempted... Maybe someday.

[Christian Ehrhardt  (paelzer)]

Yeah, I'm ok with the argument of keeping it "the same" as start for Bionic->Groovy, thanks for the thoughts and discussion.

review: Approve

[Bryce Harrington (bryce)]

Sounds good, I've tagged, pushed, and uploaded this.

Unmerged commits

5b03b08... by Bryce Harrington on 2020-11-13

changelog

37b368f... by Bryce Harrington on 2020-10-05

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

diff --git a/debian/apache2ctl b/debian/apache2ctl
index 404b9f9..1358f2a 100755
--- a/debian/apache2ctl
+++ b/debian/apache2ctl
@@ -143,6 +143,18 @@ mkdir_chown () {
     fi
 }
 
+need_systemd () {
+    # Detect if systemd is in use and should be used for managing
+    # the Apache2 httpd service.  Returns 0 if so, 1 otherwise.
+    if [ -z "${APACHE_STARTED_BY_SYSTEMD}" ]; then
+	case "$(readlink -f /proc/1/exe)" in
+	    *systemd*)
+		return 0
+		;;
+	esac
+    fi
+    return 1
+}
 
 [ ! -d ${APACHE_RUN_DIR:-/var/run/apache2} ] && mkdir -p ${APACHE_RUN_DIR:-/var/run/apache2}
 [ ! -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)
     # (this is bad if there are several apache2 instances running)
     rm -f ${APACHE_RUN_DIR:-/var/run/apache2}/*ssl_scache*
 
-    need_systemd=false
-    if [ -z "$APACHE_STARTED_BY_SYSTEMD" ] ; then
-        case "$(readlink -f /proc/1/exe)" in
-        *systemd*)
-            need_systemd=true
-            ;;
-        *)
-            ;;
-        esac
-    fi
-    if $need_systemd ; then
+    if need_systemd; then
         # If running on systemd we should not start httpd without systemd
         # or systemd will get confused about the status of httpd.
-        echo "Invoking 'systemctl start $APACHE_SYSTEMD_SERVICE'."
-        echo "Use 'systemctl status $APACHE_SYSTEMD_SERVICE' for more info."
-        systemctl start "$APACHE_SYSTEMD_SERVICE"
+        echo "Invoking 'systemctl start ${APACHE_SYSTEMD_SERVICE}'."
+        echo "Use 'systemctl status ${APACHE_SYSTEMD_SERVICE}' for more info."
+        systemctl start "${APACHE_SYSTEMD_SERVICE}"
     else
         unset APACHE_STARTED_BY_SYSTEMD
-        $HTTPD ${APACHE_ARGUMENTS} -k "$ARGV"
+        ${HTTPD} ${APACHE_ARGUMENTS} -k "${ARGV}"
     fi
 
     ERROR=$?
     ;;
 stop|graceful-stop)
-    $HTTPD ${APACHE_ARGUMENTS} -k "$ARGV"
+    ${HTTPD} ${APACHE_ARGUMENTS} -k "$ARGV"
     ERROR=$?
     ;;
 restart|graceful)
     if $HTTPD ${APACHE_ARGUMENTS} -t 2> /dev/null ; then
-        $HTTPD ${APACHE_ARGUMENTS} -k "$ARGV"
+        if need_systemd; then
+            # If running on systemd we should not directly restart httpd since
+            # systemd would be confused about httpd's status.
+            # (See LP: #1832182)
+            echo "Invoking 'systemctl restart ${APACHE_SYSTEMD_SERVICE}'."
+            echo "Use 'systemctl status ${APACHE_SYSTEMD_SERVICE}' for more info."
+            systemctl restart "${APACHE_SYSTEMD_SERVICE}"
+        else
+            unset APACHE_STARTED_BY_SYSTEMD
+            ${HTTPD} ${APACHE_ARGUMENTS} -k "${ARGV}"
+        fi
     else
-        $HTTPD ${APACHE_ARGUMENTS} -t
+        ${HTTPD} ${APACHE_ARGUMENTS} -t
     fi
     ERROR=$?
     ;;
diff --git a/debian/changelog b/debian/changelog
index 6295e4e..517a355 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,13 @@
+apache2 (2.4.46-1ubuntu1.1) groovy; urgency=medium
+
+  * 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.
+
+ -- Bryce Harrington <[e-mail]>  Fri, 13 Nov 2020 01:36:38 +0000
+
 apache2 (2.4.46-1ubuntu1) groovy; urgency=medium
 
   * Merge with Debian unstable. Remaining changes: