preflight: simplify isDaemonProcess function #3927
Conversation
| if len(cmdLine) >= 4 && cmdLine[1] == "--log-level" && cmdLine[3] == "daemon" { | ||
| return true | ||
| for _, arg := range cmdLine { | ||
| if strings.Contains(strings.TrimSpace(arg), "daemon") { |
There was a problem hiding this comment.
config set skip-check-daemon-task-running true would therefore be valid here, right?
Note: I understand a config would not run preflights, but it is still something to consider, as start and setup do run them and might have a flag
There was a problem hiding this comment.
yes, but very unlikely to end up in that situation as the config command is not a long running command
the main purpose of this isDaemonProcess is to prevent the crc cleanup command as being detected as the daemon process
There was a problem hiding this comment.
config set skip-check-daemon-task-running true would therefore be valid here, right?
for this specific instance, we can handle it by changing the check to arg == "daemon" instead of strings.Contains check
There was a problem hiding this comment.
Also not a big fan of doing string matching, we do that far too much, and there are always corner cases we have to ignore as "unlikely". In this case it has the benefit of being simple though. The correct way (= without false positives) would be to parse the process commandline using cobra, but that's most likely overkill (= a lot of code) for what we need here.
An alternative if we want to keep the current checks would be to drop the args at the beginning of the slice until we find one which ends with crcProcessName, but that's probably also overkill.
if Path of crc cli has whitespaces assumptions about the position of expected args is not always correct
One question though, was this code always broken on Windows? If I understand correctly, you are saying that C:\Program Files\OpenShift Local\crc.exe gets split as C:\Program, Files\OpenShift, Local\crc.exe?
There was a problem hiding this comment.
One question though, was this code always broken on Windows? If I understand correctly, you are saying that C:\Program Files\OpenShift Local\crc.exe gets split as C:\Program, Files\OpenShift, Local\crc.exe?
yes, this was always broken on windows, when we tested it previously we probably didn't test from a location with spaces in the path name
There was a problem hiding this comment.
But the default location will always have spaces in its name, won't it?
There was a problem hiding this comment.
yes it'll always have spaces C:\Program Files\Red Hat OpenShift Local\
There was a problem hiding this comment.
Should be fine to do what you suggested (ideally without TrimSpace):
for this specific instance, we can handle it by changing the check to arg == "daemon" instead of strings.Contains check
instead of checking for expected args at exact position we can simply look for the presence of the daemon word this also fixes the following error: ``` INFO Removing older logs Failed to remove old log files: Failed to delete file: remove C:\Users\crcqe\.crc\crcd.log: The process cannot access the file because it is being used by another process. ``` it was occurring because the psutil/process.CmdlineSlice() returns the args by splitting the args on whitespace so if Path of crc cli has whitespaces assumptions about the position of expected args is not always correct
|
@anjannath: The following tests failed, say
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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gbraad 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:
Approvers can indicate their approval by writing |
instead of checking for expected args at exact position we can simply look for the presence of the daemon word
this also fixes the following error:
it was occurring because the psutil/process.CmdlineSlice() returns the args by splitting the args on whitespace so if Path of crc cli has whitespaces assumptions about the position of expected args is not always correct