Add support for an --image flag to atest.

Add support for an --image flag to atest.

This enables creating jobs from the CLI that use autoserv to install a new OS image before running the requested test.

This was not added to the database like the reboot before/reboot after flags to maintain backward compatibility and avoid any changes to the database schema.

Also it is not working as a pure parameterized job as the current implementation is not 100% complete and would require more work to finish.  And since mixed jobs are not allowed it would also mean moving existing control file jobs to parameterized jobs.

So the implementation of adding a parameterized id to control jobs and using a known test to hold the OS image path is the most straight forward of the options.

Change-Id: I77cdda0c50c222a4c594da2626a71fa55f5957cb

BUG=chromium-os:11486
TEST=Manual testing using atest cli to create jobs with --image parameters and verifying the value is passed to autoserv.

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=5a8c6ad

[pauldean_chromium, 2011-01-10 20:32:22]

Hi Dale & Mike,

Can you guys take a look at this change?  Before he left Eric said Mike was the
king of the front-end/CLI and Dale was the king of the scheduler/server.  So I'm
interested to hear what you think of this CL.

Thanks,
paul

http://codereview.chromium.org/6181003/diff/1/frontend/afe/models.py
File frontend/afe/models.py (left):

http://codereview.chromium.org/6181003/diff/1/frontend/afe/models.py#oldcode996
frontend/afe/models.py:996:
cls.check_parameterized_job(control_file=control_file,
The current implementation of parameterized jobs requires that only control
files or parameterized jobs are used.  While this doesn't mix pure parameterized
jobs and control files jobs, it does muck enough with normal jobs by adding a
parameterized id to them that this check will fail.  So for now we are just
skipping this check.

[DaleCurtis, 2011-01-11 00:46:05]

I asked eric this the other day: Will cloned jobs re-initiate the image process?
Currently if you clone a control file based image job, it will perform the image
process again.

What's your plan for rolling this out?

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py
File scheduler/scheduler_models.py (right):

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:871: # All image update jobs have the
parameterized_job_id set.
Nit, but I think comment should be on line above.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:875: sql_str = ('SELECT test_id '
Use """ syntax like in the rest of the file.

Also, normally you never want to inject values directly into your query string
like this. With user-controlled values you could face SQL injection attacks.

The preferred method for populating values into the query string is to use some
marker character (? or %s in this case) and then pass the values in through
parameters=(...) on the execute call. See get_host_queue_entries() above.

Using parameters=(...) also takes care of having to worry about escaping or
quoting values. In your case if parameterized_job_id had spaces or any other
funny characters, the query would break.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:881: test_id = rows[0][0]
Is it possible for rows[n] to be None or something similar? Do other areas of
code to any further checking?

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:884: sql_str = ('SELECT id '
Same comment as above.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:890: update_id = rows[0][0]
Same comment as above.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:893: if test_id == update_id:
Will all parameterized jobs always be image jobs? Is there a safer test for
this?

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:895: sql_str = ('SELECT parameter_value '
Same comment as above.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:901: # The image cannot be updated without an OS
path.
Consider reflowing w/ else: return False below. Drops some code and possibly
more readable to say "if rows: blah blah; return True" Up to you.

Doing without the else: means if anyone modifies the code in the future is
harder for them to miss a case which returns no value or returns the wrong
value.

[pauldean_chromium, 2011-01-12 18:12:52]

Thanks for the comments Dale,

As far as cloning jobs goes I think we want the image update applied again.  The
use case as it was explained to me is that we want a particular test to run
immediately after an image update.  So a clone of that test should also cause an
image update.

I don't know the roll out plans now.  I don't know what tests need this feature.

Thanks,
paul

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py
File scheduler/scheduler_models.py (right):

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:871: # All image update jobs have the
parameterized_job_id set.
On 2011/01/11 00:46:05, dalec wrote:
> Nit, but I think comment should be on line above.

Done.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:875: sql_str = ('SELECT test_id '
On 2011/01/11 00:46:05, dalec wrote:
> Use """ syntax like in the rest of the file.
> 
> Also, normally you never want to inject values directly into your query string
> like this. With user-controlled values you could face SQL injection attacks.
> 
> The preferred method for populating values into the query string is to use
some
> marker character (? or %s in this case) and then pass the values in through
> parameters=(...) on the execute call. See get_host_queue_entries() above.
> 
> Using parameters=(...) also takes care of having to worry about escaping or
> quoting values. In your case if parameterized_job_id had spaces or any other
> funny characters, the query would break.

Cool, thanks for the info.  I've fixed that up.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:881: test_id = rows[0][0]
On 2011/01/11 00:46:05, dalec wrote:
> Is it possible for rows[n] to be None or something similar? Do other areas of
> code to any further checking?

In the testing I've done with regular and the new mucked up jobs the rows return
have always been either nothing or the tuple value,  I've never seen None
returned.  I haven't found good documentation on the execute method so I'm just
going off of the behavior I've seen.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:884: sql_str = ('SELECT id '
On 2011/01/11 00:46:05, dalec wrote:
> Same comment as above.

Done.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:890: update_id = rows[0][0]
On 2011/01/11 00:46:05, dalec wrote:
> Same comment as above.

Done.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:893: if test_id == update_id:
On 2011/01/11 00:46:05, dalec wrote:
> Will all parameterized jobs always be image jobs? Is there a safer test for
> this?

For now image update jobs are the only parameterized jobs.  Parameterized jobs
are not being enabled generally and cannot be run natively.  

When we want to support parameterized jobs fully this test will no longer be
valid as this strategy would be trying to create a test job and an image update
job at the same time as the same job.  When that happens we will need a more
Autotest like solution like adding a field to the database like reboot_before.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:895: sql_str = ('SELECT parameter_value '
On 2011/01/11 00:46:05, dalec wrote:
> Same comment as above.

Done.

http://codereview.chromium.org/6181003/diff/1/scheduler/scheduler_models.py#n...
scheduler/scheduler_models.py:901: # The image cannot be updated without an OS
path.
On 2011/01/11 00:46:05, dalec wrote:
> Consider reflowing w/ else: return False below. Drops some code and possibly
> more readable to say "if rows: blah blah; return True" Up to you.
> 

Good point.  Fixed.
> Doing without the else: means if anyone modifies the code in the future is
> harder for them to miss a case which returns no value or returns the wrong
> value.

[DaleCurtis, 2011-01-14 20:56:16]

Everything else is looking good to me. Once you get these fixes in and Mike has
had a chance to review we should be good to go.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/models.py
File frontend/afe/models.py (left):

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/models.py#oldco...
frontend/afe/models.py:996:
cls.check_parameterized_job(control_file=control_file,
You should comment these lines out instead of removing them, and add the comment
you put in this CL to them.

Also, is there a reason you can't just change the check_parameterized_job
function to always return true or do something similar? Then you have a single
change point instead of multiple ones and in the event a merge adds more points,
our functionality doesn't break.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/models.py#oldco...
frontend/afe/models.py:1035:
self.check_parameterized_job(control_file=self.control_file,
Same as above.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.py
File frontend/afe/rpc_interface.py (right):

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:522: @param image OS image to install before
running job
End in period.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:531: else:
No need for else here.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:537: # instantiate a Test object and from there a
parameterizedJob.
ParameterizedJob.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:543: # table afe_test_parameters.  We retrieve and
set this parameter to the
Extra space. Consider reflowing sentence, not entirely clear on which 'this' is
'this'

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:546:
known_parameterized_job.parameterizedjobparameter_set.create(
Is this correct? parameterizedjobparameter_set ? Doesn't seem to follow with
other naming conventions.

[pauldean_chromium, 2011-01-18 17:57:31]

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/models.py
File frontend/afe/models.py (left):

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/models.py#oldco...
frontend/afe/models.py:996:
cls.check_parameterized_job(control_file=control_file,
On 2011/01/14 20:56:16, dalec wrote:
> You should comment these lines out instead of removing them, and add the
comment
> you put in this CL to them.
> 
> Also, is there a reason you can't just change the check_parameterized_job
> function to always return true or do something similar? Then you have a single
> change point instead of multiple ones and in the event a merge adds more
points,
> our functionality doesn't break.

I didn't want to change the global gate keeper function in case it had a valid
use elsewhere in the code.  By just commenting out these two checks I contained
the scope of this hack to only the two places it matters for this --image
change.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.py
File frontend/afe/rpc_interface.py (right):

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:522: @param image OS image to install before
running job
On 2011/01/14 20:56:16, dalec wrote:
> End in period.

Done.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:531: else:
On 2011/01/14 20:56:16, dalec wrote:
> No need for else here.

Done.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:537: # instantiate a Test object and from there a
parameterizedJob.
On 2011/01/14 20:56:16, dalec wrote:
> ParameterizedJob.

Done.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:543: # table afe_test_parameters.  We retrieve and
set this parameter to the
On 2011/01/14 20:56:16, dalec wrote:
> Extra space. Consider reflowing sentence, not entirely clear on which 'this'
is
> 'this'

Done.

http://codereview.chromium.org/6181003/diff/9001/frontend/afe/rpc_interface.p...
frontend/afe/rpc_interface.py:546:
known_parameterized_job.parameterizedjobparameter_set.create(
On 2011/01/14 20:56:16, dalec wrote:
> Is this correct? parameterizedjobparameter_set ? Doesn't seem to follow with
> other naming conventions.

It is correct in that it works and is the way create_parameterized_job() does
it.  Look around line 470 to see this function being used.  When we come back
and fully implement parameterized jobs it probably makes sense to rename this to
fit in better with the rest of autotest.

[truty1, 2011-01-28 20:30:57]

This all looks fine except I have not spent time before understanding
parameterized jobs. The db code, to me, looks like it might get confused if more
than one image update is occurring at one time.  Can we have a discussion please
offline about how the tables reflect more than one update with different images
at the same time please?

[truty1, 2011-01-28 21:27:52]

lgtm - we just need to be sure that future upstream changes to parameterized
jobs don't break any of this.

[DaleCurtis, 2011-01-31 23:52:36]

LGTM

On 2011/01/28 21:27:52, truty1 wrote:
> lgtm - we just need to be sure that future upstream changes to parameterized
> jobs don't break any of this.

[truty1, 2011-02-01 01:21:31]

Something small to consider.

http://codereview.chromium.org/6181003/diff/14001/frontend/afe/models.py
File frontend/afe/models.py (right):

http://codereview.chromium.org/6181003/diff/14001/frontend/afe/models.py#newc...
frontend/afe/models.py:1048: #                            
parameterized_job=parameterized_job)
I'm going to point out something I noticed when patching.  You've commented out
a line that had 'self' and replaced it with a line that has 'cls'. I'm guessing
from the earlier update. Maybe this will be confusing for someone undoing this
at a later point.