Merge lp:~percona-toolkit-dev/percona-toolkit/pxc-autodiscover-nodes into lp:percona-toolkit/2.2

[Brian Fraser]

[Daniel Nichter (daniel-nichter)]

I'll focus on changed to lib/Percona/XtraDB/Cluster.pm:

268 +sub find_cluster_nodes {
269 + my ($self, %args) = @_;
270 +
271 + my $dbh = $args{dbh};

Let's use and check @required_args.

###

287 + my @addresses = grep !/\Aunspecified\z/i,
288 + split /,\s*/, $addresses;

Let's use the prevailing style: grep { ... } split ..., e.g like later:

407 + my @new_slave_nodes = grep { $self->is_cluster_node($_) } @new_slaves;

###

314 + $node_dbh->disconnect();
315 +
316 + push @nodes, $make_cxn->(dsn => $node_dsn);

Can we just keep the already connected node dbh instead of disconnect and re-connecting later? I think Cxn::new() can be passed a live dbh?

###

319 + return @nodes;

Return \@nodes. This makes testing easier:

my $got = find_cluster_nodes(...);
is_deeply(
   $got,
   $expected,
   "..."
) or diag(Dumper($got));

I think returning lists is best for small subs that are always used in list contexts, but there's no specific "rules" about this.

###

334 + my ($self, %args) = @_;
335 + my @cxns = @{$args{cxns}};

Use/check @required_args.

###

337 + PTDEBUG && _d("Removing duplicates from ", join " ", map { $_->name } @cxns);

I would make the join() clearer in this case by using parentheses.

###

340 + for my $cxn ( @cxns ) {

Use foreach like you did earlier in other place (and because it's the established style).

###

342 + my $sql = q{SELECT @@SERVER_ID};

Use @@server_id. That's the informal standard, i.e. like how FUNCTIONS() are informally always uppercase.

###

351 + PTDEBUG && _d("Removing ", $cxn->name,
352 + ", ID $id, because we've already seen it");

$id will probably always be defined, but if not, that PTDEBUG will throw an error.

###

356 + return @trimmed_cxns;

Return arrayref here too.

###

367 + my ($self, %args) = @_;
368 + my $ms = $args{MasterSlave};

Use @required_args.

###

377 + for my $node ( @$nodes ) {

Use foreach.

###

09 + return @new_nodes, @new_slaves,
410 + $self->autodetect_nodes(
411 + %args,
412 + nodes => \@new_slave_nodes,
413 + );

That's too complex, simplify. Return something like { new_nodes => $new_nodes, ... }, no function call in the return call.

###

248 +=item --[no]autodetect-nodes

Let's no add more options. I think we can add "cluster" as a value to --recursion-method.

review: Needs Fixing

[lp:~percona-toolkit-dev/percona-toolkit/pxc-autodiscover-nodes]

569. By Daniel Nichter on 2013-04-04

Merge version-in-all-bash-tools-bug-821502.

570. By Daniel Nichter on 2013-04-04

Merge hostname-bug-947893-2.2.

571. By Daniel Nichter on 2013-04-04

Merge pt-heartbeat-utc-bug-1163372-2.2.

572. By Brian Fraser on 2013-04-12

Rebased the branch to trunk

[Daniel Nichter (daniel-nichter)]

Still need to

334 + my ($self, %args) = @_;
335 + my @cxns = @{$args{cxns}};

Use/check @required_args.

and use "foreach" instead of "for", but the major stuff is done/fixed.

review: Approve

=== modified file 'bin/pt-table-checksum'
--- bin/pt-table-checksum	2013-04-04 21:45:50 +0000
+++ bin/pt-table-checksum	2013-04-12 16:10:32 +0000
@@ -3581,6 +3581,32 @@
    return $self->{hostname} || $self->{dsn_name} || 'unknown host';
 }
 
+sub remove_duplicate_cxns {
+   my ($self, %args) = @_;
+   my @cxns     = @{$args{cxns}};
+   my $seen_ids = $args{seen_ids} || {};
+   PTDEBUG && _d("Removing duplicates from ", join(" ", map { $_->name } @cxns));
+   my @trimmed_cxns;
+
+   for my $cxn ( @cxns ) {
+      my $dbh  = $cxn->dbh();
+      my $sql  = q{SELECT @@server_id};
+      PTDEBUG && _d($sql);
+      my ($id) = $dbh->selectrow_array($sql);
+      PTDEBUG && _d('Server ID for ', $cxn->name, ': ', $id);
+
+      if ( ! $seen_ids->{$id}++ ) {
+         push @trimmed_cxns, $cxn
+      }
+      else {
+         PTDEBUG && _d("Removing ", $cxn->name,
+                       ", ID ", $id, ", because we've already seen it");
+      }
+   }
+
+   return \@trimmed_cxns;
+}
+
 sub DESTROY {
    my ($self) = @_;
 
@@ -3634,6 +3660,8 @@
 use Lmo;
 use Data::Dumper;
 
+{ local $EVAL_ERROR; eval { require Cxn } };
+
 sub get_cluster_name {
    my ($self, $cxn) = @_;
    my $sql = "SHOW VARIABLES LIKE 'wsrep\_cluster\_name'";
@@ -3667,6 +3695,57 @@
    return ($val1 || '') eq ($val2 || '');
 }
 
+sub find_cluster_nodes {
+   my ($self, %args) = @_;
+
+   my $dbh = $args{dbh};
+   my $dsn = $args{dsn};
+   my $dp  = $args{DSNParser};
+   my $make_cxn = $args{make_cxn};
+
+   
+   my $sql = q{SHOW STATUS LIKE 'wsrep\_incoming\_addresses'};
+   PTDEBUG && _d($sql);
+   my (undef, $addresses) = $dbh->selectrow_array($sql);
+   PTDEBUG && _d("Cluster nodes found: ", $addresses);
+   return unless $addresses;
+
+   my @addresses = grep { !/\Aunspecified\z/i }
+                   split /,\s*/, $addresses;
+
+   my @nodes;
+   foreach my $address ( @addresses ) {
+      my ($host, $port) = split /:/, $address;
+      my $spec = "h=$host"
+               . ($port ? ",P=$port" : "");
+      my $node_dsn = $dp->parse($spec, $dsn);
+      my $node_dbh = eval { $dp->get_dbh(
+            $dp->get_cxn_params($node_dsn), { AutoCommit => 1 }) };
+      if ( $EVAL_ERROR ) {
+         print STDERR "Cannot connect to ", $dp->as_string($node_dsn),
+                      ", discovered through $sql: $EVAL_ERROR\n";
+         if ( !$port && $dsn->{P} != 3306 ) {
+            $address .= ":3306";
+            redo;
+         }
+         next;
+      }
+      PTDEBUG && _d('Connected to', $dp->as_string($node_dsn));
+      $node_dbh->disconnect();
+
+      push @nodes, $make_cxn->(dsn => $node_dsn);
+   }
+
+   return \@nodes;
+}
+
+sub remove_duplicate_cxns {
+   my ($self, %args) = @_;
+   my @cxns     = @{$args{cxns}};
+   my $seen_ids = $args{seen_ids};
+   return Cxn->remove_duplicate_cxns(%args);
+}
+
 sub same_cluster {
    my ($self, $cxn1, $cxn2) = @_;
 
@@ -3678,6 +3757,59 @@
    return ($cluster1 || '') eq ($cluster2 || '');
 }
 
+sub autodetect_nodes {
+   my ($self, %args) = @_;
+   my $ms       = $args{MasterSlave};
+   my $dp       = $args{DSNParser};
+   my $make_cxn = $args{make_cxn};
+   my $nodes    = $args{nodes};
+   my $seen_ids = $args{seen_ids};
+
+   my $new_nodes = [];
+
+   return $new_nodes unless @$nodes;
+   
+   for my $node ( @$nodes ) {
+      my $nodes_found = $self->find_cluster_nodes(
+         dbh       => $node->dbh(),
+         dsn       => $node->dsn(),
+         make_cxn  => $make_cxn,
+         DSNParser => $dp,
+      );
+      push @$new_nodes, @$nodes_found;
+   }
+
+   $new_nodes = $self->remove_duplicate_cxns(
+      cxns     => $new_nodes,
+      seen_ids => $seen_ids
+   );
+
+   my $new_slaves = [];
+   foreach my $node (@$new_nodes) {
+      my $node_slaves = $ms->get_slaves(
+         dbh      => $node->dbh(),
+         dsn      => $node->dsn(),
+         make_cxn => $make_cxn,
+      );
+      push @$new_slaves, @$node_slaves;
+   }
+
+   $new_slaves = $self->remove_duplicate_cxns(
+      cxns     => $new_slaves,
+      seen_ids => $seen_ids
+   );
+
+   my @new_slave_nodes = grep { $self->is_cluster_node($_) } @$new_slaves;
+   
+   my $slaves_of_slaves = $self->autodetect_nodes(
+         %args,
+         nodes => \@new_slave_nodes,
+   );
+   
+   my @autodetected_nodes = ( @$new_nodes, @$new_slaves, @$slaves_of_slaves );
+   return \@autodetected_nodes;
+}
+
 sub _d {
    my ($package, undef, $line) = caller 0;
    @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }
@@ -4770,6 +4902,8 @@
    my $dp      = $self->{DSNParser};
    my $methods = $self->_resolve_recursion_methods($args{dsn});
 
+   return $slaves unless @$methods;
+   
    if ( grep { m/processlist|hosts/i } @$methods ) {
       my @required_args = qw(dbh dsn);
       foreach my $arg ( @required_args ) {
@@ -8776,11 +8910,22 @@
       }
    }
 
+   my $autodiscover_cluster;
+   my $recursion_method = [];
+   foreach my $method ( @{$o->get('recursion-method')} ) {
+      if ( $method eq 'cluster' ) {
+         $autodiscover_cluster = 1;
+      }
+      else {
+         push @$recursion_method, $method
+      }
+   }
+   $o->set('recursion-method', $recursion_method);
    eval {
       MasterSlave::check_recursion_method($o->get('recursion-method'));
    };
    if ( $EVAL_ERROR ) {
-      $o->save_error("Invalid --recursion-method: $EVAL_ERROR")
+      $o->save_error($EVAL_ERROR)
    }
 
    $o->usage_or_errors();
@@ -8959,33 +9104,44 @@
       # #####################################################################
       # Find and connect to slaves.
       # #####################################################################
+      my $make_cxn_cluster = sub {
+         my $cxn = $make_cxn->(@_, prev_dsn => $master_cxn->dsn());
+         $cluster_name_for{$cxn} = $cluster->is_cluster_node($cxn);
+         return $cxn;
+      };
+
       $slaves = $ms->get_slaves(
          dbh      => $master_dbh,
          dsn      => $master_dsn,
-         make_cxn => sub {
-            my $cxn = $make_cxn->(@_, prev_dsn => $master_cxn->dsn());
-            $cluster_name_for{$cxn} = $cluster->is_cluster_node($cxn);
-            return $cxn;
-         },
+         make_cxn => $make_cxn_cluster,
       );
 
-      # If the "master" is a cluster node, then a DSN table should have been
-      # used, and it may have all nodes' DSNs so the user can run the tool
-      # on any node, in which case it has the "master" node, the DSN given
-      # on the command line.  So detect and remove this dupe.
-      if ( $cluster_name_for{$master_cxn} ) {
-         @$slaves = grep {
-            my $slave_cxn = $_;
-            if ( $cluster->same_node($master_cxn, $slave_cxn) ) {
-               PTDEBUG && _d('Removing ', $slave_cxn->name, 'from slaves',
-                  'because it is the master');
-               0;
-            }
-            else {
-               $slave_cxn;
-            }
-         } @$slaves;
-      }
+      my %seen_ids;
+      for my $cxn ($master_cxn, @$slaves) {
+         my $dbh  = $cxn->dbh();
+         my $sql  = q{SELECT @@server_id};
+         PTDEBUG && _d($cxn, $dbh, $sql);
+         my ($id) = $dbh->selectrow_array($sql);
+         $seen_ids{$id}++;
+      }
+
+      if ( $autodiscover_cluster ) {
+         my @known_nodes = grep { $cluster_name_for{$_} } $master_cxn, @$slaves;
+         my $new_cxns = $cluster->autodetect_nodes(
+                           nodes       => \@known_nodes,
+                           MasterSlave => $ms,
+                           DSNParser   => $dp,
+                           make_cxn    => $make_cxn_cluster,
+                           seen_ids    => \%seen_ids,
+                        );
+         push @$slaves, @$new_cxns;
+      }
+
+      my $trimmed_nodes = Cxn->remove_duplicate_cxns(
+                                   cxns => [ $master_cxn, @$slaves ],
+                                );
+      ($master_cxn, @$slaves) = @$trimmed_nodes;
+
       PTDEBUG && _d(scalar @$slaves, 'slaves found');
       if ( !@$slaves && $o->get('recursion-method')->[0] ne 'none' ) {
          $exit_status |= 1;
@@ -9090,7 +9246,7 @@
                warn "Diffs will only be detected if the cluster is "
                   . "consistent with " . $direct_slave->name . " because "
                   . $master_cxn->name . " is a traditional replication master "
-                  . " but these replicas are cluster nodes:\n"
+                  . "but these replicas are cluster nodes:\n"
                   . join("\n", map { '  ' . $_->name } @nodes) . "\n"
                   . "For more information, please read the Percona XtraDB "
                   . "Cluster section of the tool's documentation.\n";
@@ -11765,6 +11921,7 @@
   ===========  ==================
   processlist  SHOW PROCESSLIST
   hosts        SHOW SLAVE HOSTS
+  cluster      SHOW STATUS LIKE 'wsrep\_incoming\_addresses'
   dsn=DSN      DSNs from a table
   none         Do not find slaves
 
@@ -11775,6 +11932,13 @@
 The C<hosts> method requires replicas to be configured with C<report_host>,
 C<report_port>, etc.
 
+The C<cluster> method requires a cluster based on Galera 23.7.3 or newer,
+such as Percona XtraDB Cluster versions 5.5.29 and above.  This will
+autodiscover nodes in a cluster using C<SHOW STATUS LIKE 'wsrep\_incoming\_addresses'>.
+Note that you can combine C<cluster> with C<processlist> and C<hosts> to
+have the tool autodiscover all cluster nodes and traditional slaves,
+but this is considered highly experimental.
+
 The C<dsn> method is special: rather than automatically discovering replicas,
 this method specifies a table with replica DSNs.  The tool will only connect
 to these replicas.  This method works best when replicas do not use the same

=== modified file 'lib/Cxn.pm'
--- lib/Cxn.pm	2013-04-04 21:31:21 +0000
+++ lib/Cxn.pm	2013-04-12 16:10:32 +0000
@@ -220,6 +220,42 @@
    return $self->{hostname} || $self->{dsn_name} || 'unknown host';
 }
 
+# There's two reasons why there might be dupes:
+# If the "master" is a cluster node, then a DSN table might have been
+# used, and it may have all nodes' DSNs so the user can run the tool
+# on any node, in which case it has the "master" node, the DSN given
+# on the command line.
+# On the other hand, maybe find_cluster_nodes worked, in which case
+# we definitely have a dupe for the master cxn, but we may also have a
+# dupe for every other node if this was unsed in conjunction with a
+# DSN table.
+# So try to detect and remove those.
+sub remove_duplicate_cxns {
+   my ($self, %args) = @_;
+   my @cxns     = @{$args{cxns}};
+   my $seen_ids = $args{seen_ids} || {};
+   PTDEBUG && _d("Removing duplicates from ", join(" ", map { $_->name } @cxns));
+   my @trimmed_cxns;
+
+   for my $cxn ( @cxns ) {
+      my $dbh  = $cxn->dbh();
+      my $sql  = q{SELECT @@server_id};
+      PTDEBUG && _d($sql);
+      my ($id) = $dbh->selectrow_array($sql);
+      PTDEBUG && _d('Server ID for ', $cxn->name, ': ', $id);
+
+      if ( ! $seen_ids->{$id}++ ) {
+         push @trimmed_cxns, $cxn
+      }
+      else {
+         PTDEBUG && _d("Removing ", $cxn->name,
+                       ", ID ", $id, ", because we've already seen it");
+      }
+   }
+
+   return \@trimmed_cxns;
+}
+
 sub DESTROY {
    my ($self) = @_;
 

=== modified file 'lib/MasterSlave.pm'
--- lib/MasterSlave.pm	2013-01-03 00:19:16 +0000
+++ lib/MasterSlave.pm	2013-04-12 16:10:32 +0000
@@ -73,6 +73,8 @@
    my $dp      = $self->{DSNParser};
    my $methods = $self->_resolve_recursion_methods($args{dsn});
 
+   return $slaves unless @$methods;
+   
    if ( grep { m/processlist|hosts/i } @$methods ) {
       my @required_args = qw(dbh dsn);
       foreach my $arg ( @required_args ) {
@@ -114,7 +116,6 @@
    my ($self, $dsn) = @_;
    my $o = $self->{OptionParser};
    if ( $o->got('recursion-method') ) {
-      # Use whatever the user explicitly gave on the command line.
       return $o->get('recursion-method');
    }
    elsif ( $dsn && ($dsn->{P} || 3306) != 3306 ) {

=== modified file 'lib/Percona/XtraDB/Cluster.pm'
--- lib/Percona/XtraDB/Cluster.pm	2013-02-01 17:06:13 +0000
+++ lib/Percona/XtraDB/Cluster.pm	2013-04-12 16:10:32 +0000
@@ -30,6 +30,8 @@
 use Lmo;
 use Data::Dumper;
 
+{ local $EVAL_ERROR; eval { require Cxn } };
+
 sub get_cluster_name {
    my ($self, $cxn) = @_;
    my $sql = "SHOW VARIABLES LIKE 'wsrep\_cluster\_name'";
@@ -63,6 +65,70 @@
    return ($val1 || '') eq ($val2 || '');
 }
 
+# TODO: Check that the PXC version supports wsrep_incoming_addresses
+# Not really necessary, actually. But in case it's needed,
+# wsrep_provider_version =~ /[0-9]+\.[0-9]+\(r([0-9]+)\)/ && $1 >= 137
+sub find_cluster_nodes {
+   my ($self, %args) = @_;
+
+   my $dbh = $args{dbh};
+   my $dsn = $args{dsn};
+   my $dp  = $args{DSNParser};
+   my $make_cxn = $args{make_cxn};
+
+   # Ostensibly the caller should've done this already, but
+   # useful for safety.
+   # TODO this fails with a strange error.
+   #$dp->fill_in_dsn($dbh, $dsn);
+   
+   my $sql = q{SHOW STATUS LIKE 'wsrep\_incoming\_addresses'};
+   PTDEBUG && _d($sql);
+   my (undef, $addresses) = $dbh->selectrow_array($sql);
+   PTDEBUG && _d("Cluster nodes found: ", $addresses);
+   return unless $addresses;
+
+   my @addresses = grep { !/\Aunspecified\z/i }
+                   split /,\s*/, $addresses;
+
+   my @nodes;
+   foreach my $address ( @addresses ) {
+      my ($host, $port) = split /:/, $address;
+      my $spec = "h=$host"
+               . ($port ? ",P=$port" : "");
+      my $node_dsn = $dp->parse($spec, $dsn);
+      my $node_dbh = eval { $dp->get_dbh(
+            $dp->get_cxn_params($node_dsn), { AutoCommit => 1 }) };
+      if ( $EVAL_ERROR ) {
+         print STDERR "Cannot connect to ", $dp->as_string($node_dsn),
+                      ", discovered through $sql: $EVAL_ERROR\n";
+         # This is a bit strange, so an explanation is called for.
+         # If there wasn't a port, that means that this bug
+         # https://bugs.launchpad.net/percona-toolkit/+bug/1082406
+         # isn't fixed on this version of PXC. We tried using the
+         # master's port, but that didn't work. So try again, using
+         # the default port.
+         if ( !$port && $dsn->{P} != 3306 ) {
+            $address .= ":3306";
+            redo;
+         }
+         next;
+      }
+      PTDEBUG && _d('Connected to', $dp->as_string($node_dsn));
+      $node_dbh->disconnect();
+
+      push @nodes, $make_cxn->(dsn => $node_dsn);
+   }
+
+   return \@nodes;
+}
+
+sub remove_duplicate_cxns {
+   my ($self, %args) = @_;
+   my @cxns     = @{$args{cxns}};
+   my $seen_ids = $args{seen_ids};
+   return Cxn->remove_duplicate_cxns(%args);
+}
+
 sub same_cluster {
    my ($self, $cxn1, $cxn2) = @_;
 
@@ -75,6 +141,61 @@
    return ($cluster1 || '') eq ($cluster2 || '');
 }
 
+sub autodetect_nodes {
+   my ($self, %args) = @_;
+   my $ms       = $args{MasterSlave};
+   my $dp       = $args{DSNParser};
+   my $make_cxn = $args{make_cxn};
+   my $nodes    = $args{nodes};
+   my $seen_ids = $args{seen_ids};
+
+   my $new_nodes = [];
+
+   return $new_nodes unless @$nodes;
+   
+   for my $node ( @$nodes ) {
+      my $nodes_found = $self->find_cluster_nodes(
+         dbh       => $node->dbh(),
+         dsn       => $node->dsn(),
+         make_cxn  => $make_cxn,
+         DSNParser => $dp,
+      );
+      push @$new_nodes, @$nodes_found;
+   }
+
+   $new_nodes = $self->remove_duplicate_cxns(
+      cxns     => $new_nodes,
+      seen_ids => $seen_ids
+   );
+
+   my $new_slaves = [];
+   foreach my $node (@$new_nodes) {
+      my $node_slaves = $ms->get_slaves(
+         dbh      => $node->dbh(),
+         dsn      => $node->dsn(),
+         make_cxn => $make_cxn,
+      );
+      push @$new_slaves, @$node_slaves;
+   }
+
+   $new_slaves = $self->remove_duplicate_cxns(
+      cxns     => $new_slaves,
+      seen_ids => $seen_ids
+   );
+
+   # If some of the new slaves is a cluster node, autodetect new nodes
+   # from there too.
+   my @new_slave_nodes = grep { $self->is_cluster_node($_) } @$new_slaves;
+   
+   my $slaves_of_slaves = $self->autodetect_nodes(
+         %args,
+         nodes => \@new_slave_nodes,
+   );
+   
+   my @autodetected_nodes = ( @$new_nodes, @$new_slaves, @$slaves_of_slaves );
+   return \@autodetected_nodes;
+}
+
 sub _d {
    my ($package, undef, $line) = caller 0;
    @_ = map { (my $temp = $_) =~ s/\n/\n# /g; $temp; }

=== modified file 'sandbox/servers/pxc/5.5/my.sandbox.cnf'
--- sandbox/servers/pxc/5.5/my.sandbox.cnf	2012-10-15 16:53:37 +0000
+++ sandbox/servers/pxc/5.5/my.sandbox.cnf	2013-04-12 16:10:32 +0000
@@ -31,7 +31,7 @@
 wsrep_provider             = LIBGALERA
 wsrep_cluster_address      = CLUSTER_AD
 wsrep_sst_receive_address  = ADDR:RECEIVE_PRT
-wsrep_node_incoming_address= ADDR
+wsrep_node_incoming_address= ADDR:PORT
 wsrep_slave_threads        = 2
 wsrep_cluster_name         = CLUSTER_NAME
 wsrep_provider_options     = "gmcast.listen_addr=tcp://ADDR:LISTEN_PRT;"

=== modified file 't/pt-table-checksum/pxc.t'
--- t/pt-table-checksum/pxc.t	2013-03-08 18:07:05 +0000
+++ t/pt-table-checksum/pxc.t	2013-04-12 16:10:32 +0000
@@ -22,6 +22,8 @@
 use Sandbox;
 require "$trunk/bin/pt-table-checksum";
 
+my $ip = qr/\Q127.1\E|\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}/;
+
 my $dp = new DSNParser(opts=>$dsn_opts);
 my $sb = new Sandbox(basedir => '/tmp', DSNParser => $dp);
 my $node1 = $sb->get_dbh_for('node1');
@@ -75,34 +77,41 @@
 
 like(
    $output,
-   qr/h=127.1,P=12345 is a cluster node but no other nodes/,
+   qr/h=127(?:\Q.0.0\E)?.1,P=12345 is a cluster node but no other nodes/,
    "Dies if no other nodes are found"
 );
 
-$output = output(
-   sub { pt_table_checksum::main(@args,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns")
-   },
-   stderr => 1,
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'errors'),
-   0,
-   "No diffs: no errors"
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'skipped'),
-   0,
-   "No diffs: no skips"
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'diffs'),
-   0,
-   "No diffs: no diffs"
-);
+for my $args (
+      ["using recusion-method=dsn", '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns"],
+      ["using recursion-method=cluster", '--recursion-method', 'cluster']
+   )
+{
+   my $test = shift @$args;
+   $output = output(
+      sub { pt_table_checksum::main(@args,
+         @$args)
+      },
+      stderr => 1,
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'errors'),
+      0,
+      "No diffs: no errors ($test)"
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'skipped'),
+      0,
+      "No diffs: no skips ($test)"
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'diffs'),
+      0,
+      "No diffs: no diffs ($test)"
+   );
+}
 
 # Now really test checksumming a cluster.  To create a diff we have to disable
 # the binlog.  Although PXC doesn't need or use the binlog to communicate
@@ -135,45 +144,53 @@
    "Node3 not changed"
 );
 
-$output = output(
-   sub { pt_table_checksum::main(@args,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns")
-   },
-   stderr => 1,
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'errors'),
-   0,
-   "1 diff: no errors"
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'skipped'),
-   0,
-   "1 diff: no skips"
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'diffs'),
-   1,
-   "1 diff: 1 diff"
-) or diag($output);
-
-# 11-17T13:02:54      0      1       26       1       0   0.021 test.t
-like(
-   $output,
-   qr/^\S+\s+  # ts
-      0\s+     # errors
-      1\s+     # diffs
-      26\s+    # rows
-      \d+\s+   # chunks
-      0\s+     # skipped
-      \S+\s+   # time
-      test.t$  # table
-   /xm,
-   "1 diff: it's in test.t"
-);
+for my $args (
+      ["using recusion-method=dsn", '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns"],
+      ["using recursion-method=cluster", '--recursion-method', 'cluster']
+   )
+{
+   my $test = shift @$args;
+
+   $output = output(
+      sub { pt_table_checksum::main(@args,
+         @$args)
+      },
+      stderr => 1,
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'errors'),
+      0,
+      "1 diff: no errors ($test)"
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'skipped'),
+      0,
+      "1 diff: no skips ($test)"
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'diffs'),
+      1,
+      "1 diff: 1 diff ($test)"
+   ) or diag($output);
+
+   # 11-17T13:02:54      0      1       26       1       0   0.021 test.t
+   like(
+      $output,
+      qr/^\S+\s+  # ts
+         0\s+     # errors
+         1\s+     # diffs
+         26\s+    # rows
+         \d+\s+   # chunks
+         0\s+     # skipped
+         \S+\s+   # time
+         test.t$  # table
+      /xm,
+      "1 diff: it's in test.t ($test)"
+   );
+}
 
 # #############################################################################
 # cluster, node1 -> slave, run on node1
@@ -205,33 +222,48 @@
 # https://bugs.launchpad.net/percona-toolkit/+bug/1080385
 # Cluster nodes default to ROW format because that's what Galeara
 # works best with, even though it doesn't really use binlogs.
-$output = output(
-   sub { pt_table_checksum::main(@args,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns")
-   },
-   stderr => 1,
-);
-
-like(
-   $output,
-   qr/replica h=127.1,P=12348 has binlog_format ROW/i,
-   "--check-binlog-format warns about slave's binlog format"
-);
-
-# Now really test that diffs on the slave are detected.
-$output = output(
-   sub { pt_table_checksum::main(@args,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns",
-      qw(--no-check-binlog-format)),
-   },
-   stderr => 1,
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'diffs'),
-   1,
-   "Detects diffs on slave of cluster node1"
-) or diag($output);
+for my $args (
+      ["using recusion-method=dsn", '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns"],
+      ["using recursion-method=cluster,hosts", '--recursion-method', 'cluster,hosts']
+   )
+{
+   my $test = shift @$args;
+
+   # Wait for the slave to apply the binlogs from node1 (its master).
+   # Then change it so it's not consistent.
+   PerconaTest::wait_for_table($slave_dbh, 'test.t');
+   $sb->wait_for_slaves('cslave1');
+   $slave_dbh->do("update test.t set c='zebra' where c='z'");
+
+   $output = output(
+      sub { pt_table_checksum::main(@args,
+         @$args)
+      },
+      stderr => 1,
+   );
+
+   like(
+      $output,
+      qr/replica h=127(?:\Q.0.0\E)?\.1,P=12348 has binlog_format ROW/i,
+      "--check-binlog-format warns about slave's binlog format ($test)"
+   );
+   
+   # Now really test that diffs on the slave are detected.
+   $output = output(
+      sub { pt_table_checksum::main(@args,
+         @$args,
+         qw(--no-check-binlog-format)),
+      },
+      stderr => 1,
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'diffs'),
+      1,
+      "Detects diffs on slave of cluster node1 ($test)"
+   ) or diag($output);
+
+}
 
 $slave_dbh->disconnect;
 $sb->stop_sandbox('cslave1');
@@ -267,20 +299,28 @@
    "Slave is changed"
 );
 
-$output = output(
-   sub { pt_table_checksum::main(@args,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns",
-      qw(--no-check-binlog-format -d test)),
-   },
-   stderr => 1,
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'diffs'),
-   0,
-   "Limitation: does not detect diffs on slave of cluster node2"
-) or diag($output);
-
+for my $args (
+      ["using recusion-method=dsn", '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns"],
+      ["using recursion-method=cluster,hosts", '--recursion-method', 'cluster,hosts']
+   )
+{
+   my $test = shift @$args;
+
+   $output = output(
+      sub { pt_table_checksum::main(@args,
+         @$args,
+         qw(--no-check-binlog-format -d test)),
+      },
+      stderr => 1,
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'diffs'),
+      0,
+      "Limitation: does not detect diffs on slave of cluster node2 ($test)"
+   ) or diag($output);
+}
+   
 $slave_dbh->disconnect;
 $sb->stop_sandbox('cslave1');
 
@@ -454,96 +494,113 @@
 
 like(
    $output,
-   qr/the direct replica of h=127.1,P=12349 was not found or specified/,
+   qr/the direct replica of h=$ip,P=12349 was not found or specified/,
    "Warns that direct replica of the master isn't found or specified",
 );
 
 # Use the other DSN table with all three nodes.  Now the tool should
 # give a more specific warning than that ^.
-$output = output(
-   sub { pt_table_checksum::main($master_dsn,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns",
-      qw(-d test))
-   },
-   stderr => 1,
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'diffs'),
-   1,
-   "...check all nodes: 1 diff"
-) or diag($output);
-
-# 11-17T13:02:54      0      1       26       1       0   0.021 test.t
-like(
-   $output,
-   qr/^\S+\s+  # ts
-      0\s+     # errors
-      1\s+     # diffs
-      26\s+    # rows
-      \d+\s+   # chunks
-      0\s+     # skipped
-      \S+\s+   # time
-      test.t$  # table
-   /xm,
-   "...check all nodes: it's in test.t"
-);
-
-like(
-   $output,
-   qr/Diffs will only be detected if the cluster is consistent with h=127.1,P=12345 because h=127.1,P=12349/,
-   "Warns that diffs only detected if cluster consistent with direct replica",
-);
-
-# Restore node1 so the cluster is consistent, but then make node2 differ.
-# ptc should NOT detect this diff because the checksum query will replicate
-# to node1, node1 isn't different, so it broadcasts the result in ROW format
-# that all is ok, which node2 gets and thus false reports.  This is why
-# those ^ warnings exist.
-$node1->do("set sql_log_bin=0");
-$node1->do("update test.t set c='z' where c='zebra'");
-$node1->do("set sql_log_bin=1");
-
-$node2->do("set sql_log_bin=0");
-$node2->do("update test.t set c='zebra' where c='z'");
-$node2->do("set sql_log_bin=1");
-
-($row) = $node2->selectrow_array("select c from test.t order by c desc limit 1");
-is(
-   $row,
-   "zebra",
-   "Node2 is changed again"
-);
-
-($row) = $node1->selectrow_array("select c from test.t order by c desc limit 1");
-is(
-   $row,
-   "z",
-   "Node1 not changed again"
-);
-
-($row) = $node3->selectrow_array("select c from test.t order by c desc limit 1");
-is(
-   $row,
-   "z",
-   "Node3 not changed again"
-);
-
-# the other DSN table with all three nodes, but it won't matter because
-# node1 is going to broadcast the false-positive that there are no diffs.
-$output = output(
-   sub { pt_table_checksum::main($master_dsn,
-      '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns",
-      qw(-d test))
-   },
-   stderr => 1,
-);
-
-is(
-   PerconaTest::count_checksum_results($output, 'diffs'),
-   0,
-   "Limitation: diff not on direct replica not detected"
-) or diag($output);
+# Originally, these tested a dsn table with all nodes; now we hijack
+# those tests to also try the autodetection
+for my $args (
+      ["using recusion-method=dsn", '--recursion-method', "dsn=$node1_dsn,D=dsns,t=dsns"],
+      ["using recursion-method=cluster,hosts", '--recursion-method', 'cluster,hosts']
+   )
+{
+   my $test = shift @$args;
+
+   # Make a diff on node1.  If ptc is really auto-detecting node1, then it
+   # should report this diff.
+   $node1->do("set sql_log_bin=0");
+   $node1->do("update test.t set c='zebra' where c='z'");
+   $node1->do("set sql_log_bin=1");
+   
+   $output = output(
+      sub { pt_table_checksum::main($master_dsn,
+         @$args,
+         qw(-d test))
+      },
+      stderr => 1,
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'diffs'),
+      1,
+      "...check all nodes: 1 diff ($test)"
+   ) or diag($output);
+
+   # 11-17T13:02:54      0      1       26       1       0   0.021 test.t
+   like(
+      $output,
+      qr/^\S+\s+  # ts
+         0\s+     # errors
+         1\s+     # diffs
+         26\s+    # rows
+         \d+\s+   # chunks
+         0\s+     # skipped
+         \S+\s+   # time
+         test.t$  # table
+      /xm,
+      "...check all nodes: it's in test.t ($test)"
+   );
+
+   like(
+      $output,
+      qr/Diffs will only be detected if the cluster is consistent with h=$ip,P=12345 because h=$ip,P=12349/,
+      "Warns that diffs only detected if cluster consistent with direct replica ($test)",
+   );
+
+   # Restore node1 so the cluster is consistent, but then make node2 differ.
+   # ptc should NOT detect this diff because the checksum query will replicate
+   # to node1, node1 isn't different, so it broadcasts the result in ROW format
+   # that all is ok, which node2 gets and thus false reports.  This is why
+   # those ^ warnings exist.
+   $node1->do("set sql_log_bin=0");
+   $node1->do("update test.t set c='z' where c='zebra'");
+   $node1->do("set sql_log_bin=1");
+
+   $node2->do("set sql_log_bin=0");
+   $node2->do("update test.t set c='zebra' where c='z'");
+   $node2->do("set sql_log_bin=1");
+
+   ($row) = $node2->selectrow_array("select c from test.t order by c desc limit 1");
+   is(
+      $row,
+      "zebra",
+      "Node2 is changed again ($test)"
+   );
+
+   ($row) = $node1->selectrow_array("select c from test.t order by c desc limit 1");
+   is(
+      $row,
+      "z",
+      "Node1 not changed again ($test)"
+   );
+
+   ($row) = $node3->selectrow_array("select c from test.t order by c desc limit 1");
+   is(
+      $row,
+      "z",
+      "Node3 not changed again ($test)"
+   );
+
+   # the other DSN table with all three nodes, but it won't matter because
+   # node1 is going to broadcast the false-positive that there are no diffs.
+   $output = output(
+      sub { pt_table_checksum::main($master_dsn,
+         @$args,
+         qw(-d test))
+      },
+      stderr => 1,
+   );
+
+   is(
+      PerconaTest::count_checksum_results($output, 'diffs'),
+      0,
+      "Limitation: diff not on direct replica not detected ($test)"
+   ) or diag($output);
+
+}
 
 # ###########################################################################
 # Be sure to stop the slave on node1, else further test will die with:
@@ -587,13 +644,13 @@
 
 like(
    $output,
-   qr/h=127.1,P=12345 is in cluster pt_sandbox_cluster/,
+   qr/h=127(?:\Q.0.0\E)?.1,P=12345 is in cluster pt_sandbox_cluster/,
    "Detects that node1 is in pt_sandbox_cluster"
 );
 
 like(
    $output,
-   qr/h=127.1,P=2900 is in cluster cluster2/,
+   qr/h=127(?:\Q.0.0\E)?.1,P=2900 is in cluster cluster2/,
    "Detects that node4 is in cluster2"
 );
 
@@ -602,7 +659,7 @@
    qr/test/,
    "Different clusters, no results"
 );
-
+   
 $sb->stop_sandbox(qw(node4 node5 node6));
 
 # Restore the DSN table in case there are more tests.