From 45026cb5c1b0d40324a6183006b4a397c2d9342c Mon Sep 17 00:00:00 2001 From: Eric Fried Date: Mon, 1 Jul 2019 17:09:15 -0500 Subject: [PATCH] Get rid of args to RBDDriver.__init__() While reviewing [1] I noticed an annoying and unnecessary coupling/duplication of references to the CONF variables for RBD. This commit consolidates them down to a single source, the guts of RBDDriver.__init__(). As part of this a workaround is also removed concerning the passing of conffile=None to RADOS via python-rbd. The following change merged in 2014 so we can now return None when ``[libvirt]/images_rbd_ceph_conf`` is not set: Make python rados.Rados accept conffile=None https://github.com/ceph/ceph/pull/1787 [1] https://review.opendev.org/667421 Co-Authored-By: Lee Yarwood Change-Id: I361af845d6a733618ecd056aa7df973191184ae9 --- .../unit/virt/libvirt/storage/test_rbd.py | 11 ++++-- nova/tests/unit/virt/libvirt/test_driver.py | 2 +- .../unit/virt/libvirt/test_imagebackend.py | 12 +++--- nova/virt/libvirt/driver.py | 14 ++----- nova/virt/libvirt/imagebackend.py | 37 +++++++------------ nova/virt/libvirt/storage/rbd_utils.py | 17 +++++---- 6 files changed, 41 insertions(+), 52 deletions(-) diff --git a/nova/tests/unit/virt/libvirt/storage/test_rbd.py b/nova/tests/unit/virt/libvirt/storage/test_rbd.py index cd1c33bbaad6..457e65fa8705 100644 --- a/nova/tests/unit/virt/libvirt/storage/test_rbd.py +++ b/nova/tests/unit/virt/libvirt/storage/test_rbd.py @@ -60,6 +60,12 @@ class RbdTestCase(test.NoDBTestCase): def setUp(self): super(RbdTestCase, self).setUp() + self.rbd_pool = 'rbd' + self.rbd_connect_timeout = 5 + self.flags(images_rbd_pool=self.rbd_pool, group='libvirt') + self.flags(rbd_connect_timeout=self.rbd_connect_timeout, + group='libvirt') + rados_patcher = mock.patch.object(rbd_utils, 'rados') self.mock_rados = rados_patcher.start() self.addCleanup(rados_patcher.stop) @@ -82,10 +88,7 @@ class RbdTestCase(test.NoDBTestCase): self.mock_rbd.ImageBusy = FakeException self.mock_rbd.ImageHasSnapshots = FakeException - self.rbd_pool = 'rbd' - self.rbd_connect_timeout = 5 - self.driver = rbd_utils.RBDDriver(self.rbd_pool, None, None, - self.rbd_connect_timeout) + self.driver = rbd_utils.RBDDriver() self.volume_name = u'volume-00000001' self.snap_name = u'test-snap' diff --git a/nova/tests/unit/virt/libvirt/test_driver.py b/nova/tests/unit/virt/libvirt/test_driver.py index 0619c6012d07..22d850d518c4 100644 --- a/nova/tests/unit/virt/libvirt/test_driver.py +++ b/nova/tests/unit/virt/libvirt/test_driver.py @@ -20015,9 +20015,9 @@ class LibvirtDriverTestCase(test.NoDBTestCase, TraitsComparisonMixin): params={'vm_state': vm_states.SHELVED_OFFLOADED}) drvr = libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False) mock_rbd_driver = mock.Mock(spec=rbd_utils.RBDDriver) + mock_rbd_driver.pool = mock.sentinel.rbd_pool mock_rbd_imagebackend = mock.Mock(spec=imagebackend.Rbd) mock_rbd_imagebackend.rbd_name = mock.sentinel.rbd_name - mock_rbd_imagebackend.pool = mock.sentinel.rbd_pool # This is logged so we can't use a sentinel mock_rbd_imagebackend.path = 'rbd:pool/vol_disk' mock_rbd_imagebackend.driver = mock_rbd_driver diff --git a/nova/tests/unit/virt/libvirt/test_imagebackend.py b/nova/tests/unit/virt/libvirt/test_imagebackend.py index 5cf869099a19..c7f4c5208ef7 100644 --- a/nova/tests/unit/virt/libvirt/test_imagebackend.py +++ b/nova/tests/unit/virt/libvirt/test_imagebackend.py @@ -1683,7 +1683,7 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): def test_direct_snapshot_cleans_up_on_failures(self): image = self.image_class(self.INSTANCE, self.NAME) - test_snap = 'rbd://%s/%s/%s/snap' % (self.FSID, image.pool, + test_snap = 'rbd://%s/%s/%s/snap' % (self.FSID, image.driver.pool, image.rbd_name) with test.nested( mock.patch.object(rbd_utils.RBDDriver, 'get_fsid', @@ -1707,7 +1707,7 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): def test_cleanup_direct_snapshot(self): image = self.image_class(self.INSTANCE, self.NAME) - test_snap = 'rbd://%s/%s/%s/snap' % (self.FSID, image.pool, + test_snap = 'rbd://%s/%s/%s/snap' % (self.FSID, image.driver.pool, image.rbd_name) with test.nested( mock.patch.object(rbd_utils.RBDDriver, 'remove_snap'), @@ -1721,12 +1721,12 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): image.cleanup_direct_snapshot(dict(url=test_snap)) mock_rm.assert_called_once_with(image.rbd_name, 'snap', force=True, ignore_errors=False, - pool=image.pool) + pool=image.driver.pool) self.assertFalse(mock_destroy.called) def test_cleanup_direct_snapshot_destroy_volume(self): image = self.image_class(self.INSTANCE, self.NAME) - test_snap = 'rbd://%s/%s/%s/snap' % (self.FSID, image.pool, + test_snap = 'rbd://%s/%s/%s/snap' % (self.FSID, image.driver.pool, image.rbd_name) with test.nested( mock.patch.object(rbd_utils.RBDDriver, 'remove_snap'), @@ -1738,9 +1738,9 @@ class RbdTestCase(_ImageTestCase, test.NoDBTestCase): mock_rm.assert_called_once_with(image.rbd_name, 'snap', force=True, ignore_errors=False, - pool=image.pool) + pool=image.driver.pool) mock_destroy.assert_called_once_with(image.rbd_name, - pool=image.pool) + pool=image.driver.pool) class PloopTestCase(_ImageTestCase, test.NoDBTestCase): diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index d29143c6090e..436b7d5c53f8 100644 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -1239,14 +1239,6 @@ class LibvirtDriver(driver.ComputeDriver): ret.append(int(obj.get('unit', 0))) return max(ret) - @staticmethod - def _get_rbd_driver(): - return rbd_utils.RBDDriver( - pool=CONF.libvirt.images_rbd_pool, - ceph_conf=CONF.libvirt.images_rbd_ceph_conf, - rbd_user=CONF.libvirt.rbd_user, - rbd_connect_timeout=CONF.libvirt.rbd_connect_timeout) - def _cleanup_rbd(self, instance): # NOTE(nic): On revert_resize, the cleanup steps for the root # volume are handled with an "rbd snap rollback" command, @@ -1257,7 +1249,7 @@ class LibvirtDriver(driver.ComputeDriver): disk.endswith('disk.local')) else: filter_fn = lambda disk: disk.startswith(instance.uuid) - LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn) + rbd_utils.RBDDriver().cleanup_volumes(filter_fn) def _cleanup_lvm(self, instance, block_device_info): """Delete all LVM disks for given instance object.""" @@ -3197,7 +3189,7 @@ class LibvirtDriver(driver.ComputeDriver): if CONF.libvirt.images_type == 'rbd': filter_fn = lambda disk: (disk.startswith(instance.uuid) and disk.endswith('.rescue')) - LibvirtDriver._get_rbd_driver().cleanup_volumes(filter_fn) + rbd_utils.RBDDriver().cleanup_volumes(filter_fn) def poll_rebooting_instances(self, timeout, instances): pass @@ -5866,7 +5858,7 @@ class LibvirtDriver(driver.ComputeDriver): info = lvm.get_volume_group_info( CONF.libvirt.images_volume_group) elif CONF.libvirt.images_type == 'rbd': - info = LibvirtDriver._get_rbd_driver().get_pool_info() + info = rbd_utils.RBDDriver().get_pool_info() else: info = libvirt_utils.get_fs_info(CONF.instances_path) diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index 66f2ce3196a4..4e59279ac068 100644 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -845,25 +845,16 @@ class Rbd(Image): else: self.rbd_name = '%s_%s' % (instance.uuid, disk_name) - self.pool = CONF.libvirt.images_rbd_pool - self.rbd_user = CONF.libvirt.rbd_user - self.rbd_connect_timeout = CONF.libvirt.rbd_connect_timeout - self.ceph_conf = CONF.libvirt.images_rbd_ceph_conf + self.driver = rbd_utils.RBDDriver() - path = 'rbd:%s/%s' % (self.pool, self.rbd_name) - if self.rbd_user: - path += ':id=' + self.rbd_user - if self.ceph_conf: - path += ':conf=' + self.ceph_conf + path = 'rbd:%s/%s' % (self.driver.pool, self.rbd_name) + if self.driver.rbd_user: + path += ':id=' + self.driver.rbd_user + if self.driver.ceph_conf: + path += ':conf=' + self.driver.ceph_conf super(Rbd, self).__init__(path, "block", "rbd", is_block_dev=False) - self.driver = rbd_utils.RBDDriver( - pool=self.pool, - ceph_conf=self.ceph_conf, - rbd_user=self.rbd_user, - rbd_connect_timeout=self.rbd_connect_timeout) - self.discard_mode = CONF.libvirt.hw_disk_discard def libvirt_info(self, disk_info, cache_mode, @@ -889,16 +880,16 @@ class Rbd(Image): info.target_dev = disk_info['dev'] info.source_type = 'network' info.source_protocol = 'rbd' - info.source_name = '%s/%s' % (self.pool, self.rbd_name) + info.source_name = '%s/%s' % (self.driver.pool, self.rbd_name) info.source_hosts = hosts info.source_ports = ports info.boot_order = boot_order - auth_enabled = (CONF.libvirt.rbd_user is not None) + auth_enabled = (self.driver.rbd_user is not None) if CONF.libvirt.rbd_secret_uuid: info.auth_secret_uuid = CONF.libvirt.rbd_secret_uuid auth_enabled = True # Force authentication locally - if CONF.libvirt.rbd_user: - info.auth_username = CONF.libvirt.rbd_user + if self.driver.rbd_user: + info.auth_username = self.driver.rbd_user if auth_enabled: info.auth_secret_type = 'ceph' info.auth_secret_uuid = CONF.libvirt.rbd_secret_uuid @@ -989,7 +980,7 @@ class Rbd(Image): reason=reason) def flatten(self): - self.driver.flatten(self.rbd_name, pool=self.pool) + self.driver.flatten(self.rbd_name, pool=self.driver.pool) def get_model(self, connection): secret = None @@ -1002,8 +993,8 @@ class Rbd(Image): servers = [str(':'.join(k)) for k in zip(hosts, ports)] return imgmodel.RBDImage(self.rbd_name, - self.pool, - self.rbd_user, + self.driver.pool, + self.driver.rbd_user, secret, servers) @@ -1077,7 +1068,7 @@ class Rbd(Image): self.driver.create_snap(self.rbd_name, snapshot_name, protect=True) location = {'url': 'rbd://%(fsid)s/%(pool)s/%(image)s/%(snap)s' % dict(fsid=fsid, - pool=self.pool, + pool=self.driver.pool, image=self.rbd_name, snap=snapshot_name)} try: diff --git a/nova/virt/libvirt/storage/rbd_utils.py b/nova/virt/libvirt/storage/rbd_utils.py index f78dfd228f6c..641dc3217a0f 100644 --- a/nova/virt/libvirt/storage/rbd_utils.py +++ b/nova/virt/libvirt/storage/rbd_utils.py @@ -32,10 +32,14 @@ from oslo_utils import encodeutils from oslo_utils import excutils from oslo_utils import units +import nova.conf from nova import exception from nova.i18n import _ from nova.virt.libvirt import utils as libvirt_utils + +CONF = nova.conf.CONF + LOG = logging.getLogger(__name__) @@ -118,16 +122,15 @@ class RADOSClient(object): class RBDDriver(object): - def __init__(self, pool, ceph_conf, rbd_user, rbd_connect_timeout): - self.pool = pool - # NOTE(angdraug): rados.Rados fails to connect if ceph_conf is None: - # https://github.com/ceph/ceph/pull/1787 - self.ceph_conf = ceph_conf or '' - self.rbd_user = rbd_user or None - self.rbd_connect_timeout = rbd_connect_timeout + def __init__(self): if rbd is None: raise RuntimeError(_('rbd python libraries not found')) + self.pool = CONF.libvirt.images_rbd_pool + self.rbd_user = CONF.libvirt.rbd_user + self.rbd_connect_timeout = CONF.libvirt.rbd_connect_timeout + self.ceph_conf = CONF.libvirt.images_rbd_ceph_conf + def _connect_to_rados(self, pool=None): client = rados.Rados(rados_id=self.rbd_user, conffile=self.ceph_conf)