Read cpu.stat regardless if controller enabled. #348
Conversation
|
Change looks good after the one comment is fixed |
|
I see this PR branch also has some merge commits in it; we generally use a rebase flow to keep a clean commit history; when updating, could you do a rebase and squash the commits, so that there's only a single commit in the PR? Let me know if you need help / instructions doing so! |
The unified hierarchy provides the cpu.stat file for every cgroup, regardless if the CPU controller is enabled (in fact, setting the systemd property CPUAccounting=True does not enable this controller because of this fact). It provides the usage_usec, user_usec, and system_usec by default. Instead of reading the stat for each enabled controller (CPU and memory), just attempt to read them each time the Stat() function is called. Attempting to read the memory.stat file even if memory accounting is not enabled seems insignificant (some other files always have a read attempt, such as memory.current), and eliminates finding and looping over enabled controllers. Resolves: containerd#347 Signed-off-by: Jackson McKay <jackjaymckay@gmail.com>
|
Fix added and rebase complete, should only have one commit now. Thanks. |
There was a problem hiding this comment.
Left some thoughts / comments; not sure if I'm right though!
I did a quick draft while I was reviewing, and pushed it as a PR against your fork to show what it'd look like (if that approach is correct, merging it should make it show up in this PR as a separate commit; feel free to squash it though (probably better to);
@dcantah any thoughts? (also please double-check if my logic is correct)
| if !os.IsNotExist(err) { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
So, while looking at this PR (and I think some of this is an existing issue) I wonder if it would make sense to skip assigning values in situations where the file does not exist, or if we should skip all of that if it's not there, and explicitly construct an empty &stats.MemoryStat{}.
| @@ -630,6 +613,13 @@ func (c *Manager) Stat() (*stats.Metrics, error) { | |||
| SwapMaxUsage: getStatFileContentUint64(filepath.Join(c.path, "memory.swap.peak")), | |||
There was a problem hiding this comment.
I'm not entirely sure though if these can be present independently from memory.stat (perhaps someone else knows if that's something that we need to account for)
The unified hierarchy provides the cpu.stat file for every cgroup, regardless if the CPU controller is enabled (in fact, setting the systemd property CPUAccounting=True does not enable this controller because of this fact). It provides the usage_usec, user_usec, and system_usec by default. Instead of reading the stat for each enabled controller (CPU and memory), just attempt to read them each time the Stat() function is called.
Attempting to read the memory.stat file even if memory accounting is not enabled seems insignificant (some other files always have a read attempt, such as memory.current), and eliminates finding and looping over enabled controllers.
resolves #347