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

[Sergey Bronnikov <sergeyb@tarantool.org>]

Hi, Alexander!

On 20.11.2020 01:58, Alexander Turenko wrote:
> Not a review.
>
> On Wed, Nov 18, 2020 at 12:30:49PM +0300, sergeyb@tarantool.org wrote:
>> From: Sergey Bronnikov <sergeyb@tarantool.org>
>>
>> Option '--snapshot' 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.
>>
>> Closes https://github.com/tarantool/tarantool/issues/4801
> This change cannot close the issue in tarantool. We need to look at all
> variants of bootstrap snapshot we had over the history of supported
> release branches (and maybe a bit deeper, down to some 1.9) and manifest
> compatibility matrix between given data layouts and runtimes ([1]). Once
> the support matrix will be defined, we should catch all test fails,
> analyze and categorize them. File issue for real problems, exclude
> irrelevant fails. Setup CI. It is not a small task.

You are right. Earlier we discussed that with you and new ticket has 
been submitted [1].

1. https://github.com/tarantool/tarantool/issues/5527

>
> [1]: https://github.com/tarantool/doc/issues/836
>
> I think we should file an issue to test-run, solve it within our
> planning iteration and estimate #4801 for the next iteration.
>
> While I thinking around the task, the idea how to solve the problem with
> replication tests comes to me. I know, know. I should have to say it
> before you start the task, but I don't know everything in advance,
> learning together with you and do my best.

Discussed it verbally and submitted two tickets:

- https://github.com/tarantool/tarantool/issues/5540

- https://github.com/tarantool/test-run/issues/239

>
> Back to the idea. We can implement an option or errinj to pass a path to
> bootstrap snapshot to tarantool and feed it instead the built-in
> bootstrap.snap. This way we'll go to the right branch here:
>
>   [src/box/box.cc; box_cfg_xc()]
>
>   | if (checkpoint != NULL) {
>   |         /* Recover the instance from the local directory */
>   |         local_recovery(&instance_uuid, &replicaset_uuid,
>   |                        &checkpoint->vclock);
>   | } else {
>   |         /* Bootstrap a new instance */
>   |         bootstrap(&instance_uuid, &replicaset_uuid,
>   |                   &is_bootstrap_leader);
>   | }
>
> And so will land here:
>
>   [src/box/memtx_engine.c; memtx_engine_bootstrap()]
>
>   | /* Recover from bootstrap.snap */
>   | say_info("initializing an empty data directory");
>   | struct xlog_cursor cursor;
>   | if (xlog_cursor_openmem(&cursor, (const char *)bootstrap_bin,
>   |                         sizeof(bootstrap_bin), "bootstrap") < 0)
>   |         return -1;
>
> It is the place, where we should give our custom snapshot instead of
> `bootstrap_bin` (which contains src/box/bootstrap.snap content).
>
> I would call the test-run option that feeds a snapshot to tarantool this
> way --bootstrap instead of --snapshot to don't confuse the local
> recovery process with bootstrapping from an empty directory.

Updated in a branch:

--- a/lib/options.py
+++ b/lib/options.py
@@ -210,17 +210,17 @@ class Options:
                  Default: false.""")

          parser.add_argument(
-                "--snapshot",
+                "--bootstrap",
                  dest='snapshot_path',
-                help="""Path to a Tarantool snapshot that will be
-                loaded before testing.""")
+                help="""Path to snapshot that will be used to boostrap
+                Tarantool 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.""")
+                help="""Disable schema upgrade on testing with 
bootstrap snapshots.""")

          # XXX: We can use parser.parse_intermixed_args() on
          # Python 3.7 to understand commands like
@@ -242,7 +242,7 @@ class Options:

          snapshot_path = getattr(self.args, 'snapshot_path', '')
          if self.args.disable_schema_upgrade and not snapshot_path:
-            color_stdout("\nOption --disable-schema-upgrade requires 
--snapshot\n",
+            color_stdout("\nOption --disable-schema-upgrade requires 
--boostrap\n",
                           schema='error')
              check_error = True

>
> Let's discuss tasks, planning, worthiness of eating a pie step-by-step
> voicely.