Skip to content

fix: Do not set RebootTriggeredAt to nil too early#2125

Merged
guettli merged 5 commits into
mainfrom
fix/reboot-triggered-at-nil
Jun 30, 2026
Merged

fix: Do not set RebootTriggeredAt to nil too early#2125
guettli merged 5 commits into
mainfrom
fix/reboot-triggered-at-nil

Conversation

@guettli

@guettli guettli commented Jun 26, 2026

Copy link
Copy Markdown
Collaborator

Summary

RebootTriggeredAt was set to nil to early.

Then hasJustRebooted() returned true forever when RebootTriggeredAt is nil but ErrorType is SSHRebootTriggered.

This was introduced in 4f9d7c6 in April this year:

        if s.scope.HetznerBareMetalHost.Spec.Status.ErrorType == infrav1.ErrorTypeConnectionError {
                s.scope.HetznerBareMetalHost.ClearError()
+               s.scope.HetznerBareMetalHost.Spec.Status.RebootTriggeredAt = nil
        }

How this state is reached

The exact path that produces ErrorType=SSHRebootTriggered with RebootTriggeredAt=nil:

  1. During a reboot, the server becomes temporarily unreachable → handleIncompleteBoot sets ErrorType=ConnectionError, RebootTriggeredAt=<time>.
  2. On the next reconcile the server is reachable again → lines 435–437 clear the connection error and set RebootTriggeredAt=nil.
  3. SSH still times out (server hasn't fully rebooted yet) → the emptyErrorType case sets ErrorType=SSHRebootTriggered but (before this fix) never re-sets RebootTriggeredAt.
  4. hasJustRebooted() sees SSHRebootTriggered + nil timestamp → hasTimedOut(nil, …) returns false!false == true → stuck forever.

… is nil

When RebootTriggeredAt gets cleared (e.g. via ClearError path from connection
error handling) but ErrorType is still SSHRebootTriggered, hasJustRebooted()
incorrectly returns true forever because hasTimedOut(nil, ...) returns false.

Fix: return false early when RebootTriggeredAt is nil.
Also set RebootTriggeredAt when first detecting SSH timeout in emptyErrorType case
so that escalation timeouts are computed correctly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Committing as: thomas.guettler@syself.com
@github-actions github-actions Bot added size/XS Denotes a PR that changes 0-20 lines, ignoring generated files. area/code Changes made in the code directory labels Jun 26, 2026
Without the nil guard added in the fix, hasTimedOut(nil, ...) returns
false, causing hasJustRebooted() to return true indefinitely when a
reboot error type is set but RebootTriggeredAt was never populated.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Committing as: thomas.guettler@syself.com
@github-actions github-actions Bot added size/S Denotes a PR that changes 20-50 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-20 lines, ignoring generated files. labels Jun 26, 2026
@guettli guettli requested a review from janiskemper June 26, 2026 13:59
Comment thread pkg/services/baremetal/host/host.go Outdated
@guettli guettli requested a review from janiskemper June 29, 2026 10:58
@janiskemper

Copy link
Copy Markdown
Contributor

@guettli can you update the PR title and description again and also add a comment to the utility function you updated? Basically, I would say that this PR is about fixing a bug where we set RebootedAt too early to nil.

Keeping the change in the utility function wouldn't be necessary but might be a safe guard for the future.

@guettli guettli changed the title fix: hasJustRebooted returns true indefinitely when RebootTriggeredAt is nil fix: Do not set RebootTriggeredAt to nil too early Jun 30, 2026
@guettli guettli closed this Jun 30, 2026
@guettli guettli reopened this Jun 30, 2026
@guettli

guettli commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

@guettli can you update the PR title and description again and also add a comment to the utility function you updated? Basically, I would say that this PR is about fixing a bug where we set RebootedAt too early to nil.

Keeping the change in the utility function wouldn't be necessary but might be a safe guard for the future.

@janiskemper

PR title and description got updated.

In ec53299 I added a comment and log, when the value is nil, which is not expected.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Committing as: thomas.guettler@syself.com
@guettli guettli merged commit 8515cb0 into main Jun 30, 2026
7 checks passed
@guettli guettli deleted the fix/reboot-triggered-at-nil branch June 30, 2026 08:56
guettli added a commit that referenced this pull request Jun 30, 2026
* fix: hasJustRebooted returns true indefinitely when RebootTriggeredAt is nil

When RebootTriggeredAt gets cleared (e.g. via ClearError path from connection
error handling) but ErrorType is still SSHRebootTriggered, hasJustRebooted()
incorrectly returns true forever because hasTimedOut(nil, ...) returns false.

Fix: return false early when RebootTriggeredAt is nil.
Also set RebootTriggeredAt when first detecting SSH timeout in emptyErrorType case
so that escalation timeouts are computed correctly.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Committing as: thomas.guettler@syself.com

* test: hasJustRebooted returns false when RebootTriggeredAt is nil

Without the nil guard added in the fix, hasTimedOut(nil, ...) returns
false, causing hasJustRebooted() to return true indefinitely when a
reboot error type is set but RebootTriggeredAt was never populated.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Committing as: thomas.guettler@syself.com

* remove resetting RebootTriggeredAt. We need to wait  until ssh works correctly.

* add comment and info log.

* fix lint: remove redundant embedded field name in Logger selector

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# Committing as: thomas.guettler@syself.com

---------

Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com># Committing as: thomas.guettler@syself.com
@guettli

guettli commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator Author

Cherry-picked into release-1.1 via 240eaa5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/code Changes made in the code directory size/S Denotes a PR that changes 20-50 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants