Restart miren.service after register instead of warning#824
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/commands/server_register.go`:
- Line 205: The resumed-pending-registration success branch returns early
without restarting the service, so call restartMirenServiceIfActive(ctx) in that
branch (the code path that handles resuming an existing pending registration and
currently returns before the fresh-registration restart call) — either insert
restartMirenServiceIfActive(ctx) immediately before the early return in the
resume/approve-pending branch or refactor both success paths to use a single
post-success handler that invokes restartMirenServiceIfActive(ctx) so the
restart runs for both fresh and resumed registrations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 055958f9-360a-4a0d-b470-7d1825ca96c1
📒 Files selected for processing (1)
cli/commands/server_register.go
09810df to
33178d2
Compare
evanphx
left a comment
There was a problem hiding this comment.
We call Register directly from Install as we're doing the install, so I'm worried that a restart here will cause unnecessary churn while the system is trying to come up for the first time.
evanphx
left a comment
There was a problem hiding this comment.
Oops, missed the check. is-active being load berring for a proper install feels... weird but fine.
The "you must now restart" warning fired at the end of every Register call, including from `miren install` where install handles the unit lifecycle itself a few lines later. So the message was either noise (during install) or easy to miss (after a standalone re-register against a live cluster). Detect a running systemd miren.service and restart it ourselves. When systemctl isn't present or the unit isn't active (fresh install, docker, dev, ad-hoc), stay silent — install or the operator already owns the lifecycle in those cases. If the restart command fails, fall back to a focused warning telling the user to bounce it manually. Fixes MIR-1155.
33178d2 to
d64067c
Compare
|
Ya fair point, reworked it so we don't rely on is-active for the install case. |
The warning at the end of
miren server register("you must now restart the miren server") was the bug, but the fix had a couple of layers. The warning fires at the end of every Register call, including from insidemiren install, which then turns around and starts the systemd unit itself a few lines later. So during install it's pure noise. During a standalone re-register against a live cluster it's at least correct, but easy to miss in all the other registration output.I went down the rabbit hole of "could register just be online and not need a restart at all" first. Short version: the cloud authenticator gets wired into RPC server options at construction time with no swap path, plus the build and deployment servers read DNSHostname by value at startup. So it's a real refactor across the auth chain to make any of it dynamic. Maybe worth it someday, but registration is a once-a-quarter operation and the surface area is large.
What we landed on instead: detect a running systemd
miren.serviceand restart it ourselves. Fresh installs (systemd or docker) hit the "unit isn't active yet" branch and stay quiet, since install handles the lifecycle a few lines later anyway. Standalone re-register against a live cluster gets an auto-restart plus a "Restarted miren.service" info line. The one case it still doesn't handle is day-2 re-register on a docker install, but docker is the demo path so it pretty much never happens. Left it.Closes MIR-1155