Strengthen handling of unavailable cgroup stats#21094
Strengthen handling of unavailable cgroup stats#21094jasontedor merged 6 commits intoelastic:masterfrom jasontedor:unavailable-cgroup
Conversation
jasontedor
added
review
:Core/Infra/Stats
|
should we just throw FNF and catch on a top level as we do in other cases? |
On some systems, cgroups will be available but not configured. And in some cases, cgroups will be configured, but not for the subsystems that we are expecting (e.g., cpu and cpuacct). This commit strengthens the handling of cgroup stats on such systems.
jasontedor
changed the title
@s1monw We can throw if |
|
@jason ok can we check this higher up to skip the entire query of this subsystem |
Sorry, I wasn't completely clear in my previous comment. We do already check if the subsystem is available or not, but it might still be the case that the necessary mounts to |
This commit checks for the existence of cgroup stats upfront rather than allowing leniency in the cgroup methods.
|
looks great I would be even happier if we had a static method that says |
|
@jasontedor I am ok with pushing this since some builds are failing but I think we can do better with a nice util method? |
This commit refactors the logic for the upfront checking whether the relevant cgroup stats are available or not, and adds a note to the docs.
I pushed 4c8a2cc. |
This commit adds Javadocs for a convenience method OsProbe#areCgroupStatsAvailable.
s1monw
left a comment
s1monw
left a comment
There was a problem hiding this comment.
left some questions, LGTM otherwise
| final String[] controllers = matcher.group(1).split(","); | ||
| for (final String controller : controllers) { | ||
| controllerMap.put(controller, matcher.group(2)); | ||
| if (lines.isEmpty()) { |
There was a problem hiding this comment.
isn't this obsolete now? we don't return empty form this method?
| * @return {@code true} if the stats are available, otherwise | ||
| * {@code false} | ||
| */ | ||
| @SuppressForbidden(reason = "access /proc/self/cgroup, /sys/fs/cgroup/cpu, and /sys/fs/cgroup/cpuacct") |
| getCgroupCpuAcctCpuCfsPeriodMicros(cpuControlGroup), | ||
| getCGroupCpuAcctCpuCfsQuotaMicros(cpuControlGroup), | ||
| getCgroupCpuAcctCpuStat(cpuControlGroup)); | ||
| if (!areCgroupStatsAvailable()) { |
| @Override | ||
| public void writeTo(final StreamOutput out) throws IOException { | ||
| out.writeString(cpuAcctControlGroup); | ||
| out.writeOptionalString(cpuAcctControlGroup); |
There was a problem hiding this comment.
you require it non-null above but here it's optional?
This commit removes some leniency when reading cgroups that was leftover after a previous refactoring.
This commit fixes a serialization bug in OsStats when serializing Cgroup instances. Namely, the control groups were made non-optional in a previous refactoring but the commit that cleaned this up left one case of writing a string as optional.
|
Thanks @s1monw, I've responded to your feedback. |
|
LGTM 👍 |
On some systems, cgroups will be available but not configured. And in some cases, cgroups will be configured, but not for the subsystems that we are expecting (e.g., cpu and cpuacct). This commit strengthens the handling of cgroup stats on such systems. Relates #21094
|
Thanks @s1monw. |
3 participants
On some systems, cgroups will be available but not configured. And in
some cases, cgroups will be configured, but not for the subsystems that
we are expecting (e.g., cpu and cpuacct). This commit strengthens the
handling of cgroup stats on such systems.
Relates #21029