openapi(dm): preserve task metadata on delete#12757
Conversation
ti-chi-bot
Bot
added
the
release-note
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
area/dm
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a keep_meta query parameter to the delete task API, allowing users to retain task checkpoints and resumable metadata when deleting a task. The changes span the OpenAPI specification, generated code, master controller, and include both unit and integration tests. The reviewer pointed out a critical issue where the subtask latch is not acquired when keepMeta is true, which could lead to race conditions with concurrent operations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (57.8947%) is below the target coverage (60.0000%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. @@ Coverage Diff @@
## master #12757 +/- ##
================================================
- Coverage 53.6680% 53.6495% -0.0185%
================================================
Files 1039 1013 -26
Lines 146700 141086 -5614
================================================
- Hits 78731 75692 -3039
+ Misses 62053 59699 -2354
+ Partials 5916 5695 -221 🚀 New features to boost your workflow:
|
|
@joechenrh @xuanyu66 PTAL. |
|
/test pull-dm-integration-test |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 10
- Inline comments: 10
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (5)
- Optimistic metadata is deleted even when worker shutdown is unconfirmed (
dm/master/openapi_controller.go:502) - Optimistic cleanup failure is reported as a successful, non-retryable delete (
dm/master/server.go:672 and dm/master/openapi_controller.go:512) - Same-name task recreation can be erased by the old delete request (
dm/master/openapi_controller.go:493) - N-1 masters silently turn keep-meta requests into destructive deletes (
dm/openapi/spec/dm.yaml:552) - The
keep_metacontract does not define the retention boundary (dm/openapi/spec/dm.yaml:555)
🟡 [Minor] (4)
- Adjacent delete-mode booleans obscure destructive behavior (
dm/master/openapi_controller.go:437) - An empty
sourcesvalue silently changes cleanup from scoped to task-wide (dm/master/server.go:672) - Keep-meta deletion can block an API request for roughly five minutes (
dm/master/openapi_controller.go:502) - The integration case chains three delete semantics through shared state (
dm/tests/openapi/run.sh:504)
ℹ️ [Info] (1)
- Generated type updates include unrelated task-start and import-mode documentation (
dm/openapi/gen.types.go:462)
| Op: pb.TaskOp_Delete, | ||
| Sources: sourceNameList, | ||
| } | ||
| workerResponses := s.getSourceRespsAfterOperation(ctx, taskName, sourceNameList, []string{}, deleteReq) |
There was a problem hiding this comment.
⚠️ [Major] Optimistic metadata is deleted even when worker shutdown is unconfirmed
Why
The keep-meta path treats failed worker-removal confirmations as warnings and then unconditionally calls RemoveMetaDataWithTask. That cleanup API explicitly requires the task not to be running, so a timeout, canceled request, unreachable worker, or partial multi-source stop violates its state precondition.
Scope
dm/master/openapi_controller.go:502
Risk if unchanged
A syncer can continue processing after the master deletes its optimistic source-table, info, operation, and lock state. The worker may then recreate stale or contradictory shard-DDL metadata, making resume or same-name recreation unsafe while the DELETE still reports success.
Evidence
getSourceRespsAfterOperation can return Result=false, but lines 503-510 only log that outcome and line 512 always calls removeOptimisticMetaData(taskName, nil). dm/master/shardddl/optimist.go:304-308 documents that RemoveMetaDataWithTask may only be used after the task is no longer running. The new unit and integration coverage exercises only successful worker confirmation; the unit test's ordering assertions also show that confirmation-before-cleanup is an intentional invariant that production code does not currently explain.
Change request
Please skip or defer whole-task optimistic cleanup unless every source confirms that its subtask is gone, and return an actionable partial failure or persist retryable pending cleanup when confirmation fails. Cover timeout, cancellation, unreachable-worker, partial multi-source, and force-delete paths, and add a short why-comment documenting the worker/metadata ordering invariant.
There was a problem hiding this comment.
This follows the existing dmctl deletion behavior: worker confirmation is best-effort, and optimistic metadata cleanup still proceeds after scheduler state is removed. A stricter all-worker-confirmation requirement would be a broader lifecycle change, so I’m keeping the current boundary here.
| return resp, nil | ||
| } | ||
|
|
||
| func (s *Server) removeOptimisticMetaData(taskName string, sources []string) { |
There was a problem hiding this comment.
⚠️ [Major] Optimistic cleanup failure is reported as a successful, non-retryable delete
Why
The task definition is durably removed before optimistic metadata cleanup, and the extracted helper logs and discards its cleanup error. A crash in the same window has the same partial-state outcome; after either failure, retrying DELETE stops at getTask because the task no longer exists.
Scope
dm/master/server.go:672 and dm/master/openapi_controller.go:512
Risk if unchanged
Clients receive HTTP 204 while stale optimistic infos, operations, source tables, or locks remain with no supported way to complete cleanup. Recreating the intended resumable task can encounter conflicting state, and an etcd failure can also leave the master's in-memory optimistic keepers out of sync with persisted metadata.
Evidence
removeOptimisticMetaData has no return value and only logs errors from RemoveMetaDataWithTask at dm/master/server.go:672-681; deleteTask then returns nil at dm/master/openapi_controller.go:512-514 after RemoveSubTasks has already removed scheduler state. The default delete path, by contrast, propagates internal metadata cleanup failures. RemoveMetaDataWithTask mutates in-memory keepers before deleting the etcd prefix, increasing the partial-failure consequences.
Change request
Please return the cleanup error and make the post-removal cleanup recoverable, for example with a durable deletion tombstone or retryable cleanup job. Preserve the legacy gRPC caller's best-effort policy separately if required, but do not emit HTTP 204 until required optimistic cleanup is durably complete. Add failure-injection, retry, and restart coverage.
There was a problem hiding this comment.
This follows the existing dmctl best-effort cleanup behavior. Since scheduler state has already been removed, returning the cleanup error alone would not make DELETE retryable. Making post-removal cleanup recoverable would require a broader deletion-lifecycle change across both dmctl and OpenAPI, so I’m keeping the current behavior here.
| sourceNameList := s.getTaskSourceNameList(taskName) | ||
| // delete subtask on worker | ||
| return s.scheduler.RemoveSubTasks(taskName, sourceNameList...) | ||
| if err := s.scheduler.RemoveSubTasks(taskName, sourceNameList...); err != nil { |
There was a problem hiding this comment.
⚠️ [Major] Same-name task recreation can be erased by the old delete request
Why
RemoveSubTasks releases the scheduler's per-task latch before the keep-meta path polls workers and performs a task-name-wide optimistic prefix deletion. AddSubTasks can acquire that latch during the gap and recreate the same task name while the old DELETE is still active.
Scope
dm/master/openapi_controller.go:493
Risk if unchanged
A concurrent create/start can install fresh optimistic sharding metadata that line 512 then removes as if it belonged to the old task, corrupting or stalling shard-DDL coordination for the newly running task.
Evidence
RemoveSubTasks holds subtaskLatch only inside its own call (dm/master/scheduler/scheduler.go:973-977), while cleanup happens later at dm/master/openapi_controller.go:502-512. Creation independently acquires the same latch in AddSubTasks (dm/master/scheduler/scheduler.go:870-875), so no lifecycle exclusion or generation check protects the final task-wide deletion.
Change request
Please keep a task-name lifecycle guard or durable tombstone active through worker confirmation and optimistic cleanup, or make cleanup generation-aware so it cannot delete a replacement task's state. Add a deterministic race test for same-name recreation during keep-meta deletion.
There was a problem hiding this comment.
The concurrent same-name race is real, but the existing dmctl flow has the same RemoveSubTasks -> worker wait -> cleanup boundary. The workflow covered here recreates the task only after DELETE returns; preventing concurrent replacement would require a broader lifecycle guarantee, so I’m keeping the current boundary here.
| schema: | ||
| type: boolean | ||
| example: true | ||
| - name: "keep_meta" |
There was a problem hiding this comment.
⚠️ [Major] N-1 masters silently turn keep-meta requests into destructive deletes
Why
The behavior is added as an optional query parameter on the existing unversioned v1 DELETE route. An N-1 generated handler binds only force, ignores the unknown keep_meta query parameter, and executes the old path that removes both internal and downstream metadata.
Scope
dm/openapi/spec/dm.yaml:552
Risk if unchanged
During a rolling upgrade, a new client can receive HTTP 204 from an older leader even though its explicit preservation request was ignored, irreversibly dropping checkpoints and resumable state and causing replay or manual recovery.
Evidence
The diff keeps /api/v1/tasks/{task-name} and adds only an optional keep_meta parameter. In the parent revision, ServerInterfaceWrapper.DMAPIDeleteTask binds force without rejecting other query keys, and the old controller always removes internal and downstream metadata. Current tests exercise only the new handler and have no N/N-1 contract case.
Change request
Please use a versioned or capability-gated operation that an older server cannot silently accept, document required upgrade sequencing, and add mixed-version coverage proving that preservation requests either retain metadata or fail with an actionable unsupported-version error.
| - name: "keep_meta" | ||
| in: query | ||
| required: false | ||
| description: "whether to keep task checkpoints and resumable metadata when deleting the task" |
There was a problem hiding this comment.
⚠️ [Major] The keep_meta contract does not define the retention boundary
Why
The new public parameter says that checkpoints and resumable metadata are kept, but it does not tell callers that forced deletion retains only state already persisted or that optimistic-sharding metadata is still removed. Those are material API semantics that currently require reading implementation and tests.
Scope
dm/openapi/spec/dm.yaml:555
Risk if unchanged
A caller can combine force=true with keep_meta=true expecting the latest checkpoint and all task metadata to remain, then recreate from an older checkpoint or without metadata it assumed was preserved, causing replay, duplicate processing, or unexpected sharding recovery behavior.
Evidence
The schema description only says metadata is kept. dm/tests/openapi/run.sh:504 explains that stopping flushes the checkpoint, while lines 525-529 guarantee only already-persisted checkpoints for forced deletion. dm/master/openapi_controller_test.go:694 also requires optimistic source-table metadata to be absent even when resumable metadata is retained.
Change request
Please document exactly which metadata classes are retained or removed, that deletion itself does not flush a running task's latest checkpoint, and that callers should stop and wait for the task before deletion when they need the freshest resumable state; then regenerate the derived OpenAPI files.
| } | ||
|
|
||
| func (s *Server) deleteTask(ctx context.Context, taskName string, force bool) error { | ||
| func (s *Server) deleteTask(ctx context.Context, taskName string, force, keepMeta bool) error { |
There was a problem hiding this comment.
🟡 [Minor] Adjacent delete-mode booleans obscure destructive behavior
Why
deleteTask(ctx, taskName, force, keepMeta bool) represents two independent, safety-sensitive choices as same-typed positional arguments. Calls such as false, true are opaque and the values can be transposed without a compiler error.
Scope
dm/master/openapi_controller.go:437
Risk if unchanged
A future caller or refactor can accidentally force-delete a running task or remove metadata the request intended to retain.
Evidence
The new signature is declared at line 437, and current call sites already use multiple positional boolean combinations in dm/master/openapi_controller_test.go:703-751.
Change request
Prefer a named options value such as deleteTaskOptions{force: force, keepMeta: keepMeta}, or a behavior-specific adapter, so both destructive modes are explicit at every call site.
There was a problem hiding this comment.
deleteTask is unexported and has only one production caller, where force and keepMeta are named local variables derived directly from the corresponding request fields. The other calls are tests covering explicit combinations, so I’m keeping the current signature in this focused change.
| return resp, nil | ||
| } | ||
|
|
||
| func (s *Server) removeOptimisticMetaData(taskName string, sources []string) { |
There was a problem hiding this comment.
🟡 [Minor] An empty sources value silently changes cleanup from scoped to task-wide
Why
The extracted removeOptimisticMetaData helper presents sources as an ordinary scoped list but treats both nil and an empty slice as a request to remove all optimistic metadata for the task. That task-wide side effect is not signaled by the parameter or call site.
Scope
dm/master/server.go:672
Risk if unchanged
A caller that computes an empty source selection can unintentionally delete metadata for every source in the task, and reviewers cannot distinguish intentional task-wide cleanup from an accidentally empty list.
Evidence
The helper switches to RemoveMetaDataWithTask when len(sources) == 0 at lines 674-677, while the keep-meta path requests whole-task behavior with the opaque call removeOptimisticMetaData(taskName, nil) at dm/master/openapi_controller.go:512.
Change request
Prefer separate intent-revealing helpers such as removeAllOptimisticMetaDataForTask and removeOptimisticMetaDataForSources, with the source-scoped helper rejecting an empty list.
There was a problem hiding this comment.
An empty source list intentionally means a whole-task operation in the existing dmctl API. The two callers either pass the original dmctl selection or explicitly pass nil for whole-task cleanup, so the helper preserves the established semantics.
| Op: pb.TaskOp_Delete, | ||
| Sources: sourceNameList, | ||
| } | ||
| workerResponses := s.getSourceRespsAfterOperation(ctx, taskName, sourceNameList, []string{}, deleteReq) |
There was a problem hiding this comment.
🟡 [Minor] Keep-meta deletion can block an API request for roughly five minutes
Why
The endpoint synchronously invokes the generic per-source status poller using only the client request context, without a server-side end-to-end deadline for the new cleanup phase.
Scope
dm/master/openapi_controller.go:502
Risk if unchanged
An unavailable worker can retain a DELETE handler and one goroutine per source for a long tail after the task definition has already disappeared. Concurrent deletions can accumulate handlers and pool waiters, while clients may retry a partially completed operation.
Evidence
waitOperationOk performs 10 attempts (dm/master/server.go:78-81, dm/master/server.go:1986), each using the configured RPC timeout (dm/master/server.go:2011), whose default is 30 seconds (dm/master/config.go:40), plus retry intervals. This branch does not derive a bounded context like the downstream cleanup path above it.
Change request
Please give worker-removal confirmation an explicit end-to-end timeout with a defined timeout outcome rather than relying on client cancellation, and add focused unavailable-worker latency coverage.
There was a problem hiding this comment.
The roughly five-minute worst case comes from reusing dmctl’s existing worker-confirmation poller. For this control-plane delete operation, keeping the same confirmation behavior is an acceptable tradeoff and keeps OpenAPI aligned with dmctl. I intend to keep that behavior here. If a shorter OpenAPI-specific deadline is needed later, it can be implemented separately.
| run_sql_source1 "INSERT INTO openapi.keep_meta VALUES (1, 10);" | ||
| run_sql_tidb_with_retry "SELECT count(1) FROM ${target_schema_v1}.keep_meta WHERE id = 1;" "count(1): 1" | ||
|
|
||
| # stop-task flushes the checkpoint before keep-meta deletes the task definition. |
There was a problem hiding this comment.
🟡 [Minor] The integration case chains three delete semantics through shared state
Why
One shell case validates stopped keep-meta resume, forced keep-meta deletion, and default metadata removal in sequence using the same task and checkpoint. This mixes independent branches and makes later coverage contingent on every earlier branch and its retained state.
Scope
dm/tests/openapi/run.sh:504
Risk if unchanged
A failure in the first scenario prevents the force and default branches from running, and state leaked by one branch can mask or distort the next branch, reducing failure isolation and reproducibility.
Evidence
Lines 504-523 exercise stopped deletion and recreation, lines 525-529 reuse that running task for forced deletion, and lines 531-538 reuse the retained checkpoint again for default cleanup. The controller unit test similarly combines several delete branches in one method.
Change request
Please split the coverage into focused stopped keep-meta resume, forced keep-meta, and default cleanup cases with independent setup and teardown.
There was a problem hiding this comment.
This is intentionally a stateful workflow that follows the same checkpoint through stopped keep-meta deletion and same-name recreation, forced keep-meta retention, and final default cleanup. Each transition is asserted immediately, and the individual policies also have controller and parameter coverage. Splitting it would duplicate setup, while the script’s set -e would still stop later cases after an earlier failure.
| @@ -462,7 +462,7 @@ type StartTaskRequest struct { | |||
| // source name list | |||
There was a problem hiding this comment.
ℹ️ [Info] Generated type updates include unrelated task-start and import-mode documentation
Why
The generated output contains documentation changes unrelated to the keep-meta API behavior, which broadens the review surface and obscures which generated changes are required for this PR.
Scope
dm/openapi/gen.types.go:462
Risk if unchanged
Unrelated generator synchronization makes the behavioral change harder to audit and can introduce accidental documentation changes that are difficult to attribute or revert independently.
Evidence
The patch changes the task start-time comment at line 462 and expands full-import data_dir and import_mode documentation beginning at line 605, while the requested API addition concerns DELETE task parameters.
Change request
Please isolate the pre-existing generator synchronization from the keep-meta change, or briefly document why these unrelated generated hunks must accompany this regeneration.
There was a problem hiding this comment.
These are not source-spec changes introduced by this PR; the documentation already exists in dm/openapi/spec/dm.yaml on the base branch. A clean regeneration on that base produces the same hunks, so I’m keeping the generated output rather than editing it by hand.
|
@OliverS929: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What problem does this PR solve?
Issue Number: ref #12756
DM OpenAPI currently removes task metadata and checkpoints when a task is deleted. This prevents a task from being recreated with an updated configuration and continuing from its existing checkpoint.
What is changed and how it works?
keep_metaquery parameter to the task delete API. It defaults tofalse, so existing behavior remains unchanged.keep_meta=true, preserve downstream checkpoints and resumable internal metadata while continuing to clean optimistic shard DDL metadata.forceandkeep_metaindependent, allowing them to be combined.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
No existing delete behavior or performance is affected because
keep_metadefaults tofalse. Requests withkeep_meta=truewait for workers to remove subtasks before optimistic metadata cleanup. The new parameter should only be used after all DM masters are upgraded, because older servers ignore unknown query parameters.Do you need to update user documentation, design documentation or monitoring documentation?
The OpenAPI specification is updated with the new parameter and its default value. No additional documentation changes are required.
Release note