Repository: prometheus/procfs
Pull request #497 (2023-03-05 03:51:09)

Bug Fix: SystemCPUfreq fails when any core is offline
SystemCpufreq function does not check if a core is online before collecting metrics. In the case when a core is offline it fails and does not collect a couple of metrics.

Refer https://github.com/prometheus/node_exporter/issues/2577 for more details

taherkk: I have read the readme and steps for contribution and have tried my best to follow it. This my first open source contribution so please guide me if I have missed something.
taherkk: Tests are failing because each directory under .../cpu[0-9] has a file named online in Linux except for cpu0. This denotes if the core is online or not.
I added the file in my local setup manually and the tests are passing.

Please help me in configuring that file using fixtures.
discordianfish: You also need to sign off your commit, see the failing DCO check above
taherkk: Signed off my commit.
Not sure why the lint step is failing.
discordianfish: Here is the lint error:
```
#!/bin/bash -eo pipefail
git diff --exit-code
diff --git a/testdata/fixtures.ttar b/testdata/fixtures.ttar
index cb49947..b80316a 100644
--- a/testdata/fixtures.ttar
+++ b/testdata/fixtures.ttar
@@ -12543,11 +12543,6 @@ Mode: 444
 Directory: fixtures/sys/devices/system/cpu/cpu1
 Mode: 775
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
-Path: fixtures/sys/devices/system/cpu/cpu1/online
-Lines: 1
-1
-Mode: 544
-# ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 Directory: fixtures/sys/devices/system/cpu/cpu1/cpufreq
 Mode: 775
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -12606,6 +12601,11 @@ Lines: 1
 <unsupported>
 Mode: 664
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+Path: fixtures/sys/devices/system/cpu/cpu1/online
+Lines: 1
+1
+Mode: 544
+# ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 Directory: fixtures/sys/devices/system/cpu/cpu1/thermal_throttle
 Mode: 755
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -

Exited with code exit status 1
CircleCI received exit code 1
```

You probably didn't run `make fixtures` to create the ttar file
taherkk: I'm exploring checking for offline cpu's in `/sys/devices/system/cpu/offline`

Just sharing the approach here:
Get cpu's from the file.
Expand ranges.
Filter cpu's which not physically present (Though I have a hexacore CPU the offline file shows a range of 12-15 which are not physically present)
In the for loop add an isOnline check which uses binary search on the slice of offline cpu's. 
taherkk: @pgier, I have updated the code according to the approach mentioned earlier.

Patch:
diff --git a/sysfs/system_cpu.go b/sysfs/system_cpu.go
index a8d0ac2..9e15858 100644
--- a/sysfs/system_cpu.go
+++ b/sysfs/system_cpu.go
@@ -142,6 +142,49 @@ func parseCPUThermalThrottle(cpuPath string) (*CPUThermalThrottle, error) {
 	return &t, nil
 }
 
+func binSearch(elem uint16, elemSlice *[]uint16) bool {
+	if len(*elemSlice) == 0 {
+		return false
+	}
+
+	if len(*elemSlice) == 1 {
+		return elem == (*elemSlice)[0]
+	}
+
+	start := 0
+	end := len(*elemSlice) - 1
+
+	var mid int
+
+	for start <= end {
+		mid = (start + end) / 2
+		if (*elemSlice)[mid] == elem {
+			return true
+		} else if (*elemSlice)[mid] > elem {
+			end = mid - 1
+		} else if (*elemSlice)[mid] < elem {
+			start = mid + 1
+		}
+	}
+
+	return false
+}
+
+func filterOfflineCPUs(offlineCpus *[]uint16, cpus *[]string) error {
+	for i, cpu := range *cpus {
+		cpuName := strings.TrimPrefix(filepath.Base(cpu), "cpu")
+		cpuNameUint16, err := strconv.Atoi(cpuName)
+		if err != nil {
+			return err
+		}
+		if binSearch(uint16(cpuNameUint16), offlineCpus) {
+			*cpus = append((*cpus)[:i], (*cpus)[i+1:]...)
+		}
+	}
+
+	return nil
+}
+
 // SystemCpufreq returns CPU frequency metrics for all CPUs.
 func (fs FS) SystemCpufreq() ([]SystemCPUCpufreqStats, error) {
 	var g errgroup.Group
@@ -151,6 +194,25 @@ func (fs FS) SystemCpufreq() ([]SystemCPUCpufreqStats, error) {
 		return nil, err
 	}
 
+	line, err := util.ReadFileNoStat(fs.sys.Path("devices/system/cpu/offline"))
+	if err != nil {
+		return nil, err
+	}
+
+	if strings.TrimSpace(string(line)) != "" {
+		offlineCPUs, err := parseCPURange(line)
+		if err != nil {
+			return nil, err
+		}
+
+		if len(offlineCPUs) > 0 {
+			err = filterOfflineCPUs(&offlineCPUs, &cpus)
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+
 	systemCpufreq := make([]SystemCPUCpufreqStats, len(cpus))
 	for i, cpu := range cpus {
 		cpuName := strings.TrimPrefix(filepath.Base(cpu), "cpu")
@@ -251,12 +313,12 @@ func (fs FS) IsolatedCPUs() ([]uint16, error) {
 		return nil, err
 	}
 
-	return parseIsolatedCPUs(isolcpus)
+	return parseCPURange(isolcpus)
 }
 
-func parseIsolatedCPUs(data []byte) ([]uint16, error) {
+func parseCPURange(data []byte) ([]uint16, error) {
 
-	var isolcpusInt = []uint16{}
+	var cpusInt = []uint16{}
 
 	for _, cpu := range strings.Split(strings.TrimRight(string(data), "\n"), ",") {
 		if cpu == "" {
@@ -277,7 +339,7 @@ func parseIsolatedCPUs(data []byte) ([]uint16, error) {
 			}
 
 			for i := startRange; i <= endRange; i++ {
-				isolcpusInt = append(isolcpusInt, uint16(i))
+				cpusInt = append(cpusInt, uint16(i))
 			}
 			continue
 		}
@@ -286,7 +348,7 @@ func parseIsolatedCPUs(data []byte) ([]uint16, error) {
 		if err != nil {
 			return nil, err
 		}
-		isolcpusInt = append(isolcpusInt, uint16(cpuN))
+		cpusInt = append(cpusInt, uint16(cpuN))
 	}
-	return isolcpusInt, nil
+	return cpusInt, nil
 }


Test patch:
diff --git a/sysfs/system_cpu_test.go b/sysfs/system_cpu_test.go
index afce58d..864e2ea 100644
--- a/sysfs/system_cpu_test.go
+++ b/sysfs/system_cpu_test.go
@@ -165,7 +165,7 @@ func TestIsolatedParsingCPU(t *testing.T) {
 	}
 	for _, params := range testParams {
 		t.Run("blabla", func(t *testing.T) {
-			res, err := parseIsolatedCPUs(params.in)
+			res, err := parseCPURange(params.in)
 			if !reflect.DeepEqual(res, params.res) {
 				t.Fatalf("should have %v result: got %v", params.res, res)
 			}
@@ -193,3 +193,24 @@ func TestIsolatedCPUs(t *testing.T) {
 		t.Errorf("Error not correct: want %v, have %v", nil, err)
 	}
 }
+
+func TestBinSearch(t *testing.T) {
+	var testParams = []struct {
+		elem  uint16
+		elems []uint16
+		res   bool
+	}{
+		{3, []uint16{1, 3, 5, 7, 9}, true},
+		{4, []uint16{1, 3, 5, 7, 9}, false},
+		{2, []uint16{}, false},
+	}
+
+	for _, param := range testParams {
+		res := binSearch(param.elem, &param.elems)
+
+		if res != param.res {
+			t.Fatalf("Result not correct: want %v, have %v", param.res, res)
+		}
+
+	}
+}
diff --git a/testdata/fixtures.ttar b/testdata/fixtures.ttar
index e9a8cd0..f0707fd 100644
--- a/testdata/fixtures.ttar
+++ b/testdata/fixtures.ttar
@@ -12729,6 +12729,11 @@ Lines: 1
 1,2-7,9
 Mode: 664
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+Path: fixtures/sys/devices/system/cpu/offline
+Lines: 1
+2,12-15
+Mode: 664
+# ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
 Directory: fixtures/sys/devices/system/node
 Mode: 775
 # ttar - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -