Note: There are a few cases of duplicates in user autocompletion which are being worked on.

replace columns in shipit database to track kickoff status

RESOLVED INCOMPLETE

Status

Release Engineering
Ship It
RESOLVED INCOMPLETE
3 years ago
a year ago

People

(Reporter: zeller, Assigned: zeller)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

3 years ago
Per comment 1 in Bug 886522:

Ship It currently tracks a release from initial submission, through review, and until the automation starts (currently known as "complete"). We currently have a few different states for a release to be in: Submitted, Pending, Running (currently known as "complete"). We currently have two columns ("ready" and "complete") that let us represent this state in the database. After this work is complete we will have more states for the overall release: Submitted, Pending, Running. In order to avoid column explosion, we'll be replacing the "ready" and "complete" columns with a single enum known as "kickoffStatus".

Here is a general sense of what kickoffStatus enumeration could stand for:
* kickoffStatus:
** 0 - Submitted - initial state
** 1 - Pending - set after a release is reviewed through the web
** 2 - Running - set by release runner after release is successful
** 3 - Completed - could be set release runner after postrelease is successful
** 4 - Failed - could be set by release runner is a release or postrelease is unsuccessful
(Assignee)

Updated

3 years ago
Assignee: nobody → johnlzeller
(Assignee)

Updated

3 years ago
Blocks: 886522
(Assignee)

Comment 1

3 years ago
Created attachment 8483356 [details] [diff] [review]
kickoff-bug1062187.patch

This patch needs to be tested, but I'd like feedback. What are the commands to test the downgrade/upgrade with migrate_repo? I had them saved, and I cannot find them.

PS I only need feedback from one of you :)
Attachment #8483356 - Flags: feedback?(rail)
Attachment #8483356 - Flags: feedback?(bhearsum)
(Assignee)

Comment 2

3 years ago
Created attachment 8483357 [details] [diff] [review]
tools-bug1062187.patch

This patch needs to be tested as well :)

PS Again, I only need feedback from one of you :)
Attachment #8483357 - Flags: feedback?(rail)
Attachment #8483357 - Flags: feedback?(bhearsum)
(Assignee)

Comment 3

3 years ago
Created attachment 8483370 [details] [diff] [review]
kickoff-bug1062187.patch

I realized that I hadn't added pushsnipStatus and postreleaseStatus as mentioned in bug 886522 comment 1.

Copied from comment 1:
This patch needs to be tested, but I'd like feedback. What are the commands to test the downgrade/upgrade with migrate_repo? I had them saved, and I cannot find them.

PS I only need feedback from one of you :)
Attachment #8483356 - Attachment is obsolete: true
Attachment #8483356 - Flags: feedback?(rail)
Attachment #8483356 - Flags: feedback?(bhearsum)
Attachment #8483370 - Flags: feedback?(rail)
Attachment #8483370 - Flags: feedback?(bhearsum)
(Assignee)

Comment 4

3 years ago
Created attachment 8483371 [details] [diff] [review]
tools-bug1062187.patch

Again, forgot about the pushsnipStatus and postreleaseStatus columns.

Copied from comment 2:
This patch needs to be tested as well :)

PS Again, I only need feedback from one of you :)
Attachment #8483357 - Attachment is obsolete: true
Attachment #8483357 - Flags: feedback?(rail)
Attachment #8483357 - Flags: feedback?(bhearsum)
Attachment #8483371 - Flags: feedback?(rail)
Attachment #8483371 - Flags: feedback?(bhearsum)
(Assignee)

Comment 5

3 years ago
Bug 886522 comment 1 points to 4 new status columns:

* kickoffStatus:
** Submitted - initial state
** Pending - set after a release is reviewed through the web
** Running - set by release runner after release is successfully
* readyForRelease - set by the "ready for release" Buildbot builder
* pushsnipStatus/postreleaseStatus:
** Not run - initial state
** Pending - set after the button is clicked on web interface
** Running - set by release runner after it successfully submits a the job to Buildbot
** Complete - set by the the builder after it successfully pushes snipptes
** Failed - set by the the builder if it fails to push snippets

I think readyForRelease is unnecessary since bug 826753 adds the ability to see this via helper functions. Any objections?
(Assignee)

Comment 6

3 years ago
^^ comment 5
Flags: needinfo?(rail)
Flags: needinfo?(bhearsum)
(Assignee)

Updated

3 years ago
Blocks: 1062205
I'm deferring all of this to Rail.
Flags: needinfo?(bhearsum)
Attachment #8483370 - Flags: feedback?(bhearsum)
Attachment #8483371 - Flags: feedback?(bhearsum)
(In reply to John Zeller [:zeller] from comment #5)
> Any objections?

Sounds good to me.
Flags: needinfo?(rail)
Comment on attachment 8483371 [details] [diff] [review]
tools-bug1062187.patch

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

::: buildfarm/release/release-runner.py
@@ +102,2 @@
>          self.release_api.update(
> +            release['name'], kickoffStatus=2, status='Started')

I'd use constant like variables instead of integers. Something like

PENDING, RUNNING, STARTED, FAILED, BLAH = range(5)
....
 ..., kickoffStatus=STARTED,...

It will be easier to grep and replace the status codes in the future.
Attachment #8483371 - Flags: feedback?(rail) → feedback+
Comment on attachment 8483370 [details] [diff] [review]
kickoff-bug1062187.patch

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

In overall it looks good. I didn't look at the migration script (close to 0 experience).
It would be great to run the final patch through bherasum.

::: kickoff/templates/includes/releases_complete.html
@@ +1,1 @@
> +{% for rel in releases if rel.kickoffStatus == 3 %}

I would use "constants" here as well. This applies to the whole patch.
Attachment #8483370 - Flags: feedback?(rail) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8488916 [details] [diff] [review]
tools-bug1062187.patch

Made the changes you requested in the previous comment for tools. Here is the partial patch to make it clear what has changed: http://zeller.pastebin.mozilla.org/6458465
Attachment #8483371 - Attachment is obsolete: true
Attachment #8488916 - Flags: feedback?(rail)
Comment on attachment 8488916 [details] [diff] [review]
tools-bug1062187.patch

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

lgtm!
Attachment #8488916 - Flags: feedback?(rail) → feedback+
(Assignee)

Comment 13

3 years ago
I am having trouble getting migrate_repo to play nice. Version 7 adds the Columns kickoffStatus, pushsnipStatus, and postreleaseStatus, and then removes ready and complete from the 3 releases tables. It fails on the drop Columns calls: 

sqlalchemy.exc.OperationalError: (OperationalError) no such column: ready u'\nCREATE TABLE fennec_release (\n\tname VARCHAR(100) NOT NULL, \n\tsubmitter VARCHAR(250) NOT NULL, \n\t"submittedAt" DATETIME NOT NULL, \n\tversion VARCHAR(10) NOT NULL, \n\t"buildNumber" INTEGER NOT NULL, \n\tbranch VARCHAR(50) NOT NULL, \n\t"mozillaRevision" VARCHAR(100) NOT NULL, \n\t"l10nChangesets" TEXT NOT NULL, \n\t"dashboardCheck" BOOLEAN NOT NULL, \n\tcomplete BOOLEAN NOT NULL, \n\tstatus VARCHAR(250), \n\t"mozillaRelbranch" VARCHAR(50), \n\t"enUSPlatforms" VARCHAR(500), \n\tcomment TEXT, \n\tPRIMARY KEY (name), \n\tCHECK ("dashboardCheck" IN (0, 1)), \n\tCHECK (ready IN (0, 1)), \n\tCHECK (complete IN (0, 1))\n)\n\n' ()

I thought the problem lied in the fact that model.py had been updated to not include ready and complete Columns, but when I added them in again, the error persists. Any ideas?

See attachment 8483370 [details] [diff] [review] (kickoff-bug1062187.patch) for the migrate_repo version 7 I have.
Flags: needinfo?(bhearsum)
(In reply to John Zeller [:zeller] from comment #13)
> I am having trouble getting migrate_repo to play nice. Version 7 adds the
> Columns kickoffStatus, pushsnipStatus, and postreleaseStatus, and then
> removes ready and complete from the 3 releases tables. It fails on the drop
> Columns calls: 
> 
> sqlalchemy.exc.OperationalError: (OperationalError) no such column: ready
> u'\nCREATE TABLE fennec_release (\n\tname VARCHAR(100) NOT NULL,
> \n\tsubmitter VARCHAR(250) NOT NULL, \n\t"submittedAt" DATETIME NOT NULL,
> \n\tversion VARCHAR(10) NOT NULL, \n\t"buildNumber" INTEGER NOT NULL,
> \n\tbranch VARCHAR(50) NOT NULL, \n\t"mozillaRevision" VARCHAR(100) NOT
> NULL, \n\t"l10nChangesets" TEXT NOT NULL, \n\t"dashboardCheck" BOOLEAN NOT
> NULL, \n\tcomplete BOOLEAN NOT NULL, \n\tstatus VARCHAR(250),
> \n\t"mozillaRelbranch" VARCHAR(50), \n\t"enUSPlatforms" VARCHAR(500),
> \n\tcomment TEXT, \n\tPRIMARY KEY (name), \n\tCHECK ("dashboardCheck" IN (0,
> 1)), \n\tCHECK (ready IN (0, 1)), \n\tCHECK (complete IN (0, 1))\n)\n\n' ()
> 
> I thought the problem lied in the fact that model.py had been updated to not
> include ready and complete Columns, but when I added them in again, the
> error persists. Any ideas?
> 
> See attachment 8483370 [details] [diff] [review] (kickoff-bug1062187.patch)
> for the migrate_repo version 7 I have.

The problem here is that it's trying to create CHECK constraints for ready and complete, but the columns no longer exist. That traceback may be coming from Table.__init__ instead of drop()...it's hard to tell. It might be worthwhile putting those on separate lines to figure out where it's happening exactly.

Also, make sure that your database version and schema match. Eg, did you somehow alter the database to be different than what the version in the migrate_version table is?
Flags: needinfo?(bhearsum)
Comment on attachment 8483370 [details] [diff] [review]
kickoff-bug1062187.patch

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

Random drive-by comment: make sure you _migrate_ the data to the new columns before dropping, too. That is to say, you need to make sure that the new columns are set correctly for all of the old releases. You may need to check the state of ready/complete to determine what they should be.
(Assignee)

Comment 16

3 years ago
Created attachment 8491196 [details]
007_Add_kickoffStatus_and_remove_ready_and_complete.py

So I incorporated the migration of data as bhearsum suggests in the above comment. I ran into an issue with dropping the ready and complete columns, because sqlite doesn't support certains types of alter table commands. The issue seems to be that it drops the column, but the CHECK constraints are still there, and it trips on itself.

Currently the migration script copies all the rows from the database, omitting the ready and complete columns. Upgrade is working, but downgrading is running into one of 2 errors:
* sqlalchemy.exc.OperationalError: (OperationalError) Cannot add a NOT NULL column with default value NULL '\nALTER TABLE fennec_release ADD ready BOOLEAN NOT NULL' ()
* sqlalchemy.exc.ArgumentError: Column must be constructed with a non-blank name or assign a non-blank .name before adding to a Table.

Is the shipit database in production mysql? If that is the case, then the drop column commands should be easy to do, I am just developing in sqlite.

I also am going to try to drop check constraints with sqlalchemy, to see if it handles the fact that sqlite doesn't support it.
Flags: needinfo?(bhearsum)
(In reply to John Zeller [:zeller] from comment #16)
> Created attachment 8491196 [details]
> 007_Add_kickoffStatus_and_remove_ready_and_complete.py
> 
> So I incorporated the migration of data as bhearsum suggests in the above
> comment. I ran into an issue with dropping the ready and complete columns,
> because sqlite doesn't support certains types of alter table commands. The
> issue seems to be that it drops the column, but the CHECK constraints are
> still there, and it trips on itself.
> 
> Currently the migration script copies all the rows from the database,
> omitting the ready and complete columns. Upgrade is working, but downgrading
> is running into one of 2 errors:
> * sqlalchemy.exc.OperationalError: (OperationalError) Cannot add a NOT NULL
> column with default value NULL '\nALTER TABLE fennec_release ADD ready
> BOOLEAN NOT NULL' ()
> * sqlalchemy.exc.ArgumentError: Column must be constructed with a non-blank
> name or assign a non-blank .name before adding to a Table.
> 
> Is the shipit database in production mysql? If that is the case, then the
> drop column commands should be easy to do, I am just developing in sqlite.

Can you test with mysql locally to confirm this? Or if you can't easily install it on your laptop, we can set-up a database for you on dev-master1 or elsewhere.
Flags: needinfo?(bhearsum)
(Assignee)

Comment 18

3 years ago
(In reply to Ben Hearsum [:bhearsum] from comment #17)
> (In reply to John Zeller [:zeller] from comment #16)
> > Created attachment 8491196 [details]
> > 007_Add_kickoffStatus_and_remove_ready_and_complete.py
> > 
> > So I incorporated the migration of data as bhearsum suggests in the above
> > comment. I ran into an issue with dropping the ready and complete columns,
> > because sqlite doesn't support certains types of alter table commands. The
> > issue seems to be that it drops the column, but the CHECK constraints are
> > still there, and it trips on itself.
> > 
> > Currently the migration script copies all the rows from the database,
> > omitting the ready and complete columns. Upgrade is working, but downgrading
> > is running into one of 2 errors:
> > * sqlalchemy.exc.OperationalError: (OperationalError) Cannot add a NOT NULL
> > column with default value NULL '\nALTER TABLE fennec_release ADD ready
> > BOOLEAN NOT NULL' ()
> > * sqlalchemy.exc.ArgumentError: Column must be constructed with a non-blank
> > name or assign a non-blank .name before adding to a Table.
> > 
> > Is the shipit database in production mysql? If that is the case, then the
> > drop column commands should be easy to do, I am just developing in sqlite.
> 
> Can you test with mysql locally to confirm this? Or if you can't easily
> install it on your laptop, we can set-up a database for you on dev-master1
> or elsewhere.

Ah, yup works fine with mysql. Changing migrate back, keeping the data migration parts of it.
(Assignee)

Comment 19

3 years ago
Created attachment 8491946 [details] [diff] [review]
kickoff-bug1062187.patch

I added that status types into a class called StatusType to facilitate a more compact form. This is helpful for sending to templates and such.

Only a couple of questions left here, but most of the rest of this is ready to go.

* This introduces a new status (among many), FAILED. Ready and Complete don't take into account this possible status. When downgrading the database from version 7 (this new version) to version 6, we must migrate the data from kickoffStatus to ready and complete. Once we start using FAILED, what should this downgrade to? I currently have FAILED downgrading to ready=1 and complete=1. But I could be convinced of ready=0 and complete=0.
* Is StatusType a good enough name for the class holding these statuses?
Attachment #8483370 - Attachment is obsolete: true
Attachment #8491196 - Attachment is obsolete: true
Attachment #8491946 - Flags: review?(rail)
(Assignee)

Comment 20

3 years ago
Created attachment 8491947 [details] [diff] [review]
update release runner to use new status types

Made the changes you requested
Attachment #8488916 - Attachment is obsolete: true
Attachment #8491947 - Flags: review?(rail)
(Assignee)

Comment 21

3 years ago
Created attachment 8491950 [details] [diff] [review]
kickoff-bug1062187.patch

Opps, left in a testing piece. Removed it :)

All questions in comment 19 still stand.
Attachment #8491946 - Attachment is obsolete: true
Attachment #8491946 - Flags: review?(rail)
Attachment #8491950 - Flags: review?(rail)
Comment on attachment 8491950 [details] [diff] [review]
kickoff-bug1062187.patch

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

302 bhearsum

::: vendor/lib/python/mozilla/release/info.py
@@ +9,5 @@
>      return bool(re.match(FINAL_RELEASE_REGEX, version))
> +
> +class StatusType:
> +    def __init__(self):
> +        self.INACTIVE, self.PENDING, self.RUNNING, self.COMPLETED, self.FAILED = range(5)

I'd go with:

class StatusType:
    INACTIVE, PENDING, RUNNING, COMPLETED, FAILED = range(5)

So you can use:
StatusType.INACTIVE instead of StatusType().INACTIVE
Attachment #8491950 - Flags: review?(rail) → review?(bhearsum)
Comment on attachment 8491947 [details] [diff] [review]
update release runner to use new status types

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

::: buildfarm/release/release-runner.py
@@ +108,5 @@
> +        self.release_api.update(release['name'], kickoffStatus=status_type.INACTIVE, status=why)
> +        # TODO: Possibly mark as status_type.FAILED instead and change shipit to show 
> +        #       status_type.FAILED in the submitted category 
> +
> +    # TODO: Add functions for pushsnipStatus and postreleaseStatus changes

Testing in production? :)
What holds us to add the functions mentioned in TODOs?
(Assignee)

Comment 24

3 years ago
(In reply to Rail Aliiev [:rail] from comment #23)
> Comment on attachment 8491947 [details] [diff] [review]
> tools-bug1062187.patch
> 
> Review of attachment 8491947 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: buildfarm/release/release-runner.py
> @@ +108,5 @@
> > +        self.release_api.update(release['name'], kickoffStatus=status_type.INACTIVE, status=why)
> > +        # TODO: Possibly mark as status_type.FAILED instead and change shipit to show 
> > +        #       status_type.FAILED in the submitted category 
> > +
> > +    # TODO: Add functions for pushsnipStatus and postreleaseStatus changes
> 
> Testing in production? :)
> What holds us to add the functions mentioned in TODOs?

Mistakenly left these in actually! 2nd TODO is for another bug, but for the first. What do you think about that change of also adding "Failed" to the status column on a fail, and using the FAILED kickoffStatus.
Flags: needinfo?(rail)
Attachment #8491947 - Attachment description: tools-bug1062187.patch → update release runner to use new status types
Attachment #8491950 - Attachment description: kickoff-bug1062187.patch → add status type columns to ship it
Comment on attachment 8491947 [details] [diff] [review]
update release runner to use new status types

(In reply to John Zeller [:zeller] from comment #24)
> (In reply to Rail Aliiev [:rail] from comment #23)
> > Comment on attachment 8491947 [details] [diff] [review]
> > tools-bug1062187.patch
> > 
> > Review of attachment 8491947 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: buildfarm/release/release-runner.py
> > @@ +108,5 @@
> > > +        self.release_api.update(release['name'], kickoffStatus=status_type.INACTIVE, status=why)
> > > +        # TODO: Possibly mark as status_type.FAILED instead and change shipit to show 
> > > +        #       status_type.FAILED in the submitted category 
> > > +
> > > +    # TODO: Add functions for pushsnipStatus and postreleaseStatus changes
> > 
> > Testing in production? :)
> > What holds us to add the functions mentioned in TODOs?
> 
> Mistakenly left these in actually! 2nd TODO is for another bug, but for the
> first. What do you think about that change of also adding "Failed" to the
> status column on a fail, and using the FAILED kickoffStatus.

Yeah, it'd be better. At least the function name and its action will be in tact.
Attachment #8491947 - Flags: review?(rail)
Flags: needinfo?(rail)
Comment on attachment 8491950 [details] [diff] [review]
kickoff-bug1062187.patch

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

(In reply to John Zeller [:zeller] from comment #19)
> Created attachment 8491946 [details] [diff] [review]
> kickoff-bug1062187.patch
> 
> I added that status types into a class called StatusType to facilitate a
> more compact form. This is helpful for sending to templates and such.
> 
> Only a couple of questions left here, but most of the rest of this is ready
> to go.
> 
> * This introduces a new status (among many), FAILED. Ready and Complete
> don't take into account this possible status. When downgrading the database
> from version 7 (this new version) to version 6, we must migrate the data
> from kickoffStatus to ready and complete. Once we start using FAILED, what
> should this downgrade to? I currently have FAILED downgrading to ready=1 and
> complete=1. But I could be convinced of ready=0 and complete=0.

Either way sounds fine to me.

::: kickoff/model.py
@@ +24,5 @@
>      mozillaRevision = db.Column(db.String(100), nullable=False)
>      l10nChangesets = db.Column(db.Text(), nullable=False)
>      dashboardCheck = db.Column(db.Boolean(), nullable=False, default=False)
> +    kickoffStatus = db.Column(db.Integer(), nullable=True, default=0)
> +        # 0 - Submitted, 1 - Pending/Ready, 2 - Running, 3 - Completed (Postrelease), 4 - Failed

You should be using StatusType rather than plain integers here (and everywhere else - including the migration script). No need for these docstrings either - that class is self documenting.

I'm a bit confused about kickoffStatus upon second viewing, though. Most of your patch matches the plan from https://bugzilla.mozilla.org/show_bug.cgi?id=886522#c1, but this column has extra attributes and I can't figure out why. Can you explain this?

::: migrate_repo/versions/007_Add_kickoffStatus_and_remove_ready_and_complete.py
@@ +1,1 @@
> +from sqlalchemy import Column, Integer, Boolean, MetaData, Table, update

Can you attach a before/after state of one of the tables?

@@ +37,5 @@
> +            row = dict(zip(ta.columns.keys(), row))
> +            if row['ready']:
> +                status += 1
> +            if row['complete']:
> +                status += 2

It doesn't matter in this case, but be careful this sort of thing in the future. If ready=0, complete=1 was a valid state, you'd end up setting status to RUNNING.

::: vendor/lib/python/mozilla/release/info.py
@@ +9,5 @@
>      return bool(re.match(FINAL_RELEASE_REGEX, version))
> +
> +class StatusType:
> +    def __init__(self):
> +        self.INACTIVE, self.PENDING, self.RUNNING, self.COMPLETED, self.FAILED = range(5)

A few things:
* This change needs to go into the tools repo and then be imported here. Anything from vendor/ is imported from elsewhere.
* This should go in lib/python/kickoff IMO. This is specifically related to Ship It, not release automation in general.
* Thinking about the names in context, I think "Status" would be better. "Status.PENDING" reads much nicer than "StatusType.PENDING".
Attachment #8491950 - Attachment description: add status type columns to ship it → kickoff-bug1062187.patch
Attachment #8491950 - Flags: review?(bhearsum) → review-
Blocks: 1091342
Mass component change for ship it bugs.
Component: Release Automation → Ship It
Release promotion changed a lot of things. The status can be determined runtime from TC now.
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.