Add support for saving VM state after test failure

Add support for saving VM state after test failure

BUG=chromium-os:9577
TEST=Run suite_Smoke with failure and without failure, load VM state to
ensure correctness

Change-Id: I342fd7077f35fa908f0d4e90f7c27e83f2992b07
Change-Id: I385a9614ed6ce953f33ad1830e0f566451903dd7

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=1904d00

[thieule, 2010-11-24 00:55:08]

This is the test CL associated with the save VM support.

PTAL

[sosa, 2010-11-24 01:02:54]

You may want to include @petkov or @kmixter for these changes.  Also not sure
about the checkpoint format ... if we use something from the lsb-release it may
be more helpful to us ... however, using the timestamps is more generic and we
could upstream.

http://codereview.chromium.org/5303005/diff/1/client/common_lib/test.py
File client/common_lib/test.py (right):

http://codereview.chromium.org/5303005/diff/1/client/common_lib/test.py#newco...
client/common_lib/test.py:426: checkpoint_name = "%s-%s" %
(self.tagged_testname, now)
Don't mix "" and ''

http://codereview.chromium.org/5303005/diff/1/client/common_lib/test.py#newco...
client/common_lib/test.py:426: checkpoint_name = "%s-%s" %
(self.tagged_testname, now)
It may be more useful to get something about the machine ... lsb-release entry
maybe like the version of chromeos?

[thieule, 2010-11-24 01:09:13]

+petkov
+kmixter

Can you guys PTAL?

In a previous CL (http://codereview.chromium.org/5174009/), I've added the
ability to redirect the VM guest USB serial port to the QEMU monitor.  This
enables a guest to trigger a "savevm" command.  I now want to modify the test so
that it will execute this command when a test fails.

Thanks,
Thieu

[ericli, 2010-11-24 01:18:19]

Please add the new function to client/bin/site_utils.py and keep the vanilla
state of client/common_lib/utils.py.

Thanks.

On 2010/11/24 01:09:13, thieule wrote:
> +petkov
> +kmixter
> 
> Can you guys PTAL?
> 
> In a previous CL (http://codereview.chromium.org/5174009/), I've added the
> ability to redirect the VM guest USB serial port to the QEMU monitor.  This
> enables a guest to trigger a "savevm" command.  I now want to modify the test
so
> that it will execute this command when a test fails.
> 
> Thanks,
> Thieu

[thieule, 2010-11-24 01:28:21]

I've modified client/common_lib/test.py to invoke save_vm_state() which is
currently defined in client/common_lib/utils.py.  It seems strange for a
function defined in client/common_lib/test.py to invoke another function in
client/bin/site_utils.py.  Is there a particular reason for this?

On Tue, Nov 23, 2010 at 5:18 PM, <ericli@chromium.org> wrote:

> Please add the new function to client/bin/site_utils.py and keep the
> vanilla
> state of client/common_lib/utils.py.
>
> Thanks.
>
>
> On 2010/11/24 01:09:13, thieule wrote:
>
>> +petkov
>> +kmixter
>>
>
>  Can you guys PTAL?
>>
>
>  In a previous CL (http://codereview.chromium.org/5174009/), I've added
>> the
>> ability to redirect the VM guest USB serial port to the QEMU monitor.
>>  This
>> enables a guest to trigger a "savevm" command.  I now want to modify the
>> test
>>
> so
>
>> that it will execute this command when a test fails.
>>
>
>  Thanks,
>> Thieu
>>
>
>
>
> http://codereview.chromium.org/5303005/
>

[ericli, 2010-11-24 02:30:58]

There are two problems.

1. In high level, there is the base client/common_lib, and two derived
packages, client/bin and server. Anything runs on client should goto
client/bin, anything runs on server should go to server, and if it could be
called on both client/server, put it in client/common_lib.

But in the early days of autotest, there is no server package at all. Only
common_lib and bin. So lots of client logic was implemented inside
common_lib. Not perfect, but it reflects its organic growth.

So some times changes to client bin package need to be made to common_lib
package, just like your change to common_lib/test.py.  I looked the code and
I would not suggest to move that change into client/bin/test.py, even in
theory that is more appropriate, simply because lots of client test logic
was implemented inside common_lib/test.py.

And also that is the same reason I would suggest to move the definition of
save_vm_state() from common_lib into bin, since it is a client function
call.

2. Autotest provide a nice framework for project like chromeos to extends
its functionalities without bother with upstream changes. Most of the
classes defined from autotest will support a site_* extension, and it will
be automatically loaded by autotest. In your case, after save_vm_state() is
defined inside site_utils.py, it will still be available through
utils.save_vm_state().

The benefit of following this design is to avoid any diverge of source code
from upstream, thus to avoid any painful merge from autotest upstream.


On Tue, Nov 23, 2010 at 5:28 PM, Thieu Le <thieule@chromium.org> wrote:

> I've modified client/common_lib/test.py to invoke save_vm_state() which is
> currently defined in client/common_lib/utils.py.  It seems strange for a
> function defined in client/common_lib/test.py to invoke another function in
> client/bin/site_utils.py.  Is there a particular reason for this?
>
> On Tue, Nov 23, 2010 at 5:18 PM, <ericli@chromium.org> wrote:
>
>> Please add the new function to client/bin/site_utils.py and keep the
>> vanilla
>> state of client/common_lib/utils.py.
>>
>> Thanks.
>>
>>
>> On 2010/11/24 01:09:13, thieule wrote:
>>
>>> +petkov
>>> +kmixter
>>>
>>
>>  Can you guys PTAL?
>>>
>>
>>  In a previous CL (http://codereview.chromium.org/5174009/), I've added
>>> the
>>> ability to redirect the VM guest USB serial port to the QEMU monitor.
>>>  This
>>> enables a guest to trigger a "savevm" command.  I now want to modify the
>>> test
>>>
>> so
>>
>>> that it will execute this command when a test fails.
>>>
>>
>>  Thanks,
>>> Thieu
>>>
>>
>>
>>
>> http://codereview.chromium.org/5303005/
>>
>
>


-- 
Eric Li
李咏竹
Google Kirkland

[thieule, 2010-11-24 18:00:45]

Fix code review feedbacks.

PTAL

http://codereview.chromium.org/5303005/diff/1/client/common_lib/test.py
File client/common_lib/test.py (right):

http://codereview.chromium.org/5303005/diff/1/client/common_lib/test.py#newco...
client/common_lib/test.py:426: checkpoint_name = "%s-%s" %
(self.tagged_testname, now)
On 2010/11/24 01:02:54, sosa wrote:
> Don't mix "" and ''

Done.

http://codereview.chromium.org/5303005/diff/1/client/common_lib/test.py#newco...
client/common_lib/test.py:426: checkpoint_name = "%s-%s" %
(self.tagged_testname, now)
On 2010/11/24 01:02:54, sosa wrote:
> It may be more useful to get something about the machine ... lsb-release entry
> maybe like the version of chromeos?

The qcow2 file is tied to a particular base image.  The qcow2 file and
chromiumos_qemu_image.bin will be uploaded to a common Google Storage folder
that will contain the build information.  I was using the checkpoint name as a
way to identify the various VM states after a test failure.  The timestamp in
the checkpoint name can be used to correlate with the logs.  Also, some tests
are executed more than once in a suite and I needed a way to uniquely identify
each instance.

[petkov, 2010-11-24 20:36:53]

a couple of nits.

eric -- i assume we won't be able to upstream the test.py change, right? is
there a better place for this call?

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py
File client/bin/site_utils.py (right):

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py#new...
client/bin/site_utils.py:5: import os, logging, platform, time
sort

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py#new...
client/bin/site_utils.py:44: """Saves the current state of the virtual machine
period

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py#new...
client/bin/site_utils.py:59: if proc.find('QEMU') >= 0 and
os.path.exists('/dev/ttyUSB0'):
'QEMU' in proc?

http://codereview.chromium.org/5303005/diff/9001/client/common_lib/test.py
File client/common_lib/test.py (right):

http://codereview.chromium.org/5303005/diff/9001/client/common_lib/test.py#ne...
client/common_lib/test.py:22: from datetime import datetime
sort

[thieule, 2010-11-30 23:05:08]

Moved save VM state code to be site specific.
Addressed more code review feedbacks.

PTAL

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py
File client/bin/site_utils.py (right):

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py#new...
client/bin/site_utils.py:5: import os, logging, platform, time
On 2010/11/24 20:36:53, petkov wrote:
> sort

Done.

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py#new...
client/bin/site_utils.py:44: """Saves the current state of the virtual machine
On 2010/11/24 20:36:53, petkov wrote:
> period

Done.

http://codereview.chromium.org/5303005/diff/9001/client/bin/site_utils.py#new...
client/bin/site_utils.py:59: if proc.find('QEMU') >= 0 and
os.path.exists('/dev/ttyUSB0'):
On 2010/11/24 20:36:53, petkov wrote:
> 'QEMU' in proc?

I was a bit surprised myself as well to find python platform.processor()
returns:

'QEMU Virtual CPU version 0.12.3'

http://codereview.chromium.org/5303005/diff/9001/client/common_lib/test.py
File client/common_lib/test.py (right):

http://codereview.chromium.org/5303005/diff/9001/client/common_lib/test.py#ne...
client/common_lib/test.py:22: from datetime import datetime
On 2010/11/24 20:36:53, petkov wrote:
> sort

Done.

[Chris Masone, 2010-12-01 00:09:51]

LGTM

slick little change.

[petkov, 2010-12-01 00:12:43]

LGTM

http://codereview.chromium.org/5303005/diff/21001/client/bin/site_utils.py
File client/bin/site_utils.py (right):

http://codereview.chromium.org/5303005/diff/21001/client/bin/site_utils.py#ne...
client/bin/site_utils.py:59: if proc.find('QEMU') >= 0 and
os.path.exists('/dev/ttyUSB0'):
I think

if 'QEMU' in proc and...

is the preferred syntax by some style guidelines...