From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [94.100.177.105]) (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 5117B469710 for ; Fri, 20 Nov 2020 22:27:25 +0300 (MSK) References: <20201116164006.fw3jwes4dwbx7nsd@tkn_work_nb> <8365255bb9eef01293f66d9a7293730fecc49e2b.1605691680.git.sergeyb@tarantool.org> <20201119225859.evkmxyki4xbxm7aa@tkn_work_nb> From: Sergey Bronnikov Message-ID: <416b34e1-8fd6-07be-3a7e-fae01170e447@tarantool.org> Date: Fri, 20 Nov 2020 22:27:23 +0300 MIME-Version: 1.0 In-Reply-To: <20201119225859.evkmxyki4xbxm7aa@tkn_work_nb> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Tarantool-patches] [PATCH] Add options for upgrade testing List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Turenko Cc: tarantool-patches@dev.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 >> >> 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.