Allow to attach telemetry devices without reprovisioning #737

ebadyano · 2019-07-31T18:59:32Z

By using ES_JAVA_OPTS we can provision a node, run a benchmark, and then “dynamically” (i.e. without reprovisioning) start the node again with telemetry attached.

Relates to #697
Relates to #711

By using ES_JAVA_OPTS we can provision a node, run a benchmark, and then “dynamically” (i.e. without reprovisioning) start the node again with telemetry attached. Relates to elastic#697 Relates to elastic#711

danielmitterdorfer

Thanks for the PR. The overall direction looks fine but I had some comments regarding the implementation.

danielmitterdorfer · 2019-08-01T06:07:18Z

esrally/mechanic/launcher.py

+        #node_telemetry_dir = "%s/telemetry" % node_configuration.node_root_path
+        node_telemetry_dir = os.path.join(node_configuration.node_root_path, "telemetry")
+
+        print("here in oy %s" % node_telemetry_dir)


oh, thanks for catching

danielmitterdorfer · 2019-08-01T06:07:30Z

esrally/mechanic/launcher.py

@@ -299,14 +299,21 @@ def _start_node(self, node_configuration, node_count_on_host):
        car = node_configuration.car
        binary_path = node_configuration.binary_path
        data_paths = node_configuration.data_paths
-        node_telemetry_dir = "%s/telemetry" % node_configuration.node_root_path
+        #node_telemetry_dir = "%s/telemetry" % node_configuration.node_root_path


danielmitterdorfer · 2019-08-01T07:40:19Z

esrally/mechanic/launcher.py

@@ -340,7 +352,7 @@ def _set_env(self, env, k, v, separator=' '):
            if k not in env:
                env[k] = v
            else:  # merge
-                env[k] = v + separator + env[k]
+                env[k] = env[k] + separator + v


Can you please explain why you've changed the order here?

For some reason JFR telemetry would not start and rally would never return with pid if the the options didn't start with "-XX:+UnlockDiagnosticVMOptions" That's how it was originally, but now that we changed the telemetry to return list instead of string the order got reversed when we were adding them to the env[k]. So I reversed again so it's in the original order. I could change the order in telemetry instead if you prefer.

I see. That's very subtle but it makes sense that we preserve the order in the merge routine (i.e. as you've implemented it now). Can you please add a test so we ensure that -XX:+UnlockDiagnosticVMOptions is the first option when we activate multiple telemetry devices (you can just use the full list of JVM args in the assertion)?

Did a bit more testing, so it seems that order of options for other telemetry devices doesn't matter. It's that specifically for jfr that -XX:+UnlockDiagnosticVMOptions has to come before -XX:+DebugNonSafepoints but other telemetry can be before or after jfr options. So maybe modifying order in telemery for jfr makes more sense than in _set_env.. WDYT?

Yes, your proposal makes sense to me.

danielmitterdorfer · 2019-08-01T07:49:14Z

tests/mechanic/launcher_test.py

@@ -119,6 +119,7 @@ def wait(self):


 class ProcessLauncherTests(TestCase):
+    @mock.patch('os.path.join', return_value="/telemetry")


Why do we need to mock this? IMHO we shouldn't have mocked quite a few of the dependencies of this test case to begin with and specifically not esrally.mechanic.provisioner.NodeConfiguration.

I think we need to clean this up in a separate PR. @drawlerr can you please work on a new PR (based on master) and rewrite this test case to mock only dependencies that have side effects? (as a guideline: things in os are likely ok, things like esrally.config.Config or esrally.metrics.MetricsStore should not be mocked).

I modified the launcher to use os.path instead of concatenating strings. If I don't update the test I get the following error. Not sure how to get away without mocking it..

________________ ProcessLauncherTests.test_daemon_start_stop __________________ args = (<tests.mechanic.launcher_test.ProcessLauncherTests testMethod=test_daemon_start_stop>, <MagicMock name='Process' id='...ion' id='4573833480'>, <MagicMock name='MetricsStore' id='4573862824'>, <MagicMock name='Config' id='4577452888'>, ...) keywargs = {} extra_args = [<MagicMock name='Process' id='4574868424'>, <MagicMock name='wait_for_pidfile' id='4578813376'>, <MagicMock name='Nod...tricsStore' id='4573862824'>, <MagicMock name='Config' id='4577452888'>, <MagicMock name='chdir' id='4578781056'>, ...] entered_patchers = [<unittest.mock._patch object at 0x10d987438>, <unittest.mock._patch object at 0x10d987ef0>, <unittest.mock._patch obj...bject at 0x10d987a58>, <unittest.mock._patch object at 0x10d987320>, <unittest.mock._patch object at 0x10d987ac8>, ...] exc_info = (<class 'TypeError'>, TypeError("test_daemon_start_stop() missing 1 required positional argument: 'path'"), <traceback object at 0x110efe5c8>) patching = <unittest.mock._patch object at 0x10d987438> arg = <MagicMock name='kill' id='4578728480'> @wraps(func) def patched(*args, **keywargs): extra_args = [] entered_patchers = [] exc_info = tuple() try: for patching in patched.patchings: arg = patching.__enter__() entered_patchers.append(patching) if patching.attribute_name is not None: keywargs.update(arg) elif patching.new is DEFAULT: extra_args.append(arg) args += tuple(extra_args) > return func(*args, **keywargs) E TypeError: test_daemon_start_stop() missing 1 required positional argument: 'path' /usr/local/Cellar/python/3.7.1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/unittest/mock.py:1191: TypeError

You also need to remove path from the argument list of the test. Otherwise the test framework cannot call test_daemon_start_stop.

right, sorry if if remove both mock and path in the arguments I get this:

@mock.patch('os.kill') @mock.patch('subprocess.Popen',new=MockPopen) @mock.patch('esrally.mechanic.java_resolver.java_home', return_value=(12, "/java_home/")) @mock.patch('esrally.utils.jvm.supports_option', return_value=True) @mock.patch('os.chdir') @mock.patch('esrally.config.Config') @mock.patch('esrally.metrics.MetricsStore') @mock.patch('esrally.mechanic.provisioner.NodeConfiguration') @mock.patch('esrally.mechanic.launcher.wait_for_pidfile', return_value=MOCK_PID_VALUE) @mock.patch('psutil.Process') def test_daemon_start_stop(self, process, wait_for_pidfile, node_config, ms, cfg, chdir, supports, java_home, kill): proc_launcher = launcher.ProcessLauncher(cfg, ms, paths.races_root(cfg)) > nodes = proc_launcher.start([node_config]) tests/mechanic/launcher_test.py:135: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ esrally/mechanic/launcher.py:294: in start return [self._start_node(node_configuration, node_count_on_host) for node_configuration in node_configurations] esrally/mechanic/launcher.py:294: in <listcomp> return [self._start_node(node_configuration, node_count_on_host) for node_configuration in node_configurations] esrally/mechanic/launcher.py:302: in _start_node node_telemetry_dir = os.path.join(node_configuration.node_root_path, "telemetry") _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ a = <MagicMock name='NodeConfiguration.node_root_path' id='4598523160'> p = ('telemetry',) def join(a, *p): """Join two or more pathname components, inserting '/' as needed. If any component is an absolute path, all previous path components will be discarded. An empty last part will result in a path that ends with a separator.""" > a = os.fspath(a) E TypeError: expected str, bytes or os.PathLike object, not MagicMock /usr/local/Cellar/python/3.7.1/Frameworks/Python.framework/Versions/3.7/lib/python3.7/posixpath.py:80: TypeError

As discussed in another channel this is the result of mocking other dependencies so it is ok if you keep the mock on os.path.join here as this will be cleaned up in a dedicated PR.

danielmitterdorfer

I did a final test with an activated telemetry device and found an issue with the order of JVM arguments. Can you please check again and address this?

danielmitterdorfer · 2019-08-13T07:35:43Z

esrally/mechanic/launcher.py

@@ -352,7 +349,7 @@ def _set_env(self, env, k, v, separator=' '):
            if k not in env:
                env[k] = v
            else:  # merge
-                env[k] = env[k] + separator + v
+                env[k] = v + separator + env[k]


I tested this with

esrally --test-mode --distribution-version=2.4.6 --on-error=abort --challenge=append-no-conflicts-index-only --telemetry=jfr,gc

and indeed you have to preserve the order of the JVM arguments otherwise you'll get an error that the JVM process could not be created. Can you please:

Change it to your previous implementation so that the order is preserved

Add a test in ProcessLauncher that adds a telemetry device and checks ES_JAVA_OPTS?

danielmitterdorfer · 2019-08-13T07:36:36Z

esrally/mechanic/telemetry.py

@@ -191,7 +191,7 @@ def instrument_java_opts(self, car, candidate_id):

    def java_opts(self, log_file):
        recording_template = self.telemetry_params.get("recording-template")
-        java_opts = ["-XX:+UnlockDiagnosticVMOptions", "-XX:+DebugNonSafepoints"]
+        java_opts = ["-XX:+DebugNonSafepoints", "-XX:+UnlockDiagnosticVMOptions"]


If the order is preserved (see my comment above) also this needs to be reversed again (-XX:+UnlockDiagnosticVMOptions must be specified before -XX:+DebugNonSafepoints).

danielmitterdorfer

Thanks for adding the test. I left two more comments but I think we're almost there.

danielmitterdorfer · 2019-08-16T08:41:57Z

esrally/mechanic/launcher.py

        if v is not None:
            if k not in env:
                env[k] = v
            else:  # merge
-                env[k] = v + separator + env[k]
+                if (prepend == True):


We can rewrite this as

if k not in env: env[k] = v elif prepend: # prepend it else: # append it

danielmitterdorfer · 2019-08-16T09:00:39Z

tests/mechanic/launcher_test.py

@@ -139,6 +140,25 @@ def test_daemon_start_stop(self, process, wait_for_pidfile, node_config, ms, cfg
        proc_launcher.keep_running = False
        proc_launcher.stop(nodes)
        self.assertTrue(kill.called)
+
+    @mock.patch('os.path.join', return_value="/java_home/bin")


This should not be necessary and this test should work without mocking os.path.join.

ebadyano · 2019-08-16T12:00:50Z

@danielmitterdorfer Thank you for the review! I addressed your latest comments.

danielmitterdorfer

I left one final comment. Also, can you please reword the PR title a bit so the intent is clearer? E.g. "Allow to attach telemetry without reprovisioning"?

danielmitterdorfer · 2019-08-16T12:44:54Z

esrally/mechanic/launcher.py

@@ -348,10 +348,9 @@ def _set_env(self, env, k, v, separator=' ', prepend=False):
        if v is not None:
            if k not in env:
                env[k] = v
-            else:  # merge
-                if (prepend == True):
+            elif (prepend == True):


While this will work correctly, idiomatic usage is elif prepend instead of elif (prepend == True). Can you please change that?

danielmitterdorfer

Thanks for iterating! LGTM! :)

By using ES_JAVA_OPTS we can provision a node, run a benchmark, and then “dynamically” (i.e. without reprovisioning) start the node again with telemetry attached. Relates to elastic#697 Relates to elastic#711

Revert PR#711 to use ES_JAVA_OPTS for telemetry devices

3661748

By using ES_JAVA_OPTS we can provision a node, run a benchmark, and then “dynamically” (i.e. without reprovisioning) start the node again with telemetry attached. Relates to elastic#697 Relates to elastic#711

ebadyano requested a review from danielmitterdorfer July 31, 2019 18:59

danielmitterdorfer requested changes Aug 1, 2019

View reviewed changes

Address Daniel's comments

51ed029

ebadyano added this to the 1.3.0 milestone Aug 12, 2019

ebadyano added :Benchmark Candidate Management Anything affecting how Rally sets up Elasticsearch :misc Changes that don't affect users directly: linter fixes, test improvements, etc. enhancement Improves the status quo labels Aug 12, 2019

Another round of review.

a4db9f7

danielmitterdorfer requested changes Aug 13, 2019

View reviewed changes

Address comments, add a test for launcher _set_env

d625b2b

danielmitterdorfer requested changes Aug 16, 2019

View reviewed changes

Address latest comments.

fec2fbf

danielmitterdorfer reviewed Aug 16, 2019

View reviewed changes

ebadyano changed the title ~~Revert PR#711 to use ES_JAVA_OPTS for telemetry devices~~ Allow to attach telemetry devices without reprovisioning Aug 16, 2019

Fix to use elif

7c20f4f

danielmitterdorfer approved these changes Aug 19, 2019

View reviewed changes

ebadyano merged commit 556de2d into elastic:master Aug 20, 2019

ebadyano deleted the master-javaopts branch December 16, 2022 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to attach telemetry devices without reprovisioning #737

Allow to attach telemetry devices without reprovisioning #737

ebadyano commented Jul 31, 2019

danielmitterdorfer left a comment

danielmitterdorfer Aug 1, 2019

ebadyano Aug 12, 2019

danielmitterdorfer Aug 1, 2019

danielmitterdorfer Aug 1, 2019

ebadyano Aug 12, 2019

danielmitterdorfer Aug 12, 2019

ebadyano Aug 12, 2019

danielmitterdorfer Aug 12, 2019

danielmitterdorfer Aug 1, 2019

ebadyano Aug 12, 2019

danielmitterdorfer Aug 12, 2019

ebadyano Aug 12, 2019

danielmitterdorfer Aug 12, 2019

danielmitterdorfer left a comment

danielmitterdorfer Aug 13, 2019

danielmitterdorfer Aug 13, 2019

danielmitterdorfer left a comment •

edited

Loading

danielmitterdorfer Aug 16, 2019

danielmitterdorfer Aug 16, 2019

ebadyano commented Aug 16, 2019

danielmitterdorfer left a comment

danielmitterdorfer Aug 16, 2019

danielmitterdorfer left a comment

		@@ -119,6 +119,7 @@ def wait(self):


		class ProcessLauncherTests(TestCase):
		@mock.patch('os.path.join', return_value="/telemetry")

Allow to attach telemetry devices without reprovisioning #737

Allow to attach telemetry devices without reprovisioning #737

Conversation

ebadyano commented Jul 31, 2019

danielmitterdorfer 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

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

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebadyano commented Aug 16, 2019

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer left a comment

Choose a reason for hiding this comment

danielmitterdorfer left a comment •

edited

Loading