Repository: prometheus/procfs
Pull request #181 (2019-06-14 21:39:28)

Fixed edge cases, added more tests, and extracted failed/spare device information from mdstat
This pull request aims to add functionality as required in [this issue](https://github.com/prometheus/node_exporter/issues/261) in [this comment](https://github.com/prometheus/node_exporter/issues/261#issuecomment-500433010)

This PR modifies the MdStat struct to export failed device count and spare device count, so that node_exporter could expose them. It also adds several edge test cases.  

- Added code to extract Failed and Spare device counts from `/proc/mdstat`
- Refactored some variable names to be more descriptive.
- Added code to extract "recovering" or "resyncing" state from `/proc/mdstat` 
- Added more test cases from `https://github.com/prometheus/node_exporter/mdadm_linux_test.go`
- Added a test case to check for a `read-only raid0 device`: md101 in fixtures.ttar file. 

@discordianfish @pgier 


Patch:
diff --git a/Makefile.common b/Makefile.common
index d7aea1b..6b94f83 100644
--- a/Makefile.common
+++ b/Makefile.common
@@ -248,9 +248,7 @@ proto:
 ifdef GOLANGCI_LINT
 $(GOLANGCI_LINT):
 	mkdir -p $(FIRST_GOPATH)/bin
-	curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/$(GOLANGCI_LINT_VERSION)/install.sh \
-		| sed -e '/install -d/d' \
-		| sh -s -- -b $(FIRST_GOPATH)/bin $(GOLANGCI_LINT_VERSION)
+	curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $(FIRST_GOPATH)/bin $(GOLANGCI_LINT_VERSION)
 endif
 
 ifdef GOVENDOR
diff --git a/fixtures.ttar b/fixtures.ttar
index 951d909..1be3fc0 100644
--- a/fixtures.ttar
+++ b/fixtures.ttar
@@ -3,7 +3,7 @@ Directory: fixtures
 Mode: 775
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 Directory: fixtures/proc
-Mode: 755
+Mode: 775
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 Directory: fixtures/proc/26231
 Mode: 755
@@ -356,32 +356,62 @@ debug 0
 Mode: 644
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 Path: fixtures/proc/mdstat
-Lines: 26
+Lines: 56
 Personalities : [linear] [multipath] [raid0] [raid1] [raid6] [raid5] [raid4] [raid10]
-md3 : active raid6 sda1[8] sdh1[7] sdg1[6] sdf1[5] sde1[11] sdd1[3] sdc1[10] sdb1[9]
+
+md3 : active raid6 sda1[8] sdh1[7] sdg1[6] sdf1[5] sde1[11] sdd1[3] sdc1[10] sdb1[9] sdd1[10](S) sdd2[11](S)
       5853468288 blocks super 1.2 level 6, 64k chunk, algorithm 2 [8/8] [UUUUUUUU]
 
 md127 : active raid1 sdi2[0] sdj2[1]
       312319552 blocks [2/2] [UU]
 
-md0 : active raid1 sdk[2](S) sdi1[0] sdj1[1]
+md0 : active raid1 sdi1[0] sdj1[1]
       248896 blocks [2/2] [UU]
 
-md4 : inactive raid1 sda3[0] sdb3[1]
+md4 : inactive raid1 sda3[0](F) sdb3[1](S)
       4883648 blocks [2/2] [UU]
 
-md6 : active raid1 sdb2[2] sda2[0]
+md6 : active raid1 sdb2[2](F) sdc[1](S) sda2[0]
       195310144 blocks [2/1] [U_]
       [=>...................]  recovery =  8.5% (16775552/195310144) finish=17.0min speed=259783K/sec
 
-md8 : active raid1 sdb1[1] sda1[0]
+md8 : active raid1 sdb1[1] sda1[0] sdc[2](S) sde[3](S)
       195310144 blocks [2/2] [UU]
       [=>...................]  resync =  8.5% (16775552/195310144) finish=17.0min speed=259783K/sec
 
-md7 : active raid6 sdb1[0] sde1[3] sdd1[2] sdc1[1]
+md7 : active raid6 sdb1[0] sde1[3] sdd1[2] sdc1[1](F)
       7813735424 blocks super 1.2 level 6, 512k chunk, algorithm 2 [4/3] [U_UU]
       bitmap: 0/30 pages [0KB], 65536KB chunk
 
+md9 : active raid1 sdc2[2] sdd2[3] sdb2[1] sda2[0] sde[4](F) sdf[5](F) sdg[6](S)
+      523968 blocks super 1.2 [4/4] [UUUU]
+      resync=DELAYED
+
+md10 : active raid0 sda1[0] sdb1[1]
+       314159265 blocks 64k chunks
+
+md11 : active (auto-read-only) raid1 sdb2[0] sdc2[1] sdc3[2](F) hda[4](S) ssdc2[3](S)
+      4190208 blocks super 1.2 [2/2] [UU]
+      resync=PENDING
+
+md12 : active raid0 sdc2[0] sdd2[1]
+      3886394368 blocks super 1.2 512k chunks
+
+md126 : active raid0 sdb[1] sdc[0]
+      1855870976 blocks super external:/md127/0 128k chunks
+
+md219 : inactive sdb[2](S) sdc[1](S) sda[0](S)
+        7932 blocks super external:imsm
+
+md00 : active raid0 xvdb[0]
+      4186624 blocks super 1.2 256k chunks
+
+md120 : active linear sda1[1] sdb1[0]
+        2095104 blocks super 1.2 0k rounding
+
+md101 : active (read-only) raid0 sdb[2] sdd[1] sdc[0]
+      322560 blocks super 1.2 512k chunks
+
 unused devices: <none>
 Mode: 644
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
diff --git a/mdstat.go b/mdstat.go
index 71c1067..2af3ada 100644
--- a/mdstat.go
+++ b/mdstat.go
@@ -22,8 +22,8 @@ import (
 )
 
 var (
-	statuslineRE = regexp.MustCompile(`(\d+) blocks .*\[(\d+)/(\d+)\] \[[U_]+\]`)
-	buildlineRE  = regexp.MustCompile(`\((\d+)/\d+\)`)
+	statusLineRE   = regexp.MustCompile(`(\d+) blocks .*\[(\d+)/(\d+)\] \[[U_]+\]`)
+	recoveryLineRE = regexp.MustCompile(`\((\d+)/\d+\)`)
 )
 
 // MDStat holds info parsed from /proc/mdstat.
@@ -34,8 +34,12 @@ type MDStat struct {
 	ActivityState string
 	// Number of active disks.
 	DisksActive int64
-	// Total number of disks the device consists of.
+	// Total number of disks the device requires.
 	DisksTotal int64
+	// Number of failed disks.
+	DisksFailed int64
+	// Spare disks in the device.
+	DisksSpare int64
 	// Number of blocks the device holds.
 	BlocksTotal int64
 	// Number of blocks on the device that are in sync.
@@ -59,29 +63,38 @@ func (fs FS) MDStat() ([]MDStat, error) {
 
 // parseMDStat parses data from mdstat file (/proc/mdstat) and returns a slice of
 // structs containing the relevant info.
-func parseMDStat(mdstatData []byte) ([]MDStat, error) {
+func parseMDStat(mdStatData []byte) ([]MDStat, error) {
 	mdStats := []MDStat{}
-	lines := strings.Split(string(mdstatData), "\n")
-	for i, l := range lines {
-		if strings.TrimSpace(l) == "" || l[0] == ' ' ||
-			strings.HasPrefix(l, "Personalities") || strings.HasPrefix(l, "unused") {
+	lines := strings.Split(string(mdStatData), "\n")
+
+	for i, line := range lines {
+		if strings.TrimSpace(line) == "" || line[0] == ' ' ||
+			strings.HasPrefix(line, "Personalities") ||
+			strings.HasPrefix(line, "unused") {
 			continue
 		}
 
-		deviceFields := strings.Fields(l)
+		deviceFields := strings.Fields(line)
 		if len(deviceFields) < 3 {
-			return nil, fmt.Errorf("not enough fields in mdline (expected at least 3): %s", l)
+			return nil, fmt.Errorf("not enough fields in mdline (expected at least 3): %s", line)
 		}
-		mdName := deviceFields[0]
-		activityState := deviceFields[2]
+		mdName := deviceFields[0] // mdx
+		state := deviceFields[2]  // active or inactive
 
 		if len(lines) <= i+3 {
-			return mdStats, fmt.Errorf("missing lines for md device %s", mdName)
+			return nil, fmt.Errorf(
+				"error parsing %s: too few lines for md device",
+				mdName,
+			)
 		}
 
-		active, total, size, err := evalStatusLine(lines[i+1])
+		// Failed disks have the suffix (F) & Spare disks have the suffix (S).
+		fail := int64(strings.Count(line, "(F)"))
+		spare := int64(strings.Count(line, "(S)"))
+		active, total, size, err := evalStatusLine(lines[i], lines[i+1])
+
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("error parsing md device lines: %s", err)
 		}
 
 		syncLineIdx := i + 2
@@ -89,20 +102,38 @@ func parseMDStat(mdstatData []byte) ([]MDStat, error) {
 			syncLineIdx++
 		}
 
-		// If device is recovering/syncing at the moment, get the number of currently
+		// If device is syncing at the moment, get the number of currently
 		// synced bytes, otherwise that number equals the size of the device.
 		syncedBlocks := size
-		if strings.Contains(lines[syncLineIdx], "recovery") || strings.Contains(lines[syncLineIdx], "resync") {
-			syncedBlocks, err = evalRecoveryLine(lines[syncLineIdx])
-			if err != nil {
-				return nil, err
+		recovering := strings.Contains(lines[syncLineIdx], "recovery")
+		resyncing := strings.Contains(lines[syncLineIdx], "resync")
+
+		// Append recovery and resyncing state info.
+		if recovering || resyncing {
+			if recovering {
+				state = "recovering"
+			} else {
+				state = "resyncing"
+			}
+
+			// Handle case when resync=PENDING or resync=DELAYED.
+			if strings.Contains(lines[syncLineIdx], "PENDING") ||
+				strings.Contains(lines[syncLineIdx], "DELAYED") {
+				syncedBlocks = 0
+			} else {
+				syncedBlocks, err = evalRecoveryLine(lines[syncLineIdx])
+				if err != nil {
+					return nil, fmt.Errorf("error parsing sync line in md device %s: %s", mdName, err)
+				}
 			}
 		}
 
 		mdStats = append(mdStats, MDStat{
 			Name:          mdName,
-			ActivityState: activityState,
+			ActivityState: state,
 			DisksActive:   active,
+			DisksFailed:   fail,
+			DisksSpare:    spare,
 			DisksTotal:    total,
 			BlocksTotal:   size,
 			BlocksSynced:  syncedBlocks,
@@ -112,39 +143,51 @@ func parseMDStat(mdstatData []byte) ([]MDStat, error) {
 	return mdStats, nil
 }
 
-func evalStatusLine(statusline string) (active, total, size int64, err error) {
-	matches := statuslineRE.FindStringSubmatch(statusline)
-	if len(matches) != 4 {
-		return 0, 0, 0, fmt.Errorf("unexpected statusline: %s", statusline)
-	}
+func evalStatusLine(deviceLine, statusLine string) (active, total, size int64, err error) {
 
-	size, err = strconv.ParseInt(matches[1], 10, 64)
+	sizeStr := strings.Fields(statusLine)[0]
+	size, err = strconv.ParseInt(sizeStr, 10, 64)
 	if err != nil {
-		return 0, 0, 0, fmt.Errorf("unexpected statusline %s: %s", statusline, err)
+		return 0, 0, 0, fmt.Errorf("unexpected statusLine %s: %s", statusLine, err)
+	}
+
+	if strings.Contains(deviceLine, "raid0") || strings.Contains(deviceLine, "linear") {
+		// In the device deviceLine, only disks have a number associated with them in [].
+		total = int64(strings.Count(deviceLine, "["))
+		return total, total, size, nil
+	}
+
+	if strings.Contains(deviceLine, "inactive") {
+		return 0, 0, size, nil
+	}
+
+	matches := statusLineRE.FindStringSubmatch(statusLine)
+	if len(matches) != 4 {
+		return 0, 0, 0, fmt.Errorf("couldn't find all the substring matches: %s", statusLine)
 	}
 
 	total, err = strconv.ParseInt(matches[2], 10, 64)
 	if err != nil {
-		return 0, 0, 0, fmt.Errorf("unexpected statusline %s: %s", statusline, err)
+		return 0, 0, 0, fmt.Errorf("unexpected statusLine %s: %s", statusLine, err)
 	}
 
 	active, err = strconv.ParseInt(matches[3], 10, 64)
 	if err != nil {
-		return 0, 0, 0, fmt.Errorf("unexpected statusline %s: %s", statusline, err)
+		return 0, 0, 0, fmt.Errorf("unexpected statusLine %s: %s", statusLine, err)
 	}
 
 	return active, total, size, nil
 }
 
-func evalRecoveryLine(buildline string) (syncedBlocks int64, err error) {
-	matches := buildlineRE.FindStringSubmatch(buildline)
+func evalRecoveryLine(recoveryLine string) (syncedBlocks int64, err error) {
+	matches := recoveryLineRE.FindStringSubmatch(recoveryLine)
 	if len(matches) != 2 {
-		return 0, fmt.Errorf("unexpected buildline: %s", buildline)
+		return 0, fmt.Errorf("unexpected recoveryLine: %s", recoveryLine)
 	}
 
 	syncedBlocks, err = strconv.ParseInt(matches[1], 10, 64)
 	if err != nil {
-		return 0, fmt.Errorf("%s in buildline: %s", err, buildline)
+		return 0, fmt.Errorf("%s in recoveryLine: %s", err, recoveryLine)
 	}
 
 	return syncedBlocks, nil


Test patch:
diff --git a/fs_test.go b/fs_test.go
index 6e57745..1764bd4 100644
--- a/fs_test.go
+++ b/fs_test.go
@@ -33,7 +33,7 @@ func TestNewFS(t *testing.T) {
 func getProcFixtures(t *testing.T) FS {
 	fs, err := NewFS(procTestFixtures)
 	if err != nil {
-		t.Fatal(err)
+		t.Fatalf("Creating pseudo fs from getProcFixtures failed at fixtures/proc with error: %s", err)
 	}
 	return fs
 }
diff --git a/mdstat_test.go b/mdstat_test.go
index 99a5adf..f2d2648 100644
--- a/mdstat_test.go
+++ b/mdstat_test.go
@@ -13,24 +13,33 @@
 
 package procfs
 
-import (
-	"testing"
-)
+import "testing"
+
+func TestFS_MDStat(t *testing.T) {
+	fs := getProcFixtures(t)
+	mdStats, err := fs.MDStat()
 
-func TestMDStat(t *testing.T) {
-	mdStats, err := getProcFixtures(t).MDStat()
 	if err != nil {
 		t.Fatalf("parsing of reference-file failed entirely: %s", err)
 	}
 
 	refs := map[string]MDStat{
-		"md3":   {"md3", "active", 8, 8, 5853468288, 5853468288},
-		"md127": {"md127", "active", 2, 2, 312319552, 312319552},
-		"md0":   {"md0", "active", 2, 2, 248896, 248896},
-		"md4":   {"md4", "inactive", 2, 2, 4883648, 4883648},
-		"md6":   {"md6", "active", 1, 2, 195310144, 16775552},
-		"md8":   {"md8", "active", 2, 2, 195310144, 16775552},
-		"md7":   {"md7", "active", 3, 4, 7813735424, 7813735424},
+		"md127": {Name: "md127", ActivityState: "active", DisksActive: 2, DisksTotal: 2, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 312319552, BlocksSynced: 312319552},
+		"md0":   {Name: "md0", ActivityState: "active", DisksActive: 2, DisksTotal: 2, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 248896, BlocksSynced: 248896},
+		"md4":   {Name: "md4", ActivityState: "inactive", DisksActive: 0, DisksTotal: 0, DisksFailed: 1, DisksSpare: 1, BlocksTotal: 4883648, BlocksSynced: 4883648},
+		"md6":   {Name: "md6", ActivityState: "recovering", DisksActive: 1, DisksTotal: 2, DisksFailed: 1, DisksSpare: 1, BlocksTotal: 195310144, BlocksSynced: 16775552},
+		"md3":   {Name: "md3", ActivityState: "active", DisksActive: 8, DisksTotal: 8, DisksFailed: 0, DisksSpare: 2, BlocksTotal: 5853468288, BlocksSynced: 5853468288},
+		"md8":   {Name: "md8", ActivityState: "resyncing", DisksActive: 2, DisksTotal: 2, DisksFailed: 0, DisksSpare: 2, BlocksTotal: 195310144, BlocksSynced: 16775552},
+		"md7":   {Name: "md7", ActivityState: "active", DisksActive: 3, DisksTotal: 4, DisksFailed: 1, DisksSpare: 0, BlocksTotal: 7813735424, BlocksSynced: 7813735424},
+		"md9":   {Name: "md9", ActivityState: "resyncing", DisksActive: 4, DisksTotal: 4, DisksSpare: 1, DisksFailed: 2, BlocksTotal: 523968, BlocksSynced: 0},
+		"md10":  {Name: "md10", ActivityState: "active", DisksActive: 2, DisksTotal: 2, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 314159265, BlocksSynced: 314159265},
+		"md11":  {Name: "md11", ActivityState: "resyncing", DisksActive: 2, DisksTotal: 2, DisksFailed: 1, DisksSpare: 2, BlocksTotal: 4190208, BlocksSynced: 0},
+		"md12":  {Name: "md12", ActivityState: "active", DisksActive: 2, DisksTotal: 2, DisksSpare: 0, DisksFailed: 0, BlocksTotal: 3886394368, BlocksSynced: 3886394368},
+		"md120": {Name: "md120", ActivityState: "active", DisksActive: 2, DisksTotal: 2, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 2095104, BlocksSynced: 2095104},
+		"md126": {Name: "md126", ActivityState: "active", DisksActive: 2, DisksTotal: 2, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 1855870976, BlocksSynced: 1855870976},
+		"md219": {Name: "md219", ActivityState: "inactive", DisksTotal: 0, DisksFailed: 0, DisksActive: 0, DisksSpare: 3, BlocksTotal: 7932, BlocksSynced: 7932},
+		"md00":  {Name: "md00", ActivityState: "active", DisksActive: 1, DisksTotal: 1, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 4186624, BlocksSynced: 4186624},
+		"md101": {Name: "md101", ActivityState: "active", DisksActive: 3, DisksTotal: 3, DisksFailed: 0, DisksSpare: 0, BlocksTotal: 322560, BlocksSynced: 322560},
 	}
 
 	if want, have := len(refs), len(mdStats); want != have {
@@ -41,4 +50,20 @@ func TestMDStat(t *testing.T) {
 			t.Errorf("%s: want %v, have %v", md.Name, want, have)
 		}
 	}
+
+}
+
+func TestInvalidMdstat(t *testing.T) {
+	invalidMount := []byte(`
+Personalities : [invalid]
+md3 : invalid
+      314159265 blocks 64k chunks
+
+unused devices: <none>
+`)
+
+	_, err := parseMDStat(invalidMount)
+	if err == nil {
+		t.Fatalf("parsing of invalid reference file did not find any errors")
+	}
 }