Enable libkrun provider to open a debug console #22759
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
I'm not sure I like adding significant side effects to |
pkg/machine/apple/apple.go
Outdated
| os.Chmod(kdFile.Path, 0744) | ||
|
|
||
| f.WriteString("#!/bin/sh\n") | ||
| f.WriteString(fmt.Sprintf("exec %s", strings.Join(cmd.Args, " "))) | ||
| f.Close() |
pkg/machine/apple/apple.go
Outdated
| os.Chmod(kdFile.Path, 0744) | ||
|
|
||
| f.WriteString("#!/bin/sh\n") | ||
| f.WriteString(fmt.Sprintf("exec %s", strings.Join(cmd.Args, " "))) |
There was a problem hiding this comment.
strings.Join() on the Args will not work, this does not handle special chars/spaces etc... and will just break with hard to diagnose errors.
I am not sure what the purpose of spawning another terminal is, couldn't we just attach it to the current podman machine start command stdout/err?
There was a problem hiding this comment.
strings.Join() on the Args will not work, this does not handle special chars/spaces etc... and will just break with hard to diagnose errors.
What's the preferred way to do it?
I am not sure what the purpose of spawning another terminal is, couldn't we just attach it to the current podman machine start command stdout/err?
I think the UX is significantly better this way. Stealing podman's stdout/stdin feels weird, I would expect podman to display debugging messages and not be interactive, just as happens with other provides.
There was a problem hiding this comment.
strings.Join() on the Args will not work, this does not handle special chars/spaces etc... and will just break with hard to diagnose errors.
What's the preferred way to do it?
I don't think there is any ready to use interface for writing string slices arguments as shell escaped args. Looping over the args and using fmt.Sprintf("%q",arg) may work although I am unsure if it handles quoting like the shell.
But maybe stepping back why do you have to create this tmp script anyway, can't you append the args to the terminal command?
There was a problem hiding this comment.
I am not sure what the purpose of spawning another terminal is, couldn't we just attach it to the current podman machine start command stdout/err?
I think the UX is significantly better this way. Stealing podman's stdout/stdin feels weird, I would expect podman to display debugging messages and not be interactive, just as happens with other provides.
But isn't this a fundamental different behaviour here, you are spawning the full krun in another terminal unlike vfkit which is started as direct child and then opens a GUI window.
We don't do it today but I think we should read stderr in order to return the error strings to the user if something fails. I don't see how this would work with your solution.
There was a problem hiding this comment.
I don't think there is any ready to use interface for writing string slices arguments as shell escaped args. Looping over the args and using fmt.Sprintf("%q",arg) may work although I am unsure if it handles quoting like the shell.
Did it this way in v2, thanks!
But maybe stepping back why do you have to create this tmp script anyway, can't you append the args to the terminal command?
On macOS, /usr/bin/open allows you to pass arguments to Terminal itself but, AFAIK, there isn't a way to tell Terminal to pass any arguments to command being executed on it. Ref: https://stackoverflow.com/questions/29510815/how-to-pass-command-line-arguments-to-a-program-run-with-the-open-command
|
I think @slp is on the right track here. What he is proposing follows the same model as the other providers. Paul's point about how the various providers are or are not children and their subsequent "terminals" is also valid. But I think this is the closest we have; and not having this function, of a spawned terminal, is more of a risk that anything. Long term the hope is that all providers would drop a boot.log file which would show the last boot up, similar to capturing the serial console output. But we need an interactive terminal with libkrun today. @slp my advice is to address any code reviews and get this passing tests. Thanks for the contribution. |
|
@Luap99 PTAL |
|
I think we need here the "No New Tests" label to have CI to move forward. |
pkg/machine/apple/apple.go
Outdated
| return nil, nil, err | ||
| } | ||
| for arg := range cmd.Args { | ||
| _, err = f.WriteString(fmt.Sprintf("%q", arg)) |
There was a problem hiding this comment.
you never add a space between the args so I don't see how this can work.
There was a problem hiding this comment.
Didn't push the latest amend, sorry.
rhatdan
added
the
No New Tests
When running with "log-level=debug" and libkrun as machine provider, spawn a Terminal to execute "krunkit" to enable users to have full access to the VMs console for debugging purposes. Users obtain an interactive, text console with scrollback. It's possible to interact with both the kernel and GRUB2. To obtain even additional debugging information, users can add "console=hvc0" to the linux kernel command line through GRUB2 (it may be worth considering extending the initial configuration of the VM to add that argument by default). Signed-off-by: Sergio Lopez <slp@redhat.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude, slp The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| return nil, nil, err | ||
| } | ||
|
|
||
| cmd = exec.Command("/usr/bin/open", "-Wa", "Terminal", kdFile.Path) |
There was a problem hiding this comment.
i assume this will exist on all Mac installations?
There was a problem hiding this comment.
Yes, it's part of the base OS: https://www.unix.com/man-page/osx/1/open/
|
one question, else LGTM |
|
@Luap99 PTANL |
|
/lgtm |
When running with "log-level=debug" and libkrun as machine provider, spawn a Terminal to execute "krunkit" to enable users to have full access to the VMs console for debugging purposes.
Users obtain an interactive, text console with scrollback. It's possible to interact with both the kernel and GRUB2. To obtain even additional debugging information, users can add "console=hvc0" to the linux kernel command line through GRUB2 (it may be worth considering extending the initial configuration of the VM to add that argument by default).
Does this PR introduce a user-facing change?