Fix parsing of hugetlb.<size>.events files#379
Conversation
|
@voidbar You can refer https://github.com/containerd/project/blob/main/CONTRIBUTING.md#sign-your-work to add the DCO to fix the Project Checks failure |
574c655 to
7417fb9
Compare
|
@akhilerm done |
|
/cc @dcantah |
cgroup2/utils.go
Outdated
| events := make(map[string]uint64) | ||
| err := readKVStatsFile(path, "hugetlb."+pagesize+".events", events) | ||
| if err != nil { | ||
| log.L.WithError(err).Errorf("failed to read hugetlb events for %s", filepath.Join(path, "hugetlb."+pagesize+".events")) | ||
| } |
There was a problem hiding this comment.
Should this fail hard if the file doesn't exist, or ignore non-existing (os.IsNotExist(err)) ? (Not sure if it's guaranteed to exist?)
There was a problem hiding this comment.
From the docs it can be absent
A read-only flat-keyed file which **exists on non-root** cgroups.
we can wrap this logic in existence check and fail hard in case we failed to parse it
There was a problem hiding this comment.
Yes, it looks like getStatFileContentUint64 (used for non-KV-files) is fairly relaxed on errors, and at most logs an error;
Lines 266 to 293 in f1e92d8
To keep aligned with that behavior, perhaps a getKVStatsFileContentUint64 utility that handles things similarly would make sense; it could get the name of the property looked for ("max") passed as an argument, which would allow returning early (instead of reading all properties potentialy present in the file);
// Gets uint64 parsed content of key-value cgroup stat file
func getKVStatsFileContentUint64(filePath string, propertyName string) uint64 {
f, err := os.Open(filepath.Join(path, file))
if err != nil {
return 0
}
defer f.Close()
s := bufio.NewScanner(f)
for s.Scan() {
name, value, err := parseKV(s.Text())
if name == propertyName {
if err != nil {
log.L.Errorf("unable to parse %q as a uint from Cgroup file %q: %w", propertyName, filePath, err)
return 0
}
return value
}
}
return s.Err()
}Perhaps a log for failing to open the file for reasons other than the file not being found would be good (the above was based on the getStatFileContentUint64 code, which doesn't do so, but ... maybe should?).
Do we know if these KV-files could also have magic values (like max)? I see the other utility has some special handling for that before attempting to parse it as an uint;
Lines 282 to 288 in f1e92d8
There was a problem hiding this comment.
hugetlb..max
Set/show the hard limit of “hugepagesize” hugetlb usage. The default value is “max”. It exists for all the cgroup except root.
the max value is used for hugetlb..max file @thaJeztah
There was a problem hiding this comment.
There was a problem hiding this comment.
I implemented a simple getKVStatsFileContentUint64. I think it strikes the balance between being too strict and too lenient. It follows the same assumptions as getStatsFileContentUint64 and provides only the value required.
|
@akhilerm @thaJeztah can we take this to main? |
Signed-off-by: Tomer Lev <me@tomerlev.com>
|
Why do I have still this issue on my Ubuntu server. Client: Docker |
|
@Charlesm54 containerd v2.2.0 does not yet have this fix; it's back ported to be included in a v2.2.1 patch release, but it's not released yet. |
Container Runtime Interface (CRI) - Redact all query parameters in CRI error logs (containerd/containerd#12546) Image Distribution - Fix image defaults on Darwin to usable configuration (containerd/containerd#12544) - Fix possible panic from WithMediaTypeKeyPrefix (containerd/containerd#12516) Runtime - Update runc binary to v1.3.4 (containerd/containerd#12593) - Fix parsing of hugetlb..events files (containerd/cgroups#379) Signed-off-by: David Mandy <smallprogramzhusir@gmail.com>
Fixes #378
According to the docs: