Add flag to handle running processes automatically #954

bartier · 2020-04-06T21:55:19Z

This PR implements a explicit flag --kill-running-processes to Rally that kills automatically previous running Rally benchmarks.

This is useful when you stop your terminal with CTRL-C and the Rally processes keeps running and the next time you run Rally you'll receive an error something like that:

[ERROR] Cannot race. There are other Rally processes running on this machine (PIDs: [4122, 4123, 4124, 4172, 4234, 4235]) but only one Rally benchmark is allowed to run at the same time. Please check and terminate these processes and retry again.

Instead of receiving the message above, you can add --kill-running-processes to automatically terminate these processes for you.

Ps: I'm not sure if I handled correctly the method with_actor_system. I tried follow the logic in this method and make the necessary changes.

def with_actor_system(runnable, cfg, kill_running_processes):
    import thespian.actors
    logger = logging.getLogger(__name__)
    already_running = actor.actor_system_already_running()
    logger.info("Actor system already running locally? [%s]", str(already_running))
    try:
        if already_running and kill_running_processes:
            actors = actor.bootstrap_actor_system(try_join=False, prefer_local_only=already_running)
        else:
            actors = actor.bootstrap_actor_system(try_join=already_running, prefer_local_only=not already_running)

        # We can only support remote benchmarks if we have a dedicated daemon that is not only bound to 127.0.0.1
        cfg.add(config.Scope.application, "system", "remote.benchmarking.supported", already_running)

Link to with_actor_system changes

If the changes in with_actor_system is not applied, I receive the following errors:

Refers to #922

danielmitterdorfer

Thanks for the PR. I did a first pass and left some suggestions.

danielmitterdorfer · 2020-04-08T08:22:30Z

esrally/rally.py


+        if kill_running_processes:
+            console.info("Killing running processes ...", flush=True)
+            for pid in pids:


We already had this functionality prior to 8adb0f8. Can you please check how it was done there and restore that functionality? I also don't recall that it was necessary to make the actor system bootstrap code aware of this so I'd try to avoid it (after restoring the functionality removed in 8adb0f8).

Sure! I restored the previous functionality and avoided changes in the actor system bootstrap code. Thanks for your suggestion. Can you check again if something needs to be changed?

danielmitterdorfer · 2020-04-08T08:23:03Z

esrally/rally.py


+        if kill_running_processes:
+            console.info("Killing running processes ...", flush=True)


I think a log message is sufficient?

Changed. Can you check again?

danielmitterdorfer · 2020-04-08T08:23:50Z

docs/command_line_reference.rst

+``kill-running-processes``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Only one Rally benchmark is allowed to run at the same time. If any processes is running, it is going to kill them and allow Rally to continue to run a new benchmark.


I think we could add an explanation why we want that (in order to ensure that benchmark results are not skewed due to unintentionally running multiple benchmarks at the same time).

Added more information in the docs about why we want that. Can you check again?

danielmitterdorfer · 2020-04-08T12:07:03Z

@elasticmachine test this please

danielmitterdorfer

Thanks! That looks much better; I left a few more comments.

danielmitterdorfer · 2020-04-08T12:06:40Z

docs/command_line_reference.rst

+``kill-running-processes``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Rally attempts to generate benchmark results that are not skewed unintentionally. Consequently, if some benchmark is running, Rally will not allow you to start another one. Instead, you should stop the current benchmark and start another one manually. This flag can be added to handle automatically for you this stop-start processes.


nit: for you this stop-start process -> this process for you?

danielmitterdorfer · 2020-04-08T12:09:33Z

tests/utils/process_test.py

+        self.assertTrue(rally_process_e.killed)
+        self.assertTrue(rally_process_mac.killed)
+        self.assertFalse(own_rally_process.killed)
+        self.assertFalse(night_rally_process.killed)


Nit: Missing trailing new line

danielmitterdorfer · 2020-04-08T12:17:11Z

esrally/rally.py

+        if other_rally_processes:
+            pids = [p.pid for p in other_rally_processes]
+
+            msg = "There are other Rally processes running on this machine (PIDs: %s) but only one Rally benchmark " \


We are using .format and recently f-strings so can you please rewrite this as:

msg = f"There are other Rally processes running on this machine (PIDs: {pids}) but only one Rally " \ f"benchmark is allowed to run at the same time.\n\nPlease rerun with --kill-running-processes to " \ f"terminate them automatically."

(I also suggested a slightly different wording)

danielmitterdorfer · 2020-04-08T12:20:55Z

esrally/rally.py



-def with_actor_system(runnable, cfg):
+def with_actor_system(runnable, cfg, kill_running_processes):


This seems to be a leftover?

danielmitterdorfer · 2020-04-08T12:20:59Z

esrally/rally.py


-    with_actor_system(racecontrol.run, cfg)
+    with_actor_system(racecontrol.run, cfg, kill_running_processes)


This seems to be a leftover?

danielmitterdorfer · 2020-04-08T12:22:14Z

esrally/rally.py

-        raise exceptions.RallyError(msg)
+    logger = logging.getLogger(__name__)
+
+    kill_running_processes = cfg.opts("system", "kill.running.processes", default_value=False, mandatory=False)


I think we can simplify this to:

kill_running_processes = cfg.opts("system", "kill.running.processes")

The value should be mandatory at this point because we added it previously in bootstrap code?

danielmitterdorfer · 2020-04-08T12:23:45Z

esrally/rally.py

@@ -23,6 +23,7 @@
 import sys
 import time
 import uuid
+import signal


The linter in CI raised an error that this import is unused:

12:20:20 esrally/rally.py:26:0: W0611: Unused import signal (unused-import)

Can you please remove it?

bartier · 2020-04-08T12:53:29Z

@danielmitterdorfer I made the changes you suggested. Can you check again and run the tests?

danielmitterdorfer · 2020-04-08T12:56:45Z

@elasticmachine test this please

danielmitterdorfer

Thanks for your change @bartier! It looks good to me now and the CI build passes as well. I'll merge it soon and it wil be released with the next Rally release 1.5.0.

bartier · 2020-04-08T14:21:42Z

@danielmitterdorfer
Some issues are too complex for me to understand, but I try to help any way I can, as well improve my Python skills =)

Thanks for your attention. Glad to help!

Handle running processes automatically with a explicit flag

746d602

bartier mentioned this pull request Apr 8, 2020

[Feature request] Handle running processes automatically with a explicit flag #922

Closed

danielmitterdorfer reviewed Apr 8, 2020

View reviewed changes

danielmitterdorfer added :Usability Makes Rally easier to use enhancement Improves the status quo labels Apr 8, 2020

danielmitterdorfer added this to the 1.5.0 milestone Apr 8, 2020

bartier added 2 commits April 8, 2020 08:14

Code Review

4e820f3

Restore old test case

dc46560

danielmitterdorfer reviewed Apr 8, 2020

View reviewed changes

bartier added 2 commits April 8, 2020 09:24

Remove unused import

7a4f74a

Code Review

5e6acb9

danielmitterdorfer approved these changes Apr 8, 2020

View reviewed changes

danielmitterdorfer merged commit 97eaa37 into elastic:master Apr 8, 2020

bartier mentioned this pull request Apr 8, 2020

Tipo: F / Cidade: Campinas - SP - BR elastic/Elastic-Contributor-Program#561

Open

danielmitterdorfer mentioned this pull request Feb 16, 2021

Unable to use --kill-running-proccesses #1186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to handle running processes automatically #954

Add flag to handle running processes automatically #954

bartier commented Apr 6, 2020

danielmitterdorfer left a comment

danielmitterdorfer Apr 8, 2020

bartier Apr 8, 2020

danielmitterdorfer Apr 8, 2020

bartier Apr 8, 2020

danielmitterdorfer Apr 8, 2020

bartier Apr 8, 2020

danielmitterdorfer commented Apr 8, 2020

danielmitterdorfer left a comment

danielmitterdorfer Apr 8, 2020

danielmitterdorfer Apr 8, 2020

danielmitterdorfer Apr 8, 2020

danielmitterdorfer Apr 8, 2020

danielmitterdorfer Apr 8, 2020

danielmitterdorfer Apr 8, 2020

danielmitterdorfer Apr 8, 2020

bartier commented Apr 8, 2020

danielmitterdorfer commented Apr 8, 2020

danielmitterdorfer left a comment

bartier commented Apr 8, 2020


		if kill_running_processes:
		console.info("Killing running processes ...", flush=True)



		def with_actor_system(runnable, cfg):
		def with_actor_system(runnable, cfg, kill_running_processes):


		with_actor_system(racecontrol.run, cfg)
		with_actor_system(racecontrol.run, cfg, kill_running_processes)

Add flag to handle running processes automatically #954

Add flag to handle running processes automatically #954

Conversation

bartier commented Apr 6, 2020

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

danielmitterdorfer commented Apr 8, 2020

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

bartier commented Apr 8, 2020

danielmitterdorfer commented Apr 8, 2020

danielmitterdorfer left a comment

Choose a reason for hiding this comment

bartier commented Apr 8, 2020