From 478dc240a7a26bd291db02e514cecb0e2a4843e0 Mon Sep 17 00:00:00 2001 From: James Page Date: Tue, 10 Apr 2018 09:58:44 +0100 Subject: [PATCH] Improve idempotency of block device processing Record details of each device that is successfully processed as an OSD data device to ensure that they are not re-processed during subsequent hook executions. "successfully processed" in this case means that the call to either ceph-disk or ceph-volume to prepare (and potentially activate) the OSD completed without error. This avoids potential data loss on reboot when osd-reformat is set to True as the charm will only every process a block device once. Change-Id: I2c6e9d5670c8d1d70584ae19b34eaf16be5dea19 --- ceph/utils.py | 15 +++++++++++++++ unit_tests/test_utils.py | 29 +++++++++++++++++++++++++++-- 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/ceph/utils.py b/ceph/utils.py index 3ba3796..631755f 100644 --- a/ceph/utils.py +++ b/ceph/utils.py @@ -76,6 +76,7 @@ from charmhelpers.contrib.openstack.utils import ( get_os_codename_install_source, ) from charmhelpers.contrib.storage.linux import lvm +from charmhelpers.core.unitdata import kv CEPH_BASE_DIR = os.path.join(os.sep, 'var', 'lib', 'ceph') OSD_BASE_DIR = os.path.join(CEPH_BASE_DIR, 'osd') @@ -1472,6 +1473,13 @@ def osdize(dev, osd_format, osd_journal, reformat_osd=False, def osdize_dev(dev, osd_format, osd_journal, reformat_osd=False, ignore_errors=False, encrypt=False, bluestore=False): + db = kv() + osd_devices = db.get('osd-devices', []) + if dev in osd_devices: + log('Device {} already processed by charm,' + ' skipping'.format(dev)) + return + if not os.path.exists(dev): log('Path {} does not exist - bailing'.format(dev)) return @@ -1529,6 +1537,13 @@ def osdize_dev(dev, osd_format, osd_journal, reformat_osd=False, log('lsblk output: {}'.format(lsblk_output), WARNING) raise + # NOTE: Record processing of device only on success to ensure that + # the charm only tries to initialize a device of OSD usage + # once during its lifetime. + osd_devices.append(dev) + db.set('osd-devices', osd_devices) + db.flush() + def _ceph_disk(dev, osd_format, osd_journal, encrypt=False, bluestore=False): """ diff --git a/unit_tests/test_utils.py b/unit_tests/test_utils.py index f564afe..2d71613 100644 --- a/unit_tests/test_utils.py +++ b/unit_tests/test_utils.py @@ -67,6 +67,7 @@ class CephTestCase(unittest.TestCase): call(['udevadm', 'settle']), ]) + @patch.object(utils, 'kv') @patch.object(utils.subprocess, 'check_call') @patch.object(utils, 'zap_disk') @patch.object(utils, '_ceph_disk') @@ -77,8 +78,11 @@ class CephTestCase(unittest.TestCase): @patch.object(utils, 'is_block_device') def test_osdize_dev_ceph_disk(self, _is_blk, _cmp, _mounted, _exists, _is_active_bluestore_device, _ceph_disk, - _zap_disk, _check_call): + _zap_disk, _check_call, _kv): """Test that _ceph_disk is called for < Luminous 12.2.4""" + db = MagicMock() + _kv.return_value = db + db.get.return_value = [] _is_blk.return_value = True _mounted.return_value = False _exists.return_value = True @@ -90,7 +94,11 @@ class CephTestCase(unittest.TestCase): _ceph_disk.assert_called_with('/dev/sdb', 'xfs', None, False, False) _check_call.assert_called_with(['ceph-disk', 'prepare']) _zap_disk.assert_called_once() + db.get.assert_called_with('osd-devices', []) + db.set.assert_called_with('osd-devices', ['/dev/sdb']) + db.flush.assert_called_once() + @patch.object(utils, 'kv') @patch.object(utils.subprocess, 'check_call') @patch.object(utils, 'zap_disk') @patch.object(utils, '_ceph_volume') @@ -101,8 +109,11 @@ class CephTestCase(unittest.TestCase): @patch.object(utils, 'is_block_device') def test_osdize_dev_ceph_volume(self, _is_blk, _cmp, _mounted, _exists, _is_active_bluestore_device, _ceph_volume, - _zap_disk, _check_call): + _zap_disk, _check_call, _kv): """Test that _ceph_volume is called for >= Luminous 12.2.4""" + db = MagicMock() + _kv.return_value = db + db.get.return_value = [] _is_blk.return_value = True _mounted.return_value = False _exists.return_value = True @@ -114,6 +125,20 @@ class CephTestCase(unittest.TestCase): _ceph_volume.assert_called_with('/dev/sdb', None, False, False) _check_call.assert_called_with(['ceph-volume', 'prepare']) _zap_disk.assert_called_once() + db.get.assert_called_with('osd-devices', []) + db.set.assert_called_with('osd-devices', ['/dev/sdb']) + db.flush.assert_called_once() + + @patch.object(utils, 'kv') + def test_osdize_dev_already_processed(self, _kv): + """Ensure that previously processed disks are skipped""" + db = MagicMock() + _kv.return_value = db + utils.osdize('/dev/sdb', osd_format='xfs', osd_journal=None, + reformat_osd=True, bluestore=False) + db.get.return_value = ['/dev/sdb'] + db.get.assert_called_with('osd-devices', []) + db.set.assert_not_called() @patch.object(utils.subprocess, 'check_call') @patch.object(utils.os.path, 'exists')