1074147 - For Win32 get a set of plain mochitests running on holly with the content sandbox enabled

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Bob Owen (:bobowen) PTO until 15th Apr]

Apart from some media tests, all plain mochitests that pass with e10s on Windows also pass with the current settings for the content process sandbox.

The media tests will be conditionally disabled by bug 1067305, so it would then be good to get these running on holly.

Comment 1

[Bob Owen (:bobowen) PTO until 15th Apr]

Attachment: Part 1: Add option for --content-sandbox to desktop_unittest.py to be passed onto mochitest suites v1

Comment 2

[Bob Owen (:bobowen) PTO until 15th Apr]

Attachment: Part 2: On holly add mochitest plain tests with the content sandbox for win32 v1

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Between this and mochtest-e10s, I'm really concerned about what affect these recent additions are going to have on non-AWS backlog once they're enabled in production. We're talking about an adding an extra 10+ mochitest runs per commit? Are we prepared for that?

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8496814 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v1

Any reason we can't be a bit less verbose and just call these mochitest-csb?

Comment 5

[Ryan VanderMeulen [:RyanVM]]

catlee's on PTO. 302 laura :)

Comment 6

[Chris AtLee [:catlee]]

is this just on holly right now? what is the plan after that?

Comment 7

[Bob Owen (:bobowen) PTO until 15th Apr]

Attachment: Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #4)
> Comment on attachment 8496814 [details] [diff] [review]
> Part 2: On holly add mochitest plain tests with the content sandbox for
> win32 v1
> 
> Any reason we can't be a bit less verbose and just call these mochitest-csb?

Thanks again for your help on this.
As I mentioned on IRC, while doing the tbpl changes, I'd realised it was a bit long and changed it to exactly this. :)

After a bit of wrestling, I managed to get Armen's script running, I'll upload the output.

I'm going to try and use the instructions for running with Mozharness locally to test the desktop_unittest.py changes and then I'll look for reviews.

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #3)
> Between this and mochtest-e10s, I'm really concerned about what affect these
> recent additions are going to have on non-AWS backlog once they're enabled
> in production. We're talking about an adding an extra 10+ mochitest runs per
> commit? Are we prepared for that?

We discussed this on IRC, but just to summarise here:
We want to get to the point where the sandbox is enabled whenever e10s is, but there's a fair amount of work before we can do that.

As we've put some effort into getting nearly all the plain mochitests (that run for e10s) working on Win7 & 8, I wanted to get these running somewhere, to make it easier to catch regressions.
Holly seemed like the obvious candidate, as it's running the e10s stuff.

Given your justified concerns, I'll make sure that if there are discussions over running these anywhere else, that people appreciate that this is not something we would be able to do without steps to make sure the infrastructure can cope.

On a related note, we are going to be looking into turning on a less secure version of the sandbox that (hopefully) doesn't break anything. This will be tied to e10s, so shouldn't have any extra testing backlog implications.

Comment 8

[Bob Owen (:bobowen) PTO until 15th Apr]

Attachment: Results of running list_builder_differences.sh over Part 2.

Here are the results from Armen's script.

(In reply to Chris AtLee [:catlee] from comment #6)
> is this just on holly right now? what is the plan after that?

Sorry missed this just before I posted.
The short answer from comment 7:
Yes, and to get to a point where we can turn the sandbox on whenever e10s is on.

Comment 9

[Bob Owen (:bobowen) PTO until 15th Apr]

Comment on attachment 8496813 [details] [diff] [review]
Part 1: Add option for --content-sandbox to desktop_unittest.py to be passed onto mochitest suites v1

I've tested this using the instructions on:
https://wiki.mozilla.org/ReleaseEngineering/Mozharness/How_to_run_tests_as_a_developer

I've tested with (all 3) and without the --content-sandbox option specified and confirmed that the process is sandboxed when 'warn' or 'on' are passed.

I also tried passing the option with a reftest-suite to make sure I got the warning and that the option wasn't appending the the test run command.

On a related note I also tried using the --run-tests option that you mentioned on IRC.

At first it was complaining that there were no mochitest_options in the in-tree config, this was because the in-tree config is loaded in the download_and_extract step, which hasn't been run.

So, I added the following to the start of preflight_run_tests (in testbase.py):
        # If the in tree config hasn't been loaded by a previous step, load it here.
        if len(self.tree_config) == 0:
            self._read_tree_config()

this seemed to do the trick.
I also had to specify a --binary-path, because this is set up by the install step and without this it was just looking in the build directory.

You still have to pass --installer-url and --test-url (it moans if you don't with developer_config.py specified).
The installer one seems to be used to get to the other downloaded files.
The test one doesn't seem to be used, because I could pass anything.

Do you want me to create a bug and upload my patch to testbase.py for the in-tree config load?

Comment 10

[Bob Owen (:bobowen) PTO until 15th Apr]

Comment on attachment 8497530 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

I've added an attachment with the results of list_builder_differences.sh for this patch.
The Windows XP ones don't pass yet, but I need to look at that next.

Bill - we talked about this briefly on IRC, but just asking for feedback to confirm that you are OK with my adding these test runs to holly.
Basically this adds content sandbox (M-csb) tests for win32 plain mochitests, similar to the M-e10s ones.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8497530 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

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

As long as these are separate from the normal e10s tests, I have no problem with it. Hopefully we can run everything together pretty soon, which will address the concerns about infrastructure load.

Comment 12

[Armen [:armenzg]]

Comment on attachment 8496813 [details] [diff] [review]
Part 1: Add option for --content-sandbox to desktop_unittest.py to be passed onto mochitest suites v1

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

Feel free to land this on the default branch of mozharness.
It will be live once Buildduty merges the changes to production.

::: scripts/desktop_unittest.py
@@ +287,5 @@
> +            if c.get('content_sandbox') in ["warn", "on"]:
> +                if suite_category == "mochitest":
> +                    base_cmd.append('--content-sandbox=' + c['content_sandbox'])
> +                else:
> +                    self.warning("--content-sandbox does not currently work with suites other than mochitest.")

I would turn this to self.fatal() too abort the execution of the script. Works for you?

Comment 13

[Armen [:armenzg]]

(In reply to Bob Owen (:bobowen) from comment #9)
> So, I added the following to the start of preflight_run_tests (in
> testbase.py):
>         # If the in tree config hasn't been loaded by a previous step, load
> it here.
>         if len(self.tree_config) == 0:
>             self._read_tree_config()
> 
You're right. As developers start trying mozharness we will discover all those places where we have bugs.
I have found some of these while working on mulet reftests.

Filed as bug 1075992.

Comment 14

[Armen [:armenzg]]

Comment on attachment 8497530 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

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

This patch looks good. I also run test-masters.sh to verify that it does not produce errors for any masters.

Comment 15

[Bob Owen (:bobowen) PTO until 15th Apr]

Attachment: Part 1: Add option for --content-sandbox to desktop_unittest.py to be passed onto mochitest suites v2

r=armenzg - from comment 12

(In reply to Armen Zambrano - Automation & Tools Engineer (:armenzg) from comment #12)

> > +                    self.warning("--content-sandbox does not currently work with suites other than mochitest.")
> 
> I would turn this to self.fatal() too abort the execution of the script.
> Works for you?

I think you're right, I've changed to fatal, thanks.

Comment 16

[Bob Owen (:bobowen) PTO until 15th Apr]

Please land Part 1 on build/mozharness default branch, thanks.

I'm going to wait until this is merged to production before landing Part 2, just in case they got merged out of order.

Comment 17

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8498751 [details] [diff] [review]
Part 1: Add option for --content-sandbox to desktop_unittest.py to be passed onto mochitest suites v2

I can confirm this patch is the same as the previous one, except for:

48c48
< +                    self.warning("--content-sandbox does not currently work with suites other than mochitest.")
---
> +                    self.fatal("--content-sandbox does not currently work with suites other than mochitest.")

Therefore r+'ing

Comment 18

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8498751 [details] [diff] [review]
Part 1: Add option for --content-sandbox to desktop_unittest.py to be passed onto mochitest suites v2

Landed on default: https://hg.mozilla.org/build/mozharness/rev/9ff10f4f0a59

Comment 19

[Chris Cooper [:coop] (he/him)]

Merged to production, and deployed.

Comment 20

[Bob Owen (:bobowen) PTO until 15th Apr]

(In reply to Chris Cooper [:coop] from comment #19)
> Merged to production, and deployed.

Thanks Chris.
I think Friday afternoon is the wrong time to land Part 2, so I'll wait until Monday.

Comment 21

[Bob Owen (:bobowen) PTO until 15th Apr]

Now that Part 1 has been merged, would you mind checking in Part 2 to build/buildbot-configs default branch?

Comment 22

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8497530 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

Checked in on default:
https://hg.mozilla.org/build/buildbot-configs/rev/f93537cc7a29

Will reconfig shortly, so it gets merged to production branch, and deployed to production.

Comment 23

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8497530 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

In production

Comment 24

[Bob Owen (:bobowen) PTO until 15th Apr]

Treeherder change has landed in Live, so marking this as resolved.