diff --git a/cinder/backup/api.py b/cinder/backup/api.py index c53ac42c566..ec312d75488 100644 --- a/cinder/backup/api.py +++ b/cinder/backup/api.py @@ -349,7 +349,8 @@ class API(base.Base): 'data_timestamp': data_timestamp, 'parent': parent, 'host': latest_host, - 'metadata': metadata or {} + 'metadata': metadata or {}, + 'availability_zone': availability_zone } try: backup = objects.Backup(context=context, **kwargs) diff --git a/cinder/scheduler/driver.py b/cinder/scheduler/driver.py index fe92905bee2..0e3b5bb3554 100644 --- a/cinder/scheduler/driver.py +++ b/cinder/scheduler/driver.py @@ -155,7 +155,7 @@ class Scheduler(object): raise NotImplementedError(_( "Must implement schedule_get_pools")) - def get_backup_host(self, volume, driver=None): + def get_backup_host(self, volume, availability_zone, driver=None): """Must override schedule method for scheduler to work.""" raise NotImplementedError(_( "Must implement get_backup_host")) diff --git a/cinder/scheduler/filter_scheduler.py b/cinder/scheduler/filter_scheduler.py index db53196e8c0..9c968777a78 100644 --- a/cinder/scheduler/filter_scheduler.py +++ b/cinder/scheduler/filter_scheduler.py @@ -22,7 +22,7 @@ Weighing Functions. from __future__ import annotations -from typing import Optional +from typing import (Optional, Union) # noqa: H301 from oslo_config import cfg from oslo_log import log as logging @@ -604,5 +604,10 @@ class FilterScheduler(driver.Scheduler): LOG.debug("Choosing %s", backend_state.backend_id) return top_backend - def get_backup_host(self, volume: objects.Volume, driver=None): - return self.host_manager.get_backup_host(volume, driver) + def get_backup_host(self, + volume: objects.Volume, + availability_zone: Union[str, None], + driver=None): + return self.host_manager.get_backup_host(volume, + availability_zone, + driver) diff --git a/cinder/scheduler/host_manager.py b/cinder/scheduler/host_manager.py index 6d340dd241e..c4155d03440 100644 --- a/cinder/scheduler/host_manager.py +++ b/cinder/scheduler/host_manager.py @@ -990,12 +990,26 @@ class HostManager(object): # we just convert them into string to compare them. return str(value) == str(capability) - def get_backup_host(self, volume: objects.Volume, driver=None) -> str: + def get_az(self, + volume: objects.Volume, + availability_zone: Union[str, None]) -> Union[str, None]: + if availability_zone: + az = availability_zone + elif volume: + az = volume.availability_zone + else: + az = None + return az + + def get_backup_host(self, + volume: objects.Volume, + availability_zone: Union[str, None], + driver=None) -> str: if volume: volume_host = volume_utils.extract_host(volume.host, 'host') else: volume_host = None - az = volume.availability_zone if volume else None + az = self.get_az(volume, availability_zone) return self._get_available_backup_service_host(volume_host, az, driver) def _get_any_available_backup_service(self, availability_zone, diff --git a/cinder/scheduler/manager.py b/cinder/scheduler/manager.py index c5842844357..bd4b8b64586 100644 --- a/cinder/scheduler/manager.py +++ b/cinder/scheduler/manager.py @@ -635,11 +635,14 @@ class SchedulerManager(manager.CleanableManager, manager.Manager): return requested, not_requested def create_backup(self, context, backup): + availability_zone = backup.availability_zone volume_id = backup.volume_id volume = self.db.volume_get(context, volume_id) try: + # Bug #1952805: an incremental backup will already have a host set, + # and we must respect it if not backup.host: - host = self.driver.get_backup_host(volume) + host = self.driver.get_backup_host(volume, availability_zone) backup.host = host backup.save() self.backup_api.create_backup(context, backup) diff --git a/cinder/tests/unit/backup/test_backup.py b/cinder/tests/unit/backup/test_backup.py index a85cd905d26..6e80f6f5a97 100644 --- a/cinder/tests/unit/backup/test_backup.py +++ b/cinder/tests/unit/backup/test_backup.py @@ -2295,3 +2295,28 @@ class BackupAPITestCase(BaseBackupTest): volume_id, None, incremental=is_incremental) mock_rollback.assert_called_with(self.ctxt, "fake_reservation") + + @mock.patch('cinder.scheduler.rpcapi.SchedulerAPI.create_backup') + @mock.patch.object(api.API, '_get_available_backup_service_host', + return_value='fake_host') + @mock.patch.object(quota.QUOTAS, 'rollback') + @mock.patch.object(quota.QUOTAS, 'reserve') + def test_create_backup_with_specified_az( + self, mock_reserve, mock_rollback, mock_get_service, + mock_create): + + self.ctxt.user_id = 'fake_user' + self.ctxt.project_id = 'fake_project' + mock_reserve.return_value = 'fake_reservation' + mock_get_service.return_value = 'host_1' + + volume_id = self._create_volume_db_entry(status='available', + host='testhost#rbd', + size=1) + backup = self.api.create(self.ctxt, + name='test', + description='test', + volume_id=volume_id, + container='test', + availability_zone='test_az') + self.assertEqual('test_az', backup.availability_zone) diff --git a/cinder/tests/unit/scheduler/test_scheduler.py b/cinder/tests/unit/scheduler/test_scheduler.py index c2c51a0c1e0..9133771bd10 100644 --- a/cinder/tests/unit/scheduler/test_scheduler.py +++ b/cinder/tests/unit/scheduler/test_scheduler.py @@ -29,6 +29,7 @@ from cinder import exception from cinder.message import message_field from cinder import objects from cinder.scheduler import driver +from cinder.scheduler import host_manager from cinder.scheduler import manager from cinder.tests.unit.backup import fake_backup from cinder.tests.unit import fake_constants as fake @@ -601,7 +602,7 @@ class SchedulerManagerTestCase(test.TestCase): self.manager.create_backup(self.context, backup=backup) mock_save.assert_called_once() - mock_host.assert_called_once_with(volume) + mock_host.assert_called_once_with(volume, None) mock_volume_get.assert_called_once_with(self.context, backup.volume_id) mock_create.assert_called_once_with(self.context, backup) @@ -642,7 +643,7 @@ class SchedulerManagerTestCase(test.TestCase): self.manager.create_backup(self.context, backup=backup) - mock_host.assert_called_once_with(volume) + mock_host.assert_called_once_with(volume, None) mock_volume_get.assert_called_once_with(self.context, backup.volume_id) mock_volume_update.assert_called_once_with( self.context, @@ -652,6 +653,14 @@ class SchedulerManagerTestCase(test.TestCase): mock_error.assert_called_once_with( backup, 'Service not found for creating backup.') + def test_get_az(self): + volume = fake_volume.fake_db_volume() + volume['status'] = 'backing-up' + volume['previous_status'] = 'available' + hm = host_manager.HostManager() + az = hm.get_az(volume, availability_zone='test_az') + self.assertEqual('test_az', az) + class SchedulerTestCase(test.TestCase): """Test case for base scheduler driver class.""" diff --git a/releasenotes/notes/bug-1912624-bakup-a-z-regression-452f4bc9dfd41871.yaml b/releasenotes/notes/bug-1912624-bakup-a-z-regression-452f4bc9dfd41871.yaml new file mode 100644 index 00000000000..28b9f8a74f5 --- /dev/null +++ b/releasenotes/notes/bug-1912624-bakup-a-z-regression-452f4bc9dfd41871.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + `Bug #1912624 `_: + Corrected regression introduced by the refactoring of the backup + service in the ussuri release, which prevented the creation of + a volume backup in a different availability zone.