PMM-14832 make mount path configurable#208
Conversation
nmarukovich
requested review from
ademidoff and
maxkondr
and removed request for
a team
|
|
There was a problem hiding this comment.
Pull request overview
This PR makes the filesystem collector’s mount-info source configurable so deployments affected by shareProcessNamespace (where /proc/1/mounts reflects the pause container’s restricted mount namespace) can point the collector at a more accurate mounts file.
Changes:
- Add
--collector.filesystem.mount-info-pathto override the mounts file used by the filesystem collector. - Update
mountPointDetailsto read the override path when provided and limit fallback behavior to the default mode. - Add a unit test validating that the override path takes precedence over
--path.procfs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
collector/filesystem_linux.go |
Introduces a new CLI flag and uses it to select the mounts file path in mountPointDetails. |
collector/filesystem_linux_test.go |
Adds coverage for the new override flag behavior using fixtures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestMountInfoPathOverride(t *testing.T) { | ||
| // --path.procfs points to fixtures with 25+ mount points, | ||
| // but --collector.filesystem.mount-info-path overrides to a file with only 1 mount. | ||
| if _, err := kingpin.CommandLine.Parse([]string{ | ||
| "--path.procfs", "./fixtures/proc", | ||
| "--collector.filesystem.mount-info-path", "./fixtures_hidepid/proc/mounts", | ||
| }); err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
This test sets the global --collector.filesystem.mount-info-path via kingpin.CommandLine.Parse but never restores it. Because kingpin flag values persist across parses and Go test order is not guaranteed, this can make other tests in this package order-dependent/flaky (they don’t pass the flag, so they may inherit the previous value). Consider saving the prior *mountInfoPath and restoring it in t.Cleanup, or explicitly setting the flag back to empty in other tests that call mountPointDetails.
| "how many stat calls to process simultaneously"). | ||
| Hidden().Default("4").Int() | ||
| var mountInfoPath = kingpin.Flag("collector.filesystem.mount-info-path", | ||
| "Override path to mounts file. If set, disables the default /proc/1/mounts fallback logic."). |
There was a problem hiding this comment.
--collector.filesystem.mount-info-path is treated as an exact filesystem path and bypasses --path.procfs entirely. That can be surprising for users who expect a value like mounts or self/mounts to be resolved under the configured procfs mountpoint (similar to other proc-based reads in this package). Either document that the flag must be an absolute/fully-qualified path, or resolve relative values against procFilePath(...) for consistency.
| "Override path to mounts file. If set, disables the default /proc/1/mounts fallback logic."). | |
| "Override absolute path to mounts file. This must be a fully-qualified filesystem path; relative paths are not resolved against --path.procfs. If set, disables the default /proc/1/mounts fallback logic."). |
| if errors.Is(err, os.ErrNotExist) && *mountInfoPath == "" { | ||
| // Fallback to `/proc/mounts` if `/proc/1/mounts` is missing due hidepid. |
There was a problem hiding this comment.
The fallback to procFilePath("mounts") is only triggered on os.ErrNotExist. In common hidepid/permission-restricted setups, opening /proc/1/mounts often fails with EACCES (permission denied) rather than being missing, which would currently return an error and skip the intended fallback. Consider broadening the fallback condition (e.g., also on os.ErrPermission or on any open error when mountInfoPath is unset), and update the comment accordingly.
| if errors.Is(err, os.ErrNotExist) && *mountInfoPath == "" { | |
| // Fallback to `/proc/mounts` if `/proc/1/mounts` is missing due hidepid. | |
| if err != nil && *mountInfoPath == "" { | |
| // Fallback to `/proc/mounts` if `/proc/1/mounts` is missing or inaccessible (e.g. hidepid/permission restrictions). |
|
@nmarukovich Could you please sign the CLA? |
By design, node_exporter first tries to read /proc/1/mounts and only falls back to /proc/self/mounts if /proc/1/mounts is not accessible.
However, with shareProcessNamespace enabled (like we have in postgres operator):
PID 1 inside the Pod belongs to the /pause container.
The /pause container runs in a restricted mount namespace.
As a result, /proc/1/mounts only reflects the limited mount view of the /pause container.
It does not expose the full set of actual node mounts.
Because /proc/1/mounts is technically accessible, node_exporter does not fall back to /proc/self/mounts.
This leads to incomplete or incorrect mount metrics being collected by node_exporter