Nir Soffer has uploaded a new change for review.
Change subject: utils: Wait for terminated process ......................................................................
utils: Wait for terminated process
utils.terminating was not waiting for a terminated process, passing the process to zombiereaper. This behavior may be disastrous storage and mostly unwanted for other code.
When we run a child process touching shared storage, we must wait for the child process. If we kill the child process without waiting, engine may start the same operation on another host, while the original child process is still running, possibly in interruptible state, trying to write to storage.
To avoid this issue, we always invoke wait() after killing the process.
Special code that do not want to wait for the child process should not use this context manager.
Change-Id: Ida04e2c7ba092efdc2927ed9f460b0098ba2ad94 Signed-off-by: Nir Soffer nsoffer@redhat.com --- M lib/vdsm/utils.py M tests/utilsTests.py 2 files changed, 7 insertions(+), 34 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/24/65324/1
diff --git a/lib/vdsm/utils.py b/lib/vdsm/utils.py index a428c8e..e5c28b8 100644 --- a/lib/vdsm/utils.py +++ b/lib/vdsm/utils.py @@ -738,8 +738,7 @@ if proc.poll() is None: logging.debug('Terminating process pid=%d' % proc.pid) proc.kill() - if proc.poll() is None: - zombiereaper.autoReapPID(proc.pid) + proc.wait() except Exception: logging.exception('Failed to kill process %d' % proc.pid)
diff --git a/tests/utilsTests.py b/tests/utilsTests.py index 388a65d..6ff85d4 100644 --- a/tests/utilsTests.py +++ b/tests/utilsTests.py @@ -42,9 +42,8 @@ from vdsm import cmdutils from vdsm import commands from vdsm import panic -from vdsm.common import zombiereaper
-from monkeypatch import MonkeyPatch, MonkeyPatchScope +from monkeypatch import MonkeyPatch from vmTestsData import VM_STATUS_DUMP from monkeypatch import Patch from testlib import forked, online_cpus, namedTemporaryDir @@ -83,22 +82,11 @@ self.patch.revert()
-def wait_for_removal(path, timeout, wait=0.1): - deadline = utils.monotonic_time() + timeout - while True: - if not os.path.exists(path): - return True - if utils.monotonic_time() > deadline: - return False - time.sleep(wait) - - class TerminatingTests(TestCaseBase):
def setUp(self): self.proc = commands.execCmd([EXT_SLEEP, "2"], sync=False) self.kill_proc = self.proc.kill - self.reaped = set()
def tearDown(self): if self.proc.poll() is None: @@ -108,8 +96,7 @@ def test_terminating(self): with utils.terminating(self.proc): self.assertIsNone(self.proc.poll()) - proc_path = "/proc/%d" % self.proc.pid - self.assertTrue(wait_for_removal(proc_path, timeout=1)) + self.assertEqual(self.proc.returncode, -signal.SIGKILL)
def test_terminating_with_kill_exception(self): class FakeKillError(Exception): @@ -118,24 +105,11 @@ def fake_kill(): raise FakeKillError("fake kill exception")
- with MonkeyPatchScope([(zombiereaper, - 'autoReapPID', - self.reaped.add - )]): - self.proc.kill = fake_kill - with utils.terminating(self.proc): - self.assertIsNone(self.proc.poll()) - self.assertTrue(self.proc.pid not in self.reaped) + self.proc.kill = fake_kill + with utils.terminating(self.proc): + self.assertIsNone(self.proc.poll())
- def test_terminating_with_infected_kill(self): - with MonkeyPatchScope([(zombiereaper, - 'autoReapPID', - self.reaped.add - )]): - self.proc.kill = lambda: None - with utils.terminating(self.proc): - self.assertIsNone(self.proc.poll()) - self.assertTrue(self.proc.pid in self.reaped) + self.assertIsNone(self.proc.returncode)
class RetryTests(TestCaseBase):
gerrit-hooks has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 1:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 1: Verified+1
Verified by the tests.
gerrit-hooks has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 2:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 3:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Nir Soffer has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 3: Continuous-Integration+1
Verfied on travis: https://travis-ci.org/nirs/vdsm/builds/166549485
Francesco Romani has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 3: Code-Review+1
gerrit-hooks has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 4:
* Update tracker: IGNORE, no Bug-Url found * Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url. * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Piotr Kliczewski has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 4: Code-Review+1
gerrit-hooks has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 5:
* Update Tracker::IGNORE, not relevant for branch: master * Check Bug-Url::IGNORE, not relevant for branch: master * Check Public Bug::WARN, no public bug url found * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Dan Kenigsberg has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 5: Code-Review+1
what you say here is very clear; but since https://gerrit.ovirt.org/#/c/51407/ was very explicit about this intended semantics, I'd like to hear from Piotr+Bronheim if they are aware of anything that depends on it, and would be broken now.
Yaniv Bronhaim has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 5:
we don't use terminating context anywhere yet anyway, only in execmd itself when calling communicate which should be stuck if the process still runs. if this context manager waits for the process I wouldn't call it "terminating", it just signals the process and wait for it to exit
Yaniv Bronhaim has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 5:
(1 comment)
https://gerrit.ovirt.org/#/c/65324/5/lib/vdsm/utils.py File lib/vdsm/utils.py:
Line 737: try: Line 738: if proc.poll() is None: Line 739: logging.debug('Terminating process pid=%d' % proc.pid) Line 740: proc.kill() Line 741: proc.wait() Dan said that he doesn't understand my comment - we invented this context to cover proc object in such way that won't leave zombies if the context exists in the middle - for safety to avoid depending on the old death signal that we had.
Now, instead of just trying to kill the process and "reap" it, the context won't continue until the process exits... at least for me it is not the expected behavior. however, we still don't use it so we can change it if that way you see more usages for this context Line 742: except Exception: Line 743: logging.exception('Failed to kill process %d' % proc.pid) Line 744: Line 745:
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has posted comments on this change.
Change subject: utils: Wait for terminated process ......................................................................
Patch Set 5: Code-Review+2
Given Yaniv report, I suggest that we introduce a try_terminate() method if and when we need the semantics of attempting termination, but not waiting for actual death.
From Dan Kenigsberg danken@redhat.com:
Dan Kenigsberg has submitted this change and it was merged.
Change subject: utils: Wait for terminated process ......................................................................
utils: Wait for terminated process
utils.terminating was not waiting for a terminated process, passing the process to zombiereaper. This behavior may be disastrous storage and mostly unwanted for other code.
When we run a child process touching shared storage, we must wait for the child process. If we kill the child process without waiting, engine may start the same operation on another host, while the original child process is still running, possibly in interruptible state, trying to write to storage.
To avoid this issue, we always invoke wait() after killing the process.
Special code that do not want to wait for the child process should not use this context manager.
Change-Id: Ida04e2c7ba092efdc2927ed9f460b0098ba2ad94 Signed-off-by: Nir Soffer nsoffer@redhat.com Reviewed-on: https://gerrit.ovirt.org/65324 Reviewed-by: Francesco Romani fromani@redhat.com Reviewed-by: Piotr Kliczewski piotr.kliczewski@gmail.com Continuous-Integration: Jenkins CI Reviewed-by: Dan Kenigsberg danken@redhat.com --- M lib/vdsm/utils.py M tests/utilsTests.py 2 files changed, 7 insertions(+), 34 deletions(-)
Approvals: Piotr Kliczewski: Looks good to me, but someone else must approve Nir Soffer: Verified Jenkins CI: Passed CI tests Dan Kenigsberg: Looks good to me, approved Francesco Romani: Looks good to me, but someone else must approve
vdsm-patches@lists.fedorahosted.org