DEENG-42 - Add reboot/shutdown feature on launcher side #56

patrick330602 · 2021-10-29T06:47:42Z

CreateProcess unfortunately always throws error 87 for me so I used ShellExecute here.

DistroLauncher/OOBE.cpp

CarlosNihelton

There are some issues blocking compilation and needed to be addressed.

As I started to check those, I came to think of suggestions that could make easier to implement the feature you're working on. Please consider those approaches and maybe doing so we can avoid adding dependencies. Let me know if I'm being naive in those considerations.

DistroLauncher/OOBE.cpp

- We can avoid adding more deps. - That modifies files which may conflict with upstream in the future. - Also the pipe thing can be simplified.

- Pipe not needed to read a file inside Linux filesystem. - Shutdown and relaunch the distro now by using `_wsystem`. - Removed dependences of Shell32.lib and Shlwapi.lib.

CarlosNihelton

I think I need to approve what I requested to change in order to stop being a reviewer and request review.

CarlosNihelton · 2021-11-10T21:35:25Z

Apart from the conflict which is very straightforward to fix, @didrocks, could you please review that code?

I just noticed we don't really shutdown the VM in this feature. Writting to /etc/wsl.conf won't prevent the next start to log in as root, without stopping WSL itself. We need to let the launcher know the uid and register the default user properly by calling WSL API as the original code does when installing the distro. The other option would be to stop the VM and who knows what the user would be doing on other distros or other Ubuntu releases that could be running at that moment.

In my machine at least, editing the registry the right key in HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Lxss takes effect imediately, while editing /etc/wsl.conf/ only takes effect after restarting the VM (not the distro).

Previously we had that pipe code reading /var/lib/ubuntu-wsl/assigned_account, which has been removed. BTW that file doesn't exist in any of the Ubuntu instances I run on WSL on my Windows machine.

The way I see to solve this issue is make Subiquity write a file aside of the launcher-status containing only the UID of the user created by the OOBE and read it here, just before the shutdown check. Then we would be able to register the default user without relying on wsl --shutdown and ensure the prompt will not appear to the user as root if a user is created by the OOBE.

didrocks

Just work out the ^M issue, but otherwise looks good to me :)

DistroLauncher/DistroLauncher.vcxproj

didrocks · 2021-11-12T09:29:23Z

DistroLauncher/OOBE.cpp

-			CloseHandle(writePipe);
+		// read the OOBE exit status file.
+		// Even without interop activated Windows can still access Linux files under WSL.
+		const TCHAR* launcherStatusPath = L"/run/subiquity/launcher-status";


I would move this file to something else, as we may reuse it for other things to tell the launcher to reboot/shutdown.

So maybe /run/launcher-command directly or any better name?

That's fine. We just need to update Subiquity accordingly. If we go with my other suggestion, on the default UID, I'd like to have them close, in the same directory, but separate to make things simple. Of course we could read the /etc/wsl.conf generated by the OOBE, find the username and call this function (or do what it does: launch id -u, create a pipe to read the stdout and register the uid as the default):

WSL/DistroLauncher/DistroLauncher.cpp

Line 61 in dd78a1a

HRESULT SetDefaultUser(std::wstring_view userName)

Two files of one word each seems simpler and straightforward to me. If you believe that could be somewhat error prone due lack of validation, we can move to some standard syntax, like yaml or ini and put both pieces of information on the same file.

What do you think?

Of course we could read the /etc/wsl.conf generated by the OOBE,
I think if this is not useful (which is not as we will use the API call), let’s not add it to wsl.conf anymore.

So yeah, let’s do one file maybe? If you prefer yaml or json, or just a keyfile, this is fine. Maybe one with action: and one for extra info like user. That way, that file becomes the communication handler between the rootfs (distro) and the laucher (windows)?

didrocks · 2021-11-12T09:33:57Z

The way I see to solve this issue is make Subiquity write a file aside of the launcher-status containing only the UID of the user created by the OOBE and read it here, just before the shutdown check.
Ack, and we remove in subiquity the wsl.conf overwrite which is not needed.

Then we would be able to register the default user without relying on wsl --shutdown and ensure the prompt will not appear to the user as root if a user is created by the OOBE.

I’m surprised that terminate is not enough, there are one VM per WSL app, no? (Honest question, that may be implemented differently but we tried to decipher this some time ago with jibel and it seemed like it)

CarlosNihelton · 2021-11-12T12:38:46Z

The way I see to solve this issue is make Subiquity write a file aside of the launcher-status containing only the UID of the user created by the OOBE and read it here, just before the shutdown check.
Ack, and we remove in subiquity the wsl.conf overwrite which is not needed.

Then we would be able to register the default user without relying on wsl --shutdown and ensure the prompt will not appear to the user as root if a user is created by the OOBE.

I’m surprised that terminate is not enough, there are one VM per WSL app, no? (Honest question, that may be implemented differently but we tried to decipher this some time ago with jibel and it seemed like it)

Well, there is one VM per Windows user running WSL. When a user launches more than one instance, let's say one Ubuntu and one Alpine, they share the same VM and kernel. When we do wsl -t Ubuntu we "shutdown" just the instances registered with "Ubuntu" WSL_ID, the VM is still running, Alpine will still be running. When we do wsl --shutdown all distros the user is running at that moment are killed, together with the VM.

In order to have the changes in /etc/wsl.conf taking effect, we need the VM to shutdown. When the last running distro is stopped, a few seconds later the VM stops too. If we quickly restart the VM might not have shutdown either. The issue I'm foreseeing here is when the user has other distros running in the same time. Shutting down Ubuntu will not be enough to make all the changes in /etc/wsl.conf to take effect. At least the default user I see as problematic. But calling WSL API solves that, since updates to the Windows registry take effect immediately.

didrocks · 2021-11-12T12:56:47Z

Ah, that matches exactly what I read in the early days. However, we were not capable of seeing this in practice. Out of curiousity, I think jibel and I would be interested in how you discovered and tested this on an instance :)

However, this is annoying that wsl.conf is not fully reread (as this is per application/rootfs) without shutting down the VM. Mind trying other parameter changes - still having one other application running, to ensure that the other parameters like interoperability and mount for instance - are taken into account? Otherwise, the whole reboot thingy has to be revisited.

For instance, we modified the boot parameter and this was taken immediately into account after a --terminate / relaunching the application. I think this is worth exploring

CarlosNihelton · 2021-11-12T13:04:29Z

Ah, that matches exactly what I read in the early days. However, we were not capable of seeing this in practice. Out of curiousity, I think jibel and I would be interested in how you discovered and tested this on an instance :)

However, this is annoying that wsl.conf is not fully reread (as this is per application/rootfs) without shutting down the VM. Mind trying other parameter changes - still having one other application running, to ensure that the other parameters like interoperability and mount for instance - are taken into account? Otherwise, the whole reboot thingy has to be revisited.

That is exactly what happens with network. Recently my instances are not getting DNS working at start when /etc/resolv.conf is generated by WSL. I noticed some issues reported around that, more related to VPN, but is happening very often. In that case, I disabled the generation of that file thru the config in /etc/wsl.conf. Guess what: only takes effect after shutting down the VM. You can see something like that documented in this issue.

CarlosNihelton · 2021-11-12T14:15:14Z

Ah, that matches exactly what I read in the early days. However, we were not capable of seeing this in practice. Out of curiousity, I think jibel and I would be interested in how you discovered and tested this on an instance :)
However, this is annoying that wsl.conf is not fully reread (as this is per application/rootfs) without shutting down the VM. Mind trying other parameter changes - still having one other application running, to ensure that the other parameters like interoperability and mount for instance - are taken into account? Otherwise, the whole reboot thingy has to be revisited.

That is exactly what happens with network. Recently my instances are not getting DNS working at start when /etc/resolv.conf is generated by WSL. I noticed some issues reported around that, more related to VPN, but is happening very often. In that case, I disabled the generation of that file thru the config in /etc/wsl.conf. Guess what: only takes effect after shutting down the VM. You can see something like that documented in this issue.

Just to make sure this is clear after our hangout and tests together (somebody may ask in the future and this will be documented)

wsl -t distro will take the /etc/wsl.conf changes into effect after some time, which might take 8s or even more depending on the host power and how busy it is. Thus, just relaunching the distro quickly, without checking if it stopped completely will certainly result in changes not being considered. wsl --shutdown speeds up that, but at price of shutting down all the WSL distros currently running, thus undesired.

I'll make sure to add checks with wls -l --running in the launcher code before relaunching the distro.

jibel · 2021-11-15T07:51:25Z

My understanding is that:

If all the sessions to a distro are closed, then this distro will automatically shutdown in 8s or so. Auto-termination of the distro does not happen if something is still running inside the distro (eg nohup sleep 300 & -> auto shutdown will happen after 5 minutes + ~8s)
wsl -t <distro>, will terminate the distro ASAP and kill everything running in the distro. It's the equivalent to kill -9 -1
wsl --shutdown ends the entire subsystem and kill all the distributions.

So it's correct to use -t <distro> and reiterate until no instance of the distro is running with --list --running.

- By reading the UID file left by the OOBE installer. - Still considering a more robust approach.

- A process runner runs `wsl --list --running --quiet` and checks if stdout contains this distro name. - A timeout is given to avoid the risk of running the launcher forever.

CarlosNihelton · 2021-11-17T12:43:55Z

I'd appreciate if one of you guys could go over my last two commits in this branch/PR. Once we approve those, I intend to merge this and start a new one with the following objectives:

Turn those two files left by the OOBE into an YAML file with a more robust schema.
Use a yaml library.
Change the CI to support the above.
Remove the default user entry in wsl.conf from Subiquity.

I did a interesting research yesterday which will allow adding libraries to this project without changing the project definition files, keeping the diff between our fork and upstream minimal.
I intend to change the launcher first and Subiquity after.

didrocks

Some comments :)

DistroLauncher/DistroLauncher.vcxproj

DistroLauncher/OOBE.cpp

- Renamed the `timeoutSeconds` as `maxNoOfRetries`.

didrocks · 2021-11-17T14:37:50Z

This looks good to me! Note that you have a conflict apparently in OOBE.h. Then, good to merge to me with your testing :)

patrick330602 requested review from CarlosNihelton and didrocks October 29, 2021 06:48

didrocks requested changes Oct 29, 2021

View reviewed changes

DistroLauncher/OOBE.cpp Outdated Show resolved Hide resolved

DistroLauncher/OOBE.cpp Outdated Show resolved Hide resolved

DistroLauncher/OOBE.cpp Outdated Show resolved Hide resolved

add reboot/shutdown feature

f9c981e

patrick330602 force-pushed the ft-reboot-shutdown-setup branch from c50140f to f9c981e Compare October 29, 2021 16:01

patrick330602 requested a review from didrocks October 29, 2021 16:06

CarlosNihelton requested changes Oct 29, 2021

View reviewed changes

CarlosNihelton added 2 commits November 10, 2021 17:08

Fixing compilation issues reported.

7abf14f

- We can avoid adding more deps. - That modifies files which may conflict with upstream in the future. - Also the pipe thing can be simplified.

Simplified code.

6c46164

- Pipe not needed to read a file inside Linux filesystem. - Shutdown and relaunch the distro now by using `_wsystem`. - Removed dependences of Shell32.lib and Shlwapi.lib.

CarlosNihelton force-pushed the ft-reboot-shutdown-setup branch from 255c113 to f9c981e Compare November 10, 2021 20:10

CarlosNihelton approved these changes Nov 10, 2021

View reviewed changes

CarlosNihelton self-assigned this Nov 11, 2021

CarlosNihelton self-requested a review November 11, 2021 17:36

didrocks requested changes Nov 12, 2021

View reviewed changes

CarlosNihelton added 2 commits November 16, 2021 10:14

Configure the distro default user.

31d661f

- By reading the UID file left by the OOBE installer. - Still considering a more robust approach.

Check whether the distro is still running.

7780209

- A process runner runs `wsl --list --running --quiet` and checks if stdout contains this distro name. - A timeout is given to avoid the risk of running the launcher forever.

CarlosNihelton requested a review from jibel November 17, 2021 12:38

Fixing unexpected long diff in DistroLauncher.vcxproj

c1f88f8

didrocks requested changes Nov 17, 2021

View reviewed changes

No need for extra confirmations in EnsureStopped()

edfeece

- Renamed the `timeoutSeconds` as `maxNoOfRetries`.

CarlosNihelton approved these changes Nov 17, 2021

View reviewed changes

Merge branch 'main' into ft-reboot-shutdown-setup

6cf74e7

CarlosNihelton merged commit 451fc78 into main Nov 17, 2021

CarlosNihelton deleted the ft-reboot-shutdown-setup branch November 17, 2021 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DEENG-42 - Add reboot/shutdown feature on launcher side #56

DEENG-42 - Add reboot/shutdown feature on launcher side #56

patrick330602 commented Oct 29, 2021

CarlosNihelton left a comment

CarlosNihelton left a comment

CarlosNihelton commented Nov 10, 2021 •

edited

Loading

didrocks left a comment

didrocks Nov 12, 2021

CarlosNihelton Nov 12, 2021

didrocks Nov 12, 2021

didrocks commented Nov 12, 2021

CarlosNihelton commented Nov 12, 2021

didrocks commented Nov 12, 2021 •

edited

Loading

CarlosNihelton commented Nov 12, 2021

CarlosNihelton commented Nov 12, 2021 •

edited

Loading

jibel commented Nov 15, 2021 •

edited

Loading

CarlosNihelton commented Nov 17, 2021 •

edited

Loading

didrocks left a comment

didrocks commented Nov 17, 2021

DEENG-42 - Add reboot/shutdown feature on launcher side #56

DEENG-42 - Add reboot/shutdown feature on launcher side #56

Conversation

patrick330602 commented Oct 29, 2021

CarlosNihelton left a comment

Choose a reason for hiding this comment

CarlosNihelton left a comment

Choose a reason for hiding this comment

CarlosNihelton commented Nov 10, 2021 • edited Loading

didrocks left a comment

Choose a reason for hiding this comment

didrocks Nov 12, 2021

Choose a reason for hiding this comment

CarlosNihelton Nov 12, 2021

Choose a reason for hiding this comment

didrocks Nov 12, 2021

Choose a reason for hiding this comment

didrocks commented Nov 12, 2021

CarlosNihelton commented Nov 12, 2021

didrocks commented Nov 12, 2021 • edited Loading

CarlosNihelton commented Nov 12, 2021

CarlosNihelton commented Nov 12, 2021 • edited Loading

jibel commented Nov 15, 2021 • edited Loading

CarlosNihelton commented Nov 17, 2021 • edited Loading

didrocks left a comment

Choose a reason for hiding this comment

didrocks commented Nov 17, 2021

CarlosNihelton commented Nov 10, 2021 •

edited

Loading

didrocks commented Nov 12, 2021 •

edited

Loading

CarlosNihelton commented Nov 12, 2021 •

edited

Loading

jibel commented Nov 15, 2021 •

edited

Loading

CarlosNihelton commented Nov 17, 2021 •

edited

Loading