resouce_manager: impl resource controller for tokens client by CabinfeverB · Pull Request #5811 · tikv/pd

CabinfeverB · 2022-12-29T13:14:23Z

Signed-off-by: Cabinfever_B cabinfeveroier@gmail.com

What problem does this PR solve?

Issue Number: ref #5851

What is changed and how does it work?

Check List

Tests

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

Release note

None.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2022-12-29T13:14:25Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

JmPotato
nolouch

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

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

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

codecov · 2022-12-29T13:23:46Z

Codecov Report

Base: 75.51% // Head: 75.69% // Increases project coverage by +0.18% 🎉

Coverage data is based on head (91eb7b3) compared to base (6e35a4d).
Patch coverage: 70.23% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5811      +/-   ##
==========================================
+ Coverage   75.51%   75.69%   +0.18%     
==========================================
  Files         341      342       +1     
  Lines       34413    34829     +416     
==========================================
+ Hits        25986    26363     +377     
- Misses       6191     6223      +32     
- Partials     2236     2243       +7

Flag	Coverage Δ
unittests	`75.69% <70.23%> (+0.18%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/mcs/resource_manager/client/client.go	`68.47% <68.47%> (ø)`
pkg/mcs/resource_manager/client/model.go	`80.85% <71.87%> (+80.85%)`	⬆️
pkg/mcs/resource_manager/client/config.go	`47.36% <100.00%> (+47.36%)`	⬆️
pkg/mcs/resource_manager/client/limiter.go	`82.66% <100.00%> (+13.70%)`	⬆️
pkg/mcs/resource_manager/server/token_bukets.go	`92.68% <100.00%> (+3.49%)`	⬆️
server/tso/local_allocator.go	`64.86% <0.00%> (-13.52%)`	⬇️
server/region_syncer/server.go	`82.96% <0.00%> (-4.40%)`	⬇️
pkg/id/id.go	`86.44% <0.00%> (-3.39%)`	⬇️
pkg/storage/kv/etcd_kv.go	`72.22% <0.00%> (-3.18%)`	⬇️
pkg/utils/etcdutil/etcdutil.go	`86.04% <0.00%> (-2.33%)`	⬇️
... and 30 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/mcs/resource_manager/tenant_client/limiter.go

Signed-off-by: JmPotato <ghzpotato@gmail.com>

pkg/mcs/resource_manager/tenant_client/client.go

pkg/mcs/resource_manager/tenant_client/limiter.go

Signed-off-by: JmPotato <ghzpotato@gmail.com>

pkg/mcs/resource_manager/tenant_client/client.go

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> merge Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

rest lgtm

nolouch · 2023-01-18T19:00:09Z

pkg/mcs/resource_manager/client/client.go

+	}
+	log.Info("create resource group cost controller", zap.String("name", group.GetName()))
+	gc := newGroupCostController(group, c.config, c.lowTokenNotifyChan)
+	gc.initRunState()


A future case: If user change mode from RU to RAW mode. it need re-init?

nolouch · 2023-01-18T19:02:50Z

pkg/mcs/resource_manager/client/client.go

+	if err != nil {
+		return err
+	}
+	gc.mu.Lock()


This is in hot path, we may need to do some benchmark later. BTW, maybe we can compare it with the buffer channel.

We can compare it in another PR.

nolouch · 2023-01-18T19:10:25Z

pkg/mcs/resource_manager/client/client.go

+	}
+}
+
+func (gc *groupCostController) updateAvgResourcePerSec() {


Suggested change

func (gc *groupCostController) updateAvgResourcePerSec() {

func (gc *groupCostController) updateAvgRaWResourcePerSec() {

nolouch · 2023-01-18T19:10:43Z

pkg/mcs/resource_manager/client/client.go

+
+func (gc *groupCostController) updateAvgResourcePerSec() {
+	for typ, counter := range gc.run.resourceTokens {
+		if !gc.calcAvg(counter, getResourceValueFromConsumption(gc.run.consumption, typ)) {


Suggested change

if !gc.calcAvg(counter, getResourceValueFromConsumption(gc.run.consumption, typ)) {

if !gc.calcAvg(counter, getRawResourceValueFromConsumption(gc.run.consumption, typ)) {

nolouch · 2023-01-18T19:40:43Z

pkg/mcs/resource_manager/client/client.go

+		if !gc.calcAvg(counter, getResourceValueFromConsumption(gc.run.consumption, typ)) {
+			continue
+		}
+		log.Debug("[resource group controllor] update avg ru per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))


Suggested change

log.Debug("[resource group controllor] update avg ru per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))

log.Debug("[resource group controllor] update avg raw resource per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))

nolouch · 2023-01-19T02:19:30Z

pkg/mcs/resource_manager/client/client.go

+	}
+
+	var cfg tokenBucketReconfigureArgs
+	if trickleTimeMs == 0 {


maybe we need some comments, here is main difference.

nolouch · 2023-01-19T06:38:34Z

pkg/mcs/resource_manager/client/client.go

+}
+
+// Start starts resourceGroupController service
+func (c *resourceGroupsController) Start(ctx context.Context) error {


How about add a Stop function, I may use it when resource group features disable.

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch · 2023-01-19T07:18:35Z

pkg/mcs/resource_manager/client/client.go

+		TargetRequestPeriodMs: uint64(c.config.targetPeriod / time.Millisecond),
+	}
+	go func() {
+		log.Info("[resource group controllor] send token bucket request", zap.Time("now", now), zap.Any("req", req.Requests), zap.String("source", source))


Debug level.

nolouch · 2023-01-19T07:40:50Z

pkg/mcs/resource_manager/client/client.go

+
+var _ ResourceGroupKVInterceptor = (*resourceGroupsController)(nil)
+
+type resourceGroupsController struct {


Suggested change

type resourceGroupsController struct {

type ResourceGroupsController struct {

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato · 2023-01-19T07:55:39Z

pkg/mcs/resource_manager/client/client.go

@@ -0,0 +1,790 @@
+// Copyright 2022 TiKV Project Authors.


Suggested change

// Copyright 2022 TiKV Project Authors.

// Copyright 2023 TiKV Project Authors.

JmPotato · 2023-01-19T07:57:25Z

pkg/mcs/resource_manager/client/client.go

+}
+
+// Start starts resourceGroupController service
+func (c *ResourceGroupsController) Start(ctx context.Context) error {


This function doesn't need to return anything.

JmPotato · 2023-01-19T08:00:33Z

pkg/mcs/resource_manager/client/client.go

+	if err != nil {
+		return err
+	}
+	lastedGroups := make(map[string]struct{})


Suggested change

lastedGroups := make(map[string]struct{})

latestGroups := make(map[string]struct{})

reaming @CabinfeverB

JmPotato · 2023-01-19T08:01:49Z

pkg/mcs/resource_manager/client/client.go

+		name := res.GetResourceGroupName()
+		v, ok := c.groupsController.Load(name)
+		if !ok {
+			log.Warn("A non-existent resource group was found when handle token response.", zap.String("name", name))


Shouldn't we continue here?

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch

lgtm

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot · 2023-01-19T10:27:58Z

@BornChanger: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In 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 ti-community-infra/tichi repository.

JmPotato

The rest LGTM.

JmPotato · 2023-01-19T10:45:44Z

pkg/mcs/resource_manager/client/client.go

+func (c *ResourceGroupsController) OnRequestWait(
+	ctx context.Context, resourceGroupName string, info RequestInfo,
+) (err error) {
+	if _, ok := defaultWhiteList[resourceGroupName]; ok {


This check could be done by https://github.com/tikv/client-go/pull/648/files#diff-34812587e42268a5d84933be286601fc58fdff17ab336b09ba75455ffe5ed447R96. I prefer keeping it there.

JmPotato · 2023-01-19T10:45:51Z

pkg/mcs/resource_manager/client/client.go

+
+// OnResponse is used to consume tokens atfer receiving response
+func (c *ResourceGroupsController) OnResponse(_ context.Context, resourceGroupName string, req RequestInfo, resp ResponseInfo) error {
+	if _, ok := defaultWhiteList[resourceGroupName]; ok {


Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato · 2023-01-19T11:31:50Z

/merge

ti-chi-bot · 2023-01-19T11:31:52Z

@JmPotato: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

ti-chi-bot · 2023-01-19T11:31:54Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 37abfa8

impl resouce manager tenant side client

75190c9

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-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. labels Dec 29, 2022

ti-chi-bot requested review from nolouch and rleungx December 29, 2022 13:14

nolouch reviewed Dec 29, 2022

View reviewed changes

pkg/mcs/resource_manager/tenant_client/limiter.go Outdated Show resolved Hide resolved

CabinfeverB and others added 2 commits January 9, 2023 16:41

Merge branch 'master' into resource-manager/tenant_side_client_1

0e3c929

Introduce the RU coefficient config and refine some code

cf6b0d0

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato reviewed Jan 11, 2023

View reviewed changes

Refine the code

1a6271d

Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato mentioned this pull request Jan 11, 2023

Introduce the resource control client tikv/client-go#648

Merged

CabinfeverB mentioned this pull request Jan 11, 2023

Tracking issue for resource manager #5851

Closed

7 tasks

JmPotato reviewed Jan 11, 2023

View reviewed changes

pkg/mcs/resource_manager/tenant_client/client.go Outdated Show resolved Hide resolved

Merge branch 'master' into resource-manager/tenant_side_client_1

49a78a8

nolouch reviewed Jan 11, 2023

View reviewed changes

pkg/mcs/resource_manager/tenant_client/client.go Outdated Show resolved Hide resolved

nolouch reviewed Jan 11, 2023

View reviewed changes

pkg/mcs/resource_manager/tenant_client/client.go Outdated Show resolved Hide resolved

nolouch reviewed Jan 11, 2023

View reviewed changes

pkg/mcs/resource_manager/tenant_client/client.go Outdated Show resolved Hide resolved

CabinfeverB marked this pull request as draft January 12, 2023 02:41

ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 12, 2023

CabinfeverB added 4 commits January 12, 2023 16:56

refactor

d1a17b1

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

refactor

14228c1

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge

4ffccbc

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

merge

9141699

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> merge Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

CabinfeverB force-pushed the resource-manager/tenant_side_client_1 branch from a915b08 to 9141699 Compare January 12, 2023 09:39

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/needs-linked-issue release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jan 12, 2023

refactor limiter

395a01c

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Jan 19, 2023

View reviewed changes

address comment

606d6ba

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch reviewed Jan 19, 2023

View reviewed changes

address comment

166909f

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato reviewed Jan 19, 2023

View reviewed changes

CabinfeverB added 2 commits January 19, 2023 16:35

address comment

07b4d67

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

d711d4e

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

nolouch approved these changes Jan 19, 2023

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 19, 2023

CabinfeverB added 3 commits January 19, 2023 16:59

address comment

e83f208

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

c1b42ee

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

d1d56d1

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

BornChanger approved these changes Jan 19, 2023

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2023

JmPotato reviewed Jan 19, 2023

View reviewed changes

CabinfeverB added 2 commits January 19, 2023 18:48

merge master

16956b6

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

address comment

37abfa8

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2023

JmPotato approved these changes Jan 19, 2023

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Jan 19, 2023

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jan 19, 2023

Merge branch 'master' into resource-manager/tenant_side_client_1

91eb7b3

ti-chi-bot merged commit 402c2bf into tikv:master Jan 19, 2023

	func (gc *groupCostController) updateAvgResourcePerSec() {
	func (gc *groupCostController) updateAvgRaWResourcePerSec() {

	if !gc.calcAvg(counter, getResourceValueFromConsumption(gc.run.consumption, typ)) {
	if !gc.calcAvg(counter, getRawResourceValueFromConsumption(gc.run.consumption, typ)) {

	log.Debug("[resource group controllor] update avg ru per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))
	log.Debug("[resource group controllor] update avg raw resource per sec", zap.String("name", gc.Name), zap.String("type", rmpb.RawResourceType_name[int32(typ)]), zap.Float64("avgRUPerSec", counter.avgRUPerSec))


		var _ ResourceGroupKVInterceptor = (*resourceGroupsController)(nil)

		type resourceGroupsController struct {

	// Copyright 2022 TiKV Project Authors.
	// Copyright 2023 TiKV Project Authors.

	lastedGroups := make(map[string]struct{})
	latestGroups := make(map[string]struct{})

Conversation

CabinfeverB commented Dec 29, 2022 • 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 commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nolouch left a comment

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!

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

nolouch left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Jan 19, 2023

Uh oh!

JmPotato left a comment

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!

JmPotato commented Jan 19, 2023

Uh oh!

ti-chi-bot commented Jan 19, 2023

Uh oh!

ti-chi-bot commented Jan 19, 2023

Uh oh!

Reviewers

Assignees

Labels

CabinfeverB commented Dec 29, 2022 •

edited

Loading

ti-chi-bot commented Dec 29, 2022 •

edited

Loading

codecov bot commented Dec 29, 2022 •

edited

Loading