* [Tarantool-patches] [PATCH] xlog: make log directory if needed
@ 2020-06-29 12:44 Ilya Kosarev
2020-06-29 12:48 ` Cyrill Gorcunov
2020-07-09 8:39 ` [Tarantool-patches] [PATCH] " Kirill Yukhin
0 siblings, 2 replies; 6+ messages in thread
From: Ilya Kosarev @ 2020-06-29 12:44 UTC (permalink / raw)
To: tarantool-patches
Tarantool's box.backup.start() returns the list of files needed to
successfully run new instance. The problem was that it doesn't include
empty indexes log directories. This means that after starting the
instance using backed up files and inserting something into previously
empty index we could run into log file creation error for example on
box.snapshot() call. Now this is not a problem as far as according
directories are created if needed.
Closes #5090
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability
Issue: https://github.com/tarantool/tarantool/issues/5027
@ChangeLog:
* Create missing folders for vinyl spaces and indexes if needed to
avoid confusing fails of tarantool started from backup.
src/box/xlog.c | 7 ++
src/lib/core/util.c | 23 ++++++
src/trivia/util.h | 3 +
test/box/gh-5090-empty-vinyl-backup.result | 82 ++++++++++++++++++++
test/box/gh-5090-empty-vinyl-backup.test.lua | 30 +++++++
test/box/lua/simple_instance.lua | 3 +
6 files changed, 148 insertions(+)
create mode 100644 test/box/gh-5090-empty-vinyl-backup.result
create mode 100644 test/box/gh-5090-empty-vinyl-backup.test.lua
create mode 100644 test/box/lua/simple_instance.lua
diff --git a/src/box/xlog.c b/src/box/xlog.c
index b5b082a204..698b99fa9d 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -824,6 +824,13 @@ xlog_create(struct xlog *xlog, const char *name, int flags,
xlog->is_inprogress = true;
snprintf(xlog->filename, sizeof(xlog->filename), "%s%s", name, inprogress_suffix);
+ /* Make directory if needed (gh-5090). */
+ if (make_dir(xlog->filename) != 0) {
+ diag_set(SystemError, "failed to create path '%s'",
+ xlog->filename);
+ goto err;
+ }
+
flags |= O_RDWR | O_CREAT | O_EXCL;
/*
diff --git a/src/lib/core/util.c b/src/lib/core/util.c
index d7f2344ed6..a6ed6e598f 100644
--- a/src/lib/core/util.c
+++ b/src/lib/core/util.c
@@ -216,6 +216,29 @@ abspath(const char *filename)
return abspath;
}
+/** Make missing directories from the path. */
+int
+make_dir(char *path)
+{
+ char *path_sep = path;
+ while (*path_sep == '/') {
+ /* Don't create root */
+ ++path_sep;
+ }
+ while ((path_sep = strchr(path_sep, '/'))) {
+ /* Recursively create path hierarchy */
+ *path_sep = '\0';
+ int rc = mkdir(path, 0777);
+ if (rc == -1 && errno != EEXIST) {
+ *path_sep = '/';
+ return -1;
+ }
+ *path_sep = '/';
+ ++path_sep;
+ }
+ return 0;
+}
+
char *
int2str(long long int val)
{
diff --git a/src/trivia/util.h b/src/trivia/util.h
index 29c7f01943..42408be1cc 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -410,6 +410,9 @@ find_path(const char *argv0);
char *
abspath(const char *filename);
+int
+make_dir(char *path);
+
char *
int2str(long long int val);
diff --git a/test/box/gh-5090-empty-vinyl-backup.result b/test/box/gh-5090-empty-vinyl-backup.result
new file mode 100644
index 0000000000..d746a8fe87
--- /dev/null
+++ b/test/box/gh-5090-empty-vinyl-backup.result
@@ -0,0 +1,82 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd('create server vinyl with script="box/lua/simple_instance.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server vinyl')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch vinyl')
+ | ---
+ | - true
+ | ...
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+index = s:create_index('primary', {parts={1, 'unsigned'}})
+ | ---
+ | ...
+
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+backup_files = box.backup.start()
+ | ---
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+
+backup_files = test_run:eval('vinyl', 'backup_files')[1]
+ | ---
+ | ...
+for _, file in pairs(backup_files) do os.execute('cp ' .. file .. ' .') end
+ | ---
+ | ...
+
+test_run:drop_cluster({'vinyl'})
+ | ---
+ | ...
+
+test_run:cmd("create server vinyl with script='box/lua/simple_instance.lua'")
+ | ---
+ | - true
+ | ...
+for _, file in pairs(backup_files) do os.execute('mv ' .. file:match('.*/(.*)') .. ' simple_instance/') end
+ | ---
+ | ...
+test_run:cmd('start server vinyl')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch vinyl')
+ | ---
+ | - true
+ | ...
+
+box.space.test:insert{1}
+ | ---
+ | - [1]
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+
+-- Cleanup.
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:drop_cluster({'vinyl'})
+ | ---
+ | ...
diff --git a/test/box/gh-5090-empty-vinyl-backup.test.lua b/test/box/gh-5090-empty-vinyl-backup.test.lua
new file mode 100644
index 0000000000..d461ad4fad
--- /dev/null
+++ b/test/box/gh-5090-empty-vinyl-backup.test.lua
@@ -0,0 +1,30 @@
+test_run = require('test_run').new()
+
+test_run:cmd('create server vinyl with script="box/lua/simple_instance.lua"')
+test_run:cmd('start server vinyl')
+test_run:cmd('switch vinyl')
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+index = s:create_index('primary', {parts={1, 'unsigned'}})
+
+box.snapshot()
+backup_files = box.backup.start()
+
+test_run:cmd('switch default')
+
+backup_files = test_run:eval('vinyl', 'backup_files')[1]
+for _, file in pairs(backup_files) do os.execute('cp ' .. file .. ' .') end
+
+test_run:drop_cluster({'vinyl'})
+
+test_run:cmd("create server vinyl with script='box/lua/simple_instance.lua'")
+for _, file in pairs(backup_files) do os.execute('mv ' .. file:match('.*/(.*)') .. ' simple_instance/') end
+test_run:cmd('start server vinyl')
+test_run:cmd('switch vinyl')
+
+box.space.test:insert{1}
+box.snapshot()
+
+-- Cleanup.
+test_run:cmd('switch default')
+test_run:drop_cluster({'vinyl'})
diff --git a/test/box/lua/simple_instance.lua b/test/box/lua/simple_instance.lua
new file mode 100644
index 0000000000..9ca79f6d07
--- /dev/null
+++ b/test/box/lua/simple_instance.lua
@@ -0,0 +1,3 @@
+box.cfg{}
+
+require('console').listen(os.getenv('ADMIN'))
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] xlog: make log directory if needed
2020-06-29 12:44 [Tarantool-patches] [PATCH] xlog: make log directory if needed Ilya Kosarev
@ 2020-06-29 12:48 ` Cyrill Gorcunov
2020-06-29 12:49 ` Cyrill Gorcunov
2020-07-09 8:39 ` [Tarantool-patches] [PATCH] " Kirill Yukhin
1 sibling, 1 reply; 6+ messages in thread
From: Cyrill Gorcunov @ 2020-06-29 12:48 UTC (permalink / raw)
To: Ilya Kosarev; +Cc: tarantool-patches
On Mon, Jun 29, 2020 at 03:44:19PM +0300, Ilya Kosarev wrote:
...
>
> +/** Make missing directories from the path. */
> +int
> +make_dir(char *path)
> +{
> + char *path_sep = path;
> + while (*path_sep == '/') {
> + /* Don't create root */
> + ++path_sep;
> + }
There is a helper function, please make sure that
1) path is having EOS
2) you don't go outside of path memory
IOW, I think you should pass some @len parameter
as well and return -1 in case if path is full of '/'
symbols without EOS.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] xlog: make log directory if needed
2020-06-29 12:48 ` Cyrill Gorcunov
@ 2020-06-29 12:49 ` Cyrill Gorcunov
2020-06-30 10:19 ` [Tarantool-patches] [PATCH v2] " Ilya Kosarev
0 siblings, 1 reply; 6+ messages in thread
From: Cyrill Gorcunov @ 2020-06-29 12:49 UTC (permalink / raw)
To: Ilya Kosarev, tarantool-patches
On Mon, Jun 29, 2020 at 03:48:09PM +0300, Cyrill Gorcunov wrote:
> On Mon, Jun 29, 2020 at 03:44:19PM +0300, Ilya Kosarev wrote:
> ...
> >
> > +/** Make missing directories from the path. */
> > +int
> > +make_dir(char *path)
> > +{
> > + char *path_sep = path;
> > + while (*path_sep == '/') {
> > + /* Don't create root */
> > + ++path_sep;
> > + }
>
> There is a helper function, please make sure that
Typo: this is a helper function.
> 1) path is having EOS
> 2) you don't go outside of path memory
>
> IOW, I think you should pass some @len parameter
> as well and return -1 in case if path is full of '/'
> symbols without EOS.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Tarantool-patches] [PATCH v2] xlog: make log directory if needed
2020-06-29 12:49 ` Cyrill Gorcunov
@ 2020-06-30 10:19 ` Ilya Kosarev
2020-06-30 11:17 ` Cyrill Gorcunov
0 siblings, 1 reply; 6+ messages in thread
From: Ilya Kosarev @ 2020-06-30 10:19 UTC (permalink / raw)
To: tarantool-patches
Tarantool's box.backup.start() returns the list of files needed to
successfully run new instance. The problem was that it doesn't include
empty indexes log directories. This means that after starting the
instance using backed up files and inserting something into previously
empty index we could run into log file creation error for example on
box.snapshot() call. Now this is not a problem as far as according
directories are created if needed. Corresponding test case added.
Closes #5090
---
Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5090-empty-vinyl-backup
Issue: https://github.com/tarantool/tarantool/issues/5090
@ChangeLog:
* Create missing folders for vinyl spaces and indexes if needed to
avoid confusing fails of tarantool started from backup.
Changes in v2:
- specified mkdirpath method contract
src/box/xlog.c | 7 ++
src/lib/core/util.c | 26 +++++++
src/trivia/util.h | 3 +
test/box/gh-5090-empty-vinyl-backup.result | 82 ++++++++++++++++++++
test/box/gh-5090-empty-vinyl-backup.test.lua | 30 +++++++
test/box/lua/simple_instance.lua | 3 +
6 files changed, 151 insertions(+)
create mode 100644 test/box/gh-5090-empty-vinyl-backup.result
create mode 100644 test/box/gh-5090-empty-vinyl-backup.test.lua
create mode 100644 test/box/lua/simple_instance.lua
diff --git a/src/box/xlog.c b/src/box/xlog.c
index b5b082a204..95be05f250 100644
--- a/src/box/xlog.c
+++ b/src/box/xlog.c
@@ -824,6 +824,13 @@ xlog_create(struct xlog *xlog, const char *name, int flags,
xlog->is_inprogress = true;
snprintf(xlog->filename, sizeof(xlog->filename), "%s%s", name, inprogress_suffix);
+ /* Make directory if needed (gh-5090). */
+ if (mkdirpath(xlog->filename) != 0) {
+ diag_set(SystemError, "failed to create path '%s'",
+ xlog->filename);
+ goto err;
+ }
+
flags |= O_RDWR | O_CREAT | O_EXCL;
/*
diff --git a/src/lib/core/util.c b/src/lib/core/util.c
index d7f2344ed6..dfce317f07 100644
--- a/src/lib/core/util.c
+++ b/src/lib/core/util.c
@@ -216,6 +216,32 @@ abspath(const char *filename)
return abspath;
}
+/**
+ * Make missing directories from @a path.
+ * @a path has to be null-terminated.
+ * @a path has to be mutable. It allows to avoid copying.
+ * However it is guaranteed to be unmodified after execution.
+ */
+int
+mkdirpath(char *path)
+{
+ char *path_sep = path;
+ while (*path_sep == '/') {
+ /* Don't create root */
+ ++path_sep;
+ }
+ while ((path_sep = strchr(path_sep, '/'))) {
+ /* Recursively create path hierarchy */
+ *path_sep = '\0';
+ int rc = mkdir(path, 0777);
+ *path_sep = '/';
+ if (rc == -1 && errno != EEXIST)
+ return -1;
+ ++path_sep;
+ }
+ return 0;
+}
+
char *
int2str(long long int val)
{
diff --git a/src/trivia/util.h b/src/trivia/util.h
index 29c7f01943..16667e27e9 100644
--- a/src/trivia/util.h
+++ b/src/trivia/util.h
@@ -410,6 +410,9 @@ find_path(const char *argv0);
char *
abspath(const char *filename);
+int
+mkdirpath(char *path);
+
char *
int2str(long long int val);
diff --git a/test/box/gh-5090-empty-vinyl-backup.result b/test/box/gh-5090-empty-vinyl-backup.result
new file mode 100644
index 0000000000..d746a8fe87
--- /dev/null
+++ b/test/box/gh-5090-empty-vinyl-backup.result
@@ -0,0 +1,82 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+
+test_run:cmd('create server vinyl with script="box/lua/simple_instance.lua"')
+ | ---
+ | - true
+ | ...
+test_run:cmd('start server vinyl')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch vinyl')
+ | ---
+ | - true
+ | ...
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+ | ---
+ | ...
+index = s:create_index('primary', {parts={1, 'unsigned'}})
+ | ---
+ | ...
+
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+backup_files = box.backup.start()
+ | ---
+ | ...
+
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+
+backup_files = test_run:eval('vinyl', 'backup_files')[1]
+ | ---
+ | ...
+for _, file in pairs(backup_files) do os.execute('cp ' .. file .. ' .') end
+ | ---
+ | ...
+
+test_run:drop_cluster({'vinyl'})
+ | ---
+ | ...
+
+test_run:cmd("create server vinyl with script='box/lua/simple_instance.lua'")
+ | ---
+ | - true
+ | ...
+for _, file in pairs(backup_files) do os.execute('mv ' .. file:match('.*/(.*)') .. ' simple_instance/') end
+ | ---
+ | ...
+test_run:cmd('start server vinyl')
+ | ---
+ | - true
+ | ...
+test_run:cmd('switch vinyl')
+ | ---
+ | - true
+ | ...
+
+box.space.test:insert{1}
+ | ---
+ | - [1]
+ | ...
+box.snapshot()
+ | ---
+ | - ok
+ | ...
+
+-- Cleanup.
+test_run:cmd('switch default')
+ | ---
+ | - true
+ | ...
+test_run:drop_cluster({'vinyl'})
+ | ---
+ | ...
diff --git a/test/box/gh-5090-empty-vinyl-backup.test.lua b/test/box/gh-5090-empty-vinyl-backup.test.lua
new file mode 100644
index 0000000000..d461ad4fad
--- /dev/null
+++ b/test/box/gh-5090-empty-vinyl-backup.test.lua
@@ -0,0 +1,30 @@
+test_run = require('test_run').new()
+
+test_run:cmd('create server vinyl with script="box/lua/simple_instance.lua"')
+test_run:cmd('start server vinyl')
+test_run:cmd('switch vinyl')
+
+s = box.schema.space.create('test', {engine = 'vinyl'})
+index = s:create_index('primary', {parts={1, 'unsigned'}})
+
+box.snapshot()
+backup_files = box.backup.start()
+
+test_run:cmd('switch default')
+
+backup_files = test_run:eval('vinyl', 'backup_files')[1]
+for _, file in pairs(backup_files) do os.execute('cp ' .. file .. ' .') end
+
+test_run:drop_cluster({'vinyl'})
+
+test_run:cmd("create server vinyl with script='box/lua/simple_instance.lua'")
+for _, file in pairs(backup_files) do os.execute('mv ' .. file:match('.*/(.*)') .. ' simple_instance/') end
+test_run:cmd('start server vinyl')
+test_run:cmd('switch vinyl')
+
+box.space.test:insert{1}
+box.snapshot()
+
+-- Cleanup.
+test_run:cmd('switch default')
+test_run:drop_cluster({'vinyl'})
diff --git a/test/box/lua/simple_instance.lua b/test/box/lua/simple_instance.lua
new file mode 100644
index 0000000000..9ca79f6d07
--- /dev/null
+++ b/test/box/lua/simple_instance.lua
@@ -0,0 +1,3 @@
+box.cfg{}
+
+require('console').listen(os.getenv('ADMIN'))
--
2.17.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH v2] xlog: make log directory if needed
2020-06-30 10:19 ` [Tarantool-patches] [PATCH v2] " Ilya Kosarev
@ 2020-06-30 11:17 ` Cyrill Gorcunov
0 siblings, 0 replies; 6+ messages in thread
From: Cyrill Gorcunov @ 2020-06-30 11:17 UTC (permalink / raw)
To: Ilya Kosarev; +Cc: tarantool-patches
On Tue, Jun 30, 2020 at 01:19:44PM +0300, Ilya Kosarev wrote:
> Tarantool's box.backup.start() returns the list of files needed to
> successfully run new instance. The problem was that it doesn't include
> empty indexes log directories. This means that after starting the
> instance using backed up files and inserting something into previously
> empty index we could run into log file creation error for example on
> box.snapshot() call. Now this is not a problem as far as according
> directories are created if needed. Corresponding test case added.
>
> Closes #5090
Reviewed-by: Cyrill Gorcunov <gorcunov@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Tarantool-patches] [PATCH] xlog: make log directory if needed
2020-06-29 12:44 [Tarantool-patches] [PATCH] xlog: make log directory if needed Ilya Kosarev
2020-06-29 12:48 ` Cyrill Gorcunov
@ 2020-07-09 8:39 ` Kirill Yukhin
1 sibling, 0 replies; 6+ messages in thread
From: Kirill Yukhin @ 2020-07-09 8:39 UTC (permalink / raw)
To: Ilya Kosarev; +Cc: tarantool-patches
Hello,
On 29 июн 15:44, Ilya Kosarev wrote:
> Tarantool's box.backup.start() returns the list of files needed to
> successfully run new instance. The problem was that it doesn't include
> empty indexes log directories. This means that after starting the
> instance using backed up files and inserting something into previously
> empty index we could run into log file creation error for example on
> box.snapshot() call. Now this is not a problem as far as according
> directories are created if needed.
>
> Closes #5090
> ---
> Branch: https://github.com/tarantool/tarantool/tree/i.kosarev/gh-5027-fix-tuple-fields-nullability
Wrong.
> Issue: https://github.com/tarantool/tarantool/issues/5027
Wrong.
>
> @ChangeLog:
> * Create missing folders for vinyl spaces and indexes if needed to
> avoid confusing fails of tarantool started from backup.
Need to mention an issue in brackets: (gh-5090).
Otherwise LGTM.
Fixed and checked into 1.10, 2.3, 2.4 and master.
--
Regards, Kirill Yukhin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-07-09 8:39 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29 12:44 [Tarantool-patches] [PATCH] xlog: make log directory if needed Ilya Kosarev
2020-06-29 12:48 ` Cyrill Gorcunov
2020-06-29 12:49 ` Cyrill Gorcunov
2020-06-30 10:19 ` [Tarantool-patches] [PATCH v2] " Ilya Kosarev
2020-06-30 11:17 ` Cyrill Gorcunov
2020-07-09 8:39 ` [Tarantool-patches] [PATCH] " Kirill Yukhin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox