Propose running init/rm command on hyperv machine in elevated mode when required#27932
Conversation
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
|
@l0rd related to this there are the start/stop actions on legacy machines. User should execute them as admin. Should we update start/stop and "mess up" the code a bit for a use case which is marginal? Or, as they are legacy stuff, it is ok for the user to run podman as admin if they want to work on old machines? |
I would not care about legacy machines. There is no request for that. |
|
Adding label |
Ah great, so this can be tested then |
l0rd
left a comment
There was a problem hiding this comment.
Thank you @lstocchi for this PR. It looks good except for a couple of things when machine init is re-executed:
- The image gets pulled and extracted twice (these steps should be skipped when
--reexec) - The message that informs the user that the command completed successfully, and how to start the machine, gets lost
I have added a few minor comments too.
Updated. Good point for the image pulled twice 👍 |
What about using the mechanism used for the WSL install with an output file created and printed in case of error? |
058ff14 to
958a6e9
Compare
Updated👍 |
|
A friendly reminder that this PR had no activity for 30 days. |
| // Do not clean up on relaunch: the elevated child process | ||
| // completed init successfully and is using the resources | ||
| // (e.g. the disk image) that cleanup would remove. |
There was a problem hiding this comment.
go callbackFuncs.CleanOnSignal() has the same problem: when a user hits CTRL+C, then the callbackfuncs.Clean() will be invoked for the original, non-privileged, init. What about ensuring that the cleanup is done by the same process that performed the action (created the lock, downloaded the image, etc.)?
There was a problem hiding this comment.
My bad. The error cleanup is skipped only if the error is ErrRelaunchSucceed because that's not really an error.
| lpFile: uintptr(unsafe.Pointer(exe)), | ||
| lpParameters: uintptr(unsafe.Pointer(arg)), | ||
| lpDirectory: uintptr(unsafe.Pointer(cwd)), | ||
| nShow: syscall.SW_SHOWNORMAL, |
There was a problem hiding this comment.
What about SW_HIDE? Opening another window may annoy the user. This code already existed, but it may still be better to hide it. What do you think?
There was a problem hiding this comment.
After testing, we decided to keep SW_SHOWNORMAL. Otherwise, commands that required an interaction (such as podman machine rm) would hang forever.
Signed-off-by: lstocchi <lstocchi@redhat.com>
cd1110f to
9b00d75
Compare
|
@l0rd it should be ready. To summarize what are the new changes we agreed in the call.
|
This commit adds automatic UAC elevation prompts for HyperV machine init/rm actions when administrator privileges are required. Previously, users had to manually run Podman as administrator when creating the first machine or removing the last machine, which requires Windows Registry modifications. When the HyperV command gets relaunched as elevated, the error of the elevated process is saved on a file to be displayed by the caller. The implementation is the same as that used by WSL. Signed-off-by: lstocchi <lstocchi@redhat.com>
…ror handling The old ErrRelaunchAttempt name was ambiguous — it reads as though the relaunch attempt failed, when it actually signals success. Rename to ErrRelaunchSucceeded and update comments at every call site to clarify that this is not a real error but a sentinel indicating the elevated child process completed the operation successfully. Also fix a bug in WSL's launchElevate where a failed elevated process was incorrectly wrapped with the sentinel, causing callers to treat the failure as success and print "Machine init complete." Signed-off-by: lstocchi <lstocchi@redhat.com>
|
@containers/podman-maintainers PTAL |
Honny1
left a comment
There was a problem hiding this comment.
Code LGTM, defer to Windows experts.
Luca remarkable knowledge of the Windows virtualization platform would be beneficial for reviewing PRs and triaging issues. lstocchi contributions: - podman-container-tools#28535 - podman-container-tools#27932 - podman-container-tools#27931 - podman-container-tools#27885 - podman-container-tools#27650 - podman-container-tools#26277 - podman-container-tools#26201 - podman-container-tools#20478 Signed-off-by: Mario Loriedo <mario.loriedo@gmail.com>
This PR adds automatic UAC elevation prompts for HyperV machine when administrator privileges are required. Similar to what happens on WSL when we need to install WSL features.
Previously, users had to manually run Podman as administrator when creating the first machine or removing the last machine, which requires Windows Registry modifications.
it fixes #27627
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?