[teleport-update] PID-based failure detection and rollback #49175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sclevine
added
the
no-changelog
hugoShaka
approved these changes
Nov 19, 2024
hugoShaka
reviewed
Nov 19, 2024
Comment on lines
+131
to
+142
| g.Go(func() error { | ||
| return tickFile(ctx, s.PIDPath, pidC, tickC) | ||
| }) | ||
| err := s.waitForStablePID(ctx, minRunningIntervalsBeforeStable, maxCrashesBeforeFailure, | ||
| initPID, pidC, func(pid int) error { | ||
| p, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| return trace.Wrap(err) | ||
| } | ||
| return trace.Wrap(p.Signal(syscall.Signal(0))) | ||
| }) | ||
| cancel() |
There was a problem hiding this comment.
nit: is the asynchronous approach necessary here?
There was a problem hiding this comment.
Separating the PID file read and monitoring logic into separate gorountines made both parts easier to test. For example, with a single goroutine I'd have to write the PID file before or after the tick, when waitForStablePID could still be reading it. There are definitely other approaches to detecting whether waitForStablePID is finished reading (e.g., hooking verifyPID), but this was easiest for me to reason about.
vapopov
approved these changes
Nov 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds support for detecting various failures scenarios that could leave agents inaccessible during agent auto-updates.
teleport-updatewill rollback the agent version immediately in these cases.This is implemented by monitoring
/run/teleport.pidfor changes that indicate different failure modes. For example, if Teleport crashes after a softsystemctl reload, systemd is unaware, and a stale PID can be detected in/run/teleport.pidwith no running process. Alternatively, if Teleport crashes after a hard restart, the PID file is rapidly created/removed with different PID values that can be detected. Other cases, such as hanging on quit, are covered as well. This catches fatal errors in new versions, as well as client-too-new errors (e.g., v17 agent on v16 cluster).Notably, connection failures, including clients rejected by the server for being outdated, do not trigger a revert.
Compared the previous version of this functionality implemented in
teleport-ent-upgrader, this new logic detects changes faster (<30s vs. >5m), and rolls the version back instead of rolling it forward. Additionally, this logic detects failures after a soft reload, while the previous logic did not in most cases.This is the eighth in a series of PRs implementing
teleport-update:Setup Command: #49174
Link Command: #48712
Update Command: #48244
Reloading with rollbacks: #47929
Linking: #47879
Enable Command: #47565
Initial scaffolding PR: #46418
The
teleport-updatebinary will be used to enable, disable, and trigger automatic Teleport agent updates. The new auto-updates system manages a local installation of the cluster-specified version of Teleport stored in/var/lib/teleport/versions.RFD: #47126
Goal (internal): https://github.com/gravitational/cloud/issues/10289
Example: Upgrading to v17 on a v16 cluster, with successful rollback.