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 hack for Docker cgroups #22757

Merged
merged 2 commits into from Jan 24, 2017

Conversation

Projects
None yet
4 participants
@jasontedor
Copy link
Member

commented Jan 24, 2017

Docker cgroups are mounted in the wrong place (i.e., inconsistently with /proc/self/cgroup). This commit adds an undocumented hack for working around, for now.

Relates #21029

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

Also, I should point out that I built a new Docker image from a SNAPSHOT build of Elasticsearch with this fix, and I was able to obtain the expected cgroup stats.

Add hack for Docker cgroups
Docker cgroups are mounted in the wrong place (i.e., inconsistently with
/proc/self/cgroup). This commit adds an undocumented hack for working
around, for now.

@jasontedor jasontedor force-pushed the jasontedor:docker-cgroups-hack branch to 1401bef Jan 24, 2017

@rjernst
Copy link
Member

left a comment

Looks good, a few minor comments.

core/src/main/java/org/elasticsearch/monitor/os/OsProbe.java Outdated
@@ -191,6 +191,11 @@ private String readSingleLine(final Path path) throws IOException {
// pattern for lines in /proc/self/cgroup
private static final Pattern CONTROL_GROUP_PATTERN = Pattern.compile("\\d+:([^:,]+(?:,[^:,]+)?):(/.*)");

// this property is to support a hack to workaround an issue with Docker containers not mounting the cgroups hierarchy inconsistently

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 24, 2017

Member

should this be "not mounting...consistently" or "mounting...inconsistently"? But I would think not the current double negative.

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 24, 2017

Author Member

Yes, I'll fix in the morning before merging.

core/src/main/java/org/elasticsearch/monitor/os/OsProbe.java Outdated
// this property is to support a hack to workaround an issue with Docker containers not mounting the cgroups hierarchy inconsistently
// with respect to /proc/self/cgroup; for Docker containers this should be set to "/"
private static final String CONTROL_GROUPS_PATH_OVERRRIDE =
(String) BootstrapInfo.getSystemProperties().get("es.cgroups.path.override");

This comment has been minimized.

Copy link
@rjernst

rjernst Jan 24, 2017

Member

This can just be System.getProperty? I thought the point of BootstrapInfo.getSystemProperties() was when you wanted to call System.getProperties() (which we have in forbidden apis)

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 24, 2017

Author Member

You're right, I'll change.

Cleanup cgroups hierarchy hack
This commit cleans up a few issues with the cgroups hierarchy hack:
 - renames the property from es.cgroups.path.override to
   es.cgroups.hierarchy.override
 - fixes a double negative in a comment
 - just uses System#getProperty instead of
   BootstrapInfo#getSystemProperties
@s1monw

s1monw approved these changes Jan 24, 2017

Copy link
Contributor

left a comment

LGTM

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

@elasticmachine test this please

@s1monw

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2017

Also, I should point out that I built a new Docker image from a SNAPSHOT build of Elasticsearch with this fix, and I was able to obtain the expected cgroup stats.

thanks for verifying. it's hard to test I guess...

I wonder if we should port this to 5.1.x - but I might be mistaking that this exists already in 5.1.x.. not sure

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

I wonder if we should port this to 5.1.x - but I might be mistaking that this exists already in 5.1.x.. not sure

+1, I'll port to 5.1 as well (you're not mistaken).

@jasontedor jasontedor added the v5.1.3 label Jan 24, 2017

@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

retest this please

@jasontedor jasontedor merged commit bcffc6f into elastic:master Jan 24, 2017

1 of 2 checks passed

elasticsearch-ci Build started sha1 is merged.
Details
CLA Commit author is a member of Elasticsearch
Details

jasontedor added a commit that referenced this pull request Jan 24, 2017

Add hack for Docker cgroups
Docker cgroups are mounted in the wrong place (i.e., inconsistently with
/proc/self/cgroup). This commit adds an undocumented hack for working
around, for now.

Relates #22757

jasontedor added a commit that referenced this pull request Jan 24, 2017

Add hack for Docker cgroups
Docker cgroups are mounted in the wrong place (i.e., inconsistently with
/proc/self/cgroup). This commit adds an undocumented hack for working
around, for now.

Relates #22757

jasontedor added a commit that referenced this pull request Jan 24, 2017

Add hack for Docker cgroups
Docker cgroups are mounted in the wrong place (i.e., inconsistently with
/proc/self/cgroup). This commit adds an undocumented hack for working
around, for now.

Relates #22757
@jasontedor

This comment has been minimized.

Copy link
Member Author

commented Jan 24, 2017

Thanks @rjernst and @s1monw.

@jasontedor jasontedor deleted the jasontedor:docker-cgroups-hack branch Jan 24, 2017

controllerMap.put(controller, matcher.group(2));
if (CONTROL_GROUPS_HIERARCHY_OVERRIDE != null) {
/*
* Docker violates the relationship between /proc/self/cgroups and the /sys/fs/cgroup hierarchy. It's possible that this

This comment has been minimized.

Copy link
@dliappis

dliappis Jan 25, 2017

Contributor

@jasontedor Very minor typo, but should be /proc/self/cgroup

This comment has been minimized.

Copy link
@jasontedor

jasontedor Jan 25, 2017

Author Member

Thanks @dliappis, I pushed cb822b4.

dliappis added a commit to elastic/elasticsearch-docker that referenced this pull request Jan 25, 2017

Add hack for obtaining cgroup stats
Docker mounts cgroup stats inconsistently with the hierarchy 
in /proc/self/cgroup. This breaks the logic for traversing
from /proc/self/cgroup to the stats for a process running inside a
container. In core Elasticsearch, an undocumented hack was added to
support this, for now. Thus, for cgroup stats to be available from
Elasticsearch running inside a Docker container, the Elasticsearch
process should be started with `-Des.cgroups.hierarchy.override=/`.

Add hack for obtaining cgroup stats and comment it.

Relates #25 
Relates elastic/elasticsearch#22757

dliappis added a commit to elastic/elasticsearch-docker that referenced this pull request Jan 26, 2017

Add hack for obtaining cgroup stats
Docker mounts cgroup stats inconsistently with the hierarchy 
in /proc/self/cgroup. This breaks the logic for traversing
from /proc/self/cgroup to the stats for a process running inside a
container. In core Elasticsearch, an undocumented hack was added to
support this, for now. Thus, for cgroup stats to be available from
Elasticsearch running inside a Docker container, the Elasticsearch
process should be started with `-Des.cgroups.hierarchy.override=/`.

Add hack for obtaining cgroup stats and comment it.

Relates #25 
Relates elastic/elasticsearch#22757

dliappis added a commit to elastic/elasticsearch-docker that referenced this pull request Jan 26, 2017

Add hack for obtaining cgroup stats
Docker mounts cgroup stats inconsistently with the hierarchy 
in /proc/self/cgroup. This breaks the logic for traversing
from /proc/self/cgroup to the stats for a process running inside a
container. In core Elasticsearch, an undocumented hack was added to
support this, for now. Thus, for cgroup stats to be available from
Elasticsearch running inside a Docker container, the Elasticsearch
process should be started with `-Des.cgroups.hierarchy.override=/`.

Add hack for obtaining cgroup stats and comment it.

Relates #25 
Relates elastic/elasticsearch#22757
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.