1334425 - PushApk can't run because of a version mismatch with scriptworker

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[Johan Lorenzo [:jlorenzo]]

In bug 1329944, scriptworker has been upgraded to 1.0.0b8 on the puppet side[1], but not in the pushapkscript requirements[2]. This prevents pushapk to run: 

> Traceback (most recent call last):
>   File "/builds/scriptworker/lib/python3.5/site-packages/pkg_resources/__init__.py", line 635, in _build_master
>     ws.require(__requires__)
>   File "/builds/scriptworker/lib/python3.5/site-packages/pkg_resources/__init__.py", line 943, in require
>     needed = self.resolve(parse_requirements(requirements))
>   File "/builds/scriptworker/lib/python3.5/site-packages/pkg_resources/__init__.py", line 834, in resolve
>     raise VersionConflict(dist, req).with_context(dependent_req)
> pkg_resources.ContextualVersionConflict: (scriptworker 1.0.0b8 (/builds/scriptworker/lib/python3.5/site-packages), Requirement.parse('scriptworker==1.0.0b7'), {'pushapkscript'})

This error is only detected when you run a task (for instance[3]). 

That happened to me quite a few times on my loaner. That's why I try to always run a task. But let's be honest, this is fragile, hard to remember, and painful to do. Hence, the current process leverages some questions:

1. Should we use the >= operator instead of == in requirements.txt, explicitly for scriptworker? For instance: >= 1.0
2. I noticed signing script doesn't have such issue. Scriptworker is frozen to 0.9.0 in requirements[4]. How does that work? :D 
3. Should puppet fetch the matching version of scriptworker, based on the content of pushapkscript-*.tag.gz? I'm not that's feasible. This solution would also prevent from patching scriptworker in only one puppet patch :(

The short-term fix I see: Release pushapkscript with scriptworker 2.0.0. But I'd like to get a better strategy to avoid this case from happening again. What do you think, Aki?  


[1] https://hg.mozilla.org/build/puppet/rev/5f2003b3df9a02ec3306d13613dd4fc04f56398d
[2] https://github.com/mozilla-releng/pushapkscript/blob/bdd86bb16ba7a3e6b24ff597da71553ff6108012/requirements.txt#L5
[3] https://tools.taskcluster.net/task-inspector/#S6oRjEhuR8ajAHf9qxaX4g/0
[4] https://github.com/mozilla-releng/signingscript/blob/7f3e7a819820457dc564474406d85ea5b09c7b05/requirements-prod.txt#L69

Comment 1

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1334425 - PushApk can't run because of a version mismatch with scriptworker

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

Comment 2

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8831102 [details]
Bug 1334425 - PushApk can't run because of a version mismatch with scriptworker

Got an r+ on IRC from :mtabara. mtabara++

Comment 3

[Johan Lorenzo [:jlorenzo]]

Short-term fix landed in default at: https://hg.mozilla.org/build/puppet/rev/645e2a4bfc7517b42015f3923053eb3d58faca39

Published on production at: https://hg.mozilla.org/build/puppet/rev/7e7b1740ab1a81cd040778ed45a9ba6ec7147939

Comment 4

[Aki Sasaki (not active)]

(In reply to Johan Lorenzo [:jlorenzo] from comment #0)
> In bug 1329944, scriptworker has been upgraded to 1.0.0b8 on the puppet
> side[1], but not in the pushapkscript requirements[2]. This prevents pushapk
> to run:
>
> > Traceback (most recent call last):
> >   File "/builds/scriptworker/lib/python3.5/site-packages/pkg_resources/__init__.py", line 635, in _build_master
> >     ws.require(__requires__)
> >   File "/builds/scriptworker/lib/python3.5/site-packages/pkg_resources/__init__.py", line 943, in require
> >     needed = self.resolve(parse_requirements(requirements))
> >   File "/builds/scriptworker/lib/python3.5/site-packages/pkg_resources/__init__.py", line 834, in resolve
> >     raise VersionConflict(dist, req).with_context(dependent_req)
> > pkg_resources.ContextualVersionConflict: (scriptworker 1.0.0b8 (/builds/scriptworker/lib/python3.5/site-packages), Requirement.parse('scriptworker==1.0.0b7'), {'pushapkscript'})
>
> This error is only detected when you run a task (for instance[3]).

Really?  That sucks.  I would have expected it to error out at |pip install| time, though maybe the puppet module does a --nodeps type thing?

> That happened to me quite a few times on my loaner. That's why I try to
> always run a task. But let's be honest, this is fragile, hard to remember,
> and painful to do.

Agreed.  Let's not do that anymore.

> 1. Should we use the >= operator instead of == in requirements.txt,
> explicitly for scriptworker? For instance: >= 1.0

Actually, I don't pin any requirements in signingscript, because I know I'm going to pin them in puppet, and no one installs it except people wanting to develop signingscript and puppet.
https://github.com/mozilla-releng/signingscript/blob/7f3e7a819820457dc564474406d85ea5b09c7b05/setup.py#L24-L30

> 2. I noticed signing script doesn't have such issue. Scriptworker is frozen
> to 0.9.0 in requirements[4]. How does that work? :D

- setup.py doesn't pin them at all, and ignores requirements*.txt
- requirements-dev.txt duplicates the unpinned base requirements
https://github.com/mozilla-releng/signingscript/blob/7f3e7a819820457dc564474406d85ea5b09c7b05/requirements-dev.txt
- we run dephash to generate the requirements-prod.txt, to expand the requirements, pin them to versions, and add hashes.  Since nothing uses requirements-prod.txt, it doesn't matter if it's old.  But if we do update it, it's saying "these are the latest tested changes; if it's broken with the new deps you can re-dephash to see what's changed."  You can also use this file to |pip download| and use to populate puppet with pinned versions.
https://github.com/escapewindow/dephash
https://github.com/mozilla-releng/signingscript/blob/7f3e7a819820457dc564474406d85ea5b09c7b05/requirements-prod.txt
http://scriptworker.readthedocs.io/en/latest/releases.html#requirements

> 3. Should puppet fetch the matching version of scriptworker, based on the
> content of pushapkscript-*.tag.gz? I'm not that's feasible. This solution
> would also prevent from patching scriptworker in only one puppet patch :(
>
> The short-term fix I see: Release pushapkscript with scriptworker 2.0.0. But
> I'd like to get a better strategy to avoid this case from happening again.
> What do you think, Aki?

As mentioned above, let's stop pinning in setup.py, and puppet's pin will do the trick.

I think you're also importing more of scriptworker than the other 3 instance types.
balrogscript has to be py2 for now, so it doesn't import scriptworker at all.  beetmoverscript now uses chain of trust, so scriptworker does its downloads for it during chain of trust verification, so no need to import download_artifacts.  Same with signingscript.  When pushapkscript uses the chain of trust, it won't need to import download_artifacts at all either, and it'll be a bit more resilient against scriptworker changes.

I've been thinking about pulling the async utils (retry_async, download_file, etc; functions that are generic and not scriptworker-specific) out into their own module, which might help.  But if scriptworker and *script require different versions of that module, we'll hit issues again unless the *script is run out of a 2nd virtualenv, like balrogscript's py2 virtualenv.  When I think about it, really the simplest and most straightforward fix is not pinning in the script, and pin in puppet.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Attachment: pushapkscript - Only pin version in Puppet

Thanks for the details Aki. I agree with you, pinning in puppet is the most simple and immediate solution. 

I also agree `download_artifacts` is just a temporary solution until CoT is enabled in pushapk. IMO, creating another package looks overkill for now. If dependencies management remains a problem after this bug is fixed, having 2 virtualenvs sounds like a good solution.

Comment 6

[Johan Lorenzo [:jlorenzo]]

Attachment: signingscript - Clean up unused requirement files

In order to prevent further confusion, I'd like to remove these files. This would also make pushapkscript and signingscript more consistent with one another.

Comment 7

[Aki Sasaki (not active)]

(In reply to Johan Lorenzo [:jlorenzo] from comment #6)
> Created attachment 8831636 [details] [review]
> signingscript - Clean up unused requirement files
> 
> In order to prevent further confusion, I'd like to remove these files. This
> would also make pushapkscript and signingscript more consistent with one
> another.

Hm. I'm not 100% sure about this one.  As mentioned above, I use requirements-prod.txt to know which packages to put on puppet, and seeing the diff between the previous checked-in copy and the new copy tells me which packages need updating.  I also use the same requirements-*.txt pattern in scriptworker.

We could potentially move those to a packaging/ subdir or something, though in scriptworker we use the requirements-docs.txt for readthedocs.

I'd be curious as to how confusing this is for others.  `setup.py` and README.{md,rst} is the source of truth.

Comment 8

[Aki Sasaki (not active)]

Comment on attachment 8831636 [details] [review]
signingscript - Clean up unused requirement files

Clearing review per comment 7.  We can discuss further and reopen if wanted.

Comment 9

[Johan Lorenzo [:jlorenzo]]

Sounds good to me. We can discuss that later if that became a source of confusion for others. Thanks for the details! 

Landed attachment #8831634 [details] [review] on master at https://github.com/mozilla-releng/pushapkscript/commit/38924909327765fd8a2ba0d73ef9478c44434841