scheduler: add Relay as a worker stage and fix bug #2219

lance6716 · 2021-10-13T09:29:16Z

What problem does this PR solve?

What is changed and how it works?

Now scheduler of DM master will maintain DM worker in 4 stage: Offline, Free, Relay, Bound. Relay is newly added to easily and correctly handle some stage transition.

Check List

Tests

Unit test
Integration test

Code changes

Has exported function/method change
Has exported variable/fields change

Side effects

Increased code complexity
Breaking backward compatibility (but fix a bug)

Related changes

Need to cherry-pick to the release branch
Need to update the documentation AND METRICS
Need to be included in the release note

ti-chi-bot · 2021-10-13T09:29:17Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Ehco1996
GMHDBJD

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

lance6716 · 2021-10-14T07:08:06Z

for some TODO, I need reviewer's opinion and fix them in next PR

dm/master/scheduler/worker.go

lance6716 · 2021-10-15T11:24:17Z

Can we add more comments to descript our behavior to make it easier for our collaborators to read? For example (just suggestion):

If worker fails, it fallbacks to other workers without relay. Last worker goes online and dispatches back to open relay sync.
worker hangs with priority fallback other relay worker. If no other relay worker is found source will fallback idle worker, and with relay sync. If last worker goes online and schedules back with relay sync.

Later I'll add some comment of choose bound to tryBoundForWorker and tryBoundForSource. Currently we don't implement "Last worker goes online and dispatches back to open relay sync" in my memory.

Ehco1996

LGTM
after rest comment are resolved

glorv · 2021-10-20T02:08:29Z

dm/master/scheduler/scheduler.go

-// returns (true, nil) after bounded.
+// tryBoundForWorker tries to bind a source to the given worker. The order of picking source is
+// - try to bind the last bound source
+// - try to bind sources on which the worker has unfinished load task


I think try to bind to the unfinished load task priority is higher than last bound souce.

glorv · 2021-10-20T02:31:01Z

dm/master/scheduler/scheduler.go

+			relaySource := workers[i].RelaySourceID()
+			another := i ^ 1 // make use of XOR to flip 0 and 1
+			toBindSource := inputSources[another]
+			if relaySource != "" && toBindSource != "" && relaySource != toBindSource {


If SourceA finished dump phase on WorkerA and then workerA down, then sourceA was scheduled to WorkerB and enable relay on workerB, then workerA up again, sourceA should be transferred to workerA again. But in this situation, it will return a false positive error.

before transferWorkerAndSource: (workerA, ""), (workerB, sourceA), workerB has relay source of sourceA

in this check of line 555, when iterating workerA, relaySource is "" so not pass; when iterating workerB toBindSouce is "" so not pass. It won't raise false positive error.
This is to check workerB has started relay for sourceA but requested to bind to sourceB

glorv · 2021-10-20T02:50:20Z

dm/master/scheduler/scheduler.go

 			// set the stage as Bound and record the bound relationship if exists.
 			if bound, ok := sbm[name]; ok {
 				// source bounds without source configuration should be deleted later
 				if _, ok := scm[bound.Source]; ok {
 					boundsToTrigger = append(boundsToTrigger, bound)
+					// TODO: if etcd has saved KV that worker1 started relay for source1, but bound to source2, should
+					// we stop relay or stop source to avoid DM master leader failed to bootstrap?


Maybe stop source is a simpler choice because we don't support migrate relay bind relation to another worker.

glorv · 2021-10-20T02:54:16Z

dm/master/scheduler/scheduler_test.go

@@ -1521,6 +1521,10 @@ func (t *testScheduler) TestTransferWorkerAndSource(c *C) {
 	c.Assert(s.bounds[sourceID2], DeepEquals, worker1)
 	c.Assert(s.bounds[sourceID3], DeepEquals, worker4)
 	c.Assert(s.bounds[sourceID4], DeepEquals, worker3)
+
+	c.Assert(worker1.StartRelay(sourceID2), IsNil)


Shall we add some unit tests for the newly added logic

glorv · 2021-10-20T03:02:37Z

dm/master/scheduler/worker.go

+	defer w.mu.Unlock()
+
+	switch w.stage {
+	case WorkerOffline, WorkerRelay:


Is it possible that the worker is in these two states? maybe add a log.DPanic here?

WorkerOffline can directly turn on relay, to ensure the worker has no time sync without relay.
WorkerRelay can turn on relay just like WorkerBound can ToBound again. Maybe we'll change it in future.

lance6716 · 2021-10-28T06:24:39Z

/cc @GMHDBJD

GMHDBJD · 2021-10-28T06:56:29Z

_utils/terror_gen/errors_release.txt

@@ -518,7 +518,7 @@ ErrSchedulerSourceOpRelayExist,[code=46023:class=scheduler:scope=internal:level=
 ErrSchedulerLatchInUse,[code=46024:class=scheduler:scope=internal:level=low], "Message: when %s, resource %s is in use by other client, Workaround: Please try again later"
 ErrSchedulerSourceCfgUpdate,[code=46025:class=scheduler:scope=internal:level=low], "Message: source can only update relay-log related parts for now"
 ErrSchedulerWrongWorkerInput,[code=46026:class=scheduler:scope=internal:level=medium], "Message: require DM master to modify worker [%s] with source [%s], but currently the worker is bound to source [%s]"
-ErrSchedulerCantTransferToRelayWorker,[code=46027:class=scheduler:scope=internal:level=medium], "Message: require DM worker to be bound to source [%s], but it has been started relay for source [%s]"
+ErrSchedulerBoundDiffWithStartedRelay,[code=46027:class=scheduler:scope=internal:level=medium], "Message: require DM worker [%s] to be bound to source [%s], but it has been started relay for source [%s]"


Workaround? stop-relay for previous source and start for current source?

GMHDBJD · 2021-10-28T07:22:55Z

dm/master/scheduler/scheduler.go

+	for _, workerName := range workers {
+		worker := s.workers[workerName]
+		if relaySource := worker.RelaySourceID(); relaySource != "" && relaySource != source {
+			busyWorkers = append(busyWorkers, workerName)
+			busySources = append(busySources, relaySource)
 		}


how about move to L1117

GMHDBJD · 2021-10-28T07:47:36Z

dm/master/scheduler/scheduler.go

-		delete(s.relayWorkers[source], w)
+	for _, workerName := range workers {
+		delete(s.relayWorkers[source], workerName)
+		s.workers[workerName].StopRelay()


Do we need to call tryBoundForWorker for this worker if it stopRelay and become free? or in later pr.

we need to wait DM worker DisableRelay then it can receive a new source bound 😂 much work to do, I'll leave it in future

GMHDBJD · 2021-10-28T07:55:39Z

dm/master/scheduler/scheduler.go

 // - remove the bound relationship in the scheduler.
 // this func is called after the bound relationship removed from etcd.
 func (s *Scheduler) updateStatusForUnbound(source string) {
 	w, ok := s.bounds[source]
 	if !ok {
 		return
 	}
-	w.ToFree()
+	if err := w.Unbound(); err != nil {
+		s.logger.DPanic("can updateStatusForUnbound", zap.Error(err))


Suggested change

s.logger.DPanic("can updateStatusForUnbound", zap.Error(err))

s.logger.DPanic("cannot updateStatusForUnbound", zap.Error(err))

GMHDBJD · 2021-10-28T08:01:00Z

tests/ha_cases/run.sh

@@ -270,7 +270,45 @@ function test_last_bound() {
 	echo "[$(date)] <<<<<< finish test_last_bound >>>>>>"
 }

+function test_exclusive_relay() {


How about add test for original issue.
e.g.

worker1 --relay-> source1 worker2 --relay-> source2 worker3 free restart worker1，then worker3 --> source1 restart worker2, then worker1 --> source2 now worker1 should not error

do you mean worker1 should not be bound? source2 should keep unbound until worker2 has up?

GMHDBJD

LGTM

lance6716 · 2021-10-28T09:00:22Z

/merge

ti-chi-bot · 2021-10-28T09:00:25Z

This pull request has been accepted and is ready to merge.

Commit hash: 93c4d59

ti-chi-bot · 2021-10-28T09:29:27Z

In response to a cherrypick label: new pull request created: #2275.

lance6716 added 4 commits October 12, 2021 19:01

scheduler: little refinement

e38ea8a

save work

dddc5ea

save work

cc005b8

save work

c6f14f8

ti-chi-bot added do-not-merge/work-in-progress needs-rebase labels Oct 13, 2021

ti-chi-bot requested review from Ehco1996 and lichunzhu October 13, 2021 09:29

ti-chi-bot added the size/XXL label Oct 13, 2021

lance6716 added 2 commits October 13, 2021 19:24

Merge branch 'master' of github.com:pingcap/dm into relay-bug

8f92387

fix terror

6f6dcd4

ti-chi-bot added size/XL and removed needs-rebase size/XXL labels Oct 13, 2021

lance6716 added 5 commits October 13, 2021 19:27

make fmt

86499bf

maybe finished now

6c1937c

fix one test

63fd7e2

fix another

505a59d

Merge branch 'master' into relay-bug

4f304c6

Ehco1996 added this to the v2.0.8 milestone Oct 14, 2021

lance6716 added 2 commits October 14, 2021 14:36

add comments and simply retry test

7b1d395

fix unstable test

83c9b53

lance6716 changed the title ~~WIP: add Relay as a worker stage and fix bug~~ scheduler: add Relay as a worker stage and fix bug Oct 14, 2021

ti-chi-bot removed the do-not-merge/work-in-progress label Oct 14, 2021

D3Hunter reviewed Oct 14, 2021

View reviewed changes

dm/master/scheduler/worker.go Outdated Show resolved Hide resolved

address comments

bd0b9c3

Ehco1996 modified the milestones: v2.0.8, v2.1.0 Oct 18, 2021

Ehco1996 approved these changes Oct 19, 2021

View reviewed changes

ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Oct 19, 2021

lance6716 added 2 commits October 19, 2021 10:07

Merge branch 'master' of github.com:pingcap/dm into relay-bug

10e99c6

add comment

d05b06b

glorv reviewed Oct 20, 2021

View reviewed changes

address comment

1b52c6a

ti-chi-bot added size/XXL and removed size/XL labels Oct 20, 2021

lance6716 mentioned this pull request Oct 21, 2021

*: add etcd key for start-relay without worker name #2249

Closed

ti-chi-bot requested a review from GMHDBJD October 28, 2021 06:24

GMHDBJD reviewed Oct 28, 2021

View reviewed changes

lance6716 added 2 commits October 28, 2021 16:49

address comment

963e6d2

Merge branch 'master' of github.com:pingcap/dm into relay-bug

93c4d59

GMHDBJD approved these changes Oct 28, 2021

View reviewed changes

ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Oct 28, 2021

ti-chi-bot added the status/can-merge label Oct 28, 2021

ti-chi-bot merged commit dd26cf9 into pingcap:master Oct 28, 2021

ti-chi-bot pushed a commit to ti-chi-bot/dm that referenced this pull request Oct 28, 2021

scheduler: add Relay as a worker stage and fix bug (pingcap#2219)

9e01c7f

ti-chi-bot mentioned this pull request Oct 28, 2021

scheduler: add Relay as a worker stage and fix bug (#2219) #2275

Merged

lance6716 mentioned this pull request Oct 28, 2021

scheduler: add Relay as a worker stage and fix bug pingcap/tiflow#3161

Merged

ti-chi-bot added a commit that referenced this pull request Oct 29, 2021

scheduler: add Relay as a worker stage and fix bug (#2219) (#2275)

274ddc4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scheduler: add Relay as a worker stage and fix bug #2219

scheduler: add Relay as a worker stage and fix bug #2219

lance6716 commented Oct 13, 2021 •

edited

ti-chi-bot commented Oct 13, 2021 •

edited

lance6716 commented Oct 14, 2021

lance6716 commented Oct 15, 2021 •

edited

Ehco1996 left a comment

glorv Oct 20, 2021

glorv Oct 20, 2021

lance6716 Oct 20, 2021

glorv Oct 20, 2021

glorv Oct 20, 2021

glorv Oct 20, 2021

lance6716 Oct 20, 2021

lance6716 commented Oct 28, 2021

GMHDBJD Oct 28, 2021

GMHDBJD Oct 28, 2021

GMHDBJD Oct 28, 2021 •

edited

lance6716 Oct 28, 2021

GMHDBJD Oct 28, 2021

GMHDBJD Oct 28, 2021

lance6716 Oct 28, 2021 •

edited

GMHDBJD Oct 28, 2021

GMHDBJD left a comment

lance6716 commented Oct 28, 2021

ti-chi-bot commented Oct 28, 2021

ti-chi-bot commented Oct 28, 2021

	s.logger.DPanic("can updateStatusForUnbound", zap.Error(err))
	s.logger.DPanic("cannot updateStatusForUnbound", zap.Error(err))

scheduler: add Relay as a worker stage and fix bug #2219

scheduler: add Relay as a worker stage and fix bug #2219

Conversation

lance6716 commented Oct 13, 2021 • edited

What problem does this PR solve?

What is changed and how it works?

Check List

ti-chi-bot commented Oct 13, 2021 • edited

lance6716 commented Oct 14, 2021

lance6716 commented Oct 15, 2021 • edited

Ehco1996 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

lance6716 commented Oct 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD Oct 28, 2021 • edited

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

lance6716 Oct 28, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GMHDBJD left a comment

Choose a reason for hiding this comment

lance6716 commented Oct 28, 2021

ti-chi-bot commented Oct 28, 2021

ti-chi-bot commented Oct 28, 2021

lance6716 commented Oct 13, 2021 •

edited

ti-chi-bot commented Oct 13, 2021 •

edited

lance6716 commented Oct 15, 2021 •

edited

GMHDBJD Oct 28, 2021 •

edited

lance6716 Oct 28, 2021 •

edited