Delete containers based on labels, not state files

This rewrites 50-heat-docker-cmd to not require any state directory to
make decisions on what containers to delete. Instead the labels on the
running containers are used to make decisions on when containers
should be deleted.

This also changes the behaviour to ignore the config name and strictly
use the config id, so all containers for a given config id are deleted
when that config no longer appears in the list of configs. This maps
correctly to heat's software deployment model and allows a future
change to properly implement replace_on_change[1] behaviour.

Containers created with an older docker-cmd (or create any other way)
will never be deleted by 50-heat-config-docker-cmd. This will result
in docker run failures when unique container names are re-used and
will require manual intervention to delete the old containers before
continuing.

[1] http://docs.openstack.org/developer/heat/template_guide/openstack.html#OS::Heat::SoftwareConfig-prop-inputs-*-replace_on_change

Change-Id: Iaf56c2b5fcb4969ce09480742f13a04d35bd2bae

heat-config-docker-cmd/os-refresh-config/configure.d/50-heat-config-docker-cmd

@@ -24,10 +24,6 @@ import yaml
 CONF_FILE = os.environ.get('HEAT_SHELL_CONFIG',
                            '/var/run/heat-config/heat-config')
 
-WORKING_DIR = os.environ.get(
-    'HEAT_DOCKER_CMD_WORKING',
-    '/var/lib/heat-config/heat-config-docker-cmd')
-
 DOCKER_CMD = os.environ.get('HEAT_DOCKER_CMD', 'docker')
 
 
@@ -48,9 +44,6 @@ def main(argv=sys.argv):
         log.warning('No config file %s' % CONF_FILE)
         return 1
 
-    if not os.path.isdir(WORKING_DIR):
-        os.makedirs(WORKING_DIR, 0o700)
-
     try:
         configs = json.load(open(CONF_FILE))
     except ValueError as e:
@@ -59,10 +52,7 @@ def main(argv=sys.argv):
 
     cmd_configs = list(build_configs(configs))
     try:
-        delete_missing_projects(cmd_configs)
+        delete_missing_configs(cmd_configs)
-        for c in cmd_configs:
-            delete_changed_project(c)
-            write_project(c)
     except Exception as e:
         log.exception(e)
 
@@ -77,68 +67,55 @@ def build_configs(configs):
         yield c
 
 
-def current_projects():
+def delete_missing_configs(configs):
-    for proj_file in os.listdir(WORKING_DIR):
+    config_ids = [c['id'] for c in configs]
-        if proj_file.endswith('.json'):
+    for conf_id in current_config_ids():
-            proj = proj_file[:-5]
+        if conf_id not in config_ids:
-            yield proj
+            log.debug('%s no longer exists, deleting containers' % conf_id)
+            remove_containers(conf_id)
 
 
-def remove_project(proj):
+def execute(cmd):
-    proj_file = os.path.join(WORKING_DIR, '%s.json' % proj)
-    with open(proj_file, 'r') as f:
-        proj_data = json.load(f)
-        for name in extract_container_names(proj, proj_data):
-            remove_container(name)
-    os.remove(proj_file)
-
-
-def remove_container(name):
-    cmd = [DOCKER_CMD, 'rm', '-f', name]
     log.debug(' '.join(cmd))
     subproc = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                stderr=subprocess.PIPE)
-    stdout, stderr = subproc.communicate()
+    cmd_stdout, cmd_stderr = subproc.communicate()
-    log.info(stdout)
+    log.debug(cmd_stdout)
-    log.debug(stderr)
+    log.debug(cmd_stderr)
+    return cmd_stdout, cmd_stderr, subproc.returncode
 
 
-def delete_missing_projects(configs):
+def current_config_ids():
-    config_names = [c['name'] for c in configs]
+    # List all config_id labels for containers managed by docker-cmd
-    for proj in current_projects():
+    cmd = [
-        if proj not in config_names:
+        DOCKER_CMD, 'ps', '-a',
-            log.debug('%s no longer exists, deleting containers' % proj)
+        '--filter', 'label=managed_by=docker-cmd',
-            remove_project(proj)
+        '--format', '{{.Label "config_id"}}'
+    ]
+    cmd_stdout, cmd_stderr, returncode = execute(cmd)
+    if returncode != 0:
+        return set()
+    return set(cmd_stdout.split())
 
 
-def extract_container_names(proj, proj_data):
+def remove_containers(conf_id):
-        # For now, assume a docker-compose v1 format where the
+    cmd = [
-        # root keys are service names
+        DOCKER_CMD, 'ps', '-q', '-a',
-        for name in sorted(proj_data):
+        '--filter', 'label=managed_by=docker-cmd',
-            yield '%s' % (name)
+        '--filter', 'label=config_id=%s' % conf_id
+    ]
+    cmd_stdout, cmd_stderr, returncode = execute(cmd)
+    if returncode == 0:
+        for container in cmd_stdout.split():
+            remove_container(container)
 
 
-def delete_changed_project(c):
+def remove_container(container):
-    proj = c['name']
+    cmd = [DOCKER_CMD, 'rm', '-f', container]
-    proj_file = os.path.join(WORKING_DIR, '%s.json' % proj)
+    cmd_stdout, cmd_stderr, returncode = execute(cmd)
-    proj_data = c.get('config', {})
+    if returncode != 0:
-    if os.path.isfile(proj_file):
+        log.error('Error removing container: %s' % container)
-        with open(proj_file, 'r') as f:
+        log.error(cmd_stderr)
-            prev_proj_data = json.load(f)
-            if proj_data != prev_proj_data:
-                log.debug('%s has changed, deleting containers' % proj)
-                remove_project(proj)
-
-
-def write_project(c):
-    proj = c['name']
-    proj_file = os.path.join(WORKING_DIR, '%s.json' % proj)
-    proj_data = c.get('config', {})
-
-    with os.fdopen(os.open(
-            proj_file, os.O_CREAT | os.O_WRONLY | os.O_TRUNC, 0o600),
-            'w') as f:
-        json.dump(proj_data, f, indent=2, separators=(',', ': '))
 
 
 if __name__ == '__main__':

tests/common.py

@@ -36,3 +36,22 @@ class RunScriptTest(testtools.TestCase):
     def json_from_file(self, path):
         with open(path) as f:
             return json.load(f)
+
+    def json_from_files(self, path, count, delete_after=True):
+        for i in range(count + 1):
+            if i == 0:
+                filename = path
+            else:
+                filename = '%s_%d' % (path, i)
+
+            # check there are not more files than the exact number requested
+            if i == count:
+                self.assertFalse(
+                    os.path.isfile(filename),
+                    'More than %d invocations' % count
+                )
+            else:
+                with open(filename) as f:
+                    yield json.load(f)
+                if delete_after:
+                    os.remove(filename)

tests/config-tool-fake.py

@@ -44,6 +44,10 @@ def main(argv=sys.argv):
         return
 
     response = json.loads(os.environ.get('TEST_RESPONSE'))
+    # if the response is a list, assume multiple invocations
+    if isinstance(response, list):
+        response = response[suffix]
+
     for k, v in response.get('files', {}).iteritems():
         open(k, 'w')
         with open(k, 'w') as f:

tests/test_hook_docker_cmd.py

@@ -17,7 +17,6 @@ import os
 import tempfile
 
 import fixtures
-from testtools import matchers
 
 from tests import common
 
@@ -102,7 +101,6 @@ class HookDockerCmdTest(common.RunScriptTest):
 
         self.env = os.environ.copy()
         self.env.update({
-            'HEAT_DOCKER_CMD_WORKING': self.working_dir.join(),
             'HEAT_DOCKER_CMD': self.fake_tool_path,
             'TEST_STATE_PATH': self.test_state_path,
         })
@@ -110,10 +108,16 @@ class HookDockerCmdTest(common.RunScriptTest):
     def test_hook(self):
 
         self.env.update({
-            'TEST_RESPONSE': json.dumps({
+            'TEST_RESPONSE': json.dumps([{
                 'stdout': '',
-                'stderr': 'Creating abcdef001_db_1...'
+                'stderr': 'Creating db...'
-            })
+            }, {
+                'stdout': '',
+                'stderr': 'Creating web...'
+            }, {
+                'stdout': '',
+                'stderr': 'one.txt\ntwo.txt\nthree.txt'
+            }])
         })
         returncode, stdout, stderr = self.run_cmd(
             [self.hook_path], self.env, json.dumps(self.data))
@@ -122,15 +126,13 @@ class HookDockerCmdTest(common.RunScriptTest):
 
         self.assertEqual({
             'deploy_stdout': '',
-            'deploy_stderr': 'Creating abcdef001_db_1...\n'
+            'deploy_stderr': 'Creating db...\n'
-                             'Creating abcdef001_db_1...\n'
+                             'Creating web...\n'
-                             'Creating abcdef001_db_1...',
+                             'one.txt\ntwo.txt\nthree.txt',
             'deploy_status_code': 0
         }, json.loads(stdout))
 
-        state_0 = self.json_from_file(self.test_state_path)
+        state = list(self.json_from_files(self.test_state_path, 3))
-        state_1 = self.json_from_file('%s_1' % self.test_state_path)
-        state_2 = self.json_from_file('%s_2' % self.test_state_path)
         self.assertEqual([
             self.fake_tool_path,
             'run',
@@ -149,7 +151,7 @@ class HookDockerCmdTest(common.RunScriptTest):
             '--detach=true',
             '--privileged=false',
             'xxx'
-        ], state_0['args'])
+        ], state[0]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'run',
@@ -175,14 +177,14 @@ class HookDockerCmdTest(common.RunScriptTest):
             '--volume=/run:/run',
             '--volume=db:/var/lib/db',
             'xxx'
-        ], state_1['args'])
+        ], state[1]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'exec',
             'web',
             '/bin/ls',
             '-l'
-        ], state_2['args'])
+        ], state[2]['args'])
 
     def test_hook_exit_codes(self):
 
@@ -202,37 +204,42 @@ class HookDockerCmdTest(common.RunScriptTest):
             'deploy_status_code': 0
         }, json.loads(stdout))
 
-        state_0 = self.json_from_file(self.test_state_path)
+        state = list(self.json_from_files(self.test_state_path, 1))
         self.assertEqual([
             self.fake_tool_path,
             'exec',
             'web',
             '/bin/ls',
             '-l'
-        ], state_0['args'])
+        ], state[0]['args'])
 
     def test_hook_failed(self):
 
         self.env.update({
-            'TEST_RESPONSE': json.dumps({
+            'TEST_RESPONSE': json.dumps([{
                 'stdout': '',
-                'stderr': 'Error: image library/xxx:latest not found',
+                'stderr': 'Creating db...'
-                'returncode': 1
+            }, {
-            })
+                'stdout': '',
+                'stderr': 'Creating web...'
+            }, {
+                'stdout': '',
+                'stderr': 'No such file or directory',
+                'returncode': 2
+            }])
         })
         returncode, stdout, stderr = self.run_cmd(
             [self.hook_path], self.env, json.dumps(self.data))
 
         self.assertEqual({
             'deploy_stdout': '',
-            'deploy_stderr': 'Error: image library/xxx:latest not found\n'
+            'deploy_stderr': 'Creating db...\n'
-                             'Error: image library/xxx:latest not found\n'
+                             'Creating web...\n'
-                             'Error: image library/xxx:latest not found',
+                             'No such file or directory',
-            'deploy_status_code': 1
+            'deploy_status_code': 2
         }, json.loads(stdout))
 
-        state_0 = self.json_from_file(self.test_state_path)
+        state = list(self.json_from_files(self.test_state_path, 3))
-        state_1 = self.json_from_file('%s_1' % self.test_state_path)
         self.assertEqual([
             self.fake_tool_path,
             'run',
@@ -251,7 +258,7 @@ class HookDockerCmdTest(common.RunScriptTest):
             '--detach=true',
             '--privileged=false',
             'xxx'
-        ], state_0['args'])
+        ], state[0]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'run',
@@ -277,9 +284,22 @@ class HookDockerCmdTest(common.RunScriptTest):
             '--volume=/run:/run',
             '--volume=db:/var/lib/db',
             'xxx'
-        ], state_1['args'])
+        ], state[1]['args'])
+        self.assertEqual([
+            self.fake_tool_path,
+            'exec',
+            'web',
+            '/bin/ls',
+            '-l'
+        ], state[2]['args'])
 
     def test_cleanup_deleted(self):
+        self.env.update({
+            'TEST_RESPONSE': json.dumps({
+                # first run, no running containers
+                'stdout': '\n'
+            })
+        })
         conf_dir = self.useFixture(fixtures.TempDir()).join()
         with tempfile.NamedTemporaryFile(dir=conf_dir, delete=False) as f:
             f.write(json.dumps([self.data]))
@@ -289,12 +309,33 @@ class HookDockerCmdTest(common.RunScriptTest):
             returncode, stdout, stderr = self.run_cmd(
                 [self.cleanup_path], self.env)
 
-        # on the first run, abcdef001.json is written out, no docker calls made
+        # on the first run, no docker rm calls made
-        configs_path = os.path.join(self.env['HEAT_DOCKER_CMD_WORKING'],
+        state = list(self.json_from_files(self.test_state_path, 1))
-                                    'abcdef001.json')
+        self.assertEqual([
-        self.assertThat(configs_path, matchers.FileExists())
+            self.fake_tool_path,
-        self.assertThat(self.test_state_path,
+            'ps',
-                        matchers.Not(matchers.FileExists()))
+            '-a',
+            '--filter',
+            'label=managed_by=docker-cmd',
+            '--format',
+            '{{.Label "config_id"}}'
+        ], state[0]['args'])
+
+        self.env.update({
+            'TEST_RESPONSE': json.dumps([{
+                # list config_id labels, 3 containers same config
+                'stdout': 'abc123\nabc123\nabc123\n'
+            }, {
+                # list containers with config_id
+                'stdout': '111\n222\n333\n'
+            }, {
+                'stdout': '111 deleted'
+            }, {
+                'stdout': '222 deleted'
+            }, {
+                'stdout': '333 deleted'
+            }])
+        })
 
         # run again with empty config data
         with tempfile.NamedTemporaryFile(dir=conf_dir, delete=False) as f:
@@ -305,28 +346,54 @@ class HookDockerCmdTest(common.RunScriptTest):
             returncode, stdout, stderr = self.run_cmd(
                 [self.cleanup_path], self.env)
 
-        # on the second run, abcdef001.json is deleted, docker rm is run on
+        # on the second run, abc123 is deleted,
-        # both containers
+        # docker rm is run on all containers
-        configs_path = os.path.join(self.env['HEAT_DOCKER_CMD_WORKING'],
+        state = list(self.json_from_files(self.test_state_path, 5))
-                                    'abcdef001.json')
+        self.assertEqual([
-        self.assertThat(configs_path,
+            self.fake_tool_path,
-                        matchers.Not(matchers.FileExists()))
+            'ps',
-        state_0 = self.json_from_file(self.test_state_path)
+            '-a',
-        state_1 = self.json_from_file('%s_1' % self.test_state_path)
+            '--filter',
+            'label=managed_by=docker-cmd',
+            '--format',
+            '{{.Label "config_id"}}'
+        ], state[0]['args'])
+        self.assertEqual([
+            self.fake_tool_path,
+            'ps',
+            '-q',
+            '-a',
+            '--filter',
+            'label=managed_by=docker-cmd',
+            '--filter',
+            'label=config_id=abc123'
+        ], state[1]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'rm',
             '-f',
-            'db',
+            '111',
-        ], state_0['args'])
+        ], state[2]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'rm',
             '-f',
-            'web',
+            '222',
-        ], state_1['args'])
+        ], state[3]['args'])
+        self.assertEqual([
+            self.fake_tool_path,
+            'rm',
+            '-f',
+            '333',
+        ], state[4]['args'])
 
     def test_cleanup_changed(self):
+        self.env.update({
+            'TEST_RESPONSE': json.dumps([{
+                # list config_id labels, 3 containers same config
+                'stdout': 'abc123\nabc123\nabc123\n'
+            }])
+        })
         conf_dir = self.useFixture(fixtures.TempDir()).join()
         with tempfile.NamedTemporaryFile(dir=conf_dir, delete=False) as f:
             f.write(json.dumps([self.data]))
@@ -336,16 +403,37 @@ class HookDockerCmdTest(common.RunScriptTest):
             returncode, stdout, stderr = self.run_cmd(
                 [self.cleanup_path], self.env)
 
-        # on the first run, abcdef001.json is written out, no docker calls made
+        # on the first run, no docker rm calls made
-        configs_path = os.path.join(self.env['HEAT_DOCKER_CMD_WORKING'],
+        state = list(self.json_from_files(self.test_state_path, 1))
-                                    'abcdef001.json')
+        self.assertEqual([
-        self.assertThat(configs_path, matchers.FileExists())
+            self.fake_tool_path,
-        self.assertThat(self.test_state_path,
+            'ps',
-                        matchers.Not(matchers.FileExists()))
+            '-a',
+            '--filter',
+            'label=managed_by=docker-cmd',
+            '--format',
+            '{{.Label "config_id"}}'
+        ], state[0]['args'])
 
         # run again with changed config data
+        self.env.update({
+            'TEST_RESPONSE': json.dumps([{
+                # list config_id labels, 3 containers same config
+                'stdout': 'abc123\nabc123\nabc123\n'
+            }, {
+                # list containers with config_id
+                'stdout': '111\n222\n333\n'
+            }, {
+                'stdout': '111 deleted'
+            }, {
+                'stdout': '222 deleted'
+            }, {
+                'stdout': '333 deleted'
+            }])
+        })
         new_data = copy.deepcopy(self.data)
         new_data['config']['web']['image'] = 'yyy'
+        new_data['id'] = 'def456'
         with tempfile.NamedTemporaryFile(dir=conf_dir, delete=False) as f:
             f.write(json.dumps([new_data]))
             f.flush()
@@ -354,22 +442,43 @@ class HookDockerCmdTest(common.RunScriptTest):
             returncode, stdout, stderr = self.run_cmd(
                 [self.cleanup_path], self.env)
 
-        # on the second run, abcdef001.json is written with the new data,
+        # on the second run, abc123 is deleted,
-        # docker rm is run on both containers
+        # docker rm is run on all containers
-        configs_path = os.path.join(self.env['HEAT_DOCKER_CMD_WORKING'],
+        state = list(self.json_from_files(self.test_state_path, 5))
-                                    'abcdef001.json')
+        self.assertEqual([
-        self.assertThat(configs_path, matchers.FileExists())
+            self.fake_tool_path,
-        state_0 = self.json_from_file(self.test_state_path)
+            'ps',
-        state_1 = self.json_from_file('%s_1' % self.test_state_path)
+            '-a',
+            '--filter',
+            'label=managed_by=docker-cmd',
+            '--format',
+            '{{.Label "config_id"}}'
+        ], state[0]['args'])
+        self.assertEqual([
+            self.fake_tool_path,
+            'ps',
+            '-q',
+            '-a',
+            '--filter',
+            'label=managed_by=docker-cmd',
+            '--filter',
+            'label=config_id=abc123'
+        ], state[1]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'rm',
             '-f',
-            'db',
+            '111',
-        ], state_0['args'])
+        ], state[2]['args'])
         self.assertEqual([
             self.fake_tool_path,
             'rm',
             '-f',
-            'web',
+            '222',
-        ], state_1['args'])
+        ], state[3]['args'])
+        self.assertEqual([
+            self.fake_tool_path,
+            'rm',
+            '-f',
+            '333',
+        ], state[4]['args'])