conn(dm): cap pooled DB connection lifetime and idle time - #12790
conn(dm): cap pooled DB connection lifetime and idle time#12790andreifedorov-bolt wants to merge 1 commit into
Conversation
Without SetConnMaxLifetime / SetConnMaxIdleTime the *sql.DB pool holds
its TCP connections indefinitely. When the peer TiDB / MySQL server
silently drops one -- rolling restart, `wait_timeout` expiry, or a
stateful middlebox timing out the flow -- the next statement re-using
that pooled connection surfaces as `mysql.ErrInvalidConn`.
`retry.IsUnretryableConnectionError` classifies `ErrInvalidConn` as
unretryable (because it can't tell whether a preceding write reached
the downstream), so the syncer / loader / dumper unit exits with an
error and `dm_{syncer,loader,mydumper}_exit_with_error_count` fires.
The task-checker auto-resumes seconds later, which makes the failure
look like a spurious blip -- but with enough tasks it turns into
sustained "resumable" error noise on the DM alerts.
Applying two conservative defaults in `Apply` avoids the failure mode
entirely, since the pool never hands out a long-dead connection:
* ConnMaxLifetime = 30m -- well below the MySQL/TiDB default
`wait_timeout` of 28800s (8h) and any typical middlebox idle
timeout, at ~2 reconnects/min per pooled slot.
* ConnMaxIdleTime = 5m -- retires connections that are only used
intermittently before the wall clock does.
Both are also exposed on `RawDBConfig` so callers that already
programmatically build a config can override them without touching
package defaults.
ti-chi-bot
Bot
added
release-note
|
Hi @andreifedorov-bolt. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Welcome @andreifedorov-bolt! |
|
|
ti-chi-bot
Bot
added
the
size/M
|
/label needs-cherry-pick-release-8.5 Requesting a backport to |
ti-chi-bot
Bot
added
the
needs-cherry-pick-release-8.5
📝 WalkthroughWalkthrough
ChangesConnection pool lifetime management
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant ApplyWithPingTimeout
participant RawDBConfig
participant sqlDB as "sql.DB"
Caller->>ApplyWithPingTimeout: call with rawCfg
ApplyWithPingTimeout->>ApplyWithPingTimeout: init connMaxLifetime=defaultConnMaxLifetime, connMaxIdleTime=defaultConnMaxIdleTime
ApplyWithPingTimeout->>RawDBConfig: read ConnMaxLifetime, ConnMaxIdleTime
RawDBConfig-->>ApplyWithPingTimeout: return configured values if greater than zero
ApplyWithPingTimeout->>sqlDB: SetConnMaxLifetime(connMaxLifetime)
ApplyWithPingTimeout->>sqlDB: SetConnMaxIdleTime(connMaxIdleTime)
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dm/pkg/conn/basedb.go (1)
165-181: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd unit tests for the RawDBCfg pool-limit selection.
Test nil
RawDBCfg, zero/negativeConnMaxLifetime, zero/negativeConnMaxIdleTime, positive overrides for each field, and both overrides together. CoverApplyWithPingTimeoutwithsqlmock; if the chosen pool settings are not directly observable, extract the default/override selection into a pure helper and test that helper instead.🤖 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 `@dm/pkg/conn/basedb.go` around lines 165 - 181, Add unit tests covering RawDBCfg pool-limit selection in the connection setup around ApplyWithPingTimeout: verify nil configuration, zero or negative ConnMaxLifetime and ConnMaxIdleTime, positive individual overrides, and both overrides together. Use sqlmock to exercise ApplyWithPingTimeout; if pool settings cannot be observed directly, extract the default/override selection into a pure helper and test that helper.Source: Coding guidelines
🧹 Nitpick comments (1)
dm/pkg/conn/basedb.go (1)
48-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid an absolute stale-connection guarantee.
SetConnMaxLifetimeandSetConnMaxIdleTimebound connection age and idle time. They do not prove that the peer is reachable. A peer can drop a connection before either limit expires. Replace “never” and “before the server or any middlebox drops them” with wording that says these limits reduce stale-connection reuse.Please verify the final wording against the supported Go version’s
database/sqldocumentation.Also applies to: 225-230
🤖 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 `@dm/pkg/conn/basedb.go` around lines 48 - 53, The comments describing defaultConnMaxLifetime and the related connection lifetime/idle-time settings overstate their guarantees. Revise the wording to say SetConnMaxLifetime and SetConnMaxIdleTime reduce the likelihood of reusing stale connections, without claiming they prove peer reachability or prevent all server/middlebox disconnects; verify the terminology against the supported Go version’s database/sql documentation.
🤖 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.
Outside diff comments:
In `@dm/pkg/conn/basedb.go`:
- Around line 165-181: Add unit tests covering RawDBCfg pool-limit selection in
the connection setup around ApplyWithPingTimeout: verify nil configuration, zero
or negative ConnMaxLifetime and ConnMaxIdleTime, positive individual overrides,
and both overrides together. Use sqlmock to exercise ApplyWithPingTimeout; if
pool settings cannot be observed directly, extract the default/override
selection into a pure helper and test that helper.
---
Nitpick comments:
In `@dm/pkg/conn/basedb.go`:
- Around line 48-53: The comments describing defaultConnMaxLifetime and the
related connection lifetime/idle-time settings overstate their guarantees.
Revise the wording to say SetConnMaxLifetime and SetConnMaxIdleTime reduce the
likelihood of reusing stale connections, without claiming they prove peer
reachability or prevent all server/middlebox disconnects; verify the terminology
against the supported Go version’s database/sql documentation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3636b8d6-6307-44d8-88ac-9a00c69f0230
📒 Files selected for processing (2)
dm/config/dbconfig/config.godm/pkg/conn/basedb.go
|
/check-issue-triage-complete |
1 similar comment
|
/check-issue-triage-complete |
|
/ok-to-test |
ti-chi-bot
Bot
added
ok-to-test
| } | ||
|
|
||
| // SetConnMaxLifetime sets the maximum lifetime of a pooled connection. | ||
| func (c *RawDBConfig) SetConnMaxLifetime(d time.Duration) *RawDBConfig { |
There was a problem hiding this comment.
Do we really need SetConnMaxLifetime() if ConnMaxLifetime isn't private?
There was a problem hiding this comment.
Kept for symmetry with the existing SetReadTimeout / SetWriteTimeout / SetMaxIdleConns setters on the same struct — DM's RawDBConfig is used almost exclusively via the chained fluent-builder pattern, e.g.
dbCfg.RawDBCfg = dbconfig.DefaultRawDBConfig().
SetReadTimeout(maxDMLConnectionTimeout).
SetMaxIdleConns(s.cfg.WorkerCount)14 non-test call sites use it that way (e.g. dm/syncer/syncer.go:3173,3216,3226, dm/syncer/data_validator.go:287,295,1326,1355,1395, dm/syncer/checkpoint.go:484, dm/syncer/sharding_group.go:457, dm/syncer/online-ddl-tools/online_ddl.go:136, dm/checker/checker.go:587,599, dm/config/source_config.go:280). Only one non-test caller assigns the field directly (dm/relay/relay.go:1099) and it does so on an already-built config, not in a builder chain.
Without SetConnMaxLifetime / SetConnMaxIdleTime a caller wanting to override the new fields in a chain would have to break out to a temporary and mutate the public field, which reads worse and diverges from the convention. Happy to drop them if you'd prefer to move the codebase away from the fluent pattern going forward — the fields are public either way, so it's purely stylistic.
|
/cc @D3Hunter |
|
@dveeden: 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dveeden 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 |
D3Hunter
left a comment
D3Hunter
left a comment
There was a problem hiding this comment.
Summary
- Total findings: 5
- Inline comments: 5
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (4)
- Connection-lifetime comments promise a guarantee the pool cannot provide (
dm/pkg/conn/basedb.go:48 and dm/pkg/conn/basedb.go:225) - Pool limits do not retire DM's long-lived worker connections (
dm/pkg/conn/basedb.go:231) - New pool policy lacks testable deterministic coverage (
dm/pkg/conn/basedb.go:165 and dm/pkg/conn/basedb.go:231; dm/config/dbconfig/config.go:53) - Connection-lifetime overrides are not available through DM configuration (
dm/config/dbconfig/config.go:53 and dm/config/dbconfig/config.go:108; applied in dm/pkg/conn/basedb.go:165)
🟡 [Minor] (1)
- Exported setters hide their non-positive-value semantics (
dm/config/dbconfig/config.go:53 and dm/config/dbconfig/config.go:83)
| // defaultConnMaxLifetime is the default maximum lifetime of a pooled | ||
| // database/sql connection. It is set well below any reasonable server-side | ||
| // `wait_timeout` (MySQL/TiDB default is 8h) and any typical stateful | ||
| // middlebox idle timeout, so the pool never hands out a connection whose |
There was a problem hiding this comment.
⚠️ [Major] Connection-lifetime comments promise a guarantee the pool cannot provide
Why
The new rationale says the pool closes and reopens connections before any server or middlebox drops them and therefore never returns a connection whose peer has disappeared. The configured limits only retire connections after a fixed age or idle interval; they cannot predict a rolling restart, keepalive failure, or a server or middlebox timeout shorter than those limits, and database/sql opens replacements on demand rather than as part of these setters.
Scope
dm/pkg/conn/basedb.go:48 and dm/pkg/conn/basedb.go:225
Risk if unchanged
Maintainers can treat the invalid-connection failure mode as eliminated and rely on these defaults in deployments where peers disappear sooner, obscuring the remaining operational risk and making later tuning or incident diagnosis misleading.
Evidence
The implementation always uses the hard-coded 30-minute lifetime and 5-minute idle defaults unless a positive override is supplied; it does not read the server wait_timeout, discover a middlebox timeout, or detect restarts. The standard-library contract for both setters states that expired connections may be closed lazily before reuse, which is weaker than the comments' before and never claims.
Change request
Please rewrite both comments to say that the limits reduce the chance of reusing stale connections by retiring sufficiently old or idle entries, remove the universal before/never guarantee and the claim that the setters reopen TCP connections, and state that shorter external timeouts or abrupt peer loss can still produce ErrInvalidConn.
| // gone away surfaces as `mysql.ErrInvalidConn` on the next statement, | ||
| // which the DM retry classifier treats as unretryable and causes the | ||
| // syncer/loader/dumper unit to exit with an error. | ||
| db.SetConnMaxLifetime(connMaxLifetime) |
There was a problem hiding this comment.
⚠️ [Major] Pool limits do not retire DM's long-lived worker connections
Why
SetConnMaxLifetime and SetConnMaxIdleTime are enforced by database/sql when pooled connections are idle or checked out again, but DM reserves a *sql.Conn in each BaseConn and keeps it for the lifetime of the syncer or loader worker. Those connections never become idle in the pool and are not checked out again, so neither new limit rotates the connections that the change is intended to protect.
Scope
dm/pkg/conn/basedb.go:231
Risk if unchanged
After a task has run longer than the configured lifetime, a server restart, wait_timeout, or middlebox drop can still make the next DML on a reserved worker connection return mysql.ErrInvalidConn; DM will continue to classify the ambiguous write as unretryable and exit the unit, leaving the reported production failure mode in place.
Evidence
BaseDB.GetBaseConn obtains d.DB.Conn and stores the resulting fixed connection in BaseConn (dm/pkg/conn/basedb.go:304-319); CreateConns creates these once and retains them in each DBConn (dm/syncer/dbconn/db.go:333-346). The standard pool cleaner only scans idle freeConn entries, while a reserved sql.Conn owns its driver connection until Conn.Close, so the settings added at lines 231-232 cannot expire these worker connections.
Change request
Can we rotate long-lived BaseConn instances at a safe transaction boundary (or stop pinning them and acquire through sql.DB) and add a case that keeps a worker connection past the limit and verifies it is replaced before the next statement?
| } | ||
|
|
||
| var maxIdleConns int | ||
| connMaxLifetime := defaultConnMaxLifetime |
There was a problem hiding this comment.
⚠️ [Major] New pool policy lacks testable deterministic coverage
Why
The change adds default, non-positive fallback, and positive-override policy directly inside ApplyWithPingTimeout, a method that must open and ping a real MySQL connection, and also changes the lifecycle of every database pool. It adds neither a focused test seam nor unit or integration coverage proving that the selected limits are applied and that an expired connection is retired before reuse.
Scope
dm/pkg/conn/basedb.go:165 and dm/pkg/conn/basedb.go:231; dm/config/dbconfig/config.go:53
Risk if unchanged
A later refactor can silently alter nil-config handling, invert zero or negative fallback semantics, ignore positive overrides, or stop applying the limits across syncer, loader, dumper, checker, and relay. The stale-connection production failure could then return without deterministic regression coverage catching it.
Evidence
Lines 165-180 select two defaults and conditionally replace them for positive RawDBConfig durations, while lines 231-232 apply the selected values. The diff changes only config.go and basedb.go and adds no *_test.go, integration, upgrade, or failure-path coverage for default selection, override selection, or stale-connection replacement.
Change request
Can we extract effective connection-pool option selection into a private pure helper and add table-driven unit cases for nil RawDBCfg, zero or negative values, and positive overrides? Please also add a deterministic lifecycle test using a controllable driver or server timeout and condition-based synchronization rather than an arbitrary time.Sleep, so it verifies that an expired pooled connection is replaced before the next statement.
| MaxIdleConns int | ||
| ReadTimeout string | ||
| WriteTimeout string | ||
| // ConnMaxLifetime caps the total lifetime of a pooled connection. |
There was a problem hiding this comment.
⚠️ [Major] Connection-lifetime overrides are not available through DM configuration
Why
The new defaults are intended to stay below the database or middlebox timeout, but the only overrides live in RawDBConfig, while DBConfig.RawDBCfg is excluded from TOML, JSON, and YAML and SourceConfig.GenerateDBConfig replaces it with an internally constructed value. A DM operator therefore cannot adapt the 30-minute lifetime or 5-minute idle limit to an environment whose wait_timeout or network idle timeout is shorter.
Scope
dm/config/dbconfig/config.go:53 and dm/config/dbconfig/config.go:108; applied in dm/pkg/conn/basedb.go:165
Risk if unchanged
After upgrade, deployments with a timeout below these fixed defaults can continue receiving the same stale pooled connection and unretryable mysql.ErrInvalidConn that this change is meant to prevent, with no supported configuration-based mitigation.
Evidence
ApplyWithPingTimeout only accepts positive overrides from config.RawDBCfg, but the enclosing RawDBCfg field has toml:"-" json:"-" yaml:"-" tags. The normal source path at dm/config/source_config.go:280 always creates DefaultRawDBConfig() and does not carry user-supplied lifetime values.
Change request
Can we expose backward-compatible optional lifetime and idle-time settings in the supported source and downstream configuration path, carry them into RawDBConfig, validate their units and bounds, and document how operators should choose values relative to server and middlebox timeouts?
| return c | ||
| } | ||
|
|
||
| // SetConnMaxLifetime sets the maximum lifetime of a pooled connection. |
There was a problem hiding this comment.
🟡 [Minor] Exported setters hide their non-positive-value semantics
Why
SetConnMaxLifetime and SetConnMaxIdleTime use names and one-line docs that closely mirror database/sql, where a non-positive duration disables the corresponding limit, but ApplyWithPingTimeout interprets the same values as a request for DM's positive default. The field comments mention this behavior, but callers using the exported fluent setters should not have to inspect a separate field declaration to discover that the familiar sentinel means something different here.
Scope
dm/config/dbconfig/config.go:53 and dm/config/dbconfig/config.go:83
Risk if unchanged
A programmatic caller can pass zero intending to disable connection retirement and silently receive the 30-minute or 5-minute default instead; the phrase process-wide default also overstates the scope because the fallback is local to the default DB provider path.
Evidence
Both setters assign d without validation or explanation, while DefaultDBProviderImpl.ApplyWithPingTimeout accepts an override only when the stored value is greater than zero. This differs from the standard-library methods whose names these APIs reuse.
Change request
Please document on both exported setters that only positive durations override the DM provider defaults and that zero or negative durations do not disable the limits; also replace process-wide default/see basedb.Apply with the precise provider behavior, or expose an explicit way to request the disabled state if that is intended to be supported.
| // gone away surfaces as `mysql.ErrInvalidConn` on the next statement, | ||
| // which the DM retry classifier treats as unretryable and causes the | ||
| // syncer/loader/dumper unit to exit with an error. | ||
| db.SetConnMaxLifetime(connMaxLifetime) |
There was a problem hiding this comment.
These pool settings do not rotate DM’s long-lived worker connections, so the primary syncer failure remains.
SetConnMaxLifetime and SetConnMaxIdleTime are enforced when a database/sql connection is idle in the pool or is checked out/returned. However, BaseDB.GetBaseConn calls DB.Conn, and dm/syncer/dbconn.CreateConns retains the resulting *sql.Conn for the unit lifetime. Operations on an already checked-out *sql.Conn reuse its owned driver connection without evaluating the pool lifetime or idle-time limits.
Concrete failure: if a running syncer receives no events for longer than the downstream wait_timeout, its pinned DML/DDL/checkpoint connection remains checked out and is not retired by either new setting. The next Begin, Exec, or Commit can still encounter the server-closed socket and return mysql.ErrInvalidConn.
Please rotate or validate the pinned BaseConn handles at a safe boundary, or avoid task-lifetime *sql.Conn ownership. A regression test should cover the long-lived BaseConn path; testing ordinary *sql.DB pool reuse would not exercise the affected syncer behavior.
3 participants
What problem does this PR solve?
Issue Number: close #12789
dm/pkg/conn/basedb.go#Applyonly callsdb.SetMaxIdleConns; neitherdb.SetConnMaxLifetimenordb.SetConnMaxIdleTimeis ever set, so apooled
*sql.DBconnection to the upstream MySQL or the downstreamTiDB / MySQL lives for as long as the DM worker process.
When the peer server silently drops such a connection (rolling restart,
wait_timeoutexpiry, or a stateful middlebox timing out the flow), thenext statement re-using it surfaces as
mysql.ErrInvalidConn, whichdm/pkg/retry/errors.go#IsUnretryableConnectionErrorclassifies asunretryable. The syncer / loader / dumper unit therefore exits with an
error and
dm_{syncer,loader,mydumper}_exit_with_error_count{resumable_err="true"}increments. Task-checker auto-resumes seconds later, so each individual
failure looks spurious, but with many concurrent tasks the aggregate
becomes a steady stream of DM "resumable error" alerts.
What is changed and how it works?
Apply two conservative defaults in
Applyso the pool proactivelycloses and re-opens a connection before its peer can drop it:
SetConnMaxLifetime(30m)— well below the MySQL/TiDB defaultwait_timeoutof 28800 s (8 h) and any typical middlebox idle timeout.At the default
MaxIdleConnsthis is roughly two reconnects per minuteper pooled slot — negligible next to steady-state syncer traffic.
SetConnMaxIdleTime(5m)— retires connections that are only usedintermittently before the wall clock does.
Both defaults are also exposed as
ConnMaxLifetime/ConnMaxIdleTimeon
RawDBConfig(with matchingSet…methods for symmetry), so callersthat build a
DBConfigprogrammatically can raise or lower them withouttouching package-level defaults. A zero (or negative)
RawDBCfgvaluekeeps the package default, so this is fully backwards-compatible with
every existing caller.
Check List
Tests
database/sqlconnection lifetime isdocumented and its handlers are simple setters; there is no additional
observable behaviour beyond what
database/sqlalready tests. Theexisting
dm/pkg/conntest suite (includingTestGetBaseConn,TestFailDBPing,TestGetBaseConnWontBlock) passes unchanged.Questions
Will it cause performance regression or break compatibility?
No. Steady-state churn is on the order of
MaxIdleConns / 30 min— ahandful of extra
sql.Opens per minute per worker per unit — which isnegligible against the normal statement rate. Every caller keeps its
current behaviour unless it explicitly overrides
RawDBConfig.Do you need to update user documentation, design documentation or monitoring documentation?
No.
Release note
Summary by CodeRabbit
New Features
Bug Fixes