Fix RBD connector

RBD nos-brick connector has a bug where it only detaches the first
volume if we are running cinderlib in a container as root and the host
doesn't have the ceph-common package installed.

The library says it has detached it, but it really hasn't, so we end up
with a /dev/rbdX device leftover in our system.

This patch fixes this by taking into account that directories and
symlinks may already exist.

The connect_volume method will, on failure, clean up (unmap and remove
config file) and raise BrickException.  And the disconnect_volume will
ignore if an unlink fails because the link doesn't exist anymore.

Closes-Bug: #1819705
Change-Id: Ib61acdd4c469ac4316e2f5e518301c44dd3a6321
This commit is contained in:
Gorka Eguileor 2019-03-06 15:33:42 +01:00
parent f9dab3d6fb
commit b6bc89d61e
2 changed files with 417 additions and 15 deletions

View File

@ -26,20 +26,26 @@ Here we take care of:
Some of these changes may be later moved to OS-Brick. For now we just copied it
from the nos-brick repository.
"""
import errno
import functools
import os
import traceback
from os_brick import exception
from os_brick.initiator import connector
from os_brick.initiator import connectors
from os_brick.privileged import rootwrap
from oslo_concurrency import processutils as putils
from oslo_log import log as logging
from oslo_privsep import priv_context
from oslo_utils import fileutils
from oslo_utils import strutils
import six
LOG = logging.getLogger(__name__)
class RBDConnector(connectors.rbd.RBDConnector):
""""Connector class to attach/detach RBD volumes locally.
@ -86,8 +92,19 @@ class RBDConnector(connectors.rbd.RBDConnector):
# create the symlinks, ensure they exist
if self.containerized:
self._ensure_link(real_path, link_name)
except Exception:
fileutils.delete_if_exists(conf)
except Exception as exec_exception:
try:
try:
self._unmap(real_path, conf, connection_properties)
finally:
fileutils.delete_if_exists(conf)
except Exception:
exc = traceback.format_exc()
LOG.error('Exception occurred while cleaning up after '
'connection error\n%s', exc)
finally:
raise exception.BrickException('Error connecting volume: %s' %
six.text_type(exec_exception))
return {'path': real_path,
'conf': conf,
@ -96,10 +113,16 @@ class RBDConnector(connectors.rbd.RBDConnector):
def _ensure_link(self, source, link_name):
self._ensure_dir(os.path.dirname(link_name))
if self.im_root:
# If the link exists, remove it in case it's a leftover
if os.path.exists(link_name):
os.remove(link_name)
try:
os.symlink(source, link_name)
except Exception:
pass
except OSError as exc:
# Don't fail if symlink creation fails because it exists.
# It means that ceph-common has just created it.
if exc.errno != errno.EEXIST:
raise
else:
self._execute('ln', '-s', '-f', source, link_name,
run_as_root=True)
@ -122,6 +145,13 @@ class RBDConnector(connectors.rbd.RBDConnector):
return False
return True
def _unmap(self, real_dev_path, conf_file, connection_properties):
if os.path.exists(real_dev_path):
cmd = ['rbd', 'unmap', real_dev_path, '--conf', conf_file]
cmd += self._get_rbd_args(connection_properties)
self._execute(*cmd, root_helper=self._root_helper,
run_as_root=True)
def disconnect_volume(self, connection_properties, device_info,
force=False, ignore_errors=False):
self._setup_rbd_class()
@ -130,21 +160,21 @@ class RBDConnector(connectors.rbd.RBDConnector):
link_name = self.get_rbd_device_name(pool, volume)
real_dev_path = os.path.realpath(link_name)
if os.path.exists(real_dev_path):
cmd = ['rbd', 'unmap', real_dev_path, '--conf', conf_file]
cmd += self._get_rbd_args(connection_properties)
self._execute(*cmd, root_helper=self._root_helper,
run_as_root=True)
if self.containerized:
unlink_root(link_name)
self._unmap(real_dev_path, conf_file, connection_properties)
if self.containerized:
unlink_root(link_name)
fileutils.delete_if_exists(conf_file)
def _ensure_dir(self, path):
if self.im_root:
os.makedirs(path)
try:
os.makedirs(path, 0o755)
except OSError as exc:
# Don't fail if directory already exists, as our job is done.
if exc.errno != errno.EEXIST:
raise
else:
self._execute('mkdir', '-p', path, run_as_root=True)
self._execute('mkdir', '-p', '-m0755', path, run_as_root=True)
def _setup_class(self):
try:
@ -176,10 +206,17 @@ def unlink_root(*links, **kwargs):
if os.getuid() == 0:
for link in links:
with exc.context(catch_exception, error_msg, links):
os.unlink(link)
try:
os.unlink(link)
except OSError as exc:
# Ignore file doesn't exist errors
if exc.errno != errno.ENOENT:
raise
else:
with exc.context(catch_exception, error_msg, links):
# Ignore file doesn't exist errors
putils.execute('rm', *links, run_as_root=True,
check_exit_code=(0, errno.ENOENT),
root_helper=ROOT_HELPER)
if not no_errors and raise_at_end and exc:

View File

@ -0,0 +1,365 @@
# Copyright (c) 2019, Red Hat, Inc.
# All Rights Reserved.
#
# 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 errno
import ddt
import mock
from os_brick import exception
from cinderlib import nos_brick
from cinderlib.tests.unit import base
@ddt.ddt
class TestRBDConnector(base.BaseTest):
def setUp(self):
self.connector = nos_brick.RBDConnector('sudo')
self.connector.im_root = False
self.connector.containerized = False
self.connector._setup_rbd_class = lambda *args: None
@mock.patch.object(nos_brick.RBDConnector, '_get_rbd_args')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.path.exists', return_value=True)
def test__unmap_exists(self, exists_mock, exec_mock, args_mock):
args_mock.return_value = [mock.sentinel.args]
self.connector._unmap(mock.sentinel.path, mock.sentinel.conf,
mock.sentinel.conn_props)
exists_mock.assert_called_once_with(mock.sentinel.path)
exec_mock.assert_called_once_with(
'rbd', 'unmap', mock.sentinel.path, '--conf', mock.sentinel.conf,
mock.sentinel.args, root_helper='sudo', run_as_root=True)
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.path.exists', return_value=False)
def test__unmap_doesnt_exist(self, exists_mock, exec_mock):
self.connector._unmap(mock.sentinel.path, mock.sentinel.conf,
mock.sentinel.conn_props)
exists_mock.assert_called_once_with(mock.sentinel.path)
exec_mock.assert_not_called()
@ddt.data(True, False)
@mock.patch('oslo_utils.fileutils.delete_if_exists')
@mock.patch('cinderlib.nos_brick.unlink_root')
@mock.patch('os.path.realpath')
@mock.patch.object(nos_brick.RBDConnector, 'get_rbd_device_name')
@mock.patch.object(nos_brick.RBDConnector, '_unmap')
def test_disconnect_volume(self, is_containerized, unmap_mock,
dev_name_mock, path_mock, unlink_mock,
delete_mock):
self.connector.containerized = is_containerized
conn_props = {'name': 'pool/volume'}
dev_info = {'conf': mock.sentinel.conf_file}
self.connector.disconnect_volume(conn_props, dev_info)
dev_name_mock.assert_called_once_with('pool', 'volume')
path_mock.assert_called_once_with(dev_name_mock.return_value)
unmap_mock.assert_called_once_with(path_mock.return_value,
mock.sentinel.conf_file,
conn_props)
if is_containerized:
unlink_mock.assert_called_once_with(dev_name_mock.return_value)
else:
unlink_mock.assert_not_called()
delete_mock.assert_called_once_with(mock.sentinel.conf_file)
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.path.islink')
@mock.patch('os.path.exists')
@mock.patch('os.path.realpath')
@mock.patch.object(nos_brick.RBDConnector, 'get_rbd_device_name')
@mock.patch.object(nos_brick.RBDConnector, '_create_ceph_conf')
def test_connect_volume_exists(self, conf_mock, dev_name_mock, path_mock,
exists_mock, islink_mock, exec_mock):
conn_props = {'auth_username': mock.sentinel.username,
'name': 'pool/volume',
'cluster_name': mock.sentinel.cluster_name,
'hosts': mock.sentinel.hosts,
'ports': mock.sentinel.ports,
'keyring': mock.sentinel.keyring}
result = self.connector.connect_volume(conn_props)
conf_mock.assert_called_once_with(mock.sentinel.hosts,
mock.sentinel.ports,
'sentinel.cluster_name',
mock.sentinel.username,
mock.sentinel.keyring)
dev_name_mock.assert_called_once_with('pool', 'volume')
path_mock.assert_called_once_with(dev_name_mock.return_value)
islink_mock.assert_called_with(dev_name_mock.return_value)
exists_mock.assert_called_once_with(path_mock.return_value)
exec_mock.assert_not_called()
expected = {'path': path_mock.return_value,
'conf': conf_mock.return_value,
'type': 'block'}
self.assertEqual(expected, result)
@ddt.data(True, False)
@mock.patch.object(nos_brick.RBDConnector, '_ensure_link')
@mock.patch.object(nos_brick.RBDConnector, '_get_rbd_args')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.path.islink')
@mock.patch('os.path.exists', return_value=False)
@mock.patch('os.path.realpath')
@mock.patch.object(nos_brick.RBDConnector, 'get_rbd_device_name')
@mock.patch.object(nos_brick.RBDConnector, '_create_ceph_conf')
def test_connect_volume(self, is_containerized, conf_mock, dev_name_mock,
path_mock, exists_mock, islink_mock, exec_mock,
args_mock, link_mock):
exec_mock.return_value = (' a ', '')
args_mock.return_value = [mock.sentinel.args]
self.connector.containerized = is_containerized
conn_props = {'auth_username': mock.sentinel.username,
'name': 'pool/volume',
'cluster_name': mock.sentinel.cluster_name,
'hosts': mock.sentinel.hosts,
'ports': mock.sentinel.ports,
'keyring': mock.sentinel.keyring}
result = self.connector.connect_volume(conn_props)
conf_mock.assert_called_once_with(mock.sentinel.hosts,
mock.sentinel.ports,
'sentinel.cluster_name',
mock.sentinel.username,
mock.sentinel.keyring)
dev_name_mock.assert_called_once_with('pool', 'volume')
path_mock.assert_called_once_with(dev_name_mock.return_value)
islink_mock.assert_called_with(dev_name_mock.return_value)
exists_mock.assert_called_once_with(path_mock.return_value)
exec_mock.assert_called_once_with(
'rbd', 'map', 'volume', '--pool', 'pool', '--conf',
conf_mock.return_value, mock.sentinel.args,
root_helper='sudo', run_as_root=True)
if is_containerized:
link_mock.assert_called_once_with('a', dev_name_mock.return_value)
else:
link_mock.assert_not_called()
expected = {'path': 'a',
'conf': conf_mock.return_value,
'type': 'block'}
self.assertEqual(expected, result)
@mock.patch('oslo_utils.fileutils.delete_if_exists')
@mock.patch.object(nos_brick.RBDConnector, '_unmap')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_link')
@mock.patch.object(nos_brick.RBDConnector, '_get_rbd_args')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.path.islink')
@mock.patch('os.path.exists', return_value=False)
@mock.patch('os.path.realpath')
@mock.patch.object(nos_brick.RBDConnector, 'get_rbd_device_name')
@mock.patch.object(nos_brick.RBDConnector, '_create_ceph_conf')
def test_connect_volume_map_fail(self, conf_mock, dev_name_mock, path_mock,
exists_mock, islink_mock, exec_mock,
args_mock, link_mock, unmap_mock,
delete_mock):
exec_mock.side_effect = Exception
unmap_mock.side_effect = Exception
args_mock.return_value = [mock.sentinel.args]
conn_props = {'auth_username': mock.sentinel.username,
'name': 'pool/volume',
'cluster_name': mock.sentinel.cluster_name,
'hosts': mock.sentinel.hosts,
'ports': mock.sentinel.ports,
'keyring': mock.sentinel.keyring}
with self.assertRaises(exception.BrickException):
self.connector.connect_volume(conn_props)
link_mock.assert_not_called()
conf_mock.assert_called_once_with(mock.sentinel.hosts,
mock.sentinel.ports,
'sentinel.cluster_name',
mock.sentinel.username,
mock.sentinel.keyring)
dev_name_mock.assert_called_once_with('pool', 'volume')
path_mock.assert_called_once_with(dev_name_mock.return_value)
islink_mock.assert_called_with(dev_name_mock.return_value)
exists_mock.assert_called_once_with(path_mock.return_value)
exec_mock.assert_called_once_with(
'rbd', 'map', 'volume', '--pool', 'pool', '--conf',
conf_mock.return_value, mock.sentinel.args, root_helper='sudo',
run_as_root=True)
unmap_mock.assert_called_once_with(path_mock.return_value,
conf_mock.return_value,
conn_props)
delete_mock.assert_called_once_with(conf_mock.return_value)
@mock.patch('oslo_utils.fileutils.delete_if_exists')
@mock.patch.object(nos_brick.RBDConnector, '_unmap')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_link')
@mock.patch.object(nos_brick.RBDConnector, '_get_rbd_args')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.path.islink')
@mock.patch('os.path.exists', return_value=False)
@mock.patch('os.path.realpath')
@mock.patch.object(nos_brick.RBDConnector, 'get_rbd_device_name')
@mock.patch.object(nos_brick.RBDConnector, '_create_ceph_conf')
def test_connect_volume_link_fail(self, conf_mock, dev_name_mock,
path_mock, exists_mock, islink_mock,
exec_mock, args_mock, link_mock,
unmap_mock, delete_mock):
exec_mock.return_value = (' a ', '')
link_mock.side_effect = Exception
self.connector.containerized = True
args_mock.return_value = [mock.sentinel.args]
conn_props = {'auth_username': mock.sentinel.username,
'name': 'pool/volume',
'cluster_name': mock.sentinel.cluster_name,
'hosts': mock.sentinel.hosts,
'ports': mock.sentinel.ports,
'keyring': mock.sentinel.keyring}
with self.assertRaises(exception.BrickException):
self.connector.connect_volume(conn_props)
link_mock.assert_called_once_with('a', dev_name_mock.return_value)
conf_mock.assert_called_once_with(mock.sentinel.hosts,
mock.sentinel.ports,
'sentinel.cluster_name',
mock.sentinel.username,
mock.sentinel.keyring)
dev_name_mock.assert_called_once_with('pool', 'volume')
path_mock.assert_called_once_with(dev_name_mock.return_value)
islink_mock.assert_called_with(dev_name_mock.return_value)
exists_mock.assert_called_once_with(path_mock.return_value)
exec_mock.assert_called_once_with(
'rbd', 'map', 'volume', '--pool', 'pool', '--conf',
conf_mock.return_value, mock.sentinel.args, root_helper='sudo',
run_as_root=True)
unmap_mock.assert_called_once_with('a',
conf_mock.return_value,
conn_props)
delete_mock.assert_called_once_with(conf_mock.return_value)
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.makedirs')
def test__ensure_dir(self, mkdir_mock, exec_mock):
self.connector._ensure_dir(mock.sentinel.path)
exec_mock.assert_called_once_with('mkdir', '-p', '-m0755',
mock.sentinel.path, run_as_root=True)
mkdir_mock.assert_not_called()
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.makedirs')
def test__ensure_dir_root(self, mkdir_mock, exec_mock):
self.connector.im_root = True
self.connector._ensure_dir(mock.sentinel.path)
mkdir_mock.assert_called_once_with(mock.sentinel.path, 0o755)
exec_mock.assert_not_called()
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.makedirs', side_effect=OSError(errno.EEXIST, ''))
def test__ensure_dir_root_exists(self, mkdir_mock, exec_mock):
self.connector.im_root = True
self.connector._ensure_dir(mock.sentinel.path)
mkdir_mock.assert_called_once_with(mock.sentinel.path, 0o755)
exec_mock.assert_not_called()
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch('os.makedirs', side_effect=OSError(errno.EPERM, ''))
def test__ensure_dir_root_fails(self, mkdir_mock, exec_mock):
self.connector.im_root = True
with self.assertRaises(OSError) as exc:
self.connector._ensure_dir(mock.sentinel.path)
self.assertEqual(mkdir_mock.side_effect, exc.exception)
mkdir_mock.assert_called_once_with(mock.sentinel.path, 0o755)
exec_mock.assert_not_called()
@mock.patch('os.path.exists')
@mock.patch('os.remove')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_dir')
@mock.patch('os.symlink')
def test__ensure_link(self, link_mock, dir_mock, exec_mock, remove_mock,
exists_mock):
source = '/dev/rbd0'
link = '/dev/rbd/rbd/volume-xyz'
self.connector._ensure_link(source, link)
dir_mock.assert_called_once_with('/dev/rbd/rbd')
exec_mock.assert_called_once_with('ln', '-s', '-f', source, link,
run_as_root=True)
exists_mock.assert_not_called()
remove_mock.assert_not_called()
link_mock.assert_not_called()
@mock.patch('os.path.exists', return_value=False)
@mock.patch('os.remove')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_dir')
@mock.patch('os.symlink')
def test__ensure_link_root(self, link_mock, dir_mock, exec_mock,
remove_mock, exists_mock):
self.connector.im_root = True
source = '/dev/rbd0'
link = '/dev/rbd/rbd/volume-xyz'
self.connector._ensure_link(source, link)
dir_mock.assert_called_once_with('/dev/rbd/rbd')
exec_mock.assert_not_called()
remove_mock.assert_not_called()
exists_mock.assert_called_once_with(link)
link_mock.assert_called_once_with(source, link)
@mock.patch('os.path.exists', return_value=False)
@mock.patch('os.remove')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_dir')
@mock.patch('os.symlink', side_effect=OSError(errno.EEXIST, ''))
def test__ensure_link_root_appears(self, link_mock, dir_mock, exec_mock,
remove_mock, exists_mock):
self.connector.im_root = True
source = '/dev/rbd0'
link = '/dev/rbd/rbd/volume-xyz'
self.connector._ensure_link(source, link)
dir_mock.assert_called_once_with('/dev/rbd/rbd')
exec_mock.assert_not_called()
exists_mock.assert_called_once_with(link)
remove_mock.assert_not_called()
link_mock.assert_called_once_with(source, link)
@mock.patch('os.path.exists', return_value=False)
@mock.patch('os.remove')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_dir')
@mock.patch('os.symlink', side_effect=OSError(errno.EPERM, ''))
def test__ensure_link_root_fails(self, link_mock, dir_mock, exec_mock,
remove_mock, exists_mock):
self.connector.im_root = True
source = '/dev/rbd0'
link = '/dev/rbd/rbd/volume-xyz'
with self.assertRaises(OSError) as exc:
self.connector._ensure_link(source, link)
self.assertEqual(link_mock.side_effect, exc.exception)
dir_mock.assert_called_once_with('/dev/rbd/rbd')
exec_mock.assert_not_called()
exists_mock.assert_called_once_with(link)
remove_mock.assert_not_called()
link_mock.assert_called_once_with(source, link)
@mock.patch('os.path.exists')
@mock.patch('os.remove')
@mock.patch('os.path.realpath')
@mock.patch.object(nos_brick.RBDConnector, '_execute')
@mock.patch.object(nos_brick.RBDConnector, '_ensure_dir')
@mock.patch('os.symlink', side_effect=[OSError(errno.EEXIST, ''), None])
def test__ensure_link_root_replace(self, link_mock, dir_mock, exec_mock,
path_mock, remove_mock, exists_mock):
self.connector.im_root = True
source = '/dev/rbd0'
path_mock.return_value = '/dev/rbd1'
link = '/dev/rbd/rbd/volume-xyz'
self.connector._ensure_link(source, link)
dir_mock.assert_called_once_with('/dev/rbd/rbd')
exec_mock.assert_not_called()
exists_mock.assert_called_once_with(link)
remove_mock.assert_called_once_with(link)
link_mock.assert_called_once_with(source, link)