Closed Bug 1074147 Opened 10 years ago Closed 10 years ago

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

Categories

(Release Engineering :: General, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bobowen, Assigned: bobowen)

References

Details

Attachments

(3 files, 2 obsolete files)

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.
Depends on: 1074156
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?
Flags: needinfo?(catlee)
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?
catlee's on PTO. 302 laura :)
Flags: needinfo?(catlee) → needinfo?(laura)
is this just on holly right now? what is the plan after that?
(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.
Attachment #8496814 - Attachment is obsolete: true
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 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?
Attachment #8496813 - Flags: review?(armenzg)
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.
Attachment #8497530 - Flags: review?(armenzg)
Attachment #8497530 - Flags: feedback?(wmccloskey)
Depends on: 1075527
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.
Attachment #8497530 - Flags: feedback?(wmccloskey) → feedback+
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?
Attachment #8496813 - Flags: review?(armenzg) → review+
(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 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.
Attachment #8497530 - Flags: review?(armenzg) → review+
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.
Attachment #8496813 - Attachment is obsolete: true
Attachment #8498751 - Flags: review+
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.
Keywords: checkin-needed
Flags: needinfo?(pmoore)
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
Attachment #8498751 - Flags: review+ → review?(pmoore)
Flags: needinfo?(pmoore)
Attachment #8498751 - Flags: review?(pmoore) → review+
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
Attachment #8498751 - Flags: checked-in+
Flags: needinfo?(laura)
Keywords: checkin-needed
Merged to production, and deployed.
(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.
Now that Part 1 has been merged, would you mind checking in Part 2 to build/buildbot-configs default branch?
Flags: needinfo?(pmoore)
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.
Attachment #8497530 - Flags: checked-in+
Flags: needinfo?(pmoore)
Comment on attachment 8497530 [details] [diff] [review]
Part 2: On holly add mochitest plain tests with the content sandbox for win32 v2

In production
Treeherder change has landed in Live, so marking this as resolved.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: