external workload: wire GCV2 coordination#69521
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
do-not-merge/needs-tests-checked
do-not-merge/work-in-progress
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds store-bound GCV2 manager lifecycle handling, PD keyspace-level GC checks, upgrade-time worker aborts, post-GC task notifications, and effective ChangesGCV2 External Workload Manager Integration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GCWorker
participant Storage
participant ExternalWorkloadManager
participant PDClient
GCWorker->>Storage: retrieve bound manager
GCWorker->>PDClient: check keyspace-level GC mode
GCWorker->>ExternalWorkloadManager: RecycleGCV2(safePoint)
GCWorker->>ExternalWorkloadManager: RegisterGCV2(safePoint, gcLifeTime)
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ti-chi-bot
Bot
added
the
size/XL
ti-chi-bot
Bot
added
size/L
ti-chi-bot
Bot
added
size/XL
ti-chi-bot
Bot
removed
the
size/XL
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #69521 +/- ##
================================================
- Coverage 76.3270% 73.8022% -2.5249%
================================================
Files 2041 2056 +15
Lines 561007 594317 +33310
================================================
+ Hits 428200 438619 +10419
- Misses 131906 153902 +21996
- Partials 901 1796 +895
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
ti-chi-bot
Bot
added
size/XL
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/executor/set_test.go (1)
48-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood regression test for the clamped-value fix; consider also covering the skip path.
This test directly validates the fix for the previously flagged clamping issue. Consider adding a companion case (e.g.,
Role() != RoleMasteror non-keyspace-level GCMeta()) assertingUpdateGCLifeTimeis not called, to guard the gating condition against regression.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/executor/set_test.go` around lines 48 - 83, Add a companion regression case around setGCLifeTimeManager/TestSetGCLifeTimeNotifiesExternalWorkloadWithEffectiveValue to cover the skip path: verify that UpdateGCLifeTime is not invoked when the manager is not eligible, such as when Role() does not return config.RoleMaster or when Meta() does not advertise pd.KeyspaceConfigGCManagementTypeKeyspaceLevel. Reuse the existing setGCLifeTimeManager helper or a variant of it to assert updateCnt stays zero after the same SET GLOBAL flow, so the gating logic in the external workload notification path is protected against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/executor/set_test.go`:
- Around line 48-83: Add a companion regression case around
setGCLifeTimeManager/TestSetGCLifeTimeNotifiesExternalWorkloadWithEffectiveValue
to cover the skip path: verify that UpdateGCLifeTime is not invoked when the
manager is not eligible, such as when Role() does not return config.RoleMaster
or when Meta() does not advertise
pd.KeyspaceConfigGCManagementTypeKeyspaceLevel. Reuse the existing
setGCLifeTimeManager helper or a variant of it to assert updateCnt stays zero
after the same SET GLOBAL flow, so the gating logic in the external workload
notification path is protected against regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2021041a-3884-40d2-8e9c-d7166826d098
📒 Files selected for processing (4)
pkg/executor/BUILD.bazelpkg/executor/set.gopkg/executor/set_test.gopkg/store/gcworker/gc_worker.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/store/gcworker/gc_worker.go
|
@ChangRui-Ryan: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
|
@wfxr: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn response to this: Instructions 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. |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 13
- Inline comments: 13
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (9)
- Abort helper hides unconditional process termination below the bootstrap layer (
pkg/extworkload/util.go:68 (called from pkg/session/session.go:4601 and pkg/session/upgrade_run.go:48)) - Auto-analyze workers can complete GC without advancing controller state (
pkg/store/gcworker/gc_worker.go:853) - Global GC lifetime changes are dropped on non-master TiDB roles (
pkg/executor/set.go:197) - GCV2 initialization can be reordered behind the first GC notification (
cmd/tidb-server/main.go:694 (cross-function handoff through pkg/session/session.go:4548 and pkg/store/gcworker/gc_worker.go:128,215,454,847)) - Mixed-version and rollback behavior is not gated or proven (
cmd/tidb-server/main.go:682; pkg/store/gcworker/gc_worker.go:842; pkg/executor/set.go:195) - Recycle-then-register can drop the replacement GCV2 task (
pkg/store/gcworker/gc_worker.go:854) - A temporary PAUSED controller response becomes a fatal worker restart loop (
cmd/tidb-server/main.go:348) - Upgrade abort runs before the synchronized bootstrap-version recheck (
pkg/session/session.go:4601) - The GCV2 lifetime API does not encode its seconds unit (
pkg/extworkload/external_workload.go:37; pkg/extworkload/manager.go:137)
🟡 [Minor] (4)
- Best-effort controller calls can add 30-60 seconds to synchronous paths (
pkg/executor/set.go:213; pkg/store/gcworker/gc_worker.go:854-873) - Best-effort GCV2 failures are not exposed as outcome metrics (
pkg/store/gcworker/gc_worker.go:855; pkg/executor/set.go:213; pkg/extworkload/manager.go:112-125,137-169) - Effective GC lifetime can be read from a stale sysvar cache (
pkg/executor/set.go:198) - External-workload synchronization is embedded in the generic SET path (
pkg/executor/set.go:195)
| return | ||
| } | ||
|
|
||
| role := mgr.Role() |
There was a problem hiding this comment.
⚠️ [Major] Auto-analyze workers can complete GC without advancing controller state
Why
RoleAutoAnalyzeWorker is a valid external-workload role, and these TiDB instances still start the normal GC worker and participate in its leader election. However, both notification branches omit that role, so a successful GC on an auto-analyze instance broadcasts the new safe point to PD but emits neither the recycle nor registration transition to the external controller.
Scope
pkg/store/gcworker/gc_worker.go:853
Risk if unchanged
If an auto-analyze instance becomes and remains GC leader, completed tasks at or below the new safe point are not recycled and no next GCV2 round is registered. The controller can retain obsolete work, lose the heartbeat used to schedule later rounds, and diverge from the keyspace's actual GC state.
Evidence
ExternalWorkload.Valid accepts RoleAutoAnalyzeWorker; BootstrapSession starts StartGCWorker without a role check; and GCWorker.tick/leaderTick contain no external-role filter. Yet notifyGCV2AfterGC recycles only master, TTL, and GCV2 roles and registers only master and TTL roles. The new test constructs only RoleGCV2Worker, so it also omits the master, TTL, and auto-analyze routing branches.
Change request
Please include auto-analyze workers in the appropriate recycle/register actions, as for TTL workers, or explicitly prevent that role from running or winning the GC worker. Add a role-matrix test for every valid ExternalWorkloadRole, asserting master and TTL recycle/register behavior, dedicated-worker recycle-only behavior, the lifetime passed to registration, and relevant failure paths.
There was a problem hiding this comment.
Thanks for checking this path. Auto-analyze workers are already started with skip-gc-worker = true, so that role does not run or win the GC worker and cannot reach this post-GC notification. I therefore kept auto-analyze out of the recycle/register role matrix rather than duplicating the existing worker-startup restriction.
| if err != nil { | ||
| return nil, nil, err | ||
| } | ||
| initializeExternalWorkloadGCV2(context.Background(), storage, externalWorkloadManager) |
There was a problem hiding this comment.
⚠️ [Major] GCV2 initialization can be reordered behind the first GC notification
Why
The manager is published on the store before BootstrapSession, but InitializeGCV2 is deferred until after BootstrapSession returns. Bootstrap starts the GC worker, whose first Starter-mode tick can launch a GC job immediately, so a completed GC can recycle and register safe point S before startup later seeds the controller with safe point 0.
Scope
cmd/tidb-server/main.go:694 (cross-function handoff through pkg/session/session.go:4548 and pkg/store/gcworker/gc_worker.go:128,215,454,847)
Risk if unchanged
The controller can observe a non-monotonic S -> 0 transition. Depending on replacement and queue semantics, that can recreate stale work, overwrite current task state, retain obsolete work, or schedule the next round from a stale heartbeat after restart.
Evidence
initExternalWorkloadManager binds the manager before BootstrapSession(storage). During bootstrap, raw.StartGCWorker() starts a first production Starter tick that does not apply gcWaitTime until one job has completed. If that job finishes first, notifyGCV2AfterGC sends safe point S, after which initializeExternalWorkloadGCV2 unconditionally calls RegisterGCV2(ctx, 0, gcLifeTime). The protocol describes this as the safe point of the most recent keyspace-level GC.
Change request
Please establish a monotonic initialization barrier before the GC worker can observe the manager, or seed from current controller/GC state under an ordering mechanism that prevents a later zero registration. Add a focused startup-ordering test in which the first GC notification completes before initialization and assert that the controller never observes S followed by 0.
There was a problem hiding this comment.
Thanks for raising the ordering concern. I traced the current controller and worker behavior before deciding not to add a TiDB-local barrier. The zero value is a startup heartbeat, not an update of the PD GC safe point; controller registrations are stored as task rows and scheduling uses the latest heartbeat, while an activated worker computes GC state from TiDB/PD. The GC path also waits for the GC state-cache synchronization interval before performing GC operations. A local barrier would not provide a cross-node monotonic protocol, so startup ordering remains unchanged.
| } | ||
| } | ||
| } | ||
| externalWorkloadManager := initExternalWorkloadManager(context.Background(), storage) |
There was a problem hiding this comment.
⚠️ [Major] Mixed-version and rollback behavior is not gated or proven
Why
This change starts creating and mutating controller-side GCV2 task state whenever the existing external-workload config is enabled, but there is no TiDB/controller capability gate or rollout epoch. An N-1 TiDB lacks the new initialization, post-GC recycle/register, lifetime-update, and upgrade-abort paths, while controller task state can outlive a rollback.
Scope
cmd/tidb-server/main.go:682; pkg/store/gcworker/gc_worker.go:842; pkg/executor/set.go:195
Risk if unchanged
During a rolling upgrade, controller progress can pause when GC leadership moves to an older node. After downgrade, outstanding tasks created by the newer binary can remain unadvanced or repeatedly schedule workers that the older binary never recycles, without an actionable incompatibility error or cleanup step.
Evidence
The change attaches the manager before bootstrap and activates GCV2 RPCs for eligible roles without adding min-version or capability negotiation, a persisted rollout gate, downgrade cleanup, a mixed-version test, or an N-to-N-1 rollback test. The new tests use one in-process binary and mocked managers.
Change request
Please define and prove the N/N-1 compatibility contract. Add an explicit capability/version gate or implement rollback cleanup, then add a mixed-version test that transfers GC leadership across N and N-1 nodes plus a downgrade test with pre-existing controller tasks.
There was a problem hiding this comment.
Thanks. This integration is opt-in through the existing Starter external-workload configuration. The rollout contract is to keep it disabled during a mixed-version TiDB rollout, enable it only after the supporting fleet is in place, and disable/drain it before rollback. Under that contract an N-1 node never participates in the controller protocol while the feature is active, so I did not add a second binary capability gate in this PR.
| logutil.BgLogger().Warn("failed to update external workload GC life time", | ||
| zap.String("name", name), | ||
| zap.String("val", gcLifeTimeVal), | ||
| zap.Error(err)) |
There was a problem hiding this comment.
🟡 [Minor] Best-effort controller calls can add 30-60 seconds to synchronous paths
Why
Controller errors are treated as recoverable and log-only, yet the calls remain inline with a 30-second per-RPC timeout. A degraded controller can block SET GLOBAL tidb_gc_life_time for 30 seconds and keep a completed GC job marked running for up to 60 seconds across sequential recycle/register calls.
Scope
pkg/executor/set.go:213; pkg/store/gcworker/gc_worker.go:854-873
Risk if unchanged
Controller latency directly inflates administrative SQL tail latency and delays GC worker completion and cooldown accounting even though neither path requires the RPC to succeed for local success.
Evidence
requestTimeout is 30 seconds. SetExecutor calls UpdateGCLifeTime synchronously after persisting the variable, while notifyGCV2AfterGC serially invokes recycle and register before runGCJob returns and clears gcIsRunning through the done channel.
Change request
Please move best-effort coordination off the SQL and GC completion paths into a bounded single-owner reconciler with explicit backpressure, or apply a much smaller dependency budget. Preserve update ordering and coalesce superseded lifetime values rather than spawning unbounded work.
There was a problem hiding this comment.
Thanks for the latency concern. I kept the calls synchronous and bounded in this PR because they preserve ordering with the persisted SET and the completed GC transition. Moving them off-path safely needs a single-owner reconciler with persistence, coalescing, and backpressure; simply spawning asynchronous work could reorder or lose updates. The existing 30-second per-RPC bound and warning logs remain, and a reconciler can be handled as a separate design change.
|
|
||
| role := mgr.Role() | ||
| if role == config.RoleMaster || role == config.RoleTTLTaskWorker || role == config.RoleGCV2Worker { | ||
| if err := mgr.RecycleGCV2(ctx, safePoint); err != nil { |
There was a problem hiding this comment.
🟡 [Minor] Best-effort GCV2 failures are not exposed as outcome metrics
Why
The changed paths deliberately swallow controller failures, so operators need an alertable success/failure signal. The existing metric interceptor increments only an attempt counter before invocation, and UpdateGCLifeTime does not attach metric labels at all.
Scope
pkg/store/gcworker/gc_worker.go:855; pkg/executor/set.go:213; pkg/extworkload/manager.go:112-125,137-169
Risk if unchanged
A controller outage or sustained PAUSED/error response can silently disable coordination except for sampled log inspection; dashboards cannot distinguish healthy task traffic from repeated failures or measure timeout saturation.
Evidence
ExternalWorkloadTaskCounter has only worker-type and action labels and is incremented before the invocation returns. No result or latency metric is recorded, while the new callers convert errors into warnings and continue.
Change request
Please add bounded-cardinality outcome and latency metrics for these RPCs, including UpdateGCLifeTime, so alerts can distinguish success, timeout/cancellation, PAUSED, and other failures without using keyspace identifiers as labels.
There was a problem hiding this comment.
Thanks. The existing counter records attempts and failures remain visible through structured warning logs, but I agree that result and latency metrics would improve alerting. I did not add a GCV2-only metric shape here because outcome classification should be consistent across all external-workload RPCs, including PAUSED, timeout, cancellation, and other errors. That is better handled as a focused metrics follow-up.
| func loadExternalWorkloadGCLifeTime(ctx context.Context, storage kv.Storage) (time.Duration, error) { | ||
| se, err := session.CreateSession(storage) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| defer se.Close() | ||
| gcLifeTimeVal, err := variable.GetSysVar(vardef.TiDBGCLifetime).GetGlobalFromHook(ctx, se.GetSessionVars()) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| gcLifeTime, err := time.ParseDuration(gcLifeTimeVal) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| return gcLifeTime, nil | ||
| } | ||
|
|
||
| func initializeExternalWorkloadGCV2(ctx context.Context, storage kv.Storage, mgr extworkload.Manager) { |
There was a problem hiding this comment.
maybe move those logic away into extworkload pkg
There was a problem hiding this comment.
Thanks for the suggestion. I kept this startup orchestration in cmd/tidb-server because loading the effective global sysvar requires creating a session; moving that into extworkload would introduce a reverse dependency from the coordination package back to session startup. The manager operation and duration conversion remain owned by extworkload.
| return nil | ||
| } | ||
|
|
||
| func (w *GCWorker) notifyGCV2AfterGC(ctx context.Context, safePoint uint64) { |
There was a problem hiding this comment.
except w.loadDurationWithDefault, most code are extworkload related, maybe make a function in extworkload and pass param to it
There was a problem hiding this comment.
Thanks for the suggestion. I kept this orchestration in gcworker because it depends on the private GC lifetime loader and must run at the exact point after a successful local safe-point broadcast. Moving it would either expose GC-worker internals or split the role transition across packages. The extworkload manager continues to own RPC execution, timeouts, and protocol conversion.
ti-chi-bot
Bot
added
approved
needs-1-more-lgtm
|
@AmoebaProtozoa: The following test 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ChangRui-Ryan, D3Hunter, wfxr, wjhuang2016 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
[LGTM Timeline notifier]Timeline:
|
5 participants
What problem does this PR solve?
Issue Number: ref #67765
Problem Summary:
In Starter deployments, TiDB instances are ephemeral. After the serving TiDB instances for a keyspace have been gone for a while, obsolete data can still remain in storage until another TiDB instance runs keyspace-level GC again. If that data stays around for too long, it can incur unnecessary storage cost.
GCV2 worker is the compensation path for this case. It gives TiDB a hook to notify the external workload controller after keyspace-level GC progresses, so the controller can later bring up a TiDB instance to finish a final cleanup round after the user TiDB has exited. The goal is to clean up left-over data promptly without keeping a serving TiDB instance alive just for GC.
What changed and how does it work?
This PR wires the GCV2 path through the existing Starter-only external workload manager:
tidb_gc_life_timewhen the master role uses keyspace-level GC.tidb_gc_life_timechanges to the controller after TiDB persists the sysvar.No external workload manager is created outside Starter deploy mode, and
[external-workload]remains Starter-only config, so non-Starter deployments do not take this path. A dedicated GCV2 worker fails closed if the manager cannot be initialized.Check List
Tests
Release note
Summary by CodeRabbit
New Features
Bug Fixes