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

scaling for tiflash #2237

Merged
merged 7 commits into from
Apr 23, 2020
Merged

scaling for tiflash #2237

merged 7 commits into from
Apr 23, 2020

Conversation

DanielZhangQD
Copy link
Contributor

@DanielZhangQD DanielZhangQD commented Apr 20, 2020 •
edited
Loading

What problem does this PR solve?

Scaling for TiFlash

What is changed and how does it work?

Add new function to support scaling TiFlash

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
    • Scale-out TiFlash successfully, Scale in TiFlash successfully, the store is deleted and the PVCs are marked as defer deleting, Scale-out TiFlash again and the old PVCs are removed and the scaling out succeeds.
    • Scale-out PD successfully, Scale in PD successfully, the member is deleted and the PVC is marked as defer deleting, Scale-out PD again and the old PVC is removed and the scaling out succeeds.

Code changes

  • Has Go code change

Does this PR introduce a user-facing change?:

Support scaling for TiFlash

@DanielZhangQD DanielZhangQD mentioned this pull request Apr 20, 2020
Closed
13 tasks
DanielZhangQD
DanielZhangQD commented Apr 20, 2020
View reviewed changes
pkg/controller/pod_control.go
@@ -305,7 +305,6 @@ func (fpc *FakePodControl) UpdateMetaInfo(_ *v1alpha1.TidbCluster, pod *corev1.P
}

setIfNotEmpty(pod.Labels, label.NameLabelKey, TestName)
setIfNotEmpty(pod.Labels, label.ComponentLabelKey, TestComponentName)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to set it here, the caller function already set it and this will overwrite the setting in the cases.

DanielZhangQD
DanielZhangQD commented Apr 20, 2020
View reviewed changes
pkg/controller/pvc_control.go
@@ -127,6 +127,7 @@ func (rpc *realPVCControl) UpdateMetaInfo(tc *v1alpha1.TidbCluster, pvc *corev1.
if pvc.Labels[label.ClusterIDLabelKey] == clusterID &&
pvc.Labels[label.MemberIDLabelKey] == memberID &&
pvc.Labels[label.StoreIDLabelKey] == storeID &&
pvc.Labels[label.AnnPodNameKey] == podName &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add pod name label so we can list all the PVCs of one Pod.

@DanielZhangQD
Copy link
Contributor Author

@weekface comments addressed, PTAL.

@yeya24
Copy link
Contributor

yeya24 commented Apr 20, 2020

Thanks for the fix. Is there a rc release for this?

weekface
weekface previously approved these changes Apr 21, 2020
View reviewed changes
Yisaer
Yisaer suggested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@Yisaer Yisaer left a comment

Choose a reason for hiding this comment

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

I think we can directly manage tiflash in webhook.

pkg/manager/member/pd_member_manager.go Outdated Show resolved Hide resolved
pkg/manager/member/tiflash_scaler.go
return err
}

if controller.PodWebhookEnabled {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can manage tiflash scaling in the webhook instead of in the controller as webhook would be the default choice to manage the lifecyle of each component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When will Webhook be the default choice? I think it's not decided yet, btw, before v1.15, all of the requests for Pod will be sent to the Webhook if enabled, if something wrong with the Webhook, it will block the whole cluster, so I would like to support it in controller first.

pkg/manager/member/scaler.go
ns := tc.GetNamespace()
// for unit test
skipReason := map[int32]string{}
skipReason := map[string]string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why change the skipReason type here, is it for the legibility？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now there will be multiple PVCs for one Pod, the string will be podName if no PVC found for the Pod and will be the pvcName if any PVC found, and then we can verify the result in the UT cases.

@DanielZhangQD
Copy link
Contributor Author

@Yisaer comments addressed, PTAL.

@DanielZhangQD
Copy link
Contributor Author

Thanks for the fix. Is there a rc release for this?

The fixes for #2218 and #2219 are moved to #2242 and it will be cherry-picked to release-1.1, whether to release another rc is not determined yet.

weekface
weekface approved these changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@weekface weekface left a comment

Choose a reason for hiding this comment

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

LGTM

@DanielZhangQD
Copy link
Contributor Author

/run-e2e-tests

1 similar comment
@DanielZhangQD
Copy link
Contributor Author

/run-e2e-tests

Yisaer
Yisaer approved these changes Apr 23, 2020
View reviewed changes
pkg/manager/member/scaler.go
skipReason[ordinal] = skipReasonScalerPVCNotFound
return skipReason, nil
}
// pvcName := ordinalPVCName(memberType, setName, ordinal)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this line

@DanielZhangQD DanielZhangQD merged commit 4cb0761 into pingcap:master Apr 23, 2020
@DanielZhangQD DanielZhangQD deleted the flash-scale branch April 23, 2020 05:19
@DanielZhangQD
Copy link
Contributor Author

/run-cherry-picker

@sre-bot
Copy link
Contributor

sre-bot commented May 6, 2020

cherry pick to release-1.1 failed

@DanielZhangQD DanielZhangQD mentioned this pull request May 6, 2020
Merged
DanielZhangQD added a commit that referenced this pull request May 6, 2020
@DanielZhangQD
Automated cherry pick of #2237: scaling for tiflash (#2381)
6545fb9 
* scaling for tiflash

* address comments

* remove minor fixes

* revert incorrect changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@weekface weekface weekface approved these changes

@Yisaer Yisaer Yisaer approved these changes

@cofyc cofyc Awaiting requested review from cofyc

Assignees
No one assigned
Labels
needs-cherry-pick-1.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@DanielZhangQD @yeya24 @sre-bot @weekface @Yisaer