refactor (shell) : Refactor CRC unix/linux shell detection logic using gopsutil (#4562) #4572

rohanKanojia · 2025-01-16T15:20:06Z

Description

Refactor CRC Shell detection for UNIX/Linux Systems to rely on parent process name. I was only able to get this approach working for Linux/Unix systems where gopsutil library is correctly detecting parent PIDs. On WSL, it's not able to see beyond windows processes.

Type of change

Bug fix (non-breaking change which fixes an issue)
Feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change
Chore (non-breaking change which doesn't affect codebase;
test, version modification, documentation, etc.)

Proposed changes

Move parsing ps output and picking recent pid logic to shell_windows. It would only be used in case of WSL.
Instead of parsing ps output and picking up recent pid. Inspect parent process of current process and keep going up until we find a shell process. Pick that shell process as currently active shell.
- As of now I've set a depth limit of 10 levels, if we're not able to find any shell process after going this deep, we return blank value.

Testing

Start CRC cluster and run oc-env / podman-env commands on bash, zsh or fish shells.

Contribution Checklist

I Keep It Small and Simple: The smaller the PR is, the easier it is to review and have it merged
I have performed a self-review of my code
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Which platform have you tested the code changes on?
- Linux
- Windows
- MacOS (QE Lab instance)

openshift-ci · 2025-01-16T15:20:10Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cfergeau · 2025-01-17T10:19:18Z

On WSL, it's not able to see beyond windows processes.

What about Windows with powershell or cmd?

rohanKanojia · 2025-01-17T11:30:50Z

@cfergeau : In this PR I had kept changes limited to parts I had touched on in #4526 . In my previous pull request for Shell detection, I had only made changes for WSL and Unix, Linux platforms.

I just tested this approach on PowerShell and CMD. I can verify that this approach is able to detect shell processes from parent tree.

Some Debug logs on PowerShell:

PS C:\Users\rokum\go\src\github.com\crc-org\crc> C:\Users\rokum\go\bin\crc.exe oc-env
process name : crc.exe
parent process id : 8060
process name : powershell.exe
parent process id : 15508
process name : WindowsTerminal.exe
parent process id : 8592
process name : explorer.exe
parent process id : 8476
process name :
 detectShellByCheckingProcessTree
$Env:PATH = "C:\Users\rokum\.crc\bin\oc;$Env:PATH"
# Run this command to configure your shell:
# & crc oc-env | Invoke-Expression

Some Debug logs on CMD:

C:\Users\rokum>C:\Users\rokum\go\bin\crc.exe oc-env
process name : crc.exe
parent process id : 14804
process name : cmd.exe
parent process id : 8592
process name : explorer.exe
parent process id : 8476
process name :
 detectShellByCheckingProcessTree
SET PATH=C:\Users\rokum\.crc\bin\oc;%PATH%
REM Run this command to configure your shell:
REM @FOR /f "tokens=*" %i IN ('crc oc-env') DO @call %i

Shall I modify this PR to cover PowerShell and CMD as well?

cfergeau

Another potential follow-up is to use detectShellByCheckingProcessTree in detect() in shell_windows.go as both seem to have similar functionalities.

cfergeau · 2025-01-23T10:51:24Z

pkg/os/shell/shell_windows.go

@@ -101,3 +105,83 @@ func detect() (string, error) {

 	return shellType(shell, "cmd"), nil
 }
+
+// detectShellByInvokingCommand is a utility method that tries to detect current shell in use by invoking `ps` command.
+// This method is extracted so that it could be used by unix systems as well as Windows (in case of WSL). It executes


I think now it is only used on Windows in the WSL case?

Sorry, I forgot to update this comment after moving the method. Thanks for noticing! It should be okay now.

cfergeau · 2025-01-23T10:52:45Z

pkg/os/shell/shell_unix.go

 )

 var (
-	ErrUnknownShell = errors.New("Error: Unknown shell")
+	ErrUnknownShell                             = errors.New("Error: Unknown shell")
+	getCurrentProcessID                         = os.Getpid


In my opinion, it is slightly easier to follow if you name this getPid, in a way this removes one level of indirection.

Thanks, I've renamed the getter to getPid as you suggested.

pkg/os/shell/shell_unix.go

rohanKanojia · 2025-01-24T13:43:23Z

Another potential follow-up is to use detectShellByCheckingProcessTree in detect() in shell_windows.go as both seem to have similar functionalities.

Can I tackle this one as a follow-up task?

cfergeau · 2025-01-24T14:58:46Z

Another potential follow-up is to use detectShellByCheckingProcessTree in detect() in shell_windows.go as both seem to have similar functionalities.

Can I tackle this one as a follow-up task?

Yup definitely!

cfergeau

minor comments left, looks good to me!

cfergeau · 2025-01-28T11:50:32Z

pkg/os/shell/shell_unix.go

 )

 var (
 	ErrUnknownShell = errors.New("Error: Unknown shell")
+	getPid          = getCurrentProcess


With the changes you did, getPid is no longer very accurate :-/

Sorry for the leftover typo, I've renamed it.

pkg/os/shell/shell_unix.go

cfergeau · 2025-01-28T11:53:32Z

pkg/os/shell/shell_unix.go

+// process.Process object. This implementation is used in production code.
+type RealProcess struct {
+	realProcess *process.Process
+}


If you don't give a name to realProcess, you don't need to reimplement Name, go will reuse the implementation from *process.Process:

type RealProcess struct { *process.Process }

Thanks, I've updated it as per your suggestion.

cfergeau · 2025-01-28T12:00:54Z

pkg/os/shell/shell_unix.go

+	return &RealProcess{parentProcess}, nil
+}
+
+func NewRealProcess(pid int32) (*RealProcess, error) {


getCurrentProcess is the single user of NewRealProcess, you could consider merging the 2 especially as most of their code is error-handling.

You're right. I've merged both methods into single one

…-org#4562) + Move parsing ps output and picking recent pid approach to shell_windows. It would only be used in case of WSL. + Instead of parsing ps output and picking up recent pid. Inspect parent process of current process and keep going up until we find a shell process. Pick that shell process as currently active shell. Signed-off-by: Rohan Kumar <[email protected]>

We're currently using an old version of gopsutil library. Upgrade to the latest version in order keep dependency up to date. This commit includes changes to update the dependency version in project and also vendor directory updates Signed-off-by: Rohan Kumar <[email protected]>

openshift-ci · 2025-01-28T17:36:23Z

@rohanKanojia: 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/security	`dfcf769`	link	false	`/test security`
ci/prow/e2e-crc	`dfcf769`	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-sigs/prow repository. I understand the commands that are listed here.

cfergeau · 2025-01-29T08:02:43Z

/lgtm
/approve

openshift-ci · 2025-01-29T08: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

rohanKanojia force-pushed the pr/issue4562 branch from d66474e to 44d2c5a Compare January 16, 2025 15:20

openshift-ci bot added the do-not-merge/work-in-progress label Jan 16, 2025

rohanKanojia changed the title ~~refactor (shell) : Refactor unix/linux shell detection mechanism (#4562)~~ refactor (shell) : Refactor CRC unix/linux shell detection logic using gopsutil (#4562) Jan 16, 2025

rohanKanojia force-pushed the pr/issue4562 branch 4 times, most recently from 1e916f7 to 75de20c Compare January 17, 2025 05:50

rohanKanojia marked this pull request as ready for review January 17, 2025 08:58

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 17, 2025

openshift-ci bot requested review from cfergeau and praveenkumar January 17, 2025 08:58

rohanKanojia force-pushed the pr/issue4562 branch 2 times, most recently from a509dee to a81df8a Compare January 23, 2025 04:21

cfergeau reviewed Jan 23, 2025

View reviewed changes

rohanKanojia force-pushed the pr/issue4562 branch from a81df8a to 676563a Compare January 24, 2025 13:27

rohanKanojia force-pushed the pr/issue4562 branch from aba563f to d3375d7 Compare January 24, 2025 13:51

cfergeau reviewed Jan 28, 2025

View reviewed changes

rohanKanojia added 2 commits January 28, 2025 20:54

rohanKanojia force-pushed the pr/issue4562 branch from d3375d7 to dfcf769 Compare January 28, 2025 15:29

openshift-ci bot assigned cfergeau Jan 29, 2025

openshift-ci bot added the lgtm label Jan 29, 2025

openshift-ci bot added the approved label Jan 29, 2025

praveenkumar merged commit 4851d7a into crc-org:main Jan 30, 2025
28 of 36 checks passed

rohanKanojia deleted the pr/issue4562 branch January 30, 2025 07:58

rohanKanojia mentioned this pull request Jan 30, 2025

Refactor Shell detection on windows in order to use gopsutil #4588

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor (shell) : Refactor CRC unix/linux shell detection logic using gopsutil (#4562) #4572

refactor (shell) : Refactor CRC unix/linux shell detection logic using gopsutil (#4562) #4572

rohanKanojia commented Jan 16, 2025

openshift-ci bot commented Jan 16, 2025

cfergeau commented Jan 17, 2025

rohanKanojia commented Jan 17, 2025

cfergeau left a comment

cfergeau Jan 23, 2025

rohanKanojia Jan 24, 2025

cfergeau Jan 23, 2025

rohanKanojia Jan 24, 2025 •

edited

Loading

rohanKanojia commented Jan 24, 2025

cfergeau commented Jan 24, 2025

cfergeau left a comment

cfergeau Jan 28, 2025

rohanKanojia Jan 28, 2025 •

edited

Loading

cfergeau Jan 28, 2025

rohanKanojia Jan 28, 2025

cfergeau Jan 28, 2025

rohanKanojia Jan 28, 2025

openshift-ci bot commented Jan 28, 2025

cfergeau commented Jan 29, 2025

openshift-ci bot commented Jan 29, 2025

refactor (shell) : Refactor CRC unix/linux shell detection logic using gopsutil (#4562) #4572

refactor (shell) : Refactor CRC unix/linux shell detection logic using gopsutil (#4562) #4572

Conversation

rohanKanojia commented Jan 16, 2025

Description

Type of change

Proposed changes

Testing

Contribution Checklist

openshift-ci bot commented Jan 16, 2025

cfergeau commented Jan 17, 2025

rohanKanojia commented Jan 17, 2025

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

rohanKanojia Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

rohanKanojia commented Jan 24, 2025

cfergeau commented Jan 24, 2025

cfergeau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohanKanojia Jan 28, 2025 • 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

openshift-ci bot commented Jan 28, 2025

cfergeau commented Jan 29, 2025

openshift-ci bot commented Jan 29, 2025

rohanKanojia Jan 24, 2025 •

edited

Loading

rohanKanojia Jan 28, 2025 •

edited

Loading