feat(resourcemanager): add service limit for keyspace #9354

JmPotato · 2025-05-26T09:44:38Z

What problem does this PR solve?

Issue Number: ref #9296.

What is changed and how does it work?

Add a keyspace-level service limiter to control the maximum request rate for each keyspace.
The service limit is an unburstable RU per second rate limit that applies across all resource groups within a keyspace.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)

With a resource group rg1 whose RU is 5000 and is non-burstable, set the service limit to 10000 RU > 5000 RU:

Set the service limit to 2500 RU:

rg1 5000 RU + rg2 2500 RU with 2500 RU service limit.

Release note

None.

ti-chi-bot · 2025-05-26T09:44:41Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

JmPotato · 2025-05-27T05:26:12Z

/cc @rleungx @lhy1024 @glorv

ti-chi-bot · 2025-05-27T05:26:15Z

@JmPotato: GitHub didn't allow me to request PR reviews from the following users: glorv.

Note that only tikv members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cc @rleungx @lhy1024 @glorv

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.

codecov · 2025-05-27T05:41:44Z

Codecov Report

Attention: Patch coverage is 89.57346% with 22 lines in your changes missing coverage. Please review.

Project coverage is 76.09%. Comparing base (3bc8b2c) to head (139cb8c).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #9354      +/-   ##
==========================================
- Coverage   76.13%   76.09%   -0.05%     
==========================================
  Files         477      478       +1     
  Lines       74282    74482     +200     
==========================================
+ Hits        56557    56674     +117     
- Misses      14211    14281      +70     
- Partials     3514     3527      +13

Flag	Coverage Δ
unittests	`76.09% <89.57%> (-0.05%)`	⬇️

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.

JmPotato · 2025-05-27T06:19:16Z

/hold

JmPotato · 2025-05-27T10:25:05Z

/unhold

pkg/mcs/resourcemanager/server/apis/v1/api.go

rleungx · 2025-05-28T07:09:33Z

pkg/mcs/resourcemanager/server/service_limit.go

+	oldServiceLimit := krl.ServiceLimit
+	krl.ServiceLimit = newServiceLimit
+	now := time.Now()
+	// If the old service limit was 0 (no limit) or the new service limit is 0,


Do we support forbidding a keyspace via setting the service limit?

Currently, we do not have this requirement. If there is, it should be relatively easy to achieve by using a negative service limit value. Or can we also achieve this by changing the state of the keyspace?

rleungx · 2025-05-28T07:13:24Z

pkg/mcs/resourcemanager/server/service_limit.go

+		krl.ServiceLimit*serviceLimiterBurstFactor,
+	)
+	// Update the last update time.
+	krl.LastUpdate = now


How about moving it outside and merging it with line 65?

refillTokensLocked is widely used in the unit tests, moving this line out will cause us to manually update LastUpdate in the test.

server/api/min_resolved_ts.go

pkg/mcs/resourcemanager/server/manager.go

glorv · 2025-05-28T07:33:21Z

pkg/mcs/resourcemanager/server/apis/v1/api.go

+		c.String(http.StatusBadRequest, err.Error())
+		return
+	}
+	if req.ServiceLimit < 0 {


What's the expected behavior if limit is 0, IIRC, the ru_per_sec does not allow 0 value.

Currently, a zero value means no restriction at all, which is equivalent to the absence of a Service Limit.

pkg/mcs/resourcemanager/server/manager.go

Signed-off-by: JmPotato <github@ipotato.me>

rleungx · 2025-05-29T08:44:33Z

pkg/mcs/resourcemanager/server/apis/v1/api.go

 	configEndpoint.DELETE("/group/:name", s.deleteResourceGroup)
 	configEndpoint.GET("/controller", s.getControllerConfig)
 	configEndpoint.POST("/controller", s.setControllerConfig)
+	// Without keyspace name, it will get/set the service limit of the null keyspace.


Does the null keyspace also have the service limit?

I don't want to make the null keyspace too special within the system; also it's useful for the test's convenience.

pkg/mcs/resourcemanager/server/apis/v1/api.go

pkg/mcs/resourcemanager/server/manager.go

pkg/mcs/resourcemanager/server/service_limit.go

rleungx

The rest LGTM

pkg/mcs/resourcemanager/server/service_limit.go

Signed-off-by: JmPotato <github@ipotato.me>

glorv

LGTM

ti-chi-bot · 2025-05-30T06:09:11Z

@glorv: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

LGTM

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.

lhy1024 · 2025-05-30T08:51:49Z

pkg/mcs/resourcemanager/server/manager.go

 	}
 	// Update the cache.
-	m.keyspaceNameLookup[id] = loadedName
+	m.updateKeyspaceNameLookup(id, loadedName)


Is it possible to change the keyspace name? If keyspace is modified or deleted, do we need to update it?

Currently, once a keyspace is created, its ID and name cannot be changed. Therefore, we don’t need to consider cache misses at this stage.

ti-chi-bot · 2025-05-30T13:28:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: glorv, lhy1024, rleungx

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [lhy1024,rleungx]

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 · 2025-05-30T13:28:54Z

[LGTM Timeline notifier]

Timeline:

2025-05-30 06:58:04.906756512 +0000 UTC m=+174905.968774124: ☑️ agreed by rleungx.
2025-05-30 13:28:53.899265928 +0000 UTC m=+16420.101611192: ☑️ agreed by lhy1024.

JmPotato added component/keyspace Key space. component/resource-control labels May 26, 2025

JmPotato force-pushed the keyspace_service_limit branch 2 times, most recently from 1236150 to 5543513 Compare May 27, 2025 05:25

JmPotato marked this pull request as ready for review May 27, 2025 05:25

ti-chi-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2025

ti-chi-bot bot requested a review from rleungx May 27, 2025 05:26

ti-chi-bot bot requested a review from lhy1024 May 27, 2025 05:26

JmPotato force-pushed the keyspace_service_limit branch from 5543513 to 96efc42 Compare May 27, 2025 05:33

ti-chi-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 27, 2025

JmPotato force-pushed the keyspace_service_limit branch from 96efc42 to 34bdd9b Compare May 27, 2025 10:25

ti-chi-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 27, 2025

JmPotato commented May 28, 2025

View reviewed changes

pkg/mcs/resourcemanager/server/apis/v1/api.go Outdated Show resolved Hide resolved

rleungx reviewed May 28, 2025

View reviewed changes

glorv reviewed May 28, 2025

View reviewed changes

JmPotato force-pushed the keyspace_service_limit branch from 34bdd9b to 0665873 Compare May 28, 2025 08:50

JmPotato mentioned this pull request May 28, 2025

Tracking issue for the Keyspace Service Limit #9296

Open

6 tasks

glorv reviewed May 28, 2025

View reviewed changes

pkg/mcs/resourcemanager/server/manager.go Outdated Show resolved Hide resolved

JmPotato force-pushed the keyspace_service_limit branch from cc931e9 to c2df450 Compare May 29, 2025 02:46

feat(resourcemanager): add service limit for keyspace

cbda230

Signed-off-by: JmPotato <github@ipotato.me>

JmPotato added 2 commits May 29, 2025 15:58

Fix TestKeyspaceServiceLimit

57711e6

Signed-off-by: JmPotato <github@ipotato.me>

test(mcs): add integration tests for Service Limit API

0843c7f

Signed-off-by: JmPotato <github@ipotato.me>

JmPotato force-pushed the keyspace_service_limit branch from 6df751f to 0843c7f Compare May 29, 2025 08:43

rleungx reviewed May 29, 2025

View reviewed changes

pkg/mcs/resourcemanager/server/apis/v1/api.go Show resolved Hide resolved

rleungx reviewed May 29, 2025

View reviewed changes

pkg/mcs/resourcemanager/server/apis/v1/api.go Outdated Show resolved Hide resolved

pkg/mcs/resourcemanager/server/manager.go Outdated Show resolved Hide resolved

rleungx reviewed May 29, 2025

View reviewed changes

pkg/mcs/resourcemanager/server/service_limit.go Show resolved Hide resolved

rleungx reviewed May 29, 2025

View reviewed changes

pkg/mcs/resourcemanager/server/service_limit.go Outdated Show resolved Hide resolved

Address the comments

4e8043c

Signed-off-by: JmPotato <github@ipotato.me>

JmPotato force-pushed the keyspace_service_limit branch from d390f9f to 4e8043c Compare May 29, 2025 09:22

Update the test etcd keys

97d5072

Signed-off-by: JmPotato <github@ipotato.me>

JmPotato requested review from glorv, nolouch and rleungx and removed request for glorv May 30, 2025 05:41

JmPotato mentioned this pull request May 30, 2025

mcs: suppport keyspace name in resource group api #9371

Merged

glorv approved these changes May 30, 2025

View reviewed changes

rleungx approved these changes May 30, 2025

View reviewed changes

ti-chi-bot bot added needs-1-more-lgtm Indicates a PR needs 1 more LGTM. approved labels May 30, 2025

lhy1024 reviewed May 30, 2025

View reviewed changes

lhy1024 approved these changes May 30, 2025

View reviewed changes

ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 30, 2025

Merge branch 'master' into keyspace_service_limit

139cb8c

ti-chi-bot bot merged commit 67b518d into tikv:master May 30, 2025
25 checks passed

JmPotato deleted the keyspace_service_limit branch May 31, 2025 02:54

feat(resourcemanager): add service limit for keyspace #9354

feat(resourcemanager): add service limit for keyspace #9354

Uh oh!

Conversation

JmPotato commented May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

Uh oh!

ti-chi-bot bot commented May 26, 2025

Uh oh!

JmPotato commented May 27, 2025

Uh oh!

ti-chi-bot bot commented May 27, 2025

Uh oh!

codecov bot commented May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

JmPotato commented May 27, 2025

Uh oh!

JmPotato commented May 27, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JmPotato May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rleungx left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glorv left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 30, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot bot commented May 30, 2025

Uh oh!

ti-chi-bot bot commented May 30, 2025

[LGTM Timeline notifier]

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JmPotato commented May 26, 2025 •

edited

Loading

codecov bot commented May 27, 2025 •

edited

Loading

JmPotato May 29, 2025 •

edited

Loading