From 7603ebea9b2f484f3cf8f193f64748fb91b67bf6 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Wed, 21 Oct 2015 17:28:16 +0100 Subject: [PATCH 1/4] Attach to a container's log_stream before they're started So we're not displaying output of all previous logs for a container, we attach, if possible, to a container before the container is started. LogPrinter checks if a container has a log_stream already attached and print from that rather than always attempting to attach one itself. Signed-off-by: Mazz Mosley --- compose/cli/log_printer.py | 10 ++++-- compose/cli/main.py | 11 +++++-- compose/container.py | 8 +++++ compose/project.py | 6 ++-- compose/service.py | 54 ++++++++++++++++++++++----------- tests/integration/state_test.py | 4 ++- 6 files changed, 66 insertions(+), 27 deletions(-) diff --git a/compose/cli/log_printer.py b/compose/cli/log_printer.py index 6e1499e1d..66920726c 100644 --- a/compose/cli/log_printer.py +++ b/compose/cli/log_printer.py @@ -73,9 +73,13 @@ def build_no_log_generator(container, prefix, color_func): def build_log_generator(container, prefix, color_func): - # Attach to container before log printer starts running - stream = container.attach(stdout=True, stderr=True, stream=True, logs=True) - line_generator = split_buffer(stream) + # if the container doesn't have a log_stream we need to attach to container + # before log printer starts running + if container.log_stream is None: + stream = container.attach(stdout=True, stderr=True, stream=True, logs=True) + line_generator = split_buffer(stream) + else: + line_generator = split_buffer(container.log_stream) for line in line_generator: yield prefix + line diff --git a/compose/cli/main.py b/compose/cli/main.py index c800d95f9..5505b89f5 100644 --- a/compose/cli/main.py +++ b/compose/cli/main.py @@ -565,16 +565,18 @@ class TopLevelCommand(DocoptCommand): start_deps = not options['--no-deps'] service_names = options['SERVICE'] timeout = int(options.get('--timeout') or DEFAULT_TIMEOUT) + detached = options.get('-d') to_attach = project.up( service_names=service_names, start_deps=start_deps, strategy=convergence_strategy_from_opts(options), do_build=not options['--no-build'], - timeout=timeout + timeout=timeout, + detached=detached ) - if not options['-d']: + if not detached: log_printer = build_log_printer(to_attach, service_names, monochrome) attach_to_logs(project, log_printer, service_names, timeout) @@ -636,7 +638,10 @@ def convergence_strategy_from_opts(options): def build_log_printer(containers, service_names, monochrome): if service_names: - containers = [c for c in containers if c.service in service_names] + containers = [ + container + for container in containers if container.service in service_names + ] return LogPrinter(containers, monochrome=monochrome) diff --git a/compose/container.py b/compose/container.py index a03acf56f..64773b9e6 100644 --- a/compose/container.py +++ b/compose/container.py @@ -19,6 +19,7 @@ class Container(object): self.client = client self.dictionary = dictionary self.has_been_inspected = has_been_inspected + self.log_stream = None @classmethod def from_ps(cls, client, dictionary, **kwargs): @@ -146,6 +147,13 @@ class Container(object): log_type = self.log_driver return not log_type or log_type == 'json-file' + def attach_log_stream(self): + """A log stream can only be attached if the container uses a json-file + log driver. + """ + if self.has_api_logs: + self.log_stream = self.attach(stdout=True, stderr=True, stream=True) + def get(self, key): """Return a value from the container or None if the value is not set. diff --git a/compose/project.py b/compose/project.py index d4934c268..68edaddcb 100644 --- a/compose/project.py +++ b/compose/project.py @@ -290,7 +290,8 @@ class Project(object): start_deps=True, strategy=ConvergenceStrategy.changed, do_build=True, - timeout=DEFAULT_TIMEOUT): + timeout=DEFAULT_TIMEOUT, + detached=False): services = self.get_services(service_names, include_deps=start_deps) @@ -308,7 +309,8 @@ class Project(object): for container in service.execute_convergence_plan( plans[service.name], do_build=do_build, - timeout=timeout + timeout=timeout, + detached=detached ) ] diff --git a/compose/service.py b/compose/service.py index 3bb47432b..aefeda318 100644 --- a/compose/service.py +++ b/compose/service.py @@ -395,11 +395,17 @@ class Service(object): def execute_convergence_plan(self, plan, do_build=True, - timeout=DEFAULT_TIMEOUT): + timeout=DEFAULT_TIMEOUT, + detached=False): (action, containers) = plan + should_attach_logs = not detached if action == 'create': container = self.create_container(do_build=do_build) + + if should_attach_logs: + container.attach_log_stream() + self.start_container(container) return [container] @@ -407,15 +413,16 @@ class Service(object): elif action == 'recreate': return [ self.recreate_container( - c, - timeout=timeout + container, + timeout=timeout, + attach_logs=should_attach_logs ) - for c in containers + for container in containers ] elif action == 'start': - for c in containers: - self.start_container_if_stopped(c) + for container in containers: + self.start_container_if_stopped(container, attach_logs=should_attach_logs) return containers @@ -428,16 +435,7 @@ class Service(object): else: raise Exception("Invalid action: {}".format(action)) - def recreate_container(self, - container, - timeout=DEFAULT_TIMEOUT): - """Recreate a container. - - The original container is renamed to a temporary name so that data - volumes can be copied to the new container, before the original - container is removed. - """ - log.info("Recreating %s" % container.name) + def _recreate_stop_container(self, container, timeout): try: container.stop(timeout=timeout) except APIError as e: @@ -448,26 +446,46 @@ class Service(object): else: raise + def _recreate_rename_container(self, container): # Use a hopefully unique container name by prepending the short id self.client.rename( container.id, - '%s_%s' % (container.short_id, container.name)) + '%s_%s' % (container.short_id, container.name) + ) + def recreate_container(self, + container, + timeout=DEFAULT_TIMEOUT, + attach_logs=False): + """Recreate a container. + + The original container is renamed to a temporary name so that data + volumes can be copied to the new container, before the original + container is removed. + """ + log.info("Recreating %s" % container.name) + + self._recreate_stop_container(container, timeout) + self._recreate_rename_container(container) new_container = self.create_container( do_build=False, previous_container=container, number=container.labels.get(LABEL_CONTAINER_NUMBER), quiet=True, ) + if attach_logs: + new_container.attach_log_stream() self.start_container(new_container) container.remove() return new_container - def start_container_if_stopped(self, container): + def start_container_if_stopped(self, container, attach_logs=False): if container.is_running: return container else: log.info("Starting %s" % container.name) + if attach_logs: + container.attach_log_stream() return self.start_container(container) def start_container(self, container): diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index ef7276bd8..02e9d3152 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -18,6 +18,7 @@ from compose.service import ConvergenceStrategy class ProjectTestCase(DockerClientTestCase): def run_up(self, cfg, **kwargs): kwargs.setdefault('timeout', 1) + kwargs.setdefault('detached', True) project = self.make_project(cfg) project.up(**kwargs) @@ -184,7 +185,8 @@ def converge(service, do_build=True): """Create a converge plan from a strategy and execute the plan.""" plan = service.convergence_plan(strategy) - return service.execute_convergence_plan(plan, do_build=do_build, timeout=1) + containers, logging_threads = zip(*service.execute_convergence_plan(plan, do_build=do_build, timeout=1)) + return containers class ServiceStateTest(DockerClientTestCase): From bee063c07dd8ca8c7f0295a9a319c14b815c4422 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Mon, 26 Oct 2015 10:27:57 +0000 Subject: [PATCH 2/4] Fix tests Signed-off-by: Mazz Mosley --- tests/integration/service_test.py | 1 + tests/integration/state_test.py | 3 +-- tests/unit/cli/log_printer_test.py | 1 + tests/unit/service_test.py | 4 +--- 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 8a8e4d54d..38d7d5b55 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -362,6 +362,7 @@ class ServiceTest(DockerClientTestCase): new_container, = service.execute_convergence_plan( ConvergencePlan('recreate', [old_container])) + self.assertEqual(list(new_container.get('Volumes')), ['/data']) self.assertEqual(new_container.get('Volumes')['/data'], volume_path) diff --git a/tests/integration/state_test.py b/tests/integration/state_test.py index 02e9d3152..3230aefc6 100644 --- a/tests/integration/state_test.py +++ b/tests/integration/state_test.py @@ -185,8 +185,7 @@ def converge(service, do_build=True): """Create a converge plan from a strategy and execute the plan.""" plan = service.convergence_plan(strategy) - containers, logging_threads = zip(*service.execute_convergence_plan(plan, do_build=do_build, timeout=1)) - return containers + return service.execute_convergence_plan(plan, do_build=do_build, timeout=1) class ServiceStateTest(DockerClientTestCase): diff --git a/tests/unit/cli/log_printer_test.py b/tests/unit/cli/log_printer_test.py index 2c9168980..575fcaf7b 100644 --- a/tests/unit/cli/log_printer_test.py +++ b/tests/unit/cli/log_printer_test.py @@ -16,6 +16,7 @@ def build_mock_container(reader): name='myapp_web_1', name_without_project='web_1', has_api_logs=True, + log_stream=None, attach=reader, wait=mock.Mock(return_value=0), ) diff --git a/tests/unit/service_test.py b/tests/unit/service_test.py index b5bac2919..494c2cde7 100644 --- a/tests/unit/service_test.py +++ b/tests/unit/service_test.py @@ -323,9 +323,7 @@ class ServiceTest(unittest.TestCase): new_container = service.recreate_container(mock_container) mock_container.stop.assert_called_once_with(timeout=10) - self.mock_client.rename.assert_called_once_with( - mock_container.id, - '%s_%s' % (mock_container.short_id, mock_container.name)) + mock_container.rename_to_tmp_name.assert_called_once_with() new_container.start.assert_called_once_with() mock_container.remove.assert_called_once_with() From 30a84f1be6c68d1cbeaad865242f891f5cff82df Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 27 Oct 2015 11:55:35 +0000 Subject: [PATCH 3/4] Move rename functionality into Container Signed-off-by: Mazz Mosley --- compose/container.py | 9 +++++++++ compose/service.py | 9 +-------- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/compose/container.py b/compose/container.py index 64773b9e6..dd69e8ddd 100644 --- a/compose/container.py +++ b/compose/container.py @@ -192,6 +192,15 @@ class Container(object): def remove(self, **options): return self.client.remove_container(self.id, **options) + def rename_to_tmp_name(self): + """Rename the container to a hopefully unique temporary container name + by prepending the short id. + """ + self.client.rename( + self.id, + '%s_%s' % (self.short_id, self.name) + ) + def inspect_if_not_inspected(self): if not self.has_been_inspected: self.inspect() diff --git a/compose/service.py b/compose/service.py index aefeda318..518a0d274 100644 --- a/compose/service.py +++ b/compose/service.py @@ -446,13 +446,6 @@ class Service(object): else: raise - def _recreate_rename_container(self, container): - # Use a hopefully unique container name by prepending the short id - self.client.rename( - container.id, - '%s_%s' % (container.short_id, container.name) - ) - def recreate_container(self, container, timeout=DEFAULT_TIMEOUT, @@ -466,7 +459,7 @@ class Service(object): log.info("Recreating %s" % container.name) self._recreate_stop_container(container, timeout) - self._recreate_rename_container(container) + container.rename_to_tmp_name() new_container = self.create_container( do_build=False, previous_container=container, From 76d52b1c5f02e526010468c22d5a438883ab4777 Mon Sep 17 00:00:00 2001 From: Mazz Mosley Date: Tue, 27 Oct 2015 11:59:09 +0000 Subject: [PATCH 4/4] Remove redundant try/except Code cleanup. We no longer need this as the api returns a 304 for any stopped containers, which doesn't raise an error. Signed-off-by: Mazz Mosley --- compose/service.py | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/compose/service.py b/compose/service.py index 518a0d274..2ca004cfb 100644 --- a/compose/service.py +++ b/compose/service.py @@ -435,17 +435,6 @@ class Service(object): else: raise Exception("Invalid action: {}".format(action)) - def _recreate_stop_container(self, container, timeout): - try: - container.stop(timeout=timeout) - except APIError as e: - if (e.response.status_code == 500 - and e.explanation - and 'no such process' in str(e.explanation)): - pass - else: - raise - def recreate_container(self, container, timeout=DEFAULT_TIMEOUT, @@ -458,7 +447,7 @@ class Service(object): """ log.info("Recreating %s" % container.name) - self._recreate_stop_container(container, timeout) + container.stop(timeout=timeout) container.rename_to_tmp_name() new_container = self.create_container( do_build=False,