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 7BD75469719 for ; Tue, 15 Sep 2020 10:46:38 +0300 (MSK) References: From: Serge Petrenko Message-ID: Date: Tue, 15 Sep 2020 10:46:37 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-GB Subject: Re: [Tarantool-patches] [PATCH v2 3/4] replication: add is_anon flag to ballot List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy , tarantool-patches@dev.tarantool.org, gorcunov@gmail.com 15.09.2020 02:11, Vladislav Shpilevoy пишет: > Ballot is a message sent in response on vote request, which is > sent by applier first thing after connection establishment. > > It contains basic info about the remote instance such as whether > it is read only, if it is still loading, and more. > > The ballot didn't contain a flag whether the instance is > anonymous. That led to a problem, when applier was connected to a > remote instance, was added to struct replicaset inside a struct > replica object, but it was unknown whether it is anonymous. It was > added as not anonymous by default. > > If the remote instance was in fact anonymous and sent a subscribe > response back to the first instance with the anon flag = true, > then it looked like the remote instance was not anonymous, and > suddenly became such, without even a reconnect. It could lead to > an assertion. > > The bug is hidden behind another bug, because of which the leader > instance on boostrap registers all replicas listed in its > box.cfg.replication, even anonymous ones. > > The patch makes the ballot contain the anon flag. Now both relay > and applier send whether their host is anonymous. Relay does it by > sending the ballot, applier sends it in scope of subscribe > request. By the time a replica gets UUID and is added into struct > replicaset, its anon flag is determined. > > Also the patch makes anon_count updated on each replica hash table > change. Previously it was only updated when something related to > relay was done. Now anon is updated by applier actions too, and > it is not ok to update the counter on relay-specific actions. > > The early registration bug is a subject for a next patch. > > Part of #5287 > > @TarantoolBot document > Title: IPROTO_BALLOT_IS_ANON flag > > There is a request type IPROTO_BALLOT, with code 0x29. It has > fields IPROTO_BALLOT_IS_RO (0x01), IPROTO_BALLOT_VCLOCK (0x02), > IPROTO_BALLOT_GC_VCLOCK (0x03), IPROTO_BALLOT_IS_LOADING (0x04). > > Now it gets a new field IPROTO_BALLOT_IS_ANON (0x05). The field > is a boolean, and equals to box.cfg.replication_anon of the > sender. > --- Hi! Thanks for the patch! Please see one comment below. > src/box/box.cc | 1 + > src/box/iproto_constants.h | 1 + > src/box/replication.cc | 14 ++++++++++++-- > src/box/xrow.c | 14 ++++++++++++-- > src/box/xrow.h | 5 +++++ > 5 files changed, 31 insertions(+), 4 deletions(-) > > diff --git a/src/box/box.cc b/src/box/box.cc > index faffd5769..145b53ec8 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -2080,6 +2080,7 @@ void > box_process_vote(struct ballot *ballot) > { > ballot->is_ro = cfg_geti("read_only") != 0; > + ballot->is_anon = replication_anon; > /* > * is_ro is true on initial load and is set to box.cfg.read_only > * after box_cfg() returns, during dynamic box.cfg parameters setting. > diff --git a/src/box/iproto_constants.h b/src/box/iproto_constants.h > index 4f5a2b195..9c2fb6058 100644 > --- a/src/box/iproto_constants.h > +++ b/src/box/iproto_constants.h > @@ -149,6 +149,7 @@ enum iproto_ballot_key { > IPROTO_BALLOT_VCLOCK = 0x02, > IPROTO_BALLOT_GC_VCLOCK = 0x03, > IPROTO_BALLOT_IS_LOADING = 0x04, > + IPROTO_BALLOT_IS_ANON = 0x05, > }; > > #define bit(c) (1ULL< diff --git a/src/box/replication.cc b/src/box/replication.cc > index ef0e2411d..8408f395e 100644 > --- a/src/box/replication.cc > +++ b/src/box/replication.cc > @@ -290,6 +290,8 @@ replica_clear_id(struct replica *replica) > } > if (replica_is_orphan(replica)) { > replica_hash_remove(&replicaset.hash, replica); > + replicaset.anon_count -= replica->anon; > + assert(replicaset.anon_count >= 0); replica_clear_id() is only called on _cluster entry deletion. So you're surely working with a normal replica, not anonymous. We may add an assset(!replica->anon) somewhere. Don't know if it's really needed though. > replica_delete(replica); > } > } > @@ -332,6 +334,7 @@ replica_on_applier_connect(struct replica *replica) > assert(replica->applier_sync_state == APPLIER_DISCONNECTED); > > replica->uuid = applier->uuid; > + replica->anon = applier->ballot.is_anon; > replica->applier_sync_state = APPLIER_CONNECTED; > replicaset.applier.connected++; > > @@ -362,6 +365,7 @@ replica_on_applier_connect(struct replica *replica) > } else { > /* Add a new struct replica */ > replica_hash_insert(&replicaset.hash, replica); > + replicaset.anon_count += replica->anon; > } > } > > @@ -390,7 +394,9 @@ replica_on_applier_reconnect(struct replica *replica) > if (orig == NULL) { > orig = replica_new(); > orig->uuid = applier->uuid; > + orig->anon = applier->ballot.is_anon; > replica_hash_insert(&replicaset.hash, orig); > + replicaset.anon_count += orig->anon; > } > > if (orig->applier != NULL) { > @@ -526,6 +532,7 @@ replicaset_update(struct applier **appliers, int count) > > assert(!tt_uuid_is_nil(&applier->uuid)); > replica->uuid = applier->uuid; > + replica->anon = applier->ballot.is_anon; > > if (replica_hash_search(&uniq, replica) != NULL) { > replica_clear_applier(replica); > @@ -582,6 +589,7 @@ replicaset_update(struct applier **appliers, int count) > } else { > /* Add a new struct replica */ > replica_hash_insert(&replicaset.hash, replica); > + replicaset.anon_count += replica->anon; > } > > replica->applier_sync_state = APPLIER_CONNECTED; > @@ -593,6 +601,8 @@ replicaset_update(struct applier **appliers, int count) > replica_hash_foreach_safe(&replicaset.hash, replica, next) { > if (replica_is_orphan(replica)) { > replica_hash_remove(&replicaset.hash, replica); > + replicaset.anon_count -= replica->anon; > + assert(replicaset.anon_count >= 0); > replica_delete(replica); > } > } > @@ -907,12 +917,12 @@ replica_on_relay_stop(struct replica *replica) > replica->gc = NULL; > } else { > assert(replica->gc == NULL); > - assert(replicaset.anon_count > 0); > - replicaset.anon_count--; > } > } > if (replica_is_orphan(replica)) { > replica_hash_remove(&replicaset.hash, replica); > + replicaset.anon_count -= replica->anon; > + assert(replicaset.anon_count >= 0); > replica_delete(replica); > } > } > diff --git a/src/box/xrow.c b/src/box/xrow.c > index 95ddb1fe7..2edcb2f8f 100644 > --- a/src/box/xrow.c > +++ b/src/box/xrow.c > @@ -447,9 +447,11 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot, > uint64_t sync, uint32_t schema_version) > { > size_t max_size = IPROTO_HEADER_LEN + mp_sizeof_map(1) + > - mp_sizeof_uint(UINT32_MAX) + mp_sizeof_map(4) + > + mp_sizeof_uint(UINT32_MAX) + mp_sizeof_map(5) + > mp_sizeof_uint(UINT32_MAX) + mp_sizeof_bool(ballot->is_ro) + > mp_sizeof_uint(UINT32_MAX) + mp_sizeof_bool(ballot->is_loading) + > + mp_sizeof_uint(IPROTO_BALLOT_IS_ANON) + > + mp_sizeof_bool(ballot->is_anon) + > mp_sizeof_uint(UINT32_MAX) + > mp_sizeof_vclock_ignore0(&ballot->vclock) + > mp_sizeof_uint(UINT32_MAX) + > @@ -465,11 +467,13 @@ iproto_reply_vote(struct obuf *out, const struct ballot *ballot, > char *data = buf + IPROTO_HEADER_LEN; > data = mp_encode_map(data, 1); > data = mp_encode_uint(data, IPROTO_BALLOT); > - data = mp_encode_map(data, 4); > + data = mp_encode_map(data, 5); > data = mp_encode_uint(data, IPROTO_BALLOT_IS_RO); > data = mp_encode_bool(data, ballot->is_ro); > data = mp_encode_uint(data, IPROTO_BALLOT_IS_LOADING); > data = mp_encode_bool(data, ballot->is_loading); > + data = mp_encode_uint(data, IPROTO_BALLOT_IS_ANON); > + data = mp_encode_bool(data, ballot->is_anon); > data = mp_encode_uint(data, IPROTO_BALLOT_VCLOCK); > data = mp_encode_vclock_ignore0(data, &ballot->vclock); > data = mp_encode_uint(data, IPROTO_BALLOT_GC_VCLOCK); > @@ -1227,6 +1231,7 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot) > { > ballot->is_ro = false; > ballot->is_loading = false; > + ballot->is_anon = false; > vclock_create(&ballot->vclock); > > const char *start = NULL; > @@ -1276,6 +1281,11 @@ xrow_decode_ballot(struct xrow_header *row, struct ballot *ballot) > goto err; > ballot->is_loading = mp_decode_bool(&data); > break; > + case IPROTO_BALLOT_IS_ANON: > + if (mp_typeof(*data) != MP_BOOL) > + goto err; > + ballot->is_anon = mp_decode_bool(&data); > + break; > case IPROTO_BALLOT_VCLOCK: > if (mp_decode_vclock_ignore0(&data, > &ballot->vclock) != 0) > diff --git a/src/box/xrow.h b/src/box/xrow.h > index 58d47b12d..0dc9eb71a 100644 > --- a/src/box/xrow.h > +++ b/src/box/xrow.h > @@ -332,6 +332,11 @@ xrow_encode_auth(struct xrow_header *row, const char *salt, size_t salt_len, > struct ballot { > /** Set if the instance is running in read-only mode. */ > bool is_ro; > + /** > + * A flag whether the instance is anonymous, not having an > + * ID, and not going to request it. > + */ > + bool is_anon; > /** > * Set if the instance hasn't finished bootstrap or recovery, or > * is syncing with other replicas in the replicaset. -- Serge Petrenko