1300754 - [release-runner] Report error when en-US artifacts are never uploaded

Status: RESOLVED FIXED

(Release Engineering :: Release Automation, defect)

Description

[Johan Lorenzo [:jlorenzo]]

Follow up Bug #1288573

Like said in bug 1288573 comment 17, a release may be stuck for ever if one of artifacts hasn't been published. We should have a better error handling for this scenario.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Attachment: Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

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

Comment 2

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

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

Comment 3

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

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

Comment 4

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

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

Comment 5

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

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

Comment 6

[Johan Lorenzo [:jlorenzo]]

I manually tested with builds that are already in CI. For the case where we actually wait, I'd prefer to wait on bug 1301380. Otherwise people will get notifications on the production mailing list.

In the meantime, I wrote tests in order to get better confidence about the changes done to are_en_us_builds_completed(). It now stores the data gotten from Taskcluster and don't ask for them anymore. This allows to register when the first build is declared as completed, which leads to a timeout after 1 day.

Comment 7

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

https://reviewboard.mozilla.org/r/77664/#review76242

::: tox.ini:15
(Diff revision 5)
>      jinja2==2.6
>      mock==1.0.1
>      webob==1.2.3
>      gevent==0.13.8
>      IPy==0.81
> +    taskcluster==0.3.4

That's probably not the best place to put this type of dependency, but I haven't found the right spot :S

Comment 8

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

Per discussion on #promotion, :mtabara may take over the review. I just couldn't neither r? nor NI him.

Comment 9

[Rail Aliiev [:rail]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

https://reviewboard.mozilla.org/r/77664/#review76692

In overall it looks sane, even though something tells me that you used classes just to keep some state between iterations in the loop. Maybe you can use LRU cache instead?

::: lib/python/kickoff/build_status.py:16
(Diff revision 5)
> +
> +
>  # TODO: Bug 1300147. Avoid having 7 parameters by using a release object that contains only what's needed.
>  def are_en_us_builds_completed(index, queue, release_name, branch, revision, tc_product_name, platforms):
>      try:
> -        tasks_to_watch = [
> +        watcher = _BUILD_WATCHERS[release_name]

If I read this correctly, this block tries to reuse cached objects between runs. If release runner exits, there is no value of keeping the state. Can you make sure we don't exit between runs? This is not a blocker, but if the process exits, you could simplify the code. Also what happens to the time if the process is reastarted? Maybe compare `_now` to `submittedAt`?

::: lib/python/kickoff/build_status.py:47
(Diff revision 5)
> +        self.taskcluster_product_name = tc_product_name
> +
> +        self.release_name = release_name
> +        self.branch = branch
> +        self.revision = revision
> +        self.task_per_platforms = {p: None for p in platforms}

This should probably be "per platform" (singular)

::: lib/python/kickoff/build_status.py:51
(Diff revision 5)
> +        self.revision = revision
> +        self.task_per_platforms = {p: None for p in platforms}
> +
> +        self._timeout_watcher = TimeoutWatcher()
>  
> -    log.debug('All tasks have been found: %s', tasks_to_watch)
> +    @property

Something tells me that this shouldn't be a property. It does a lot of networking. Not a big deal though.

::: lib/python/kickoff/build_status.py:56
(Diff revision 5)
> -    log.debug('All tasks have been found: %s', tasks_to_watch)
> -    return _are_all_tasks_completed(queue, tasks_to_watch)
> +    @property
> +    def are_builds_completed(self):
> +        if self._timeout_watcher.has_timed_out:
> +            raise TimeoutWatcher.TimeoutError(self.release_name, self._timeout_watcher.start_timestamp)
>  
> +        self._fetch_missing_tasks()

"fetch missing tasks" is a bit misleading. Trying to imagine my future self in 6 months. :) I'd probably rename this to `fetch_tasks` - I don't need to know about the internals (the method fetches only not completed tasks, what is only an optimization).

::: lib/python/kickoff/build_status.py:83
(Diff revision 5)
> +
> +    @property
> +    def _platforms_with_no_task(self):
> +        return [platform for platform, task in self.task_per_platforms.iteritems() if task is None]
> +
> +    def _fetch_latest_state_of_non_completed_tasks(self):

I think this method is redundant - according to https://docs.taskcluster.net/reference/core/index "service that indexes successfully completed tasks".

::: lib/python/kickoff/build_status.py:117
(Diff revision 5)
> +            platform: task for platform, task in self._existing_task_per_platforms.iteritems()
> +            if task['state'] == 'completed'
> +        }
> +
> +    @property
> +    def _existing_task_per_platforms(self):

I think it should be "per platform" (singular).

::: lib/python/kickoff/build_status.py:128
(Diff revision 5)
> +    def _start_timeout_watcher_if_needed(self):
> +        completed = self._completed_task_per_platforms
> +
> +        if len(completed) > 0:
> +            try:
> +                self._timeout_watcher.start_if_not_already_started()

I'd rename this to be just `start()` because you are catching `AlreadyStartedError`, so it's clear that you shouldn't start it twice. Also `start_if_...` makes me think that it shouldn't throw any exceptions, because the name conatins "if".

::: lib/python/kickoff/build_status.py:159
(Diff revision 5)
> +            raise self.AlreadyStartedError()
> +
> +        self.start_timestamp = self._now()
> +
> +    @property
> +    def _has_started(self):

I'd rename it to `_started`.

::: lib/python/kickoff/build_status.py:167
(Diff revision 5)
> +    @staticmethod
> +    def _now():
> +        return datetime.utcnow()
> +
> +    @property
> +    def has_timed_out(self):

Similar here, `timed_out`

::: lib/python/kickoff/test/test_build_status.py:19
(Diff revision 5)
> +        self.revision = 'abcdef123456'
> +        self.tc_product_name = 'firefox'
> +        self.platforms = ('linux', 'win32', 'win64')
> +
> +
> +class AreEnUsBuildsCompletedTest(BuildsCompletedBase):

Tests!!!! Woot! I think, you will end up reducing the amount of tests after you address the comments above ;)

::: tox.ini:15
(Diff revision 5)
>      jinja2==2.6
>      mock==1.0.1
>      webob==1.2.3
>      gevent==0.13.8
>      IPy==0.81
> +    taskcluster==0.3.4

I think this is OK, but 0.3.4 is not what we use in production, see http://hg.mozilla.org/build/puppet/file/tip/modules/releaserunner/manifests/init.pp#l58

Comment 10

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

https://reviewboard.mozilla.org/r/77664/#review76692

> If I read this correctly, this block tries to reuse cached objects between runs. If release runner exits, there is no value of keeping the state. Can you make sure we don't exit between runs? This is not a blocker, but if the process exits, you could simplify the code. Also what happens to the time if the process is reastarted? Maybe compare `_now` to `submittedAt`?

Done. We now compare `_now` to when the earliest task was resovlved.

I cached the tasks between runs, in order to lower the number of network requests and get less logs.

> Something tells me that this shouldn't be a property. It does a lot of networking. Not a big deal though.

I agree. I wasn't sure about it either. It's not a property anymore

> "fetch missing tasks" is a bit misleading. Trying to imagine my future self in 6 months. :) I'd probably rename this to `fetch_tasks` - I don't need to know about the internals (the method fetches only not completed tasks, what is only an optimization).

Renamed into `_fetch_completed_tasks()`

> I think this method is redundant - according to https://docs.taskcluster.net/reference/core/index "service that indexes successfully completed tasks".

Nice! I removed that function

> I'd rename this to be just `start()` because you are catching `AlreadyStartedError`, so it's clear that you shouldn't start it twice. Also `start_if_...` makes me think that it shouldn't throw any exceptions, because the name conatins "if".

My bad, that name was an artifact of a previous revision where no exception was thrown. It's now named `start()`

Comment 11

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77664/diff/5-6/

Comment 12

[Rail Aliiev [:rail]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

https://reviewboard.mozilla.org/r/77664/#review76952

::: lib/python/kickoff/build_status.py:71
(Diff revisions 5 - 6)
> +                # Tasks are always completed if they are referenced in the index
> +                # https://docs.taskcluster.net/reference/core/index
>                  task_id = task_for_revision(
>                      self.taskcluster_index, self.branch, self.revision, self.taskcluster_product_name, platform
>                  )['taskId']
> +                resolved_timestamp = self.taskcluster_queue.status(task_id)['status']['runs'][-1]['resolved']

What happens to `resolved_datetimes` if you have zero tasks ready at some point? Or if the builds are red across all platforms, so you don't get any `resolvedAt` set?

Comment 13

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77664/diff/6-7/

Comment 14

[Rail Aliiev [:rail]]

Comment on attachment 8789448 [details]
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded

https://reviewboard.mozilla.org/r/77664/#review77022

Wheeeee! Ship-it! Thank you for you patience. :)

Comment 15

[Johan Lorenzo [:jlorenzo]]

https://hg.mozilla.org/build/tools/rev/be22c551f172f280ada5e75074e53448f8e9b00c
Bug 1300754 - [release-runner] Report error when en-US artifacts are never uploaded r=rail

Comment 16

[Johan Lorenzo [:jlorenzo]]

Pushed to default (comment 15) and updated bm8{3,5} with [1]. Thank you rail for the proofreading and the suggestions :D 

[1] https://github.com/mozilla-releng/build-ansible/compare/master...rail:relpro