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

add reboot queue backoff reset command #667

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Conversation

YZ775
Copy link
Contributor

@YZ775 YZ775 commented Oct 20, 2023

This PR is implementation of following issue.

@YZ775 YZ775 self-assigned this Oct 20, 2023
@YZ775 YZ775 force-pushed the add-reboot-queue-backoff-reset branch 4 times, most recently from 5d5e89e to 78cd6ef Compare October 23, 2023 08:41
@YZ775 YZ775 marked this pull request as ready for review October 23, 2023 08:48
@YZ775 YZ775 force-pushed the add-reboot-queue-backoff-reset branch from 78cd6ef to 7dbab97 Compare October 23, 2023 08:50
@YZ775 YZ775 requested review from zeroalphat and yokaze October 25, 2023 01:23
zeroalphat
zeroalphat previously approved these changes Oct 25, 2023
Copy link
Contributor

@zeroalphat zeroalphat left a comment

Choose a reason for hiding this comment

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

LGTM

docs/ckecli.md Outdated
### `ckecli reboot-queue reset-backoff`

Reset `drain_backoff_count` and `drain_backoff_expire` of the entries in reboot queue.
Resetting these values allows CKE to try draining nodes again immediately.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • It is better to use a verb that guarantees the action being actually done, instead of using allow.
  • This command cause the retry of entire reboot sequence, not only draining.
  • Try V-ing means to check the result of the action. Use "try to" if it may fail.
Suggested change
Resetting these values allows CKE to try draining nodes again immediately.
Resetting these values makes CKE try to reboot nodes again immediately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. I fixed it.

@YZ775 YZ775 force-pushed the add-reboot-queue-backoff-reset branch from 7dbab97 to 3b4af28 Compare October 25, 2023 06:05
@YZ775 YZ775 requested review from yokaze and zeroalphat October 25, 2023 06:07
Copy link
Contributor

@yokaze yokaze left a comment

Choose a reason for hiding this comment

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

LGTM

@YZ775 YZ775 merged commit 3b4a6f4 into main Oct 25, 2023
6 checks passed
@YZ775 YZ775 deleted the add-reboot-queue-backoff-reset branch October 25, 2023 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants