Code review comment for lp:~mwhudson/lava-dispatcher/more-validation

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)",
> "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.

Noone uses install_python anyway, do they? :)

> 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.

It does sound interesting, for sure. I think it's probably reasonably
easy to implement, too?

> > 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
>
> We can do that with "extends" (schema inheritance) but I'd have to
> check if it can loosen the specification. In any way I'm sure we can
> support that with a non-standard extension (like extends + loosen)

extends cannot loosen the specification (I checked).

I think maybe having a core schema and two extensions (one for the
scheduler and one for the dispatcher) would be simple enough, or just
having a small piece of code that mutates one schema into the other
would also be OK I think.

> > dispatcher, and I'd like to make the 'server' parameter optional for the
> > submit_results command. Not sure what to do about that though...
>
> I think server should really go away. It should be just the pathname
> and we should really really really really merge the two.

The dispatcher needs the server as long as it is the one submitting the
results (I realize that you would like to change this too :-p). Not
sure what you mean by merging them.

Cheers,
mwh