-
Notifications
You must be signed in to change notification settings - Fork 425
reporter: extract container.ID from cgroupv2 path #548
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
Closed
Closed
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
49426af
reporter: extract container.ID from cgroupv2 path
florianl 7d0c039
fixup: add small LRU to cache regex result
florianl aa21cdb
fixup: merge reporter caches into a single cache
florianl 877544e
fixup: function comment
florianl daa9dc6
fixup: update regex
florianl 55426a6
fixup: simplify test
florianl 7d42302
fixup: just log instead of return
florianl File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,13 +29,13 @@ type CollectorReporter struct { | |
|
|
||
| // NewCollector builds a new CollectorReporter | ||
| func NewCollector(cfg *Config, nextConsumer xconsumer.Profiles) (*CollectorReporter, error) { | ||
| cgroupv2ID, err := lru.NewSynced[libpf.PID, string](cfg.CGroupCacheElements, | ||
| pidToContainerID, err := lru.NewSynced[libpf.PID, string](cfg.PIDToContainerIDCacheElements, | ||
| func(pid libpf.PID) uint32 { return uint32(pid) }) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| // Set a lifetime to reduce the risk of invalid data in case of PID reuse. | ||
| cgroupv2ID.SetLifetime(90 * time.Second) | ||
| pidToContainerID.SetLifetime(90 * time.Second) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for this PR, but we can and should do better here (e.g. |
||
|
|
||
| // Next step: Dynamically configure the size of this LRU. | ||
| // Currently, we use the length of the JSON array in | ||
|
|
@@ -59,13 +59,13 @@ func NewCollector(cfg *Config, nextConsumer xconsumer.Profiles) (*CollectorRepor | |
|
|
||
| return &CollectorReporter{ | ||
| baseReporter: &baseReporter{ | ||
| cfg: cfg, | ||
| name: cfg.Name, | ||
| version: cfg.Version, | ||
| pdata: data, | ||
| cgroupv2ID: cgroupv2ID, | ||
| traceEvents: xsync.NewRWMutex(tree), | ||
| hostmetadata: hostmetadata, | ||
| cfg: cfg, | ||
| name: cfg.Name, | ||
| version: cfg.Version, | ||
| pdata: data, | ||
| pidToContainerID: pidToContainerID, | ||
| traceEvents: xsync.NewRWMutex(tree), | ||
| hostmetadata: hostmetadata, | ||
| runLoop: &runLoop{ | ||
| stopSignal: make(chan libpf.Void), | ||
| }, | ||
|
|
@@ -85,7 +85,7 @@ func (r *CollectorReporter) Start(ctx context.Context) error { | |
| }, func() { | ||
| // Allow the GC to purge expired entries to avoid memory leaks. | ||
| r.pdata.Purge() | ||
| r.cgroupv2ID.PurgeExpired() | ||
| r.pidToContainerID.PurgeExpired() | ||
| }) | ||
|
|
||
| // When Stop() is called and a signal to 'stop' is received, then: | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| // Copyright The OpenTelemetry Authors | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package reporter // import "go.opentelemetry.io/ebpf-profiler/reporter" | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| //nolint:lll | ||
| func TestExtractContainerID(t *testing.T) { | ||
| tests := []struct { | ||
| line string | ||
| expectedContainerID string | ||
| }{ | ||
| { | ||
| line: "0::/kubepods.slice/kubepods-burstable.slice/kubepods-burstable-podf6f2d169_f2ae_4afa-95ed_06ff2ed6b288.slice/cri-containerd-b4d6d161c62525d726fa394b27df30e14f8ea5646313ada576b390de70cfc8cc.scope", | ||
| expectedContainerID: "b4d6d161c62525d726fa394b27df30e14f8ea5646313ada576b390de70cfc8cc", | ||
| }, | ||
| { | ||
| line: "0::/kubepods/besteffort/pod05e102bf-8744-4942-a241-9b6f07983a53/f52a212505a606972cf8614c3cb856539e71b77ecae33436c5ac442232fbacf8", | ||
| expectedContainerID: "f52a212505a606972cf8614c3cb856539e71b77ecae33436c5ac442232fbacf8", | ||
| }, | ||
| { | ||
| line: "0::/kubepods/besteffort/pod897277d4-5e6f-4999-a976-b8340e8d075e/crio-a4d6b686848a610472a2eed3ae20d4d64b6b4819feb9fdfc7fd7854deaf59ef3", | ||
| expectedContainerID: "a4d6b686848a610472a2eed3ae20d4d64b6b4819feb9fdfc7fd7854deaf59ef3", | ||
| }, | ||
| { | ||
| line: "0::/kubepods.slice/kubepods-besteffort.slice/kubepods-besteffort-pod4c9f1974_5c46_44c2_b42f_3bbf0e98eef9.slice/cri-containerd-bacb920470900725e0aa7d914fee5eb0854315448b024b6b8420ad8429c607ba.scope", | ||
| expectedContainerID: "bacb920470900725e0aa7d914fee5eb0854315448b024b6b8420ad8429c607ba", | ||
| }, | ||
| { | ||
| line: "0::/user.slice/user-1000.slice/user@1000.service/app.slice/app-org.gnome.Terminal.slice/vte-spawn-868f9513-eee8-457d-8e36-1b37ae8ae622.scope", | ||
| }, | ||
| { | ||
| line: "0::/../../user.slice/user-501.slice/session-3.scope", | ||
| }, | ||
| { | ||
| line: "0::/system.slice/docker-b1eba9dfaeba29d8b80532a574a03ea3cac29384327f339c26da13649e2120df.scope/init", | ||
| expectedContainerID: "b1eba9dfaeba29d8b80532a574a03ea3cac29384327f339c26da13649e2120df", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| tc := tc | ||
| t.Run(tc.expectedContainerID, func(t *testing.T) { | ||
| reader := bytes.NewReader([]byte(tc.line)) | ||
|
|
||
| gotContainerID := extractContainerID(reader) | ||
| assert.Equal(t, tc.expectedContainerID, gotContainerID) | ||
| }) | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we're losing traces on
lookupContainerIDerrors (e.g. if a PID dies). If we continue we'd be reporting traces without container ID.Thinking about this some more, it might be better to get rid of this LRU completely and fetch the container ID when we first parse a PID in updatePIDInformation.
We'd then store the container ID inside ProcessMeta in the process registry
ProcessManager.pidToProcessInfo. The wins are two-fold:Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this earlier #548 (comment), that changes to the API of reporter to bring it closer to ProcessManager, should be a separate change.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My earlier comment would only replace a small part of this PR. With my current comment, I propose replacing this entire PR with something else (we'd keep the
containerIDextraction logic but nothing else). If you agree that this is the way to go, then it's faster to just do that instead of merging this PR and then creating another PR that will remove most of it.Also I don't propose any changes to the reporter API, we just need to add an extra field to
samples.TraceEventMeta.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened #577, but didn't have time to test. If everything is OK, you can cherry-pick the last commit into this PR (or close this PR and review #577).
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think, #577 should be the way forward. Profiling capabilities should not be mixed with such functionality.
This PR introduces a basic support for container IDs. But there are use cases, where pod ID, cgroupv1 data or other information is relevant to the reporter.
Therefore, I have the following suggestion:
To me processmanager should focus on properly handling processes rather than extracting every possible process data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that
processmanagershould have more plugin/hook based system to collect the meta data.After 15min brief look, I would prefer #577 as first step. With the following rationale:
reporterindicates it just reports things, this is making it collect data tooreportermight be too late to collect the container ID in some short lived process cases, where asprocessmanagerwould be more suitable to do itreporterneeds extra LRUs with memory overhead (and CPU for cleaning it?);processmanagercan stash the information in existing data structuresprocessmanageralready collects and caches process meta dataAdditionally the proposal is to add
processmanagerhooks this indicates that the natural place to collect this information is in context ofprocessmanager.To me just collecting the metadata in
processmanagermakes perfect sense. It would be later step to convert it to more plugin based approach. But even then theprocessmanagershould contain the data in attributes or similar. The reasoning is same as #384 for symbol data. The data collection should happen early.Processmanager is responsible for the process metadata also, and this is such data. #384 proposes to make processmanager also responsible for the symbols. I think
processmanageris the central piece for everything. I suppose the fear is that it becomes too monolithic? So agreeably it needs to be more plugin based in the future.But I think also that containerID is pretty commonly wanted thing - it is a pretty fundamental piece of process metadata. I would not try to abstract it away.
So I propose the
processmanagerrole should be extended to include this piece of the process metadata.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @fabled, I think there's no good reason to go with an LRU now that I've written the code in #577, it's a semantically wrong solution, open to race conditions and less performant (it keeps expiring and extracting container IDs for each PID). In addition, we already extract race-sensitive process metadata in
ProcessManagerso it's ugly from a consistency POV to split this logic and do that in the reporter too.