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

Emit an event if failed to sync labels to tikv stores #2587

Merged
merged 6 commits into from
Jun 10, 2020
Merged

Emit an event if failed to sync labels to tikv stores #2587

merged 6 commits into from
Jun 10, 2020

Conversation

PengJi
Copy link
Contributor

@PengJi PengJi commented May 28, 2020

What problem does this PR solve?

fix: #2536

What is changed and how does it work?

Check List

Tests

  • Unit test

Code changes

  • Has Go code change

Side effects

  • None

Related changes

  • Need to cherry-pick to the release branch

Does this PR introduce a user-facing change?:

Emit an event if failed to sync labels to tikv stores

@Yisaer Yisaer requested a review from weekface May 29, 2020 07:01
@Yisaer Yisaer requested review from cofyc and DanielZhangQD May 29, 2020 07:01
Yisaer
Yisaer reviewed May 29, 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.

The code LGTM. Please format pkg/manager/member/tikv_member_manager.go to pass the CI.

weekface
weekface reviewed May 29, 2020
View reviewed changes
pkg/manager/member/tikv_member_manager.go Outdated
@@ -725,7 +729,9 @@ func (tkmm *tikvMemberManager) setStoreLabelsForTiKV(tc *v1alpha1.TidbCluster) (
if !tkmm.storeLabelsEqualNodeLabels(store.Store.Labels, ls) {
set, err := pdCli.SetStoreLabels(store.Store.Id, ls)
if err != nil {
klog.Warningf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
evtStr := fmt.Sprintf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
tkmm.recorder.Event(tc, corev1.EventTypeWarning, unHealthEventReason, evtStr)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest to define a new const:

const FailedSetStoreLabels = "FailedSetStoreLabels"

and then replace unHealthEventReason to FailedSetStoreLabels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

cofyc
cofyc reviewed May 29, 2020
View reviewed changes
pkg/manager/member/tikv_member_manager.go Outdated
klog.Warningf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
evtStr := fmt.Sprintf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
tkmm.recorder.Event(tc, corev1.EventTypeWarning, FailedSetStoreLabels, evtStr)
klog.Warning(evtStr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
klog.Warning(evtStr)

this is not necessary, the recorder will print logs too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

cofyc
cofyc reviewed May 29, 2020
View reviewed changes
pkg/manager/member/tikv_member_manager.go Outdated
@@ -725,7 +729,9 @@ func (tkmm *tikvMemberManager) setStoreLabelsForTiKV(tc *v1alpha1.TidbCluster) (
if !tkmm.storeLabelsEqualNodeLabels(store.Store.Labels, ls) {
set, err := pdCli.SetStoreLabels(store.Store.Id, ls)
if err != nil {
klog.Warningf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
evtStr := fmt.Sprintf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
evtStr := fmt.Sprintf("failed to set pod: [%s/%s]'s store labels: %v", ns, podName, ls)
evtStr := fmt.Sprintf("failed to set labels %v for store (id: %d, pod: %s/%s): %v ", ls, store.Store.Id, ns, podName, err)

more informative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@cofyc
Copy link
Contributor

cofyc commented Jun 1, 2020

/run-e2e-tests

weekface
weekface approved these changes Jun 1, 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

@cofyc
Copy link
Contributor

cofyc commented Jun 8, 2020

/merge
Thanks! Great job!

@cofyc
Copy link
Contributor

cofyc commented Jun 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

@PengJi merge failed.

@cofyc
Copy link
Contributor

cofyc commented Jun 8, 2020

we had a problem in CI, will fix it soon

@cofyc
Copy link
Contributor

cofyc commented Jun 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

@PengJi merge failed.

@cofyc
Copy link
Contributor

cofyc commented Jun 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

/run-all-tests

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

@PengJi merge failed.

@cofyc
Copy link
Contributor

cofyc commented Jun 8, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 8, 2020

Your auto merge job has been accepted, waiting for:

  • 2607

@weekface
Copy link
Contributor

weekface commented Jun 9, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

Your auto merge job has been accepted, waiting for:

  • 2633

@cofyc
Copy link
Contributor

cofyc commented Jun 9, 2020

/run-e2e-tests

@cofyc
Copy link
Contributor

cofyc commented Jun 9, 2020

/merge

@sre-bot
Copy link
Contributor

sre-bot commented Jun 9, 2020

Your auto merge job has been accepted, waiting for:

  • 2671

@cofyc cofyc merged commit 619fc57 into pingcap:master Jun 10, 2020
sre-bot pushed a commit to sre-bot/tidb-operator that referenced this pull request Jun 10, 2020
@PengJi @sre-bot
cherry pick pingcap#2587 to release-1.1
d99193b 
Signed-off-by: sre-bot <sre-bot@pingcap.com>
@sre-bot sre-bot mentioned this pull request Jun 10, 2020
Merged
@sre-bot
Copy link
Contributor

sre-bot commented Jun 10, 2020

cherry pick to release-1.1 in PR #2675

sre-bot added a commit that referenced this pull request Jun 10, 2020
@sre-bot @PengJi
Emit an event if failed to sync labels to tikv stores (#2587) (#2675)
0b21296 
Signed-off-by: sre-bot <sre-bot@pingcap.com>

Co-authored-by: jipeng <jipengpro@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Yisaer Yisaer Yisaer left review comments

@cofyc cofyc cofyc approved these changes

@weekface weekface weekface approved these changes

@DanielZhangQD DanielZhangQD Awaiting requested review from DanielZhangQD

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

Successfully merging this pull request may close these issues.

Emit an event if failed to sync labels to tikv stores
5 participants
@PengJi @cofyc @sre-bot @weekface @Yisaer