resource_manager: add degraded mode #6063

CabinfeverB · 2023-02-28T09:45:53Z

What problem does this PR solve?

Issue Number: ref #5851

What is changed and how does it work?

Add a timer after each token request is sent. If the timer does not return successfully after one second, the controller will enter degraded mode. In degraded mode, a resource group in low-token process will receive the same fill rate as the configured.

Check List

Tests

Unit test
Integration test

Code changes

Has the configuration change
Has persistent data change

Release note

None.

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

ti-chi-bot · 2023-02-28T09:45:56Z

[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.

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

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

codecov · 2023-02-28T12:29:11Z

Codecov Report

Patch coverage: 71.79% and no project coverage change.

Comparison is base (8ba42df) 74.47% compared to head (fa69657) 74.48%.

❗ Current head fa69657 differs from pull request most recent head 7524367. Consider uploading reports for the commit 7524367 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6063   +/-   ##
=======================================
  Coverage   74.47%   74.48%           
=======================================
  Files         393      393           
  Lines       38446    38519   +73     
=======================================
+ Hits        28631    28689   +58     
- Misses       7275     7290   +15     
  Partials     2540     2540

Flag	Coverage Δ
unittests	`74.48% <71.79%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/storage/endpoint/key_path.go	`93.33% <ø> (ø)`
server/server.go	`75.27% <0.00%> (ø)`
client/resource_group/controller/controller.go	`62.20% <68.35%> (+1.43%)`	⬆️
pkg/mcs/resource_manager/server/config.go	`69.73% <71.42%> (-0.27%)`	⬇️
client/resource_group/controller/config.go	`87.50% <80.00%> (-12.50%)`	⬇️
client/resource_group/controller/limiter.go	`69.93% <100.00%> (+8.68%)`	⬆️
pkg/mcs/resource_manager/server/grpc_service.go	`67.77% <100.00%> (+0.73%)`	⬆️
pkg/mcs/resource_manager/server/manager.go	`82.20% <100.00%> (ø)`
pkg/mcs/resource_manager/server/server.go	`59.22% <100.00%> (ø)`
pkg/storage/endpoint/resource_group.go	`85.71% <100.00%> (ø)`
... and 1 more

... and 26 files with indirect coverage changes

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 in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

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

kevin-xianliu · 2023-03-02T02:17:14Z

We can have a switch to enable/disable degraded mode.

JmPotato · 2023-03-02T03:58:45Z

client/resource_group/controller/controller.go

@@ -297,6 +320,10 @@ func (c *ResourceGroupsController) sendTokenBucketRequests(ctx context.Context,
 		Requests:              requests,
 		TargetRequestPeriodMs: uint64(defaultTargetPeriod / time.Millisecond),
 	}
+	if c.run.responseDeadline == nil {
+		c.run.responseDeadline = time.NewTimer(time.Second)


I suggest using Stop() and Rest() here to re-use the timer as much as possible rather than creating a new one each time.

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

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> add degraded mode switch Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

JmPotato · 2023-03-06T07:50:52Z

client/resource_group/controller/config.go

 	)
 }

 // GenerateConfig generates the configuration by the given request unit configuration.
-func GenerateConfig(ruConfig *RequestUnitConfig) *Config {
+func GenerateConfig(ruConfig *RequestUnitConfig, rmServerConfig *RMServerConfig) *Config {


Can we merge the ruConfig and rmServerConfig into one?

JmPotato · 2023-03-06T07:51:53Z

client/resource_group/controller/controller.go

@@ -210,6 +247,7 @@ func (c *ResourceGroupsController) Stop() error {
 		return errors.Errorf("resource groups controller does not start")
 	}
 	c.loopCancel()
+	c.run.responseDeadline.Stop()


Is this necessary since there is a defer already before?

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

JmPotato · 2023-03-07T06:28:09Z

client/resource_group/controller/controller.go

@@ -112,14 +117,17 @@ func NewResourceGroupController(
 	requestUnitConfig *RequestUnitConfig,


Do we still need this parameter?

I can't decide if we need to use the specified config on the client side, so I keep it.

JmPotato · 2023-03-07T06:29:14Z

client/resource_group/controller/controller.go

@@ -31,7 +32,7 @@ import (
 )

 const (
-	requestUnitConfigPath  = "resource_group/ru_config"
+	controllerConfigPath   = "resource_group/control"


Maybe a little bit more specific would be better.

Suggested change

controllerConfigPath = "resource_group/control"

controllerConfigPath = "resource_group/controller_config"

Do we need the suffix? Other config key path don't have "_config".

JmPotato · 2023-03-07T06:29:57Z

client/resource_group/controller/config.go

+
+	// RequestUnit is the configuration determines the coefficients of the RRU and WRU cost.
+	// This configuration should be modified carefully.
+	RequestUnit RequestUnitConfig


Where are the toml and json tags?

JmPotato · 2023-03-07T06:31:07Z

pkg/mcs/resource_manager/server/config.go

+type ControllerConfig struct {
+	// EnableDegradedMode is to control whether resource control client enable degraded mode when server is disconnect.
+	EnableDegradedMode bool `toml:"enable-degraded-mode" json:"enable-degraded-mode"`
+
 	// RequestUnit is the configuration determines the coefficients of the RRU and WRU cost.
 	// This configuration should be modified carefully.
 	RequestUnit RequestUnitConfig


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

nolouch · 2023-03-08T06:49:16Z

client/resource_group/controller/controller.go

@@ -161,6 +169,9 @@ func (c *ResourceGroupsController) Start(ctx context.Context) {
 	c.initRunState()
 	c.loopCtx, c.loopCancel = context.WithCancel(ctx)
 	go func() {
+		c.run.responseDeadline = time.NewTimer(time.Second)
+		c.run.responseDeadline.Stop()


remove this line?

ref #6063 (comment), we new a Timer but don't need it send Time channel at beginning.

nolouch · 2023-03-08T06:52:12Z

client/resource_group/controller/controller.go

@@ -344,6 +381,10 @@ func (c *ResourceGroupsController) sendTokenBucketRequests(ctx context.Context,
 		Requests:              requests,
 		TargetRequestPeriodMs: uint64(defaultTargetPeriod / time.Millisecond),
 	}
+	if c.responseDeadlineCh == nil {
+		c.run.responseDeadline.Reset(time.Second)


How about considering making the deadline timeout be configurable?

nolouch · 2023-03-08T06:55:49Z

client/resource_group/controller/controller.go

+	if c.responseDeadlineCh != nil {
+		if c.run.responseDeadline.Stop() {
+			select {
+			case <-c.run.responseDeadline.C:


why need this line?

As the comment of Timer.Stop(),

Stop does not close the channel, to prevent a read from the channel succeedingincorrectly. To ensure the channel is empty after a call to Stop, check the return value and drain the channel.

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

nolouch

rest lgtm

nolouch · 2023-03-10T05:47:24Z

client/resource_group/controller/controller.go

@@ -195,6 +212,8 @@ func (c *ResourceGroupsController) Start(ctx context.Context) {
 				c.updateAvgRequestResourcePerSec()
 				if !c.run.requestInProgress {
 					c.collectTokenBucketRequests(c.loopCtx, "low_ru", true /* only select low tokens resource group */)
+				} else if c.run.inDegradedMode {


I wonder not in else may avoid some unexpect in here.

if c.run.inDegradedMode

nolouch · 2023-03-10T05:49:22Z

client/resource_group/controller/controller.go

+
+func (gc *groupCostController) applyBasicConfigForRawResourceTokenCounter() {
+	for typ, counter := range gc.run.resourceTokens {
+		if !counter.limiter.IsLowTokens() {


maybe we can not skip it to make sure be in downgrade, or add log here.

I want low-token buckets to enter degraded mode. It will log if successfully enter degraded mode

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

HuSharp · 2023-03-10T09:08:58Z

client/resource_group/controller/controller.go

+	if err != nil {
+		return nil, err
+	}


It looks duplicated

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

nolouch

lgtm

nolouch · 2023-03-13T07:10:16Z

client/resource_group/controller/config.go

@@ -61,8 +61,28 @@ const (
 	defaultWriteCostPerByte = 1. / 1024
 	// 1 RU = 3 millisecond CPU time
 	defaultCPUMsCost = 1. / 3
+
+	defaultDegradedModeWaitDuration = "1s"


we can off this by default first. after do more test then on it.

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

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

nolouch · 2023-03-15T09:05:01Z

ptal @JmPotato

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

HuSharp

rest LGTM

HuSharp · 2023-03-16T02:10:54Z

pkg/storage/endpoint/key_path.go

@@ -45,7 +45,7 @@ const (
 	// resource group storage endpoint has prefix `resource_group`
 	resourceGroupSettingsPath = "settings"
 	resourceGroupStatesPath   = "states"
-	requestUnitConfigPath     = "ru_config"
+	requestUnitConfigPath     = "controller"


Do we also need to change this requestUnitConfigPath name?

HuSharp · 2023-03-16T02:12:00Z

pkg/storage/endpoint/resource_group.go

@@ -61,7 +61,7 @@ func (se *StorageEndpoint) LoadResourceGroupStates(f func(k, v string)) error {
 	return se.loadRangeByPrefix(resourceGroupStatesPath+"/", f)
 }

-// SaveRequestUnitConfig stores the request unit config to storage.
-func (se *StorageEndpoint) SaveRequestUnitConfig(config interface{}) error {
+// SaveControllerConfig stores the request unit config to storage.


ditto for comment.

ti-chi-bot · 2023-03-16T02:12:22Z

@HuSharp: 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.

In response to this:

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

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

nolouch

lgtm

JmPotato · 2023-03-16T08:17:20Z

/merge

ti-chi-bot · 2023-03-16T08:17:23Z

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

/run-all-tests

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-03-16T08:17:23Z

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

Commit hash: fa69657

add degraded mode

4185528

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

ti-chi-bot added the release-note-none label Feb 28, 2023

ti-chi-bot requested review from JmPotato and Yisaer February 28, 2023 09:46

CabinfeverB requested review from nolouch and removed request for Yisaer February 28, 2023 12:19

fix data race

dbf29c3

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

CabinfeverB force-pushed the resource-manager/degraded_mode branch from 925b286 to dbf29c3 Compare February 28, 2023 12:20

fix data race

8b04b5c

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

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

JmPotato reviewed Mar 2, 2023

View reviewed changes

CabinfeverB added 5 commits March 2, 2023 18:46

merge master

59fd846

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

merge master

2145d9d

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

address comment

9470dd1

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

add degraded mode switch

764ee66

Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com> add degraded mode switch Signed-off-by: Cabinfever_B <cabinfeveroier@gmail.com>

Merge branch 'master' into resource-manager/degraded_mode

795b057

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

JmPotato reviewed Mar 6, 2023

View reviewed changes

address comment

9be4226

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

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

merge master

534924f

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 Mar 7, 2023

JmPotato reviewed Mar 7, 2023

View reviewed changes

address comment

8a03d91

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

nolouch reviewed Mar 8, 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 Mar 8, 2023

address comment

e26ef16

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

nolouch reviewed Mar 10, 2023

View reviewed changes

address comment

373b7ed

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 Mar 10, 2023

HuSharp reviewed Mar 10, 2023

View reviewed changes

CabinfeverB added 2 commits March 10, 2023 17:31

address comment

dbd2036

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

disable degraded mode

746d559

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

nolouch approved these changes Mar 13, 2023

View reviewed changes

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

address comment

e453550

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

CabinfeverB force-pushed the resource-manager/degraded_mode branch from cb4c4c8 to e453550 Compare March 13, 2023 07:40

CabinfeverB added 2 commits March 13, 2023 16:08

address comment

0f6679a

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

merge master

5d298c6

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

merge master

703131c

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

HuSharp approved these changes Mar 16, 2023

View reviewed changes

CabinfeverB added 2 commits March 16, 2023 14:31

address comment

1205ea1

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

address comment

2202440

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

nolouch mentioned this pull request Mar 16, 2023

resource_manager/client: introduce RU and Request metrics #6170

Merged

address comment

fa69657

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

nolouch approved these changes Mar 16, 2023

View reviewed changes

JmPotato approved these changes Mar 16, 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 Mar 16, 2023

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

Merge branch 'master' into resource-manager/degraded_mode

7524367

ti-chi-bot merged commit 7a0ce10 into tikv:master Mar 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_manager: add degraded mode #6063

resource_manager: add degraded mode #6063

CabinfeverB commented Feb 28, 2023 •

edited

Loading

ti-chi-bot commented Feb 28, 2023 •

edited

Loading

codecov bot commented Feb 28, 2023 •

edited

Loading

kevin-xianliu commented Mar 2, 2023

JmPotato Mar 2, 2023

JmPotato Mar 6, 2023

JmPotato Mar 6, 2023

JmPotato Mar 7, 2023

CabinfeverB Mar 7, 2023

JmPotato Mar 7, 2023

CabinfeverB Mar 7, 2023

JmPotato Mar 7, 2023

JmPotato Mar 7, 2023

nolouch Mar 8, 2023

CabinfeverB Mar 8, 2023

nolouch Mar 8, 2023

CabinfeverB Mar 8, 2023

nolouch Mar 8, 2023

CabinfeverB Mar 8, 2023

nolouch left a comment

nolouch Mar 10, 2023

nolouch Mar 10, 2023

CabinfeverB Mar 10, 2023

HuSharp Mar 10, 2023

nolouch left a comment

nolouch Mar 13, 2023

nolouch commented Mar 15, 2023

HuSharp left a comment

HuSharp Mar 16, 2023

HuSharp Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

nolouch left a comment

JmPotato commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

		@@ -112,14 +117,17 @@ func NewResourceGroupController(
		requestUnitConfig *RequestUnitConfig,

	controllerConfigPath = "resource_group/control"
	controllerConfigPath = "resource_group/controller_config"

resource_manager: add degraded mode #6063

resource_manager: add degraded mode #6063

Conversation

CabinfeverB commented Feb 28, 2023 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Release note

ti-chi-bot commented Feb 28, 2023 • edited Loading

codecov bot commented Feb 28, 2023 • edited Loading

Codecov Report

kevin-xianliu commented Mar 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nolouch commented Mar 15, 2023

HuSharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 16, 2023

nolouch left a comment

Choose a reason for hiding this comment

JmPotato commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

ti-chi-bot commented Mar 16, 2023

CabinfeverB commented Feb 28, 2023 •

edited

Loading

ti-chi-bot commented Feb 28, 2023 •

edited

Loading

codecov bot commented Feb 28, 2023 •

edited

Loading