From 302402df330a52fbe9e531cf5603babad0c1f367 Mon Sep 17 00:00:00 2001 From: Lucian Petrut Date: Fri, 15 Dec 2017 14:37:14 +0200 Subject: [PATCH] Add Windows volume backup support This patch makes a few small changes that are required in order to have the Cinder Backup service working on Windows. - all physial disks must be open in byte mode. 'rb+' must be used when writing. - reading passed the disk size boundary will not return an empty string, raising an IOError instead. For this reason, we're avoiding doing it. - we ensure that the chunk size is a multiple of the sector size. - the chmod command is not available on Windows. Although changing owners is possible, it is not needed. For this reason, the 'temporary_chown' helper will be a noop on Windows. It's easier to do it here rather than do platform checks wherever this gets called. - when connecting the volumes, we pass the 'expect_raw_disk' argument, which provides a hint to the connector about what we expect. This allows the SMBFS connector to return a mounted raw disk path instead of a virtual image path. - when the driver provides temporary snapshots to be used during the backup process, the API is bypassed. For this reason, we need to ensure that the snapshot state and progress gets updated accordingly. Otherwise, this breaks the nova assisted snapshot workflow. We're doing platform checks, ensuring that we don't break/change the current workflow. The Swift and Posix backup drivers are known to be working on Windows. Implements: blueprint windows-smb-backup Depends-On: #I20f791482fb0912772fa62d2949fa5becaec5675 Change-Id: I8769a135974240fdf7cebd4b6d74aaa439ba1f27 --- cinder/backup/chunkeddriver.py | 33 ++++++++- cinder/backup/manager.py | 16 +++-- .../unit/backup/drivers/test_backup_swift.py | 69 +++++++++++++++++++ cinder/tests/unit/backup/test_backup.py | 17 +++-- cinder/tests/unit/test_utils.py | 10 +++ cinder/utils.py | 7 ++ cinder/volume/driver.py | 1 + cinder/volume/drivers/windows/iscsi.py | 3 + cinder/volume/drivers/windows/smbfs.py | 3 + ...indows-volume-backup-b328858a20f5a499.yaml | 9 +++ 10 files changed, 156 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/windows-volume-backup-b328858a20f5a499.yaml diff --git a/cinder/backup/chunkeddriver.py b/cinder/backup/chunkeddriver.py index a5436154244..9c528b455db 100644 --- a/cinder/backup/chunkeddriver.py +++ b/cinder/backup/chunkeddriver.py @@ -25,6 +25,7 @@ import abc import hashlib import json import os +import sys import eventlet from oslo_config import cfg @@ -41,6 +42,9 @@ from cinder import objects from cinder.objects import fields from cinder.volume import utils as volume_utils +if sys.platform == 'win32': + from os_win import utilsfactory as os_win_utilsfactory + LOG = logging.getLogger(__name__) chunkedbackup_service_opts = [ @@ -116,6 +120,13 @@ class ChunkedBackupDriver(driver.BackupDriver): self._get_compressor(CONF.backup_compression_algorithm) self.support_force_delete = True + if sys.platform == 'win32' and self.chunk_size_bytes % 4096: + # The chunk size must be a multiple of the sector size. In order + # to fail out early and avoid attaching the disks, we'll just + # enforce the chunk size to be a multiple of 4096. + err = _("Invalid chunk size. It must be a multiple of 4096.") + raise exception.InvalidConfigurationValue(message=err) + def _get_object_writer(self, container, object_name, extra_metadata=None): """Return writer proxy-wrapped to execute methods in native thread.""" writer = self.get_object_writer(container, object_name, extra_metadata) @@ -453,6 +464,12 @@ class ChunkedBackupDriver(driver.BackupDriver): extra_usage_info= object_meta) + def _get_win32_phys_disk_size(self, disk_path): + win32_diskutils = os_win_utilsfactory.get_diskutils() + disk_number = win32_diskutils.get_device_number_from_device_name( + disk_path) + return win32_diskutils.get_disk_size(disk_number) + def backup(self, backup, volume_file, backup_metadata=True): """Backup the given volume. @@ -488,6 +505,13 @@ class ChunkedBackupDriver(driver.BackupDriver): 'backup. Do a full backup.') raise exception.InvalidBackup(reason=err) + if sys.platform == 'win32': + # When dealing with Windows physical disks, we need the exact + # size of the disk. Attempting to read passed this boundary will + # lead to an IOError exception. At the same time, we cannot + # seek to the end of file. + win32_disk_size = self._get_win32_phys_disk_size(volume_file.name) + (object_meta, object_sha256, extra_metadata, container, volume_size_bytes) = self._prepare_backup(backup) @@ -526,7 +550,14 @@ class ChunkedBackupDriver(driver.BackupDriver): LOG.debug('Cancel the backup process of %s.', backup.id) break data_offset = volume_file.tell() - data = volume_file.read(self.chunk_size_bytes) + + if sys.platform == 'win32': + read_bytes = min(self.chunk_size_bytes, + win32_disk_size - data_offset) + else: + read_bytes = self.chunk_size_bytes + data = volume_file.read(read_bytes) + if data == b'': break diff --git a/cinder/backup/manager.py b/cinder/backup/manager.py index 0cb3900b65b..d0b1ae3c1c1 100644 --- a/cinder/backup/manager.py +++ b/cinder/backup/manager.py @@ -275,6 +275,10 @@ class BackupManager(manager.ThreadPoolManager): try: temp_snapshot = objects.Snapshot.get_by_id( ctxt, backup.temp_snapshot_id) + # We may want to consider routing those calls through the + # cinder API. + temp_snapshot.status = fields.SnapshotStatus.DELETING + temp_snapshot.save() self.volume_rpcapi.delete_snapshot(ctxt, temp_snapshot) except exception.SnapshotNotFound: LOG.debug("Could not find temp snapshot %(snap)s to clean " @@ -432,12 +436,12 @@ class BackupManager(manager.ThreadPoolManager): if (isinstance(device_path, six.string_types) and not os.path.isdir(device_path)): if backup_device.secure_enabled: - with open(device_path) as device_file: + with open(device_path, 'rb') as device_file: updates = backup_service.backup( backup, tpool.Proxy(device_file)) else: with utils.temporary_chown(device_path): - with open(device_path) as device_file: + with open(device_path, 'rb') as device_file: updates = backup_service.backup( backup, tpool.Proxy(device_file)) # device_path is already file-like so no need to open it @@ -551,15 +555,16 @@ class BackupManager(manager.ThreadPoolManager): # with native threads proxy-wrapping the device file object. try: device_path = attach_info['device']['path'] + open_mode = 'rb+' if os.name == 'nt' else 'wb' if (isinstance(device_path, six.string_types) and not os.path.isdir(device_path)): if secure_enabled: - with open(device_path, 'wb') as device_file: + with open(device_path, open_mode) as device_file: backup_service.restore(backup, volume.id, tpool.Proxy(device_file)) else: with utils.temporary_chown(device_path): - with open(device_path, 'wb') as device_file: + with open(device_path, open_mode) as device_file: backup_service.restore(backup, volume.id, tpool.Proxy(device_file)) # device_path is already file-like so no need to open it @@ -987,7 +992,8 @@ class BackupManager(manager.ThreadPoolManager): protocol, use_multipath=use_multipath, device_scan_attempts=device_scan_attempts, - conn=conn) + conn=conn, + expect_raw_disk=True) vol_handle = connector.connect_volume(conn['data']) return {'conn': conn, 'device': vol_handle, 'connector': connector} diff --git a/cinder/tests/unit/backup/drivers/test_backup_swift.py b/cinder/tests/unit/backup/drivers/test_backup_swift.py index d7faf0a4b04..c31d4f9bc82 100644 --- a/cinder/tests/unit/backup/drivers/test_backup_swift.py +++ b/cinder/tests/unit/backup/drivers/test_backup_swift.py @@ -32,6 +32,7 @@ import mock from oslo_config import cfg from swiftclient import client as swift +from cinder.backup import chunkeddriver from cinder.backup.drivers import swift as swift_dr from cinder import context from cinder import db @@ -873,3 +874,71 @@ class BackupSwiftTestCase(test.TestCase): self.assertEqual('none', result[0]) self.assertEqual(already_compressed_data, result[1]) + + +class WindowsBackupSwiftTestCase(BackupSwiftTestCase): + # We're running all the parent class tests, while doing + # some patching in order to simulate Windows behavior. + def setUp(self): + self._mock_utilsfactory = mock.Mock() + + platform_patcher = mock.patch('sys.platform', 'win32') + platform_patcher.start() + self.addCleanup(platform_patcher.stop) + + super(WindowsBackupSwiftTestCase, self).setUp() + + read = self.volume_file.read + + def win32_read(sz): + # We're simulating the Windows behavior. + if self.volume_file.tell() > fake_get_size(): + raise IOError() + return read(sz) + + read_patcher = mock.patch.object( + self.volume_file, 'read', win32_read) + read_patcher.start() + self.addCleanup(read_patcher.stop) + + def fake_get_size(*args, **kwargs): + pos = self.volume_file.tell() + sz = self.volume_file.seek(0, 2) + self.volume_file.seek(pos) + return sz + + self._disk_size_getter_mocker = mock.patch.object( + swift_dr.SwiftBackupDriver, + '_get_win32_phys_disk_size', + fake_get_size) + + self._disk_size_getter_mocker.start() + self.addCleanup(self._disk_size_getter_mocker.stop) + + def test_invalid_chunk_size(self): + self.flags(backup_swift_object_size=1000) + # We expect multiples of 4096 + self.assertRaises(exception.InvalidConfigurationValue, + swift_dr.SwiftBackupDriver, + self.ctxt) + + @mock.patch.object(chunkeddriver, 'os_win_utilsfactory', create=True) + def test_get_phys_disk_size(self, mock_utilsfactory): + # We're patching this method in setUp, so we need to + # retrieve the original one. Note that we'll get an unbound + # method. + service = swift_dr.SwiftBackupDriver(self.ctxt) + get_disk_size = self._disk_size_getter_mocker.temp_original + + disk_utils = mock_utilsfactory.get_diskutils.return_value + disk_utils.get_device_number_from_device_name.return_value = ( + mock.sentinel.dev_num) + disk_utils.get_disk_size.return_value = mock.sentinel.disk_size + + disk_size = get_disk_size(service, mock.sentinel.disk_path) + + self.assertEqual(mock.sentinel.disk_size, disk_size) + disk_utils.get_device_number_from_device_name.assert_called_once_with( + mock.sentinel.disk_path) + disk_utils.get_disk_size.assert_called_once_with( + mock.sentinel.dev_num) diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index 21ac6cdd5ee..0ce5de24681 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -591,7 +591,7 @@ class BackupTestCase(BaseBackupTest): @mock.patch('cinder.utils.brick_get_connector_properties') @mock.patch('cinder.volume.rpcapi.VolumeAPI.get_backup_device') @mock.patch('cinder.utils.temporary_chown') - @mock.patch('six.moves.builtins.open') + @mock.patch('six.moves.builtins.open', wraps=open) @mock.patch.object(os.path, 'isdir', return_value=False) def test_create_backup(self, mock_isdir, mock_open, mock_temporary_chown, mock_get_backup_device, mock_get_conn): @@ -616,7 +616,6 @@ class BackupTestCase(BaseBackupTest): mock_attach_device.return_value = attach_info properties = {} mock_get_conn.return_value = properties - mock_open.return_value = open('/dev/null', 'rb') self.backup_mgr.create_backup(self.ctxt, backup) @@ -630,6 +629,7 @@ class BackupTestCase(BaseBackupTest): force=True, ignore_errors=True) + mock_open.assert_called_once_with('/dev/null', 'rb') vol = objects.Volume.get_by_id(self.ctxt, vol_id) self.assertEqual('available', vol['status']) self.assertEqual('backing-up', vol['previous_status']) @@ -1011,10 +1011,14 @@ class BackupTestCase(BaseBackupTest): @mock.patch('cinder.utils.brick_get_connector_properties') @mock.patch('cinder.utils.temporary_chown') - @mock.patch('six.moves.builtins.open') + @mock.patch('six.moves.builtins.open', wraps=open) @mock.patch.object(os.path, 'isdir', return_value=False) + @ddt.data({'os_name': 'nt', 'exp_open_mode': 'rb+'}, + {'os_name': 'posix', 'exp_open_mode': 'wb'}) + @ddt.unpack def test_restore_backup(self, mock_isdir, mock_open, - mock_temporary_chown, mock_get_conn): + mock_temporary_chown, mock_get_conn, + os_name, exp_open_mode): """Test normal backup restoration.""" vol_size = 1 vol_id = self._create_volume_db_entry(status='restoring-backup', @@ -1024,7 +1028,6 @@ class BackupTestCase(BaseBackupTest): properties = {} mock_get_conn.return_value = properties - mock_open.return_value = open('/dev/null', 'wb') mock_secure_enabled = ( self.volume_mocks['secure_file_operations_enabled']) mock_secure_enabled.return_value = False @@ -1036,8 +1039,10 @@ class BackupTestCase(BaseBackupTest): '_attach_device') mock_attach_device.return_value = attach_info - self.backup_mgr.restore_backup(self.ctxt, backup, vol_id) + with mock.patch('os.name', os_name): + self.backup_mgr.restore_backup(self.ctxt, backup, vol_id) + mock_open.assert_called_once_with('/dev/null', exp_open_mode) mock_temporary_chown.assert_called_once_with('/dev/null') mock_get_conn.assert_called_once_with() mock_secure_enabled.assert_called_once_with(self.ctxt, vol) diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py index f7ab2dd35d1..195eb3cd1fe 100644 --- a/cinder/tests/unit/test_utils.py +++ b/cinder/tests/unit/test_utils.py @@ -342,6 +342,16 @@ class TemporaryChownTestCase(test.TestCase): mock_stat.assert_called_once_with(test_filename) self.assertFalse(mock_exec.called) + @mock.patch('os.name', 'nt') + @mock.patch('os.stat') + @mock.patch('cinder.utils.execute') + def test_temporary_chown_win32(self, mock_exec, mock_stat): + with utils.temporary_chown(mock.sentinel.path): + pass + + mock_exec.assert_not_called() + mock_stat.assert_not_called() + class TempdirTestCase(test.TestCase): @mock.patch('tempfile.mkdtemp') diff --git a/cinder/utils.py b/cinder/utils.py index 4e941144180..d2e87ee079c 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -431,6 +431,13 @@ def temporary_chown(path, owner_uid=None): :params owner_uid: UID of temporary owner (defaults to current user) """ + + if os.name == 'nt': + LOG.debug("Skipping chown for %s as this operation is " + "not available on Windows.", path) + yield + return + if owner_uid is None: owner_uid = os.getuid() diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 3ef1904d67c..44f1320c603 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -1255,6 +1255,7 @@ class BaseVD(object): temp_snap_ref.destroy() temp_snap_ref.status = fields.SnapshotStatus.AVAILABLE + temp_snap_ref.progress = '100%' temp_snap_ref.save() return temp_snap_ref diff --git a/cinder/volume/drivers/windows/iscsi.py b/cinder/volume/drivers/windows/iscsi.py index 11fb9c399ac..0e618135034 100644 --- a/cinder/volume/drivers/windows/iscsi.py +++ b/cinder/volume/drivers/windows/iscsi.py @@ -345,3 +345,6 @@ class WindowsISCSIDriver(driver.ISCSIDriver): additional_size_mb = (new_size - old_size) * 1024 self._tgt_utils.extend_wt_disk(volume.name, additional_size_mb) + + def backup_use_temp_snapshot(self): + return False diff --git a/cinder/volume/drivers/windows/smbfs.py b/cinder/volume/drivers/windows/smbfs.py index 3f240a0d89f..4abea70f249 100644 --- a/cinder/volume/drivers/windows/smbfs.py +++ b/cinder/volume/drivers/windows/smbfs.py @@ -656,3 +656,6 @@ class WindowsSmbfsDriver(remotefs_drv.RevertToSnapshotMixin, # The SMBFS driver does not manage file permissions. We chose # to let this up to the deployer. pass + + def backup_use_temp_snapshot(self): + return True diff --git a/releasenotes/notes/windows-volume-backup-b328858a20f5a499.yaml b/releasenotes/notes/windows-volume-backup-b328858a20f5a499.yaml new file mode 100644 index 00000000000..e05601c5691 --- /dev/null +++ b/releasenotes/notes/windows-volume-backup-b328858a20f5a499.yaml @@ -0,0 +1,9 @@ +--- +features: + - | + The Cinder Volume Backup service can now be run on Windows. It supports + backing up volumes exposed by SMBFS/iSCSI Windows Cinder Volume backends, + as well as any other Cinder backend that's accessible on Windows (e.g. + SANs exposing volumes via iSCSI/FC). + + The Swift and Posix backup drivers are known to be working on Windows.