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.

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

Add option to hide processes running in a container #1062

Merged
merged 3 commits into from Aug 29, 2022

Conversation

valdaarhun
Copy link
Contributor

Might close #1036

The patch makes use of NSpid to check whether a process runs inside a pid namespace that is different from the host's init process' pid namespace. If this check is true, the cgroup name is checked to ensure it isn't a process running on the host that happens to run in a different pid namespace.

@BenBE
Copy link
Member

BenBE commented Aug 9, 2022

Please have a look at the compile issues mentioned by the CI. There seem to be some headers missing for iswdigit AFAICS.

@BenBE BenBE added Linux 🐧 Linux related issues new feature Completely new feature labels Aug 9, 2022
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
Comment on lines 1692 to 1693
if (!LinuxProcessList_checkPidNamespace((Process *)lp, procFd))
goto errorReadingProcess;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this should cause a full bail-out for reading this process …

@cgzones @fasterit What do you think?

linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
Comment on lines 1546 to 1561
if ((ss->flags & PROCESS_FLAG_LINUX_CGROUP) || hideRunningInContainer) {
LinuxProcessList_readCGroupFile(lp, procFd);
if (hideRunningInContainer && lp->cgroup &&
!(String_startsWith(lp->cgroup, "/user") || String_startsWith(lp->cgroup, "/machine") || String_startsWith(lp->cgroup, "/system")) ) {
if (!LinuxProcessList_checkPidNamespace((Process *)lp, procFd))
goto errorReadingProcess;
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move this block of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before moving the code, there was a significant lag between the moment one enables the "hide running in a container" option and the moment the change is actually seen on the screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My theory is that it's because, initially, none of the processes are marked as "running in container". If the option is toggled just after this condition.

if ((ss->flags & PROCESS_FLAG_LINUX_CGROUP) || hideRunningInContainer)

The change will still not take effect in the next iteration of the loop, since the process will still not be marked as "running in a container" in this condition.

if (preExisting && hideRunningInContainer && Process_isRunningInContainer(proc))

It finally takes effect after one more iteration.

@BenBE BenBE requested a review from cgzones August 10, 2022 16:06
cgzones added a commit to cgzones/htop that referenced this pull request Aug 14, 2022
Can be used to hide processes of containers.

Alternative to htop-dev#1062
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
@valdaarhun valdaarhun force-pushed the FEAT_hide_container_procs branch 2 times, most recently from 166a314 to 12219cf Compare August 15, 2022 18:57
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The overall changes look okay now, but the commit history could manage some cleanup with git rebase (squashing the cleanups into the commits they apply to). This gives a much nicer history and allows for better understanding of what changes where done why (otherwise half of the changes land on some fixups unnecessarily).

P.S.: Feel free to clean up the history on your PR as much as you like. I think targeting about 3 commits (e.g.: 1. whitespace fix in linux/LinuxProcessList.c around line 1640, 2. Implementing the overall feature, 3. adding UI for it) might be good. Each commit for itself should be able to compile.

Process.h Outdated Show resolved Hide resolved
linux/LinuxProcessList.c Outdated Show resolved Hide resolved
@valdaarhun
Copy link
Contributor Author

Feel free to clean up the history on your PR as much as you like

Cool. I'll clean up the commit history and push it again.

valdaarhun and others added 2 commits August 18, 2022 00:44
Add actionToggle and fix LinuxProcessList_checkPidNamespace

Read cgroup file irrespective of flags

Improve logic to check if running in container

Add isContainerOrVMSlice()

Also change "(Process *)lp" to "proc"

Remove check for root slice

Remove Process_isRunningInContainer

Co-authored-by: BenBE <BenBE@geshi.org>
Co-authored-by: BenBE <BenBE@geshi.org>
@valdaarhun
Copy link
Contributor Author

I have cleaned up the commit history as best as I could.

Copy link
Member

@BenBE BenBE left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@cgzones cgzones merged commit 7b83459 into htop-dev:main Aug 29, 2022
@BenBE BenBE added this to the 3.2.2 milestone Aug 29, 2022
@m0vie
Copy link

m0vie commented May 6, 2023

The O keyboard shortcut is not documented in the F1 help. Could this also be added?
Same for the man page.

@valdaarhun
Copy link
Contributor Author

@m0vie Hi, thanks for pointing this out. This must have slipped my mind. It's been resolved in #1250 .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linux 🐧 Linux related issues new feature Completely new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude processes running in a container (i.e. Docker)
4 participants