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

chore: support leave member by using any pods #7890

Closed

Conversation

newborn22
Copy link
Collaborator

related issue: #7753

@CLAassistant
Copy link

CLAassistant commented Jul 29, 2024

CLA assistant check
All committers have signed the CLA.

@newborn22 newborn22 marked this pull request as draft July 29, 2024 08:50
@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines. label Jul 29, 2024
@apecloud-bot
Copy link
Collaborator

This branch name is not following the standards: feature/|bugfix/|release/|hotfix/|support/|releasing/|dependabot/

// For the purpose of upgrade compatibility, if the version of Lorry is 0.7 and
// the version of KB is upgraded to 0.8 or newer, lorry client will return an NotImplemented error,
// in this case, here just ignore it.
if err2 == lorry.NotImplemented {
r.reqCtx.Log.Info("lorry leave member api is not implemented")
} else if unableToConnect(err2) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When a pod is pending, new lorry clients may encounter issues. This situation can also be resolved here.

// remove current member from db cluster
err = manager.LeaveMemberFromCluster(ctx, cluster, manager.GetCurrentMemberName())
// remove member from db cluster, the member may be other pod, depending on if podName is assigned in req.Parameters
err = manager.LeaveMemberFromCluster(ctx, cluster, memberNameToLeave)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change in the API can be reflected in the DBManager interface.

Comment on lines +649 to +660
if err2 := lorryCli.LeaveMember(r.reqCtx.Ctx, nil); err2 != nil {
// For the purpose of upgrade compatibility, if the version of Lorry is 0.7 and
// the version of KB is upgraded to 0.8 or newer, lorry client will return an NotImplemented error,
// in this case, here just ignore it.
if err2 == lorry.NotImplemented {
r.reqCtx.Log.Info("lorry leave member api is not implemented")
} else if unableToConnect(err2) {
r.reqCtx.Log.Info(fmt.Sprintf("when leaving pod %s by lorry, can not connect lorry on pod %s, try to leave member by other pods", pod.Name, pod.Name))
err3 := r.leaveMemberByOtherPods(desiredPods, pod)
if err == nil {
err = err3
}
Copy link
Contributor

Choose a reason for hiding this comment

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

re-org lorryCli.LeaveMember, unableToConnect and r.leaveMemberByOtherPods into one single method of r seems more readable.

@free6om free6om changed the title supporting leave member by using any pods chore: support leave member by using any pods Jul 29, 2024
@free6om free6om marked this pull request as ready for review July 29, 2024 09:16
@apecloud-bot
Copy link
Collaborator

This branch name is not following the standards: feature/|bugfix/|release/|hotfix/|support/|releasing/|dependabot/

@free6om
Copy link
Contributor

free6om commented Jul 29, 2024

fixed #7753

parameters["podName"] = podToLeave.Spec.Hostname

for _, pod := range desiredPods {
lorryCli, err1 := lorry.NewClient(*pod)
Copy link
Contributor

Choose a reason for hiding this comment

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

It should follow the API definition, otherwise there will be risks in executing the action on other pods.

@newborn22 newborn22 closed this Jul 29, 2024
@newborn22 newborn22 deleted the fix/leave_member_by_other_lorry_when_node_unhealth branch July 29, 2024 14:13
@github-actions github-actions bot added this to the 0.9.2 milestone Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/L Denotes a PR that changes 100-499 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants