1424482 - Get rid of balrog_props in beetmoverscript and beetmover jobs

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, enhancement, P2)

Description

[Mihai Tabara [:mtabara]⌚️GMT]

https://trello.com/c/5ck1TKn5/177-replace-balrogpropsjson-with-values-at-graph-generation-time-rather-than-downloading-a-build-artifact

This worked fine while we had nightlies for Linux. Then it gradually evolved to something nasty for all-nightly-platforms but it got extremely hackish for when we started adding releases.

This has to stop now. We need all that information in tree and have beetmover drop this dependency. The more products we need in beetmover, the more hackish this becomes.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Attachment: beetmover PR

Comment 2

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8951196 [details] [review]
beetmover PR

Comments in the PR. Feel free to reset me again when you want me to glance again

Comment 3

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

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

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

https://reviewboard.mozilla.org/r/220560/#review226540


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/transforms/task.py:518
(Diff revision 1)
>  
>          # locale key, if this is a locale beetmover job
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:519
(Diff revision 1)
>          # locale key, if this is a locale beetmover job
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:520
(Diff revision 1)
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:521
(Diff revision 1)
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:522
(Diff revision 1)
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,
> +            'hash-type': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:523
(Diff revision 1)
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,
> +            'hash-type': basestring,
> +            'platform': basestring,

Error: Undefined name 'basestring' [flake8: F821]

Comment 5

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

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

Comment 6

[Code Review Bot [:reviewbot]]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

https://reviewboard.mozilla.org/r/220560/#review226570


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/transforms/task.py:518
(Diff revision 2)
>  
>          # locale key, if this is a locale beetmover job
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:519
(Diff revision 2)
>          # locale key, if this is a locale beetmover job
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:520
(Diff revision 2)
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:521
(Diff revision 2)
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:522
(Diff revision 2)
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,
> +            'hash-type': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:523
(Diff revision 2)
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,
> +            'hash-type': basestring,
> +            'platform': basestring,

Error: Undefined name 'basestring' [flake8: F821]

Comment 7

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

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

Comment 8

[Code Review Bot [:reviewbot]]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

https://reviewboard.mozilla.org/r/220560/#review226896


Code analysis found 6 defects in this patch:
 - 6 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: taskcluster/taskgraph/transforms/task.py:518
(Diff revision 3)
>  
>          # locale key, if this is a locale beetmover job
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:519
(Diff revision 3)
>          # locale key, if this is a locale beetmover job
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:520
(Diff revision 3)
>          Optional('locale'): basestring,
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:521
(Diff revision 3)
>  
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:522
(Diff revision 3)
> +        Required('release-properties'): {
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,
> +            'hash-type': basestring,

Error: Undefined name 'basestring' [flake8: F821]

::: taskcluster/taskgraph/transforms/task.py:523
(Diff revision 3)
> +            'app-name': basestring,
> +            'app-version': basestring,
> +            'branch': basestring,
> +            'build-id': basestring,
> +            'hash-type': basestring,
> +            'platform': basestring,

Error: Undefined name 'basestring' [flake8: F821]

Comment 9

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

https://reviewboard.mozilla.org/r/220560/#review226906

Nice to see tangiable progress here in such a short time, awesome work!

Comment 10

[Mihai Tabara [:mtabara]⌚️GMT]

Not sure if those lint/f8 errors are solved already or not but might be worth running the checks before you consider landing this to central.

Comment 11

[Mihai Tabara [:mtabara]⌚️GMT]

I triggered Fennec/Devedition staging releases to test my checksums changes and noticed two issues:

1. the release-generate-checksums-fennec job has "Firefox" within its payload release props. I'm reusing the functions that were added in this bug so I'm thinking there could be some error here https://hg.mozilla.org/projects/maple/file/tip/taskcluster/taskgraph/transforms/beetmover.py#l459 or we need to add "fennec" as part of that check too.  Because of appName = Firefox, beetmoverscript thinks it's a Firefox job, hence submits the files under the wrong bucket/path.

2. The "beetmover-source-linux64-fennec-source/opt" is also failing, most likely for the same reason as the release-props are the same as above. 


Offtopic: This reminds, we really need to resurrect the conversation with CloudOps to address the passwords / buckets issue.

Comment 12

[Mihai Tabara [:mtabara]⌚️GMT]

Dropping a NI to Johan, to discuss this together on Monday, before I forget :}

Comment 13

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8951308 [details]
Bug 1424482 - beetmover: Get rid of balrog_props in favor of task payload

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

Comment 14

[Johan Lorenzo [:jlorenzo]]

Thank you for finding this out, Mihai!

The diff generated by taskcluster-diff.py is quite big to be humanely parsed. I used a few tricks to make sure I didn't create new platform values for instance, but I had missed this case. I relied on these commands:

> cd build/braindump/taskcluster/taskgraph-diff
> grep 'platform":' json/1424482/*.diff | cut -d':' -f'2-' | sort | uniq 
This checks no new platform was added by printing out all possible type of platforms.

> grep 'appName":' json/1424482/*.diff | cut -d':' -f'2-' | sort | uniq
Same thing with app names

> grep -i 'firefox"' json/1424482/*fennec*.jso
New check: Checks no fennec graph show any Firefox app name.

Comment 15

[Mihai Tabara [:mtabara]⌚️GMT]

All devedition beetmover-repackage jobs are failing since we landed this patch. Turns out the hacks in beetmoverscript are quite ugly and make this difficult to debug.

In the old way of using balrog props, the platform would be set to `linux` whereas now, taken from in-tree is `linux-devedition`, which is correct one, but breaks the other automation.

When it gets in the `update_props()` function, it appends another "devedition" to it so some of the artifacts can't be found in the templating.

I pushed an inoffensive fix for this, https://github.com/mozilla-releng/beetmoverscript/pull/101/commits/f98a0d905398a791f66cb39fd899ff901b3c8e47 will roll-out another dev beetmoverscript.

Comment 16

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1424482 - Deploy new version of beetmoverscript a=versionbump

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

Comment 17

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8954679 [details]
Bug 1424482 - Deploy new version of beetmoverscript a=versionbump

default: https://hg.mozilla.org/build/puppet/rev/cb2aa031c097de4e33842a24592ae8e3d66704b7
prod: https://hg.mozilla.org/build/puppet/rev/1fd7f854d59701195d260dcd8de1b48a8adb590e

Comment 18

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8951196 [details] [review]
beetmover PR

Rebased and landed on top of master at: https://github.com/mozilla-releng/beetmoverscript/commit/821df29851b2dc350f0321103180907fa1d24e70

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d69d6fe4a1b8
beetmover: Get rid of balrog_props in favor of task payload r=mtabara

Comment 20

[Cosmin Sabou [:CosminS]]

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

Comment 21

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1424482 - part 2: Remove upstream artifact when no path is defined r=mtabara a=bustage

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/e33efdb3e151
part 2: Remove upstream artifact when no path is defined r=mtabara a=bustage

Comment 23

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8955112 [details] [diff] [review]
Bug 1424482 - part 2: Remove upstream artifact when no path is defined r=mtabara a=bustage

See comment 22.