Skip to content

Add hack for Docker cgroups#22757

Merged
jasontedor merged 2 commits intoelastic:masterfrom
jasontedor:docker-cgroups-hack
Jan 24, 2017
Merged

Add hack for Docker cgroups#22757
jasontedor merged 2 commits intoelastic:masterfrom
jasontedor:docker-cgroups-hack

Conversation

@jasontedor
Copy link
Copy Markdown
Member

@jasontedor jasontedor 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
Copy link
Copy Markdown
Member Author

jasontedor 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.

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.
Copy link
Copy Markdown
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks good, a few minor comments.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

@jasontedor jasontedor Jan 24, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You're right, I'll change.

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
Copy link
Copy Markdown
Contributor

@s1monw s1monw left a comment

Choose a reason for hiding this comment

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

LGTM

@s1monw
Copy link
Copy Markdown
Contributor

s1monw commented Jan 24, 2017

@elasticmachine test this please

@s1monw
Copy link
Copy Markdown
Contributor

s1monw 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
Copy link
Copy Markdown
Member Author

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
Copy link
Copy Markdown
Member Author

retest this please

@jasontedor jasontedor merged commit bcffc6f into elastic:master Jan 24, 2017
jasontedor added a commit that referenced this pull request 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 #22757
jasontedor added a commit that referenced this pull request 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 #22757
jasontedor added a commit that referenced this pull request 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 #22757
@jasontedor
Copy link
Copy Markdown
Member Author

Thanks @rjernst and @s1monw.

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
Copy link
Copy Markdown
Contributor

@dliappis dliappis Jan 25, 2017

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks @dliappis, I pushed cb822b4.

dliappis pushed a commit to elastic/elasticsearch-docker that referenced this pull request Jan 25, 2017
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 pushed a commit to elastic/elasticsearch-docker that referenced this pull request Jan 26, 2017
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 pushed a commit to elastic/elasticsearch-docker that referenced this pull request Jan 26, 2017
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants