config: add allow-same-cluster with a static routing validation - #6284
3AceShowHand wants to merge 18 commits into
Conversation
ti-chi-bot
Bot
added
do-not-merge/needs-linked-issue
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (17)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds ChangesSame-cluster replication
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant APIClient
participant ChangefeedController
participant ValidateSameClusterRouting
participant UpstreamCluster
APIClient->>ChangefeedController: Create or update with allow-same-cluster
ChangefeedController->>ValidateSameClusterRouting: Validate filter and routing
ValidateSameClusterRouting-->>ChangefeedController: Success or ErrInvalidReplicaConfig
ChangefeedController->>UpstreamCluster: Skip identity rejection when valid
UpstreamCluster-->>APIClient: Replicated routed data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.07% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 56 functions across 14 files. (8 skipped: 8 unsupported.)
✨ 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. A rabbit reads each line, Comment |
ti-chi-bot
Bot
added
the
size/XXL
ti-chi-bot
Bot
added
size/L
TestReplicaConfig_AllowSameCluster now covers the whole path in pkg/config: TOML decoding of the top-level `allow-same-cluster` option, Clone, and the conversion to the ChangefeedConfig used by the upstream/downstream check. The separate test in changefeed_test.go is removed.
|
/test all |
|
@3AceShowHand: No jobs can be run with The following commands are available to trigger optional jobs: 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. |
…ster case Mirror the table_route configuration: the case now maps allow_same_cluster_src.t1 to allow_same_cluster_dst.t1_routed with target-schema/target-table, so the sink writes into a schema which is not matched by the filter and the changefeed cannot capture its own writes.
3AceShowHand
changed the title
ti-chi-bot
Bot
added
release-note-none
The nil argument checks and the inconsistent downstream keyspace-name error are internal conditions, so use the predefined normalized error: callers now get CDC:ErrInternalCheckFailed instead of a message-only error. The detail of the keyspace error is passed as format arguments, no fmt.Sprintf at the call site.
|
/test all |
|
@3AceShowHand: No jobs can be run with The following commands are available to trigger optional jobs: 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@coderabbitai[bot]: 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. |
A changefeed which may replicate into the upstream cluster is only safe when it cannot capture the writes of its own sink, that is route(S) & S = empty. Gate 1: ReplicaConfig.ValidateAndAdjust rejects the option unless table routing is enabled. Gate 2: routing.ValidateSameClusterRouting, called from getVerifiedTables, rejects a replicated table which no route rule matches, a rule which maps a table to itself, and a route target which the filter replicates as well. Unit tests cover both gates, and the integration test now also expects the two rejections.
Describe the invariant route(S) & S = empty, the two enable-time gates, the boundary for tables created after the changefeed, and the verification.
ti-chi-bot
Bot
added
size/XL
The gate now searches for a witness table which the filter replicates and whose routed target the filter replicates as well, and rejects the configuration only when such a witness exists. It reasons about the patterns instead of the tables known at that moment, so tables created later are covered too, and every filter rule must be covered by some matcher with a target. Pattern forms the decision cannot handle, such as negated rules and "?" wildcards, are rejected as unsupported instead of being approximated. The table level check in downstreamadapter/routing and its api/v2 wiring are removed: with an exact decision they could never fire.
Record the invariant, the criteria the gate follows (reject only real conflicts, cover future tables, one decision point, errors point at the configuration), the supported input contract, and the known conservative cases.
|
/test next-gen |
|
/test pull-unit-test-next-gen |
…raints The constraint algebra (requirement with exact/head/tail, its merging, the per-pattern requirement derivation and the satisfiability search) is replaced by generating candidate names and verifying them against the source rule, the matcher and the routed target. Verification keeps the decision sound, and the generated atoms (literal text, required heads and tails, their combinations and the boundary names which the substitution completes) keep it complete, so no witness is missed and no safe configuration is rejected. same_cluster.go drops from 599 to 460 lines and loses six abstractions. A new test pins the boundary case where only a name shorter than the pattern text witnesses the conflict.
`allow-same-cluster` used to skip the upstream/downstream verification on its own, while the proof that skipping is safe lived in `ReplicaConfig` validation. Any path which read a persisted configuration without re-running that validation (resume, direct etcd edits) could therefore bypass the verification without a proof. `IsSameUpstreamDownstream` now runs `ValidateSameClusterRouting` on the configuration in use before it honours the flag, so the flag can never disable the verification on its own. The static validation moves from `pkg/config` to `sink/../check`'s package as `check.ValidateSameClusterRouting`, next to the verification it guards and named like the other static validations on master (`ValidateActiveActiveTSOIndexes`, `ValidateNoStaticRouteConflict`). The config layer no longer validates the flag: the CLI stops rejecting it locally and the server rejects it while creating, updating or resuming a changefeed. Helper names and log wording use verification instead of check.
|
/test all |
|
@3AceShowHand: No jobs can be run with The following commands are available to trigger optional jobs: 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. |
3AceShowHand
changed the title
ti-chi-bot
Bot
added
release-note
|
/test next-gen |
|
/test pull-unit-test-next-gen |
ti-chi-bot
Bot
added
release-note
|
/test all |
|
@3AceShowHand: No jobs can be run with The following commands are available to trigger optional jobs: 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. |
|
@coderabbitai[bot]: 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: coderabbitai[bot] 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 |
|
@3AceShowHand: No jobs can be run with The following commands are available to trigger optional jobs: 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. |
|
/test next-gen |
|
/test pull-unit-test-next-gen |
|
/test all |
|
@3AceShowHand: No jobs can be run with The following commands are available to trigger optional jobs: 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. |
|
/test pull-cdc-mysql-integration-light-next-gen |
1 participant
What problem does this PR solve?
Issue Number: close #6285
Support copying data into separate databases within the same TiDB logical cluster through an explicit
allow-same-clusteroption, while preventing replication from writing back into the source range.Table-level separation alone is insufficient: with
a.t1 → b.t1_copyandb.t2 → c.t2_copy, the target tables are outside the filter, but replicatingDROP DATABASE awould delete source databaseb.What is changed and how it works?
allow-same-clusterand its API v2 counterpartallow_same_cluster. The default same-cluster rejection remains unchanged.CDC:ErrInvalidReplicaConfig.schema.tablefilter rule must be covered by a routing matcher.target-schemaexpressions. Database DDL considers every matching schema rule, including rules with disjoint table matchers.Accepted example:
src.* → dst.*, whendstis outside the source database range. Rejected examples:src.t1 → src.t1_copy, thea → b → csource chain above, and*.orders → {schema}_backup.orders_copy.Supported rule parts are literals,
*, a single leading/trailing wildcard, and supported quoted names. Targets support literal text around one corresponding{schema}or{table}placeholder. Unsupported syntax is rejected. This is a conservative contract: coverage must come from a single matcher, and different schema expressions are rejected on overlapping source ranges even if they happen to produce the same name.Review entry points:
pkg/check/same_cluster.go: parsing, coverage, database isolation, and schema-routing consistency.pkg/check/same_cluster_runtime_test.go: comparison against the real filter and DDL router.tests/integration_tests/same_upstream_downstream/: eight configurations covering normal CLI operations, rejected create/update requests, state checks, continued replication after rejected updates, and database CREATE/ALTER/DROP routing.docs/design/2026-09-18-allow-same-cluster-gate-design.md: examples, supported inputs, and conservative limitations.Check List
Tests
go test -race -tags=intest ./pkg/check -count=1passed; failpoint injection was not enabled.go test ./pkg/config ./api/v2 -run 'TestReplicaConfig_AllowSameCluster|TestReplicaConfigConversion' -count=1passed.go test -race -tags=intest ./api/v2 ./cmd/cdc/cli -run 'TestRejectedUpdatePreservesChangefeedInfo|TestSameClusterUpdateConfigs|TestApplyChanges|TestChangefeedUpdateCli' -count=1. These verify rejected updates preserve stored configuration and all seven invalid fixtures produce their expected errors after CLI merging.same_upstream_downstream; not executed locally. They use configuration files and CLI operations, with no direct metadata edits. Each rejected update is followed by a configuration query to confirm the original configuration is unchanged.make fmtandgit diff --checkpassed.Questions
Will it cause performance regression or break compatibility?
The option is new and disabled by default. Opt-in requires cross-database isolation; same-database table renaming is unsupported. Validation operates on configuration patterns at create/update/resume time, without the previous schema/table candidate cross product. Multi-rule configurations still require overlap checks.
Do you need to update user documentation, design documentation or monitoring documentation?
The design document describes the database-isolation contract and its limitations. User-facing documentation should explain the new option and supported routing configurations. No monitoring changes.
Release note
Summary by CodeRabbit
allow-same-clusteroption for changefeeds, allowing replication within the same TiDB cluster when safe table routing is configured.