Improve nova statedir ownership logic

The nova_compute container kolla config is currently set to recursively change
the ownership of /var/lib/nova to nova:nova on startup.

This is necessary when upgrading from an non-container deployment to a
containerized deployment as the nova uids are not consistent across the host
and container image.

If the nova instancedir is an NFS mount then open filehandles are
broken and every VM using that NFS export fails with I/O errors.

This change re-implements the nova statedir ownership logic to target only the
files/directories controlled by nova.

Requires dist-git change: https://review.rdoproject.org/r/15018

Change-Id: I57d421feb6356d28002e77fb9bfa50a397758cbf
Closes-bug: 1778465
(cherry picked from commit 58624abf5f)
This commit is contained in:
Oliver Walsh 2018-06-25 16:28:17 +01:00 committed by Martin Schuppert
parent ddfb33ce06
commit aff9312637
11 changed files with 548 additions and 9 deletions

View File

@ -1,4 +1,4 @@
[DEFAULT] [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_id_option=--load-list $IDFILE
test_list_option=--list test_list_option=--list

View File

@ -0,0 +1,42 @@
heat_template_version: queens
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 }

View File

@ -88,6 +88,16 @@ resources:
MySQLClient: MySQLClient:
type: ../../puppet/services/database/mysql-client.yaml 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: NovaComputeBase:
type: ../../puppet/services/nova-compute.yaml type: ../../puppet/services/nova-compute.yaml
properties: properties:
@ -156,9 +166,6 @@ outputs:
- path: /var/log/nova - path: /var/log/nova
owner: nova:nova owner: nova:nova
recurse: true recurse: true
- path: /var/lib/nova
owner: nova:nova
recurse: true
- path: - path:
str_replace: str_replace:
template: /etc/ceph/CLUSTER.client.USER.keyring template: /etc/ceph/CLUSTER.client.USER.keyring
@ -167,10 +174,21 @@ outputs:
USER: {get_param: CephClientUserName} USER: {get_param: CephClientUserName}
owner: nova:nova owner: nova:nova
perm: '0600' perm: '0600'
docker_config_scripts: {get_attr: [NovaComputeCommon, docker_config_scripts]}
docker_config: 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: step_4:
nova_compute: nova_compute:
image: &nova_compute_image {get_param: DockerNovaComputeImage} image: *nova_compute_image
ulimit: {get_param: DockerNovaComputeUlimit} ulimit: {get_param: DockerNovaComputeUlimit}
ipc: host ipc: host
net: host net: host
@ -211,6 +229,7 @@ outputs:
state: directory state: directory
with_items: with_items:
- /var/lib/nova - /var/lib/nova
- /var/lib/nova/instances
- /var/lib/libvirt - /var/lib/libvirt
- name: ensure ceph configurations exist - name: ensure ceph configurations exist
file: file:
@ -261,6 +280,11 @@ outputs:
- step|int == 2 - step|int == 2
- nova_compute_enabled.rc == 0 - nova_compute_enabled.rc == 0
service: name=openstack-nova-compute state=stopped enabled=no service: name=openstack-nova-compute state=stopped enabled=no
- name: Set upgrade marker in nova statedir
when:
- step|int == 2
- 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 - name: Set fact for removal of openstack-nova-compute package
when: step|int == 2 when: step|int == 2
set_fact: set_fact:
@ -291,3 +315,9 @@ outputs:
- step|int == 1 - step|int == 1
- nova_compute_enabled|bool - nova_compute_enabled|bool
- release == 'ocata' - 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

View File

@ -45,6 +45,16 @@ resources:
MySQLClient: MySQLClient:
type: ../../puppet/services/database/mysql-client.yaml 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: NovaIronicBase:
type: ../../puppet/services/nova-ironic.yaml type: ../../puppet/services/nova-ironic.yaml
properties: properties:
@ -89,13 +99,21 @@ outputs:
- path: /var/log/nova - path: /var/log/nova
owner: nova:nova owner: nova:nova
recurse: true recurse: true
- path: /var/lib/nova docker_config_scripts: {get_attr: [NovaComputeCommon, docker_config_scripts]}
owner: nova:nova
recurse: true
docker_config: 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: step_5:
nova_compute: nova_compute:
image: {get_param: DockerNovaComputeIronicImage} image: *nova_ironic_image
net: host net: host
privileged: true privileged: true
user: root user: root
@ -147,6 +165,11 @@ outputs:
- step|int == 2 - step|int == 2
- nova_ironic_enabled.rc == 0 - nova_ironic_enabled.rc == 0
service: name=openstack-nova-compute state=stopped enabled=no service: name=openstack-nova-compute state=stopped enabled=no
- name: Set upgrade marker in nova statedir
when:
- step|int == 2
- nova_ironic_enabled|bool
file: path=/var/lib/nova/upgrade_marker state=touch owner=nova group=nova
fast_forward_upgrade_tasks: fast_forward_upgrade_tasks:
- name: Check if nova ironic is deployed - name: Check if nova ironic is deployed
command: systemctl is-enabled --quiet openstack-nova-compute command: systemctl is-enabled --quiet openstack-nova-compute
@ -167,3 +190,9 @@ outputs:
- step|int == 1 - step|int == 1
- release == 'ocata' - release == 'ocata'
- nova_ironic_enabled|bool - 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

View File

View File

@ -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()

View File

View File

@ -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')

View File

@ -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
<https://bugs.launchpad.net/tripleo/+bug/1778465>`__).

View File

@ -1,6 +1,7 @@
# The order of packages is significant, because pip processes them in the order # 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 # of appearance. Changing the order has an impact on the overall integration
# process, which may cause wedges in the gate later. # 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 openstackdocstheme>=1.18.1 # Apache-2.0
PyYAML>=3.10 # MIT PyYAML>=3.10 # MIT
Jinja2!=2.9.0,!=2.9.1,!=2.9.2,!=2.9.3,!=2.9.4,>=2.8 # BSD License (3 clause) Jinja2!=2.9.0,!=2.9.1,!=2.9.2,!=2.9.3,!=2.9.4,>=2.8 # BSD License (3 clause)

View File

@ -24,6 +24,11 @@ commands =
python ./tools/yaml-validate.py . python ./tools/yaml-validate.py .
bash -c ./tools/roles-data-validation.sh bash -c ./tools/roles-data-validation.sh
bash -c ./tools/check-up-to-date.sh bash -c ./tools/check-up-to-date.sh
flake8 ./docker_config_scripts/
[testenv:flake8]
commands =
flake8 ./docker_config_scripts/
[testenv:templates] [testenv:templates]
commands = python ./tools/process-templates.py commands = python ./tools/process-templates.py