- Notifications
You must be signed in to change notification settings - Fork642
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
cmd: add support for restarting a running instance#3323
base:master
Are you sure you want to change the base?
Conversation
cmd/limactl/shell.go Outdated
| } | ||
| launchHostAgentForeground := false | ||
| time.Sleep(500 * time.Millisecond) |
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.
Needing asleep is always a red flag; it means there is a race, and there is still a chance that the sleep is not long enough.
I suspect thesleep exists because we need to wait for the PID files to be deleted before we can start the instance again. I wonder if it isn't possible to make sureStopGracefully will not return until the files are deleted, and return an error when the files still exist after a somewhat longish timeout.
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 suspect the sleep exists because we need to wait for the PID files to be deleted before we can start the instance again.
It is indeed the case, theStopGracefully() function is not waiting for the host agent and driver processes to shut down, let me see if I can fix this in this PR itself.
cmd/limactl/restart.go Outdated
| restartCmd := &cobra.Command{ | ||
| Use: "restart INSTANCE", | ||
| Short: "Restart a running instance", | ||
| Args: WrapArgsError(cobra.MinimumNArgs(1)), |
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.
The implementation seems ignoringargs[1:]
pkg/instance/restart.go Outdated
| return nil | ||
| } | ||
| func waitForInstanceShutdown(ctx context.Context, inst *store.Instance) error { |
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 think this should be part ofwaitForHostAgentTermination inpkg/instance/stop.go, or at least called fromStopGracefully. That makes it available to an external caller and protectslimactl stop && limactl start from failing too.
Is there a reason we would ever wantStopGracefully to return before the processes have shut down?
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.
The reason I added this function is because I was facing this issue while restarting a VM:
FATA[0001] instance "default" seems running (hint: remove "/Users/ansumansahoo/.lima/default/ha.pid" if the instance is not running)But we don't need this function anymore becausenetworks.Reconcile() ensures everything is shutdown before a restart.
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.
But we don't need this function anymore because
networks.Reconcile()ensures everything is shutdown before a restart.
How does it ensure that? It is not immediately obvious to me. Does it just take sufficient time on your machine, so the problem no longer appears (in which case this would be a variant of thesleep mechanism, and not a guarantee)?
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.
You are correct; I didn't think of it that way. Thanks! To be on the safer side, I will add the function that ensures the instance has been shutdown before starting it again.
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.
- Needs checking for maximum number of args
- I don't understand why we don't need to wait for the instance to be stopped
- commits should be squashed
pkg/instance/restart.go Outdated
| return nil | ||
| } | ||
| func waitForInstanceShutdown(ctx context.Context, inst *store.Instance) error { |
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.
But we don't need this function anymore because
networks.Reconcile()ensures everything is shutdown before a restart.
How does it ensure that? It is not immediately obvious to me. Does it just take sufficient time on your machine, so the problem no longer appears (in which case this would be a variant of thesleep mechanism, and not a guarantee)?
I just tested restarting a stopped instance: ❯limactl restartFATA[0000] expected status "Running", got "Stopped" (maybe use `limactl stop -f`?) I think this is correct, you need to specify But I wonder if the error message is correct. The user obviously expects the instance to be running when the command returns. So the suggested command should probably be @lima-vm/maintainers WDYT? |
It probably should just start the instance automatically, with a warning |
ec0a9fb to93a8909Comparepkg/instance/stop.go Outdated
| if inst.Status != store.StatusRunning { | ||
| if isRestart, ok := ctx.Value("restart").(bool); ok && isRestart { |
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.
Is this the correct way of doing?
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.
Doesn't seem so? Is there any reason to use ctx here?
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 just tested restarting a stopped instance:
It probably should just start the instance automatically, with a warning
I usedctx here to find out who is callingStopGracefully. If the instance is being restarted and is already shut down, it won’t cause an error and will continue with starting the instance.
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.
Why not just add an argument to the function?
Signed-off-by: Ansuman Sahoo <anshumansahoo500@gmail.com>
This PR adds a new command
limactl restart INSTANCEwhich gracefully terminates and restarts Lima instances.