Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for detecting docker container #2047

Merged
merged 1 commit into from
Feb 27, 2018

Conversation

ashu-mehra
Copy link
Contributor

@ashu-mehra ashu-mehra commented Nov 23, 2017

Add support in port library for detecting if we are running in
docker container or not.
This is required for cgroup related APIs in port library,
such as omrsysinfo_cgroup_get_memlimit, to work properly.
This is because when running in a docker container,
the cgroups listed in /proc/self/cgroup do not match the cgroups
of the process in the container. /proc/self/cgroup actually
shows the cgroups as seen from the host.

Eg if a process is running in a docker container,
its /proc/PID/cgroup on docker will show something like:

$ cat /proc/8364/cgroup
11:cpuset:/docker/<container id>
10:perf_event:/docker/<container id>
9:memory:/dockera/<container id>
...

but the actual cgroup of the process would be "/".

Once we figure out that we are running in docker container,
then cgroup APIs in port library should use "/" as cgroup name
for reading resource limits.

Note that as and when docker containers start supporting
cgroup namespace, then above mentioned mismatch would also be fixed.
Following issues and PRs are related to adding support for
cgroup namespace in docker container:
opencontainers/runc#1184
opencontainers/runtime-spec#62
opencontainers/runtime-spec#66

There is no direct mechanism to detect if we are running in
docker container.
Most reliable mechanism that I have come across is
by checking the cgroup for PID 1 (by reading /proc/1/cgroup file)
for all subsystems. If all cgroups are "/", then it indicates the
process is running on host system.
Else we conclude it to be running in a docker container.
Note that this mechanism will break when docker containers start
supporting cgroup namespace.

Signed-off-by: Ashutosh Mehra [email protected]

@ashu-mehra
Copy link
Contributor Author

@DanHeidinga can you please review this.

@DanHeidinga
Copy link
Contributor

@ashu-mehra Can you address the travis build failures before I review?

@ashu-mehra
Copy link
Contributor Author

Sorry, last minute changes resulted in those failures. I have fixed them. It should pass now.

@charliegracie
Copy link
Contributor

genie-omr build all

@ashu-mehra
Copy link
Contributor Author

@DanHeidinga can you please review this.

Copy link
Contributor

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ashu-mehra This is a little hard to review as it mixes together two things:

  1. fixing a bug(?) around detecting and handling non-v1 cgroup support
  2. adding two new apis for container name and type

I'm not sure I follow the need for (2). Can you clarify why these apis are useful and what port library consumers would do with this info?

omrsysinfo_get_container_name(struct OMRPortLibrary *portLibrary, OMRContainerType containerType)
{
Assert_PRT_true(OMR_CONTAINER_NONE == containerType);
return "none";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return NULL on unsupported platforms while this returns none

@@ -1012,6 +1012,13 @@ typedef struct OMROSKernelInfo {
#define OMR_CGROUP_SUBSYSTEM_MEMORY ((uint64_t)0x2)
#define OMR_CGROUP_SUBSYSTEM_ALL (OMR_CGROUP_SUBSYSTEM_CPU | OMR_CGROUP_SUBSYSTEM_MEMORY)

typedef enum OMRContainerType {
OMR_CONTAINER_NONE,
OMR_CONTAINER_DOCKER,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does DOCKER imply any runc-style container runtime or does it mean exactly Docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked runc container and it displays same behavior with respect to /proc/pid/cgroup file. So I believe the detection mechanism should work for any runc-style container, not just Docker.
Since Docker is most popular system at the moment, I named it so.
Do you want to rename it as something like OMR_CONTAINER_RUNC?


#if defined(LINUX) && !defined(OMRZTPF)
{
const char *envValue = getenv("CONTAINER_TYPE");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick google search doesn't show any references to an env var called "CONTAINER_TYPE". I don't see any reference to this in the API doc for this function and I'm not sure why this api looks for that env var. Can you expand on the why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a fallback mechanism for current detection logic. As I mentioned in the commit message, container detection logic is flaky and can break in future. If that happens, then we can use this env variable to indicate to runtime that its running in a container.
For example, we can set this env variable in Dockerfile to enable container awareness in the runtime.

}

_end:
if (0 == rc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you refactor this to have a single return point?

@@ -3431,6 +3472,7 @@ readCgroupFile(struct OMRPortLibrary *portLibrary, int pid, OMRCgroupEntry **cgr
requiredSize = portLibrary->str_printf(portLibrary, NULL, (uint32_t)-1, "/proc/%d/cgroup", pid);
Assert_PRT_true(requiredSize <= PATH_MAX);
portLibrary->str_printf(portLibrary, cgroup, sizeof(cgroup), "/proc/%d/cgroup", pid);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code here writes into cgroup but the loop below ignores it and uses ROOT_CGROUP. Is this intentional?

Copy link
Contributor Author

@ashu-mehra ashu-mehra Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cgroup at this point is storing path /proc/pid/cgroup. Same var is also used for storing cgroup name by parsing /proc/pid/cgroup file. This may be the cause of confusion. I will update code to rename the variable.
Use of ROOT_CGROUP is intentional as we don't want to use the cgroup name we get from /proc/pid/cgroup file when 'useRootCgroup` is set.


if (TRUE == useRootCgroup) {
cgroupToUse = ROOT_CGROUP;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this leave us doing a bunch of unnecessary work before we ignore it to use the ROOT_CGROUP here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to anyway parse the /proc/pid/cgroup file to get list of the subsystems.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment in the code to this method to explain that?

* For any other cgroup name, assume we are in a 'docker' container.
*/
rc = readCgroupFile(portLibrary, 1, FALSE, &cgEntryList, NULL);
if (0 != rc) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goto is unnecessary if the != is changed to == and the code below is moved into the if block.

omrthread_monitor_enter(cgroupEntryListMonitor);
if (NULL == PPG_cgroupEntryList) {
if (OMR_CONTAINER_DOCKER == PPG_containerType) {
rc = readCgroupFile(portLibrary, getpid(), TRUE, &PPG_cgroupEntryList, &PPG_cgroupSubsystemsAvailable);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A local variable for the TRUE vs FALSE would be clearer rather than duplicating the readCgroupFile call

omrthread_monitor_exit(cgroupEntryListMonitor);
if (0 != rc) {
goto _end;
if (isCgroupV1Available(portLibrary)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I read this code correctly that the cgroup detection is only possible on cgroup v1 systems?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For cgroup v2, the file system type mounted on /sys/fs/cgroup is different. The function isCgroupV1Available() is currently only handling cgroup v1, therefore I named it so.
As and when we add support for cgroup v2, I believe we can rename the function then.

}
_end:
if (NULL != cgEntryList) {
freeCgroupEntries(portLibrary, cgEntryList);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems allocation heavy for the information being gathered. Can this be done without allocating and freeing the cgroup list?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks possible. Let me see if I can refactor it to make it light weight.

@ashu-mehra
Copy link
Contributor Author

I'm not sure I follow the need for (2). Can you clarify why these apis are useful and what port library consumers would do with this info?

I thought it may come handy if runtimes want to display information about the container they are running in for diagnostic purpose. For example, OpenJ9 can display the container type in javacore?

@ashu-mehra
Copy link
Contributor Author

@DanHeidinga I have updated the code as per the comments. Can you please take a look.

@ashu-mehra
Copy link
Contributor Author

This is a little hard to review as it mixes together two things:

  1. fixing a bug(?) around detecting and handling non-v1 cgroup support

I should point out that the change is not to handle non-v1 cgroup support. Changes are mainly to make cgroup support work properly in (docker) container by detecting that we are in the container environment.

@ashu-mehra
Copy link
Contributor Author

@DanHeidinga can you please review the updated change set.

@mstoodle
Copy link
Contributor

another ping for @DanHeidinga who maybe missed the first one due to conference circuit prep distractions :) .

@DanHeidinga
Copy link
Contributor

Thanks @mstoodle and @ashu-mehra - My goal is to have reviewed this by end of week (hold me to it!)

Copy link
Contributor

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this PR together @ashu-mehra - it's important area for OMR. My apologies for the delayed review on this.

The cgroup detection and parsing updates look reasonable. The two other apis added to this PR don't really fit with it's purpose.

They should be handled in a separate PR and I would argue should wait to be standardized until there is a second container system to ensure that the API is flexible enough to apply to both Docker (cgroups) and whatever the other container systems would be.

if (0 != rc) {
portLibrary->error_set_last_error(portLibrary, errno, OMRPORT_ERROR_SYSINFO_SYS_FS_CGROUP_STATFS_FAILED);
result = FALSE;
goto _end;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goto _end; is unnecessary

} else if (TMPFS_MAGIC != buf.f_type) {
portLibrary->error_set_last_error_with_message_format(portLibrary, OMRPORT_ERROR_SYSINFO_SYS_FS_CGROUP_TMPFS_NOT_MOUNTED, "tmpfs is not mounted on " OMR_CGROUP_V1_MOUNT_POINT);
result = FALSE;
goto _end;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goto _end; is unnecessary


if (TRUE == useRootCgroup) {
cgroupToUse = ROOT_CGROUP;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment in the code to this method to explain that?

/** see @ref omrsysinfo_get_container_type "omrsysinfo_get_container_type"*/
int32_t ( *sysinfo_get_container_type)(struct OMRPortLibrary *portLibrary, OMRContainerType *containerType);
/** see @ref omrsysinfo_get_container_name "omrsysinfo_get_container_name"*/
const char * ( *sysinfo_get_container_name)(struct OMRPortLibrary *portLibrary, OMRContainerType containerType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two APIs don't belong in this changeset. Given the VM only supports detecting linux cgroups or nothing, we don't have enough examples to generalize to a good portable api.

I would rather forgo these APIs now and add them when we have second (or third) container system to work with. That will result in a better designed api that is easier to maintain longterm.

@@ -256,6 +256,18 @@ struct {
#if defined(LINUX)

#define OMR_CGROUP_V1_MOUNT_POINT "/sys/fs/cgroup"
#define ROOT_CGROUP "/"

/* Following is the description of /proc/<pid>/cgroup copied from 'man' page for proc:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't copy from the man page for proc as the documentation is under GPLv2: http://man7.org/linux/man-pages/man5/proc.5.license.html

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the comment to put it in my own words instead of copying from man page.

@@ -299,6 +311,12 @@ struct {
{ "memory", OMR_CGROUP_SUBSYSTEM_MEMORY }
};

const char *OMRContainerNames[OMR_CONTAINER_NUM_CONTAINERS] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per comment above, this belongs in a separate changeset or should wait until we have more examples of container systems to build the api on.

goto _end;
}

_end:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

label can be removed.

* @return 0 on success, otherwise negative error code
*/
static int32_t
getContainerType(struct OMRPortLibrary *portLibrary, OMRContainerType *containerType)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this method from this PR.

if (0 != rc) {
goto _end;
if (OMR_CONTAINER_DOCKER == PPG_containerType) {
useRootCgroup = TRUE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be reworked when the container type api is moved out.

@@ -3706,13 +3817,14 @@ int32_t
omrsysinfo_cgroup_get_memlimit(struct OMRPortLibrary *portLibrary, uint64_t *limit)
{
int32_t rc = OMRPORT_ERROR_SYSINFO_CGROUP_UNSUPPORTED_PLATFORM;

Assert_PRT_true(NULL != limit);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the function prototype to indicate limit cannot be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@ashu-mehra
Copy link
Contributor Author

@DanHeidinga I have updated the change set to remove sysinfo_get_container_type and sysinfo_get_container_name APIs.
Other comments are also addressed. Please review the updated change set.

@@ -902,6 +902,7 @@ omrsysinfo_cgroup_are_subsystems_enabled(struct OMRPortLibrary *portLibrary, uin
* omrsysinfo_cgroup_enable_limits() before calling this function.
* When the fuction returns OMRPORT_ERROR_SYSINFO_CGROUP_UNSUPPORTED_PLATFORM,
* value of *limits is unspecified.
* Note that 'limit' parameter should not be NULL.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: should not -> must not

This is a bit of a nitpick but must is a stronger requirement than should. Since the code will assert it is not null, must is a better choice.

@@ -1653,3 +1653,4 @@ omrsysinfo_cgroup_get_memlimit(struct OMRPortLibrary *portLibrary, uint64_t *lim
{
return OMRPORT_ERROR_SYSINFO_CGROUP_UNSUPPORTED_PLATFORM;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you remove this file from the PR? It doesn't have any real changes.

if (NULL == cgroupFile) {
rc = portLibrary->error_set_last_error(portLibrary, errno, OMRPORT_ERROR_SYSINFO_PROCESS_CGROUP_FILE_FOPEN_FAILED);
goto _end;
}

while (0 == feof(cgroupFile)) {
char cgroup[PATH_MAX];
char subsystems[1024];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment that points to the subsystemNames array to indicate why 1024 is likely sufficient?

break;
} else if (3 != rc) {
rc = portLibrary->error_set_last_error_with_message_format(portLibrary, OMRPORT_ERROR_SYSINFO_PROCESS_CGROUP_FILE_READ_FAILED, "unexpected format of /proc/1/cgroup file");
goto _end;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return 0 on success, otherwise negative error code

This may exit the method with a positive, non-zero return value contrary to the function contract.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rc is reassigned return value of error_set_last_error_with_message_format which is negative error code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed that - sorry.

goto _end;
}

if (strcmp(ROOT_CGROUP, cgroup)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make this a boolean comparison and explicitly add the != 0 check to the if statement? The port library (or more specifically, new code) tries to ensure that all comparisons result in boolean values.

* @return 0 on success, otherwise negative error code
*/
static int32_t
isRunningInDocker(struct OMRPortLibrary *portLibrary, BOOLEAN *inDocker)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be isRunningInContainer? It appears to generically check for a container runtime, not docker specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isRunningInContainer is also fine. The mechanism works for runc based containers, not specific to docker.

* Checks if the process is running inside docker container
*
* @param[in] portLibrary pointer to OMRPortLibrary
* @param[out] inDocker pointer to BOOLEAN which on successful return indicates if the process is running in docker container or not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not return TRUE if running in a container and FALSE for not / errors?

The current approach means user code needs to check two things:

  BOOLEAN inDocker = false;
   if (0 == isRunningInDocker(portLib, &inDocker)) {
      if (inDocker == TRUE) {
        /* take action */
      }
    }

Is there a meaningful distinction between error and not in docker?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am inclined to make this distinction between error and not in docker.
Its caller omrsysinfo_cgroup_is_system_available should not attempt to do any processing if any error is encountered.
If we treat error case same as 'not in docker' case, then we may end up in trouble if the error happened when its actually running in docker. In such case it would attempt to read non-existing cgroup control files for determining resource limits.
To be on safer side, if any error occurs, better to not use cgroup limits at all and fallback to regular behavior.

}
omrthread_monitor_enter(cgroupEntryListMonitor);
if (NULL == PPG_cgroupEntryList) {
BOOLEAN useRootCgroup = FALSE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is useRootCgroup required or can the code pass inDocker instead?

Copy link
Contributor Author

@ashu-mehra ashu-mehra Feb 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use inDocker (or inContainer) instead.

Add support in port library for detecting if we are running in
docker container or not.
This is required for cgroup related APIs in port library,
such as omrsysinfo_cgroup_get_memlimit, to work properly.
This is because when running in a docker container,
the cgroups listed in /proc/self/cgroup do not match the cgroups
of the process in the container. /proc/self/cgroup actually
shows the cgroups as seen from the host.

Eg if a process is running in a docker container,
its /proc/PID/cgroup on docker will show something like:

$ cat /proc/8364/cgroup
11:cpuset:/docker/<container id>
10:perf_event:/docker/<container id>
9:memory:/dockera/<container id>
...

but the actual cgroup of the process would be "/".

Once we figure out that we are running in docker container,
then cgroup APIs in port library would use "/" as cgroup name
for reading resource limits.

Note that as and when docker containers start supporting
cgroup namespace, then above mentioned mismatch would also be fixed.
Following issues and PRs are related to adding support for
cgroup namespace in docker container:
opencontainers/runc#1184
opencontainers/runtime-spec#62
opencontainers/runtime-spec#66

There is no direct mechanism to detect if we are running in
docker container.
Most reliable mechanism that I have come across is
by checking the cgroup for PID 1 (by reading /proc/1/cgroup file)
for all subsystems. If all cgroups are "/", then it indicates the
process is running on host system.
Else we conclude it to be running in a docker container.
Note that this mechanism will break when docker containers start
supporting cgroup namespace.

Signed-off-by: Ashutosh Mehra <[email protected]>
@ashu-mehra
Copy link
Contributor Author

@DanHeidinga change set is updated. Please review.

@DanHeidinga
Copy link
Contributor

@ashu-mehra I've applied these changes to an OpenJ9 build and added some debug prints but I'm not seeing any output. Is there a commandline flag / env var I need to set to enable cgroup detection?

goto _end;
rc = isRunningInContainer(portLibrary, &inContainer);
if (0 != rc) {
goto _end;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a level 1 tracepoint here? Assuming the error condition is rare, we should capture some diagnostics to know why we failed to determine if we were in a container or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should capture some diagnostics to know why we failed to determine if we were in a container or not.

I agree. I actually want to add tracepoints in the code at various error conditions and thought of taking that up it in a separate PR. Is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works for me.

omrthread_monitor_exit(cgroupEntryListMonitor);
if (0 != rc) {
goto _end;
rc = isRunningInContainer(portLibrary, &inContainer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isRunningInContainer immediately calls isCgroupV1Available so this ends up doing the isCgroupV1Available work twice.

Can this be commoned into a single call? I hesitate to suggest a isCgroupV1AndInContainer(portLibrary, &isCgroupV1, &inContainer) call as we should be able to come up with a better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed that but didn't bother as isCgroupV1Available just adds an overhead of one syscall.
But if we want to common it out, how about calling the new function checkCgroupPrereqs(&cgroupVersion, &inContainer) where cgroupVersion would be an enum { V1, V2 } (currently the code only supports V1, but sooner than later we would have to consider V2 as well).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'll have to touch this code again to add V2 support, I'm OK with leaving it as is now.

@ashu-mehra
Copy link
Contributor Author

ashu-mehra commented Feb 21, 2018

@DanHeidinga OpenJ9 does not yet have the code to enable cgroup detection.
If you use enable_cgroup branch from my openj9 fork, it would enable cgroups if -XX:+UseContainerSupport is specified.
I built OpenJ9 using this branch and used verbose gc to check if its working. I started the docker container with 1G memory.
Running java without -XX:+UseContainerSupport, I get following output:

# ./java -verbose:gc -version

<?xml version="1.0" ?>

<verbosegc xmlns="http://www.ibm.com/j9/verbosegc" version="da53b9f_CMPRSS">

<initialized id="1" timestamp="2018-02-21T08:56:57.605">
  <attribute name="gcPolicy" value="-Xgcpolicy:gencon" />
  <attribute name="maxHeapSize" value="0x78100000" />
  <attribute name="initialHeapSize" value="0x800000" />
   ...
  <system>
    <attribute name="physicalMemory" value="8057266176" />
    <attribute name="numCPUs" value="4" />
    <attribute name="architecture" value="amd64" />
    <attribute name="os" value="Linux" />
    <attribute name="osVersion" value="3.10.0-693.5.2.el7.x86_64" />
  </system>

Note that physicalMemory is same as my system memory ~ 8G and maxHeapSize is ~ 1.9G.
Running java with -XX:+UseContainerSupport:

# ./java -XX:+UseContainerSupport -verbose:gc -version

<?xml version="1.0" ?>

<verbosegc xmlns="http://www.ibm.com/j9/verbosegc" version="da53b9f_CMPRSS">

<initialized id="1" timestamp="2018-02-21T08:57:24.097">
  <attribute name="gcPolicy" value="-Xgcpolicy:gencon" />
  <attribute name="maxHeapSize" value="0x20000000" />
  <attribute name="initialHeapSize" value="0x800000" />
  ...
  <system>
    <attribute name="physicalMemory" value="1073741824" />

Note that physicalMemory is is now 1G and maxHeapSize is 512M.

Copy link
Contributor

@DanHeidinga DanHeidinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm. @charliegracie can you give this a second review and merge?

omrthread_monitor_exit(cgroupEntryListMonitor);
if (0 != rc) {
goto _end;
rc = isRunningInContainer(portLibrary, &inContainer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we'll have to touch this code again to add V2 support, I'm OK with leaving it as is now.

@charliegracie charliegracie self-assigned this Feb 27, 2018
@charliegracie
Copy link
Contributor

@genie-omr build all zos

@charliegracie charliegracie merged commit 4310e3a into eclipse-omr:master Feb 27, 2018
@ashu-mehra ashu-mehra deleted the detect_docker branch March 1, 2018 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants