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

[Sergey Bronnikov <sergeyb@tarantool.org>]

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