1385401 - Fetch Google Play listings outside of the push-apk task

Status: RESOLVED FIXED

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

Comment 1

[Mihai Tabara [:mtabara]⌚️GMT]

Still no progress on this yet. Shipping 55.0 and related took most of my time. Will have a look at this as soon as my plate gets emptier.

Comment 2

[Mihai Tabara [:mtabara]⌚️GMT]

Something like two different tasks in the graph so that we can ignore the GP string sync changes pull.

Comment 4

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] push_apk.py: Add option to not update listings and "what's new"

This patch and the following ones were reviewed and checked in on Github. I just attached them here to track what's happened.

Comment 5

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] push_apk.py: Pass listings/whatsnew via on-disk files

Comment 6

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] get_apk.py: Deactivate latest_nightly test because of bug 1346752

Comment 7

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] Move to common

Comment 8

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] Create get_l10n_strings.py script

Comment 9

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] Drop python 2 support

Comment 10

[Johan Lorenzo [:jlorenzo]]

Attachment: [mozapkpublisher] Enforce incoming locales to comply to a given schema

Comment 11

[Johan Lorenzo [:jlorenzo]]

Attachment: [pushapksript] Conditionally update Google Play listings based on upstream tasks

Comment 12

[Johan Lorenzo [:jlorenzo]]

Attachment: [scriptworker] Support optional upstream artifacts

Comment 13

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1385401 - Fetch Google Play listings outside of the push-apk task

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

Comment 14

[Johan Lorenzo [:jlorenzo]]

Status update: The happy path works![1]

* Scriptworker manages to download google_play_strings.json from a different task. 
* Pushapkscript passes the file to mozapkpublisher.
* Mozapkpublisher reads it and makes the right requests to Google Play.

However, the cases where the fetch task failed aren't correctly handled. At the moment:

* Even if the fetch task failed, scriptworker tries to download chainOfTrust.asc (and errors out on it).
* If the previous task did generate chainOfTrust.asc, scriptworker fails to find the sha of google_play_strings.json in it. 

I need to fix these 2 cases before. On the bright side, `"requires": "all-resolved"` works like a charm. The push-apk task does start even if the fetch task failed.

Improvements for the future:

* Find what's the exit code if get_l10n_strings.py failed due to some network error. Then we would be able to retry on it.
* Think of what are the edge cases of allowing chainOfTrust.asc not to be present.

[1] https://tools.taskcluster.net/groups/RakKiHfTRTaj4owi9f51CA/tasks/a7NlawieRO207Y21-oF4tg/runs/4/logs/public%2Flogs%2Flive_backing.log

Comment 15

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8933622 [details] [review]
[scriptworker] Support optional upstream artifacts

(In reply to Johan Lorenzo [:jlorenzo] from comment #14)
> However, the cases where the fetch task failed aren't correctly handled. At
> the moment:
> 
> * Even if the fetch task failed, scriptworker tries to download
> chainOfTrust.asc (and errors out on it).
> * If the previous task did generate chainOfTrust.asc, scriptworker fails to
> find the sha of google_play_strings.json in it. 

First case is fixed. We allow chainOfTrust.json.asc to be absent, only if there is no mandatory upstream artifact needed. The corollary is: if there is no upstream artifact at all, then chainOfTrust.json.asc is still allowed to be absent. Here's an example of how scriptworker behaved in such case: https://tools.taskcluster.net/groups/RakKiHfTRTaj4owi9f51CA/tasks/a7NlawieRO207Y21-oF4tg/runs/15/logs/public%2Flogs%2Fchain_of_trust.log

Second case is fixed too. If an optional artifact fails to be downloaded for whatever reason (this includes bad shas), SHA verification will be skipped on that artifact. Here's an example of that case: https://tools.taskcluster.net/groups/LJpaV_Z7TYG3phzMH5gJ_A/tasks/Kmw6-ImSTZCoYq6KUJckGw/runs/5/logs/public%2Flogs%2Fchain_of_trust.log


I hope I didn't break the security model with such assumptions. That's why I'd prefer to have 2 pairs of eyes on this review. Please tell me what you guys think!

Comment 16

[Aki Sasaki (not active)]

Comment on attachment 8933626 [details]
Bug 1385401 - Fetch Google Play listings outside of the push-apk task

https://reviewboard.mozilla.org/r/204570/#review211130

::: taskcluster/ci/google-play-strings/kind.yml:17
(Diff revision 1)
> +   google-play-strings:
> +      description: Download strings to display on Google Play from https://l10n.mozilla-community.org/stores_l10n/
> +      attributes:
> +         build_type: google_play_strings
> +         build_platform: android-nightly
> +         nightly: true

If we can add an attributes['shipping_phase'] (of either `build` or `promote` depending when we want this to happen) and an attributes['shipping_product'] of `fennec`, we'll be ahead of the game for shippable builds. We can do this by either setting those attributes directly, or setting task['shipping-phase'] and task['shipping-product'] (dash not underscore).

Not a blocker.

::: taskcluster/taskgraph/transforms/google_play_strings.py:14
(Diff revision 1)
> +
> +import functools
> +
> +from taskgraph.transforms.base import TransformSequence
> +from taskgraph.util.schema import resolve_keyed_by, Schema
> +# from taskgraph.util.scriptworker import

leftover?

::: taskcluster/taskgraph/transforms/google_play_strings.py:28
(Diff revision 1)
> +    # the dependent task (object) for this beetmover job, used to inform beetmover.
> +    Required('name'): basestring,
> +    Required('label'): basestring,
> +    Required('description'): basestring,
> +    Required('job-from'): basestring,
> +    Required('attributes'): object,

It might be best to use `task_description_schema` for these.

::: taskcluster/taskgraph/transforms/push_apk.py:43
(Diff revision 1)
>      Required('treeherder'): object,
>      Required('run-on-projects'): list,
>      Required('worker-type'): optionally_keyed_by('project', basestring),
>      Required('worker'): object,
>      Required('scopes'): None,
> +    Required('requires'): basestring,

Here too. Already started with `shipping-phase` and `shipping-product`.

Comment 17

[Aki Sasaki (not active)]

Comment on attachment 8933622 [details] [review]
[scriptworker] Support optional upstream artifacts

Comments in PR.

Comment 18

[Johan Lorenzo [:jlorenzo]]

(In reply to Aki Sasaki [:aki] from comment #16)
Comments have been addressed in https://hg.mozilla.org/projects/maple/rev/2c5b2aa85d79e0b2b57cb35e579b825108d089ae

Comment 19

[[e-mail] (:bhearsum)]

FYI: This caused https://bugzilla.mozilla.org/show_bug.cgi?id=1426445, which I've landed a fix for on maple (https://hg.mozilla.org/projects/maple/rev/f5047440978ee8f81507dde1119d3e5dd7e7d03f)

Comment 20

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8933622 [details] [review]
[scriptworker] Support optional upstream artifacts

Was r'+d by Mihai at https://github.com/mozilla-releng/scriptworker/pull/169#pullrequestreview-81815058

Merged on master at https://github.com/mozilla-releng/scriptworker/commit/4b6a85d95b49f5573537298927c6c6e1c75e38bd

Version 6.0.0 bumped at https://github.com/mozilla-releng/scriptworker/pull/172

Comment 21

[Johan Lorenzo [:jlorenzo]]

Attachment: [pushapkscript] Allow tasks to not have GP strings defined in upstreamArtifacts, for older trees

Comment 22

[Johan Lorenzo [:jlorenzo]]

Attachment: [puppet] Bug 1385401 - pushapk_scriptworker: bump scriptworker and pushapkscript a=versionbump

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

Comment 23

[Johan Lorenzo [:jlorenzo]]

Every patch but attachment 8933626 [details] have landed. This means all the changes done to scriptworker, pushapkscript, and mozapkpublisher are live (on both {dep-,}pushapkworker-1). I reran a push-apk job from mozilla-central[1]. m-c doesn't define google_play_strings.json in upstream artifacts yet. This means, m-b and m-r will still able to run even if attachment 8933626 [details] lands on m-c. In other words, the remaining tasks are:
1. un-bitrot attachment 8933626 [details]
2. test the taskgraph output against m-c, m-b, m-r and maple
3. perform a staging release on maple to make sure pushapk can deal with google-play-strings.json whether the uppstream task passed or not. 

[1] https://tools.taskcluster.net/groups/cJ_dBavXSym5ysDswMFrtw/tasks/e8aXVZm2R4qBD2GQEeq-Cg/runs/1/logs/public%2Flogs%2Fchain_of_trust.log -- The job failed because the APK is already uploaded on Google Play (which is true). This is the last step of the pushapk task, meaning the rest of the task passed fine.

Comment 24

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8933626 [details]
Bug 1385401 - Fetch Google Play listings outside of the push-apk task

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

Comment 25

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8933626 [details]
Bug 1385401 - Fetch Google Play listings outside of the push-apk task

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

Comment 26

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8933626 [details]
Bug 1385401 - Fetch Google Play listings outside of the push-apk task

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

Comment 27

[Johan Lorenzo [:jlorenzo]]

(In reply to Johan Lorenzo [:jlorenzo] from comment #23)
> 1. un-bitrot attachment 8933626 [details]
Done. I also added Ben's patch (comment 19)

> 2. test the taskgraph output against m-c, m-b, m-r and maple
I used taskcluster-diff[1]. No failure in any branch. The docker image gets in every branch, which I think is expected. Otherwise, the task that fetches strings gets pulled only in the promote/ship/nightly graphs. 

Note: I removed the default value of "requires" in [2]. The default value was added by default in the first version of my patch. This caused a bigger payload to send to TC and it was harder to read the diff. In order to make next merge easier, I applied the changes to maple too[3]. 

> 3. perform a staging release on maple to make sure pushapk can deal with
> google-play-strings.json whether the upstream task passed or not. 
* Happy path: https://tools.taskcluster.net/groups/CGFP7G8MQ8uVCTpaLSl8nw/tasks/BzroCcnMTku3mglq-Rp1iA/runs/5/logs/public%2Flogs%2Flive_backing.log#L9
* No google_play_strings.json found: https://tools.taskcluster.net/groups/aJRvGhmNS5qJzcZ8iTxYfw/tasks/QTC1wCk2Tn-m4GdAzsMOoA/runs/2/logs/public%2Flogs%2Flive_backing.log#L8

Note: I had to comment out[4] directly on dep-pushapkworker, because releases keys signed the APKs (dep keys used to). Ben said on IRC that we may want to switch back to dep signing. Releases keys were used to make Update Verify tests happy on desktop.


In a nutshell, tests were done. yaml lint and pyflake are passing locally. I don't expect a decision task to be broken. I don't expect a push-apk task to fail as well. Let's land it!


[1] https://hg.mozilla.org/build/braindump/file/f1c6916868bd/taskcluster/taskgraph-diff
[2] https://reviewboard.mozilla.org/r/204570/diff/3-4/
[3] https://hg.mozilla.org/projects/maple/rev/50465d4b211d34ed5ccb4e7aeb23f3531853f4bf
[4] https://github.com/mozilla-releng/pushapkscript/blob/f7f7dadd0cda3827eb6fd49d8e3d4228a26c4007/pushapkscript/script.py#L48-L49

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a32acea9d091
Fetch Google Play listings outside of the push-apk task r=aki

Comment 30

[Arthur Iakab [arthur_iakab]]

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

Comment 31

[Johan Lorenzo [:jlorenzo]]

Crap, mozilla-central got broken[1] because the docker-worker signature couldn't be verified[2]. It couldn't be caught earlier. That's the only difference between the dep and the prod instance of pushapkworker :/

I'm investigating why the public key of this AWS AMI missing. If I don't find something before next Android nightly (tomorrow 10am UTC), I'll backout comment 30.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=ece8a96dfaa436c9bcf53105877b3923a264ae31&filter-searchStr=push-apk&selectedJob=154535128
[2] https://tools.taskcluster.net/groups/aAhpETT2R5a-dET48TIqFA/tasks/AIGCulLKTxaH2hmfmgNHDw/runs/0/logs/public%2Flogs%2Fchain_of_trust.log#L56

Comment 32

[Johan Lorenzo [:jlorenzo]]

gpg_key_repo on pushapkworker-1 is at the most recent release[1]. That makes me wonder if the workerType taskcluster-generic is allowed to run under Chain Of Trust. Does that sound plausible, Aki? If so, do you think we should add the pub key of taskcluster-generic AMI (if one exists) to [2]? Otherwise, should we use a different worker-type? 

[1] https://github.com/mozilla-releng/cot-gpg-keys/releases/tag/production-20171213162233
[2] https://github.com/mozilla-releng/cot-gpg-keys/tree/master/docker-worker

Comment 33

[Aki Sasaki (not active)]

The docker-worker workerTypes with valid keys are:

- decision
- docker-image
- all level 3 build workerTypes

and that's it. This is intentional. If one of those works for you, e.g. the android l3 build workerType, please use that. If not, let's investigate why; if needed, we can ask the taskcluster team to add another workerType to the list.

Comment 34

[Johan Lorenzo [:jlorenzo]]

Thanks for the info, Aki. If I remember correctly, the android build worker didn't suit because it fetched the hg repo, which is not necessary for that task. I'll have a closer look at it next week.

In the meantime, I backed out comment 30: https://hg.mozilla.org/mozilla-central/rev/814510bf944793895b0e7b6bcb137314a9fe650f.

Comment 35

[Aki Sasaki (not active)]

I might be wrong, but I'm under the impression that the android build workerType doesn't require the tree. The task definition has that info: https://tools.taskcluster.net/groups/cecmymuWTM6Xsr9GWvrE5w/tasks/ZGY3un2jQr-j33lf4kSU1g/details

If you use a different task.payload.command and different docker image, I believe it should work.

Comment 36

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8933626 [details]
Bug 1385401 - Fetch Google Play listings outside of the push-apk task

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

Comment 37

[Johan Lorenzo [:jlorenzo]]

(In reply to Aki Sasaki [:aki] from comment #35)
Looks like you're right :D

I just changed the worker type (in my patch) to aws-provisioner-v1/gecko-{level}-b-android. Maple seems happy about it[1]. The task didn't fetch any tree or cache. It just downloaded the right docker image and google_play_strings.json. I'll reland the patch. Chain Of Trust shouldn't be broken anymore, now that chainOfTrust.json.asc is signed by a known worker type.

[1] https://tools.taskcluster.net/groups/PyMdQeK2QG-jUr8tc70pJA/tasks/WC7Sh6eATaeJpi5b-2j0ww/details

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/310b03d60222
Fetch Google Play listings outside of the push-apk task r=aki

Comment 39

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/310b03d60222

Comment 40

[Johan Lorenzo [:jlorenzo]]

It worked!
* https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=7c9de87c6eb02d7448297bff489f27cb20c33d24&filter-searchStr=pub&selectedJob=154986212
* https://tools.taskcluster.net/groups/OYfOhwaRSQ6aHwM1t74uQQ/tasks/W2ake869STihiPC6DHW5FQ/details

I won't uplift this patch to mozilla-beta, as it will ride the train by the end of the week.