Merge lp:~mwhudson/lava-dispatcher/more-validation into lp:lava-dispatcher

[Michael Hudson-Doyle]

This beefs up the validation we do of the job file and moves it into a module level function so that I can call it in the scheduler and (a) give more immediate feedback about invalid job files and (b) handle the job json without having to worry about getting KeyErrors or TypeErrors all over the place.

[Zygmunt Krynicki (zyga)]

+1

We should think how to validate commands better. Perhaps we could attempt constructing them and calling per-command validate()

review: Approve

[lp:~mwhudson/lava-dispatcher/more-validation]

242. By Michael Hudson-Doyle on 2012-03-08

allow actions to validate their parameters

243. By Michael Hudson-Doyle on 2012-03-08

add validation to all existing actions

244. By Michael Hudson-Doyle on 2012-03-08

fix --validate

245. By Michael Hudson-Doyle on 2012-03-08

oops, validate_parameters is a classmethod

[Michael Hudson-Doyle (mwhudson)]

On Thu, 08 Mar 2012 09:23:19 -0000, Zygmunt Krynicki <email address hidden> wrote:
> Review: Approve
>
> +1
>
> We should think how to validate commands better.

Yes, good idea.

> Perhaps we could attempt constructing them and calling per-command
> validate()

Have a look at what I just pushed. I worry a bit that some of the
errors will be a bit opaque.

One thing that is interesting here is that my immediate reason for doing
this is that I want to do validation in the scheduler but the scheduler
actually accepts a slightly different schema to the dispatcher -- for
example, 'target' is optional for the scheduler but not at all for the
dispatcher, and I'd like to make the 'server' parameter optional for the
submit_results command. Not sure what to do about that though...

Cheers,
mwh

[lp:~mwhudson/lava-dispatcher/more-validation]

246. By Michael Hudson-Doyle on 2012-03-09

merge trunk

247. By Michael Hudson-Doyle on 2012-03-09

add changes

[Zygmunt Krynicki (zyga)]

> On Thu, 08 Mar 2012 09:23:19 -0000, Zygmunt Krynicki
> <email address hidden> wrote:
> > Review: Approve
> >
> > +1
> >
> > We should think how to validate commands better.
>
> Yes, good idea.
>
> > Perhaps we could attempt constructing them and calling per-command
> > validate()
>
> Have a look at what I just pushed. I worry a bit that some of the
> errors will be a bit opaque.

I will (just started reading now). I was thinking that we could expand the schema with custom error traps. This would allow us to keep validation to the schema _and_ provide good user experience. I have not thought much about implementation but I generally think that without general-purpose expressions (turing complete) - something I _don't_ want to offer we need to focus on node-level error traps and violation-specific error messages:

Imagine the following schema:

I want to validate a "cmd_lava_test_install" action. Here's the schema for that action. Let's start with the basic schema. No error handling (apart from validation)

{
    "type": "object",
    "additionalProperties": false,
    "properties": {
        "command": {
            "type": "string",
            "enum": "lava_test_install"
        },
        "tests": {
            "type": "array",
            "items": "string",
            "uniqueItems": true,
            "minItems": 1,
        },
        "install_python": {
            "optional": true,
            "type": "array",
            "items": "string",
            "uniqueItems": true,
            "minItems": 1,
        },
        "register": {
            "optional": true,
            "type": "array",
            "items": "string",
            "uniqueItems": true,
            "minItems": 1,
        }
    }
}

Now let's suppose we want to provide nice error message to all kinds of errors related to the "tests" property:

"lava-errors": {
    "optional": "lava_test_install action must define which tests to install using thest 'tests' property",
    "type": "lava_test_install 'tests' property must be an array of tests to install",
    "items": "lava_test_install 'tests' property must be an array of strings (where each string is a name of test to install)",
    "uniqueItems": "lava_test_install 'tests' property cannot contain duplicates",
    "minItems": "lava_test_install 'tests' property cannot be empty. If you don't want to install any tests then simply omit this action",
}

Each of those items would be used when an error would have been reported by the validator. Notice how this also shows the logical bug in the definition of the schema above. If I just want to install python/json tests then I cannot do it with tests.minItems = 1. Perhaps tests should be optional as well.

Anyway I think this can be very powerful. Remember that we can construct type unions (type may be an alternative of types) and that "dependencies" (http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.8) and "disallow" (http://tools.ietf.org/html/draft-zyp-json-schema-03#section-5.25) are super-powerful to fine tune everything. We could use it to provide conditional validation.

> One thing that is interesting here is that my immediate reason for doing
> ...

[Zygmunt Krynicki (zyga)]

8 +* Make the validation of the job file that happens before a job starts
9 + more rigorous.

Yay for changelogs!

108 +
109 + parameters_schema = null_or_trivial_schema
110 +
111 def run(self):

Won't this break?

If I say:

params: {"foo": 1, "bar": 2}

Then the code will call

run(foo=1, bar=2)

But run() will raise a TypeError then.

82 + 'boot': {'type': 'string'},
83 + 'system': {'type': 'string'},
84 + 'data': {'type': 'string'},

I smell URL validation.

85 + 'pkg': {'type': 'string', 'optional': True},

?

86 + 'use_cache': {'type': 'bool', 'optional': True},

If you have optional then also specify default. This is super nice when you have json-document (and we will soon in the dispatcher). Then you can just say: params.use_cache and the default will be magically there. DRY (no need to keep the default on the python function).

87 + 'rootfstype': {'type': 'string', 'optional': True},

What was this? Is this an enum?

182 + if 'hwpack' in parameters:
183 + if 'rootfs' not in parameters:
184 + raise ValueError('must specify rootfs when specifying hwpack')

You can specify that via the schema:

"hwpack": {
   "requires": "rootfs" // 3rd draft renames this to "dependencies"
}

"rootfs" {
   "requires": "hwpack" // 3rd draft renames this to "dependencies"
}

185 + if 'image' in parameters:
186 + raise ValueError('cannot specify image and hwpack')

This tells me that the schema is a type union. You should list all valid types as complete alternatives and don't resort to optional:

{
"type": [schema_rootfs_plus_hwpack, schema_image]
}

189 + elif 'kernel_matrix' in parameters:
190 + raise ValueError('cannot specify kernel_matrix with an image')

I don't get this. What was kernel_matrix again?

212 + 'result_disk': {'type': 'string', 'optional': True},

What is this?
Hint: let's start using "title" and "description". I'll pull them in json-document pydoc support.

233 + 'timeout': {'type': 'integer', 'optional': True},

Let's add validation against ranges (0 -> something). I'll implement them in the validator.

Otherwise +1

review: Approve

[Michael Hudson-Doyle (mwhudson)]

On Fri, 09 Mar 2012 10:16:20 -0000, Zygmunt Krynicki <email address hidden> wrote:
> Review: Approve
>
> 8 +* Make the validation of the job file that happens before a job starts
> 9 + more rigorous.
>
> Yay for changelogs!

:)

> 108 +
> 109 + parameters_schema = null_or_trivial_schema
> 110 +
> 111 def run(self):
>
> Won't this break?
>
> If I say:
>
> params: {"foo": 1, "bar": 2}
>
> Then the code will call
>
> run(foo=1, bar=2)
>
> But run() will raise a TypeError then.

Ah, maybe null_or_trivial_schema is mis-named. null_or_trivial_schema
matches {} or None/null -- nothing else. I'll change it to
null_or_empty_schema.

> 82 + 'boot': {'type': 'string'},
> 83 + 'system': {'type': 'string'},
> 84 + 'data': {'type': 'string'},
>
> I smell URL validation.

Heh yes.

> 85 + 'pkg': {'type': 'string', 'optional': True},
>
> ?
>
> 86 + 'use_cache': {'type': 'bool', 'optional': True},
>
> If you have optional then also specify default. This is super nice when you have json-document (and we will soon in the dispatcher). Then you can just say: params.use_cache and the default will be magically there. DRY (no need to keep the default on the python function).

Ah yes, that would be nice. I'll add the defaults.
`
Also would be nice to not have parameters as arguments to the run
function, I like the sound of params.use_cache instead...

> 87 + 'rootfstype': {'type': 'string', 'optional': True},
>
> What was this? Is this an enum?

I ... guess it's an enum. I don't know if we want to encoode all the
filesystems supported for mkfs in our master images in the schema
though? I'll leave it as string for now.

> 182 + if 'hwpack' in parameters:
> 183 + if 'rootfs' not in parameters:
> 184 + raise ValueError('must specify rootfs when specifying hwpack')
>
> You can specify that via the schema:
>
> "hwpack": {
> "requires": "rootfs" // 3rd draft renames this to "dependencies"
> }
>
> "rootfs" {
> "requires": "hwpack" // 3rd draft renames this to "dependencies"
> }

Nice. Although your next suggestion is actually better...

> 185 + if 'image' in parameters:
> 186 + raise ValueError('cannot specify image and hwpack')
>
> This tells me that the schema is a type union. You should list all valid types as complete alternatives and don't resort to optional:
>
> {
> "type": [schema_rootfs_plus_hwpack, schema_image]
> }

Ah, I didn't know you could do that.

> 189 + elif 'kernel_matrix' in parameters:
> 190 + raise ValueError('cannot specify kernel_matrix with an image')
>
> I don't get this. What was kernel_matrix again?

I don't remember entirely either. Let's not try to solve all the
problems of the world at once :-)

> 212 + 'result_disk': {'type': 'string', 'optional': True},
>
> What is this?

I don't remember. I'm pretty sure it's useless, but didn't want to do
the legwork to check if anyone has used it...

> Hint: let's start using "title" and "description". I'll pull them in json-document pydoc support.
>
>
> 233 + 'timeout': {'type': 'integer', 'optional': True},
>
> Let's add validation against ranges (0 -> something). I'll implement them in the validator.

What is the syntax for ranges?

>
> Otherwise +1

T...

[lp:~mwhudson/lava-dispatcher/more-validation]

248. By Michael Hudson-Doyle on 2012-03-11

merge trunk

249. By Michael Hudson-Doyle on 2012-03-11

null_or_trivial_schema -> null_or_empty_schema

250. By Michael Hudson-Doyle on 2012-03-11

attempt to use more sophisticated schema features
i failed, but left the attempt in a comment for when the validator bugs get fixed

[Michael Hudson-Doyle (mwhudson)]

On Fri, 09 Mar 2012 10:04:18 -0000, Zygmunt Krynicki <email address hidden> wrote:
> > On Thu, 08 Mar 2012 09:23:19 -0000, Zygmunt Krynicki
> > <email address hidden> wrote:
> > > Review: Approve
> > >
> > > +1
> > >
> > > We should think how to validate commands better.
> >
> > Yes, good idea.
> >
> > > Perhaps we could attempt constructing them and calling per-command
> > > validate()
> >
> > Have a look at what I just pushed. I worry a bit that some of the
> > errors will be a bit opaque.
>
> I will (just started reading now). I was thinking that we could expand
> the schema with custom error traps. This would allow us to keep
> validation to the schema _and_ provide good user experience. I have
> not thought much about implementation but I generally think that
> without general-purpose expressions (turing complete) - something I
> _don't_ want to offer

Yes, having a turing complete schema would more or less totally defeat
the point of the whole excercise :-)

> we need to focus on node-level error traps and violation-specific
> error messages:

When I thought about this, I actually had a different, orthogonoal idea.
How about, when you submit an invalid job to LAVA, you get back a terse
message and a link you can visit for further details? I think the
difficulty in interpreting the messages comes partly from the
difficulty in locating where the problem comes from, so just showing the
message in a helpful way like so:

http://people.linaro.org/~mwh/err.html

will probably help a lot (it helps that job definitions are usually
pretty short of course, this perhaps wouldn't work so well for bundles).

If we implement what you suggest, we could have the best of both worlds
I guess.

> Imagine the following schema:
>
> I want to validate a "cmd_lava_test_install" action. Here's the schema
> for that action. Let's start with the basic schema. No error handling
> (apart from validation)
>
> {
> "type": "object",
> "additionalProperties": false,
> "properties": {
> "command": {
> "type": "string",
> "enum": "lava_test_install"
> },
> "tests": {
> "type": "array",
> "items": "string",
> "uniqueItems": true,
> "minItems": 1,
> },
> "install_python": {
> "optional": true,
> "type": "array",
> "items": "string",
> "uniqueItems": true,
> "minItems": 1,
> },
> "register": {
> "optional": true,
> "type": "array",
> "items": "string",
> "uniqueItems": true,
> "minItems": 1,
> }
> }
> }
>
> Now let's suppose we want to provide nice error message to all kinds
> of errors related to the "tests" property:
>
> "lava-errors": {
> "optional": "lava_test_install action must define which tests to install using thest 'tests' property",
> "type": "lava_test_install 'tests' property must be an array of tests to install",
> "items": "lava_test_install 'tests' property must be an array of strings (where each string is a name of test to install)",
> "uniqueI...

=== modified file 'doc/changes.rst'
--- doc/changes.rst	2012-03-09 01:18:19 +0000
+++ doc/changes.rst	2012-03-11 23:40:23 +0000
@@ -10,6 +10,8 @@
   issue where the server gets busy, and doesn't connect quickly enough
   for getting the tarballs
 * Stop reading the long-obsolete 'image_type' field from the job json.
+* Make the validation of the job file that happens before a job starts
+  more rigorous.
 
 .. _version_0_5_6:
 
 
=== modified file 'lava-dispatch'
--- lava-dispatch	2012-03-01 17:10:04 +0000
+++ lava-dispatch	2012-03-11 23:40:23 +0000
@@ -26,7 +26,7 @@
 
 from json_schema_validator.errors import ValidationError
 
-from lava_dispatcher.job import LavaTestJob
+from lava_dispatcher.job import LavaTestJob, validate_job_data
 from lava_dispatcher.config import get_config
 
 parser = optparse.OptionParser('%prog: lava-dispatch <json job file>')
@@ -83,7 +83,7 @@
 #FIXME Return status
 if options.validate:
     try:
-        job.validate()
+        validate_job_data(job.job_data)
     except ValidationError as e:
         print e
 else:
 
=== modified file 'lava_dispatcher/actions/__init__.py'
--- lava_dispatcher/actions/__init__.py	2012-02-24 01:36:44 +0000
+++ lava_dispatcher/actions/__init__.py	2012-03-11 23:40:23 +0000
@@ -24,6 +24,15 @@
 import imp
 import os
 
+from json_schema_validator.schema import Schema
+from json_schema_validator.validator import Validator
+
+
+null_or_empty_schema = {
+    'type': ['object', 'null'],
+    'additionalProperties': False,
+    }
+
 
 class classproperty(object):
     """Like the builtin @property, but binds to the class not instances."""
@@ -56,6 +65,14 @@
     def test_name(self, **params):
         return self.command_name
 
+    param_schema = None
+
+    @classmethod
+    def validate_parameters(cls, params):
+        if cls.parameters_schema:
+            schema = Schema(cls.parameters_schema)
+            Validator.validate(schema, params)
+
 
 def _find_commands(module):
     cmds = {}
 
=== modified file 'lava_dispatcher/actions/android_deploy.py'
--- lava_dispatcher/actions/android_deploy.py	2011-12-19 02:23:53 +0000
+++ lava_dispatcher/actions/android_deploy.py	2012-03-11 23:40:23 +0000
@@ -23,5 +23,19 @@
 
 
 class cmd_deploy_linaro_android_image(BaseAction):
+
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'boot': {'type': 'string'},
+            'system': {'type': 'string'},
+            'data': {'type': 'string'},
+            'pkg': {'type': 'string', 'optional': True},
+            'use_cache': {'type': 'bool', 'optional': True, 'default': True},
+            'rootfstype': {'type': 'string', 'optional': True, 'default': 'ext4'},
+            },
+        'additionalProperties': False,
+        }
+
     def run(self, boot, system, data, pkg=None, use_cache=True, rootfstype='ext4'):
         self.client.deploy_linaro_android(boot, system, data, pkg, use_cache, rootfstype)
 
=== modified file 'lava_dispatcher/actions/android_install_binaries.py'
--- lava_dispatcher/actions/android_install_binaries.py	2012-01-27 03:10:08 +0000
+++ lava_dispatcher/actions/android_install_binaries.py	2012-03-11 23:40:23 +0000
@@ -19,11 +19,14 @@
 
 import ConfigParser
 import logging
-from lava_dispatcher.actions import BaseAction
+from lava_dispatcher.actions import BaseAction, null_or_empty_schema
 from lava_dispatcher.client.master import _deploy_tarball_to_board
 
 
 class cmd_android_install_binaries(BaseAction):
+
+    parameters_schema = null_or_empty_schema
+
     def run(self):
         try:
             driver_tarball = self.client.device_option(
 
=== modified file 'lava_dispatcher/actions/boot_control.py'
--- lava_dispatcher/actions/boot_control.py	2012-01-26 10:20:09 +0000
+++ lava_dispatcher/actions/boot_control.py	2012-03-11 23:40:23 +0000
@@ -22,12 +22,16 @@
 
 import logging
 
-from lava_dispatcher.actions import BaseAction
+from lava_dispatcher.actions import BaseAction, null_or_empty_schema
 from lava_dispatcher.client.base import CriticalError
 
+
 class cmd_boot_linaro_android_image(BaseAction):
     """ Call client code to boot to the master image
     """
+
+    parameters_schema = null_or_empty_schema
+
     def run(self):
         client = self.client
         try:
@@ -39,6 +43,9 @@
 class cmd_boot_linaro_image(BaseAction):
     """ Call client code to boot to the test image
     """
+
+    parameters_schema = null_or_empty_schema
+
     def run(self):
         client = self.client
         status = 'pass'
@@ -55,6 +62,9 @@
 class cmd_boot_master_image(BaseAction):
     """ Call client code to boot to the master image
     """
+
+    parameters_schema = null_or_empty_schema
+
     def run(self):
         client = self.client
         client.boot_master_image()
 
=== modified file 'lava_dispatcher/actions/deploy.py'
--- lava_dispatcher/actions/deploy.py	2012-01-31 01:20:47 +0000
+++ lava_dispatcher/actions/deploy.py	2012-03-11 23:40:23 +0000
@@ -21,6 +21,63 @@
 
 
 class cmd_deploy_linaro_image(BaseAction):
-    def run(self, hwpack=None, rootfs=None, image=None, kernel_matrix=None, use_cache=True, rootfstype='ext3'):
+
+    # This is how the schema for parameters should look, but there are bugs in
+    # json_schema_validation that means it doesn't work (see
+    # https://github.com/zyga/json-schema-validator/pull/6).
+
+    ## parameters_schema = {
+    ##     'type': [
+    ##         {
+    ##             'type': 'object',
+    ##             'properties': {
+    ##                 'image': {'type': 'string'},
+    ##                 },
+    ##             'additionalProperties': False,
+    ##             },
+    ##         {
+    ##             'type': 'object',
+    ##             'properties': {
+    ##                 'hwpack': {'type': 'string'},
+    ##                 'rootfs': {'type': 'string'},
+    ##                 'kernel_matrix': {'type': 'string', 'optional': True},
+    ##                 'use_cache': {'type': 'bool', 'optional': True, 'default': True},
+    ##                 'rootfstype': {'type': 'string', 'optional': True, 'default': 'ext3'},
+    ##                 },
+    ##             'additionalProperties': False,
+    ##             },
+    ##         ],
+    ##     }
+
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'hwpack': {'type': 'string', 'optional': True},
+            'rootfs': {'type': 'string', 'optional': True},
+            'image': {'type': 'string', 'optional': True},
+            'kernel_matrix': {'type': 'string', 'optional': True},
+            'use_cache': {'type': 'bool', 'optional': True},
+            'rootfstype': {'type': 'string', 'optional': True},
+            },
+        'additionalProperties': False,
+        }
+
+    @classmethod
+    def validate_parameters(cls, parameters):
+        super(cmd_deploy_linaro_image, cls).validate_parameters(parameters)
+        if 'hwpack' in parameters:
+            if 'rootfs' not in parameters:
+                raise ValueError('must specify rootfs when specifying hwpack')
+            if 'image' in parameters:
+                raise ValueError('cannot specify image and hwpack')
+        elif 'image' not in parameters:
+            raise ValueError('must specify image if not specifying a hwpack')
+        elif 'kernel_matrix' in parameters:
+            raise ValueError('cannot specify kernel_matrix with an image')
+
+    def run(self, hwpack=None, rootfs=None, image=None, kernel_matrix=None,
+            use_cache=True, rootfstype='ext3'):
         self.client.deploy_linaro(
-            hwpack=hwpack, rootfs=rootfs, image=image, kernel_matrix=kernel_matrix, use_cache=use_cache, rootfstype=rootfstype)
+            hwpack=hwpack, rootfs=rootfs, image=image,
+            kernel_matrix=kernel_matrix, use_cache=use_cache,
+            rootfstype=rootfstype)
 
=== modified file 'lava_dispatcher/actions/launch_control.py'
--- lava_dispatcher/actions/launch_control.py	2012-02-24 02:13:45 +0000
+++ lava_dispatcher/actions/launch_control.py	2012-03-11 23:40:23 +0000
@@ -77,6 +77,17 @@
 
 class cmd_submit_results(BaseAction):
 
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'server': {'type': 'string'},
+            'stream': {'type': 'string'},
+            'result_disk': {'type': 'string', 'optional': True},
+            'token': {'type': 'string', 'optional': True},
+            },
+        'additionalProperties': False,
+        }
+
     def _get_bundles_from_device(self, result_disk):
         err_msg = ''
         status = 'fail'
 
=== modified file 'lava_dispatcher/actions/lava-android-test.py'
--- lava_dispatcher/actions/lava-android-test.py	2011-11-24 03:00:54 +0000
+++ lava_dispatcher/actions/lava-android-test.py	2012-03-11 23:40:23 +0000
@@ -36,6 +36,15 @@
 
 class cmd_lava_android_test_run(AndroidTestAction):
 
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'test_name': {'type': 'string'},
+            'timeout': {'type': 'integer', 'optional': True},
+            },
+        'additionalProperties': False,
+        }
+
     def test_name(self, test_name, timeout=-1):
         return super(cmd_lava_android_test_run, self).test_name() + \
                ' (%s)' % test_name
@@ -61,6 +70,17 @@
     """
     lava-test deployment to test image rootfs by chroot
     """
+
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'tests': {'type': 'array', 'items': {'type': 'string'}},
+            'option': {'type': 'string', 'optional': True},
+            'timeout': {'type': 'integer', 'optional': True},
+            },
+        'additionalProperties': False,
+        }
+
     def run(self, tests, option=None, timeout=2400):
         self.check_lava_android_test_installed()
         with self.client.android_tester_session() as session:
 
=== modified file 'lava_dispatcher/actions/lava-test.py'
--- lava_dispatcher/actions/lava-test.py	2012-03-08 21:52:44 +0000
+++ lava_dispatcher/actions/lava-test.py	2012-03-11 23:40:23 +0000
@@ -49,6 +49,16 @@
 
 class cmd_lava_test_run(BaseAction):
 
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'test_name': {'type': 'string'},
+            'test_options': {'type': 'string', 'optional': True},
+            'timeout': {'type': 'integer', 'optional': True},
+            },
+        'additionalProperties': False,
+        }
+
     def test_name(self, test_name, test_options = "", timeout=-1):
         return super(cmd_lava_test_run, self).test_name() + ' (%s)' % test_name
 
@@ -85,6 +95,22 @@
     """
     lava-test deployment to test image rootfs by chroot
     """
+
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'tests': {'type': 'array', 'items': {'type': 'string'}},
+            'install_python': {
+                'type': 'array', 'items': {'type': 'string'}, 'optional': True
+                },
+            'register': {
+                'type': 'array', 'items': {'type': 'string'}, 'optional': True
+                },
+            'timeout': {'type': 'integer', 'optional': True},
+            },
+        'additionalProperties': False,
+        }
+
     def run(self, tests, install_python = None, register = None, timeout=2400):
         logging.info("Executing lava_test_install (%s) command" % ",".join(tests))
 
@@ -112,6 +138,15 @@
     add apt repository to test image rootfs by chroot
     arg could be 'deb uri distribution [component1] [component2][...]' or ppa:<ppa_name>
     """
+
+    parameters_schema = {
+        'type': 'object',
+        'properties': {
+            'arg': {'type': 'string'},
+            },
+        'additionalProperties': False,
+        }
+
     def run(self, arg):
         with self.client.reliable_session() as session:
 
 
=== modified file 'lava_dispatcher/job.py'
--- lava_dispatcher/job.py	2012-03-09 01:18:19 +0000
+++ lava_dispatcher/job.py	2012-03-11 23:40:23 +0000
@@ -28,8 +28,7 @@
 
 from lava_dispatcher.actions import get_all_cmds
 from lava_dispatcher.client.base import CriticalError, GeneralError
-from lava_dispatcher.config import get_config
-from lava_dispatcher.context import LavaContext 
+from lava_dispatcher.context import LavaContext
 
 
 job_schema = {
@@ -38,12 +37,15 @@
     'properties': {
         'actions': {
             'items': {
+                'type': 'object',
                 'properties': {
                     'command': {
                         'optional': False,
+                        'type': 'string',
                         },
                     'parameters': {
                         'optional': True,
+                        'type': 'object',
                         },
                     'metadata': {
                         'optional': True,
@@ -53,12 +55,15 @@
                 },
             },
         'device_type': {
+            'type': 'string',
             'optional': True,
             },
         'job_name': {
+            'type': 'string',
             'optional': True,
             },
         'target': {
+            'type': 'string',
             'optional': True,
             },
         'timeout': {
@@ -66,11 +71,27 @@
             'optional': False,
             },
         'logging_level': {
+            'type': 'string',
+            'enum': ["CRITICAL", "ERROR", "WARNING", "INFO", "DEBUG"],
             'optional': True,
             },
         },
     }
 
+
+def validate_job_data(job_data):
+    schema = Schema(job_schema)
+    validator = Validator()
+    validator.validate(schema, job_data)
+    lava_commands = get_all_cmds()
+    for action in job_data['actions']:
+        command_name = action['command']
+        command = lava_commands.get(command_name)
+        if command is None:
+            raise ValueError("action %r not known" % command_name)
+        command.validate_parameters(action.get('parameters'))
+
+
 class LavaTestJob(object):
     def __init__(self, job_json, oob_file, config):
         self.job_status = 'pass'
@@ -92,19 +113,8 @@
         except :
             return None
 
-    def validate(self):
-        schema = Schema(job_schema)
-        validator = Validator()
-        validator.validate(schema, self.job_data)
-
-        lava_commands = get_all_cmds()
-        for action in self.job_data['actions']:
-            command_name = action['command']
-            if command_name not in lava_commands:
-                raise CriticalError("action %r not known" % command_name)
-
     def run(self):
-        self.validate()
+        validate_job_data(self.job_data)
         self._set_logging_level()
         lava_commands = get_all_cmds()