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

RESOLVED FIXED

Status

()

bugzilla.mozilla.org
Extensions: TrackingFlags
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dkl, Assigned: dkl)

Tracking

Production
Dependency tree / graph

Details

(Whiteboard: [ateamtrack: p=bugzilla q=2013q2 m=3])

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
Created attachment 759926 [details]
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
Attachment #759926 - Flags: review?(glob)
Attachment #759926 - Attachment is patch: false
Created attachment 761426 [details] [diff] [review]
migrate_tracking_flags.pl.patch

reattaching as a patch so i can use splinter :)
Attachment #759926 - Attachment is obsolete: true
Attachment #759926 - Flags: review?(glob)
Attachment #761426 - Flags: review?(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.
Attachment #761426 - Flags: review?(glob) → review-

Updated

4 years ago
Blocks: 885464
(Assignee)

Comment 3

4 years ago
Created attachment 765745 [details] [diff] [review]
migrate_tracking_flags.pl.patch
Attachment #761426 - Attachment is obsolete: true
Attachment #765745 - Flags: review?(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
Attachment #765745 - Flags: review?(glob) → review+
(Assignee)

Comment 5

4 years ago
Created attachment 793728 [details] [diff] [review]
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
Attachment #765745 - Attachment is obsolete: true
Attachment #793728 - Flags: review?(glob)
(Assignee)

Updated

4 years ago
Blocks: 908427
Depends on: 909843
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).
Attachment #793728 - Flags: review?(glob) → review-
(Assignee)

Updated

4 years ago
Blocks: 909878
(Assignee)

Comment 7

4 years ago
Created attachment 799114 [details] [diff] [review]
880829_2.patch
Attachment #793728 - Attachment is obsolete: true
Attachment #799114 - Flags: review?(glob)
(Assignee)

Comment 8

4 years ago
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 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
Attachment #799114 - Flags: review?(glob) → review-
Depends on: 917203
(Assignee)

Updated

4 years ago
Blocks: 897810
(Assignee)

Comment 10

4 years ago
Created attachment 809892 [details] [diff] [review]
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
Attachment #799114 - Attachment is obsolete: true
Attachment #809892 - Flags: review?(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 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.
Attachment #809892 - Flags: review?(glob) → review+
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?
Blocks: 917928
Flags: needinfo?(glob)
Flags: needinfo?(dkl)
(Assignee)

Comment 14

4 years ago
(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
Flags: needinfo?(dkl)

Updated

4 years ago
No longer blocks: 917928
note bug 909878 is the bug which is tracking the downtime, and related issues.
Flags: needinfo?(glob)
(Assignee)

Comment 16

4 years ago
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
added the DROP permission to stage and prod users, will remove after the migration.
(Assignee)

Comment 18

4 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Depends on: 926109
You need to log in before you can comment on or make changes to this bug.