1408051 - Remove mozharness' copy of virtualenv and use the one under /third_party/python instead

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, enhancement)

Description

[Andrew Halberstadt [:ahal]]

In bug 1297515 we copied the in-tree virtualenv to testing/mozharness/external_tools. We needed to do this to upgrade pip, since buildbot was downloading the 'testing/mozharness' directory directly from hg.m.o and there was no easy way to also package other directories.

This means there are now two copies of virtualenv in the tree. Once everything is off of buildbot, we can remove the copy under testing/mozharness/external_tools and instead package virtualenv into the mozharness.zip artifact that taskcluster builds produce.

Comment 1

[Andrew Halberstadt [:ahal]]

Catlee indicated this should be actionable now that everything is off buildbot. If anyone would like to try and take a stab at this I'm happy to mentor. Otherwise I may get around to this eventually :).

Comment 2

[Aki Sasaki (not active)]

We have various bits of releng automation (merge day which gets it from the archiver [1], l10n bumper which downloads it from hg.m.o as a tarball [2]) which would need to be updated to get virtualenv from the new location.

[1] https://github.com/mozilla-releng/releasewarrior-2.0/blob/master/docs/mergeduty/howto.md#connect-to-remote-instance-and-get-a-copy-of-mozharness
[2] https://hg.mozilla.org/build/puppet/file/tip/modules/l10n_bumper/templates/download_mozharness.sh.erb#l7

Comment 3

[Chris AtLee [:catlee]]

Attachment: Bug 1408051: Remove mozharness' copy of virtualenv

Review commit: https://reviewboard.mozilla.org/r/244686/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/244686/

Comment 4

[Tom Prince [:tomprince]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review250744

::: testing/mozharness/mozharness/base/python.py:344
(Diff revision 1)
>          c = self.config
>          dirs = self.query_abs_dirs()
>          venv_path = self.query_virtualenv_path()
>          self.info("Creating virtualenv %s" % venv_path)
>  
> -        # Always use the virtualenv that is vendored since that is deterministic.
> +        for p in ('base_work_dir', 'abs_src_dir'):

This comment is probably worth keeping.

::: testing/mozharness/mozharness/base/python.py:510
(Diff revision 1)
>          if sys.version_info[:2] < (2, 7):
>              self.warning('Resource monitoring will not be enabled! Python 2.7+ required.')
>              return
>  
>          try:
> +            self.activate_virtualenv()

Does this want to be in the `try:` block? I think a failure here is distinct enough that it shouldn't be conveted to the generic error about the resource monitor.

Comment 5

[Chris AtLee [:catlee]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244686/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a00ab75aea2b
Remove mozharness' copy of virtualenv r=tomprince

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out changeset a00ab75aea2b (bug 1408051) for talos failures.

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=672e64a2425343f6098aeaef0b14444ee6133138

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a00ab75aea2b5d6008589c74596a983e2b964e3c

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179031613&repo=autoland&lineNumber=177

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

Mercurial backout link: https://hg.mozilla.org/integration/autoland/rev/810472f77310c897700baf11b8758b7916e8583b

Comment 9

[Chris AtLee [:catlee]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244686/diff/2-3/

Comment 10

[Tom Prince [:tomprince]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review250914

::: python/mozbuild/mozbuild/action/test_archive.py:376
(Diff revision 3)
>              'pattern': 'mozharness/**',
>          },
> +        {
> +            'source': buildconfig.topsrcdir,
> +            'base': 'third_party/python/virtualenv',
> +            'dest': 'third_party/python/virtualenv',

Would it be better to prefix this with `mozharness/`? Or, maybe even better, change the prefix to match the old location: `mozharness/external_tools/virtualenv`? It is unusual to have a a zip or tarball extract to multiple top-level directories.

This means we don't need the changes to the taskcluster scripts (which isn't the reason), and some changes to the logic for finding the virtualenv location.

Comment 11

[Tom Prince [:tomprince]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review250914

> Would it be better to prefix this with `mozharness/`? Or, maybe even better, change the prefix to match the old location: `mozharness/external_tools/virtualenv`? It is unusual to have a a zip or tarball extract to multiple top-level directories.
> 
> This means we don't need the changes to the taskcluster scripts (which isn't the reason), and some changes to the logic for finding the virtualenv location.

To be clear:

We should definitely have mozharness.zip only have one top-level directory. I don't have strong feelings on where virtualenv goes inside of it.

Comment 12

[Chris AtLee [:catlee]]

(In reply to Tom Prince [:tomprince] from comment #11)
> Comment on attachment 8976582 [details]
> Bug 1408051: Remove mozharness' copy of virtualenv
> 
> https://reviewboard.mozilla.org/r/244686/#review250914
> 
> > Would it be better to prefix this with `mozharness/`? Or, maybe even better, change the prefix to match the old location: `mozharness/external_tools/virtualenv`? It is unusual to have a a zip or tarball extract to multiple top-level directories.
> > 
> > This means we don't need the changes to the taskcluster scripts (which isn't the reason), and some changes to the logic for finding the virtualenv location.
> 
> To be clear:
> 
> We should definitely have mozharness.zip only have one top-level directory.
> I don't have strong feelings on where virtualenv goes inside of it.

I wanted to try and keep the structure of mozharness.zip as close as possible to the original source directory. I'll look into moving virtualenv into mozharness/third_party/... inside the zip file.

Comment 13

[Chris AtLee [:catlee]]

(In reply to Tom Prince [:tomprince] from comment #4)
> ::: testing/mozharness/mozharness/base/python.py:510
> (Diff revision 1)
> >          if sys.version_info[:2] < (2, 7):
> >              self.warning('Resource monitoring will not be enabled! Python 2.7+ required.')
> >              return
> >  
> >          try:
> > +            self.activate_virtualenv()
> 
> Does this want to be in the `try:` block? I think a failure here is distinct
> enough that it shouldn't be conveted to the generic error about the resource
> monitor.

Now I remember why I put this in. If the main create_virtualenv action fails, then activate_virtualenv() will also raise an exception in the post-action handler. The original exception from create_virtualenv is then lost. I think this is actually a more fundamental bug in how BaseScript is not logging exceptions from the main action methods.

Comment 14

[Chris AtLee [:catlee]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244686/diff/3-4/

Comment 15

[Tom Prince [:tomprince]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

https://reviewboard.mozilla.org/r/244686/#review251104

::: taskcluster/scripts/tester/test-macosx.sh:41
(Diff revision 4)
>  
>      echo "Downloading mozharness"
>  
>      while [[ $attempt < $max_attempts ]]; do
>          if curl --fail -o mozharness.zip --retry 10 -L $MOZHARNESS_URL; then
> -            rm -rf mozharness
> +            rm -rf mozharness third_party

This change is not needed.

Comment 16

[Chris AtLee [:catlee]]

Comment on attachment 8976582 [details]
Bug 1408051: Remove mozharness' copy of virtualenv

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/244686/diff/4-5/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5afad9b7fd21
Remove mozharness' copy of virtualenv r=tomprince

Comment 18

[Andreea Pavel [:apavel]]

Backed out for slowing down setup of test environments on OS X

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5afad9b7fd2114e7fd63d6b3674e7d9f169c2565

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179220602&repo=autoland

Backout: https://hg.mozilla.org/integration/autoland/rev/2375438b837c39405dd2c3fa143c12611be82e02

Comment 19

[Chris AtLee [:catlee]]

The jobs are taking like 20 minutes to set up their virtualenv on OSX. Not sure why yet....

Comment 20

[Chris AtLee [:catlee]]

With some more debug logging, it looks like the new pip is spending a long time fetching indexes from https endpoints. e.g.

old:
18:13:47     INFO -    Getting page https://pypi.pub.build.mozilla.org/pub
18:13:47     INFO -    Starting new HTTPS connection (1): pypi.pub.build.mozilla.org
18:13:47     INFO -    "GET /pub HTTP/1.1" 301 246
18:13:47     INFO -    "GET /pub/ HTTP/1.1" 200 None
18:13:48     INFO -    Analyzing links from page https://pypi.pub.build.mozilla.org/pub/

new:
15:38:39     INFO -    Getting page https://pypi.pub.build.mozilla.org/pub
15:38:39     INFO -    Starting new HTTPS connection (1): pypi.pub.build.mozilla.org
15:38:39     INFO -    https://pypi.pub.build.mozilla.org:443 "GET /pub HTTP/1.1" 301 246
15:38:39     INFO -    https://pypi.pub.build.mozilla.org:443 "GET /pub/ HTTP/1.1" 200 None
15:40:40     INFO -    Analyzing links from page https://pypi.pub.build.mozilla.org/pub/

Aki found this, which may be relevant: https://github.com/pyca/pyopenssl/issues/625

Comment 21

[Andrew Halberstadt [:ahal]]

It might be worth upgrading our in-tree pip and trying again (current is 9.0.3, released is 18.0). Also this reply has a workaround that seems worth investigating:
https://github.com/pyca/pyopenssl/issues/625#issuecomment-300322038

Comment 22

[Hamzah Akhtar]

Attachment: Bug 1408051 - Remove mozharness' copy of virtualenv and use the one under /third_party/python instead

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7864e0d0295f
Remove mozharness' copy of virtualenv and use the one under /third_party/python instead r=ahal,rail

Comment 24

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/7864e0d0295f

Comment 25

[Andrew Halberstadt [:ahal]]

I think we should consider backing this out and fixing the regressions on our own time.

Comment 26

[Andrew Halberstadt [:ahal]]

Attachment: Bug 1408051 - Backed out changeset 7864e0d0295f

This change had a few regressions that will be easier to fix without the pressure of time.

Comment 27

[Andrew Halberstadt [:ahal]]

The somewhat good news is that bug 1655666 has a patch, and the issues from bug 1655164 and bug 1655180 look the same. So I think there is only thing to fix here. I couldn't figure out why it was happening from a brief glance, so decided to back this change out in the meantime.

Here is the traceback:

[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL - Uncaught exception: Traceback (most recent call last):
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/base/script.py", line 2137, in run
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     self.run_action(action)
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/base/script.py", line 2076, in run_action
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     self._possibly_run_method(method_name, error_if_missing=True)
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/base/script.py", line 2031, in _possibly_run_method
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     return getattr(self, method_name)()
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/mozilla/testing/raptor.py", line 483, in install_chrome_android
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     self.device.shell_output("settings put global verifier_verify_adb_installs 0")
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/mozilla/testing/android.py", line 57, in device
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     adb = self.adb_path
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/mozilla/testing/android.py", line 45, in adb_path
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     self.activate_virtualenv()
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -   File "/builds/task_1595246656/workspace/mozharness/mozharness/base/python.py", line 442, in activate_virtualenv
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL -     exec(open(activate).read(), dict(__file__=activate))
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL - IOError: [Errno 2] No such file or directory: '/builds/task_1595246656/workspace/build/venv/bin/activate_this.py'
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL - Running post_fatal callback...
[task 2020-07-20T12:13:21.829Z] 12:13:21    FATAL - Exiting -1

Comment 28

[Andrew Halberstadt [:ahal]]

I just tested and the version of virtualenv we have in-tree does produce an activate_this.py script, so I have no idea why it appears to be missing. Maybe the whole venv path is wrong or something?

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6693b3743ad9
Backed out changeset 7864e0d0295f

Comment 30

[Hamzah Akhtar]

I think it's happening because it's not able to find abs_src_dir or base_work_dir in the dictionary

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6693b3743ad9

Comment 32

[Julien Cristau [:jcristau]]

Do you also want the backout on beta, or are we ok there?

Comment 33

[Andrew Halberstadt [:ahal]]

Thanks for asking! I don't think it's a huge deal to leave it, but I guess ideally it would be backed out on beta too.

Julien, would you be able to handle that for me?

Comment 34

[Tom Prince [:tomprince]]

I think with the fix from bug 1655666 we don't need to land it on beta, unless there are other issues it is causing. :ahal, with that fix, is there any reason this couldn't land again? I didn't investigate all the regressions, but based on comment 27, it sounds like they all had the same root cause.

Comment 35

[Andrew Halberstadt [:ahal]]

I was under the impression that bug 1655666 was a separate issue from the other two, but I haven't investigated very closely. If they are all the same issue then I guess I backed it out for now reason :(.

Hamzah, can you confirm if bug 1655666 fixed the other regression?

Comment 36

[Hamzah Akhtar]

bug1655666 fixed perma android regression(bug 1655164) , but it didn't fix bug 1655180.
I tried fixing that and did a try run . Everything seemed to work fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc11189ee0e590df5038b3db3bca67c60a44dd0
Andrew if you could please confirm this then that would be great.

Comment 37

[Andrew Halberstadt [:ahal]]

(In reply to Hamzah Akhtar from comment #36)

bug1655666 fixed perma android regression(bug 1655164) , but it didn't fix bug 1655180.

Ah ok, thanks for clarifying! It looks like a separate patch landed in bug 1655164 though (aside from the one in bug 1655666). Maybe that second patch also ended up fixing bug 1655180?

I tried fixing that and did a try run . Everything seemed to work fine.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdc11189ee0e590df5038b3db3bca67c60a44dd0
Andrew if you could please confirm this then that would be great.

That bug talks about mach raptor being broken, and your try push doesn't contain any raptor tasks (even if it did, I think the issue was only showing up locally). So it's hard to say.

Could you answer the following please?

1. Can you reproduce the mach raptor issue on the latest central with only the patch from this bug? (i.e excluding the fix from your try push)
2. If so, can you confirm that your try push from comment 36 fixes it?

Either way sounds like we're close here.

Comment 38

[Hamzah Akhtar]

After looking at that try run that I mentioned above again, I remembered that it was related to perma Android regression, I think that is fixed now.
About the mach raptor command being broken, yeah I was able to reproduce to it using the patch in this bug, also I was able to fix it
I'll submit a patch for that in bug 1655180

Comment 39

[Andrew Halberstadt [:ahal]]

Perfect, thanks for clarifying. It might be cleanest to incorporate the changes from that fix in the patch here, but it's up to you :).

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d25219ad617a
Remove mozharness' copy of virtualenv and use the one under /third_party/python instead r=ahal,rail

Comment 41

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/d25219ad617a

Comment 42

[Mitchell Hentges [:mhentges] 🦀]

Comment on attachment 9162914 [details]
Bug 1408051 - Remove mozharness' copy of virtualenv and use the one under /third_party/python instead

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Resolves pyvenv.cfg intermittent on MinGW builds
• User impact if declined: None
• Fix Landed on Version: 81
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a build-only patch
• String or UUID changes made by this patch:

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr78/rev/ab4b0bc06dc2