Support fetching PSI stats for cgroupv2 containers [dnm / carry] #3627
Conversation
Ah! they fixed it and now our CI is b0rked again. |
Fixed by #3628 |
libcontainer/cgroups/fs2/psi.go
Outdated
| return nil, fmt.Errorf("invalid PSI value: %q", f) | ||
| return nil, fmt.Errorf("invalid PSI value (%s): %q", kv[0], f) |
There was a problem hiding this comment.
This is redundant, since f already contains both the key and the value. So,
before:
invalid PSI value: "avg10=foo"
after:
invalid PSI value (avg10): "avg10=foo"
So, I'd remove this commit entirely.
There was a problem hiding this comment.
🤦♂️ you're right! Yup, will remove
I'll also squash all commits if things progress, but thought it would help review describing my steps
| @@ -21,6 +24,11 @@ type Stats struct { | |||
| NetworkInterfaces []*NetworkInterface `json:"network_interfaces"` | |||
| } | |||
|
|
|||
| type ( | |||
| PSIData = cgroups.PSIData | |||
There was a problem hiding this comment.
FWIW, not sure if we need the aliases at all; I need to look into history to understand why we did so for the other ones (separate types from libcontainer); perhaps we don't need those either.
It may have been to allow external consumer to use the "types" package without requiring libcontainer, but if that was the design, we already broke that.
If, on the other hand, nobody is expected to consume "types", we may not need all the duplicated definitions and we could just alias or use them directly
There was a problem hiding this comment.
We expect types to be consistent i.e. not depending on libcontainer internals, i..e consumer of runc events exect the format to be consistent even though we may have change libcontainer/cgroups for some reasons.
So some of the duplication is intentional, and some differences are also intentional i.e. camelCase vs snake_case.
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>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
So that no conversion is needed Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
Failure on centos stream 9; Wrapped for readability: So, the interesting bit may be that the error is on read; could it be that the func statPSI(dirPath string, file string, stats *cgroups.PSIStats) error {
f, err := cgroups.OpenFile(dirPath, file, os.O_RDONLY)
if err != nil {
if errors.Is(err, os.ErrNotExist) || errors.Is(err, syscall.ENOTSUP) {
// 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
return nil
}
return err
}
defer f.Close()
sc := bufio.NewScanner(f)
for sc.Scan() {
// etc
} |
|
Still not sure what's failing on Fedora (tests seem to pass, but then it marks it as failed (exit status 1)) |
Moves error handling to this function, so that the caller doesn't have to be aware of which errors to ignore. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This is the test case I've added in commit 57edce4659725c7c4a9727e7cca5e9bc27898572to make sure we don't panic if resources were not provided. |
Apparently the PSI pointer is never initialized so it's nil. Perhaps it's better to revert the struct to pointer-to-struct conversion patch (and ditch the |
|
@thaJeztah PTAL ^^^ (prev comment) |
Testing my suggestions from #3358 / dqminh#35