*: fix node bootstrap not idempotent #1774

nolouch · 2017-04-17T02:44:51Z

CLAassistant · 2017-04-17T02:44:57Z

All committers have signed the CLA.

siddontang · 2017-04-17T03:29:29Z

please add test for the bootstrap flow.

siddontang · 2017-04-17T07:26:58Z

src/server/node.rs

+        for _ in 0..MAX_CHECK_CLUSTER_BOOTSTRAPPED_RETRY_COUNT {
+            match self.pd_client.get_region(b"") {
+                Ok(region) => {
+                    if region.get_id() == region_id {


I think we must check region epoch, if not equal, there must be some fatal error here.

siddontang · 2017-04-18T06:25:51Z

any update? @nolouch

disksing · 2017-04-18T06:42:23Z

src/server/node.rs

    fn bootstrap_cluster(&mut self, engine: &DB, region: metapb::Region) -> Result<()> {
        let region_id = region.get_id();
        match self.pd_client.bootstrap_cluster(self.store.clone(), region) {
            Err(PdError::ClusterBootstrapped(_)) => {
                error!("cluster {} is already bootstrapped", self.cluster_id);
-                try!(store::clear_region(engine, region_id));
+                try!(store::clear_prepare_bootstrap(engine, region_id));


I guess you want to clear_prepare_bootstrap_state() if pd returns Ok() at L237.

you are right

siddontang · 2017-04-20T01:24:02Z

src/raftstore/store/bootstrap.rs

+pub fn clear_prepare_bootstrap_state(engine: &DB) -> Result<()> {
+    let wb = WriteBatch::new();
+    try!(wb.delete(&keys::prepare_bootstrap_key()));
+    try!(engine.write(wb));


use engine.delete directly.

siddontang · 2017-04-20T01:24:22Z

src/raftstore/store/keys.rs

@@ -60,6 +61,9 @@ pub const REGION_STATE_SUFFIX: u8 = 0x01;
 pub fn store_ident_key() -> Vec<u8> {
    STORE_IDENT_KEY.to_vec()
 }
+pub fn prepare_bootstrap_key() -> Vec<u8> {


add a blank line

siddontang · 2017-04-20T01:25:55Z

src/server/node.rs

+                Ok(region) => {
+                    if region.get_id() == first_region.get_id() {
+                        if !self.check_region_epoch(region.clone(), first_region.clone()) {
+                            return Err(box_err!("first region epoch inconsistent with pd info"));


add more info here.

siddontang · 2017-04-20T01:27:14Z

src/server/node.rs

                Ok(())
            }
            // TODO: should we clean region for other errors too?
            Err(e) => panic!("bootstrap cluster {} err: {:?}", self.cluster_id, e),
            Ok(_) => {
+                if let Err(e) = store::clear_prepare_bootstrap_state(engine) {
+                    warn!("clear prepare bootstrap state failed: {:?}", e);


return error

siddontang · 2017-04-20T01:29:51Z

src/server/node.rs

+            match self.pd_client.get_region(b"") {
+                Ok(region) => {
+                    if region.get_id() == first_region.get_id() {
+                        if !self.check_region_epoch(region.clone(), first_region.clone()) {


you can use & here, no need to clone

disksing · 2017-04-20T02:53:39Z

tests/raftstore/test_bootstrap.rs

+    // assume that there is a node  bootstrap the cluster and add region in pd successfully
+    cluster.add_first_region().unwrap();
+    // now  at same time start the another node, and will recive cluster is not bootstrap
+    // try to bootstrap with a new region


I think we need to test both when pd is bootstrapped and pd is not bootstrapped.

I start the cluster two times, in the second time the cluster is bootstrapped.

nolouch · 2017-04-20T10:41:51Z

PTAL @siddontang @disksing

disksing · 2017-04-20T11:05:27Z

LGTM.

andelf

LGTM

siddontang · 2017-04-22T07:04:22Z

tests/raftstore/pd.rs

@@ -362,6 +373,10 @@ impl TestPdClient {
        Ok(())
    }

+    fn is_regions_empty(&self) -> Result<(bool)> {


need Result here?

siddontang · 2017-04-22T07:05:52Z

tests/raftstore/test_bootstrap.rs

+use super::node::new_node_cluster;
+use super::util::*;
+
+fn test_bootstrap_idempotent<T: Simulator>(cluster: &mut Cluster<T>) {


can we check conf ver and version inconsistent for check_region_epoch?

any update? @nolouch

siddontang · 2017-04-22T07:08:01Z

tests/raftstore/test_bootstrap.rs

+    cluster.check_regions_number(1);
+    cluster.shutdown();
+    sleep_ms(500);
+    cluster.start();


start twice to check what?

I think we must check the prepare bootstrap key in RocksDB too.

any update?

first the cluster is not bootstraped. second is bootstraped.

For the first start, check the bootstrap key in RocksDB directly

For the second start, check again too.

I have checked it in here https://github.com/pingcap/tikv/pull/1774/files#diff-7d211b12fe1e345db17e9dbfb322abd3R186

no, not the same.
Here we must check the prepare key in RocksDB explicitly.

… into shuning/fix-bootstrap

siddontang · 2017-04-25T08:34:31Z

@nolouch

CI failed

thread 'raftstore::test_bootstrap::test_node_bootstrap_witch_check_epoch' panicked at 'called `Result::unwrap()` on an `Err` value: Other(StringError("region version inconsist: 1 with 2"))', /checkout/src/libcore/result.rs:859

nolouch · 2017-04-25T12:28:45Z

PTAL @siddontang

siddontang · 2017-04-26T02:07:26Z

tests/raftstore/test_bootstrap.rs

+
+    // to check meet inconsistent epoch when bootstrap，will meet error in here
+    let e = sim.wl()
+        .run_node_with_handle_error(1, cluster.cfg.clone(), engine.clone())


if you only want to check epoch function work right or not, you don't need to do it.

You can just extract the check_epoch function to a common function, not in Node, then test it directly.

IMO, your test is complex and not easy to understand.

… into shuning/fix-bootstrap

siddontang · 2017-04-26T07:55:53Z

Ping @nolouch

siddontang · 2017-04-26T09:36:32Z

src/server/node.rs

+        r3.set_end_key(keys::EMPTY_KEY.to_vec());
+        r3.mut_region_epoch().set_version(1);
+        r3.mut_region_epoch().set_conf_ver(2);
+        match check_region_epoch(&r1, &r2).unwrap_err() {


you check use assert!(check_region_epoch().is_err()) directly.

siddontang · 2017-04-26T09:37:16Z

tests/raftstore/cluster.rs

+        match self.sim.try_read() {
+            Ok(s) => keys = s.get_node_ids(),
+            Err(sync::TryLockError::Poisoned(e)) => {
+                let s = e.into_inner();


why do we need to check this?

I met panic on panic and test core dump. because test case panic and then run drop will call shutdown, and the Lock reason will panic again. this check to prevent it and catch the first panic stack information.

… into shuning/fix-bootstrap

siddontang · 2017-04-27T11:59:32Z

LGTM

PTAL @disksing

disksing · 2017-04-28T02:49:31Z

tests/raftstore/test_bootstrap.rs

+                                    cfg.raft_store.use_sst_file_snapshot);
+
+
+    // assume there is a node has bootstraped the cluster and add region in pd successfully


bootstrapped

disksing · 2017-04-28T02:52:12Z

tests/raftstore/test_bootstrap.rs

+
+#[test]
+fn test_bootstrap_with_prepared_data() {
+    test_node_bootstrap_with_prepared_data();


Why not add #[test] to test_node_bootstrap_with_prepared_data directly?

like other tests do and easy to find #[test]

… into shuning/fix-bootstrap

siddontang · 2017-04-28T06:51:58Z

PTAL @disksing

disksing · 2017-04-28T08:20:45Z

LGTM.

* improve error handling of tpcc Signed-off-by: Ping Yu <[email protected]> * remove not relevant Signed-off-by: Ping Yu <[email protected]> --------- Signed-off-by: Ping Yu <[email protected]>

nolouch added 2 commits April 14, 2017 20:00

*: bootstrap cluster with prepare

785ab3e

fix

ab87922

siddontang reviewed Apr 17, 2017

View reviewed changes

disksing reviewed Apr 18, 2017

View reviewed changes

add test

d419e73

siddontang reviewed Apr 20, 2017

View reviewed changes

disksing reviewed Apr 20, 2017

View reviewed changes

nolouch added 2 commits April 20, 2017 15:20

address comment

9983b4d

address comment

067c8db

andelf reviewed Apr 21, 2017

View reviewed changes

andelf and others added 2 commits April 21, 2017 16:48

Merge branch 'master' into shuning/fix-bootstrap

dab9955

Merge branch 'master' into shuning/fix-bootstrap

e5e5587

siddontang reviewed Apr 22, 2017

View reviewed changes

siddontang approved these changes Apr 22, 2017

View reviewed changes

siddontang reviewed Apr 22, 2017

View reviewed changes

nolouch added 2 commits April 25, 2017 14:34

address comment

19fa048

Merge branch 'shuning/fix-bootstrap' of https://github.com/pingcap/tikv…

4767f7a

… into shuning/fix-bootstrap

nolouch added 2 commits April 25, 2017 17:50

fix

ae05692

Merge branch 'master' into shuning/fix-bootstrap

5409f36

siddontang reviewed Apr 26, 2017

View reviewed changes

nolouch added 2 commits April 26, 2017 15:54

address comment

75bad78

Merge branch 'shuning/fix-bootstrap' of https://github.com/pingcap/tikv…

8df4bcf

… into shuning/fix-bootstrap

fix ci

e6aefcc

siddontang reviewed Apr 26, 2017

View reviewed changes

nolouch added 5 commits April 26, 2017 18:32

address comment

a1df814

Merge branch 'master' into shuning/fix-bootstrap

f0aa06e

address comment

942ac5b

Merge branch 'shuning/fix-bootstrap' of https://github.com/pingcap/tikv…

654b59d

… into shuning/fix-bootstrap

Merge branch 'master' into shuning/fix-bootstrap

a79653c

siddontang approved these changes Apr 27, 2017

View reviewed changes

disksing reviewed Apr 28, 2017

View reviewed changes

nolouch added 2 commits April 28, 2017 13:47

address comment

08f9442

Merge branch 'shuning/fix-bootstrap' of https://github.com/pingcap/tikv…

10c0282

… into shuning/fix-bootstrap

Merge branch 'master' into shuning/fix-bootstrap

f21d07d

disksing approved these changes Apr 28, 2017

View reviewed changes

Merge branch 'master' into shuning/fix-bootstrap

df6f5ad

nolouch merged commit 80d8dad into master Apr 28, 2017

nolouch deleted the shuning/fix-bootstrap branch April 28, 2017 13:14

		cfg.raft_store.use_sst_file_snapshot);


		// assume there is a node has bootstraped the cluster and add region in pd successfully

*: fix node bootstrap not idempotent #1774

*: fix node bootstrap not idempotent #1774

Conversation

nolouch commented Apr 17, 2017

CLAassistant commented Apr 17, 2017 • edited Loading

siddontang commented Apr 17, 2017

Choose a reason for hiding this comment

siddontang commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch commented Apr 20, 2017

disksing commented Apr 20, 2017

andelf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Apr 25, 2017

nolouch commented Apr 25, 2017

Choose a reason for hiding this comment

siddontang commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

siddontang commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddontang commented Apr 28, 2017

disksing commented Apr 28, 2017

CLAassistant commented Apr 17, 2017 •

edited

Loading

nolouch Apr 26, 2017 •

edited

Loading