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

Enhance health command to check if there are any active alarms #12150

Merged
merged 1 commit into from
Oct 22, 2020
Merged

Enhance health command to check if there are any active alarms #12150

merged 1 commit into from
Oct 22, 2020

Conversation

agargi
Copy link
Contributor

@agargi agargi commented Jul 19, 2020 •
edited
Loading

command: Enhance health command to check if there are any active alarms

Enhanced "etcdctl endpoint health" to check alarms (in addition to checking quorum read on a key). Fixes 12032

@agargi agargi changed the title command: Enhance health command to check if there are any active alarms Enhance health command to check if there are any active alarms Jul 20, 2020
xiang90
xiang90 reviewed Jul 26, 2020
View reviewed changes
etcdctl/ctlv3/command/ep_command.go Outdated

if eh.Health == true {
resp, err := cli.AlarmList(ctx)
if err == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Break out on the not error case. Save one indentation to make code cleaner.

Copy link
Contributor Author

@agargi agargi Aug 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@xiang90 Have reduced one indentation level and committed again (after squashing).

@xiang90
Copy link
Contributor

xiang90 commented Jul 26, 2020

LGTH after clean up the code a little bit.

There is no test. But have you tried it manually?

@jingyih
Copy link
Contributor

jingyih commented Jul 26, 2020

Could you provide an example output of this command?

@agargi
Copy link
Contributor Author

agargi commented Aug 2, 2020

Could you provide an example output of this command?

@jingyih Please see below:

Failure scenario:
image

Success scenario:
image

@agargi
Copy link
Contributor Author

agargi commented Aug 3, 2020

LGTH after clean up the code a little bit.

There is no test. But have you tried it manually?

@xiang90 Yes. Please refer to snapshots pasted in other message.

jingyih
jingyih reviewed Aug 13, 2020
View reviewed changes
etcdctl/ctlv3/command/ep_command.go

if eh.Health == true {
resp, err := cli.AlarmList(ctx)
if err == nil && len(resp.Alarms) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

if err != nil, eh.Health should be false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jingyih By default, eh.Health is set to false. If the "cli.get" call fails, it means key-based health check has failed and hence no need to check for alarm-based health check. Only if key-based health check succeeds (which means eh.Health is set to true), alarm-based health check is performed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh sorry I was referring to the err on line 138. If we cannot get alarm list, i.e., cli.AlarmList returns a non-nil err, the health check should also fail, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jingyih Oh sorry for missing this. Made code change and committed again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jingyih Can you please review this and commit (if change is acceptable)?

jingyih
jingyih reviewed Sep 16, 2020
View reviewed changes
Copy link
Contributor

@jingyih jingyih left a comment

Choose a reason for hiding this comment

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

Sorry for the late response. The logic looks right to me. Just one nit.

etcdctl/ctlv3/command/ep_command.go Outdated
@@ -16,6 +16,7 @@ package command

import (
"fmt"
"go.etcd.io/etcd/v3/etcdserver/etcdserverpb"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: move this to the next code block (i.e., somewhere around line 24)

@xiang90
Copy link
Contributor

xiang90 commented Oct 19, 2020

/cc @agargi can you resolve the one nit? then it should be ready for merging. thanks!

@agargi agargi force-pushed the etcdctl_health_endpoint branch 6 times, most recently from 0d33d59 to a94f6da Compare October 21, 2020 15:19
@agargi
Copy link
Contributor Author

agargi commented Oct 21, 2020 •
edited
Loading

@ptabor Can you pls help here resolve the conflict? You made re-alignment around package names but I'm not sure why I'm getting a conflict (since I have now added a new import as per the new structure).

https://github.com/etcd-io/etcd/pull/12150/conflicts

@ptabor
Copy link
Contributor

ptabor commented Oct 22, 2020

Rebasing this commit on top of current master should solve the problem:

git checkout master
git pull
git checkout etcdctl_health_endpoint
git rebase master

@agargi
Copy link
Contributor Author

agargi commented Oct 22, 2020

Thanks @ptabor

@xiang90 Conflict is resolved and code can be merged now

@xiang90 xiang90 merged commit 8866d55 into etcd-io:master Oct 22, 2020
@agargi agargi deleted the etcdctl_health_endpoint branch October 24, 2020 16:21
@tobgu tobgu mentioned this pull request Aug 25, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jingyih jingyih jingyih left review comments

@xiang90 xiang90 xiang90 left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

etcdctl endpoint health should check for alarms
4 participants
@agargi @xiang90 @jingyih @ptabor