[Tarantool-patches] [test-run] Add options for upgrade testing

[Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

First option '--snapshot-version' specifies snapshot version which loaded in
tarantool before testing. Prepared snapshots lies in a directory
test/static/snapshots.

Second option '--disable-chema-upgrade' allows to skip execution
of `upgrade.lua` script.

Ticket: #4801
---
 lib/options.py          | 14 +++++++++
 lib/tarantool_server.py | 66 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/lib/options.py b/lib/options.py
index 8bacb4a..d627eb2 100644
--- a/lib/options.py
+++ b/lib/options.py
@@ -201,6 +201,20 @@ class Options:
                 help="""Run the server under 'luacov'.
                 Default: false.""")
 
+        parser.add_argument(
+                "--snapshot-version",
+                dest='snapshot_version',
+                default=None,
+                help="""Version of Tarantool snapshot loaded before
+                testing.""")
+
+        parser.add_argument(
+                "--disable-schema-upgrade",
+                dest='disable_schema_upgrade',
+                action="store_true",
+                default=False,
+                help="""Disable schema upgrade on testing with snapshots.""")
+
         # XXX: We can use parser.parse_intermixed_args() on
         # Python 3.7 to understand commands like
         # ./test-run.py foo --exclude bar baz
diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index fd102b9..035ba5f 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -2,6 +2,7 @@ import errno
 import gevent
 import glob
 import inspect  # for caller_globals
+import json
 import os
 import os.path
 import re
@@ -32,6 +33,7 @@ from lib.utils import format_process
 from lib.utils import signame
 from lib.utils import warn_unix_socket
 from lib.utils import prefix_each_line
+from lib.options import Options
 from test import TestRunGreenlet, TestExecutionError
 
 
@@ -448,6 +450,9 @@ class TarantoolLog(object):
 
 
 class TarantoolServer(Server):
+
+    SNAPSHOT_DIRECTORY = 'static/snapshots'
+
     default_tarantool = {
         "bin": "tarantool",
         "logfile": "tarantool.log",
@@ -641,6 +646,22 @@ class TarantoolServer(Server):
         if 'test_run_current_test' in caller_globals.keys():
             self.current_test = caller_globals['test_run_current_test']
 
+        self.snapshot_basename = Options().args.snapshot_version
+        if self.snapshot_basename:
+            self.snapshot_name = '{}.snap'.format(self.snapshot_basename)
+            self.snapshot_path = os.path.join(self.testdir, self.SNAPSHOT_DIRECTORY, self.snapshot_name)
+            if not os.path.exists(self.snapshot_path):
+                color_stdout("Snapshot {} have been not found\n".format(self.snapshot_path),
+                            schema='error')
+                raise TarantoolStartError
+                server.kill_current_test()
+        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
+        if self.disable_schema_upgrade and not self.snapshot_basename:
+            color_stdout("option --disable-schema-upgrade depends on --snapshot-version\n",
+                        schema='error')
+            raise TarantoolStartError
+            server.kill_current_test()
+
     def __del__(self):
         self.stop()
 
@@ -727,6 +748,15 @@ class TarantoolServer(Server):
             self.cleanup()
         self.copy_files()
 
+        if self.snapshot_basename:
+            (instance_name, _) = os.path.splitext(os.path.basename(self.script))
+            instance_dir = os.path.join(self.vardir, instance_name)
+            if not os.path.exists(instance_dir):
+                os.mkdir(instance_dir)
+            snapshot_dest = os.path.join(instance_dir, '00000000000000000000.snap')
+            color_log("Copying snapshot {} to {}".format(self.snapshot_name, snapshot_dest))
+            shutil.copy(self.snapshot_path, snapshot_dest)
+
         if self.use_unix_sockets:
             path = os.path.join(self.vardir, self.name + ".socket-admin")
             warn_unix_socket(path)
@@ -767,6 +797,7 @@ class TarantoolServer(Server):
                     if (e.errno == errno.ENOENT):
                         continue
                     raise
+
         shutil.copy('.tarantoolctl', self.vardir)
         shutil.copy(os.path.join(self.TEST_RUN_DIR, 'test_run.lua'),
                     self.vardir)
@@ -776,7 +807,12 @@ class TarantoolServer(Server):
                         self.vardir)
 
     def prepare_args(self, args=[]):
-        return [self.ctl_path, 'start', os.path.basename(self.script)] + args
+        ctl_args = [self.ctl_path, 'start', os.path.basename(self.script)] + args
+        if self.disable_schema_upgrade:
+            auto_upgrade = 'box.error.injection.set("ERRINJ_AUTO_UPGRADE", true)'
+            ctl_args = [self.binary, '-e', auto_upgrade] + ctl_args
+
+        return ctl_args
 
     def pretest_clean(self):
         # Don't delete snap and logs for 'default' tarantool server
@@ -819,6 +855,7 @@ class TarantoolServer(Server):
 
         # redirect stdout from tarantoolctl and tarantool
         os.putenv("TEST_WORKDIR", self.vardir)
+
         self.process = subprocess.Popen(args,
                                         cwd=self.vardir,
                                         stdout=self.log_des,
@@ -861,6 +898,33 @@ class TarantoolServer(Server):
         self.admin = CON_SWITCH[self.tests_type]('localhost', port)
         self.status = 'started'
 
+        # make sure schema is old
+        if self.disable_schema_upgrade:
+            version = self.extract_schema_from_snapshot()
+            conn = AdminConnection('localhost', self.admin.port)
+            current_ver = yaml.safe_load(conn.execute('box.space._schema:get{"version"}'))
+            conn.disconnect()
+            assert(version, current_ver)
+
+    def extract_schema_from_snapshot(self):
+        ctl_args = [self.ctl_path, 'cat', self.snapshot_path,
+                        '--format=json', '--show-system']
+        # Extract schema version, example of record:
+        # {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
+        #   "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
+        with open(os.devnull, 'w') as fp:
+            proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE, stderr=fp)
+            content = proc.stdout.read()
+            proc.wait()
+            for line in content.split('\n'):
+                if line:
+                    json_line = json.loads(line)
+                    t = json_line['BODY']['tuple']
+                    if t[0] == 'version':
+                        return t
+
+        return None
+
     def crash_detect(self):
         if self.crash_expected:
             return
-- 
2.23.0


-- 
sergeyb@

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Alexander Turenko]

The most important question: as I see from the code both options do not
work with 'core = app' tests. I think we should support those option in
AppServer too.

Other comments are below. They are mostly like 'we can polish it a bit
here and there'.

WBR, Alexander Turenko.

On Mon, Mar 30, 2020 at 05:48:23PM +0300, Sergey Bronnikov wrote:
> First option '--snapshot-version' specifies snapshot version which loaded in

Nit: Over 72 symbols.

> tarantool before testing. Prepared snapshots lies in a directory
> test/static/snapshots.
> 
> Second option '--disable-chema-upgrade' allows to skip execution

Typo: chema -> schema.

> of `upgrade.lua` script.

Nit: It is not obvious that it is about src/box/lua script, because we
have several upgrade.lua in tests:

$ find . -name upgrade.lua
./src/box/lua/upgrade.lua
./test/sql/upgrade/upgrade.lua
./test/vinyl/upgrade.lua
./test/xlog/upgrade.lua

> 
> Ticket: #4801

Nit: tarantool/tarantool issue will not be linked this way rfom
tarantool/test-run repository commit. I usually use full link in such
cases.

> ---

Nit: Please, place branch name here.

>  lib/options.py          | 14 +++++++++
>  lib/tarantool_server.py | 66 ++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 79 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/options.py b/lib/options.py
> index 8bacb4a..d627eb2 100644
> --- a/lib/options.py
> +++ b/lib/options.py
> @@ -201,6 +201,20 @@ class Options:
>                  help="""Run the server under 'luacov'.
>                  Default: false.""")
>  
> +        parser.add_argument(
> +                "--snapshot-version",
> +                dest='snapshot_version',
> +                default=None,
> +                help="""Version of Tarantool snapshot loaded before
> +                testing.""")

I would accept a path to a snapshot file here. This way does not require
knowledge from a user where test-run expect snapshots to be placed (and
that '.snap' should not be added). It also more flexible: a user may
choose where it is better to place snapshots within its project (or even
outside it).

> +
> +        parser.add_argument(
> +                "--disable-schema-upgrade",
> +                dest='disable_schema_upgrade',
> +                action="store_true",
> +                default=False,
> +                help="""Disable schema upgrade on testing with snapshots.""")
> +
>          # XXX: We can use parser.parse_intermixed_args() on
>          # Python 3.7 to understand commands like
>          # ./test-run.py foo --exclude bar baz
> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
> index fd102b9..035ba5f 100644
> --- a/lib/tarantool_server.py
> +++ b/lib/tarantool_server.py
> @@ -2,6 +2,7 @@ import errno
>  import gevent
>  import glob
>  import inspect  # for caller_globals
> +import json
>  import os
>  import os.path
>  import re
> @@ -32,6 +33,7 @@ from lib.utils import format_process
>  from lib.utils import signame
>  from lib.utils import warn_unix_socket
>  from lib.utils import prefix_each_line
> +from lib.options import Options
>  from test import TestRunGreenlet, TestExecutionError
>  
>  
> @@ -448,6 +450,9 @@ class TarantoolLog(object):
>  
>  
>  class TarantoolServer(Server):
> +
> +    SNAPSHOT_DIRECTORY = 'static/snapshots'
> +
>      default_tarantool = {
>          "bin": "tarantool",
>          "logfile": "tarantool.log",
> @@ -641,6 +646,22 @@ class TarantoolServer(Server):
>          if 'test_run_current_test' in caller_globals.keys():
>              self.current_test = caller_globals['test_run_current_test']
>  
> +        self.snapshot_basename = Options().args.snapshot_version
> +        if self.snapshot_basename:
> +            self.snapshot_name = '{}.snap'.format(self.snapshot_basename)
> +            self.snapshot_path = os.path.join(self.testdir, self.SNAPSHOT_DIRECTORY, self.snapshot_name)
> +            if not os.path.exists(self.snapshot_path):
> +                color_stdout("Snapshot {} have been not found\n".format(self.snapshot_path),
> +                            schema='error')

Can we move this check to lib/options.py? There is check() function
there.

> +                raise TarantoolStartError
> +                server.kill_current_test()
> +        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
> +        if self.disable_schema_upgrade and not self.snapshot_basename:
> +            color_stdout("option --disable-schema-upgrade depends on --snapshot-version\n",
> +                        schema='error')
> +            raise TarantoolStartError
> +            server.kill_current_test
> +

And this check too?

I would also verify whether tarantool is built in debug: otherwise error
injections are not available. When the build is not Debug we obviously
unable to disable upgrade and it worth to give a user exact reason.

cls.version() captures `tarantool --version` output.

>      def __del__(self):
>          self.stop()
>  
> @@ -727,6 +748,15 @@ class TarantoolServer(Server):
>              self.cleanup()
>          self.copy_files()
>  
> +        if self.snapshot_basename:
> +            (instance_name, _) = os.path.splitext(os.path.basename(self.script))
> +            instance_dir = os.path.join(self.vardir, instance_name)
> +            if not os.path.exists(instance_dir):
> +                os.mkdir(instance_dir)
> +            snapshot_dest = os.path.join(instance_dir, '00000000000000000000.snap')

We can keep a snapshot name, it may be useful for debugging.

> +            color_log("Copying snapshot {} to {}".format(self.snapshot_name, snapshot_dest))
> +            shutil.copy(self.snapshot_path, snapshot_dest)
> +
>          if self.use_unix_sockets:
>              path = os.path.join(self.vardir, self.name + ".socket-admin")
>              warn_unix_socket(path)
> @@ -767,6 +797,7 @@ class TarantoolServer(Server):
>                      if (e.errno == errno.ENOENT):
>                          continue
>                      raise
> +
>          shutil.copy('.tarantoolctl', self.vardir)
>          shutil.copy(os.path.join(self.TEST_RUN_DIR, 'test_run.lua'),
>                      self.vardir)
> @@ -776,7 +807,12 @@ class TarantoolServer(Server):
>                          self.vardir)
>  
>      def prepare_args(self, args=[]):
> -        return [self.ctl_path, 'start', os.path.basename(self.script)] + args
> +        ctl_args = [self.ctl_path, 'start', os.path.basename(self.script)] + args
> +        if self.disable_schema_upgrade:
> +            auto_upgrade = 'box.error.injection.set("ERRINJ_AUTO_UPGRADE", true)'
> +            ctl_args = [self.binary, '-e', auto_upgrade] + ctl_args

It does not fail when the errinj is not available (say, on old tarantool
version). I experimented around and found that this variant will fails
in the case:

 | -e "require('fiber').sleep(0) assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', 'no such errinj')"

Filed https://github.com/tarantool/tarantool/issues/4983

Please, mention the issue in a comment here if you'll use this
fiber.sleep(0) hack.

> +
> +        return ctl_args
>  
>      def pretest_clean(self):
>          # Don't delete snap and logs for 'default' tarantool server
> @@ -819,6 +855,7 @@ class TarantoolServer(Server):
>  
>          # redirect stdout from tarantoolctl and tarantool
>          os.putenv("TEST_WORKDIR", self.vardir)
> +
>          self.process = subprocess.Popen(args,
>                                          cwd=self.vardir,
>                                          stdout=self.log_des,
> @@ -861,6 +898,33 @@ class TarantoolServer(Server):
>          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
>          self.status = 'started'
>  
> +        # make sure schema is old
> +        if self.disable_schema_upgrade:
> +            version = self.extract_schema_from_snapshot()
> +            conn = AdminConnection('localhost', self.admin.port)
> +            current_ver = yaml.safe_load(conn.execute('box.space._schema:get{"version"}'))
> +            conn.disconnect()

self.admin is a connection, it can be used here.

> +            assert(version, current_ver)

Nit: It does not check whether version are equivalent.

Nit: assert is statement, not a function in Python (it is better to use
it w/o parentheses).

Nit: I would explicitly raise RuntimeError(<...message...>), because I
would not want depend on optimization level here. This check is not only
for test-run code, but also depends on tarantool behaviour.

BTW, CI (`make lint`) show errors:
https://travis-ci.com/github/tarantool/test-run/builds/156536350

> +
> +    def extract_schema_from_snapshot(self):

Nit: We can make it a class method.

Nit: We can call find_exe() if cls.ctl_path is not defined.

> +        ctl_args = [self.ctl_path, 'cat', self.snapshot_path,
> +                        '--format=json', '--show-system']
> +        # Extract schema version, example of record:
> +        # {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
> +        #   "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
> +        with open(os.devnull, 'w') as fp:
> +            proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE, stderr=fp)

In theory snapshots may be quite large. Can we read it line-by-line? I
guess `for line in iter(proc.stdout.readline, '')` should work, see:

https://stackoverflow.com/a/2813530/1598057 (comments)
https://web.archive.org/web/20181102115150/bugs.python.org/issue3907

> +            content = proc.stdout.read()
> +            proc.wait()
> +            for line in content.split('\n'):
> +                if line:
> +                    json_line = json.loads(line)
> +                    t = json_line['BODY']['tuple']
> +                    if t[0] == 'version':
> +                        return t

It looks fragile. Can we at least check space id? Those IDs are fixed
for system spaces.

It also worth to consider only INSERT/REPLACE operations.

> +
> +        return None
> +
>      def crash_detect(self):
>          if self.crash_expected:
>              return
> -- 
> 2.23.0
> 
> 
> -- 
> sergeyb@

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

Alexander,

thanks for review. See my comments inline.
All changes on top of branch.

On 19:17 Fri 15 May , Alexander Turenko wrote:
> The most important question: as I see from the code both options do not
> work with 'core = app' tests. I think we should support those option in
> AppServer too.

Fixed with small refactoring of AppServer, TarantoolServer and Server
classes.

> Other comments are below. They are mostly like 'we can polish it a bit
> here and there'.
>
> WBR, Alexander Turenko.
> 
> On Mon, Mar 30, 2020 at 05:48:23PM +0300, Sergey Bronnikov wrote:
> > First option '--snapshot-version' specifies snapshot version which loaded in
> 
> Nit: Over 72 symbols.

Fixed.

> > tarantool before testing. Prepared snapshots lies in a directory
> > test/static/snapshots.
> > 
> > Second option '--disable-chema-upgrade' allows to skip execution
> 
> Typo: chema -> schema.

Fixed.

> > of `upgrade.lua` script.
> 
> Nit: It is not obvious that it is about src/box/lua script, because we
> have several upgrade.lua in tests:
> 
> $ find . -name upgrade.lua
> ./src/box/lua/upgrade.lua
> ./test/sql/upgrade/upgrade.lua
> ./test/vinyl/upgrade.lua
> ./test/xlog/upgrade.lua

Fixed too.

> > 
> > Ticket: #4801
> 
> Nit: tarantool/tarantool issue will not be linked this way rfom
> tarantool/test-run repository commit. I usually use full link in such
> cases.

Done.

> > ---
> 
> Nit: Please, place branch name here.

Will do next time.

> >  lib/options.py          | 14 +++++++++
> >  lib/tarantool_server.py | 66 ++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 79 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/options.py b/lib/options.py
> > index 8bacb4a..d627eb2 100644
> > --- a/lib/options.py
> > +++ b/lib/options.py
> > @@ -201,6 +201,20 @@ class Options:
> >                  help="""Run the server under 'luacov'.
> >                  Default: false.""")
> >  
> > +        parser.add_argument(
> > +                "--snapshot-version",
> > +                dest='snapshot_version',
> > +                default=None,
> > +                help="""Version of Tarantool snapshot loaded before
> > +                testing.""")
> 
> I would accept a path to a snapshot file here. This way does not require
> knowledge from a user where test-run expect snapshots to be placed (and
> that '.snap' should not be added). It also more flexible: a user may
> choose where it is better to place snapshots within its project (or even
> outside it).

Don't forget we need a way to run upgrade testing for several tarantool
versions automatically. With your suggestion we should specify one
option several times and I don't like this approach, because it will
overload command line for running tests.

> > +
> > +        parser.add_argument(
> > +                "--disable-schema-upgrade",
> > +                dest='disable_schema_upgrade',
> > +                action="store_true",
> > +                default=False,
> > +                help="""Disable schema upgrade on testing with snapshots.""")
> > +
> >          # XXX: We can use parser.parse_intermixed_args() on
> >          # Python 3.7 to understand commands like
> >          # ./test-run.py foo --exclude bar baz
> > diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
> > index fd102b9..035ba5f 100644
> > --- a/lib/tarantool_server.py
> > +++ b/lib/tarantool_server.py
> > @@ -2,6 +2,7 @@ import errno
> >  import gevent
> >  import glob
> >  import inspect  # for caller_globals
> > +import json
> >  import os
> >  import os.path
> >  import re
> > @@ -32,6 +33,7 @@ from lib.utils import format_process
> >  from lib.utils import signame
> >  from lib.utils import warn_unix_socket
> >  from lib.utils import prefix_each_line
> > +from lib.options import Options
> >  from test import TestRunGreenlet, TestExecutionError
> >  
> >  
> > @@ -448,6 +450,9 @@ class TarantoolLog(object):
> >  
> >  
> >  class TarantoolServer(Server):
> > +
> > +    SNAPSHOT_DIRECTORY = 'static/snapshots'
> > +
> >      default_tarantool = {
> >          "bin": "tarantool",
> >          "logfile": "tarantool.log",
> > @@ -641,6 +646,22 @@ class TarantoolServer(Server):
> >          if 'test_run_current_test' in caller_globals.keys():
> >              self.current_test = caller_globals['test_run_current_test']
> >  
> > +        self.snapshot_basename = Options().args.snapshot_version
> > +        if self.snapshot_basename:
> > +            self.snapshot_name = '{}.snap'.format(self.snapshot_basename)
> > +            self.snapshot_path = os.path.join(self.testdir, self.SNAPSHOT_DIRECTORY, self.snapshot_name)
> > +            if not os.path.exists(self.snapshot_path):
> > +                color_stdout("Snapshot {} have been not found\n".format(self.snapshot_path),
> > +                            schema='error')
> 
> Can we move this check to lib/options.py? There is check() function
> there.

These conditions are not about constraints checking but also set
internal variables. So I cannot move them to lib/options.py.

> > +                raise TarantoolStartError
> > +                server.kill_current_test()
> > +        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
> > +        if self.disable_schema_upgrade and not self.snapshot_basename:
> > +            color_stdout("option --disable-schema-upgrade depends on --snapshot-version\n",
> > +                        schema='error')
> > +            raise TarantoolStartError
> > +            server.kill_current_test
> > +
> 
> And this check too?

Moved to lib/options.py.

> I would also verify whether tarantool is built in debug: otherwise error
> injections are not available. When the build is not Debug we obviously
> unable to disable upgrade and it worth to give a user exact reason.

Good point! Added.

> cls.version() captures `tarantool --version` output.
> 
> >      def __del__(self):
> >          self.stop()
> >  
> > @@ -727,6 +748,15 @@ class TarantoolServer(Server):
> >              self.cleanup()
> >          self.copy_files()
> >  
> > +        if self.snapshot_basename:
> > +            (instance_name, _) = os.path.splitext(os.path.basename(self.script))
> > +            instance_dir = os.path.join(self.vardir, instance_name)
> > +            if not os.path.exists(instance_dir):
> > +                os.mkdir(instance_dir)
> > +            snapshot_dest = os.path.join(instance_dir, '00000000000000000000.snap')
> 
> We can keep a snapshot name, it may be useful for debugging.

AFAIU you mean 'keep original name', done.

> > +            color_log("Copying snapshot {} to {}".format(self.snapshot_name, snapshot_dest))
> > +            shutil.copy(self.snapshot_path, snapshot_dest)
> > +
> >          if self.use_unix_sockets:
> >              path = os.path.join(self.vardir, self.name + ".socket-admin")
> >              warn_unix_socket(path)
> > @@ -767,6 +797,7 @@ class TarantoolServer(Server):
> >                      if (e.errno == errno.ENOENT):
> >                          continue
> >                      raise
> > +
> >          shutil.copy('.tarantoolctl', self.vardir)
> >          shutil.copy(os.path.join(self.TEST_RUN_DIR, 'test_run.lua'),
> >                      self.vardir)
> > @@ -776,7 +807,12 @@ class TarantoolServer(Server):
> >                          self.vardir)
> >  
> >      def prepare_args(self, args=[]):
> > -        return [self.ctl_path, 'start', os.path.basename(self.script)] + args
> > +        ctl_args = [self.ctl_path, 'start', os.path.basename(self.script)] + args
> > +        if self.disable_schema_upgrade:
> > +            auto_upgrade = 'box.error.injection.set("ERRINJ_AUTO_UPGRADE", true)'
> > +            ctl_args = [self.binary, '-e', auto_upgrade] + ctl_args
> 
> It does not fail when the errinj is not available (say, on old tarantool
> version). I experimented around and found that this variant will fails
> in the case:
> 
>  | -e "require('fiber').sleep(0) assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', 'no such errinj')"

Updated.

> Filed https://github.com/tarantool/tarantool/issues/4983
> 
> Please, mention the issue in a comment here if you'll use this
> fiber.sleep(0) hack.

Mentioned.

> > +
> > +        return ctl_args
> >  
> >      def pretest_clean(self):
> >          # Don't delete snap and logs for 'default' tarantool server
> > @@ -819,6 +855,7 @@ class TarantoolServer(Server):
> >  
> >          # redirect stdout from tarantoolctl and tarantool
> >          os.putenv("TEST_WORKDIR", self.vardir)
> > +
> >          self.process = subprocess.Popen(args,
> >                                          cwd=self.vardir,
> >                                          stdout=self.log_des,
> > @@ -861,6 +898,33 @@ class TarantoolServer(Server):
> >          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
> >          self.status = 'started'
> >  
> > +        # make sure schema is old
> > +        if self.disable_schema_upgrade:
> > +            version = self.extract_schema_from_snapshot()
> > +            conn = AdminConnection('localhost', self.admin.port)
> > +            current_ver = yaml.safe_load(conn.execute('box.space._schema:get{"version"}'))
> > +            conn.disconnect()
> 
> self.admin is a connection, it can be used here.

Fixed.

> > +            assert(version, current_ver)
> 
> Nit: It does not check whether version are equivalent.
> 
> Nit: assert is statement, not a function in Python (it is better to use
> it w/o parentheses).
> 
> Nit: I would explicitly raise RuntimeError(<...message...>), because I
> would not want depend on optimization level here. This check is not only
> for test-run code, but also depends on tarantool behaviour.

Well, replaced assert with raise.

> BTW, CI (`make lint`) show errors:
> https://travis-ci.com/github/tarantool/test-run/builds/156536350

Most warnings fixed.

> > +
> > +    def extract_schema_from_snapshot(self):
> 
> Nit: We can make it a class method.

What do you mean? It's already a class method.

> Nit: We can call find_exe() if cls.ctl_path is not defined.

Fixed.

> > +        ctl_args = [self.ctl_path, 'cat', self.snapshot_path,
> > +                        '--format=json', '--show-system']
> > +        # Extract schema version, example of record:
> > +        # {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
> > +        #   "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
> > +        with open(os.devnull, 'w') as fp:
> > +            proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE, stderr=fp)
> 
> In theory snapshots may be quite large. Can we read it line-by-line? I
> guess `for line in iter(proc.stdout.readline, '')` should work, see:
> 
> https://stackoverflow.com/a/2813530/1598057 (comments)
> https://web.archive.org/web/20181102115150/bugs.python.org/issue3907

Fixed.

> > +            content = proc.stdout.read()
> > +            proc.wait()
> > +            for line in content.split('\n'):
> > +                if line:
> > +                    json_line = json.loads(line)
> > +                    t = json_line['BODY']['tuple']
> > +                    if t[0] == 'version':
> > +                        return t
> 
> It looks fragile. Can we at least check space id? Those IDs are fixed
> for system spaces.

Discussed it and decided to check that box.space._schema.id == 272 in
tuples. Added such check.

> It also worth to consider only INSERT/REPLACE operations.

Added filter for operations.

> > +
> > +        return None
> > +
> >      def crash_detect(self):
> >          if self.crash_expected:
> >              return
> > -- 
> > 2.23.0
> > 
> > 
> > -- 
> > sergeyb@

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Alexander Turenko]

(Just looked at answers, don't see the new version of the patch for
now.)

On Thu, May 28, 2020 at 12:39:34PM +0300, Sergey Bronnikov wrote:
> Alexander,
> 
> thanks for review. See my comments inline.
> All changes on top of branch.

It is ligurio/gh-4801-add-snapshot-option, right? I see some 'tmp',
'temp' commits. Please, squash fixups if they are not intended to land
as separate commits into master.

> > > diff --git a/lib/options.py b/lib/options.py
> > > index 8bacb4a..d627eb2 100644
> > > --- a/lib/options.py
> > > +++ b/lib/options.py
> > > @@ -201,6 +201,20 @@ class Options:
> > >                  help="""Run the server under 'luacov'.
> > >                  Default: false.""")
> > >  
> > > +        parser.add_argument(
> > > +                "--snapshot-version",
> > > +                dest='snapshot_version',
> > > +                default=None,
> > > +                help="""Version of Tarantool snapshot loaded before
> > > +                testing.""")
> > 
> > I would accept a path to a snapshot file here. This way does not require
> > knowledge from a user where test-run expect snapshots to be placed (and
> > that '.snap' should not be added). It also more flexible: a user may
> > choose where it is better to place snapshots within its project (or even
> > outside it).
> 
> Don't forget we need a way to run upgrade testing for several tarantool
> versions automatically. With your suggestion we should specify one
> option several times and I don't like this approach, because it will
> overload command line for running tests.

For several snapshots? Each is a separate ./test-run.py run I guess. The
option (as it is shown in the snippet above) does not allow to be
specified several times (it would use `nargs='*'` if it would do).
However I don't sure that specifying this option several times would be
intuitive: this way ./test-run.py should run the whole testing several
times with different snapshots. Usually test-run.py run testing only
once.

Just example, I want to verify starting from a snapshot using existing
tests on my own commit:

 | $ (cd test && for s in snapshots/*.snap; do ./test-run.py --snapshot $s; done)

It does not work. Ouch. Okay...

 | $ (cd test && for s in snapshots/*.snap; do ./test-run.py s=${s#snapshots/} --snapshot-verson ${s%.snap}; done)

Doable. Now I got some snapshot from a client and want to answer whether
it is safe to update to a new tarantool version.

 | $ (cd test && ./test-run.py --snapshot ../0000<...>1234.snap)

Ugh, again, I need some additional actions:

 | $ mv 0000<...>1234.snap test/snapshots
 | $ (cd test && ./test-run.py --snapshot-version 0000<...>1234)

Doable too. But do you find it convenient?

> > BTW, CI (`make lint`) show errors:
> > https://travis-ci.com/github/tarantool/test-run/builds/156536350
> 
> Most warnings fixed.

Aren't it is better to keep CI green and fix them all? Or lift the limit
up and keep CI green? I vote for the former, but okay, any way is
acceptable.

> > > +
> > > +    def extract_schema_from_snapshot(self):
> > 
> > Nit: We can make it a class method.
> 
> What do you mean? It's already a class method.

@classmethod

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

Hi,

On 14:13 Tue 02 Jun , Alexander Turenko wrote:
> (Just looked at answers, don't see the new version of the patch for
> now.)
> 
> On Thu, May 28, 2020 at 12:39:34PM +0300, Sergey Bronnikov wrote:
> > Alexander,
> > 
> > thanks for review. See my comments inline.
> > All changes on top of branch.
> 
> It is ligurio/gh-4801-add-snapshot-option, right? I see some 'tmp',
> 'temp' commits. Please, squash fixups if they are not intended to land
> as separate commits into master.

Sorry, forgot to put branch in order. Now there is a single commit on
top of branch and it is ready for review.

> > > > diff --git a/lib/options.py b/lib/options.py
> > > > index 8bacb4a..d627eb2 100644
> > > > --- a/lib/options.py
> > > > +++ b/lib/options.py
> > > > @@ -201,6 +201,20 @@ class Options:
> > > >                  help="""Run the server under 'luacov'.
> > > >                  Default: false.""")
> > > >  
> > > > +        parser.add_argument(
> > > > +                "--snapshot-version",
> > > > +                dest='snapshot_version',
> > > > +                default=None,
> > > > +                help="""Version of Tarantool snapshot loaded before
> > > > +                testing.""")
> > > 
> > > I would accept a path to a snapshot file here. This way does not require
> > > knowledge from a user where test-run expect snapshots to be placed (and
> > > that '.snap' should not be added). It also more flexible: a user may
> > > choose where it is better to place snapshots within its project (or even
> > > outside it).
> > 
> > Don't forget we need a way to run upgrade testing for several tarantool
> > versions automatically. With your suggestion we should specify one
> > option several times and I don't like this approach, because it will
> > overload command line for running tests.
> 
> For several snapshots? Each is a separate ./test-run.py run I guess. The
> option (as it is shown in the snippet above) does not allow to be
> specified several times (it would use `nargs='*'` if it would do).
> However I don't sure that specifying this option several times would be
> intuitive: this way ./test-run.py should run the whole testing several
> times with different snapshots. Usually test-run.py run testing only
> once.
> 
> Just example, I want to verify starting from a snapshot using existing
> tests on my own commit:
> 
>  | $ (cd test && for s in snapshots/*.snap; do ./test-run.py --snapshot $s; done)
> 
> It does not work. Ouch. Okay...
> 
>  | $ (cd test && for s in snapshots/*.snap; do ./test-run.py s=${s#snapshots/} --snapshot-verson ${s%.snap}; done)
> 
> Doable. Now I got some snapshot from a client and want to answer whether
> it is safe to update to a new tarantool version.
> 
>  | $ (cd test && ./test-run.py --snapshot ../0000<...>1234.snap)
> 
> Ugh, again, I need some additional actions:
> 
>  | $ mv 0000<...>1234.snap test/snapshots
>  | $ (cd test && ./test-run.py --snapshot-version 0000<...>1234)
> 
> Doable too. But do you find it convenient?

Discussed it orally and decided to make an option that accept path to a snapshot file.
Implemented in a branch.

> > > BTW, CI (`make lint`) show errors:
> > > https://travis-ci.com/github/tarantool/test-run/builds/156536350
> > 
> > Most warnings fixed.
> 
> Aren't it is better to keep CI green and fix them all? Or lift the limit
> up and keep CI green? I vote for the former, but okay, any way is
> acceptable.

Fixed all warnings and now CI is green.

> > > > +
> > > > +    def extract_schema_from_snapshot(self):
> > > 
> > > Nit: We can make it a class method.
> > 
> > What do you mean? It's already a class method.
> 
> @classmethod

function moved to another place and no sense to make it as classmethod.

Sergey

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

Hi, Alexander!

I just want to ask you to take a look on updated patch again.
I have fixed code according to your comments and also
fixed a bug when path for snapshot copying was invalid.

Branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4801-add-snapshot-option
CI: https://travis-ci.com/github/tarantool/test-run/builds/199375391

On 14:29 Wed 10 Jun , Sergey Bronnikov wrote:
> Hi,

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Leonid Vasiliev]

Hi! Thank your for the patch.
LGTM.
To add per line comments, I copied "diff" and marked my comments with
"Comment!" in that.
See some minor comments below:


Option '--snapshot-version' specifies a path to snapshot
that will be loaded in Tarantool before testing.

Option '--disable-schema-upgrade' allows to skip execution
of Tarantool upgrade script using error injection mechanism.

Part of: https://github.com/tarantool/tarantool/issues/4801
---
  lib/app_server.py       | 30 ++++++++++++++++++++++++++++--
  lib/options.py          | 26 ++++++++++++++++++++++++++
  lib/server.py           | 32 ++++++++++++++++++++++++++++----
  lib/tarantool_server.py | 33 ++++++++++++++++++++++++++++-----
  lib/utils.py            | 30 ++++++++++++++++++++++++++++++
  5 files changed, 140 insertions(+), 11 deletions(-)

diff --git a/lib/app_server.py b/lib/app_server.py
index 2cb8a87..efe9096 100644
--- a/lib/app_server.py
+++ b/lib/app_server.py
@@ -1,12 +1,15 @@
  import errno
  import glob
  import os
+import re
  import shutil
+import shlex
+import subprocess
  import sys

  from gevent.subprocess import Popen, PIPE

-from lib.colorer import color_log
+from lib.colorer import color_log, color_stdout
  from lib.preprocessor import TestState
  from lib.server import Server
  from lib.tarantool_server import Test
@@ -73,6 +76,10 @@ class AppServer(Server):
          self.binary = TarantoolServer.binary
          self.use_unix_sockets_iproto = ini['use_unix_sockets_iproto']

+        if self.disable_schema_upgrade and not self.test_debug():
+            color_stdout("Can't disable schema upgrade on release build\n",
+                         schema='error')
+
      @property
      def logfile(self):
          # remove suite name using basename
@@ -86,7 +93,12 @@ class AppServer(Server):
          return os.path.join(self.vardir, file_name)

      def prepare_args(self, args=[]):
-        return [os.path.join(os.getcwd(), self.current_test.name)] + args
+        cli_args = [os.path.join(os.getcwd(), self.current_test.name)] 
+ args
+        if self.disable_schema_upgrade:
+            cli_args = [self.binary, '-e',
+                        self.DISABLE_AUTO_UPGRADE] + cli_args
+
+        return cli_args

      def deploy(self, vardir=None, silent=True, need_init=True):
          self.vardir = vardir
@@ -170,3 +182,17 @@ class AppServer(Server):
                                       test_suite.ini))

          test_suite.tests = tests
+
+    def test_option_get(self, option_list_str, silent=False):
+        args = [self.binary] + shlex.split(option_list_str)
+        if not silent:
+            print " ".join([os.path.basename(self.binary)] + args[1:])
+        output = subprocess.Popen(args,
+                                  stdout=subprocess.PIPE,
+                                  stderr=subprocess.STDOUT).stdout.read()
+        return output
+
+    def test_debug(self):
+        if re.findall(r"-Debug", self.test_option_get("-V", True), re.I):
+            return True
+        return False
diff --git a/lib/options.py b/lib/options.py
index 47bbc0f..0a318ef 100644
--- a/lib/options.py
+++ b/lib/options.py
@@ -209,6 +209,20 @@ class Options:
                  help="""Update or create file with reference output 
(.result).
                  Default: false.""")

+        parser.add_argument(
+                "--snapshot-path",
+                dest='snapshot_path',
+                default=None,
+                help="""Path to a Tarantool snapshot that will be
+                loaded before testing.""")
+
+        parser.add_argument(
+                "--disable-schema-upgrade",
+                dest='disable_schema_upgrade',
+                action="store_true",
+                default=False,
+                help="""Disable schema upgrade on testing with 
snapshots.""")
+
          # XXX: We can use parser.parse_intermixed_args() on
          # Python 3.7 to understand commands like
          # ./test-run.py foo --exclude bar baz
@@ -227,5 +241,17 @@ class Options:
                  color_stdout(format_str.format(op1, op2), schema='error')
                  check_error = True

+        if getattr(self, 'disable_schema_upgrade', '') and \
+                not (self, 'snapshot_path', ''):
+            color_stdout("option --disable-schema-upgrade \
+                depends on --snapshot-version\n", schema='error')
+            check_error = True
+
+        if getattr(self, 'snapshot_path', ''):
+            snapshot_path = getattr(self, 'snapshot_path', '')
+            if not os.path.exists(snapshot_path):
+                color_stdout("path {} not exist\n", snapshot_path, 
schema='error')

More than 80 characters per line.

+                check_error = True
+
          if check_error:
              exit(-1)
diff --git a/lib/server.py b/lib/server.py
index 321eac7..b994248 100644
--- a/lib/server.py
+++ b/lib/server.py
@@ -1,6 +1,7 @@
  import glob
  import os
  import shutil
+import os.path

Comment! Unneeded. "os" has been imported previously.

  from itertools import product
  from lib.server_mixins import ValgrindMixin
  from lib.server_mixins import GdbMixin
@@ -8,7 +9,8 @@ from lib.server_mixins import GdbServerMixin
  from lib.server_mixins import LLdbMixin
  from lib.server_mixins import StraceMixin
  from lib.server_mixins import LuacovMixin
-from lib.colorer import color_stdout
+from lib.colorer import color_stdout, color_log
+from lib.options import Options
  from lib.utils import print_tail_n


@@ -24,6 +26,9 @@ class Server(object):
      DEFAULT_INSPECTOR = 0
      TEST_RUN_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__),
                                                  ".."))
+    DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \
+        assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 
'ok', \
+        'no such errinj')"

Comment! If I understand correctly, Turenko wanted to mark this with a 
comment
about https://github.com/tarantool/tarantool/issues/4983

      @property
      def vardir(self):
@@ -73,8 +78,7 @@ class Server(object):
          core = ini['core'].lower().strip()
          cls.mdlname = "lib.{0}_server".format(core.replace(' ', '_'))
          cls.clsname = "{0}Server".format(core.title().replace(' ', ''))
-        corecls = __import__(cls.mdlname,
-                             fromlist=cls.clsname).__dict__[cls.clsname]
+        corecls = __import__(cls.mdlname, 
fromlist=cls.clsname).__dict__[cls.clsname]

Comment! Unneeded changes and now it's more than 80 symbols per line.

          return corecls.__new__(corecls, ini, *args, **kwargs)

      def __init__(self, ini, test_suite=None):
@@ -84,6 +88,9 @@ class Server(object):
          self.inspector_port = int(ini.get(
              'inspector_port', self.DEFAULT_INSPECTOR
          ))
+        self.testdir = os.path.abspath(os.curdir)
+        self.disable_schema_upgrade = None
+        self.snapshot_path = Options().args.snapshot_path

          # filled in {Test,AppTest,LuaTest,PythonTest}.execute()
          # or passed through execfile() for PythonTest (see
@@ -94,6 +101,15 @@ class Server(object):
          # default servers running in TestSuite.run_all()
          self.test_suite = test_suite

+        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
+        if self.snapshot_path:
+            self.snapshot_path = os.path.abspath(self.snapshot_path)
+            if os.path.exists(self.snapshot_path):
+                self.snapshot_basename = 
os.path.basename(self.snapshot_path)
+            else:
+                color_stdout("\nSnapshot {} have been not 
found\n".format(self.snapshot_path),
+                             schema='error')

Comment! More than 80 characters per line. But I think it's ok here.

+
      def prepare_args(self, args=[]):
          return args

@@ -110,7 +126,15 @@ class Server(object):
                      os.remove(f)

      def install(self, binary=None, vardir=None, mem=None, silent=True):
-        pass
+        if self.snapshot_basename:
+            (instance_name, _) = 
os.path.splitext(os.path.basename(self.script))
+            instance_dir = os.path.join(self.vardir, instance_name)
+            if not os.path.exists(instance_dir):
+                os.mkdir(instance_dir)
+            snapshot_dest = os.path.join(instance_dir, 
"00000000000000000000.snap")

Comment! More than 80 characters per line.

+            color_log("Copying snapshot {} to {}\n".format(
+                self.snapshot_path, snapshot_dest))
+            shutil.copy(self.snapshot_path, snapshot_dest)

      def init(self):
          pass
diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index fd102b9..4f4c6b4 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -27,7 +27,7 @@ from lib.colorer import color_stdout, color_log
  from lib.preprocessor import TestState
  from lib.server import Server
  from lib.test import Test
-from lib.utils import find_port
+from lib.utils import find_port, extract_schema_from_snapshot
  from lib.utils import format_process
  from lib.utils import signame
  from lib.utils import warn_unix_socket
@@ -609,7 +609,6 @@ class TarantoolServer(Server):
          }
          ini.update(_ini)
          Server.__init__(self, ini, test_suite)
-        self.testdir = os.path.abspath(os.curdir)
          self.sourcedir = os.path.abspath(os.path.join(os.path.basename(
              sys.argv[0]), "..", ".."))
          self.name = "default"
@@ -641,6 +640,10 @@ class TarantoolServer(Server):
          if 'test_run_current_test' in caller_globals.keys():
              self.current_test = caller_globals['test_run_current_test']

+        if self.disable_schema_upgrade and not self.test_debug():
+            color_stdout("Can't disable schema upgrade on release build\n",
+                         schema='error')
+
      def __del__(self):
          self.stop()

@@ -745,6 +748,9 @@ class TarantoolServer(Server):
          path = os.path.join(self.vardir, self.name + '.control')
          warn_unix_socket(path)

+        super(TarantoolServer, self).install(self.binary,
+                                             self.vardir, None, 
silent=True)
+
      def deploy(self, silent=True, **kwargs):
          self.install(silent)
          self.start(silent=silent, **kwargs)
@@ -776,7 +782,13 @@ class TarantoolServer(Server):
                          self.vardir)

      def prepare_args(self, args=[]):
-        return [self.ctl_path, 'start', os.path.basename(self.script)] 
+ args
+        cli_args = [self.ctl_path, 'start',
+                    os.path.basename(self.script)] + args
+        if self.disable_schema_upgrade:
+            cli_args = [self.binary, '-e',
+                        self.DISABLE_AUTO_UPGRADE] + cli_args
+
+        return cli_args

      def pretest_clean(self):
          # Don't delete snap and logs for 'default' tarantool server
@@ -861,6 +873,18 @@ class TarantoolServer(Server):
          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
          self.status = 'started'

+        if not self.ctl_path:
+            self.ctl_path = self.find_exe(self.builddir)
+        if self.disable_schema_upgrade:
+            version = extract_schema_from_snapshot(self.ctl_path,
+                                                   self.builddir, 
self.snapshot_path)

Comment! More than 80 symbols per line.

+            current_ver = yaml.safe_load(self.admin.execute(
+                'box.space._schema:get{"version"}'))
+            if version == current_ver:
+                raise_msg = 'Versions are inequal ({} vs {})'.format(
+                    version, current_ver)
+                raise Exception(raise_msg)
+
      def crash_detect(self):
          if self.crash_expected:
              return
@@ -1066,7 +1090,6 @@ class TarantoolServer(Server):
          if not silent:
              print " ".join([os.path.basename(self.binary)] + args[1:])
          output = subprocess.Popen(args,
-                                  cwd=self.vardir,
                                    stdout=subprocess.PIPE,
                                    stderr=subprocess.STDOUT).stdout.read()
          return output
@@ -1126,7 +1149,7 @@ class TarantoolServer(Server):
      def get_param(self, param=None):
          if param is not None:
              return yaml.safe_load(self.admin("box.info." + param,
-                                  silent=True))[0]
+                                             silent=True))[0]
          return yaml.safe_load(self.admin("box.info", silent=True))

      def get_lsn(self, node_id):
diff --git a/lib/utils.py b/lib/utils.py
index cc2f6b7..7401d5c 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -3,8 +3,10 @@ import sys
  import six
  import collections
  import signal
+import subprocess
  import random
  import fcntl
+import json
  import difflib
  import time
  from gevent import socket
@@ -264,3 +266,31 @@ def just_and_trim(src, width):
      if len(src) > width:
          return src[:width - 1] + '>'
      return src.ljust(width)
+
+
+def extract_schema_from_snapshot(ctl_path, builddir, snapshot_path):
+    """
+    Extract schema version from snapshot, example of record:
+     {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
+       "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
+    """

Comment! If my internal python interpreter is working fine, the return value
is a list: "['version', 2, 3, 1]". Typically version is a string 
like:"2.3.1".
I don't mind, but specify it in description.

+    SCHEMA_SPACE_ID = 272
+
+    ctl_args = [ctl_path, 'cat', snapshot_path,
+                '--format=json', '--show-system']
+    proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE)
+    version = None
+    while True:
+        line = proc.stdout.readline()
+        if not line:
+            break
+        json_line = json.loads(line.strip())
+        query_type = json_line["HEADER"]["type"]
+        space_id = json_line["BODY"]["space_id"]
+        if query_type == "INSERT" or query_type == "REPLACE" and 
space_id == SCHEMA_SPACE_ID:

Comment! More than 80 characters per line.

+            query_tuple = json_line['BODY']['tuple']
+            if query_tuple[0] == 'version':
+                version = query_tuple
+                break
+
+    return version
-- 
2.7.4

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

Hi, Leonid!

thanks for review!

On 13.11.2020 15:41, Leonid Vasiliev wrote:
> Hi! Thank your for the patch.
> LGTM.
> To add per line comments, I copied "diff" and marked my comments with
> "Comment!" in that.
> See some minor comments below:
>
>
> Option '--snapshot-version' specifies a path to snapshot
> that will be loaded in Tarantool before testing.
>
> Option '--disable-schema-upgrade' allows to skip execution
> of Tarantool upgrade script using error injection mechanism.
>
> Part of: https://github.com/tarantool/tarantool/issues/4801
> ---
>  lib/app_server.py       | 30 ++++++++++++++++++++++++++++--
>  lib/options.py          | 26 ++++++++++++++++++++++++++
>  lib/server.py           | 32 ++++++++++++++++++++++++++++----
>  lib/tarantool_server.py | 33 ++++++++++++++++++++++++++++-----
>  lib/utils.py            | 30 ++++++++++++++++++++++++++++++
>  5 files changed, 140 insertions(+), 11 deletions(-)
>
> diff --git a/lib/app_server.py b/lib/app_server.py
> index 2cb8a87..efe9096 100644
> --- a/lib/app_server.py
> +++ b/lib/app_server.py
> @@ -1,12 +1,15 @@
>  import errno
>  import glob
>  import os
> +import re
>  import shutil
> +import shlex
> +import subprocess
>  import sys
>
>  from gevent.subprocess import Popen, PIPE
>
> -from lib.colorer import color_log
> +from lib.colorer import color_log, color_stdout
>  from lib.preprocessor import TestState
>  from lib.server import Server
>  from lib.tarantool_server import Test
> @@ -73,6 +76,10 @@ class AppServer(Server):
>          self.binary = TarantoolServer.binary
>          self.use_unix_sockets_iproto = ini['use_unix_sockets_iproto']
>
> +        if self.disable_schema_upgrade and not self.test_debug():
> +            color_stdout("Can't disable schema upgrade on release 
> build\n",
> +                         schema='error')
> +
>      @property
>      def logfile(self):
>          # remove suite name using basename
> @@ -86,7 +93,12 @@ class AppServer(Server):
>          return os.path.join(self.vardir, file_name)
>
>      def prepare_args(self, args=[]):
> -        return [os.path.join(os.getcwd(), self.current_test.name)] + 
> args
> +        cli_args = [os.path.join(os.getcwd(), 
> self.current_test.name)] + args
> +        if self.disable_schema_upgrade:
> +            cli_args = [self.binary, '-e',
> +                        self.DISABLE_AUTO_UPGRADE] + cli_args
> +
> +        return cli_args
>
>      def deploy(self, vardir=None, silent=True, need_init=True):
>          self.vardir = vardir
> @@ -170,3 +182,17 @@ class AppServer(Server):
>                                       test_suite.ini))
>
>          test_suite.tests = tests
> +
> +    def test_option_get(self, option_list_str, silent=False):
> +        args = [self.binary] + shlex.split(option_list_str)
> +        if not silent:
> +            print " ".join([os.path.basename(self.binary)] + args[1:])
> +        output = subprocess.Popen(args,
> +                                  stdout=subprocess.PIPE,
> + stderr=subprocess.STDOUT).stdout.read()
> +        return output
> +
> +    def test_debug(self):
> +        if re.findall(r"-Debug", self.test_option_get("-V", True), 
> re.I):
> +            return True
> +        return False
> diff --git a/lib/options.py b/lib/options.py
> index 47bbc0f..0a318ef 100644
> --- a/lib/options.py
> +++ b/lib/options.py
> @@ -209,6 +209,20 @@ class Options:
>                  help="""Update or create file with reference output 
> (.result).
>                  Default: false.""")
>
> +        parser.add_argument(
> +                "--snapshot-path",
> +                dest='snapshot_path',
> +                default=None,
> +                help="""Path to a Tarantool snapshot that will be
> +                loaded before testing.""")
> +
> +        parser.add_argument(
> +                "--disable-schema-upgrade",
> +                dest='disable_schema_upgrade',
> +                action="store_true",
> +                default=False,
> +                help="""Disable schema upgrade on testing with 
> snapshots.""")
> +
>          # XXX: We can use parser.parse_intermixed_args() on
>          # Python 3.7 to understand commands like
>          # ./test-run.py foo --exclude bar baz
> @@ -227,5 +241,17 @@ class Options:
>                  color_stdout(format_str.format(op1, op2), 
> schema='error')
>                  check_error = True
>
> +        if getattr(self, 'disable_schema_upgrade', '') and \
> +                not (self, 'snapshot_path', ''):
> +            color_stdout("option --disable-schema-upgrade \
> +                depends on --snapshot-version\n", schema='error')
> +            check_error = True
> +
> +        if getattr(self, 'snapshot_path', ''):
> +            snapshot_path = getattr(self, 'snapshot_path', '')
> +            if not os.path.exists(snapshot_path):
> +                color_stdout("path {} not exist\n", snapshot_path, 
> schema='error')
>
> More than 80 characters per line.

Decided to increase limit in test-run CI to 100 symbols. So 80 is ok for 
test-run source code.


>
> +                check_error = True
> +
>          if check_error:
>              exit(-1)
> diff --git a/lib/server.py b/lib/server.py
> index 321eac7..b994248 100644
> --- a/lib/server.py
> +++ b/lib/server.py
> @@ -1,6 +1,7 @@
>  import glob
>  import os
>  import shutil
> +import os.path
>
> Comment! Unneeded. "os" has been imported previously.
>
Removed extra import.


>  from itertools import product
>  from lib.server_mixins import ValgrindMixin
>  from lib.server_mixins import GdbMixin
> @@ -8,7 +9,8 @@ from lib.server_mixins import GdbServerMixin
>  from lib.server_mixins import LLdbMixin
>  from lib.server_mixins import StraceMixin
>  from lib.server_mixins import LuacovMixin
> -from lib.colorer import color_stdout
> +from lib.colorer import color_stdout, color_log
> +from lib.options import Options
>  from lib.utils import print_tail_n
>
>
> @@ -24,6 +26,9 @@ class Server(object):
>      DEFAULT_INSPECTOR = 0
>      TEST_RUN_DIR = 
> os.path.abspath(os.path.join(os.path.dirname(__file__),
>                                                  ".."))
> +    DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \
> +        assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) 
> == 'ok', \
> +        'no such errinj')"
>
> Comment! If I understand correctly, Turenko wanted to mark this with a 
> comment
> about https://github.com/tarantool/tarantool/issues/4983

Added comment with explanation and link to an issue.


>
>      @property
>      def vardir(self):
> @@ -73,8 +78,7 @@ class Server(object):
>          core = ini['core'].lower().strip()
>          cls.mdlname = "lib.{0}_server".format(core.replace(' ', '_'))
>          cls.clsname = "{0}Server".format(core.title().replace(' ', ''))
> -        corecls = __import__(cls.mdlname,
> - fromlist=cls.clsname).__dict__[cls.clsname]
> +        corecls = __import__(cls.mdlname, 
> fromlist=cls.clsname).__dict__[cls.clsname]
>
> Comment! Unneeded changes and now it's more than 80 symbols per line.
>

Decided to increase limit in test-run CI to 100 symbols. So 80 is ok for 
test-run source code.

> return corecls.__new__(corecls, ini, *args, **kwargs)
>
>      def __init__(self, ini, test_suite=None):
> @@ -84,6 +88,9 @@ class Server(object):
>          self.inspector_port = int(ini.get(
>              'inspector_port', self.DEFAULT_INSPECTOR
>          ))
> +        self.testdir = os.path.abspath(os.curdir)
> +        self.disable_schema_upgrade = None
> +        self.snapshot_path = Options().args.snapshot_path
>
>          # filled in {Test,AppTest,LuaTest,PythonTest}.execute()
>          # or passed through execfile() for PythonTest (see
> @@ -94,6 +101,15 @@ class Server(object):
>          # default servers running in TestSuite.run_all()
>          self.test_suite = test_suite
>
> +        self.disable_schema_upgrade = 
> Options().args.disable_schema_upgrade
> +        if self.snapshot_path:
> +            self.snapshot_path = os.path.abspath(self.snapshot_path)
> +            if os.path.exists(self.snapshot_path):
> +                self.snapshot_basename = 
> os.path.basename(self.snapshot_path)
> +            else:
> +                color_stdout("\nSnapshot {} have been not 
> found\n".format(self.snapshot_path),
> +                             schema='error')
>
> Comment! More than 80 characters per line. But I think it's ok here.

Decided to increase limit in test-run CI to 100 symbols. So 80 is ok for 
test-run source code.

>
> +
>      def prepare_args(self, args=[]):
>          return args
>
> @@ -110,7 +126,15 @@ class Server(object):
>                      os.remove(f)
>
>      def install(self, binary=None, vardir=None, mem=None, silent=True):
> -        pass
> +        if self.snapshot_basename:
> +            (instance_name, _) = 
> os.path.splitext(os.path.basename(self.script))
> +            instance_dir = os.path.join(self.vardir, instance_name)
> +            if not os.path.exists(instance_dir):
> +                os.mkdir(instance_dir)
> +            snapshot_dest = os.path.join(instance_dir, 
> "00000000000000000000.snap")
>
> Comment! More than 80 characters per line.

Decided to increase limit in test-run CI to 100 symbols. So 80 is ok for 
test-run source code.

>
> +            color_log("Copying snapshot {} to {}\n".format(
> +                self.snapshot_path, snapshot_dest))
> +            shutil.copy(self.snapshot_path, snapshot_dest)
>
>      def init(self):
>          pass
> diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
> index fd102b9..4f4c6b4 100644
> --- a/lib/tarantool_server.py
> +++ b/lib/tarantool_server.py
> @@ -27,7 +27,7 @@ from lib.colorer import color_stdout, color_log
>  from lib.preprocessor import TestState
>  from lib.server import Server
>  from lib.test import Test
> -from lib.utils import find_port
> +from lib.utils import find_port, extract_schema_from_snapshot
>  from lib.utils import format_process
>  from lib.utils import signame
>  from lib.utils import warn_unix_socket
> @@ -609,7 +609,6 @@ class TarantoolServer(Server):
>          }
>          ini.update(_ini)
>          Server.__init__(self, ini, test_suite)
> -        self.testdir = os.path.abspath(os.curdir)
>          self.sourcedir = os.path.abspath(os.path.join(os.path.basename(
>              sys.argv[0]), "..", ".."))
>          self.name = "default"
> @@ -641,6 +640,10 @@ class TarantoolServer(Server):
>          if 'test_run_current_test' in caller_globals.keys():
>              self.current_test = caller_globals['test_run_current_test']
>
> +        if self.disable_schema_upgrade and not self.test_debug():
> +            color_stdout("Can't disable schema upgrade on release 
> build\n",
> +                         schema='error')
> +
>      def __del__(self):
>          self.stop()
>
> @@ -745,6 +748,9 @@ class TarantoolServer(Server):
>          path = os.path.join(self.vardir, self.name + '.control')
>          warn_unix_socket(path)
>
> +        super(TarantoolServer, self).install(self.binary,
> +                                             self.vardir, None, 
> silent=True)
> +
>      def deploy(self, silent=True, **kwargs):
>          self.install(silent)
>          self.start(silent=silent, **kwargs)
> @@ -776,7 +782,13 @@ class TarantoolServer(Server):
>                          self.vardir)
>
>      def prepare_args(self, args=[]):
> -        return [self.ctl_path, 'start', 
> os.path.basename(self.script)] + args
> +        cli_args = [self.ctl_path, 'start',
> +                    os.path.basename(self.script)] + args
> +        if self.disable_schema_upgrade:
> +            cli_args = [self.binary, '-e',
> +                        self.DISABLE_AUTO_UPGRADE] + cli_args
> +
> +        return cli_args
>
>      def pretest_clean(self):
>          # Don't delete snap and logs for 'default' tarantool server
> @@ -861,6 +873,18 @@ class TarantoolServer(Server):
>          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
>          self.status = 'started'
>
> +        if not self.ctl_path:
> +            self.ctl_path = self.find_exe(self.builddir)
> +        if self.disable_schema_upgrade:
> +            version = extract_schema_from_snapshot(self.ctl_path,
> +                                                   self.builddir, 
> self.snapshot_path)
>
> Comment! More than 80 symbols per line.
>
> +            current_ver = yaml.safe_load(self.admin.execute(
> +                'box.space._schema:get{"version"}'))
> +            if version == current_ver:
> +                raise_msg = 'Versions are inequal ({} vs {})'.format(
> +                    version, current_ver)
> +                raise Exception(raise_msg)
> +
>      def crash_detect(self):
>          if self.crash_expected:
>              return
> @@ -1066,7 +1090,6 @@ class TarantoolServer(Server):
>          if not silent:
>              print " ".join([os.path.basename(self.binary)] + args[1:])
>          output = subprocess.Popen(args,
> -                                  cwd=self.vardir,
>                                    stdout=subprocess.PIPE,
> stderr=subprocess.STDOUT).stdout.read()
>          return output
> @@ -1126,7 +1149,7 @@ class TarantoolServer(Server):
>      def get_param(self, param=None):
>          if param is not None:
>              return yaml.safe_load(self.admin("box.info." + param,
> -                                  silent=True))[0]
> +                                             silent=True))[0]
>          return yaml.safe_load(self.admin("box.info", silent=True))
>
>      def get_lsn(self, node_id):
> diff --git a/lib/utils.py b/lib/utils.py
> index cc2f6b7..7401d5c 100644
> --- a/lib/utils.py
> +++ b/lib/utils.py
> @@ -3,8 +3,10 @@ import sys
>  import six
>  import collections
>  import signal
> +import subprocess
>  import random
>  import fcntl
> +import json
>  import difflib
>  import time
>  from gevent import socket
> @@ -264,3 +266,31 @@ def just_and_trim(src, width):
>      if len(src) > width:
>          return src[:width - 1] + '>'
>      return src.ljust(width)
> +
> +
> +def extract_schema_from_snapshot(ctl_path, builddir, snapshot_path):
> +    """
> +    Extract schema version from snapshot, example of record:
> + {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
> +       "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
> +    """
>
> Comment! If my internal python interpreter is working fine, the return 
> value
> is a list: "['version', 2, 3, 1]". Typically version is a string 
> like:"2.3.1".
> I don't mind, but specify it in description.

Documented in a docstring:

--- a/lib/utils.py
+++ b/lib/utils.py
@@ -274,6 +274,8 @@ def extract_schema_from_snapshot(ctl_path, builddir, 
snapshot_path):
      Extract schema version from snapshot, example of record:
{"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
         "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
+
+    :returns: [u'version', 2, 3, 1]
      """
      SCHEMA_SPACE_ID = 272


>
> +    SCHEMA_SPACE_ID = 272
> +
> +    ctl_args = [ctl_path, 'cat', snapshot_path,
> +                '--format=json', '--show-system']
> +    proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE)
> +    version = None
> +    while True:
> +        line = proc.stdout.readline()
> +        if not line:
> +            break
> +        json_line = json.loads(line.strip())
> +        query_type = json_line["HEADER"]["type"]
> +        space_id = json_line["BODY"]["space_id"]
> +        if query_type == "INSERT" or query_type == "REPLACE" and 
> space_id == SCHEMA_SPACE_ID:
>
> Comment! More than 80 characters per line.

Decided to increase limit in test-run CI to 100 symbols. So 80 is ok for 
test-run source code.

>
> +            query_tuple = json_line['BODY']['tuple']
> +            if query_tuple[0] == 'version':
> +                version = query_tuple
> +                break
> +
> +    return version

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Alexander Turenko]

On Tue, Nov 10, 2020 at 12:55:46PM +0300, Sergey Bronnikov wrote:
> Hi, Alexander!
> 
> I just want to ask you to take a look on updated patch again.
> I have fixed code according to your comments and also
> fixed a bug when path for snapshot copying was invalid.
> 
> Branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4801-add-snapshot-option
> CI: https://travis-ci.com/github/tarantool/test-run/builds/199375391
> 
> On 14:29 Wed 10 Jun , Sergey Bronnikov wrote:
> > Hi,

Hi!

Sorry for the delay.

I looked over the code and didn't perform any testing of the changes. I
hope the success path works as expected, however there are certainly
broken error paths and there are code hunks for test-run.py parameters
validation, which should not spread far away from options.py. See
details below.

WBR, Alexander Turenko.

> Option '--snapshot-version' specifies a path to snapshot
> that will be loaded in Tarantool before testing.

I would name it --snapshot, because the argument of the option is path
to a snapshot, not a version of some kind.

Ouch, I see, it is --snapshot-path now, but the commit message does not
reflect this change. --snapshot-path is okay for me too (despite that
personally I would prefer --snapshot).

(lib/app_server.py)
> +    def test_option_get(self, option_list_str, silent=False):
> +        args = [self.binary] + shlex.split(option_list_str)
> +        if not silent:
> +            print " ".join([os.path.basename(self.binary)] + args[1:])
> +        output = subprocess.Popen(args,
> +                                  stdout=subprocess.PIPE,
> +                                  stderr=subprocess.STDOUT).stdout.read()
> +        return output
> +
> +    def test_debug(self):
> +        if re.findall(r"-Debug", self.test_option_get("-V", True), re.I):
> +            return True
> +        return False

I don't see a reason to duplicate the code, it is already present in
TarantoolServer. We run all tests against the same tarantool. We should
move test-run.py parameters validation code out from the *Server classes
and the need to have this code in AppServer will gone.

> +        if getattr(self, 'disable_schema_upgrade', '') and \
> +                not (self, 'snapshot_path', ''):
> +            color_stdout("option --disable-schema-upgrade \
> +                depends on --snapshot-version\n", schema='error')
> +            check_error = True
> +
> +        if getattr(self, 'snapshot_path', ''):
> +            snapshot_path = getattr(self, 'snapshot_path', '')
> +            if not os.path.exists(snapshot_path):
> +                color_stdout("path {} not exist\n", snapshot_path, schema='error')
> +                check_error = True

First, it does not work at all as well as the validation for conflicting
--gdb, --lldb and so options above. It tries to obtain values from
`self` instead of `self.args`.

Second, there is no reason to use getattr() for options that have
default values.

Third, there is no --snapshot-version option anymore.

Fourth, the first message contains extra whitespaces:

 | option --disable-schema-upgrade                 depends on --snapshot-version

The second one is printed in two lines without leading newline
character:

 | path {} not exist
 | fooalex@tkn_work_nb test-run ((df4a0e8...) *)

(It should be 'not exist*s*'.)

This code would not work even if getattr() from `self` would do, because
of missed getattr(): `not (self, 'snapshot_path', '')`.

It is also strange to use '' as default value for a boolean option.

> -from lib.colorer import color_stdout
> +from lib.colorer import color_stdout, color_log
> +from lib.options import Options

Nit: Most of imports are function-per-line. I would keep the style
instead of randomly using one or another.

> +    DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \
> +        assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', \
> +        'no such errinj')"

You and me knows why fiber.sleep() is here. Nobody else will understand
this code. Please, refer the relevant issue and mention that it is
workaround for it.

> -        corecls = __import__(cls.mdlname,
> -                             fromlist=cls.clsname).__dict__[cls.clsname]
> +        corecls = __import__(cls.mdlname, fromlist=cls.clsname).__dict__[cls.clsname]

Unrelated change.

> +        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
> +        if self.snapshot_path:
> +            self.snapshot_path = os.path.abspath(self.snapshot_path)
> +            if os.path.exists(self.snapshot_path):
> +                self.snapshot_basename = os.path.basename(self.snapshot_path)
> +            else:
> +                color_stdout("\nSnapshot {} have been not found\n".format(self.snapshot_path),
> +                             schema='error')

I don't understand this logic. If there is no requested snapshot file,
we'll print the warning and continue? In my understanding we should give
an error and stop, if an incorrect option is passed.

And parameters verification should not be grounded somewhere in
unrelated code. Preferably it should be in options.py. If some
test-run's facilities are used to verify the option, it should be
somewhere after options parsing in some top-level code. Maybe in
module_init(). However here we just verify whether the file exists, so
let's move to options.py.

>      def install(self, binary=None, vardir=None, mem=None, silent=True):
> -        pass
> +        if self.snapshot_basename:
> +            (instance_name, _) = os.path.splitext(os.path.basename(self.script))
> +            instance_dir = os.path.join(self.vardir, instance_name)
> +            if not os.path.exists(instance_dir):
> +                os.mkdir(instance_dir)
> +            snapshot_dest = os.path.join(instance_dir, "00000000000000000000.snap")
> +            color_log("Copying snapshot {} to {}\n".format(
> +                self.snapshot_path, snapshot_dest))
> +            shutil.copy(self.snapshot_path, snapshot_dest)

We assume tarantoolctl layout (as it is defined in test/.tarantoolctl):

 | vardir/
 | + instance_name/
 |   + *.{snap,xlog}
 | + instance_name.log
 | + instance_name.control

It is not relevant for AppServer and UnittestServer.

>  from lib.preprocessor import TestState
>  from lib.server import Server
>  from lib.test import Test
> -from lib.utils import find_port
> +from lib.utils import find_port, extract_schema_from_snapshot
>  from lib.utils import format_process
>  from lib.utils import signame
>  from lib.utils import warn_unix_socket

Nit: Most of imports are function-per-line, I would keep it this way. It
looks more accurate.

> @@ -609,7 +609,6 @@ def __init__(self, _ini=None, test_suite=None):
>          }
>          ini.update(_ini)
>          Server.__init__(self, ini, test_suite)
> -        self.testdir = os.path.abspath(os.curdir)
>          self.sourcedir = os.path.abspath(os.path.join(os.path.basename(
>              sys.argv[0]), "..", ".."))
>          self.name = "default"

I see that it is moved to the parent class, but:

1. It does not look related to your changes, but if it is related, you
   should describe a reason in the commit message.
2. If it is a side refactoring, you should move it to its own commit
   with explained motivation.

For now, I don't see any reason for this change.

> @@ -641,6 +640,10 @@ def __init__(self, _ini=None, test_suite=None):
>          if 'test_run_current_test' in caller_globals.keys():
>              self.current_test = caller_globals['test_run_current_test']
>  
> +        if self.disable_schema_upgrade and not self.test_debug():
> +            color_stdout("Can't disable schema upgrade on release build\n",
> +                         schema='error')
> +

Should not we verify it on the upper level, because it is like options
validation? Say, right in options.py or at least in module_init()?

Should not we give an error and don't start testing in the case?

BTW, there is `debug` property, which calls test_debug().

> @@ -861,6 +873,18 @@ def start(self, silent=True, wait=True, wait_load=True, rais=True, args=[],
>          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
>          self.status = 'started'
>  
> +        if not self.ctl_path:
> +            self.ctl_path = self.find_exe(self.builddir)

1. Are there situations, when there is no `ctl_path` value at starting a
   server? So `prepare_args()` call above is incorrect? I don't get the
   trick. I guess `lib/__init__.py` call to `find_exe()` should be
   enough to fill `cls.ctl_path`.
2. `find_exe()` sets `cls.ctl_path` on its own.

> +        if self.disable_schema_upgrade:
> +            version = extract_schema_from_snapshot(self.ctl_path,
> +                                                   self.builddir, self.snapshot_path)
> +            current_ver = yaml.safe_load(self.admin.execute(
> +                'box.space._schema:get{"version"}'))
> +            if version == current_ver:
> +                raise_msg = 'Versions are inequal ({} vs {})'.format(
> +                    version, current_ver)
> +                raise Exception(raise_msg)
> +

AFAIS, the usual recommendation is to raise standard or user-defined
Exception subclasses. It seems, RuntimeError fits best here.

However, I guess, RuntimeError and TarantoolStartError will lead to
different behaviour. The former just fails the test, but the latter
stops testing at whole. Should not we stop testing in the case?

Will the new server left running in the case? It is undesirable
situation.

> @@ -1066,7 +1090,6 @@ def test_option_get(self, option_list_str, silent=False):
>          if not silent:
>              print " ".join([os.path.basename(self.binary)] + args[1:])
>          output = subprocess.Popen(args,
> -                                  cwd=self.vardir,
>                                    stdout=subprocess.PIPE,
>                                    stderr=subprocess.STDOUT).stdout.read()
>          return output

1. It looks unrelated.
2. There is not any described reason for this change.

> @@ -1126,7 +1149,7 @@ def is_correct(run_name):
>      def get_param(self, param=None):
>          if param is not None:
>              return yaml.safe_load(self.admin("box.info." + param,
> -                                  silent=True))[0]
> +                                             silent=True))[0]
>          return yaml.safe_load(self.admin("box.info", silent=True))

Unrelated change.

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

Hi, Alexander!

thanks for review!

I have updated the whole patch and I'll send it as reply to this mail.

Below are my answers regarding each your comment.


On 16.11.2020 19:40, Alexander Turenko wrote:
> On Tue, Nov 10, 2020 at 12:55:46PM +0300, Sergey Bronnikov wrote:
>> Hi, Alexander!
>>
>> I just want to ask you to take a look on updated patch again.
>> I have fixed code according to your comments and also
>> fixed a bug when path for snapshot copying was invalid.
>>
>> Branch: https://github.com/tarantool/test-run/tree/ligurio/gh-4801-add-snapshot-option
>> CI: https://travis-ci.com/github/tarantool/test-run/builds/199375391
>>
>> On 14:29 Wed 10 Jun , Sergey Bronnikov wrote:
>>> Hi,
> Hi!
>
> Sorry for the delay.
>
> I looked over the code and didn't perform any testing of the changes. I
> hope the success path works as expected, however there are certainly
> broken error paths and there are code hunks for test-run.py parameters
> validation, which should not spread far away from options.py. See
> details below.
>
> WBR, Alexander Turenko.
>
>> Option '--snapshot-version' specifies a path to snapshot
>> that will be loaded in Tarantool before testing.
> I would name it --snapshot, because the argument of the option is path
> to a snapshot, not a version of some kind.
>
> Ouch, I see, it is --snapshot-path now, but the commit message does not
> reflect this change. --snapshot-path is okay for me too (despite that
> personally I would prefer --snapshot).

Yes, it was renamed as you suggested previous time, but not everywhere.

Fixed it.

+++ b/lib/options.py
@@ -210,9 +210,8 @@ class Options:
                  Default: false.""")

          parser.add_argument(
-                "--snapshot-path",
+                "--snapshot",
                  dest='snapshot_path',
-                default=None,
                  help="""Path to a Tarantool snapshot that will be
                  loaded before testing.""")

@@ -241,17 +240,23 @@ class Options:
                  color_stdout(format_str.format(op1, op2), schema='error')
                  check_error = True

-        if getattr(self, 'disable_schema_upgrade', '') and \
-                not (self, 'snapshot_path', ''):
-            color_stdout("option --disable-schema-upgrade \
-                depends on --snapshot-version\n", schema='error')
+        snapshot_path = getattr(self.args, 'snapshot_path', '')
+        if self.args.disable_schema_upgrade and not snapshot_path:
+            color_stdout("option --disable-schema-upgrade requires 
--snapshot\n",
+                        schema='error')
              check_error = True

>
> (lib/app_server.py)
>> +    def test_option_get(self, option_list_str, silent=False):
>> +        args = [self.binary] + shlex.split(option_list_str)
>> +        if not silent:
>> +            print " ".join([os.path.basename(self.binary)] + args[1:])
>> +        output = subprocess.Popen(args,
>> +                                  stdout=subprocess.PIPE,
>> +                                  stderr=subprocess.STDOUT).stdout.read()
>> +        return output
>> +
>> +    def test_debug(self):
>> +        if re.findall(r"-Debug", self.test_option_get("-V", True), re.I):
>> +            return True
>> +        return False
> I don't see a reason to duplicate the code, it is already present in
> TarantoolServer. We run all tests against the same tarantool. We should
> move test-run.py parameters validation code out from the *Server classes
> and the need to have this code in AppServer will gone.
>
Function test_debug() implemented in utils.py and called in 
lib.tarantool_server.py.

Functions test_debug() and test_get_option() are moved from 
lib/app_server.py.

@@ -182,17 +174,3 @@ class AppServer(Server):
                                       test_suite.ini))

          test_suite.tests = tests
-
-    def test_option_get(self, option_list_str, silent=False):
-        args = [self.binary] + shlex.split(option_list_str)
-        if not silent:
-            print " ".join([os.path.basename(self.binary)] + args[1:])
-        output = subprocess.Popen(args,
-                                  stdout=subprocess.PIPE,
- stderr=subprocess.STDOUT).stdout.read()
-        return output
-
-    def test_debug(self):
-        if re.findall(r"-Debug", self.test_option_get("-V", True), re.I):
-            return True
-        return False

>> +        if getattr(self, 'disable_schema_upgrade', '') and \
>> +                not (self, 'snapshot_path', ''):
>> +            color_stdout("option --disable-schema-upgrade \
>> +                depends on --snapshot-version\n", schema='error')
>> +            check_error = True
>> +
>> +        if getattr(self, 'snapshot_path', ''):
>> +            snapshot_path = getattr(self, 'snapshot_path', '')
>> +            if not os.path.exists(snapshot_path):
>> +                color_stdout("path {} not exist\n", snapshot_path, schema='error')
>> +                check_error = True
> First, it does not work at all as well as the validation for conflicting
> --gdb, --lldb and so options above. It tries to obtain values from
> `self` instead of `self.args`.
>
> Second, there is no reason to use getattr() for options that have
> default values.
>
> Third, there is no --snapshot-version option anymore.

Fixed.

@@ -241,17 +240,23 @@ class Options:
                  color_stdout(format_str.format(op1, op2), schema='error')
                  check_error = True

-        if getattr(self, 'disable_schema_upgrade', '') and \
-                not (self, 'snapshot_path', ''):
-            color_stdout("option --disable-schema-upgrade \
-                depends on --snapshot-version\n", schema='error')
+        snapshot_path = getattr(self.args, 'snapshot_path', '')
+        if self.args.disable_schema_upgrade and not snapshot_path:
+            color_stdout("option --disable-schema-upgrade requires 
--snapshot\n",
+                        schema='error')
              check_error = True

-        if getattr(self, 'snapshot_path', ''):
-            snapshot_path = getattr(self, 'snapshot_path', '')
+        if snapshot_path:
              if not os.path.exists(snapshot_path):
-                color_stdout("path {} not exist\n", snapshot_path, 
schema='error')
+                color_stdout("Path {} not 
exists\n".format(snapshot_path), schema='error')
                  check_error = True
+            else:
+                self.args.snapshot_path = os.path.abspath(snapshot_path)

          if check_error:
              exit(-1)


BTW code with checking conflicting options fixed too in a separate patch.

>
> Fourth, the first message contains extra whitespaces:
>
>   | option --disable-schema-upgrade                 depends on --snapshot-version

-        if getattr(self, 'disable_schema_upgrade', '') and \
-                not (self, 'snapshot_path', ''):
-            color_stdout("option --disable-schema-upgrade \
-                depends on --snapshot-version\n", schema='error')
+        snapshot_path = getattr(self.args, 'snapshot_path', '')
+        if self.args.disable_schema_upgrade and not snapshot_path:
+            color_stdout("option --disable-schema-upgrade requires 
--snapshot\n",
+                        schema='error')
              check_error = True

> The second one is printed in two lines without leading newline
> character:
>
>   | path {} not exist
>   | fooalex@tkn_work_nb test-run ((df4a0e8...) *)
>
> (It should be 'not exist*s*'.)

Thanks! Missed it. Format string uses {}, but method "format()" with 
argument is missed.

Fixed.

-                color_stdout("path {} not exist\n", snapshot_path, 
schema='error')
+               color_stdout("Path {} not 
exists\n".format(snapshot_path), schema='error')
                  check_error = True


>
> This code would not work even if getattr() from `self` would do, because
> of missed getattr(): `not (self, 'snapshot_path', '')`.
>
> It is also strange to use '' as default value for a boolean option.

Fixed!

@@ -241,17 +240,23 @@ class Options:
                  color_stdout(format_str.format(op1, op2), schema='error')
                  check_error = True

-        if getattr(self, 'disable_schema_upgrade', '') and \
-                not (self, 'snapshot_path', ''):
-            color_stdout("option --disable-schema-upgrade \
-                depends on --snapshot-version\n", schema='error')
+        snapshot_path = getattr(self.args, 'snapshot_path', '')
+        if self.args.disable_schema_upgrade and not snapshot_path:
+            color_stdout("option --disable-schema-upgrade requires 
--snapshot\n",
+                        schema='error')
              check_error = True

-        if getattr(self, 'snapshot_path', ''):
-            snapshot_path = getattr(self, 'snapshot_path', '')
+        if snapshot_path:
              if not os.path.exists(snapshot_path):
-                color_stdout("path {} not exist\n", snapshot_path, 
schema='error')
+                color_stdout("Path {} not 
exists\n".format(snapshot_path), schema='error')
                  check_error = True
+            else:
+                self.args.snapshot_path = os.path.abspath(snapshot_path)

          if check_error:
              exit(-1)


>
>> -from lib.colorer import color_stdout
>> +from lib.colorer import color_stdout, color_log
>> +from lib.options import Options
> Nit: Most of imports are function-per-line. I would keep the style
> instead of randomly using one or another.
>
>> +    DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \
>> +        assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', \
>> +        'no such errinj')"
> You and me knows why fiber.sleep() is here. Nobody else will understand
> this code. Please, refer the relevant issue and mention that it is
> workaround for it.
      DEFAULT_INSPECTOR = 0
      TEST_RUN_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__),
                                                  ".."))
+    # assert(false) hangs due to gh-4983, added fiber.sleep(0) to 
workaround  it
      DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \
          assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 
'ok', \
          'no such errinj')"

>> -        corecls = __import__(cls.mdlname,
>> -                             fromlist=cls.clsname).__dict__[cls.clsname]
>> +        corecls = __import__(cls.mdlname, fromlist=cls.clsname).__dict__[cls.clsname]
> Unrelated change.

Reverted back.

>> +        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
>> +        if self.snapshot_path:
>> +            self.snapshot_path = os.path.abspath(self.snapshot_path)
>> +            if os.path.exists(self.snapshot_path):
>> +                self.snapshot_basename = os.path.basename(self.snapshot_path)
>> +            else:
>> +                color_stdout("\nSnapshot {} have been not found\n".format(self.snapshot_path),
>> +                             schema='error')
> I don't understand this logic. If there is no requested snapshot file,
> we'll print the warning and continue? In my understanding we should give
> an error and stop, if an incorrect option is passed.
>
> And parameters verification should not be grounded somewhere in
> unrelated code. Preferably it should be in options.py. If some
> test-run's facilities are used to verify the option, it should be
> somewhere after options parsing in some top-level code. Maybe in
> module_init(). However here we just verify whether the file exists, so
> let's move to options.py.

checks for arguments moved to __init__.py and options.py.

--- a/lib/__init__.py
+++ b/lib/__init__.py
@@ -6,6 +6,7 @@ from lib.options import Options
  from lib.tarantool_server import TarantoolServer
  from lib.unittest_server import UnittestServer
  from utils import warn_unix_sockets_at_start
+from utils import test_debug


  __all__ = ['Options']
@@ -56,6 +57,9 @@ def module_init():
      TarantoolServer.find_exe(args.builddir)
      UnittestServer.find_exe(args.builddir)

+    is_debug = test_debug(TarantoolServer().version())
+    Options().check_schema_upgrade_option(is_debug)
+


lib.options.py:

@@ -241,17 +240,23 @@ class Options:
                  color_stdout(format_str.format(op1, op2), schema='error')
                  check_error = True

-        if getattr(self, 'disable_schema_upgrade', '') and \
-                not (self, 'snapshot_path', ''):
-            color_stdout("option --disable-schema-upgrade \
-                depends on --snapshot-version\n", schema='error')
+        snapshot_path = getattr(self.args, 'snapshot_path', '')
+        if self.args.disable_schema_upgrade and not snapshot_path:
+            color_stdout("option --disable-schema-upgrade requires 
--snapshot\n",
+                        schema='error')
              check_error = True

-        if getattr(self, 'snapshot_path', ''):
-            snapshot_path = getattr(self, 'snapshot_path', '')
+        if snapshot_path:
              if not os.path.exists(snapshot_path):
-                color_stdout("path {} not exist\n", snapshot_path, 
schema='error')
+                color_stdout("Path {} not 
exists\n".format(snapshot_path), schema='error')
                  check_error = True
+            else:
+                self.args.snapshot_path = os.path.abspath(snapshot_path)

          if check_error:
              exit(-1)
+
+    def check_schema_upgrade_option(self, is_debug):
+        if self.args.disable_schema_upgrade and not is_debug:
+            color_stdout("Can't disable schema upgrade on release 
build\n", schema='error')
+            exit(-1)

>
>>       def install(self, binary=None, vardir=None, mem=None, silent=True):
>> -        pass
>> +        if self.snapshot_basename:
>> +            (instance_name, _) = os.path.splitext(os.path.basename(self.script))
>> +            instance_dir = os.path.join(self.vardir, instance_name)
>> +            if not os.path.exists(instance_dir):
>> +                os.mkdir(instance_dir)
>> +            snapshot_dest = os.path.join(instance_dir, "00000000000000000000.snap")
>> +            color_log("Copying snapshot {} to {}\n".format(
>> +                self.snapshot_path, snapshot_dest))
>> +            shutil.copy(self.snapshot_path, snapshot_dest)
> We assume tarantoolctl layout (as it is defined in test/.tarantoolctl):
>
>   | vardir/
>   | + instance_name/
>   |   + *.{snap,xlog}
>   | + instance_name.log
>   | + instance_name.control
>
> It is not relevant for AppServer and UnittestServer.

Fixed in updated patch. In short: code with copying snapshot

moved from base class Server to AppServer and TarantoolServer classes.

Code in both classes are similar but I think it is no sense to add a 
common function for it.


>>   from lib.preprocessor import TestState
>>   from lib.server import Server
>>   from lib.test import Test
>> -from lib.utils import find_port
>> +from lib.utils import find_port, extract_schema_from_snapshot
>>   from lib.utils import format_process
>>   from lib.utils import signame
>>   from lib.utils import warn_unix_socket
> Nit: Most of imports are function-per-line, I would keep it this way. It
> looks more accurate.
Fixed in a whole patch.
>> @@ -609,7 +609,6 @@ def __init__(self, _ini=None, test_suite=None):
>>           }
>>           ini.update(_ini)
>>           Server.__init__(self, ini, test_suite)
>> -        self.testdir = os.path.abspath(os.curdir)
>>           self.sourcedir = os.path.abspath(os.path.join(os.path.basename(
>>               sys.argv[0]), "..", ".."))
>>           self.name = "default"
> I see that it is moved to the parent class, but:
>
> 1. It does not look related to your changes, but if it is related, you
>     should describe a reason in the commit message.
> 2. If it is a side refactoring, you should move it to its own commit
>     with explained motivation.
>
> For now, I don't see any reason for this change.

Reverted change back.

>> @@ -641,6 +640,10 @@ def __init__(self, _ini=None, test_suite=None):
>>           if 'test_run_current_test' in caller_globals.keys():
>>               self.current_test = caller_globals['test_run_current_test']
>>   
>> +        if self.disable_schema_upgrade and not self.test_debug():
>> +            color_stdout("Can't disable schema upgrade on release build\n",
>> +                         schema='error')
>> +
> Should not we verify it on the upper level, because it is like options
> validation? Say, right in options.py or at least in module_init()?
>
> Should not we give an error and don't start testing in the case?
arguments validation code moved to __init__.py and options.py.
>
> BTW, there is `debug` property, which calls test_debug().
>
>> @@ -861,6 +873,18 @@ def start(self, silent=True, wait=True, wait_load=True, rais=True, args=[],
>>           self.admin = CON_SWITCH[self.tests_type]('localhost', port)
>>           self.status = 'started'
>>   
>> +        if not self.ctl_path:
>> +            self.ctl_path = self.find_exe(self.builddir)
> 1. Are there situations, when there is no `ctl_path` value at starting a
>     server? So `prepare_args()` call above is incorrect? I don't get the
>     trick. I guess `lib/__init__.py` call to `find_exe()` should be
>     enough to fill `cls.ctl_path`.
> 2. `find_exe()` sets `cls.ctl_path` on its own.

There is a similar condition in method print_exe():

     @classmethod
     def print_exe(cls):
         color_stdout('Installing the server ...\n', schema='serv_text')

         if cls.binary:
             color_stdout('    Found executable   at ', schema='serv_text')
             color_stdout(cls.binary + '\n', schema='path')

         if cls.ctl_path:
             color_stdout('    Found tarantoolctl at ', schema='serv_text')
             color_stdout(cls.ctl_path + '\n', schema='path')

So I kept change as is.

>
>> +        if self.disable_schema_upgrade:
>> +            version = extract_schema_from_snapshot(self.ctl_path,
>> +                                                   self.builddir, self.snapshot_path)
>> +            current_ver = yaml.safe_load(self.admin.execute(
>> +                'box.space._schema:get{"version"}'))
>> +            if version == current_ver:
>> +                raise_msg = 'Versions are inequal ({} vs {})'.format(
>> +                    version, current_ver)
>> +                raise Exception(raise_msg)
>> +
> AFAIS, the usual recommendation is to raise standard or user-defined
> Exception subclasses. It seems, RuntimeError fits best here.
>
> However, I guess, RuntimeError and TarantoolStartError will lead to
> different behaviour. The former just fails the test, but the latter
> stops testing at whole. Should not we stop testing in the case?
>
> Will the new server left running in the case? It is undesirable
> situation.

all checks for arguments moved to options.py and Exceptions replaced 
with "exit(1)".

>
>> @@ -1066,7 +1090,6 @@ def test_option_get(self, option_list_str, silent=False):
>>           if not silent:
>>               print " ".join([os.path.basename(self.binary)] + args[1:])
>>           output = subprocess.Popen(args,
>> -                                  cwd=self.vardir,
>>                                     stdout=subprocess.PIPE,
>>                                     stderr=subprocess.STDOUT).stdout.read()
>>           return output
> 1. It looks unrelated.
> 2. There is not any described reason for this change.
Reverted change back.
>
>> @@ -1126,7 +1149,7 @@ def is_correct(run_name):
>>       def get_param(self, param=None):
>>           if param is not None:
>>               return yaml.safe_load(self.admin("box.info." + param,
>> -                                  silent=True))[0]
>> +                                             silent=True))[0]
>>           return yaml.safe_load(self.admin("box.info", silent=True))
> Unrelated change.
Reverted change back.

Re: [Tarantool-patches] [test-run] Add options for upgrade testing

[Sergey Bronnikov]

On 18.11.2020 12:18, Sergey Bronnikov wrote:
> Hi, Alexander!
>
> thanks for review!
>
> I have updated the whole patch and I'll send it as reply to this mail.

Updated patch - 
https://lists.tarantool.org/pipermail/tarantool-patches/2020-November/020783.html 

[Tarantool-patches] [PATCH] Add options for upgrade testing

[sergeyb]

From: Sergey Bronnikov <sergeyb@tarantool.org>

Option '--snapshot' specifies a path to snapshot that will be loaded in
Tarantool before testing.

Option '--disable-schema-upgrade' allows to skip execution
of Tarantool upgrade script using error injection mechanism.

Closes https://github.com/tarantool/tarantool/issues/4801
---
 lib/__init__.py         |  4 ++++
 lib/app_server.py       | 15 +++++++++++++-
 lib/options.py          | 31 +++++++++++++++++++++++++++++
 lib/server.py           |  9 +++++++++
 lib/tarantool_server.py | 44 ++++++++++++++++++++++++++++++++++++-----
 lib/utils.py            | 37 ++++++++++++++++++++++++++++++++++
 6 files changed, 134 insertions(+), 6 deletions(-)

diff --git a/lib/__init__.py b/lib/__init__.py
index 398439d..a3d1597 100644
--- a/lib/__init__.py
+++ b/lib/__init__.py
@@ -6,6 +6,7 @@ from lib.options import Options
 from lib.tarantool_server import TarantoolServer
 from lib.unittest_server import UnittestServer
 from utils import warn_unix_sockets_at_start
+from utils import test_debug
 
 
 __all__ = ['Options']
@@ -56,6 +57,9 @@ def module_init():
     TarantoolServer.find_exe(args.builddir)
     UnittestServer.find_exe(args.builddir)
 
+    is_debug = test_debug(TarantoolServer().version())
+    Options().check_schema_upgrade_option(is_debug)
+
 
 # Init
 ######
diff --git a/lib/app_server.py b/lib/app_server.py
index 2cb8a87..706ae9c 100644
--- a/lib/app_server.py
+++ b/lib/app_server.py
@@ -9,11 +9,13 @@ from gevent.subprocess import Popen, PIPE
 from lib.colorer import color_log
 from lib.preprocessor import TestState
 from lib.server import Server
+from lib.server import DEFAULT_SNAPSHOT_NAME
 from lib.tarantool_server import Test
 from lib.tarantool_server import TarantoolServer
 from lib.tarantool_server import TarantoolStartError
 from lib.utils import find_port
 from lib.utils import format_process
+from lib.utils import safe_makedirs
 from lib.utils import warn_unix_socket
 from test import TestRunGreenlet, TestExecutionError
 
@@ -86,7 +88,12 @@ class AppServer(Server):
         return os.path.join(self.vardir, file_name)
 
     def prepare_args(self, args=[]):
-        return [os.path.join(os.getcwd(), self.current_test.name)] + args
+        cli_args = [os.path.join(os.getcwd(), self.current_test.name)] + args
+        if self.disable_schema_upgrade:
+            cli_args = [self.binary, '-e',
+                        self.DISABLE_AUTO_UPGRADE] + cli_args
+
+        return cli_args
 
     def deploy(self, vardir=None, silent=True, need_init=True):
         self.vardir = vardir
@@ -119,6 +126,12 @@ class AppServer(Server):
         # cannot check length of path of *.control unix socket created by it.
         # So for 'app' tests type we don't check *.control unix sockets paths.
 
+        if self.snapshot_path:
+            snapshot_dest = os.path.join(self.vardir, DEFAULT_SNAPSHOT_NAME)
+            color_log("Copying snapshot {} to {}\n".format(
+                self.snapshot_path, snapshot_dest))
+            shutil.copy(self.snapshot_path, snapshot_dest)
+
     def stop(self, silent):
         if not self.process:
             return
diff --git a/lib/options.py b/lib/options.py
index 47bbc0f..9dcb70e 100644
--- a/lib/options.py
+++ b/lib/options.py
@@ -209,6 +209,19 @@ class Options:
                 help="""Update or create file with reference output (.result).
                 Default: false.""")
 
+        parser.add_argument(
+                "--snapshot",
+                dest='snapshot_path',
+                help="""Path to a Tarantool snapshot that will be
+                loaded before testing.""")
+
+        parser.add_argument(
+                "--disable-schema-upgrade",
+                dest='disable_schema_upgrade',
+                action="store_true",
+                default=False,
+                help="""Disable schema upgrade on testing with snapshots.""")
+
         # XXX: We can use parser.parse_intermixed_args() on
         # Python 3.7 to understand commands like
         # ./test-run.py foo --exclude bar baz
@@ -227,5 +240,23 @@ class Options:
                 color_stdout(format_str.format(op1, op2), schema='error')
                 check_error = True
 
+        snapshot_path = getattr(self.args, 'snapshot_path', '')
+        if self.args.disable_schema_upgrade and not snapshot_path:
+            color_stdout("\nOption --disable-schema-upgrade requires --snapshot\n",
+                        schema='error')
+            check_error = True
+
+        if snapshot_path:
+            if not os.path.exists(snapshot_path):
+                color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')
+                check_error = True
+            else:
+                self.args.snapshot_path = os.path.abspath(snapshot_path)
+
         if check_error:
             exit(-1)
+
+    def check_schema_upgrade_option(self, is_debug):
+        if self.args.disable_schema_upgrade and not is_debug:
+            color_stdout("Can't disable schema upgrade on release build\n", schema='error')
+            exit(-1)
diff --git a/lib/server.py b/lib/server.py
index 321eac7..f0fb03a 100644
--- a/lib/server.py
+++ b/lib/server.py
@@ -9,12 +9,14 @@ from lib.server_mixins import LLdbMixin
 from lib.server_mixins import StraceMixin
 from lib.server_mixins import LuacovMixin
 from lib.colorer import color_stdout
+from lib.options import Options
 from lib.utils import print_tail_n
 
 
 DEFAULT_CHECKPOINT_PATTERNS = ["*.snap", "*.xlog", "*.vylog", "*.inprogress",
                                "[0-9]*/"]
 
+DEFAULT_SNAPSHOT_NAME = "00000000000000000000.snap"
 
 class Server(object):
     """Server represents a single server instance. Normally, the
@@ -24,6 +26,10 @@ class Server(object):
     DEFAULT_INSPECTOR = 0
     TEST_RUN_DIR = os.path.abspath(os.path.join(os.path.dirname(__file__),
                                                 ".."))
+    # assert(false) hangs due to gh-4983, added fiber.sleep(0) to workaround it
+    DISABLE_AUTO_UPGRADE = "require('fiber').sleep(0) \
+        assert(box.error.injection.set('ERRINJ_AUTO_UPGRADE', true) == 'ok', \
+        'no such errinj')"
 
     @property
     def vardir(self):
@@ -84,6 +90,9 @@ class Server(object):
         self.inspector_port = int(ini.get(
             'inspector_port', self.DEFAULT_INSPECTOR
         ))
+        self.testdir = os.path.abspath(os.curdir)
+        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
+        self.snapshot_path = Options().args.snapshot_path
 
         # filled in {Test,AppTest,LuaTest,PythonTest}.execute()
         # or passed through execfile() for PythonTest (see
diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index fd102b9..17b6f44 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -26,12 +26,16 @@ from lib.box_connection import BoxConnection
 from lib.colorer import color_stdout, color_log
 from lib.preprocessor import TestState
 from lib.server import Server
+from lib.server import DEFAULT_SNAPSHOT_NAME
 from lib.test import Test
 from lib.utils import find_port
+from lib.utils import extract_schema_from_snapshot
 from lib.utils import format_process
+from lib.utils import safe_makedirs
 from lib.utils import signame
 from lib.utils import warn_unix_socket
 from lib.utils import prefix_each_line
+from lib.utils import test_debug
 from test import TestRunGreenlet, TestExecutionError
 
 
@@ -609,7 +613,6 @@ class TarantoolServer(Server):
         }
         ini.update(_ini)
         Server.__init__(self, ini, test_suite)
-        self.testdir = os.path.abspath(os.curdir)
         self.sourcedir = os.path.abspath(os.path.join(os.path.basename(
             sys.argv[0]), "..", ".."))
         self.name = "default"
@@ -775,8 +778,29 @@ class TarantoolServer(Server):
             shutil.copy(os.path.join(self.TEST_RUN_DIR, 'pretest_clean.lua'),
                         self.vardir)
 
+        if self.snapshot_path:
+            # Copy snapshot to the workdir.
+            # Usually Tarantool looking for snapshots on start in a current directory
+            # or in a directories that specified in memtx_dir or vinyl_dir box settings.
+            # Before running test current directory (workdir) passed to a new instance in
+            # an environment variable TEST_WORKDIR and then .tarantoolctl.in
+            # adds to it instance_name and set to memtx_dir and vinyl_dir.
+            (instance_name, _) = os.path.splitext(os.path.basename(self.script))
+            instance_dir = os.path.join(self.vardir, instance_name)
+            safe_makedirs(instance_dir)
+            snapshot_dest = os.path.join(instance_dir, DEFAULT_SNAPSHOT_NAME)
+            color_log("Copying snapshot {} to {}\n".format(
+                self.snapshot_path, snapshot_dest))
+            shutil.copy(self.snapshot_path, snapshot_dest)
+
     def prepare_args(self, args=[]):
-        return [self.ctl_path, 'start', os.path.basename(self.script)] + args
+        cli_args = [self.ctl_path, 'start',
+                    os.path.basename(self.script)] + args
+        if self.disable_schema_upgrade:
+            cli_args = [self.binary, '-e',
+                        self.DISABLE_AUTO_UPGRADE] + cli_args
+
+        return cli_args
 
     def pretest_clean(self):
         # Don't delete snap and logs for 'default' tarantool server
@@ -861,6 +885,18 @@ class TarantoolServer(Server):
         self.admin = CON_SWITCH[self.tests_type]('localhost', port)
         self.status = 'started'
 
+        if not self.ctl_path:
+            self.ctl_path = self.find_exe(self.builddir)
+        if self.disable_schema_upgrade:
+            version = extract_schema_from_snapshot(self.ctl_path,
+                                                   self.builddir, self.snapshot_path)
+            current_ver = yaml.safe_load(self.admin.execute(
+                'box.space._schema:get{"version"}'))
+            if version == current_ver:
+                raise_msg = 'Versions are inequal ({} vs {})'.format(
+                    version, current_ver)
+                raise Exception(raise_msg)
+
     def crash_detect(self):
         if self.crash_expected:
             return
@@ -1075,9 +1111,7 @@ class TarantoolServer(Server):
         print self.test_option_get(option_list_str)
 
     def test_debug(self):
-        if re.findall(r"-Debug", self.test_option_get("-V", True), re.I):
-            return True
-        return False
+        return test_debug(self.test_option_get("-V", True))
 
     @staticmethod
     def find_tests(test_suite, suite_path):
diff --git a/lib/utils.py b/lib/utils.py
index cc2f6b7..2aeedc5 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -1,10 +1,13 @@
 import os
+import re
 import sys
 import six
 import collections
 import signal
+import subprocess
 import random
 import fcntl
+import json
 import difflib
 import time
 from gevent import socket
@@ -264,3 +267,37 @@ def just_and_trim(src, width):
     if len(src) > width:
         return src[:width - 1] + '>'
     return src.ljust(width)
+
+
+def extract_schema_from_snapshot(ctl_path, builddir, snapshot_path):
+    """
+    Extract schema version from snapshot, example of record:
+     {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
+       "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
+    """
+    SCHEMA_SPACE_ID = 272
+
+    ctl_args = [ctl_path, 'cat', snapshot_path,
+                '--format=json', '--show-system']
+    proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE)
+    version = None
+    while True:
+        line = proc.stdout.readline()
+        if not line:
+            break
+        json_line = json.loads(line.strip())
+        query_type = json_line["HEADER"]["type"]
+        space_id = json_line["BODY"]["space_id"]
+        if query_type == "INSERT" or query_type == "REPLACE" and space_id == SCHEMA_SPACE_ID:
+            query_tuple = json_line['BODY']['tuple']
+            if query_tuple[0] == 'version':
+                version = query_tuple
+                break
+
+    return version
+
+
+def test_debug(version):
+    if re.findall(r"-Debug", version, re.I):
+        return True
+    return False
-- 
2.25.1

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Alexander Turenko]

Not a review.

On Wed, Nov 18, 2020 at 12:30:49PM +0300, sergeyb@tarantool.org wrote:
> From: Sergey Bronnikov <sergeyb@tarantool.org>
> 
> Option '--snapshot' specifies a path to snapshot that will be loaded in
> Tarantool before testing.
> 
> Option '--disable-schema-upgrade' allows to skip execution
> of Tarantool upgrade script using error injection mechanism.
> 
> Closes https://github.com/tarantool/tarantool/issues/4801

This change cannot close the issue in tarantool. We need to look at all
variants of bootstrap snapshot we had over the history of supported
release branches (and maybe a bit deeper, down to some 1.9) and manifest
compatibility matrix between given data layouts and runtimes ([1]). Once
the support matrix will be defined, we should catch all test fails,
analyze and categorize them. File issue for real problems, exclude
irrelevant fails. Setup CI. It is not a small task.

[1]: https://github.com/tarantool/doc/issues/836

I think we should file an issue to test-run, solve it within our
planning iteration and estimate #4801 for the next iteration.

While I thinking around the task, the idea how to solve the problem with
replication tests comes to me. I know, know. I should have to say it
before you start the task, but I don't know everything in advance,
learning together with you and do my best.

Back to the idea. We can implement an option or errinj to pass a path to
bootstrap snapshot to tarantool and feed it instead the built-in
bootstrap.snap. This way we'll go to the right branch here:

 [src/box/box.cc; box_cfg_xc()]

 | if (checkpoint != NULL) {
 |         /* Recover the instance from the local directory */                            
 |         local_recovery(&instance_uuid, &replicaset_uuid,                               
 |                        &checkpoint->vclock);                                           
 | } else {
 |         /* Bootstrap a new instance */
 |         bootstrap(&instance_uuid, &replicaset_uuid,                                    
 |                   &is_bootstrap_leader);                                               
 | }

And so will land here:

 [src/box/memtx_engine.c; memtx_engine_bootstrap()]

 | /* Recover from bootstrap.snap */
 | say_info("initializing an empty data directory");
 | struct xlog_cursor cursor;
 | if (xlog_cursor_openmem(&cursor, (const char *)bootstrap_bin,
 |                         sizeof(bootstrap_bin), "bootstrap") < 0)
 |         return -1;

It is the place, where we should give our custom snapshot instead of
`bootstrap_bin` (which contains src/box/bootstrap.snap content).

I would call the test-run option that feeds a snapshot to tarantool this
way --bootstrap instead of --snapshot to don't confuse the local
recovery process with bootstrapping from an empty directory.

Let's discuss tasks, planning, worthiness of eating a pie step-by-step
voicely.

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Sergey Bronnikov]

Hi, Alexander!

On 20.11.2020 01:58, Alexander Turenko wrote:
> Not a review.
>
> On Wed, Nov 18, 2020 at 12:30:49PM +0300, sergeyb@tarantool.org wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Option '--snapshot' specifies a path to snapshot that will be loaded in
>> Tarantool before testing.
>>
>> Option '--disable-schema-upgrade' allows to skip execution
>> of Tarantool upgrade script using error injection mechanism.
>>
>> Closes https://github.com/tarantool/tarantool/issues/4801
> This change cannot close the issue in tarantool. We need to look at all
> variants of bootstrap snapshot we had over the history of supported
> release branches (and maybe a bit deeper, down to some 1.9) and manifest
> compatibility matrix between given data layouts and runtimes ([1]). Once
> the support matrix will be defined, we should catch all test fails,
> analyze and categorize them. File issue for real problems, exclude
> irrelevant fails. Setup CI. It is not a small task.

You are right. Earlier we discussed that with you and new ticket has 
been submitted [1].

1. https://github.com/tarantool/tarantool/issues/5527

>
> [1]: https://github.com/tarantool/doc/issues/836
>
> I think we should file an issue to test-run, solve it within our
> planning iteration and estimate #4801 for the next iteration.
>
> While I thinking around the task, the idea how to solve the problem with
> replication tests comes to me. I know, know. I should have to say it
> before you start the task, but I don't know everything in advance,
> learning together with you and do my best.

Discussed it verbally and submitted two tickets:

- https://github.com/tarantool/tarantool/issues/5540

- https://github.com/tarantool/test-run/issues/239

>
> Back to the idea. We can implement an option or errinj to pass a path to
> bootstrap snapshot to tarantool and feed it instead the built-in
> bootstrap.snap. This way we'll go to the right branch here:
>
>   [src/box/box.cc; box_cfg_xc()]
>
>   | if (checkpoint != NULL) {
>   |         /* Recover the instance from the local directory */
>   |         local_recovery(&instance_uuid, &replicaset_uuid,
>   |                        &checkpoint->vclock);
>   | } else {
>   |         /* Bootstrap a new instance */
>   |         bootstrap(&instance_uuid, &replicaset_uuid,
>   |                   &is_bootstrap_leader);
>   | }
>
> And so will land here:
>
>   [src/box/memtx_engine.c; memtx_engine_bootstrap()]
>
>   | /* Recover from bootstrap.snap */
>   | say_info("initializing an empty data directory");
>   | struct xlog_cursor cursor;
>   | if (xlog_cursor_openmem(&cursor, (const char *)bootstrap_bin,
>   |                         sizeof(bootstrap_bin), "bootstrap") < 0)
>   |         return -1;
>
> It is the place, where we should give our custom snapshot instead of
> `bootstrap_bin` (which contains src/box/bootstrap.snap content).
>
> I would call the test-run option that feeds a snapshot to tarantool this
> way --bootstrap instead of --snapshot to don't confuse the local
> recovery process with bootstrapping from an empty directory.

Updated in a branch:

--- a/lib/options.py
+++ b/lib/options.py
@@ -210,17 +210,17 @@ class Options:
                  Default: false.""")

          parser.add_argument(
-                "--snapshot",
+                "--bootstrap",
                  dest='snapshot_path',
-                help="""Path to a Tarantool snapshot that will be
-                loaded before testing.""")
+                help="""Path to snapshot that will be used to boostrap
+                Tarantool before testing.""")

          parser.add_argument(
                  "--disable-schema-upgrade",
                  dest='disable_schema_upgrade',
                  action="store_true",
                  default=False,
-                help="""Disable schema upgrade on testing with 
snapshots.""")
+                help="""Disable schema upgrade on testing with 
bootstrap snapshots.""")

          # XXX: We can use parser.parse_intermixed_args() on
          # Python 3.7 to understand commands like
@@ -242,7 +242,7 @@ class Options:

          snapshot_path = getattr(self.args, 'snapshot_path', '')
          if self.args.disable_schema_upgrade and not snapshot_path:
-            color_stdout("\nOption --disable-schema-upgrade requires 
--snapshot\n",
+            color_stdout("\nOption --disable-schema-upgrade requires 
--boostrap\n",
                           schema='error')
              check_error = True

>
> Let's discuss tasks, planning, worthiness of eating a pie step-by-step
> voicely.

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Alexander Turenko]

Sorry for the long delay.

I looked over the patch, tested it a bit and proposed several fixups.

I pasted them here (for ease commenting) and pushed to the branch
(upward your commits). If you're okay with them, I can squash commits
and push the result. Or, maybe, extract several commits (re debug
property, re xlog module) and squash the rest (keeping the conflicting
options fix separate, of course).

The branch: ligurio/gh-4801-add-snapshot-option.

And sorry that I reworked a lot of code. I tried to minimize review
ping-ponging, but make myself comfortable with the result. And amount of
changes becomes larger than I initially expect. A lot of low hanging
fruits of the kind 'okay, but there is the easy way to make it a bit
better' appears.

I hope such moments will be less and less frequent with a time we'll
work in touch. A review is often about compromises and it is the work on
ourself for both sides.

WBR, Alexander Turenko.

----

Catched the patch version from the ligurio/gh-4801-add-snapshot-option
branch (8300e499b888c0d66daa941c956462767274b996 with conflicting option
fix 12b791c897cbf84da28326ce3af8a061703c77e3 upward).

Verified for a 'core = app' test. Server.cleanup() removes the snapshot
before it'll be read by tarantool. Fixed:

 | commit 04ffc8d6ca21f00c130e33e9db7947fb1b96b783
 | Author: Alexander Turenko <alexander.turenko@tarantool.org>
 | Date:   Wed Nov 25 02:25:08 2020 +0300
 | 
 |     FIXUP: Add options for upgrade testing
 |     
 |     The --bootstrap option don't work on 'core = app' tests before this
 |     fixup, because pretest_clean() removes all snapshots in a working
 |     directory. pretest_clean() is called before each test, while deploy() is
 |     called once a testing begins. We should copy the snapshot right before
 |     tarantool startup.
 | 
 | diff --git a/lib/app_server.py b/lib/app_server.py
 | index c32c5cf..a046b21 100644
 | --- a/lib/app_server.py
 | +++ b/lib/app_server.py
 | @@ -44,6 +44,14 @@ class AppTest(Test):
 |                                      server.logfile, retval)
 |          self.current_test_greenlet = tarantool
 |  
 | +        # Copy the snapshot right before starting the server.
 | +        # Otherwise pretest_clean() would remove it.
 | +        if server.snapshot_path:
 | +            snapshot_dest = os.path.join(server.vardir, DEFAULT_SNAPSHOT_NAME)
 | +            color_log("Copying snapshot {} to {}\n".format(
 | +                server.snapshot_path, snapshot_dest))
 | +            shutil.copy(server.snapshot_path, snapshot_dest)
 | +
 |          try:
 |              tarantool.start()
 |              tarantool.join()
 | @@ -125,12 +133,6 @@ class AppServer(Server):
 |          # cannot check length of path of *.control unix socket created by it.
 |          # So for 'app' tests type we don't check *.control unix sockets paths.
 |  
 | -        if self.snapshot_path:
 | -            snapshot_dest = os.path.join(self.vardir, DEFAULT_SNAPSHOT_NAME)
 | -            color_log("Copying snapshot {} to {}\n".format(
 | -                self.snapshot_path, snapshot_dest))
 | -            shutil.copy(self.snapshot_path, snapshot_dest)
 | -
 |      def stop(self, silent):
 |          if not self.process:
 |              return

Verified for a 'core = tarantool' test: it works. Observed that 'Processing
file <filename>...' lines are printed to the terminal and fixed it:

 | commit 7f25709a84af1cf862037e0e2bf99318836b05d0
 | Author: Alexander Turenko <alexander.turenko@tarantool.org>
 | Date:   Wed Nov 25 20:14:31 2020 +0300
 | 
 |     FIXUP: Add options for upgrade testing
 |     
 |     Explicitly discard `tarantoolctl cat` stderr output. Otherwise test-run
 |     prints 'Processing file <filename>...' lines to the terminal before
 |     starting a server for 'core = tarantool' test (or when 'start server'
 |     command is used inside a test) if --disable-schema-upgrade option is
 |     passed.
 | 
 | diff --git a/lib/utils.py b/lib/utils.py
 | index ef7d8a8..bdf920e 100644
 | --- a/lib/utils.py
 | +++ b/lib/utils.py
 | @@ -281,7 +281,8 @@ def extract_schema_from_snapshot(ctl_path, builddir, snapshot_path):
 |  
 |      ctl_args = [ctl_path, 'cat', snapshot_path,
 |                  '--format=json', '--show-system']
 | -    proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE)
 | +    with open(os.devnull, 'w') as devnull:
 | +        proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE, stderr=devnull)
 |      version = None
 |      while True:
 |          line = proc.stdout.readline()

(I unable to use subprocess.DEVNULL, because it is supported since
Python 3.3.)

The last thing I want to verify is whether we'll able to reimplement the
option in the way that is described in #5540 ([1]) without changing
anything on the tarantool side (including snapshots we pass). In other
words, whether there is any sense in our agreement about calling the
option as --bootstrap in order to reimplement it later without breaking
backward compatibility.

[1]: https://github.com/tarantool/tarantool/issues/5540

I tried to pass a bootstrap snapshot (src/box/bootstrap.snap from the
latest 1.10) and got the following errors:

 | [001] 2020-11-25 02:24:08.839 [22154] main/103/merger.test.lua box.cc:2747 E> ER_UNKNOWN_REPLICA: Replica 1e00d635-4bea-4eb8-86d8-52807fcd0adb is not registered with replica set 00000000-0000-0000-0000-000000000000
 | [001] 2020-11-25 02:24:08.839 [22154] main/103/merger.test.lua F> can't initialize storage: Replica 1e00d635-4bea-4eb8-86d8-52807fcd0adb is not registered with replica set 00000000-0000-0000-0000-000000000000

I investigated the differences between a bootstrap snapshot and the
snapshot that is written to the file system after a first box.cfg() and
created the utility function to transform the former to the latter.

I'll place four patches with support of this approach at the of the
email and on the branch.

See the rest of comments and proposed patches are inline.

> diff --git a/lib/__init__.py b/lib/__init__.py
> index 398439d..a3d1597 100644
> --- a/lib/__init__.py
> +++ b/lib/__init__.py
> @@ -6,6 +6,7 @@ from lib.options import Options
>  from lib.tarantool_server import TarantoolServer
>  from lib.unittest_server import UnittestServer
>  from utils import warn_unix_sockets_at_start
> +from utils import test_debug
>  
>  
>  __all__ = ['Options']
> @@ -56,6 +57,9 @@ def module_init():
>      TarantoolServer.find_exe(args.builddir)
>      UnittestServer.find_exe(args.builddir)
>  
> +    is_debug = test_debug(TarantoolServer().version())
> +    Options().check_schema_upgrade_option(is_debug)
> +

There is no need to create an instance of TarantoolServer to call a
class method. In fact, there is no need to move the `test_debug()`
method implementation to utils and, moreover, there is no need to keep
this method at all. We can just use the 'version()' class method at
appropriate place and save the result within a class field. It still
will be available as the field of an instance.

And, to be honest, the name 'test_debug()' is dubious. It may mean 'to
test a debug build' or 'to test whether a build is debug'. Too common,
IMHO.

BTW, while we're here I would note that it would be good to fix the
problem with the 'release_disable' suite.ini option in 'core = app' and
'core = unittest' test suites:
https://github.com/tarantool/test-run/issues/199

I propose the following change:

 | commit d4cc26096643f41888e7ed004392d27bce60ee78
 | Author: Alexander Turenko <alexander.turenko@tarantool.org>
 | Date:   Thu Nov 26 22:56:06 2020 +0300
 | 
 |     FIXUP: Add options for upgrade testing
 |     
 |     Move TarantoolServer's property 'debug' to a class level instead of
 |     instance level.
 |     
 |     Drop unnecessary test_debug() function (it is not used anywhere in
 |     tests, but was used to implement the property). Drop the same named
 |     utils function, because it is unnecessary too now.
 |     
 |     Eliminate unnecesary instantiation of TarantoolServer at options
 |     verification in lib/__init__.py. Use the 'debug' property directly
 |     instead of the 'version()' class method.
 |     
 |     The purpose of changes is simplification of the code.
 | 
 | diff --git a/lib/__init__.py b/lib/__init__.py
 | index a3d1597..767f0d6 100644
 | --- a/lib/__init__.py
 | +++ b/lib/__init__.py
 | @@ -6,7 +6,6 @@ from lib.options import Options
 |  from lib.tarantool_server import TarantoolServer
 |  from lib.unittest_server import UnittestServer
 |  from utils import warn_unix_sockets_at_start
 | -from utils import test_debug
 |  
 |  
 |  __all__ = ['Options']
 | @@ -57,8 +56,7 @@ def module_init():
 |      TarantoolServer.find_exe(args.builddir)
 |      UnittestServer.find_exe(args.builddir)
 |  
 | -    is_debug = test_debug(TarantoolServer().version())
 | -    Options().check_schema_upgrade_option(is_debug)
 | +    Options().check_schema_upgrade_option(TarantoolServer.debug)
 |  
 |  
 |  # Init
 | diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
 | index 8de1a30..4121a4d 100644
 | --- a/lib/tarantool_server.py
 | +++ b/lib/tarantool_server.py
 | @@ -35,7 +35,6 @@ from lib.utils import safe_makedirs
 |  from lib.utils import signame
 |  from lib.utils import warn_unix_socket
 |  from lib.utils import prefix_each_line
 | -from lib.utils import test_debug
 |  from test import TestRunGreenlet, TestExecutionError
 |  
 |  
 | @@ -461,10 +460,6 @@ class TarantoolServer(Server):
 |      }
 |  
 |      # ----------------------------PROPERTIES--------------------------------- #
 | -    @property
 | -    def debug(self):
 | -        return self.test_debug()
 | -
 |      @property
 |      def name(self):
 |          if not hasattr(self, '_name') or not self._name:
 | @@ -692,7 +687,10 @@ class TarantoolServer(Server):
 |                          ctl_dir + '/?.lua;' + \
 |                          ctl_dir + '/?/init.lua;' + \
 |                          os.environ.get("LUA_PATH", ";;")
 | +                cls.debug = bool(re.findall(r'-Debug', str(cls.version()),
 | +                                 re.I))
 |                  return exe
 | +
 |          raise RuntimeError("Can't find server executable in " + path)
 |  
 |      @classmethod
 | @@ -1112,9 +1110,6 @@ class TarantoolServer(Server):
 |      def test_option(self, option_list_str):
 |          print self.test_option_get(option_list_str)
 |  
 | -    def test_debug(self):
 | -        return test_debug(self.test_option_get("-V", True))
 | -
 |      @staticmethod
 |      def find_tests(test_suite, suite_path):
 |          test_suite.ini['suite'] = suite_path
 | diff --git a/lib/utils.py b/lib/utils.py
 | index bdf920e..e0bd394 100644
 | --- a/lib/utils.py
 | +++ b/lib/utils.py
 | @@ -1,5 +1,4 @@
 |  import os
 | -import re
 |  import sys
 |  import six
 |  import collections
 | @@ -298,9 +297,3 @@ def extract_schema_from_snapshot(ctl_path, builddir, snapshot_path):
 |                  break
 |  
 |      return version
 | -
 | -
 | -def test_debug(version):
 | -    if re.findall(r"-Debug", version, re.I):
 | -        return True
 | -    return False

> diff --git a/lib/options.py b/lib/options.py
> index 47bbc0f..9dcb70e 100644
> --- a/lib/options.py
> +++ b/lib/options.py
> @@ -209,6 +209,19 @@ class Options:
>                  help="""Update or create file with reference output (.result).
>                  Default: false.""")
>  
> +        parser.add_argument(
> +                "--snapshot",
> +                dest='snapshot_path',
> +                help="""Path to a Tarantool snapshot that will be
> +                loaded before testing.""")

(The cited patch is a bit old, now it is --bootstrap.)

Typo: boostrap -> bootstrap. Fixed in a fixup commit.

Omitting of the value is the same as passing `default=None`, but the
latter is more obvious. See, you use getattr(), because your guess was
that it works as `default=argparse.SUPPRESS` (see docs, [1]). And using
the explicit default would be more consistent with other arguments. If
omitting of `default` would work like argparse.SUPRESS, the code, which
uses `Options().args.snapshot_path` in server.py, would fail when the
argument would not be passed.

[1]: https://docs.python.org/2/library/argparse.html#default

I would be against of using `default=argparse.SUPPRESS`: this way we
would be obligated to use getattr() (and remember to use it) every time
we need to obtain the option. But there is no reason to introduce the
extra complexity.

I propose the following change:

 | commit 637e3f309b5c43b031f66440f12fabf02b1d26dc
 | Author: Alexander Turenko <alexander.turenko@tarantool.org>
 | Date:   Thu Nov 26 23:45:00 2020 +0300
 | 
 |     FIXUP: Add options for upgrade testing
 |     
 |     Explicitly set default=None for the new option. It does not change
 |     anything, but is consistent with other options and more obvious for a
 |     reader.
 |     
 |     Removed unnecessary getattr().
 | 
 | diff --git a/lib/options.py b/lib/options.py
 | index fc905cb..0476a36 100644
 | --- a/lib/options.py
 | +++ b/lib/options.py
 | @@ -212,6 +212,7 @@ class Options:
 |          parser.add_argument(
 |                  "--bootstrap",
 |                  dest='snapshot_path',
 | +                default=None,
 |                  help="""Path to snapshot that will be used to bootstrap
 |                  Tarantool before testing.""")
 |  
 | @@ -240,7 +241,7 @@ class Options:
 |                  check_error = True
 |                  break
 |  
 | -        snapshot_path = getattr(self.args, 'snapshot_path', '')
 | +        snapshot_path = self.args.snapshot_path
 |          if self.args.disable_schema_upgrade and not snapshot_path:
 |              color_stdout("\nOption --disable-schema-upgrade requires --bootstrap\n",
 |                           schema='error')

By the way, the option is named '--bootstrap', but the destination is
'snapshot_path'. The thought whether it may be confusing spins in my
mind, but I don't know. I left it unchanged.

> @@ -227,5 +240,23 @@ class Options:
>                  color_stdout(format_str.format(op1, op2), schema='error')
>                  check_error = True
>  
> +        snapshot_path = getattr(self.args, 'snapshot_path', '')
> +        if self.args.disable_schema_upgrade and not snapshot_path:
> +            color_stdout("\nOption --disable-schema-upgrade requires --snapshot\n",
> +                        schema='error')
> +            check_error = True

Typo: --boostrap -> --bootstrap. Fixed in a fixup commit.

> +
> +        if snapshot_path:
> +            if not os.path.exists(snapshot_path):
> +                color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')
> +                check_error = True
> +            else:
> +                self.args.snapshot_path = os.path.abspath(snapshot_path)
> +

I like the idea to normalize the path before test-run calls chdir(). I
think other path options should do the same: I didn't try, but they may
be broken for the case, when test-run is called outside of the test/
directory.

But I found it being a bit counter-intuitive to normalize an option in a
'check()' method. If I look over the options, I may end up with the
guess that the destination variable contains a relative path. There are
other ways to normalize:

1. A custom 'action' add_argument() parameter.
2. A custom 'type' add_argument() parameter.
3. A separate normalize() Options method.

The first looks most intuitive on the first glance, but it requires to
define an Action class, not just a function. In fact, the second choice
is the simplest one. I propose to use it:

 | commit cda60788573dc4396c6bd0e6bf196593cb3d1485
 | Author: Alexander Turenko <alexander.turenko@tarantool.org>
 | Date:   Fri Nov 27 00:59:44 2020 +0300
 | 
 |     FIXUP: Add options for upgrade testing
 |     
 |     Move normalization of the --bootstrap option value into the option
 |     declaration block.
 |     
 |     The intention is to simplify the code.
 |     
 |     Corner case: `./test-run.py --bootstrap ../00000000000000000000.snap/`
 |     will be accepted now as if would we don't place the slash at the end. I
 |     would ignore the case for the sake of code simplicity.
 | 
 | diff --git a/lib/options.py b/lib/options.py
 | index 0476a36..c478030 100644
 | --- a/lib/options.py
 | +++ b/lib/options.py
 | @@ -213,6 +213,7 @@ class Options:
 |                  "--bootstrap",
 |                  dest='snapshot_path',
 |                  default=None,
 | +                type=os.path.abspath,
 |                  help="""Path to snapshot that will be used to bootstrap
 |                  Tarantool before testing.""")
 |  
 | @@ -247,12 +248,9 @@ class Options:
 |                           schema='error')
 |              check_error = True
 |  
 | -        if snapshot_path:
 | -            if not os.path.exists(snapshot_path):
 | -                color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')
 | -                check_error = True
 | -            else:
 | -                self.args.snapshot_path = os.path.abspath(snapshot_path)
 | +        if snapshot_path and not os.path.exists(snapshot_path):
 | +            color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')
 | +            check_error = True
 |  
 |          if check_error:
 |              exit(-1)

BTW, you may fix exit(-1) (which gives 255) to exit(1). I guess it was
the unintentional mistake. I left it unchanged (just my laziness,
sorry).

> @@ -84,6 +90,9 @@ class Server(object):
>          self.inspector_port = int(ini.get(
>              'inspector_port', self.DEFAULT_INSPECTOR
>          ))
> +        self.testdir = os.path.abspath(os.curdir)
> +        self.disable_schema_upgrade = Options().args.disable_schema_upgrade
> +        self.snapshot_path = Options().args.snapshot_path

I would use the option variables directly, it looks simpler for me (less
transitive def-use dependencies, so the dependency tree has lesser
height). However other options are also saved in *Server instances, so
okay.

> @@ -609,7 +613,6 @@ class TarantoolServer(Server):
>          }
>          ini.update(_ini)
>          Server.__init__(self, ini, test_suite)
> -        self.testdir = os.path.abspath(os.curdir)

We discussed it in the past review and you said that reverted it, but it
seems, you don't. I did it in a fixup commit.

> @@ -775,8 +778,29 @@ class TarantoolServer(Server):
>              shutil.copy(os.path.join(self.TEST_RUN_DIR, 'pretest_clean.lua'),
>                          self.vardir)
>  
> +        if self.snapshot_path:
> +            # Copy snapshot to the workdir.
> +            # Usually Tarantool looking for snapshots on start in a current directory
> +            # or in a directories that specified in memtx_dir or vinyl_dir box settings.
> +            # Before running test current directory (workdir) passed to a new instance in
> +            # an environment variable TEST_WORKDIR and then .tarantoolctl.in
> +            # adds to it instance_name and set to memtx_dir and vinyl_dir.

I replaced '.tarantoolctl.in' with 'tarantoolctl' in a fixup commit. It
looks as typo. Let me know, if I misunderstood something.

> @@ -861,6 +885,18 @@ class TarantoolServer(Server):
>          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
>          self.status = 'started'
>  
> +        if not self.ctl_path:
> +            self.ctl_path = self.find_exe(self.builddir)

I remember, you said that the same if-condition is present in the
print_exe() classmethod. But this situation is different: we already use
self.ctl_path inside self.prepare_args() call above. If it is None,
False or [], Popen will fail with AttributeError or TypeError. So
despite it may have a sense in another place of the code, here it just
gives a reader the wrong idea that self.ctl_path may be None.

Removed in a fixup commit.

> +        if self.disable_schema_upgrade:
> +            version = extract_schema_from_snapshot(self.ctl_path,
> +                                                   self.builddir, self.snapshot_path)
> +            current_ver = yaml.safe_load(self.admin.execute(
> +                'box.space._schema:get{"version"}'))
> +            if version == current_ver:
> +                raise_msg = 'Versions are inequal ({} vs {})'.format(
> +                    version, current_ver)
> +                raise Exception(raise_msg)
> +

I made several fixups here:

 | commit 947df1416c0f3b6330ad8d257077e46b2d06d821
 | Author: Alexander Turenko <alexander.turenko@tarantool.org>
 | Date:   Thu Nov 26 19:42:11 2020 +0300
 | 
 |     FIXUP: Add options for upgrade testing
 |     
 |     Several problems are fixed within the hunk:
 |     
 |     1. Added a short explanation comment.
 |     2. Gave a bit more descriptive names to variables.
 |     3. Fixed actual version acquiring (added [0]).
 |     4. Fixed versions comparison (`==` -> `!=`).
 |     5. Print a bit more descriptive error message.
 |     6. Changed the exception to TarantoolStartError.
 |     
 |     The 6th point is dubious. I mean, `raise Exception(<...>)` should be
 |     changed to some more-or-less concrete exception class: RuntimeError or
 |     TarantoolStartError. Neither of them handled good by test-run: it prints
 |     a traceback and it looks like internal test-run error. But at least it
 |     kills the server, so it don't hang is the prcess table after the
 |     testing.
 |     
 |     I would leave it as is here and maybe investigate later if there will be
 |     a demand.
 | 
 | diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
 | index 5b47ff0..8de1a30 100644
 | --- a/lib/tarantool_server.py
 | +++ b/lib/tarantool_server.py
 | @@ -886,15 +886,18 @@ class TarantoolServer(Server):
 |          self.admin = CON_SWITCH[self.tests_type]('localhost', port)
 |          self.status = 'started'
 |  
 | +        # Verify that the schema actually was not upgraded.
 |          if self.disable_schema_upgrade:
 | -            version = extract_schema_from_snapshot(self.ctl_path,
 | -                                                   self.builddir, self.snapshot_path)
 | -            current_ver = yaml.safe_load(self.admin.execute(
 | -                'box.space._schema:get{"version"}'))
 | -            if version == current_ver:
 | -                raise_msg = 'Versions are inequal ({} vs {})'.format(
 | -                    version, current_ver)
 | -                raise Exception(raise_msg)
 | +            expected_version = extract_schema_from_snapshot(
 | +                self.ctl_path, self.builddir, self.snapshot_path)
 | +            actual_version = yaml.safe_load(self.admin.execute(
 | +                'box.space._schema:get{"version"}'))[0]
 | +            if expected_version != actual_version:
 | +                color_stdout('Schema version check fails: expected '
 | +                             '{}, got {}\n'.format(expected_version,
 | +                                                   actual_version),
 | +                             schema='error')
 | +                raise TarantoolStartError(self.name)
 |  
 |      def crash_detect(self):
 |          if self.crash_expected:

----

The patches about using a bootstrap snapshot (and simplification of
extract_schema_from_snapshot()). To be removed (maybe except
extract_schema_from_snapshot()) after #5540.

commit a34c28ea61c9a56b8ed2be2f045ebea8f94d0f8a
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Fri Nov 27 02:17:43 2020 +0300

    FIXUP: Add options for upgrade testing
    
    Add xlog utility functions. Will be used in the following fixup commits.

diff --git a/lib/xlog.py b/lib/xlog.py
new file mode 100644
index 0000000..dbaa4dc
--- /dev/null
+++ b/lib/xlog.py
@@ -0,0 +1,232 @@
+"""Xlog and snapshot utility functions."""
+
+import os
+import msgpack
+import subprocess
+import json
+from uuid import uuid4
+
+
+__all__ = ['init', 'snapshot_is_for_bootstrap', 'prepare_bootstrap_snapshot']
+
+
+# {{{ Constants
+
+# Xlog binary format constants.
+ROW_MARKER = b'\xd5\xba\x0b\xab'
+EOF_MARKER = b'\xd5\x10\xad\xed'
+XLOG_FIXHEADER_SIZE = 19
+VCLOCK_MAX = 32
+VCLOCK_STR_LEN_MAX = 1 + VCLOCK_MAX * (2 + 2 + 20 + 2) + 1
+XLOG_META_LEN_MAX = 1024 + VCLOCK_STR_LEN_MAX
+
+
+# The binary protocol (iproto) constants.
+#
+# Widely reused for xlog / snapshot files.
+IPROTO_REQUEST_TYPE = 0x00
+IPROTO_LSN = 0x03
+IPROTO_TIMESTAMP = 0x04
+IPROTO_INSERT = 2
+IPROTO_SPACE_ID = 0x10
+IPROTO_TUPLE = 0x21
+
+
+# System space IDs.
+BOX_SCHEMA_ID = 272
+BOX_CLUSTER_ID = 320
+
+# }}} Constants
+
+
+tarantool_cmd = 'tarantool'
+tarantoolctl_cmd = 'tarantoolctl'
+debug_f = lambda x: None  # noqa: E731
+
+
+def init(tarantool=None, tarantoolctl=None, debug=None):
+    """ Redefine module level globals.
+
+        If the function is not called, tarantool and tarantoolctl
+        will be called according to the PATH environment variable.
+
+        Beware: tarantool and tarantoolctl are lists. Example:
+
+        tarantool_cmd = ['/path/to/tarantool']
+        tarantoolctl_cmd = tarantool_cmd + ['/path/to/tarantoolctl']
+        xlog.init(tarantool=tarantool_cmd, tarantoolctl=tarantoolctl_cmd)
+    """
+    global tarantool_cmd
+    global tarantoolctl_cmd
+    global debug_f
+
+    if tarantool:
+        assert isinstance(tarantool, list)
+        tarantool_cmd = tarantool
+    if tarantool_cmd:
+        assert isinstance(tarantoolctl, list)
+        tarantoolctl_cmd = tarantoolctl
+    if debug:
+        debug_f = debug
+
+
+# {{{ General purpose helpers
+
+def crc32c(data):
+    """ Use tarantool's implementation of CRC32C algorithm.
+
+        Python has no built-in implementation of CRC32C.
+    """
+    lua = "print(require('digest').crc32_update(0, io.stdin:read({})))".format(
+        len(data))
+    with open(os.devnull, 'w') as devnull:
+        process = subprocess.Popen(tarantool_cmd + ['-e', lua],
+                                   stdin=subprocess.PIPE,
+                                   stdout=subprocess.PIPE,
+                                   stderr=devnull)
+    process.stdin.write(data)
+    res, _ = process.communicate()
+    return int(res)
+
+# }}} General purpose helpers
+
+
+# {{{ parse xlog / snapshot
+
+def xlog_rows(xlog_path):
+    cmd = tarantoolctl_cmd + ['cat', xlog_path, '--format=json',
+                              '--show-system']
+    with open(os.devnull, 'w') as devnull:
+        process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=devnull)
+    for line in process.stdout.readlines():
+        yield json.loads(line)
+
+# }}} parse xlog / snapshot
+
+
+# {{{ xlog encode data helpers
+
+def encode_xrow_header(xrow):
+    packer = msgpack.Packer(use_bin_type=False)
+    xrow_header = ROW_MARKER
+    # xrow size
+    xrow_header += packer.pack(len(xrow))
+    # previous xrow crc32
+    xrow_header += packer.pack(0)
+    # current xrow crc32
+    xrow_header += packer.pack(crc32c(xrow))
+    # padding
+    padding_size = XLOG_FIXHEADER_SIZE - len(xrow_header)
+    xrow_header += packer.pack(b'\x00' * (padding_size - 1))
+    assert(len(xrow_header) == XLOG_FIXHEADER_SIZE)
+    return xrow_header
+
+
+def encode_xrow(header, body):
+    packer = msgpack.Packer(use_bin_type=False)
+    header = packer.pack(header)
+    body = packer.pack(body)
+    xrow_data = header + body
+    return encode_xrow_header(xrow_data) + xrow_data
+
+# }}} xlog encode data helpers
+
+
+# {{{ xlog write data helpers
+
+def xlog_seek_end(xlog):
+    """Set the file position right before the end marker."""
+    WHENCE_END = 2
+    xlog.seek(-4, WHENCE_END)
+    eof_marker = xlog.read(4)
+    if eof_marker != EOF_MARKER:
+        raise RuntimeError('Invalid eof marker: {}'.format(eof_marker))
+    xlog.seek(-4, WHENCE_END)
+
+
+def xlog_write_eof(xlog):
+    xlog.write(EOF_MARKER)
+
+# }}} xlog write data helpers
+
+
+# {{{ xlog write meta helpers
+
+def xlog_meta_write_instance_uuid(xlog, instance_uuid):
+    xlog.seek(0)
+    xlog.seek(xlog.read(XLOG_META_LEN_MAX).find(b'Instance: '))
+    # Trick: old and new UUID have the same size.
+    xlog.write(b'Instance: ' + instance_uuid)
+
+# }}} xlog write meta helpers
+
+
+def snapshot_is_for_bootstrap(snapshot_path):
+    """ A bootstrap snapshot (src/box/bootstrap.snap) has no
+        <cluster_uuid> and <instance_uuid> in _schema and
+        _cluster system spaces.
+    """
+    cluster_uuid_exists = False
+    instance_uuid_exists = False
+
+    for row in xlog_rows(snapshot_path):
+        if row['HEADER']['type'] == 'INSERT' and         \
+           row['BODY']['space_id'] == BOX_SCHEMA_ID and \
+           row['BODY']['tuple'][0] == 'cluster':
+            cluster_uuid_exists = True
+
+        if row['HEADER']['type'] == 'INSERT' and          \
+           row['BODY']['space_id'] == BOX_CLUSTER_ID and \
+           row['BODY']['tuple'][0] == 1:
+            instance_uuid_exists = True
+
+        if cluster_uuid_exists and instance_uuid_exists:
+            break
+
+    if cluster_uuid_exists != instance_uuid_exists:
+        raise RuntimeError('A cluster UUID and an instance UUID should exist '
+                           'or not exist both')
+
+    return not cluster_uuid_exists
+
+
+def prepare_bootstrap_snapshot(snapshot_path):
+    """ Prepare a bootstrap snapshot to use with local recovery."""
+    cluster_uuid = str(uuid4()).encode('ascii')
+    debug_f('Cluster UUID: {}'.format(cluster_uuid))
+    instance_uuid = str(uuid4()).encode('ascii')
+    instance_id = 1
+    debug_f('Instance ID: {}'.format(instance_id))
+    debug_f('Instance UUID: {}'.format(instance_uuid))
+
+    last_row = list(xlog_rows(snapshot_path))[-1]
+    lsn = int(last_row['HEADER']['lsn'])
+    timestamp = float(last_row['HEADER']['timestamp'])
+
+    with open(snapshot_path, 'rb+') as xlog:
+        xlog_meta_write_instance_uuid(xlog, instance_uuid)
+        xlog_seek_end(xlog)
+
+        # Write cluster UUID to _schema.
+        lsn += 1
+        xlog.write(encode_xrow({
+            IPROTO_REQUEST_TYPE: IPROTO_INSERT,
+            IPROTO_LSN: lsn,
+            IPROTO_TIMESTAMP: timestamp,
+        }, {
+            IPROTO_SPACE_ID: BOX_SCHEMA_ID,
+            IPROTO_TUPLE: ['cluster', cluster_uuid],
+        }))
+
+        # Write replica ID and replica UUID to _cluster.
+        lsn += 1
+        xlog.write(encode_xrow({
+            IPROTO_REQUEST_TYPE: IPROTO_INSERT,
+            IPROTO_LSN: lsn,
+            IPROTO_TIMESTAMP: timestamp,
+        }, {
+            IPROTO_SPACE_ID: BOX_CLUSTER_ID,
+            IPROTO_TUPLE: [1, instance_uuid],
+        }))
+
+        xlog_write_eof(xlog)

commit 4e860a6a405d08aebc23a233783c043027aaa1f5
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Fri Nov 27 02:52:47 2020 +0300

    FIXUP: Add options for upgrade testing
    
    Move the module search path tweaking into the upper level. The next
    commit will use the xlog module, which requires msgpack. So we need to
    add the necessary search paths before importing the module.

diff --git a/lib/__init__.py b/lib/__init__.py
index 767f0d6..dccb76b 100644
--- a/lib/__init__.py
+++ b/lib/__init__.py
@@ -2,10 +2,14 @@ import os
 import sys
 import shutil
 
-from lib.options import Options
-from lib.tarantool_server import TarantoolServer
-from lib.unittest_server import UnittestServer
-from utils import warn_unix_sockets_at_start
+# monkey patch tarantool and msgpack
+from lib.utils import check_libs
+check_libs()
+
+from lib.options import Options                   # noqa: E402
+from lib.tarantool_server import TarantoolServer  # noqa: E402
+from lib.unittest_server import UnittestServer    # noqa: E402
+from utils import warn_unix_sockets_at_start      # noqa: E402
 
 
 __all__ = ['Options']
diff --git a/lib/box_connection.py b/lib/box_connection.py
index 6eb5121..12a5abd 100644
--- a/lib/box_connection.py
+++ b/lib/box_connection.py
@@ -26,13 +26,8 @@ import ctypes
 import socket
 
 from tarantool_connection import TarantoolConnection
-
-# monkey patch tarantool and msgpack
-from lib.utils import check_libs
-check_libs()
-
-from tarantool import Connection as tnt_connection  # noqa: E402
-from tarantool import Schema                        # noqa: E402
+from tarantool import Connection as tnt_connection
+from tarantool import Schema
 
 
 SEPARATOR = '\n'

commit 4c93e1a105737c74f77035aa638100b09c2367ce
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Fri Nov 27 02:58:16 2020 +0300

    FIXUP: Add options for upgrade testing
    
    Accept only bootstrap snapshots, but prepare them to use with local
    recovery. We have no other way to feed an external bootstrap snapshot to
    tarantool until [1] will be implemented.
    
    [1]: https://github.com/tarantool/tarantool/issues/5540

diff --git a/lib/__init__.py b/lib/__init__.py
index dccb76b..c8e8626 100644
--- a/lib/__init__.py
+++ b/lib/__init__.py
@@ -10,6 +10,8 @@ from lib.options import Options                   # noqa: E402
 from lib.tarantool_server import TarantoolServer  # noqa: E402
 from lib.unittest_server import UnittestServer    # noqa: E402
 from utils import warn_unix_sockets_at_start      # noqa: E402
+from lib.colorer import color_log                 # noqa: E402
+import xlog                                       # noqa: E402
 
 
 __all__ = ['Options']
@@ -60,6 +62,14 @@ def module_init():
     TarantoolServer.find_exe(args.builddir)
     UnittestServer.find_exe(args.builddir)
 
+    # Initialize xlog module with found tarantool / tarantoolctl.
+    # Set color_log() as the function for write debug logs.
+    tarantool_cmd = [TarantoolServer.binary]
+    tarantoolctl_cmd = tarantool_cmd + [TarantoolServer.ctl_path]
+    xlog.init(tarantool=tarantool_cmd, tarantoolctl=tarantoolctl_cmd,
+              debug=lambda x: color_log(' | ' + x + '\n'))
+
+    Options().check_bootstrap_option()
     Options().check_schema_upgrade_option(TarantoolServer.debug)
 
 
diff --git a/lib/app_server.py b/lib/app_server.py
index a046b21..f2916f0 100644
--- a/lib/app_server.py
+++ b/lib/app_server.py
@@ -17,6 +17,7 @@ from lib.utils import find_port
 from lib.utils import format_process
 from lib.utils import warn_unix_socket
 from test import TestRunGreenlet, TestExecutionError
+from xlog import prepare_bootstrap_snapshot
 
 
 def run_server(execs, cwd, server, logfile, retval):
@@ -51,6 +52,9 @@ class AppTest(Test):
             color_log("Copying snapshot {} to {}\n".format(
                 server.snapshot_path, snapshot_dest))
             shutil.copy(server.snapshot_path, snapshot_dest)
+            color_log('Preparing snapshot {} for local recovery...\n'.format(
+                      snapshot_dest))
+            prepare_bootstrap_snapshot(snapshot_dest)
 
         try:
             tarantool.start()
diff --git a/lib/options.py b/lib/options.py
index c478030..324f59d 100644
--- a/lib/options.py
+++ b/lib/options.py
@@ -5,6 +5,7 @@ from itertools import product
 from lib.singleton import Singleton
 
 from lib.colorer import color_stdout
+from xlog import snapshot_is_for_bootstrap
 
 
 def env_int(name, default):
@@ -255,6 +256,12 @@ class Options:
         if check_error:
             exit(-1)
 
+    def check_bootstrap_option(self):
+        if not snapshot_is_for_bootstrap(self.args.snapshot_path):
+            color_stdout('Expected a boostrap snapshot, one for local recovery '
+                         'is given\n', schema='error')
+            exit(1)
+
     def check_schema_upgrade_option(self, is_debug):
         if self.args.disable_schema_upgrade and not is_debug:
             color_stdout("Can't disable schema upgrade on release build\n", schema='error')
diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index 4121a4d..78994c6 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -36,6 +36,7 @@ from lib.utils import signame
 from lib.utils import warn_unix_socket
 from lib.utils import prefix_each_line
 from test import TestRunGreenlet, TestExecutionError
+from xlog import prepare_bootstrap_snapshot
 
 
 def save_join(green_obj, timeout=None):
@@ -791,6 +792,9 @@ class TarantoolServer(Server):
             color_log("Copying snapshot {} to {}\n".format(
                 self.snapshot_path, snapshot_dest))
             shutil.copy(self.snapshot_path, snapshot_dest)
+            color_log('Preparing snapshot {} for local recovery...\n'.format(
+                      snapshot_dest))
+            prepare_bootstrap_snapshot(snapshot_dest)
 
     def prepare_args(self, args=[]):
         cli_args = [self.ctl_path, 'start',

commit 2fe384b4287033695f43eb695e776e3ecf8ae968
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Fri Nov 27 03:49:18 2020 +0300

    FIXUP: Add options for upgrade testing
    
    Use xlog.xlog_rows() helper in the extract_schema_from_snapshot()
    function.
    
    Several other changes:
    
    1. Moved to the xlog module, since we have it separate from other utils.
    2. Prettify the example of json formatted xrow.
    3. Get rid of one-time-use variables. It looks more readable for me
       here.
    4. Expect only 'INSERT' queries, because the source is a snapshot. The
       function should be changed anyway to extract a last version from an
       xlog file: traverse till end, handle 'UPDATE'.

diff --git a/lib/tarantool_server.py b/lib/tarantool_server.py
index 78994c6..d954032 100644
--- a/lib/tarantool_server.py
+++ b/lib/tarantool_server.py
@@ -29,7 +29,6 @@ from lib.server import Server
 from lib.server import DEFAULT_SNAPSHOT_NAME
 from lib.test import Test
 from lib.utils import find_port
-from lib.utils import extract_schema_from_snapshot
 from lib.utils import format_process
 from lib.utils import safe_makedirs
 from lib.utils import signame
@@ -37,6 +36,7 @@ from lib.utils import warn_unix_socket
 from lib.utils import prefix_each_line
 from test import TestRunGreenlet, TestExecutionError
 from xlog import prepare_bootstrap_snapshot
+from xlog import extract_schema_from_snapshot
 
 
 def save_join(green_obj, timeout=None):
@@ -890,8 +890,7 @@ class TarantoolServer(Server):
 
         # Verify that the schema actually was not upgraded.
         if self.disable_schema_upgrade:
-            expected_version = extract_schema_from_snapshot(
-                self.ctl_path, self.builddir, self.snapshot_path)
+            expected_version = extract_schema_from_snapshot(self.snapshot_path)
             actual_version = yaml.safe_load(self.admin.execute(
                 'box.space._schema:get{"version"}'))[0]
             if expected_version != actual_version:
diff --git a/lib/utils.py b/lib/utils.py
index e0bd394..cc2f6b7 100644
--- a/lib/utils.py
+++ b/lib/utils.py
@@ -3,10 +3,8 @@ import sys
 import six
 import collections
 import signal
-import subprocess
 import random
 import fcntl
-import json
 import difflib
 import time
 from gevent import socket
@@ -266,34 +264,3 @@ def just_and_trim(src, width):
     if len(src) > width:
         return src[:width - 1] + '>'
     return src.ljust(width)
-
-
-def extract_schema_from_snapshot(ctl_path, builddir, snapshot_path):
-    """
-    Extract schema version from snapshot, example of record:
-     {"HEADER":{"lsn":2,"type":"INSERT","timestamp":1584694286.0031},
-       "BODY":{"space_id":272,"tuple":["version",2,3,1]}}
-
-    :returns: [u'version', 2, 3, 1]
-    """
-    SCHEMA_SPACE_ID = 272
-
-    ctl_args = [ctl_path, 'cat', snapshot_path,
-                '--format=json', '--show-system']
-    with open(os.devnull, 'w') as devnull:
-        proc = subprocess.Popen(ctl_args, stdout=subprocess.PIPE, stderr=devnull)
-    version = None
-    while True:
-        line = proc.stdout.readline()
-        if not line:
-            break
-        json_line = json.loads(line.strip())
-        query_type = json_line["HEADER"]["type"]
-        space_id = json_line["BODY"]["space_id"]
-        if query_type == "INSERT" or query_type == "REPLACE" and space_id == SCHEMA_SPACE_ID:
-            query_tuple = json_line['BODY']['tuple']
-            if query_tuple[0] == 'version':
-                version = query_tuple
-                break
-
-    return version
diff --git a/lib/xlog.py b/lib/xlog.py
index dbaa4dc..0105dfc 100644
--- a/lib/xlog.py
+++ b/lib/xlog.py
@@ -7,7 +7,8 @@ import json
 from uuid import uuid4
 
 
-__all__ = ['init', 'snapshot_is_for_bootstrap', 'prepare_bootstrap_snapshot']
+__all__ = ['init', 'snapshot_is_for_bootstrap', 'prepare_bootstrap_snapshot',
+           'extract_schema_from_snapshot']
 
 
 # {{{ Constants
@@ -230,3 +231,25 @@ def prepare_bootstrap_snapshot(snapshot_path):
         }))
 
         xlog_write_eof(xlog)
+
+
+def extract_schema_from_snapshot(snapshot_path):
+    """
+    Extract schema version from snapshot.
+
+    Example of record:
+
+     {
+       "HEADER": {"lsn":2, "type": "INSERT", "timestamp": 1584694286.0031},
+       "BODY": {"space_id": 272, "tuple": ["version", 2, 3, 1]}
+     }
+
+    :returns: [u'version', 2, 3, 1]
+    """
+    for row in xlog_rows(snapshot_path):
+        if row['HEADER']['type'] == 'INSERT' and \
+           row['BODY']['space_id'] == BOX_SCHEMA_ID:
+            res = row['BODY']['tuple']
+            if res[0] == 'version':
+                return res
+    return None

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Sergey Bronnikov]

Hi,

On 27.11.2020 04:45, Alexander Turenko wrote:
> Sorry for the long delay.
>
> I looked over the patch, tested it a bit and proposed several fixups.
>
> I pasted them here (for ease commenting) and pushed to the branch
> (upward your commits). If you're okay with them, I can squash commits
> and push the result. Or, maybe, extract several commits (re debug
> property, re xlog module) and squash the rest (keeping the conflicting
> options fix separate, of course).
>
> The branch: ligurio/gh-4801-add-snapshot-option.
>
> And sorry that I reworked a lot of code. I tried to minimize review
> ping-ponging, but make myself comfortable with the result. And amount of
> changes becomes larger than I initially expect. A lot of low hanging
> fruits of the kind 'okay, but there is the easy way to make it a bit
> better' appears.
Many thanks for patches! Especially those that simplifies a code.
> I hope such moments will be less and less frequent with a time we'll
> work in touch. A review is often about compromises and it is the work on
> ourself for both sides.
>
> WBR, Alexander Turenko.
>
<snipped>
> new file mode 100644
> index 0000000..dbaa4dc
> --- /dev/null
> +++ b/lib/xlog.py
> @@ -0,0 +1,232 @@
> +"""Xlog and snapshot utility functions."""
> +
> +import os
> +import msgpack
> +import subprocess
> +import json
> +from uuid import uuid4
> +
> +
> +__all__ = ['init', 'snapshot_is_for_bootstrap', 'prepare_bootstrap_snapshot']
> +
> +
> +# {{{ Constants
> +
> +# Xlog binary format constants.
> +ROW_MARKER = b'\xd5\xba\x0b\xab'
> +EOF_MARKER = b'\xd5\x10\xad\xed'
> +XLOG_FIXHEADER_SIZE = 19
> +VCLOCK_MAX = 32
> +VCLOCK_STR_LEN_MAX = 1 + VCLOCK_MAX * (2 + 2 + 20 + 2) + 1
> +XLOG_META_LEN_MAX = 1024 + VCLOCK_STR_LEN_MAX
> +
> +
> +# The binary protocol (iproto) constants.
> +#
> +# Widely reused for xlog / snapshot files.
> +IPROTO_REQUEST_TYPE = 0x00
> +IPROTO_LSN = 0x03
> +IPROTO_TIMESTAMP = 0x04
> +IPROTO_INSERT = 2
> +IPROTO_SPACE_ID = 0x10
> +IPROTO_TUPLE = 0x21
> +
> +
> +# System space IDs.
> +BOX_SCHEMA_ID = 272
> +BOX_CLUSTER_ID = 320
> +
> +# }}} Constants
> +
> +
> +tarantool_cmd = 'tarantool'
> +tarantoolctl_cmd = 'tarantoolctl'
> +debug_f = lambda x: None  # noqa: E731
> +
> +
> +def init(tarantool=None, tarantoolctl=None, debug=None):
> +    """ Redefine module level globals.
> +
> +        If the function is not called, tarantool and tarantoolctl
> +        will be called according to the PATH environment variable.
> +
> +        Beware: tarantool and tarantoolctl are lists. Example:
> +
> +        tarantool_cmd = ['/path/to/tarantool']
> +        tarantoolctl_cmd = tarantool_cmd + ['/path/to/tarantoolctl']
> +        xlog.init(tarantool=tarantool_cmd, tarantoolctl=tarantoolctl_cmd)
> +    """
> +    global tarantool_cmd
> +    global tarantoolctl_cmd
> +    global debug_f
> +
> +    if tarantool:
> +        assert isinstance(tarantool, list)
> +        tarantool_cmd = tarantool
> +    if tarantool_cmd:
> +        assert isinstance(tarantoolctl, list)
> +        tarantoolctl_cmd = tarantoolctl
> +    if debug:
> +        debug_f = debug
> +
> +
> +# {{{ General purpose helpers
> +
> +def crc32c(data):
> +    """ Use tarantool's implementation of CRC32C algorithm.
> +
> +        Python has no built-in implementation of CRC32C.
> +    """
> +    lua = "print(require('digest').crc32_update(0, io.stdin:read({})))".format(
> +        len(data))
> +    with open(os.devnull, 'w') as devnull:
> +        process = subprocess.Popen(tarantool_cmd + ['-e', lua],
> +                                   stdin=subprocess.PIPE,
> +                                   stdout=subprocess.PIPE,
> +                                   stderr=devnull)
> +    process.stdin.write(data)
> +    res, _ = process.communicate()
> +    return int(res)
> +
> +# }}} General purpose helpers
> +
> +
> +# {{{ parse xlog / snapshot
> +
> +def xlog_rows(xlog_path):
> +    cmd = tarantoolctl_cmd + ['cat', xlog_path, '--format=json',
> +                              '--show-system']
> +    with open(os.devnull, 'w') as devnull:
> +        process = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=devnull)
> +    for line in process.stdout.readlines():
> +        yield json.loads(line)
> +
> +# }}} parse xlog / snapshot
> +
> +
> +# {{{ xlog encode data helpers
> +
> +def encode_xrow_header(xrow):
> +    packer = msgpack.Packer(use_bin_type=False)
> +    xrow_header = ROW_MARKER
> +    # xrow size
> +    xrow_header += packer.pack(len(xrow))
> +    # previous xrow crc32
> +    xrow_header += packer.pack(0)
> +    # current xrow crc32
> +    xrow_header += packer.pack(crc32c(xrow))
> +    # padding
> +    padding_size = XLOG_FIXHEADER_SIZE - len(xrow_header)
> +    xrow_header += packer.pack(b'\x00' * (padding_size - 1))
> +    assert(len(xrow_header) == XLOG_FIXHEADER_SIZE)
> +    return xrow_header
> +
> +
> +def encode_xrow(header, body):
> +    packer = msgpack.Packer(use_bin_type=False)
> +    header = packer.pack(header)
> +    body = packer.pack(body)
> +    xrow_data = header + body
> +    return encode_xrow_header(xrow_data) + xrow_data
> +
> +# }}} xlog encode data helpers
> +
> +
> +# {{{ xlog write data helpers
> +
> +def xlog_seek_end(xlog):
> +    """Set the file position right before the end marker."""
> +    WHENCE_END = 2
> +    xlog.seek(-4, WHENCE_END)
> +    eof_marker = xlog.read(4)
> +    if eof_marker != EOF_MARKER:
> +        raise RuntimeError('Invalid eof marker: {}'.format(eof_marker))
> +    xlog.seek(-4, WHENCE_END)
> +
> +
> +def xlog_write_eof(xlog):
> +    xlog.write(EOF_MARKER)
> +
> +# }}} xlog write data helpers
> +
> +
> +# {{{ xlog write meta helpers
> +
> +def xlog_meta_write_instance_uuid(xlog, instance_uuid):
> +    xlog.seek(0)
> +    xlog.seek(xlog.read(XLOG_META_LEN_MAX).find(b'Instance: '))
> +    # Trick: old and new UUID have the same size.
> +    xlog.write(b'Instance: ' + instance_uuid)
> +
> +# }}} xlog write meta helpers
> +
> +
> +def snapshot_is_for_bootstrap(snapshot_path):
> +    """ A bootstrap snapshot (src/box/bootstrap.snap) has no
> +        <cluster_uuid> and <instance_uuid> in _schema and
> +        _cluster system spaces.
> +    """
> +    cluster_uuid_exists = False
> +    instance_uuid_exists = False
> +
> +    for row in xlog_rows(snapshot_path):
> +        if row['HEADER']['type'] == 'INSERT' and         \
> +           row['BODY']['space_id'] == BOX_SCHEMA_ID and \
> +           row['BODY']['tuple'][0] == 'cluster':
> +            cluster_uuid_exists = True
> +
> +        if row['HEADER']['type'] == 'INSERT' and          \
> +           row['BODY']['space_id'] == BOX_CLUSTER_ID and \
> +           row['BODY']['tuple'][0] == 1:
> +            instance_uuid_exists = True
> +
> +        if cluster_uuid_exists and instance_uuid_exists:
> +            break
> +
> +    if cluster_uuid_exists != instance_uuid_exists:
> +        raise RuntimeError('A cluster UUID and an instance UUID should exist '
> +                           'or not exist both')
> +
> +    return not cluster_uuid_exists
> +

snapshot_is_for_bootstrap() accept a path that can be None

(see default value in argument --bootstrap) when option --bootstrap is 
not specified.

Due to this xlog_rows() may fail. xlog_rows() returns iterator and we 
can return something like

"yield None" but we should handle this properly everywhere where 
xlog_rows() called.

So I just added patch to snapshot_is_for_bootstrap():

--- a/lib/options.py
+++ b/lib/options.py
@@ -257,7 +257,7 @@ class Options:
              exit(-1)

      def check_bootstrap_option(self):
-        if not snapshot_is_for_bootstrap(self.args.snapshot_path):
+        if self.args.snapshot_path and not 
snapshot_is_for_bootstrap(self.args.snapshot_path):
              color_stdout('Expected a boostrap snapshot, one for local 
recovery '
                           'is given\n', schema='error')
              exit(1)


> +
> +def prepare_bootstrap_snapshot(snapshot_path):
> +    """ Prepare a bootstrap snapshot to use with local recovery."""
> +    cluster_uuid = str(uuid4()).encode('ascii')
> +    debug_f('Cluster UUID: {}'.format(cluster_uuid))
> +    instance_uuid = str(uuid4()).encode('ascii')
> +    instance_id = 1
> +    debug_f('Instance ID: {}'.format(instance_id))
> +    debug_f('Instance UUID: {}'.format(instance_uuid))
> +
> +    last_row = list(xlog_rows(snapshot_path))[-1]
> +    lsn = int(last_row['HEADER']['lsn'])
> +    timestamp = float(last_row['HEADER']['timestamp'])
> +
> +    with open(snapshot_path, 'rb+') as xlog:
> +        xlog_meta_write_instance_uuid(xlog, instance_uuid)
> +        xlog_seek_end(xlog)
> +
> +        # Write cluster UUID to _schema.
> +        lsn += 1
> +        xlog.write(encode_xrow({
> +            IPROTO_REQUEST_TYPE: IPROTO_INSERT,
> +            IPROTO_LSN: lsn,
> +            IPROTO_TIMESTAMP: timestamp,
> +        }, {
> +            IPROTO_SPACE_ID: BOX_SCHEMA_ID,
> +            IPROTO_TUPLE: ['cluster', cluster_uuid],
> +        }))
> +
> +        # Write replica ID and replica UUID to _cluster.
> +        lsn += 1
> +        xlog.write(encode_xrow({
> +            IPROTO_REQUEST_TYPE: IPROTO_INSERT,
> +            IPROTO_LSN: lsn,
> +            IPROTO_TIMESTAMP: timestamp,
> +        }, {
> +            IPROTO_SPACE_ID: BOX_CLUSTER_ID,
> +            IPROTO_TUPLE: [1, instance_uuid],
> +        }))
> +
> +        xlog_write_eof(xlog)
>

<snipped>

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Alexander Turenko]

> > +def snapshot_is_for_bootstrap(snapshot_path):
> > +    """ A bootstrap snapshot (src/box/bootstrap.snap) has no
> > +        <cluster_uuid> and <instance_uuid> in _schema and
> > +        _cluster system spaces.
> > +    """
> > +    cluster_uuid_exists = False
> > +    instance_uuid_exists = False
> > +
> > +    for row in xlog_rows(snapshot_path):
> > +        if row['HEADER']['type'] == 'INSERT' and         \
> > +           row['BODY']['space_id'] == BOX_SCHEMA_ID and \
> > +           row['BODY']['tuple'][0] == 'cluster':
> > +            cluster_uuid_exists = True
> > +
> > +        if row['HEADER']['type'] == 'INSERT' and          \
> > +           row['BODY']['space_id'] == BOX_CLUSTER_ID and \
> > +           row['BODY']['tuple'][0] == 1:
> > +            instance_uuid_exists = True
> > +
> > +        if cluster_uuid_exists and instance_uuid_exists:
> > +            break
> > +
> > +    if cluster_uuid_exists != instance_uuid_exists:
> > +        raise RuntimeError('A cluster UUID and an instance UUID should exist '
> > +                           'or not exist both')
> > +
> > +    return not cluster_uuid_exists
> > +
> 
> snapshot_is_for_bootstrap() accept a path that can be None
> 
> (see default value in argument --bootstrap) when option --bootstrap is not
> specified.
> 
> Due to this xlog_rows() may fail. xlog_rows() returns iterator and we can
> return something like
> 
> "yield None" but we should handle this properly everywhere where xlog_rows()
> called.
> 
> So I just added patch to snapshot_is_for_bootstrap():
> 
> --- a/lib/options.py
> +++ b/lib/options.py
> @@ -257,7 +257,7 @@ class Options:
>              exit(-1)
> 
>      def check_bootstrap_option(self):
> -        if not snapshot_is_for_bootstrap(self.args.snapshot_path):
> +        if self.args.snapshot_path and not
> snapshot_is_for_bootstrap(self.args.snapshot_path):
>              color_stdout('Expected a boostrap snapshot, one for local
> recovery '
>                           'is given\n', schema='error')
>              exit(1)

My bad. Thanks for catching it up.

The fix you pasted here looks better for me than one that is on the
branch (which returns True from snapshot_is_for_bootstrap() when a
snapshot path is None).

Applied to the squashed commits on the branch.

----

We had another short discussion with Mons (after his discussion with
Vlad) regarding possible ways to test an instance with an old schema or
an upgraded schema. He highlighted that this is quite far from users'
scenarious.

This convenced me that it would be dubious decision to assume that we'll
actually implement [1] for testing purposes and so name the option as
--bootstrap and restrict accepted snapshots only to ones that are
prepared for bootstrap (as opposite to local recovery). Sorry for those
flips back and forth, but now I think it would be more wise to implement
the simplest feature (--snapshot option, as in your original patch) and
postpone further activities until we'll have at least rough data from
test fails analysis (that is part of [2]).

So, I prepared your branch to push to master with the --snapshot option
without all my patches around mangling bootstrap snapshots. In
particular, I extracted a couple of preparatory patches and apply all
proposed fixups. The result looks so:

 | $ git log --reverse --oneline -4 ligurio/gh-4801-add-snapshot-option | cat
 | dfb6d66 refactoring: make 'debug' property class level
 | 8e80565 Allow to extract a schema version from a snapshot
 | f93af28 Add options for upgrade testing
 | 7d73f2f Update code with checks of conflicting arguments

I'm okay with the state on the branch [3] (it is updated as shown above)
and I ready to push it if you don't have objections against the result
of squashing. I included my variant of extract_schema_from_snapshot()
(but it leans on PATH, it is enough for usages from TarantoolServer).

I performed the following tests using tarantool 2.7.0-73-gb53cb2aec
repository (debug build).

0. make lint
   No errors / warnings. Okay.

1. An app test.
    | (cd test && ./test-run.py box-tap/merger.test.lua)
   Passed. Okay.

2. An app test. Snapshot from 1.10.
    | (cd test && ./test-run.py                              \
    |     --snapshot ../../../1.10/00000000000000000000.snap \
    |     box-tap/merger.test.lua)
   Passed. Upgrade entries are present in the log. Okay.

3. An app test. Snapshot from 1.10. No schema upgrade.
    | (cd test && ./test-run.py                              \
    |     --snapshot ../../../1.10/00000000000000000000.snap \
    |     box-tap/merger.test.lua --disable-schema-upgrade)
   Passed. No upgrade entries in the log. Okay.

4. A tarantool test.
    | (cd test && ./test-run.py box/net.box_timeout_gh-1533.test.lua)
   Passed. Okay.

5. A tarantool test. Snapshot from 1.10.
    | (cd test && ./test-run.py                              \
    |     --snapshot ../../../1.10/00000000000000000000.snap \
    |     box/net.box_timeout_gh-1533.test.lua)
   Passed. Upgrade entries are present in the log. Okay.

6. A tarantool test. Snapshot from 1.10. No schema upgrade.
    | (cd test && ./test-run.py                              \
    |     --snapshot ../../../1.10/00000000000000000000.snap \
    |     box/net.box_timeout_gh-1533.test.lua --disable-schema-upgrade)
   Passed. No upgrade entries in the log. Okay.

7. --disable-schema-upgrade without --snapshot.
    | (cd test && ./test-run.py --disable-schema-upgrade)
   Gives an error. Okay.

8. Conflicting options.
    | (cd test && ./test-run.py --gdb --valgrind)
   Gives an error. Okay.

9. Run the whole test suite.
    | (cd test && ./test-run.py --long --force)
   Passed except replication/election_qsync_stress.test.lua, which is
   known as flaky on this revision. Okay.

Rebuilt the same tarantool revision as RelWithDebInfo and performed the
following test:

10. Attempt to use --disable-schema-upgrade on a RelWithDebInfo build.
     | (cd test && ./test-run.py                              \
     |     --snapshot ../../../1.10/00000000000000000000.snap \
     |     box/net.box_timeout_gh-1533.test.lua --disable-schema-upgrade)
    Gives an error. Okay.

I rewrote my 'snapshot mangling' patches to introduce the new
--bootstrap option and pushed them to the branch [4]. I propose to don't
push them to master at least until we'll found them useful.

[1]: https://github.com/tarantool/tarantool/issues/5540
[2]: https://github.com/tarantool/tarantool/issues/4801
[3]: ligurio/gh-4801-add-snapshot-option
[4]: Totktonada/gh-4801-add-bootstrap-option

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Sergey Bronnikov]

On 02.12.2020 06:40, Alexander Turenko wrote:

<snipped>
> I'm okay with the state on the branch [3] (it is updated as shown above)
> and I ready to push it if you don't have objections against the result
> of squashing. I included my variant of extract_schema_from_snapshot()
> (but it leans on PATH, it is enough for usages from TarantoolServer).

I have tested patches again and it seems it works as expected.

Let's merge patches to master.

>
> I performed the following tests using tarantool 2.7.0-73-gb53cb2aec
> repository (debug build).
>
> 0. make lint
>     No errors / warnings. Okay.
>
> 1. An app test.
>      | (cd test && ./test-run.py box-tap/merger.test.lua)
>     Passed. Okay.
>
> 2. An app test. Snapshot from 1.10.
>      | (cd test && ./test-run.py                              \
>      |     --snapshot ../../../1.10/00000000000000000000.snap \
>      |     box-tap/merger.test.lua)
>     Passed. Upgrade entries are present in the log. Okay.
>
> 3. An app test. Snapshot from 1.10. No schema upgrade.
>      | (cd test && ./test-run.py                              \
>      |     --snapshot ../../../1.10/00000000000000000000.snap \
>      |     box-tap/merger.test.lua --disable-schema-upgrade)
>     Passed. No upgrade entries in the log. Okay.
>
> 4. A tarantool test.
>      | (cd test && ./test-run.py box/net.box_timeout_gh-1533.test.lua)
>     Passed. Okay.
>
> 5. A tarantool test. Snapshot from 1.10.
>      | (cd test && ./test-run.py                              \
>      |     --snapshot ../../../1.10/00000000000000000000.snap \
>      |     box/net.box_timeout_gh-1533.test.lua)
>     Passed. Upgrade entries are present in the log. Okay.
>
> 6. A tarantool test. Snapshot from 1.10. No schema upgrade.
>      | (cd test && ./test-run.py                              \
>      |     --snapshot ../../../1.10/00000000000000000000.snap \
>      |     box/net.box_timeout_gh-1533.test.lua --disable-schema-upgrade)
>     Passed. No upgrade entries in the log. Okay.
>
> 7. --disable-schema-upgrade without --snapshot.
>      | (cd test && ./test-run.py --disable-schema-upgrade)
>     Gives an error. Okay.
>
> 8. Conflicting options.
>      | (cd test && ./test-run.py --gdb --valgrind)
>     Gives an error. Okay.
>
> 9. Run the whole test suite.
>      | (cd test && ./test-run.py --long --force)
>     Passed except replication/election_qsync_stress.test.lua, which is
>     known as flaky on this revision. Okay.
>
> Rebuilt the same tarantool revision as RelWithDebInfo and performed the
> following test:
>
> 10. Attempt to use --disable-schema-upgrade on a RelWithDebInfo build.
>       | (cd test && ./test-run.py                              \
>       |     --snapshot ../../../1.10/00000000000000000000.snap \
>       |     box/net.box_timeout_gh-1533.test.lua --disable-schema-upgrade)
>      Gives an error. Okay.
>
> I rewrote my 'snapshot mangling' patches to introduce the new
> --bootstrap option and pushed them to the branch [4]. I propose to don't
> push them to master at least until we'll found them useful.
>
> [1]: https://github.com/tarantool/tarantool/issues/5540
> [2]: https://github.com/tarantool/tarantool/issues/4801
> [3]: ligurio/gh-4801-add-snapshot-option
> [4]: Totktonada/gh-4801-add-bootstrap-option

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Alexander Turenko]

On Wed, Dec 02, 2020 at 01:49:39PM +0300, Sergey Bronnikov wrote:
> 
> On 02.12.2020 06:40, Alexander Turenko wrote:
> 
> <snipped>
> > I'm okay with the state on the branch [3] (it is updated as shown above)
> > and I ready to push it if you don't have objections against the result
> > of squashing. I included my variant of extract_schema_from_snapshot()
> > (but it leans on PATH, it is enough for usages from TarantoolServer).
> 
> I have tested patches again and it seems it works as expected.
> 
> Let's merge patches to master.

Nice.

Fixed a typo:

 | diff --git a/lib/options.py b/lib/options.py
 | index bd3adc7..445d0d9 100644
 | --- a/lib/options.py
 | +++ b/lib/options.py
 | @@ -256,7 +256,7 @@ class Options:
 |              check_error = True
 | 
 |          if snapshot_path and not os.path.exists(snapshot_path):
 | -            color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')
 | +            color_stdout("\nPath {} does not exist\n".format(snapshot_path), schema='error')
 |              check_error = True
 | 
 |          if check_error:

Pushed to master of the test-run repository.

Alexander, I need your approve to push the corresponding submodule
update into tarantool. Please, take in at a glance. I pushed the update
into the [1] branch to trigger CI, for your convenience.

[1]: Totktonada/test-run-snapshot-cli-option

The commit itself:

commit b27ebb49e173a23259be73d8808b204efda3be03
Author: Alexander Turenko <alexander.turenko@tarantool.org>
Date:   Wed Dec 2 22:17:08 2020 +0300

    test: update test-run (--snapshot)
    
    * Added --snapshot and --disable-schema-upgrade arguments (#240).
    * Fixed reporting of an error for conflicting arguments (#241).
    
    The `--snapshot path/to/snapshot` argument copies a given snapshot to a
    snapshot directory before start a tarantool instance. This allows to
    verify various functionality in the case, when tarantool is upgraded
    from a snapshot that is left by an older tarantool version (as opposite
    to test it on a freshly bootstrapped instance). There are limitations:
    when a test spawns a replica set, the option does not work correctly.
    The reason is that the same instance UUIDs (and IDs) cannot be used by
    different instances in a replica set. Maybe there are other pitfalls.
    
    The `--disable-schema-upgrade` argument instructs tarantool to skip
    execution of the schema upgrade script (using ERRINJ_AUTO_UPGRADE). This
    way we can verify that, when an instance works on an old schema version,
    a functionality is workable or at least gives correct error message.
    
    This commit only brings the new options into test-run. It does NOT add
    any new testing targets / rules.
    
    Part of #4801

diff --git a/test-run b/test-run
index 08a4817bc..26aa3875a 160000
--- a/test-run
+++ b/test-run
@@ -1 +1 @@
-Subproject commit 08a4817bc85ac162a60fa26e943d7485a6fb2a6c
+Subproject commit 26aa3875a7a51952cdb9f487c0e14bbef380db0c

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Alexander Turenko]

Alexander said he doesn't have objections.

Updated the test-run submodule in 2.7.0-81-gaf0e365e2,
2.6.1-68-g84b4f3314, 2.5.2-42-g28ef5bc44, 1.10.8-29-g024f078db.

WBR, Alexander Turenko.

On Thu, Dec 03, 2020 at 05:09:19AM +0300, Alexander Turenko wrote:
> On Wed, Dec 02, 2020 at 01:49:39PM +0300, Sergey Bronnikov wrote:
> > 
> > On 02.12.2020 06:40, Alexander Turenko wrote:
> > 
> > <snipped>
> > > I'm okay with the state on the branch [3] (it is updated as shown above)
> > > and I ready to push it if you don't have objections against the result
> > > of squashing. I included my variant of extract_schema_from_snapshot()
> > > (but it leans on PATH, it is enough for usages from TarantoolServer).
> > 
> > I have tested patches again and it seems it works as expected.
> > 
> > Let's merge patches to master.
> 
> Nice.
> 
> Fixed a typo:
> 
>  | diff --git a/lib/options.py b/lib/options.py
>  | index bd3adc7..445d0d9 100644
>  | --- a/lib/options.py
>  | +++ b/lib/options.py
>  | @@ -256,7 +256,7 @@ class Options:
>  |              check_error = True
>  | 
>  |          if snapshot_path and not os.path.exists(snapshot_path):
>  | -            color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')
>  | +            color_stdout("\nPath {} does not exist\n".format(snapshot_path), schema='error')
>  |              check_error = True
>  | 
>  |          if check_error:
> 
> Pushed to master of the test-run repository.
> 
> Alexander, I need your approve to push the corresponding submodule
> update into tarantool. Please, take in at a glance. I pushed the update
> into the [1] branch to trigger CI, for your convenience.
> 
> [1]: Totktonada/test-run-snapshot-cli-option
> 
> The commit itself:
> 
> commit b27ebb49e173a23259be73d8808b204efda3be03
> Author: Alexander Turenko <alexander.turenko@tarantool.org>
> Date:   Wed Dec 2 22:17:08 2020 +0300
> 
>     test: update test-run (--snapshot)
>     
>     * Added --snapshot and --disable-schema-upgrade arguments (#240).
>     * Fixed reporting of an error for conflicting arguments (#241).
>     
>     The `--snapshot path/to/snapshot` argument copies a given snapshot to a
>     snapshot directory before start a tarantool instance. This allows to
>     verify various functionality in the case, when tarantool is upgraded
>     from a snapshot that is left by an older tarantool version (as opposite
>     to test it on a freshly bootstrapped instance). There are limitations:
>     when a test spawns a replica set, the option does not work correctly.
>     The reason is that the same instance UUIDs (and IDs) cannot be used by
>     different instances in a replica set. Maybe there are other pitfalls.
>     
>     The `--disable-schema-upgrade` argument instructs tarantool to skip
>     execution of the schema upgrade script (using ERRINJ_AUTO_UPGRADE). This
>     way we can verify that, when an instance works on an old schema version,
>     a functionality is workable or at least gives correct error message.
>     
>     This commit only brings the new options into test-run. It does NOT add
>     any new testing targets / rules.
>     
>     Part of #4801
> 
> diff --git a/test-run b/test-run
> index 08a4817bc..26aa3875a 160000
> --- a/test-run
> +++ b/test-run
> @@ -1 +1 @@
> -Subproject commit 08a4817bc85ac162a60fa26e943d7485a6fb2a6c
> +Subproject commit 26aa3875a7a51952cdb9f487c0e14bbef380db0c

Re: [Tarantool-patches] [PATCH] Add options for upgrade testing

[Alexander Turenko]

> +                color_stdout("\nPath {} not exists\n".format(snapshot_path), schema='error')

BTW, 'does not' should be used to express the negative form of the
present simple tense.