From 04595cd4e4701deaab81045e9e7458440f433460 Mon Sep 17 00:00:00 2001 From: nicbaz <932244+nicbaz@users.noreply.github.com> Date: Tue, 26 Aug 2025 13:24:13 +0200 Subject: [PATCH 1/2] filesystem: take super options into account for read-only With the latest change implemented to use `mountinfo` instead of `mounts` there was a regression in filesystem readonly detection due to super options not taken into account: filesystems that would previously be marked a "read-only" would not anymore because that information had moved to super options instead of mount options on certain occasions. fixes #3157 Signed-off-by: nicbaz <932244+nicbaz@users.noreply.github.com> --- collector/filesystem_common.go | 2 +- collector/filesystem_linux.go | 35 +++++++++++++++++---------- collector/filesystem_linux_test.go | 39 ++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 14 deletions(-) diff --git a/collector/filesystem_common.go b/collector/filesystem_common.go index 4161b3fe57..1e61e8e705 100644 --- a/collector/filesystem_common.go +++ b/collector/filesystem_common.go @@ -82,7 +82,7 @@ type filesystemCollector struct { } type filesystemLabels struct { - device, mountPoint, fsType, options, deviceError, major, minor string + device, mountPoint, fsType, mountOptions, superOptions, deviceError, major, minor string } type filesystemStats struct { diff --git a/collector/filesystem_linux.go b/collector/filesystem_linux.go index 127a3be1d6..7140f50e1b 100644 --- a/collector/filesystem_linux.go +++ b/collector/filesystem_linux.go @@ -23,6 +23,7 @@ import ( "io" "log/slog" "os" + "slices" "strings" "sync" "time" @@ -110,11 +111,8 @@ func (c *filesystemCollector) GetStats() ([]filesystemStats, error) { func (c *filesystemCollector) processStat(labels filesystemLabels) filesystemStats { var ro float64 - for _, option := range strings.Split(labels.options, ",") { - if option == "ro" { - ro = 1 - break - } + if isFilesystemReadOnly(labels) { + ro = 1 } success := make(chan struct{}) @@ -198,7 +196,7 @@ func parseFilesystemLabels(r io.Reader) ([]filesystemLabels, error) { for scanner.Scan() { parts := strings.Fields(scanner.Text()) - if len(parts) < 10 { + if len(parts) < 11 { return nil, fmt.Errorf("malformed mount point information: %q", scanner.Text()) } @@ -219,15 +217,26 @@ func parseFilesystemLabels(r io.Reader) ([]filesystemLabels, error) { parts[4] = strings.ReplaceAll(parts[4], "\\011", "\t") filesystems = append(filesystems, filesystemLabels{ - device: parts[m+3], - mountPoint: rootfsStripPrefix(parts[4]), - fsType: parts[m+2], - options: parts[5], - major: fmt.Sprint(major), - minor: fmt.Sprint(minor), - deviceError: "", + device: parts[m+3], + mountPoint: rootfsStripPrefix(parts[4]), + fsType: parts[m+2], + mountOptions: parts[5], + superOptions: parts[10], + major: fmt.Sprint(major), + minor: fmt.Sprint(minor), + deviceError: "", }) } return filesystems, scanner.Err() } + +// see https://github.com/prometheus/node_exporter/issues/3157#issuecomment-2422761187 +// if either mount or super options contain "ro" the filesystem is read-only +func isFilesystemReadOnly(labels filesystemLabels) bool { + if slices.Contains(strings.Split(labels.mountOptions, ","), "ro") || slices.Contains(strings.Split(labels.superOptions, ","), "ro") { + return true + } + + return false +} diff --git a/collector/filesystem_linux_test.go b/collector/filesystem_linux_test.go index d088598f3c..7b3e22c446 100644 --- a/collector/filesystem_linux_test.go +++ b/collector/filesystem_linux_test.go @@ -45,6 +45,45 @@ func Test_parseFilesystemLabelsError(t *testing.T) { } } +func Test_isFilesystemReadOnly(t *testing.T) { + tests := map[string]struct { + labels filesystemLabels + expected bool + }{ + "/media/volume1": { + labels: filesystemLabels{ + mountOptions: "rw,nosuid,nodev,noexec,relatime", + superOptions: "rw,devices", + }, + expected: false, + }, + "/media/volume2": { + labels: filesystemLabels{ + mountOptions: "ro,relatime", + superOptions: "rw,fd=22,pgrp=1,timeout=300,minproto=5,maxproto=5,direct", + }, expected: true, + }, + "/media/volume3": { + labels: filesystemLabels{ + mountOptions: "rw,user_id=1000,group_id=1000", + superOptions: "ro", + }, expected: true, + }, + "/media/volume4": { + labels: filesystemLabels{ + mountOptions: "ro,nosuid,noexec", + superOptions: "ro,nodev", + }, expected: true, + }, + } + + for _, tt := range tests { + if got := isFilesystemReadOnly(tt.labels); got != tt.expected { + t.Errorf("Expected %t, got %t", tt.expected, got) + } + } +} + func TestMountPointDetails(t *testing.T) { if _, err := kingpin.CommandLine.Parse([]string{"--path.procfs", "./fixtures/proc"}); err != nil { t.Fatal(err) From 1aa3c5b5c04b757d1ddc295715aa3fca704a29e7 Mon Sep 17 00:00:00 2001 From: nicbaz <932244+nicbaz@users.noreply.github.com> Date: Wed, 27 Aug 2025 15:37:37 +0200 Subject: [PATCH 2/2] filesystem: use faster integer to string implementation Signed-off-by: nicbaz <932244+nicbaz@users.noreply.github.com> --- collector/filesystem_linux.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/collector/filesystem_linux.go b/collector/filesystem_linux.go index 7140f50e1b..e7712856a9 100644 --- a/collector/filesystem_linux.go +++ b/collector/filesystem_linux.go @@ -24,6 +24,7 @@ import ( "log/slog" "os" "slices" + "strconv" "strings" "sync" "time" @@ -222,8 +223,8 @@ func parseFilesystemLabels(r io.Reader) ([]filesystemLabels, error) { fsType: parts[m+2], mountOptions: parts[5], superOptions: parts[10], - major: fmt.Sprint(major), - minor: fmt.Sprint(minor), + major: strconv.Itoa(major), + minor: strconv.Itoa(minor), deviceError: "", }) }