From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp40.i.mail.ru (smtp40.i.mail.ru [94.100.177.100]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3339F469710 for ; Thu, 19 Nov 2020 12:44:25 +0300 (MSK) References: <20200330144823.GA55948@pony.bronevichok.ru> <20200515161727.jtlqplcdkbbcsgh2@tkn_work_nb> <20200528093934.GA19447@pony.bronevichok.ru> <20200602111351.fctlh3aly3euuo4m@tkn_work_nb> <20200610112919.GA78626@pony.bronevichok.ru> <20201110095546.GA3880@pony.bronevichok.ru> <2817d4c9-0ce6-c6b0-75d3-af803d68bb89@tarantool.org> From: Sergey Bronnikov Message-ID: <8fb4180a-cd38-e138-15c7-e1e600a28d3e@tarantool.org> Date: Thu, 19 Nov 2020 12:44:23 +0300 MIME-Version: 1.0 In-Reply-To: <2817d4c9-0ce6-c6b0-75d3-af803d68bb89@tarantool.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [test-run] Add options for upgrade testing List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Leonid Vasiliev , Alexander Turenko Cc: tarantool-patches@dev.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