Merge pull request #586 from dnephin/speed_up_fig_up

Speed up fig up

fig/cli/main.py

@@ -425,6 +425,7 @@ class TopLevelCommand(Command):
             --no-color            Produce monochrome output.
             --no-deps             Don't start linked services.
             --no-recreate         If containers already exist, don't recreate them.
+            --no-build            Don't build an image, even if it's missing
         """
         insecure_registry = options['--allow-insecure-ssl']
         detached = options['-d']
@@ -440,7 +441,8 @@ class TopLevelCommand(Command):
             start_links=start_links,
             recreate=recreate,
             insecure_registry=insecure_registry,
-            detach=options['-d']
+            detach=options['-d'],
+            do_build=not options['--no-build'],
         )
 
         to_attach = [c for s in project.get_services(service_names) for c in s.containers()]

fig/project.py

@@ -167,14 +167,26 @@ class Project(object):
             else:
                 log.info('%s uses an image, skipping' % service.name)
 
-    def up(self, service_names=None, start_links=True, recreate=True, insecure_registry=False, detach=False):
+    def up(self,
+           service_names=None,
+           start_links=True,
+           recreate=True,
+           insecure_registry=False,
+           detach=False,
+           do_build=True):
         running_containers = []
         for service in self.get_services(service_names, include_links=start_links):
             if recreate:
-                for (_, container) in service.recreate_containers(insecure_registry=insecure_registry, detach=detach):
+                for (_, container) in service.recreate_containers(
+                        insecure_registry=insecure_registry,
+                        detach=detach,
+                        do_build=do_build):
                     running_containers.append(container)
             else:
-                for container in service.start_or_create_containers(insecure_registry=insecure_registry, detach=detach):
+                for container in service.start_or_create_containers(
+                        insecure_registry=insecure_registry,
+                        detach=detach,
+                        do_build=do_build):
                     running_containers.append(container)
 
         return running_containers

fig/service.py

@@ -50,6 +50,17 @@ DOCKER_CONFIG_HINTS = {
     'workdir'   : 'working_dir',
 }
 
+DOCKER_START_KEYS = [
+    'cap_add',
+    'cap_drop',
+    'dns',
+    'dns_search',
+    'env_file',
+    'net',
+    'privileged',
+    'restart',
+]
+
 VALID_NAME_CHARS = '[a-zA-Z0-9]'
 
 
@@ -145,7 +156,8 @@ class Service(object):
 
     def scale(self, desired_num):
         """
-        Adjusts the number of containers to the specified number and ensures they are running.
+        Adjusts the number of containers to the specified number and ensures
+        they are running.
 
         - creates containers until there are at least `desired_num`
         - stops containers until there are at most `desired_num` running
@@ -192,12 +204,24 @@ class Service(object):
                 log.info("Removing %s..." % c.name)
                 c.remove(**options)
 
-    def create_container(self, one_off=False, insecure_registry=False, **override_options):
+    def create_container(self,
+                         one_off=False,
+                         insecure_registry=False,
+                         do_build=True,
+                         **override_options):
         """
         Create a container for this service. If the image doesn't exist, attempt to pull
         it.
         """
-        container_options = self._get_container_create_options(override_options, one_off=one_off)
+        container_options = self._get_container_create_options(
+            override_options,
+            one_off=one_off)
+
+        if (do_build and
+                self.can_be_built() and
+                not self.client.images(name=self.full_name)):
+            self.build()
+
         try:
             return Container.create(self.client, **container_options)
         except APIError as e:
@@ -212,7 +236,7 @@ class Service(object):
                 return Container.create(self.client, **container_options)
             raise
 
-    def recreate_containers(self, insecure_registry=False, **override_options):
+    def recreate_containers(self, insecure_registry=False, do_build=True, **override_options):
         """
         If a container for this service doesn't exist, create and start one. If there are
         any, stop them, create+start new ones, and remove the old containers.
@@ -220,7 +244,10 @@ class Service(object):
         containers = self.containers(stopped=True)
         if not containers:
             log.info("Creating %s..." % self._next_container_name(containers))
-            container = self.create_container(insecure_registry=insecure_registry, **override_options)
+            container = self.create_container(
+                insecure_registry=insecure_registry,
+                do_build=do_build,
+                **override_options)
             self.start_container(container)
             return [(None, container)]
         else:
@@ -259,7 +286,7 @@ class Service(object):
         container.remove()
 
         options = dict(override_options)
-        new_container = self.create_container(**options)
+        new_container = self.create_container(do_build=False, **options)
         self.start_container(new_container, intermediate_container=intermediate_container)
 
         intermediate_container.remove()
@@ -273,8 +300,7 @@ class Service(object):
             log.info("Starting %s..." % container.name)
             return self.start_container(container, **options)
 
-    def start_container(self, container=None, intermediate_container=None, **override_options):
+    def start_container(self, container, intermediate_container=None, **override_options):
-        container = container or self.create_container(**override_options)
         options = dict(self.options, **override_options)
         port_bindings = build_port_bindings(options.get('ports') or [])
 
@@ -307,14 +333,19 @@ class Service(object):
         )
         return container
 
-    def start_or_create_containers(self, insecure_registry=False, detach=False):
+    def start_or_create_containers(
+            self,
+            insecure_registry=False,
+            detach=False,
+            do_build=True):
         containers = self.containers(stopped=True)
 
         if not containers:
             log.info("Creating %s..." % self._next_container_name(containers))
             new_container = self.create_container(
                 insecure_registry=insecure_registry,
-                detach=detach
+                detach=detach,
+                do_build=do_build,
             )
             return [self.start_container(new_container)]
         else:
@@ -407,16 +438,13 @@ class Service(object):
         container_options['environment'] = merge_environment(container_options)
 
         if self.can_be_built():
-            if len(self.client.images(name=self._build_tag_name())) == 0:
+            container_options['image'] = self.full_name
-                self.build()
-            container_options['image'] = self._build_tag_name()
         else:
             container_options['image'] = self._get_image_name(container_options['image'])
 
         # Delete options which are only used when starting
-        for key in ['privileged', 'net', 'dns', 'dns_search', 'restart', 'cap_add', 'cap_drop', 'env_file']:
+        for key in DOCKER_START_KEYS:
-            if key in container_options:
+            container_options.pop(key, None)
-                del container_options[key]
 
         return container_options
 
@@ -431,7 +459,7 @@ class Service(object):
 
         build_output = self.client.build(
             self.options['build'],
-            tag=self._build_tag_name(),
+            tag=self.full_name,
             stream=True,
             rm=True,
             nocache=no_cache,
@@ -451,14 +479,15 @@ class Service(object):
                     image_id = match.group(1)
 
         if image_id is None:
-            raise BuildError(self)
+            raise BuildError(self, event if all_events else 'Unknown')
 
         return image_id
 
     def can_be_built(self):
         return 'build' in self.options
 
-    def _build_tag_name(self):
+    @property
+    def full_name(self):
         """
         The tag to give to images built for this service.
         """

tests/integration/service_test.py

@@ -10,19 +10,24 @@ from docker.errors import APIError
 from .testcases import DockerClientTestCase
 
 
+def create_and_start_container(service, **override_options):
+    container = service.create_container(**override_options)
+    return service.start_container(container, **override_options)
+
+
 class ServiceTest(DockerClientTestCase):
     def test_containers(self):
         foo = self.create_service('foo')
         bar = self.create_service('bar')
 
-        foo.start_container()
+        create_and_start_container(foo)
 
         self.assertEqual(len(foo.containers()), 1)
         self.assertEqual(foo.containers()[0].name, 'figtest_foo_1')
         self.assertEqual(len(bar.containers()), 0)
 
-        bar.start_container()
+        create_and_start_container(bar)
-        bar.start_container()
+        create_and_start_container(bar)
 
         self.assertEqual(len(foo.containers()), 1)
         self.assertEqual(len(bar.containers()), 2)
@@ -39,7 +44,7 @@ class ServiceTest(DockerClientTestCase):
 
     def test_project_is_added_to_container_name(self):
         service = self.create_service('web')
-        service.start_container()
+        create_and_start_container(service)
         self.assertEqual(service.containers()[0].name, 'figtest_web_1')
 
     def test_start_stop(self):
@@ -65,7 +70,7 @@ class ServiceTest(DockerClientTestCase):
     def test_kill_remove(self):
         service = self.create_service('scalingtest')
 
-        service.start_container()
+        create_and_start_container(service)
         self.assertEqual(len(service.containers()), 1)
 
         service.remove_stopped()
@@ -177,21 +182,21 @@ class ServiceTest(DockerClientTestCase):
 
     def test_start_container_passes_through_options(self):
         db = self.create_service('db')
-        db.start_container(environment={'FOO': 'BAR'})
+        create_and_start_container(db, environment={'FOO': 'BAR'})
         self.assertEqual(db.containers()[0].environment['FOO'], 'BAR')
 
     def test_start_container_inherits_options_from_constructor(self):
         db = self.create_service('db', environment={'FOO': 'BAR'})
-        db.start_container()
+        create_and_start_container(db)
         self.assertEqual(db.containers()[0].environment['FOO'], 'BAR')
 
     def test_start_container_creates_links(self):
         db = self.create_service('db')
         web = self.create_service('web', links=[(db, None)])
 
-        db.start_container()
+        create_and_start_container(db)
-        db.start_container()
+        create_and_start_container(db)
-        web.start_container()
+        create_and_start_container(web)
 
         self.assertEqual(
             set(web.containers()[0].links()),
@@ -206,9 +211,9 @@ class ServiceTest(DockerClientTestCase):
         db = self.create_service('db')
         web = self.create_service('web', links=[(db, 'custom_link_name')])
 
-        db.start_container()
+        create_and_start_container(db)
-        db.start_container()
+        create_and_start_container(db)
-        web.start_container()
+        create_and_start_container(web)
 
         self.assertEqual(
             set(web.containers()[0].links()),
@@ -222,19 +227,19 @@ class ServiceTest(DockerClientTestCase):
     def test_start_normal_container_does_not_create_links_to_its_own_service(self):
         db = self.create_service('db')
 
-        db.start_container()
+        create_and_start_container(db)
-        db.start_container()
+        create_and_start_container(db)
 
-        c = db.start_container()
+        c = create_and_start_container(db)
         self.assertEqual(set(c.links()), set([]))
 
     def test_start_one_off_container_creates_links_to_its_own_service(self):
         db = self.create_service('db')
 
-        db.start_container()
+        create_and_start_container(db)
-        db.start_container()
+        create_and_start_container(db)
 
-        c = db.start_container(one_off=True)
+        c = create_and_start_container(db, one_off=True)
 
         self.assertEqual(
             set(c.links()),
@@ -252,7 +257,7 @@ class ServiceTest(DockerClientTestCase):
             build='tests/fixtures/simple-dockerfile',
             project='figtest',
         )
-        container = service.start_container()
+        container = create_and_start_container(service)
         container.wait()
         self.assertIn('success', container.logs())
         self.assertEqual(len(self.client.images(name='figtest_test')), 1)
@@ -265,45 +270,45 @@ class ServiceTest(DockerClientTestCase):
             build='this/does/not/exist/and/will/throw/error',
             project='figtest',
         )
-        container = service.start_container()
+        container = create_and_start_container(service)
         container.wait()
         self.assertIn('success', container.logs())
 
     def test_start_container_creates_ports(self):
         service = self.create_service('web', ports=[8000])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertEqual(list(container['NetworkSettings']['Ports'].keys()), ['8000/tcp'])
         self.assertNotEqual(container['NetworkSettings']['Ports']['8000/tcp'][0]['HostPort'], '8000')
 
     def test_start_container_stays_unpriviliged(self):
         service = self.create_service('web')
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertEqual(container['HostConfig']['Privileged'], False)
 
     def test_start_container_becomes_priviliged(self):
         service = self.create_service('web', privileged = True)
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertEqual(container['HostConfig']['Privileged'], True)
 
     def test_expose_does_not_publish_ports(self):
         service = self.create_service('web', expose=[8000])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertEqual(container['NetworkSettings']['Ports'], {'8000/tcp': None})
 
     def test_start_container_creates_port_with_explicit_protocol(self):
         service = self.create_service('web', ports=['8000/udp'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertEqual(list(container['NetworkSettings']['Ports'].keys()), ['8000/udp'])
 
     def test_start_container_creates_fixed_external_ports(self):
         service = self.create_service('web', ports=['8000:8000'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertIn('8000/tcp', container['NetworkSettings']['Ports'])
         self.assertEqual(container['NetworkSettings']['Ports']['8000/tcp'][0]['HostPort'], '8000')
 
     def test_start_container_creates_fixed_external_ports_when_it_is_different_to_internal_port(self):
         service = self.create_service('web', ports=['8001:8000'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertIn('8000/tcp', container['NetworkSettings']['Ports'])
         self.assertEqual(container['NetworkSettings']['Ports']['8000/tcp'][0]['HostPort'], '8001')
 
@@ -312,7 +317,7 @@ class ServiceTest(DockerClientTestCase):
             '127.0.0.1:8001:8000',
             '0.0.0.0:9001:9000/udp',
         ])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service).inspect()
         self.assertEqual(container['NetworkSettings']['Ports'], {
             '8000/tcp': [
                 {
@@ -361,74 +366,74 @@ class ServiceTest(DockerClientTestCase):
 
     def test_network_mode_none(self):
         service = self.create_service('web', net='none')
-        container = service.start_container()
+        container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.NetworkMode'), 'none')
 
     def test_network_mode_bridged(self):
         service = self.create_service('web', net='bridge')
-        container = service.start_container()
+        container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.NetworkMode'), 'bridge')
 
     def test_network_mode_host(self):
         service = self.create_service('web', net='host')
-        container = service.start_container()
+        container = create_and_start_container(service)
         self.assertEqual(container.get('HostConfig.NetworkMode'), 'host')
 
     def test_dns_single_value(self):
         service = self.create_service('web', dns='8.8.8.8')
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['Dns'], ['8.8.8.8'])
+        self.assertEqual(container.get('HostConfig.Dns'), ['8.8.8.8'])
 
     def test_dns_list(self):
         service = self.create_service('web', dns=['8.8.8.8', '9.9.9.9'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['Dns'], ['8.8.8.8', '9.9.9.9'])
+        self.assertEqual(container.get('HostConfig.Dns'), ['8.8.8.8', '9.9.9.9'])
 
     def test_restart_always_value(self):
         service = self.create_service('web', restart='always')
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['RestartPolicy']['Name'], 'always')
+        self.assertEqual(container.get('HostConfig.RestartPolicy.Name'), 'always')
 
     def test_restart_on_failure_value(self):
         service = self.create_service('web', restart='on-failure:5')
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['RestartPolicy']['Name'], 'on-failure')
+        self.assertEqual(container.get('HostConfig.RestartPolicy.Name'), 'on-failure')
-        self.assertEqual(container['HostConfig']['RestartPolicy']['MaximumRetryCount'], 5)
+        self.assertEqual(container.get('HostConfig.RestartPolicy.MaximumRetryCount'), 5)
 
     def test_cap_add_list(self):
         service = self.create_service('web', cap_add=['SYS_ADMIN', 'NET_ADMIN'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['CapAdd'], ['SYS_ADMIN', 'NET_ADMIN'])
+        self.assertEqual(container.get('HostConfig.CapAdd'), ['SYS_ADMIN', 'NET_ADMIN'])
 
     def test_cap_drop_list(self):
         service = self.create_service('web', cap_drop=['SYS_ADMIN', 'NET_ADMIN'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['CapDrop'], ['SYS_ADMIN', 'NET_ADMIN'])
+        self.assertEqual(container.get('HostConfig.CapDrop'), ['SYS_ADMIN', 'NET_ADMIN'])
 
     def test_dns_search_single_value(self):
         service = self.create_service('web', dns_search='example.com')
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['DnsSearch'], ['example.com'])
+        self.assertEqual(container.get('HostConfig.DnsSearch'), ['example.com'])
 
     def test_dns_search_list(self):
         service = self.create_service('web', dns_search=['dc1.example.com', 'dc2.example.com'])
-        container = service.start_container().inspect()
+        container = create_and_start_container(service)
-        self.assertEqual(container['HostConfig']['DnsSearch'], ['dc1.example.com', 'dc2.example.com'])
+        self.assertEqual(container.get('HostConfig.DnsSearch'), ['dc1.example.com', 'dc2.example.com'])
 
     def test_working_dir_param(self):
         service = self.create_service('container', working_dir='/working/dir/sample')
-        container = service.create_container().inspect()
+        container = service.create_container()
-        self.assertEqual(container['Config']['WorkingDir'], '/working/dir/sample')
+        self.assertEqual(container.get('Config.WorkingDir'), '/working/dir/sample')
 
     def test_split_env(self):
         service = self.create_service('web', environment=['NORMAL=F1', 'CONTAINS_EQUALS=F=2', 'TRAILING_EQUALS='])
-        env = service.start_container().environment
+        env = create_and_start_container(service).environment
         for k,v in {'NORMAL': 'F1', 'CONTAINS_EQUALS': 'F=2', 'TRAILING_EQUALS': ''}.iteritems():
             self.assertEqual(env[k], v)
 
     def test_env_from_file_combined_with_env(self):
         service = self.create_service('web', environment=['ONE=1', 'TWO=2', 'THREE=3'], env_file=['tests/fixtures/env/one.env', 'tests/fixtures/env/two.env'])
-        env = service.start_container().environment
+        env = create_and_start_container(service).environment
         for k,v in {'ONE': '1', 'TWO': '2', 'THREE': '3', 'FOO': 'baz', 'DOO': 'dah'}.iteritems():
             self.assertEqual(env[k], v)
 
@@ -438,7 +443,7 @@ class ServiceTest(DockerClientTestCase):
         os.environ['FILE_DEF_EMPTY'] = 'E2'
         os.environ['ENV_DEF'] = 'E3'
         try:
-            env = service.start_container().environment
+            env = create_and_start_container(service).environment
             for k,v in {'FILE_DEF': 'F1', 'FILE_DEF_EMPTY': '', 'ENV_DEF': 'E3', 'NO_DEF': ''}.iteritems():
                 self.assertEqual(env[k], v)
         finally:

tests/unit/service_test.py

@@ -189,20 +189,30 @@ class ServiceTest(unittest.TestCase):
         self.mock_client.pull.assert_called_once_with('someimage:sometag', insecure_registry=True)
         mock_log.info.assert_called_once_with('Pulling foo (someimage:sometag)...')
 
+    @mock.patch('fig.service.Container', autospec=True)
     @mock.patch('fig.service.log', autospec=True)
-    def test_create_container_from_insecure_registry(self, mock_log):
+    def test_create_container_from_insecure_registry(
+            self,
+            mock_log,
+            mock_container):
         service = Service('foo', client=self.mock_client, image='someimage:sometag')
         mock_response = mock.Mock(Response)
         mock_response.status_code = 404
         mock_response.reason = "Not Found"
-        Container.create = mock.Mock()
+        mock_container.create.side_effect = APIError(
-        Container.create.side_effect = APIError('Mock error', mock_response, "No such image")
+            'Mock error', mock_response, "No such image")
-        try:
+
+        # We expect the APIError because our service requires a
+        # non-existent image.
+        with self.assertRaises(APIError):
             service.create_container(insecure_registry=True)
-        except APIError:  # We expect the APIError because our service requires a non-existent image.
+
-            pass
+        self.mock_client.pull.assert_called_once_with(
-        self.mock_client.pull.assert_called_once_with('someimage:sometag', insecure_registry=True, stream=True)
+            'someimage:sometag',
-        mock_log.info.assert_called_once_with('Pulling image someimage:sometag...')
+            insecure_registry=True,
+            stream=True)
+        mock_log.info.assert_called_once_with(
+            'Pulling image someimage:sometag...')
 
     def test_parse_repository_tag(self):
         self.assertEqual(parse_repository_tag("root"), ("root", ""))
@@ -218,6 +228,23 @@ class ServiceTest(unittest.TestCase):
         service.create_container()
         self.assertEqual(Container.create.call_args[1]['image'], 'someimage:latest')
 
+    def test_create_container_with_build(self):
+        self.mock_client.images.return_value = []
+        service = Service('foo', client=self.mock_client, build='.')
+        service.build = mock.create_autospec(service.build)
+        service.create_container(do_build=True)
+
+        self.mock_client.images.assert_called_once_with(name=service.full_name)
+        service.build.assert_called_once_with()
+
+    def test_create_container_no_build(self):
+        self.mock_client.images.return_value = []
+        service = Service('foo', client=self.mock_client, build='.')
+        service.create_container(do_build=False)
+
+        self.assertFalse(self.mock_client.images.called)
+        self.assertFalse(self.mock_client.build.called)
+
 
 class ServiceVolumesTest(unittest.TestCase):