collector/filesystem: fix readonly false positives under ro rootfs bind mount - #3758
Open
locker95 wants to merge 1 commit into
Open
collector/filesystem: fix readonly false positives under ro rootfs bind mount#3758locker95 wants to merge 1 commit into
locker95 wants to merge 1 commit into
Conversation
…nd mount Since 65902fa (prometheus#3659) fixed the missing comma separator in mountOptionsString, isFilesystemReadOnly actually matches the per-mount "ro" flag. In the recommended containerized deployment the host root is bind-mounted read-only into the container (`-v /:/host:ro,rslave` with --path.rootfs=/host), so every host mount carries a per-mount "ro" flag in the exporter's mount namespace, and node_filesystem_readonly now reports 1 for host filesystems that are writable on the host. Only consider the superblock options, which are shared across mount namespaces, for mount points below --path.rootfs. Filesystems that really are read-only (superblock "ro", ext4 "emergency_ro") are still reported, and mounts outside the rootfs prefix keep the per-mount semantics from prometheus#3484. Fixes prometheus#3755 Signed-off-by: Dean Chen <862469039@qq.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #3755
The regression
After upgrading from 1.11.x to 1.12.0/1.12.1,
node_filesystem_readonlyreports1for host filesystems that are clearly writable, when node_exporter runs in the recommended containerized deployment (-v /:/host:ro,rslave+--path.rootfs=/host).Root cause
isFilesystemReadOnly()(introduced for #3157/#3484) considers a filesystem read-only when either the per-mount options (mountinfo field 6) or the super options (last mountinfo field) containro:mountOptionsString()concatenated the options map without separators (e.g.ro,relatimebecamerorelatime), sostrings.Split(opts, ",")could never matchroalongside other options. The per-mount check was effectively dead — that hid this problem (while also causing node_filesystem_readonly does not report readonly bind mounts correctly #3484).rocheck actually started matching.In the recommended containerized deployment the host root is bind-mounted read-only into the container. On current Docker (25.0+/API 1.44+, kernel >= 5.12)
robind mounts are recursively read-only, so every host mount propagated viarslavecarries a per-mountroflag in the exporter's mount namespace — even though the filesystems are writable on the host. Unless the container also runs with--pid=host(in which case/proc/1/mountinfoprovides the host's view), the exporter reads its own namespace's mountinfo and now reportsreadonly 1for every host filesystem. That is exactly the reporter's setup in #3755, and the same false positive was previously observed with the statfs-based detection in #2506.The per-mount
roseen through--path.rootfsdescribes the exporter's own view of the host mounts (an artifact of the deployment), not the state of the host filesystem.The fix
For mount points below the configured
--path.rootfsprefix, only consider the super options for the read-only decision. The superblock is shared across mount namespaces, soro/emergency_rothere reliably reflects the state of the host filesystem regardless of how the exporter observes it:superOptions=rw, per-mountroignored) — fixes the regression.ro(e.g.mount -o ro, or an ext4 emergency remount triggered byerrors=remount-ro) andemergency_ro(Linux 6.15+, filesystem: support ext4 super block emergency_ro flag #3717) are honored for host mounts.--pid=hostwhere mountinfo is read from the host's PID 1) are unchanged: the per-mountrosemantics from node_filesystem_readonly does not report readonly bind mounts correctly #3484 (read-only bind mounts of a writable device) are preserved.Tests
collector/fixtures_bindmount_ro/mirrors the reporter's environment as seen from the container's mount namespace:/hostand/host/var/datawith per-mountrobut superblockrw(must be0), plus a superblock-romount and anemergency_romount under/host(must stay1), and arobind mount of the container itself outside the rootfs prefix (must stay1, node_filesystem_readonly does not report readonly bind mounts correctly #3484 semantics).TestPathRootfsReadOnlyrunsmountPointDetails+isFilesystemReadOnlyagainst that fixture. It fails on current master (/and/var/dataare reported read-only) and passes with this change.hostMountcases inTest_isFilesystemReadOnlycover the unit-level behavior.Verified locally with
GOOS=linux go build,GOOS=linux go vetandGOOS=linux go test -c(development on a non-Linux host — the Linux test suite runs in CI).