RFR: 8146115 - Improve docker container detection and resource configuration usage

[Bob Vandette]

David,  Thank you for taking the time and providing a detailed review of these changes.

Where I haven’t responded, I’ll update the implementation based on your comments.

> On Sep 26, 2017, at 1:19 AM, David Holmes <David.Holmes at oracle.com> wrote:
> 
> Hi Bob,
> 
> I think there are some high-level decisions to be made regarding Linux only or seemingly shared support for containers - see below - but overall I don't have any major issues. I can't comment on the details of the actual low-level cgroup queries etc.
> 
> Comments below ...
> 
> Bob writes:
>> Please review these changes that improve on docker container detection and the
>> automatic configuration of the number of active CPUs and total and free memory
>> based on the containers resource limitation settings and metric data files.
>> http://cr.openjdk.java.net/~bobv/8146115/webrev.00/ <http://cr.openjdk.java.net/~bobv/8146115/webrev.00/>
>> These changes are enabled with -XX:+UseContainerSupport.
> 
> If this is all confined to Linux only then this should be a linux-only flag and all the changes should be confined to linux code. No shared osContainer API is needed as it can be defined as a nested class of os::Linux, and will only be called from os_linux.cpp.

I received feedback on my other Container work where I was asked to make sure it was possible to support
other container technologies.  The addition of the shared osContainer API is to prepare for this and recognize that
this will eventually be supported other platforms.

> 
>> You can enable logging for this support via -Xlog:os+container=trace.
> 
> I'm not sure that is the right tagging for all situations. For example in os::Linux::available_memory you might log the actual amount for os+memory, regardless of whether containerized on not. You can of course also have container specific logging in the same function.
> 
>> Since the dynamic selection of CPUs based on cpusets, quotas and shares
>> may not satisfy every users needs, I’ve added an additional flag to allow the
>> number of CPUs to be overridden.  This flag is named -XX:ActiveProcessorCount=xx.
> 
> I would suggest that ActiveProcessorCount be constrained to being >1 - this is in line with our plans to get rid of AssumeMP/os::is_MP and always build in MP support. Otherwise a count of 1 today won't behave the same as a count of 1 in the future.
What if I return true for is_MP anytime ActiveProcessorCount is set.  I’d like to provide the ability of specifying a single processor.

> 
> Also you have defined this globally but only accounted for it on Linux. I think it makes sense to support this flag on all platforms (a generalization of AssumeMP). Otherwise it needs to be defined as a Linux-only flag in the pd_globals.hpp file
Good idea.

> 
> ---
> 
> src/os/linux/vm/os_linux.cpp
> 
> In os::Linux::available_memory you might want more container specific logging to track why you don't use container info even if containerized. It might help expose a misconfigured container.
> 
> In os::Linux::print_container_info I'd probably just print nothing if not containerized.
> 
> General style issue: we're not constrained to only declare local variables at the start of a block. You should be able to declare the variable at, or close to, the point of initialization.
> 
> Style issue:
> 
> 2121     if (i < 0) st->print("OSContainer::active_processor_count() failed");
> 2122     else
> 
> and elsewhere. Please move the st->print to its own line. Others may argue for always using blocks ({}) in if/else.

There doesn’t seem to be consistency on this issue.

> 
> 2128       st->print("OSContainer::memory_limit_in_bytes: %ld", j);
> 
> And elsewhere: %ld should be JLONG_FORMAT when printing a jlong.

I found a few places already where I needed to add this.  I’ll look for all of them.

> 
> 5024   // User has overridden the number of active processors
> 5025   if (!FLAG_IS_DEFAULT(ActiveProcessorCount)) {
> 5026     log_trace(os)("active_processor_count: "
> 5027                   "active processor count set by user : %d",
> 5028                   (int)ActiveProcessorCount);
> 5029     return ActiveProcessorCount;
> 5030   }
> 
> We don't normally check flags in runtime code like this - this will be executed on every call, and you will see that logging each time. This should be handled during initialization (os::Posix::init()? - if applying this flag globally) - with logging occurring once. The above should just reduce to:
> 
> if (ActiveProcessorCount > 0) {
>  return ActiveProcessorCount; // explicit user control of number of cpus
> }
> 
> Even then I do get concerned about having to always check for the least common cases before the most common one. :(

This is not in a highly used function so it should be ok.

> 
> ---
> 
> The osContainer_<os>.hpp files seem to be unnecessary as they are all empty.

I’ll remove them.  I wasn’t sure if there was a convention to move more of osContainer_linux.cpp -> osContainer_linux.hpp.

For example: class CgroupSubsystem

> 
> ---
> 
> osContainer.hpp:
> 
> The pd_* methods should be private.
> 
> You've exposed all of the potential cpu container functions as-if they might be used directly, rather than just internally when calculating available-processors. That's okay but you then need to document all the possible return values - in particular the distinction between -2, -1, 0 and > 0.
> 
> ---
> 
> osContainer.cpp
> 
>  41 bool OSContainer::is_containerized() {
>  42   if (!_is_initialized) OSContainer::init();
>  43   return _is_containerized;
>  44 }
> 
> As OSContainer::init() is called at VM initialization the above should simply assert that _is_initialized is true - else it means your call to ::init is in the wrong place and serves no purpose. That allows is_containerized to be trivially inlined and placed in the .hpp file.
> 
> ---
> 
> osContainer_linux.cpp
> 
> As Paul commented you should check whether you should be using os::strdup, os::malloc, os::free etc to interact with NMT correctly - and also respond to OOM appropriately.
> 
> 34 class CgroupSubsystem: CHeapObj<mtInternal> {
> 
> You defined this class as CHeapObj and added a destructor to free a few things, but I can't see where the instances of this class will themselves ever be freed

What’s the latest thinking on freeing CHeap Objects on termination?  Is it really worth wasting cpu cycles when our
process is about to terminate?  If not, I’ll just remove the destructors.

> 
> 62     void set_subsystem_path(char *cgroup_path) {
> 
> If this takes a "const char*" will it save you from casting string literals to "char*" elsewhere?

I tried several different ways of declaring the container accessor functions and
always ended up with warnings due to scanf not being able to validate arguments
since the format string didn’t end up being a string literal.  I originally was using templates
and then ended up with the macros.  I tried several different casts but could resolve the problem.

> 
> 170 GEN_CONTAINER_GET_INFO(jlong, jlong, "%ld")
> 
> %ld should be JLONG_FORMAT
> 
> 417   if (memlimit == 9223372036854771712)
> 
> Large constants should be defined using the CONST64 macro. And they should only be defined once as Paul suggested.
> 
> 485  * Algorythm:
> 
> Typo: -> Algorithm
> 
> 509   cpus = OSContainer::cpu_cpuset_cpus();
> 516     log_error(os,container)("Error getting cpuset_cpucount");
> 
> That seems to be the wrong error message for the function called.
> 
> 522     share_count = ceilf((float)share / 1024.0f);
> 
> The cast to float is not needed as you are dividing by a float.
> 
> 509   cpus = OSContainer::cpu_cpuset_cpus();
> 510   if (cpus != (char *)CONTAINER_ERROR) {
> 
> This is invalid code - you can't cast -2 to char*. The only way to report an error from a char* function is an actual error message, or else NULL.

I was trying to make error reporting consistent for all types.  I’ll change the error for strings to NULL.

> 
> 509   cpus = OSContainer::cpu_cpuset_cpus();
> 
> As you are inside a pd_* implementation, I don't think you want or need to call the public versions of the other OSContainer functions - you can just call the local pd_ variants directly. Nothing written at the OSContainer level should be allowed to affect what you calculate here inside your implementation.
> 
> 555  * Return the number of miliseconds per period
> 
> Typo: -> milliseconds
> 
> 525   else share_count = cpu_count;
> 
> Style (various locations): separate line and possibly {}
> 
> Thanks,
> David
> ——

Thanks,
Bob.

> 
>> Bob.
>