880829 - Migrate current custom field based tracking flags to the new Tracking Flags extension tables

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, enhancement)

Description

[David Lawrence [:dkl]]

Attachment: Script to migrate old tracking flags to new tracking flag tables (v1)

bugzilla.mozilla.org is currently utilizing custom fields to aid in tracking releases for Firefox, Thunderbird, and other projects. This is not ideal as every release requires even more custom fields to be created causing certain tables in Bugzilla to grow very large in number of columns. This new TrackingFlags extension will move the fields to their own tables which will allow us to continue to grow without affecting the core Bugzilla tables and also hopefully improve performance as a result. 

More info:
https://wiki.mozilla.org/BMO/TrackingFlags

This bug is for reviewing/testing the migration script separately as we will be performing the migration after the main extension code has been deployed and has been used for other new fields needed. 

Please review and test locally to make sure that the older tracking flags are still properly visible and searchable and not data loss has occurred.

dkl

Comment 1

[:glob ✱]

Attachment: migrate_tracking_flags.pl.patch

reattaching as a patch so i can use splinter :)

Comment 2

[:glob ✱]

Comment on attachment 761426 [details] [diff] [review]
migrate_tracking_flags.pl.patch

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

i'm still working through testing the migrated fields, however there's a few issues which need to be addressed.

::: extensions/TrackingFlags/bin/migrate_tracking_flags.pl
@@ +57,5 @@
> +
> +    # Create product/component visibility
> +    foreach my $prod_name (keys %{ $bmo_tracking_flags{$field_re} }) {
> +        $product_cache{$prod_name} ||= Bugzilla::Product->new({ name => $prod_name });
> +        $product_cache{$prod_name} || next; # die "No such product $prod_name\n";

this should warn then next, to ensure issues aren't missed.

@@ +70,5 @@
> +                component_id     => undef
> +            });
> +        }
> +        else {
> +            foreach my $comp_name (@$components) {

$comp_name could be a regex, which you don't support:
  "mozilla.org" => ["Release Engineering", qr/^Release Engineering: /],

@@ +75,5 @@
> +                $component_cache{"${prod_name}:${comp_name}"}
> +                    ||= Bugzilla::Component->new({ name    => $comp_name,
> +                                                   product => $product_cache{$prod_name} });
> +                $component_cache{"${prod_name}:${comp_name}"}
> +                    || next; # die "No such product $prod_name and component $comp_name\n";

this should warn then next, to ensure issues aren't missed.
(this is how i caught the lack of regex support).

@@ +126,5 @@
> +        Bugzilla::Extension::TrackingFlags::Flag::Value->create({
> +            tracking_flag_id => $new_flag->flag_id,
> +            value            => $value->name,
> +            setter_group_id  => $group_cache{$group_name}->id,
> +            sortkey          => $value->sortkey

you need to migrate is_active too, as we have values which are inactive.

@@ +158,5 @@
> +    my $count = 1;
> +    my $total = scalar @$bugs;
> +    foreach my $row (@$bugs) {
> +        my ($id, $value) = @$row;
> +        indicate_progress({ current => $count++, total => $total, every => 25 });

you need to set $| to a true value for this to be useful

@@ +196,5 @@
> +
> +    my @drop_columns;
> +    foreach my $field (@$fields) {
> +        next if $field->name !~ /^cf_(blocking|tracking|status)_/;
> +        next if $field->name =~ /^cf_blocking_192/; # FIXME: These fields are currently broken

is this FIXME correct?

@@ +207,5 @@
> +                = Bugzilla::Extension::TrackingFlags::Flag->new({ name => $field->name });
> +
> +            next if $new_flag;
> +
> +            print "----------------------------------\n" . 

nit: trailing whitespace

@@ +253,5 @@
> +        }
> +    }
> +
> +    # Drop each custom flag's value table and the column from the bz schema object
> +    if (@drop_columns) {

mysql doesn't support transactional ddl, so this should be
  if (!$dry_run && @drop_columns) {

@@ +287,5 @@
> +    $dbh->bz_rollback_transaction() if $dry_run;
> +    die "$@" if $@;
> +}
> +
> +exit(0);

nit: no need to exit() at the end of a script.

Comment 3

[David Lawrence [:dkl]]

Attachment: migrate_tracking_flags.pl.patch

Comment 4

[:glob ✱]

Comment on attachment 765745 [details] [diff] [review]
migrate_tracking_flags.pl.patch

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

this looks great: took 18 minutes to run.

i hit an error while dropping cf_blocking_192, due to the bug_cf_blocking_192 table:

Failed SQL: [DROP TABLE cf_blocking_192] Error: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails [for Statement "DROP TABLE cf_blocking_192"] at /opt/bugzilla/htdocs/880829/extensions/TrackingFlags/bin/../../../Bugzilla/DB.pm line 1015.

there's an orphaned cf_status192 table that we should also drop.


these three tables can be dropped manually (or at the end of the script):
  bug_cf_blocking_192
  cf_blocking_192
  cf_status192

Comment 5

[David Lawrence [:dkl]]

Attachment: 880829_1.patch

Here is a patch that removes the old tracking flag custom code we used for custom fields. I have done quite a bit of testing myself but have a few other places I wanted to make sure things still behaved as before. Please take a look at the patch I have so far and try it out on your end as well.

dkl

Comment 6

[:glob ✱]

Comment on attachment 793728 [details] [diff] [review]
880829_1.patch

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

unfortunately you've removed too much, as there's non-tracking flags which use cf_visible_in_products (cf_colo_site, cf_office, cf_crash_signature, cf_due_date, and cf_locale).  $other_setters also needs to be retained for cf_colo_site.

::: extensions/TrackingFlags/lib/Constants.pm
@@ +46,5 @@
> +            collapsed   => 1,
> +            sortkey     => 1
> +        }
> +    );
> +    return [ sort { $a->{'sortkey'} <=> $b->{'sortkey'} } @flag_types ];

nit: sortkeys for flag_types should be unique, to ensure consistent sorting.

::: template/en/default/bug/create/create.html.tmpl
@@ +699,3 @@
>  
>              <table cellpadding="0" cellspacing="0">
>                <tr>

the layout of the fields on create_bug is wrong - it should match the current layout (two columns, tracking/project flags first).

Comment 7

[David Lawrence [:dkl]]

Attachment: 880829_2.patch

Comment 8

[David Lawrence [:dkl]]

Note to glob: I looked and the enter_bug => 1 filter, added by your previous work, is done in Extension.pm for create.html.tmpl (enter_bug.cgi) so we should be good to go with review on this bug.

dkl

Comment 9

[:glob ✱]

Comment on attachment 799114 [details] [diff] [review]
880829_2.patch

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

(In reply to David Lawrence [:dkl] from comment #8)
> Note to glob: I looked and the enter_bug => 1 filter, added by your previous
> work, is done in Extension.pm for create.html.tmpl (enter_bug.cgi) so we
> should be good to go with review on this bug.
true, but the migration script needs to be updated to migrate the enter_bug field.

the layout of the fields on enter_bug doesn't match the current layout.

the thunderbird fields are incorrectly visible on the Core product.
it looks like migrate_flag_visibility incorrectly migrates all possible matches for products in $bmo_tracking_flags, instead of the first valid match.

::: extensions/BMO/Extension.pm
@@ +243,3 @@
>      }
>  
> +    print STDERR "$field_name\n";

oops :)

::: extensions/TrackingFlags/bin/migrate_tracking_flags.pl
@@ +134,5 @@
> +    foreach my $value (@{ $field->legal_values }) {
> +        my $group_name = 'everyone';
> +
> +        if ($field->name =~ /^cf_(blocking|tracking)_/) {
> +            if ($value->name ne '---' && $value->name ne '?') {

this needs to be:
  $value ne '---' && $value !~ /\?$/

the current code doesn't set the group to 'everyone' for blocking_b2g's values (eg "leo?")

::: extensions/TrackingFlags/lib/Constants.pm
@@ +46,5 @@
> +            collapsed   => 1,
> +            sortkey     => 4
> +        },
> +   );
> +    return [ sort { $a->{'sortkey'} <=> $b->{'sortkey'} } @flag_types ];

nit: indentation

Comment 10

[David Lawrence [:dkl]]

Attachment: 880829_3.patch

1. Found the issue in migration script where flags were enabled for more products than they were supposed to. I was copying the cf_visible_in_product which is a tied hash instead of just taking a reference to it. Therefore regexp order was no longer being maintained. 

2. Are you stating that the enter_bug.cgi layout is no longer the same because you feel it should mirror what is on production now or do you not like the new layout? I think the new layout with the tracking flags in the first column with the normal flags in the second column is a better use of space and looks better to me. Also I have combined all of the different tracking flag types into a single table so all of the labels and selects line up properly. Please take a second look.

dkl

Comment 11

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #10)
> because you feel it should mirror what is on production now or do you not
> like the new layout?

the former.  i will have a second look :)

Comment 12

[:glob ✱]

Comment on attachment 809892 [details] [diff] [review]
880829_3.patch

r=glob

the updated layout on enter_bug is much better, nice work :)

note - the migration script doesn't honour the is_obsolete attribute; however as this is only set on one tracking flag (cf_blocking_fx) we can simply delete that flag after migration -- it isn't worth the risk of updating the code for that fix.

Comment 13

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

1) Per convo in #infra, this work requires taking bugzilla offline, and also requires a treeclosure. Linking to the tracking bug#917928 for Saturday's treeclosure.

2) Can you confirm how long this will take? The notice says "...beginning at 12:00pm PST (7:00pm UTC) for approximately 2 hours". If that is "...for 2 hours or less", then cool. If this will need >2hrs, can you give upper bound, for treeclosure announcement?

Comment 14

[David Lawrence [:dkl]]

(In reply to John O'Duinn [:joduinn] from comment #13)
> 2) Can you confirm how long this will take? The notice says "...beginning at
> 12:00pm PST (7:00pm UTC) for approximately 2 hours". If that is "...for 2
> hours or less", then cool. If this will need >2hrs, can you give upper
> bound, for treeclosure announcement?

In theory it will be less than 2 hours. The actual migration script takes around 20-30 mins. We want to 
spend some time doing verification afterwards and then roll back if there are any issues we spot before
bring Bugzilla back up. The verification should hopefully not take too long.

dkl

Comment 15

[:glob ✱]

note bug 909878 is the bug which is tracking the downtime, and related issues.

Comment 16

[David Lawrence [:dkl]]

I went through the migration on bugzilla-dev.allizom.org and all went well til the end. The user 'dev_user' does not have rights to DROP TABLE so when we run the script we will need to update the db_user in localconfig to a user which has the proper rights.

Failed SQL: [DROP TABLE cf_tracking_seamonkey223] Error: DBD::mysql::db do failed: DROP command denied to user 'dev_user'@'10.22.70.209' for table 'cf_tracking_seamonkey223' [for Statement "DROP TABLE cf_tracking_seamonkey223"] at /data/www/bugzilla-dev.allizom.org/extensions/TrackingFlags/bin/../../../Bugzilla/DB.pm line 1015.

dkl

Comment 17

[Sheeri Cabral [:sheeri]]

added the DROP permission to stage and prod users, will remove after the migration.

Comment 18

[David Lawrence [:dkl]]

Migration complete. Took the whole 2 hours as we also did stage before prod which added a small amount of time. 

Encountered the normal errors due to the cf_blocking_192 and cf_status192 tables but after manual clean up everything seemed to be fine.

I did quite a bit of sanity checking and all seemed well so far so we will hear soon if anything is not right. 

Closing this for now.

Thanks for everyone's help!

dkl