From 5b777ee5f18c0c8e3766550c62b69a89fa7a5642 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 26 Oct 2014 13:22:16 -0400 Subject: [PATCH 1/2] Cleanup service unit tests and restructure some service create logic. Signed-off-by: Daniel Nephin --- fig/service.py | 49 +++++++++++++----- tests/integration/service_test.py | 85 ++++++++++++++++--------------- tests/unit/service_test.py | 26 +++++++--- 3 files changed, 98 insertions(+), 62 deletions(-) diff --git a/fig/service.py b/fig/service.py index bd3000c62..461bc1fe5 100644 --- a/fig/service.py +++ b/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: @@ -273,8 +297,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): - container = container or self.create_container(**override_options) + def start_container(self, container, intermediate_container=None, **override_options): options = dict(self.options, **override_options) port_bindings = build_port_bindings(options.get('ports') or []) @@ -407,16 +430,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: - self.build() - container_options['image'] = self._build_tag_name() + container_options['image'] = self.full_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']: - if key in container_options: - del container_options[key] + for key in DOCKER_START_KEYS: + container_options.pop(key, None) return container_options @@ -431,7 +451,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 +471,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. """ diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index a1740272b..0e08ac1cb 100644 --- a/tests/integration/service_test.py +++ b/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() - bar.start_container() + create_and_start_container(bar) + 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() - db.start_container() - web.start_container() + create_and_start_container(db) + create_and_start_container(db) + 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() - db.start_container() - web.start_container() + create_and_start_container(db) + create_and_start_container(db) + 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() - db.start_container() + create_and_start_container(db) + 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() - db.start_container() + create_and_start_container(db) + 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,28 +366,28 @@ 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() - self.assertEqual(container['HostConfig']['Dns'], ['8.8.8.8']) + container = create_and_start_container(service) + 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() - self.assertEqual(container['HostConfig']['Dns'], ['8.8.8.8', '9.9.9.9']) + container = create_and_start_container(service) + 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') @@ -417,12 +422,12 @@ class ServiceTest(DockerClientTestCase): def test_working_dir_param(self): service = self.create_service('container', working_dir='/working/dir/sample') - container = service.create_container().inspect() - self.assertEqual(container['Config']['WorkingDir'], '/working/dir/sample') + container = service.create_container() + 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) @@ -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: diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 336f783fe..88ebd1d6b 100644 --- a/tests/unit/service_test.py +++ b/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() - Container.create.side_effect = APIError('Mock error', mock_response, "No such image") - try: + mock_container.create.side_effect = APIError( + 'Mock error', mock_response, "No such image") + + # 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('someimage:sometag', insecure_registry=True, stream=True) - mock_log.info.assert_called_once_with('Pulling image someimage:sometag...') + + self.mock_client.pull.assert_called_once_with( + '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", "")) From 3056ae4be329333e0f63568fc7d20a1d379b4172 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sun, 26 Oct 2014 13:23:15 -0400 Subject: [PATCH 2/2] Add a no-build option to fig up, to save time when services were already freshly built. Signed-off-by: Daniel Nephin --- fig/cli/main.py | 4 +++- fig/project.py | 18 +++++++++++++++--- fig/service.py | 20 ++++++++++++++------ tests/integration/service_test.py | 28 ++++++++++++++-------------- tests/unit/service_test.py | 17 +++++++++++++++++ 5 files changed, 63 insertions(+), 24 deletions(-) diff --git a/fig/cli/main.py b/fig/cli/main.py index c367266bb..8cdaee620 100644 --- a/fig/cli/main.py +++ b/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()] diff --git a/fig/project.py b/fig/project.py index 8d71a4c29..e013da4e9 100644 --- a/fig/project.py +++ b/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 diff --git a/fig/service.py b/fig/service.py index 461bc1fe5..d06d271f6 100644 --- a/fig/service.py +++ b/fig/service.py @@ -54,7 +54,7 @@ DOCKER_START_KEYS = [ 'cap_add', 'cap_drop', 'dns', - 'dns_search', + 'dns_search', 'env_file', 'net', 'privileged', @@ -236,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. @@ -244,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: @@ -283,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() @@ -330,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: diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 0e08ac1cb..fca4da67d 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -391,34 +391,34 @@ class ServiceTest(DockerClientTestCase): def test_restart_always_value(self): service = self.create_service('web', restart='always') - container = service.start_container().inspect() - self.assertEqual(container['HostConfig']['RestartPolicy']['Name'], 'always') + container = create_and_start_container(service) + 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() - self.assertEqual(container['HostConfig']['RestartPolicy']['Name'], 'on-failure') - self.assertEqual(container['HostConfig']['RestartPolicy']['MaximumRetryCount'], 5) + container = create_and_start_container(service) + self.assertEqual(container.get('HostConfig.RestartPolicy.Name'), 'on-failure') + 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() - self.assertEqual(container['HostConfig']['CapAdd'], ['SYS_ADMIN', 'NET_ADMIN']) + container = create_and_start_container(service) + 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() - self.assertEqual(container['HostConfig']['CapDrop'], ['SYS_ADMIN', 'NET_ADMIN']) + container = create_and_start_container(service) + 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() - self.assertEqual(container['HostConfig']['DnsSearch'], ['example.com']) + container = create_and_start_container(service) + 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() - self.assertEqual(container['HostConfig']['DnsSearch'], ['dc1.example.com', 'dc2.example.com']) + container = create_and_start_container(service) + 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') @@ -433,7 +433,7 @@ class ServiceTest(DockerClientTestCase): 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) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 88ebd1d6b..68dcf06ab 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -228,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):