metadata: Fix metadata association problems concerning children processes #1232
Conversation
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
|
I officially declare my dislike of @pre-commit bot. |
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
kakkoyun
force-pushed
the
fix_metadata_assoc
branch
from
fb4f5d8
to
44c7ac7
Compare
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
kakkoyun
force-pushed
the
fix_metadata_assoc
branch
from
44c7ac7
to
df8560b
Compare
|
|
||
| for _, namespace := range namespaces { | ||
| for _, inode := range nsInodes { | ||
| if namespace.Inode == inode && ccg.Path == cg.Path { |
There was a problem hiding this comment.
Nicely done! This should handle the case where containers share the same PID namespace
https://kubernetes.io/docs/tasks/configure-pod-container/share-process-namespace/
scripts/LICENSE.txt
Outdated
| @@ -1,4 +1,4 @@ | |||
| Copyright 2023 The Parca Authors | |||
| Copyright 2022 The Parca Authors | |||
There was a problem hiding this comment.
Yes, let's keep 2022 for now, the check is not flexible, here is the related issue Lucas-C/pre-commit-hooks#23 and someone opened a PR today!
pkg/discovery/discovery_manager.go
Outdated
| @@ -1,4 +1,6 @@ | |||
| // Copyright 2022 The Parca Authors | |||
| // TODO: This license is not consistent with license used in the project. | |||
| // Delete the inconsistent license and above line and rerun pre-commit to insert a good license. | |||
There was a problem hiding this comment.
Never fight a bot, it does not get tired 😆
The TODOs from 2022 → 2023 → 2022 pass would need to be cleaned up, so the check can succeed
The autofix was added recently to assist contributors with style nits, if the majority thinks it does not help, I have no strong opinion for keeping it, although it only seems to require a bit of tuning/fixing here
There was a problem hiding this comment.
I wrote those when I was frustrated with the bot. We can keep it if we tune it.
Let's give it more chance
I don't want them to fight against me when Skynet has risen.
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
There was a problem hiding this comment.
LGTM! Let's check on the demo environment to ensure that it's working as expected.
Two things that I thought of but that can be done later on:
- Was this tested in minikube, too? Just in case the groups v1 env there could bite us.
- Once our plates are less full perhaps we should add tests for `pkg/cgroup/cgroup.go and other of the files that handle this logic.
| for pid := range adjPIDs { | ||
| pids = append(pids, pid) | ||
| } | ||
| sort.Ints(pids) |
There was a problem hiding this comment.
Why do we need to sort them?
The operation only gets triggered when Kubernetes reschedule the pod/container. So in regular operation, it's very infrequent. |
I agree 👍 To note, none of those functions are new. Logically nothing has changed.
The screenshots from the tests that I have conducted on Minikube. |
Fixes #1157
In this PR, to associate the metadata for the container to all processes in the container, we traverse all the PID namespaces (type:
pidandpid_for_children) and group the process by the PID namespace. And then, as a further check, we check if they belong to the same groups (CPU or systemd). You can see the result of the association in the below screenshots.This is a stopgap solution to be able to release a patch. A long-term, cleaner solution will be provided for the main branch. It needs a little more time.
After merging this, we can release a patch version.
Test Output
xref: https://kubernetes.io/docs/tasks/configure-pod-container/share-process-namespace/