diff --git a/.testr.conf b/.testr.conf index 5837838fb8..1899232d8c 100644 --- a/.testr.conf +++ b/.testr.conf @@ -1,4 +1,4 @@ [DEFAULT] -test_command=OS_STDOUT_CAPTURE=1 OS_STDERR_CAPTURE=1 OS_TEST_TIMEOUT=60 OS_LOG_CAPTURE=1 ${PYTHON:-python} -m subunit.run discover -t ./tripleo_heat_templates ./tripleo_heat_templates $LISTOPT $IDOPTION +test_command=OS_STDOUT_CAPTURE=1 OS_STDERR_CAPTURE=1 OS_TEST_TIMEOUT=60 OS_LOG_CAPTURE=1 ${PYTHON:-python} -m subunit.run discover -t ./ $LISTOPT $IDOPTION test_id_option=--load-list $IDFILE test_list_option=--list diff --git a/docker/services/nova-compute-common.yaml b/docker/services/nova-compute-common.yaml new file mode 100644 index 0000000000..65f95baf09 --- /dev/null +++ b/docker/services/nova-compute-common.yaml @@ -0,0 +1,42 @@ +heat_template_version: rocky + +description: > + Contains a static list of common things necessary for nova-compute containers + +parameters: + + # Required parameters + EndpointMap: + default: {} + description: Mapping of service endpoint -> protocol. Typically set + via parameter_defaults in the resource registry. + type: json + ServiceData: + default: {} + description: Dictionary packing service data + type: json + ServiceNetMap: + default: {} + description: Mapping of service_name -> network name. Typically set + via parameter_defaults in the resource registry. This + mapping overrides those in ServiceNetMapDefaults. + type: json + DefaultPasswords: + default: {} + type: json + RoleName: + default: '' + description: Role name on which the service is applied + type: string + RoleParameters: + default: {} + description: Parameters specific to the role + type: json + +outputs: + docker_config_scripts: + description: Shared docker config scripts + value: + nova_statedir_ownership.py: + mode: "0700" + content: { get_file: ../../docker_config_scripts/nova_statedir_ownership.py } diff --git a/docker/services/nova-compute.yaml b/docker/services/nova-compute.yaml index ea5bf589da..035f31f841 100644 --- a/docker/services/nova-compute.yaml +++ b/docker/services/nova-compute.yaml @@ -93,6 +93,16 @@ resources: MySQLClient: type: ../../puppet/services/database/mysql-client.yaml + NovaComputeCommon: + type: ./nova-compute-common.yaml + properties: + EndpointMap: {get_param: EndpointMap} + ServiceData: {get_param: ServiceData} + ServiceNetMap: {get_param: ServiceNetMap} + DefaultPasswords: {get_param: DefaultPasswords} + RoleName: {get_param: RoleName} + RoleParameters: {get_param: RoleParameters} + NovaComputeBase: type: ../../puppet/services/nova-compute.yaml properties: @@ -168,9 +178,6 @@ outputs: - path: /var/log/nova owner: nova:nova recurse: true - - path: /var/lib/nova - owner: nova:nova - recurse: true - path: str_replace: template: /etc/ceph/CLUSTER.client.USER.keyring @@ -179,10 +186,21 @@ outputs: USER: {get_param: CephClientUserName} owner: nova:nova perm: '0600' + docker_config_scripts: {get_attr: [NovaComputeCommon, docker_config_scripts]} docker_config: + step_3: + nova_statedir_owner: + image: &nova_compute_image {get_param: DockerNovaComputeImage} + user: root + privileged: false + detach: false + volumes: + - /var/lib/nova:/var/lib/nova:shared + - /var/lib/docker-config-scripts/:/docker-config-scripts/ + command: "/docker-config-scripts/nova_statedir_ownership.py" step_4: nova_compute: - image: &nova_compute_image {get_param: DockerNovaComputeImage} + image: *nova_compute_image ulimit: {get_param: DockerNovaComputeUlimit} ipc: host net: host @@ -223,6 +241,7 @@ outputs: state: directory with_items: - /var/lib/nova + - /var/lib/nova/instances - /var/lib/libvirt - name: ensure ceph configurations exist file: @@ -277,6 +296,9 @@ outputs: - name: Stop and disable nova-compute service when: nova_compute_enabled|bool service: name=openstack-nova-compute state=stopped enabled=no + - name: Set upgrade marker in nova statedir + when: nova_compute_enabled|bool + file: path=/var/lib/nova/upgrade_marker state=touch owner=nova group=nova - name: Set fact for removal of openstack-nova-compute package set_fact: remove_nova_compute_package: {get_param: UpgradeRemoveUnusedPackages} @@ -304,3 +326,9 @@ outputs: - step|int == 1 - nova_compute_enabled|bool - release == 'ocata' + - name: Set upgrade marker in nova statedir + when: + - step|int == 1 + - nova_compute_enabled|bool + - release == 'ocata' + file: path=/var/lib/nova/upgrade_marker state=touch owner=nova group=nova diff --git a/docker/services/nova-ironic.yaml b/docker/services/nova-ironic.yaml index 7af1c78155..9949433caa 100644 --- a/docker/services/nova-ironic.yaml +++ b/docker/services/nova-ironic.yaml @@ -49,6 +49,16 @@ resources: MySQLClient: type: ../../puppet/services/database/mysql-client.yaml + NovaComputeCommon: + type: ./nova-compute-common.yaml + properties: + EndpointMap: {get_param: EndpointMap} + ServiceData: {get_param: ServiceData} + ServiceNetMap: {get_param: ServiceNetMap} + DefaultPasswords: {get_param: DefaultPasswords} + RoleName: {get_param: RoleName} + RoleParameters: {get_param: RoleParameters} + NovaIronicBase: type: ../../puppet/services/nova-ironic.yaml properties: @@ -93,13 +103,21 @@ outputs: - path: /var/log/nova owner: nova:nova recurse: true - - path: /var/lib/nova - owner: nova:nova - recurse: true + docker_config_scripts: {get_attr: [NovaComputeCommon, docker_config_scripts]} docker_config: + step_3: + nova_statedir_owner: + image: &nova_ironic_image {get_param: DockerNovaComputeIronicImage} + user: root + privileged: false + detach: false + volumes: + - /var/lib/nova:/var/lib/nova:shared + - /var/lib/docker-config-scripts/:/docker-config-scripts/ + command: "/docker-config-scripts/nova_statedir_ownership.py" step_5: nova_compute: - image: {get_param: DockerNovaComputeIronicImage} + image: *nova_ironic_image net: host privileged: true user: root @@ -154,6 +172,9 @@ outputs: - name: Stop and disable nova-compute service when: nova_ironic_enabled|bool service: name=openstack-nova-compute state=stopped enabled=no + - name: Set upgrade marker in nova statedir + when: nova_ironic_enabled|bool + file: path=/var/lib/nova/upgrade_marker state=touch owner=nova group=nova - when: step|int == 3 block: - name: Set fact for removal of openstack-nova-compute package @@ -183,3 +204,9 @@ outputs: - step|int == 1 - release == 'ocata' - nova_ironic_enabled|bool + - name: Set upgrade marker in nova statedir + when: + - step|int == 1 + - release == 'ocata' + - nova_ironic_enabled|bool + file: path=/var/lib/nova/upgrade_marker state=touch owner=nova group=nova diff --git a/docker_config_scripts/__init__.py b/docker_config_scripts/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/docker_config_scripts/nova_statedir_ownership.py b/docker_config_scripts/nova_statedir_ownership.py new file mode 100755 index 0000000000..9e5afb8b95 --- /dev/null +++ b/docker_config_scripts/nova_statedir_ownership.py @@ -0,0 +1,165 @@ +#!/usr/bin/env python +# +# Copyright 2018 Red Hat Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. +from __future__ import print_function +import logging +import os +import pwd +import stat +import sys + +logging.basicConfig(stream=sys.stdout, level=logging.DEBUG) +LOG = logging.getLogger('nova_statedir') + + +class PathManager(object): + """Helper class to manipulate ownership of a given path""" + def __init__(self, path): + self.path = path + 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 + + def __str__(self): + return "uid: {} gid: {} path: {}{}".format( + self.uid, + self.gid, + self.path, + '/' if self.is_dir else '' + ) + + def has_owner(self, uid, gid): + return self.uid == uid and self.gid == gid + + def has_either(self, uid, gid): + return self.uid == uid or self.gid == gid + + def chown(self, uid, gid): + target_uid = -1 + target_gid = -1 + if self.uid != uid: + target_uid = uid + if self.gid != gid: + target_gid = gid + if (target_uid, target_gid) != (-1, -1): + LOG.info('Changing ownership of %s from %d:%d to %d:%d', + self.path, + self.uid, + self.gid, + self.uid if target_uid == -1 else target_uid, + self.gid if target_gid == -1 else target_gid) + os.chown(self.path, target_uid, target_gid) + self._update() + else: + LOG.info('Ownership of %s already %d:%d', + self.path, + uid, + gid) + + +class NovaStatedirOwnershipManager(object): + """Class to manipulate the ownership of the nova statedir (/var/lib/nova). + + The nova uid/gid differ on the host and container images. An upgrade + that switches from host systemd services to docker requires a change in + ownership. Previously this was a naive recursive chown, however this + causes issues if nova instance are shared via an NFS mount: any open + filehandles in qemu/libvirt fail with an I/O error (LP1778465). + + Instead the upgrade/FFU ansible tasks now lay down a marker file when + stopping and disabling the host systemd services. We use this file to + determine the host nova uid/gid. We then walk the tree and update any + files that have the host uid/gid to the docker nova uid/gid. As files + owned by root/qemu etc... are ignored this avoids the issues with open + filehandles. The marker is removed once the tree has been walked. + + For subsequent runs, or for a new deployment, we simply ensure that the + docker nova user/group owns all directories. This is required as the + directories are created with root ownership in host_prep_tasks (the + docker nova uid/gid is not known in this context). + """ + def __init__(self, statedir, upgrade_marker='upgrade_marker', + nova_user='nova'): + self.statedir = statedir + self.nova_user = nova_user + + self.upgrade_marker_path = os.path.join(statedir, upgrade_marker) + self.upgrade = os.path.exists(self.upgrade_marker_path) + + self.target_uid, self.target_gid = self._get_nova_ids() + self.previous_uid, self.previous_gid = self._get_previous_nova_ids() + self.id_change = (self.target_uid, self.target_gid) != \ + (self.previous_uid, self.previous_gid) + + def _get_nova_ids(self): + nova_uid, nova_gid = pwd.getpwnam(self.nova_user)[2:4] + return nova_uid, nova_gid + + def _get_previous_nova_ids(self): + if self.upgrade: + statinfo = os.stat(self.upgrade_marker_path) + return statinfo.st_uid, statinfo.st_gid + else: + return self._get_nova_ids() + + def _walk(self, top): + for f in os.listdir(top): + pathname = os.path.join(top, f) + + 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) + self._walk(pathname) + 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 + ) + + def run(self): + LOG.info('Applying nova statedir ownership') + LOG.info('Target ownership for %s: %d:%d', + self.statedir, + self.target_uid, + self.target_gid) + + pathinfo = PathManager(self.statedir) + LOG.info("Checking %s", pathinfo) + pathinfo.chown(self.target_uid, self.target_gid) + + self._walk(self.statedir) + + if self.upgrade: + LOG.info('Removing upgrade_marker %s', + self.upgrade_marker_path) + os.unlink(self.upgrade_marker_path) + + LOG.info('Nova statedir ownership complete') + +if __name__ == '__main__': + NovaStatedirOwnershipManager('/var/lib/nova').run() diff --git a/docker_config_scripts/tests/__init__.py b/docker_config_scripts/tests/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/docker_config_scripts/tests/test_nova_statedir_ownership.py b/docker_config_scripts/tests/test_nova_statedir_ownership.py new file mode 100644 index 0000000000..66d4767869 --- /dev/null +++ b/docker_config_scripts/tests/test_nova_statedir_ownership.py @@ -0,0 +1,260 @@ +# +# Copyright 2018 Red Hat Inc. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +import contextlib +import mock +import six +import stat + +from oslotest import base + +from docker_config_scripts.nova_statedir_ownership import \ + NovaStatedirOwnershipManager +from docker_config_scripts.nova_statedir_ownership import PathManager + +# Real chown would require root, so in order to test this we need to fake +# all of the methods that interact with the filesystem + +current_uid = 100 +current_gid = 100 + + +class FakeStatInfo(object): + def __init__(self, st_mode, st_uid, st_gid): + self.st_mode = st_mode + self.st_uid = st_uid + self.st_gid = st_gid + + def get_ids(self): + return (self.st_uid, self.st_gid) + + +def generate_testtree1(nova_uid, nova_gid): + return { + '/var/lib/nova': + FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=nova_uid, + st_gid=nova_gid), + '/var/lib/nova/instances': + FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=nova_uid, + st_gid=nova_gid), + '/var/lib/nova/instances/foo': + FakeStatInfo(st_mode=stat.S_IFDIR, + st_uid=nova_uid, + st_gid=nova_gid), + '/var/lib/nova/instances/foo/bar': + FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=0, + st_gid=0), + '/var/lib/nova/instances/foo/baz': + FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=nova_uid, + st_gid=nova_gid), + '/var/lib/nova/instances/foo/abc': + FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=0, + st_gid=nova_gid), + '/var/lib/nova/instances/foo/def': + FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=nova_uid, + st_gid=0), + } + + +def generate_testtree2(marker_uid, marker_gid, *args, **kwargs): + tree = generate_testtree1(*args, **kwargs) + tree.update({ + '/var/lib/nova/upgrade_marker': + FakeStatInfo(st_mode=stat.S_IFREG, + st_uid=marker_uid, + st_gid=marker_gid) + }) + return tree + + +def generate_fake_stat(testtree): + def fake_stat(path): + return testtree.get(path) + return fake_stat + + +def generate_fake_chown(testtree): + def fake_chown(path, uid, gid): + if uid != -1: + testtree[path].st_uid = uid + if gid != -1: + testtree[path].st_gid = gid + return fake_chown + + +def generate_fake_exists(testtree): + def fake_exists(path): + return path in testtree + return fake_exists + + +def generate_fake_listdir(testtree): + def fake_listdir(path): + path_parts = path.split('/') + for entry in testtree: + entry_parts = entry.split('/') + if (entry_parts[:len(path_parts)] == path_parts and + len(entry_parts) == len(path_parts) + 1): + yield entry + return fake_listdir + + +def generate_fake_unlink(testtree): + def fake_unlink(path): + del testtree[path] + return fake_unlink + + +@contextlib.contextmanager +def fake_testtree(testtree): + fake_stat = generate_fake_stat(testtree) + fake_chown = generate_fake_chown(testtree) + fake_exists = generate_fake_exists(testtree) + fake_listdir = generate_fake_listdir(testtree) + fake_unlink = generate_fake_unlink(testtree) + with mock.patch('os.chown', + side_effect=fake_chown) as fake_chown: + with mock.patch('os.path.exists', + side_effect=fake_exists) as fake_exists: + with mock.patch('os.listdir', + side_effect=fake_listdir) as fake_listdir: + with mock.patch('pwd.getpwnam', + return_value=(0, 0, current_uid, current_gid)): + with mock.patch('os.stat', + side_effect=fake_stat) as fake_stat: + with mock.patch( + 'os.unlink', + side_effect=fake_unlink + ) as fake_unlink: + yield (fake_chown, + fake_exists, + fake_listdir, + fake_stat, + fake_unlink) + + +def assert_ids(testtree, path, uid, gid): + statinfo = testtree[path] + assert (uid, gid) == (statinfo.st_uid, statinfo.st_gid), \ + "{}: expected {}:{} actual {}:{}".format( + path, uid, gid, statinfo.st_uid, statinfo.st_gid + ) + + +class PathManagerCase(base.BaseTestCase): + def test_file(self): + testtree = generate_testtree1(current_uid, current_gid) + + with fake_testtree(testtree): + pathinfo = PathManager('/var/lib/nova/instances/foo/baz') + self.assertTrue(pathinfo.has_owner(current_uid, current_gid)) + self.assertTrue(pathinfo.has_either(current_uid, 0)) + self.assertTrue(pathinfo.has_either(0, current_gid)) + self.assertFalse(pathinfo.is_dir) + self.assertEqual(str(pathinfo), 'uid: {} gid: {} path: {}'.format( + current_uid, current_gid, '/var/lib/nova/instances/foo/baz' + )) + + def test_dir(self): + testtree = generate_testtree1(current_uid, current_gid) + + with fake_testtree(testtree): + pathinfo = PathManager('/var/lib/nova') + self.assertTrue(pathinfo.has_owner(current_uid, current_gid)) + self.assertTrue(pathinfo.has_either(current_uid, 0)) + self.assertTrue(pathinfo.has_either(0, current_gid)) + self.assertTrue(pathinfo.is_dir) + self.assertEqual(str(pathinfo), 'uid: {} gid: {} path: {}'.format( + current_uid, current_gid, '/var/lib/nova/' + )) + + def test_chown(self): + testtree = generate_testtree1(current_uid, current_gid) + + with fake_testtree(testtree): + pathinfo = PathManager('/var/lib/nova/instances/foo/baz') + self.assertTrue(pathinfo.has_owner(current_uid, current_gid)) + pathinfo.chown(current_uid+1, current_gid) + assert_ids(testtree, pathinfo.path, current_uid+1, current_gid) + + def test_chgrp(self): + testtree = generate_testtree1(current_uid, current_gid) + + with fake_testtree(testtree): + pathinfo = PathManager('/var/lib/nova/instances/foo/baz') + self.assertTrue(pathinfo.has_owner(current_uid, current_gid)) + pathinfo.chown(current_uid, current_gid+1) + assert_ids(testtree, pathinfo.path, current_uid, current_gid+1) + + def test_chown_chgrp(self): + testtree = generate_testtree1(current_uid, current_gid) + + with fake_testtree(testtree): + pathinfo = PathManager('/var/lib/nova/instances/foo/baz') + self.assertTrue(pathinfo.has_owner(current_uid, current_gid)) + pathinfo.chown(current_uid+1, current_gid+1) + assert_ids(testtree, pathinfo.path, current_uid+1, current_gid+1) + + +class NovaStatedirOwnershipManagerTestCase(base.BaseTestCase): + def test_no_upgrade_marker(self): + testtree = generate_testtree1(current_uid, current_gid) + + with fake_testtree(testtree) as (fake_chown, _, _, _, _): + NovaStatedirOwnershipManager('/var/lib/nova').run() + fake_chown.assert_not_called() + + def test_upgrade_marker_no_id_change(self): + testtree = generate_testtree2(current_uid, + current_gid, + current_uid, + current_gid) + + with fake_testtree(testtree) as (fake_chown, _, _, _, fake_unlink): + NovaStatedirOwnershipManager('/var/lib/nova').run() + fake_chown.assert_not_called() + fake_unlink.assert_called_with('/var/lib/nova/upgrade_marker') + + def test_upgrade_marker_id_change(self): + other_uid = current_uid + 1 + other_gid = current_gid + 1 + testtree = generate_testtree2(other_uid, + other_gid, + other_uid, + other_gid) + + # Determine which paths should change uid/gid + expected_changes = {} + for k, v in six.iteritems(testtree): + if k == '/var/lib/nova/upgrade_marker': + # Ignore the marker, it should be deleted + continue + if v.st_uid == other_uid or v.st_gid == other_gid: + expected_changes[k] = ( + current_uid if v.st_uid == other_uid else v.st_uid, + current_gid if v.st_gid == other_gid else v.st_gid + ) + + with fake_testtree(testtree) as (_, _, _, _, fake_unlink): + NovaStatedirOwnershipManager('/var/lib/nova').run() + for fn, expected in six.iteritems(expected_changes): + assert_ids(testtree, fn, expected[0], expected[1]) + fake_unlink.assert_called_with('/var/lib/nova/upgrade_marker') diff --git a/releasenotes/notes/nova_statedir_ownership-54c75dfe8ad64b4f.yaml b/releasenotes/notes/nova_statedir_ownership-54c75dfe8ad64b4f.yaml new file mode 100644 index 0000000000..aea1b0f12b --- /dev/null +++ b/releasenotes/notes/nova_statedir_ownership-54c75dfe8ad64b4f.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + The nova statedir ownership logic has been reimplemented to target only the + files/directories controlled by nova. + Resolves VM I/O errors when using an NFS backend (`bug 1778465 + `__). diff --git a/test-requirements.txt b/test-requirements.txt index 74f393cb5b..7235018a40 100644 --- a/test-requirements.txt +++ b/test-requirements.txt @@ -1,6 +1,7 @@ # The order of packages is significant, because pip processes them in the order # of appearance. Changing the order has an impact on the overall integration # process, which may cause wedges in the gate later. +hacking>=0.11.0,<0.12 # Apache-2.0 openstackdocstheme>=1.18.1 # Apache-2.0 PyYAML>=3.12 # MIT Jinja2>=2.10 # BSD License (3 clause) diff --git a/tox.ini b/tox.ini index 6a192fd062..c6c00050af 100644 --- a/tox.ini +++ b/tox.ini @@ -24,6 +24,11 @@ commands = python ./tools/yaml-validate.py . bash -c ./tools/roles-data-validation.sh bash -c ./tools/check-up-to-date.sh + flake8 ./docker_config_scripts/ + +[testenv:flake8] +commands = + flake8 ./docker_config_scripts/ [testenv:templates] commands = python ./tools/process-templates.py