[DM] UpdateSubTasks only checks the first source's stage, so a running subtask's config can be updated

[joechenrh]

What did you do?

Have a task that spans several sources, put only part of them into a non-running stage, and then update the task configuration.

For example, with a task on mysql-01 and mysql-02:

1. stop only mysql-01 (source_name_list on the stop endpoint accepts a subset, so a partial stop is a supported operation), leaving mysql-02 running;
2. submit a task configuration update whose first subtask config entry is mysql-01.

The update endpoint always carries a full task body, so the generated subtask config list contains every source of the task, and its order follows the source list in the body.

What did you expect to see?

The update is rejected while any of the sources it covers is still running.

What did you see instead?

The update succeeds. The subtask config of mysql-02 is rewritten in etcd while mysql-02 is still running, and the running Syncer instance for mysql-02 keeps using the old config. The new config only takes effect the next time that subtask is recreated, e.g. after a restart or a failover.

Analysis

1. Only the first entry's stage is checked.

tiflow/dm/master/scheduler/scheduler.go

Lines 1066 to 1082 in cc048ec

	cfg := cfgs[0]
	v, ok := s.subTaskCfgs.Load(cfg.Name)
	if !ok {
	return terror.ErrSchedulerTaskNotExist.Generate(cfg.Name)
	}
	cfgM := v.(map[string]config.SubTaskConfig)
	for _, cfg := range cfgs {
	_, ok = cfgM[cfg.SourceID]
	if !ok {
	return terror.ErrSchedulerSubTaskNotExist.Generate(cfg.Name, cfg.SourceID)
	}
	}
	// check whether in running stage
	stage := s.GetExpectSubTaskStage(cfg.Name, cfg.SourceID)
	if stage.Expect == pb.Stage_Running {
	return terror.ErrSchedulerSubTaskCfgUpdate.Generate(cfg.Name, cfg.SourceID)
	}

	cfg := cfgs[0]
	...
	// check whether in running stage
	stage := s.GetExpectSubTaskStage(cfg.Name, cfg.SourceID)
	if stage.Expect == pb.Stage_Running {
		return terror.ErrSchedulerSubTaskCfgUpdate.Generate(cfg.Name, cfg.SourceID)
	}

cfgs[1:] are never checked, although the loop right below (L1085) and the etcd write (L1099) operate on all of them.

2. The per-worker check does not cover the stage either.

worker.checkSubtasksCanUpdate (scheduler.go#L1090) ends up in Syncer.CheckCanUpdateCfg:

tiflow/dm/syncer/syncer.go

Line 3420 in cc048ec

func (s *Syncer) CheckCanUpdateCfg(newCfg *config.SubTaskConfig) error {

It verifies that the pessimistic shard DDL has no unresolved tables, that a few individual fields did not change (safe-mode, foreign_key_checks, and the foreign key causality related fields), and that no field outside the updatable set changed. It never inspects the subtask stage, so a running syncer returns success.

3. A second way the same check passes silently.

GetExpectSubTaskStage acquires the per-task latch and returns an invalid stage when it cannot:

tiflow/dm/master/scheduler/scheduler.go

Lines 1768 to 1775 in cc048ec

	func (s *Scheduler) GetExpectSubTaskStage(task, source string) ha.Stage {
	invalidStage := ha.NewSubTaskStage(pb.Stage_InvalidStage, source, task)
	release, err := s.subtaskLatch.tryAcquire(task)
	if err != nil {
	return invalidStage
	}
	defer release()

	invalidStage := ha.NewSubTaskStage(pb.Stage_InvalidStage, source, task)

	release, err := s.subtaskLatch.tryAcquire(task)
	if err != nil {
		return invalidStage
	}

Stage_InvalidStage != Stage_Running, so if the latch happens to be held by another operation on the same task, the guard at L1080 passes regardless of the real stage.

Note also that the stage being compared is the expect stage in etcd, which the stop path writes synchronously and returns. It says nothing about whether the worker has actually paused the subtask yet.

Risk

• The configuration reported by the API/etcd and the configuration actually in effect on a running subtask diverge, with no error surfaced to the caller.
• The divergence is resolved at an unpredictable later moment. The new config is picked up only when the subtask is recreated, so a routine restart or a failover can silently change replication behaviour (route rules, binlog filters, block-allow-list) long after the update was issued.
• For a multi-source task, whether the check fires depends on the order of the sources in the request body, which makes the behaviour inconsistent between otherwise equivalent requests.

Versions of the cluster

Found by code inspection. All line references above are against master at commit cc048ec82154a02ccd65520d1dd4477087ad48d3.

DM version (run dmctl -V or dm-worker -V or dm-master -V):

master @ cc048ec82154a02ccd65520d1dd4477087ad48d3

Upstream MySQL/MariaDB server version:

n/a (found by code inspection)

Downstream TiDB cluster version (execute SELECT tidb_version(); in a MySQL client):

n/a (found by code inspection)

How did you deploy DM: tiup or manually?

n/a (found by code inspection)

[joechenrh]

/area dm

[joechenrh]

/type bug

[joechenrh]

/severity moderate