limactl start: add `--foreground` flag #2146

norio-nomura · 2024-01-17T09:35:23Z

This change adds the --foreground flag to limactl start.

This flag:

Launches limactl hostagent with syscall.Exec, which allows limactl start --foreground to be used in macOS's launchd.plist, making it possible to hide limactl hostagent.
When launched from the terminal, it uses the terminal for standard output and standard error instead of the default ha.std{out,err}.log files. In this case, a message indicating that the default log files are not used is written to the log files.
When launched from something other than the terminal, such as launchd, it uses the ha.std{out,err}.log files as before.
Cannot be used on Windows.

The following are the updated steps to set up the Docker instance of Lima to start at login using launchd on macOS:

# Create the docker instance
limactl create template://docker --vm-type vz --rosetta --network vzNAT --tty=false

# Create the launchd plist
echo '<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
	<key>Label</key>
	<string>io.lima-vm.docker</string>
	<key>ProgramArguments</key>
	<array>
		<string>'"$(which limactl)"'</string>
		<string>start</string>
		<string>--foreground</string>
		<string>docker</string>
	</array>
	<key>RunAtLoad</key>
	<true/>
	<key>StandardErrorPath</key>
	<string>launchd.stderr.log</string>
	<key>StandardOutPath</key>
	<string>launchd.stdout.log</string>
	<key>WorkingDirectory</key>
	<string>'"$(limactl list docker --format '{{.Dir}}')"'</string>
</dict>
</plist>' > ~/Library/LaunchAgents/io.lima-vm.docker.plist

# Load the launchd plist and launchd will start the docker instance
launchctl load ~/Library/LaunchAgents/io.lima-vm.docker.plist

that changed from #2140:

Changed to limactl create because it's no longer necessary to run limactl start before loading into launchd.
Changed from limactl hostagent to limactl start --foreground.
Changed Standard{Error,Out}Path to launchd.std{err,out}.log, so that only the output of limactl start is written.

AkihiroSuda · 2024-01-17T10:39:18Z

cmd/limactl/start.go

+			return err
+		}
+		if foreground {
+			ctx = start.WithLaunchHostAgentForeground(ctx, foreground)


Is there any reason to use a context for this?
Wondering if we can just add an argument to start.Start()

There's no particular reason. I just mimicked the way the timeout flag is written.

The timeout is contextualized because it has to be passed to many functions.
This is different from the foreground flag that is only used once.

Understood, I'll try changing it.

cmd/limactl/start.go

AkihiroSuda · 2024-01-17T11:34:03Z

Please squash the commits

norio-nomura · 2024-01-17T23:49:32Z

cmd/limactl/edit.go

@@ -122,7 +122,7 @@ func editAction(cmd *cobra.Command, args []string) error {
 	if err != nil {
 		return err
 	}
-	return start.Start(ctx, inst)
+	return start.Start(ctx, inst, start.DefaultLaunchHostAgentForeground)


Would it be better if the --foreground flag was also available for limactl edit?

AkihiroSuda · 2024-01-17T23:49:58Z

pkg/start/start.go

@@ -33,6 +35,10 @@ import (
 // to be running before timing out.
 const DefaultWatchHostAgentEventsTimeout = 10 * time.Minute

+// DefaultLaunchHostAgentForeground is the default value of the boolean to use for
+// Start()
+const DefaultLaunchHostAgentForeground = false


No need to define this const

AkihiroSuda

Thanks

This change adds the `--foreground` flag to `limactl start`. This flag: - Launches `limactl hostagent` with `syscall.Exec`, which allows `limactl start --foreground` to be used in macOS's `launchd.plist`, making it possible to hide `limactl hostagent`. - When launched from the terminal, it uses the terminal for standard output and standard error instead of the default `ha.std{out,err}.log` files. In this case, a message indicating that the default log files are not used is written to the log files. - When launched from something other than the terminal, such as `launchd`, it uses the `ha.std{out,err}.log` files as before. - Cannot be used on Windows. The following are the updated steps to set up the Docker instance of Lima to start at login using launchd on macOS: ```bash # Create the docker instance limactl create template://docker --vm-type vz --rosetta --network vzNAT --tty=false # Create the launchd plist echo '<?xml version="1.0" encoding="UTF-8"?> <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd"> <plist version="1.0"> <dict> <key>Label</key> <string>io.lima-vm.docker</string> <key>ProgramArguments</key> <array> <string>'"$(which limactl)"'</string> <string>start</string> <string>--foreground</string> <string>docker</string> </array> <key>RunAtLoad</key> <true/> <key>StandardErrorPath</key> <string>launchd.stderr.log</string> <key>StandardOutPath</key> <string>launchd.stdout.log</string> <key>WorkingDirectory</key> <string>'"$(limactl list docker --format '{{.Dir}}')"'</string> </dict> </plist>' > ~/Library/LaunchAgents/io.lima-vm.docker.plist # Load the launchd plist and launchd will start the docker instance launchctl load ~/Library/LaunchAgents/io.lima-vm.docker.plist ``` that changed from lima-vm#2140: - Changed to `limactl create` because it's no longer necessary to run `limactl start` before loading into `launchd`. - Changed from `limactl hostagent` to `limactl start --foreground`. - Changed `Standard{Error,Out}Path` to `launchd.std{err,out}.log`, so that only the output of `limactl start` is written. Signed-off-by: Norio Nomura <norio.nomura@gmail.com> Update cmd/limactl/start.go Change `instance` to `hostagent` Co-authored-by: Akihiro Suda <suda.kyoto@gmail.com> Add `launchHostAgentForeground` to `start.Start()` Avoid using `Context` to passing `launchHostAgentForeground` Remove `start.DefaultLaunchHostAgentForeground`

jandubois

Thanks, LGTM

jandubois · 2024-01-18T07:12:51Z

pkg/start/start.go

-	if err := haCmd.Start(); err != nil {
+	if launchHostAgentForeground {
+		logrus.Info("Running the host agent in the foreground")
+		if isatty.IsTerminal(os.Stdin.Fd()) || isatty.IsCygwinTerminal(os.Stdin.Fd()) {


I wonder if it is time to create a wrapper for this condition:

$ ag IsCygwin cmd/limactl/main.go 82: if runtime.GOOS == "windows" && isatty.IsCygwinTerminal(os.Stdout.Fd()) { cmd/limactl/list.go 171: if isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) { cmd/limactl/shell.go 176: if isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) { pkg/progressbar/progressbar.go 15: if isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) { pkg/qemu/entitlementutil/entitlementutil.go 88: if isatty.IsTerminal(os.Stdout.Fd()) || isatty.IsCygwinTerminal(os.Stdout.Fd()) {

Not sure what the right package is, maybe uiutil.isatty()?

Yes, but it can be another PR

norio-nomura · 2024-01-18T13:49:40Z

Thanks! 🙏🏻

AkihiroSuda reviewed Jan 17, 2024

View reviewed changes

cmd/limactl/start.go Outdated Show resolved Hide resolved

AkihiroSuda added this to the v0.20.0 milestone Jan 17, 2024

norio-nomura force-pushed the limactl-start-foreground branch 2 times, most recently from 324d034 to 3a74a52 Compare January 17, 2024 12:27

norio-nomura commented Jan 17, 2024

View reviewed changes

AkihiroSuda reviewed Jan 17, 2024

View reviewed changes

AkihiroSuda previously approved these changes Jan 17, 2024

View reviewed changes

AkihiroSuda requested a review from jandubois January 17, 2024 23:50

norio-nomura dismissed AkihiroSuda’s stale review via 599712b January 17, 2024 23:56

norio-nomura force-pushed the limactl-start-foreground branch from 3a74a52 to 599712b Compare January 17, 2024 23:56

jandubois approved these changes Jan 18, 2024

View reviewed changes

AkihiroSuda approved these changes Jan 18, 2024

View reviewed changes

AkihiroSuda merged commit 3057806 into lima-vm:master Jan 18, 2024
25 checks passed

norio-nomura deleted the limactl-start-foreground branch January 18, 2024 13:49

norio-nomura mentioned this pull request Jan 18, 2024

CLI flag to generate autostart files #2151

Merged

jandubois mentioned this pull request Jan 18, 2024

v0.20 planning #2138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

limactl start: add `--foreground` flag #2146

limactl start: add `--foreground` flag #2146

norio-nomura commented Jan 17, 2024

AkihiroSuda Jan 17, 2024

norio-nomura Jan 17, 2024 •

edited

Loading

AkihiroSuda Jan 17, 2024

norio-nomura Jan 17, 2024

AkihiroSuda commented Jan 17, 2024

norio-nomura Jan 17, 2024

AkihiroSuda Jan 17, 2024

AkihiroSuda left a comment

jandubois left a comment

jandubois Jan 18, 2024

AkihiroSuda Jan 18, 2024

norio-nomura commented Jan 18, 2024

limactl start: add --foreground flag #2146

limactl start: add --foreground flag #2146

Conversation

norio-nomura commented Jan 17, 2024

AkihiroSuda Jan 17, 2024

Choose a reason for hiding this comment

norio-nomura Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

AkihiroSuda Jan 17, 2024

Choose a reason for hiding this comment

norio-nomura Jan 17, 2024

Choose a reason for hiding this comment

AkihiroSuda commented Jan 17, 2024

norio-nomura Jan 17, 2024

Choose a reason for hiding this comment

AkihiroSuda Jan 17, 2024

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

jandubois left a comment

Choose a reason for hiding this comment

jandubois Jan 18, 2024

Choose a reason for hiding this comment

AkihiroSuda Jan 18, 2024

Choose a reason for hiding this comment

norio-nomura commented Jan 18, 2024

limactl start: add `--foreground` flag #2146

limactl start: add `--foreground` flag #2146

norio-nomura Jan 17, 2024 •

edited

Loading