1338150 - ReleaseRunner support for Fennec

Status: RESOLVED FIXED

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

Description

[Simon Fraser [:sfraser] ⌚️GMT]

build/tools/buildfarm/release/release-runner.py needs to support Fennec:

* Ability to check for Fennec releases
* Check revision exists
* Skip other sanity tests (partials, l10n)

out of scope: adjust graph submission for fennec, this is only adjustment for sanity tests.

Comment 1

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: release-runner.diff

So the basis for this patch is to allow the features we need, but not to enable them until:

a) The config file is updated
b) The task graph submission part has also been added

This diff goes in tandem with the proposed puppet patch, being attached next.

Comment 2

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: release-runner-puppet.diff

Add in the new .yml format config file, which will allow us to use a config file rather than code to adjust which sanity checks are done for which product.

Don't remove the .ini file at the moment, so there's no race condition between puppet and the changes to build/tools/ - both config files will be there, then the script will change to use the new one, then we can clear up the old config file.

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8835508 [details] [diff] [review]
release-runner-puppet.diff

Review of attachment 8835508 [details] [diff] [review]:
-----------------------------------------------------------------

This one should just work. A nit below.

::: modules/releaserunner/manifests/init.pp
@@ +69,2 @@
>              group   => "${users::builder::group}",
>              content => template("releaserunner/release-runner.ini.erb"),

Can you also remove the old config file and its template? It'd be a bit confusing to have 2 of them.

Comment 4

[Simon Fraser [:sfraser] ⌚️GMT]

Not immediately, no - it means there would be a race.  Puppet has to work at the exact moment the new code lands, or release-runner.py will fail to start. If we leave both in place, it'll work with the old config until the new code is deployed, then use the new config, then we can remove the original.

Comment 5

[Rail Aliiev [:rail]]

Comment on attachment 8835500 [details] [diff] [review]
release-runner.diff

Review of attachment 8835500 [details] [diff] [review]:
-----------------------------------------------------------------

Wow! I like it! Much better to use YAML, than old and crufty ini format!

::: buildfarm/release/release-runner.py
@@ +70,5 @@
>      new_valid_releases = []
> +
> +    # results in:
> +    # { 'firefox': ['long_revision', 'l10n_changesets', 'partial_updates']}
> +    checks = {r['product'].lower(): r['checks'] for r in releases_config}

Does this require ship-it changes?

@@ +293,5 @@
>  
>      # Shorthand
> +    api_root = config['api'].get('api_root')
> +    username = config['api'].get('username')
> +    password = config['api'].get('password')

Can you get the values without using `get()` so we fail early. Setting them to None doesn't make too much sense... The same applies for other values I think.

Comment 6

[Simon Fraser [:sfraser] ⌚️GMT]

(In reply to Rail Aliiev [:rail] ⌚️ET from comment #5)

> ::: buildfarm/release/release-runner.py
> @@ +70,5 @@
> >      new_valid_releases = []
> > +
> > +    # results in:
> > +    # { 'firefox': ['long_revision', 'l10n_changesets', 'partial_updates']}
> > +    checks = {r['product'].lower(): r['checks'] for r in releases_config}
> 
> Does this require ship-it changes?

It uses the same api as the existing version of the script, so it won't need changes to ship-it unless you'd prefer we move the list of checks to perform out of the .yml file and into ship-it


> 
> @@ +293,5 @@
> >  
> >      # Shorthand
> > +    api_root = config['api'].get('api_root')
> > +    username = config['api'].get('username')
> > +    password = config['api'].get('password')
> 
> Can you get the values without using `get()` so we fail early. Setting them
> to None doesn't make too much sense... The same applies for other values I
> think.

Certainly can! I was trying to copy the .ini get_config() closely - perhaps too closely, your suggestion's definitely an improvement.

Comment 7

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: release-runner-2.diff

updated diff to avoid use of .get()

Comment 8

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: bug 1338150 update config for last few errors

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

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

https://reviewboard.mozilla.org/r/111268/#review113298

Comment 10

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

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

Comment 11

[Rail Aliiev [:rail]]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

https://reviewboard.mozilla.org/r/111268/#review113324

::: modules/releaserunner/manifests/services.pp:13
(Diff revision 2)
>  
>      supervisord::supervise {
>          "releaserunner":
> -            command      => "${releaserunner::settings::tools_dst}/buildfarm/release/release-runner.sh ${releaserunner::settings::root} ${releaserunner::settings::logfile} ${releaserunner::settings::root}/release-runner.ini",
> +            command      => "${releaserunner::settings::tools_dst}/buildfarm/release/release-runner.sh ${releaserunner::settings::root} ${releaserunner::settings::logfile} ${releaserunner::settings::root}/release-runner.yml",
>              user         => $::config::builder_username,
> -            require      => [ File["${releaserunner::settings::root}/release-runner.ini"],
> +            require      => [ File["${releaserunner::settings::root}/release-runner.yml"],

This won't work for bm81, which uses the `old-release-runner` branch of the tools repo. We'll need to add a new variable and make it per master. I'd suggest to modify dev/prod and explicitly set the config name to yml, and add "old" env based on "prod" and set the config name to ini. Then change https://hg.mozilla.org/build/puppet/file/tip/manifests/moco-nodes.pp#l536 to use "old" and pass it using this approach https://hg.mozilla.org/build/puppet/file/tip/modules/releaserunner/manifests/init.pp#l17

Comment 12

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

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

Comment 13

[Rail Aliiev [:rail]]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

https://reviewboard.mozilla.org/r/111268/#review113354

Comment 14

[Simon Fraser [:sfraser] ⌚️GMT]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

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

Comment 15

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

https://reviewboard.mozilla.org/r/111268/#review113364

vim didn't show anything suspicious when color syntax was set to yaml then erb. I didn't see any other missing =. This looks good to me.

Comment 16

[Simon Fraser [:sfraser] ⌚️GMT]

Attachment: bug 1338150 swap to using .get() so previous 'None' behaviour continues

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

Comment 17

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8836754 [details]
bug 1338150 swap to using .get() so previous 'None' behaviour continues

https://reviewboard.mozilla.org/r/112098/#review113378

Looks good to me

Comment 18

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8836754 [details]
bug 1338150 swap to using .get() so previous 'None' behaviour continues

https://reviewboard.mozilla.org/r/112098/#review113638

For some reasons, the r+ didn't made my last comment :(

Comment 19

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8835559 [details] [diff] [review]
release-runner-2.diff

Review of attachment 8835559 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/python/kickoff/__init__.py
@@ +16,5 @@
>  # regex beta tracking bug is 1252333,
>  # regex release tracking bug is 1263976
> +# RELEASE_PATTERNS = [
> +#     r"Firefox-.*",
> +#     r"Fennec-.*",

I noticed while writting bug 1340548, we may only want to filter out betas, for now.

That's what was done at the time of Firefox relpro (see bug 1252333 and bug 1263976)

Comment 20

[Simon Fraser [:sfraser] ⌚️GMT]

Are betas filtered out with the release patterns? That's in the config file now, so should be easy enough to add.

Comment 21

[Aki Sasaki (not active)]

Simon -- was comment 19 the only reason this hasn't landed?

Comment 22

[Simon Fraser [:sfraser] ⌚️GMT]

I think it was, I waited for an answer then lost track of the bug. I don't know if the diff is still good, or indeed what it does any more, I'll need to re-read it all.

Comment 23

[Mihai Tabara [:mtabara]⌚️GMT]

FWIW, I'm also about to resume work on this in bug 1347635 this week (+ other Fennec related issues from tcmigration), now that I'm off releaseduty. Please let me know if anyone else is working on this so that I don't step on others toes. Happy to help or defer, but just wanted to clarify.

Comment 24

[Mihai Tabara [:mtabara]⌚️GMT]

Sounds like:

a) bm81 (old release runner) expectedly ignores Fennec because of regex[1]
b) bm83/bm85 (relpro release runner) expectedly ignore Fennec because of puppet yml file[2]

We might try on bm83 a staging release of Fennec to see if indeed all the pieces nicely glue together.
I'll dig for more in the meantime to see the code changes.

[1]: https://hg.mozilla.org/build/tools/file/old-release-runner/buildfarm/release/release-runner.py#l39
[2]: https://hg.mozilla.org/build/puppet/file/tip/modules/releaserunner/templates/release-runner.yml.erb#l63

Comment 25

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8835559 [details] [diff] [review]
release-runner-2.diff

These patches landed 6 months ago. Marking them as such to eliminate confusion.

Comment 26

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

These patches landed 6 months ago. Marking them as such to eliminate confusion.

Comment 27

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8836754 [details]
bug 1338150 swap to using .get() so previous 'None' behaviour continues

These patches landed 6 months ago. Marking them as such to eliminate confusion.

Comment 28

[Mihai Tabara [:mtabara]⌚️GMT]

I think we can declare victory here. Fennec support in release runner has been already added 6 months ago when patches from this bug landed. 

Leftovers:
* continue with missing bbconfigs and possibly other misc in bug 1347635
* perform a staging release to test everything in bug 1390491 (this means we will uncomment puppet[1] specifically on bm83 and not land that in production)
* declare victory and test in Fennec beta/release

Thanks again for work in this.

[1]: https://hg.mozilla.org/build/puppet/file/tip/modules/releaserunner/templates/release-runner.yml.erb#l63