Merge lp:~longbow/percona-xtrabackup/fix_691090 into lp:percona-xtrabackup/2.0

[Valentine Gostev]

xtrabackup_suspended and xtrabackup_checkpoints now always created in tmp_dir.

previsously modified tests were added as separate tests

[Stewart Smith (stewart)]

Please also update the documentation as the documentation for the behavior of --suspend-at-end and --stream doesn't exist (and it should).

review: Needs Fixing

[Alexey Kopytov (akopytov)]

I would also like to see a separate test case rather than a modification of existing one(s).

review: Needs Fixing

[Alexey Kopytov (akopytov)]

I don't see any point in using the sakila database in the test cases. It is rather big, so it just increases test suite execution time while contributing nothing to test coverage.

In fact, the bug is about placing auxiliary files in datadir. So you don't even have to create any database in test cases. Just start mysqld, change datadir permissions and try to back up. There's no need for prepare/restore/etc. It's not what the bug was about.

I.e. just testing that innobackupex does not fail in the backup mode would be sufficient.

review: Needs Fixing

[Valentine Gostev (longbow)]

If we care about test execution time, previous approach was better way

[Alexey Kopytov (akopytov)]

I prefer tests to be orthogonal, i.e. have a single and clearly defined objective. Having test cases that try to test multiple things at a time leads to a mess when some functionality is changed and you have to modify/adjust them.

[Alexey Kopytov (akopytov)]

By the way, don't forget to reference the bug number in the test case comments.

[Valentine Gostev (longbow)]

In general I agree, but making a separate test case anyway causes additional time expenses (unpacking tarball with server + system db install) compared to reloading sakila. See updated diff

[Alexey Kopytov (akopytov)]

Reducing a test execution time when the functionality is unaffected is
okay. Optimizing tests at the cost of their functionality is usually not
okay.

Server tarballs are currently not unpacked for every test. Instead, the
existing server installation is reused.

[Alexey Kopytov (akopytov)]

Please add comments referencing the bug number (see my previous comment about this).

And while you are at it, you don't have to call stop_mysqld at the end of tests now, it is called from run.sh anyway.

review: Needs Fixing

[Stewart Smith (stewart)]

On Wed, 26 Oct 2011 07:16:23 -0000, Valentine Gostev <email address hidden> wrote:
> In general I agree, but making a separate test case anyway causes
> additional time expenses (unpacking tarball with server + system db
> install) compared to reloading sakila. See updated diff

we can fix execution time a number of ways in the future - having the
small, specific test cases though makes dev a lot easier.

e.g. the problem with the mysql test suite is that there are way too
many giant tests that test 143 things, making the first step of solving
a test failure being rewriting a test for the part of the test that
actuall fails and not the 100 other things that pass.
--
Stewart Smith

[lp:~longbow/percona-xtrabackup/fix_691090]

318. By Valentine Gostev on 2011-10-28

Files xtrabackup_checkpoints and xtrabackup_suspended should be kept in tmp
dir, since user running innobackupex may not have permissions to write mysql
datadir.

Modified xb_basic and xb_stream tests to make datadir readonly while
innobackupex is running.

[Alexey Kopytov (akopytov)]

The problem with the tests is that if innobackupex fails for whatever reasons with permissions on $mysql_datadir modified, that directory cannot be removed, because the current user has no write access. So it stays there forever.

What's worse is that a Jenkins run for this MP has already poisoned slaves. There are failures like this: http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-param/80/BUILD_TYPE=release,Host=el6-64,xtrabackuptarget=xtradb55/testReport/junit/%28root%29/t_xb_perm_stream/sh/. And there are phantom xb_perm_* test failures even when the branch being tests does not have those tests!

review: Needs Fixing

Unmerged revisions

318. By Valentine Gostev on 2011-10-28

Files xtrabackup_checkpoints and xtrabackup_suspended should be kept in tmp
dir, since user running innobackupex may not have permissions to write mysql
datadir.

Modified xb_basic and xb_stream tests to make datadir readonly while
innobackupex is running.

=== modified file 'innobackupex'
--- innobackupex	2011-10-21 17:47:31 +0000
+++ innobackupex	2011-10-26 07:13:20 +0000
@@ -419,17 +419,17 @@
             and Die "Failed to scp file '$option_remote_host:$backup_dir/xtrabackup_logfile': $!";
         unlink $tmp_logfile || Die "Failed to delete '$tmp_logfile': $!";
 
-        system("scp $option_scp_opt '$orig_datadir/xtrabackup_checkpoints' '$option_remote_host:$backup_dir/xtrabackup_checkpoints'")
+        system("scp $option_scp_opt '$option_tmpdir/xtrabackup_checkpoints' '$option_remote_host:$backup_dir/xtrabackup_checkpoints'")
             and Die "Failed to scp file '$option_remote_host:$backup_dir/xtrabackup_checkpoints': $!";
-        unlink "$orig_datadir/xtrabackup_checkpoints" || Die "Failed to delete '$orig_datadir/xtrabackup_checkpoints': $!";
+        unlink "$option_tmpdir/xtrabackup_checkpoints" || Die "Failed to delete '$option_tmpdir/xtrabackup_checkpoints': $!";
     } elsif ($option_stream eq 'tar') {
         system("cd $option_tmpdir; tar chf - xtrabackup_logfile")
             and Die "Failed to stream 'xtrabackup_logfile': $!";
         unlink $tmp_logfile || Die "Failed to delete '$tmp_logfile': $!";
 
-        system("cd $orig_datadir; tar chf - xtrabackup_checkpoints")
+        system("cd $option_tmpdir; tar chf - xtrabackup_checkpoints")
             and Die "Failed to stream 'xtrabackup_checkpoints': $!";
-        unlink "$orig_datadir/xtrabackup_checkpoints" || Die "Failed to delete '$orig_datadir/xtrabackup_checkpoints': $!";
+        unlink "$option_tmpdir/xtrabackup_checkpoints" || Die "Failed to delete '$option_tmpdir/xtrabackup_checkpoints': $!";
     }
 
     print STDERR "\n$prefix Backup created in directory '$backup_dir'\n";
@@ -758,7 +758,7 @@
         $options = $options . " --target-dir=$backup_dir";
     } else {
         #(datadir) for 'xtrabackup_suspended' and 'xtrabackup_checkpoints'
-        $options = $options . " --log-stream --target-dir=./";
+        $options = $options . " --log-stream --target-dir=" . $option_tmpdir;
     }
 
     # prepare command line for running ibbackup
@@ -1469,7 +1469,7 @@
         $binlog_info = $backup_dir . '/xtrabackup_binlog_info';
         $slave_info = $backup_dir . '/xtrabackup_slave_info';
         } else {
-        $suspend_file = get_option(\%config, 'mysqld', 'datadir') . '/xtrabackup_suspended';
+        $suspend_file = $option_tmpdir . '/xtrabackup_suspended';
         $tmp_logfile = $option_tmpdir . '/xtrabackup_logfile';
         $mysql_stdout = $option_tmpdir . '/mysql-stdout';
         $mysql_stderr = $option_tmpdir . '/mysql-stderr';

=== added file 'test/t/xb_perm_basic.sh'
--- test/t/xb_perm_basic.sh	1970-01-01 00:00:00 +0000
+++ test/t/xb_perm_basic.sh	2011-10-26 07:13:20 +0000
@@ -0,0 +1,11 @@
+. inc/common.sh
+
+init
+run_mysqld
+
+mkdir -p $topdir/backup
+chmod -R 500 $mysql_datadir
+innobackupex  --no-timestamp $topdir/backup
+chmod -R 700 $mysql_datadir
+
+stop_mysqld

=== added file 'test/t/xb_perm_stream.sh'
--- test/t/xb_perm_stream.sh	1970-01-01 00:00:00 +0000
+++ test/t/xb_perm_stream.sh	2011-10-26 07:13:20 +0000
@@ -0,0 +1,12 @@
+. inc/common.sh
+
+init
+run_mysqld
+
+# Take backup
+chmod -R 500 $mysql_datadir
+mkdir -p $topdir/backup
+innobackupex --stream=tar $topdir/backup > $topdir/backup/out.tar
+chmod -R 700 $mysql_datadir
+
+stop_mysqld