Skip to content

fix: cap DNS domain labels per container, add opt-in --min-container-age (B2 of #369) - #371

Merged
blue4209211 merged 4 commits into
mainfrom
port/upstream-b2-cardinality
Oct 8, 2026
Merged

blue4209211 merged 4 commits into
mainfrom
port/upstream-b2-cardinality

Conversation

@mayankpande88

@mayankpande88 mayankpande88 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Second batch of #369: two limits on how many series the agent creates, ported from upstream coroot-node-agent.

Change Upstream What it does
Domain label cap coroot/coroot-node-agent@33c46ec (Nikolay Sivko), re-implemented container_dns_requests_total keeps the first --max-fqdns-per-container (default 50) distinct domains per container. Requests for any later domain are counted under domain="~other".
Minimum container age coroot/coroot-node-agent@34ea61f (Nikolay Sivko), plus the createdAt part of coroot/coroot-node-agent@75d6656 --min-container-age, off by default (0): when set, no series for a container until it has existed that long. Age counts from the earlier of its first process start and when the agent found it, so a restarted service doesn't disappear for 30s after each restart.

Default: upstream ships 30s; this PR ships 0 (off). With 30s, a pod in CrashLoopBackOff at the 5-minute maximum backoff is removed between restarts and comes back as a new container each time. If it dies within 30s it never reports: no OOM kills, memory or CPU. Jobs that finish in under 30s would never appear either. Set it per install where short-lived series are a measured problem.

Engineering detail

Domain cap: why it's re-implemented. Upstream records DNS in its own per-container vector. Here DNS goes through L7Stats.observe with destination and workload labels, so the cap lives in L7Stats and applies to the counter and the histogram alike. L7Stats never deletes series, so without a cap the domain label grows for as long as the container lives. The DNS payload is now parsed once per request instead of twice.

Minimum age: changes from upstream.

  • Upstream holds c.lock for the whole Collect. This fork doesn't, so the age check reads startedAt, zombieAt and createdAt under RLock (youngerThan), and the sweep of exited processes goes through onProcessExit.
  • Upstream treats any taskstats error as a process exit. Here a pid counts as exited only when /proc/<pid> is gone, because a failed netlink call for a live process would otherwise close its uprobes.

Pid reuse (faeabcb, from review): the exited-process sweep removes a pid only while that exact *Process is still registered, so a reused pid can't untrack the new process. CI checks cover it; the e2e below ran before this commit and never reached that path.

Default switched to 0 (d43f186, from review): with default flags, a 20s transient unit that the agent detected had 15 series 8s after starting; with MIN_CONTAINER_AGE=30s it had none.

CI: gofmt, goimports, vet, golangci-lint, go test (excluding /containers) and the build all pass in a Linux container with Go 1.26.5.

Found during the e2e, not changed here. On every build, main included, a burst of new systemd units started within about a minute of the agent starting can go undetected: 1–2 of 10 per burst. A unit that does nothing after it starts then never appears. On warm agents this happened once in 60 starts. In a later run, 2 of 3 transient systemd-run units went undetected on warm agents. I'll follow it up separately.

Local e2e: I built agent binaries from this branch and from #370 and ran both side by side as systemd services on a local Debian 12 VM (kernel 6.1, systemd 252). This branch ran with MAX_FQDNS_PER_CONTAINER=5.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request introduces mechanisms to reduce metric cardinality and handle missed process exit events. It adds a --min-container-age flag to suppress metrics for short-lived containers and a --max-fqdns-per-container flag to bucket excess unique FQDNs under a ~other label. Additionally, it updates updateDelays to detect and clean up dead processes. The review feedback highlights a critical race condition where recycled PIDs could lead to the incorrect termination of newly started processes. To resolve this, the reviewer suggests returning *Process pointers from updateDelays and implementing a safe onProcessExitIf helper to verify the process instance before cleanup.

Comment thread containers/container.go
Comment thread containers/container.go
Base automatically changed from port/upstream-b1-push-lifecycle to main October 7, 2026 18:21
@blue4209211

Copy link
Copy Markdown
Contributor

Review

The code is sound, but I'd hold this on one product question before merge.

Before merge: rebase onto main. #370 was squash-merged, so the PR shows 17 files; its own change is only containers/container.go, containers/l7.go and flags/flags.go.

--min-container-age default of 30s can hide crash-looping pods

  • A Kubernetes container keeps its ID across restarts within a pod (/k8s/<ns>/<pod>/<container>), and the age counts from first discovery, so an early crash loop reports after 30s, including OOM kills. 👍
  • But a container that has been a zombie longer than gcInterval (5 min) is deleted from the registry, and CrashLoopBackOff's maximum backoff is 5 min. Once a pod reaches that backoff, each restart can create a fresh Container with a new createdAt. If it crashes within 30s each time, it never reports again: no container_oom_kills_total, memory or CPU for exactly the pods we investigate.
  • Every new pod is also invisible for its first 30s, and Jobs or CronJobs that finish in under 30s never appear. That's the stated trade-off, but those failures are in scope for us.
  • The e2e covered systemd units on a VM, not a Kubernetes crash loop.

Options:

  • (a) default 0 for our installs and let customers opt in;
  • (b) exempt containers that have had an OOM kill or a process exit/restart;
  • (c) keep the discovery time across the 5-min GC, e.g. remembered per ContainerID.

This is a product call. I'd lean towards (a) or (b).

Checked and fine

  • No deadlock: dnsDomainLabel takes s.mu, and observe doesn't hold it at that point.
  • Dead-process sweep: a pid counts as exited only when /proc/<pid> is gone, and onProcessExitIf can't untrack a reused pid.
  • DNS parsing: the DNS payload is parsed once per request, and the cap applies to the counter and the histogram alike.

Low: the domain cap keeps the first 50 domains forever, so a container that resolves many names at startup pins those, and later domains go to ~other. This matches upstream and is fine at the default.

mayankpande88 and others added 3 commits October 8, 2026 09:21
Port of coroot/coroot-node-agent@33c46ec ("cap unique FQDN labels per
container in container_dns_requests_total"). After the first
--max-fqdns-per-container (default 50) distinct domains a container
resolves, new domains are counted under domain="~other".

Upstream keeps DNS metrics in a separate per-container vector. Here DNS
is recorded through L7Stats.observe with destination and workload labels,
so the cap lives in L7Stats and applies to the counter and the histogram
alike. L7Stats never deletes series, so without a cap the domain label
grows for the container's lifetime.
(cherry picked from commit 34ea61f99a4e5acce57f582665adc87cafcb1083)

Includes the createdAt half of coroot/coroot-node-agent@75d6656 ("fix:
systemd services disappearing after a restart"), which only applies to
this flag: age counts from the earlier of the first process start and
container discovery, so a restarted unit is not hidden for the minimum
age after each restart.

Fork adaptations: Collect here does not hold c.lock, so the dead-pid
sweep goes through onProcessExit and the age check reads its fields
under RLock (youngerThan). A pid counts as dead only when /proc/<pid> is
gone; upstream treats any taskstats error as an exit, which here would
close a live process's uprobes.

The default is upstream's 30s: containers that live less than 30s,
such as short jobs, produce no series.
The sweep in Collect finds exited processes without holding c.lock. If
the pid was reused and the new process registered before the exit is
handled, onProcessExit would untrack the new process. updateDelays now
returns the *Process it found gone, and onProcessExitIf handles the
exit only while that same process is registered.
@mayankpande88
mayankpande88 force-pushed the port/upstream-b2-cardinality branch from faeabcb to 9b45e84 Compare October 8, 2026 03:51
With 30s, a pod in CrashLoopBackOff at the 5-minute maximum backoff
stays a zombie past gcInterval, is removed between restarts and comes
back as a new container each time. If it dies within 30s it never
reports: no OOM kills, memory or CPU for the pods most worth looking
at. Jobs that finish in under 30s never appear either. Keep the flag,
and enable it per install where short-lived series are a measured
problem.
@mayankpande88 mayankpande88 changed the title fix: cap DNS domain labels and skip short-lived containers (B2 of #369) fix: cap DNS domain labels per container, add opt-in --min-container-age (B2 of #369) Oct 8, 2026
@mayankpande88

Copy link
Copy Markdown
Contributor Author

Thanks, the crash-loop case is real.

  • Rebased onto main. The diff is now the 3 files.
  • Went with (a): --min-container-age now defaults to 0 (d43f186). The flag and the code stay, so an install can turn it on where short-lived series are a measured problem.
  • Exited-process sweep: it doesn't depend on the flag and still runs.
  • Checked on a VM with default flags: a 20s unit the agent detected had series 8s after starting. With MIN_CONTAINER_AGE=30s it had none.

@blue4209211
blue4209211 merged commit 89cf33a into main Oct 8, 2026
7 checks passed
@blue4209211
blue4209211 deleted the port/upstream-b2-cardinality branch October 8, 2026 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants