Repository: simphony/simphony-remote

Issue:
View not working on mac due to incorrect IP
We found that View does not work on mac because inquiring the docker client returns a 0.0.0.0 ip. This works on linux, but on mac the container is residing on a separate virtual machine. Start works because it retrieves the information directly after starting the container, and this is handled by the container manager (which knows this) but with View, the docker client is inquired directly, so the container manager cannot do any magic.

the docker client should be considered private.


Discussion:

Patch:
diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py
index 16b2f12..923c9ae 100644
--- a/remoteappmanager/docker/container_manager.py
+++ b/remoteappmanager/docker/container_manager.py
@@ -120,15 +120,64 @@ class ContainerManager(LoggingMixin):
         """
         labels = {
             SIMPHONY_NS+"user": user_name,
-            SIMPHONY_NS+"mapping_id": mapping_id,
-        }
+            SIMPHONY_NS+"mapping_id": mapping_id}
+        filters = {
+            'label': ['{0}={1}'.format(k, v) for k, v in labels.items()]}
+
+        containers = yield self.containers_from_filters(filters=filters)
+        return containers
 
+    @gen.coroutine
+    def container_from_url_id(self, url_id):
+        """Retrieves and returns the container by its url_id, if present.
+        If not present, returns None.
+        """
+        labels = {SIMPHONY_NS+"url_id": url_id}
         filters = {
-            'label': ['{0}={1}'.format(k, v) for k, v in labels.items()]
-        }
+            'label': ['{0}={1}'.format(k, v) for k, v in labels.items()]}
+
+        containers = yield self.containers_from_filters(filters=filters)
+        return containers[0] if len(containers) else None
+
+    @gen.coroutine
+    def containers_from_filters(self, filters):
+        """Returns the currently running containers for a given filter
+
+        Parameters
+        ----------
+        filters: dict
+            A dictionary of filters as in dockerpy
 
+        Return
+        ------
+        A list of Container objects, or an empty list if nothing is found.
+        """
+        containers = []
         infos = yield self.docker_client.containers(filters=filters)
-        return [Container.from_docker_containers_dict(info) for info in infos]
+        for info in infos:
+            try:
+                container = Container.from_docker_containers_dict(info)
+            except Exception:
+                self.log.exception("Unable to parse container info.")
+                continue
+
+            # override the ip and port obtained by the docker info with the
+            # appropriate ip and port, considering that we might be using a
+            # separate docker machine
+            try:
+                ip, port = yield from self._get_ip_and_port(
+                    container.docker_id)
+            except RuntimeError:
+                self.log.exception(
+                    "Unable to retrieve ip/port "
+                    "for container {}".format(container.docker_id))
+                continue
+
+            container.ip = ip
+            container.port = port
+            containers.append(container)
+
+        return containers
 
     @gen.coroutine
     def image(self, image_id_or_name):
@@ -301,13 +350,25 @@ class ContainerManager(LoggingMixin):
         Return
         ------
         A tuple (ip, port)
+
+        Raises
+        ------
+        RuntimeError:
+            If for some reason it cannot retrieve the information
         """
 
         # retrieve the actual port binding
-        resp = yield self.docker_client.port(container_id, self.container_port)
+        try:
+            resp = yield self.docker_client.port(container_id,
+                                                 self.container_port)
+        except Exception as e:
+            raise RuntimeError("Failed to get port info for {}. "
+                               "Exception: {}.".format(container_id,
+                                                       str(e)))
 
         if resp is None:
-            raise RuntimeError("Failed to get port info for %s" % container_id)
+            raise RuntimeError("Failed to get port info for {}. "
+                               "Port response was None.".format(container_id))
 
         # We assume we are running on linux without any additional docker
         # machine. The container will therefore be reachable at 127.0.0.1.
@@ -320,7 +381,13 @@ class ContainerManager(LoggingMixin):
             if url.scheme == 'tcp':
                 ip = url.hostname
 
-        port = int(resp[0]['HostPort'])
+        try:
+            port = int(resp[0]['HostPort'])
+        except (KeyError, IndexError, ValueError, TypeError) as e:
+            raise RuntimeError("Failed to get port info for {}. "
+                               "Exception: {}.".format(container_id,
+                                                       str(e)))
+
         return ip, port
 
     @gen.coroutine
diff --git a/remoteappmanager/handlers/home_handler.py b/remoteappmanager/handlers/home_handler.py
index 9ba81f6..2eb351d 100644
--- a/remoteappmanager/handlers/home_handler.py
+++ b/remoteappmanager/handlers/home_handler.py
@@ -11,7 +11,6 @@ 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
 
 
 class HomeHandler(BaseHandler):
@@ -125,11 +124,15 @@ class HomeHandler(BaseHandler):
         It is not different from pasting the appropriate URL in the
         web browser, but we validate the container id first.
         """
-        container = yield self._container_from_options(options)
+        url_id = options["url_id"][0]
+
+        container_manager = self.application.container_manager
+        container = yield container_manager.container_from_url_id(url_id)
         if not container:
             self.finish("Unable to view the application")
             return
 
+        # make sure the container is actually running and working
         yield self._wait_for_container_ready(container)
 
         # in case the reverse proxy is not already set up
@@ -142,10 +145,12 @@ class HomeHandler(BaseHandler):
     def _actionhandler_stop(self, options):
         """Stops a running container.
         """
+        url_id = options["url_id"][0]
+
         app = self.application
         container_manager = app.container_manager
 
-        container = yield self._container_from_options(options)
+        container = yield container_manager.container_from_url_id(url_id)
         if not container:
             self.finish("Unable to view the application")
             return
@@ -201,35 +206,6 @@ class HomeHandler(BaseHandler):
             })
         return images_info
 
-    @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, 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"
-            )
-            return None
-
-        container_dict = yield container_manager.docker_client.containers(
-            filters={'id': container_id})
-
-        if not container_dict:
-            self.log.exception(
-                "Failed to retrieve valid "
-                "container from container id: {}".format(container_id))
-            return None
-
-        return Container.from_docker_containers_dict(container_dict[0])
-
     @gen.coroutine
     def _start_container(self, user_name, app, policy, mapping_id):
         """Start the container. This method is a helper method that
diff --git a/remoteappmanager/templates/home.html b/remoteappmanager/templates/home.html
index 7eaf6cb..6b6b7a0 100644
--- a/remoteappmanager/templates/home.html
+++ b/remoteappmanager/templates/home.html
@@ -38,7 +38,7 @@
                     <button type="submit" name="action" value="start" class="btn btn-primary">Start</button>
                 </div>
             {% else %}
-                <input type="hidden" name="container_id" value="{{info.container.docker_id}}">
+                <input type="hidden" name="url_id" value="{{info.container.url_id}}">
                 <div class="col-sm-1 va">
                     <button type="submit" name="action" value="view" class="btn btn-success">View</button>
                 </div>


Test patch:
diff --git a/tests/docker/test_container_manager.py b/tests/docker/test_container_manager.py
index 7610825..5dbd238 100644
--- a/tests/docker/test_container_manager.py
+++ b/tests/docker/test_container_manager.py
@@ -53,13 +53,53 @@ class TestContainerManager(AsyncTestCase):
                              name='/remoteexec-image_3Alatest_user',
                              image_name='simphony/mayavi-4.4.4:latest',  # noqa
                              image_id='imageid',
-                             ip='0.0.0.0',
-                             port=None,
+                             ip='127.0.0.1',
+                             port=666,
                              url_id='url_id')
 
         self.assertEqual(len(result), 1)
         utils.assert_containers_equal(self, result[0], expected)
 
+    @gen_test
+    def test_containers_from_url_id(self):
+        ''' Test containers_for_mapping_id 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
+
+        result = yield self.manager.container_from_url_id("url_id")
+        expected = Container(docker_id='someid',
+                             mapping_id="mapping",
+                             name='/remoteexec-image_3Alatest_user',
+                             image_name='simphony/mayavi-4.4.4:latest',  # noqa
+                             image_id='imageid',
+                             ip='127.0.0.1',
+                             port=666,
+                             url_id='url_id')
+
+        utils.assert_containers_equal(self, result, expected)
+
+    @gen_test
+    def test_containers_from_url_id_exceptions(self):
+        ''' Test containers_for_mapping_id returns a list of Container '''
+        # The mock client mocks the output of docker Client.containers
+        docker_client = utils.mock_docker_client_with_running_containers()
+        docker_client.port = mock.Mock(side_effect=Exception("Boom!"))
+        self.mock_docker_client = docker_client
+        self.manager.docker_client.client = docker_client
+
+        result = yield self.manager.container_from_url_id("url_id")
+        self.assertEqual(result, None)
+
+        # Making it so that no valid dictionary is returned.
+        docker_client.port = mock.Mock(return_value=1234)
+        self.mock_docker_client = docker_client
+        self.manager.docker_client.client = docker_client
+
+        result = yield self.manager.container_from_url_id("url_id")
+        self.assertEqual(result, None)
+
     @gen_test
     def test_race_condition_spawning(self):
         # Start the operations, and retrieve the future.
diff --git a/tests/handlers/test_home_handler.py b/tests/handlers/test_home_handler.py
index c532f9e..2d32b6e 100644
--- a/tests/handlers/test_home_handler.py
+++ b/tests/handlers/test_home_handler.py
@@ -67,6 +67,9 @@ class TestHomeHandler(TempMixin, utils.AsyncHTTPTestCase):
         app.container_manager.containers_from_mapping_id = mock_coro_factory(
             [Container()]
         )
+        app.container_manager.container_from_url_id = mock_coro_factory(
+            Container()
+        )
         app.container_manager.stop_and_remove_container = mock_coro_factory()
 
         mock_application = mock.Mock()
@@ -149,7 +152,7 @@ class TestHomeHandler(TempMixin, utils.AsyncHTTPTestCase):
     def test_post_stop(self):
         body = urllib.parse.urlencode(
             {'action': 'stop',
-             'container_id': '12345'
+             'url_id': '12345'
              }
         )
 
@@ -157,16 +160,7 @@ class TestHomeHandler(TempMixin, utils.AsyncHTTPTestCase):
                         ".handlers"
                         ".home_handler"
                         ".HomeHandler"
-                        "._container_from_options",
-                        new_callable=mock_coro_factory
-                        ) as mock_container_from_options, \
-            mock.patch("remoteappmanager"
-                       ".handlers"
-                       ".home_handler"
-                       ".HomeHandler"
-                       ".redirect") as redirect:
-
-            mock_container_from_options.return_value = Container()
+                        ".redirect") as redirect:
 
             self.fetch("/user/username/",
                        method="POST",
@@ -181,7 +175,7 @@ class TestHomeHandler(TempMixin, utils.AsyncHTTPTestCase):
     def test_post_view(self):
         body = urllib.parse.urlencode(
             {'action': 'view',
-             'container_id': '12345'
+             'url_id': '12345'
              }
         )
 
@@ -190,21 +184,12 @@ class TestHomeHandler(TempMixin, utils.AsyncHTTPTestCase):
                         ".home_handler"
                         "._wait_for_http_server_2xx",
                         new_callable=mock_coro_factory), \
-                mock.patch("remoteappmanager"
-                           ".handlers"
-                           ".home_handler"
-                           ".HomeHandler"
-                           "._container_from_options",
-                           new_callable=mock_coro_factory
-                           ) as mock_container_from_options, \
                 mock.patch("remoteappmanager"
                            ".handlers"
                            ".home_handler"
                            ".HomeHandler"
                            ".redirect") as redirect:
 
-            mock_container_from_options.return_value = Container()
-
             self.fetch("/user/username/",
                        method="POST",
                        headers={