Merge lp:~mwhudson/lava-scheduler/create-private-job into lp:lava-scheduler

[Michael Hudson-Doyle]

This branch enables the creation of private testjobs. It does so by looking for the bundle stream named by the submit_results action and copying the access data over to the new job. Because this means poking around in the JSON way more than before, I also finally implemented validation of the json at submit_job time -- this branch depends on https://code.launchpad.net/~mwhudson/lava-dispatcher/more-validation/+merge/96507 -- which should be a useful usability improvement in its own right anyway. It did mean that many tests needed to be built up to create more realistic jobs though.

So, what do you think? I think I more or less like this approach -- it avoids repetition of access information and the potential for inconsistency there. I do find grubbing through the actions for the submit_results action to be a bit strange still, both for the UX and the implementation -- I think I would be happier if the bundle_stream name and (ideally, optional, at least for jobs that run through the scheduler) xml-rpc endpoint were top level properties of the job description. But that can wait for another day, perhaps.

Cheers,
mwh

[Zygmunt Krynicki (zyga)]

I think you forgot the prerequisite branch - this makes reading it harder.

60 + try:
61 + bundle_stream = BundleStream.objects.get(pathname=stream)
62 + except BundleStream.DoesNotExist:
63 + raise ValueError("stream %s not found" % stream)

429 + "lava-dashboard",

So it's official (netcraft confirms it). The scheduler depends on the dashboard now. Can we please look at how to merge them? This is a bigger topic. I'd like to get rid of lots of things from the dashboard proper. Perhaps we should move the base models to lava-server (tree-wise) application. lava.silo or lava.testdata or something like that.

[Zygmunt Krynicki (zyga)]

Otherwise this looks good :-) +1

review: Approve

Unmerged revisions

=== modified file 'lava_scheduler_app/models.py'
--- lava_scheduler_app/models.py	2012-03-07 01:56:57 +0000
+++ lava_scheduler_app/models.py	2012-03-09 04:16:33 +0000
@@ -3,16 +3,17 @@
 from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
 from django.db import models
-from django.db.models.query import QuerySet
 from django.utils.translation import ugettext as _
 
-
-from django_restricted_resource.managers import RestrictedResourceManager
 from django_restricted_resource.models import RestrictedResource
-from django_restricted_resource.utils import filter_bogus_users
+
+from dashboard_app.models import BundleStream
+
+from lava_dispatcher.job import validate_job_data
 
 from linaro_django_xmlrpc.models import AuthToken
 
+
 class JSONDataError(ValueError):
     """Error raised when JSON is syntactically valid but ill-formed."""
 
@@ -30,12 +31,9 @@
 def validate_job_json(data):
     try:
         ob = simplejson.loads(data)
+        validate_job_data(ob)
     except ValueError, e:
         raise ValidationError(str(e))
-    else:
-        if not isinstance(ob, dict):
-            raise ValidationError(
-                "job json must be an object, not %s" % type(ob).__name__)
 
 
 class DeviceType(models.Model):
@@ -266,6 +264,7 @@
     @classmethod
     def from_json_and_user(cls, json_data, user):
         job_data = simplejson.loads(json_data)
+        validate_job_data(job_data)
         if 'target' in job_data:
             target = Device.objects.get(hostname=job_data['target'])
             device_type = None
@@ -279,6 +278,25 @@
 
         is_check = job_data.get('health_check', False)
 
+        submitter = user
+        group = None
+        is_public = True
+
+        for action in job_data['actions']:
+            if not action['command'].startswith('submit_results'):
+                continue
+            stream = action['parameters']['stream']
+            try:
+                bundle_stream = BundleStream.objects.get(pathname=stream)
+            except BundleStream.DoesNotExist:
+                raise ValueError("stream %s not found" % stream)
+            if not bundle_stream.is_owned_by(submitter):
+                raise ValueError(
+                    "you cannot submit to the stream %s" % stream)
+            user, group, is_public = (bundle_stream.user,
+                                      bundle_stream.group,
+                                      bundle_stream.is_public)
+
         tags = []
         for tag_name in job_data.get('device_tags', []):
             try:
@@ -286,9 +304,10 @@
             except Tag.DoesNotExist:
                 raise JSONDataError("tag %r does not exist" % tag_name)
         job = TestJob(
-            definition=json_data, submitter=user, requested_device=target,
-            requested_device_type=device_type, description=job_name,
-            health_check=is_check, user=user)
+            definition=json_data, submitter=submitter,
+            requested_device=target, requested_device_type=device_type,
+            description=job_name, health_check=is_check, user=user,
+            group=group, is_public=is_public)
         job.save()
         for tag in tags:
             job.tags.add(tag)

=== modified file 'lava_scheduler_app/tests.py'
--- lava_scheduler_app/tests.py	2012-02-28 03:53:16 +0000
+++ lava_scheduler_app/tests.py	2012-03-09 04:16:33 +0000
@@ -3,7 +3,9 @@
 import json
 import xmlrpclib
 
-from django.contrib.auth.models import Permission, User
+from dashboard_app.models import BundleStream
+
+from django.contrib.auth.models import Group, Permission, User
 from django.test import TransactionTestCase
 from django.test.client import Client
 
@@ -84,9 +86,22 @@
         device.save()
         return device
 
+    def make_job_data(self, actions=[], **kw):
+        data = {'actions': actions, 'timeout': 1}
+        data.update(kw)
+        if 'target' not in data and 'device_type' not in data:
+            if DeviceType.objects.all():
+                data['device_type'] = DeviceType.objects.all()[0].name
+            else:
+                data['device_type'] = self.ensure_device_type().name
+        return data
+
+    def make_job_json(self, **kw):
+        return json.dumps(self.make_job_data(**kw))
+
     def make_testjob(self, definition=None, submitter=None, **kwargs):
         if definition is None:
-            definition = json.dumps({})
+            definition = self.make_job_json()
         if submitter is None:
             submitter = self.make_user()
         if 'user' not in kwargs:
@@ -107,70 +122,68 @@
 class TestTestJob(TestCaseWithFactory):
 
     def test_from_json_and_user_sets_definition(self):
-        self.factory.ensure_device_type(name='panda')
-        definition = json.dumps({'device_type':'panda'})
+        definition = self.factory.make_job_json()
         job = TestJob.from_json_and_user(definition, self.factory.make_user())
         self.assertEqual(definition, job.definition)
 
     def test_from_json_and_user_sets_submitter(self):
-        self.factory.ensure_device_type(name='panda')
         user = self.factory.make_user()
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda'}), user)
+            self.factory.make_job_json(), user)
         self.assertEqual(user, job.submitter)
 
     def test_from_json_and_user_sets_device_type(self):
         panda_type = self.factory.ensure_device_type(name='panda')
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda'}), self.factory.make_user())
+            self.factory.make_job_json(device_type='panda'),
+            self.factory.make_user())
         self.assertEqual(panda_type, job.requested_device_type)
 
     def test_from_json_and_user_sets_target(self):
         panda_board = self.factory.make_device(hostname='panda01')
         job = TestJob.from_json_and_user(
-            json.dumps({'target':'panda01'}), self.factory.make_user())
+            self.factory.make_job_json(target='panda01'),
+            self.factory.make_user())
         self.assertEqual(panda_board, job.requested_device)
 
     def test_from_json_and_user_does_not_set_device_type_from_target(self):
         panda_type = self.factory.ensure_device_type(name='panda')
         self.factory.make_device(device_type=panda_type, hostname='panda01')
         job = TestJob.from_json_and_user(
-            json.dumps({'target':'panda01'}), self.factory.make_user())
+            self.factory.make_job_json(target='panda01'),
+            self.factory.make_user())
         self.assertEqual(None, job.requested_device_type)
 
     def test_from_json_and_user_sets_date_submitted(self):
-        self.factory.ensure_device_type(name='panda')
         before = datetime.datetime.now()
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda'}), self.factory.make_user())
+            self.factory.make_job_json(),
+            self.factory.make_user())
         after = datetime.datetime.now()
         self.assertTrue(before < job.submit_time < after)
 
     def test_from_json_and_user_sets_status_to_SUBMITTED(self):
-        self.factory.ensure_device_type(name='panda')
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda'}), self.factory.make_user())
+            self.factory.make_job_json(),
+            self.factory.make_user())
         self.assertEqual(job.status, TestJob.SUBMITTED)
 
     def test_from_json_and_user_sets_no_tags_if_no_tags(self):
-        self.factory.ensure_device_type(name='panda')
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda', 'device_tags':[]}),
+            self.factory.make_job_json(device_tags=[]),
             self.factory.make_user())
         self.assertEqual(set(job.tags.all()), set([]))
 
     def test_from_json_and_user_errors_on_unknown_tags(self):
-        self.factory.ensure_device_type(name='panda')
         self.assertRaises(
             JSONDataError, TestJob.from_json_and_user,
-            json.dumps({'device_type':'panda', 'device_tags':['unknown']}),
+            self.factory.make_job_json(device_tags=['unknown']),
             self.factory.make_user())
 
     def test_from_json_and_user_sets_tag_from_device_tags(self):
-        self.factory.ensure_device_type(name='panda')
         self.factory.ensure_tag('tag')
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda', 'device_tags':['tag']}),
+            self.factory.make_job_json(device_tags=['tag']),
             self.factory.make_user())
         self.assertEqual(
             set(tag.name for tag in job.tags.all()), set(['tag']))
@@ -180,7 +193,7 @@
         self.factory.ensure_tag('tag1')
         self.factory.ensure_tag('tag2')
         job = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda', 'device_tags':['tag1', 'tag2']}),
+            self.factory.make_job_json(device_tags=['tag1', 'tag2']),
             self.factory.make_user())
         self.assertEqual(
             set(tag.name for tag in job.tags.all()), set(['tag1', 'tag2']))
@@ -189,15 +202,75 @@
         self.factory.ensure_device_type(name='panda')
         self.factory.ensure_tag('tag')
         job1 = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda', 'device_tags':['tag']}),
+            self.factory.make_job_json(device_tags=['tag']),
             self.factory.make_user())
         job2 = TestJob.from_json_and_user(
-            json.dumps({'device_type':'panda', 'device_tags':['tag']}),
+            self.factory.make_job_json(device_tags=['tag']),
             self.factory.make_user())
         self.assertEqual(
             set(tag.pk for tag in job1.tags.all()),
             set(tag.pk for tag in job2.tags.all()))
 
+    def test_from_json_and_user_rejects_invalid_json(self):
+        self.assertRaises(
+            ValueError, TestJob.from_json_and_user, '{',
+            self.factory.make_user())
+
+    def test_from_json_and_user_rejects_invalid_job(self):
+        # job data must have the 'actions' and 'timeout' properties, so this
+        # will be rejected.
+        self.assertRaises(
+            ValueError, TestJob.from_json_and_user, '{}',
+            self.factory.make_user())
+
+    def make_job_json_for_stream_name(self, stream_name):
+        return self.factory.make_job_json(
+            actions=[
+                {
+                    'command':'submit_results',
+                    'parameters': {
+                        'server': '...',
+                        'stream': stream_name,
+                        }
+                    }
+                ])
+
+    def test_from_json_and_user_sets_group_from_bundlestream(self):
+        group = Group.objects.create(name='group')
+        user = self.factory.make_user()
+        user.groups.add(group)
+        b = BundleStream.objects.create(
+            group=group, slug='blah', is_public=True)
+        b.save()
+        j = self.make_job_json_for_stream_name(b.pathname)
+        job = TestJob.from_json_and_user(j, user)
+        self.assertEqual(group, job.group)
+
+    def test_from_json_and_user_sets_is_public_from_bundlestream(self):
+        group = Group.objects.create(name='group')
+        user = self.factory.make_user()
+        user.groups.add(group)
+        b = BundleStream.objects.create(
+            group=group, slug='blah', is_public=False)
+        b.save()
+        j = self.make_job_json_for_stream_name(b.pathname)
+        job = TestJob.from_json_and_user(j, user)
+        self.assertEqual(False, job.is_public)
+
+    def test_from_json_and_user_rejects_missing_bundlestream(self):
+        user = self.factory.make_user()
+        j = self.make_job_json_for_stream_name('no such stream')
+        self.assertRaises(ValueError, TestJob.from_json_and_user, j, user)
+
+    def test_from_json_and_user_rejects_inaccessible_bundlestream(self):
+        stream_user = self.factory.make_user()
+        job_user = self.factory.make_user()
+        b = BundleStream.objects.create(
+            user=stream_user, slug='blah', is_public=True)
+        b.save()
+        j = self.make_job_json_for_stream_name(b.pathname)
+        self.assertRaises(ValueError, TestJob.from_json_and_user, j, job_user)
+
 
 class TestSchedulerAPI(TestCaseWithFactory):
 
@@ -231,8 +304,7 @@
             Permission.objects.get(codename='add_testjob'))
         user.save()
         server = self.server_proxy('test', 'test')
-        self.factory.ensure_device_type(name='panda')
-        definition = json.dumps({'device_type':'panda'})
+        definition = self.factory.make_job_json()
         job_id = server.scheduler.submit_job(definition)
         job = TestJob.objects.get(id=job_id)
         self.assertEqual(definition, job.definition)
@@ -296,14 +368,19 @@
 
     def test_getJobForBoard_returns_json(self):
         device = self.factory.make_device(hostname='panda01')
-        definition = {'foo': 'bar', 'target': 'panda01'}
+        definition = self.factory.make_job_data(target='panda01')
         self.factory.make_testjob(
             requested_device=device, definition=json.dumps(definition))
         self.assertEqual(
             definition, self.source.getJobForBoard('panda01'))
 
-    health_job = json.dumps({'health_check': True})
-    ordinary_job = json.dumps({'health_check': False})
+    @property
+    def health_job(self):
+        return self.factory.make_job_json(health_check=True)
+
+    @property
+    def ordinary_job(self):
+        return self.factory.make_job_json(health_check=False)
 
     def assertHealthJobAssigned(self, device):
         job_data = self.source.getJobForBoard(device.hostname)
@@ -371,7 +448,7 @@
     def test_getJobForBoard_considers_device_type(self):
         panda_type = self.factory.ensure_device_type(name='panda')
         self.factory.make_device(hostname='panda01', device_type=panda_type)
-        definition = {'foo': 'bar'}
+        definition = self.factory.make_job_data()
         self.factory.make_testjob(
             requested_device_type=panda_type,
             definition=json.dumps(definition))
@@ -383,8 +460,8 @@
         panda_type = self.factory.ensure_device_type(name='panda')
         panda01 = self.factory.make_device(
             hostname='panda01', device_type=panda_type)
-        first_definition = {'foo': 'bar', 'target': 'panda01'}
-        second_definition = {'foo': 'baz', 'target': 'panda01'}
+        first_definition = self.factory.make_job_data(foo='bar', target='panda01')
+        second_definition = self.factory.make_job_data(foo='baz', target='panda01')
         self.factory.make_testjob(
             requested_device=panda01, definition=json.dumps(first_definition),
             submit_time=datetime.datetime.now() - datetime.timedelta(days=1))
@@ -399,12 +476,13 @@
         panda_type = self.factory.ensure_device_type(name='panda')
         panda01 = self.factory.make_device(
             hostname='panda01', device_type=panda_type)
-        type_definition = {'foo': 'bar'}
+        type_definition = self.factory.make_job_data()
         self.factory.make_testjob(
             requested_device_type=panda_type,
             definition=json.dumps(type_definition),
             submit_time=datetime.datetime.now() - datetime.timedelta(days=1))
-        device_definition = {'foo': 'baz', 'target': 'panda01'}
+        device_definition = self.factory.make_job_data(
+            foo='baz', target='panda01')
         self.factory.make_testjob(
             requested_device=panda01,
             definition=json.dumps(device_definition))
@@ -417,7 +495,7 @@
         panda01 = self.factory.make_device(
             hostname='panda01', device_type=panda_type)
         self.factory.make_device(hostname='panda02', device_type=panda_type)
-        definition = {'foo': 'bar', 'target': 'panda01'}
+        definition = self.factory.make_job_data(foo='bar', target='panda01')
         self.factory.make_testjob(
             requested_device=panda01,
             definition=json.dumps(definition))
@@ -515,7 +593,7 @@
     def test_getJobForBoard_inserts_target_into_json(self):
         panda_type = self.factory.ensure_device_type(name='panda')
         self.factory.make_device(hostname='panda01', device_type=panda_type)
-        definition = {'foo': 'bar'}
+        definition = self.factory.make_job_data(device_type='panda')
         self.factory.make_testjob(
             requested_device_type=panda_type,
             definition=json.dumps(definition))

=== modified file 'lava_scheduler_daemon/dbjobsource.py'
--- lava_scheduler_daemon/dbjobsource.py	2012-03-07 01:59:51 +0000
+++ lava_scheduler_daemon/dbjobsource.py	2012-03-09 04:16:33 +0000
@@ -88,25 +88,11 @@
     def _get_json_data(self, job):
         json_data = json.loads(job.definition)
         json_data['target'] = job.actual_device.hostname
-        # The rather extreme paranoia in what follows could be much reduced if
-        # we thoroughly validated job data in submit_job.  We don't (yet?)
-        # and there is no sane way to report errors at this stage, so,
-        # paranoia (the dispatcher will choke on bogus input in a more
-        # informative way).
-        if 'actions' not in json_data:
-            return json_data
-        actions = json_data['actions']
-        for action in actions:
-            if not isinstance(action, dict):
-                continue
-            if action.get('command') != 'submit_results':
-                continue
-            params = action.get('parameters')
-            if not isinstance(params, dict):
-                continue
+        for action in json_data['actions']:
+            if not action['command'].startswith('submit_results'):
+                continue
+            params = action['parameters']
             params['token'] = job.submit_token.secret
-            if not 'server' in params or not isinstance(params['server'], unicode):
-                continue
             parsed = urlparse.urlsplit(params['server'])
             netloc = job.submitter.username + '@' + parsed.hostname
             parsed = list(parsed)

=== modified file 'setup.py'
--- setup.py	2012-03-07 03:49:20 +0000
+++ setup.py	2012-03-09 04:16:33 +0000
@@ -35,6 +35,8 @@
     install_requires=[
         "django-restricted-resource",
         "django-tables2 >= 0.9.4",
+        "lava-dashboard",
+        "lava-dispatcher",
         "lava-server >= 0.10",
         "simplejson",
         "south >= 0.7.3",