From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id B0B7C2795E for ; Fri, 23 Aug 2019 11:33:59 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id qOxcPKeGvjhB for ; Fri, 23 Aug 2019 11:33:59 -0400 (EDT) Received: from smtp50.i.mail.ru (smtp50.i.mail.ru [94.100.177.110]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 169F327958 for ; Fri, 23 Aug 2019 11:33:58 -0400 (EDT) Received: from [185.6.245.156] (port=52087 helo=[100.96.163.205]) by smtp50.i.mail.ru with esmtpa (envelope-from ) id 1i1BZc-0000tK-Tm for tarantool-patches@freelists.org; Fri, 23 Aug 2019 18:33:57 +0300 From: Serge Petrenko Content-Type: multipart/alternative; boundary="Apple-Mail=_865B5DAC-7BF3-42A3-A2CF-1F0A28B4B40B" Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.11\)) Subject: [tarantool-patches] Re: [PATCH] replication: enter orphan mode on every erroneous config change Date: Fri, 23 Aug 2019 18:33:56 +0300 References: <20190819121101.29815-1-sergepetrenko@tarantool.org> <20190821094455.GX13834@esperanza> In-Reply-To: <20190821094455.GX13834@esperanza> Message-Id: <2D6789E2-1CE2-4512-849F-AE6CCEE4B689@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org --Apple-Mail=_865B5DAC-7BF3-42A3-A2CF-1F0A28B4B40B Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi! Thank you for review. I=E2=80=99ve addressed your comments in v2. Please, check it out > 21 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 12:44, Vladimir Davydov = =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0=B0=D0=BB(=D0=B0= ): >=20 > On Mon, Aug 19, 2019 at 03:11:01PM +0300, Serge Petrenko wrote: >> We only entered orphan mode on bootrap and local recovery, but threw = an >> error when replicaton config was changed on the fly. >> For consistency, in this case we should also enter orphan mode when >> an instance fails to connect to quorum remote instances. >>=20 >> Closes #4424 >=20 > A doc bot request is probably required. >=20 >> --- >> https://github.com/tarantool/tarantool/issues/4424 >> = https://github.com/tarantool/tarantool/tree/sp/gh-4424-repl-config-errors >>=20 >> src/box/box.cc | 9 ++-- >> src/box/replication.cc | 30 ++++++------- >> src/box/replication.h | 2 +- >> test/replication/misc.result | 77 = ++++++++++++++++++++++++++++++++-- >> test/replication/misc.test.lua | 29 ++++++++++++- >> 5 files changed, 125 insertions(+), 22 deletions(-) >>=20 >> diff --git a/src/box/box.cc b/src/box/box.cc >> index 66cd6d3a4..43cc32d87 100644 >> --- a/src/box/box.cc >> +++ b/src/box/box.cc >> @@ -666,7 +666,7 @@ cfg_get_replication(int *p_count) >> * Sync box.cfg.replication with the cluster registry, but >> * don't start appliers. >> */ >> -static void >> +static int >> box_sync_replication(bool connect_quorum) >> { >> int count =3D 0; >> @@ -679,9 +679,10 @@ box_sync_replication(bool connect_quorum) >> applier_delete(appliers[i]); /* doesn't affect = diag */ >> }); >>=20 >> - replicaset_connect(appliers, count, connect_quorum); >> + int rc =3D replicaset_connect(appliers, count, connect_quorum); >=20 > A function typically returns -1 as an error condition, but in this = case > -1 means something different. Besdies, this function can also throw an > exception, and there isn't a single word in the comment regarding it, > which is confusing. >=20 > Can you somehow manage without using a return code? >=20 >>=20 >> guard.is_active =3D false; >> + return rc; >> } >>=20 >> void >> @@ -703,7 +704,9 @@ box_set_replication(void) >> * to connect to at least replication_connect_quorum >> * masters. >> */ >> - box_sync_replication(true); >> + if (box_sync_replication(true) !=3D 0) { >> + return; >> + } >=20 > The comment above is clearly outdated by this patch. >=20 >> /* Follow replica */ >> replicaset_follow(); >> /* Wait until appliers are in sync */ >> diff --git a/src/box/replication.cc b/src/box/replication.cc >> index 28f7acedc..e9d0a9206 100644 >> --- a/src/box/replication.cc >> +++ b/src/box/replication.cc >> @@ -598,14 +598,14 @@ applier_on_connect_f(struct trigger *trigger, = void *event) >> applier_pause(applier); >> } >>=20 >> -void >> +int >> replicaset_connect(struct applier **appliers, int count, >> bool connect_quorum) >> { >> if (count =3D=3D 0) { >> /* Cleanup the replica set. */ >> replicaset_update(appliers, count); >> - return; >> + return 0; >> } >>=20 >> say_info("connecting to %d replicas", count); >> @@ -660,9 +660,13 @@ replicaset_connect(struct applier **appliers, = int count, >> count - state.connected, count); >> /* Timeout or connection failure. */ >> if (connect_quorum && state.connected < quorum) { >> - diag_set(ClientError, ER_CFG, "replication", >> - "failed to connect to one or more = replicas"); >> - goto error; >> + /* Destroy appliers */ >> + for (int i =3D 0; i < count; i++) { >> + trigger_clear(&triggers[i].base); >> + applier_stop(appliers[i]); >> + } >> + box_set_orphan(true); >> + return -1; >> } >> } else { >> say_info("connected to %d replicas", state.connected); >> @@ -685,16 +689,14 @@ replicaset_connect(struct applier **appliers, = int count, >> try { >> replicaset_update(appliers, count); >> } catch (Exception *e) { >> - goto error; >> - } >> - return; >> -error: >> - /* Destroy appliers */ >> - for (int i =3D 0; i < count; i++) { >> - trigger_clear(&triggers[i].base); >> - applier_stop(appliers[i]); >> + /* Destroy appliers */ >> + for (int i =3D 0; i < count; i++) { >> + trigger_clear(&triggers[i].base); >> + applier_stop(appliers[i]); >> + } >=20 > Copy-and-paste... Let's please try to avoid that. I'd prefer if you > called replicaset_follow and _sync in any case and let them decide > if the instance should enter the orphan state. >=20 >> + diag_raise(); >> } >> - diag_raise(); >> + return 0; >> } >>=20 >> bool >> diff --git a/test/replication/misc.test.lua = b/test/replication/misc.test.lua >> index 99e995509..91a77b584 100644 >> --- a/test/replication/misc.test.lua >> +++ b/test/replication/misc.test.lua >> @@ -9,6 +9,8 @@ replication_timeout =3D box.cfg.replication_timeout >> replication_connect_timeout =3D box.cfg.replication_connect_timeout >> box.cfg{replication_timeout=3D0.05, replication_connect_timeout=3D0.05,= replication=3D{}} >> box.cfg{replication =3D {'127.0.0.1:12345', box.cfg.listen}} >> +box.info.status >> +box.info.ro >>=20 >> -- gh-3606 - Tarantool crashes if box.cfg.replication is updated = concurrently >> fiber =3D require('fiber') >> @@ -19,7 +21,9 @@ f() >> c:get() >> c:get() >>=20 >> -box.cfg{replication_timeout =3D replication_timeout, = replication_connect_timeout =3D replication_connect_timeout} >> +box.cfg{replication =3D "", replication_timeout =3D = replication_timeout, replication_connect_timeout =3D = replication_connect_timeout} >> +box.info.status >> +box.info.ro >>=20 >> -- gh-3111 - Allow to rebootstrap a replica from a read-only master >> replica_uuid =3D uuid.new() >> @@ -293,3 +297,26 @@ test_run:cmd("cleanup server replica") >> test_run:cmd("delete server replica") >> test_run:cleanup_cluster() >> box.schema.user.revoke('guest', 'replication') >> + >> +-- >> +-- gh-4424 Always enter orphan mode on error in replication >> +-- configuration change. >> +-- >> +replication_connect_timeout =3D box.cfg.replication_connect_timeout >> +replication_connect_quorum =3D box.cfg.replication_connect_quorum >> + >> +box.cfg{replication=3D"12345", replication_connect_timeout=3D0.1, = replication_connect_quorum=3D1} >=20 > Let's please also test a more sophisticated use case: when there are = two > replicas and we manage to connect to one of them but fail to connect = to > another. >=20 >> +box.info.status >> +box.info.ro >> +box.cfg{replication =3D ""} >> +box.info.status >> +box.info.ro >> +-- no switch to orphan when quorum =3D=3D 0 >> +box.cfg{replication=3D "12345", replication_connect_quorum=3D0} >> +box.info.status >> +box.info.ro >> +test_run:cmd("setopt delimiter ';'") >> +box.cfg{replication=3D"", >> + replication_connect_timeout=3Dreplication_connect_timeout, >> + replication_connect_quorum=3Dreplication_connect_quorum}; >> +test_run:cmd("setopt delimiter ''"); >=20 > Nit: I'd rewrite it without using 'setopt delimiter':: >=20 > box.cfg{replication =3D {}} > box.cfg{replication_connect_timeout =3D replication_connect_timeout) > box.cfg{replication_connect_quorum =3D replication_connect_quorum) -- Serge Petrenko sergepetrenko@tarantool.org --Apple-Mail=_865B5DAC-7BF3-42A3-A2CF-1F0A28B4B40B Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi! = Thank you for review.
I=E2=80=99ve addressed your = comments in v2. Please, check it out


21 =D0=B0=D0=B2=D0=B3. 2019 =D0=B3., =D0=B2 12:44, Vladimir = Davydov <vdavydov.dev@gmail.com> =D0=BD=D0=B0=D0=BF=D0=B8=D1=81=D0= =B0=D0=BB(=D0=B0):

On Mon, Aug = 19, 2019 at 03:11:01PM +0300, Serge Petrenko wrote:
We = only entered orphan mode on bootrap and local recovery, but threw an
error when replicaton config was changed on the fly.
For consistency, in this case we should also enter orphan = mode when
an instance fails to connect to quorum remote = instances.

Closes #4424

A doc bot request is probably required.

---
https://github.com/tarantool/tarantool/issues/4424
https://github.com/tarantool/tarantool/tree/sp/gh-4424-repl-con= fig-errors

src/box/box.cc =             &n= bsp;   |  9 ++--
src/box/replication.cc =         | 30 ++++++-------
src/box/replication.h =          |  2 +-
test/replication/misc.result   | 77 = ++++++++++++++++++++++++++++++++--
test/replication/misc.test.lua | 29 ++++++++++++-
5 files changed, 125 insertions(+), 22 deletions(-)

diff --git a/src/box/box.cc = b/src/box/box.cc
index 66cd6d3a4..43cc32d87 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -666,7 +666,7 @@ cfg_get_replication(int *p_count)
 * Sync box.cfg.replication with the cluster registry, = but
 * don't start appliers.
 */
-static void
+static int
box_sync_replication(bool connect_quorum)
{
= int count =3D 0;
@@ -679,9 +679,10 @@ = box_sync_replication(bool connect_quorum)
= applier_delete(appliers[i]); /* doesn't affect diag */
= });

- replicaset_connect(appliers, = count, connect_quorum);
+ int rc =3D = replicaset_connect(appliers, count, connect_quorum);

A function typically returns -1 as an error condition, but in = this case
-1 means = something different. Besdies, this function can also throw an
exception, and there isn't a = single word in the comment regarding it,
which is confusing.

Can you somehow manage without using a return code?


= guard.is_active =3D false;
+ return = rc;
}

void
@@ = -703,7 +704,9 @@ box_set_replication(void)
 * to connect to at least = replication_connect_quorum
 * masters.
=  */
- = box_sync_replication(true);
+ if = (box_sync_replication(true) !=3D 0) {
+ = return;
+ }

The comment above is clearly = outdated by this patch.

/* Follow replica */
= replicaset_follow();
/* Wait until appliers are in = sync */
diff --git a/src/box/replication.cc = b/src/box/replication.cc
index 28f7acedc..e9d0a9206 = 100644
--- a/src/box/replication.cc
+++ b/src/box/replication.cc
@@ -598,14 +598,14 @@ applier_on_connect_f(struct trigger = *trigger, void *event)
applier_pause(applier);
}

-void
+int
replicaset_connect(struct applier **appliers, int count,
= =    bool= connect_quorum)
{
if (count = =3D=3D 0) {
/* Cleanup the replica set. */
= = replicaset_update(appliers, count);
- = return;
+ return 0;
}

say_info("connecting to %d = replicas", count);
@@ -660,9 +660,13 @@ = replicaset_connect(struct applier **appliers, int count,
= = =  count - = state.connected, count);
/* Timeout or connection failure. = */
if (connect_quorum && state.connected < = quorum) {
- diag_set(ClientError, ER_CFG, = "replication",
-  "failed to connect to one = or more replicas");
- goto error;
+ /* = Destroy appliers */
+ for (int i =3D 0; i < count; = i++) {
+ trigger_clear(&triggers[i].base);
+ = applier_stop(appliers[i]);
+ }
+ = = = box_set_orphan(true);
+ return -1;
}
= } else {
say_info("connected to %d = replicas", state.connected);
@@ -685,16 +689,14 @@ = replicaset_connect(struct applier **appliers, int count,
= try {
replicaset_update(appliers, = count);
} catch (Exception *e) {
- goto = error;
- }
- return;
-error:
- = /* Destroy appliers */
- for (int = i =3D 0; i < count; i++) {
- = trigger_clear(&triggers[i].base);
- = applier_stop(appliers[i]);
+ /* = Destroy appliers */
+ for (int i =3D 0; i < count; = i++) {
+ trigger_clear(&triggers[i].base);
+ = applier_stop(appliers[i]);
+ }

Copy-and-paste... Let's please try to avoid that. I'd prefer = if you
called = replicaset_follow and _sync in any case and let them decide
if the instance should enter the = orphan state.

+ = diag_raise();
}
- = diag_raise();
+ return 0;
}

bool
diff --git = a/test/replication/misc.test.lua b/test/replication/misc.test.lua
index 99e995509..91a77b584 100644
--- = a/test/replication/misc.test.lua
+++ = b/test/replication/misc.test.lua
@@ -9,6 +9,8 @@ = replication_timeout =3D box.cfg.replication_timeout
replication_connect_timeout =3D = box.cfg.replication_connect_timeout
box.cfg{replication_timeout=3D0.05, = replication_connect_timeout=3D0.05, replication=3D{}}
box.cfg{replication =3D {'127.0.0.1:12345', = box.cfg.listen}}
+box.info.status
+box.info.ro

-- gh-3606 - Tarantool crashes if box.cfg.replication is = updated concurrently
fiber =3D require('fiber')
@@ -19,7 +21,9 @@ f()
c:get()
c:get()

-box.cfg{replication_timeout =3D replication_timeout, = replication_connect_timeout =3D replication_connect_timeout}
+box.cfg{replication =3D "", replication_timeout =3D = replication_timeout, replication_connect_timeout =3D = replication_connect_timeout}
+box.info.status
+box.info.ro

-- gh-3111 - Allow to rebootstrap a replica = from a read-only master
replica_uuid =3D uuid.new()
@@ -293,3 +297,26 @@ test_run:cmd("cleanup server = replica")
test_run:cmd("delete server replica")
test_run:cleanup_cluster()
box.schema.user.revoke('guest', 'replication')
+
+--
+-- gh-4424 Always enter = orphan mode on error in replication
+-- configuration = change.
+--
+replication_connect_timeout =3D = box.cfg.replication_connect_timeout
+replication_connect_quorum =3D = box.cfg.replication_connect_quorum
+
+box.cfg{replication=3D"12345", = replication_connect_timeout=3D0.1, replication_connect_quorum=3D1}

Let's please also test a more sophisticated use case: when = there are two
replicas and = we manage to connect to one of them but fail to connect to
another.

+box.info.status
+box.info.ro
+box.cfg{replication =3D = ""}
+box.info.status
+box.info.ro
+-- = no switch to orphan when quorum =3D=3D 0
+box.cfg{replication=3D "12345", = replication_connect_quorum=3D0}
+box.info.status
+box.info.ro
+test_run:cmd("setopt delimiter ';'")
+box.cfg{replication=3D"",
+ =        = replication_connect_timeout=3Dreplication_connect_timeout,
+=        = replication_connect_quorum=3Dreplication_connect_quorum};
+test_run:cmd("setopt delimiter ''");

Nit: I'd rewrite it without using 'setopt = delimiter'::

box.cfg{replication =3D {}}
box.cfg{replication_connect_timeout =3D = replication_connect_timeout)
box.cfg{replication_connect_quorum =3D = replication_connect_quorum)


--
Serge = Petrenko


= --Apple-Mail=_865B5DAC-7BF3-42A3-A2CF-1F0A28B4B40B--