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

mapping some short label names to k8s well-known labels #4688

Merged
merged 4 commits into from
Sep 15, 2022

Conversation

glorv
Copy link
Contributor

@glorv glorv commented Aug 31, 2022

What problem does this PR solve?

Support shorten some labels for k8s well-known labels when used in pd's location labels

Closes #4678

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 bootstrap a test k8s cluster and use kubectl label nodes kind-control-plane to add 2 labels topology.kubernetes.io/region=us-west-2, topology.kubernetes.io/zone=us-west-2a(kind itself can add the label kubernetes.io/hostname=kind-control-plane), then I add replicate-labels = ["region", "zone", "host"] to example/basic/tidb-cluster.yml and use it to start a cluster. After the cluster is started, tidb log contains [2022/09/14 06:09:15.541 +00:00] [INFO] [http_handler.go:2188] ["update server labels"] [labels="{\"host\":\"kind-control-plane\",\"region\":\"us-west-2\",\"zone\":\"us-west-2a\"}"], that show the labels are effective. Label the node with failure-domain.beta.kubernetes.io/region=us-west-2, failure-domain.beta.kubernetes.io/zone=us-west-2a has the same effect.
  • 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.

Allow use shorten label names to refer to some k8s well-known labels when used in pd's location labels 

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 31, 2022

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • KanShiori
  • csuzhangxc

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.

@glorv glorv requested review from KanShiori and csuzhangxc and removed request for july2993 August 31, 2022 08:20
@codecov-commenter
Copy link

codecov-commenter commented Aug 31, 2022

Codecov Report

Merging #4688 (186e5a3) into master (9d80655) will increase coverage by 6.65%.
The diff coverage is 80.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4688      +/-   ##
==========================================
+ Coverage   62.41%   69.06%   +6.65%     
==========================================
  Files         196      200       +4     
  Lines       21781    24401    +2620     
==========================================
+ Hits        13595    16853    +3258     
+ Misses       6942     6261     -681     
- Partials     1244     1287      +43     
Flag Coverage Δ
e2e 49.00% <0.00%> (?)
unittest 62.41% <100.00%> (+<0.01%) ⬆️

@glorv
Copy link
Contributor Author

glorv commented Sep 7, 2022

friendly ping @KanShiori @handlerww PTAL

Copy link
Member

@nolouch nolouch left a comment

Choose a reason for hiding this comment

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

lgtm

@ti-chi-bot
Copy link
Member

@nolouch: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

lgtm

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.

@nolouch
Copy link
Member

nolouch commented Sep 9, 2022

PTAL @KanShiori @csuzhangxc this feature is very important for some scenarios. like stale read, multi-region deploy, and placement rule.

@KanShiori
Copy link
Collaborator

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

@KanShiori
Copy link
Collaborator

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: d486eb6

@KanShiori
Copy link
Collaborator

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

@KanShiori
Copy link
Collaborator

/test pull-e2e-kind

@ti-chi-bot
Copy link
Member

@glorv: 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.

@nolouch
Copy link
Member

nolouch commented Sep 15, 2022

/test pull-e2e-kind-basic

@nolouch
Copy link
Member

nolouch commented Sep 15, 2022

Why ci failed?

@glorv
Copy link
Contributor Author

glorv commented Sep 15, 2022

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

@glorv
Copy link
Contributor Author

glorv commented Sep 15, 2022

/test pull-e2e-kind-basic

@glorv
Copy link
Contributor Author

glorv commented Sep 15, 2022

/test pull-e2e-kind

@ti-chi-bot ti-chi-bot merged commit 00a126c into pingcap:master Sep 15, 2022
@glorv glorv deleted the short-labels branch September 16, 2022 01:48
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.

shorten some default location labels
6 participants