[CWS] cut allocation in `GetProcContainerContext` #32183

paulcacheux · 2024-12-15T17:52:55Z

What does this PR do?

The function GetProcContainerContext is called a lot, and each code results in a read of the proc cgroup file. This PR improves this situation a bit by reading only the last cgroup (which is the one we use for proc context right now), and cuts most of split related allocations.

Example profile showing why it's important

Motivation

Describe how you validated your changes

Possible Drawbacks / Trade-offs

Additional Notes

agent-platform-auto-pr · 2024-12-15T18:47:51Z

Uncompressed package size comparison

Comparison with ancestor 52f0517517169d661a94895a296c73b85424c86d

Diff per package

package	diff	status	size	ancestor	threshold
datadog-dogstatsd-amd64-deb	0.00MB	✅	78.59MB	78.59MB	10.00MB
datadog-dogstatsd-x86_64-rpm	0.00MB	✅	78.67MB	78.67MB	10.00MB
datadog-dogstatsd-x86_64-suse	0.00MB	✅	78.67MB	78.67MB	10.00MB
datadog-dogstatsd-arm64-deb	0.00MB	✅	55.79MB	55.79MB	10.00MB
datadog-heroku-agent-amd64-deb	0.00MB	✅	505.06MB	505.06MB	70.00MB
datadog-iot-agent-amd64-deb	0.00MB	✅	113.31MB	113.31MB	10.00MB
datadog-iot-agent-x86_64-rpm	0.00MB	✅	113.38MB	113.38MB	10.00MB
datadog-iot-agent-x86_64-suse	0.00MB	✅	113.38MB	113.38MB	10.00MB
datadog-iot-agent-arm64-deb	0.00MB	✅	108.78MB	108.78MB	10.00MB
datadog-iot-agent-aarch64-rpm	0.00MB	✅	108.85MB	108.85MB	10.00MB
datadog-agent-x86_64-rpm	-0.00MB	✅	1197.00MB	1197.00MB	140.00MB
datadog-agent-x86_64-suse	-0.00MB	✅	1197.00MB	1197.00MB	140.00MB
datadog-agent-amd64-deb	-0.00MB	✅	1187.76MB	1187.76MB	140.00MB
datadog-agent-aarch64-rpm	-0.00MB	✅	942.99MB	943.00MB	140.00MB
datadog-agent-arm64-deb	-0.01MB	✅	933.78MB	933.78MB	140.00MB

Decision

✅ Passed

agent-platform-auto-pr · 2024-12-15T19:07:58Z

Test changes on VM

Use this command from test-infra-definitions to manually test this PR changes on a VM:

inv aws.create-vm --pipeline-id=51483519 --os-family=ubuntu

Note: This applies to commit a78bc45

cit-pr-commenter · 2024-12-15T19:12:47Z

Regression Detector

Regression Detector Results

Metrics dashboard
Target profiles
Run ID: 86179cfa-6709-4782-b0a5-4b51eadf0d44

Baseline: 52f0517
Comparison: a78bc45
Diff

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI	trials	links
➖	file_to_blackhole_1000ms_latency	egress throughput	+0.18	[-0.60, +0.95]	1	Logs
➖	file_to_blackhole_500ms_latency	egress throughput	+0.08	[-0.69, +0.86]	1	Logs
➖	file_to_blackhole_1000ms_latency_linear_load	egress throughput	+0.08	[-0.39, +0.55]	1	Logs
➖	file_to_blackhole_300ms_latency	egress throughput	+0.02	[-0.61, +0.65]	1	Logs
➖	file_to_blackhole_100ms_latency	egress throughput	+0.01	[-0.71, +0.73]	1	Logs
➖	uds_dogstatsd_to_api	ingress throughput	+0.01	[-0.09, +0.11]	1	Logs
➖	tcp_dd_logs_filter_exclude	ingress throughput	+0.00	[-0.01, +0.01]	1	Logs
➖	file_to_blackhole_0ms_latency	egress throughput	-0.02	[-0.87, +0.83]	1	Logs
➖	file_to_blackhole_0ms_latency_http2	egress throughput	-0.04	[-0.97, +0.89]	1	Logs
➖	file_to_blackhole_0ms_latency_http1	egress throughput	-0.04	[-0.88, +0.80]	1	Logs
➖	quality_gate_logs	% cpu utilization	-0.08	[-3.04, +2.88]	1	Logs
➖	tcp_syslog_to_blackhole	ingress throughput	-0.35	[-0.41, -0.29]	1	Logs
➖	quality_gate_idle	memory utilization	-0.70	[-0.75, -0.66]	1	Logs bounds checks dashboard
➖	file_tree	memory utilization	-0.71	[-0.84, -0.57]	1	Logs
➖	quality_gate_idle_all_features	memory utilization	-1.00	[-1.13, -0.87]	1	Logs bounds checks dashboard
➖	otel_to_otel_logs	ingress throughput	-1.63	[-2.33, -0.92]	1	Logs
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	-1.76	[-2.46, -1.05]	1	Logs

Bounds Checks: ❌ Failed

perf	experiment	bounds_check_name	replicates_passed	links
❌	file_to_blackhole_100ms_latency	lost_bytes	9/10
✅	file_to_blackhole_0ms_latency	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency	memory_usage	10/10
✅	file_to_blackhole_0ms_latency_http1	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency_http1	memory_usage	10/10
✅	file_to_blackhole_0ms_latency_http2	lost_bytes	10/10
✅	file_to_blackhole_0ms_latency_http2	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency	memory_usage	10/10
✅	file_to_blackhole_1000ms_latency_linear_load	memory_usage	10/10
✅	file_to_blackhole_100ms_latency	memory_usage	10/10
✅	file_to_blackhole_300ms_latency	lost_bytes	10/10
✅	file_to_blackhole_300ms_latency	memory_usage	10/10
✅	file_to_blackhole_500ms_latency	lost_bytes	10/10
✅	file_to_blackhole_500ms_latency	memory_usage	10/10
✅	quality_gate_idle	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_idle_all_features	memory_usage	10/10	bounds checks dashboard
✅	quality_gate_logs	lost_bytes	10/10
✅	quality_gate_logs	memory_usage	10/10

Explanation

Confidence level: 90.00%
Effect size tolerance: |Δ mean %| ≥ 5.00%

Performance changes are noted in the perf column of each table:

✅ = significantly better comparison variant performance
❌ = significantly worse comparison variant performance
➖ = no significant change in performance

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.
Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.
Its configuration does not mark it "erratic".

CI Pass/Fail Decision

✅ Passed. All Quality Gates passed.

quality_gate_idle_all_features, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_idle, bounds check memory_usage: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check lost_bytes: 10/10 replicas passed. Gate passed.
quality_gate_logs, bounds check memory_usage: 10/10 replicas passed. Gate passed.

spikat

Some nit/ideas comments but LGTM :) thanks

spikat · 2024-12-18T16:11:09Z

pkg/security/utils/cgroup.go

+	if index < 0 {
+		index = 0
+	}
+	firstLine := string(data[index:])


Should we name this var "lastLine" instead ?

ah yeah sorry..

spikat · 2024-12-18T16:16:43Z

pkg/security/utils/cgroup.go

@@ -85,15 +123,14 @@ func GetProcContainerID(tgid, pid uint32) (containerutils.ContainerID, error) {
 // GetProcContainerContext returns the container ID which the process belongs to along with its manager. Returns "" if the process does not belong
 // to a container.
 func GetProcContainerContext(tgid, pid uint32) (containerutils.ContainerID, model.CGroupContext, error) {
-	cgroups, err := GetProcControlGroups(tgid, pid)
-	if err != nil || len(cgroups) == 0 {
+	cgroup, err := GetLastProcControlGroups(tgid, pid)


It's not directly related to your PR, but I wonder if we should add a check and log in case the cgroup.ID != 0. It "should" not happen but .. We never knows! WDYT ?

definitely, I wan't this PR to be "perf improvement" but no functionality change, but I have another PR coming that parses specifically for the 0:: cgroup

paulcacheux · 2024-12-18T19:09:56Z

/merge

dd-devflow · 2024-12-18T19:10:12Z

Devflow running: `/merge`

View all feedbacks in Devflow UI.

2024-12-18 19:10:11 UTC ℹ️ MergeQueue: waiting for PR to be ready

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals.
Note: if you pushed new commits since the last approval, you may need additional approval.
You can remove it from the waiting list with /remove command.

2024-12-18 19:15:22 UTC ℹ️ MergeQueue: merge request added to the queue

The median merge time in main is 32m.

2024-12-18 19:35:52 UTC ℹ️ MergeQueue: This merge request was merged

github-actions bot added component/system-probe team/agent-security short review PR is simple enough to be reviewed quickly labels Dec 15, 2024

paulcacheux force-pushed the paulcacheux/get-first-cgroup branch from 58212f4 to 33a6c46 Compare December 15, 2024 18:18

paulcacheux added qa/done QA done before merge and regressions are covered by tests changelog/no-changelog labels Dec 15, 2024

paulcacheux marked this pull request as ready for review December 16, 2024 08:07

paulcacheux requested a review from a team as a code owner December 16, 2024 08:07

[CWS] cut allocation in GetProcContainerContext

9bc6714

paulcacheux force-pushed the paulcacheux/get-first-cgroup branch from 33a6c46 to 9bc6714 Compare December 18, 2024 15:32

spikat approved these changes Dec 18, 2024

View reviewed changes

github-actions bot added medium review PR review might take time and removed short review PR is simple enough to be reviewed quickly labels Dec 18, 2024

fix index issue and apply review suggestion

a78bc45

paulcacheux force-pushed the paulcacheux/get-first-cgroup branch from 6cc3f10 to a78bc45 Compare December 18, 2024 18:20

dd-mergequeue bot merged commit a2eda15 into main Dec 18, 2024
231 of 233 checks passed

dd-mergequeue bot deleted the paulcacheux/get-first-cgroup branch December 18, 2024 19:35

github-actions bot added this to the 7.62.0 milestone Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CWS] cut allocation in `GetProcContainerContext` #32183

[CWS] cut allocation in `GetProcContainerContext` #32183

paulcacheux commented Dec 15, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 15, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 15, 2024 •

edited

Loading

cit-pr-commenter bot commented Dec 15, 2024 •

edited

Loading

Fine details of change detection per experiment

Explanation

spikat left a comment

spikat Dec 18, 2024

paulcacheux Dec 18, 2024

spikat Dec 18, 2024

paulcacheux Dec 18, 2024

paulcacheux commented Dec 18, 2024

dd-devflow bot commented Dec 18, 2024 •

edited

Loading

[CWS] cut allocation in GetProcContainerContext #32183

[CWS] cut allocation in GetProcContainerContext #32183

Conversation

paulcacheux commented Dec 15, 2024 • edited Loading

What does this PR do?

Motivation

Describe how you validated your changes

Possible Drawbacks / Trade-offs

Additional Notes

agent-platform-auto-pr bot commented Dec 15, 2024 • edited Loading

Uncompressed package size comparison

Decision

agent-platform-auto-pr bot commented Dec 15, 2024 • edited Loading

Test changes on VM

cit-pr-commenter bot commented Dec 15, 2024 • edited Loading

Regression Detector

Regression Detector Results

Optimization Goals: ✅ No significant changes detected

Fine details of change detection per experiment

Bounds Checks: ❌ Failed

Explanation

CI Pass/Fail Decision

spikat left a comment

Choose a reason for hiding this comment

spikat Dec 18, 2024

Choose a reason for hiding this comment

paulcacheux Dec 18, 2024

Choose a reason for hiding this comment

spikat Dec 18, 2024

Choose a reason for hiding this comment

paulcacheux Dec 18, 2024

Choose a reason for hiding this comment

paulcacheux commented Dec 18, 2024

dd-devflow bot commented Dec 18, 2024 • edited Loading

Devflow running: /merge

[CWS] cut allocation in `GetProcContainerContext` #32183

[CWS] cut allocation in `GetProcContainerContext` #32183

paulcacheux commented Dec 15, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 15, 2024 •

edited

Loading

agent-platform-auto-pr bot commented Dec 15, 2024 •

edited

Loading

cit-pr-commenter bot commented Dec 15, 2024 •

edited

Loading

dd-devflow bot commented Dec 18, 2024 •

edited

Loading

Devflow running: `/merge`