1032975 - Add new table(s) to shipit database

Status: RESOLVED FIXED

(Release Engineering :: Release Automation: Other, defect)

Description

[John Zeller [:zeller]]

Before we can ship the changes in bug 826753, we must add new table(s) to accommodate the additional functionality

Comment 1

[John Zeller [:zeller]]

Attachment: schemas.sql

I am thinking that we should add 2 tables to the shipit database, called release_data and release_status. I have attached the schemas I have for each table.

For release_data, the table would be used to store all of the data coming from pulse.m.o messages. My methodology here is that we could store all of this data (the messages pertaining to release), rather than just cherry picking the steps needed for status. This way we can derive more detail when needed. I am open to the fact that this could be considered feature creep and planning for an unspecified need. Nevertheless, it had been a nice abstraction for me while in dev, so that I can focus on how to sort out the necessary data.

For release_status, I tried to keep the schema here fairly high level, consisting of a simple status integer (0 = not started, 1 = started, and 2 = finished). More detail could be necessary, but I feel that should be determined by what we do with the release_data table. Currently the plan is to update a single release_status row for each release, and store any meta data about that release (error, warnings, other messages, etc, etc) in release_data.

What do we think of this setup? I would love some feedback. (Going to add a few, but not necessary for everyone to respond)

Comment 2

[Rail Aliiev [:rail]]

My only concern is the size and performance of the release_data table. Do we have any estimates on how many messages per release we store there?

Also, you can optimize column types. Probably would be great to run the schema though sheeri, when it's ready.

Comment 3

[John Zeller [:zeller]]

Attachment: schema.sqlite

This has been a tough one to settle on. I want to hold only pertinent information. And also make it useful for phase 2. I think this is the compromise, but I would love feedback.

The idea here is that as shipit-agent digests pulse messages, it sends each valid one to the REST API which turns it into a new release_data row, and then compares all release_data rows for that name ('Firefox-31.0b5-build1') against the current release_status row for that name, and then submits an update to that release_status row. All the intergers in the release_status columns are set like this (0 = not started, 1 = started, 2 = finished).

Comment 4

[John Zeller [:zeller]]

(In reply to John Zeller [:zeller] from comment #3)
> Created attachment 8458881 [details]
> schema.sqlite
> 
> This has been a tough one to settle on. I want to hold only pertinent
> information. And also make it useful for phase 2. I think this is the
> compromise, but I would love feedback.
> 
> The idea here is that as shipit-agent digests pulse messages, it sends each
> valid one to the REST API which turns it into a new release_data row, and
> then compares all release_data rows for that name ('Firefox-31.0b5-build1')
> against the current release_status row for that name, and then submits an
> update to that release_status row. All the intergers in the release_status
> columns are set like this (0 = not started, 1 = started, 2 = finished).

I am also considering removing the following Columns from release_data:
* routing_key
* sent or submittedAt

Comment 5

[[e-mail] (:bhearsum)]

Comment on attachment 8458881 [details]
schema.sqlite

I'll provide feedback on this as part of the patch in bug 826753.

Comment 6

[John Zeller [:zeller]]

Attachment: bug1032975.patch

Comments leading up to this review can be seen in bug 826753

Comment 7

[John Zeller [:zeller]]

Attachment: bug1032975.patch

Added migrate_repo

Comment 8

[John Zeller [:zeller]]

Attachment: bug1032975.patch

Adjusted a few things so that it won't cause merge conflicts

Comment 9

[[e-mail] (:bhearsum)]

Comment on attachment 8461895 [details] [diff] [review]
bug1032975.patch

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

::: kickoff/model.py
@@ +204,5 @@
> +    """A base class to store release events primarily from buildbot."""
> +    __tablename__ = 'release_events'
> +    name = db.Column(db.String(500), nullable=False, primary_key=True)
> +    sent = db.Column(db.DateTime(pytz.utc), nullable=False)
> +    event_name = db.Column(db.String(500), nullable=False)

event_name needs to have primary_key=True set, too. Don't forget to update it in the migrate script too.

Comment 10

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #9)
> Comment on attachment 8461895 [details] [diff] [review]
> bug1032975.patch
> 
> Review of attachment 8461895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: kickoff/model.py
> @@ +204,5 @@
> > +    """A base class to store release events primarily from buildbot."""
> > +    __tablename__ = 'release_events'
> > +    name = db.Column(db.String(500), nullable=False, primary_key=True)
> > +    sent = db.Column(db.DateTime(pytz.utc), nullable=False)
> > +    event_name = db.Column(db.String(500), nullable=False)
> 
> event_name needs to have primary_key=True set, too. Don't forget to update
> it in the migrate script too.

Are we sure event_name is unique enough?

Comment 11

[John Zeller [:zeller]]

Attachment: bug1032975.patch

Comment 12

[[e-mail] (:bhearsum)]

Comment on attachment 8462762 [details] [diff] [review]
bug1032975.patch

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

Looks good! We'll get this landed on Monday.

Comment 13

[[e-mail] (:bhearsum)]

Comment on attachment 8462762 [details] [diff] [review]
bug1032975.patch

I tried to land this but ran into sqlalchemy.exc.OperationalError: (OperationalError) (1071, 'Specified key was too long; max key length is 767 bytes') '\nCREATE TABLE release_events (\n\tname VARCHAR(100) NOT NULL, \n\tsent DATETIME NOT NULL, \n\tevent_name VARCHAR(500) NOT NULL, \n\tplatform VARCHAR(500), \n\tresults INTEGER NOT NULL, \n\t`chunkNum` INTEGER NOT NULL, \n\t`chunkTotal` INTEGER NOT NULL, \n\tPRIMARY KEY (name, event_name)\n)\n\n' ()


According to https://dev.mysql.com/doc/refman/5.5/en/innodb-restrictions.html, 255 characters of VARCHAR is the maximum key length. With "name" already being 100 chars, we're limited to ~150 chars for event_name. Do you think that will be long enough, John?

Comment 14

[John Zeller [:zeller]]

(In reply to Ben Hearsum [:bhearsum] from comment #13)
> Comment on attachment 8462762 [details] [diff] [review]
> bug1032975.patch
> 
> I tried to land this but ran into sqlalchemy.exc.OperationalError:
> (OperationalError) (1071, 'Specified key was too long; max key length is 767
> bytes') '\nCREATE TABLE release_events (\n\tname VARCHAR(100) NOT NULL,
> \n\tsent DATETIME NOT NULL, \n\tevent_name VARCHAR(500) NOT NULL,
> \n\tplatform VARCHAR(500), \n\tresults INTEGER NOT NULL, \n\t`chunkNum`
> INTEGER NOT NULL, \n\t`chunkTotal` INTEGER NOT NULL, \n\tPRIMARY KEY (name,
> event_name)\n)\n\n' ()
> 
> 
> According to
> https://dev.mysql.com/doc/refman/5.5/en/innodb-restrictions.html, 255
> characters of VARCHAR is the maximum key length. With "name" already being
> 100 chars, we're limited to ~150 chars for event_name. Do you think that
> will be long enough, John?

Shoot. Yeah I think 150 is probably okay for event_name. The longest builderName I am seeing in my dataset is 50. Also we could stand to shorten the varchar for name to at least 50, which would buy us another 50 for event_name.

Comment 15

[John Zeller [:zeller]]

Attachment: bug1032975.patch

Comment 16

[[e-mail] (:bhearsum)]

Comment on attachment 8463445 [details] [diff] [review]
bug1032975.patch

John landed this himself (yay!) and I did the database upgrades to stage/prod. Dev is still down.

Comment 17

[John Zeller [:zeller]]

Attachment: bug1032975-2.patch

Seeing as how we are adding the status_type property to pulse messages. I think we might reconsider adding it as a Column in the ReleaseEvents table. What do you think?

Comment 18

[[e-mail] (:bhearsum)]

Comment on attachment 8464168 [details] [diff] [review]
bug1032975-2.patch

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

Yes, I think this makes sense. You need to create a new version though. The production database is already upgraded to version 5, so we can't re-uprgade it. r+ with this in version 6.

Comment 19

[John Zeller [:zeller]]

Attachment: bug1032975-2.patch

Once I test migrate_repo, this is good to go I think!

Comment 20

[John Zeller [:zeller]]

Attachment: bug1032975-2.patch

Tested! Good to go!

Comment 21

[[e-mail] (:bhearsum)]

Comment on attachment 8465632 [details] [diff] [review]
bug1032975-2.patch

Per our Vidyo conversation, status_type is getting a new name.

Comment 22

[John Zeller [:zeller]]

Attachment: bug1032975-2.patch

Changed status_type to category

Comment 23

[[e-mail] (:bhearsum)]

Comment on attachment 8469448 [details] [diff] [review]
bug1032975-2.patch

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

Looks fine. I'm going to wait until Monday to land this, though.

Comment 24

[John Zeller [:zeller]]

Attachment: add group column to ship it tables

Changed category to group

Comment 25

[[e-mail] (:bhearsum)]

Comment on attachment 8469480 [details] [diff] [review]
add group column to ship it tables

I checked this in and upgraded the dev, stage, and prod databases.