Add cpuset statistics #2408

piowag · 2020-05-15T12:47:59Z

Adds statistics for cpuset cgroup

Signed-off-by: Piotr Wagner piotr.wagner@intel.com

kolyshkin · 2020-05-16T02:00:40Z

@piowag you need to squash your commits

kolyshkin · 2020-05-16T02:03:08Z

libcontainer/cgroups/fs/cpuset.go

+	}
+
+	for _, s := range strings.Split(fileContent, ",") {
+		if strings.Contains(s, "-") {


you can avoid this check and go right to splitting. If len(splitted) == 1 it means there is no dash.

2 or 3 could also mean no dash

libcontainer/cgroups/fs/cpuset.go

kolyshkin · 2020-05-16T02:04:46Z

libcontainer/cgroups/fs/cpuset.go

+				return err
+			}
+			if min > max {
+				return fmt.Errorf("Couldn't parse %s", filepath.Join(path, filename))


Perpahs invalid values in %s?

libcontainer/cgroups/fs/cpuset.go

libcontainer/cgroups/fs/cpuset_test.go

kolyshkin · 2020-05-16T02:20:29Z

libcontainer/cgroups/fs/cpuset.go

+	if err != nil {
+		return err
+	}
+	schedRelaxDomainLevel, err := fscommon.GetCgroupParamInt(path, "cpuset.sched_relax_domain_level")


Are you sure you want to return an error if any of these files do not exist? What if some older kernel do not provide some?

Changed stat gathering method to try to gather as much as possible instead of quitting at first error

libcontainer/cgroups/fs/cpuset.go

libcontainer/cgroups/fscommon/utils.go

libcontainer/cgroups/fs/cpuset_test.go

libcontainer/cgroups/fs/cpuset.go

kolyshkin

Some comments are not yet acted upon. I do not insist on fixing every nit, but at least explain why you prefer it the way you did it.

kolyshkin · 2020-06-16T03:27:27Z

@piowag please let us know if you still can / want to work on this one

piowag · 2020-06-16T12:58:39Z

Sorry, I was absent a little. Yes I still can and want to work on this one.

libcontainer/cgroups/fs/cpuset.go

libcontainer/cgroups/fs/cpuset_test.go

kolyshkin

LGTM

kolyshkin · 2020-07-07T20:30:06Z

One thing I don't like about this patch is it requires all the cpuset files being present. On my system (kernel 5.6) I do not have, for example, cpuset.mem_hardwall. Does that mean that GetStats() will now return an error?

For example, (* BlkioGroup).GetStat ignores the ENOENT, returning no errors if some files are not present.

kolyshkin

Need to figure out whether to ignore ENOENT

AkihiroSuda · 2020-12-14T05:44:17Z

@opencontainers/runc-maintainers PTAL. This has been open so long.

kolyshkin · 2020-12-16T01:12:17Z

libcontainer/cgroups/fs/cpuset_test.go

+		SchedLoadBalance:      1,
+		SchedRelaxDomainLevel: -1}
+	if !reflect.DeepEqual(expectedStats, actualStats.CpusetStats) {
+		t.Fatalf("Expected Cpuset stats usage %#v but found %#v\n",


nit: no need for \n here.

kolyshkin · 2020-12-16T01:14:13Z

libcontainer/cgroups/fs/cpuset_test.go

+			err := cpuset.GetStats(helper.CgroupPath, &actualStats)
+
+			if err != nil {
+				t.Errorf(fmt.Sprintf("test case '%s' failed unexpectedly: '%s'", failCase.desc, err))


nit: no need to use fmt.Sprintf here, as t.Errorf is perfectly capable of handling %-style arguments.

Perhaps use sub-tests for these cases, then it would not be needed to include the name of the failed test in the error?

kolyshkin · 2020-12-16T01:15:23Z

libcontainer/cgroups/fs/cpuset_test.go

+			err := cpuset.GetStats(helper.CgroupPath, &actualStats)
+
+			if err == nil {
+				t.Errorf(fmt.Sprintf("test case '%s' failed to return expected error", failCase.desc))


kolyshkin · 2020-12-16T01:15:58Z

libcontainer/cgroups/fscommon/utils.go

@@ -72,6 +72,25 @@ func GetCgroupParamUint(path, file string) (uint64, error) {
 	return res, nil
 }

+// GetCgroupParamInt reads a single int64 value from specidfied cgroup file.


typo: s/specidfied/specified/

kolyshkin

Left a few minor comments, overall LGTM

thaJeztah · 2020-12-16T01:24:25Z

types/events.go

@@ -12,6 +12,7 @@ type Event struct {
 // stats is the runc specific stats structure for stability when encoding and decoding stats.
 type Stats struct {
 	CPU               Cpu                 `json:"cpu"`
+	Cpuset            Cpuset              `json:"cpuset"`


should probably be "CPUSet" ?

Suggested change

Cpuset Cpuset `json:"cpuset"`

CPUSet CPUSet `json:"cpuset"`

thaJeztah · 2020-12-16T01:24:43Z

types/events.go

@@ -70,6 +71,20 @@ type Cpu struct {
 	Throttling Throttling `json:"throttling,omitempty"`
 }

+type Cpuset struct {


Suggested change

type Cpuset struct {

type CPUSet struct {

thaJeztah · 2020-12-16T01:25:25Z

types/events.go

@@ -70,6 +71,20 @@ type Cpu struct {
 	Throttling Throttling `json:"throttling,omitempty"`
 }

+type Cpuset struct {
+	Cpus                  []uint16 `json:"cpus,omitempty"`
+	CpuExclusive          uint64   `json:"cpu_exclusive"`


Suggested change

CpuExclusive uint64 `json:"cpu_exclusive"`

CPUExclusive uint64 `json:"cpu_exclusive"`

thaJeztah · 2020-12-16T01:25:55Z

types/events.go

@@ -70,6 +71,20 @@ type Cpu struct {
 	Throttling Throttling `json:"throttling,omitempty"`
 }

+type Cpuset struct {
+	Cpus                  []uint16 `json:"cpus,omitempty"`


Suggested change

Cpus []uint16 `json:"cpus,omitempty"`

CPUs []uint16 `json:"cpus,omitempty"`

Not 100% sure if it should be lowercase or uppercase "s"

thaJeztah · 2020-12-16T01:37:45Z

libcontainer/cgroups/stats.go

@@ -121,6 +148,7 @@ type HugetlbStats struct {

 type Stats struct {
 	CpuStats    CpuStats    `json:"cpu_stats,omitempty"`
+	CpusetStats CpusetStats `json:"cpuset_stats,omitempty"`


Ideally

Suggested change

CpusetStats CpusetStats `json:"cpuset_stats,omitempty"`

CPUSetStats CPUSetStats `json:"cpuset_stats,omitempty"`

But I can see that being inconsistent with the other field; is that something that can still be changed, or would that break many things?

thaJeztah · 2020-12-16T01:38:21Z

libcontainer/cgroups/stats.go

+type CpusetStats struct {
+	// List of the physical numbers of the CPUs on which processes
+	// in that cpuset are allowed to execute
+	Cpus []uint16 `json:"cpus,omitempty"`


Suggested change

Cpus []uint16 `json:"cpus,omitempty"`

CPUs []uint16 `json:"cpus,omitempty"`

thaJeztah · 2020-12-16T01:38:42Z

libcontainer/cgroups/stats.go

+	// in that cpuset are allowed to execute
+	Cpus []uint16 `json:"cpus,omitempty"`
+	// cpu_exclusive flag
+	CpuExclusive uint64 `json:"cpu_exclusive"`


Suggested change

CpuExclusive uint64 `json:"cpu_exclusive"`

CPUExclusive uint64 `json:"cpu_exclusive"`

thaJeztah · 2020-12-16T01:40:20Z

libcontainer/cgroups/fs/cpuset_test.go

+			err := cpuset.GetStats(helper.CgroupPath, &actualStats)
+
+			if err != nil {
+				t.Errorf(fmt.Sprintf("test case '%s' failed unexpectedly: '%s'", failCase.desc, err))


Perhaps use sub-tests for these cases, then it would not be needed to include the name of the failed test in the error?

kolyshkin · 2020-12-17T00:10:05Z

@Creatone thanks for working on this! Can you please squash the commits?

kolyshkin

LGTM

AkihiroSuda · 2021-01-12T03:24:44Z

GitHub web ui says "Required statuses must pass before merging".

Seems to need to be rebased again to pick the recent CI changes, sorry

AkihiroSuda · 2021-01-15T03:58:20Z

@cyphar @kolyshkin Can we merge this to master directly without waiting for CI retrigger?

Creatone · 2021-01-18T19:20:56Z

@AkihiroSuda @kolyshkin

AkihiroSuda

Thanks!

thaJeztah · 2021-01-19T16:40:28Z

events.go

+	s.CPUSet.CPUs = cg.CPUSetStats.CPUs
+	s.CPUSet.CPUExclusive = cg.CPUSetStats.CPUExclusive
+	s.CPUSet.Mems = cg.CPUSetStats.Mems
+	s.CPUSet.MemExclusive = cg.CPUSetStats.MemExclusive
+	s.CPUSet.MemHardwall = cg.CPUSetStats.MemHardwall
+	s.CPUSet.MemoryMigrate = cg.CPUSetStats.MemoryMigrate
+	s.CPUSet.MemoryPressure = cg.CPUSetStats.MemoryPressure
+	s.CPUSet.MemorySpreadPage = cg.CPUSetStats.MemorySpreadPage
+	s.CPUSet.MemorySpreadSlab = cg.CPUSetStats.MemorySpreadSlab
+	s.CPUSet.SchedLoadBalance = cg.CPUSetStats.SchedLoadBalance
+	s.CPUSet.SchedRelaxDomainLevel = cg.CPUSetStats.SchedRelaxDomainLevel


Looks like these types are compatible, and this could be simplified to;

Suggested change

s.CPUSet.CPUs = cg.CPUSetStats.CPUs

s.CPUSet.CPUExclusive = cg.CPUSetStats.CPUExclusive

s.CPUSet.Mems = cg.CPUSetStats.Mems

s.CPUSet.MemExclusive = cg.CPUSetStats.MemExclusive

s.CPUSet.MemHardwall = cg.CPUSetStats.MemHardwall

s.CPUSet.MemoryMigrate = cg.CPUSetStats.MemoryMigrate

s.CPUSet.MemoryPressure = cg.CPUSetStats.MemoryPressure

s.CPUSet.MemorySpreadPage = cg.CPUSetStats.MemorySpreadPage

s.CPUSet.MemorySpreadSlab = cg.CPUSetStats.MemorySpreadSlab

s.CPUSet.SchedLoadBalance = cg.CPUSetStats.SchedLoadBalance

s.CPUSet.SchedRelaxDomainLevel = cg.CPUSetStats.SchedRelaxDomainLevel

s.CPUSet = types.CPUSet(cg.CPUSetStats)

Tricky :) Thanks!

Co-authored-by: Piotr Wagner <piotr.wagner@intel.com> Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>

thaJeztah

LGTM (not a maintainer)

thanks!

kolyshkin

LGTM

piowag force-pushed the cpuset_getstats branch from 7acae16 to 9721496 Compare May 15, 2020 14:16

kolyshkin reviewed May 16, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 16, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Show resolved Hide resolved

kolyshkin reviewed May 16, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 16, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 16, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset_test.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 16, 2020

View reviewed changes

piowag force-pushed the cpuset_getstats branch 3 times, most recently from 253558c to c76d065 Compare May 26, 2020 11:52

kolyshkin reviewed May 30, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 30, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 30, 2020

View reviewed changes

libcontainer/cgroups/fscommon/utils.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 30, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset_test.go Outdated Show resolved Hide resolved

kolyshkin reviewed May 30, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin requested changes Jun 10, 2020

View reviewed changes

piowag force-pushed the cpuset_getstats branch from c76d065 to e08d195 Compare June 16, 2020 12:46

piowag requested a review from kolyshkin June 17, 2020 17:58

kolyshkin reviewed Jun 22, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset.go Outdated Show resolved Hide resolved

kolyshkin reviewed Jun 22, 2020

View reviewed changes

libcontainer/cgroups/fs/cpuset_test.go Outdated Show resolved Hide resolved

piowag force-pushed the cpuset_getstats branch from e08d195 to f6c0dd9 Compare June 25, 2020 08:22

kolyshkin approved these changes Jul 7, 2020

View reviewed changes

kolyshkin requested changes Jul 7, 2020

View reviewed changes

piowag force-pushed the cpuset_getstats branch from f6c0dd9 to fba52dd Compare July 31, 2020 07:13

Creatone dismissed AkihiroSuda’s stale review via a4b3075 December 7, 2020 13:08

Creatone force-pushed the cpuset_getstats branch from 8fe4901 to a4b3075 Compare December 7, 2020 13:08

AkihiroSuda previously approved these changes Dec 8, 2020

View reviewed changes

kolyshkin reviewed Dec 16, 2020

View reviewed changes

kolyshkin requested changes Dec 16, 2020

View reviewed changes

thaJeztah requested changes Dec 16, 2020

View reviewed changes

Creatone dismissed AkihiroSuda’s stale review via 00840bf December 16, 2020 11:05

Creatone force-pushed the cpuset_getstats branch from 00840bf to 7f30901 Compare December 17, 2020 10:04

kolyshkin previously approved these changes Jan 11, 2021

View reviewed changes

AkihiroSuda previously approved these changes Jan 12, 2021

View reviewed changes

Creatone dismissed stale reviews from AkihiroSuda and kolyshkin via 3b0d1be January 18, 2021 19:20

Creatone force-pushed the cpuset_getstats branch from 7f30901 to 3b0d1be Compare January 18, 2021 19:20

AkihiroSuda previously approved these changes Jan 19, 2021

View reviewed changes

thaJeztah reviewed Jan 19, 2021

View reviewed changes

Implement GetStat for cpuset cgroup.

ab27e12

Co-authored-by: Piotr Wagner <piotr.wagner@intel.com> Signed-off-by: Paweł Szulik <pawel.szulik@intel.com>

Creatone dismissed AkihiroSuda’s stale review via ab27e12 January 19, 2021 17:02

Creatone force-pushed the cpuset_getstats branch from 3b0d1be to ab27e12 Compare January 19, 2021 17:02

AkihiroSuda approved these changes Jan 20, 2021

View reviewed changes

thaJeztah approved these changes Jan 20, 2021

View reviewed changes

kolyshkin approved these changes Jan 21, 2021

View reviewed changes

kolyshkin merged commit d9751a9 into opencontainers:master Jan 21, 2021

	CpuExclusive uint64 `json:"cpu_exclusive"`
	CPUExclusive uint64 `json:"cpu_exclusive"`

	Cpus []uint16 `json:"cpus,omitempty"`
	CPUs []uint16 `json:"cpus,omitempty"`

	CpusetStats CpusetStats `json:"cpuset_stats,omitempty"`
	CPUSetStats CPUSetStats `json:"cpuset_stats,omitempty"`

Add cpuset statistics #2408

Add cpuset statistics #2408

Conversation

piowag commented May 15, 2020

kolyshkin commented May 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Jun 16, 2020

piowag commented Jun 16, 2020

kolyshkin left a comment

Choose a reason for hiding this comment

kolyshkin commented Jul 7, 2020

kolyshkin left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kolyshkin commented Dec 17, 2020

kolyshkin left a comment

Choose a reason for hiding this comment

AkihiroSuda commented Jan 12, 2021

AkihiroSuda commented Jan 15, 2021

Creatone commented Jan 18, 2021

AkihiroSuda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

kolyshkin left a comment

Choose a reason for hiding this comment