From 0f5b6a07d0659af9bdce37813faec7f4b922f2ca Mon Sep 17 00:00:00 2001 From: "afanasev.s" Date: Tue, 21 Dec 2021 23:45:46 +0300 Subject: [PATCH] Fix incorrect logging format Fix incorrect logging format for multiple variables because of what this functionality didn't work correctly and some log messages were skipped. The logging calls require two arguments, but they are passed in a tuple so it's interpreted as one argument only and it fails as is missing the second argument. Closes-Bug: 2110149 Change-Id: I74ed44134b50782c105a0e82f3af34a5fa45d119 --- watcher/common/cinder_helper.py | 13 +++--- watcher/common/nova_helper.py | 4 +- watcher/common/placement_helper.py | 2 +- watcher/tests/common/test_cinder_helper.py | 45 +++++-------------- watcher/tests/common/test_nova_helper.py | 9 +--- watcher/tests/common/test_placement_helper.py | 9 +--- 6 files changed, 24 insertions(+), 58 deletions(-) diff --git a/watcher/common/cinder_helper.py b/watcher/common/cinder_helper.py index 1790613f3..a686cd67d 100644 --- a/watcher/common/cinder_helper.py +++ b/watcher/common/cinder_helper.py @@ -179,8 +179,9 @@ class CinderHelper(object): LOG.error(error_msg) return False LOG.debug( - "Volume migration succeeded : volume %s is now on host '%s'.", ( - volume.id, host_name)) + "Volume migration succeeded : " + "volume %(volume)s is now on host '%(host)s'.", + {'volume': volume.id, 'host': host_name}) return True def migrate(self, volume, dest_node): @@ -193,8 +194,8 @@ class CinderHelper(object): message=(_("Volume type must be same for migrating"))) source_node = getattr(volume, 'os-vol-host-attr:host') - LOG.debug("Volume %s found on host '%s'.", - (volume.id, source_node)) + LOG.debug("Volume %(volume)s found on host '%(host)s'.", + {'volume': volume.id, 'host': source_node}) self.cinder.volumes.migrate_volume( volume, dest_node, False, True) @@ -210,8 +211,8 @@ class CinderHelper(object): source_node = getattr(volume, 'os-vol-host-attr:host') LOG.debug( - "Volume %s found on host '%s'.", - (volume.id, source_node)) + "Volume %(volume)s found on host '%(host)s'.", + {'volume': volume.id, 'host': source_node}) self.cinder.volumes.retype( volume, dest_type, "on-demand") diff --git a/watcher/common/nova_helper.py b/watcher/common/nova_helper.py index 7417bcf2c..e0635dcb8 100644 --- a/watcher/common/nova_helper.py +++ b/watcher/common/nova_helper.py @@ -731,8 +731,8 @@ class NovaHelper(object): host_name = getattr(new_volume, "os-vol-host-attr:host") LOG.debug( "Volume update succeeded : " - "Volume %s is now on host '%s'.", - (new_volume.id, host_name)) + "Volume %(volume)s is now on host '%(host)s'.", + {'volume': new_volume.id, 'host': host_name}) return True def _check_nova_api_version(self, client, version): diff --git a/watcher/common/placement_helper.py b/watcher/common/placement_helper.py index c6cf14705..c4ecdc174 100644 --- a/watcher/common/placement_helper.py +++ b/watcher/common/placement_helper.py @@ -122,7 +122,7 @@ class PlacementHelper(object): if resp.status_code == HTTPStatus.OK: json = resp.json() return json['allocations'] - msg = ("Failed to get allocations for consumer %(c_uuid). " + msg = ("Failed to get allocations for consumer %(c_uuid)s. " "Got %(status_code)d: %(err_text)s.") args = { 'c_uuid': consumer_uuid, diff --git a/watcher/tests/common/test_cinder_helper.py b/watcher/tests/common/test_cinder_helper.py index b07bd0fb0..5334d08f2 100644 --- a/watcher/tests/common/test_cinder_helper.py +++ b/watcher/tests/common/test_cinder_helper.py @@ -154,13 +154,8 @@ class TestCinderHelper(base.TestCase): volume_type = self.fake_volume_type() cinder_util.cinder.volume_types.list.return_value = [volume_type] - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559 merges - self.assertRaises(TypeError, cinder_util.migrate, - volume, 'host@backend#pool') - # result = cinder_util.migrate(volume, 'host@backend#pool') - # self.assertTrue(result) + result = cinder_util.migrate(volume, 'host@backend#pool') + self.assertTrue(result) @mock.patch.object(time, 'sleep', mock.Mock()) def test_migrate_fail(self, mock_cinder): @@ -187,14 +182,8 @@ class TestCinderHelper(base.TestCase): volume_type = self.fake_volume_type() cinder_util.cinder.volume_types.list.return_value = [volume_type] - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559 merges - self.assertRaises(TypeError, cinder_util.migrate, - volume, 'host@backend#pool') - - # result = cinder_util.migrate(volume, 'host@backend#pool') - # self.assertFalse(result) + result = cinder_util.migrate(volume, 'host@backend#pool') + self.assertFalse(result) @mock.patch.object(time, 'sleep', mock.Mock()) def test_retype_success(self, mock_cinder): @@ -205,13 +194,8 @@ class TestCinderHelper(base.TestCase): setattr(volume, 'migration_status', 'success') cinder_util.cinder.volumes.get.return_value = volume - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559 merges - self.assertRaises(TypeError, cinder_util.retype, - volume, 'notfake_type') - # result = cinder_util.retype(volume, 'notfake_type') - # self.assertTrue(result) + result = cinder_util.retype(volume, 'notfake_type') + self.assertTrue(result) @mock.patch.object(time, 'sleep', mock.Mock()) def test_retype_fail(self, mock_cinder): @@ -232,13 +216,8 @@ class TestCinderHelper(base.TestCase): setattr(volume, 'migration_status', 'error') cinder_util.cinder.volumes.get.return_value = volume - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559 merges - self.assertRaises(TypeError, cinder_util.retype, - volume, 'notfake_type') - # result = cinder_util.retype(volume, 'notfake_type') - # self.assertFalse(result) + result = cinder_util.retype(volume, 'notfake_type') + self.assertFalse(result) @mock.patch.object(time, 'sleep', mock.Mock()) def test_create_volume_success(self, mock_cinder): @@ -403,12 +382,8 @@ class TestCinderHelper(base.TestCase): setattr(volume, 'os-vol-host-attr:host', 'host@backend#pool') cinder_util.cinder.volumes.get.return_value = volume cinder_util.check_volume_deleted = mock.MagicMock(return_value=True) - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559 merges - self.assertRaises(TypeError, cinder_util.check_migrated, volume) - # result = cinder_util.check_migrated(volume) - # self.assertTrue(result) + result = cinder_util.check_migrated(volume) + self.assertTrue(result) @mock.patch.object(time, 'sleep', mock.Mock()) def test_check_migrated_fail(self, mock_cinder): diff --git a/watcher/tests/common/test_nova_helper.py b/watcher/tests/common/test_nova_helper.py index c8b9a24e4..5e6401516 100644 --- a/watcher/tests/common/test_nova_helper.py +++ b/watcher/tests/common/test_nova_helper.py @@ -576,13 +576,8 @@ class TestNovaHelper(base.TestCase): new_volume = self.fake_volume( id=utils.generate_uuid(), status='in-use') - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559/7 merges - self.assertRaises(TypeError, nova_util.swap_volume, - old_volume, new_volume) - # result = nova_util.swap_volume(old_volume, new_volume) - # self.assertTrue(result) + result = nova_util.swap_volume(old_volume, new_volume) + self.assertTrue(result) # verify that the method will return False when the status of # new_volume is 'fake-use'. diff --git a/watcher/tests/common/test_placement_helper.py b/watcher/tests/common/test_placement_helper.py index 6a2b1667b..5b7a18e2b 100644 --- a/watcher/tests/common/test_placement_helper.py +++ b/watcher/tests/common/test_placement_helper.py @@ -233,13 +233,8 @@ class TestPlacementHelper(base.TestCase): kss_req.return_value = fake_requests.FakeResponse( HTTPStatus.NOT_FOUND, content=jsonutils.dump_as_bytes(self.fake_err_msg)) - # the logging in this function has a bug, temporarily changing the - # assert to catch the exception - # https://review.opendev.org/c/openstack/watcher/+/822559 merges - self.assertRaises(ValueError, self.client.get_allocations_for_consumer, - c_uuid) - # result = self.client.get_allocations_for_consumer(c_uuid) - # self.assertIsNone(result) + result = self.client.get_allocations_for_consumer(c_uuid) + self.assertIsNone(result) def test_get_usages_for_resource_provider_OK(self, kss_req): rp_uuid = uuidutils.generate_uuid()