From 61bb1ea4849a4d7d2b98a6689d5e1813b06639bd Mon Sep 17 00:00:00 2001 From: Joffrey F Date: Tue, 27 Nov 2018 17:09:36 -0800 Subject: [PATCH] Don't append slugs to containers created by "up" This change reverts the new naming convention introduced in 1.23 for service containers. One-off containers will now use a slug instead of a sequential number as they do not present addressability concerns and benefit from being capable of running in parallel. Signed-off-by: Joffrey F --- compose/container.py | 11 +++++++++- compose/service.py | 36 +++++++++++++++++-------------- tests/acceptance/cli_test.py | 23 ++++++++++---------- tests/integration/service_test.py | 9 +++----- tests/integration/state_test.py | 8 +++---- tests/unit/container_test.py | 17 +++++++++------ tests/unit/service_test.py | 4 ++-- 7 files changed, 60 insertions(+), 48 deletions(-) diff --git a/compose/container.py b/compose/container.py index 026306866a7..8a2fb240e0d 100644 --- a/compose/container.py +++ b/compose/container.py @@ -7,6 +7,7 @@ from docker.errors import ImageNotFound from .const import LABEL_CONTAINER_NUMBER +from .const import LABEL_ONE_OFF from .const import LABEL_PROJECT from .const import LABEL_SERVICE from .const import LABEL_SLUG @@ -82,12 +83,16 @@ def service(self): @property def name_without_project(self): if self.name.startswith('{0}_{1}'.format(self.project, self.service)): - return '{0}_{1}{2}'.format(self.service, self.number, '_' + self.slug if self.slug else '') + return '{0}_{1}'.format(self.service, self.number if self.number is not None else self.slug) else: return self.name @property def number(self): + if self.one_off: + # One-off containers are no longer assigned numbers and use slugs instead. + return None + number = self.labels.get(LABEL_CONTAINER_NUMBER) if not number: raise ValueError("Container {0} does not have a {1} label".format( @@ -104,6 +109,10 @@ def slug(self): def full_slug(self): return self.labels.get(LABEL_SLUG) + @property + def one_off(self): + return self.labels.get(LABEL_ONE_OFF) == 'True' + @property def ports(self): self.inspect_if_not_inspected() diff --git a/compose/service.py b/compose/service.py index 240ca9cd378..17d631e8a73 100644 --- a/compose/service.py +++ b/compose/service.py @@ -738,16 +738,18 @@ def get_volumes_from_names(self): return [s.source.name for s in self.volumes_from if isinstance(s.source, Service)] def _next_container_number(self, one_off=False): + if one_off: + return None containers = itertools.chain( self._fetch_containers( all=True, - filters={'label': self.labels(one_off=one_off)} + filters={'label': self.labels(one_off=False)} ), self._fetch_containers( all=True, - filters={'label': self.labels(one_off=one_off, legacy=True)} + filters={'label': self.labels(one_off=False, legacy=True)} ) ) - numbers = [c.number for c in containers] + numbers = [c.number for c in containers if c.number is not None] return 1 if not numbers else max(numbers) + 1 def _fetch_containers(self, **fetch_options): @@ -825,7 +827,7 @@ def _get_container_create_options( one_off=False, previous_container=None): add_config_hash = (not one_off and not override_options) - slug = generate_random_id() if previous_container is None else previous_container.full_slug + slug = generate_random_id() if one_off else None container_options = dict( (k, self.options[k]) @@ -834,7 +836,7 @@ def _get_container_create_options( container_options.update(override_options) if not container_options.get('name'): - container_options['name'] = self.get_container_name(self.name, number, slug, one_off) + container_options['name'] = self.get_container_name(self.name, number, slug) container_options.setdefault('detach', True) @@ -1122,12 +1124,12 @@ def labels(self, one_off=False, legacy=False): def custom_container_name(self): return self.options.get('container_name') - def get_container_name(self, service_name, number, slug, one_off=False): - if self.custom_container_name and not one_off: + def get_container_name(self, service_name, number, slug=None): + if self.custom_container_name and slug is None: return self.custom_container_name container_name = build_container_name( - self.project, service_name, number, slug, one_off, + self.project, service_name, number, slug, ) ext_links_origins = [l.split(':')[0] for l in self.options.get('external_links', [])] if container_name in ext_links_origins: @@ -1384,13 +1386,13 @@ def mode(self): # Names -def build_container_name(project, service, number, slug, one_off=False): +def build_container_name(project, service, number, slug=None): bits = [project.lstrip('-_'), service] - if one_off: - bits.append('run') - return '_'.join( - bits + ([str(number), truncate_id(slug)] if slug else [str(number)]) - ) + if slug: + bits.extend(['run', truncate_id(slug)]) + else: + bits.append(str(number)) + return '_'.join(bits) # Images @@ -1579,8 +1581,10 @@ def build_mount(mount_spec): def build_container_labels(label_options, service_labels, number, config_hash, slug): labels = dict(label_options or {}) labels.update(label.split('=', 1) for label in service_labels) - labels[LABEL_CONTAINER_NUMBER] = str(number) - labels[LABEL_SLUG] = slug + if number is not None: + labels[LABEL_CONTAINER_NUMBER] = str(number) + if slug is not None: + labels[LABEL_SLUG] = slug labels[LABEL_VERSION] = __version__ if config_hash: diff --git a/tests/acceptance/cli_test.py b/tests/acceptance/cli_test.py index f42268f2cf1..d49c16073b3 100644 --- a/tests/acceptance/cli_test.py +++ b/tests/acceptance/cli_test.py @@ -602,10 +602,10 @@ def test_ps_services_filter_status(self): def test_ps_all(self): self.project.get_service('simple').create_container(one_off='blahblah') result = self.dispatch(['ps']) - assert 'simple-composefile_simple_run_1' not in result.stdout + assert 'simple-composefile_simple_run_' not in result.stdout result2 = self.dispatch(['ps', '--all']) - assert 'simple-composefile_simple_run_1' in result2.stdout + assert 'simple-composefile_simple_run_' in result2.stdout def test_pull(self): result = self.dispatch(['pull']) @@ -973,11 +973,11 @@ def test_down(self): result = self.dispatch(['down', '--rmi=local', '--volumes']) assert 'Stopping v2-full_web_1' in result.stderr assert 'Stopping v2-full_other_1' in result.stderr - assert 'Stopping v2-full_web_run_2' in result.stderr + assert 'Stopping v2-full_web_run_' in result.stderr assert 'Removing v2-full_web_1' in result.stderr assert 'Removing v2-full_other_1' in result.stderr - assert 'Removing v2-full_web_run_1' in result.stderr - assert 'Removing v2-full_web_run_2' in result.stderr + assert 'Removing v2-full_web_run_' in result.stderr + assert 'Removing v2-full_web_run_' in result.stderr assert 'Removing volume v2-full_data' in result.stderr assert 'Removing image v2-full_web' in result.stderr assert 'Removing image busybox' not in result.stderr @@ -1039,8 +1039,8 @@ def test_up_attached(self): stopped=True )[0].name_without_project - assert '{} | simple'.format(simple_name) in result.stdout - assert '{} | another'.format(another_name) in result.stdout + assert '{} | simple'.format(simple_name) in result.stdout + assert '{} | another'.format(another_name) in result.stdout assert '{} exited with code 0'.format(simple_name) in result.stdout assert '{} exited with code 0'.format(another_name) in result.stdout @@ -2340,10 +2340,9 @@ def test_logs_follow_logs_from_restarted_containers(self): result = wait_on_process(proc) - assert len(re.findall( - r'logs-restart-composefile_another_1_[a-f0-9]{12} exited with code 1', - result.stdout - )) == 3 + assert result.stdout.count( + r'logs-restart-composefile_another_1 exited with code 1' + ) == 3 assert result.stdout.count('world') == 3 def test_logs_default(self): @@ -2714,7 +2713,7 @@ def test_forward_exitval(self): ) result = wait_on_process(proc, returncode=1) - assert re.findall(r'exit-code-from_another_1_[a-f0-9]{12} exited with code 1', result.stdout) + assert 'exit-code-from_another_1 exited with code 1' in result.stdout def test_exit_code_from_signal_stop(self): self.base_dir = 'tests/fixtures/exit-code-from' diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index edc19528716..000f6838c7c 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -32,7 +32,6 @@ from compose.const import LABEL_ONE_OFF from compose.const import LABEL_PROJECT from compose.const import LABEL_SERVICE -from compose.const import LABEL_SLUG from compose.const import LABEL_VERSION from compose.container import Container from compose.errors import OperationFailedError @@ -1269,16 +1268,15 @@ def test_scale_with_stopped_containers(self): test that those containers are restarted and not removed/recreated. """ service = self.create_service('web') - valid_numbers = [service._next_container_number(), service._next_container_number()] - service.create_container(number=valid_numbers[0]) - service.create_container(number=valid_numbers[1]) + service.create_container(number=1) + service.create_container(number=2) ParallelStreamWriter.instance = None with mock.patch('sys.stderr', new_callable=StringIO) as mock_stderr: service.scale(2) for container in service.containers(): assert container.is_running - assert container.number in valid_numbers + assert container.number in [1, 2] captured_output = mock_stderr.getvalue() assert 'Creating' not in captured_output @@ -1610,7 +1608,6 @@ def test_labels(self): labels = ctnr.labels.items() for pair in expected.items(): assert pair in labels - assert ctnr.labels[LABEL_SLUG] == ctnr.full_slug def test_empty_labels(self): labels_dict = {'foo': '', 'bar': ''} diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index a41986f4659..b7d38a4ba86 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -198,14 +198,14 @@ def test_service_recreated_when_dependency_created(self): db, = [c for c in containers if c.service == 'db'] assert set(get_links(web)) == { - 'composetest_db_{}_{}'.format(db.number, db.slug), + 'composetest_db_1', 'db', - 'db_{}_{}'.format(db.number, db.slug) + 'db_1', } assert set(get_links(nginx)) == { - 'composetest_web_{}_{}'.format(web.number, web.slug), + 'composetest_web_1', 'web', - 'web_{}_{}'.format(web.number, web.slug) + 'web_1', } diff --git a/tests/unit/container_test.py b/tests/unit/container_test.py index 66c6c15788f..fde17847a13 100644 --- a/tests/unit/container_test.py +++ b/tests/unit/container_test.py @@ -5,6 +5,7 @@ from .. import mock from .. import unittest +from compose.const import LABEL_ONE_OFF from compose.const import LABEL_SLUG from compose.container import Container from compose.container import get_container_name @@ -32,7 +33,6 @@ def setUp(self): "com.docker.compose.project": "composetest", "com.docker.compose.service": "web", "com.docker.compose.container-number": "7", - "com.docker.compose.slug": "092cd63296fdc446ad432d3905dd1fcbe12a2ba6b52" }, } } @@ -88,20 +88,23 @@ def test_name(self): assert container.name == "composetest_db_1" def test_name_without_project(self): - self.container_dict['Name'] = "/composetest_web_7_092cd63296fd" + self.container_dict['Name'] = "/composetest_web_7" container = Container(None, self.container_dict, has_been_inspected=True) - assert container.name_without_project == "web_7_092cd63296fd" + assert container.name_without_project == "web_7" def test_name_without_project_custom_container_name(self): self.container_dict['Name'] = "/custom_name_of_container" container = Container(None, self.container_dict, has_been_inspected=True) assert container.name_without_project == "custom_name_of_container" - def test_name_without_project_noslug(self): - self.container_dict['Name'] = "/composetest_web_7" - del self.container_dict['Config']['Labels'][LABEL_SLUG] + def test_name_without_project_one_off(self): + self.container_dict['Name'] = "/composetest_web_092cd63296f" + self.container_dict['Config']['Labels'][LABEL_SLUG] = ( + "092cd63296fdc446ad432d3905dd1fcbe12a2ba6b52" + ) + self.container_dict['Config']['Labels'][LABEL_ONE_OFF] = 'True' container = Container(None, self.container_dict, has_been_inspected=True) - assert container.name_without_project == 'web_7' + assert container.name_without_project == 'web_092cd63296fd' def test_inspect_if_not_inspected(self): mock_client = mock.create_autospec(docker.APIClient) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index 6c9ea151125..99adea34b43 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -175,10 +175,10 @@ def test_memory_swap_limit(self): def test_self_reference_external_link(self): service = Service( name='foo', - external_links=['default_foo_1_bdfa3ed91e2c'] + external_links=['default_foo_1'] ) with pytest.raises(DependencyError): - service.get_container_name('foo', 1, 'bdfa3ed91e2c') + service.get_container_name('foo', 1) def test_mem_reservation(self): self.mock_client.create_host_config.return_value = {}