Merge lp:~doanac/ubuntu-ci-services-itself/ppa-cleaner into lp:ubuntu-ci-services-itself

[Andy Doan]

Add ppa-cleaning management command to the ppa-assigner

This adds ppa-cleaning logic to the PPA-assigner via a django management command. In the future, we'll update our charm so that this command is run via upstart.

The main work was done in ppa-assigner/ppa_assigner/management/commands/clean_ppas.py. I refactored a little bit of code from models.py into a new launchpad.py utility.

[Vincent Ladeuil (vila)]

21 +# Read for details: https://help.launchpad.net/API/SigningRequests
22 +OAUTH_CONSUMER_KEY = 'TODO'
23 +OAUTH_TOKEN = 'TODO'
24 +OAUTH_TOKEN_SECRET = 'TODO'
25 +

Not sure how this relates to your other MP, I'll consider you've addressed the TODO there for this review.

60 - lockname = bundle.data.get('lockname')

\o/ no more lock ;)

79 + if type(state) != int:

O_o can you add a comment to explain ? As I read it it means we'll be ignoring some requests. I'd rather error out unless you have a strong reason to do so... which will make a perfect comment ;)

118 +def lp_collection(url):

You probably want to talk to wgrant about that. If I remember correctly, if the collection is big/huge, you'll run into timeouts and will need to make several requests to acquire parts of the collection.

So better check with wgrant first if you're under a reasonable size for the collection or if you already need to handle chunks.

138 + 'OAuth realm="https://api.launchpad.net/"',
139 + 'oauth_version="1.0"',
140 + 'oauth_consumer_key="%s"' % settings.OAUTH_CONSUMER_KEY,
141 + 'oauth_token="%s"' % settings.OAUTH_TOKEN,
142 + 'oauth_signature_method="PLAINTEXT"',
143 + 'oauth_signature="%%26%s"' % settings.OAUTH_TOKEN_SECRET,
144 + 'oauth_timestamp="%d"' % int(oauth.time.time()),
145 + 'oauth_nonce="%s"' % oauth.generate_nonce(),

Great, we already have 3 settings parametrized, I think we also want OAuth realm so we can test against staging or a local launchpad.

161 + url = 'https://api.launchpad.net/1.0/~%s/+archive/%s/?ws.op='

That one probably needs to be parametrized in the same way.

223 + # TODO check for oauth before launching

Sounds like a good test to add ;)

249 + logging.exception('unable to request clean ppa, will retry')

Better add the exception encountered if only to start learning about which ones exist.

266 + logging.exception('error checking if ppa is clean')

idem

354 - # TODO test if this is needed or if there is some type
355 - # of looping logic needed

Yeah, see above, not sure you're talking about the same looping logic but that matches what I'm talking about ;)

364 logging.exception('Could not populate PPAs from launchpad')

Again, mention the exception, the logs are for us and we want precise info ;)

By just reading the proposal, this seems better than the previous version, so +once the tweaks above are addressed ;)

review: Needs Fixing

[Andy Doan (doanac)]

On 12/16/2013 07:34 AM, Vincent Ladeuil wrote:
> Review: Needs Fixing
>
> 21 +# Read for details: https://help.launchpad.net/API/SigningRequests
> 22 +OAUTH_CONSUMER_KEY = 'TODO'
> 23 +OAUTH_TOKEN = 'TODO'
> 24 +OAUTH_TOKEN_SECRET = 'TODO'
> 25 +
>
> Not sure how this relates to your other MP, I'll consider you've addressed the TODO there for this review.

TODO's are filler. Our juju-deployer configs will override this during
deployment.

> 60 - lockname = bundle.data.get('lockname')
>
> \o/ no more lock ;)

Good catch, revno 25

> 79 + if type(state) != int:
>
> O_o can you add a comment to explain ? As I read it it means we'll be ignoring some requests. I'd rather error out unless you have a strong reason to do so... which will make a perfect comment ;)

good catch, revno 26

> 118 +def lp_collection(url):
>
> You probably want to talk to wgrant about that. If I remember correctly, if the collection is big/huge, you'll run into timeouts and will need to make several requests to acquire parts of the collection.
>
> So better check with wgrant first if you're under a reasonable size for the collection or if you already need to handle chunks.

No issue here. Launchpad returns these back in chunks of items. This
function deal with grabbing those chunks and returning them back
one-by-one so that we don't get overloaded.

> 138 + 'OAuth realm="https://api.launchpad.net/"',
> 139 + 'oauth_version="1.0"',
> 140 + 'oauth_consumer_key="%s"' % settings.OAUTH_CONSUMER_KEY,
> 141 + 'oauth_token="%s"' % settings.OAUTH_TOKEN,
> 142 + 'oauth_signature_method="PLAINTEXT"',
> 143 + 'oauth_signature="%%26%s"' % settings.OAUTH_TOKEN_SECRET,
> 144 + 'oauth_timestamp="%d"' % int(oauth.time.time()),
> 145 + 'oauth_nonce="%s"' % oauth.generate_nonce(),
>
> Great, we already have 3 settings parametrized, I think we also want OAuth realm so we can test against staging or a local launchpad.

revno 27

> 161 + url = 'https://api.launchpad.net/1.0/~%s/+archive/%s/?ws.op='
>
> That one probably needs to be parametrized in the same way.

revno: 28

>
> 249 + logging.exception('unable to request clean ppa, will retry')
>
> Better add the exception encountered if only to start learning about which ones exist.
>
> 266 + logging.exception('error checking if ppa is clean')
>

good catch - revno 29

>
> 354 - # TODO test if this is needed or if there is some type
> 355 - # of looping logic needed
>
> Yeah, see above, not sure you're talking about the same looping logic but that matches what I'm talking about ;)

as noted. this MP actually makes this stuff work properly.

> 364 logging.exception('Could not populate PPAs from launchpad')
>
> Again, mention the exception, the logs are for us and we want precise info ;)

no - good catch. i was thinking logging.exception automatically included
the stack trace.

However - in this case i'm re-raising the exception. So I thought the
caller should log the exception. I was concerned both people would log
the stack trace and it would make it 2x longer to read than needed.

I'll go ahead and log it here, because I don't think my rationale is
strong enough :)

> By just reading the proposal, this seems better than the previous versi...

[Chris Johnston (cjohnston)]

Attempt to merge into lp:ubuntu-ci-services-itself failed due to conflicts:

text conflict in juju-deployer/ppa-assigner.yaml

[Chris Johnston (cjohnston)]

Voting does not meet specified criteria. Required: Approve >= 1. Got: 1 Needs Fixing.

[Andy Doan (doanac)]

+1 based on vila's review comment

review: Approve

[lp:~doanac/ubuntu-ci-services-itself/ppa-cleaner]

47. By Andy Doan on 2013-12-18

[r=Andy Doan] Add ppa-cleaning management command to the ppa-assigner from Andy Doan

=== modified file 'ppa-assigner/local_settings.py.example'
--- ppa-assigner/local_settings.py.example	2013-11-26 20:55:48 +0000
+++ ppa-assigner/local_settings.py.example	2013-12-18 18:24:41 +0000
@@ -19,6 +19,11 @@
 
 LAUNCHPAD_PPA_USER = 'TODO'
 
+# Read for details:  https://help.launchpad.net/API/SigningRequests
+OAUTH_CONSUMER_KEY = 'TODO'
+OAUTH_TOKEN = 'TODO'
+OAUTH_TOKEN_SECRET = 'TODO'
+
 BASEDIR = os.path.abspath(os.path.join(os.path.dirname(__file__), '..'))
 
 DATABASES = {

=== modified file 'ppa-assigner/ppa_assigner/api.py'
--- ppa-assigner/ppa_assigner/api.py	2013-11-22 17:47:12 +0000
+++ ppa-assigner/ppa_assigner/api.py	2013-12-18 18:24:41 +0000
@@ -13,11 +13,13 @@
 # You should have received a copy of the GNU Affero General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+import urllib2
+
 from tastypie.api import Api
 from tastypie.http import HttpAccepted, HttpApplicationError
 from tastypie.resources import ALL
 
-from ppa_assigner.models import PPA, LaunchpadError
+from ppa_assigner.models import PPA
 
 from ci_utils.tastypie.resource import AuthorizedResource
 
@@ -28,15 +30,15 @@
         resource_name = 'ppa'
 
         filtering = {
-            'reserved': ALL,
-            'lockname': ALL,
+            'state': ALL,
+            'ticket_id': ALL,
+            'timestamp': ALL,
         }
 
     def obj_create(self, bundle, **kwargs):
         '''We don't really create an object, so override tastypie.'''
-        lockname = bundle.data.get('lockname')
-        clean = bundle.data.get('clean')
-        bundle.obj = PPA.reserve(lockname, clean)
+        ticket_id = bundle.data.get('ticket_id')
+        bundle.obj = PPA.reserve(ticket_id)
         return bundle
 
     def patch_list(self, request, **kwargs):
@@ -45,10 +47,22 @@
                 # populate is a bit different than normal "patch" operations
                 PPA.populate_from_launchpad()
                 return HttpAccepted()
-            except LaunchpadError as e:
+            except urllib2.URLError as e:
                 return HttpApplicationError(e)
         return super(PPAResource, self).patch_list(request, **kwargs)
 
+    def hydrate_state(self, bundle):
+        state = bundle.data['state']
+        # tastypie by default will only let us refer to the state by their
+        # numeric value. This is awkward for users, so this lets a user
+        # specify something like "dirty" rather than 20.
+        if type(state) != int:
+            for val, label in PPA.STATES:
+                if state == label:
+                    bundle.data['state'] = val
+                    break
+        return bundle
+
 
 v1_api = Api(api_name='v1')
 v1_api.register(PPAResource())

=== added file 'ppa-assigner/ppa_assigner/launchpad.py'
--- ppa-assigner/ppa_assigner/launchpad.py	1970-01-01 00:00:00 +0000
+++ ppa-assigner/ppa_assigner/launchpad.py	2013-12-18 18:24:41 +0000
@@ -0,0 +1,87 @@
+# Ubuntu CI Services
+# Copyright 2012-2013 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Affero General Public License version 3, as
+# published by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU Affero General Public License for more details.
+
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import json
+import logging
+import urllib
+import urllib2
+
+from django.conf import settings
+from oauth import oauth
+
+
+def lp_collection(url):
+    '''iterate through each item returned in an lp_collection'''
+    resp = urllib2.urlopen(url)
+    resp = resp.read()
+    data = json.loads(resp)
+
+    for d in data['entries']:
+        yield d
+
+    if 'next_collection_link' in data:
+        for d in lp_collection(data['next_collection_link']):
+            yield d
+
+
+def _oauth_headers():
+    '''oauth header required for a launchpad api post
+
+    https://help.launchpad.net/API/SigningRequests
+    '''
+    values = [
+        'OAuth realm="%s"' % settings.OAUTH_REALM,
+        'oauth_version="1.0"',
+        'oauth_consumer_key="%s"' % settings.OAUTH_CONSUMER_KEY,
+        'oauth_token="%s"' % settings.OAUTH_TOKEN,
+        'oauth_signature_method="PLAINTEXT"',
+        'oauth_signature="%%26%s"' % settings.OAUTH_TOKEN_SECRET,
+        'oauth_timestamp="%d"' % int(oauth.time.time()),
+        'oauth_nonce="%s"' % oauth.generate_nonce(),
+    ]
+    return {
+        'Authorization': ', '.join(values),
+        'Accept': 'application/json',
+        'Content-Type': 'application/x-www-form-urlencoded'
+    }
+
+
+def _lp_post(url, data):
+    req = urllib2.Request(url, urllib.urlencode(data), _oauth_headers())
+    return urllib2.urlopen(req)
+
+
+def get_publishing_history(ppa):
+    '''A generator for the source and binary publish history of a ppa.'''
+    url = settings.LAUNCHPAD_API_BASE + '/~%s/+archive/%s/?ws.op='
+    url = url % (settings.LAUNCHPAD_PPA_USER, ppa.split('/')[-1])
+
+    for x in lp_collection(url + 'getPublishedSources'):
+        yield x
+    for x in lp_collection(url + 'getPublishedBinaries'):
+        yield x
+
+
+def request_ppa_clean(ppa):
+    '''call requestDeletion operation on ppa's source and binary pub history'''
+    for x in get_publishing_history(ppa):
+        if x['date_removed']:
+            continue
+        logging.debug('requesting deletion of: %s', x['self_link'])
+        resp = _lp_post(x['self_link'], {'ws.op': 'requestDeletion'})
+        code = resp.getcode()
+        if code != 200:
+            raise urllib2.HTTPError(
+                x['self_link'], code, resp.read(), resp.headers.headers, None)

=== added directory 'ppa-assigner/ppa_assigner/management'
=== added file 'ppa-assigner/ppa_assigner/management/__init__.py'
=== added directory 'ppa-assigner/ppa_assigner/management/commands'
=== added file 'ppa-assigner/ppa_assigner/management/commands/__init__.py'
=== added file 'ppa-assigner/ppa_assigner/management/commands/clean_ppas.py'
--- ppa-assigner/ppa_assigner/management/commands/clean_ppas.py	1970-01-01 00:00:00 +0000
+++ ppa-assigner/ppa_assigner/management/commands/clean_ppas.py	2013-12-18 18:24:41 +0000
@@ -0,0 +1,79 @@
+# Ubuntu CI Services
+# Copyright 2012-2013 Canonical Ltd.
+
+# This program is free software: you can redistribute it and/or modify it
+# under the terms of the GNU Affero General Public License version 3, as
+# published by the Free Software Foundation.
+
+# This program is distributed in the hope that it will be useful, but
+# WITHOUT ANY WARRANTY; without even the implied warranties of
+# MERCHANTABILITY, SATISFACTORY QUALITY, or FITNESS FOR A PARTICULAR
+# PURPOSE.  See the GNU Affero General Public License for more details.
+
+# You should have received a copy of the GNU Affero General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+import logging
+import time
+
+from optparse import make_option
+
+from django.core.management.base import BaseCommand
+
+from ppa_assigner import launchpad
+from ppa_assigner.models import PPA
+
+
+class Command(BaseCommand):
+    help = 'Runs forever to take dirty PPAs and get them ready for re-use.'
+    option_list = BaseCommand.option_list + (
+        make_option('--interval', dest='interval', default=10, type=int,
+                    help='Interval to sleep between checks.'),
+    )
+
+    # TODO check for oauth before launching
+
+    def handle(self, *args, **options):
+        level = logging.INFO
+        if int(options['verbosity']) > 1:
+            level = logging.DEBUG
+        logging.basicConfig(level=level)
+
+        interval = options['interval']
+        try:
+            while True:
+                self.dirty_to_cleaning()
+                self.cleaning_to_available()
+                time.sleep(interval)
+        except KeyboardInterrupt:
+            print('command cancelled by user')
+
+    def dirty_to_cleaning(self):
+        logging.debug('Check for PPAs that need to be cleaned...')
+        qs = PPA.objects.select_for_update().filter(state=PPA.DIRTY)
+        if qs.count():
+            logging.info('Requesting launchpad clean: %s', qs[0].name)
+            try:
+                launchpad.request_ppa_clean(qs[0].name)
+                qs.filter(pk__in=(qs[0].pk,)).update(state=PPA.CLEANING)
+            except Exception as e:
+                logging.exception('unable to request clean ppa, will retry', e)
+
+    def cleaning_to_available(self):
+        logging.debug('Check for PPAs that are avialable...')
+        qs = PPA.objects.select_for_update().filter(state=PPA.CLEANING)
+        if qs.count():
+            if self._lp_is_clean(qs[0]):
+                logging.info('ppa is available for use: %s', qs[0].name)
+                qs.filter(pk__in=(qs[0].pk,)).update(state=PPA.AVAILABLE)
+
+    def _lp_is_clean(self, ppa):
+        logging.debug('Checking if ppa is ready: %s', ppa.name)
+        try:
+            for x in launchpad.get_publishing_history(ppa.name):
+                if not x['date_removed']:
+                    return False
+        except Exception as e:
+            logging.exception('error checking if ppa is clean', e)
+            return False
+        return True

=== modified file 'ppa-assigner/ppa_assigner/models.py'
--- ppa-assigner/ppa_assigner/models.py	2013-11-26 20:55:48 +0000
+++ ppa-assigner/ppa_assigner/models.py	2013-12-18 18:24:41 +0000
@@ -14,36 +14,43 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import datetime
-import json
 import logging
 import urllib2
 
 from django.conf import settings
 from django.db import models
 
-
-class LaunchpadError(Exception):
-    '''Raised when an error occurrs interacting with Launchpad.'''
+from ppa_assigner import launchpad
 
 
 class PPA(models.Model):
+    AVAILABLE = 0
+    RESERVED = 10
+    DIRTY = 20
+    CLEANING = 30
+    STATES = (
+        (AVAILABLE, 'available'),
+        (RESERVED, 'reserved'),
+        (DIRTY, 'dirty'),
+        (CLEANING, 'cleaning'),
+    )
     name = models.CharField(max_length=4096, unique=True)
 
-    reserved = models.BooleanField(default=False)
-    lockname = models.CharField(max_length=4096, blank=True)
+    state = models.PositiveIntegerField(choices=STATES, default=AVAILABLE)
+    ticket_id = models.PositiveIntegerField()
     timestamp = models.DateTimeField(auto_now_add=True)
 
     def __unicode__(self):
-        reserved = ''
-        if self.reserved:
-            reserved = ' (reserved)'
-            if self.lockname:
-                reserved = '%s - %s' % (reserved, self.lockname)
-        return '%s %s' % (self.name, reserved)
+        val = '%s (%s)' % (self.name, self._get_state_str())
+        if self.state == PPA.RESERVED:
+            val += ' ticket=%s' % self.ticket_id
+        return val
 
-    def clean_artifacts(self):
-        """Cleans up all the artifacts residing in the ppa's."""
-        raise LaunchpadError('TODO how to clean via LP')
+    def _get_state_str(self):
+        for state, label in PPA.STATES:
+            if self.state == state:
+                return label
+        raise RuntimeError('Unsupported state: %s' % self.state)
 
     @staticmethod
     def _merge_ppas(lp_ppas):
@@ -53,12 +60,12 @@
 
         logging.info('adding new ppas: %r', add)
         for x in add:
-            PPA.objects.create(name=x)
+            PPA.objects.create(name=x, ticket_id=0)
 
         logging.info('removing ppas: %r', add)
         for x in remove:
             ppa = PPA.objects.get(name=x)
-            if ppa.reserved:
+            if ppa.state == PPA.RESERVED:
                 # log something, but we may as well delete. because if the
                 # ppa doesn't exist in LP we are already in a catastrophic
                 # situation and shouldn't let any other user re-use this
@@ -68,51 +75,27 @@
     @staticmethod
     def populate_from_launchpad():
         '''Get list of PPAs from launchpad and ensure they exist here.'''
-        url = 'https://api.launchpad.net/1.0/~%s/ppas'
+        url = settings.LAUNCHPAD_API_BASE + '/~%s/ppas'
         url = url % settings.LAUNCHPAD_PPA_USER
         logging.info('populating ppa model from: %s', url)
         try:
-            resp = urllib2.urlopen(url)
-            resp = resp.read()
-            data = json.loads(resp)
-            # TODO test if this is needed or if there is some type
-            # of looping logic needed
-            assert data['total_size'] == len(data['entries'])
             ppas = ['ppa:%s/%s' % (settings.LAUNCHPAD_PPA_USER, x['name'])
-                    for x in data['entries']]
+                    for x in launchpad.lp_collection(url)]
             PPA._merge_ppas(set(ppas))
-
         except urllib2.URLError as e:
-            logging.exception('Could not populate PPAs from launchpad')
-            raise LaunchpadError(e)
+            logging.exception('Could not populate PPAs from launchpad', e)
+            raise
 
     @staticmethod
-    def reserve(lockname='', clean=False):
+    def reserve(ticket_id):
         # "select_for_update" provides proper db-level locking so we
         # don't accidentally have 2 concurrent requests find and reserve the
         # same PPA
-        ppa = None
-        qs = PPA.objects.select_for_update().filter(reserved=False)
+        qs = PPA.objects.select_for_update().filter(state=PPA.AVAILABLE)
         if qs.count() == 0:
             raise PPA.DoesNotExist('No free PPAs exist in pool')
         pk = qs[0].pk
-        qs.filter(pk__in=(pk,)).update(reserved=True, lockname=lockname)
-
-        # we now hold a lock, be careful
-        try:
-            ppa = PPA.objects.get(pk=pk)
-            ppa.timestamp = datetime.datetime.now()
-            if clean:
-                ppa.clean_artifacts()
-
-            ppa.save()
-            return ppa
-        except Exception as e:
-            if ppa:
-                ppa.reserved = False
-                ppa.lockname = ''
-                ppa.save()
-            if not isinstance(e, LaunchpadError):
-                logging.exception('Unexpected exception while cleaning')
-                e = LaunchpadError(e)
-            raise e
+        now = datetime.datetime.utcnow()
+        qs.filter(pk__in=(pk,)).update(
+            state=PPA.RESERVED, ticket_id=ticket_id, timestamp=now)
+        return PPA.objects.get(id=pk)

=== modified file 'ppa-assigner/ppa_assigner/settings.py'
--- ppa-assigner/ppa_assigner/settings.py	2013-12-12 23:22:27 +0000
+++ ppa-assigner/ppa_assigner/settings.py	2013-12-18 18:24:41 +0000
@@ -25,6 +25,12 @@
 MANAGERS = ADMINS
 
 LAUNCHPAD_PPA_USER = 'TODO'
+LAUNCHPAD_API_BASE = 'https://api.launchpad.net/1.0'
+
+OAUTH_REALM = 'https://api.launchpad.net/'
+OAUTH_CONSUMER_KEY = 'TODO'
+OAUTH_TOKEN = 'TODO'
+OAUTH_TOKEN_SECRET = 'TODO'
 
 DATABASES = {
     'default': {

=== modified file 'ppa-assigner/ppa_assigner/tests.py'
--- ppa-assigner/ppa_assigner/tests.py	2013-12-10 18:05:51 +0000
+++ ppa-assigner/ppa_assigner/tests.py	2013-12-18 18:24:41 +0000
@@ -28,7 +28,8 @@
 from django.test import TestCase, TransactionTestCase, skipUnlessDBFeature
 
 from ci_utils.tastypie.test import TastypieTestCase
-from ppa_assigner.models import PPA, LaunchpadError
+from ppa_assigner.models import PPA
+from ppa_assigner.management.commands.clean_ppas import Command
 
 
 class TestModel(TestCase):
@@ -36,53 +37,22 @@
     def setUp(self):
         super(TestModel, self).setUp()
         for n in range(5):
-            PPA.objects.create(name='ci_pool-%s' % str(n).zfill(3))
-
-    @mock.patch('ppa_assigner.models.PPA.clean_artifacts')
-    def testReserveNoClean(self, clean):
-        p = PPA.reserve('testlockname')
-        self.assertIsNotNone(p)
-        self.assertFalse(clean.called)
-        self.assertEqual('testlockname', p.lockname)
-
-        reserved = PPA.objects.filter(reserved=True).count()
-        self.assertEqual(1, reserved)
+            name = 'ci_pool-%s' % str(n).zfill(3)
+            PPA.objects.create(name=name, ticket_id=0)
 
     def testReserveTimestamp(self):
-        p = PPA.reserve()
-        delta = datetime.datetime.now() - p.timestamp
+        ticket = 1234
+        p = PPA.reserve(ticket)
+        delta = datetime.datetime.now(p.timestamp.tzinfo) - p.timestamp
         # the lock should have happened in less than a second
+        self.assertEqual(PPA.RESERVED, p.state)
+        self.assertEqual(ticket, p.ticket_id)
         self.assertLess(delta.seconds, 1)
 
-    @mock.patch('ppa_assigner.models.PPA.clean_artifacts')
-    def testReserveClean(self, clean):
-        p = PPA.reserve('testlockname', True)
-        self.assertIsNotNone(p)
-        self.assertTrue(clean.called)
-        self.assertEqual('testlockname', p.lockname)
-
-        reserved = PPA.objects.filter(reserved=True).count()
-        self.assertEqual(1, reserved)
-
-    @mock.patch('ppa_assigner.models.PPA.clean_artifacts')
-    def testReserveCleanFail(self, clean):
-        '''ensure ppa isn't locked if clean operation fails.'''
-        clean.side_effect = RuntimeError
-        locked_before = PPA.objects.filter(reserved=True).count()
-        with self.assertRaises(LaunchpadError):
-            PPA.reserve(clean=True)
-
-        locked_after = PPA.objects.filter(reserved=True).count()
-        self.assertEqual(locked_before, locked_after)
-
     def testReserveNoFree(self):
-        PPA.objects.update(reserved=True)
+        PPA.objects.update(state=PPA.RESERVED)
         with self.assertRaises(PPA.DoesNotExist):
-            PPA.reserve()
-
-    def testCleanOperation(self):
-        # TODO more logic for this
-        PPA.reserve(clean=True)
+            PPA.reserve(1234)
 
 
 class TestConcurrency(TransactionTestCase):
@@ -93,7 +63,8 @@
         super(TestConcurrency, self).setUp()
         transaction.enter_transaction_management()
         for n in range(5):
-            PPA.objects.create(name='ci_pool-%s' % str(n).zfill(3))
+            name = 'ci_pool-%s' % str(n).zfill(3)
+            PPA.objects.create(name=name, ticket_id=0)
         transaction.commit()
         self.addCleanup(transaction.abort)
 
@@ -119,7 +90,7 @@
     def _reserve(self, params):
         params['signal'].set()
         start = time.time()
-        PPA.reserve()
+        PPA.reserve(1234)
         params['time'] = time.time() - start
         # connections are per-thread, so close this
         connection.close()
@@ -144,49 +115,47 @@
     def setUp(self):
         super(TestApi, self).setUp('/api/v1')
         for n in range(5):
-            PPA.objects.create(name='ci_pool-%s' % str(n).zfill(3))
+            name = 'ci_pool-%s' % str(n).zfill(3)
+            PPA.objects.create(name=name, ticket_id=0)
 
     def testGetAll(self):
         obj = self.get('ppa/')
         self.assertEqual(PPA.objects.all().count(), len(obj['objects']))
 
     def testGetQuery(self):
-        p = PPA.reserve()
-        obj = self.get('ppa/', {'reserved': False})
-        count = PPA.objects.filter(reserved=False).count()
+        p = PPA.reserve(1234)
+        obj = self.get('ppa/', {'state': PPA.AVAILABLE})
+        count = PPA.objects.filter(state=PPA.AVAILABLE).count()
         self.assertEqual(count, len(obj['objects']))
 
-        obj = self.getResource('ppa/', {'reserved': True})
+        obj = self.getResource('ppa/', {'state': PPA.RESERVED})
         self.assertEqual(p.name, obj['name'])
-
-        p.lockname = 'foo'
-        p.save()
-        obj = self.getResource('ppa/', {'reserved': True})
-        self.assertEqual(p.lockname, obj['lockname'])
-
-    def testReserveNoClean(self):
-        params = {'lockname': 'foo'}
+        self.assertEqual(p.ticket_id, obj['ticket_id'])
+
+    def testReserve(self):
+        params = {'ticket_id': 1234}
         obj = self.createResource('ppa/', params)
-        self.assertEqual(obj['lockname'], params['lockname'])
-        self.assertTrue(obj['reserved'])
-
-    @mock.patch('ppa_assigner.models.PPA.clean_artifacts')
-    def testReserveClean(self, clean):
-        obj = self.createResource('ppa/', {'lockname': 'foo', 'clean': True})
-        self.assertTrue(obj['reserved'])
-        self.assertTrue(clean.called)
+        self.assertEqual(obj['ticket_id'], params['ticket_id'])
 
     def testReserveNoFree(self):
-        PPA.objects.update(reserved=True)
-        params = {'lockname': 'foo'}
+        PPA.objects.update(state=PPA.DIRTY)
+        params = {'ticket_id': 1234}
         with self.assertRaises(PPA.DoesNotExist):
             self.post('ppa/', params)
 
-    def testFree(self):
-        p = PPA.reserve()
-        self.patch('ppa/%d/' % p.id, {'reserved': False})
-        p = PPA.objects.get(pk=p.id)
-        self.assertFalse(p.reserved)
+    def testFreeNumeric(self):
+        '''check we can free a ppa with the numeric value in the db'''
+        p = PPA.reserve(1234)
+        self.patch('ppa/%d/' % p.id, {'state': PPA.DIRTY})
+        p = PPA.objects.get(pk=p.id)
+        self.assertEqual(p.state, PPA.DIRTY)
+
+    def testFreeString(self):
+        '''check we can free a ppa with a string value'''
+        p = PPA.reserve(1234)
+        self.patch('ppa/%d/' % p.id, {'state': 'dirty'})
+        p = PPA.objects.get(pk=p.id)
+        self.assertEqual(p.state, PPA.DIRTY)
 
     @mock.patch('urllib2.urlopen')
     def testPopulateFail(self, urlopen):
@@ -219,3 +188,79 @@
         expected = ['ppa:%s/%s' % (user, x['name']) for x in data['entries']]
         found = [x.name for x in PPA.objects.all().order_by('name')]
         self.assertListEqual(expected, found)
+
+
+class TestCleaner(TestCase):
+
+    def setUp(self):
+        super(TestCleaner, self).setUp()
+        for n in range(5):
+            name = 'ci_pool-%s' % str(n).zfill(3)
+            PPA.objects.create(name=name, ticket_id=0)
+
+    @mock.patch('ppa_assigner.launchpad._lp_post')
+    @mock.patch('ppa_assigner.launchpad.get_publishing_history')
+    def testDirtyToClean(self, get_publishing_history, lp_post):
+        p = PPA.objects.get(name='ci_pool-002')
+        p.state = PPA.DIRTY
+        p.save()
+
+        # verify we make one _lp_post call
+        get_publishing_history.return_value = [
+            {'self_link': 'foo', 'date_removed': None},
+            {'self_link': 'foo', 'date_removed': '123'},
+        ]
+        resp_200 = mock.Mock()
+        resp_200.getcode.return_value = 200
+        lp_post.return_value = resp_200
+        Command().dirty_to_cleaning()
+        p = PPA.objects.get(name='ci_pool-002')
+        self.assertEqual(PPA.CLEANING, p.state)
+        lp_post.assert_called_once()
+
+    @mock.patch('ppa_assigner.launchpad.request_ppa_clean')
+    def testDirtyToCleanFailure(self, request_clean):
+        '''make sure state only changes if the call passes.'''
+        p = PPA.objects.get(name='ci_pool-002')
+        p.state = PPA.DIRTY
+        p.save()
+        request_clean.side_effect = RuntimeError('foo bar mocked error')
+
+        Command().dirty_to_cleaning()
+        p = PPA.objects.get(name='ci_pool-002')
+        self.assertEqual(PPA.DIRTY, p.state)
+
+    @mock.patch('ppa_assigner.launchpad.get_publishing_history')
+    def testCleaning(self, get_publishing_history):
+        p = PPA.objects.get(name='ci_pool-002')
+        p.state = PPA.CLEANING
+        p.save()
+
+        # first pass we still have an item, so the state shouldn't change
+        get_publishing_history.return_value = [
+            {'date_removed': None},
+            {'date_removed': '123'},
+        ]
+        Command().cleaning_to_available()
+        p = PPA.objects.get(name='ci_pool-002')
+        self.assertEqual(PPA.CLEANING, p.state)
+
+        get_publishing_history.return_value = [
+            {'date_removed': '123'},
+            {'date_removed': '123'},
+        ]
+        Command().cleaning_to_available()
+        p = PPA.objects.get(name='ci_pool-002')
+        self.assertEqual(PPA.AVAILABLE, p.state)
+
+    @mock.patch('ppa_assigner.launchpad.get_publishing_history')
+    def testCleaningFailure(self, get_publishing_history):
+        '''make sure it handles an exception from launchpad gracefully.'''
+        p = PPA.objects.get(name='ci_pool-002')
+        p.state = PPA.CLEANING
+        p.save()
+        get_publishing_history.side_effect = RuntimeError('foo bar mocked')
+
+        Command().cleaning_to_available()
+        p = PPA.objects.get(name='ci_pool-002')
+        self.assertEqual(PPA.CLEANING, p.state)

=== modified file 'ppa-assigner/setup.py'
--- ppa-assigner/setup.py	2013-11-24 18:27:59 +0000
+++ ppa-assigner/setup.py	2013-12-18 18:24:41 +0000
@@ -37,6 +37,7 @@
     'django-tastypie==0.9.15',
     'South==0.7.5',
     'mock==1.0.1',
+    'oauth==1.0.1'
 ]
 
 setup(