resource_controller: load RU config from the remote server when initializing controller #6048

JmPotato · 2023-02-24T08:37:50Z

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

What problem does this PR solve?

Issue Number: ref #6038. Should merge after #6041.

What is changed and how does it work?

- Load RU config from the remote server when initializing controller.

Check List

Tests

Unit test
Integration test

Release note

None.

ti-chi-bot · 2023-02-24T08:37:52Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

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

JmPotato · 2023-02-24T08:38:30Z

/cc @HuSharp

Please help take a look also.

ti-chi-bot · 2023-02-24T08:38:32Z

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

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

In response to this:

/cc @HuSharp

Please help take a look also.

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/test-infra repository.

codecov · 2023-02-24T08:57:46Z

Codecov Report

Patch coverage: 57.89% and project coverage change: -0.10 ⚠️

Comparison is base (f999ad5) 74.00% compared to head (ea85630) 73.90%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6048      +/-   ##
==========================================
- Coverage   74.00%   73.90%   -0.10%     
==========================================
  Files         380      380              
  Lines       37728    37740      +12     
==========================================
- Hits        27920    27892      -28     
- Misses       7352     7377      +25     
- Partials     2456     2471      +15

Flag	Coverage Δ
unittests	`73.90% <57.89%> (-0.10%)`	⬇️

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

Impacted Files	Coverage Δ
client/resource_group/controller/controller.go	`61.37% <55.55%> (-0.24%)`	⬇️
client/resource_group/controller/config.go	`100.00% <100.00%> (+62.50%)`	⬆️
pkg/errs/errs.go	`75.00% <0.00%> (-25.00%)`	⬇️
pkg/mcs/discovery/register.go	`53.48% <0.00%> (-18.61%)`	⬇️
pkg/tso/local_allocator.go	`62.16% <0.00%> (-9.46%)`	⬇️
pkg/dashboard/adapter/manager.go	`79.31% <0.00%> (-9.20%)`	⬇️
pkg/election/leadership.go	`76.28% <0.00%> (-4.13%)`	⬇️
pkg/utils/etcdutil/etcdutil.go	`76.42% <0.00%> (-4.07%)`	⬇️
pkg/tso/allocator_manager.go	`63.20% <0.00%> (-2.60%)`	⬇️
pkg/member/member.go	`62.69% <0.00%> (-1.56%)`	⬇️
... and 14 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.

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

HuSharp · 2023-03-01T03:20:47Z

client/resource_group/controller/controller.go

+			return nil, err
+		}
+	}
+	// TODO: do we need to reload the config from remote server periodically?


Do u mean we need to watch it?

Yes, but since the RU model config won't be modified frequently, I think it's ok to leave this as a nice-to-have option.

Maybe we can consider adding an API to the triage reload later. it's more "manual" and no need to restart the node.

CabinfeverB · 2023-03-01T07:09:12Z

client/resource_group/controller/controller.go

+			return nil, err
+		}
+	}
+	// TODO: do we need to reload the config from remote server periodically?


IMO, reload is not required for now, and the cluster control surface can restart tidb

I just removed this TODO.

HuSharp · 2023-03-01T07:39:15Z

client/resource_group/controller/controller.go

+	var err error
+	if requestUnitConfig == nil {
+		if requestUnitConfig, err = loadRequestUnitConfig(ctx, provider); err != nil {
+			return nil, err


If we don't need to reload, maybe we can move the error in if block.

This is because requestUnitConfig has been defined as the function parameter, we can not re-assign it with :=.

I mean what about

if requestUnitConfig == nil { var err error

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

HuSharp

LGTM!

ti-chi-bot · 2023-03-01T07:58:13Z

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

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.

nolouch · 2023-03-01T09:44:37Z

/merge

ti-chi-bot · 2023-03-01T09:44:39Z

@nolouch: 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-01T09:44:41Z

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

Commit hash: ea85630

…alizing controller (tikv#6048) ref tikv#6038, ref tikv#6041 - Load RU config from the remote server when initializing controller. Signed-off-by: JmPotato <ghzpotato@gmail.com>

JmPotato added the component/resource-control label Feb 24, 2023

JmPotato requested review from nolouch and CabinfeverB February 24, 2023 08:37

ti-chi-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 24, 2023

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

JmPotato force-pushed the remote_ru_model branch from 98fa01e to 12d1230 Compare March 1, 2023 02:23

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

Load RU config from the remote server when initializing controller

db3c363

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

JmPotato force-pushed the remote_ru_model branch from 12d1230 to db3c363 Compare March 1, 2023 02:27

HuSharp reviewed Mar 1, 2023

View reviewed changes

CabinfeverB approved these changes Mar 1, 2023

View reviewed changes

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

HuSharp reviewed Mar 1, 2023

View reviewed changes

JmPotato added 2 commits March 1, 2023 15:44

Remove the TODO

fa9f0ff

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

Address the comment

ea85630

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

HuSharp approved these changes Mar 1, 2023

View reviewed changes

nolouch approved these changes Mar 1, 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 1, 2023

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

ti-chi-bot merged commit c82b237 into tikv:master Mar 1, 2023

JmPotato deleted the remote_ru_model branch March 1, 2023 09:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resource_controller: load RU config from the remote server when initializing controller #6048

resource_controller: load RU config from the remote server when initializing controller #6048

JmPotato commented Feb 24, 2023 •

edited

Loading

ti-chi-bot commented Feb 24, 2023 •

edited

Loading

JmPotato commented Feb 24, 2023

ti-chi-bot commented Feb 24, 2023

codecov bot commented Feb 24, 2023 •

edited

Loading

HuSharp Mar 1, 2023 •

edited

Loading

JmPotato Mar 1, 2023

nolouch Mar 1, 2023 •

edited

Loading

CabinfeverB Mar 1, 2023

JmPotato Mar 1, 2023

HuSharp Mar 1, 2023

JmPotato Mar 1, 2023

HuSharp Mar 1, 2023

JmPotato Mar 1, 2023

HuSharp left a comment

ti-chi-bot commented Mar 1, 2023

nolouch commented Mar 1, 2023

ti-chi-bot commented Mar 1, 2023

ti-chi-bot commented Mar 1, 2023

resource_controller: load RU config from the remote server when initializing controller #6048

resource_controller: load RU config from the remote server when initializing controller #6048

Conversation

JmPotato commented Feb 24, 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 24, 2023 • edited Loading

JmPotato commented Feb 24, 2023

ti-chi-bot commented Feb 24, 2023

codecov bot commented Feb 24, 2023 • edited Loading

Codecov Report

HuSharp Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

JmPotato Mar 1, 2023

Choose a reason for hiding this comment

nolouch Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

CabinfeverB Mar 1, 2023

Choose a reason for hiding this comment

JmPotato Mar 1, 2023

Choose a reason for hiding this comment

HuSharp Mar 1, 2023

Choose a reason for hiding this comment

JmPotato Mar 1, 2023

Choose a reason for hiding this comment

HuSharp Mar 1, 2023

Choose a reason for hiding this comment

JmPotato Mar 1, 2023

Choose a reason for hiding this comment

HuSharp left a comment

Choose a reason for hiding this comment

ti-chi-bot commented Mar 1, 2023

nolouch commented Mar 1, 2023

ti-chi-bot commented Mar 1, 2023

ti-chi-bot commented Mar 1, 2023

JmPotato commented Feb 24, 2023 •

edited

Loading

ti-chi-bot commented Feb 24, 2023 •

edited

Loading

codecov bot commented Feb 24, 2023 •

edited

Loading

HuSharp Mar 1, 2023 •

edited

Loading

nolouch Mar 1, 2023 •

edited

Loading