Incorrect Broker CPU utilization values on Kubernetes

[kyguy]

A bug was introduced with the original work done for the broker CPU utilization reporting for Kubernetes mode [1] The formula used to calculate CPU utilization for a Kafka broker running in a container,

 cpuUtl = (getProcessCpuLoad() * logicalProcessorsOfNode) / (cpuQuota / cpuPeriod)

can produce values that exceed the allowed interval of [0.0, 1.0]. Note that this formula was intended to adjust the values from ((com.sun.management.OperatingSystemMXBean) ManagementFactory.getOperatingSystemMXBean()).getProcessCpuLoad() for container boundaries. However, under certain conditions, the formula breaks and incorrectly produces values greater than 1.0.

For example, for the following values:

getProcessCpuLoad() = 1.0
logicalProcessorsOfNode = 4
cpuQuota = 200000
cpuPeriod = 100000

cpuUtl = (1.0 * 4) / (200000 / 100000)
cpuUtil = 2.0

These incorrect CPU utilization values cause Cruise Control to overestimate CPU usage and prevent Cruise Control from executing partition rebalances when the cluster has CPU resources available.

This ContainerMetricUtils class [2] was originally created as a substitute for the getSystemCpuLoad() [3] as we waited for the backports to make getSystemCpuLoad() "container aware" for OpenJDK 8, 11, 13, and 14. Since those backports have now long been completed, I suggest it is time we replace the ContainerMetricUtils class with getSystemCpuLoad() as we originally planned!

Let me know what you think! If you agree, feel free to assign to me and I will send in a fix!

cc @amuraru

[1] #1277
[2]

cruise-control/cruise-control-metrics-reporter/src/main/java/com/linkedin/kafka/cruisecontrol/metricsreporter/metric/ContainerMetricUtils.java

Lines 90 to 108 in 4e5927b

	public static double getContainerProcessCpuLoad(double cpuUtil) throws IOException {
	int logicalProcessorsOfNode = getAvailableProcessors();
	double cpuQuota = getCpuQuota();
	if (cpuQuota == NO_CPU_QUOTA) {
	return cpuUtil;
	}
	// Get the number of CPUs of a node that can be used by the operating environment
	double cpuLimit = cpuQuota / getCpuPeriod();
	// Get the minimal number of CPUs needed to achieve the reported CPU utilization
	double cpus = cpuUtil * logicalProcessorsOfNode;
	/* Calculate the CPU utilization of a JVM process with respect to the operating environment.
	* Since the operating environment will only use the CPU resources allocated by CGroups,
	* it will always be that: cpuLimit >= cpus and the result is in the [0.0,1.0] interval.
	*/
	return cpus / cpuLimit;
	}

[3] https://bugs.openjdk.java.net/browse/JDK-8226575

[kyguy]

On second thought, the formula should still hold and provide correct values between the interval of [0.0, 1.0] so long as the behavior of getProcessCpuLoad() is still providing values with respect to the number of logical CPUs available to the node instead of the container.

In the example above, getProcessCpuLoad() shouldn't be able to provide a value greater than 0.5 when the cpuQuota = 200000 and cpuPeriod = 100000 since the JVM would be restricted to use at most 2 CPUs worth of cycles. However, getProcessCpuLoad() could exceed 0.5 if this method had been patched to be calculated against the logical CPUs available to the container it was run inside of.

I'll continue investigating. In any case, we should still switch to use the new "container aware" Java methods exclusively.

[1] https://bugs.openjdk.java.net/browse/JDK-8226575

[amuraru]

On second thought, the formula should still hold and provide correct values between the interval of [0.0, 1.0] so long as the behavior of getProcessCpuLoad() is still providing values with respect to the number of logical CPUs available to the node instead of the container.

https://bugs.openjdk.java.net/browse/JDK-8269851 changed the behaviour making getProcessCpuLoad container aware as well

[kyguy]

Thanks @amuraru that is exactly what I was looking for, getProcessCpuLoad has been patched! So when running Cruise Control on Kubernetes we want to set cruise.control.metrics.reporter.kubernetes.mode to:

• true when using unpatched Java versions where getProcessCpuLoad is not "container aware".
• false when using patched Java versions where getProcessCpuLoad is "container aware".

I can't think of many situations where someone would want to use unpatched Java version so maybe it makes sense that we deprecate cruise.control.metrics.reporter.kubernetes.mode now that is not needed but I guess we can wait for the maintainers to see what they think!

[amuraru]

I can't think of many situations where someone would want to use unpatched Java version so maybe it makes sense that we deprecate cruise.control.metrics.reporter.kubernetes.mode now that is not needed but I guess we can wait for the maintainers to see what they think!

I agree - what we could do to improve this is to have cc provide an "official" docker image that can be consumed by downstream users that would take care of selecting the "right" jvm version as a base image