From 1031491ec689f3f965bd189f4d333bcb94ac27fa Mon Sep 17 00:00:00 2001 From: Emilien Macchi Date: Wed, 12 Feb 2020 10:21:38 -0500 Subject: [PATCH] Cleanup containers in the same loop as they are created Split delete_missing_and_updated() into 2 methods: * delete_missing(), that will remove all containers installed on the host but missing from the given config. This runs outside of the loop, once. * delete_updated(), that will remove a container installed on the host (if present), that is part of the config, if config_data changed or didn't exist. It runs within the create loop, so the downtime between a container removal and creation should be shorter than before. * make delete_missing(), delete_updated() and rename_containers() returning True, if any container has been touched by either. Use that flag in order to keep the container_names contents always actual. * in order to make that cached container_names working and saving off extra podman ps/inspect calls, rework it to return a list instead of an iterator. There is no huge lists of containers, iterators buy us nothing here, while podman CLI calls are the more expensive thing and we optimize the latter instead. (cherry picked from commit eb3d3b75bb8bf300fb52390610a6a53ddbfaa48e) Co-Authored-By: Bogdan Dobrelya Change-Id: I3c6d0670e11d035287d12f4207489a13e0891943 Closes-Bug: #1862954 --- paunch/builder/compose1.py | 72 ++++++---- paunch/runner.py | 7 +- paunch/tests/test_builder_compose1.py | 193 ++++++++++++-------------- 3 files changed, 144 insertions(+), 128 deletions(-) diff --git a/paunch/builder/compose1.py b/paunch/builder/compose1.py index bdff3d4..6d4410d 100644 --- a/paunch/builder/compose1.py +++ b/paunch/builder/compose1.py @@ -11,6 +11,7 @@ # under the License. # +import itertools import json import tenacity import yaml @@ -36,15 +37,29 @@ class ComposeV1Builder(object): if pull_returncode != 0: return stdout, stderr, pull_returncode - self.delete_missing_and_updated() - deploy_status_code = 0 key_fltr = lambda k: self.config[k].get('start_order', 0) container_names = self.runner.container_names(self.config_id) + + # Cleanup containers missing from the config. + # Also applying new containers configs is an opportunity for + # renames to their preferred names. + changed = self.delete_missing(container_names) + changed |= self.runner.rename_containers() + if changed: + # If anything has been changed, refresh the container_names + container_names = self.runner.container_names(self.config_id) desired_names = set([cn[-1] for cn in container_names]) for container in sorted(self.config, key=key_fltr): + # Before creating the container, figure out if it needs to be + # removed because of its configuration has changed. + # If anything has been deleted, refresh the container_names/desired + if self.delete_updated(container, container_names): + container_names = self.runner.container_names(self.config_id) + desired_names = set([cn[-1] for cn in container_names]) + self.log.debug("Running container: %s" % container) action = self.config[container].get('action', 'run') exit_codes = self.config[container].get('exit_codes', [0]) @@ -87,39 +102,44 @@ class ComposeV1Builder(object): self.log.info("stderr: %s" % cmd_stderr) return stdout, stderr, deploy_status_code - def delete_missing_and_updated(self): - container_names = self.runner.container_names(self.config_id) + def delete_missing(self, container_names): + deleted = False for cn in container_names: container = cn[0] - # if the desired name is not in the config, delete it if cn[-1] not in self.config: - self.log.debug("Deleting container (removed): %s" % container) + self.log.debug("Deleting container (removed): " + "%s" % container) self.runner.remove_container(container) - continue + deleted = True + return deleted - ex_data_str = self.runner.inspect( - container, '{{index .Config.Labels "config_data"}}') - if not ex_data_str: - self.log.debug("Deleting container (no config_data): %s" % - container) - self.runner.remove_container(container) - continue + def delete_updated(self, container, container_names): + # If a container is not deployed, there is nothing to delete + if (container not in + list(itertools.chain.from_iterable(container_names))): + return False - try: - ex_data = yaml.safe_load(str(ex_data_str)) - except Exception: - ex_data = None + ex_data_str = self.runner.inspect( + container, '{{index .Config.Labels "config_data"}}') + if not ex_data_str: + self.log.debug("Deleting container (no_config_data): " + "%s" % container) + self.runner.remove_container(container) + return True - new_data = self.config.get(cn[-1]) - if new_data != ex_data: - self.log.debug("Deleting container (changed config_data): %s" % - container) - self.runner.remove_container(container) + try: + ex_data = yaml.safe_load(str(ex_data_str)) + except Exception: + ex_data = None - # deleting containers is an opportunity for renames to their - # preferred name - self.runner.rename_containers() + new_data = self.config[container] + if new_data != ex_data: + self.log.debug("Deleting container (changed config_data): %s" + % container) + self.runner.remove_container(container) + return True + return False def label_arguments(self, cmd, container): if self.labels: diff --git a/paunch/runner.py b/paunch/runner.py index d174ca7..474e84f 100644 --- a/paunch/runner.py +++ b/paunch/runner.py @@ -101,13 +101,16 @@ class DockerRunner(object): cmd_stdout, cmd_stderr, returncode = self.execute(cmd, self.log) if returncode != 0: return + result = [] for line in cmd_stdout.split("\n"): if line: - yield line.split() + result.append(line.split()) + return result def rename_containers(self): current_containers = [] need_renaming = {} + renamed = False for entry in self.container_names(): current_containers.append(entry[0]) @@ -129,7 +132,9 @@ class DockerRunner(object): self.log.info('Renaming "%s" to "%s"' % ( current, desired)) self.rename_container(current, desired) + renamed = True current_containers.append(desired) + return renamed def rename_container(self, container, name): cmd = [self.docker_cmd, 'rename', container, name] diff --git a/paunch/tests/test_builder_compose1.py b/paunch/tests/test_builder_compose1.py index cd18045..247f7cc 100644 --- a/paunch/tests/test_builder_compose1.py +++ b/paunch/tests/test_builder_compose1.py @@ -26,7 +26,9 @@ from paunch.tests import base class TestComposeV1Builder(base.TestCase): @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) - def test_apply(self, mock_cpu): + @mock.patch("paunch.builder.compose1.ComposeV1Builder.delete_updated", + return_value=False) + def test_apply(self, mock_delete_updated, mock_cpu): orig_call = tenacity.wait.wait_random_exponential.__call__ orig_argspec = inspect.getargspec(orig_call) config = { @@ -60,14 +62,24 @@ class TestComposeV1Builder(base.TestCase): ('', '', 1), # inspect for missing image centos:7 ('Pulled centos:7', 'ouch', 1), # pull centos:6 fails ('Pulled centos:7', '', 0), # pull centos:6 succeeds - ('', '', 0), # ps for delete_missing_and_updated container_names - ('', '', 0), # ps for after delete_missing_and_updated renames - ('', '', 0), # ps to only create containers which don't exist + # container_names for delete_missing + ('''five five +six six +two two +three-12345678 three''', '', 0), + ('', '', 0), # stop five + ('', '', 0), # rm five + ('', '', 0), # stop six + ('', '', 0), # rm six + # container_names for rename_containers + ('three-12345678 three', '', 0), + ('', '', 0), # rename three + # desired/container_names to be refreshed after delete/rename + ('three three', '', 0), # renamed three already exists ('Created one-12345678', '', 0), ('Created two-12345678', '', 0), - ('Created three-12345678', '', 0), ('Created four-12345678', '', 0), - ('a\nb\nc', '', 0) + ('a\nb\nc', '', 0) # exec four ] r.discover_container_name = lambda n, c: '%s-12345678' % n r.unique_container_name = lambda n: '%s-12345678' % n @@ -85,7 +97,6 @@ class TestComposeV1Builder(base.TestCase): 'Pulled centos:7', 'Created one-12345678', 'Created two-12345678', - 'Created three-12345678', 'Created four-12345678', 'a\nb\nc' ], stdout) @@ -110,7 +121,7 @@ class TestComposeV1Builder(base.TestCase): mock.call( ['docker', 'pull', 'centos:7'], mock.ANY ), - # ps for delete_missing_and_updated container_names + # container_names for delete_missing mock.call( ['docker', 'ps', '-a', '--filter', 'label=managed_by=tester', @@ -118,14 +129,22 @@ class TestComposeV1Builder(base.TestCase): '--format', '{{.Names}} {{.Label "container_name"}}'], mock.ANY ), - # ps for after delete_missing_and_updated renames + # rm containers missing in config + mock.call(['docker', 'stop', 'five'], mock.ANY), + mock.call(['docker', 'rm', 'five'], mock.ANY), + mock.call(['docker', 'stop', 'six'], mock.ANY), + mock.call(['docker', 'rm', 'six'], mock.ANY), + # container_names for rename mock.call( ['docker', 'ps', '-a', '--filter', 'label=managed_by=tester', '--format', '{{.Names}} {{.Label "container_name"}}'], mock.ANY ), - # ps to only create containers which don't exist + # rename three from an ephemeral to the static name + mock.call(['docker', 'rename', 'three-12345678', 'three'], + mock.ANY), + # container_names to be refreshed after delete/rename mock.call( ['docker', 'ps', '-a', '--filter', 'label=managed_by=tester', @@ -153,16 +172,6 @@ class TestComposeV1Builder(base.TestCase): '--detach=true', '--cpuset-cpus=0,1,2,3', 'centos:7'], mock.ANY ), - # run three - mock.call( - ['docker', 'run', '--name', 'three-12345678', - '--label', 'config_id=foo', - '--label', 'container_name=three', - '--label', 'managed_by=tester', - '--label', 'config_data=%s' % json.dumps(config['three']), - '--detach=true', '--cpuset-cpus=0,1,2,3', 'centos:6'], - mock.ANY - ), # run four mock.call( ['docker', 'run', '--name', 'four-12345678', @@ -180,19 +189,22 @@ class TestComposeV1Builder(base.TestCase): ]) @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) - def test_apply_idempotency(self, mock_cpu): + @mock.patch("paunch.runner.DockerRunner.container_names") + @mock.patch("paunch.runner.DockerRunner.discover_container_name", + return_value='one') + def test_apply_idempotency(self, mock_dname, mock_cnames, mock_cpu): config = { - # not running yet + # running with the same config and given an ephemeral name 'one': { 'start_order': 0, 'image': 'centos:7', }, - # running, but with a different config + # not running yet 'two': { 'start_order': 1, 'image': 'centos:7', }, - # running with the same config + # running, but with a different config 'three': { 'start_order': 2, 'image': 'centos:7', @@ -202,47 +214,46 @@ class TestComposeV1Builder(base.TestCase): 'start_order': 10, 'image': 'centos:7', }, - 'four_ls': { + 'one_ls': { 'action': 'exec', 'start_order': 20, - 'command': ['four', 'ls', '-l', '/'] + 'command': ['one', 'ls', '-l', '/'] } + # five is running but is not managed by us } - + # represents the state before and after renaming/removing things + mock_cnames.side_effect = ( + # delete_missing + [['five', 'five'], ['one-12345678', 'one'], ['three', 'three']], + # rename_containers + [['one-12345678', 'one']], + # refresh container_names/desired after del/rename + [['one', 'one'], ['three', 'three']], + # refresh container_names/desired after delete_updated + [['one', 'one']] + ) r = runner.DockerRunner(managed_by='tester', docker_cmd='docker') exe = mock.Mock() exe.side_effect = [ # inspect for image centos:7 ('exists', '', 0), - # ps for delete_missing_and_updated container_names - ('''five five -six six -two-12345678 two -three-12345678 three''', '', 0), # stop five ('', '', 0), # rm five ('', '', 0), - # stop six - ('', '', 0), - # rm six - ('', '', 0), - # inspect two - ('{"start_order": 1, "image": "centos:6"}', '', 0), - # stop two, changed config data - ('', '', 0), - # rm two, changed config data - ('', '', 0), - # inspect three - ('{"start_order": 2, "image": "centos:7"}', '', 0), - # ps for after delete_missing_and_updated renames - ('', '', 0), - # ps to only create containers which don't exist - ('three-12345678 three', '', 0), - ('Created one-12345678', '', 0), + ('', '', 0), # ps for rename one + # inspect one + ('{"start_order": 0, "image": "centos:7"}', '', 0), ('Created two-12345678', '', 0), + # inspect three + ('{"start_order": 42, "image": "centos:7"}', '', 0), + # stop three, changed config data + ('', '', 0), + # rm three, changed config data + ('', '', 0), + ('Created three-12345678', '', 0), ('Created four-12345678', '', 0), - ('a\nb\nc', '', 0) + ('a\nb\nc', '', 0) # exec one ] r.discover_container_name = lambda n, c: '%s-12345678' % n r.unique_container_name = lambda n: '%s-12345678' % n @@ -252,8 +263,8 @@ three-12345678 three''', '', 0), stdout, stderr, deploy_status_code = builder.apply() self.assertEqual(0, deploy_status_code) self.assertEqual([ - 'Created one-12345678', 'Created two-12345678', + 'Created three-12345678', 'Created four-12345678', 'a\nb\nc' ], stdout) @@ -265,54 +276,17 @@ three-12345678 three''', '', 0), ['docker', 'inspect', '--type', 'image', '--format', 'exists', 'centos:7'], mock.ANY ), - # ps for delete_missing_and_updated container_names - mock.call( - ['docker', 'ps', '-a', - '--filter', 'label=managed_by=tester', - '--filter', 'label=config_id=foo', - '--format', '{{.Names}} {{.Label "container_name"}}'], - mock.ANY - ), # rm containers not in config mock.call(['docker', 'stop', 'five'], mock.ANY), mock.call(['docker', 'rm', 'five'], mock.ANY), - mock.call(['docker', 'stop', 'six'], mock.ANY), - mock.call(['docker', 'rm', 'six'], mock.ANY), - # rm two, changed config + # rename one from an ephemeral to the static name + mock.call(['docker', 'rename', 'one-12345678', 'one'], + mock.ANY), + # check the renamed one, config hasn't changed mock.call(['docker', 'inspect', '--type', 'container', '--format', '{{index .Config.Labels "config_data"}}', - 'two-12345678'], mock.ANY), - mock.call(['docker', 'stop', 'two-12345678'], mock.ANY), - mock.call(['docker', 'rm', 'two-12345678'], mock.ANY), - # check three, config hasn't changed - mock.call(['docker', 'inspect', '--type', 'container', - '--format', '{{index .Config.Labels "config_data"}}', - 'three-12345678'], mock.ANY), - # ps for after delete_missing_and_updated renames - mock.call( - ['docker', 'ps', '-a', - '--filter', 'label=managed_by=tester', - '--format', '{{.Names}} {{.Label "container_name"}}'], - mock.ANY - ), - # ps to only create containers which don't exist - mock.call( - ['docker', 'ps', '-a', - '--filter', 'label=managed_by=tester', - '--filter', 'label=config_id=foo', - '--format', '{{.Names}} {{.Label "container_name"}}'], - mock.ANY - ), - # run one - mock.call( - ['docker', 'run', '--name', 'one-12345678', - '--label', 'config_id=foo', - '--label', 'container_name=one', - '--label', 'managed_by=tester', - '--label', 'config_data=%s' % json.dumps(config['one']), - '--detach=true', '--cpuset-cpus=0,1,2,3', 'centos:7'], - mock.ANY - ), + 'one'], mock.ANY), + # don't run one, its already running # run two mock.call( ['docker', 'run', '--name', 'two-12345678', @@ -320,10 +294,25 @@ three-12345678 three''', '', 0), '--label', 'container_name=two', '--label', 'managed_by=tester', '--label', 'config_data=%s' % json.dumps(config['two']), - '--detach=true', '--cpuset-cpus=0,1,2,3', 'centos:7'], - mock.ANY + '--detach=true', '--cpuset-cpus=0,1,2,3', + 'centos:7'], mock.ANY + ), + # rm three, changed config + mock.call(['docker', 'inspect', '--type', 'container', + '--format', '{{index .Config.Labels "config_data"}}', + 'three'], mock.ANY), + mock.call(['docker', 'stop', 'three'], mock.ANY), + mock.call(['docker', 'rm', 'three'], mock.ANY), + # run three + mock.call( + ['docker', 'run', '--name', 'three-12345678', + '--label', 'config_id=foo', + '--label', 'container_name=three', + '--label', 'managed_by=tester', + '--label', 'config_data=%s' % json.dumps(config['three']), + '--detach=true', '--cpuset-cpus=0,1,2,3', + 'centos:7'], mock.ANY ), - # don't run three, its already running # run four mock.call( ['docker', 'run', '--name', 'four-12345678', @@ -331,12 +320,14 @@ three-12345678 three''', '', 0), '--label', 'container_name=four', '--label', 'managed_by=tester', '--label', 'config_data=%s' % json.dumps(config['four']), - '--detach=true', '--cpuset-cpus=0,1,2,3', 'centos:7'], - mock.ANY + '--detach=true', '--cpuset-cpus=0,1,2,3', + 'centos:7'], mock.ANY ), - # execute within four + # FIXME(bogdando): shall exec ls in the renamed one! + # Why discover_container_name is never called to get it as c_name? mock.call( - ['docker', 'exec', 'four-12345678', 'ls', '-l', '/'], mock.ANY + ['docker', 'exec', 'one-12345678', 'ls', '-l', + '/'], mock.ANY ), ])