-
Notifications
You must be signed in to change notification settings - Fork 712
autostart: Ensure instance is started/stopped by launchd or systemctl
#4139
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
783c0f2 to
428fcb1
Compare
|
How to test this PR? |
|
Also please explain what issue is being solved by this PR |
opened #4141 |
ac9db28 to
31d7007
Compare
89c500c to
7082f48
Compare
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.
Should be *_linux.go?
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.
Code that is platform-dependent and can only be built on Linux is already isolated in systemd_linux.go. systemd.go has code that can be built on any platform, only the operation is platform-dependent.
If we put the code itself included in systemd.go into systemd_linux.go, I think the number of dummy codes required for systemd_others.go will increase.
|
|
||
| [Service] | ||
| ExecStart={{.Binary}} start %i --foreground | ||
| ExecStart={{.Binary}} start %i --foreground --progress |
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.
?
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.
Should be *_darwin.go
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 code included in launchd.go is based on the darwin platform, but the build is not based on the darwin platform.
The runtime guard should be enough.
| <string>start</string> | ||
| <string>{{ .Instance }}</string> | ||
| <string>--foreground</string> | ||
| <string>--progress</string> |
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.
?
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.
Since there is no way to arbitrarily pass the --progress option from the limactl start process that the user starts to the limactl hostagent that is started via launchctl, It is to be handed over at the time of registration.
limactl hostagent collects progress logs regardless of whether there is a process to observe progress.
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 is --progress needed 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.
To pass --progress to limactl hostagent.
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.
User -> limactl start --progress -> launchctl -> launchd -> limactl start --progress -> limactl hostagent --progress.
There is no api to passing --progress from user initiated limactl start to limactl hostagent via launchctl.
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.
Don't we need to support propagating other limactl start flags too?
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.
For the time being, what I thought was necessary was the --progress flag, and I included it in this PR because it can be realized without changing the contents of the registration file when limactl start is executed.
I don't know the need for other flags.
I wanted to be able to change the environment variables rather than the flag, but I thought it would be too big to include it, so I put it off.
7082f48 to
994efee
Compare
I was researching how to test and took a detour to fix |
994efee to
f3861a0
Compare
| if runtime.GOOS == "windows" { | ||
| t.Skip("skipping testing on windows host") | ||
| } |
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.
We can instead add //go:build !windows at the top of the file.
| if runtime.GOOS == "windows" { | ||
| t.Skip("skipping testing on windows host") | ||
| } |
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.
We can instead add //go:build !windows at the top of the file.
2af6c71 to
497f9ed
Compare
- Hide creating/deleting an autostart entry file into `Register`/`Unregister` as the implementation details. - Separate the `launchd` and `systemd`-specific code into packages. Signed-off-by: Norio Nomura <norio.nomura@gmail.com>
…ctl` If the instance isn’t launched by `launchd`, it won’t be stopped on log out. This changes to use `launchctl` or `systemctl` to start/stop the instance if it’s registered to autostart. ## Affected sub commands: - `limactl start` - `limactl stop` - `limactl restart` - `limactl edit` - `limactl shell` ## API changes ### `pkg/autostart` - Introduced `AutoStartedIdentifier()`: - If not empty, it indicates whether the instance was started by `launchd` or `systemd`. - Added `RequestStart()`: - Delegates the operation to `launchd` or `systemd`. - Added `RequestStop()`: - Delegates the operation to `launchd` or `systemd`. ### `pkg/autostart/launchd` - Added `AutoStartedServiceName()`: - Uses the XPC_SERVICE_NAME environment variable as the service name. - Added `RequestStart()`: - Uses `launchctl enable service-target` to avoid failing `bootstrap`. - Uses `launchctl bootstrap domain-target plist-path`. - Added `RequestStop()`: - Uses `launchctl bootout service-target` if the instance is launched by `launchd`. - Added `--progress` to the `limactl` option in `io.lima-vm.autostart.INSTANCE.plist`: - Required to support `limactl start --progress`. ### `pkg/autostart/systemd` - Added `AutoStartedServiceName()`: - Uses `CurrentUnitName()` by `github.com/coreos/go-systemd/v22/util` as the service identifier. - Added `RequestStart()`: - Uses `systemctl --user start unit-name`. - Added `RequestStop()`: - Uses `systemctl --user stop unit-name` if the instance is launched by `systemd`. - Added `--progress` to the `limactl` option in `lima-vm@INSTANCE.service`: - Required to support `limactl start --progress`. ### `pkg/hostagent` - Add `AutoStartedIdentifier` to `Info`. - If not empty, it indicates whether the instance was started by `launchd` or `systemd`. ### `pkg/instance` - `StartWithPaths()`: - Use `autostart.IsRegistered()` to check if the instance is registered to autostart. - If `launchHostAgentForeground` is true, ignore autostart registration. - If the instance is registered to autostart, use `autostart.RequestStart()` instead of launching HostAgent. - `StopGracefully()`: - Use `autostart.RequestStop()`. - `Restart()`, `RestartForcibly()`: - Use `autostart.IsRegistered()` to skip `networks.Reconcile()` if the instance is registered to autostart. Signed-off-by: Norio Nomura <norio.nomura@gmail.com> bump `coreos/go-systemd` to v22.6.0 Signed-off-by: Norio Nomura <norio.nomura@gmail.com> # Conflicts: # cmd/limactl/edit.go # cmd/limactl/shell.go # cmd/limactl/start.go # pkg/instance/restart.go
…hctl bootstrap` Because instances may be stopped without unloading the plist file. e.g. `limactl stop -f` or `limactl factory-reset`. If the plist file is not unloaded, `launchctl bootstrap` will fail. Signed-off-by: Norio Nomura <norio.nomura@gmail.com>
Signed-off-by: Norio Nomura <norio.nomura@gmail.com>
497f9ed to
4a9a91e
Compare
This PR is formed with two parts:
autoStartManagerto prepare for the following commitlaunchdorsystemctlRefactor to add
autoStartManagerRegister/Unregisteras the implementation details.launchdandsystemd-specific code into packages.Use
launchctlorsystemctlto start/stop the instance if it’s registered to autostart.If the instance isn’t launched by
launchd, it won’t be stopped on log out.Affected sub commands:
limactl startlimactl stoplimactl restartlimactl editlimactl shellAPI changes
pkg/autostartAutoStartedIdentifier():launchdorsystemd.RequestStart():launchdorsystemd.RequestStop():launchdorsystemd.pkg/autostart/launchdAutoStartedServiceName():RequestStart():launchctl enable service-targetto avoid failingbootstrap.launchctl bootstrap domain-target plist-path.RequestStop():launchctl bootout service-targetif the instance is launched bylaunchd.--progressto thelimactloption inio.lima-vm.autostart.INSTANCE.plist:limactl start --progress.pkg/autostart/systemdAutoStartedServiceName():CurrentUnitName()bygithub.com/coreos/go-systemd/v22/utilas the service identifier.RequestStart():systemctl --user start unit-name.RequestStop():systemctl --user stop unit-nameif the instance is launched bysystemd.--progressto thelimactloption inlima-vm@INSTANCE.service:limactl start --progress.pkg/hostagentAutoStartedIdentifiertoInfo.launchdorsystemd.pkg/instanceStartWithPaths():autostart.IsRegistered()to check if the instance is registered to autostart.launchHostAgentForegroundis true, ignore autostart registration.autostart.RequestStart()instead of launching HostAgent.StopGracefully():autostart.RequestStop().Restart(),RestartForcibly():autostart.IsRegistered()to skipnetworks.Reconcile()if the instance is registered to autostart.