Skip to content

[Fix-18644][Registry] Reconcile HA roles using instance ownership - #18645

Open
michaellx1057 wants to merge 5 commits into
apache:devfrom
michaellx1057:Fix-18644
Open

michaellx1057 wants to merge 5 commits into
apache:devfrom
michaellx1057:Fix-18644

Conversation

@michaellx1057

@michaellx1057 michaellx1057 commented Sep 16, 2026

Copy link
Copy Markdown

Was this PR generated or assisted by AI?

YES — Codex assisted with source analysis, implementation, regression tests and this description. The contributor reviewed the changes and validation evidence.

Purpose of the pull request

Closes #18644.

A restarted HA server can adopt its predecessor’s selector because the address is unchanged. A server that loses ownership can also retain its ACTIVE role. This can leave multiple Masters running their coordinators.

This PR distinguishes server instances, reconciles roles with current selector ownership, and prevents a coordinator from restarting before its previous worker has finished.

This follows the identity-reuse discussion in #18624. The serial-coordinator restart fix #18623 and the etcd REMOVE-value fix #18641 address related but separate parts of the behavior.

Brief change log

  • Append the construction timestamp to the existing serverIdentify.
  • Run initial election and REMOVE-triggered ownership checks on a single election worker. Each check reads the current selector before publishing the local role.
  • Stop accepting election work during HA shutdown and wait for any role publication already in progress. This does not join the election worker, since an Alert listener can close the HA server from that worker.
  • In both Master demotion and normal shutdown, request both coordinators to stop before waiting for either worker to finish.
  • Wait for the previous coordinator worker to exit before allowing another start.
  • Add regression tests for ownership reconciliation, callback ordering, shutdown and coordinator restart.

Registry paths and the Registry SPI are unchanged. No new lifecycle enum is introduced.

Verification

Current revision: 3afd40f

Targeted tests passed on JDK 8 1.8.0_504: 47 cases in total, with no failures, errors or skips:

  • 20 HA tests
  • 8 serial-coordinator tests
  • 8 task-group-coordinator tests
  • 1 MasterCoordinator shutdown test
  • 10 existing Alert tests

The concurrency tests use controlled DAO calls to exercise shutdown and restart while a worker is still busy. They cover specific interleavings; they are not a guarantee against every possible race. The full repository test suite was not run.

Controlled deployment verification

A minimal backport of 3afd40f, together with #18641 (b47fc0b), was tested on DolphinScheduler 3.4.2 with Java 8, Kubernetes 1.33.13 and a 3-member etcd 3.6.14 cluster. This used version-appropriate 3.4.2 changes, not dev JARs deployed into 3.4.2.

The sequence was:

  1. Scale from 1 to 3 Masters.
  2. Restart the elected Master at the same address.
  3. Scale back to 1 Master.

The recreated Master remained STAND_BY while etcd still contained its predecessor’s selector. At the recorded post-restart checkpoints, both coordinator workers were present only on the elected Master.

After scale-down, membership slots converged from 3 to 1 without restarting the surviving Pod. Three SERIAL_WAIT canary instances were run at each stage: 9 workflow runs and 27 Shell tasks completed successfully, with one actual output marker per task log.

The original image and complete StatefulSet configuration were restored afterward.

Complete live Java shutdown completion was not captured. Controlled-DAO tests provide separate coverage for the coordinator shutdown sequence. Network partitions, production deployments with other Registry backends, the full Alert lifecycle and long-soak behavior were not tested.

Earlier evidence

The earlier reproduction and validation comments remain useful historical context:

Those comments describe earlier revisions. The implementation and current validation are summarized above.

Compatibility and limits

  • The timestamp distinguishes successive instances at the same address under normal clock behavior. It is not a strict uniqueness guarantee for construction in the same millisecond or after clock rollback.
  • REMOVE events request a fresh ownership check, including events containing this instance’s identity. The existing retry settings are retained; no immediate demotion on Registry exceptions is added.
  • Coordinator shutdown waits for the current worker to finish. A database call that does not return can therefore delay shutdown and subsequent role changes.
  • This provides local lifecycle ordering, not distributed fencing. Already-issued database operations are not cancelled, and delayed watches or connectivity loss can still allow transient cross-Master overlap.
  • In the combined etcd validation, the existing broad member-prefix subscription also delivered selector REMOVE events to the heartbeat parser. Restoring the DELETE payload exposed a caught parse-error log for those events. Actual member removal and election completed in the tested sequence; path-boundary filtering remains a separate issue.

Pull Request Notice

Pull Request Notice

Comment thread docs/docs/en/guide/upgrade/incompatible.md Outdated
@ruanwenjun

Copy link
Copy Markdown
Member

Will there be any issues if the service restarts quickly and assumes the role of the previous leader?

@github-actions github-actions Bot removed the document label Sep 16, 2026
@michaellx1057

Copy link
Copy Markdown
Author

@ruanwenjun

Will there be any issues if the service restarts quickly and assumes the role of the previous leader?

Thanks for asking. Yes—in our reproduction, the restarted Master became ACTIVE without acquiring ownership of the existing coordinator selector. That selector still belonged to the previous process's etcd lease and was subsequently removed. A matching address does not transfer the old process's lease to the new process.

Deployment and operational context

We run a self-hosted DolphinScheduler 3.4.2 deployment with:

  • Kubernetes v1.33.13: 6 nodes, including 3 control-plane nodes.
  • DS Master: 2 replicas in a StatefulSet for availability and failover, reusing stable addresses such as ds-master-0.ds-master:5678 after restart.
  • DS Registry: a separate 3-member etcd 3.6.14 cluster, distinct from Kubernetes control-plane etcd.

We encountered scheduling problems during Master maintenance, restarts and replica-count changes. The controlled reproduction kept DS at 3.4.2 throughout; it did not involve a mixed-version upgrade. We temporarily reduced the deployment to 1 Master afterward. The proposed HA patch has not been deployed to this environment.

Restarting a Master at the same address

In a separate controlled run on 2026-09-16 (all times below are UTC+8):

  1. Restore the Master StatefulSet to 2 replicas and check both Pods.
  2. Restart ds-master-0 by deleting its Pod, allowing the StatefulSet to recreate it at the same address.
  3. Observe both Masters' role transitions and serial-coordinator logs alongside the etcd selector's lease and watch events.

The recreated Master 0 became ACTIVE while the coordinator selector was still attached to the predecessor's lease.

Both Masters logged ACTIVE and started their coordinators:

# ds-master-0
[WI-0][TI-0] - 2026-09-16 15:08:47.609 INFO  [Master-Server] o.a.d.r.a.h.DefaultServerStatusChangeListener:[27] - The status is active now.
[WI-0][TI-0] - 2026-09-16 15:08:47.610 INFO  [Master-Server] o.a.d.s.m.e.w.s.WorkflowSerialCoordinator:[90] - WorkflowSerialCoordinator started...

# ds-master-1
[WI-0][TI-0] - 2026-09-16 15:08:48.281 INFO  [grpc-default-executor-0] o.a.d.r.a.h.DefaultServerStatusChangeListener:[27] - The status is active now.
[WI-0][TI-0] - 2026-09-16 15:08:48.282 INFO  [grpc-default-executor-0] o.a.d.s.m.e.w.s.WorkflowSerialCoordinator:[90] - WorkflowSerialCoordinator started...

The etcd watch recorded deletion of the old selector, followed by Master 1 writing a new selector. Selected decoded fields:

DELETE /services/dolphinscheduler/nodes/master-coordinator
  mod_revision: 4808188
  value: ""
  prev_value: ds-master-0.ds-master:5678
  prev_lease: 2559627622716187248
  prev_mod_revision: 4807750

PUT /services/dolphinscheduler/nodes/master-coordinator
  mod_revision: 4808190
  value: ds-master-1.ds-master:5678
  lease: 2559627622716203084

More than 7 minutes later, both coordinators were still processing the same workflow instance:

# ds-master-0
[WI-0][TI-0] - 2026-09-16 15:16:18.357 INFO  [WorkflowSerialCoordinator-Thread] o.a.d.s.m.e.w.s.SerialCommandPriorityHandler:[54] - Stop the pre WorkflowInstance: 109018 due to the workflow using SERIAL_PRIORITY strategy

# ds-master-1
[WI-0][TI-0] - 2026-09-16 15:16:18.693 INFO  [WorkflowSerialCoordinator-Thread] o.a.d.s.m.e.w.s.SerialCommandPriorityHandler:[54] - Stop the pre WorkflowInstance: 109018 due to the workflow using SERIAL_PRIORITY strategy

There are recurring entries between these points, so this was sustained coordinator overlap. These logs do not establish duplicate business writes. The overlap also involved the etcd REMOVE payload issue and failure to demote after losing an election; address reuse was the initial ownership problem, not the entire failure chain.

Related scale-down observation

In an earlier round, we scaled from 2 Masters to 1. This exposed the separate REMOVE-event problem tracked in #18640/#18641; it is not the same experiment as the restart above.

Before recovery, the database snapshot showed an unassigned workflow waiting to run:

# 2026-09-16 15:00:16.984562+08; selected fields from the database snapshot
id=111167, state=14 (SERIAL_WAIT), host=<empty>, end_time=<empty>
start_time=2026-09-16 15:00:00.030

The remaining Master logged:

[WI-0][TI-0] - 2026-09-16 15:00:32.273 ERROR [grpc-default-executor-28] o.a.d.s.m.c.AbstractClusterSubscribeListener:[36] - Unknown cluster change event: Event(watchedPath=/nodes/master, eventPath=/nodes/master/ds-master-1.ds-master:5678, eventData=, type=REMOVE)

We initiated a restart of the remaining Master at approximately 15:00:47, using the same DS 3.4.2 image. The delayed instance then completed:

[WI-111167][TI-0] - 2026-09-16 15:03:15.108 INFO  [ds-workflow-eventbus-worker-6] o.a.d.s.m.e.w.s.AbstractWorkflowStateAction:[180] - Success set WorkflowExecuteRunnable: realtime_compare_2m-20260916150000030 state from: RUNNING_EXECUTION to SUCCESS

Database readback confirmed instance 111167 was SUCCESS at 15:03:15.105, and the command queue was empty at 15:04:21. Instance 111168 also completed at 15:03:24.923. Their elapsed times were approximately 195 and 85 seconds, compared with about 5 seconds for adjacent executions. These durations include the recovery process.

DS workflow history after recovery

This screenshot shows the final SUCCESS records after recovery. The fault-time SERIAL_WAIT state is established by the database snapshot; we did not capture a UI screenshot at that point.

Supplementary MySQL check

We also tested the original HA implementation with a real MySQL 8.0.46 Registry. The successor became ACTIVE, but selector row 1 still belonged to the predecessor client, with its value and update time unchanged. This was an isolated fixture deliberately retaining the predecessor, not a production JDBC restart.

This is why I think the new process should acquire its own selector rather than adopt the predecessor's role based on its address. Adding an instance timestamp addresses that ownership ambiguity. Handling a lost election and stopping the old coordinator are separate parts of the fix.

@michaellx1057

michaellx1057 commented Sep 16, 2026

Copy link
Copy Markdown
Author

While checking the demotion and re-election path, I found a related lifecycle issue in the existing Master coordinators. close() interrupts the worker thread, but a database call may continue after interruption. A subsequent start() can reset the shared running flag and let the old polling loop continue alongside the new one.

The exception → standby → retry path in this PR can exercise that sequence, so I have pushed a separate follow-up commit (3bb22b2) rather than treating the role-transition tests as sufficient.

The change keeps the existing dedicated-thread structure. Each run has its own stop flag. If the previous run is still finishing, start() records the restart request; the replacement starts after the previous run finishes its business work. A later close() cancels that request. This applies to both WorkflowSerialCoordinator and TaskGroupCoordinator, since the Master listener restarts both.

The added tests use real coordinators with deliberately blocked DAO/handler calls, including repeated role changes through MasterCoordinatorListener. They check that the old run cannot resume polling, remaining canceled work is discarded, and shutdown cancels a pending restart. The original regression fails before the fix; the affected tests pass on JDK 8.

This addresses overlap within the same coordinator instance. It does not cancel an already-issued database operation or provide fencing between different Masters. If the old database call never returns, its replacement remains pending rather than running concurrently.

@michaellx1057

michaellx1057 commented Sep 16, 2026

Copy link
Copy Markdown
Author

I tested a smaller candidate to check which changes are needed for this fix. 7fab651 now narrows this PR to the instance timestamp, ownership-based role reconciliation, election/role-update synchronization, and the coordinator thread handoff protection from 3bb22b2. It removes immediate demotion on election exceptions, the closed handling, and the related Alert resource-cleanup changes.

One clarification to my earlier coordinator comment: the overlap does not require the exception → standby → retry path. Without the handoff protection, tests also reproduced it during normal loss and reacquisition of leadership. A blocked DAO call was allowed to return after reactivation, and both the old and new threads continued polling. This is why I am keeping the handoff protection while removing the broader lifecycle changes.

Validation

The combined candidate passed 26 targeted tests on JDK 8. The final upstream revision was also clean-built through the 132-module Master/Alert reactor on actual Zulu JDK 8 1.8.0_504: all 36 distinct selected cases passed (13 HA, 6 serial-coordinator, 7 task-group-coordinator and 10 existing Alert cases), with no failures, errors or skips.

We also tested a 3.4.2 backport in the same Kubernetes / etcd deployment described above, including a leader restart at the same address and scaling from 2 Masters to 1. The backport also covers the serial-coordinator restartability fix already present in dev (#18623). It is not a deployment of dev jars into 3.4.2.

Selected fields from the validation logs follow. Times are UTC+8 on 2026-09-16; thread-dump details are omitted for readability. The thread lists contain only the serial and task-group coordinators.

20:56:28 — 2 Masters ready
selector = ds-master-0.ds-master:5678#1789563214717
ds-master-0 = [WorkflowSerialCoordinator-Thread, TaskGroupCoordinator-Thread]
ds-master-1 = []

21:01:36 — after restarting the active Master-0
selector = ds-master-1.ds-master:5678#1789563298359
ds-master-0 = []
ds-master-1 = [WorkflowSerialCoordinator-Thread, TaskGroupCoordinator-Thread]

21:06:11 — after scaling down and removing active Master-1
selector = ds-master-0.ds-master:5678#1789563580896
ds-master-0 = [WorkflowSerialCoordinator-Thread, TaskGroupCoordinator-Thread]

The rebuilt Master-0 remained standby while Master-1 owned the selector. After Master-1 was removed, Master-0 acquired the selector and started both coordinators.

Natural scheduled executions also completed successfully. For example, selected database fields show:

id=111563  host=ds-master-1.ds-master:5678  state=7 (SUCCESS)
start=21:04:00.051  end=21:04:06.056

id=111566  host=ds-master-0.ds-master:5678  state=7 (SUCCESS)
start=21:06:00.008  end=21:06:03.848

The error-command count remained at its baseline of 12 throughout the recorded checkpoints. The original image and single-Master topology were restored afterward.

An earlier attempt restored the original deployment before completing the restart check because the test's 90-second wait was shorter than the configured 120-second termination grace period. The results above are from the subsequent run with the corrected timeout.

These observations support the reduced scope for the reported restart and scale-down scenarios. The snapshots do not prove that no transient overlap occurred between observations, and the patch does not provide cross-Master fencing. The separate etcd REMOVE-value fix remains tracked in #18641.

@ruanwenjun

Copy link
Copy Markdown
Member

The current code looks too messy and feels overly AI-driven, making it difficult to tell exactly what problem it’s meant to solve.
An event-driven approach might be a better solution. The WorkflowSerialCoordinator only needs to ensure that all internal threads have finished its work before the close method returns. The higher-level AbstractHAServer is responsible for executing state-change events and controlling the active/

@michaellx1057

Copy link
Copy Markdown
Author

The current code looks too messy and feels overly AI-driven, making it difficult to tell exactly what problem it’s meant to solve. An event-driven approach might be a better solution. The WorkflowSerialCoordinator only needs to ensure that all internal threads have finished its work before the close method returns. The higher-level AbstractHAServer is responsible for executing state-change events and controlling the active/

@ruanwenjun Thanks for the suggestions. I've updated the implementation to use a single election worker and simplified the lifecycle logic.

Demotion and normal Master shutdown now share the same cleanup path: both coordinators are asked to stop before we wait for either worker to finish.

@michaellx1057

michaellx1057 commented Sep 17, 2026

Copy link
Copy Markdown
Author

I've updated the PR description to match the current implementation and completed another deployment test of 3afd40f together with #18641 (b47fc0b).

I used a minimal backport to our DolphinScheduler 3.4.2 deployment, running Java 8, Kubernetes 1.33.13 and a 3-member etcd 3.6.14 cluster. The sequence was 1→3 Masters, restarting the elected Master at the same address, then scaling back to 1.

The restart reached the retained-selector window. Selected observations from September 17, UTC+8:

18:13:10.612  recreated master-0 #1789639988110: STAND_BY
18:13:10.832  etcd still holds predecessor master-0 #1789639329898
18:13:12.779  master-1: ACTIVE

The recreated process did not inherit its predecessor’s role. Post-restart thread dumps showed both coordinator workers only on master-1. After scale-down, master-0 became the owner and its slots converged 3→2→1 without restarting that surviving Pod.

Three SERIAL_WAIT canary instances completed at each stage: 9 workflow runs / 27 Shell tasks, with one actual output marker per task log. The original image and complete StatefulSet configuration were restored afterward.

This supports the tested maintenance sequence. Complete live Java shutdown completion was not captured; controlled-DAO tests cover the coordinator shutdown interleavings separately. This was not a partition or all-backend test.

One separate issue remained visible: the broad member-prefix subscription also delivers selector events to the heartbeat parser. With the restored DELETE payload, selector REMOVE logs a caught parse error. Member removal and election still completed in this run.

Run ownership reconciliation on a single election worker. Request both coordinator workers to stop before joining either on demotion and normal Master shutdown. Add deterministic lifecycle and shutdown regression coverage.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend bug Something isn't working first time contributor First-time contributor test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] [Registry] HA server can adopt a predecessor's selector and retain ACTIVE after losing election

3 participants