Support fetching PSI stats for cgroupv2 containers by dqminh · Pull Request #3358 · opencontainers/runc

dqminh · 2022-01-28T17:26:01Z

This supports fetching PSI stats for cgroupv2 containers. We read the PSI metrics if they are available from:

cpu.pressure
memory.pressure
io.pressure

See more about PSI at https://www.kernel.org/doc/html/latest/accounting/psi.html

AkihiroSuda · 2022-01-31T02:48:21Z

+const examplePsiData = `some avg10=1.71 avg60=2.36 avg300=2.57 total=230548833
+full avg10=1.00 avg60=1.01 avg300=1.00 total=157622356`
+
+func TestStatCPUPsi(t *testing.T) {


Can we have integration tests too?

added one for runc events

nit: capitalize PSI

kolyshkin · 2022-01-31T19:40:19Z

Can we please have s/Psi/PSI/ since this is an acronym? I know we use things like Json before, but in the new code we can do better.

kolyshkin

LGTM

kolyshkin

a few typos in helpers.bash, otherwise LGTM

kolyshkin

LGTM

dqminh · 2022-02-24T11:40:51Z

ping @opencontainers/runc-maintainers

dqminh · 2022-03-22T11:48:14Z

ping @opencontainers/runc-maintainers again

kolyshkin · 2022-03-23T02:58:24Z

@dqminh can you please rebase? We have changed something in the tests and they need to be re-run.

dqminh · 2022-03-23T11:26:44Z

@kolyshkin thanks, rebase is done. I had to add # shellcheck disable=SC2030 for the integration test.

bobbypage · 2022-03-29T19:24:46Z

ping @kolyshkin, this would be great to get in, especially for use downstream in cAdvisor / k8s

kolyshkin · 2022-03-29T22:10:23Z

 	if err := statCpu(m.dirPath, st); err != nil && !os.IsNotExist(err) {
 		errs = append(errs, err)
 	}
+	// psi ( since kernel 4.20)


nit: extra space after (

kolyshkin · 2022-03-29T22:32:37Z

+}
+
+func parsePSIData(psi []string) (data cgroups.PSIData, err error) {
+	for i := 0; i < len(psi); i++ {


I think it'll be more readable to use

for _, f := range psi {

(and use f instead of psi[i] below).

kolyshkin · 2022-03-29T22:35:13Z

+	"github.com/opencontainers/runc/libcontainer/cgroups"
+)
+
+const examplePsiData = `some avg10=1.71 avg60=2.36 avg300=2.57 total=230548833


nit: capitalize PSI

kolyshkin

A few nits, otherwise looks good.

We read output from the following files if they exists: - cpu.pressure - memory.pressure - io.pressure Each are in format: ``` some avg10=0.00 avg60=0.00 avg300=0.00 total=0 full avg10=0.00 avg60=0.00 avg300=0.00 total=0 ``` Signed-off-by: Daniel Dao <dqminh89@gmail.com>

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

dqminh · 2022-05-13T16:16:36Z

@kolyshkin updated the nits, also added a fix to check for ENOTSUP when kernel compiled with PSI_DEFAULT_DISABLED, so reading *.pressure file return ENOTSUP as seen in https://cirrus-ci.com/task/4929935776153600

bobbypage · 2022-06-07T20:23:29Z

ping @kolyshkin for another round of reviews :)

kolyshkin · 2022-06-07T22:49:56Z

+	// open *.pressure file returns
+	// - ErrNotExist when kernel < 4.20 or CONFIG_PSI is disabled
+	// - ENOTSUP when we requires psi=1 in kernel command line to enable PSI support
+	if err := statPSI(m.dirPath, "cpu.pressure", &st.CpuStats.PSI); err != nil && !errors.Is(err, os.ErrNotExist) && !errors.Is(err, syscall.ENOTSUP) {


It's probably better to use unix.ENOTSUP from golang.org/x/sys/unix here (as syscall is considered an obsoleted package).

kolyshkin · 2022-06-07T23:00:36Z

+		psi)
+			# If PSI is not compiled in the kernel, the file will not exist.
+			# If PSI is compiled, but not enabled, read will fail with ENOTSUPP.
+			if [[ ! $(cat /sys/fs/cgroup/cpu.pressure) ]]; then


No need to use [[ here.

if ! cat /sys/fs/cgroup/cpu.pressure &>/dev/null; then

should be sufficient.

bobbypage · 2022-07-01T22:50:29Z

gentle ping to @dqminh regarding the updates mentioned. would be great to get this in so we can consume PSI metrics downstream in cAdvisor / k8s

AkihiroSuda · 2022-07-29T14:49:53Z

@kolyshkin Maybe we can address those issues after merging?

Furisto · 2022-09-08T15:31:14Z

Hey @dqminh this is a great PR. There has been no activity for some time so I am wondering if you are still interested? Otherwise I would be willing to take over and make the requested changes.

Furisto · 2022-09-19T09:28:58Z

@kolyshkin Given that this PR has stalled, how would you like to proceed? I can make the requested changes. Would be great if we could make PSI metrics available.

thaJeztah

Left some suggestions; I was coding along while writing, so I'll push a PR against @dqminh's branch for consideration (also happy to carry in a separate PR)

thaJeztah · 2022-10-06T13:04:57Z

+	Some PSIData `json:"some,omitempty"`
+	Full PSIData `json:"full,omitempty"`


Wondering if it would make sense to make these pointers, as it looks like that's how they're consumed in various places, plus it would allow parsePSIData to return nil, err (which may be slightly cleaner in in tent.

thaJeztah · 2022-10-06T13:26:07Z

+	for _, f := range psi {
+		kv := strings.SplitN(f, "=", 2)
+		if len(kv) != 2 {
+			return data, fmt.Errorf("invalid psi data: %q", f)


nit: these should probably use PSI (capitalised) as well

thaJeztah · 2022-10-06T13:27:25Z

+type PSIData struct {
+	Avg10  float64 `json:"avg10"`
+	Avg60  float64 `json:"avg60"`
+	Avg300 float64 `json:"avg300"`
+	Total  uint64  `json:"total"`
+}
+
+type PSIStats struct {
+	Some PSIData `json:"some,omitempty"`
+	Full PSIData `json:"full,omitempty"`
+}


Wondering if we could just make these an alias for cgroups.PSIData and cgroups.PSIStat. That way we wouldn't need the conversion function.

thaJeztah · 2022-10-06T13:50:13Z

+		case "avg10":
+			v, err := strconv.ParseFloat(kv[1], 64)
+			if err != nil {
+				return data, fmt.Errorf("invalid psi value: %q", f)
+			}
+			data.Avg10 = v
+		case "avg60":
+			v, err := strconv.ParseFloat(kv[1], 64)
+			if err != nil {
+				return data, fmt.Errorf("invalid psi value: %q", f)
+			}
+			data.Avg60 = v
+		case "avg300":


These are quire repetitive; perhaps we could combine them 🤔

thaJeztah · 2022-10-06T14:00:08Z

 type CpuStats struct {
 	CpuUsage       CpuUsage       `json:"cpu_usage,omitempty"`
 	ThrottlingData ThrottlingData `json:"throttling_data,omitempty"`
+	PSI            PSIStats       `json:"psi,omitempty"`


I think we need this to be a pointer for the omitempty to actually do something; https://go.dev/play/p/R68xTF5hreC

dqminh · 2022-10-11T10:03:46Z

@thaJeztah i'm really sorry about the delay. We have been running with the patch for so long (!) that i forgot it hasn't been merged.

I left a few comments in the PR on my fork. Generally i think we can try to make the refactoring of existing practices separately from the current change.

szuecs · 2022-11-04T21:56:36Z

@dqminh I read the comments and tried to fix everything and opened dqminh#37 . I have here issue I can't compile it and test wait for fetching containers since 15 minutes.
Dirty, because I did not commit and pushed changes when I tried to make.

% make
go build -trimpath "-buildmode=pie"  -tags "seccomp" -ldflags "-X main.gitCommit=v1.1.0-181-g6414629a-dirty -X main.version=1.1.0+dev " -o runc .
# pkg-config --cflags  -- libseccomp
Package libseccomp was not found in the pkg-config search path.
Perhaps you should add the directory containing `libseccomp.pc'
to the PKG_CONFIG_PATH environment variable
Package 'libseccomp', required by 'virtual:world', not found
pkg-config: exit status 1
make: *** [Makefile:33: runc] Error 2
zsh: exit 2     make

Tests run and fail for some reason, that might be problem of my system, not the code, but let's see what the test suite says.

I hope this helps to get it merged. :)

h-vetinari · 2022-11-07T01:23:52Z

Stray comment: As of linux 6.1 there's an interface called cgroup.pressure to switch PSI accounting on/off per cgroup. I suppose the code here will have to check if accounting is turned off.

szuecs · 2022-12-06T16:13:57Z

FYI: I created #3679 as follow up to get this merged

…ainers#3358](opencontainers#3358)

dqminh mentioned this pull request Jan 28, 2022

Expose Pressure Stall Information as metrics google/cadvisor#3052

Closed

AkihiroSuda added area/cgroupv2 impact/changelog kind/feature labels Jan 31, 2022

AkihiroSuda added this to the 1.2.0 milestone Jan 31, 2022

AkihiroSuda reviewed Jan 31, 2022

View reviewed changes

Comment thread libcontainer/cgroups/stats.go

AkihiroSuda reviewed Jan 31, 2022

View reviewed changes

dqminh force-pushed the psi branch from 42389a3 to 9c1fe51 Compare January 31, 2022 11:45

kolyshkin reviewed Jan 31, 2022

View reviewed changes

Comment thread tests/integration/events.bats Outdated

dqminh force-pushed the psi branch 2 times, most recently from 31e4197 to 5699010 Compare February 3, 2022 17:53

kolyshkin approved these changes Feb 7, 2022

View reviewed changes

kolyshkin requested a review from AkihiroSuda February 8, 2022 03:49

kolyshkin reviewed Feb 8, 2022

View reviewed changes

Comment thread tests/integration/helpers.bash

kolyshkin reviewed Feb 8, 2022

View reviewed changes

Comment thread tests/integration/helpers.bash Outdated

kolyshkin requested changes Feb 8, 2022

View reviewed changes

dqminh force-pushed the psi branch from 5699010 to d13b351 Compare February 15, 2022 10:41

kolyshkin previously approved these changes Feb 16, 2022

View reviewed changes

dqminh dismissed kolyshkin’s stale review via a07e8f8 March 23, 2022 09:58

dqminh force-pushed the psi branch 2 times, most recently from a07e8f8 to 521a41d Compare March 23, 2022 10:42

dqminh mentioned this pull request Mar 23, 2022

Support for exposing PSI metrics google/cadvisor#3083

Closed

kolyshkin reviewed Mar 29, 2022

View reviewed changes

dqminh force-pushed the psi branch 4 times, most recently from 1f36e91 to c495b2b Compare May 13, 2022 15:56

dqminh added 2 commits May 13, 2022 16:58

integration test for PSI metrics in runc events

6414629

Signed-off-by: Daniel Dao <dqminh89@gmail.com>

dqminh force-pushed the psi branch from c495b2b to 6414629 Compare May 13, 2022 15:58

kolyshkin reviewed Jun 7, 2022

View reviewed changes

thaJeztah requested changes Oct 6, 2022

View reviewed changes

This was referenced Oct 6, 2022

Review suggestions dqminh/runc#35

Open

Support fetching PSI stats for cgroupv2 containers [dnm / carry] #3627

Closed

lukasmrtvy mentioned this pull request Oct 18, 2022

Podman support. google/cadvisor#3021

Merged

szuecs mentioned this pull request Dec 6, 2022

Support fetching PSI stats for cgroupv2 containers - takeover #3679

Closed

john-liuqiming pushed a commit to cloud-native-isv/runc that referenced this pull request Mar 30, 2023

merge PR [Support fetching PSI stats for cgroupv2 containers opencont…

db39fa8

…ainers#3358](opencontainers#3358)

kolyshkin mentioned this pull request Jun 13, 2023

libct/cg/stats: support PSI for cgroup v2 #3900

Merged

lifubang closed this in #3900 Jul 14, 2023

		Some PSIData `json:"some,omitempty"`
		Full PSIData `json:"full,omitempty"`

Conversation

dqminh commented Jan 28, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented Jan 31, 2022

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

dqminh commented Feb 24, 2022

Uh oh!

dqminh commented Mar 22, 2022

Uh oh!

kolyshkin commented Mar 23, 2022

Uh oh!

dqminh commented Mar 23, 2022

Uh oh!

bobbypage commented Mar 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

dqminh commented May 13, 2022

Uh oh!

bobbypage commented Jun 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bobbypage commented Jul 1, 2022

Uh oh!

AkihiroSuda commented Jul 29, 2022

Uh oh!

Furisto commented Sep 8, 2022

Uh oh!

Furisto commented Sep 19, 2022

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dqminh commented Oct 11, 2022

Uh oh!

szuecs commented Nov 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

h-vetinari commented Nov 7, 2022

Uh oh!

bobbypage commented Jun 7, 2022 •

edited

Loading

szuecs commented Nov 4, 2022 •

edited

Loading

szuecs commented Dec 6, 2022 •

edited

Loading