Expose release sanity errors to ship-it status

RESOLVED FIXED

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sylvestre, Assigned: jlorenzo)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
when entering the changeset in the form, we should check if the changeset exists on the branch or not.

I don't know if we have something for that (besides parsing the hg page).
(Assignee)

Comment 1

2 years ago
I don't think we need to parse the hg page, just getting the HTTP status code would be enough. For instance, [1] returns a nice 404.

[1] http://hg.mozilla.org/releases/mozilla-beta/rev/nonexisting
(Assignee)

Comment 2

2 years ago
Note: https://hg.mozilla.org/releases/mozilla-beta/json-rev/nonexisting returns a correct result with a smaller network footprint.
We do have a check *after* we start a release (not in the UI): http://hg.mozilla.org/build/tools/file/tip/lib/python/kickoff/sanity.py#l171. There is also an additional check to make sure that the in-tree display version corresponds to ship-it version: http://hg.mozilla.org/build/tools/file/tip/lib/python/kickoff/sanity.py#l183

Let's not duplicate these tests in the UI.

BTW, the tests above (actually the second one) caught 2 issues yesterday and refused to start a release process.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 4

2 years ago
yes but this needs your help while if we do that in ship-it, the issue would be catch by the release managers directly.

By the way, the current error message is cryptic. Having something like "Changeset 'xx' does not exist in branch 'foo/bar'"
Do you want to port release sanity to JS? :P
(Reporter)

Comment 6

2 years ago
no, I am sure Johan wants to do it :p
Let's reopen this bug and morph the title.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: ship it should check if the changeset exists or not → Expose release sanity errors to ship-it status
BTW, we can probably run some sanity checks before the binaries are ready. For example, all revision and l1n0 related checks. Not sure if this is a good idea to repeat them over and over again.

As a possible solution we can cache the results (in the DB?) or implement these checks in v2 better.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → jlorenzo
Comment on attachment 8788450 [details]
Bug 1300707 - Expose release sanity errors to ship-it status

https://reviewboard.mozilla.org/r/76954/#review75076
Attachment #8788450 - Flags: review?(rail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

2 years ago
Comment on attachment 8788450 [details]
Bug 1300707 - Expose release sanity errors to ship-it status

Due to the infinite loop happening at [1], the patch has changed a lot. Like agreed, we now have 2 sets of sanity checks:
1. "prebuild" also split into 2 (revisions and l10n checks)
2. "postbuild"

The current implementation has a limitation: If a build is not ready, we perform the prebuild checks at each iteration. I'm not sure if we can live with that for now. In order to fix that, I'd recommend to store that the prebuild checks have already been done. A good way to store that would be in a release class that wraps up the release object we get from ship-it (bug 1300147). If we want to go that way, I'd prefer handling it in bug 1300147.

What do you think :rail? 


[1] https://hg.mozilla.org/build/tools/file/4e0eca3d8476/buildfarm/release/release-runner.py#l296
Attachment #8788450 - Flags: review+ → feedback?(rail)
(Assignee)

Comment 22

2 years ago
mozreview-review
Comment on attachment 8788450 [details]
Bug 1300707 - Expose release sanity errors to ship-it status

https://reviewboard.mozilla.org/r/76954/#review75484

::: buildfarm/release/release-runner.py:61
(Diff revision 10)
>      'i686.tar.bz2',
>      'x86_64.tar.bz2',
>  ])
>  
>  
> +def get_prebuild_sanity_checked_releases(release_runner):

I didn't put this function in ReleaseRunner, based on the fact that we shouldn't really use rr.mark_as_failed() in it.

After some thoughts, it may be better to use a Release class that exposes mark_as_failed().

::: buildfarm/release/release-runner.py:86
(Diff revision 10)
> +
> +def assign_l10n_changesets(release_runner, releases):
> +    releases_with_l10n = [r for r in releases]
> +
> +    for release in releases_with_l10n:
> +        release['l10n_changesets'] = parsePlainL10nChangesets(release_runner.get_release_l10n(release['name']))

Similarly here, a release class could expose that property and will be in charge of fetching the content of it. That class could also cache the result.
(Assignee)

Updated

2 years ago
See Also: → bug 1300147
Comment on attachment 8788450 [details]
Bug 1300707 - Expose release sanity errors to ship-it status

https://reviewboard.mozilla.org/r/76954/#review76742

I'm verry happy to see that the sanity tests are logically split now! It's much easier to run them separately and chain them. I have some comments/suggestions below. Also you can run more tests before the builds are ready (see below). \o/

::: buildfarm/release/release-runner.py:75
(Diff revision 10)
> +    def is_release_valid(release):
> +        try:
> +            Sanitizer(**release).run()
> +            return True
> +        except Exception as e:
> +            release_runner.mark_as_failed(release, 'Sanity checks failed. Errors: %s' % e)

It's a bit weird to modify the state in a get-function. Also calling `mark_as_failed()` from different files sounds a bit messy. I'd explicitly loope over each release, sanity check them and set as failed in release runner, leaving the helper functinos here.

::: buildfarm/release/release-runner.py:92
(Diff revision 10)
> +
> +    return releases_with_l10n
> +
> +
> +def assign_long_revision(releases):
> +    releases_with_long_rev = [r for r in releases]

I don't think you need to (shallow) copy here, the release objects are mutable.

::: buildfarm/release/release-runner.py:94
(Diff revision 10)
> +
> +
> +def assign_long_revision(releases):
> +    releases_with_long_rev = [r for r in releases]
> +    for release in releases_with_long_rev:
> +        release['mozillaRevision'] = long_revision(release['branch'], release['mozillaRevision'])

What happens if the revision doesn't exist? You'll probably get a 404 and an uncaught exception here. Probably you should run `test_versions_repo_and_revision_check` before you assign long revision.

::: lib/python/kickoff/sanity/base.py:93
(Diff revision 10)
> +        self.kwargs = kwargs
> +        self.repo_path = self.kwargs['branch']
> +        self.revision = self.kwargs['mozillaRevision']
> +
> +        # TODO avoid duplicating the logic
> +        self.branch = self.kwargs["branch"].split("/")[-1]

I thought you added `branchShortName`, no?

::: lib/python/kickoff/sanity/postbuild.py:23
(Diff revision 10)
> +class PostBuildTestSuite(ReleaseSanitizerTestSuite):
> +    def __init__(self, **kwargs):
> +        ReleaseSanitizerTestSuite.__init__(self, **kwargs)
> +        self.partial_updates = self.kwargs["partial_updates"]
> +
> +    def test_partials_validity(self, result):

This can be move to pre-build tests

::: lib/python/kickoff/sanity/postbuild.py:87
(Diff revision 10)
> +                           version=pversion,
> +                           build_number=buildno,
> +                           url=_url)
> +            self.assertEqual(result, releases_sha, candidate_sha, err_msg)
> +
> +    def test_partials_release_candidate_validity(self, result):

The same here
Attachment #8788450 - Flags: feedback?(rail) → feedback+
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
Comment on attachment 8788450 [details]
Bug 1300707 - Expose release sanity errors to ship-it status

https://reviewboard.mozilla.org/r/76954/#review76742

> It's a bit weird to modify the state in a get-function. Also calling `mark_as_failed()` from different files sounds a bit messy. I'd explicitly loope over each release, sanity check them and set as failed in release runner, leaving the helper functinos here.

Agreed, it does feel weird. Releases are now loop'd over in `run_prebuild_sanity_checks()`. This function lives in release-runner.py. If a sanity check fails here, we mark the release as failed there.

> What happens if the revision doesn't exist? You'll probably get a 404 and an uncaught exception here. Probably you should run `test_versions_repo_and_revision_check` before you assign long revision.

I merged fetches and sanity checks under the same function `check_and_assign_long_revision()`. I did the same for L10n and Partials checks.

> I thought you added `branchShortName`, no?

Let's bring back `branchShortName`.
Comment hidden (mozreview-request)
Comment on attachment 8788450 [details]
Bug 1300707 - Expose release sanity errors to ship-it status

https://reviewboard.mozilla.org/r/76954/#review77058
Attachment #8788450 - Flags: review?(rail) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

2 years ago
https://hg.mozilla.org/build/tools/rev/1005cd769d3555db50badbefe67bad08668cce77
Bug 1300707 - Expose release sanity errors to ship-it status r=rail
(Assignee)

Comment 30

2 years ago
I missed some variables to be initialized in revision 11. Rail saw [1] and agreed to ship it (over IRC).

It's now landed and deployed on bm83 and bm85.

[1] https://reviewboard.mozilla.org/r/76954/diff/11-13/
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.