1245566 - Enable taskcluster scheduling for valgrind-mochitest runs

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Julian Seward [:jseward]]

Bug 1229348 adds target |mochitest-valgrind| to |./mach mozharness|.

This bug builds on that, adding taskcluster scheduling for such runs.

Comment 1

[Julian Seward [:jseward]]

Attachment: WIP patch

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1245566-4-001_basic_tc_scheduling.cset

Enables Valgrind Mochitest runs on Taskcluster, manual request only for now,
using the try syntax "-b o -p linux64_tc -u mochitest-valgrind -t none"

Comment 3

[Julian Seward [:jseward]]

Attachment: bug1245566-5-002_mochitest_valgrind_mods.cset

Changes needed to the Valgrind/Mochitest infrastructure needed to make
Taskcluster runs feasible:

mozbase/mozlog/mozlog/formatters/tbplformatter.py:
mozharness/mozharness/mozilla/testing/errors.py:
  When converting a structured valgrind_error log message into text, use
  the prefix TEST-UNEXPECTED-VALGRIND-ERROR rather than
  TEST-VALGRIND-ERROR so that treeherder/log_parser/parsers.py actually
  recognises the text as an error.
  Update mozharness/mozharness/mozilla/testing/errors.py to match.

mozharness/configs/unittests/linux_unittest.py:
  Increase mochitest timeout for Valgrind runs to 900 seconds,
  with up to 50 timeouts allowed.

mozbase/mozdebug/mozdebug/mozdebug.py:
  Rationalise get_default_valgrind_args():
  - move tool specific leak-check args into the tool-specific function
  - create a new tool-specific function get_default_valgrind_none_specific_args()
    to make it easy to do debugging runs with --tool=none.  Not used
    in production.

Comment 4

[James Graham [:jgraham]]

Comment on attachment 8719455 [details] [diff] [review]
bug1245566-5-002_mochitest_valgrind_mods.cset

Review of attachment 8719455 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/mozbase/mozdebug/mozdebug/mozdebug.py
@@ +246,3 @@
>  def get_default_valgrind_tool_specific_args():
> +    return get_default_valgrind_memcheck_specific_args()
> +    # return get_default_valgrind_none_specific_args()

I'm not a big fan of checking in comment-out code. Isn't there a way that we can make the tool a command line option defaulting to memcheck, and then have this function dispatch to get the right args based on that?

Comment 5

[Julian Seward [:jseward]]

(In reply to James Graham [:jgraham] from comment #4)
> I'm not a big fan of checking in comment-out code.

Yeah, I was in two minds myself about that.  I can simply remove the
support for the 'none' tool and make get_default_valgrind_tool_specific_args()
directly return the arguments for memcheck.  Ie pretty much as it was
originally, except that the tool specific args --leak-check=full and
--show-possibly-lost=no have been moved into the tool-specific function,
where they should have been to begin with.  Would that be OK ?

Comment 6

[James Graham [:jgraham]]

Sure.

Comment 7

[Julian Seward [:jseward]]

Attachment: bug1245566-6-002_mochitest_valgrind_mods.cset

Addresses review comments in comment 4 and comment 5.

Comment 8

[Armen [:armenzg]]

Comment on attachment 8719445 [details] [diff] [review]
bug1245566-4-001_basic_tc_scheduling.cset

Review of attachment 8719445 [details] [diff] [review]:
-----------------------------------------------------------------

Feel free to land after

::: testing/taskcluster/tasks/branches/base_job_flags.yml
@@ +102,5 @@
>      - android-api-15
>      - android-partner-sample1
>      - android-b2gdroid
>      - linux
> +    - linux64_tc

We can't land this piece.
This is to allow me to use '-p linux64_tc' on my try syntax (hence avoiding to schedule Buildbot linux64 jobs).

::: testing/taskcluster/tasks/branches/try/job_flags.yml
@@ +60,5 @@
>        opt:
>          task: tasks/builds/opt_linux32_clobber.yml
>        debug:
>          task: tasks/builds/dbg_linux32_clobber.yml
> +  linux64_tc:

Remove this as well.

@@ +222,5 @@
>          task: tasks/tests/fx_linux64_mochitest_gl.yml
> +  mochitest-valgrind:
> +    allowed_build_tasks:
> +      tasks/builds/opt_linux64_clobber.yml:
> +        task: tasks/tests/fx_linux64_mochitest_vg.yml

If you land this piece, it means that every time that someone pushes to try, it will be scheduled for them.

You can keep this piece out if you don't want that to happen.
In other words, '-u all' would trigger all the valgrind tests.

Instead, what you can do is to keep your patch around for your pushes until you get everything green.

::: testing/taskcluster/tasks/tests/fx_linux64_mochitest_vg.yml
@@ +23,5 @@
> +      MOZHARNESS_CONFIG: >
> +        mozharness/configs/unittests/linux_unittest.py
> +        mozharness/configs/remove_executables.py
> +  metadata:
> +    name: '[TC] Linux64 mochitest-valgrind'

Could you please add -{{chunk}}?

I'm building a tool and I will be enforcing such format for chunked jobs.

Comment 9

[Julian Seward [:jseward]]

Attachment: bug1245566-7-001_basic_tc_scheduling-LANDABLE.cset

Addresses review comments in comment 8.

Armen, thank you for the review.  I think I made the right changes
but I am not entirely confident about that.  Here's a new version.
Could you please give it a check and see if it is what you meant?

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/292b51653b91
https://hg.mozilla.org/integration/mozilla-inbound/rev/156d0a87f596

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/292b51653b91
https://hg.mozilla.org/mozilla-central/rev/156d0a87f596