Modify autotest to support preserving minidumps generated during tests.

Modify autotest to support preserving minidumps generated during tests.

Crash information is retained for all tests that are not testing the
crash facilities themselves.  Crash tests touch
/tmp/crash-test-in-progress to indicate that crash facilities are being
tested and minidumps should not be retained all the time.  At the end of
each job, autotest enumerates the test results directory and creates
stack traces for all minidumps.

BUG=chromium-os:12207
TEST=logging_UserCrash, logging_CrashSender, manual test which involves
purposely crashing a test to determine if minidump information is
properly collected

Change-Id: Iad4180ed6aba2ef434aa29f2aac587c8d5d4dfcc

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

[thieule, 2011-02-23 01:27:30]

This CL is the first of two CLs that add support to preserve crash information
(minidumps) when tests fail, except crash related tests.  Crash related tests
work as before.

This CL applies to autotest while the second CL applies to crash-reporter.

PTAL

[kmixter1, 2011-02-23 02:00:39]

Nice! A few comments/questions below.  You should test by running bvts (since
these are server side tests).  I also often test usercrash and crashsender
multiple times in a row (like 50) because the PFQ is quite good at finding
unanticipated race conditions.  Not necessary, but be prepared.

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

http://codereview.chromium.org/6557003/diff/1/client/bin/test.py#newcode110
client/bin/test.py:110: def _call_run_once(self, constraints, profile_only,
Eric should comment on this since it's common code and he's edited it more than
I have.

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py
File client/cros/crash_test.py (right):

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py#newco...
client/cros/crash_test.py:20: _CRASH_TEST_IN_PROGRESS =
'/tmp/crash-test-in-progress'
This is similar in use to _MOCK_CRASH_SENDING.  I can believe you can't reuse
it, but just want to make sure you've verified it.

http://codereview.chromium.org/6557003/diff/1/server/server_job.py
File server/server_job.py (right):

http://codereview.chromium.org/6557003/diff/1/server/server_job.py#newcode1100
server/server_job.py:1100: # by default provide a stub that generates no site
data
was this just moved?  Were you just moving it to get it closer to point of use?

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py
File server/site_server_job.py (right):

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:12: class site_server_job(base_server_job):
Will this be used by all server tests automatically?

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:15: symbol_dir = '%s/../../../lib/debug' %
utils.get_server_dir()
I guess this only works for tests autotests run within build roots which does
cover a lot of important cases.  May be worth commenting where this expects
debug files to be in case we want to make sure the automatically run tests
(those by cautotest) also can generate minidumps.

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:29: if (rc == 0):
extra parens?

[ericli, 2011-02-23 04:49:34]

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

http://codereview.chromium.org/6557003/diff/1/client/bin/test.py#newcode123
client/bin/test.py:123: for hook in self.after_iteration_hooks:
common_test.base_test._call_run_once() will also call
self.after_iteration_hooks.  I would suggest you to directly patch into
ommon_test.base_test._call_run_once()  and try to upstream it.

I dont know if they will accept it or not but we at least should try.

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py
File client/cros/crash_test.py (right):

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py#newco...
client/cros/crash_test.py:20: _CRASH_TEST_IN_PROGRESS =
'/tmp/crash-test-in-progress'
any reason why not in constants.py?
On 2011/02/23 02:00:39, kmixter1 wrote:
> This is similar in use to _MOCK_CRASH_SENDING.  I can believe you can't reuse
> it, but just want to make sure you've verified it.

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py
File server/site_server_job.py (right):

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:53: self._find_and_generate_minidump_stacktraces()
There is already a try/finally block inside base_server_job.run() method.

Seems like you could:
1. Add _find_and_generate_minidump_stactraces() function to server/utils.py
2. Tnen make the call from CRASHDUMPS_CONTROL_FILE/CRASHINFO_CONTROL_FILE.

Then you could avoid this site_server_job file.

Take a  read server/crashcollec.py, it should be a better place to host these
functions.

[thieule, 2011-03-03 01:27:41]

The autotest changes were upstreamed, thanks Eric.

I've moved the stack trace generation to site_crashcollector.py.

PTAL

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

http://codereview.chromium.org/6557003/diff/1/client/bin/test.py#newcode123
client/bin/test.py:123: for hook in self.after_iteration_hooks:
On 2011/02/23 04:49:34, ericli wrote:
> common_test.base_test._call_run_once() will also call
> self.after_iteration_hooks.  I would suggest you to directly patch into
> ommon_test.base_test._call_run_once()  and try to upstream it.
> 
> I dont know if they will accept it or not but we at least should try.

Done.

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py
File client/cros/crash_test.py (right):

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py#newco...
client/cros/crash_test.py:20: _CRASH_TEST_IN_PROGRESS =
'/tmp/crash-test-in-progress'
On 2011/02/23 02:00:39, kmixter1 wrote:
> This is similar in use to _MOCK_CRASH_SENDING.  I can believe you can't reuse
> it, but just want to make sure you've verified it.

_MOCK_CRASH_SENDING is set after the crash but before sending the info.  This is
unfortunately too late since I need to know if we're running crash tests while
handling the crash in UserCollector::HandleCrash().

http://codereview.chromium.org/6557003/diff/1/client/cros/crash_test.py#newco...
client/cros/crash_test.py:20: _CRASH_TEST_IN_PROGRESS =
'/tmp/crash-test-in-progress'
On 2011/02/23 04:49:34, ericli wrote:
> any reason why not in constants.py?

I'm going to leave this constant in this file to be consistent with the rest of
the constants here.  We can have a separate CL that cleans up all these private
constants.

http://codereview.chromium.org/6557003/diff/1/server/server_job.py
File server/server_job.py (right):

http://codereview.chromium.org/6557003/diff/1/server/server_job.py#newcode1100
server/server_job.py:1100: # by default provide a stub that generates no site
data
On 2011/02/23 02:00:39, kmixter1 wrote:
> was this just moved?  Were you just moving it to get it closer to point of
use?

This block needed to be moved down because it imports site_server_job. 
site_server_job.py relies on base_server_job being declared.  With this block
being higher up in the file, base_server_job has not been declared yet.

Anyways, no need for this anymore since we're using site_crashcollector instead
of site_server_job.

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py
File server/site_server_job.py (right):

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:12: class site_server_job(base_server_job):
On 2011/02/23 02:00:39, kmixter1 wrote:
> Will this be used by all server tests automatically?

Yes.

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:15: symbol_dir = '%s/../../../lib/debug' %
utils.get_server_dir()
On 2011/02/23 02:00:39, kmixter1 wrote:
> I guess this only works for tests autotests run within build roots which does
> cover a lot of important cases.  May be worth commenting where this expects
> debug files to be in case we want to make sure the automatically run tests
> (those by cautotest) also can generate minidumps.

Done.

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:29: if (rc == 0):
On 2011/02/23 02:00:39, kmixter1 wrote:
> extra parens?

Done.

http://codereview.chromium.org/6557003/diff/1/server/site_server_job.py#newco...
server/site_server_job.py:53: self._find_and_generate_minidump_stacktraces()
On 2011/02/23 04:49:34, ericli wrote:
> There is already a try/finally block inside base_server_job.run() method.
> 
> Seems like you could:
> 1. Add _find_and_generate_minidump_stactraces() function to server/utils.py
> 2. Tnen make the call from CRASHDUMPS_CONTROL_FILE/CRASHINFO_CONTROL_FILE.
> 
> Then you could avoid this site_server_job file.
> 
> Take a  read server/crashcollec.py, it should be a better place to host these
> functions.

Done.

I've moved the minidump generation to site_crashcollector.py.

[ericli, 2011-03-03 01:35:42]

LGTM.

This is a much cleaner logic.
Ken should comment on the minidumps logic, not me.

[kmixter1, 2011-03-03 01:49:54]

http://codereview.chromium.org/6557003/diff/7001/server/site_crashcollect.py
File server/site_crashcollect.py (right):

http://codereview.chromium.org/6557003/diff/7001/server/site_crashcollect.py#...
server/site_crashcollect.py:1: # Copyright (c) 2011 The Chromium OS Authors. All
rights reserved.
Can we name this differently? "crash_collector" is a term in crash-reporter
codebase that collects the basic crash dump data from the kernel.  It's not the
most descriptive either, admittedly. Maybe site_stacktrace_generator or
site_crash_stacktrace?

[thieule, 2011-03-03 02:22:45]

http://codereview.chromium.org/6557003/diff/7001/server/site_crashcollect.py
File server/site_crashcollect.py (right):

http://codereview.chromium.org/6557003/diff/7001/server/site_crashcollect.py#...
server/site_crashcollect.py:1: # Copyright (c) 2011 The Chromium OS Authors. All
rights reserved.
On 2011/03/03 01:49:54, kmixter1 wrote:
> Can we name this differently? "crash_collector" is a term in crash-reporter
> codebase that collects the basic crash dump data from the kernel.  It's not
the
> most descriptive either, admittedly. Maybe site_stacktrace_generator or
> site_crash_stacktrace?

Unfortunately, the name is dictated by autotest in crashcollect.py. 
site_crashcollect.py is meant as a site specific extension to crashcollect.py. 
If you want, we can create another file called site_crash_stacktrace.py, put the
new code in there, and then call find_and_generate_minidump_stacktraces from
get_site_crashdumps.  It'll just be an indirection.  I don't think we should
change the site_crashcollect name in autotest since existing tests (outside of
Google) may depend on this and I'm not sure how well that will upstream.

[kmixter1, 2011-03-03 02:35:33]

LGTM as is.

On Wed, Mar 2, 2011 at 6:22 PM, <thieule@chromium.org> wrote:

>
>
> http://codereview.chromium.org/6557003/diff/7001/server/site_crashcollect.py
> File server/site_crashcollect.py (right):
>
>
>
http://codereview.chromium.org/6557003/diff/7001/server/site_crashcollect.py#...
> server/site_crashcollect.py:1: # Copyright (c) 2011 The Chromium OS
> Authors. All rights reserved.
> On 2011/03/03 01:49:54, kmixter1 wrote:
>
>> Can we name this differently? "crash_collector" is a term in
>>
> crash-reporter
>
>> codebase that collects the basic crash dump data from the kernel.
>>
> It's not the
>
>> most descriptive either, admittedly. Maybe site_stacktrace_generator
>>
> or
>
>> site_crash_stacktrace?
>>
>
> Unfortunately, the name is dictated by autotest in crashcollect.py.
> site_crashcollect.py is meant as a site specific extension to
> crashcollect.py.  If you want, we can create another file called
> site_crash_stacktrace.py, put the new code in there, and then call
> find_and_generate_minidump_stacktraces from get_site_crashdumps.  It'll
> just be an indirection.  I don't think we should change the
> site_crashcollect name in autotest since existing tests (outside of
> Google) may depend on this and I'm not sure how well that will upstream.
>
>
> http://codereview.chromium.org/6557003/
>