Repository: jcrist/skein (python)

Issue:
Unable to start a new skein client daemon after having killed the previous one
For some reason, the skein client daemon can be abrupty killed. It turns out we can't start a new skein client daemon after that for the following reason:
When starting a new skein client daemon, a file ~/.skein/daemon is created. This file contains information such that socker address and PID of the skein client daemon. When the skein client daemon is abruptly killed, the process is killed but the file still exists. When trying to start a new skein client daemon, skein first checks whether a daemon is already started by looking for the file ~/.skein/daemon. If it exists, it is assumed a daemon is already started, otherwise it is assumed no daemon is started.
This issue consists in robustifying the way skein checks whether or not a daemon is already started.

Pull request description:
Check if PID exists in global driver
Previously we'd fail loudly if the driver file had a pid/address that was no longer correct. Users would have to run:

```
skein driver stop --force
skein driver start
```

in these situations to get a new working global driver. Now we check if the PID doesn't exist, and handle these cases specially:

- `skein driver start` warns that an old driver had died, and starts a new one.
- `skein driver stop` no longer requires `--force` to handle this case (`--force` is still required if the PID exists, but can't be connected with).

Fixes #140

Patch:
diff --git a/skein/core.py b/skein/core.py
index b128fa5..091022a 100644
--- a/skein/core.py
+++ b/skein/core.py
@@ -25,7 +25,7 @@ from .model import (Security, ApplicationSpec, ApplicationReport,
                     ApplicationState, ContainerState, Container,
                     FinalStatus, Resources, container_instance_from_string,
                     LogLevel)
-from .utils import cached_property, grpc_fork_support_disabled
+from .utils import cached_property, grpc_fork_support_disabled, pid_exists
 
 
 __all__ = ('Client', 'ApplicationClient', 'properties')
@@ -393,12 +393,19 @@ class Client(_ClientBase):
         address : str
             The address of the driver
         """
-        try:
-            client = Client.from_global_driver()
-        except DriverNotRunningError:
-            pass
-        else:
-            return client.address
+        address, pid = _read_driver()
+
+        if address is not None:
+            try:
+                Client(address=address)
+                return address
+            except ConnectionError:
+                if pid_exists(pid):
+                    # PID exists, but we can't connect, reraise
+                    raise
+                # PID doesn't exist, warn and continue as normal
+                context.warn("Previous driver at %s, PID %d has died. Restarting."
+                             % (address, pid))
         address, _ = _start_driver(set_global=True,
                                    keytab=keytab,
                                    principal=principal,
@@ -426,7 +433,13 @@ class Client(_ClientBase):
 
         if not force:
             # Attempt to connect first, errors on failure
-            Client(address=address)
+            try:
+                Client(address=address)
+            except ConnectionError:
+                if pid_exists(pid):
+                    # PID exists, but we can't connect, reraise
+                    raise
+                # PID doesn't exist, continue cleanup as normal
         try:
             os.kill(pid, signal.SIGTERM)
         except OSError as exc:
diff --git a/skein/utils.py b/skein/utils.py
index 2dc6319..0d3fb6c 100644
--- a/skein/utils.py
+++ b/skein/utils.py
@@ -1,5 +1,6 @@
 from __future__ import print_function, division, absolute_import
 
+import errno
 import os
 from contextlib import contextmanager
 from datetime import datetime, timedelta
@@ -7,6 +8,15 @@ from datetime import datetime, timedelta
 from .compatibility import unicode, UTC
 
 
+def pid_exists(pid):
+    try:
+        os.kill(pid, 0)
+    except OSError as exc:
+        if exc.errno == errno.ESRCH:
+            return False
+    return True
+
+
 @contextmanager
 def grpc_fork_support_disabled():
     """Temporarily disable fork support in gRPC.


Test patch:
diff --git a/skein/test/conftest.py b/skein/test/conftest.py
index ce4cf98..3e9addc 100644
--- a/skein/test/conftest.py
+++ b/skein/test/conftest.py
@@ -10,14 +10,6 @@ import pytest
 import skein
 
 
-def pid_exists(pid):
-    try:
-        os.kill(pid, 0)
-    except OSError:
-        return False
-    return True
-
-
 @contextmanager
 def set_skein_config(tmpdir):
     tmpdir = str(tmpdir)
diff --git a/skein/test/test_cli.py b/skein/test/test_cli.py
index 14a26eb..bd21cf4 100644
--- a/skein/test/test_cli.py
+++ b/skein/test/test_cli.py
@@ -14,10 +14,10 @@ from skein.compatibility import PY2
 from skein.exceptions import context
 from skein.cli import main
 from skein.core import _write_driver
+from skein.utils import pid_exists
 from skein.test.conftest import (run_application, sleep_until_killed,
                                  check_is_shutdown, wait_for_containers,
-                                 set_skein_config, ensure_shutdown,
-                                 pid_exists)
+                                 set_skein_config, ensure_shutdown)
 
 
 bad_spec_yaml = """
@@ -224,8 +224,19 @@ def test_cli_driver_force_stop(tmpdir, capsys):
         with closing(sock):
             # PID is not a skein driver
             _write_driver(address, proc.pid)
+            with open(driver_file) as fil:
+                contents = fil.read()
             assert os.path.exists(driver_file)
 
+            run_command('driver start', error=True)
+            out, err = capsys.readouterr()
+            assert not out
+            assert err
+            assert os.path.exists(driver_file)
+            with open(driver_file) as fil:
+                contents2 = fil.read()
+            assert contents == contents2
+
             run_command('driver stop', error=True)
             out, err = capsys.readouterr()
             assert not out
@@ -239,18 +250,36 @@ def test_cli_driver_force_stop(tmpdir, capsys):
             assert not os.path.exists(driver_file)
             assert proc.wait() is not None
 
-            # Find a PID that doesn't exist
-            pid = 1234
-            while pid_exists(pid):
-                pid += 1
-            _write_driver(address, pid)
-            assert os.path.exists(driver_file)
 
-            run_command('driver stop --force')
+@pytest.mark.parametrize('cmd', ['start', 'stop'])
+def test_cli_driver_after_bad_driver_file(cmd, tmpdir, capsys):
+    with set_skein_config(str(tmpdir)):
+        run_command('config gencerts')
+        driver_file = os.path.join(skein.properties.config_dir, 'driver')
+
+        sock = socket.socket()
+        sock.bind(('', 0))
+        address = 'localhost:%d' % sock.getsockname()[1]
+
+        # Find a PID that doesn't exist
+        pid = 1234
+        while pid_exists(pid):
+            pid += 1
+        _write_driver(address, pid)
+        assert os.path.exists(driver_file)
+
+        if cmd == 'start':
+            run_command('driver start')
             out, err = capsys.readouterr()
-            assert not out
-            assert not err
-            assert not os.path.exists(driver_file)
+            assert out
+            assert 'Previous driver' in err
+            assert os.path.exists(driver_file)
+
+        run_command('driver stop')
+        out, err = capsys.readouterr()
+        assert not out
+        assert not err
+        assert not os.path.exists(driver_file)
 
 
 def test_cli_application_submit_errors(tmpdir, capsys, global_client):
diff --git a/skein/test/test_core.py b/skein/test/test_core.py
index 1cd5df9..c828df1 100644
--- a/skein/test/test_core.py
+++ b/skein/test/test_core.py
@@ -10,9 +10,9 @@ import pytest
 import skein
 from skein.core import Properties
 from skein.exceptions import FileNotFoundError, FileExistsError
+from skein.utils import pid_exists
 from skein.test.conftest import (run_application, wait_for_containers,
-                                 wait_for_completion, get_logs, KEYTAB_PATH,
-                                 pid_exists)
+                                 wait_for_completion, get_logs, KEYTAB_PATH)
 
 
 def test_properties():