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

[Sergey Bronnikov <sergeyb@tarantool.org>]

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.