ReleaseRunner support for Fennec

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sfraser, Assigned: sfraser)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
Posted patch release-runner.diff (obsolete) — Splinter Review
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.
Attachment #8835500 - Flags: review?(rail)
Assignee: nobody → sfraser
Posted patch release-runner-puppet.diff (obsolete) — Splinter Review
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.
Attachment #8835508 - Flags: review?(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.
Attachment #8835508 - Flags: review?(rail)
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 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.
Attachment #8835500 - Flags: review?(rail)
(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.
updated diff to avoid use of .get()
Attachment #8835559 - Flags: review?
Comment hidden (mozreview-request)

Comment 9

2 years ago
mozreview-review
Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

https://reviewboard.mozilla.org/r/111268/#review113298
Attachment #8835577 - Flags: review?(rail) → review+

Updated

2 years ago
Attachment #8835559 - Flags: review? → review+

Updated

2 years ago
Attachment #8835500 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8835508 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 11

2 years ago
mozreview-review
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
Attachment #8835577 - Flags: review+ → review-
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8835577 [details]
bug 1338150 update config for last few errors

https://reviewboard.mozilla.org/r/111268/#review113354
Attachment #8835577 - Flags: review?(rail) → review+
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
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.
Attachment #8835577 - Flags: review?(jlorenzo) → review+

Comment 17

2 years ago
mozreview-review
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

2 years ago
mozreview-review
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 :(
Attachment #8836754 - Flags: review?(jlorenzo) → review+
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)
See Also: → 1252333, 1263976
Are betas filtered out with the release patterns? That's in the config file now, so should be easy enough to add.

Updated

2 years ago
Priority: -- → P2
See Also: → 1347635
Simon -- was comment 19 the only reason this hasn't landed?
Flags: needinfo?(sfraser)
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.
Flags: needinfo?(sfraser)
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.
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 on attachment 8835559 [details] [diff] [review]
release-runner-2.diff

These patches landed 6 months ago. Marking them as such to eliminate confusion.
Attachment #8835559 - Flags: checked-in+
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.
Attachment #8835577 - Flags: checked-in+
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.
Attachment #8836754 - Flags: checked-in+
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
Status: NEW → RESOLVED
Closed: 2 years ago
Priority: P2 → P1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.