Merge lp:~sergei.glushchenko/percona-xtrabackup/bug489290_targetdir_rel_pwd into lp:percona-xtrabackup/2.0

[Sergei Glushchenko]

Behaviour of '--target-dir' option changed. If relative path specified, it considered to be relative to the directory where xtrabackup executed.

Jenkins: http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-trunk-kewpie/21/
(it may be a bit hard to see if it breaks anything as everything isn't passing everywhere yet)

[Alexey Kopytov (akopytov)]

Sergei,

On 16.01.12 17:55, Sergei Glushchenko wrote:

> Jenkins: http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-trunk-kewpie/21/
> (it may be a bit hard to see if it breaks anything as everything isn't passing everywhere yet)
>

Um, it's insanity. I suggest we don't use kewpie until we have a clean
baseline. Can you please rewrite that test case to the old testing
framework?

> +/**************************************************************************
> +Expands target dir name to full path name if it is relative.*/
> +static void
> +expand_target_dir_name()
> +{
> + char current_dir[FN_REFLEN];
> + size_t current_dir_len;
> +
> + /* xtrabackup_target_dir should be relative to pwd, not to datadir.
> + convert it to absolute path if it's relative */
> + if (xtrabackup_target_dir && (xtrabackup_target_dir[0] != FN_LIBCHAR))
> + {
> + if (my_getwd(current_dir, sizeof(current_dir), MYF(MY_WME)))
> + {
> + fprintf(stderr, "xtrabackup: cannot my_getwd\n");

You don't have to do that. With the MY_WME flag my_getwd() will print
the following message on error:

Can't get working directory (Errcode: %d)

So there's no need for an explicit error message. I know it's a
copy-paste from elsewhere, but that code does not have to do that either.

> + exit(EXIT_FAILURE);
> + }
> + current_dir_len = strlen(current_dir);
> + strnmov(current_dir + current_dir_len, xtrabackup_target_dir,
> + sizeof(current_dir) - current_dir_len - 1);
> + cleanup_dirname(xtrabackup_real_target_dir, current_dir);
> + xtrabackup_target_dir = xtrabackup_real_target_dir;

Wouldn't cleanup_dirname(xtrabackup_target_dir, xtrabackup_target_dir)
be sufficient? It just looks like you're duplicating a part of
cleanup_dirname().

Or, even better, why not use unpack_dirname() to allow "~" in
--target-dir argument?

> + }
> +
> +}
> +
> static void
> xtrabackup_backup_func(void)
> {
> @@ -3706,6 +3732,9 @@
> fprintf(stderr, "xtrabackup: uses posix_fadvise().\n");
> #endif
>
> + /* ensure target dir will not be relative to datadir */
> + expand_target_dir_name();
> +
> /* cd to datadir */
>
> if (my_setwd(mysql_real_data_home,MYF(MY_WME)))
> @@ -4622,6 +4651,9 @@
> {
> ulint n;
>
> + /* ensure target dir will not be relative to datadir */
> + expand_target_dir_name();
> +

Would calling expand_target_dir_name() or cleanup_dirname() or
unpack_dirname() from get_one_option() be a better place to do the
conversion to avoid code duplication?

> /* cd to datadir */
>
> if (my_setwd(mysql_real_data_home,MYF(MY_WME)))
>
 needsfixing

[Alexey Kopytov (akopytov)]

Please also merge multiple revisions into a single one.

review: Needs Fixing

[Sergei Glushchenko (sergei.glushchenko)]

Aleksey,

> > Jenkins: http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-trunk-kewpie/21/
> > (it may be a bit hard to see if it breaks anything as everything isn't passing everywhere yet)
> >
>
> Um, it's insanity. I suggest we don't use kewpie until we have a clean
> baseline. Can you please rewrite that test case to the old testing
> framework?

That is probably so but it wasn't my initiative to use kewpie for this test.

> Would calling expand_target_dir_name() or cleanup_dirname() or
> unpack_dirname() from get_one_option() be a better place to do the
> conversion to avoid code duplication?

Yes. It will help us to avoid code duplication, but in this case we will do extra work when using xtrabackup with --prepare option. Besides it wouldn't help us if we're not specifying --target-dir option.

> You don't have to do that. With the MY_WME flag my_getwd() will print
> the following message on error:
>
> Can't get working directory (Errcode: %d)

I thought that it is the good practice to start error message with "xtrabackup:".
It could help when doing output parsing.

> Wouldn't cleanup_dirname(xtrabackup_target_dir, xtrabackup_target_dir)
> be sufficient? It just looks like you're duplicating a part of
> cleanup_dirname().

You wouldn't beleive it, but after call to normalize_dirname, cleanup_dirname, etc string "./xtrabackup_backupfiles/" will stay the same. I'm using cleanup_dirname in order to "cleanup" dirname, for example to remove extra "/./", "//", etc.

[Sergei Glushchenko (sergei.glushchenko)]

What's wrong if I make multiple revisions in one branch?

[Alexey Kopytov (akopytov)]

Sergei,

On 24.01.12 9:06, Sergei Glushchenko wrote:
> Aleksey,
>
>>> Jenkins: http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-trunk-kewpie/21/
>>> (it may be a bit hard to see if it breaks anything as everything isn't passing everywhere yet)
>>>
>>
>> Um, it's insanity. I suggest we don't use kewpie until we have a clean
>> baseline. Can you please rewrite that test case to the old testing
>> framework?
>
> That is probably so but it wasn't my initiative to use kewpie for this test.
>

I think it's a good initiative to use something when it's actually ready
to be used, and don't use it when it's not.

>> Would calling expand_target_dir_name() or cleanup_dirname() or
>> unpack_dirname() from get_one_option() be a better place to do the
>> conversion to avoid code duplication?
>
> Yes. It will help us to avoid code duplication, but in this case we will do extra work when using xtrabackup with --prepare option.

Why?

> Besides it wouldn't help us if we're not specifying --target-dir option.
>

Then you can do it in main() after the call to handle_options.

>> You don't have to do that. With the MY_WME flag my_getwd() will print
>> the following message on error:
>>
>> Can't get working directory (Errcode: %d)
>
> I thought that it is the good practice to start error message with "xtrabackup:".
> It could help when doing output parsing.
>

I don't see any use in it. XtraBackup can print messages starting with
"InnoDB:", "xtrabackup:", or without any prefix. So I don't think there
are tools that depend on a specific format, simply because there is no
consistent format.

>> Wouldn't cleanup_dirname(xtrabackup_target_dir, xtrabackup_target_dir)
>> be sufficient? It just looks like you're duplicating a part of
>> cleanup_dirname().
>
> You wouldn't beleive it, but after call to normalize_dirname, cleanup_dirname, etc string "./xtrabackup_backupfiles/" will stay the same. I'm using cleanup_dirname in order to "cleanup" dirname, for example to remove extra "/./", "//", etc.
>

Right, they don't actually unpack "." at the start. So using
my_realpath() instead should do the trick.

[Alexey Kopytov (akopytov)]

On 24.01.12 9:08, Sergei Glushchenko wrote:
> What's wrong if I make multiple revisions in one branch?

It's not very friendly to people who read the revision history. No
matter what is your goal when you do so, you would like to see something
like "Fixed bug ..." instead of the following:

"
374. By Sergei Glushchenko (sergei.glushchenko: 222) on 2012-01-16

    testcase renamed
373. By Sergei Glushchenko (sergei.glushchenko: 222) on 2012-01-13

    doc change committed
372. By Sergei Glushchenko (sergei.glushchenko: 222) on 2012-01-13

    testcase committed
371. By Sergei Glushchenko (sergei.glushchenko: 222) on 2012-01-13

    fix committed
"

Right?

It's OK to have multiple revisions for your personal use when you are
working on something. But will the above make any sense to a reader once
they are merged? I don't think so. So I always try to merge revisions
before submitting a MP.

When I have to make some changes once the original revision has been
merged, that's a different story of course. In this case it must be a
separate revision.

[Alexey Kopytov (akopytov)]

review: Needs Fixing

[Alexey Kopytov (akopytov)]

I also find it useful when revision comments include the bug number, the bug synopsis, and a few sentences describing the original problem (i.e. the root cause analysis for the bug), and the solution to that problem. This both simplifies code reviews, and increases readability of revision history.

[Sergei Glushchenko (sergei.glushchenko)]

Alexey,

> Right, they don't actually unpack "." at the start. So using
> my_realpath() instead should do the trick.

Thanks for suggestion! Looks like my_realpath() have exactly the same behavior I need.

I will keep kewpie test and will write another one using old framework. That was my first intention actually. But Stewart told me that kewpie will be sufficient.

As for merging multiple revisions into single one, I find that it reasonable. What is the best way to do that? My suggestion is to use "bzr uncommit" multiple times and then "bzr commit" for the whole bunch of changes, and after that "bzr push --overwrite". Is there any better way to do that?

[Alexey Kopytov (akopytov)]

On 24.01.12 15:12, Sergei Glushchenko wrote:
> As for merging multiple revisions into single one, I find that it reasonable. What is the best way to do that? My suggestion is to use "bzr uncommit" multiple times and then "bzr commit" for the whole bunch of changes, and after that "bzr push --overwrite". Is there any better way to do that?

Yes, "bzr uncommit" (either with -r, or simply multiple time) + "bzr
commit" + "bzr push --overwrite", see
https://code.launchpad.net/~hrvojem/percona-xtrabackup/mngtools/+merge/89194/comments/192080

[Sergei Glushchenko (sergei.glushchenko)]

Fix for bug #489290.
Bug description:
 ./xtrabackup --backup
creates database "xtrabackup_backupfiles".
The reasons are following:
1. Default value for target-dir option is "./xtrabackup_backupfiles/"
2. xtrabackup changes current working directory to datadir when backup option is specified
Suggested fix is to use my_load_path for target-dir value before changing working directory. Some code in innobackupex rely on this behaviour (--stream mode) so it was changed too.

new Jenkins build:
http://jenkins.percona.com/view/Percona%20Xtrabackup/job/percona-xtrabackup-param/115/

[Alexey Kopytov (akopytov)]

As discussed on IRC, the fix for this bug essentially fixes a part of bug #489290. So this bugfix depends on the fix for bug #489290 and should be rebased on the latter once its merged to trunk.

review: Needs Resubmitting

[Alexey Kopytov (akopytov)]

Sergei,

Do you really need expand_target_dir_name()? It looks like all you have to do is "my_load_path(xtrabackup_real_target_dir, xtrabackup_target_dir, NULL);". I don't see a reason to wrap that single line into a separate function.

review: Needs Information

[Alexey Kopytov (akopytov)]

Sorry, commented on a wrong MP.

Unmerged revisions

390. By Sergei Glushchenko on 2012-01-30

Bug 489290:
 ./xtrabackup --backup
creates database "xtrabackup_backupfiles".
The reasons are following:
1. Default value for target-dir option is "./xtrabackup_backupfiles/"
2. xtrabackup changes current working directory to datadir when backup option is specified
Solution is to use my_load_path to expand target-dir value before changing working directory.

389. By Stewart Smith on 2012-01-30

merge other patches from 1.6: bug711207

388. By Stewart Smith on 2012-01-30

merge fix for placement of xtrabackup_suspended and xtrabackup_checkpoint

387. By Stewart Smith on 2012-01-30

merge innodb version detection failure fix

386. By Stewart Smith on 2012-01-28

merge 1.9.0 release notes

385. By Stewart Smith on 2012-01-23

merge fix for bug 733658: pass options to both ssh and scp

384. By Stewart Smith on 2012-01-23

merge trademark policy in docs

383. By Stewart Smith on 2012-01-23

merge fix of broken links in docs

382. By Hrvoje Matijakovic on 2012-01-23

Merge from lp:~hrvojem/percona-xtrabackup/bug919203

381. By Hrvoje Matijakovic on 2012-01-19

Merge from lp:~hrvojem/percona-xtrabackup/mngtools.

=== modified file 'doc/source/xtrabackup_bin/creating_a_backup.rst'
--- doc/source/xtrabackup_bin/creating_a_backup.rst	2011-07-07 05:32:50 +0000
+++ doc/source/xtrabackup_bin/creating_a_backup.rst	2012-01-26 03:26:24 +0000
@@ -18,7 +18,7 @@
 
   xtrabackup --backup --datadir=/var/lib/mysql/ --target-dir=/data/backups/mysql/
 
-This takes a backup of :file:`/var/lib/mysql` and stores it at :file:`/data/backups/mysql/`. The :option:`--target-dir` option deserves special explanation. Because the backup is performed from the data directory itself, **the target directory is relative to the data directory unless you specify an absolute path**. If you specify a relative path such as ``--target-dir=backups``, for example, don't look for the backup in the directory from which you executed |xtrabackup| - it will be a subdirectory of the :option:`--datadir` directory instead!
+This takes a backup of :file:`/var/lib/mysql` and stores it at :file:`/data/backups/mysql/`. If you specify a relative path, the target directory will be relative to the current directory.
 
 During the backup process, you should see a lot of output showing the data files being copied, as well as the log file thread repeatedly scanning the log files and copying from it. Here is an example that shows the log thread scanning the log in the background, and a file copying thread working on the ``ibdata1`` file: ::
 

=== modified file 'doc/source/xtrabackup_bin/xbk_option_reference.rst'
--- doc/source/xtrabackup_bin/xbk_option_reference.rst	2011-07-28 05:29:04 +0000
+++ doc/source/xtrabackup_bin/xbk_option_reference.rst	2012-01-26 03:26:24 +0000
@@ -121,7 +121,7 @@
 
    This option specifies the destination directory for the backup. If the directory does not exist, :program:`xtrabackup` creates it. If the directory does exist and is empty, :program:`xtrabackup` will succeed. :program:`xtrabackup` will not overwrite existing files, however; it will fail with operating system error 17, ``file exists``.
 
-   Note that for :option:`--backup`, if this option is a relative path, it is interpreted as being relative to the :option:`--datadir`, not relative to the current working directory from which :program:`xtrabackup` is executed. For :option:`--prepare`, relative paths are interpreted as being relative to the current working directory.
+   Note that for :option:`--backup`, if this option is a relative path, it is interpreted as being relative to the current working directory from which :program:`xtrabackup` is executed. For :option:`--prepare`, relative paths are interpreted as being relative to the current working directory.
 
 .. option:: --throttle
 

=== modified file 'innobackupex'
--- innobackupex	2012-01-23 21:39:00 +0000
+++ innobackupex	2012-01-26 03:26:24 +0000
@@ -436,9 +436,9 @@
             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";
@@ -807,8 +807,8 @@
     if (!$option_remote_host && !$option_stream) {
         $options = $options . " --target-dir=$backup_dir";
     } else {
-        #(datadir) for 'xtrabackup_suspended' and 'xtrabackup_checkpoints'
-        $options = $options . " --log-stream --target-dir=./";
+        #(tmpdir) for 'xtrabackup_suspended' and 'xtrabackup_checkpoints'
+        $options = $options . " --log-stream --target-dir=$option_tmpdir";
     }
 
     # prepare command line for running ibbackup
@@ -1589,7 +1589,7 @@
         $galera_info = $backup_dir . '/xtrabackup_galera_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';
         if ($option_stream) {
             $backup_config_file = $option_tmpdir . '/backup-my.cnf';

=== added file 'kewpie/percona_tests/xtrabackup_main/bug489290_test.py'
--- kewpie/percona_tests/xtrabackup_main/bug489290_test.py	1970-01-01 00:00:00 +0000
+++ kewpie/percona_tests/xtrabackup_main/bug489290_test.py	2012-01-26 03:26:24 +0000
@@ -0,0 +1,90 @@
+#! /usr/bin/env python
+# -*- mode: python; indent-tabs-mode: nil; -*-
+# vim:expandtab:shiftwidth=2:tabstop=2:smarttab:
+#
+# Copyright (C) 2012 Sergei Glushcenko
+#
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+
+import os
+import time
+import shutil
+import signal
+import subprocess
+
+from lib.util.mysqlBaseTestCase import mysqlBaseTestCase
+
+server_requirements = [[]]
+servers = []
+server_manager = None
+test_executor = None
+# we explicitly use the --no-timestamp option
+# here.  We will be using a generic / vanilla backup dir
+backup_path = None
+
+class basicTest(mysqlBaseTestCase):
+
+    def setUp(self):
+        master_server = servers[0] # assumption that this is 'master'
+        backup_path = os.path.join(master_server.vardir, '_xtrabackup')
+        # remove backup path
+        if os.path.exists(backup_path):
+            shutil.rmtree(backup_path)
+
+    def test_bug489290(self):
+        self.servers = servers
+        logging = test_executor.logging
+        if servers[0].type not in ['mysql','percona']:
+            return
+        else:
+            xtrabackup = test_executor.system_manager.xtrabackup_path
+            master_server = servers[0] # assumption that this is 'master'
+            backup_path = os.path.join(master_server.vardir, 'full_backup')
+            backup_path_rel = os.path.relpath(backup_path)
+            output_path = os.path.join(master_server.vardir, 'innobackupex.out')
+
+            self.assertFalse(os.path.exists(backup_path))
+
+            # populate our server with a test bed
+            test_cmd = "./gentest.pl --gendata=conf/percona/percona.zz"
+            retcode, output = self.execute_randgen(test_cmd, test_executor, master_server)
+        
+            # take a backup
+            cmd = [ xtrabackup
+                  , "--defaults-file=%s" %master_server.cnf_file
+                  , "--datadir=%s" %master_server.datadir
+                  , "--backup"
+                  , "--target-dir=%s" %backup_path_rel
+                  ]
+            cmd = " ".join(cmd)
+            retcode, output = self.execute_cmd(cmd, output_path, os.getcwd(), True)
+            self.assertTrue(retcode==0,output)
+
+            # assure that backup been put in the right place
+            self.assertTrue(os.path.exists(backup_path))
+
+            # prepare backup
+            cmd = [ xtrabackup
+                  , "--defaults-file=%s" %master_server.cnf_file
+                  , "--prepare"
+                  , "--target-dir=%s" %backup_path_rel
+                  ]
+            cmd = " ".join(cmd)
+            retcode, output = self.execute_cmd(cmd, output_path, os.getcwd(), True)
+            self.assertTrue(retcode==0,output)
+
+            # assure that backup been put in the right place
+            self.assertTrue(os.path.exists(backup_path))

=== added file 'test/t/bug489290.sh'
--- test/t/bug489290.sh	1970-01-01 00:00:00 +0000
+++ test/t/bug489290.sh	2012-01-26 03:26:24 +0000
@@ -0,0 +1,42 @@
+. inc/common.sh
+
+init
+run_mysqld
+load_dbase_schema sakila
+load_dbase_data sakila
+
+backup_dir=$topdir/backup
+mkdir -p $backup_dir
+vlog "Backup created in directory $backup_dir"
+
+cwd=`pwd`
+cd $topdir
+
+# Backup
+xtrabackup --datadir=$mysql_datadir --backup --target-dir=backup
+
+# Stat
+xtrabackup --datadir=$mysql_datadir --stat --target-dir=backup
+
+# Assure that directory is correct
+if [ ! -f $backup_dir/ibdata1 ] ; then
+	vlog "Backup not found in $backup_dir"
+	exit -1
+fi
+
+# Prepare
+xtrabackup --datadir=$mysql_datadir --prepare --target-dir=backup
+
+cd $cwd
+
+stop_mysqld
+
+# Restore
+rm -rf $mysql_datadir/ibdata1 $mysql_datadir/ib_logfile* \
+    $mysql_datadir/test/*.ibd
+cp -r $topdir/backup/* $mysql_datadir
+
+run_mysqld
+
+# Check sakila
+run_cmd ${MYSQL} ${MYSQL_ARGS} -e "SELECT count(*) from actor" sakila

=== modified file 'xtrabackup.c'
--- xtrabackup.c	2012-01-09 05:27:16 +0000
+++ xtrabackup.c	2012-01-26 03:26:24 +0000
@@ -3696,6 +3696,17 @@
 	OS_THREAD_DUMMY_RETURN;
 }
 
+/**************************************************************************
+Uses my_realpath to build absolute path for target dir.*/
+static void
+expand_target_dir_name()
+{
+	char buf[FN_REFLEN];
+
+	strnmov(buf, xtrabackup_target_dir, sizeof(buf) - 1);
+	xtrabackup_target_dir = my_load_path(xtrabackup_real_target_dir, buf, NULL);
+}
+
 static void
 xtrabackup_backup_func(void)
 {
@@ -6209,6 +6220,9 @@
 		exit(EXIT_FAILURE);
 	}
 
+	/* Ensure target dir is not relative to datadir */
+	expand_target_dir_name();
+
 	/* Redirect stdout to stderr for not doing a streaming backup or dumping
 	configuration for --print-param. */
 	if (!xtrabackup_stream && !xtrabackup_print_param && dup2(2, 1) < 0) {