Provision, start, stop subcommands #722

drawlerr · 2019-07-04T03:45:10Z

Implement provision, start and stop subcommands for independently managing the benchmarking candidate.

Relates #697
Closes #733

First swing at provision, start, stop functions.

Report trial-id, pid, container_id regardless.

dliappis · 2019-07-08T13:40:55Z

@drawlerr I took an initial pass and have the following top level comments:

stop command doesn't seem to wipe away unneeded files (see Allow to manage Elasticsearch nodes separately from benchmarking #697 (comment) #stop)

e.g. after ./rally stop --trial-id 9cbdf669-d2c4-424e-8a55-df4ee1d17ce0

I still see:

$ du -sh ~/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/*
0B	/Users/dl/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/heapdump
423M	/Users/dl/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/install
48K	/Users/dl/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/logs

Does --node-ip get used for all_node_ids?

To setup a cluster we need all_node_ips to contain all IP addresses as it's used by the elasticsearch.yml template for discovery.
I know this is what #697 specifies and this PR implements it well, but the trial-id needs to be collected by some orchestrator (shell script, ansible play ...) and currently it's contained between the Rally banner and the success message:
```
$ ./rally provision --node-ip 127.0.0.1 --docker --distribution-version=7.2.0

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

c9a4f171-48b4-48ad-8f4e-28c9ad262922

-------------------------------
[INFO] SUCCESS (took 1 seconds)
-------------------------------```
```
So the process invoking provision will have a hard time matching the id; should this be something like trial-id: ...? I was thinking of a simple json structure but again the orchestrator could have trouble skipping noise and multiline matching the json structure.
Alternatively we could skip printing the banner and the final info message, but perhaps it's an unnecessary complication. @danielmitterdorfer WDYT?

dliappis

I left some initial comments.

drawlerr · 2019-07-08T14:45:41Z

@drawlerr I took an initial pass and have the following top level comments:
stop command doesn't seem to wipe away unneeded files (see #697 (comment) #stop)
e.g. after ./rally stop --trial-id 9cbdf669-d2c4-424e-8a55-df4ee1d17ce0
I still see:
$ du -sh ~/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/*
0B	/Users/dl/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/heapdump
423M	/Users/dl/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/install
48K	/Users/dl/.rally/benchmarks/races/9cbdf669-d2c4-424e-8a55-df4ee1d17ce0/rally-node-0/logs

I didn't realize this was a requirement of the "stop" subcommand. Sounds maybe like a separate "cleanup" command?

Does --node-ip get used for all_node_ips?
To setup a cluster we need all_node_ips to contain all IP addresses as it's used by the elasticsearch.yml template for discovery.

It does, but it should probably be a list instead of a single element. I'll fix it.

I know this is what #697 specifies and this PR implements it well, but the trial-id needs to be collected by some orchestrator (shell script, ansible play ...) and currently it's contained between the Rally banner and the success message:
$ ./rally provision --node-ip 127.0.0.1 --docker --distribution-version=7.2.0

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

c9a4f171-48b4-48ad-8f4e-28c9ad262922

-------------------------------
[INFO] SUCCESS (took 1 seconds)
-------------------------------```
So the process invoking provision will have a hard time matching the id; should this be something like trial-id: ...? I was thinking of a simple json structure but again the orchestrator could have trouble skipping noise and multiline matching the json structure.
Alternatively we could skip printing the banner and the final info message, but perhaps it's an unnecessary complication. @danielmitterdorfer WDYT?

This is tricky. I used quiet mode (--quiet) to suppress all output except for the "forced" output of the UUID. (see integration-test.sh for example usage)

dliappis · 2019-07-09T15:47:20Z

Unrelated, but mentioned it since this PR touches provisioner_test.py I observed that most of the unittests under BareProvisionerTests, e.g. test_prepare_without_plugins while succeeding in their asserts, have an underlying failure with subprocess.Popen() invoked from process#run_subprocess_with_logging -- it manifests e.g. if you run the unittest from the IDE:

Ran 1 test in 0.007s

OK
Could not execute command.
Traceback (most recent call last):
  File "/Users/dl/source/elastic/rally/esrally/utils/process.py", line 52, in exit_status_as_bool
    return_code = runnable()
  File "/Users/dl/source/elastic/rally/esrally/utils/jvm.py", line 33, in <lambda>
    return process.exit_status_as_bool(lambda: process.run_subprocess_with_logging("%s/bin/java %s -version" % (java_home, option)))
  File "/Users/dl/source/elastic/rally/esrally/utils/process.py", line 76, in run_subprocess_with_logging
    with subprocess.Popen(command_line_args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT, env=env) as command_line_process:
  File "/Users/dl/.pyenv/versions/3.6.5/lib/python3.6/subprocess.py", line 709, in __init__
    restore_signals, start_new_session)
  File "/Users/dl/.pyenv/versions/3.6.5/lib/python3.6/subprocess.py", line 1344, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: '/usr/local/javas/java8/bin/java': '/usr/local/javas/java8/bin/java'

Process finished with exit code 0

This can be fixed by e.g. patching the afforementioned function e.g. with: @mock.patch("esrally.utils.process.run_subprocess_with_logging")

dliappis

I took another pass and left a few more comments re: use of --node-ips and how it translates to all_node_ips.

tests/mechanic/provisioner_test.py

esrally/mechanic/mechanic.py

dliappis · 2019-07-10T15:53:49Z

esrally/rally.py

+    "list": dispatch_list,
+    "race": race,
+    "download": mechanic.download,
+    "provision": mechanic.provision,


I think we want to document the provision and start / stop commands either in this PR or in a separate PR. @danielmitterdorfer WDYT?

PRs should be self-contained thus I'd opt for documenting it in this PR.

danielmitterdorfer · 2019-07-11T06:01:39Z

I think we want to document the provision and start / stop commands either in this PR or in a separate PR. @danielmitterdorfer WDYT?

Docs should always go along with the code changes, i.e. they should be included in this PR not in a separate one.

danielmitterdorfer

I did a first pass and left some comments and suggestions. On a high level I think the following needs to be addressed:

Handling of telemetry devices
Only allowing to provision one node

I think we also need an option to cleanup an installation on termination (i.e. in the stop subcommand). IMHO this does not need to be a separate subcommand.

I also spotted a lot of unrelated changes (like reformattings + it seems to be based on a yet unmerged PR) which made the PR harder to review and I suggest that we move such changes in the future to dedicated PRs.

docs/migrate.rst

tests/metrics_test.py

integration-test.sh

esrally/reporter.py

danielmitterdorfer · 2019-07-12T08:10:44Z

esrally/rally.py

+    "list": dispatch_list,
+    "race": race,
+    "download": mechanic.download,
+    "provision": mechanic.provision,


PRs should be self-contained thus I'd opt for documenting it in this PR.

esrally/mechanic/telemetry.py

esrally/rally.py

danielmitterdorfer · 2019-07-12T12:14:38Z

esrally/rally.py

+        action="store_true",
+        default=False
+    )
+    for p in [start_parser, stop_parser]:


This is missing support to enable telemetry devices.

…on start and stop.

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

Thanks so much for working on this PR! As we've discussed offline earlier, there are too many changes that we need to make before addressing this properly (mainly around gathering metrics properly with the new subcommand structure) and we should tackle them first in separate PRs. When we've finished this work, we can revisit this change again. It is very likely that this will happen in a separate PR but we will draw from your work here and the knowledge you've acquired. As such, I'm going to close this one unmerged as it is likely that this code as it is now will not be merged but rather be used as reference later on.

Dennis Lawler added 17 commits June 25, 2019 13:30

Change filestore to be indexed by unique ID

19d7d92

Merge branch 'master' into unique-id

f59fc81

Remove unused find_by_timestamp.

3fb85f8

Update Elasticsearch store to use find_by_trial_id.

527e93a

Update docker.rst to reflect new folder structure with unique IDs.

6aaf98c

Add note about races stored by ID.

121cddf

Move if-else to dict.

af6f84b

Provision, start, stop

c963108

First swing at provision, start, stop functions.

Fix some defaults for provision subcommand.

9cbb99b

Use standard csv_to_list.

c34f95a

Add docker support.

bd22b83

Give docker opt a default, make it non-mandatory.

a6cc00d

Fix docker stop command.

4dd499c

Use unique race ID for node config.

e5c6c2a

Add --quiet to provision, start, stop.

9f61701

Report trial-id, pid, container_id regardless.

Add integration test.

530fe3a

Remove unused import.

df26aef

drawlerr requested review from dliappis and ebadyano July 4, 2019 03:45

drawlerr added this to the 1.3.0 milestone Jul 4, 2019

dliappis mentioned this pull request Jul 5, 2019

Update DiskIo telemetry device to persist the counters #721

Closed

dliappis reviewed Jul 8, 2019

View reviewed changes

dliappis requested a review from danielmitterdorfer July 8, 2019 13:41

Dennis Lawler added 5 commits July 8, 2019 09:58

Fix up naming for some private functions in mechanic.

900f843

Add args for node ips.

f7947f9

Fix unit test breakage.

7b70884

Fix node_ips to no longer be a mandatory config param.

e8fd47b

Fix docker_provisioner's DockerProvisioner instantiation.

18d00c7

Fix up broken provisioner_test tests.

318c27d

dliappis reviewed Jul 9, 2019

View reviewed changes

tests/mechanic/provisioner_test.py Outdated Show resolved Hide resolved

esrally/mechanic/mechanic.py Outdated Show resolved Hide resolved

Minor cleanup in provisioner_test.

06ede97

dliappis reviewed Jul 10, 2019

View reviewed changes

Make node.ips mandatory, give it a better default.

9daabcb

Dennis Lawler added 2 commits July 11, 2019 08:19

Add Race ID to race list.

e08da42

Change compare args from timestamp to ID.

d9f4af9

danielmitterdorfer requested changes Jul 12, 2019

View reviewed changes

Dennis Lawler added 7 commits July 12, 2019 15:54

Minor cleanup from code review.

9237726

Remove function-level invocation change from integration-test.sh.

915e21a

Remove '...and exit' from parser help strings.

d6ba661

Provision only 1 node ID at a time.

6e0d8ec

Add metrics store to start, stop.

0886830

Merge branch 'master' into provision-start-stop

81b8f34

Cleanup imports in mechanic.py.

41ba07c

danielmitterdorfer mentioned this pull request Jul 23, 2019

Allow to "attach" to an existing Elasticsearch node #349

Closed

danielmitterdorfer self-requested a review July 23, 2019 15:13

Dennis Lawler added 4 commits July 23, 2019 19:24

Update integration test with challenge arg.

0682e52

Change Telemetry to use generic params dictionary, provide telemetry …

63766e5

…on start and stop.

Fix unit test breakage cause by changes to JFR telemetry.

809f0d8

Fix GC telemetry too.

e852b21

danielmitterdorfer mentioned this pull request Jul 24, 2019

Update DiskIo telemetry device to persist the counters #731

Merged

Move Docker-related utility functions into DockerLauncher.

aaed4c0

danielmitterdorfer closed this Aug 1, 2019

danielmitterdorfer removed this from the 1.3.0 milestone Aug 1, 2019

dliappis mentioned this pull request Dec 2, 2019

Fail when distribution-version is used together with benchmark-only pipeline #833

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provision, start, stop subcommands #722

Provision, start, stop subcommands #722

drawlerr commented Jul 4, 2019 •

edited

Loading

dliappis commented Jul 8, 2019

dliappis left a comment

drawlerr commented Jul 8, 2019 •

edited by dliappis

Loading

dliappis commented Jul 9, 2019

dliappis left a comment

dliappis Jul 10, 2019

danielmitterdorfer Jul 12, 2019

danielmitterdorfer commented Jul 11, 2019

danielmitterdorfer left a comment

danielmitterdorfer Jul 12, 2019

danielmitterdorfer Jul 12, 2019

danielmitterdorfer commented Aug 1, 2019

Provision, start, stop subcommands #722

Provision, start, stop subcommands #722

Conversation

drawlerr commented Jul 4, 2019 • edited Loading

dliappis commented Jul 8, 2019

dliappis left a comment

Choose a reason for hiding this comment

drawlerr commented Jul 8, 2019 • edited by dliappis Loading

dliappis commented Jul 9, 2019

dliappis left a comment

Choose a reason for hiding this comment

dliappis Jul 10, 2019

Choose a reason for hiding this comment

danielmitterdorfer Jul 12, 2019

Choose a reason for hiding this comment

danielmitterdorfer commented Jul 11, 2019

danielmitterdorfer left a comment

Choose a reason for hiding this comment

danielmitterdorfer Jul 12, 2019

Choose a reason for hiding this comment

danielmitterdorfer Jul 12, 2019

Choose a reason for hiding this comment

danielmitterdorfer commented Aug 1, 2019

drawlerr commented Jul 4, 2019 •

edited

Loading

drawlerr commented Jul 8, 2019 •

edited by dliappis

Loading