Skip to content
This repository has been archived by the owner on Apr 12, 2022. It is now read-only.

Add hack for obtaining cgroup stats #25

Merged
merged 2 commits into from
Jan 25, 2017
Merged

Add hack for obtaining cgroup stats #25

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67fd123
Add hack for obtaining cgroup stats
jasontedor Jan 24, 2017
490bcd6
Add comment explaining the cgroup hack
jasontedor Jan 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions build/elasticsearch/bin/es-docker
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,6 @@ do
fi
done < <(env)

echo "-Des.cgroups.hierarchy.override=/" >> config/jvm.options
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it is better to do this fix in this file (which is the CMD) or in the Dockerfile. There are + and - for both approaches:

As shown above, though the CMD (bin/es-docker) script

As this is always invoked when starting the container (unless the user overrides our CMD) there is a corner case that if a user ships his own jvm.options via a bind mount (or in the Dockerfile, while building his own docker image by forking ours) there could be a duplicate entry or generally the user get surprised where did that entry come from.

On the other hand having the echo line in bin/es-docker will always make sure that the parameter is present, but we could ensure it's done only if needed with a combo of grep + echo.

Via the Dockerfile

If we just add the above echo command as a RUN line in the Dockerfile (after extracting the tarball) we ensure that things are correct in the image-shipped jvm.options file and also no unexpected surprises if the user decides to bind mount jvm.options or create his own image.

Downside of this approach is that it doesn't "enforce" the option for the user, in case he opts to use his own config file.

I'd like to hear your opinion on this. FWIW I am usually leaning in favor of solutions that don't try too hard to be clever and invasive so I think I'd favor doing it in the Dockerfile.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dliappis Given your thoughts here, which I agree with, I wonder what you think of:

diff --git a/build/elasticsearch/bin/es-docker b/build/elasticsearch/bin/es-docker
index b50046e..de544fd 100755
--- a/build/elasticsearch/bin/es-docker
+++ b/build/elasticsearch/bin/es-docker
@@ -22,6 +22,6 @@ do
     fi
 done < <(env)
 
-echo "-Des.cgroups.hierarchy.override=/" >> config/jvm.options
+export ES_JAVA_OPTS="-Des.cgroups.hierarchy.override=/ $ES_JAVA_OPTS"
 
 exec bin/elasticsearch ${es_opts}

Copy link

Choose a reason for hiding this comment

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

I like this much more. Mutating that file could be really nasty, since it might be owned by the user and living on some persistent storage that they control.

If we can apply this fix via an environment variable, is there any reason not to do so with an ENV declaration in the Dockerfile? I think that's the most transparent and obvious place for future readers to understand what's happening.

Wherever we do it, let's also have a comment linking back to this thread, since it's a pretty subtle thing that's going on.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct me if I'm wrong please, but I don't think we can do it in the Dockerfile since that bakes the value of ES_JAVA_OPTS into the image but users can override this externally when running the image.

I will push a comment explaining the situation later this morning.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dliappis @Jarpy I pushed 490bcd6.

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct me if I'm wrong please, but I don't think we can do it in the Dockerfile since that bakes the value of ES_JAVA_OPTS into the image but users can override this externally when running the image.

Yes that is correct and in the website docs docker-compose.yml example we are showing how to use ES_JAVA_OPTS as one method of defining the jvm heap size.

@jasontedor Just so that I understand better this solution, basically es.cgroups.hierarchy.override can be defined as a jvm option, as documented here hence will be honored when presented through ES_JAVA_OPTS?
I am asking this as my understanding is that most parameters defined in jvm.options can also be passed as -E style cli args. If that is true, the user can run the image using docker run ... -e es.cgroups.hierarchy.override=/ and so it could be the value here instead of the existing es_opts=''?

(Note that I tried the last point with 6.0.0-alpha1-SNAPSHOT and received:

uncaught exception in thread [main]
org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: unknown setting [es.cgroups.hierarchy.override] please check that any required plugins are installed, or check the breaking changes documentation for removed settings

)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just so that I understand better this solution, basically es.cgroups.hierarchy.override can be defined as a jvm option, as documented here hence will be honored when presented through ES_JAVA_OPTS?

Yes.

I am asking this as my understanding is that most parameters defined in jvm.options can also be passed as -E style cli args. If that is true, the user can run the image using docker run ... -e es.cgroups.hierarchy.override=/ and so it could be the value here instead of the existing es_opts=''?

This part is not right, ES_JAVA_OPTS and jvm.options control JVM flags, so options in ES_JAVA_OPTS can also be set in jvm.options and vice-versa. The -E CLI flags to Elasticsearch are Elasticsearch settings and can also be set in elasticsearch.yml.

(Note that I tried the last point with 6.0.0-alpha1-SNAPSHOT and received:

uncaught exception in thread [main]
org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: unknown >setting [es.cgroups.hierarchy.override] please check that any required plugins are installed, or >check the breaking changes documentation for removed settings

)

This is expected since es.cgroups.hierarchy.override is not an Elasticsearch setting, we are reading it as JVM system property which is set via the JVM flag -Des.cgroups.hierarchy.override=/; this can be done via jvm.options or ES_JAVA_OPTS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification @jasontedor !


exec bin/elasticsearch ${es_opts}