Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor CRC shell detection mechanism to use process parent programatically #4562

Open
2 tasks
rohanKanojia opened this issue Jan 9, 2025 · 6 comments · May be fixed by #4572
Open
2 tasks

Refactor CRC shell detection mechanism to use process parent programatically #4562

rohanKanojia opened this issue Jan 9, 2025 · 6 comments · May be fixed by #4572
Assignees
@rohanKanojia

Comments

@rohanKanojia
Copy link
Contributor

rohanKanojia commented Jan 9, 2025 •
edited
Loading

Description

Originally posted by @cfergeau in #4526 (comment)

If we want to do some more extensive shell guessing by iterating over processes, could we not limit ourselves to the parents of the crc process?
I'd prefer that we extend this approach and drop the calls to an external command, and the parsing which comes with it.
In the scenario described above on macos, with pstree I get this so I can find the right terminal without relying on "latest launched"
(I'm not suggesting to use pstree directly, but to do something similar in go)

Acceptance Criteria

  • Investigate whether shell can be detected correctly using parent process approach programmatically without using any command
  • If investigation is successful , refactor current shell detection mechanism to be based on this approach.
@rohanKanojia rohanKanojia mentioned this issue Jan 9, 2025
Merged
14 tasks
@cfergeau
Copy link
Contributor

cfergeau commented Jan 9, 2025 •
edited
Loading

I'd summarize this as

getNameAndItsPpid may not have the behaviour we want now, but can it be fixed to do what we need? Imo, this is what we want to do, start from the current process, and look at the process names/command lines until one is a shell process.

@rohanKanojia rohanKanojia changed the title Refactor CRC shell detection mechanism to use process parent programattically Refactor CRC shell detection mechanism to use process parent programatically Jan 9, 2025
@rohanKanojia rohanKanojia self-assigned this Jan 13, 2025
@rohanKanojia rohanKanojia moved this to Work In Progress in Project planning: crc Jan 15, 2025
@rohanKanojia
Copy link
Contributor Author

I modified getProcessEntry method to list all the processes that are included with the CreateToolhelp32Snapshot call by adding some debug statements. However, I'm not able to see shell processes in that list:

rohan@rokumar-lenovo:~$ /mnt/c/Users/rokum/go/bin/crc.exe oc-env --log-level debug
DEBU CRC version: 2.45.0+291032
DEBU OpenShift version: 4.17.10
DEBU MicroShift version: 4.17.10
DEBU Running 'crc oc-env'
Process ID: 0, [System Process]
Process ID: 4, System
Process ID: 172, Secure System
Process ID: 232, Registry
Process ID: 720, smss.exe
Process ID: 1168, csrss.exe
Process ID: 1264, wininit.exe
Process ID: 1292, csrss.exe
Process ID: 1348, services.exe
Process ID: 1380, winlogon.exe

Process ID: 10016, wsl.exe

I see that the process being listed is wsl.exe . I'm not able to get any additional information about linux shell type from this API.

@cfergeau
Copy link
Contributor

wsl is probably special, as it provides a linux virtual machine. There could be some mismatch in the APIs to use to walk process lists, the win32 APIs (getProcessEntry) would only see windows processes, but the shell we are looking for is a linux process running in the VM. So yes, with wsl maybe it is more convenient to invoke an external process.
What about cmd.exe, or macos shells, do you encounter the same limitations?

@rohanKanojia
Copy link
Contributor Author

rohanKanojia commented Jan 15, 2025 •
edited
Loading

The changes in #4526 (especially sorting and getting latest process approach) are only applicable for WSL, Unix and Linux environments.

I'm able to get the parent process approach working on Linux using these steps (it involved executing ps command using exec package to get information about parent process id and name):

  1. Get current process id using os.getpid()
  2. Get process name using ps command ps -p <pid> -o comm=
  3. Check if it’s one of bash, zsh or fish . If yes, then this is the shell process we were looking for.
  4. Get parent of current process using ps -o ppid= -p <pid>
  5. Repeat the same process from step 2

Shall I proceed with this approach?

@cfergeau
Copy link
Contributor

Maybe https://pkg.go.dev/github.com/shirou/gopsutil/[email protected]/process provides all you need (walking the process list and getting their command line).

I don't really like spawning external processes when we can avoid it, it is slow, depends on the user environment, text output needs to be parsed, good error reporting means more text parsing, ...

@rohanKanojia
Copy link
Contributor Author

rohanKanojia commented Jan 16, 2025 •
edited
Loading

@cfergeau : Thanks, I'm able to get it working with gopsutil library.

I'll update Unix/Linux shell detection code to use this approach.

rohanKanojia added a commit to rohankanojia-forks/crc that referenced this issue Jan 16, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
b57cce0 
…-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 added a commit to rohankanojia-forks/crc that referenced this issue Jan 16, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
eba34f6 
…-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 added a commit to rohankanojia-forks/crc that referenced this issue Jan 16, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
d66474e 
…-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 added a commit to rohankanojia-forks/crc that referenced this issue Jan 16, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
44d2c5a 
…-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 rohanKanojia linked a pull request Jan 16, 2025 that will close this issue
Open
12 tasks
rohanKanojia added a commit to rohankanojia-forks/crc that referenced this issue Jan 16, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
2c30556 
…-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 added a commit to rohankanojia-forks/crc that referenced this issue Jan 17, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
f31fa0d 
…-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 added a commit to rohankanojia-forks/crc that referenced this issue Jan 17, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
1e916f7 
…-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 added a commit to rohankanojia-forks/crc that referenced this issue Jan 17, 2025
@rohanKanojia
refactor (shell) : Refactor unix/linux shell detection mechanism (crc…
75de20c 
…-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 rohanKanojia moved this from Work In Progress to Ready for review in Project planning: crc Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rohanKanojia rohanKanojia

Labels
None yet
Projects
Project planning: crc
Status: Ready for review
Milestone
No milestone
2 participants
@cfergeau @rohanKanojia