Skip to content

resource-manager: price remote read bytes separately#11018

Open
YuhaoZhang00 wants to merge 1 commit into
tikv:masterfrom
YuhaoZhang00:feat/remote-read-ru-cost
Open

resource-manager: price remote read bytes separately#11018
YuhaoZhang00 wants to merge 1 commit into
tikv:masterfrom
YuhaoZhang00:feat/remote-read-ru-cost

Conversation

@YuhaoZhang00

@YuhaoZhang00 YuhaoZhang00 commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: close #11019

Remote coprocessor reads currently use the same RU-v1 byte price as local reads, so the resource manager cannot price the two factual byte subsets independently.

What is changed and how does it work?

  • Add the optional read-cost-per-byte-remote request-unit configuration. When omitted, the controller uses 0.5× the normal read-byte price; an explicit zero remains valid.
  • Let response adapters optionally expose factual remote read bytes. Legacy adapters continue charging all bytes at the normal price, and malformed remote subsets are clamped to total bytes.
  • Settle RU-v1 reads as base + local_bytes * normal_rate + remote_bytes * remote_rate + CPU; paging precharge and token-bucket behavior are unchanged.
Add a separately configurable RU-v1 price for factual remote coprocessor read bytes. Default the remote rate to half of the normal read-byte rate while preserving legacy response behavior and the existing paging settlement and token-bucket flow.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has the configuration change

Release note

Support charging remote coprocessor read bytes with a separate request-unit price.

Summary by CodeRabbit

  • New Features

    • Added separate request-unit pricing for bytes processed by remote coprocessors.
    • Remote read pricing defaults to half the local read-byte rate when no override is configured.
    • Added an optional read-cost-per-byte-remote configuration setting, including support for explicit zero values.
    • Remote and local read bytes are now accounted for separately while total read usage remains unchanged.
  • Tests

    • Added coverage for configuration, cost splitting, paging settlement, and configuration updates.

Signed-off-by: Yuhao Zhang <yhzhang00@outlook.com>
@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has signed the dco. labels Jul 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign niubell, rleungx for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added contribution This PR is from a community contributor. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. labels Jul 17, 2026
@ti-chi-bot

ti-chi-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Hi @YuhaoZhang00. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

@ti-chi-bot ti-chi-bot Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 17, 2026
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

Remote Read RU Accounting

Layer / File(s) Summary
RU cost configuration and generation
client/resource_group/controller/config.go, pkg/mcs/resourcemanager/server/config.go, pkg/mcs/resourcemanager/server/config_test.go, tests/integrations/mcs/resourcemanager/resource_manager_test.go
Adds an optional remote per-byte cost, derives a default from the local cost when unset, and exposes it through RUConfig.RemoteReadBytesCost.
Local and remote read accounting
client/resource_group/controller/model.go, client/resource_group/controller/testutil.go, client/resource_group/controller/model_test.go
Splits total read bytes into local and remote portions, applies separate RU rates, caps remote bytes to total bytes, and validates settlement behavior.
Controller configuration snapshots
pkg/mcs/resourcemanager/server/manager.go, pkg/mcs/resourcemanager/server/manager_test.go
Deep-copies the optional remote cost pointer and validates snapshot updates for present and absent remote-cost settings.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested labels: type/development

Suggested reviewers: nolouch, jmpotato

Sequence Diagram(s)

sequenceDiagram
  participant ResponseInfo
  participant KVCalculator
  participant RUConfig
  ResponseInfo->>KVCalculator: report total and remote read bytes
  KVCalculator->>RUConfig: apply local and remote read cost rates
  KVCalculator-->>ResponseInfo: record total read bytes and CPU usage
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title is concise and accurately summarizes the main change: separate pricing for remote read bytes.
Description check ✅ Passed The description includes the required problem statement, issue number, change summary, tests, config change, and release note.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.26%. Comparing base (c2a47d8) to head (625c020).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11018      +/-   ##
==========================================
+ Coverage   79.22%   79.26%   +0.03%     
==========================================
  Files         541      541              
  Lines       75965    76000      +35     
==========================================
+ Hits        60187    60243      +56     
+ Misses      11531    11519      -12     
+ Partials     4247     4238       -9     
Flag Coverage Δ
unittests 79.26% <100.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@YuhaoZhang00

Copy link
Copy Markdown
Contributor Author

/check-issue-triage-complete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

contribution This PR is from a community contributor. dco-signoff: yes Indicates the PR's author has signed the dco. needs-ok-to-test Indicates a PR created by contributors and need ORG member send '/ok-to-test' to start testing. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

resource-manager: support a separate RU price for remote coprocessor read bytes

1 participant