Repository: simphony/simphony-remote

Issue:
Running containers not in cache when container is started without a public port
Start any incompatible image not exposing a port (e.g. `quay.io/travisci/travis-python:latest`), the container is up and running but we do not keep it in our cache of running containers because the port is unavailable (an exception is raised).

From the user point of view, there is no View/Stop button which makes sense.
From the admin point of view, the container has to be stopped and removed automatically.

Needed for #6 


Discussion:

Patch:
diff --git a/remoteappmanager/docker/container.py b/remoteappmanager/docker/container.py
index f4e8677..6123205 100644
--- a/remoteappmanager/docker/container.py
+++ b/remoteappmanager/docker/container.py
@@ -23,7 +23,7 @@ class Container(HasTraits):
     ip = Unicode()
 
     #: ...and port where the container service will be listening
-    port = Int()
+    port = Int(None, allow_none=True)
 
     @property
     def url(self):
@@ -38,3 +38,44 @@ class Container(HasTraits):
             ip=self.ip,
             port=self.port,
         )
+
+    def __repr__(self):
+        return ('<Container(docker_id={0}, name={1}, image_name={2}, '
+                'image_id={3}, ip={4}, port={5}>').format(
+                    self.docker_id, self.name,
+                    self.image_name, self.image_id,
+                    self.ip, self.port)
+
+    @classmethod
+    def from_docker_dict(cls, docker_dict):
+        """Returns a Container object with the info given by a
+        docker Client.
+
+        Parameters
+        ----------
+        docker_dict : dict
+            One item from the result of docker.Client.containers
+
+        Returns
+        -------
+        container : Container
+
+        Examples
+        --------
+        >>> # containers is a list of dict
+        >>> containers = docker.Client().containers()
+
+        >>> Container.from_docker_dict(containers[0])
+        """
+        if docker_dict.get('Ports'):
+            ip = docker_dict['Ports'][0].get('IP', "")
+            port = docker_dict['Ports'][0].get('PublicPort')
+        else:
+            ip = ""
+            port = None
+
+        return cls(docker_id=docker_dict.get('Id', ''),
+                   name=docker_dict.get('Names', ('',))[0],
+                   image_name=docker_dict.get('Image', ''),
+                   image_id=docker_dict.get('ImageID', ''),
+                   ip=ip, port=port)
diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py
index 19559cf..94b7b39 100644
--- a/remoteappmanager/docker/container_manager.py
+++ b/remoteappmanager/docker/container_manager.py
@@ -93,26 +93,44 @@ class ContainerManager(LoggingMixin):
             self._stop_pending.remove(container_id)
 
     @gen.coroutine
-    def containers_for_image(self, image_id):
-        """Returns the containers for a given image that are managed
-        by this object.
+    def containers_for_image(self, image_id_or_name, user_name=None):
+        """Returns the currently running containers for a given image.
+
+        If `user_name` is given, only returns containers started by the
+        given user name.
 
         It is a coroutine because we might want to run an inquire to the docker
         service if not present.
 
         Parameters
         ----------
-        image_id: str
-            The image id
+        image_id_or_name: str
+            The image id or name
+
+        Optional parameters
+        -------------------
+        user_name : str
+            Name of the user who started the container
 
         Return
         ------
         A list of container objects, or an empty list if not present.
         """
-        try:
-            return self._containers_for_image[image_id]
-        except KeyError:
-            return []
+        if user_name:
+            user_labels = _get_container_labels(user_name)
+            if user_labels:
+                filters = {'label': '{0}={1}'.format(*user_labels.popitem())}
+        else:
+            filters = {}
+
+        filters['ancestor'] = image_id_or_name
+
+        containers = yield self.docker_client.containers(filters=filters)
+        return [Container.from_docker_dict(container)
+                for container in containers
+                # Require further filtering as ancestor include grandparents
+                if (container.get('Image') == image_id_or_name or
+                    container.get('ImageID') == image_id_or_name)]
 
     @gen.coroutine
     def all_images(self):
diff --git a/remoteappmanager/handlers/home_handler.py b/remoteappmanager/handlers/home_handler.py
index c940fe4..a9fa728 100644
--- a/remoteappmanager/handlers/home_handler.py
+++ b/remoteappmanager/handlers/home_handler.py
@@ -11,6 +11,7 @@ from tornado.httpclient import AsyncHTTPClient, HTTPError
 from tornado.log import app_log
 
 from remoteappmanager.handlers.base_handler import BaseHandler
+from remoteappmanager.docker.container import Container
 
 
 # FIXME: replace these with ORM objects
@@ -31,7 +32,7 @@ class HomeHandler(BaseHandler):
 
         for image in all_images:
             containers = yield container_manager.containers_for_image(
-                image.docker_id)
+                image.docker_id, self.current_user)
             container = (containers[0] if len(containers) > 0 else None)
             # For now we assume we have only one.
             images_info.append({
@@ -65,37 +66,47 @@ class HomeHandler(BaseHandler):
                            exc_info=True)
             return
 
-        yield handler(self.current_user, options)
-
-    # Subhandling after post
-
-    @gen.coroutine
-    def _actionhandler_start(self, user_name, options):
-        """Sub handling. Acts in response to a "start" request from
-        the user."""
-        # Start the single-user server
-
         try:
-            image_name = options["image_name"][0]
-            container = yield self._start_container(user_name, image_name)
+            yield handler(self.current_user, options)
         except Exception as e:
             # Create a random reference number for support
             ref = str(uuid.uuid1())
-            self.log.exception("Failed to spawn docker image. %s "
-                               "Ref: %s",
-                               str(e), ref)
+            self.log.exception("Failed with POST action: {0}. {1} "
+                               "Ref: {2}".format(
+                                   action, str(e), ref))
 
             images_info = yield self._get_images_info()
 
             # Render the home page again with the error message
             # User-facing error message (less info)
-            message = ('Failed to start "{image_name}". Reason: {error_type} '
+            message = ('Failed to {action} "{image_name}". '
+                       'Reason: {error_type} '
                        '(Ref: {ref})')
             self.render('home.html', images_info=images_info,
                         error_message=message.format(
-                            image_name=image_name,
+                            action=action,
+                            image_name=options["image_name"][0],
                             error_type=type(e).__name__,
                             ref=ref))
+
+    # Subhandling after post
+
+    @gen.coroutine
+    def _actionhandler_start(self, user_name, options):
+        """Sub handling. Acts in response to a "start" request from
+        the user."""
+        # Start the single-user server
+
+        try:
+            # FIXME: too many operations in one try block, should be separated
+            # for better error handling
+            image_name = options["image_name"][0]
+            container = yield self._start_container(user_name, image_name)
+            yield self._wait_for_container_ready(container)
+        except Exception as e:
+            # Clean up, if the container is running
+            yield self._stop_and_remove_container(user_name, image_name)
+            raise e
         else:
             # The server is up and running. Now contact the proxy and add
             # the container url to it.
@@ -112,10 +123,16 @@ class HomeHandler(BaseHandler):
         It is not different from pasting the appropriate URL in the
         web browser, but we validate the container id first.
         """
-        container = self._container_from_options(options)
+        container = yield self._container_from_options(options)
         if not container:
+            self.finish("Unable to view the application")
             return
 
+        yield self._wait_for_container_ready(container)
+
+        # in case the reverse proxy is not already set up
+        yield self.application.reverse_proxy_add_container(container)
+
         url = self.application.container_url_abspath(container)
         self.log.info('Redirecting to ' + url)
         self.redirect(url)
@@ -127,11 +144,18 @@ class HomeHandler(BaseHandler):
         app = self.application
         container_manager = app.container_manager
 
-        container = self._container_from_options(options)
+        container = yield self._container_from_options(options)
         if not container:
+            self.finish("Unable to view the application")
             return
 
-        yield app.reverse_proxy_remove_container(container)
+        try:
+            yield app.reverse_proxy_remove_container(container)
+        except HTTPError as http_error:
+            # The reverse proxy may be absent to start with
+            if http_error.code != 404:
+                raise http_error
+
         yield container_manager.stop_and_remove_container(container.docker_id)
 
         # We don't have fancy stuff at the moment to change the button, so
@@ -140,31 +164,35 @@ class HomeHandler(BaseHandler):
 
     # private
 
+    @gen.coroutine
     def _container_from_options(self, options):
         """Support routine to reduce duplication.
         Retrieves and returns the container if valid and present.
-        If not present, performs the http response and returns None.
+
+        If not present, returns None
         """
 
         container_manager = self.application.container_manager
+
         try:
             container_id = options["container_id"][0]
         except (KeyError, IndexError):
             self.log.exception(
                 "Failed to retrieve valid container_id from form"
             )
-            self.finish("Unable to retrieve valid container_id value")
             return None
 
-        try:
-            container = container_manager.containers[container_id]
-        except KeyError:
-            self.log.error("Unable to find container_id {} in manager".format(
-                container_id))
-            self.finish("Unable to find specified container_id")
-            return None
+        container_dict = yield container_manager.docker_client.containers(
+            filters={'id': container_id})
 
-        return container
+        if container_dict:
+            return Container.from_docker_dict(container_dict[0])
+        else:
+            self.log.exception(
+                "Failed to retrieve valid container from container id: %s",
+                container_id
+            )
+            return None
 
     @gen.coroutine
     def _start_container(self, user_name, image_name):
@@ -212,46 +240,21 @@ class HomeHandler(BaseHandler):
             e.reason = 'error'
             raise e
 
-        # Note, we use the jupyterhub ORM server, but we don't use it for
-        # any database activity.
-        # Note: the end / is important. We want to get a 200 from the actual
-        # websockify server, not the nginx (which presents the redirection
-        # page).
-        server_url = "http://{}:{}{}/".format(
-            container.ip,
-            container.port,
-            self.application.container_url_abspath(container))
+        return container
 
-        try:
-            yield _wait_for_http_server_2xx(
-                server_url,
-                self.application.config.network_timeout)
-        except TimeoutError as e:
-            # Note: Using TimeoutError instead of gen.TimeoutError as above
-            # is not a mistake.
-            self.log.warning(
-                "{user}'s container never showed up at {url} "
-                "after {http_timeout} seconds. Giving up.".format(
-                    user=user_name,
-                    url=server_url,
-                    http_timeout=self.application.config.network_timeout,
-                )
-            )
-            e.reason = 'timeout'
-            raise e
-        except Exception as e:
-            self.log.exception(
-                "Unhandled error waiting for {user}'s server "
-                "to show up at {url}: {error}".format(
-                    user=user_name,
-                    url=server_url,
-                    error=e,
-                )
-            )
-            e.reason = 'error'
-            raise e
+    @gen.coroutine
+    def _stop_and_remove_container(self, user_name, image_name):
+        """ Stop and remove the container associated with the given
+        user name and image name, if exists.
+        """
+        container_manager = self.application.container_manager
+        containers = yield container_manager.containers_for_image(
+            image_name, user_name)
 
-        return container
+        # Assume only one container per image
+        if containers:
+            container_id = containers[0].docker_id
+            yield container_manager.stop_and_remove_container(container_id)
 
     def _parse_form(self):
         """Extract the form options from the form and return them
@@ -262,6 +265,29 @@ class HomeHandler(BaseHandler):
 
         return form_options
 
+    @gen.coroutine
+    def _wait_for_container_ready(self, container):
+        """ Wait until the container is ready to be connected
+
+        Parameters
+        ----------
+        container: Container
+           The container to be connected
+        """
+        # Note, we use the jupyterhub ORM server, but we don't use it for
+        # any database activity.
+        # Note: the end / is important. We want to get a 200 from the actual
+        # websockify server, not the nginx (which presents the redirection
+        # page).
+        server_url = "http://{}:{}{}/".format(
+            container.ip,
+            container.port,
+            self.application.container_url_abspath(container))
+
+        yield _wait_for_http_server_2xx(
+            server_url,
+            self.application.config.network_timeout)
+
 
 @gen.coroutine
 def _wait_for_http_server_2xx(url, timeout=10):


Test patch:
diff --git a/tests/docker/test_container.py b/tests/docker/test_container.py
index 7d1cbae..faea36f 100644
--- a/tests/docker/test_container.py
+++ b/tests/docker/test_container.py
@@ -1,6 +1,7 @@
 from unittest import TestCase
 
 from remoteappmanager.docker.container import Container
+from tests.utils import assert_containers_equal
 
 
 class TestContainer(TestCase):
@@ -18,3 +19,64 @@ class TestContainer(TestCase):
         )
 
         self.assertEqual(container.host_url, "http://123.45.67.89:31337")
+
+    def test_from_docker_dict_with_public_port(self):
+        '''Test convertion from "docker ps" to Container with public port'''
+        # With public port
+        container_dict = {
+            'Command': '/startup.sh',
+            'Created': 1466756760,
+            'HostConfig': {'NetworkMode': 'default'},
+            'Id': '248e45e717cd740ae763a1c565',
+            'Image': 'empty-ubuntu:latest',
+            'ImageID': 'sha256:f4610c7580b8f0a9a25086b6287d0069fb8a',
+            'Labels': {'eu.simphony-project.docker.ui_name': 'Empty Ubuntu',
+                       'eu.simphony-project.docker.user': 'user'},
+            'Names': ['/remoteexec-user-empty-ubuntu_3Alatest'],
+            'Ports': [{'IP': '0.0.0.0',
+                       'PrivatePort': 8888,
+                       'PublicPort': 32823,
+                       'Type': 'tcp'}],
+            'State': 'running',
+            'Status': 'Up 56 minutes'}
+
+        # Container with public port
+        actual = Container.from_docker_dict(container_dict)
+        expected = Container(
+            docker_id='248e45e717cd740ae763a1c565',
+            name='/remoteexec-user-empty-ubuntu_3Alatest',
+            image_name='empty-ubuntu:latest',
+            image_id='sha256:f4610c7580b8f0a9a25086b6287d0069fb8a',
+            ip='0.0.0.0', port=32823)
+
+        assert_containers_equal(self, actual, expected)
+
+    def test_from_docker_dict_without_public_port(self):
+        '''Test convertion from "docker ps" to Container with public port'''
+        # With public port
+        container_dict = {
+            'Command': '/startup.sh',
+            'Created': 1466756760,
+            'HostConfig': {'NetworkMode': 'default'},
+            'Id': '812c765d0549be0ab831ae8348',
+            'Image': 'novnc-ubuntu:latest',
+            'ImageID': 'sha256:f4610c75d3c0dfa25d3c0dfa25d3c0dfa2',
+            'Labels': {'eu.simphony-project.docker.ui_name': 'Empty Ubuntu',
+                       'eu.simphony-project.docker.user': 'user'},
+            'Names': ['/remoteexec-user-empty-ubuntu_3Alatest'],
+            'Ports': [{'IP': '0.0.0.0',
+                       'PrivatePort': 8888,
+                       'Type': 'tcp'}],
+            'State': 'running',
+            'Status': 'Up 56 minutes'}
+
+        # Container without public port
+        actual = Container.from_docker_dict(container_dict)
+        expected = Container(
+            docker_id='812c765d0549be0ab831ae8348',
+            name='/remoteexec-user-empty-ubuntu_3Alatest',
+            image_name='novnc-ubuntu:latest',
+            image_id='sha256:f4610c75d3c0dfa25d3c0dfa25d3c0dfa2',
+            ip='0.0.0.0', port=None)
+
+        assert_containers_equal(self, actual, expected)
diff --git a/tests/docker/test_container_manager.py b/tests/docker/test_container_manager.py
index f878bb1..5ed4774 100644
--- a/tests/docker/test_container_manager.py
+++ b/tests/docker/test_container_manager.py
@@ -33,24 +33,63 @@ class TestContainerManager(AsyncTestCase):
         self.assertTrue(mock_client.remove_container.called)
 
     @gen_test
-    def test_container_for_image(self):
-        result = yield self.manager.containers_for_image("imageid")
-        self.assertEqual(len(result), 0)
+    def test_containers_for_image_results(self):
+        ''' Test containers_for_image returns a list of Container '''
+        # The mock client mocks the output of docker Client.containers
+        docker_client = utils.mock_docker_client_with_running_containers()
+        self.mock_docker_client = docker_client
+        self.manager.docker_client.client = docker_client
+
+        # The output should be a list of Container
+        results = yield self.manager.containers_for_image("imageid")
+        expected = [Container(docker_id='someid',
+                              name='/remoteexec-image_3Alatest_user',
+                              image_name='simphony/mayavi-4.4.4:latest',  # noqa
+                              image_id='imageid', ip='0.0.0.0', port=None),
+                    Container(docker_id='someid',
+                              name='/remoteexec-image_3Alatest_user2',
+                              image_name='simphony/mayavi-4.4.4:latest',  # noqa
+                              image_id='imageid', ip='0.0.0.0', port=None),
+                    Container(docker_id='someid',
+                              name='/remoteexec-image_3Alatest_user3',
+                              image_name='simphony/mayavi-4.4.4:latest',  # noqa
+                              image_id='imageid', ip='', port=None)]
+
+        for result, expected_container in zip(results, expected):
+            utils.assert_containers_equal(self, result, expected_container)
 
-        yield self.manager.start_container("username", "imageid")
+    @gen_test
+    def test_containers_for_image_client_api_without_user(self):
+        ''' Test containers_for_images(image_id) use of Client API'''
+        # The mock client mocks the output of docker Client.containers
+        docker_client = utils.mock_docker_client_with_running_containers()
+        self.manager.docker_client.client = docker_client
 
-        result = yield self.manager.containers_for_image("imageid")
-        self.assertEqual(len(result), 1)
+        # We assume the client.containers(filters=...) is tested by docker-py
+        # Instead we test if the correct arguments are passed to the Client API
+        yield self.manager.containers_for_image("imageid")
+        call_args = self.manager.docker_client.client.containers.call_args
 
-        expected = {'name': 'remoteexec-username-imageid',
-                    'image_id': 'imageid',
-                    'image_name': 'imageid',
-                    'ip': '127.0.0.1',
-                    'port': 666,
-                    'docker_id': 'containerid'}
+        # filters is one of the keyword argument
+        self.assertIn('filters', call_args[1])
+        self.assertEqual(call_args[1]['filters']['ancestor'], "imageid")
 
-        for key, value in expected.items():
-            self.assertEqual(getattr(result[0], key), value)
+    @gen_test
+    def test_containers_for_image_client_api_with_user(self):
+        ''' Test containers_for_images(image_id, user) use of Client API'''
+        # The mock client mocks the output of docker Client.containers
+        docker_client = utils.mock_docker_client_with_running_containers()
+        self.manager.docker_client.client = docker_client
+
+        # We assume the client.containers(filters=...) is tested by docker-py
+        # Instead we test if the correct arguments are passed to the Client API
+        yield self.manager.containers_for_image("imageid", "userABC")
+        call_args = self.manager.docker_client.client.containers.call_args
+
+        # filters is one of the keyword argument
+        self.assertIn('filters', call_args[1])
+        self.assertEqual(call_args[1]['filters']['ancestor'], "imageid")
+        self.assertIn("userABC", call_args[1]['filters']['label'])
 
     @gen_test
     def test_race_condition_spawning(self):
diff --git a/tests/utils.py b/tests/utils.py
index ed44914..f957a35 100644
--- a/tests/utils.py
+++ b/tests/utils.py
@@ -47,6 +47,56 @@ def mock_docker_client():
     return docker_client
 
 
+def mock_docker_client_with_running_containers():
+    """Same as above, but it behaves as if one of the images have two
+    containers running for different users."""
+    client = mock_docker_client()
+    client.containers.return_value = [
+        # user
+        {'Command': '/sbin/init -D',
+         'Created': 1466766499,
+         'HostConfig': {'NetworkMode': 'default'},
+         'Id': 'someid',
+         'Image': 'simphony/mayavi-4.4.4:latest',
+         'ImageID': 'imageid',
+         'Labels': {'eu.simphony-project.docker.user': 'user'},
+         'Names': ['/remoteexec-image_3Alatest_user'],
+         'Ports': [{'IP': '0.0.0.0',
+                    'PublicIP': 34567,
+                    'PrivatePort': 22,
+                    'Type': 'tcp'}],
+         'State': 'running',
+         'Status': 'Up About an hour'},
+        # user2
+        {'Command': '/sbin/init -D',
+         'Created': 1466766499,
+         'HostConfig': {'NetworkMode': 'default'},
+         'Id': 'someid',
+         'Image': 'simphony/mayavi-4.4.4:latest',
+         'ImageID': 'imageid',
+         'Labels': {'eu.simphony-project.docker.user': 'user2'},
+         'Names': ['/remoteexec-image_3Alatest_user2'],
+         'Ports': [{'IP': '0.0.0.0',
+                    'PublicIP': 34567,
+                    'PrivatePort': 22,
+                    'Type': 'tcp'}],
+         'State': 'running',
+         'Status': 'Up About an hour'},
+        # user3 (somehow there is no port
+        {'Command': '/sbin/init -D',
+         'Created': 1466766499,
+         'HostConfig': {'NetworkMode': 'default'},
+         'Id': 'someid',
+         'Image': 'simphony/mayavi-4.4.4:latest',
+         'ImageID': 'imageid',
+         'Labels': {'eu.simphony-project.docker.user': 'user3'},
+         'Names': ['/remoteexec-image_3Alatest_user3'],
+         'State': 'running',
+         'Status': 'Up About an hour'}]
+
+    return client
+
+
 def mock_docker_client_with_existing_stopped_container():
     """Same as above, but it behaves as if one of the containers is already
     started."""
@@ -167,3 +217,14 @@ def invocation_argv():
     yield
 
     sys.argv[:] = saved_argv
+
+
+def assert_containers_equal(test_case, actual, expected):
+    if (expected.docker_id != actual.docker_id or
+            expected.name != actual.name or
+            expected.image_name != actual.image_name or
+            expected.image_id != actual.image_id or
+            expected.ip != actual.ip or
+            expected.port != actual.port):
+        message = '{!r} is not identical to the expected {!r}'
+        test_case.fail(message.format(actual, expected))