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

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

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign praveenkumar for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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

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.

cfergeau · 2025-01-23T11:38:45Z

pkg/os/shell/shell_unix.go

 	"path/filepath"
+
+	"github.com/shirou/gopsutil/v3/process"


Why not v4? We probably should switch crc to the latest version, but this can be done in a follow-up commit if you prefer.

I've added a new commit on top of this one to upgrade to v4

cfergeau · 2025-01-23T11:40:21Z

pkg/os/shell/shell_unix.go

+type AbstractProcess interface {
+	Name() (string, error)
+	Ppid() (int32, error)
+}


It seems this code could be simpler if you use Parent() instead of Ppid() ?

And I think it's possible to remove getCurrentProcessID = os.Getpid and AbstractProcessSupplier and to replace both with var newProcess = process.NewProcess.

Or will this prevent some of the "failure" tests that you have from being doable?

Regardless of the previous suggestion, I'd add a comment saying these types are needed for the unit tests.
You could add var _ AbstractProcess = process.Process to make it clear that this type implements the interface.

Thanks for the suggestion, I've updated it to use Parent(), as you suggested. It's much clearer now!

…-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]>

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?

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]>

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!

openshift-ci · 2025-01-24T15:54:15Z

@rohanKanojia: 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	`d3375d7`	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.

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

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

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

cfergeau Jan 23, 2025

rohanKanojia Jan 24, 2025

cfergeau Jan 23, 2025

cfergeau Jan 23, 2025

rohanKanojia Jan 24, 2025 •

edited

Loading

rohanKanojia commented Jan 24, 2025

cfergeau commented Jan 24, 2025

openshift-ci bot commented Jan 24, 2025

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

Are you sure you want to change the base?

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

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

cfergeau Jan 23, 2025

Choose a reason for hiding this comment

rohanKanojia Jan 24, 2025

Choose a reason for hiding this comment

cfergeau Jan 23, 2025

Choose a reason for hiding this comment

rohanKanojia Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

cfergeau Jan 23, 2025

Choose a reason for hiding this comment

rohanKanojia Jan 24, 2025

Choose a reason for hiding this comment

cfergeau Jan 23, 2025

Choose a reason for hiding this comment

cfergeau Jan 23, 2025

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

openshift-ci bot commented Jan 24, 2025

rohanKanojia Jan 24, 2025 •

edited

Loading

rohanKanojia Jan 24, 2025 •

edited

Loading