Use concurrent.futures to fix multiprocessing issues #53

jarovo · 2020-09-22T00:13:57Z

This makes the behavior of broker in case of exception rised in the child much better (without this PR, there is a deadlock and possible no output on the terminal about the exception. With this PR the exception is propagated to the parent process.

There were issues that I think were arising from using a decorator that replaced the decorated method with some nested method (the wraps function is commonly used in that case). It seems to me such method cannot be pickled but that seemed necessary for sending it to the concurrent.futures

JacobCallahan · 2020-09-24T14:28:11Z

broker/providers/test_provider.py

@@ -37,7 +37,10 @@ def _set_attributes(self, host_inst, broker_args=None):
    def construct_host(self, provider_params, host_classes, **kwargs):
        host_params = provider_params.copy()
        host_params.update(kwargs)
-        host_inst = host_classes[host_params["host_type"]](**host_params)
+        host_type = host_classes[host_params["host_type"]]
+        host_type.connect = MagicMock()


have you considered using the MockStub class from helpers here?

It is actually not needed to mock something here. The connect got called due to a bug in my PR which I have fixed now.

I like to use MagicMock though. It is in standard library, quite well documented and thus it is likely the other developers will know how to use it. I did use it to make the code raise an exception in broker._act in other test. This was done trough the pytest-mock that patches the class only for the time the test needs it.

.travis.yml

broker/broker.py

broker/providers/test_provider.py

tests/test_broker.py

The CF is easier to use and less error prone than using the multiprocessing directly. It also allows easier switch between processes and threads. Another advantage is error and treacebacks reporting to the parent process is better.

This is to work arround issue with TestProvider class attempts to be collected as test class.

JacobCallahan

some small changes

broker/broker.py

JacobCallahan

ACK

JacobCallahan · 2020-10-02T18:30:08Z

@mshriver do you want to weigh in as well?

mshriver · 2020-10-05T13:30:45Z

broker/broker.py

+            with self.EXECUTOR(max_workers=max_workers_count) as workers:
+                completed_futures = as_completed(workers.submit(self.func, instance, *args, **kwargs)
+                                                 for _ in range(count))
+                for f in completed_futures:


~~Might be code golfing here, but couldn't you directly return here instead of initializing and extending the list?~~

return [f.result() for f in completed_futures]

I take that back, can't unpack iterables within comprehension to get the extend behavior.

broker/broker.py

jarovo · 2020-10-05T15:57:52Z

There are some problems with the inventory.yaml unlink when doing multiple checkouts. I recommend not merging before it is known why this happens.

Some of the mp tests were failing like this: tests/test_broker.py::test_mp_checkout FAILED [ 50%] tests/test_broker.py::test_mp_checkout_twice PASSED [ 60%] tests/test_broker.py::test_mp_decorator PASSED [ 70%] tests/providers/test_ansible_tower.py::test_exec_workflow PASSED [ 80%] tests/providers/test_ansible_tower.py::test_host_creation PASSED [ 90%] tests/providers/test_ansible_tower.py::test_workflow_lookup_failure PASSED [100%] ========================================================================================== FAILURES ========================================================================================== ______________________________________________________________________________________ test_mp_checkout ______________________________________________________________________________________ concurrent.futures.process._RemoteTraceback: """ Traceback (most recent call last): File "/usr/lib64/python3.9/concurrent/futures/process.py", line 243, in _process_worker r = call_item.fn(*call_item.args, **call_item.kwargs) File "/home/jhenner/work/sat/broker/broker/broker.py", line 116, in _checkout helpers.update_inventory(add=host.to_dict()) File "/home/jhenner/work/sat/broker/broker/helpers.py", line 154, in update_inventory inventory_file.unlink() # This making mp tests to fail. Perhaps a race condition. File "/usr/lib64/python3.9/pathlib.py", line 1340, in unlink self._accessor.unlink(self) FileNotFoundError: [Errno 2] No such file or directory: 'inventory.yaml' """ The above exception was the direct cause of the following exception: def test_mp_checkout(): """Test that broker can checkout multiple hosts using multiprocessing""" broker_inst = broker.VMBroker(nick="test_nick", _count=2) > broker_inst.checkout() tests/test_broker.py:50: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ broker/broker.py:126: in checkout hosts = self._checkout(connect=connect) broker/broker.py:62: in mp_split results.extend(f.result()) /usr/lib64/python3.9/concurrent/futures/_base.py:433: in result return self.__get_result() _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <Future at 0x7f4c662e9790 state=finished raised FileNotFoundError> def __get_result(self): if self._exception: > raise self._exception E FileNotFoundError: [Errno 2] No such file or directory: 'inventory.yaml' It seem sto be a run condition when working with the inventory file. Fixing by moving the work with the inventory out of the concurrently executed code.

This makes the tests/test_broker.py::test_mp_checkout fail more often when there is a run condition disovered in #53

jarovo · 2020-10-06T18:00:28Z

I think the problem with race condition between the children of single process is fixed by this PR. We are ready to merge @mshriver @JacobCallahan

This was referenced Sep 22, 2020

Identify why test_mp_checkout is failing and how to fix it #44

Closed

Fix pickling, enable test_mp_checkout #55

Merged

JacobCallahan reviewed Sep 24, 2020

View reviewed changes

jarovo commented Sep 24, 2020

View reviewed changes

.travis.yml Show resolved Hide resolved

jarovo commented Sep 24, 2020

View reviewed changes

broker/broker.py Outdated Show resolved Hide resolved

jarovo requested a review from JacobCallahan September 24, 2020 18:07

jarovo changed the title ~~Use concurrent.futures work fix multiprocessing issues~~ Use concurrent.futures to fix multiprocessing issues Sep 24, 2020

JacobCallahan requested changes Sep 24, 2020

View reviewed changes

broker/broker.py Outdated Show resolved Hide resolved

JacobCallahan requested changes Sep 25, 2020

View reviewed changes

broker/broker.py Show resolved Hide resolved

broker/broker.py Show resolved Hide resolved

broker/providers/test_provider.py Outdated Show resolved Hide resolved

tests/test_broker.py Outdated Show resolved Hide resolved

Jaroslav Henner added 11 commits September 30, 2020 21:24

Add test for two multiprocessed checkouts

9b8c8fe

Extract mp_split, fix multiple checkouts issue.

843e992

Add test for exception during checkout.

e52e645

Make SomeException pickable

2744ce0

Test the mp_split directly.

5e69f4f

Use concurrent futures.

23af46f

The CF is easier to use and less error prone than using the multiprocessing directly. It also allows easier switch between processes and threads. Another advantage is error and treacebacks reporting to the parent process is better.

Add test requirements

7b34613

Don't run pytest on other dir than tests

b2f2f23

This is to work arround issue with TestProvider class attempts to be collected as test class.

Remove annotations

b986bef

Put back the host classes

fbf5d9f

Make MAX_WORKERS default to number of items being produced.

98c05ae

jarovo marked this pull request as ready for review September 30, 2020 19:26

jarovo requested a review from JacobCallahan September 30, 2020 19:26

JacobCallahan requested changes Oct 1, 2020

View reviewed changes

broker/broker.py Outdated Show resolved Hide resolved

broker/broker.py Outdated Show resolved Hide resolved

Implement review suggestions.

481fadf

JacobCallahan approved these changes Oct 2, 2020

View reviewed changes

mshriver reviewed Oct 5, 2020

View reviewed changes

broker/broker.py Outdated Show resolved Hide resolved

Remove the debug f string to support Py<3.8

b7caaf2

jarovo marked this pull request as draft October 5, 2020 15:57

jarovo mentioned this pull request Oct 5, 2020

Remove PyPi secrets from git, Remove Travis, use Github Actions and Py3.6 Py3.7 testing #64

Closed

Jaroslav Henner added 2 commits October 5, 2020 23:39

Make the mp_checkout test discover show the run condition more often.

91cd187

This makes the tests/test_broker.py::test_mp_checkout fail more often when there is a run condition disovered in #53

jarovo marked this pull request as ready for review October 6, 2020 11:41

jarovo mentioned this pull request Oct 6, 2020

Race condition danger when using multiple instances of broker #65

Closed

JacobCallahan merged commit ed0868e into SatelliteQE:master Oct 12, 2020

jarovo mentioned this pull request Oct 13, 2020

Don't run pytest on other dir than tests #59

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use concurrent.futures to fix multiprocessing issues #53

Use concurrent.futures to fix multiprocessing issues #53

jarovo commented Sep 22, 2020 •

edited

JacobCallahan Sep 24, 2020

jarovo Sep 24, 2020 •

edited

JacobCallahan left a comment

JacobCallahan left a comment

JacobCallahan commented Oct 2, 2020

mshriver Oct 5, 2020 •

edited

jarovo commented Oct 5, 2020 •

edited

jarovo commented Oct 6, 2020

Use concurrent.futures to fix multiprocessing issues #53

Use concurrent.futures to fix multiprocessing issues #53

Conversation

jarovo commented Sep 22, 2020 • edited

JacobCallahan Sep 24, 2020

Choose a reason for hiding this comment

jarovo Sep 24, 2020 • edited

Choose a reason for hiding this comment

JacobCallahan left a comment

Choose a reason for hiding this comment

JacobCallahan left a comment

Choose a reason for hiding this comment

JacobCallahan commented Oct 2, 2020

mshriver Oct 5, 2020 • edited

Choose a reason for hiding this comment

jarovo commented Oct 5, 2020 • edited

jarovo commented Oct 6, 2020

jarovo commented Sep 22, 2020 •

edited

jarovo Sep 24, 2020 •

edited

mshriver Oct 5, 2020 •

edited

jarovo commented Oct 5, 2020 •

edited