-
Notifications
You must be signed in to change notification settings - Fork 250
Fix parsing of hugetlb.<size>.events files #379
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| // Gets uint64 parsed content of single value cgroup stat file | |
| func getStatFileContentUint64(filePath string) uint64 { | |
| f, err := os.Open(filePath) | |
| if err != nil { | |
| return 0 | |
| } | |
| defer f.Close() | |
| // We expect an unsigned 64 bit integer, or a "max" string | |
| // in some cases. | |
| buf := make([]byte, 32) | |
| n, err := f.Read(buf) | |
| if err != nil { | |
| return 0 | |
| } | |
| trimmed := strings.TrimSpace(string(buf[:n])) | |
| if trimmed == "max" { | |
| return math.MaxUint64 | |
| } | |
| res, err := parseUint(trimmed, 10, 64) | |
| if err != nil { | |
| log.L.Errorf("unable to parse %q as a uint from Cgroup file %q", trimmed, filePath) | |
| return res | |
| } | |
| return res |
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
| trimmed := strings.TrimSpace(string(buf[:n])) | |
| if trimmed == "max" { | |
| return math.MaxUint64 | |
| } | |
| res, err := parseUint(trimmed, 10, 64) | |
| if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? |
AkihiroSuda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
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. |
Fixes #378
According to the docs: