1306707 - Remove support for 12 character ('short') SHA revisions

Status: RESOLVED WONTFIX

(Tree Management :: Treeherder: Data Ingestion, defect, P3)

Description

[William Lachance (:wlach)]

I'd like us to consider unsupporting submitting <40 character revisions. Is there any reason we shouldn't? We have a bunch of code associated with keeping the short and long revision fields in sync that I'd like not to have to rewrite when moving stuff into the main db (bug 1306674).

Comment 1

[William Lachance (:wlach)]

Do we have any submitters who are giving us 12 character revisions but not 40? (or 12 then 40 later?)

Comment 2

[Ed Morley [:emorley]]

There are still some job types (eg l10n jobs) that use the 12 character SHA.

Though:
(a) I think we should just give them a hard deadline and then leave it up to them
(b) Even if we have to support 12 character jobs, perhaps we can remove support from eg push ingestion / the resultset API etc

New Relic attribute recording is likely a good way forwards here.

Comment 3

[William Lachance (:wlach)]

(In reply to Ed Morley [:emorley] from comment #2)
> There are still some job types (eg l10n jobs) that use the 12 character SHA.
> ...

Ok, that's annoying but I guess we'll just live with it in the short term.

At least the business logic to handle this will be easier to write with the Django ORM.

Comment 4

[Ed Morley [:emorley]]

I think we should at least identify which jobs still are using 12 characters and file bugs for them.

The l10n jobs (and I imagine any/most other culprits) are in builds-4hr - I'll have a shot at analysing now.

Comment 5

[Ed Morley [:emorley]]

Even builds-pending/builds-running contain 12 character SHAs :-(

Builds-pending:

{
  "pending": {
    "try": {
      "1a855f92cc45": [
        {
          "submitted_at": 1475064907,
          "id": 125865687,
          "buildername": "Windows 10 64-bit try debug test mochitest-gl-1"
        },


Builds-running (at least it has the full SHA in there):

{
  "running": {
    "autoland": {
      "cdf154426881": [
        {
          "submitted_at": 1475263520,
          "buildername": "Android armv7 API 15+ autoland build",
          "start_time": 1475263723,
          "number": 320,
          "claimed_by_name": "buildbot-master74.bb.releng.usw2.mozilla.com:/builds/buildbot/build1/master",
          "request_ids": [
            126258716
          ],
          "last_heartbeat": 1475264067,
          "id": 127438343,
          "revision": "cdf154426881032934224b0ad60bc366afebff41"
        }
      ],

Comment 6

[Ed Morley [:emorley]]

Attachment: Script to find 12 character jobs in builds-4hr archives

Comment 7

[Ed Morley [:emorley]]

Attachment: Builds-4hr jobs with 12 char SHA on 2016-09-29

Main candidates are:
* PGO builds plus their corresponding tests
* Various jobs run on comm-* (though these are < tier-3)
* Desktop+Android l10n nightlies

Comment 8

[Ed Morley [:emorley]]

Some places are still using 12 character SHAs in UI URLs too.

eg on:
https://hg.mozilla.org/mozilla-central/rev/af6e01b8574b

"[default view]" -> https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=87cd291d2db6
"[failures only]" -> https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=87cd291d2db6&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

(the perfherder links on that page use 40 character SHAs however)

Unfortunately we can't use server logs to see how many people are doing this, since the fragment part of the URL is not sent in the HTTP request. Think we'll have to resort to GitHub code search etc.

Comment 9

[Ed Morley [:emorley]]

Will, I still think we can simplify things on our side somewhat, ie we can remove support for 12 character SHAs from:
* Resultsets ingestion via our API
* Resultsets ingestion via json-pushes polling
* Resultsets ingestion via Pulse (if not already done; I can't find the schema for it)
* [DONE] Jobs ingestion via Pulse (see pulse-job.yml)
* The `revision` table (since I'm pretty sure for buildbot job ingestion we only need short_revision on the result_set table, right?)

Similarly, whilst the revision hash removal (bug 1257602) is blocked on some older repos (though we should double-check which - given B2G EOL and ESRs only run taskcluster as tier <1 so can probably just have those jobs hidden), we can probably remove support from many other places.

Comment 10

[Ed Morley [:emorley]]

I mean we could even remove `short_revision` from the `result_set` table if we were prepared to do LIKE queries against `long_revision` for these legacy buildbot job types and/or UI URLs that are still using 12 character SHAs.

Comment 11

[Ed Morley [:emorley]]

Attachment: Script to find 12 character jobs in builds-4hr archives

Comment 12

[Ed Morley [:emorley]]

Attachment: Builds-4hr jobs with 12 char SHA on 2016-11-10

Checking again shows many cases fixed.

Left are some PGO jobs that are instances of bug 1313274, plus aurora instances of bug 1306722 (needs backporting).

Comment 13

[Ed Morley [:emorley]]

Now that bug 1306718 is deployed, builds-{pending,running} use the full 40 character SHAs.

This just leaves the self-serve triggered builds (eg sheriffs triggering PGO runs but only pasting in 12 character SHAs), which is bug 1313274. An alternative and possibly quicker fix for that is to make it easier to copy the full SHA from Treeherder (bug 1340265).

Comment 14

[Ed Morley [:emorley]]

I'm not going to wait on bug 1313274 or bug 1340265 any longer. Let's just do this.

Comment 15

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/3078

Comment 16

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/4534

Comment 17

[Ed Morley [:emorley]]

After deploying, New Relic reported instances of:

treeherder.model.models:MultipleObjectsReturned: get() returned more than one Push -- it returned 2!

and

treeherder.model.models:DoesNotExist: Push matching query does not exist.

I believe this is since repository is no longer in a few of the queries, eg:
https://github.com/mozilla/treeherder/pull/4534/files#diff-d1c18844859a68f2ac3f6aa32603cdbeL54

I've rolled back the prod deploy for now - master will need a fix before we deploy again.

Comment 18

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/treeherder/pull/4560

Comment 19

[Cameron Dawson [:camd]]

This new PR should fix the issue.

Comment 20

[Ed Morley [:emorley]]

The 2nd PR landed in:
https://github.com/mozilla/treeherder/commit/bcb0bc2d7ca34e9ca780a4b737f7f7896c395802

Comment 21

[Ed Morley [:emorley]]

Actually, reopening since the API still supports 12 character revisions and their usage is being tracked via:
https://github.com/mozilla/treeherder/pull/4534/files#diff-d1c18844859a68f2ac3f6aa32603cdbeR272

The API now does the optimal MySQL query if provided with a 40 character SHA, however the fallback should be removed to prevent people from accidentally relying on it (eg when writing tools that comment on bugs etc).

When doing so it may be worth making the API explicitly return a JSON error message and HTTP 400 for SHAs that are not 40 characters (and also ensuring it's bubbled up to the jobs-view and Perfherder UIs) to prevent confusing UX from 404s from seemingly valid SHAs.

Comment 22

[Cameron Dawson [:camd]]

This would be good to get done. But I'm not going to get to it anytime soon.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

cleaning up old bugs we haven't fixed