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

Compress cgroup names based on some heuristics #849

Merged
merged 7 commits into from
Nov 28, 2021

Conversation

BenBE
Copy link
Member

@BenBE BenBE commented Oct 19, 2021

This is some WIP for #216. The current implementation handles most common cgroup namespace patterns encountered for system/user/machine slices, shortens scope identifiers, trims service names and handles both LXC and systemd-nspawn (SNC) containers.

The code probably doesn't win a beauty contest yet, but should suffice as a preview.

Points to clean/optimize:

  • Shorten pattern output boilerplate (there's much repetition for string writing)
  • Clean up code flow for some of the more complex rules
  • Extract some helpers to ease parsing / string handling
  • Improve multi-label awareness to simplify rules relying on multiple tokens (like user slices, SNC)
  • Allow length determination mode to check for required buffer size
  • Include some more legacy rules
  • Document important shortening rules

@BenBE BenBE added enhancement Extension or improvement to existing feature needs-discussion 🤔 Changes need to be discussed and require consent Linux 🐧 Linux related issues labels Oct 19, 2021
@BenBE BenBE self-assigned this Oct 19, 2021
@BenBE BenBE linked an issue Oct 19, 2021 that may be closed by this pull request
@BenBE BenBE mentioned this pull request Oct 21, 2021
Copy link
Member

@fasterit fasterit left a comment

Choose a reason for hiding this comment

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

Very quick review:

  • s.buf is not always free'd
  • N/A will never be output as the initial / (slash) remains in cgroup_short
  • This works reasonably well and makes the column much more useful
  • Consider making it a separate column (CCGROUP) or configurable or switching together with m (merge command line). Rationale: it is a heuristic and it will have error cases. So the user should have choice / a fallback option. Making it a separate column would allow the other CGROUP column to be also insanely wide so it becomes at least useful.
  • Please add a section to the htop man file for it including the cases it handles (systemd cgroup hierarchy, systemd nspawn, LXC as of now)
  • (for a v2) colors would be nice!

@BenBE
Copy link
Member Author

BenBE commented Oct 22, 2021

Very quick review:

  • s.buf is not always free'd

Can you give a rough hint which case you noticed?

Edit: Okay, I found one place (successful return branch in LinuxProcessList.c), any others?

  • N/A will never be output as the initial / (slash) remains in cgroup_short

hmmm, N/A as in process->cgroup == NULL should be handled. if String_eq(process->cgroup, "/") then having / displayed is expected behavior IMHO.

  • This works reasonably well and makes the column much more useful

This aligns with the initial feedback I got from @lucaswerkmeister who provided me with the test cases for systemd-nspawn

In the final patch I'll probably go to have this column 40 chars wide, as even with this compression there's still quite a few processes where this ain't enough. Though with #854 on its way there could be another route …

  • Consider making it a separate column (CCGROUP) or configurable or switching together with m (merge command line). Rationale: it is a heuristic and it will have error cases. So the user should have choice / a fallback option. Making it a separate column would allow the other CGROUP column to be also insanely wide so it becomes at least useful.

Sounds like a good idea. Switching this together with command line merging is probably not a good choice though, as those features are too unrelated. But with two columns the user can choose the way to go. That aside, there's one fix we have to do for the column display anyway (namely the .30 width spec I added).

  • Please add a section to the htop man file for it including the cases it handles (systemd cgroup hierarchy, systemd nspawn, LXC as of now)

Will do. Proposals for wording always welcome. ;-)

  • (for a v2) colors would be nice!

Actually this shouldn't be too hard to implement given I already have the output code provided as a callback. Thus basically adding in another parameter for the color for the callback function and replacing char* buf by some RichString … Will provide this as a separate PR though.

@BenBE BenBE removed the needs-discussion 🤔 Changes need to be discussed and require consent label Nov 7, 2021
@BenBE BenBE added this to the 3.1.2 milestone Nov 7, 2021
@BenBE BenBE merged commit b98a4f8 into htop-dev:main Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Extension or improvement to existing feature Linux 🐧 Linux related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A column for LXC and LXD containers
3 participants