From d4c5fa95e5c06cac63f3e26e3d1c85ccc36ccfdf Mon Sep 17 00:00:00 2001 From: Sean McGinnis Date: Mon, 16 Jul 2018 14:24:47 -0500 Subject: [PATCH] Address multi store nits This addresses some nits from I1f2e8fa61d6dfecd8395a1f894f74ec5bcb5573c that were not worth holding up that review to address. Change-Id: Iab60d5622dddf29871fbbc9f51deb93de26235e8 --- glance_store/location.py | 6 +- glance_store/multi_backend.py | 38 ++++----- .../tests/unit/test_multistore_filesystem.py | 83 ++++++++----------- 3 files changed, 57 insertions(+), 70 deletions(-) diff --git a/glance_store/location.py b/glance_store/location.py index 4084179a..c9cf776f 100644 --- a/glance_store/location.py +++ b/glance_store/location.py @@ -79,7 +79,8 @@ def get_location_from_uri(uri, conf=CONF): def get_location_from_uri_and_backend(uri, backend, conf=CONF): - """ + """Extract backend location from a URI. + Given a URI, return a Location object that has had an appropriate store parse the URI. @@ -112,7 +113,8 @@ def get_location_from_uri_and_backend(uri, backend, conf=CONF): def register_scheme_backend_map(scheme_map): - """ + """Registers a mapping between a scheme and a backend. + Given a mapping of 'scheme' to store_name, adds the mapping to the known list of schemes. diff --git a/glance_store/multi_backend.py b/glance_store/multi_backend.py index f8b37085..cc6aae50 100644 --- a/glance_store/multi_backend.py +++ b/glance_store/multi_backend.py @@ -23,7 +23,7 @@ from stevedore import extension from glance_store import capabilities from glance_store import exceptions -from glance_store.i18n import _, _LW +from glance_store.i18n import _ from glance_store import location @@ -123,7 +123,7 @@ def _list_driver_opts(): def register_store_opts(conf): - LOG.debug("Registering options for group %s" % _STORE_CFG_GROUP) + LOG.debug("Registering options for group %s", _STORE_CFG_GROUP) conf.register_opts(_STORE_OPTS, group=_STORE_CFG_GROUP) driver_opts = _list_driver_opts() @@ -133,7 +133,7 @@ def register_store_opts(conf): if enabled_backends[backend] not in opt_list: continue - LOG.debug("Registering options for group %s" % backend) + LOG.debug("Registering options for group %s", backend) conf.register_opts(driver_opts[opt_list], group=backend) @@ -153,7 +153,7 @@ def _load_multi_store(conf, store_entry, return mgr.driver except RuntimeError as e: LOG.warning("Failed to load driver %(driver)s. The " - "driver will be disabled" % dict(driver=str([driver, e]))) + "driver will be disabled", dict(driver=str([driver, e]))) def _load_multi_stores(conf): @@ -176,10 +176,7 @@ def _load_multi_stores(conf): def create_multi_stores(conf=CONF): - """ - Registers all store modules and all schemes - from the given config. - """ + """Registers all store modules and all schemes from the given config.""" store_count = 0 scheme_map = {} for (store_entry, store_instance, @@ -191,9 +188,9 @@ def create_multi_stores(conf=CONF): continue if not schemes: - raise exceptions.BackendException('Unable to register store %s. ' - 'No schemes associated with it.' - % store_entry) + raise exceptions.BackendException( + _('Unable to register store %s. No schemes associated ' + 'with it.') % store_entry) else: LOG.debug("Registering store %s with schemes %s", store_entry, schemes) @@ -227,7 +224,8 @@ def verify_store(): def get_store_from_store_identifier(store_identifier): - """ + """Determine backing store from identifier. + Given a store identifier, return the appropriate store object for handling that scheme. """ @@ -282,9 +280,9 @@ def add(conf, image_id, data, size, backend, context=None, def store_add_to_backend(image_id, data, size, store, context=None, verifier=None): - """ - A wrapper around a call to each stores add() method. This gives glance - a common place to check the output + """A wrapper around a call to each stores add() method. + + This gives glance a common place to check the output. :param image_id: The image add to which data is added :param data: The data to be stored @@ -349,9 +347,8 @@ def delete(uri, backend, context=None): store = get_store_from_store_identifier(backend) return store.delete(loc, context=context) - msg = _LW('Backend is not set to image, searching ' - 'all backends based on location URI.') - LOG.warn(msg) + LOG.warning('Backend is not set to image, searching all backends based on ' + 'location URI.') backends = CONF.enabled_backends for backend in backends: @@ -400,9 +397,8 @@ def get(uri, backend, offset=0, chunk_size=None, context=None): chunk_size=chunk_size, context=context) - msg = _LW('Backend is not set to image, searching ' - 'all backends based on location URI.') - LOG.warn(msg) + LOG.warning('Backend is not set to image, searching all backends based on ' + 'location URI.') backends = CONF.enabled_backends for backend in backends: diff --git a/glance_store/tests/unit/test_multistore_filesystem.py b/glance_store/tests/unit/test_multistore_filesystem.py index abf99c4d..a9ca3313 100644 --- a/glance_store/tests/unit/test_multistore_filesystem.py +++ b/glance_store/tests/unit/test_multistore_filesystem.py @@ -171,10 +171,7 @@ class TestMultiStore(base.MultiStoreBaseTest, self.assertEqual(chunk_size, image_size) def test_get_non_existing(self): - """ - Test that trying to retrieve a file that doesn't exist - raises an error - """ + """Test trying to retrieve a file that doesn't exist raises error.""" loc = location.get_location_from_uri_and_backend( "file:///%s/non-existing" % self.test_dir, 'file1', conf=self.conf) self.assertRaises(exceptions.NotFound, @@ -182,10 +179,7 @@ class TestMultiStore(base.MultiStoreBaseTest, loc) def test_get_non_existing_identifier(self): - """ - Test that trying to retrieve a store that doesn't exist - raises an error - """ + """Test trying to retrieve a store that doesn't exist raises error.""" self.assertRaises(exceptions.UnknownScheme, location.get_location_from_uri_and_backend, "file:///%s/non-existing" % self.test_dir, @@ -341,39 +335,36 @@ class TestMultiStore(base.MultiStoreBaseTest, self.assertFalse(os.path.exists(path)) def test_add_storage_full(self): - """ + """Tests adding an image without enough space. + Tests that adding an image without enough space on disk - raises an appropriate exception + raises an appropriate exception. """ self._do_test_add_write_failure(errno.ENOSPC, exceptions.StorageFull) def test_add_file_too_big(self): - """ + """Tests adding a very large image. + Tests that adding an excessively large image file - raises an appropriate exception + raises an appropriate exception. """ self._do_test_add_write_failure(errno.EFBIG, exceptions.StorageFull) def test_add_storage_write_denied(self): - """ + """Tests adding an image without store permissions. + Tests that adding an image with insufficient filestore permissions - raises an appropriate exception + raises an appropriate exception. """ self._do_test_add_write_failure(errno.EACCES, exceptions.StorageWriteDenied) def test_add_other_failure(self): - """ - Tests that a non-space-related IOError does not raise a - StorageFull exceptions. - """ + """Tests other IOErrors do not raise a StorageFull exception.""" self._do_test_add_write_failure(errno.ENOTDIR, IOError) def test_add_cleanup_on_read_failure(self): - """ - Tests the partial image file is cleaned up after a read - failure. - """ + """Tests partial image is cleaned up after a read failure.""" filesystem.ChunkedFile.CHUNKSIZE = units.Ki image_id = str(uuid.uuid4()) file_size = 5 * units.Ki # 5K @@ -393,9 +384,7 @@ class TestMultiStore(base.MultiStoreBaseTest, self.assertFalse(os.path.exists(path)) def test_delete(self): - """ - Test we can delete an existing image in the filesystem store - """ + """Test we can delete an existing image in the filesystem store.""" # First add an image image_id = str(uuid.uuid4()) file_size = 5 * units.Ki # 5K @@ -416,10 +405,7 @@ class TestMultiStore(base.MultiStoreBaseTest, self.assertRaises(exceptions.NotFound, self.store.get, loc) def test_delete_non_existing(self): - """ - Test that trying to delete a file that doesn't exist - raises an error - """ + """Test deleting file that doesn't exist raises an error.""" loc = location.get_location_from_uri_and_backend( "file:///tmp/glance-tests/non-existing", "file1", conf=self.conf) self.assertRaises(exceptions.NotFound, @@ -427,10 +413,7 @@ class TestMultiStore(base.MultiStoreBaseTest, loc) def test_delete_forbidden(self): - """ - Tests that trying to delete a file without permissions - raises the correct error - """ + """Tests deleting file without permissions raises the correct error.""" # First add an image image_id = str(uuid.uuid4()) file_size = 5 * units.Ki # 5K @@ -463,10 +446,7 @@ class TestMultiStore(base.MultiStoreBaseTest, self.store.get(loc) def test_configure_add_with_multi_datadirs(self): - """ - Tests multiple filesystem specified by filesystem_store_datadirs - are parsed correctly. - """ + """Test multiple filesystems are parsed correctly.""" store_map = [self.useFixture(fixtures.TempDir()).path, self.useFixture(fixtures.TempDir()).path] self.conf.set_override('filesystem_store_datadir', @@ -560,9 +540,11 @@ class TestMultiStore(base.MultiStoreBaseTest, self.store.configure_add) def test_configure_add_same_dir_multiple_times(self): - """ + """Tests handling of same dir in config multiple times. + Tests BadStoreConfiguration exception is raised if same directory - is specified multiple times in filesystem_store_datadirs. + is specified multiple times in filesystem_store_datadirs with different + priorities. """ store_map = [self.useFixture(fixtures.TempDir()).path, self.useFixture(fixtures.TempDir()).path] @@ -577,9 +559,11 @@ class TestMultiStore(base.MultiStoreBaseTest, self.store.configure_add) def test_configure_add_same_dir_multiple_times_same_priority(self): - """ + """Tests handling of same dir in config multiple times. + Tests BadStoreConfiguration exception is raised if same directory - is specified multiple times in filesystem_store_datadirs. + is specified multiple times in filesystem_store_datadirs with the same + priority. """ store_map = [self.useFixture(fixtures.TempDir()).path, self.useFixture(fixtures.TempDir()).path] @@ -676,7 +660,8 @@ class TestMultiStore(base.MultiStoreBaseTest, self.assertEqual(expected_file_size, new_image_file_size) def test_add_with_multiple_dirs_storage_full(self): - """ + """Tests adding dirs with storage full. + Test StorageFull exception is raised if no filesystem directory is found that can store an image. """ @@ -709,7 +694,8 @@ class TestMultiStore(base.MultiStoreBaseTest, expected_file_size) def test_configure_add_with_file_perm(self): - """ + """Tests adding with permissions. + Tests filesystem specified by filesystem_store_file_perm are parsed correctly. """ @@ -721,8 +707,9 @@ class TestMultiStore(base.MultiStoreBaseTest, self.store.configure_add() self.assertEqual(self.store.datadir, store) - def test_configure_add_with_unaccessible_file_perm(self): - """ + def test_configure_add_with_inaccessible_file_perm(self): + """Tests adding with inaccessible file permissions. + Tests BadStoreConfiguration exception is raised if an invalid file permission specified in filesystem_store_file_perm. """ @@ -735,7 +722,8 @@ class TestMultiStore(base.MultiStoreBaseTest, self.store.configure_add) def test_add_with_file_perm_for_group_other_users_access(self): - """ + """Tests adding image with file permissions. + Test that we can add an image via the filesystem backend with a required image file permission. """ @@ -778,7 +766,8 @@ class TestMultiStore(base.MultiStoreBaseTest, self.assertEqual(perm, stat.S_IMODE(mode)) def test_add_with_file_perm_for_owner_users_access(self): - """ + """Tests adding image with file permissions. + Test that we can add an image via the filesystem backend with a required image file permission. """