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

[Leonid Vasiliev <lvasiliev@tarantool.org>]

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