dm-master: join a new member into an existing cluster #350

csuzhangxc · 2019-11-12T09:33:34Z

What problem does this PR solve?

join a new member into an existing DM-master cluster.

What is changed and how it works?

prepare config for embed etcd to join an existing cluster

Check List

Tests

Unit test
Integration test

Code changes

Has exported variable/fields change
Has persistent data change

Side effects

Possible performance regression
Increased code complexity
Breaking backward compatibility

Related changes

Need to cherry-pick to the release branch
Need to update the documentation
Need to update the dm/dm-ansible
Need to be included in the release note

…owner

…djust

…-master-join # Conflicts: # dm/master/config.go # dm/master/etcd.go

# Conflicts: # _utils/terror_gen/errors_release.txt # dm/master/config.go # dm/master/etcd.go # pkg/terror/error_list.go

IANTHEREAL · 2019-11-13T08:22:03Z

dm/master/etcd.go

+			return terror.ErrMasterJoinEmbedEtcdFail.Delegate(err, "read persistent join data")
+		}
+	} else {
+		cfg.InitialCluster = strings.TrimSpace(string(s))


please adding a log here shows that we use data of dir/join as cfg. InitialCluster .

added in c9a413f.

IANTHEREAL · 2019-11-13T08:37:47Z

dm/master/etcd_test.go

+
+	// join with persistent data
+	c.Assert(ioutil.WriteFile(joinFP, []byte(joinCluster), privateDirMode), check.IsNil)
+	cfgAfter = t.cloneConfig(cfgBefore)


it seems we don't verify logic at L170 of etcd.go

🤔 Did you mean

cfg.InitialCluster = strings.Join(ms, ",")

if so, it's verified by // join with existing cluster from L122 to L129 of etcd_test.go

IANTHEREAL · 2019-11-13T08:55:40Z

Rest LGTM

csuzhangxc · 2019-11-13T12:59:52Z

/run-all-tests

IANTHEREAL · 2019-11-13T14:40:55Z

LGTM

csuzhangxc · 2019-11-14T02:12:23Z

@WangXiangUSTC PTAL again

csuzhangxc · 2019-11-14T03:31:26Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-14T05:06:11Z

/run-all-tests tidb=release-3.0

codecov · 2019-11-14T05:12:58Z

Codecov Report

Merging #350 into master will decrease coverage by 0.9901%.
The diff coverage is 84.8101%.

@@               Coverage Diff                @@
##             master       #350        +/-   ##
================================================
- Coverage   58.6717%   57.6815%   -0.9902%     
================================================
  Files           160        160                
  Lines         17436      16149      -1287     
================================================
- Hits          10230       9315       -915     
+ Misses         6250       5926       -324     
+ Partials        956        908        -48

WangXiangUSTC

rest LGTM

WangXiangUSTC · 2019-11-14T05:22:17Z

dm/master/config.go

@@ -65,6 +66,7 @@ func NewConfig() *Config {
 	fs.StringVar(&cfg.InitialCluster, "initial-cluster", "", fmt.Sprintf("initial cluster configuration for bootstrapping, e,g. dm-master=%s", defaultPeerUrls))
 	fs.StringVar(&cfg.PeerUrls, "peer-urls", defaultPeerUrls, "URLs for peer traffic")
 	fs.StringVar(&cfg.AdvertisePeerUrls, "advertise-peer-urls", "", `advertise URLs for peer traffic (default "${peer-urls}")`)
+	fs.StringVar(&cfg.Join, "join", "", `join to an existing cluster (usage: cluster's "${advertise-client-urls}"`)


seems don't have config named advertise-client-urls

It's a but, join should be the address of clients (endpoints), but use peer address before, fixed in b0f5332.

WangXiangUSTC · 2019-11-14T05:34:22Z

dm/master/etcd.go

+	// check members
+	for _, m := range listResp.Members {
+		if m.Name == "" {
+			return terror.ErrMasterJoinEmbedEtcdFail.Generate("there is a member that has not joined successfully")


this means other member joined failed, but why prevent this member join? And how to fix this error?

add some comments to explain it, and update the error message in b0f5332.

later maybe we can embed etcd client in dmctl?

WangXiangUSTC · 2019-11-14T05:36:57Z

dm/master/etcd.go

+	for _, m := range addResp.Members {
+		name := m.Name
+		if m.ID == addResp.Member.ID {
+			name = cfg.Name


if name is empty, will it generate err at L142?

csuzhangxc · 2019-11-14T07:40:02Z

if name is empty, will it generate err at L142?

@WangXiangUSTC we should check name and other items in the config adjust.

csuzhangxc · 2019-11-14T07:43:43Z

/run-all-test tidb=release-3.0

csuzhangxc · 2019-11-14T10:10:12Z

/run-all-tests tidb=release-3.0

WangXiangUSTC

LGTM

csuzhangxc · 2019-11-14T12:26:43Z

/run-all-tests tidb=release-3.0

csuzhangxc · 2019-11-14T12:33:26Z

/run-all-tests tidb=release-3.0

csuzhangxc added 22 commits November 2, 2019 19:11

*: combine etcd server, gRPC API server and other HTTP servers

9893157

*: fix tests; refine config item

025a3a8

*: add test case for parseURLs

466550b

*: refine config; add more test cases

f78762c

tests: fix CI

3ad9ff2

Merge branch 'master' into dm-master-owner

8c59390

master: add more test cases for embed etcd config

675a1c2

master: refine API tests

625a891

master: refine code

cfb0096

Merge branch 'master' into dm-master-owner

eb72b15

master: update sample config items oder

6dfbe57

Merge remote-tracking branch 'origin/dm-master-owner' into dm-master-…

3f5beac

…owner

master: refine code

3d664b4

*: prepare join embed etcd cluster

139ef1a

master: address comments

31436e8

*: add some tests for prepareJoinEtcd

fad73c9

*: add more tests for prepareJoinEtcd; tiny fix AdvertisePeerUrls a…

aee2173

…djust

master: address comment

abec6e2

master: address comments

a67bcf9

Merge remote-tracking branch 'remotes/origin/dm-master-owner' into dm…

729a1ca

…-master-join # Conflicts: # dm/master/config.go # dm/master/etcd.go

Merge remote-tracking branch 'remotes/origin/master' into dm-master-join

017f8e7

# Conflicts: # _utils/terror_gen/errors_release.txt # dm/master/config.go # dm/master/etcd.go # pkg/terror/error_list.go

master: fix merge

6255adc

csuzhangxc added 2 commits November 12, 2019 17:35

*: go mod tidy

d30a8a7

master: use random port for testing

aacb1ed

IANTHEREAL reviewed Nov 13, 2019

View reviewed changes

master: address comments

c9a413f

IANTHEREAL added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Nov 13, 2019

Merge branch 'master' into dm-master-join

0610cb6

WangXiangUSTC reviewed Nov 14, 2019

View reviewed changes

master: address comments

b0f5332

WangXiangUSTC added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Nov 14, 2019

WangXiangUSTC approved these changes Nov 14, 2019

View reviewed changes

csuzhangxc merged commit 4abf519 into pingcap:master Nov 14, 2019

csuzhangxc deleted the dm-master-join branch November 14, 2019 12:41

csuzhangxc mentioned this pull request Dec 6, 2019

Make DM become HA #406

Open

23 tasks

lichunzhu pushed a commit to lichunzhu/dm that referenced this pull request Apr 6, 2020

dm-master: join a new member into an existing cluster (pingcap#350)

f95b68b

csuzhangxc removed needs-update-dm-ansible needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated labels Jun 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dm-master: join a new member into an existing cluster #350

dm-master: join a new member into an existing cluster #350

csuzhangxc commented Nov 12, 2019 •

edited

Loading

IANTHEREAL Nov 13, 2019 •

edited

Loading

csuzhangxc Nov 13, 2019

IANTHEREAL Nov 13, 2019 •

edited

Loading

csuzhangxc Nov 13, 2019

IANTHEREAL commented Nov 13, 2019

csuzhangxc commented Nov 13, 2019

IANTHEREAL commented Nov 13, 2019

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

codecov bot commented Nov 14, 2019 •

edited

Loading

WangXiangUSTC left a comment

WangXiangUSTC Nov 14, 2019

csuzhangxc Nov 14, 2019

WangXiangUSTC Nov 14, 2019

csuzhangxc Nov 14, 2019

WangXiangUSTC Nov 14, 2019

csuzhangxc commented Nov 14, 2019 •

edited

Loading

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

WangXiangUSTC left a comment

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

dm-master: join a new member into an existing cluster #350

dm-master: join a new member into an existing cluster #350

Conversation

csuzhangxc commented Nov 12, 2019 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

IANTHEREAL Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

csuzhangxc Nov 13, 2019

Choose a reason for hiding this comment

IANTHEREAL Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

csuzhangxc Nov 13, 2019

Choose a reason for hiding this comment

IANTHEREAL commented Nov 13, 2019

csuzhangxc commented Nov 13, 2019

IANTHEREAL commented Nov 13, 2019

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

codecov bot commented Nov 14, 2019 • edited Loading

Codecov Report

WangXiangUSTC left a comment

Choose a reason for hiding this comment

WangXiangUSTC Nov 14, 2019

Choose a reason for hiding this comment

csuzhangxc Nov 14, 2019

Choose a reason for hiding this comment

WangXiangUSTC Nov 14, 2019

Choose a reason for hiding this comment

csuzhangxc Nov 14, 2019

Choose a reason for hiding this comment

WangXiangUSTC Nov 14, 2019

Choose a reason for hiding this comment

csuzhangxc commented Nov 14, 2019 • edited Loading

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

WangXiangUSTC left a comment

Choose a reason for hiding this comment

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 14, 2019

csuzhangxc commented Nov 12, 2019 •

edited

Loading

IANTHEREAL Nov 13, 2019 •

edited

Loading

IANTHEREAL Nov 13, 2019 •

edited

Loading

codecov bot commented Nov 14, 2019 •

edited

Loading

csuzhangxc commented Nov 14, 2019 •

edited

Loading