Closed Bug 1300707 Opened 8 years ago Closed 8 years ago

Expose release sanity errors to ship-it status

Categories

(Release Engineering :: Applications: Shipit, defect)

Product:
Release Engineering
Component:
Applications: Shipit
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: Sylvestre, Assigned: jlorenzo)

References

Details

Attachments

(1 file)

Bug 1300707 - Expose release sanity errors to ship-it status
58 bytes, text/x-review-board-request
rail
: review+
Details 
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).
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
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
Closed: 8 years ago
Resolution: --- → WONTFIX
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
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.
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 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)
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.
See Also: → 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+
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 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+
https://hg.mozilla.org/build/tools/rev/1005cd769d3555db50badbefe67bad08668cce77
Bug 1300707 - Expose release sanity errors to ship-it status r=rail
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
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Component: Applications: ShipIt (backend) → Applications: ShipIt
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: