1473430 - addon scriptworker tasks fail on Try

Status: RESOLVED WORKSFORME

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

Description

[Aki Sasaki (not active)]

This might be easy, might be tricky.
Looks like our sign-and-push-langpacks tasks successfully submitted the langpacks to amo-staging, but failed to download the signed file(s).

- This might be a timeout where we should auto-retry; maybe it'll Just Work if we give it enough retries.
- This might be an issue with the version, 63.0a1, being unknown to amo-staging
- Something else might be wrong, not sure.

The fixes could be:

- if it's a timeout, get the auto-retries going like we have in prod
- if it's a version issue,
  - we may have to send a special flag to addonscript to fake a version. We should only be able to pass this flag in staging.
  - something else?
- if something else is wrong, we need to debug and then find a solution.

Traceback (most recent call last):
  File "/builds/scriptworker/lib/python3.6/site-packages/scriptworker/client.py", line 203, in _handle_asyncio_loop
    await async_main(context)
  File "/builds/scriptworker/lib/python3.6/site-packages/addonscript/script.py", line 75, in async_main
    await asyncio.gather(*tasks)
  File "/builds/scriptworker/lib/python3.6/site-packages/addonscript/script.py", line 45, in sign_addon
    retry_exceptions=tuple([ClientError, asyncio.TimeoutError, SignatureError]),
  File "/builds/scriptworker/lib/python3.6/site-packages/scriptworker/utils.py", line 260, in retry_async
    return await func(*args, **kwargs)
  File "/builds/scriptworker/lib/python3.6/site-packages/addonscript/api.py", line 66, in get_signed_addon_url
    len(upload_status['files']), upload_status,
addonscript.exceptions.SignatureError: Expected 1 file. Got (0) full response: {'guid': '[e-mail]', 'active': False, 'automated_signing': True, 'url': 'https://addons.allizom.org/api/v3/addons/[e-mail]/versions/63.0a1buildid20180703225642/uploads/10b67dae367d4413ab9129f47cdf768d/', 'files': [], 'passed_review': False, 'pk': '10b67dae367d4413ab9129f47cdf768d', 'processed': True, 'reviewed': False, 'valid': False, 'validation_results': {'errors': 1, 'success': False, 'warnings': 0, 'compatibility_summary': {'notices': 0, 'errors': 0, 'warnings': 0}, 'ending_tier': 5, 'messages': [{'uid': '3ce9819534ea4d3dae570d15901b5054', 'dataPath': '/version', 'tier': 1, 'message': '"/version" should match format "versionString"', 'type': 'error', 'id': ['JSON_INVALID'], 'description': 'Your JSON file could not be parsed.'}, {'uid': '4a42a09053044de89e82317f3175f6c2', 'file': 'manifest.json', 'tier': 1, 'message': '"strict_max_version" not required.', 'type': 'notice', 'id': ['STRICT_MAX_VERSION'], 'description': '"strict_max_version" shouldn\'t be used unless the add-on is expected not to work with future versions of Firefox.'}, {'uid': 'c4a9ce75369e4b2caea7c6f00246056e', 'file': 'browser/chrome/en-US/locale/browser/aboutRobots.dtd', 'tier': 1, 'message': 'Violation of Mozilla conditions of use.', 'type': 'notice', 'id': ['MOZILLA_COND_OF_USE'], 'description': 'Words found that violate the Mozilla conditions of use. See https://www.mozilla.org/en-US/about/legal/acceptable-use/ for more details.'}], 'detected_type': 'extension', 'notices': 2, 'metadata': {'manifestVersion': 2, 'identified_files': {}, 'is_webextension': True, 'name': 'English (US) Language Pack', 'emptyFiles': [], 'processed_by_addons_linter': True, 'unknownMinifiedFiles': [], 'version': '63.0a1buildid20180703225642', 'listed': False, 'totalScannedFileSize': 1127749, 'type': 1, 'id': '[e-mail]', 'jsLibs': {}}}, 'validation_url': 'https://addons.allizom.org/en-US/developers/upload/10b67dae367d4413ab9129f47cdf768d', 'version': '63.0a1buildid20180703225642'}

Comment 1

[Aki Sasaki (not active)]

Oh, we did have 5 retries =\
An example task that failed: https://tools.taskcluster.net/groups/JzDg_-vqQDei9xN7fg_wLQ/tasks/aPGAgQE7QLCUNVBtrfNo_A/runs/5

Comment 2

[Justin Wood (:Callek)]

Thoughts:

AMO (even on stage) has the restriction of one (and only one) version number per upload. So as soon as we run this once we burn that version for that AMO deploy. (stage).

I do think the version number on trunk breaks assumptions, e.g. I don't think we can do `63.0a1buildid2018...` but it would have to match the code in https://github.com/mozilla/addons-linter/blob/8e7f2bc9d763a645dcc7da7960dd3fb3c49f4de2/src/schema/formats.js#L10

Staging AMO *also* won't produce langpacks that are installable by default on Firefox, it uses a seperate key. (Not important today, but could be important if someone wants to validate against these try releases)

Comment 3

[Aki Sasaki (not active)]

(In reply to Justin Wood (:Callek) from comment #2)
> Thoughts:
> 
> AMO (even on stage) has the restriction of one (and only one) version number
> per upload. So as soon as we run this once we burn that version for that AMO
> deploy. (stage).

ITYM only one upload per version? Boo. I wonder if we could relax that on stage, or have a dev instance with that relaxed. A docker image that gives us the AMO api and fake signing would be even better.

> I do think the version number on trunk breaks assumptions, e.g. I don't
> think we can do `63.0a1buildid2018...` but it would have to match the code
> in
> https://github.com/mozilla/addons-linter/blob/
> 8e7f2bc9d763a645dcc7da7960dd3fb3c49f4de2/src/schema/formats.js#L10

Well, if we munge the version we report to dev- or stage-AMO in transforms on try, then we could report any version.

> Staging AMO *also* won't produce langpacks that are installable by default
> on Firefox, it uses a seperate key. (Not important today, but could be
> important if someone wants to validate against these try releases)

I'm wondering if we can allow for the staging AMO key in try staging releases, like we could allow for the dep MAR signing key in try staging release update verification. If not, if it's installable if someone overrides a pref or something, that's probably sufficient for phase 1 of try-staging.

Comment 4

[Justin Wood (:Callek)]

(In reply to Aki Sasaki [:aki] from comment #3)
> (In reply to Justin Wood (:Callek) from comment #2)
> > Thoughts:
> > 
> > AMO (even on stage) has the restriction of one (and only one) version number
> > per upload. So as soon as we run this once we burn that version for that AMO
> > deploy. (stage).
> 
> ITYM only one upload per version? Boo. 

Yea that is what I meant

> I wonder if we could relax that on
> stage, or have a dev instance with that relaxed. A docker image that gives
> us the AMO api and fake signing would be even better.

AIUI we can't relax that at all, lots of baked in assumptions in the AMO codebase around version numbers and indexing/joining based on them.

We could (in theory) create our own docker image with a fake AMO API if we really wanted to go that far, I don't think the AMO team has anything of the sort though, and I'm not sure what that gets us in the end.


> 
> > I do think the version number on trunk breaks assumptions, e.g. I don't
> > think we can do `63.0a1buildid2018...` but it would have to match the code
> > in
> > https://github.com/mozilla/addons-linter/blob/
> > 8e7f2bc9d763a645dcc7da7960dd3fb3c49f4de2/src/schema/formats.js#L10
> 
> Well, if we munge the version we report to dev- or stage-AMO in transforms
> on try, then we could report any version.

We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/action/langpack_manifest.py#294

This is generated during the en-US build for the en-US langpack, and then again for each locale. So we can't modify it in taskgraph time. -- It is technically possible to modify it outside of this code in addonscript itself, but then we're diverging in logic from what we'd do (and want to do) in a release, especially if the version string format ever changes in the product.

> 
> > Staging AMO *also* won't produce langpacks that are installable by default
> > on Firefox, it uses a seperate key. (Not important today, but could be
> > important if someone wants to validate against these try releases)
> 
> I'm wondering if we can allow for the staging AMO key in try staging
> releases, like we could allow for the dep MAR signing key in try staging
> release update verification. If not, if it's installable if someone
> overrides a pref or something, that's probably sufficient for phase 1 of
> try-staging.

IIRC there is a hidden pref that can be flipped (in !release) to enable this, I think release is completely restricted.  Asking :aswan about it would be the best source of knowledge I know of.

Comment 5

[Aki Sasaki (not active)]

> We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.

Ok. Maybe the path forward here is to use the the version number from the relpromo action's input on Try, and to ignore the version.txt / version_info.txt in-tree. That gets us past one roadblock, but not past the one-upload-per-version. (though doesn't the buildid help us there?)

Comment 6

[Justin Wood (:Callek)]

(In reply to Aki Sasaki [:aki] from comment #5)
> > We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.
> 
> Ok. Maybe the path forward here is to use the the version number from the
> relpromo action's input on Try, and to ignore the version.txt /
> version_info.txt in-tree. That gets us past one roadblock, but not past the
> one-upload-per-version. (though doesn't the buildid help us there?)

That won't fix it for en-US langpack, nor in future if we use on-push locale repacks as the source of relpro publishing...

Buildid could help indeed, but does have the issue of "if we are `listed` on AMO only the most-recently uploaded listed is reported to users"

Comment 7

[Aki Sasaki (not active)]

(In reply to Justin Wood (:Callek) from comment #6)
> (In reply to Aki Sasaki [:aki] from comment #5)
> > > We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.
> > 
> > Ok. Maybe the path forward here is to use the the version number from the
> > relpromo action's input on Try, and to ignore the version.txt /
> > version_info.txt in-tree. That gets us past one roadblock, but not past the
> > one-upload-per-version. (though doesn't the buildid help us there?)
> 
> That won't fix it for en-US langpack, nor in future if we use on-push locale
> repacks as the source of relpro publishing...

Why wouldn't it fix the en-US langpack? I'm assuming we also pass in the version number into the build somehow.
On-push locale repacks in production would use the version number in-tree, no?

> Buildid could help indeed, but does have the issue of "if we are `listed` on
> AMO only the most-recently uploaded listed is reported to users"

Comment 8

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #7)
> (In reply to Justin Wood (:Callek) from comment #6)
> > (In reply to Aki Sasaki [:aki] from comment #5)
> > Buildid could help indeed, but does have the issue of "if we are `listed` on
> > AMO only the most-recently uploaded listed is reported to users"

I'm not sure this matters as much in try staging, correct?

Comment 9

[Aki Sasaki (not active)]

(In reply to Aki Sasaki [:aki] from comment #7)
> (In reply to Justin Wood (:Callek) from comment #6)
> > (In reply to Aki Sasaki [:aki] from comment #5)
> > > > We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.
> > > 
> > > Ok. Maybe the path forward here is to use the the version number from the
> > > relpromo action's input on Try, and to ignore the version.txt /
> > > version_info.txt in-tree. That gets us past one roadblock, but not past the
> > > one-upload-per-version. (though doesn't the buildid help us there?)
> > 
> > That won't fix it for en-US langpack, nor in future if we use on-push locale
> > repacks as the source of relpro publishing...
> 
> Why wouldn't it fix the en-US langpack? I'm assuming we also pass in the
> version number into the build somehow.

Sorry, not thinking. The build generally happens on push. However, on try we *could* just create the decision task, and then populate the build via release promotion action, and pass in version.

This is complex enough that maybe there should be a tool to populate your try tree the way you want before pushing: bump the version, limit the locales, etc.

Comment 10

[Justin Wood (:Callek)]

(In reply to Aki Sasaki [:aki] from comment #9)
> (In reply to Aki Sasaki [:aki] from comment #7)
> > (In reply to Justin Wood (:Callek) from comment #6)
> > > (In reply to Aki Sasaki [:aki] from comment #5)
> > > > > We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.
> > > > 
> > > > Ok. Maybe the path forward here is to use the the version number from the
> > > > relpromo action's input on Try, and to ignore the version.txt /
> > > > version_info.txt in-tree. That gets us past one roadblock, but not past the
> > > > one-upload-per-version. (though doesn't the buildid help us there?)
> > > 
> > > That won't fix it for en-US langpack, nor in future if we use on-push locale
> > > repacks as the source of relpro publishing...
> > 
> > Why wouldn't it fix the en-US langpack? I'm assuming we also pass in the
> > version number into the build somehow.
> 
> Sorry, not thinking. The build generally happens on push. However, on try we
> *could* just create the decision task, and then populate the build via
> release promotion action, and pass in version.
> 
> This is complex enough that maybe there should be a tool to populate your
> try tree the way you want before pushing: bump the version, limit the
> locales, etc.

To be clear, we [Us + AMO + L10n teams] will need to figure out a way to version langpacks for Nightly in the forseeable future anyway, to align with the localization plans that are in the pipeline anyway. So this versioning may not be an issue eventually.

Comment 11

[Aki Sasaki (not active)]

(In reply to Justin Wood (:Callek) from comment #10)
> (In reply to Aki Sasaki [:aki] from comment #9)
> > (In reply to Aki Sasaki [:aki] from comment #7)
> > > (In reply to Justin Wood (:Callek) from comment #6)
> > > > (In reply to Aki Sasaki [:aki] from comment #5)
> > > > > > We can't do this... the version number gets baked into the language pack itself, and AMO reads from the language pack as the source of truth (will complain if API differs), so addonscript uses thelanguage pack itself as the source of truth too for that reason.
> > > > > 
> > > > > Ok. Maybe the path forward here is to use the the version number from the
> > > > > relpromo action's input on Try, and to ignore the version.txt /
> > > > > version_info.txt in-tree. That gets us past one roadblock, but not past the
> > > > > one-upload-per-version. (though doesn't the buildid help us there?)
> > > > 
> > > > That won't fix it for en-US langpack, nor in future if we use on-push locale
> > > > repacks as the source of relpro publishing...
> > > 
> > > Why wouldn't it fix the en-US langpack? I'm assuming we also pass in the
> > > version number into the build somehow.
> > 
> > Sorry, not thinking. The build generally happens on push. However, on try we
> > *could* just create the decision task, and then populate the build via
> > release promotion action, and pass in version.
> > 
> > This is complex enough that maybe there should be a tool to populate your
> > try tree the way you want before pushing: bump the version, limit the
> > locales, etc.
> 
> To be clear, we [Us + AMO + L10n teams] will need to figure out a way to
> version langpacks for Nightly in the forseeable future anyway, to align with
> the localization plans that are in the pipeline anyway. So this versioning
> may not be an issue eventually.

That sounds like the best fix, as long as it covers both our production and staging needs :)

Comment 12

[Julien Cristau [:jcristau]]

Pretty sure this works nowadays.