Determining CPU online state correctly using online CPUs list

Signed-off-by: Maciej "Iwan" Iwanowski <[email protected]>

machine/machine.go

@@ -16,18 +16,15 @@
 package machine
 
 import (
-	"bytes"
 	"fmt"
 	"io/ioutil"
 	"os"
 	"path"
-	"path/filepath"
 	"regexp"
-	"strconv"
-	"strings"
-
 	// s390/s390x changes
 	"runtime"
+	"strconv"
+	"strings"
 
 	info "github.com/google/cadvisor/info/v1"
 	"github.com/google/cadvisor/utils"
@@ -54,9 +51,6 @@ var (
 	maxFreqFile        = "/sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq"
 )
 
-const sysFsCPUCoreID = "core_id"
-const sysFsCPUPhysicalPackageID = "physical_package_id"
-const sysFsCPUTopology = "topology"
 const memTypeFileName = "dimm_mem_type"
 const sizeFileName = "size"
 
@@ -66,7 +60,7 @@ func GetPhysicalCores(procInfo []byte) int {
 	if numCores == 0 {
 		// read number of cores from /sys/bus/cpu/devices/cpu*/topology/core_id to deal with processors
 		// for which 'core id' is not available in /proc/cpuinfo
-		numCores = getUniqueCPUPropertyCount(cpuBusPath, sysFsCPUCoreID)
+		numCores = sysfs.GetUniqueCPUPropertyCount(cpuBusPath, sysfs.CPUCoreID)
 	}
 	if numCores == 0 {
 		klog.Errorf("Cannot read number of physical cores correctly, number of cores set to %d", numCores)
@@ -80,7 +74,7 @@ func GetSockets(procInfo []byte) int {
 	if numSocket == 0 {
 		// read number of sockets from /sys/bus/cpu/devices/cpu*/topology/physical_package_id to deal with processors
 		// for which 'physical id' is not available in /proc/cpuinfo
-		numSocket = getUniqueCPUPropertyCount(cpuBusPath, sysFsCPUPhysicalPackageID)
+		numSocket = sysfs.GetUniqueCPUPropertyCount(cpuBusPath, sysfs.CPUPhysicalPackageID)
 	}
 	if numSocket == 0 {
 		klog.Errorf("Cannot read number of sockets correctly, number of sockets set to %d", numSocket)
@@ -236,39 +230,6 @@ func parseCapacity(b []byte, r *regexp.Regexp) (uint64, error) {
 	return m * 1024, err
 }
 
-// Looks for sysfs cpu path containing given CPU property, e.g. core_id or physical_package_id
-// and returns number of unique values of given property, exemplary usage: getting number of CPU physical cores
-func getUniqueCPUPropertyCount(cpuBusPath string, propertyName string) int {
-	pathPattern := cpuBusPath + "cpu*[0-9]"
-	sysCPUPaths, err := filepath.Glob(pathPattern)
-	if err != nil {
-		klog.Errorf("Cannot find files matching pattern (pathPattern: %s),  number of unique %s set to 0", pathPattern, propertyName)
-		return 0
-	}
-	uniques := make(map[string]bool)
-	for _, sysCPUPath := range sysCPUPaths {
-		onlinePath := filepath.Join(sysCPUPath, "online")
-		onlineVal, err := ioutil.ReadFile(onlinePath)
-		if err != nil {
-			klog.Warningf("Cannot determine CPU %s online state, skipping", sysCPUPath)
-			continue
-		}
-		onlineVal = bytes.TrimSpace(onlineVal)
-		if len(onlineVal) == 0 || onlineVal[0] != 49 {
-			klog.Warningf("CPU %s is offline, skipping", sysCPUPath)
-			continue
-		}
-		propertyPath := filepath.Join(sysCPUPath, sysFsCPUTopology, propertyName)
-		propertyVal, err := ioutil.ReadFile(propertyPath)
-		if err != nil {
-			klog.Errorf("Cannot open %s, number of unique %s  set to 0", propertyPath, propertyName)
-			return 0
-		}
-		uniques[string(propertyVal)] = true
-	}
-	return len(uniques)
-}
-
 // getUniqueMatchesCount returns number of unique matches in given argument using provided regular expression
 func getUniqueMatchesCount(s string, r *regexp.Regexp) int {
 	matches := r.FindAllString(s, -1)

machine/topology_test.go

@@ -18,14 +18,16 @@ import (
 	"encoding/json"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"reflect"
 	"sort"
 	"testing"
 
+	"github.com/stretchr/testify/assert"
+
 	info "github.com/google/cadvisor/info/v1"
 	"github.com/google/cadvisor/utils/sysfs"
 	"github.com/google/cadvisor/utils/sysfs/fakesysfs"
-	"github.com/stretchr/testify/assert"
 )
 
 func TestPhysicalCores(t *testing.T) {
@@ -40,18 +42,26 @@ func TestPhysicalCores(t *testing.T) {
 }
 
 func TestPhysicalCoresReadingFromCpuBus(t *testing.T) {
-	cpuBusPath = "./testdata/"           // overwriting package variable to mock sysfs
-	testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without core id
+	origCPUBusPath := cpuBusPath
+	defer func() {
+		cpuBusPath = origCPUBusPath
+	}()
+	cpuBusPath = "./testdata/sysfs_cpus/" // overwriting package variable to mock sysfs
+	testfile := "./testdata/cpuinfo_arm"  // mock cpuinfo without core id
 
 	testcpuinfo, err := ioutil.ReadFile(testfile)
 	assert.Nil(t, err)
 	assert.NotNil(t, testcpuinfo)
 
 	numPhysicalCores := GetPhysicalCores(testcpuinfo)
-	assert.Equal(t, 2, numPhysicalCores)
+	assert.Equal(t, 1, numPhysicalCores)
 }
 
 func TestPhysicalCoresFromWrongSysFs(t *testing.T) {
+	origCPUBusPath := cpuBusPath
+	defer func() {
+		cpuBusPath = origCPUBusPath
+	}()
 	cpuBusPath = "./testdata/wrongsysfs" // overwriting package variable to mock sysfs
 	testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without core id
 
@@ -75,6 +85,10 @@ func TestSockets(t *testing.T) {
 }
 
 func TestSocketsReadingFromCpuBus(t *testing.T) {
+	origCPUBusPath := cpuBusPath
+	defer func() {
+		cpuBusPath = origCPUBusPath
+	}()
 	cpuBusPath = "./testdata/wrongsysfs" // overwriting package variable to mock sysfs
 	testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without physical id
 
@@ -87,7 +101,14 @@ func TestSocketsReadingFromCpuBus(t *testing.T) {
 }
 
 func TestSocketsReadingFromWrongSysFs(t *testing.T) {
-	cpuBusPath = "./testdata/"           // overwriting package variable to mock sysfs
+	path, err := filepath.Abs("./testdata/sysfs_cpus/")
+	assert.NoError(t, err)
+
+	origCPUBusPath := cpuBusPath
+	defer func() {
+		cpuBusPath = origCPUBusPath
+	}()
+	cpuBusPath = path                    // overwriting package variable to mock sysfs
 	testfile := "./testdata/cpuinfo_arm" // mock cpuinfo without physical id
 
 	testcpuinfo, err := ioutil.ReadFile(testfile)

utils/sysfs/sysfs.go

@@ -15,8 +15,6 @@
 package sysfs
 
 import (
-	"bytes"
-	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -37,9 +35,21 @@ const (
 	ppcDevTree   = "/proc/device-tree"
 	s390xDevTree = "/etc" // s390/s390x changes
 
-	coreIDFilePath    = "/topology/core_id"
-	packageIDFilePath = "/topology/physical_package_id"
-	meminfoFile       = "meminfo"
+	meminfoFile = "meminfo"
+
+	sysFsCPUTopology = "topology"
+
+	// CPUPhysicalPackageID is a physical package id of cpu#. Typically corresponds to a physical socket number,
+	// but the actual value is architecture and platform dependent.
+	CPUPhysicalPackageID = "physical_package_id"
+	// CPUCoreID is the CPU core ID of cpu#. Typically it is the hardware platform's identifier
+	// (rather than the kernel's). The actual value is architecture and platform dependent.
+	CPUCoreID = "core_id"
+
+	coreIDFilePath    = "/" + sysFsCPUTopology + "/core_id"
+	packageIDFilePath = "/" + sysFsCPUTopology + "/physical_package_id"
+
+	// memory size calculations
 
 	cpuDirPattern  = "cpu*[0-9]"
 	nodeDirPattern = "node*[0-9]"
@@ -335,25 +345,145 @@ func (fs *realSysFs) GetSystemUUID() (string, error) {
 	}
 }
 
-func (fs *realSysFs) IsCPUOnline(dir string) bool {
-	cpuPath := fmt.Sprintf("%s/online", dir)
-	content, err := ioutil.ReadFile(cpuPath)
+func (fs *realSysFs) IsCPUOnline(cpuPath string) bool {
+	onlinePath, err := filepath.Abs(cpuPath + "/../online")
 	if err != nil {
-		pathErr, ok := err.(*os.PathError)
-		if ok {
-			if errors.Is(pathErr.Unwrap(), os.ErrNotExist) && isZeroCPU(dir) {
-				return true
-			}
-		}
-		klog.Warningf("unable to read %s: %s", cpuPath, err.Error())
+		klog.V(1).Infof("Unable to get absolute path for %s", cpuPath)
 		return false
 	}
-	trimmed := bytes.TrimSpace(content)
-	return len(trimmed) == 1 && trimmed[0] == 49
+
+	// Quick check to determine if file exists: if it does not then kernel CPU hotplug is disabled and all CPUs are online.
+	_, err = os.Stat(onlinePath)
+	if err != nil && os.IsNotExist(err) {
+		return true
+	}
+	if err != nil {
+		klog.V(1).Infof("Unable to stat %s: %s", onlinePath, err)
+	}
+
+	cpuID, err := getCPUID(cpuPath)
+	if err != nil {
+		klog.V(1).Infof("Unable to get CPU ID from path %s: %s", cpuPath, err)
+		return false
+	}
+
+	isOnline, err := isCPUOnline(onlinePath, cpuID)
+	if err != nil {
+		klog.V(1).Infof("Unable to get online CPUs list: %s", err)
+		return false
+	}
+	return isOnline
 }
 
-func isZeroCPU(dir string) bool {
-	regex := regexp.MustCompile("cpu([0-9]*)")
+func getCPUID(dir string) (uint16, error) {
+	regex := regexp.MustCompile("cpu([0-9]+)")
 	matches := regex.FindStringSubmatch(dir)
-	return len(matches) == 2 && matches[1] == "0"
+	if len(matches) == 2 {
+		id, err := strconv.Atoi(matches[1])
+		if err != nil {
+			return 0, err
+		}
+		return uint16(id), nil
+	}
+	return 0, fmt.Errorf("can't get CPU ID from %s", dir)
+}
+
+// isCPUOnline is copied from github.com/opencontainers/runc/libcontainer/cgroups/fs and modified to suite cAdvisor
+// needs as Apache 2.0 license allows.
+// It parses CPU list (such as: 0,3-5,10) into a struct that allows to determine quickly if CPU or particular ID is online.
+// see: https://github.com/opencontainers/runc/blob/ab27e12cebf148aa5d1ee3ad13d9fc7ae12bf0b6/libcontainer/cgroups/fs/cpuset.go#L45
+func isCPUOnline(path string, cpuID uint16) (bool, error) {
+	fileContent, err := ioutil.ReadFile(path)
+	if err != nil {
+		return false, err
+	}
+	if len(fileContent) == 0 {
+		return false, fmt.Errorf("%s found to be empty", path)
+	}
+
+	cpuList := strings.TrimSpace(string(fileContent))
+	for _, s := range strings.Split(cpuList, ",") {
+		splitted := strings.SplitN(s, "-", 3)
+		switch len(splitted) {
+		case 3:
+			return false, fmt.Errorf("invalid values in %s", path)
+		case 2:
+			min, err := strconv.ParseUint(splitted[0], 10, 16)
+			if err != nil {
+				return false, err
+			}
+			max, err := strconv.ParseUint(splitted[1], 10, 16)
+			if err != nil {
+				return false, err
+			}
+			if min > max {
+				return false, fmt.Errorf("invalid values in %s", path)
+			}
+			for i := min; i <= max; i++ {
+				if uint16(i) == cpuID {
+					return true, nil
+				}
+			}
+		case 1:
+			value, err := strconv.ParseUint(s, 10, 16)
+			if err != nil {
+				return false, err
+			}
+			if uint16(value) == cpuID {
+				return true, nil
+			}
+		}
+	}
+
+	return false, nil
+}
+
+// Looks for sysfs cpu path containing given CPU property, e.g. core_id or physical_package_id
+// and returns number of unique values of given property, exemplary usage: getting number of CPU physical cores
+func GetUniqueCPUPropertyCount(cpuBusPath string, propertyName string) int {
+	absCPUBusPath, err := filepath.Abs(cpuBusPath)
+	if err != nil {
+		klog.Errorf("Cannot make %s absolute", cpuBusPath)
+		return 0
+	}
+	pathPattern := absCPUBusPath + "/cpu*[0-9]"
+	sysCPUPaths, err := filepath.Glob(pathPattern)
+	if err != nil {
+		klog.Errorf("Cannot find files matching pattern (pathPattern: %s),  number of unique %s set to 0", pathPattern, propertyName)
+		return 0
+	}
+	onlinePath, err := filepath.Abs(cpuBusPath + "/online")
+	if err != nil {
+		klog.V(1).Infof("Unable to get absolute path for %s", cpuBusPath+"/../online")
+		return 0
+	}
+
+	if err != nil {
+		klog.V(1).Infof("Unable to get online CPUs list: %s", err)
+		return 0
+	}
+	uniques := make(map[string]bool)
+	for _, sysCPUPath := range sysCPUPaths {
+		cpuID, err := getCPUID(sysCPUPath)
+		if err != nil {
+			klog.V(1).Infof("Unable to get CPU ID from path %s: %s", sysCPUPath, err)
+			return 0
+		}
+		isOnline, err := isCPUOnline(onlinePath, cpuID)
+		if err != nil && !os.IsNotExist(err) {
+			klog.V(1).Infof("Unable to determine CPU online state: %s", err)
+			continue
+		}
+		if !isOnline && !os.IsNotExist(err) {
+			continue
+		}
+		propertyPath := filepath.Join(sysCPUPath, sysFsCPUTopology, propertyName)
+		propertyVal, err := ioutil.ReadFile(propertyPath)
+		if err != nil {
+			klog.Warningf("Cannot open %s, assuming 0 for %s of CPU %d", propertyPath, propertyName, cpuID)
+			propertyVal = []byte("0")
+		}
+		uniques[string(propertyVal)] = true
+	}
+	return len(uniques)
 }

utils/sysfs/sysfs_notx86.go

@@ -0,0 +1,19 @@
+// +build !x86
+
+// Copyright 2021 Google Inc. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package sysfs
+
+var isX86 = false