New column for LXC containers name using the CGroups #217
Conversation
| @@ -533,6 +533,33 @@ static void LinuxProcessList_readCGroupFile(LinuxProcess* process, const char* d | |||
| if (!ok) break; | |||
| char* group = strchr(buffer, ':'); | |||
| if (!group) break; | |||
|
|
|||
| if (!process->lxc) { | |||
There was a problem hiding this comment.
Does the LXC cgroup never change for a running process? (Cause it's only computed once)
There was a problem hiding this comment.
Technically it could (changed process namespace), but practically it doesn't.
| char *slashpostion = strchr((groupPathname + 13), '/'); | ||
| if (slashpostion) { | ||
| // There is a '/' in the CGroup string (after the initial "/lxc.payload"), a '\0' will truncate the string at this position | ||
| groupPathname[(slashpostion - groupPathname)] = '\0'; |
There was a problem hiding this comment.
Here we are modifying the string behind groupPathname, which points into the string group, which get written later in line 568 (int wrote = snprintf(at, left, "%s", group);).
Do we really want to modify group?
Or maybe move the LXC block to the end of the while loop, so any changes to group are discarded.
There was a problem hiding this comment.
In all fairness, the current implementation of the CGROUP column is fairly useless on modern systems as it will show lengty SystemD CGroups for services started by it that breaks the alignment of columns as the values are too long compared to the column size.
I initially did put my code inside the LinuxProcessList_readCGroupFile() function in the middle of the CGROUP column code for simplicity but maybe it could be separated in another function as there probably wont be anybody using both columns at the same time or create two sub function called inside LinuxProcessList_readCGroupFile() for each of the columns.
| char *slashpostion = strchr((groupPathname + 5), '/'); | ||
| if (slashpostion) { | ||
| // There is a '/' in the CGroup string (after the initial "/lxc/"), a '\0' will truncate the string at this position | ||
| groupPathname[(slashpostion - groupPathname)] = '\0'; |
| @@ -542,6 +569,12 @@ static void LinuxProcessList_readCGroupFile(LinuxProcess* process, const char* d | |||
| left -= wrote; | |||
| } | |||
| fclose(file); | |||
| if (!process->lxc) { | |||
There was a problem hiding this comment.
This whole block is now a no-op (it is only entered if process->lxc is NULL)
There was a problem hiding this comment.
I'm not quite happy with the current CGroup->LXC container name code, both from a code cleanliness perspective (compare with e.g. the CTID/VPID patch (#197) and from handling the disambuguity that may arise from the read value.
Here's some example cgroup file from a LXC host:
# cat /proc/60054/cgroup
11:cpuset:/lxc.payload/example
10:devices:/lxc.payload/example/init.scope
9:net_cls,net_prio:/lxc.payload/example
8:blkio:/lxc.payload/example
7:perf_event:/lxc.payload/example
6:rdma:/lxc.payload/example
5:pids:/lxc.payload/example/init.scope
4:freezer:/lxc.payload/example
3:cpu,cpuacct:/lxc.payload/example
2:memory:/lxc.payload/example/init.scope
1:name=systemd:/lxc.payload/example/init.scope
0::/lxc.payload/example/init.scope
From this example it's quite clear, that that container is probably called example. It's parent looks like this:
# cat /proc/60051/cgroup
11:cpuset:/lxc.monitor/example
10:devices:/lxc.monitor/example
9:net_cls,net_prio:/lxc.monitor/example
8:blkio:/lxc.monitor/example
7:perf_event:/lxc.monitor/example
6:rdma:/lxc.monitor/example
5:pids:/lxc.monitor/example
4:freezer:/lxc.monitor/example
3:cpu,cpuacct:/lxc.monitor/example
2:memory:/lxc.monitor/example
1:name=systemd:/lxc.monitor/example
0::/lxc.monitor/example
A process of a service started within that container looks like this:
# cat /proc/3686687/cgroup
11:cpuset:/lxc.payload/example
10:devices:/lxc.payload/example/system.slice/cron.service
9:net_cls,net_prio:/lxc.payload/example
8:blkio:/lxc.payload/example
7:perf_event:/lxc.payload/example
6:rdma:/lxc.payload/example
5:pids:/lxc.payload/example/system.slice/cron.service
4:freezer:/lxc.payload/example
3:cpu,cpuacct:/lxc.payload/example
2:memory:/lxc.payload/example/system.slice/cron.service
1:name=systemd:/lxc.payload/example/system.slice/cron.service
0::/lxc.payload/example/system.slice/cron.service
What would be good to have here is some consistent parsing of the file, that doesn't randomly select ANY of the cgroup names for further processing, but would be looking for a specific one (e.g. the pids namespace). Another bonus could be a new column SYSTEMD SERVICE or SLICE providing the slice a given process was started under (independent of LXC or not). Parsing code would be essentially the same; just another part of the string being used.
Overall the LXC column should provide 3 information:
- The container name (
pidscgroup preferred) - If it's a container monitor (cf. second example dump) ->
[m] - If names of cgroups for this container are consistent, i.e. add
[i]or[!]if any of the cgroups doesn't point to the name shown
P.S.: The init process (pid 60054 above) from inside the container looks like this:
# cat /proc/1/cgroup
11:cpuset:/
10:devices:/init.scope
9:net_cls,net_prio:/
8:blkio:/
7:perf_event:/
6:rdma:/
5:pids:/init.scope
4:freezer:/
3:cpu,cpuacct:/
2:memory:/init.scope
1:name=systemd:/init.scope
0::/init.scope
BenBE
added
enhancement
|
it would be cool to have it as we have now incus. |
|
@jb-boin Would you mind updating this PR to base off the latest commit from NB: Regarding the lengthy CGROUP names from systemd there's a compressed cgroup ( |
Fixed the issues listed on #216.
I feel like there should be a
breakto exit the while loop parsing each line of the CGroup file if the LXC container name has been found and if the CGroup column is not used (or at least skip the LXC part if both columns are used and the LXC name has been found).But it seems that the CGroup files are always parsed even if the CGroup column is not used which is not ideal.