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.

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

Support graceful shutdown TiCDC node #4624

Merged
merged 14 commits into from
Jul 25, 2022

Conversation

overvenus
Copy link
Member

What problem does this PR solve?

Close #4623

What is changed and how does it work?

Before shutdown TiCDC node, we should first resign its ownership and move out its tables.

Code changes

  • Has Go code change
  • Has CI related scripts change

Tests

  • Unit test
  • E2E test
  • Manual test
  • No code

Scale-in 3 node TiCDC to 1 node, works fine.
image

Side effects

  • Breaking backward compatibility
  • Other side effects:

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

Release Notes

This PR add basic functionality of graceful shutdown, user can not use the feature without further PRs.

None

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Jul 13, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • DanielZhangQD
  • KanShiori

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.

@CLAassistant
Copy link

CLAassistant commented Jul 13, 2022

CLA assistant check
All committers have signed the CLA.

@overvenus overvenus requested review from july2993 and KanShiori and removed request for csuzhangxc July 13, 2022 06:30
@overvenus overvenus requested a review from 3AceShowHand July 13, 2022 06:31
@codecov-commenter
Copy link

codecov-commenter commented Jul 13, 2022

Codecov Report

Merging #4624 (c9547df) into master (d3d1551) will increase coverage by 9.91%.
The diff coverage is 73.26%.

@@            Coverage Diff             @@
##           master    #4624      +/-   ##
==========================================
+ Coverage   62.48%   72.39%   +9.91%     
==========================================
  Files         184      188       +4     
  Lines       20454    23099    +2645     
==========================================
+ Hits        12781    16723    +3942     
+ Misses       6522     5213    -1309     
- Partials     1151     1163      +12     
Flag Coverage Δ
e2e 59.36% <45.45%> (?)
unittest 62.60% <69.44%> (+0.12%) ⬆️

ns := tc.GetNamespace()
tcName := tc.GetName()

if tc.Spec.PD != nil && !tc.PDIsAvailable() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If pd or tikv is not available, the status of cdc won't update, is it acceptable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, TiCDC requires PD and TiKV service.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please note that with this change, all the operations, e.g. creation, scale, upgrade will be blocked. Is this expected?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for reminder, I'll add some comments here.

@overvenus
Copy link
Member Author

/run-pull-e2e-kind
/run-pull-e2e-kind-serial

@overvenus
Copy link
Member Author

/test pull-e2e-kind pull-e2e-kind-serial

1 similar comment
@overvenus
Copy link
Member Author

/test pull-e2e-kind pull-e2e-kind-serial

var resp drainCaptureResp
err = json.Unmarshal(body, &resp)
if err != nil {
// It is likely the TiCDC does not support the API, ignore.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this correct for the unmarshal error?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we ignore the error because old ticdc does not support this API.

@@ -219,7 +219,9 @@ func (c *defaultTidbClusterControl) updateTidbCluster(tc *v1alpha1.TidbCluster)
return err
}

// works that should be done to make the ticdc cluster current state match the desired state:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to sync CDC before TiDB?

Copy link
Member Author

Choose a reason for hiding this comment

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

No need, CDC only relies on TiKV and PD.

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

@DanielZhangQD
Copy link
Contributor

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 8360cb0

@overvenus
Copy link
Member Author

/test pull-e2e-kind pull-e2e-kind-basic pull-e2e-kind-tngm

@overvenus
Copy link
Member Author

/test pull-e2e-kind

3 similar comments
@overvenus
Copy link
Member Author

/test pull-e2e-kind

@overvenus
Copy link
Member Author

/test pull-e2e-kind

@overvenus
Copy link
Member Author

/test pull-e2e-kind

@DanielZhangQD
Copy link
Contributor

/test pull-e2e-kind-serial pull-e2e-kind-basic

@ti-chi-bot
Copy link
Member

@overvenus: Your PR was out of date, I have automatically updated it for you.

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.

@DanielZhangQD
Copy link
Contributor

/test pull-e2e-kind-across-kubernetes pull-e2e-kind

@ti-chi-bot ti-chi-bot merged commit a641d3d into pingcap:master Jul 25, 2022
overvenus added a commit to overvenus/tidb-operator that referenced this pull request Sep 8, 2022
csuzhangxc pushed a commit that referenced this pull request Sep 13, 2022
* Support graceful shutdown TiCDC node (#4624)

* ticdc: support graceful upgrade TiCDC pods (#4647)

* ticdc: wait TiCDC cluster becomes healthy after resigning owner (#4665)

Signed-off-by: Neil Shen <[email protected]>
xhebox pushed a commit to KanShiori/tidb-operator that referenced this pull request Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support graceful shutdown TiCDC node
7 participants