From 6c3c8b41def1f37847fe1277a14634ccd4beb4f9 Mon Sep 17 00:00:00 2001 From: David Hill Date: Fri, 5 Jun 2020 14:12:31 -0400 Subject: [PATCH] Avoid failing on deleted file Avoid failing on deleted file as sometimes file might get deleted while the script run. Log the exception instead for troubleshooting purposes. Change-Id: I733cec2b34ef0bd0780ba5b0520127b911505e1b --- .../nova_statedir_ownership.py | 68 +++++++++------ .../tests/test_nova_statedir_ownership.py | 85 +++++++++++++++++-- 2 files changed, 120 insertions(+), 33 deletions(-) diff --git a/container_config_scripts/nova_statedir_ownership.py b/container_config_scripts/nova_statedir_ownership.py index 3c5719dbcd..aeb98b651c 100644 --- a/container_config_scripts/nova_statedir_ownership.py +++ b/container_config_scripts/nova_statedir_ownership.py @@ -36,14 +36,22 @@ class PathManager(object): """Helper class to manipulate ownership of a given path""" def __init__(self, path): self.path = path + self.uid = None + self.gid = None + self.is_dir = None + self.secontext = None self._update() def _update(self): - statinfo = os.stat(self.path) - self.is_dir = stat.S_ISDIR(statinfo.st_mode) - self.uid = statinfo.st_uid - self.gid = statinfo.st_gid - self.secontext = selinux.lgetfilecon(self.path)[1] + try: + statinfo = os.stat(self.path) + self.is_dir = stat.S_ISDIR(statinfo.st_mode) + self.uid = statinfo.st_uid + self.gid = statinfo.st_gid + self.secontext = selinux.lgetfilecon(self.path)[1] + except Exception: + LOG.exception('Could not update metadata for %s', self.path) + raise def __str__(self): return "uid: {} gid: {} path: {}{}".format( @@ -79,6 +87,7 @@ class PathManager(object): except Exception: LOG.exception('Could not change ownership of %s: ', self.path) + raise else: LOG.info('Ownership of %s already %d:%d', self.path, @@ -102,7 +111,7 @@ class PathManager(object): else: raise except Exception: - LOG.error('Could not set selinux context of %s to %s:', + LOG.exception('Could not set selinux context of %s to %s:', self.path, context) raise @@ -167,26 +176,33 @@ class NovaStatedirOwnershipManager(object): if pathname == self.upgrade_marker_path: continue - pathinfo = PathManager(pathname) - LOG.info("Checking %s", pathinfo) - if pathinfo.is_dir: - # Always chown the directories - pathinfo.chown(self.target_uid, self.target_gid) - chcon_r = chcon - if chcon: - chcon_r = pathinfo.chcon(self.target_secontext) - self._walk(pathname, chcon_r) - elif self.id_change: - # Only chown files if it's an upgrade and the file is owned by - # the host nova uid/gid - pathinfo.chown( - self.target_uid if pathinfo.uid == self.previous_uid - else pathinfo.uid, - self.target_gid if pathinfo.gid == self.previous_gid - else pathinfo.gid - ) - if chcon: - pathinfo.chcon(self.target_secontext) + try: + pathinfo = PathManager(pathname) + LOG.info("Checking %s", pathinfo) + if pathinfo.is_dir: + # Always chown the directories + pathinfo.chown(self.target_uid, self.target_gid) + chcon_r = chcon + if chcon: + chcon_r = pathinfo.chcon(self.target_secontext) + self._walk(pathname, chcon_r) + elif self.id_change: + # Only chown files if it's an upgrade and the file is owned by + # the host nova uid/gid + pathinfo.chown( + self.target_uid if pathinfo.uid == self.previous_uid + else pathinfo.uid, + self.target_gid if pathinfo.gid == self.previous_gid + else pathinfo.gid + ) + if chcon: + pathinfo.chcon(self.target_secontext) + except Exception: + # Likely to have been caused by external systems + # interacting with this directory tree, + # especially on NFS e.g snapshot dirs. + # Just ignore it and continue on to the next entry + continue def run(self): LOG.info('Applying nova statedir ownership') diff --git a/container_config_scripts/tests/test_nova_statedir_ownership.py b/container_config_scripts/tests/test_nova_statedir_ownership.py index c9bd171224..63e91199c6 100644 --- a/container_config_scripts/tests/test_nova_statedir_ownership.py +++ b/container_config_scripts/tests/test_nova_statedir_ownership.py @@ -16,6 +16,7 @@ from unittest import mock import contextlib +from os import stat as orig_stat import six import stat import sys @@ -75,6 +76,39 @@ def generate_testtree1(nova_uid, nova_gid): st_gid=nova_gid), 'nfs': False, }, + '/var/lib/nova/instances/removeddir': { + 'stat': FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=nova_uid, + st_gid=nova_gid), + 'nfs': False, + 'removed_when': 'listdir' + }, + '/var/lib/nova/instances/removedfile': { + 'stat': FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=0, + st_gid=nova_gid), + 'nfs': False, + 'removed_when': 'lgetfilecon' + }, + '/var/lib/nova/instances/removedfile2': { + 'stat': FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=0, + st_gid=nova_gid), + 'nfs': False, + 'removed_when': 'lsetfilecon' + }, + '/var/lib/nova/instances/removedfile3': { + 'nfs': False, + 'removed_when': 'stat' + }, + + '/var/lib/nova/instances/removeddir2': { + 'stat': FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=nova_uid, + st_gid=nova_gid), + 'nfs': False, + 'removed_when': 'lsetfilecon' + }, '/var/lib/nova/instances/foo': { 'stat': FakeStatInfo(st_mode=stat.S_IFDIR, st_uid=nova_uid, @@ -93,6 +127,20 @@ def generate_testtree1(nova_uid, nova_gid): st_gid=nova_gid), 'nfs': True, }, + '/var/lib/nova/instances/foo/removeddir': { + 'stat': FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=nova_uid, + st_gid=nova_gid), + 'nfs': True, + 'removed_when': 'listdir' + }, + '/var/lib/nova/instances/foo/removeddir2': { + 'stat': FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=0, + st_gid=nova_gid), + 'nfs': True, + 'removed_when': 'chown' + }, '/var/lib/nova/instances/foo/abc': { 'stat': FakeStatInfo(st_mode=stat.S_IFREG, st_uid=0, @@ -121,14 +169,25 @@ def generate_testtree2(marker_uid, marker_gid, *args, **kwargs): return tree +def check_removed(path, op, testtree): + if op == testtree.get(path, {}).get('removed_when', ''): + raise OSError(2, 'No such file or directory: ' + path) + + def generate_fake_stat(testtree): def fake_stat(path): - return testtree.get(path, {}).get('stat') + check_removed(path, 'stat', testtree) + if path.startswith('/var'): + return testtree.get(path, {}).get('stat') + else: + # Tracebacks need to use the real stat + return orig_stat(path) return fake_stat def generate_fake_chown(testtree): def fake_chown(path, uid, gid): + check_removed(path, 'chown', testtree) if uid != -1: testtree[path]['stat'].st_uid = uid if gid != -1: @@ -138,12 +197,14 @@ def generate_fake_chown(testtree): def generate_fake_exists(testtree): def fake_exists(path): + check_removed(path, 'exists', testtree) return path in testtree return fake_exists def generate_fake_listdir(testtree): def fake_listdir(path): + check_removed(path, 'listdir', testtree) path_parts = path.split('/') for entry in testtree: entry_parts = entry.split('/') @@ -155,16 +216,21 @@ def generate_fake_listdir(testtree): def generate_fake_unlink(testtree): def fake_unlink(path): + check_removed(path, 'unlink', testtree) del testtree[path] return fake_unlink +def generate_fake_lgetfilecon(testtree): + def fake_lgetfilecon(path): + check_removed(path, 'lgetfilecon', testtree) + + def generate_fake_lsetfilecon(testtree): def fake_lsetfilecon(path, context): + check_removed(path, 'lsetfilecon', testtree) if testtree[path]['nfs']: - e = OSError('Operation not supported') - e.errno = 95 - raise e + raise OSError(95, 'Operation not supported') @contextlib.contextmanager @@ -175,6 +241,7 @@ def fake_testtree(testtree): fake_listdir = generate_fake_listdir(testtree) fake_unlink = generate_fake_unlink(testtree) fake_lsetfilecon = generate_fake_lsetfilecon(testtree) + fake_lgetfilecon = generate_fake_lgetfilecon(testtree) with mock.patch('os.chown', side_effect=fake_chown) as fake_chown: with mock.patch('os.path.exists', @@ -191,7 +258,8 @@ def fake_testtree(testtree): ) as fake_unlink: with mock.patch( 'selinux.lgetfilecon', - return_value=[10, 'newcontext'], + side_effect=fake_lgetfilecon, + return_value=[10, 'newcontext'] ) as fake_lgetfilecon: with mock.patch( 'selinux.lsetfilecon', @@ -276,7 +344,7 @@ class NovaStatedirOwnershipManagerTestCase(base.BaseTestCase): with fake_testtree(testtree) as (fake_chown, _, _, _, _, _, fake_lsetfilecon): NovaStatedirOwnershipManager('/var/lib/nova').run() - fake_chown.assert_not_called() + fake_chown.assert_called_once_with('/var/lib/nova/instances/foo/removeddir2', 100, -1) fake_lsetfilecon.assert_any_call('/var/lib/nova', 'newcontext') fake_lsetfilecon.assert_any_call('/var/lib/nova/instances/foo', 'newcontext') chcon_paths = [x[0][0] for x in fake_lsetfilecon.call_args_list] @@ -290,7 +358,7 @@ class NovaStatedirOwnershipManagerTestCase(base.BaseTestCase): with fake_testtree(testtree) as (fake_chown, _, _, _, fake_unlink, _, _): NovaStatedirOwnershipManager('/var/lib/nova').run() - fake_chown.assert_not_called() + fake_chown.assert_called_once_with('/var/lib/nova/instances/foo/removeddir2', 100, -1) fake_unlink.assert_called_with('/var/lib/nova/upgrade_marker') def test_upgrade_marker_id_change(self): @@ -310,6 +378,9 @@ class NovaStatedirOwnershipManagerTestCase(base.BaseTestCase): if k == '/var/lib/_nova_secontext': # Ignore, outside tree continue + if testtree[k].get('removed_when', False): + # Ignore, deleted + continue v = v['stat'] if v.st_uid == other_uid or v.st_gid == other_gid: expected_changes[k] = (