Windows: Run 'crc daemon' as task for 'crc setup' #3088

praveenkumar · 2022-03-28T10:27:48Z

This PR is uses windows powershell to create crc daemon as task
and start it on demand for the current user. This doesn't need any
elevated privillages to run crc daemon for user context. It try to
perform following tasks

Check if there is already crcDaemon task
- If present does it the latest one using version info
Install the task using predefined xml definition
Start the task which run the crc daemon in background.

There is a known issue with windows powershell when you start a task
in background using powershell.exe then for a second the windows pops
up and disappear, PowerShell/PowerShell#3028
have more details around it but as of now this is only the way for
interactive user context otherwise the task will need the admin
privillages which we want to avoid.

cfergeau

There is a known issue with windows powershell when you start a task in background using powershell.exe then for a second the windows pops up and disappear, PowerShell/PowerShell#3028

Looks like this can be worked around though?

cfergeau · 2022-03-28T10:47:35Z

pkg/crc/preflight/preflight_daemon_powershell_windows.go

+      <WaitTimeout>PT1H</WaitTimeout>
+      <StopOnIdleEnd>true</StopOnIdleEnd>
+      <RestartOnIdle>false</RestartOnIdle>
+    </IdleSettings>


Is this stopping after being idle for a while? How is "idle" defined?

@cfergeau no it is stopping it intantly. This template is exported from a task and didn't modified.

cfergeau · 2022-03-28T10:49:22Z

pkg/crc/preflight/preflight_daemon_powershell_windows.go

@@ -0,0 +1,44 @@
+package preflight
+


Not sure a separate file is needed when you already add preflight_daemon_service_check_windows.go
A task_win.go file would make sense if most of the preflight check/fix code was moved to a higher level API to handle Windows tasks. The current separation does not soundu so useful.

I don't mind it to move this part to task_windows.go

cfergeau · 2022-03-28T10:49:45Z

pkg/crc/preflight/preflight_daemon_service_check_windows.go

+	// prepare the task script
+	binPath, err := goos.Executable()
+	if err != nil {
+		return fmt.Errorf("unable to find the current executables location: %v", err)


executable (without the trailing s)

cfergeau · 2022-03-28T10:50:00Z

pkg/crc/preflight/preflight_daemon_service_check_windows.go

+	if err != nil {
+		return fmt.Errorf("unable to find the current executables location: %v", err)
+	}
+	binPathWithArgs := fmt.Sprintf("%s daemon", strings.TrimSpace(binPath))


Why TrimSpace?

In case output have spaces in between.

But are you getting spaces when you test it? Or are there concrete cases when you know for sure there will be spaces that we need to remove? This is what is not clear to me :)

Removed, because binPath string is what we are creating so should be done it properly there.

cfergeau · 2022-03-28T10:51:26Z

pkg/crc/preflight/preflight_daemon_service_check_windows.go

+		logging.Debugf("%s task is not running: %v : %s", constants.DaemonTaskName, err, stderr)
+		return err
+	}
+	if strings.TrimSpace(stdout) != "Running" {


Is this working as expected on localized Windows versions?

@cfergeau That I didn't check and love to know if that works with localized version.

cfergeau · 2022-03-28T10:56:12Z

pkg/crc/preflight/preflight_windows.go

+var daemonServiceChecks = []Check{
+	{
+		configKeySuffix:    "check-daemon-task-install",
+		checkDescription:   "Check if daemon task is installed",


I think the other preflights are using Checking...Installing..., ...

Ah yes, I will improve that.

praveenkumar · 2022-03-28T11:34:46Z

Looks like this can be worked around though?

The workaround use vbs script which is detected as virus ( @anjannath told me) I don't find any other workaround which would work without installing any different tool.

cfergeau · 2022-03-28T11:56:58Z

Looks like this can be worked around though?

The workaround use vbs script which is detected as virus ( @anjannath told me) I don't find any other workaround which would work without installing any different tool.

Has it already been tried to have our own small helper non-console binary which would just run the appropriate powershell comnad? or would it also be detected as a virus?

praveenkumar · 2022-03-28T12:38:47Z

Has it already been tried to have our own small helper non-console binary which would just run the appropriate powershell comnad? or would it also be detected as a virus?

@cfergeau own small helper you mean create a small go binary which run crc daemon like we create for file embed?

cfergeau · 2022-03-28T12:50:34Z

Has it already been tried to have our own small helper non-console binary which would just run the appropriate powershell comnad? or would it also be detected as a virus?

@cfergeau own small helper you mean create a small go binary which run crc daemon like we create for file embed?

I think it's powershell.Execute("Register-ScheduledTask", ...) which causes the popup? I'm thinking of a non-console binary which would run this command for us, in the hope that it would allow us to hide the window.

pkg/crc/preflight/preflight_daemon_task_check_windows.go

praveenkumar · 2022-03-28T14:13:56Z

I think it's powershell.Execute("Register-ScheduledTask", ...) which causes the popup? I'm thinking of a non-console binary which would run this command for us, in the hope that it would allow us to hide the window.

It is not the Register-ScheduledTask but the command which use to run the crc daemon using powershell.exe -WindowStyle Hidden -Command crc.exe daemon if you don't use the powershell.exe -WindowStyle Hidden and direct use crc.exe daemon then user will see a powershell window will pop up and stay there which execute crc.exe daemon using -WindowStyle Hidden is kind of a neat way to don't show that window but as discribed it in the issue it is because powershell.exe is console application and there is nothing exist as a win32 host application.

pkg/crc/preflight/preflight_checks_windows.go

cfergeau · 2022-03-28T14:41:49Z

`` but the command which use to run the crc daemon using powershell.exe -WindowStyle Hidden -Command crc.exe daemon if you don't use the `powershell.exe -WindowStyle Hidden` and direct use `crc.exe daemon` then user will see a powershell window will pop up and stay there which execute `crc.exe daemon` using `-WindowStyle Hidden` is kind of a neat way to don't show that window but as discribed it in the issue it is because `powershell.exe` is console application and there is nothing exist as a win32 host application.

Ah, so each time the daemon is started, a console window flashes on the screen. Have you tried what is suggested in https://stackoverflow.com/questions/23250505/how-do-i-create-an-executable-from-golang-that-doesnt-open-a-console-window-whe instead of using powershell.exe?

gbraad

Approved

@jsliacan have you been able to test this in an update scenario?

praveenkumar · 2022-03-29T05:19:17Z

`` but the command which use to run the crc daemon using powershell.exe -WindowStyle Hidden -Command crc.exe daemon if you don't use the `powershell.exe -WindowStyle Hidden` and direct use `crc.exe daemon` then user will see a powershell window will pop up and stay there which execute `crc.exe daemon` using `-WindowStyle Hidden` is kind of a neat way to don't show that window but as discribed it in the issue it is because `powershell.exe` is console application and there is nothing exist as a win32 host application.

Ah, so each time the daemon is started, a console window flashes on the screen. Have you tried what is suggested in https://stackoverflow.com/questions/23250505/how-do-i-create-an-executable-from-golang-that-doesnt-open-a-console-window-whe instead of using powershell.exe?

Yes, if I use this during build of crc binary then there is no terminal output for any of the commands not only for crc side. I created following test script to experiment with it but this is not what we can consumed directly. With other helper command which can execute crc daemon command . But this can be part of improvement, I can create a follow up issue for this and have this discussion moved there.

package main

import (
	"fmt"
	"time"
)

func main() {
	for i := 0; i < 120; i++ {
		fmt.Println("This is for testing purpose")
		time.Sleep(time.Second)
	}
}

PS C:\Users\prkumar\work\github\practise\test> go build -ldflags -H=windowsgui -o wintest.exe main.go
PS C:\Users\prkumar\work\github\practise\test> .\wintest.exe 
PS C:\Users\prkumar\work\github\practise\test>  <-- Process directly goes to background without any print on stdout -->
PS C:\Users\prkumar\work\github\practise\test> Get-Process wintest

Handles  NPM(K)    PM(K)      WS(K)     CPU(s)     Id  SI ProcessName
-------  ------    -----      -----     ------     --  -- -----------
     82       8    12472       5232       0.03  54140   3 wintest

gbraad · 2022-03-29T07:59:19Z

.\crc.exe setup

...
INFO Checking if daemon task is installed
INFO Installing the daemon task
INFO Checking if daemon task is running
INFO Running the daemon task
...

.\crc.exe start

INFO Checking if running in a shell with administrator rights
INFO Checking Windows 10 release
INFO Checking Windows edition
INFO Checking if Hyper-V is installed and operational
INFO Checking if crc-users group exists
INFO Checking if current user is in Hyper-V Admins group
INFO Checking if Hyper-V service is enabled
INFO Checking if the Hyper-V virtual switch exists
INFO Found Virtual Switch to use: Default Switch
INFO Checking if vsock is correctly configured
INFO Checking if daemon task is installed
INFO Checking if daemon task is running
INFO Checking admin helper service is running
INFO Loading bundle: crc_podman_hyperv_3.4.4_amd64...
INFO Creating CodeReady Containers VM for Podman 3.4.4...
INFO Generating new SSH Key pair...
INFO CodeReady Containers instance is running with IP 127.0.0.1
INFO CodeReady Containers VM is running
INFO Updating authorized keys...
INFO Adding new bearer token for cockpit webconsole
podman runtime is now running.

Use the 'podman' command line interface:
  PS> & crc podman-env | Invoke-Expression
  PS> podman.exe COMMAND

cfergeau · 2022-03-29T08:02:54Z

Yes, if I use this during build of crc binary then there is no terminal output for any of the commands not only for crc side.

Yes, I suspected we do not want to build crc.exe with this flag. The program would indeed be something like:

package main

import (
	"os"
	crcos "github.com/code-ready/crc/pkg/os"
)

func main() {
        // check number of args 
        _, _, _ = crcos.RunWithDefaultLocale(os.Args[0], os.Args[1:])
}

I created following test script to experiment with it but this is not what we can consumed directly. With other helper command which can execute crc daemon command .

I'm not sure what you mean with these 2 sentences?

But this can be part of improvement, I can create a follow up issue for this and have this discussion moved there.

Sure, but this begs a few questions:

is it ok to merge this PR now, and possibly have the flashing window in the next release? Or do we hold this PR for a bit?
if this PR goes in, and we eventually realize there is no alternative to the flashing window, do we revert this? or will we live with the flashing window?

gbraad · 2022-03-29T08:07:21Z

@praveenkumar @anjannath should we modify the tray to check for this scheduled task and task accordingly if not running?
At the moment we rely on the daemon to be started from the tray... this might lead to a conflict.

cfergeau · 2022-03-29T08:08:50Z

pkg/crc/preflight/preflight_daemon_task_check_windows.go

+	var buf bytes.Buffer
+	if err := xml.EscapeText(&buf, []byte(binPathWithArgs)); err != nil {
+		return err
+	}


This belongs in genDaemonTaskInstallTemplate, not here. If you do it here, this means all callers of genDaemonTaskInstallTemplate have to be aware of it. This also kind of hardcodes in fixDaemonTaskInstalled that genDaemonTaskInstallTemplate will be doing fmt.Sprintf(xml, ...). fixDaemonTaskInstalled has no need to know that, this just muddles unrelated abstraction layers

@cfergeau you want something like following?

package main import ( "bytes" "encoding/xml" "fmt" "log" ) func genDaemonTaskInstallTemplate(crcVersion, daemonServiceName string) (string, error) { var buf bytes.Buffer fmt.Fprintf(&buf, `<?xml version="1.0" encoding="UTF-16"?> <Task version="1.3" xmlns="http://schemas.microsoft.com/windows/2004/02/mit/task"> <RegistrationInfo> <Description>Run crc daemon as a task</Description> <Version>%s</Version> </RegistrationInfo> <Settings> <DisallowStartIfOnBatteries>false</DisallowStartIfOnBatteries> <StopIfGoingOnBatteries>false</StopIfGoingOnBatteries> <Hidden>true</Hidden> <MultipleInstancesPolicy>IgnoreNew</MultipleInstancesPolicy> <IdleSettings> <Duration>PT10M</Duration> <WaitTimeout>PT1H</WaitTimeout> <StopOnIdleEnd>true</StopOnIdleEnd> <RestartOnIdle>false</RestartOnIdle> </IdleSettings> <UseUnifiedSchedulingEngine>true</UseUnifiedSchedulingEngine> </Settings> <Triggers /> <Actions Context="Author"> <Exec> <Command>powershell.exe</Command> <Arguments>-WindowStyle Hidden -Command `, crcVersion) err := xml.EscapeText(&buf, []byte(daemonServiceName)) fmt.Fprintf(&buf, `</Arguments> </Exec> </Actions> </Task>`) return buf.String(), err } func main() { s, err := genDaemonTaskInstallTemplate("2.0.1", `C:\windows\crc.exe`) if err != nil { log.Fatal(err) } fmt.Println(s) }

Not very easy to read with the XML embedded in the function, but I had in mind something like this:

func genDaemonTaskInstallTemplate(crcVersion, daemonServiceName string) (string, error) { var escapedName bytes.Buffer if err := xml.EscapeText(&escapedName, []byte(daemonServiceName)); err != nil { return "", err } return fmt.Sprintf(daemonTaskTemplate, crcVersion, escapedName.String(), ), nil }

pkg/crc/preflight/preflight_daemon_task_check_windows.go

praveenkumar · 2022-03-29T11:55:33Z

Sure, but this begs a few questions:

* is it ok to merge this PR now, and possibly have the flashing window in the next release? Or do we hold this PR for a bit?

@cfergeau I think it should be OK to take this PR as of now with flashing window.

* if this PR goes in, and we eventually realize there is no alternative to the flashing window, do we revert this? or will we live with the flashing window?

@cfergeau No, because this flashing window is not something user even notice and if even they do we have it part of the known issue. we only improve it, revert only take place if there is an alternative to task manager which can run daemon in user context without Admin priviliages.

cfergeau · 2022-03-29T11:56:58Z

@cfergeau No, because this flashing window is not something user even notice and if even they do we have it part of the known issue.

Err, sorry, but I'd definitely notice, and find this ugly and unpolished from whatever application did this. Let's not make blanket statements regarding what users would or would not notice.

praveenkumar · 2022-03-30T06:04:19Z

@cfergeau No, because this flashing window is not something user even notice and if even they do we have it part of the known issue.

Err, sorry, but I'd definitely notice, and find this ugly and unpolished from whatever application did this. Let's not make blanket statements regarding what users would or would not notice.

@cfergeau Wrong wording in the sentense. No was for the revert query. I am all up for making it hidden from the start but as of now we have to use this workaround.

praveenkumar · 2022-03-30T08:18:53Z

/hold

This PR is uses windows powershell to create `crc daemon` as task and start it on demand for the current user. This doesn't need any elevated privillages to run `crc daemon` for user context. It try to perform following tasks - Check if there is already crcDaemon task + If present does it the latest one using version info - Install the task using predefined xml definition - Start the task which run the `crc daemon` in background. There is a known issue with windows powershell when you start a task in background using `powershell.exe` then for a second the windows pops up and disappear, PowerShell/PowerShell#3028 have more details around it but as of now this is only the way for interactive user context otherwise the task will need the admin privillages which we want to avoid.

Admin helper service is created and started when installer is used. Some of the issue we are getting is user directly try to use crc from command line and see if the openshift routes are mapped in hosts file. This PR is help to detect if the service is running and it doesn't need any privilage permission but it doesn't create or start this service because that need administrator privilages.

…ecks

This PR will remove the crc daemon schedule task during the uninstallation, as of now even with this PR when the task is running and user try to uninstall then uninstaller warn user that `crc` binary in used and user have option to forceful stop it and carry on or go to task manager and kill it manually.

This PR remove preflight checks for daemon command because daemon can run independent of a crc setup. the actual check happens when you perform a `crc start`. Since those tests run as part of crc daemon which takes around 5-6 sec so initial api response become block for 5-6 sec.

openshift-ci · 2022-03-31T06:30:11Z

@praveenkumar: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/integration-crc	`70e5e28`	link	true	`/test integration-crc`
ci/prow/e2e-crc	`70e5e28`	link	true	`/test e2e-crc`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

praveenkumar · 2022-03-31T08:05:13Z

/unhold

Without this PR following scenario happen - User installed the crc first time and do the setup - As part of the setup crc daemon is started by task schedular - User reboot - Tray autostart with reboot but `crc setup --check-only` fails because daemon task is not running. With this PR, check for daemon task running should first validate if an existing process of `crc daemon` is running and the version it get from the api should be same as the binary then task running check succeed. So this PR will makes `crc setup --check-only` work again with tray autostart.

openshift-ci · 2022-03-31T08:43:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: anjannath, gbraad, praveenkumar

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:

~~OWNERS~~ [anjannath,gbraad,praveenkumar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

anjannath · 2022-03-31T08:43:52Z

Tested the end to end flow (both from cli and tray) and its working as expected

openshift-ci bot requested review from anjannath and gbraad March 28, 2022 10:27

openshift-ci bot added the approved label Mar 28, 2022

This was referenced Mar 28, 2022

[BUG] crc start fails as dependent service not started -- Daemon not available #3083

Closed

[BUG] crc start fails as dependent service not started -- Daemon not available - Cannot reach daemon API: Get "http://unix/api/version" #3086

Closed

cfergeau reviewed Mar 28, 2022

View reviewed changes

praveenkumar force-pushed the win_task branch 2 times, most recently from c4f1dfe to 44b868a Compare March 28, 2022 12:36

cfergeau reviewed Mar 28, 2022

View reviewed changes

pkg/crc/preflight/preflight_daemon_task_check_windows.go Show resolved Hide resolved

anjannath reviewed Mar 28, 2022

View reviewed changes

pkg/crc/preflight/preflight_checks_windows.go Outdated Show resolved Hide resolved

praveenkumar force-pushed the win_task branch 3 times, most recently from 36cfc03 to 6e65f8f Compare March 28, 2022 14:36

gbraad approved these changes Mar 29, 2022

View reviewed changes

openshift-ci bot assigned gbraad Mar 29, 2022

openshift-ci bot added the lgtm label Mar 29, 2022

praveenkumar mentioned this pull request Mar 29, 2022

[Windows] Try to find a better workaround instead flashing the powershell windows during crc setup #3089

Open

cfergeau reviewed Mar 29, 2022

View reviewed changes

anjannath reviewed Mar 29, 2022

View reviewed changes

pkg/crc/preflight/preflight_daemon_task_check_windows.go Show resolved Hide resolved

anjannath reviewed Mar 29, 2022

View reviewed changes

pkg/crc/preflight/preflight_daemon_task_check_windows.go Show resolved Hide resolved

openshift-ci bot removed the lgtm label Mar 29, 2022

praveenkumar force-pushed the win_task branch from 6e65f8f to 7dc9903 Compare March 29, 2022 14:17

praveenkumar mentioned this pull request Mar 30, 2022

[Windows] Change the service name of admin helper to CRCAdminHelper during upgrade #3092

Open

praveenkumar mentioned this pull request Mar 30, 2022

Remove preflight checks as part of crc daemon #3093

Closed

openshift-ci bot added the do-not-merge/hold label Mar 30, 2022

praveenkumar added 4 commits March 30, 2022 21:27

Windows: Add system network mode label for system-network specific ch…

7be921d

…ecks

praveenkumar force-pushed the win_task branch from 7dc9903 to 70e5e28 Compare March 31, 2022 06:04

gbraad approved these changes Mar 31, 2022

View reviewed changes

openshift-ci bot added the lgtm label Mar 31, 2022

openshift-ci bot removed do-not-merge/hold lgtm labels Mar 31, 2022

praveenkumar force-pushed the win_task branch from 161a6be to aa49011 Compare March 31, 2022 08:21

anjannath approved these changes Mar 31, 2022

View reviewed changes

openshift-ci bot assigned anjannath Mar 31, 2022

openshift-ci bot added the lgtm label Mar 31, 2022

anjannath mentioned this pull request Mar 31, 2022

Always start the daemon when tray is started crc-org/tray-electron#205

Merged

praveenkumar merged commit 889b132 into crc-org:main Mar 31, 2022

Windows: Run 'crc daemon' as task for 'crc setup' #3088

Windows: Run 'crc daemon' as task for 'crc setup' #3088

Conversation

praveenkumar commented Mar 28, 2022

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenkumar commented Mar 28, 2022

cfergeau commented Mar 28, 2022

praveenkumar commented Mar 28, 2022

cfergeau commented Mar 28, 2022

praveenkumar commented Mar 28, 2022

cfergeau commented Mar 28, 2022

gbraad left a comment

Choose a reason for hiding this comment

praveenkumar commented Mar 29, 2022 • edited

gbraad commented Mar 29, 2022

cfergeau commented Mar 29, 2022

gbraad commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praveenkumar commented Mar 29, 2022

cfergeau commented Mar 29, 2022

praveenkumar commented Mar 30, 2022

praveenkumar commented Mar 30, 2022

openshift-ci bot commented Mar 31, 2022

praveenkumar commented Mar 31, 2022

openshift-ci bot commented Mar 31, 2022

anjannath commented Mar 31, 2022

praveenkumar commented Mar 29, 2022 •

edited