Merge ~robru/britney/+git/britney2-ubuntu:source_ppa into ~ubuntu-release/britney/+git/britney2-ubuntu:master

[Robert Bruce Park]

Teach Britney to reject packages if other packages copied from same PPA are also rejected.

[Robert Bruce Park (robru)]

Ok, this is conceptually complete, I just need to write the tests for it ;-)

[Martin Pitt (pitti)]

The general direction looks good indeed, thanks for working on this!

review: Needs Fixing

[Martin Pitt (pitti)]

... oh, and you of course need to actually instantiate the new policy in britney.py.

[Robert Bruce Park (robru)]

(some discussion inline; everything I didn't reply to I agree with & will fix)

[Martin Pitt (pitti)]

Likewise, some remaining remarks; I didn't follow up to parts which I consider resolved/agree with.

I just rebased our britney2 to current Debian master (this also includes your speedup patch); please rebase your branch against current britney2-ubuntu.

[Robert Bruce Park (robru)]

Ok, I beleive I've addressed all the issues you've identified, will begin writing those tests shortly. let me know if I've overlooked anything, thanks.

[Martin Pitt (pitti)]

A few minor niggles left, but this looks great now! (still leaving as needs fixing for the tests).

[Robert Bruce Park (robru)]

Ok I've written some tests, please re-review.

One thought occurred to me: When I had to rebase the work, there was a merge conflict in the imports in policy.py (apparently they changed upstream). That was why I had them grouped and separated from the other imports, to hopefully avoid merge conflicts there, but it happened anyway. I wonder if it might make more sense to move the SourcePPAPolicy definition into a new file? This would avoid potential future merge conflicts if upstream touches the imports in policy.py

review: Approve

[Martin Pitt (pitti)]

I like the new test cases in general, to test the finer details of the new policy. But this mocks Britney, Options, and even Excuse, so they don't prove that the overall plumbing of this is correct. Can you please add an integration test case to test_autopkgtest.py which checks this end-to-end?

Thanks!

> move the SourcePPAPolicy definition into a new file?

If you wish, sure.

[Robert Bruce Park (robru)]

Ok, I'll move sourceppapolicy into a new file. That'll decrease the likelihood of merge conflicts on future upstream rebases, which'll make your life easier.

Can you point me at an example of an existing test that integrates britney as you describe? I'm not sure what that would look like really.

[Martin Pitt (pitti)]

All of tests/test_autopkgtest.py are of this "blackbox integration test" kind. The central helper is do_test() which sets up a few "unstable" packages in the sandbox archive, then calls britney on it, and runs the given assertions on the resulting excuses.yaml.

For that you need to be able to mock the Launchpad calls: I suggest setting $LAUNCHPAD_URL to file://<temp dir> so that you can provide defined mock replies.

618a76b... by Martin Pitt on 2016-10-20

Move build checks before running policies

For future policies such as running autopkgtests it is important to know
whether a package has built, so that expensive actions such as "trigger an
autopkgtest" are not done too early/in vain.

This requires dropping the "age != 0" check for adding the out-of-date-ness to
the Excuse, as the policies now run later. But this check only applied to an
infinitesimal age, and even with age == 0 it is still a valid excuse that there
are missing binaries.

1d68151... by Martin Pitt on 2016-10-20

Pass excuse to BasePolicy.apply_policy()

This allows tests to check whether there are any missing builds or old
binaries, so that expensive actions such as "trigger an autopkgtest" are not
done too early/in vain.

e1b2027... by Martin Pitt on 2016-10-20

Move updating of excuses into policies

It is unwieldy to have one half of output data generation in the policy but not
the other half of updating the excuse. Now that apply_policy() gets the excuse
object as argument we can move everything there.

212c91e... by Niels Thykier on 2016-10-22

britney: Remove redundant if and obsolete comment

The RC bugs part is now handled by the RCBugs policy.

Signed-off-by: Niels Thykier <email address hidden>

c341f37... by Martin Pitt on 2016-10-24

Store Testsuite-Triggers: list in sources

Compute an inverse trigger → sources map, as that's usually the direction that
britney is interested in.

fd3e644... by Martin Pitt on 2016-10-26

Drop backwards compatible autopkgtest excuse YAML structure

Consumers got ported over to "autopkgtest" being under "policy_info" now.

[Robert Bruce Park (robru)]

Ok, rebase was easy enough, just working on tests still.

8aea1a8... by Martin Pitt on 2016-10-27

Save policy state before upgrade tester

We might not (want to) run the upgrade tester for some use cases, but we still
need to save the policy state.

[Robert Bruce Park (robru)]

Ok Martin, we are really close here! Please re-review. There's just one small problem and I need your help getting past this. When apply_policy_impl gets called with the excuse object, the excuse object given to it has not actually had its is_valid property set yet, and it defaults to false. This means that, with the existing policy architecture, it is only possible to detect other invalid excuses, it's not possible to detect if my own excuse is already invalid.

So, eg, this case is working:

source ppa has 3 packages, and the second one is rejected, so when the third one is called it notices and rejects all excuses from that ppa

But this case isn't working:

source ppa has 2 packages, and the second one is rejected, so at neither invocation of apply_policy_impl am I able to detect that anything is rejected and thus reject the whole ppa.

To make matters worse, excuses class defaults to is_valid=False, which means when I inspect excuses object it's *always* false, because the logic for deciding what to set it to doesn't happen until after all the policies run. So I need a way to reach back up into britney.py and say "hey what were the results of the previous policy runs" and that just isn't in there. :-/

[Robert Bruce Park (robru)]

(if you run the tests currently, there's one failure, because of this issue)

[Martin Pitt (pitti)]

> When apply_policy_impl gets called with the excuse object, the excuse object given to it has not actually had its is_valid property set yet

Right, that only gets set at the end of should_upgrade_src() if everything is okay.

> So I need a way to reach back up into britney.py and say "hey what were the results of the previous policy runs" and that just isn't in there.

This wouldn't actually be hard to fix -- the policy verdicts are collected in should_upgrade_src() here:

        policy_verdict = PolicyVerdict.PASS
        for policy in self.policies:
            if suite in policy.applicable_suites:
                v = policy.apply_policy(policy_info, suite, src, source_t, source_u, excuse)
                if v > policy_verdict:
                    policy_verdict = v

        if policy_verdict in [PolicyVerdict.REJECTED_PERMANENTLY, PolicyVerdict.REJECTED_TEMPORARILY]:
            update_candidate = False

so in theory we could just pass the current policy_verdict to apply_policy(). However, this isn't sufficient: there are reasons why an excuse is invalid beyond policies, like "not being built everywhere" or "uninstallable". These are not (yet?) implemented as Policies, but directly in britney.py.

So I have three ideas:

 (1) Move the "if policy_verdict ..." (last line in the code above) into the loop, and pass "update_candidate" to policy.apply_policy(). This is a bit intrusive, so if you go this way, please discuss that with Niels first (nthykier on OFTC, or email him).

 (2) Drop the local "update_candidate" variable and use excuse.is_valid for it, which would IMHO be simpler and less intrusive. But likewise, please clear with Niels.

 (3) In your apply_policy_imply(), replace the broken excuse.is_valid with "excuse.missing_builds or excuse.reason". This doesn't require changes to britney.py itself, but it is assuming that every policy actually adds a reason. We do that in AutopkgtestPolicy and LPBlockBugPolicy; the Debian policies (e. g. AgePolicy) don't do that; we also don't use them in Ubuntu, but making this consistent by adding excuse.addreason() would remove a potential future trap (in case we ever actually do use AgePolicy, e. g. to mechanize the SRU policy).

If you do (1) or (2), please do this as a separate commit.

[Robert Bruce Park (robru)]

Yeah i don't like 1 or 2 because they are invasive changes to the policy framework that likely won't accepted upstream because there's no benefit to them. And i don't like 3 because it's incomplete.

What about option 4, where instead of implementing this as a policy, i simply bolt it on at the very end, after all excuses are generated? That would simplify the implementation as well, because currently there's a bit of overhead where i have to simultaneously check other packages to invalidate myself, then also once invalidated i have to go back and make sure everything else is invalidated as well, in a two-step process that's a bit clunky.

If instead i could simply iterate over a fully-formed britney.excuses, i could go back to my initial implementation that worked with a single loop a lot more cleanly.

Thoughts?

[Martin Pitt (pitti)]

I would actually go with (2). It makes the code simpler and more useful. Niels accepted several things which are not immediately useful for Debian, but could become so in the future, including the policy reorg patches recently. I'd start with that, show it to him, and carry the patch in our tree until it lands, it shouldn't be that big.

1029a4e... by Martin Pitt on 2016-11-01

Use Excuse.is_valid when iterating through checks and Policies

This gives Policies the opportunity to see if a previous check
(build/installability) or earlier policies already invalidated the update. This
allows writing policies that work on groups of packages, or skipping expensive
checks (such as triggering autopkgtests while the package is not built or
installable yet).

[Martin Pitt (pitti)]

I implemented (2) in https://git.launchpad.net/~ubuntu-release/britney/+git/britney2-ubuntu/commit/?id=1029a4eec as this isn't that hard to do; I'll send it to Niels for commenting.

[Robert Bruce Park (robru)]

With or without that patch, i still prefer this approach, as it vastly simplifies my algorithm to have access to ALL excuse objects fully formed, just having the excuse object for current source isn't good enough and makes extra work to wait for the other excuse objects in the same ppa. Can you please take a look at this one? It's much cleaner.

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

[Martin Pitt (pitti)]

How can it be so massively simpler? You still need to iterate through all excuses, and a policy already implicitly gives you that. You even split discover_ppas() and reject_sources(), so I don't see any net simplification actually. It also immediately follows the Policy loop, so there's nothing in between that does additional computation.

With the is_valid patch the approach in this MP should actually work, doesn't it?

I don't like the "bolt-on" MP because it forces any subsequent addition to also not be a policy; for example, at some point we want to do email notifications. These would all need to be bolted on too. Also, the bolt-on MP is very confusing: It looks like a policy by location and class naming, but isn't. It's just a single magical constructor.

[Robert Bruce Park (robru)]

On Nov 3, 2016 12:44 AM, "Martin Pitt" <email address hidden> wrote:
> How can it be so massively simpler? You still need to iterate through all
excuses, and a policy already implicitly gives you that. You even split
discover_ppas() and reject_sources(), so I don't see any net simplification
actually. It also immediately follows the Policy loop, so there's nothing
in between that does additional computation.

The key difference is that apply_policy_impl doesn't give us access to the
complete picture, so the policy implementation must do an awkward two-step
check for any reason to invalidate the current package and then also reach
back and invalidate other packages as well.

>
> With the is_valid patch the approach in this MP should actually work,
doesn't it?

The main problem with this approach is that the excuses dict is not fully
generated when the policy is applied, which makes it awkward because every
application of the policy needs to check all other packages, and then
disable all other packages, after each package is loaded.

The bolted-on approach lets us just scan the completed excuses once,
whereas this approach means we have to check the previously loaded excuses
for each excuse loaded. Just look at apply_policy_impl, it's huge compared
to the bolt on approach.

Also I'm not convinced that your is_valid patch really helped that much at
all. I need to know if the excuse is invalid for any reason, policies or
otherwise. I'll need to review if that's indeed the case, for some reason i
was thinking it was only capturing some invalidations and not all.

>
> I don't like the "bolt-on" MP because it forces any subsequent addition
to also not be a policy; for example, at some point we want to do email
notifications. These would all need to be bolted on too.

Well the source ppa grouping needs to the last one anyway since any other
reason to reject something needs to come first. If there's a feature to
send emails, is that really a policy anyway?

> Also, the bolt-on MP is very confusing: It looks like a policy by
location and class naming, but isn't. It's just a single magical
constructor.

Yes yes of course, this is just bike shedding, my main concern is that the
implementation is a lot simpler if i can access the complete excuses dict
rather than apply_policy_impl which shows only an awkward slice of the
information we need to reject an entire source ppa.

Think of it this way: apply_policy_impl is called within a loop, once for
each package, but contains two loops: one to check other packages for
validity and one to then reject all other packages from same ppa.

The bolt on approach contains just two loops: one to discover the source
ppa and then one to do the rejections. It looks so much cleaner!

[Martin Pitt (pitti)]

Robert Bruce Park [2016-11-03 8:06 -0000]:
> The key difference is that apply_policy_impl doesn't give us access to the
> complete picture, so the policy implementation must do an awkward two-step
> check for any reason to invalidate the current package and then also reach
> back and invalidate other packages as well.

Right, you can only look backwards, not forwards (in Excuses order).
But which way around you process it doesn't make the process
significantly more complicated -- you collect the "friends" mapping as
you go, and as soon as you find any invalid source from a PPA you
invalidate all friends as well. You have to do all these for a full
excuses map as well.

> awkward because every application of the policy needs to check all
> other packages, and then disable all other packages, after each
> package is loaded.

Not all other packages, just the ones from the same PPA, which you
already cache in the "friends" dict. And you can maintain a single
bool per PPA whether it's (so far) good or already invalid. So for
every individual Excuse check (per policy invocation) there should not
be any looping at all?

> Also I'm not convinced that your is_valid patch really helped that much at
> all. I need to know if the excuse is invalid for any reason, policies or
> otherwise. I'll need to review if that's indeed the case, for some reason i
> was thinking it was only capturing some invalidations and not all.

No, it accurately reflects (and in fact *is*) the "is-candidate".

> Well the source ppa grouping needs to the last one anyway since any other
> reason to reject something needs to come first. If there's a feature to
> send emails, is that really a policy anyway?

We can do it as a policy. But e. g. the upcoming boot speed
testing/gating will also run after the PPA policy, as it's really
expensive.

> Think of it this way: apply_policy_impl is called within a loop, once for
> each package, but contains two loops: one to check other packages for
> validity and one to then reject all other packages from same ppa.

It's not necessary to loop for the former, and you have to do the
latter in either case.

[Robert Bruce Park (robru)]

Ok I've rebased this on latest master and fixed up the tests. This approach is fundamentally broken, however. In britney.py:

        policy_info = excuse.policy_info
        policy_verdict = PolicyVerdict.PASS
        for policy in self.policies:
            if suite in policy.applicable_suites:
                v = policy.apply_policy(policy_info, suite, src, source_t, source_u, excuse)
                if v > policy_verdict:
                    policy_verdict = v

        if policy_verdict in [PolicyVerdict.REJECTED_PERMANENTLY, PolicyVerdict.REJECTED_TEMPORARILY]:
            excuse.is_valid = False

This means that excuse.is_valid does not get set until *after* all policies have run, which means that at the time my policy runs, inspecting excuse.is_valid can only find the default value, which is false, which means my policy has no way of distinguishing between an excuse that has been rejected by a policy and an excuse that has not been rejected by a policy (both cases are False).

I really would strongly prefer to inspect britney.excuses after it is fully formed than try to write a policy that reaches outside the scope of a policy and inspects other excuses before the excuse-building is even complete. It's thus far proving to be quite error prone.

[Martin Pitt (pitti)]

Oh right, that "if policy_verdict" needs to be indented one level.

[Robert Bruce Park (robru)]

Hmm, i suppose that would work. I overlooked the "excuse.is_valid = True" near the beginning of the method and just saw the excuse class definition where it defaults to false for some reason. Will you submit a patch for that then or should i?

[Martin Pitt (pitti)]

Robert Bruce Park [2016-11-14 21:33 -0000]:
> Hmm, i suppose that would work. I overlooked the "excuse.is_valid =
> True" near the beginning of the method and just saw the excuse class
> definition where it defaults to false for some reason. Will you
> submit a patch for that then or should i?

Please apply it locally in your branch for now, so that when we
forward this upstream we know exactly what we actually need.

24622f6... by Robert Bruce Park on 2016-11-15

Invalidate excuse after each policy run.

[Robert Bruce Park (robru)]

Ok this is done, integration test written, tests passing, please accept.

review: Approve

[Martin Pitt (pitti)]

I have some stylistic/pyflakes updates and another test for applying this policy to an unbuilt package (to make sure that this works as well); this also adds a HTML explanation of the blocker: http://paste.ubuntu.com/23490188/

However, there is still some weird race condition here:

$ while tests/test_autopkgtest.py -v T.test_sourceppa; do true; done
[...]
======================================================================
FAIL: test_sourceppa (__main__.T)
Packages from same source PPA should be grouped.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/test_autopkgtest.py", line 2059, in test_sourceppa
    self.assertEqual(exc['red']['policy_info']['source-ppa'], {'red': 'team/ubuntu/ppa', 'green': 'team/ubuntu/ppa'})
AssertionError: {} != {'red': 'team/ubuntu/ppa', 'green': 'team/ubuntu/ppa'}
- {}
+ {'green': 'team/ubuntu/ppa', 'red': 'team/ubuntu/ppa'}

This apparently depends on some python dict randomness, depending on whether red or green get examined first. The friend invalidation also happens multiple times, causing the reasons to be added multiple times (which looks confusing). This also needs fixing still.

I'm still looking into this, but wanted to provide some feedback of the current state. In general this looks really nice now, thanks!

BTW: Not sure if you are aware, but for debugging such issues, this is useful:

 SHOW_OUTPUT=1 SHOW_EXCUSES=1 SHOW_HTML=1 tests/test_autopkgtest.py T.test_sourceppa_policy

[Martin Pitt (pitti)]

I adjusted the preliminary britney policy support fix for upstream, sent it to Niels, and rebased our Ubuntu changes on top:

  https://git.launchpad.net/~ubuntu-release/britney/+git/britney2-ubuntu/commit/?id=4bde6d4

The "main" commit here is

  https://git.launchpad.net/~ubuntu-release/britney/+git/britney2-ubuntu/commit/?id=47f5f0

which differs from your's just in some nitpicks and two pyflakes fixes.

I did a followup commit

  https://git.launchpad.net/~ubuntu-release/britney/+git/britney2-ubuntu/commit/?id=6f16ddc

with a few fixes, particularly that unstable policy_info bit from above.

I'll land this now, let's see what happens! Thanks!

review: Approve

diff --git a/britney.py b/britney.py
index 936512b..19fc048 100755
--- a/britney.py
+++ b/britney.py
@@ -215,6 +215,7 @@ from britney_util import (old_libraries_format, undo_changes,
                           )
 from policies.policy import AgePolicy, RCBugPolicy, LPBlockBugPolicy, PolicyVerdict
 from policies.autopkgtest import AutopkgtestPolicy
+from policies.sourceppa import SourcePPAPolicy
 
 # Check the "check_field_name" reflection before removing an import here.
 from consts import (SOURCE, SOURCEVER, ARCHITECTURE, CONFLICTS, DEPENDS,
@@ -545,6 +546,7 @@ class Britney(object):
         self.policies.append(LPBlockBugPolicy(self.options))
         if getattr(self.options, 'adt_enable') == 'yes':
             self.policies.append(AutopkgtestPolicy(self.options))
+        self.policies.append(SourcePPAPolicy(self.options))
 
         for policy in self.policies:
             policy.register_hints(self._hint_parser)
@@ -1758,8 +1760,8 @@ class Britney(object):
                 if v > policy_verdict:
                     policy_verdict = v
 
-        if policy_verdict in [PolicyVerdict.REJECTED_PERMANENTLY, PolicyVerdict.REJECTED_TEMPORARILY]:
-            excuse.is_valid = False
+            if policy_verdict in [PolicyVerdict.REJECTED_PERMANENTLY, PolicyVerdict.REJECTED_TEMPORARILY]:
+                excuse.is_valid = False
 
         if suite in ('pu', 'tpu') and source_t:
             # o-o-d(ish) checks for (t-)p-u
diff --git a/policies/sourceppa.py b/policies/sourceppa.py
new file mode 100644
index 0000000..90a7ca8
--- /dev/null
+++ b/policies/sourceppa.py
@@ -0,0 +1,115 @@
+import os
+import json
+import urllib.request
+import urllib.parse
+
+from collections import defaultdict
+
+from policies.policy import BasePolicy, PolicyVerdict
+
+
+LAUNCHPAD_URL = 'https://api.launchpad.net/1.0/'
+PRIMARY = LAUNCHPAD_URL + 'ubuntu/+archive/primary'
+
+
+def query_lp_rest_api(obj, query):
+    """Do a Launchpad REST request
+
+    Request <LAUNCHPAD_URL><obj>?<query>.
+
+    Returns dict of parsed json result from launchpad.
+    Raises HTTPError, ValueError, or ConnectionError based on different
+    transient failures connecting to launchpad.
+    """
+    url = '%s%s?%s' % (LAUNCHPAD_URL, obj, urllib.parse.urlencode(query))
+    with urllib.request.urlopen(url, timeout=10) as req:
+        code = req.getcode()
+        if 200 <= code < 300:
+            return json.loads(req.read().decode('UTF-8'))
+        raise ConnectionError('Failed to reach launchpad, HTTP %s' % code)
+
+
+class SourcePPAPolicy(BasePolicy):
+    """Migrate packages copied from same source PPA together
+
+    This policy will query launchpad to determine what source PPA packages
+    were copied from, and ensure that all packages from the same PPA migrate
+    together.
+    """
+
+    def __init__(self, options):
+        super().__init__('source-ppa', options, {'unstable'})
+        self.filename = os.path.join(options.unstable, 'SourcePPA')
+        # Dict of dicts; maps pkg name -> pkg version -> source PPA URL
+        self.source_ppas_by_pkg = defaultdict(dict)
+        # Dict of sets; maps source PPA URL -> set of source names
+        self.pkgs_by_source_ppa = defaultdict(set)
+        self.britney = None
+        # self.cache contains self.source_ppas_by_pkg from previous run
+        self.cache = {}
+
+    def lp_get_source_ppa(self, pkg, version):
+        """Ask LP what source PPA pkg was copied from"""
+        cached = self.cache.get(pkg, {}).get(version)
+        if cached is not None:
+            return cached
+
+        data = query_lp_rest_api('%s/%s' % (self.options.distribution, self.options.series), {
+            'ws.op': 'getPackageUploads',
+            'archive': PRIMARY,
+            'pocket': 'Proposed',
+            'name': pkg,
+            'version': version,
+            'exact_match': 'true',
+        })
+        try:
+            return data['entries'][0]['copy_source_archive_link'] or ''
+        # IndexError means no packages in -proposed matched this name/version,
+        # which is expected to happen when bileto runs britney.
+        except IndexError:
+            return ''
+
+    def initialise(self, britney):
+        """Load and discover all source ppa data"""
+        super().initialise(britney)
+        self.britney = britney
+
+        try:
+            with open(self.filename, encoding='utf-8') as data:
+                self.cache = json.load(data)
+            self.log("Loaded cached source ppa data from %s" % self.filename)
+        except FileNotFoundError:
+            pass
+
+    def apply_policy_impl(self, sourceppa_info, suite, source_name, source_data_tdist, source_data_srcdist, excuse):
+        """Reject package if any other package copied from same PPA is invalid"""
+        accept = excuse.is_valid
+        britney_excuses = self.britney.excuses
+        version = source_data_srcdist.version
+        sourceppa = self.lp_get_source_ppa(source_name, version)
+        self.source_ppas_by_pkg[source_name][version] = sourceppa
+        if sourceppa:
+            # Check for other packages that might invalidate this one
+            for friend in self.pkgs_by_source_ppa[sourceppa]:
+                if not britney_excuses[friend].is_valid:
+                    accept = False
+            self.pkgs_by_source_ppa[sourceppa].add(source_name)
+        if not accept:
+            # Invalidate all packages in this source ppa
+            shortppa = sourceppa.replace(LAUNCHPAD_URL, '')
+            for friend in self.pkgs_by_source_ppa[sourceppa]:
+                friend_exc = britney_excuses.get(friend, excuse)
+                friend_exc.is_valid = False
+                friend_exc.addreason('source-ppa')
+                self.log("Blocking %s because %s from %s" % (friend, source_name, shortppa))
+                sourceppa_info[friend] = shortppa
+            return PolicyVerdict.REJECTED_PERMANENTLY
+        return PolicyVerdict.PASS
+
+    def save_state(self, britney):
+        """Write source ppa data to disk"""
+        tmp = self.filename + '.tmp'
+        with open(tmp, 'w', encoding='utf-8') as data:
+            json.dump(self.source_ppas_by_pkg, data)
+        os.rename(tmp, self.filename)
+        self.log("Wrote source ppa data to %s" % self.filename)
diff --git a/tests/data/sourceppa.json b/tests/data/sourceppa.json
new file mode 100644
index 0000000..70e5538
--- /dev/null
+++ b/tests/data/sourceppa.json
@@ -0,0 +1,7 @@
+{
+    "pal": {"2.0": "https://api.launchpad.net/1.0/team/ubuntu/ppa"},
+    "buddy": {"2.0": "https://api.launchpad.net/1.0/team/ubuntu/ppa"},
+    "friend": {"2.0": "https://api.launchpad.net/1.0/team/ubuntu/ppa"},
+    "noppa": {"2.0": ""},
+    "unused": {"2.0": ""}
+}
diff --git a/tests/test_autopkgtest.py b/tests/test_autopkgtest.py
index 53c91c5..b8afaae 100755
--- a/tests/test_autopkgtest.py
+++ b/tests/test_autopkgtest.py
@@ -72,6 +72,19 @@ class T(TestBase):
                                       'Conflicts': 'green'},
                       testsuite='specialtest')
 
+        # Set up sourceppa cache for testing
+        self.sourceppa_cache = {
+            'gcc-5': {'2': ''},
+            'gcc-snapshot': {'2': ''},
+            'green': {'2': '', '1.1': '', '3': ''},
+            'lightgreen': {'2': '', '1.1~beta': '', '3': ''},
+            'linux-meta-64only': {'1': ''},
+            'linux-meta-lts-grumpy': {'1': ''},
+            'linux-meta': {'0.2': '', '1': '', '2': ''},
+            'linux': {'2': ''},
+            'newgreen': {'2': ''},
+        }
+
         # create mock Swift server (but don't start it yet, as tests first need
         # to poke in results)
         self.swift = mock_swift.AutoPkgTestSwiftServer(port=18085)
@@ -96,6 +109,16 @@ class T(TestBase):
         '''
         for (pkg, fields, testsuite) in unstable_add:
             self.data.add(pkg, True, fields, True, testsuite)
+            self.sourceppa_cache.setdefault(pkg, {})
+            if fields['Version'] not in self.sourceppa_cache[pkg]:
+                self.sourceppa_cache[pkg][fields['Version']] = ''
+
+        # Set up sourceppa cache for testing
+        sourceppa_path = os.path.join(self.data.dirs[True], 'SourcePPA')
+        with open(sourceppa_path, 'w', encoding='utf-8') as sourceppa:
+            sourceppa.write(json.dumps(self.sourceppa_cache))
+        # with open(sourceppa_path, encoding='utf-8') as data:
+        #     print(data.read())
 
         self.swift.start()
         (excuses_yaml, excuses_html, out) = self.run_britney()
@@ -2015,6 +2038,26 @@ class T(TestBase):
         with open(shared_path) as f:
             self.assertEqual(orig_contents, f.read())
 
+    ################################################################
+    # Tests for source ppa grouping
+    ################################################################
+
+    def test_sourceppa(self):
+        '''Packages from same source PPA should be grouped.'''
+        self.sourceppa_cache['green'] = {'2': 'team/ubuntu/ppa'}
+        self.sourceppa_cache['red'] = {'2': 'team/ubuntu/ppa'}
+        with open(os.path.join(self.data.path, 'data/series-proposed/Blocks'), 'w') as f:
+            f.write('green 12345 1471505000\ndarkgreen 98765 1471500000\n')
+
+        exc = self.do_test(
+            [('green', {'Version': '2'}, 'autopkgtest'),
+             ('red', {'Version': '2'}, 'autopkgtest')],
+            {'green': (False, {'green': {'i386': 'RUNNING-ALWAYSFAIL', 'amd64': 'RUNNING-ALWAYSFAIL'}})},
+            {'green': [('is-candidate', False)],
+             'red': [('is-candidate', False)]}
+        )[1]
+        self.assertEqual(exc['red']['policy_info']['source-ppa'], {'red': 'team/ubuntu/ppa', 'green': 'team/ubuntu/ppa'})
+
 
 if __name__ == '__main__':
     unittest.main()
diff --git a/tests/test_sourceppa.py b/tests/test_sourceppa.py
new file mode 100755
index 0000000..477a187
--- /dev/null
+++ b/tests/test_sourceppa.py
@@ -0,0 +1,134 @@
+#!/usr/bin/python3
+# (C) 2016 Canonical Ltd.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+
+import os
+import sys
+import json
+import tempfile
+import unittest
+from unittest.mock import patch
+
+PROJECT_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
+sys.path.insert(0, PROJECT_DIR)
+
+from policies.policy import PolicyVerdict
+from policies.sourceppa import LAUNCHPAD_URL, SourcePPAPolicy
+
+
+CACHE_FILE = os.path.join(PROJECT_DIR, 'tests', 'data', 'sourceppa.json')
+
+
+class FakeOptions:
+    distribution = 'testbuntu'
+    series = 'zazzy'
+    unstable = '/tmp'
+    verbose = False
+
+
+class FakeExcuse:
+    ver = ('1.0', '2.0')
+    is_valid = True
+
+    def addreason(self, reason):
+        """Ignore reasons."""
+
+
+class FakeBritney:
+    def __init__(self):
+        self.excuses = dict(
+            pal=FakeExcuse(),
+            buddy=FakeExcuse(),
+            friend=FakeExcuse(),
+            noppa=FakeExcuse())
+
+
+class FakeData:
+    version = '2.0'
+
+
+class UtilTests(unittest.TestCase):
+    maxDiff = None
+
+    @patch('policies.sourceppa.urllib.request.urlopen')
+    def test_lp_rest_api_no_entries(self, urlopen):
+        """Don't explode if LP reports no entries match pkg/version"""
+        context = urlopen.return_value.__enter__.return_value
+        context.getcode.return_value = 200
+        context.read.return_value = b'{"entries": []}'
+        pol = SourcePPAPolicy(FakeOptions)
+        self.assertEqual(pol.lp_get_source_ppa('hello', '1.0'), '')
+
+    @patch('policies.sourceppa.urllib.request.urlopen')
+    def test_lp_rest_api_no_source_ppa(self, urlopen):
+        """Identify when package has no source PPA"""
+        context = urlopen.return_value.__enter__.return_value
+        context.getcode.return_value = 200
+        context.read.return_value = b'{"entries": [{"copy_source_archive_link": null, "other_stuff": "ignored"}]}'
+        pol = SourcePPAPolicy(FakeOptions)
+        self.assertEqual(pol.lp_get_source_ppa('hello', '1.0'), '')
+
+    @patch('policies.sourceppa.urllib.request.urlopen')
+    def test_lp_rest_api_with_source_ppa(self, urlopen):
+        """Identify source PPA"""
+        context = urlopen.return_value.__enter__.return_value
+        context.getcode.return_value = 200
+        context.read.return_value = b'{"entries": [{"copy_source_archive_link": "https://api.launchpad.net/1.0/team/ubuntu/ppa", "other_stuff": "ignored"}]}'
+        pol = SourcePPAPolicy(FakeOptions)
+        self.assertEqual(pol.lp_get_source_ppa('hello', '1.0'), 'https://api.launchpad.net/1.0/team/ubuntu/ppa')
+
+    @patch('policies.sourceppa.urllib.request.urlopen')
+    def test_lp_rest_api_errors(self, urlopen):
+        """Report errors instead of swallowing them"""
+        context = urlopen.return_value.__enter__.return_value
+        context.getcode.return_value = 500
+        context.read.return_value = b''
+        pol = SourcePPAPolicy(FakeOptions)
+        with self.assertRaisesRegex(ConnectionError, 'HTTP 500'):
+            pol.lp_get_source_ppa('hello', '1.0')
+        # Yes, I have really seen "success with no json returned" in the wild
+        context.getcode.return_value = 200
+        context.read.return_value = b''
+        with self.assertRaisesRegex(ValueError, 'Expecting value'):
+            pol.lp_get_source_ppa('hello', '1.0')
+
+    def test_approve_ppa(self):
+        """Approve packages by their PPA."""
+        pol = SourcePPAPolicy(FakeOptions)
+        pol.filename = CACHE_FILE
+        pol.initialise(FakeBritney())
+        output = {}
+        for pkg in ('pal', 'buddy', 'friend', 'noppa'):
+            self.assertEqual(pol.apply_policy_impl(output, None, pkg, None, FakeData, FakeExcuse), PolicyVerdict.PASS)
+        self.assertEqual(output, {})
+
+    def test_reject_ppa(self):
+        """Reject packages by their PPA."""
+        shortppa = 'team/ubuntu/ppa'
+        pol = SourcePPAPolicy(FakeOptions)
+        pol.filename = CACHE_FILE
+        brit = FakeBritney()
+        brit.excuses['buddy'].is_valid = False  # Just buddy is invalid but whole ppa fails
+        pol.initialise(brit)
+        output = {}
+        # This one passes because the rejection isn't known yet
+        self.assertEqual(pol.apply_policy_impl(output, None, 'pal', None, FakeData, brit.excuses['pal']), PolicyVerdict.PASS)
+        # This one fails because it is itself invalid.
+        self.assertEqual(pol.apply_policy_impl(output, None, 'buddy', None, FakeData, brit.excuses['buddy']), PolicyVerdict.REJECTED_PERMANENTLY)
+        # This one fails because buddy failed before it.
+        self.assertEqual(pol.apply_policy_impl(output, None, 'friend', None, FakeData, brit.excuses['friend']), PolicyVerdict.REJECTED_PERMANENTLY)
+        # 'noppa' not from PPA so not rejected
+        self.assertEqual(pol.apply_policy_impl(output, None, 'noppa', None, FakeData, FakeExcuse), PolicyVerdict.PASS)
+        # All are rejected however
+        for pkg in ('pal', 'buddy', 'friend'):
+            self.assertFalse(brit.excuses[pkg].is_valid)
+        self.assertDictEqual(pol.pkgs_by_source_ppa, {
+            LAUNCHPAD_URL + shortppa: {'pal', 'buddy', 'friend'}})
+        self.assertEqual(output, dict(pal=shortppa, buddy=shortppa, friend=shortppa))
+
+if __name__ == '__main__':
+    unittest.main()