Conversation
Add a docker integration test for the cAdvisor exporter. The test scrapes a single prometheus.exporter.cadvisor instance into Mimir and asserts that cAdvisor metrics appear. cAdvisor uses its raw cgroup driver, so the test needs a privileged container but no container runtime. This POC includes a temporary discovery test. It lists every cAdvisor metric that reaches Mimir so the assertion list can be pinned from a real Linux run. The discovery test fails on purpose to surface its output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the temporary discovery test with the real assertion list, pinned from a CI run. The list covers every cAdvisor collector family: build and version, cpu, memory, filesystem, network, and blkio. Leave out container_pressure_* (needs kernel CONFIG_PSI) and container_health_state (needs a Docker HEALTHCHECK on the target container), since neither is guaranteed on every host. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…afana/alloy into poc/cadvisor-integration-test
… blocked pod start A read-only host /var/run mount stops Kubernetes from mounting the service account token, so the Alloy pod failed with RunContainerError. The raw cgroup driver does not need /var/run. Only the docker and containerd plugins use it, and this test does not use them. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cAdvisor v0.60 split its library packages into a new github.com/google/cadvisor/lib submodule. Its in-repo local replace is ignored from a dependency, so add a second require and replace pair that redirect /lib to the Grafana fork. Rewrite the integration imports to the /lib paths; container/docker and info/v2 stay in the root module. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🔍 Dependency ReviewBelow are the dependency changes detected in go.mod files, with required code updates (if any), supporting evidence, and minimal example diffs to adopt the upgrades. github.com/google/cadvisor v0.54.1 -> v0.60.5 — ❌ Changes NeededSummary of required changes:
Minimal code changes (already reflected in this PR):
- "github.com/google/cadvisor/cache/memory"
- "github.com/google/cadvisor/container"
- "github.com/google/cadvisor/manager"
- "github.com/google/cadvisor/metrics"
- "github.com/google/cadvisor/storage"
- "github.com/google/cadvisor/utils/sysfs"
+ "github.com/google/cadvisor/lib/cache/memory"
+ "github.com/google/cadvisor/lib/container"
+ "github.com/google/cadvisor/lib/fs"
+ "github.com/google/cadvisor/lib/manager"
+ "github.com/google/cadvisor/lib/metrics"
+ "github.com/google/cadvisor/lib/stats"
+ "github.com/google/cadvisor/lib/storage"
+ "github.com/google/cadvisor/lib/utils/sysfs"
- "github.com/google/cadvisor/container/containerd"
- "github.com/google/cadvisor/container/crio"
- "github.com/google/cadvisor/container/raw"
- "github.com/google/cadvisor/container/systemd"
+ "github.com/google/cadvisor/lib/container/containerd"
+ "github.com/google/cadvisor/lib/container/crio"
+ "github.com/google/cadvisor/lib/container/raw"
+ "github.com/google/cadvisor/lib/container/systemd"
// still:
"github.com/google/cadvisor/container/docker"
+ // Filesystem plugins now selected per instance.
+ fsPlugins := map[string]fs.FsPlugin{
+ "btrfs": btrfs.NewPlugin(),
+ "devicemapper": devicemapper.NewPlugin(),
+ "nfs": nfs.NewPlugin(),
+ "overlay": overlay.NewPlugin(),
+ "tmpfs": tmpfs.NewPlugin(),
+ "vfs": vfs.NewPlugin(),
+ "zfs": zfs.NewPlugin(),
+ }
+ manager.PerfManagerFactory = perf.NewManager
+ manager.ResctrlManagerFactory = func(interval time.Duration, vendorID string, inHostNamespace bool) (stats.ResctrlManager, error) {
+ return intel.NewManager(interval, intel.Setup, vendorID, inHostNamespace, c.DockerOnly)
+ }
+ manager.CollectorManagerFactory = func(handler container.ContainerHandler, readFile func(string) ([]byte, error), httpClient *http.Client) (manager.CollectorManager, error) {
+ return appmetrics.NewManager(handler, readFile, httpClient, manager.ApplicationMetricsCountLimit())
+ }
- rm, err := manager.New(plugins, memoryStorage, sysFs, manager.HousekeepingConfigFlags, includedMetrics, &collectorHTTPClient, c.RawCgroupPrefixAllowlist, c.EnvMetadataAllowlist, c.PerfEventsConfig, time.Duration(c.ResctrlInterval), rawOpts)
+ rm, err := manager.New(plugins, fsPlugins, memoryStorage, sysFs, manager.HousekeepingConfigFlags, includedMetrics, &collectorHTTPClient, c.RawCgroupPrefixAllowlist, c.EnvMetadataAllowlist, c.PerfEventsConfig, time.Duration(c.ResctrlInterval), rawOpts)
- // Two families are left out on purpose:
- // - container_pressure_* (PSI) needs kernel CONFIG_PSI. Not every host has it.
- // - container_health_state needs a container with a Docker HEALTHCHECK. It
- // depends on the sibling workloads, not the exporter.
+ // container_health_state is left out on purpose: it needs a container with a
+ // Docker HEALTHCHECK, which kind's containerd runtime does not provide.
+ // Pressure stall (PSI) metrics...
+ "container_pressure_cpu_stalled_seconds_total",
+ "container_pressure_cpu_waiting_seconds_total",
+ "container_pressure_io_stalled_seconds_total",
+ "container_pressure_io_waiting_seconds_total",
+ "container_pressure_memory_stalled_seconds_total",
+ "container_pressure_memory_waiting_seconds_total",
+ // Metrics added across the 0.58–0.60 line (cgroup v2 memory events/pages, CPU
+ // burst accounting, and container start time).
+ "container_cpu_cfs_burst_periods_total",
+ "container_cpu_cfs_burst_seconds_total",
+ "container_memory_events_high_total",
+ "container_memory_events_max_total",
+ "container_memory_pgscan_total",
+ "container_memory_pgsteal_total",
+ "container_memory_workingset_refault_anon_total",
+ "container_memory_workingset_refault_file_total",
+ "container_start_time_seconds",Notes about the new submodule:
Evidence:
github.com/containerd/containerd/api v1.9.0 -> v1.10.0 — ✅ Safe
Evidence:
github.com/containerd/ttrpc v1.2.7 -> v1.2.9 — ✅ Safe
Evidence:
github.com/opencontainers/cgroups v0.0.4 -> v0.0.6 — ✅ Safe
Evidence:
github.com/opencontainers/selinux v1.13.0 -> v1.13.1 — ✅ Safe
Evidence:
github.com/opencontainers/runc (replace) v1.2.8 -> v1.3.6 —
|
cAdvisor v0.60 (grafana-v0.60.5-noglobals fork) selects filesystem plugins per instance instead of from a process-global registry. Build the default plugin set (btrfs, devicemapper, nfs, overlay, tmpfs, vfs, zfs) and pass it to manager.New, the same way the container plugins are passed. Without this the manager has no filesystem plugins and the container_fs_* metrics are empty. Bump the fork pseudo-version to the commit that adds this API. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The grafana-v0.60.5-noglobals fork now selects the resctrl manager per instance through manager.ResctrlManagerFactory instead of a global registry. The manager.New signature is unchanged and Alloy disables resctrl metrics by default, so this is a pseudo-version bump only. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cAdvisor v0.60 (grafana-v0.60.5-noglobals fork) turns the perf_event, resctrl, and application-metrics collectors into per-instance injection seams that the lean library leaves nil. Set PerfManagerFactory, ResctrlManagerFactory, and CollectorManagerFactory in the integration so these keep working as they did before the noglobals split. Bump the fork pseudo-version to the commit that makes appmetrics importable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The runc replace held v1.2.8, which govulncheck flags. runc cannot float to latest: cAdvisor (resctrl/intel) and other dependencies still import runc's libcontainer packages (intelrdt, user) that v1.4+ removed. v1.3.6 keeps those packages and is not flagged, so pin the replace to v1.3.6. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extend the pinned metric list with pressure stall (PSI) metrics and the metrics cAdvisor v0.60 adds: cgroup v2 memory events and stats (pgscan/pgsteal, workingset refault, memory events), CPU burst accounting, and container start time. All are emitted on the CI runner's cgroup v2 kernel. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
Address the factory-global race, portability issues in Kubernetes metric assertions, and stale vulnerability-ignore rationale.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates cAdvisor from v0.54.1 to v0.60.5 and adapts the integration to its split /lib module layout.
Changes:
- Rewrites cAdvisor imports and integration wiring.
- Updates dependency replacements, versions, and checksums.
- Expands Kubernetes integration metric assertions.
File summaries
| File | Reviewed changes and findings |
|---|---|
internal/static/integrations/cadvisor/cadvisor.go |
Critical (1 vote): Package-level factory globals can race during reloads or multiple exporters and overwrite instance-specific state. |
integration-tests/k8s/tests/prometheus-exporter-cadvisor/k8s_test.go |
Moderate (3 votes): PSI metrics need capability gating. Moderate (1 vote): cgroup-v2 metrics need feature-aware assertions. Moderate (2 votes): Burst metrics require a CPU-limited fixture. |
go.sum |
Refreshes dependency checksums. |
go.mod |
Updates dependencies. Moderate (1 vote): Stale Docker vulnerability ignores should be updated or removed. |
collector/go.sum |
Refreshes collector dependency checksums. |
collector/go.mod |
Regenerates collector dependencies. |
collector/builder-config.yaml |
Adds cAdvisor fork module replacements. |
Review details
Suppressed comments (2)
go.mod:60
- This bump removes the legacy github.com/docker/docker requirement, but .govulncheck.yaml still has five ignores whose reasons say cAdvisor is the remaining importer and that they should be revisited when cAdvisor drops that dependency. Please update or remove those entries with this change; otherwise the vulnerability ignore policy remains stale and can suppress findings under an obsolete rationale.
github.com/google/cadvisor v0.60.5
integration-tests/k8s/tests/prometheus-exporter-cadvisor/k8s_test.go:96
- These cgroup-v2-dependent memory metrics are not guaranteed by the test setup: the manifest only mounts the host cgroup filesystem and does not require a cgroup-v2 node. On a cgroup-v1 kind/local node cAdvisor legitimately omits these series, so QueryMetrics will time out; assert only feature-independent metrics or enforce/skip based on the cgroup version.
"container_memory_events_high_total",
"container_memory_events_max_total",
"container_memory_pgscan_total",
"container_memory_pgsteal_total",
"container_memory_workingset_refault_anon_total",
- Files reviewed: 5/7 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| manager.PerfManagerFactory = perf.NewManager | ||
| manager.ResctrlManagerFactory = func(interval time.Duration, vendorID string, inHostNamespace bool) (stats.ResctrlManager, error) { | ||
| return intel.NewManager(interval, intel.Setup, vendorID, inHostNamespace, c.DockerOnly) | ||
| } | ||
| manager.CollectorManagerFactory = func(handler container.ContainerHandler, readFile func(string) ([]byte, error), httpClient *http.Client) (manager.CollectorManager, error) { |
| // Pressure stall (PSI) metrics. Present on the CI runner kernel. | ||
| "container_pressure_cpu_stalled_seconds_total", | ||
| "container_pressure_cpu_waiting_seconds_total", | ||
| "container_pressure_io_stalled_seconds_total", | ||
| "container_pressure_io_waiting_seconds_total", | ||
| "container_pressure_memory_stalled_seconds_total", | ||
| "container_pressure_memory_waiting_seconds_total", |
| "container_cpu_cfs_burst_periods_total", | ||
| "container_cpu_cfs_burst_seconds_total", |
|
@dehaansa I think there are some govulncheck exceptions waiting for this upgrade. Do you want to add them to this PR or later? |
I briefly looked into it and I think they still end up transitive dependencies of the mongodb exporter :(. Planning to investigate further in a follow up. |
…ertions PSI and cgroup v2 memory metrics are only produced on capable kernels, so asserting them unconditionally breaks the test on hosts without CONFIG_PSI or cgroup v2 (for example local dev). The kind node shares the host kernel, so gate these assertions on the host's capabilities and assert them only where cAdvisor emits them. Also drop container_cpu_cfs_burst_* : cAdvisor only emits burst metrics for containers with a non-zero CPU quota, which no workload in this test guarantees. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cAdvisor v0.60 stopped importing the legacy github.com/docker/docker client. The module still enters the build through prometheus.exporter.mongodb (github.com/percona/percona-backup-mongodb imports docker/docker/api/types), so the advisories still apply but through a different, client-only path. Update the ignore reasons to name the real importer and revisit condition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Bump the cAdvisor fork to the commit that adds manager.Config/Factories, and pass perf, resctrl, and application-metrics factories through manager.Config.Factories instead of assigning the package-level *Factory globals in New(). This removes the reload/multi-instance race on those globals: the manager now resolves each seam once at construction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Brief description of Pull Request
Updates the cAdvisor dependency from v0.54.1 to v0.60.5 (Grafana
grafana-v0.60.5-noglobalsfork).Pull Request Details
cAdvisor v0.60 moved most of its library packages into a new
github.com/google/cadvisor/libsubmodule. The root module points/libat a local./libreplace, which Go ignores when the module is consumed as a dependency. To make it resolve,collector/builder-config.yamlnow carries a second require/replace pair that redirectsgithub.com/google/cadvisor/libto the same Grafana fork commit, and thego.modfiles are regenerated withmake generate-otel-collector-distro.The integration imports are rewritten to the new
/libpaths.container/dockerandinfo/v2stayed in the root module, so those imports are unchanged. No cAdvisor API signatures changed.Stacked on top of #7039 (the cAdvisor integration test); this PR targets that branch so its diff is only the dependency update.
Issue(s) fixed by this Pull Request
Notes to the Reviewer
PR Checklist