1318474 - Store the job metadata in the central database instead of in datasource

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect)

Description

[William Lachance (:wlach)]

This is the last bit of information in the per-project databases, once this is done we should be able to fairly rip out the rest of datasource!

Comment 1

[William Lachance (:wlach)]

Based on my experiences with the result set / push migration, I'm going to go for a phased approach here:

1. Start ingesting job metadata in ORM alongside datasource.
2. Make endpoints *use* the data in the ORM, but still use the id numbering from the per-project datasource tables.
3. Make endpoints use/return the id numbering from the ORM.
4. Stop ingesting data into the per-project datasource tables.

By doing this phased approach, I think we can get some of the benefits of the migration finished sooner (e.g. making the treeherder database useable/accessible from redash/activedata, facilitate new projects like SETA) and reduce the risk of everything going wrong by switching things over all at once. 

I may actually postpone (3) and (4) until December or later, but hoping to get (1) and (2) landed by next week.

Comment 2

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474 > mozilla:master

Comment 3

[William Lachance (:wlach)]

Comment on attachment 8811959 [details] [review]
[treeherder] wlach:1318474 > mozilla:master

Hey Cam, could you have a look at this? I think it'll be easier to review this work in chunks, instead of me generating a huge big bang patch which does everything at once.

This PR adds all the job data we care about to the job table inside the main database, and starts using it for all API endpoints. When/if this PR lands, we can pretty much start ignoring datasource for all practical purposes (we basically only use it to number the jobs that we store in the main database).

Comment 4

[Cameron Dawson [:camd]]

Sorry I've taken so long to get started on this.  I've been trying to finish up the Persona migration work for Pulse Guardian before end of month.  Starting to review this now.

Comment 5

[Cameron Dawson [:camd]]

Comment on attachment 8811959 [details] [review]
[treeherder] wlach:1318474 > mozilla:master

As I said in the PR, I flagged a couple things for you to double-check.  But overall no major concerns.  Thanks again for doing this!  :)

Comment 6

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474-part1 > mozilla:master

Comment 7

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/9d557622f7c8cc049f95673173d9af7998750de3
Bug 1318474 - Add job metadata to ORM schema and start ingesting it (#1998)

Comment 8

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474-part2 > mozilla:master

Comment 9

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/9f604fa323db1d1d2e859c85b2c01810f7f781b7
Bug 1318474 - Switch API's and some internal consumers to use Django ORM for job data

Still *ingesting* into datasource for now

https://github.com/mozilla/treeherder/commit/1abbe58211c2933aad30757163b48565542bf679
Bug 1318474 - Remove obsolete e10s-special case handling in perf etl

Comment 10

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474-fix > mozilla:master

Comment 11

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/3090bffc6a338a88e60683717355c99410aaef19
Bug 1318474 - Fix getting individual jobs (#2003)

Comment 12

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474-jobnote-deletion-fix > mozilla:master

Comment 13

[William Lachance (:wlach)]

Comment on attachment 8815854 [details] [review]
[treeherder] wlach:1318474-jobnote-deletion-fix > mozilla:master

Could you have a look at this Cam? It's a fairly simple fix, but we need it to be applied before we can promote what's on master to production.

Comment 14

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/3721af655041ea09795081307add6fbaa7a12a19
Bug 1318474 - Fix job note deletion (#2008)

We were always resetting the failure classification to an invalid value
when the last job note was deleted for a job, but we didn't notice it
before because we didn't have foreign key validation.

Comment 15

[William Lachance (:wlach)]

Still need to add "not null" constraints to the metadata columns, now that we've transferred over the data. After that, I think we can resolve this particular bug.

Comment 16

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474-disallow-null > mozilla:master

Comment 17

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/53e4a9573238869c282e6f0ef8fa4151dc19f79f
Bug 1318474 - Disallow null job metadata (#2034)

Comment 18

[William Lachance (:wlach)]

Attachment: SQL for disabling null keys

The last part of this (disabling null keys) I am applying manually, as Django can't apply this migration without generating errors. Here's the SQL that is being applied.

Comment 19

[Ed Morley [:emorley]]

(In reply to William Lachance (:wlach) from comment #18)
> The last part of this (disabling null keys) I am applying manually, as
> Django can't apply this migration without generating errors. Here's the SQL
> that is being applied.

Could you elaborate? :-)

Looking at the SQL I'm presuming it's due to the need to skip the expensive foreign key checks? Or if it's something else I'm happy to fix upstream Django bugs.

Comment 20

[William Lachance (:wlach)]

(In reply to Ed Morley [:emorley] from comment #19)
> (In reply to William Lachance (:wlach) from comment #18)
> > The last part of this (disabling null keys) I am applying manually, as
> > Django can't apply this migration without generating errors. Here's the SQL
> > that is being applied.
> 
> Could you elaborate? :-)
> 
> Looking at the SQL I'm presuming it's due to the need to skip the expensive
> foreign key checks? Or if it's something else I'm happy to fix upstream
> Django bugs.

It's a limitation of innodb, that it apparently can't handle inserts during an alter table operation (which I think we might be seeing because of Django's get_or_create logic):

" When running an online DDL operation, the thread that runs the ALTER TABLE statement will apply an “online log” of DML operations that were run concurrently on the same table from other connection threads. When the DML operations are applied, it is possible to encounter a duplicate key entry error (ERROR 1062 (23000): Duplicate entry), even if the duplicate entry is only temporary and would be reverted by a later entry in the “online log”. This is similar to the idea of a foreign key constraint check in InnoDB in which constraints must hold during a transaction."

https://dev.mysql.com/doc/refman/5.6/en/innodb-create-index-limitations.html

I've been working on documenting these gotchas, will file a bug about it tomorrow.

PostgreSQL keeps on looking better.

Comment 21

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:1318474-use-global-id > mozilla:master

Comment 22

[William Lachance (:wlach)]

Comment on attachment 8821279 [details] [review]
[treeherder] mozilla:1318474-use-global-id > mozilla:master

Hey Cam, could you take a look at this one? It's the last externally facing change as part of the datasource removal. After this is applied, all API's should return and use ORM id's instead of datasource ones (there's a backward compatibility shim for old logviewer urls that we might have in orangefactor, etc.) and we can remove the rest of datasource at our leisure.

Comment 23

[Cameron Dawson [:camd]]

Comment on attachment 8821279 [details] [review]
[treeherder] mozilla:1318474-use-global-id > mozilla:master

This looks good to me.  :)

Comment 24

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/82b3ea713fc96592489bc5b1eb03047ecea0a80b
Bug 1318474 - Make jobs model take job ORM objects as input

https://github.com/mozilla/treeherder/commit/32cfd2b6599860c6a9015d3db994b29e60c223cc
Bug 1318474 - Make web api's use ORM ids as input and output

https://github.com/mozilla/treeherder/commit/041f481a2cedd9c07c0cfc4dc9374c2eae0d0c94
Bug 1318474 - Make sure old logviewer links still work

Unfortunately there are many older links to the logviewer with datasource
ids floating around. Support them by looking for jobs with the old datasource
id and the project inside the logviewer. If there's a match, then look up the log
information for that job instead.

Comment 25

[GitHub Autolander Bot]

Attachment: [treeherder] wlach:1318474-remove-jobmodel-read-methods > mozilla:master

Comment 26

[William Lachance (:wlach)]

The PR referenced in comment 25 pretty much wraps up this bug, but I'll wait until after the holidays to ask for review.

Comment 29

[William Lachance (:wlach)]

Comment on attachment 8822275 [details] [review]
[treeherder] wlach:1318474-remove-jobmodel-read-methods > mozilla:master

Hey Cam, we're almost done with these, I promise. :)

This stops us from writing to datasource at all, paving the way for its outright removal.

Comment 30

[William Lachance (:wlach)]

Attachment: raw sql to apply

Here's the raw sql that should be applied to avoid django migrations issues with the last PR (note only the alter table statement for `job` requires pausing job ingestion)

Comment 31

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/a4a19007c0dff001c0c77d73f60c2da2abb0a2f8
Bug 1318474 - Fix SETA tests to not rely on datasource

https://github.com/mozilla/treeherder/commit/901f6288a57947459f4d0c61c9dfbb3c85474712
Bug 1318474 - Rewrite unit tests to only look for ORM representation

https://github.com/mozilla/treeherder/commit/34ac762fd60cb10547d7c343425f9a96d4ab633c
Bug 1318474 - Remove a call to datasource jobs from autoclassify code

https://github.com/mozilla/treeherder/commit/f4a319b2cf4f69e7a0eca85b4742e4ee128f43ae
Bug 1318474 - Actually remove datasource calls from jobs model

https://github.com/mozilla/treeherder/commit/f6407b74935bfebba234ea752c891334d756c956
Bug 1318474 - Stop writing to datasource altogether for jobs data

The last thing we were using it for were cycling data and giving an
id numbering for jobs, and we can easily replace those with calls to
the ORM.

Comment 32

[William Lachance (:wlach)]

Applied all SQL changes except for the jobs table one to stage, will do rest tomorrow morning (requires a tree closure).

Comment 33

[William Lachance (:wlach)]

(In reply to William Lachance (:wlach) from comment #32)
> Applied all SQL changes except for the jobs table one to stage, will do rest
> tomorrow morning (requires a tree closure).

s/stage/production/ :)

Comment 34

[Carsten Book [:Tomcat]]

(In reply to William Lachance (:wlach) from comment #32)
> Applied all SQL changes except for the jobs table one to stage, will do rest
> tomorrow morning (requires a tree closure).

trees are closed now

Comment 35

[William Lachance (:wlach)]

Schema change deployed on production, as was code. I think we're done here.

Comment 37

[GitHub Bugzilla PR Linker]

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