[Tarantool-patches] [test-run] Add options for upgrade testing
Sergey Bronnikov
sergeyb at tarantool.org
Thu Nov 19 12:44:23 MSK 2020
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
More information about the Tarantool-patches
mailing list