751066 - Pending and running builds not shown on Thunderbird Try (Build API doesn't correctly distinguish between try and try-comm-central branches)

Status: VERIFIED FIXED

(Release Engineering :: General, defect, P3)

Description

[Mark Banner (:standard8)]

I noticed that we weren't getting pending & running notifications on:

https://tbpl.mozilla.org/?tree=Thunderbird-Try

Digging deeper,

http://builddata.pub.build.mozilla.org/buildjson/builds-pending.js
http://builddata.pub.build.mozilla.org/buildjson/builds-running.js
https://secure.pub.build.mozilla.org/buildapi/pending
https://secure.pub.build.mozilla.org/buildapi/running

all show the pending/running builds for Thunderbird under the "try" branch rather than the "try-comm-central" branch.

Looking at

https://secure.pub.build.mozilla.org/buildapi/self-serve/try
https://secure.pub.build.mozilla.org/buildapi/self-serve/try-comm-central

the builds are listed correctly under the right branches.

Could this be a sql query or something that's not restrictive enough on the branch name?

Comment 1

[Chris AtLee [:catlee]]

Not really sure what's going on here...

Comment 2

[Mark Banner (:standard8)]

I could be wrong, but I suspect there's code that is matching branch name within a field, hence "try" matches "try" and "try-comm-central".

I think this could be one location, but really I'm guessing:

http://hg.mozilla.org/build/buildapi/annotate/226d1165769e/buildapi/model/pushes.py#l46

It may be we need to audit where the branch names are compared/obtained to ensure there is only exact matches going on.

Comment 3

[Chris AtLee [:catlee]]

That was my first thought too, but I don't think the pending/running reports you referred to filter by branch.

Comment 4

[Nick Thomas [:nthomas] (UTC+12)]

I suspect http://hg.mozilla.org/build/buildapi/file/default/buildapi/model/query.py#l27 is the issue.

Comment 5

[Mark Banner (:standard8)]

Attachment: Possible fix

This adjusts the function so that it effectively uses equals rather than startswith.

I've not tested it though, I don't know if you guys have a staging server for this.

Comment 6

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 651662 [details] [diff] [review]
Possible fix

>diff --git a/buildapi/model/query.py b/buildapi/model/query.py
>+    if shortname not in allBranches
>+        return 'Unknown'

Missing a trailing : on the if line. There are some release specific 'branches' like 'release-mozilla-beta-post_signing' which change, but these seem to be unused now so that's OK. Otherwise this checks out for some quick testing. 

Please land when catlee or myself are around; it'll auto deploy itself within 5 minutes.

Comment 7

[Mark Banner (:standard8)]

Attachment: Possible fix v2

Updated the patch, this is ready to land but there was bustage today. I'm not around tomorrow, so feel free to land for me, or if not I'll look at getting it landed on Thursday.

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 651868 [details] [diff] [review]
Possible fix v2

http://hg.mozilla.org/build/buildapi/rev/1dcdf4322301

Comment 9

[Nick Thomas [:nthomas] (UTC+12)]

That's fixed the issue with compile jobs, but there's something else causing issues with tests.

Comment 10

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 651868 [details] [diff] [review]
Possible fix v2

Unfortunately this regressed Firefox tests, setting branches as things like mozilla-aurora-linux-pgo-talos. 

http://hg.mozilla.org/build/buildapi/rev/6a71c42d0895

eg, from https://secure.pub.build.mozilla.org/buildapi/pending

Branch	Revision	Builder name
mozilla-aurora-android-talos	c236df02c24f	Android Tegra 250 mozilla-aurora talos remote-tdhtml
mozilla-beta-android-talos	2df27b18881c	Android Tegra 250 mozilla-beta talos remote-tdhtml
mozilla-esr10-win32-pgo-talos	3011c94976a1	Rev3 WINNT 5.1 mozilla-esr10 pgo talos chrome

Comment 11

[Mark Banner (:standard8)]

So how would we feel about changing the try-comm-central branch name to something like cc-try or tb-try-comm-central to avoid the duplication?

I know that's different from the repository name, I don't think that would cause issues, I think it was just for consistency wasn't it?

Comment 12

[Nick Thomas [:nthomas] (UTC+12)]

Hmm, I've completely lost context on this. Two other options spring to mind, getting you a list of the branches in order to craft a patch that doesn't regress anything, swapping over to use whatever self-serve uses given that works.

Comment 13

[Mark Banner (:standard8)]

(In reply to Nick Thomas [:nthomas] from comment #9)
> That's fixed the issue with compile jobs, but there's something else causing
> issues with tests.

Unit tests wouldn't have worked because of the exact same reason as this breaking Firefox tests (comment 10).

(In reply to Nick Thomas [:nthomas] from comment #12)
> Hmm, I've completely lost context on this. Two other options spring to mind,
> getting you a list of the branches in order to craft a patch that doesn't
> regress anything, swapping over to use whatever self-serve uses given that
> works.

I think the only way we could avoid breaking tests etc, would be to work up a special case for try-comm-central versus try.

I haven't been able to find what self-serve uses, but I'm actually wondering if that's mainly relying on finding the cset rather than the branch.

Comment 14

[Nick Thomas [:nthomas] (UTC+12)]

Fixed by bug 835089.

Comment 15

[Mark Banner (:standard8)]

Verified as working on Thunderbird Try. Thanks folks.