Mac: Enable daemon service to run as soon as user login #3095

praveenkumar · 2022-03-31T12:16:43Z

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 launchd
User reboot
Tray autostart with reboot but crc setup --check-only fails
because launchd agent 'com.redhat.crc.daemon' 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.

cfergeau · 2022-03-31T12:32:20Z

I thought launchd would ensure the daemon is started when the user opens a session on their mac? The launchd changes don't guarantee the daemon will be running during startup?

praveenkumar · 2022-03-31T14:22:57Z

I thought launchd would ensure the daemon is started when the user opens a session on their mac? The launchd changes don't guarantee the daemon will be running during startup?

@cfergeau As of now launchd is working same way as we are doing in linux except the socket activation part (in linux our socket activation service is enabled but that doesn't start daemon until something query to that socket) here also I think we don't want to start the daemon by default by launchd, right now it is required because of tray but in future tray is not part of crc.

praveenkumar · 2022-03-31T14:24:44Z

/retest

cfergeau · 2022-03-31T14:36:55Z

@cfergeau As of now launchd is working same way as we are doing in linux except the socket activation part (in linux our socket activation service is enabled but that doesn't start daemon until something query to that socket) here also I think we don't want to start the daemon by default by launchd, right now it is required because of tray but in future tray is not part of crc.

On linux, the daemon is not automatically started on login because we can avoid it thanks to the socket activation files. If socket activation did not exist, we would have to start the daemon automatically on login.
If the daemon is not autostarted, I don't understand what launchd is used for? What does this bring over just running crc daemon manually?

praveenkumar · 2022-03-31T15:24:19Z

If the daemon is not autostarted, I don't understand what launchd is used for? What does this bring over just running crc daemon manually.

Running daemon manually means you have to keep that terminal always open or manually run it in background, as user you have to make sure stop it when not required crc cleanup not remove that process also, so even with current state it is better than running manually but our plan is to have it auto start every restart that we can improve over time.

cfergeau · 2022-03-31T15:26:17Z

even with current state it is better than running manually but our plan is to have it auto start every restart that we can improve over time.

I am really unclear on what "the current state" is. I'm totally lost as to what happens if I run crc setup, and then reboot the machine. How will the daemon be started?
Isn't autostarting the daemon on session logging one or 2 lines to change in the launchd plist file?

praveenkumar · 2022-04-01T08:14:27Z

even with current state it is better than running manually but our plan is to have it auto start every restart that we can improve over time.

I am really unclear on what "the current state" is. I'm totally lost as to what happens if I run crc setup, and then reboot the machine. How will the daemon be started? Isn't autostarting the daemon on session logging one or 2 lines to change in the launchd plist file?

@cfergeau I thought we don't want that but yes it is easy and I updated the PR accordingly.

pkg/os/launchd/launchd_darwin.go

cfergeau · 2022-04-04T10:12:54Z

One limitation to the current approach is if the daemon was started by the tray in a previous version. Starting the daemon with launchd will silently fail, and nothing detects this old daemon version.
One way to solve this is to check the version of the running daemon as was done in an earlier iteration of this code, and kill any old version before restarting it in the proper version.

cfergeau · 2022-04-05T13:17:29Z

pkg/crc/preflight/preflight_checks_common.go

 	"github.com/pkg/errors"
 )

+// This make sure golint doesn't error out because not
+// using it on all the platforms
+var _ = isDaemonRunningWithReleasedVersion()


isDaemonRunningWithReleasedVersion is almost the same code as checkDaemonStarted which is used on all platforms, so this could be factored if you want to avoid this.

This line causes crc to freeze on startup on linux.

@cfergeau Handled in #3099 , will hold this until that merge and then rebase it

cfergeau · 2022-04-05T13:18:59Z

pkg/crc/preflight/preflight_checks_common.go

@@ -158,7 +161,57 @@ func isDaemonRunningWithReleasedVersion() bool {
 	}
 	if ver.CrcVersion != version.GetCRCVersion() {
 		logging.Debugf("Daemon is running with %s version but binary version is %s", ver.CrcVersion, version.GetCRCVersion())
+		if err := killExistingDaemonProcess(); err != nil {
+			logging.Debugf("Failed to kill the existing crc daemon process: %v", err)
+		}


This function is called isDaemonRunningWithReleasedVersion, it's not supposed to stop/kill the daemon as a side effect.

The killing of the existing daemon process can be part of the fix function for this check?

cfergeau · 2022-04-05T13:20:55Z

pkg/crc/preflight/preflight_checks_common.go

+	return nil
+}
+
+// isDaemonProcess return true if following conditions met


if any of the following conditions are met? if all of the following conditions are met? if none of the following conditions are met?

@cfergeau updated to any of the following.

cfergeau · 2022-04-05T13:23:51Z

pkg/crc/preflight/preflight_checks_darwin.go

+	if !isDaemonRunningWithReleasedVersion() {
+		if !launchd.AgentRunning(daemonConfig.Label) {
+			return fmt.Errorf("launchd agent '%s' is not running", daemonConfig.Label)
+		}


What about writing it like this? This seems clearer regarding what the code wants to do.

if olderDaemonVersionRunning() { if err := killDaemonProcess(); err != nil { return err } } if !launchd.AgentRunning(daemonConfig.Label) { return fmt.Errorf("launchd agent '%s' is not running", daemonConfig.Label) }

This will be also easy as once #3099 in.

@cfergeau It is now suggested way.

Anjan had a good point though that the killing fits better in a fix method rather than in check.

praveenkumar · 2022-04-06T15:43:40Z

/hold

praveenkumar · 2022-04-08T05:27:59Z

/unhold

praveenkumar · 2022-04-08T05:32:13Z

pkg/crc/preflight/preflight_checks_linux.go

+			return err
+		}
+	}
+	return nil


In linux I have to add it in this socket file check otherwise cross-lint error out not using killDaemonProcess function to linux. If we don't want to do that then might be we need to create a different file preflight_check_nonlinux and move this there.

linux is already taking care of stopping the daemon if needed, I'd rather not add code which is going to mess with the preeexisting code which is working fine.

gbraad · 2022-04-08T09:27:38Z

praveenkumar added the has-to-be-in-release label 4 hours ago

Why was this tag added? Is everything lined up correctly to allow this?
Which issue does this solve or related to?

praveenkumar · 2022-04-08T10:56:21Z

praveenkumar added the has-to-be-in-release label 4 hours ago

Why was this tag added?

@gbraad because I want this to be part of 2.1.0 release (upcoming one).

Is everything lined up correctly to allow this?

Yes, tray side changes were already there, when I filed this PR initially I thought it would be a trival but @cfergeau suggestions around having launchd service as auto reload and also detecting the old version of daemon and kill it, took
some time to implement and getting it reviewed.

Which issue does this solve or related to?

This will solve the issue for a user who just want to do crc setup and crc start on mac with or without tray.

gbraad · 2022-04-08T11:00:35Z

I am just concerned about how this was planned. Feels a little last minute.

We have to better describe dependency to merge, or accept a known issue?

anjannath · 2022-04-08T11:44:19Z

I am just concerned about how this was planned. Feels a little last minute.
We have to better describe dependency to merge, or accept a known issue?

The review comments are mostly addressed, i'll test this now e2e with the tray and upgrade scenario

cfergeau · 2022-04-08T08:38:42Z

cmd/crc/cmd/daemon_windows.go

-func daemonNotRunningMessage() string {
-	return genericDaemonNotRunningMessage
-}
-


fwiw, I would have moved these to start.go in the previous commit

cfergeau · 2022-04-08T08:45:23Z

pkg/crc/preflight/preflight_checks_linux.go

+			return err
+		}
+	}
+	return nil


linux is already taking care of stopping the daemon if needed, I'd rather not add code which is going to mess with the preeexisting code which is working fine.

cfergeau · 2022-04-08T08:47:53Z

pkg/crc/preflight/preflight_checks_common.go

+		if err := daemonclient.CheckIfOlderVersion(v); err != nil {
+			return true
+		}
+	}


Can this be written the usual go way?

v, err := dosomething() if err != nil { return err } err = dosomethingelse(v) if err != nil { return err }

This removes the extra indentation, construct is familiar so it's easier to read.

cfergeau · 2022-04-08T12:46:13Z

pkg/crc/preflight/preflight_checks_darwin.go

+	if !isDaemonRunningWithReleasedVersion() {
+		if !launchd.AgentRunning(daemonConfig.Label) {
+			return fmt.Errorf("launchd agent '%s' is not running", daemonConfig.Label)
+		}


Anjan had a good point though that the killing fits better in a fix method rather than in check.

openshift-ci · 2022-04-08T17:19:59Z

@praveenkumar: The following test 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/e2e-crc	`19b4b50`	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.

cfergeau

Can you add a description of the bug this fixes, or a link to the relevant issue? Apart from this, looks good to me.

openshift-ci · 2022-04-11T07:11:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, 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~~ [cfergeau,praveenkumar]

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

This PR enable the daemon service to run user session basis, if system reboot and user logs in then this service now auto started. - User installs crc first time and do crc setup - As part of the setup crc daemon is started by launchd - User reboots - Tray autostart with reboot but crc setup --check-only fails because launchd agent 'com.redhat.crc.daemon' is not running which triggers the on-boarding fixes: crc-org#3104

openshift-ci · 2022-04-11T08:57:06Z

New changes are detected. LGTM label has been removed.

openshift-ci bot requested review from cfergeau and gbraad March 31, 2022 12:16

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

praveenkumar force-pushed the mac_daemon_check branch from d2b4258 to e8f2b08 Compare April 1, 2022 08:13

praveenkumar changed the title ~~Mac: Check if daemon process is already running~~ Mac: Enable daemon service to run as soon as user login Apr 1, 2022

anjannath reviewed Apr 1, 2022

View reviewed changes

pkg/os/launchd/launchd_darwin.go Show resolved Hide resolved

cfergeau reviewed Apr 5, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Apr 6, 2022

praveenkumar force-pushed the mac_daemon_check branch 2 times, most recently from a19697b to b397133 Compare April 8, 2022 05:25

praveenkumar mentioned this pull request Apr 8, 2022

Daemon: Move daemon running check to daemon client package #3099

Closed

openshift-ci bot removed the do-not-merge/hold label Apr 8, 2022

praveenkumar added the has-to-be-in-release This PR need to go in coming release. label Apr 8, 2022

praveenkumar commented Apr 8, 2022

View reviewed changes

anjannath mentioned this pull request Apr 8, 2022

[BUG] when daemon is already started from tray crc setup fails and triggers the onboarding #3104

Closed

cfergeau reviewed Apr 8, 2022

View reviewed changes

praveenkumar force-pushed the mac_daemon_check branch from b397133 to 19b4b50 Compare April 8, 2022 15:12

cfergeau approved these changes Apr 11, 2022

View reviewed changes

openshift-ci bot assigned cfergeau Apr 11, 2022

openshift-ci bot added the lgtm label Apr 11, 2022

praveenkumar force-pushed the mac_daemon_check branch from 19b4b50 to ff32263 Compare April 11, 2022 08:57

openshift-ci bot removed the lgtm label Apr 11, 2022

praveenkumar merged commit 2af42b9 into crc-org:main Apr 11, 2022

Mac: Enable daemon service to run as soon as user login #3095

Mac: Enable daemon service to run as soon as user login #3095

Conversation

praveenkumar commented Mar 31, 2022

cfergeau commented Mar 31, 2022

praveenkumar commented Mar 31, 2022

praveenkumar commented Mar 31, 2022

cfergeau commented Mar 31, 2022

praveenkumar commented Mar 31, 2022

cfergeau commented Mar 31, 2022 • edited Loading

praveenkumar commented Apr 1, 2022

cfergeau commented Apr 4, 2022

cfergeau Apr 5, 2022 • edited Loading

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 Apr 6, 2022

praveenkumar commented Apr 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbraad commented Apr 8, 2022 • edited Loading

praveenkumar commented Apr 8, 2022

gbraad commented Apr 8, 2022

anjannath commented Apr 8, 2022 • edited Loading

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 Apr 8, 2022

cfergeau left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Apr 11, 2022

openshift-ci bot commented Apr 11, 2022

cfergeau commented Mar 31, 2022 •

edited

Loading

cfergeau Apr 5, 2022 •

edited

Loading

gbraad commented Apr 8, 2022 •

edited

Loading

anjannath commented Apr 8, 2022 •

edited

Loading