Merge ~ahasenack/ubuntu/+source/squid:fix-rotate-assertion-1794553 into ubuntu/+source/squid:ubuntu/devel

Proposed by Andreas Hasenack on 2018-10-09
Status: Merged
Approved by: Andreas Hasenack on 2018-10-09
Approved revision: 8b6a19d070c196150124af2d36457bcb97591315
Merged at revision: 8b6a19d070c196150124af2d36457bcb97591315
Proposed branch: ~ahasenack/ubuntu/+source/squid:fix-rotate-assertion-1794553
Merge into: ubuntu/+source/squid:ubuntu/devel
Diff against target: 56 lines (+34/-0)
3 files modified
debian/changelog (+7/-0)
debian/patches/fix-rotate-assertion.patch (+26/-0)
debian/patches/series (+1/-0)
Reviewer Review Type Date Requested Status
Christian Ehrhardt  Approve on 2018-10-09
Robie Basak 2018-10-09 Approve on 2018-10-09
Canonical Server Team 2018-10-09 Pending
Canonical Server Core Reviewers 2018-10-09 Pending
Review via email: mp+356351@code.launchpad.net

Description of the change

Fix an assertion error that happens during logrotate.

The logrotate script uses "squid -k rotate" to rotate the logs, but that is triggering an assertion error in squid 4.x. Upstream is aware of it, and resume discussing a fix just recently.

Even though the PR/bug aren't closed yet, I think it's worth to use the in-progress fix that they have, given its simplicity. See https://github.com/squid-cache/squid/pull/257#issuecomment-428250856 for upstream's comment.

The alternatives are:
- replace squid -k rotate in the logrotate config with a service restart, which takes about 30s.
- add a default cache_dir option to squid.conf in postinst (assertion error doesn't happen then)
- disable the icmp pinger, also via a squid.conf change. The assertion error also doesn't happen in that case.

To test:

sudo apt install squid
squid -k rotate
look for "assertion" in /var/log/squid/cache.log

Bileto ticket and associated ppa: https://bileto.ubuntu.com/#/ticket/3462 (still building atm)

Temporary ppa you can use in the meantime: ppa:ahasenack/junk (I'll remove this once the bileto one completes building).

To post a comment you must log in.
Robie Basak (racb) wrote :

I agree that this is the least worst option (better than a crash) based on the caveats listed at https://github.com/squid-cache/squid/pull/257#issuecomment-428250856

review: Approve
Christian Ehrhardt  (paelzer) wrote :

The integration of the change LGTM, but I wonder if "The FD report in Cache Manager will not have the cache.log entry after that change IIRC. If that fix is adopted, more code changes might be needed to polish the isolation of the debugging API from FD registration." will be a problem.

We'd need a squid power user to get an answer for that but I'm none.

OTOH upstream called it "No known serious problems as far as Squid functionality is concerned" while the total inability to rotate logs is a serious issue for sure.

So reviewing in a hurry and +0.95?
Maybe rbasak has time for a second look.

review: Approve
Christian Ehrhardt  (paelzer) wrote :

oh we raced, so you overall have +1.95 atm

Andreas Hasenack (ahasenack) wrote :

DEP8 tests passed

Manual test:
root@cosmic-squid-assertion-error:~# apt-cache policy squid
squid:
  Installed: 4.1-1ubuntu2
  Candidate: 4.1-1ubuntu2
  Version table:
 *** 4.1-1ubuntu2 500

root@cosmic-squid-assertion-error:~# grep assertion /var/log/squid/cache.log
root@cosmic-squid-assertion-error:~#

root@cosmic-squid-assertion-error:~# squid -k rotate
root@cosmic-squid-assertion-error:~# grep assertion /var/log/squid/cache.log
2018/10/09 20:19:42 kid1| assertion failed: comm.cc:428: "!isOpen(conn->fd)"

Installing update from the bileto ppa:
root@cosmic-squid-assertion-error:~# apt-cache policy squid
squid:
  Installed: 4.1-1ubuntu3~ppa1
  Candidate: 4.1-1ubuntu3~ppa1
  Version table:
 *** 4.1-1ubuntu3~ppa1 500
        500 http://ppa.launchpad.net/ci-train-ppa-service/3462/ubuntu cosmic/main amd64 Packages

root@cosmic-squid-assertion-error:~# > /var/log/squid/cache.log
root@cosmic-squid-assertion-error:~# grep assertion /var/log/squid/cache.log
root@cosmic-squid-assertion-error:~# squid -k rotate
root@cosmic-squid-assertion-error:~# grep assertion /var/log/squid/cache.log
root@cosmic-squid-assertion-error:~#

Tagging and uploading.

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 2b85ae0..212ff3b 100644
3--- a/debian/changelog
4+++ b/debian/changelog
5@@ -1,3 +1,10 @@
6+squid (4.1-1ubuntu3) cosmic; urgency=medium
7+
8+ * d/p/fix-rotate-assertion.patch: Fix assertion error when rotating logs.
9+ Thanks to Vitaly Lavrov <vel21ripn@gmail.com>. (LP: #1794553)
10+
11+ -- Andreas Hasenack <andreas@canonical.com> Tue, 09 Oct 2018 14:00:36 -0300
12+
13 squid (4.1-1ubuntu2) cosmic; urgency=medium
14
15 * d/usr.sbin.squid: Update apparmor profile to grant read access to squid
16diff --git a/debian/patches/fix-rotate-assertion.patch b/debian/patches/fix-rotate-assertion.patch
17new file mode 100644
18index 0000000..820cf0e
19--- /dev/null
20+++ b/debian/patches/fix-rotate-assertion.patch
21@@ -0,0 +1,26 @@
22+Description: Fix assertion error when rotating logs
23+ Upstream is still discussing
24+ (https://github.com/squid-cache/squid/pull/257#issuecomment-428250856)
25+ the details of the patch, but there are no big cons for now. The PR will be
26+ monitored and the squid package updated accordingly when needed.
27+Author: Vitaly Lavrov <vel21ripn@gmail.com>
28+Origin: other, https://github.com/squid-cache/squid/pull/257#issuecomment-427271426
29+Bug: https://bugs.squid-cache.org/show_bug.cgi?id=4796
30+Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=910337
31+Bug-Ubuntu: https://bugs.launchpad.net/ubuntu/+source/squid/+bug/1794553
32+Last-Update: 2018-10-09
33+---
34+This patch header follows DEP-3: http://dep.debian.net/deps/dep3/
35+diff --git a/src/main.cc b/src/main.cc
36+index 2a7269ed..9dc0d195 100644
37+--- a/src/main.cc
38++++ b/src/main.cc
39+@@ -1152,8 +1152,6 @@ mainInitialize(void)
40+
41+ _db_init(Debug::cache_log, Debug::debugOptions);
42+
43+- fd_open(fileno(debug_log), FD_LOG, Debug::cache_log);
44+-
45+ debugs(1, DBG_CRITICAL, "Starting Squid Cache version " << version_string << " for " << CONFIG_HOST_TYPE << "...");
46+ debugs(1, DBG_CRITICAL, "Service Name: " << service_name);
47+
48diff --git a/debian/patches/series b/debian/patches/series
49index 12952d3..59bf11b 100644
50--- a/debian/patches/series
51+++ b/debian/patches/series
52@@ -5,3 +5,4 @@
53 99-ubuntu-ssl-cert-snakeoil.patch
54 0003-installed-binary-for-debian-ci.patch
55 fix-uninitialized-var.patch
56+fix-rotate-assertion.patch

Subscribers

People subscribed via source and target branches