From 66ed9b492ecefc7bdf57aa22ecac2c551a6fb67e 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 | 19 ++++++++-------- 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, 58 insertions(+), 46 deletions(-) diff --git a/compose/container.py b/compose/container.py index 026306866..8a2fb240e 100644 --- a/compose/container.py +++ b/compose/container.py @@ -7,6 +7,7 @@ import six 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 @@ class Container(object): @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 @@ class Container(object): 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 d2f49caeb..4dafff0f4 100644 --- a/compose/service.py +++ b/compose/service.py @@ -736,16 +736,18 @@ class Service(object): 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): @@ -823,7 +825,7 @@ class Service(object): 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]) @@ -832,7 +834,7 @@ class Service(object): 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) @@ -1120,12 +1122,12 @@ class Service(object): 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: @@ -1382,13 +1384,13 @@ class ServiceNetworkMode(object): # 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 @@ -1577,8 +1579,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 5b0a0e0fd..ae01a88ef 100644 --- a/tests/acceptance/cli_test.py +++ b/tests/acceptance/cli_test.py @@ -965,11 +965,11 @@ class CLITestCase(DockerClientTestCase): 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 @@ -1031,8 +1031,8 @@ class CLITestCase(DockerClientTestCase): 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 @@ -2332,10 +2332,9 @@ class CLITestCase(DockerClientTestCase): 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): @@ -2706,7 +2705,7 @@ class CLITestCase(DockerClientTestCase): ) 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 edc195287..000f6838c 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -32,7 +32,6 @@ from compose.const import LABEL_CONTAINER_NUMBER 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 @@ class ServiceTest(DockerClientTestCase): 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 @@ class ServiceTest(DockerClientTestCase): 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 a41986f46..b7d38a4ba 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -198,14 +198,14 @@ class ProjectWithDependenciesTest(ProjectTestCase): 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 66c6c1578..fde17847a 100644 --- a/tests/unit/container_test.py +++ b/tests/unit/container_test.py @@ -5,6 +5,7 @@ import docker 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 @@ class ContainerTest(unittest.TestCase): "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 @@ class ContainerTest(unittest.TestCase): 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 6c9ea1511..99adea34b 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -175,10 +175,10 @@ class ServiceTest(unittest.TestCase): 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 = {}