From fa8ef9d63ecad759727f68a0204244b90d2e4e60 Mon Sep 17 00:00:00 2001 From: Michael Hanselmann Date: Wed, 3 Oct 2012 04:59:29 +0200 Subject: [PATCH] Wipe added space when growing disks MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit This patch adds code to wipe newly added disk space when growing disks using “gnt-instance grow-disk”. “New disk space” is defined as the delta between the old block device size (not necessarily equal to the amount recorded in the configuration) and the new recorded size. Extra caution is taken to avoid overwriting existing data. Unit tests are included. A small typo in gnt-cluster(8) (“it's”) is also fixed. Signed-off-by: Michael Hanselmann Reviewed-by: Iustin Pop --- lib/cmdlib.py | 83 +++++++++++++++++++--- man/gnt-cluster.rst | 8 +-- test/ganeti.cmdlib_unittest.py | 150 +++++++++++++++++++++++++++------------- 3 files changed, 177 insertions(+), 64 deletions(-) diff --git a/lib/cmdlib.py b/lib/cmdlib.py index 4cccd51..ae08146 100644 --- a/lib/cmdlib.py +++ b/lib/cmdlib.py @@ -8972,7 +8972,7 @@ def _CalcEta(time_taken, written, total_size): return (total_size - written) * avg_time -def _WipeDisks(lu, instance): +def _WipeDisks(lu, instance, disks=None): """Wipes instance disks. @type lu: L{LogicalUnit} @@ -8984,13 +8984,18 @@ def _WipeDisks(lu, instance): """ node = instance.primary_node - for device in instance.disks: + if disks is None: + disks = [(idx, disk, 0) + for (idx, disk) in enumerate(instance.disks)] + + for (_, device, _) in disks: lu.cfg.SetDiskID(device, node) logging.info("Pausing synchronization of disks of instance '%s'", instance.name) result = lu.rpc.call_blockdev_pause_resume_sync(node, - (instance.disks, instance), + (map(compat.snd, disks), + instance), True) result.Raise("Failed to pause disk synchronization on node '%s'" % node) @@ -9000,22 +9005,29 @@ def _WipeDisks(lu, instance): " failed", idx, instance.name) try: - for idx, device in enumerate(instance.disks): + for (idx, device, offset) in disks: # The wipe size is MIN_WIPE_CHUNK_PERCENT % of the instance disk but # MAX_WIPE_CHUNK at max. Truncating to integer to avoid rounding errors. wipe_chunk_size = \ int(min(constants.MAX_WIPE_CHUNK, device.size / 100.0 * constants.MIN_WIPE_CHUNK_PERCENT)) - lu.LogInfo("* Wiping disk %d", idx) - logging.info("Wiping disk %d for instance %s on node %s using" - " chunk size %s", idx, instance.name, node, wipe_chunk_size) - - offset = 0 size = device.size last_output = 0 start_time = time.time() + if offset == 0: + info_text = "" + else: + info_text = (" (from %s to %s)" % + (utils.FormatUnit(offset, "h"), + utils.FormatUnit(size, "h"))) + + lu.LogInfo("* Wiping disk %s%s", idx, info_text) + + logging.info("Wiping disk %d for instance %s on node %s using" + " chunk size %s", idx, instance.name, node, wipe_chunk_size) + while offset < size: wipe_size = min(wipe_chunk_size, size - offset) @@ -9039,7 +9051,8 @@ def _WipeDisks(lu, instance): instance.name) result = lu.rpc.call_blockdev_pause_resume_sync(node, - (instance.disks, instance), + (map(compat.snd, disks), + instance), False) if result.fail_msg: @@ -11832,6 +11845,23 @@ def _LoadNodeEvacResult(lu, alloc_result, early_release, use_nodes): for ops in jobs] +def _DiskSizeInBytesToMebibytes(lu, size): + """Converts a disk size in bytes to mebibytes. + + Warns and rounds up if the size isn't an even multiple of 1 MiB. + + """ + (mib, remainder) = divmod(size, 1024 * 1024) + + if remainder != 0: + lu.LogWarning("Disk size is not an even multiple of 1 MiB; rounding up" + " to not overwrite existing data (%s bytes will not be" + " wiped)", (1024 * 1024) - remainder) + mib += 1 + + return mib + + class LUInstanceGrowDisk(LogicalUnit): """Grow a disk of an instance. @@ -11933,6 +11963,8 @@ class LUInstanceGrowDisk(LogicalUnit): assert (self.owned_locks(locking.LEVEL_NODE) == self.owned_locks(locking.LEVEL_NODE_RES)) + wipe_disks = self.cfg.GetClusterInfo().prealloc_wipe_disks + disks_ok, _ = _AssembleInstanceDisks(self, self.instance, disks=[disk]) if not disks_ok: raise errors.OpExecError("Cannot activate block device to grow") @@ -11947,7 +11979,27 @@ class LUInstanceGrowDisk(LogicalUnit): self.cfg.SetDiskID(disk, node) result = self.rpc.call_blockdev_grow(node, (disk, instance), self.delta, True, True) - result.Raise("Grow request failed to node %s" % node) + result.Raise("Dry-run grow request failed to node %s" % node) + + if wipe_disks: + # Get disk size from primary node for wiping + result = self.rpc.call_blockdev_getsize(instance.primary_node, [disk]) + result.Raise("Failed to retrieve disk size from node '%s'" % + instance.primary_node) + + (disk_size_in_bytes, ) = result.payload + + if disk_size_in_bytes is None: + raise errors.OpExecError("Failed to retrieve disk size from primary" + " node '%s'" % instance.primary_node) + + old_disk_size = _DiskSizeInBytesToMebibytes(self, disk_size_in_bytes) + + assert old_disk_size >= disk.size, \ + ("Retrieved disk size too small (got %s, should be at least %s)" % + (old_disk_size, disk.size)) + else: + old_disk_size = None # We know that (as far as we can test) operations across different # nodes will succeed, time to run it for real on the backing storage @@ -11973,6 +12025,15 @@ class LUInstanceGrowDisk(LogicalUnit): # Downgrade lock while waiting for sync self.glm.downgrade(locking.LEVEL_INSTANCE) + assert wipe_disks ^ (old_disk_size is None) + + if wipe_disks: + assert instance.disks[self.op.disk] == disk + + # Wipe newly added disk space + _WipeDisks(self, instance, + disks=[(self.op.disk, disk, old_disk_size)]) + if self.op.wait_for_sync: disk_abort = not _WaitForSync(self, instance, disks=[disk]) if disk_abort: diff --git a/man/gnt-cluster.rst b/man/gnt-cluster.rst index 8082d14..3cf77d2 100644 --- a/man/gnt-cluster.rst +++ b/man/gnt-cluster.rst @@ -268,10 +268,10 @@ The ``--file-storage-dir`` option allows you set the directory to use for storing the instance disk files when using file storage as backend for instance disks. -The ``--prealloc-wipe-disks`` sets a cluster wide configuration -value for wiping disks prior to allocation. This increases security -on instance level as the instance can't access untouched data from -it's underlying storage. +The ``--prealloc-wipe-disks`` sets a cluster wide configuration value +for wiping disks prior to allocation and size changes (``gnt-instance +grow-disk``). This increases security on instance level as the instance +can't access untouched data from its underlying storage. The ``--enabled-hypervisors`` option allows you to set the list of hypervisors that will be enabled for this cluster. Instance diff --git a/test/ganeti.cmdlib_unittest.py b/test/ganeti.cmdlib_unittest.py index 58ba4a5..fe57980 100755 --- a/test/ganeti.cmdlib_unittest.py +++ b/test/ganeti.cmdlib_unittest.py @@ -1265,7 +1265,7 @@ class _DiskPauseTracker: self.history = [] def __call__(self, (disks, instance), pause): - assert instance.disks == disks + assert not (set(disks) - set(instance.disks)) self.history.extend((i.logical_id, i.size, pause) for i in disks) @@ -1336,65 +1336,117 @@ class TestWipeDisks(unittest.TestCase): ]) def testNormalWipe(self): - pt = _DiskPauseTracker() + for start_offset in [0, 280, 8895, 1563204]: + pt = _DiskPauseTracker() - progress = {} + progress = {} - def _WipeCb((disk, _), offset, size): - assert isinstance(offset, (long, int)) - assert isinstance(size, (long, int)) + def _WipeCb((disk, _), offset, size): + assert isinstance(offset, (long, int)) + assert isinstance(size, (long, int)) - max_chunk_size = (disk.size / 100.0 * constants.MIN_WIPE_CHUNK_PERCENT) + max_chunk_size = (disk.size / 100.0 * constants.MIN_WIPE_CHUNK_PERCENT) - self.assertTrue(offset >= 0) - self.assertTrue((offset + size) <= disk.size) + self.assertTrue(offset >= start_offset) + self.assertTrue((offset + size) <= disk.size) - self.assertTrue(size > 0) - self.assertTrue(size <= constants.MAX_WIPE_CHUNK) - self.assertTrue(size <= max_chunk_size) + self.assertTrue(size > 0) + self.assertTrue(size <= constants.MAX_WIPE_CHUNK) + self.assertTrue(size <= max_chunk_size) - self.assertTrue(offset == 0 or disk.logical_id in progress) + self.assertTrue(offset == start_offset or disk.logical_id in progress) - # Keep track of progress - cur_progress = progress.setdefault(disk.logical_id, 0) - self.assertEqual(cur_progress, offset) + # Keep track of progress + cur_progress = progress.setdefault(disk.logical_id, start_offset) + self.assertEqual(cur_progress, offset) - progress[disk.logical_id] += size + progress[disk.logical_id] += size - return (True, None) + return (True, None) - lu = _FakeLU(rpc=_RpcForDiskWipe(pt, _WipeCb), - cfg=_ConfigForDiskWipe()) + lu = _FakeLU(rpc=_RpcForDiskWipe(pt, _WipeCb), + cfg=_ConfigForDiskWipe()) - disks = [ - objects.Disk(dev_type=constants.LD_LV, logical_id="disk0", size=1024), - objects.Disk(dev_type=constants.LD_LV, logical_id="disk1", - size=500 * 1024), - objects.Disk(dev_type=constants.LD_LV, logical_id="disk2", size=128), - objects.Disk(dev_type=constants.LD_LV, logical_id="disk3", - size=constants.MAX_WIPE_CHUNK), - ] - - instance = objects.Instance(name="inst3560", - primary_node="node1.example.com", - disk_template=constants.DT_PLAIN, - disks=disks) - - cmdlib._WipeDisks(lu, instance) - - self.assertEqual(pt.history, [ - ("disk0", 1024, True), - ("disk1", 500 * 1024, True), - ("disk2", 128, True), - ("disk3", constants.MAX_WIPE_CHUNK, True), - ("disk0", 1024, False), - ("disk1", 500 * 1024, False), - ("disk2", 128, False), - ("disk3", constants.MAX_WIPE_CHUNK, False), - ]) - - # Ensure the complete disk has been wiped - self.assertEqual(progress, dict((i.logical_id, i.size) for i in disks)) + if start_offset > 0: + disks = [ + objects.Disk(dev_type=constants.LD_LV, logical_id="disk0", + size=128), + objects.Disk(dev_type=constants.LD_LV, logical_id="disk1", + size=start_offset + (100 * 1024)), + ] + else: + disks = [ + objects.Disk(dev_type=constants.LD_LV, logical_id="disk0", size=1024), + objects.Disk(dev_type=constants.LD_LV, logical_id="disk1", + size=500 * 1024), + objects.Disk(dev_type=constants.LD_LV, logical_id="disk2", size=128), + objects.Disk(dev_type=constants.LD_LV, logical_id="disk3", + size=constants.MAX_WIPE_CHUNK), + ] + + instance = objects.Instance(name="inst3560", + primary_node="node1.example.com", + disk_template=constants.DT_PLAIN, + disks=disks) + + if start_offset > 0: + # Test start offset with only one disk + cmdlib._WipeDisks(lu, instance, + disks=[(1, disks[1], start_offset)]) + self.assertEqual(pt.history, [ + ("disk1", start_offset + (100 * 1024), True), + ("disk1", start_offset + (100 * 1024), False), + ]) + self.assertEqual(progress, { + "disk1": disks[1].size, + }) + else: + cmdlib._WipeDisks(lu, instance) + + self.assertEqual(pt.history, [ + ("disk0", 1024, True), + ("disk1", 500 * 1024, True), + ("disk2", 128, True), + ("disk3", constants.MAX_WIPE_CHUNK, True), + ("disk0", 1024, False), + ("disk1", 500 * 1024, False), + ("disk2", 128, False), + ("disk3", constants.MAX_WIPE_CHUNK, False), + ]) + + # Ensure the complete disk has been wiped + self.assertEqual(progress, dict((i.logical_id, i.size) for i in disks)) + + +class TestDiskSizeInBytesToMebibytes(unittest.TestCase): + def testLessThanOneMebibyte(self): + for i in [1, 2, 7, 512, 1000, 1023]: + lu = _FakeLU() + result = cmdlib._DiskSizeInBytesToMebibytes(lu, i) + self.assertEqual(result, 1) + self.assertEqual(len(lu.warning_log), 1) + self.assertEqual(len(lu.warning_log[0]), 2) + (_, (warnsize, )) = lu.warning_log[0] + self.assertEqual(warnsize, (1024 * 1024) - i) + + def testEven(self): + for i in [1, 2, 7, 512, 1000, 1023]: + lu = _FakeLU() + result = cmdlib._DiskSizeInBytesToMebibytes(lu, i * 1024 * 1024) + self.assertEqual(result, i) + self.assertFalse(lu.warning_log) + + def testLargeNumber(self): + for i in [1, 2, 7, 512, 1000, 1023, 2724, 12420]: + for j in [1, 2, 486, 326, 986, 1023]: + lu = _FakeLU() + size = (1024 * 1024 * i) + j + result = cmdlib._DiskSizeInBytesToMebibytes(lu, size) + self.assertEqual(result, i + 1, msg="Amount was not rounded up") + self.assertEqual(len(lu.warning_log), 1) + self.assertEqual(len(lu.warning_log[0]), 2) + (_, (warnsize, )) = lu.warning_log[0] + self.assertEqual(warnsize, (1024 * 1024) - j) if __name__ == "__main__": -- 1.7.10.4