Use new filtered cgroups stats API#12901
Conversation
3a61f3d to
31c80bb
Compare
|
The cgroups update was handled by dependabot and merged; seems like you can drop that commit and rebase? |
In some spots we can get away with only reading a subset of the cgroup stats we are today. It would be reaaally nice for container stats in the cri plugin, but they're requested via the task API and we have no way to signify we only want a subset through this surface yet. We can still get some benefit in the stats collector and the existing sandbox stats where we only need mem and cpu. Signed-off-by: Danny Canter <danny@dcantah.dev>
31c80bb to
d7d7b10
Compare
|
@estesp done! |
fuweid
left a comment
There was a problem hiding this comment.
LGTM. Thanks!
Just have one small suggestion
| if err != nil { | ||
| return nil, fmt.Errorf("failed to load sandbox cgroup: %v: %w", cgroupPath, err) | ||
| } | ||
| stats, err := cg.Stat() |
There was a problem hiding this comment.
If we limit it to CPU and memory, we should rename the function as well—just in case other callers use it and assume it returns all information.
There was a problem hiding this comment.
That's fair. Technically on cg1 systems we'd still be getting all of the stats, and we still return this local struct today:
type cgroupMetrics struct {
v1 *cg1.Metrics
v2 *cg2.Metrics
}Would you want to change this to also just return mem and cpu values for both to be more clear?
There was a problem hiding this comment.
We can handle that in a follow-up. For now, this function is only called in one place.
In some spots we can get away with only reading a subset of the cgroup stats we are today. It would be reaaally nice for container stats in the cri plugin, but they're requested via the task API and we have no way to signify we only want a subset through this surface yet. We can still get some benefit in the stats collector and the existing sandbox stats where we only need mem and cpu.