From e54f34245880c105225675d9fb91d2ad1997f7ba Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Fri, 13 Sep 2013 15:20:03 -0400 Subject: [PATCH] Fixes ceph volume restore to larger image than source If a backup is restored to a volume that is bigger than the backup itself, which could be the case if --volume-id is supplied, we need the option to set extraneous extents to zero i.e. set extents after those from the backup to zero. Change-Id: I3c226b9e7429371f6cd9b9bd81071ff765712975 Fixes: bug 1225126 --- cinder/backup/drivers/ceph.py | 157 ++++++++++++++++++++++-------- cinder/tests/backup/fake_rados.py | 3 + cinder/tests/test_backup_ceph.py | 96 +++++++++++++----- cinder/volume/drivers/rbd.py | 5 +- etc/cinder/cinder.conf.sample | 6 +- 5 files changed, 198 insertions(+), 69 deletions(-) diff --git a/cinder/backup/drivers/ceph.py b/cinder/backup/drivers/ceph.py index 93672bebaef..e599718e7b5 100644 --- a/cinder/backup/drivers/ceph.py +++ b/cinder/backup/drivers/ceph.py @@ -78,7 +78,10 @@ service_opts = [ cfg.IntOpt('backup_ceph_stripe_unit', default=0, help='RBD stripe unit to use when creating a backup image'), cfg.IntOpt('backup_ceph_stripe_count', default=0, - help='RBD stripe count to use when creating a backup image') + help='RBD stripe count to use when creating a backup image'), + cfg.BoolOpt('restore_discard_excess_bytes', default=True, + help='If True, always discard excess bytes when restoring ' + 'volumes.') ] CONF = cfg.CONF @@ -210,6 +213,24 @@ class CephBackupDriver(BackupDriver): raise exception.InvalidParameterValue(msg) return self._utf8("volume-%s.backup.%s" % (volume_id, backup_id)) + def _discard_bytes(self, volume, offset, length): + """Trim length bytes from offset. + + If the volume is an rbd do a discard() otherwise assume it is a file + and pad with zeroes. + """ + if length: + LOG.info("discarding %s bytes from offset %s" % + (length, offset)) + if self._file_is_rbd(volume): + volume.rbd_image.discard(offset, length) + else: + zeroes = '\0' * length + chunks = int(length / self.chunk_size) + for chunk in xrange(0, chunks): + LOG.debug("writing zeroes chunk %d" % (chunk)) + volume.write(zeroes) + def _transfer_data(self, src, src_name, dest, dest_name, length): """Transfer data between files (Python IO objects).""" LOG.debug(_("transferring data between '%(src)s' and '%(dest)s'") % @@ -222,13 +243,22 @@ class CephBackupDriver(BackupDriver): for chunk in xrange(0, chunks): before = time.time() data = src.read(self.chunk_size) + # If we have reach end of source, discard any extraneous bytes from + # destination volume if trim is enabled and stop writing. + if data == '': + if CONF.restore_discard_excess_bytes: + self._discard_bytes(dest, dest.tell(), + length - dest.tell()) + + return + dest.write(data) dest.flush() delta = (time.time() - before) rate = (self.chunk_size / delta) / 1024 LOG.debug((_("transferred chunk %(chunk)s of %(chunks)s " "(%(rate)dK/s)") % - {'chunk': chunk, 'chunks': chunks, + {'chunk': chunk + 1, 'chunks': chunks, 'rate': rate})) # yield to any other pending backups @@ -238,10 +268,14 @@ class CephBackupDriver(BackupDriver): if rem: LOG.debug(_("transferring remaining %s bytes") % (rem)) data = src.read(rem) - dest.write(data) - dest.flush() - # yield to any other pending backups - eventlet.sleep(0) + if data == '': + if CONF.restore_discard_excess_bytes: + self._discard_bytes(dest, dest.tell(), rem) + else: + dest.write(data) + dest.flush() + # yield to any other pending backups + eventlet.sleep(0) def _create_base_image(self, name, size, rados_client): """Create a base backup image. @@ -370,7 +404,7 @@ class CephBackupDriver(BackupDriver): def _rbd_diff_transfer(self, src_name, src_pool, dest_name, dest_pool, src_user, src_conf, dest_user, dest_conf, src_snap=None, from_snap=None): - """Backup only extents changed between two points. + """Copy only extents changed between two points. If no snapshot is provided, the diff extents will be all those changed since the rbd volume/base was created, otherwise it will be those @@ -431,7 +465,7 @@ class CephBackupDriver(BackupDriver): def _snap_exists(self, base_name, snap_name, client): """Return True if snapshot exists in base image.""" - base_rbd = self.rbd.Image(client.ioctx, base_name) + base_rbd = self.rbd.Image(client.ioctx, base_name, read_only=True) try: snaps = base_rbd.list_snaps() finally: @@ -707,23 +741,26 @@ class CephBackupDriver(BackupDriver): def _full_restore(self, backup_id, volume_id, dest_file, dest_name, length, src_snap=None): - """Restore the given volume file from backup RBD. + """Restore volume using full copy i.e. all extents. - This will result in all extents being copied from source to destination + This will result in all extents being copied from source to + destination. """ with drivers.rbd.RADOSClient(self, self._ceph_backup_pool) as client: - + # If a source snapshot is provided we assume the base is diff + # format. if src_snap: - # If a source snapshot is provided we assume the base is diff - # format. - backup_name = self._get_backup_base_name(volume_id, - diff_format=True) + diff_format = True else: - backup_name = self._get_backup_base_name(volume_id, backup_id) + diff_format = False + + backup_name = self._get_backup_base_name(volume_id, + backup_id=backup_id, + diff_format=diff_format) # Retrieve backup volume src_rbd = self.rbd.Image(client.ioctx, backup_name, - snapshot=src_snap) + snapshot=src_snap, read_only=True) try: rbd_meta = drivers.rbd.RBDImageMetadata(src_rbd, self._ceph_backup_pool, @@ -735,11 +772,40 @@ class CephBackupDriver(BackupDriver): finally: src_rbd.close() - def _restore_rbd(self, base_name, volume_file, volume_name, restore_point): - """Restore RBD volume from RBD image.""" - rbd_user = volume_file.rbd_user - rbd_pool = volume_file.rbd_pool - rbd_conf = volume_file.rbd_conf + def _check_restore_vol_size(self, backup_base, restore_vol, restore_length, + src_pool): + """Ensure that the restore volume is the correct size. + + If the restore volume was bigger than the backup, the diff restore will + shrink it to the size of the original backup so we need to + post-process and resize it back to its expected size. + """ + with drivers.rbd.RADOSClient(self, self._ceph_backup_pool) as client: + adjust_size = 0 + base_image = self.rbd.Image(client.ioctx, self._utf8(backup_base), + read_only=True) + try: + if restore_length != base_image.size(): + adjust_size = restore_length + finally: + base_image.close() + + if adjust_size: + with drivers.rbd.RADOSClient(self, src_pool) as client: + dest_image = self.rbd.Image(client.ioctx, + self._utf8(restore_vol)) + try: + LOG.debug("adjusting restore vol size") + dest_image.resize(adjust_size) + finally: + dest_image.close() + + def _diff_restore_rbd(self, base_name, restore_file, restore_name, + restore_point, restore_length): + """Attempt restore rbd volume from backup using diff transfer.""" + rbd_user = restore_file.rbd_user + rbd_pool = restore_file.rbd_pool + rbd_conf = restore_file.rbd_conf LOG.debug(_("trying incremental restore from base='%(base)s' " "snap='%(snap)s'") % @@ -747,7 +813,7 @@ class CephBackupDriver(BackupDriver): before = time.time() try: self._rbd_diff_transfer(base_name, self._ceph_backup_pool, - volume_name, rbd_pool, + restore_name, rbd_pool, src_user=self._ceph_backup_user, src_conf=self._ceph_backup_conf, dest_user=rbd_user, dest_conf=rbd_conf, @@ -757,13 +823,21 @@ class CephBackupDriver(BackupDriver): "restore")) raise + # If the volume we are restoring to is larger than the backup volume, + # we will need to resize it after the diff import since import-diff + # appears to shrink the target rbd volume to the size of the original + # backup volume. + self._check_restore_vol_size(base_name, restore_name, restore_length, + rbd_pool) + LOG.debug(_("restore transfer completed in %.4fs") % (time.time() - before)) def _num_backup_snaps(self, backup_base_name): """Return the number of snapshots that exist on the base image.""" with drivers.rbd.RADOSClient(self, self._ceph_backup_pool) as client: - base_rbd = self.rbd.Image(client.ioctx, backup_base_name) + base_rbd = self.rbd.Image(client.ioctx, backup_base_name, + read_only=True) try: snaps = self.get_backup_snaps(base_rbd) finally: @@ -780,7 +854,7 @@ class CephBackupDriver(BackupDriver): If the backup was not incremental None is returned. """ with drivers.rbd.RADOSClient(self, self._ceph_backup_pool) as client: - base_rbd = self.rbd.Image(client.ioctx, base_name) + base_rbd = self.rbd.Image(client.ioctx, base_name, read_only=True) try: restore_point = self._get_backup_snap_name(base_rbd, base_name, backup_id) @@ -856,8 +930,11 @@ class CephBackupDriver(BackupDriver): return not_allowed - def _try_restore(self, backup, volume, volume_file): - """Attempt to restore volume from backup.""" + def _restore_volume(self, backup, volume, volume_file): + """Restore volume from backup using diff transfer if possible. + + Attempts a differential restore and reverts to full copy if diff fails. + """ volume_name = volume['name'] backup_id = backup['id'] backup_volume_id = backup['volume_id'] @@ -867,20 +944,19 @@ class CephBackupDriver(BackupDriver): diff_format=True) with drivers.rbd.RADOSClient(self, self._ceph_backup_pool) as client: - diff_restore, restore_point = \ + diff_allowed, restore_point = \ self._diff_restore_allowed(base_name, backup, volume, volume_file, client) - if diff_restore: + do_full_restore = True + if diff_allowed: + # Attempt diff try: + self._diff_restore_rbd(base_name, volume_file, volume_name, + restore_point, length) do_full_restore = False - self._restore_rbd(base_name, volume_file, volume_name, - restore_point) except exception.BackupRBDOperationFailed: LOG.debug(_("forcing full restore")) - do_full_restore = True - else: - do_full_restore = True if do_full_restore: # Otherwise full copy @@ -894,13 +970,10 @@ class CephBackupDriver(BackupDriver): 'volume=%(dest)s') % {'src': backup['id'], 'dest': target_volume['name']}) - # Ensure we are at the beginning of the volume - volume_file.seek(0) - try: - self._try_restore(backup, target_volume, volume_file) + self._restore_volume(backup, target_volume, volume_file) - # Be tolerant to IO implementations that do not support fileno() + # Be tolerant of IO implementations that do not support fileno() try: fileno = volume_file.fileno() except IOError: @@ -909,7 +982,7 @@ class CephBackupDriver(BackupDriver): else: os.fsync(fileno) - LOG.debug(_('restore finished.')) + LOG.debug(_('restore finished successfully.')) except exception.BackupOperationError as e: LOG.error(_('restore finished with error - %s') % (e)) raise @@ -926,8 +999,8 @@ class CephBackupDriver(BackupDriver): "that db entry can be removed") LOG.warning(msg) LOG.info(_("delete '%s' finished with warning") % (backup_id)) - - LOG.debug(_("delete '%s' finished") % (backup_id)) + else: + LOG.debug(_("delete '%s' finished") % (backup_id)) def get_backup_driver(context): diff --git a/cinder/tests/backup/fake_rados.py b/cinder/tests/backup/fake_rados.py index 9839d275547..f0c85c9d5f5 100644 --- a/cinder/tests/backup/fake_rados.py +++ b/cinder/tests/backup/fake_rados.py @@ -95,6 +95,9 @@ class mock_rbd(object): def resize(self, *args, **kwargs): raise NotImplementedError() + def discard(self, offset, length): + raise NotImplementedError() + def close(self): pass diff --git a/cinder/tests/test_backup_ceph.py b/cinder/tests/test_backup_ceph.py index 6314dffc00a..26e1e0faecb 100644 --- a/cinder/tests/test_backup_ceph.py +++ b/cinder/tests/test_backup_ceph.py @@ -31,6 +31,7 @@ from cinder.openstack.common import processutils from cinder import test from cinder.tests.backup.fake_rados import mock_rados from cinder.tests.backup.fake_rados import mock_rbd +from cinder import units from cinder.volume.drivers import rbd as rbddriver LOG = logging.getLogger(__name__) @@ -293,6 +294,9 @@ class BackupCephTestCase(test.TestCase): self.stubs.Set(self.service.rbd.Image, 'write', write_data) + self.stubs.Set(self.service, '_discard_bytes', + lambda *args: None) + self.service.backup(backup, self.volume_file) # Ensure the files are equal @@ -340,6 +344,9 @@ class BackupCephTestCase(test.TestCase): self.stubs.Set(self.service.rbd.Image, 'write', write_data) self.stubs.Set(self.service.rbd.RBD, 'list', rbd_list) + self.stubs.Set(self.service, '_discard_bytes', + lambda *args: None) + meta = rbddriver.RBDImageMetadata(self.service.rbd.Image(), 'pool_foo', 'user_foo', 'conf_foo') @@ -372,6 +379,8 @@ class BackupCephTestCase(test.TestCase): self.stubs.Set(self.service.rbd.RBD, 'list', rbd_list) + self.stubs.Set(self.service, '_discard_bytes', lambda *args: None) + with tempfile.NamedTemporaryFile() as test_file: self.volume_file.seek(0) @@ -390,8 +399,30 @@ class BackupCephTestCase(test.TestCase): # Ensure the files are equal self.assertEqual(checksum.digest(), self.checksum.digest()) - def test_create_base_image_if_not_exists(self): - pass + def test_discard_bytes(self): + self.service._discard_bytes(mock_rbd(), 123456, 0) + calls = [] + + def _setter(*args, **kwargs): + calls.append(True) + + self.stubs.Set(self.service.rbd.Image, 'discard', _setter) + + self.service._discard_bytes(mock_rbd(), 123456, 0) + self.assertTrue(len(calls) == 0) + + image = mock_rbd().Image() + wrapped_rbd = self._get_wrapped_rbd_io(image) + self.service._discard_bytes(wrapped_rbd, 123456, 1234) + self.assertTrue(len(calls) == 1) + + self.stubs.Set(image, 'write', _setter) + wrapped_rbd = self._get_wrapped_rbd_io(image) + self.stubs.Set(self.service, '_file_is_rbd', + lambda *args: False) + self.service._discard_bytes(wrapped_rbd, 0, + self.service.chunk_size * 2) + self.assertTrue(len(calls) == 3) def test_delete_backup_snapshot(self): snap_name = 'backup.%s.snap.3824923.1412' % (uuid.uuid4()) @@ -521,50 +552,69 @@ class BackupCephTestCase(test.TestCase): self.service.delete(backup) def test_diff_restore_allowed_true(self): - is_allowed = (True, 'restore.foo') + restore_point = 'restore.foo' + is_allowed = (True, restore_point) backup = db.backup_get(self.ctxt, self.backup_id) alt_volume_id = str(uuid.uuid4()) - self._create_volume_db_entry(alt_volume_id, 1) + volume_size = 1 + self._create_volume_db_entry(alt_volume_id, volume_size) alt_volume = db.volume_get(self.ctxt, alt_volume_id) rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) self.stubs.Set(self.service, '_get_restore_point', - lambda *args: 'restore.foo') - + lambda *args: restore_point) self.stubs.Set(self.service, '_rbd_has_extents', lambda *args: False) - self.stubs.Set(self.service, '_rbd_image_exists', lambda *args: (True, 'foo')) - - self.stubs.Set(self.service, '_file_is_rbd', lambda *args: True) + self.stubs.Set(self.service, '_file_is_rbd', + lambda *args: True) + self.stubs.Set(self.service.rbd.Image, 'size', + lambda *args: volume_size * units.GiB) resp = self.service._diff_restore_allowed('foo', backup, alt_volume, rbd_io, mock_rados()) self.assertEqual(resp, is_allowed) + def _set_service_stub(self, method, retval): + self.stubs.Set(self.service, method, lambda *args, **kwargs: retval) + def test_diff_restore_allowed_false(self): + volume_size = 1 not_allowed = (False, None) backup = db.backup_get(self.ctxt, self.backup_id) - self._create_volume_db_entry(self.volume_id, 1) + self._create_volume_db_entry(self.volume_id, volume_size) original_volume = db.volume_get(self.ctxt, self.volume_id) rbd_io = self._get_wrapped_rbd_io(self.service.rbd.Image()) - self.stubs.Set(self.service, '_get_restore_point', - lambda *args: None) + test_args = 'foo', backup, original_volume, rbd_io, mock_rados() - self.stubs.Set(self.service, '_rbd_has_extents', - lambda *args: True) - - self.stubs.Set(self.service, '_rbd_image_exists', - lambda *args: (False, 'foo')) - - self.stubs.Set(self.service, '_file_is_rbd', lambda *args: False) - - resp = self.service._diff_restore_allowed('foo', backup, - original_volume, rbd_io, - mock_rados()) + self._set_service_stub('_get_restore_point', None) + resp = self.service._diff_restore_allowed(*test_args) self.assertEqual(resp, not_allowed) + self._set_service_stub('_get_restore_point', 'restore.foo') + + self._set_service_stub('_rbd_has_extents', True) + resp = self.service._diff_restore_allowed(*test_args) + self.assertEqual(resp, not_allowed) + self._set_service_stub('_rbd_has_extents', False) + + self._set_service_stub('_rbd_image_exists', (False, 'foo')) + resp = self.service._diff_restore_allowed(*test_args) + self.assertEqual(resp, not_allowed) + self._set_service_stub('_rbd_image_exists', None) + + self.stubs.Set(self.service.rbd.Image, 'size', + lambda *args, **kwargs: volume_size * units.GiB * 2) + resp = self.service._diff_restore_allowed(*test_args) + self.assertEqual(resp, not_allowed) + self.stubs.Set(self.service.rbd.Image, 'size', + lambda *args, **kwargs: volume_size * units.GiB) + + self._set_service_stub('_file_is_rbd', False) + resp = self.service._diff_restore_allowed(*test_args) + self.assertEqual(resp, not_allowed) + self._set_service_stub('_file_is_rbd', True) def tearDown(self): self.volume_file.close() diff --git a/cinder/volume/drivers/rbd.py b/cinder/volume/drivers/rbd.py index 70b6faab451..994788eb86f 100644 --- a/cinder/volume/drivers/rbd.py +++ b/cinder/volume/drivers/rbd.py @@ -155,7 +155,7 @@ class RBDImageIOWrapper(io.RawIOBase): elif whence == 1: new_offset = self._offset + offset elif whence == 2: - new_offset = self.volume.size() - 1 + new_offset = self._rbd_meta.image.size() new_offset += offset else: raise IOError(_("Invalid argument - whence=%s not supported") % @@ -774,9 +774,8 @@ class RBDDriver(driver.VolumeDriver): """Create a new backup from an existing volume.""" volume = self.db.volume_get(context, backup['volume_id']) pool = self.configuration.rbd_pool - volname = volume['name'] - with RBDVolumeProxy(self, volname, pool) as rbd_image: + with RBDVolumeProxy(self, volume['name'], pool) as rbd_image: rbd_meta = RBDImageMetadata(rbd_image, self.configuration.rbd_pool, self.configuration.rbd_user, self.configuration.rbd_ceph_conf) diff --git a/etc/cinder/cinder.conf.sample b/etc/cinder/cinder.conf.sample index 2e74ca2fc3a..03790856dbb 100644 --- a/etc/cinder/cinder.conf.sample +++ b/etc/cinder/cinder.conf.sample @@ -172,6 +172,10 @@ # (integer value) #backup_ceph_stripe_count=0 +# If True, always discard excess bytes when restoring volumes. +# (boolean value) +#restore_discard_excess_bytes=true + # # Options defined in cinder.backup.drivers.swift @@ -1764,4 +1768,4 @@ #volume_dd_blocksize=1M -# Total option count: 378 +# Total option count: 379