diff --git a/paunch/builder/base.py b/paunch/builder/base.py index eb3a2a0..dae67b4 100644 --- a/paunch/builder/base.py +++ b/paunch/builder/base.py @@ -12,6 +12,7 @@ # import distutils.spawn +import itertools import json import os import re @@ -50,16 +51,30 @@ class BaseBuilder(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) failed_containers = [] 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) cconfig = self.config[container] action = cconfig.get('action', 'run') @@ -172,44 +187,52 @@ class BaseBuilder(object): 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: if self.cleanup: self.log.debug("Deleting container (removed): " "%s" % container) self.runner.remove_container(container) + deleted = True else: self.log.debug("Skipping container (cleanup disabled): " "%s" % container) - continue + 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) + 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 + + ex_data_str = self.runner.inspect( + container, '{{index .Config.Labels "config_data"}}') + if not ex_data_str: + if self.cleanup: + self.log.debug("Deleting container (no_config_data): " + "%s" % container) self.runner.remove_container(container) - continue + return True + else: + self.log.debug("Skipping container (cleanup disabled): " + "%s" % container) - try: - ex_data = yaml.safe_load(str(ex_data_str)) - except Exception: - ex_data = None + try: + ex_data = yaml.safe_load(str(ex_data_str)) + except Exception: + ex_data = None - 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) - - # 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 99a4a3a..078a633 100644 --- a/paunch/runner.py +++ b/paunch/runner.py @@ -272,9 +272,11 @@ class BaseRunner(object): if returncode != 0: return results += cmd_stdout.split("\n") + result = [] for line in results: if line: - yield line.split() + result.append(line.split()) + return result def remove_containers(self, conf_id): for container in self.containers_in_config(conf_id): @@ -301,6 +303,7 @@ class BaseRunner(object): def rename_containers(self): current_containers = [] need_renaming = {} + renamed = False for entry in self.container_names(): current_containers.append(entry[0]) @@ -321,7 +324,9 @@ class BaseRunner(object): else: self.log.info('Renaming "%s" to "%s"' % (current, desired)) self.rename_container(current, desired) + renamed = True current_containers.append(desired) + return renamed def validate_volume_source(self, volume): """Validate that the provided volume diff --git a/paunch/tests/test_builder_base.py b/paunch/tests/test_builder_base.py index 40ee804..51df837 100644 --- a/paunch/tests/test_builder_base.py +++ b/paunch/tests/test_builder_base.py @@ -27,7 +27,9 @@ from paunch.tests import base class TestBaseBuilder(base.TestCase): @mock.patch("psutil.Process.cpu_affinity", return_value=[0, 1, 2, 3]) - def test_apply(self, mock_cpu): + @mock.patch("paunch.builder.base.BaseBuilder.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 = { @@ -61,17 +63,25 @@ class TestBaseBuilder(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), # ps2 for delete_missing_and_updated container_names - ('', '', 0), # ps for after delete_missing_and_updated renames - ('', '', 0), # ps2 for after delete_missing_and_updated renames - ('', '', 0), # ps to only create containers which don't exist - ('', '', 0), # ps2 to only create containers which don't exist + # container_names for delete_missing (twice by managed_by) + ('', '', 0), + ('''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 @@ -89,7 +99,6 @@ class TestBaseBuilder(base.TestCase): 'Pulled centos:7', 'Created one-12345678', 'Created two-12345678', - 'Created three-12345678', 'Created four-12345678', 'a\nb\nc' ], stdout) @@ -114,7 +123,7 @@ class TestBaseBuilder(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', @@ -122,7 +131,6 @@ class TestBaseBuilder(base.TestCase): '--format', '{{.Names}} {{.Label "container_name"}}'], mock.ANY ), - # ps2 for delete_missing_and_updated container_names mock.call( ['docker', 'ps', '-a', '--filter', 'label=managed_by=paunch', @@ -130,21 +138,22 @@ class TestBaseBuilder(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 ), - # ps2 for after delete_missing_and_updated renames - mock.call( - ['docker', 'ps', '-a', - '--filter', 'label=managed_by=paunch', - '--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', @@ -152,14 +161,6 @@ class TestBaseBuilder(base.TestCase): '--format', '{{.Names}} {{.Label "container_name"}}'], mock.ANY ), - # ps2 to only create containers which don't exist - mock.call( - ['docker', 'ps', '-a', - '--filter', 'label=managed_by=paunch', - '--filter', 'label=config_id=foo', - '--format', '{{.Names}} {{.Label "container_name"}}'], - mock.ANY - ), # run one mock.call( ['docker', 'run', '--name', 'one-12345678', @@ -180,16 +181,6 @@ class TestBaseBuilder(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', @@ -208,19 +199,22 @@ class TestBaseBuilder(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.BaseRunner.container_names") + @mock.patch("paunch.runner.BaseRunner.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', @@ -230,49 +224,46 @@ class TestBaseBuilder(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', cont_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), - # ps2 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 @@ -282,8 +273,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) @@ -295,61 +286,17 @@ three-12345678 three''', '', 0), ['docker', 'inspect', '--type', 'image', '--format', 'exists', 'centos:7'], mock.ANY, False ), - # 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, False), - 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, False), - # 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 - ), - # ps2 for after delete_missing_and_updated renames - mock.call( - ['docker', 'ps', '-a', - '--filter', 'label=managed_by=paunch', - '--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, False), + # don't run one, its already running # run two mock.call( ['docker', 'run', '--name', 'two-12345678', @@ -360,7 +307,22 @@ three-12345678 three''', '', 0), '--detach=true', '--cpuset-cpus=0,1,2,3', 'centos:7'], mock.ANY ), - # don't run three, its already running + # rm three, changed config + mock.call(['docker', 'inspect', '--type', 'container', + '--format', '{{index .Config.Labels "config_data"}}', + 'three'], mock.ANY, False), + 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 + ), # run four mock.call( ['docker', 'run', '--name', 'four-12345678', @@ -371,9 +333,10 @@ three-12345678 three''', '', 0), '--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', + ['docker', 'exec', 'one-12345678', 'ls', '-l', '/'], mock.ANY ), ])