-
Notifications
You must be signed in to change notification settings - Fork 200
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
fix: make sure node is drained before rebooting when rebootDelay is g… #930
base: main
Are you sure you want to change the base?
Conversation
…reater than 0 Signed-off-by: Sebastian Gaiser <sebastiangaiser@users.noreply.github.com>
I've added some concerns in the original issue #929. Let's definitely figure out how to solve for this scenario, but I'm not sure that a second drain operation after the retry wait is the right solution. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's wise to add this as is. Let's discuss on the issue first maybe?
@@ -788,6 +788,18 @@ func rebootAsRequired(nodeID string, booter reboot.Reboot, sentinelCommand []str | |||
if rebootDelay > 0 { | |||
log.Infof("Delaying reboot for %v", rebootDelay) | |||
time.Sleep(rebootDelay) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it make sense to add this conditionally on reboot delay.
@@ -788,6 +788,18 @@ func rebootAsRequired(nodeID string, booter reboot.Reboot, sentinelCommand []str | |||
if rebootDelay > 0 { | |||
log.Infof("Delaying reboot for %v", rebootDelay) | |||
time.Sleep(rebootDelay) | |||
|
|||
log.Infof("Making sure %s is drained before reboot", node.GetName()) | |||
err = drain(client, node) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don' think we should trigger another drain.
The state should be as expected before reboot, but not triggering an extra drain. See @jackfrancis 's opinion on the issue.
I like the idea, but I think we have details to iron out. |
…reater than 0
closes #929