1091084 - b2g bumper assumes that a git ls-remote will return content

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Pete Moore [:pmoore][:pete]]

If a remote does not have a given head or tag, the git ls-remote for that head or tag will return no content. This breaks b2g bumper - it should be able to handle this situation gracefully.

Comment 1

[Pete Moore [:pmoore][:pete]]

See e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1090885#c10

Comment 2

[Chris AtLee [:catlee]]

Attachment: bumper-.patch.2

Comment 3

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8513724 [details] [diff] [review]
bumper-.patch.2

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

::: scripts/b2g_bumper.py
@@ +234,5 @@
>                  failed.append(p)
>              # Save to our cache
>              self._git_ref_cache[remote_url, revision] = abs_revision
>              p.setAttribute('revision', abs_revision)
>          if abort:

Since previously resolve_git_ref(self, remote_url, revision) could not return None, I guess this is the first time we have the opportunity to execute lines 239-252 (since abort is only True if abs_revision is None, which was not possible before).

Were you able to test locally that the generated log messages made sense and were accurate?

I see on line 252 that we will now call self.fatal(log_message) so we will never move past massage-manifests action. Maybe a less brutal way to proceed might be check in the manifest with an xml comment on the line with the problem, that the remote head/tag cannot be found, and details of the git ls-remote command that can be run to show this, both for the source repo, and the mirrored one?

The reason I suggest this is that the build will in any case break, but anyone looking at the commit that broke it from b2g bumper can see exactly what the problem is. Also there may be cases where it does not break the build, because the repo has been added but is not yet depended on (in the case a dev first adds the dependency, and then goes on to reference it later).

For example, it could write something like:

<!--

<project name="platform/external/fsck_msdos" path="external/fsck_msdos" revision="???"/>

Revision could not be determined from git mirror (no result returned):
git ls-remote https://git.mozilla.org/external/sprd-aosp/platform/external/fsck_msdos.git refs/tags/android-4.0.4_r2.1

If revision exists in source repo, but not in mirror, suspect vcs sync issue. Source repo:
git ls-remote http://sprdsource.spreadtrum.com:8085/b2g/android/platform/external/fsck_msdos.git refs/tags/android-4.0.4_r2.1

If revision also does not exist in source repo, please update b2g-manifest reference:
https://github.com/mozilla-b2g/b2g-manifest/blob/master/tarako.xml#L46

-->


The reason I suggest putting this debug information directly in the generated xml file is that:

a) it can continue processing, and does not need to "fatal" out, so other bumper steps for other repos can complete, and
b) it means that no developer needs to worry about knowing how to receive notifications from b2g bumper, or how it works, or where it is looking, or where it writes log files etc - as soon as build breaks, this changeset will be part of the push that broke the build, and everyone can see inline what the problem is, and b2g bumper carries on with its business, without alerting in nagios etc, so it becomes a sheriffing issue only, rather than needing the involvement of releng, or forwarding of bumper bot emails between releng and devs/sheriffs etc.

Maybe this is a bit of scope creep from the original bug. :D

I think the extra effort of fine tuning the debugging output will really save time in future iterations of troubleshooting though, so could be worth the effort.

Comment 4

[Pete Moore [:pmoore][:pete]]

And by the way, thanks for fixing this! I was going to work on it today, but you beat me to it! :)

Comment 5

[Chris AtLee [:catlee]]

I don't think putting this information in the XML is a good idea. Spurious errors will trigger broken builds. We should instead figure out how to expose the errors in a more public way.

Comment 6

[Pete Moore [:pmoore][:pete]]

(In reply to Chris AtLee [:catlee] from comment #5)
> I don't think putting this information in the XML is a good idea. Spurious
> errors will trigger broken builds. We should instead figure out how to
> expose the errors in a more public way.

We can test for this - we should be able to see if:
a) we can't reach the git server
b) we can reach it, but the head/tag is not available

We should also be able to check if this is a newly added entry, or a preexisting one - this helps troubleshoot the two scenarios:
1) a remote branch/tag is added to a manifest, that does not exist
2) a remote branch/tag is removed from a third party repository (although this should not be possible, since we don't delete branches/tags from our mirrors)

Again we had a tree closure yesterday, and the notification first had to be processed by releng based on alerts in #buildduty - if we already had the manifest getting updated, and a build breaking tied to the source change in-tree, we maybe could have avoided tree closures, and releng would not need to have been informed. So even when we don't break builds (by not pushing the manifest in case of error) we still don't avoid tree closures.

I'd like to prepare a patch which concisely reports the problem back as a comment in the manifest, and only commits/pushes this manifest file back to hg if it is sure the problem is not an intermittent connectivity issue, but a genuine case of the manifest referring to a non-existing branch/tag, which would always need developer involvement to resolve.

Alternatively, we could have some sort of travis CI on b2g-manifests which we require devs to enable on their own forks before issuing pull requests.

Or we could do both of the above! :)

Comment 7

[Release Engineering SlaveAPI Service]

In production: https://hg.mozilla.org/build/mozharness/rev/6375288c157b

Comment 8

[Pete Moore [:pmoore][:pete]]

Attachment: bug1091084_mozharness_v1.patch

These mozharness changes are a prerequisite to being able to run b2g bumper from travis.

Comment 9

[Pete Moore [:pmoore][:pete]]

Comment on attachment 8519158 [details] [diff] [review]
bug1091084_mozharness_v1.patch

Actually, I'm going to do this in another bug...

Obsoleting here...