Merge lp:~percona-toolkit-dev/percona-toolkit/validate-load-options-bug-996915 into lp:percona-toolkit/2.1

[Daniel Nichter]

[Daniel Nichter (daniel-nichter)]

review: Approve

=== modified file 'bin/pt-online-schema-change'
--- bin/pt-online-schema-change	2012-05-25 16:24:32 +0000
+++ bin/pt-online-schema-change	2012-05-25 21:39:18 +0000
@@ -3701,18 +3701,24 @@
    }
 
    PTDEBUG && _d('Parsing spec for max thresholds');
-   my $max_val_for = _parse_spec(
-      spec             => $args{max_spec},
-      get_status       => $args{get_status},
-      threshold_factor => 0.2, # +20%
-   );
+   my $max_val_for = _parse_spec($args{max_spec});
+   if ( $max_val_for ) {
+      _check_and_set_vals(
+         vars             => $max_val_for,
+         get_status       => $args{get_status},
+         threshold_factor => 0.2, # +20%
+      );
+   }
 
    PTDEBUG && _d('Parsing spec for critical thresholds');
-   my $critical_val_for = _parse_spec(
-      spec             => $args{critical_spec} || [],
-      get_status       => $args{get_status},
-      threshold_factor => 1.0, # double (x2; +100%)
-   );
+   my $critical_val_for = _parse_spec($args{critical_spec} || []);
+   if ( $critical_val_for ) {
+      _check_and_set_vals(
+         vars             => $critical_val_for,
+         get_status       => $args{get_status},
+         threshold_factor => 1.0, # double (x2; +100%)
+      );
+   }
 
    my $self = {
       get_status       => $args{get_status},
@@ -3726,27 +3732,29 @@
 }
 
 sub _parse_spec {
-   my ( %args ) = @_;
-   my @required_args = qw(spec get_status);
-   foreach my $arg ( @required_args ) {
-      die "I need a $arg argument" unless defined $args{$arg};
-   }
-   my ($spec, $get_status) = @args{@required_args};
+   my ($spec) = @_;
 
    return unless $spec && scalar @$spec;
-   my $threshold_factor = $args{threshold_factor} || 0.20;
 
    my %max_val_for;
    foreach my $var_val ( @$spec ) {
+      die "Empty or undefined spec\n" unless $var_val;
+      $var_val =~ s/^\s+//;
+      $var_val =~ s/\s+$//g;
+
       my ($var, $val) = split /[:=]/, $var_val;
-      die "Invalid spec: $var_val" unless $var;
+      die "$var_val does not contain a variable\n" unless $var;
+      die "$var is not a variable name\n" unless $var =~ m/^[a-zA-Z_]+$/;
+
       if ( !$val ) {
-         my $init_val = $get_status->($var);
-         PTDEBUG && _d('Initial', $var, 'value:', $init_val);
-         $val = int(($init_val * $threshold_factor) + $init_val);
-      }
-      PTDEBUG && _d('Wait if', $var, '>=', $val);
-      $max_val_for{$var} = $val;
+         PTDEBUG && _d('Will get intial value for', $var, 'later');
+         $max_val_for{$var} = undef;
+      }
+      else {
+         die "The value for $var must be a number\n"
+            unless $val =~ m/^[\d\.]+$/;
+         $max_val_for{$var} = $val;
+      }
    }
 
    return \%max_val_for; 
@@ -3826,6 +3834,34 @@
    return;
 }
 
+sub _check_and_set_vals {
+   my (%args) = @_;
+   my @required_args = qw(vars get_status threshold_factor);
+   foreach my $arg ( @required_args ) {
+      die "I need a $arg argument" unless defined $args{$arg};
+   }
+   my ($vars, $get_status, $threshold_factor) = @args{@required_args};
+
+   PTDEBUG && _d('Checking and setting values');
+   return unless $vars && scalar %$vars;
+
+   foreach my $var ( keys %$vars ) {
+      my $init_val = $get_status->($var);
+      die "Variable $var does not exist or its value is undefined\n"
+         unless defined $init_val;
+      my $val;
+      if ( defined $vars->{$var} ) {
+         $val = $vars->{$var};
+      }
+      else {
+         PTDEBUG && _d('Initial', $var, 'value:', $init_val);
+         $val = int(($init_val * $threshold_factor) + $init_val);
+         $vars->{$var} = $val;
+      }
+      PTDEBUG && _d('Wait if', $var, '>=', $val);
+   }
+}
+
 sub _d {
    my ($package, undef, $line) = caller 0;
    @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }
@@ -4979,6 +5015,18 @@
    # Explicit --chunk-size disable auto chunk sizing.
    $o->set('chunk-time', 0) if $o->got('chunk-size');
 
+   foreach my $opt ( qw(max-load critical-load) ) {
+      next unless $o->has($opt);
+      my $spec = $o->get($opt);
+      eval {
+         MySQLStatusWaiter::_parse_spec($o->get($opt));
+      };
+      if ( $EVAL_ERROR ) {
+         chomp $EVAL_ERROR;
+         $o->save_error("Invalid --$opt: $EVAL_ERROR");
+      }
+   }
+
    if ( !$o->get('help') ) {
       if ( @ARGV ) {
          $o->save_error('Specify only one DSN on the command line');
@@ -5226,13 +5274,25 @@
          };
       }
 
-      $sys_load = new MySQLStatusWaiter(
-         max_spec      => $o->get('max-load'),
-         critical_spec => $o->get('critical-load'),
-         get_status    => $get_status,
-         oktorun       => sub { return $oktorun },
-         sleep         => $sleep,
-      );
+      eval {
+         $sys_load = new MySQLStatusWaiter(
+            max_spec      => $o->get('max-load'),
+            critical_spec => $o->get('critical-load'),
+            get_status    => $get_status,
+            oktorun       => sub { return $oktorun },
+            sleep         => $sleep,
+         );
+      };
+      if ( $EVAL_ERROR ) {
+         chomp $EVAL_ERROR;
+         die "Error checking --max-load or --critial-load: $EVAL_ERROR.  "
+            . "Check that the variables specified for --max-load and "
+            . "--critical-load are spelled correctly and exist in "
+            . "SHOW GLOBAL STATUS.  Current values for these options are:\n"
+            . "  --max-load " . (join(',', @{$o->get('max-load')})) . "\n"
+            . "  --critial-load "  . (join(',', @{$o->get('critical-load')}))
+            . "\n";
+      }
       
       if ( $o->get('progress') ) {
          $replica_lag_pr = new Progress(
 
=== modified file 'bin/pt-table-checksum'
--- bin/pt-table-checksum	2012-05-24 17:25:20 +0000
+++ bin/pt-table-checksum	2012-05-25 21:39:18 +0000
@@ -5653,18 +5653,24 @@
    }
 
    PTDEBUG && _d('Parsing spec for max thresholds');
-   my $max_val_for = _parse_spec(
-      spec             => $args{max_spec},
-      get_status       => $args{get_status},
-      threshold_factor => 0.2, # +20%
-   );
+   my $max_val_for = _parse_spec($args{max_spec});
+   if ( $max_val_for ) {
+      _check_and_set_vals(
+         vars             => $max_val_for,
+         get_status       => $args{get_status},
+         threshold_factor => 0.2, # +20%
+      );
+   }
 
    PTDEBUG && _d('Parsing spec for critical thresholds');
-   my $critical_val_for = _parse_spec(
-      spec             => $args{critical_spec} || [],
-      get_status       => $args{get_status},
-      threshold_factor => 1.0, # double (x2; +100%)
-   );
+   my $critical_val_for = _parse_spec($args{critical_spec} || []);
+   if ( $critical_val_for ) {
+      _check_and_set_vals(
+         vars             => $critical_val_for,
+         get_status       => $args{get_status},
+         threshold_factor => 1.0, # double (x2; +100%)
+      );
+   }
 
    my $self = {
       get_status       => $args{get_status},
@@ -5678,27 +5684,29 @@
 }
 
 sub _parse_spec {
-   my ( %args ) = @_;
-   my @required_args = qw(spec get_status);
-   foreach my $arg ( @required_args ) {
-      die "I need a $arg argument" unless defined $args{$arg};
-   }
-   my ($spec, $get_status) = @args{@required_args};
+   my ($spec) = @_;
 
    return unless $spec && scalar @$spec;
-   my $threshold_factor = $args{threshold_factor} || 0.20;
 
    my %max_val_for;
    foreach my $var_val ( @$spec ) {
+      die "Empty or undefined spec\n" unless $var_val;
+      $var_val =~ s/^\s+//;
+      $var_val =~ s/\s+$//g;
+
       my ($var, $val) = split /[:=]/, $var_val;
-      die "Invalid spec: $var_val" unless $var;
+      die "$var_val does not contain a variable\n" unless $var;
+      die "$var is not a variable name\n" unless $var =~ m/^[a-zA-Z_]+$/;
+
       if ( !$val ) {
-         my $init_val = $get_status->($var);
-         PTDEBUG && _d('Initial', $var, 'value:', $init_val);
-         $val = int(($init_val * $threshold_factor) + $init_val);
-      }
-      PTDEBUG && _d('Wait if', $var, '>=', $val);
-      $max_val_for{$var} = $val;
+         PTDEBUG && _d('Will get intial value for', $var, 'later');
+         $max_val_for{$var} = undef;
+      }
+      else {
+         die "The value for $var must be a number\n"
+            unless $val =~ m/^[\d\.]+$/;
+         $max_val_for{$var} = $val;
+      }
    }
 
    return \%max_val_for; 
@@ -5778,6 +5786,34 @@
    return;
 }
 
+sub _check_and_set_vals {
+   my (%args) = @_;
+   my @required_args = qw(vars get_status threshold_factor);
+   foreach my $arg ( @required_args ) {
+      die "I need a $arg argument" unless defined $args{$arg};
+   }
+   my ($vars, $get_status, $threshold_factor) = @args{@required_args};
+
+   PTDEBUG && _d('Checking and setting values');
+   return unless $vars && scalar %$vars;
+
+   foreach my $var ( keys %$vars ) {
+      my $init_val = $get_status->($var);
+      die "Variable $var does not exist or its value is undefined\n"
+         unless defined $init_val;
+      my $val;
+      if ( defined $vars->{$var} ) {
+         $val = $vars->{$var};
+      }
+      else {
+         PTDEBUG && _d('Initial', $var, 'value:', $init_val);
+         $val = int(($init_val * $threshold_factor) + $init_val);
+         $vars->{$var} = $val;
+      }
+      PTDEBUG && _d('Wait if', $var, '>=', $val);
+   }
+}
+
 sub _d {
    my ($package, undef, $line) = caller 0;
    @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }
@@ -5940,6 +5976,18 @@
 
    $o->set('chunk-time', 0) if $o->got('chunk-size');
 
+   foreach my $opt ( qw(max-load critical-load) ) {
+      next unless $o->has($opt);
+      my $spec = $o->get($opt);
+      eval {
+         MySQLStatusWaiter::_parse_spec($o->get($opt));
+      };
+      if ( $EVAL_ERROR ) {
+         chomp $EVAL_ERROR;
+         $o->save_error("Invalid --$opt: $EVAL_ERROR");
+      }
+   }
+
    if ( !$o->get('help') ) {
       if ( @ARGV > 1 ) {
          $o->save_error("More than one host specified; only one allowed");
@@ -6299,12 +6347,22 @@
          };
       }
 
-      $sys_load = new MySQLStatusWaiter(
-         max_spec   => $o->get('max-load'),
-         get_status => $get_status,
-         oktorun    => sub { return $oktorun },
-         sleep      => $sleep,
-      );
+      eval {
+         $sys_load = new MySQLStatusWaiter(
+            max_spec   => $o->get('max-load'),
+            get_status => $get_status,
+            oktorun    => sub { return $oktorun },
+            sleep      => $sleep,
+         );
+      };
+      if ( $EVAL_ERROR ) {
+         chomp $EVAL_ERROR;
+         die "Error checking --max-load: $EVAL_ERROR.  "
+            . "Check that the variables specified for --max-load "
+            . "are spelled correctly and exist in "
+            . "SHOW GLOBAL STATUS.  Current value for this option is:\n"
+            . "  --max-load " . (join(',', @{$o->get('max-load')})) . "\n";
+      }
       
       if ( $o->get('progress') ) {
          $replica_lag_pr = new Progress(
 
=== modified file 'lib/MySQLStatusWaiter.pm'
--- lib/MySQLStatusWaiter.pm	2012-03-28 01:17:17 +0000
+++ lib/MySQLStatusWaiter.pm	2012-05-25 21:39:18 +0000
@@ -45,18 +45,24 @@
    }
 
    PTDEBUG && _d('Parsing spec for max thresholds');
-   my $max_val_for = _parse_spec(
-      spec             => $args{max_spec},
-      get_status       => $args{get_status},
-      threshold_factor => 0.2, # +20%
-   );
+   my $max_val_for = _parse_spec($args{max_spec});
+   if ( $max_val_for ) {
+      _check_and_set_vals(
+         vars             => $max_val_for,
+         get_status       => $args{get_status},
+         threshold_factor => 0.2, # +20%
+      );
+   }
 
    PTDEBUG && _d('Parsing spec for critical thresholds');
-   my $critical_val_for = _parse_spec(
-      spec             => $args{critical_spec} || [],
-      get_status       => $args{get_status},
-      threshold_factor => 1.0, # double (x2; +100%)
-   );
+   my $critical_val_for = _parse_spec($args{critical_spec} || []);
+   if ( $critical_val_for ) {
+      _check_and_set_vals(
+         vars             => $critical_val_for,
+         get_status       => $args{get_status},
+         threshold_factor => 1.0, # double (x2; +100%)
+      );
+   }
 
    my $self = {
       get_status       => $args{get_status},
@@ -79,27 +85,29 @@
 # Returns:
 #   Hashref with each variable's maximum permitted value.
 sub _parse_spec {
-   my ( %args ) = @_;
-   my @required_args = qw(spec get_status);
-   foreach my $arg ( @required_args ) {
-      die "I need a $arg argument" unless defined $args{$arg};
-   }
-   my ($spec, $get_status) = @args{@required_args};
+   my ($spec) = @_;
 
    return unless $spec && scalar @$spec;
-   my $threshold_factor = $args{threshold_factor} || 0.20;
 
    my %max_val_for;
    foreach my $var_val ( @$spec ) {
+      die "Empty or undefined spec\n" unless $var_val;
+      $var_val =~ s/^\s+//;
+      $var_val =~ s/\s+$//g;
+
       my ($var, $val) = split /[:=]/, $var_val;
-      die "Invalid spec: $var_val" unless $var;
+      die "$var_val does not contain a variable\n" unless $var;
+      die "$var is not a variable name\n" unless $var =~ m/^[a-zA-Z_]+$/;
+
       if ( !$val ) {
-         my $init_val = $get_status->($var);
-         PTDEBUG && _d('Initial', $var, 'value:', $init_val);
-         $val = int(($init_val * $threshold_factor) + $init_val);
-      }
-      PTDEBUG && _d('Wait if', $var, '>=', $val);
-      $max_val_for{$var} = $val;
+         PTDEBUG && _d('Will get intial value for', $var, 'later');
+         $max_val_for{$var} = undef;
+      }
+      else {
+         die "The value for $var must be a number\n"
+            unless $val =~ m/^[\d\.]+$/;
+         $max_val_for{$var} = $val;
+      }
    }
 
    return \%max_val_for; 
@@ -192,6 +200,34 @@
    return;
 }
 
+sub _check_and_set_vals {
+   my (%args) = @_;
+   my @required_args = qw(vars get_status threshold_factor);
+   foreach my $arg ( @required_args ) {
+      die "I need a $arg argument" unless defined $args{$arg};
+   }
+   my ($vars, $get_status, $threshold_factor) = @args{@required_args};
+
+   PTDEBUG && _d('Checking and setting values');
+   return unless $vars && scalar %$vars;
+
+   foreach my $var ( keys %$vars ) {
+      my $init_val = $get_status->($var);
+      die "Variable $var does not exist or its value is undefined\n"
+         unless defined $init_val;
+      my $val;
+      if ( defined $vars->{$var} ) {
+         $val = $vars->{$var};
+      }
+      else {
+         PTDEBUG && _d('Initial', $var, 'value:', $init_val);
+         $val = int(($init_val * $threshold_factor) + $init_val);
+         $vars->{$var} = $val;
+      }
+      PTDEBUG && _d('Wait if', $var, '>=', $val);
+   }
+}
+
 sub _d {
    my ($package, undef, $line) = caller 0;
    @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }
 
=== modified file 't/lib/MySQLStatusWaiter.t'
--- t/lib/MySQLStatusWaiter.t	2012-03-28 01:17:17 +0000
+++ t/lib/MySQLStatusWaiter.t	2012-05-25 21:39:18 +0000
@@ -9,7 +9,7 @@
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 14;
+use Test::More tests => 17;
 
 use MySQLStatusWaiter;
 use PerconaTest;
@@ -39,11 +39,48 @@
    return;
 }
 
+# #############################################################################
+# _parse_spec()
+# #############################################################################
+
+throws_ok(
+   sub { new MySQLStatusWaiter(
+      max_spec => [qw(100)],
+      get_status => \&get_status,
+      sleep      => \&sleep,
+      oktorun    => \&oktorun,
+   ) },
+   qr/100 is not a variable name/,
+   "Catch non-variable name"
+);
+
+throws_ok(
+   sub { new MySQLStatusWaiter(
+      max_spec => [qw(foo=bar)],
+      get_status => \&get_status,
+      sleep      => \&sleep,
+      oktorun    => \&oktorun,
+   ) },
+   qr/value for foo must be a number/,
+   "Catch non-number value"
+);
+
+throws_ok(
+   sub { new MySQLStatusWaiter(
+      max_spec => [qw(foo)],
+      get_status => \&get_status,
+      sleep      => \&sleep,
+      oktorun    => \&oktorun,
+   ) },
+   qr/foo does not exist/,
+   "Catch non-existent variable"
+);
+
 # ############################################################################
 # Use initial vals + 20%.
 # ############################################################################
 @vals = (
-   # initial values
+   # initial check for existence
    { Threads_connected => 10, },
    { Threads_running   => 5,  },
 
@@ -68,6 +105,8 @@
    { Threads_running   => 5,  },
 );
 
+$oktorun = 1;
+
 my $sw = new MySQLStatusWaiter(
    oktorun    => \&oktorun,
    get_status => \&get_status,
@@ -127,6 +166,10 @@
 # Use static vals.
 # ############################################################################
 @vals = (
+   # initial check for existence
+   { Threads_connected => 1, },
+   { Threads_running   => 1, },
+
    # first check, no wait
    { Threads_connected => 1, },
    { Threads_running   => 1, },
@@ -208,6 +251,10 @@
 # Critical thresholds (with static vals).
 # ############################################################################
 @vals = (
+   # initial check for existence
+   { Threads_running => 1, },
+   { Threads_running => 9, },
+
    # first check, no wait
    { Threads_running => 1, },
    { Threads_running => 9, },
 
=== modified file 't/pt-online-schema-change/option_sanity.t'
--- t/pt-online-schema-change/option_sanity.t	2012-05-25 16:24:32 +0000
+++ t/pt-online-schema-change/option_sanity.t	2012-05-25 21:39:18 +0000
@@ -9,7 +9,7 @@
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 6;
+use Test::More tests => 8;
 
 use PerconaTest;
 
@@ -58,6 +58,20 @@
    "Cannot --alter-foreign-keys-method=drop_swap with --no-drop-new-table"
 );
 
+$output = `$cmd h=127.1,P=12345,u=msandbox,p=msandbox,D=mysql,t=user --max-load 100 --alter "ENGINE=MyISAM" --dry-run`;
+like(
+   $output,
+   qr/Invalid --max-load/,
+   "Validates --max-load"
+);
+
+$output = `$cmd h=127.1,P=12345,u=msandbox,p=msandbox,D=mysql,t=user --critical-load 100 --alter "ENGINE=MyISAM" --dry-run`;
+like(
+   $output,
+   qr/Invalid --critical-load/,
+   "Validates --critical-load"
+);
+
 # #############################################################################
 # Done.
 # #############################################################################
 
=== modified file 't/pt-table-checksum/option_sanity.t'
--- t/pt-table-checksum/option_sanity.t	2011-12-27 17:24:35 +0000
+++ t/pt-table-checksum/option_sanity.t	2012-05-25 21:39:18 +0000
@@ -9,7 +9,7 @@
 use strict;
 use warnings FATAL => 'all';
 use English qw(-no_match_vars);
-use Test::More tests => 18;
+use Test::More tests => 19;
 
 use PerconaTest;
 shift @INC;  # our unshift (above)
@@ -157,6 +157,17 @@
 );
 
 # #############################################################################
+# --max-load
+# #############################################################################
+
+$output = `$trunk/bin/pt-table-checksum h=127.1,P=12345 --max-load 100`;
+like(
+   $output,
+   qr/Invalid --max-load/,
+   "Validates --max-load"
+);
+
+# #############################################################################
 # Done.
 # #############################################################################
 exit;