On 12/5/19 12:50 PM, Bob Vandette wrote:
It may worth considering adding Metrics::getSwapLimit and
Metrics::getSwapUsage and move the computation to the implementation of
Metrics. Bob may have an opinion.
There was no any new input regarding this so I decided to leave it unchanged.
Sorry, I didn’t respond to this. Since the calculation required for
getFreeSwapSpaceSize requires retries
due to the access of multiple changing values, I think it’s best to leave
things as they are so the caller of
these methods understands the limitations of the API.
OK with me.
Also, the fact that swap size metrics include memory sizes is fully documented
in both the cgroup and docker
online documentation so it’s probably best to be consistent.
Also it seems correct for the memory related methods to check if
(containerMetrics != null && containerMetrics.getMemoryLimit() >= 0).
BTW what does it mean if limit == 0?
Per Docker docs the minimum allowed value for memory limit (--memory option)
is 4 megabytes.
And if memory limit is unset the return value is -1. Thus, in my understanding
the value 0 is only possible
if something went wrong while retrieving this metric.
That is true but shouldn’t you return -1 in that case?
I originally thought it was ok to fall back to the host data for 0 values but I
think its better to return unavailable (-1)
I think you might want to change all >= 0 to > 0 and return -1 if any of the
values are 0. This would be more consistent.
+1
The javadoc should be changed and returns -1 when it's unavailable and
the CSR should also be updated to reflect this. I'm sure Joe can
re-approve the CSR quickly when the fix is reviewed and approved.
You should only fall back to the original logic (host values) if container
values are set to unlimited.
+1
Mandy
