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

tidb: support add location labels #4663

Merged
merged 14 commits into from
Aug 12, 2022
Merged

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Aug 4, 2022

What problem does this PR solve?

What is changed and how does it work?

Code changes

  • Has Go code change
  • Has CI related scripts change

Tests

  • Unit test
  • E2E test
  • Manual test
    • I uses kind to deploy a cluster in my local environment and edit the node kind-control-plane with a new label topology.kubernetes.io/zone: test. Then I add a pd config location-labels = ["topology.kubernetes.io/zone", "kubernetes.io/hostname"] to "example/basic/tidb-cluster.yaml" and deploy it to set up a cluster. When set the cluster version with 6.1.0, there is no logs about set tidb lables as expected. Then I changed the min version to 6.1.0 and redeploy operator, tidb log show logs like [2022/08/10 09:43:19.460 +00:00] [INFO] [http_handler.go:2186] ["update server labels"] [labels="{\"kubernetes.io/hostname\":\"kind-control-plane\",\"topology.kubernetes.io/zone\":\"test\",\"zone\":\"test\"}"] That show labels had been set to tidb successfully.
  • No code

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

Please refer to Release Notes Language Style Guide before writing the release note.

Support automatically set location labels for tidb-server

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 4, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • KanShiori
  • handlerww

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 Aug 4, 2022

CLA assistant check
All committers have signed the CLA.

@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2022

Codecov Report

Merging #4663 (cb9f4e9) into master (00002ed) will increase coverage by 9.76%.
The diff coverage is 62.68%.

@@            Coverage Diff             @@
##           master    #4663      +/-   ##
==========================================
+ Coverage   62.64%   72.40%   +9.76%     
==========================================
  Files         186      190       +4     
  Lines       20853    23413    +2560     
==========================================
+ Hits        13063    16953    +3890     
+ Misses       6593     5273    -1320     
+ Partials     1197     1187      -10     
Flag Coverage Δ
e2e 59.09% <7.46%> (?)
unittest 62.66% <69.49%> (+0.02%) ⬆️

@glorv
Copy link
Contributor Author

glorv commented Aug 4, 2022

@DanielZhangQD @handlerww PTAL

@glorv
Copy link
Contributor Author

glorv commented Aug 4, 2022

/test pull-e2e-kind-across-kubernetes

@glorv
Copy link
Contributor Author

glorv commented Aug 4, 2022

/run-e2e-tests

@glorv
Copy link
Contributor Author

glorv commented Aug 4, 2022

/test pull-e2e-kind

return fmt.Errorf("encode labels to json failed, error: %v", err)
}

url := fmt.Sprintf("%s/labels", c.getBaseURL(tc, ordinal))
Copy link
Member

Choose a reason for hiding this comment

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

Does it works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR in tidb side pingcap/tidb#36845 is merged.

@glorv
Copy link
Contributor Author

glorv commented Aug 8, 2022

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

@glorv
Copy link
Contributor Author

glorv commented Aug 8, 2022

/test pull-e2e-kind-serial

@glorv
Copy link
Contributor Author

glorv commented Aug 8, 2022

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

@glorv
Copy link
Contributor Author

glorv commented Aug 8, 2022

@KanShiori @handlerww PTAL

Comment on lines 1093 to 1099

pod, err := m.deps.PodLister.Pods(ns).Get(name)
if err != nil {
return setCount, fmt.Errorf("setServerLabels: failed to get pods %s for cluster %s/%s, error: %s", name, ns, tc.GetName(), err)
}

nodeName := pod.Spec.NodeName
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about using the node name in tidb status, see L1042

@glorv
Copy link
Contributor Author

glorv commented Aug 10, 2022

@KanShiori PTAL again, thank you

@KanShiori
Copy link
Collaborator

@glorv I left some advice about version check.

BTW, please test this feature and add the result in the Manual test section.

@glorv
Copy link
Contributor Author

glorv commented Aug 10, 2022

@glorv I left some advice about version check.

BTW, please test this feature and add the result in the Manual test section.

I have update the version change and manual test result. PTAL again thanks @KanShiori

Copy link
Collaborator

@KanShiori KanShiori left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

@glorv
Copy link
Contributor Author

glorv commented Aug 11, 2022

@DanielZhangQD @handlerww PTAL

Copy link
Contributor

@handlerww handlerww left a comment

Choose a reason for hiding this comment

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

lgtm

@glorv
Copy link
Contributor Author

glorv commented Aug 11, 2022

/merge

@ti-chi-bot
Copy link
Member

@glorv: /merge is only allowed for the committers, you can assign this pull request to the committer in list by filling /assign @committer in the comment to help merge this pull request.

In response to this:

/merge

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.

@glorv
Copy link
Contributor Author

glorv commented Aug 11, 2022

@KanShiori Shall we merge this PR now?

@glorv
Copy link
Contributor Author

glorv commented Aug 11, 2022

/run-e2e-tests

@KanShiori
Copy link
Collaborator

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: cb9f4e9

@glorv
Copy link
Contributor Author

glorv commented Aug 11, 2022

/test pull-e2e-kind-serial

1 similar comment
@glorv
Copy link
Contributor Author

glorv commented Aug 12, 2022

/test pull-e2e-kind-serial

@ti-chi-bot ti-chi-bot merged commit 4f22c6e into pingcap:master Aug 12, 2022
@glorv glorv deleted the tidb-labels branch August 12, 2022 07:30
@kolbe
Copy link
Contributor

kolbe commented Aug 12, 2022

@glorv why doesn't this PR include release notes?

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.

8 participants