[teleport-update] use new updater to reload and verify Teleport #51734
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
sclevine
commented
Feb 2, 2025
| // Setup is safe to run concurrently with other Updater commands. | ||
| func (u *Updater) Setup(ctx context.Context, restart bool) error { | ||
| // Setup teleport-updater configuration and sync systemd. | ||
|
|
There was a problem hiding this comment.
I was tempted to include the linking / reverting logic here as well, but I think it introduces complexity that may be undesirable:
- The
LocalInstallerlogic would be split between versions, which may be difficult to reason about for future changes. E.g., we would not be able to change the format of the installation within a single version. - Reverting logic would move to the new version, and never execute until it's installed.
hugoShaka
approved these changes
Feb 5, 2025
lib/autoupdate/agent/updater.go
Outdated
Comment on lines
185
to
190
| // ExecSetup execs teleport-update to verify and reload the installation. | ||
| ExecSetup func(ctx context.Context, restart bool) error | ||
| // SetupConfig installs the Teleport updater service using the running installation. | ||
| SetupConfig func(ctx context.Context) error | ||
| // TeardownConfig removes all traces of the updater and all managed installations. | ||
| TeardownConfig func(ctx context.Context) error |
There was a problem hiding this comment.
Those names are not very explicit about what those functions are doing, especially the *Config part.
There was a problem hiding this comment.
Suggested change
| // ExecSetup execs teleport-update to verify and reload the installation. | |
| ExecSetup func(ctx context.Context, restart bool) error | |
| // SetupConfig installs the Teleport updater service using the running installation. | |
| SetupConfig func(ctx context.Context) error | |
| // TeardownConfig removes all traces of the updater and all managed installations. | |
| TeardownConfig func(ctx context.Context) error | |
| // VerifyAndReload execs teleport-update to verify and reload the installation. | |
| VerifyAndReload func(ctx context.Context, restart bool) error | |
| // SetupUpdater installs the Teleport updater service using the running installation. | |
| SetupUpdater func(ctx context.Context) error | |
| // Teardown removes all traces of the updater and all managed installations. | |
| Teardown func(ctx context.Context) error |
There was a problem hiding this comment.
👍 Made these names more descriptive and added better godoc.
(Note that ExecSetup re-execs the binary with the setup subcommand, while the others just execute Namespace methods.)
Comment on lines
+760
to
+780
| if ok := revertConfig(ctx); ok { | ||
| u.Log.WarnContext(ctx, "Teleport updater encountered a configuration error and successfully reverted the installation.") | ||
| } |
There was a problem hiding this comment.
Can we also log if the revert failed?
There was a problem hiding this comment.
revertConfig is a local function that always logs when it returns false.
sclevine
force-pushed
the
sclevine/split-link
branch
from
9024972 to
2a15809
Compare
|
@vapopov looking for a second review whenever you have a chance 🙂 |
vapopov
approved these changes
Feb 6, 2025
public-teleport-github-review-bot
bot
removed request for
creack and
codingllama
sclevine
force-pushed
the
sclevine/split-link
branch
from
2a15809 to
515726c
Compare
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 moves the healthcheck and agent restart logic into the copy of
teleport-updatethat is being installed byteleport-update update. This ensures that any bugs are caught on the leading-edge of the update process, so that a broken version ofteleport-updateis less likely to be installed and prevent future updates.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/opt/teleport.RFD: #47126
Goal (internal): https://github.com/gravitational/cloud/issues/10289