Repository navigation
dxf: add DXF resource limit config - #68251
Conversation
|
Skipping CI for Draft Pull Request. |
ti-chi-bot
Bot
added
release-note
|
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 ChangesDXF Resource Limiting and API Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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/L
|
Hi @D3Hunter. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with 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. |
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
|
@D3Hunter I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details. ⏳ This process typically takes 10-30 minutes depending on the complexity of the changes. ℹ️ Learn more details on Pantheon AI. |
D3Hunter
changed the title
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/config/config_test.go (1)
1097-1108: ⚡ Quick winAdd explicit acceptance tests for boundary values
10and100.Right now only
30(valid) and9/101(invalid) are checked. Adding exact-boundary success cases better locks the[10,100]contract.Proposed test additions
+ require.NoError(t, os.WriteFile(configFile, []byte(`deploy-mode = "premium_reserved" +dxf-resource-limit = 10`), 0644)) + conf = NewConfig() + require.NoError(t, conf.Load(configFile)) + require.Equal(t, 10, conf.DXFResourceLimit) + require.NoError(t, conf.Valid()) + + require.NoError(t, os.WriteFile(configFile, []byte(`deploy-mode = "premium_reserved" +dxf-resource-limit = 100`), 0644)) + conf = NewConfig() + require.NoError(t, conf.Load(configFile)) + require.Equal(t, 100, conf.DXFResourceLimit) + require.NoError(t, conf.Valid())🤖 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/config/config_test.go` around lines 1097 - 1108, Add explicit unit tests to assert boundary acceptance for dxf-resource-limit values 10 and 100: after writing a config file with deploy-mode = "premium_reserved" and dxf-resource-limit = 10 (and separately 100), call NewConfig(), conf.Load(configFile) and assert conf.Valid() returns no error. Reference NewConfig(), conf.Load(...) and conf.Valid() when adding these two success cases adjacent to the existing invalid/valid checks.
🤖 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.
Inline comments:
In `@pkg/config/config.go`:
- Around line 1398-1404: The deploy-mode gating for dxf-resource-limit is being
enforced too early in Load(): metaData.IsDefined("dxf-resource-limit") and the
check using c.DeployMode can reject configs that are later corrected by CLI
flags in enforceCmdArgs; move or defer the validation so Load() only sets
default c.DXFResourceLimit (using DefDXFResourceLimit) when not defined, and
perform the deploy-mode check after enforceCmdArgs merges CLI overrides (or in a
separate validate/Finalize method called after enforceCmdArgs) so that
c.DeployMode reflects CLI overrides before enforcing that dxf-resource-limit is
allowed.
In `@pkg/ddl/backfilling_dist_scheduler.go`:
- Around line 777-778: diststorage.GetNodeResource() may return nil or a nodeRc
with TotalCPU == 0, and the call to
CalRangeSize(nodeRc.TotalMem/int64(nodeRc.TotalCPU), regionSplitSize,
regionSplitKeys) will panic; add a guard after calling
diststorage.GetNodeResource() to check nodeRc != nil and nodeRc.TotalCPU > 0,
and if the guard fails, handle gracefully (return an error, skip scheduling, or
use a safe default per-core memory value) before calling CalRangeSize; update
any callers that rely on the result of CalRangeSize to handle the new
error/early-return path.
In `@pkg/dxf/importinto/planner.go`:
- Around line 704-705: Validate the node resource before computing per-core
memory: check that storage.GetNodeResource() does not return nil and that
nodeRc.TotalCPU is non-zero before using nodeRc.TotalMem/int64(nodeRc.TotalCPU)
in the call to external.CalRangeSize; if nil or zero, return an error or take a
safe fallback (e.g., log via the planner's logger and abort the operation or use
a configured default CPU value) so that the calculations for rangeSize/rangeKeys
do not panic. Ensure checks are placed immediately after calling
GetNodeResource() and propagate/handle the error in the surrounding function
that calls external.CalRangeSize.
---
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 1097-1108: Add explicit unit tests to assert boundary acceptance
for dxf-resource-limit values 10 and 100: after writing a config file with
deploy-mode = "premium_reserved" and dxf-resource-limit = 10 (and separately
100), call NewConfig(), conf.Load(configFile) and assert conf.Valid() returns no
error. Reference NewConfig(), conf.Load(...) and conf.Valid() when adding these
two success cases adjacent to the existing invalid/valid checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ac949f63-03b0-44c8-9353-36d3fe55347b
📒 Files selected for processing (11)
pkg/config/config.gopkg/config/config.toml.nextgen.examplepkg/config/config_test.gopkg/ddl/backfilling_dist_scheduler.gopkg/domain/domain.gopkg/dxf/framework/handle/handle.gopkg/dxf/framework/proto/node.gopkg/dxf/framework/proto/task_test.gopkg/dxf/framework/storage/nodes.gopkg/dxf/framework/storage/table_test.gopkg/dxf/importinto/planner.go
💤 Files with no reviewable changes (1)
- pkg/dxf/framework/handle/handle.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #68251 +/- ##
================================================
+ Coverage 77.7307% 78.7930% +1.0623%
================================================
Files 1990 2003 +13
Lines 551899 562108 +10209
================================================
+ Hits 428995 442902 +13907
+ Misses 121984 116936 -5048
- Partials 920 2270 +1350
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/dxf/framework/handle/status.go`:
- Around line 117-118: The warning message is misleading: the code calls
storage.GetDXFCPUCount() but logs "use local node CPU count instead"; update the
log invocation (logutil.BgLogger().Warn) to mention that it's falling back to
the DXF-configured CPU count or dxf-resource-limit (referencing
storage.GetDXFCPUCount()) so the log accurately reflects the actual fallback
source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f5dd7fab-65af-42a6-b4c1-60deaafec6b9
📒 Files selected for processing (5)
pkg/dxf/framework/handle/BUILD.bazelpkg/dxf/framework/handle/status.gopkg/dxf/framework/handle/status_testkit_test.gopkg/dxf/framework/storage/nodes.gopkg/dxf/framework/testutil/table_util.go
✅ Files skipped from review due to trivial changes (1)
- pkg/dxf/framework/handle/BUILD.bazel
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/dxf/framework/storage/nodes.go
|
🔍 Starting code review for this PR... |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 7
- Inline comments: 6
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (3)
dxf-resource-limitnaming and docs overstate scope beyond CPU and memory (pkg/config/config.toml.nextgen.example:19, pkg/config/config.go:100, pkg/dxf/framework/proto/node.go:50, pkg/dxf/framework/proto/node.go:57)- Per-node DXF CPU limit propagation breaks slot admission invariants on heterogeneous nodes (pkg/dxf/framework/storage/nodes.go:67, pkg/dxf/framework/storage/task_table.go:293, pkg/dxf/framework/scheduler/nodes.go:155, pkg/dxf/framework/scheduler/slots.go:206)
- Unversioned
cpu_countcontract change breaks mixed-version slot admission (pkg/dxf/framework/storage/nodes.go:67, pkg/dxf/framework/storage/task_table.go:293, pkg/dxf/framework/scheduler/nodes.go:155, pkg/dxf/framework/scheduler/slots.go:206)
🟡 [Minor] (3)
dxf-resource-limitpolicy is split acrossLoadandValidwith non-equivalent gates (pkg/config/config.go:1402, pkg/config/config.go:1479, pkg/config/config_test.go:1092)- Fallback log message now contradicts the switched CPU source (pkg/dxf/framework/handle/status.go:117, pkg/dxf/framework/handle/status.go:118)
- New helper
LimitDXFCPUexpands exported API surface without external consumers (pkg/dxf/framework/proto/node.go:61)
🧹 [Nit] (1)
TestTaskBaseGetRuntimeSlotsnow mixes two behaviors under one intent label (pkg/dxf/framework/proto/task_test.go:76, pkg/dxf/framework/proto/task_test.go:91)
ingress-bot
left a comment
ingress-bot
left a comment
There was a problem hiding this comment.
This review was generated by AI and should be verified by a human reviewer.
Manual follow-up is recommended before merge.
Summary
- Total findings: 8
- Inline comments: 7
- Summary-only findings (no inline anchor): 0
Findings (highest risk first)
⚠️ [Major] (5)
- Range-splitting now bypasses injected node-resource boundaries and depends on storage global state (pkg/ddl/backfilling_dist_scheduler.go:70, pkg/ddl/backfilling_dist_scheduler.go:777, pkg/dxf/importinto/planner.go:704, pkg/dxf/framework/storage/nodes.go:37)
dxf-resource-limitnaming/documentation implies all resources are limited, but disk stays unbounded (pkg/dxf/framework/proto/node.go:57, pkg/config/config.toml.nextgen.example:19, pkg/config/config.go:100)- DXF CPU limit rounds up and exceeds configured percentage (pkg/dxf/framework/proto/node.go:61, pkg/domain/domain.go:1178, pkg/dxf/framework/storage/nodes.go:67)
- Unversioned
cpu_countsemantic switch makes mixed-version scheduling non-repeatable during rollout (pkg/dxf/framework/storage/nodes.go:67, pkg/dxf/framework/handle/status.go:120, pkg/dxf/framework/scheduler/nodes.go:157) - Range-splitting reads node resource from storage singleton instead of local planner/scheduler contracts (pkg/dxf/importinto/planner.go:704, pkg/ddl/backfilling_dist_scheduler.go:777, pkg/dxf/framework/storage/nodes.go:34)
🟡 [Minor] (3)
- Fallback status log text no longer matches the returned CPU semantics (pkg/dxf/framework/handle/status.go:117, pkg/dxf/framework/handle/status.go:118)
dxf-resource-limitwording overstates scope as all resources while disk remains unbounded (pkg/config/config.toml.nextgen.example:19, pkg/dxf/framework/proto/node.go:50, pkg/dxf/framework/proto/node.go:57)- DXF resource limit does not constrain disk budget (pkg/dxf/framework/proto/node.go:51, pkg/dxf/framework/proto/node.go:86)
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/retest |
|
@D3Hunter: PRs from untrusted users cannot be marked as trusted with 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. |
|
/ok-to-test |
ti-chi-bot
Bot
added
size/XL
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: GMHDBJD, joechenrh, terry1purcell 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 |
|
/retest |
1 similar comment
|
/retest |
5 participants
What problem does this PR solve?
Issue Number: ref #68181
Problem Summary:
Premium Reserved deployments need a TiDB-level way to limit the amount of local TiDB resources that DXF tasks may consume. The value stored in
mysql.dist_framework_meta.cpu_countshould represent usable DXF CPU rather than total local node CPU.What changed and how does it work?
dxf-resource-limitas a TiDB config item with default100and valid range[10, 100].dxf-resource-limitconfiguration unlessdeploy-mode = "premium_reserved".100, otherwiseceil(total_cpu * limit / 100)with a minimum of 1.NodeResourceto the DXF task executor manager.mysql.dist_framework_meta.cpu_countduringInitMetaSessionandRecoverMeta.Check List
Tests
go test -run 'TestDeployModeConfig|TestConfig$|TestConfigExample|TestGetJSONConfig' -count=1 -tags=intest,deadlock ./pkg/configgo test -run TestTaskBaseGetRuntimeSlots -count=1 -tags=intest,deadlock ./pkg/dxf/framework/proto./tools/check/failpoint-go-test.sh pkg/dxf/framework/storage -run TestDistFrameworkMeta -count=1./tools/check/failpoint-go-test.sh pkg/domain -run '^$' -count=1make linttested with below diff applied to make the data size large enough for auto calculation
run on local mac with 10c, and
we can see the auto calculated resource use max 3c
also tested with multiple tasks, they queue and wait too
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.
Summary by CodeRabbit