Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor TiKV auto-scaling #2862

Merged
merged 7 commits into from
Jul 6, 2020
Merged

Refactor TiKV auto-scaling #2862

merged 7 commits into from
Jul 6, 2020

Conversation

Yisaer
Copy link
Contributor

@Yisaer Yisaer commented Jul 3, 2020 •
edited by cofyc
Loading

What problem does this PR solve?

  1. refactor the tikv auto-scaling controller to be ready for the storage auto-scaling
  2. deprecated the de-nosie strategy due to it make auto-scaling in operator too complicated, and we will finally let pd to handle the auto-scaling strategy layer, and auto-scaler controller become the execution layer.

Related changes

  • Need to cherry-pick to the release branch

Does this PR introduce a user-facing change?:

Remove  `readyToScaleThresholdSeconds` in `TidbClusterAutoScaler`, Operator won't support de-noise in `TidbClusterAutoScaler`

@Yisaer
refactor
be2c895 
fix lint

mod tidy

update crd

revise funtion
@Yisaer Yisaer force-pushed the add_tikv_storage_auto_scaling branch from 0c1dea3 to be2c895 Compare July 3, 2020 09:42
@Yisaer Yisaer added the area/auto-scaling related to auto-scaling label Jul 3, 2020
@Yisaer Yisaer changed the title Refactor Tikv auto-scaling Refactor TiKV auto-scaling Jul 3, 2020
@cofyc
Copy link
Contributor

cofyc commented Jul 3, 2020 •
edited
Loading

Suggested release notes: Deprecate `readyToScaleThresholdSeconds` in `TidbClusterAutoScaler` as the operator will no longer support de-noise strategy

cofyc
cofyc reviewed Jul 3, 2020
View reviewed changes
pkg/apis/pingcap/v1alpha1/tidbclusterautoscaler_types.go Outdated
@@ -85,6 +85,7 @@ type TidbClusterAutoScalerSpec struct {
type TikvAutoScalerSpec struct {
BasicAutoScalerSpec `json:",inline"`

// Deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in which version you plan to remove these fields? you can add a note about this, e.g. Deprecated in v1.1.3, planned for removal in v1.x.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

pkg/apis/pingcap/v1alpha1/tidbclusterautoscaler_types.go Outdated Show resolved Hide resolved
pkg/apis/pingcap/v1alpha1/tidbclusterautoscaler_types.go Outdated Show resolved Hide resolved
@Yisaer Yisaer requested a review from cofyc July 6, 2020 03:25
DanielZhangQD
DanielZhangQD reviewed Jul 6, 2020
View reviewed changes
pkg/apis/pingcap/v1alpha1/tidbclusterautoscaler_types.go Outdated
@@ -85,6 +85,7 @@ type TidbClusterAutoScalerSpec struct {
type TikvAutoScalerSpec struct {
BasicAutoScalerSpec `json:",inline"`

// Deprecated in v1.1.3, planned for removal in v1.3
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this?
It's not reasonable to scale as soon as the metric exceeds the threshold.
BTW, even if the PD will handle the metrics later, we can remove this when the similar function is implemented in PD. WDYT @cofyc

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is in @Yisaer's plan, replaced by PD before v1.3.

Copy link
Contributor Author

@Yisaer Yisaer Jul 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the current de-noise design in operator is too complicated and haven't been verified whether it is necessary. If we decided let PD make the auto-scaling plan, I think operator could only expose api and do the execution. That will be quite simple and clear, easy to know the status and maintain.

@Yisaer Yisaer requested a review from DanielZhangQD July 6, 2020 06:54
DanielZhangQD
DanielZhangQD approved these changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@DanielZhangQD DanielZhangQD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cofyc
Copy link
Contributor

cofyc commented Jul 6, 2020

please update the release note to reflect the latest change

@Yisaer
Copy link
Contributor Author

Yisaer commented Jul 6, 2020

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@Yisaer merge failed.

@Yisaer
Copy link
Contributor Author

Yisaer commented Jul 6, 2020

/run-e2e-test

1 similar comment
@Yisaer
Copy link
Contributor Author

Yisaer commented Jul 6, 2020

/run-e2e-test

@Yisaer
Copy link
Contributor Author

Yisaer commented Jul 6, 2020

/merge

@ti-srebot
Copy link
Contributor

Sorry @Yisaer, you don't have permission to trigger auto merge event on this branch. The number of LGTM for this PR is 0 while it needs 2 at least

@Yisaer
Copy link
Contributor Author

Yisaer commented Jul 6, 2020

/run-e2e-in-kind

@Yisaer
Copy link
Contributor Author

Yisaer commented Jul 6, 2020

/run-e2e-tests

@Yisaer Yisaer merged commit c018634 into pingcap:master Jul 6, 2020
ti-srebot pushed a commit to ti-srebot/tidb-operator that referenced this pull request Jul 6, 2020
@Yisaer @ti-srebot
cherry pick pingcap#2862 to release-1.1
3bf11be 
Signed-off-by: ti-srebot <[email protected]>
@ti-srebot ti-srebot mentioned this pull request Jul 6, 2020
Merged
@ti-srebot
Copy link
Contributor

cherry pick to release-1.1 in PR #2871

@DanielZhangQD DanielZhangQD mentioned this pull request Jul 7, 2020
Closed
cofyc pushed a commit that referenced this pull request Jul 7, 2020
@ti-srebot @Yisaer
cherry pick #2862 to release-1.1 (#2871)
fadcee6 
Signed-off-by: ti-srebot <[email protected]>

Co-authored-by: Song Gao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cofyc cofyc cofyc approved these changes

@DanielZhangQD DanielZhangQD DanielZhangQD approved these changes

@weekface weekface Awaiting requested review from weekface

@zjj2wry zjj2wry Awaiting requested review from zjj2wry

Assignees
No one assigned
Labels
area/auto-scaling related to auto-scaling CanMerge needs-cherry-pick-1.1 status/can-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Yisaer @cofyc @ti-srebot @DanielZhangQD