Use `win32-background-launcher` to start the daemon task #3901

anjannath · 2023-10-31T09:40:25Z

Needs:

Add versioning using resource file win32-background-launcher#1
release v0.0.0.1 of win32-background-launcher

anjannath · 2023-10-31T09:41:44Z

/hold

till required tasks are finished as mentioned in #3901 (comment) and #3879 (comment)

anjannath · 2023-11-16T07:13:35Z

/test e2e-microshift-crc

gbraad · 2023-11-24T06:36:22Z

can we rename the application on Windows to crc-background-launcher instead of the cryptic win32-...
or background-launcher. This way people can identify where it belongs to, as paths are not shown in Task Manager.

adrianriobo · 2023-11-24T07:48:00Z

Tested on windows 11 https://crcqe-asia.s3.amazonaws.com/nightly/ocp/4.14.3/20231123/11-pro/qe-results/e2e-non-ux.xml cluster behaves as expected

anjannath · 2023-11-24T08:08:26Z

can we rename the application on Windows to crc-background-launcher instead of the cryptic win32-... or background-launcher. This way people can identify where it belongs to, as paths are not shown in Task Manager.

yes, can do, first have to first change the name of the executable in crc-org/win32-background-launcher

edit: created crc-org/win32-background-launcher#3

cfergeau · 2023-11-24T16:18:05Z

can we rename the application on Windows to crc-background-launcher instead of the cryptic win32-... or background-launcher. This way people can identify where it belongs to, as paths are not shown in Task Manager.

Well the launcher is generic and has nothing crc specific. Dunno if there's a way to provide a description to indicate which process it launched?

cfergeau

overall looks fine, a few comments here and there.

cfergeau · 2023-11-24T16:26:51Z

pkg/crc/constants/constants.go

+	DaemonLogFile                 = "crcd.log"
+	CrcLandingPageURL             = "https://console.redhat.com/openshift/create/local" // #nosec G101
+	DefaultAdminHelperURLBase     = "https://github.com/crc-org/admin-helper/releases/download/v%s/%s"
+	CRCWin32BackgroundLauncherURL = "https://github.com/crc-org/win32-background-launcher/releases/download/v%s/%s"


I don't think the CRC prefix is really useful here, and I'd also drop "win32". This does not need to be exported.

cfergeau · 2023-11-24T16:27:49Z

pkg/crc/constants/constants.go

@@ -47,6 +48,8 @@ const (

 	OpenShiftIngressHTTPPort  = 80
 	OpenShiftIngressHTTPSPort = 443
+
+	Win32BackgroundLauncherExeName = "win32-background-launcher.exe"


For admin-helper, we call it Executable, not ExeName

pkg/crc/preflight/preflight_windows.go

cfergeau · 2023-11-28T12:14:41Z

pkg/crc/constants/constants.go

@@ -187,6 +194,11 @@ func GetKubeAdminPasswordPath() string {
 	return filepath.Join(MachineInstanceDir, DefaultName, "kubeadmin-password")
 }

+func GetCRCWindowsBackgroundLauncherDownloadURL() string {


GetWin32BackgroundLauncher...?

cfergeau · 2023-12-04T14:00:15Z

pkg/crc/cache/cache.go

@@ -20,6 +20,7 @@ type Cache struct {
 	executablePath string
 	archiveURL     string
 	version        string
+	nameMismatch   bool


ignoreNameMismatch would be more explicit.

cfergeau · 2023-12-04T14:56:19Z

pkg/crc/cache/cache.go

@@ -33,8 +34,8 @@ func (e *VersionMismatchError) Error() string {
 	return fmt.Sprintf("%s version mismatch: %s expected but %s found in the cache", e.ExecutableName, e.ExpectedVersion, e.CurrentVersion)
 }

-func newCache(executablePath string, archiveURL string, version string, getVersion func(string) (string, error)) *Cache {
-	return &Cache{executablePath: executablePath, archiveURL: archiveURL, version: version, getVersion: getVersion}
+func newCache(executablePath string, archiveURL string, version string, nameMismatch bool, getVersion func(string) (string, error)) *Cache {


Not sure I'd add this arg by default, might be better to explicitly set it for the background launcher after creating the cache with the default values (cache.IgnoreNameMismatch() or cache.ignoreNameMismatch = true)

cfergeau

2 minor comments, but fine with me with or without changes
/lgtm
/approve

cfergeau · 2023-12-05T10:58:57Z

pkg/crc/preflight/preflight_daemon_task_check_windows.go

+		return fmt.Errorf("Missing background launcher binary at: %s", constants.Win32BackgroundLauncherPath())
+	}
+
+	binPathWithArgs := fmt.Sprintf(`"%s" daemon`, crcBinPath)


This used to be daemonCmdArgs := daemon --log-level debug`, is it intentional that you removed the debug logs?

yes, we have no way to look at the stdout of the process, and logs to the log file crcd.log is always at debug/trace level, so this flag was not useful

cfergeau · 2023-12-05T11:01:37Z

pkg/crc/cache/cache_windows.go

@@ -23,4 +23,6 @@ func NewWin32BackgroundLauncherCache() *Cache {
 			return strings.TrimSpace(stdOut), nil
 		},
 	)
+	cache.ignoreNameMismatch = true


Might be worth an explanatory comment? upstream binary is win32-..., we want to name it crc-...?

openshift-ci · 2023-12-05T11:02:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau

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~~ [cfergeau]

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

openshift-ci · 2023-12-06T04:03:06Z

New changes are detected. LGTM label has been removed.

crc-embedder is updated to download win32-background-launcher.exe from its github release at crc-org/win32-backgroun-launcher so that it is available while building the msi it'll be used to start the crc daemon as a background process

this removes the powershell script we used earlier to start the daemon task as a hidden process and uses the background launcher executable installed by the msi to start the daemon task updates the preflight checks that installs the daemon task to use the background launcher executable

on a released crc version the win32-background-laucher will be installed in the install location of crc by the msi, but while using a non installer version this'll help to download the win32-background-launcher binary by running `crc setup` this is also helpful for testing different versions of the background launcher during development

the background-launcher upstream is released as win32-background-launcher.exe since its function is generic and does nothing crc specific however when we install it and use it to launch the daemon process in background on windows it is useful to name it as crc-background-launcher.exe to make it easy for users to identify it with crc

openshift-ci · 2023-12-18T05:45:02Z

@anjannath: 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	`ce335c0`	link	true	`/test integration-crc`
ci/prow/e2e-crc	`ce335c0`	link	true	`/test e2e-crc`
ci/prow/e2e-microshift-crc	`ce335c0`	link	true	`/test e2e-microshift-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.

openshift-ci bot requested review from cfergeau and gbraad October 31, 2023 09:40

openshift-ci bot added the do-not-merge/hold label Oct 31, 2023

anjannath force-pushed the background-launcher-spike branch 2 times, most recently from 2de34a4 to bd749ff Compare November 6, 2023 11:42

anjannath removed the do-not-merge/hold label Nov 9, 2023

anjannath force-pushed the background-launcher-spike branch 2 times, most recently from da98ad6 to 985ebe3 Compare November 21, 2023 06:33

cfergeau reviewed Nov 24, 2023

View reviewed changes

anjannath force-pushed the background-launcher-spike branch from 985ebe3 to 89aeb34 Compare November 28, 2023 08:27

cfergeau reviewed Nov 28, 2023

View reviewed changes

anjannath force-pushed the background-launcher-spike branch 2 times, most recently from 87484bc to f6b64de Compare December 1, 2023 08:19

cfergeau reviewed Dec 4, 2023

View reviewed changes

anjannath force-pushed the background-launcher-spike branch from f6b64de to ed08a91 Compare December 5, 2023 10:09

cfergeau approved these changes Dec 5, 2023

View reviewed changes

openshift-ci bot assigned cfergeau Dec 5, 2023

openshift-ci bot added the lgtm label Dec 5, 2023

openshift-ci bot added the approved label Dec 5, 2023

anjannath force-pushed the background-launcher-spike branch from ed08a91 to b74d3a2 Compare December 6, 2023 04:03

openshift-ci bot removed the lgtm label Dec 6, 2023

gbraad mentioned this pull request Dec 7, 2023

[BUG] - CRC start on Windows fails with expected crcDaemon task to be Running but got Ready in v2.29 #3942

Closed

openshift-merge-robot added the needs-rebase label Dec 9, 2023

anjannath force-pushed the background-launcher-spike branch from b74d3a2 to 363d501 Compare December 18, 2023 04:10

openshift-merge-robot removed the needs-rebase label Dec 18, 2023

anjannath added 4 commits December 18, 2023 10:13

msi: add win32-background-launcher to installer

f1a9323

crc-embedder is updated to download win32-background-launcher.exe from its github release at crc-org/win32-backgroun-launcher so that it is available while building the msi it'll be used to start the crc daemon as a background process

anjannath force-pushed the background-launcher-spike branch from 363d501 to ce335c0 Compare December 18, 2023 05:08

anjannath merged commit e4fc918 into crc-org:main Dec 21, 2023
16 of 20 checks passed

anjannath mentioned this pull request Jan 8, 2024

Start daemon task on windows using win32-background-launcher #3879

Closed

5 tasks

anjannath mentioned this pull request Mar 12, 2024

[BUG] Openshift Local Ver.2.5.1 after every reboot of the PC a terminal window shows with some HTTP Requests #3250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `win32-background-launcher` to start the daemon task #3901

Use `win32-background-launcher` to start the daemon task #3901

anjannath commented Oct 31, 2023 •

edited

Loading

anjannath commented Oct 31, 2023

anjannath commented Nov 16, 2023

gbraad commented Nov 24, 2023

adrianriobo commented Nov 24, 2023

anjannath commented Nov 24, 2023 •

edited

Loading

cfergeau commented Nov 24, 2023

cfergeau left a comment

cfergeau Nov 24, 2023

cfergeau Nov 24, 2023

cfergeau Nov 28, 2023

cfergeau Dec 4, 2023

cfergeau Dec 4, 2023

cfergeau left a comment

cfergeau Dec 5, 2023

anjannath Dec 6, 2023

cfergeau Dec 5, 2023

openshift-ci bot commented Dec 5, 2023

openshift-ci bot commented Dec 6, 2023

openshift-ci bot commented Dec 18, 2023

Use win32-background-launcher to start the daemon task #3901

Use win32-background-launcher to start the daemon task #3901

Conversation

anjannath commented Oct 31, 2023 • edited Loading

anjannath commented Oct 31, 2023

anjannath commented Nov 16, 2023

gbraad commented Nov 24, 2023

adrianriobo commented Nov 24, 2023

anjannath commented Nov 24, 2023 • edited Loading

cfergeau commented Nov 24, 2023

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

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

openshift-ci bot commented Dec 5, 2023

openshift-ci bot commented Dec 6, 2023

openshift-ci bot commented Dec 18, 2023

Use `win32-background-launcher` to start the daemon task #3901

Use `win32-background-launcher` to start the daemon task #3901

anjannath commented Oct 31, 2023 •

edited

Loading

anjannath commented Nov 24, 2023 •

edited

Loading