Remove os environment variables from env prep #978

hub-cap · 2020-04-29T18:02:11Z

The launcher pulls in local environment variables from the OS prior to
launching Elasticsearch. This can be problematic if the $JAVA_HOME
variable is locally set, but explicitly not set by the Rally
launcher. For the case of the bundled JDK, a variable java_home is
explicitly unset, which should be reconciled with the local env by also
unsetting $JAVA_HOME if it exists before launching. This commit ensures
that if java_home is unset in the code, that the local env's $JAVA_HOME
variable is also unset.

Closes #976

The launcher pulls in local environment variables from the OS prior to launching elasticsearch. This can be problematic if the JAVA_HOME variable is locally set, but explicitly not set by the rally launcher. For the case of the bundled JDK, a variable java_home is explicitly unset, which should be reconciled with the local env by also unsetting JAVA_HOME if it exists before launching. This commit ensures that if java_home is unset in the code, that the local env's JAVA_HOME variable is also unset. Closes elastic#976

hub-cap · 2020-04-29T18:03:21Z

@dliappis this commit does not remove any of the JAVAXX_HOME's, but I did not see the usefulness of that inside how we operate w/ rally today. I can do it, but I think that this PR fixes the bug that was present when we moved to the bundled jdk.

dliappis

Thanks. I left two comments.

dliappis · 2020-04-30T06:22:16Z

esrally/mechanic/launcher.py

@@ -175,6 +175,9 @@ def _prepare_env(self, car_env, node_name, java_home, t):
            self._set_env(env, "PATH", os.path.join(java_home, "bin"), separator=os.pathsep, prepend=True)
            # Don't merge here!
            env["JAVA_HOME"] = java_home
+        elif env.get("JAVA_HOME"):


Should we take a cleaner (and bolder) approach and not inherit anything from the env apart from PATH?

Practically speaking I thought about replacing lines 172 and 172 with something like:
env={k:v for k, v in os.environ.items() if k == "PATH"}

TBH I can't think of what we need from the shell PATH to launch ES, we might just as well be ok with removing line 172 altogether, but typically all launched Elasticsearches (even in Docker) will have something in PATH so this sounds ok to me.

I like that approach as it is much cleaner. I can think of situations when we want to set other environment variables like LD_LIBRARY_PATH in order set a custom allocator like jemalloc to use it for analyzing native memory leaks.

I suggest that we leverage our config object here to only pass PATH by default but allow to override it with a config property. We should avoid using the config object everywhere and instead derive a property in the constructor of ProcessLauncher as follows:

from esrally.utils import opts #... self.pass_env_vars = opts.csv_to_list(self.cfg.opts("system", "passenv", mandatory=False, default_value="PATH"))

Do we want to overwriting ES_JAVA_OPTS? Or append the ExitOnOOM to it? Ill leave it currently as overwriting, unless we want to change the behaviour here.

After chatting w @dliappis, we will honor the user's ES_JAVA_OPTS if provided in the rally.ini, otherwise we will add the flag.

Do we want to always glean the PATH even if it is not specified in the config? As is if the user adds variables, they might forget the path and then 💥

I tend not to apply any special logic for PATH and would favor if the user would clearly see what environment variables are passed. I also expect that the default will only be overridden in very special circumstances (like the example I've provided above).

ok I will remove the PATH special case!

dliappis · 2020-04-30T06:24:18Z

esrally/mechanic/launcher.py

@@ -175,6 +175,9 @@ def _prepare_env(self, car_env, node_name, java_home, t):
            self._set_env(env, "PATH", os.path.join(java_home, "bin"), separator=os.pathsep, prepend=True)
            # Don't merge here!
            env["JAVA_HOME"] = java_home
+        elif env.get("JAVA_HOME"):
+            # Assume that since java_home was not passed in, it should be unset in the environment if it exists
+            del env["JAVA_HOME"]


Could we adjust the test in launcher_test#test_bundled_jdk_not_in_path() to fail with our current code, i.e. actually check that the JAVA_HOME env var remains unset even if set in os.environ?

hub-cap · 2020-04-30T19:44:17Z

I want to ensure yall are ok w/ the functionality here. I added a check to keep PATH on the environment even if it was not explicitly stated in the rally config. I assume we need PATH and it would not make sense for a user to have to manually include that every time. If this is an invalid assumption, then please correct me!

dliappis

This looks good!

However we have a number of reference to JAVA_HOME in our docs:

https://esrally.readthedocs.io/en/latest/quickstart.html?highlight=JAVA_HOME#install
https://esrally.readthedocs.io/en/stable/command_line_reference.html?highlight=JAVA_HOME#runtime-jdk

At least in the latter we should clearly explain what happens when bundled is in use and JAVA_HOME is set.

Possibly we should also document the change in behavior in the migration guide (for Rally 2.0.0) to avoid people wondering with existing JAVA_HOME isn't honored any more in https://esrally.readthedocs.io/en/stable/migrate.html?

tests/mechanic/launcher_test.py

hub-cap · 2020-05-04T16:42:55Z

Good call on the JAVA_HOME comments. Would you like this done as part of this PR or a followup?

hub-cap · 2020-05-04T18:04:55Z

Regarding JAVA_HOME, it is still needed to run rally unless you specifically invoke the jvm w/ the bundled param... I unset all my JAVA*_HOME's and got

(.venv) ➜  rally git:(del_java_home) esrally --distribution-version=7.6.0

    ____        ____
   / __ \____ _/ / /_  __
  / /_/ / __ `/ / / / / /
 / _, _/ /_/ / / / /_/ /
/_/ |_|\__,_/_/_/\__, /
                /____/

[INFO] Preparing for race ...
[ERROR] Cannot race. ("Install a JDK with one of the versions [11, 10, 9, 8] and point to it with one of ['JAVA11_HOME', 'JAVA10_HOME', 'JAVA9_HOME', 'JAVA8_HOME', 'JAVA_HOME'].", None)

hub-cap · 2020-05-04T18:08:24Z

Running with --runtime-jdk=bundled using 7.6.0 I got

/home/baz/.rally/benchmarks/races/873a53a5-f7f2-4780-910f-cbb4f471eca5/rally-node-0/install/elasticsearch-7.6.0/jdk/bin/java -Xms1g -Xmx1g -XX:+UseConcMarkSweepGC -

And what is interesting, running with bundled and a version that had no bundled jdk, rally used /bin/java on my machine, which i suspect is a bug, and im going to eyeball it.

hub-cap · 2020-05-04T19:16:07Z

As per @rjernst , the fallback from finding java on the path (which is why /bin/java is running) changed at 7.0. Prior to 7.0, ES would look for java on the path, whereas 7.0 and up it falls back to using the bundled JDK.

Our code, however, allows for a bundled jdk to run the path jdk prior to 7.0. And not specifying a jdk version, as well as not having JAVA_HOME specified fails on 7.0 and above.

Should we change this so the code in 7.0 and above just uses the bundled JDK if the JAVA_HOME variables are not set?

edit:

I have found out how the car definitions ensure that one of the environment variables for a given runtime jdk are ensured. I wonder still if we should be erroring out if bundled is not set explicitly, or should be falling back to using the bundled JDK.

hub-cap · 2020-05-04T20:32:17Z

I have added a note about the bundled jdk in the migration docs.

dliappis

LGTM

dliappis · 2020-05-05T09:10:58Z

docs/migrate.rst

+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+Rally can optionally use the bundled runtime JDK by setting ``--runtime-jdk="bundled"``. This setting will use the JDK that is bundled with
+elasticsearch and not honor any ``JAVA_HOME`` settings you may have set.


nit: capitalization

this will curse me till the day I die I think.

tests/mechanic/launcher_test.py

This commit removes an explicit add of the JAVA_HOME environment variable from the _prepare_env method, which will, if the bundled JDK is selected, ensure that Elasticsearch will not use a JAVA_HOME environment variable instead of the bundled JDK. This commit does not remove the use of the JAVA_HOME environment variables in the event that the bundled JDK is not chosen, which is resolved in a different place. Closes elastic#976

hub-cap added bug Something's wrong :misc Changes that don't affect users directly: linter fixes, test improvements, etc. labels Apr 29, 2020

hub-cap added this to the 2.0.0 milestone Apr 29, 2020

hub-cap requested review from danielmitterdorfer and dliappis April 29, 2020 18:02

hub-cap self-assigned this Apr 29, 2020

dliappis reviewed Apr 30, 2020

View reviewed changes

Fixing up logic, adding tests

bd4e654

hub-cap requested a review from dliappis April 30, 2020 19:44

Remove path special casing

778041a

dliappis reviewed May 4, 2020

View reviewed changes

tests/mechanic/launcher_test.py Show resolved Hide resolved

tests/mechanic/launcher_test.py Show resolved Hide resolved

Add message about the runtime bundled jdk

212becd

hub-cap requested a review from dliappis May 4, 2020 20:34

dliappis approved these changes May 5, 2020

View reviewed changes

hub-cap added 3 commits May 5, 2020 10:15

Merge remote-tracking branch 'upstream/master' into del_java_home

2d14472

cApS

97fd616

hub-cap changed the title ~~Remove JAVA_HOME from env if it is not set~~ Remove os environment variables from env prep May 5, 2020

hub-cap merged commit f068edf into elastic:master May 5, 2020

hub-cap deleted the del_java_home branch May 5, 2020 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove os environment variables from env prep #978

Remove os environment variables from env prep #978

hub-cap commented Apr 29, 2020 •

edited

Loading

hub-cap commented Apr 29, 2020

dliappis left a comment

dliappis Apr 30, 2020

danielmitterdorfer Apr 30, 2020

hub-cap Apr 30, 2020

hub-cap Apr 30, 2020

hub-cap Apr 30, 2020

danielmitterdorfer May 4, 2020

hub-cap May 4, 2020

dliappis Apr 30, 2020

hub-cap commented Apr 30, 2020

dliappis left a comment

hub-cap commented May 4, 2020

hub-cap commented May 4, 2020 •

edited

Loading

hub-cap commented May 4, 2020

hub-cap commented May 4, 2020 •

edited

Loading

hub-cap commented May 4, 2020

dliappis left a comment

dliappis May 5, 2020

hub-cap May 5, 2020

Remove os environment variables from env prep #978

Remove os environment variables from env prep #978

Conversation

hub-cap commented Apr 29, 2020 • edited Loading

hub-cap commented Apr 29, 2020

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Apr 30, 2020

dliappis left a comment

Choose a reason for hiding this comment

hub-cap commented May 4, 2020

hub-cap commented May 4, 2020 • edited Loading

hub-cap commented May 4, 2020

hub-cap commented May 4, 2020 • edited Loading

hub-cap commented May 4, 2020

dliappis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented Apr 29, 2020 •

edited

Loading

hub-cap commented May 4, 2020 •

edited

Loading

hub-cap commented May 4, 2020 •

edited

Loading