Buildbot: Change --copy-crashdumps to do the actual archiving to CloudStorage.

Buildbot: Change --copy-crashdumps to do the actual archiving to CloudStorage.

This flag now both enables core dump collection by setting appropriate rlimits
and takes care of archiving cores and crashed binaries.

BUG=
R=kustermann@google.com

Committed: https://github.com/dart-lang/sdk/commit/dec352c1ac5d566c98f3fa31b5c63286403a6808

[Vyacheslav Egorov (Google), 2017-01-18 15:59:57]

vegorov@google.com changed reviewers:
+ kustermann@google.com, whesse@google.com

[Vyacheslav Egorov (Google), 2017-01-18 16:00:06]

Please take a look.

[kustermann, 2017-01-18 17:12:36]

LGTM

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:326: print("\nLooking for a core dump
${name}: ${exists ? '' : 'not'} found");
Maybe remove this line?

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:341: TestUtils.copyFile(new Path(binName),
new Path("core.${binBaseName}"));
Consider making binBaseName contain the configuration as well:

You can launch e.g. ./tools/test.py -mdebug,release --copy-coredumps

Then we have only core.dart  not core.out_releasex64_dart and
core.out_debugx64_dart

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode24
tools/utils.py:24: from glob import glob
Does this need to be in the try?

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode670
tools/utils.py:670: # directory and all binaries copied by
UnexpectedCrashDumpArchiver into
add `(from tools/testing/dart/test_progress.dart)` ?

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode748
tools/utils.py:748: tar.close()
Strictly speaking, we don't need .tar.gz we just need .gz!

It's only one file after all - but you can also leave it.

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode752
tools/utils.py:752: http_url = '%s%s' % (http_prefix, tarname.lstrip('/'))
How could [tarname] ever contain a '/' ?

[Vyacheslav Egorov (Google), 2017-01-18 19:57:00]

Thanks for the review.

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:326: print("\nLooking for a core dump
${name}: ${exists ? '' : 'not'} found");
On 2017/01/18 17:12:35, kustermann wrote:
> Maybe remove this line? 

Done.

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:341: TestUtils.copyFile(new Path(binName),
new Path("core.${binBaseName}"));
On 2017/01/18 17:12:35, kustermann wrote:
> Consider making binBaseName contain the configuration as well:
> 
> You can launch e.g. ./tools/test.py -mdebug,release --copy-coredumps
> 
> Then we have only core.dart  not core.out_releasex64_dart and
> core.out_debugx64_dart
> 

Done.

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode24
tools/utils.py:24: from glob import glob
On 2017/01/18 17:12:36, kustermann wrote:
> Does this need to be in the try?

Done.

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode670
tools/utils.py:670: # directory and all binaries copied by
UnexpectedCrashDumpArchiver into
On 2017/01/18 17:12:35, kustermann wrote:
> add `(from tools/testing/dart/test_progress.dart)` ? 

Done.

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode748
tools/utils.py:748: tar.close()
On 2017/01/18 17:12:35, kustermann wrote:
> Strictly speaking, we don't need .tar.gz we just need .gz!
> 
> It's only one file after all - but you can also leave it.

Acknowledged.

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode752
tools/utils.py:752: http_url = '%s%s' % (http_prefix, tarname.lstrip('/'))
On 2017/01/18 17:12:35, kustermann wrote:
> How could [tarname] ever contain a '/' ?

Now it can't - it used to use an absolute path.

[Vyacheslav Egorov (Google), 2017-01-18 19:58:06]

We are waiting for access to hardware slaves to update core_pattern on them.

We need to also need to ensure they have access to cloud storage.

[Vyacheslav Egorov (Google), 2017-01-19 15:13:32]

Description was changed from

==========
Buildbot: Change --copy-crashdumps to do the actual archiving to CloudStorage.

This flag now both enables core dump collection by setting appropriate rlimits
and takes care of archiving cores and crashed binaries.

BUG=
==========

to

==========
Buildbot: Change --copy-crashdumps to do the actual archiving to CloudStorage.

This flag now both enables core dump collection by setting appropriate rlimits
and takes care of archiving cores and crashed binaries.

BUG=
R=kustermann@google.com

Committed:
https://github.com/dart-lang/sdk/commit/dec352c1ac5d566c98f3fa31b5c63286403a6808
==========

[Vyacheslav Egorov (Google), 2017-01-19 15:13:33]

Committed patchset #2 (id:20001) manually as
dec352c1ac5d566c98f3fa31b5c63286403a6808 (presubmit successful).

[Bill Hesse, 2017-01-24 16:01:21]

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
File tools/testing/dart/test_progress.dart (right):

https://codereview.chromium.org/2644593002/diff/1/tools/testing/dart/test_pro...
tools/testing/dart/test_progress.dart:341: TestUtils.copyFile(new Path(binName),
new Path("core.${binBaseName}"));
On 2017/01/18 17:12:35, kustermann wrote:
> Consider making binBaseName contain the configuration as well:
> 
> You can launch e.g. ./tools/test.py -mdebug,release --copy-coredumps
> 
> Then we have only core.dart  not core.out_releasex64_dart and
> core.out_debugx64_dart
> 

On buildbots, we often test modes separately, but we do sometimes combine ia32
and x64 on non-vm bots.

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode710
tools/utils.py:710: archive_coredumps = coredumps[:10]
Do we want to make sure the binaries are in archive_coredumps, by scanning the
rest of coredumps for them and adding them?

[Vyacheslav Egorov (Google), 2017-01-24 16:12:13]

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py
File tools/utils.py (right):

https://codereview.chromium.org/2644593002/diff/1/tools/utils.py#newcode710
tools/utils.py:710: archive_coredumps = coredumps[:10]
On 2017/01/24 16:01:21, Bill Hesse wrote:
> Do we want to make sure the binaries are in archive_coredumps, by scanning the
> rest of coredumps for them and adding them?

Nice catch Bill!

Though we changed the way this code is organized in a later CL - now we
coredumps are tied to binaries. So the problem has gone away.