Merge ~vorlon/britney/+git/britney2-ubuntu:futz-with-email-frequency into ~ubuntu-release/britney/+git/britney2-ubuntu:master

[Steve Langasek]

Refinement of the email handling about stuck packages.

Packages that are not valid candidates but also not temporarily rejected by policy should be notified about sooner than 3 days. Packages that are valid candidates but stuck because of update_output should be notified about later. Implement this more nuanced policy, per LP: #1671468.

Supersedes https://code.launchpad.net/~robru/britney/+git/britney2-ubuntu/+merge/320837

[Iain Lane (laney)]

I originally had an objection to sending email after only 1 day of a package being stuck.

Consider packages that take longer than a day to build - these will be invalid (out of date), so you'll mail before they are ready AFAICS. I don't think proposed-migration can actually tell if a package is out of date because it is still building or because it's failed to build, either, so as it stands I don't think this is fixable. That's why we currently wait at least 3 days before mailing.

On reflection, though, I think I'm okay with merging this change and we'll see if that is a problem in reality.

Would you please comment the code where you calculate last_due? That is quite difficult to reason about and it would benefit from some explanation up front. You can just push that directly yourself.

review: Approve

diff --git a/britney2/policies/email.py b/britney2/policies/email.py
index a453624..5be71d2 100644
--- a/britney2/policies/email.py
+++ b/britney2/policies/email.py
@@ -1,6 +1,7 @@
 import os
 import re
 import json
+import math
 import socket
 import smtplib
 
@@ -12,7 +13,7 @@ from britney2.policies.policy import BasePolicy, PolicyVerdict
 
 
 # Recurring emails should never be more than this many days apart
-MAX_FREQUENCY = 30
+MAX_INTERVAL = 30
 
 API_PREFIX = 'https://api.launchpad.net/1.0/'
 USER = API_PREFIX + '~'
@@ -28,13 +29,13 @@ BOTS = {
 MESSAGE = """From: Ubuntu Release Team <[e-mail]>
 To: {recipients}
 X-Proposed-Migration: notice
-Subject: [proposed-migration] {source_name} {version} stuck in {series}-proposed for {rounded_age} days.
+Subject: [proposed-migration] {source_name} {version} stuck in {series}-proposed for {age} day{plural}.
 
 Hi,
 
 {source_name} {version} needs attention.
 
-It has been stuck in {series}-proposed for {rounded_age} days.
+It has been stuck in {series}-proposed for {age} day{plural}.
 
 You either sponsored or uploaded this package, please investigate why it hasn't been approved for migration.
 
@@ -102,6 +103,14 @@ class EmailPolicy(BasePolicy, Rest):
             with open(self.filename, encoding='utf-8') as data:
                 self.cache = json.load(data)
             self.log("Loaded cached email data from %s" % self.filename)
+        tmp = self.filename + '.new'
+        if os.path.exists(tmp):
+            # if we find a record on disk of emails sent from an incomplete
+            # britney run, merge them in now.
+            with open(tmp, encoding='utf-8') as data:
+                self.cache.update(json.load(data))
+            self._save_progress(self.cache)
+            self.save_state()
 
     def _scrape_gpg_emails(self, person):
         """Find email addresses from one person's GPG keys."""
@@ -158,36 +167,48 @@ class EmailPolicy(BasePolicy, Rest):
 
     def apply_policy_impl(self, email_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse):
         """Send email if package is rejected."""
+        max_age = 5 if excuse.is_valid else 1
         series = self.options.series
         version = source_data_srcdist.version
-        age = excuse.daysold or 0
-        rounded_age = int(age)
+        age = int(excuse.daysold) or 0
+        plural = '' if age == 1 else 's'
         # an item is stuck if it's
         # - old enough
         # - not blocked
         # - not temporarily rejected (e.g. by the autopkgtest policy when tests
         #   are still running)
-        stuck = age >= 3 and 'block' not in excuse.reason and \
+        stuck = age >= max_age and 'block' not in excuse.reason and \
             excuse.current_policy_verdict != PolicyVerdict.REJECTED_TEMPORARILY
+        if self.dry_run:
+            self.log("[email dry run] Considering: %s/%s: %s" %
+                     (source_name, version, "stuck" if stuck else "not stuck"))
+        if not stuck:
+            return PolicyVerdict.PASS
 
         cached = self.cache.get(source_name, {}).get(version)
         try:
-            emails, sent_age = cached
-            sent = (age - sent_age) < min(MAX_FREQUENCY, age / 2.0)
+            emails, last_sent = cached
+            # migration of older data
+            last_sent = int(last_sent)
+            last_due = int(math.pow(2, int(math.log(age + 2 - max_age, 2)))
+                           + max_age - 2)
+            if last_due - max_age >= MAX_INTERVAL:
+                last_due = int((age - max_age - MAX_INTERVAL) / MAX_INTERVAL) \
+                           * MAX_INTERVAL + max_age + MAX_INTERVAL
+            if last_due < max_age:
+                last_due = max_age
+
         except TypeError:
             # This exception happens when source_name, version never seen before
             emails = []
-            sent_age = 0
-            sent = False
+            last_sent = 0
+            last_due = max_age
         if self.dry_run:
-            self.log("[email dry run] Considering: %s/%s: %s" %
-                     (source_name, version, "stuck" if stuck else "not stuck"))
-            if stuck:
-                self.log("[email dry run] Age %d >= threshold %d: would email: %s" %
-                         (age, max_age, self.lp_get_emails(source_name, version)))
+            self.log("[email dry run] Age %d >= threshold %d: would email: %s" %
+                     (age, max_age, self.lp_get_emails(source_name, version)))
             # don't update the cache file in dry run mode; we'll see all output each time
             return PolicyVerdict.PASS
-        if stuck and not sent:
+        if last_sent < last_due:
             if not emails:
                 emails = self.lp_get_emails(source_name, version)
             if emails:
@@ -199,19 +220,29 @@ class EmailPolicy(BasePolicy, Rest):
                     server = smtplib.SMTP(self.email_host)
                     server.sendmail('[e-mail]', emails, msg)
                     server.quit()
-                    sent_age = age
+                    # record the age at which the mail should have been sent
+                    last_sent = last_due
                 except socket.error as err:
                     self.log("Failed to send mail! Is SMTP server running?")
                     self.log(err)
-        self.emails_by_pkg[source_name][version] = (emails, sent_age)
-        self.save_state()
+        self.emails_by_pkg[source_name][version] = (emails, last_sent)
+        self._save_progress(self.emails_by_pkg)
         return PolicyVerdict.PASS
 
-    def save_state(self, britney=None):
-        """Write source ppa data to disk"""
-        tmp = self.filename + '.tmp'
+    def _save_progress(self, my_data):
+        """Checkpoint after each sent mail"""
+        tmp = self.filename + '.new'
         with open(tmp, 'w', encoding='utf-8') as data:
-            json.dump(self.emails_by_pkg, data)
-        os.rename(tmp, self.filename)
+            json.dump(my_data, data)
+        return tmp
+
+    def save_state(self, britney=None):
+        """Save email notification status of all pending packages"""
+        if not self.dry_run:
+            try:
+                os.rename(self.filename + '.new', self.filename)
+            # if we haven't written any cache, don't clobber the old one
+            except FileNotFoundError:
+                pass
         if britney:
             self.log("Wrote email data to %s" % self.filename)
diff --git a/tests/test_email.py b/tests/test_email.py
index 1899d9c..f971cf8 100755
--- a/tests/test_email.py
+++ b/tests/test_email.py
@@ -225,8 +225,11 @@ class T(unittest.TestCase):
 
     @patch('britney2.policies.email.EmailPolicy.lp_get_emails')
     @patch('britney2.policies.email.smtplib', autospec=True)
-    def test_smtp_repetition(self, smtp, lp):
+    def smtp_repetition(self, smtp, lp, valid, expected):
         """Resend mails periodically, with decreasing frequency."""
+        if not isinstance(valid,list):
+            valid = [valid]*len(expected)
+        FakeExcuse.is_valid = valid
         lp.return_value = ['[e-mail]']
         sendmail = smtp.SMTP().sendmail
         e = EmailPolicy(FakeOptions, None)
@@ -236,13 +239,34 @@ class T(unittest.TestCase):
             previous = sendmail.call_count
             age = hours / 24
             FakeExcuse.daysold = age
+            try:
+                FakeExcuse.is_valid = valid[len(called)]
+            except IndexError:
+                # we've already gotten all the mails we expect
+                pass
             e.apply_policy_impl(None, None, 'unity8', None, FakeSourceData, FakeExcuse)
             if sendmail.call_count > previous:
                 e.initialise(None)  # Refill e.cache from disk
                 called.append(age)
+                name, args, kwargs = sendmail.mock_calls[-1]
+                text = args[2]
+                self.assertNotIn(' 1 days.', text)
+        self.assertSequenceEqual(called, expected)
+
+    def test_smtp_repetition(self):
+        """Confirm that emails are sent at appropriate intervals."""
         # Emails were sent when daysold reached these values:
-        self.assertSequenceEqual(called, [
-            3.0, 6.0, 12.0, 24.0, 48.0, 78.0, 108.0, 138.0, 168.0, 198.0
+        self.smtp_repetition(valid=False, expected=[
+            1, 3, 7, 15, 31, 61, 91, 121, 151, 181
+        ])
+        self.smtp_repetition(valid=True, expected=[
+            5, 7, 11, 19, 35, 65, 95, 125, 155, 185
+        ])
+        self.smtp_repetition(valid=[False, False, True], expected=[
+            1, 3, 5, 7, 11, 19, 35, 65, 95, 125, 155, 185
+        ])
+        self.smtp_repetition(valid=[False, False, True, False, True], expected=[
+            1, 3, 5, 7, 11, 19, 35, 65, 95, 125, 155, 185
         ])