[package_cache] Lock usage_mem_lock based on package id
Previously usage_cache incorrectly used the whole definition as key. This lead to incorrect deletion of package cache during downloads. This commit also updates the way errors during exclusive package are handled and updates test to include cleanup, deletion and non-deletion of cached packages Implements bp: murano-engine-package-cache Change-Id: I6567d60fe5094c4ff06a87d9392f71e319f93d9c
This commit is contained in:
Kirill Zaitsev
parent
763496d348
commit
93c5238f98
|
|
@ -270,31 +270,35 @@ class ApiPackageLoader(package_loader.MuranoPackageLoader):
|
|||
ipc_lock = m_utils.ExclusiveInterProcessLock(
|
||||
path=usage_lock_path, sleep_func=eventlet.sleep)
|
||||
|
||||
with usage_mem_locks[pkg_id].write_lock(False) as acquired:
|
||||
if not acquired:
|
||||
# the package is in use by other deployment in this process
|
||||
# will do nothing, someone else would delete it
|
||||
continue
|
||||
acquired_ipc_lock = ipc_lock.acquire(blocking=False)
|
||||
if not acquired_ipc_lock:
|
||||
# the package is in use by other deployment in another
|
||||
# process, will do nothing, someone else would delete it
|
||||
continue
|
||||
try:
|
||||
with usage_mem_locks[pkg_id].write_lock(False) as acquired:
|
||||
if not acquired:
|
||||
# the package is in use by other deployment in this
|
||||
# process will do nothing, someone else would delete it
|
||||
continue
|
||||
acquired_ipc_lock = ipc_lock.acquire(blocking=False)
|
||||
if not acquired_ipc_lock:
|
||||
# the package is in use by other deployment in another
|
||||
# process, will do nothing, someone else would delete
|
||||
continue
|
||||
|
||||
shutil.rmtree(stale_directory,
|
||||
ignore_errors=True)
|
||||
ipc_lock.release()
|
||||
shutil.rmtree(stale_directory,
|
||||
ignore_errors=True)
|
||||
ipc_lock.release()
|
||||
|
||||
for lock_type in ('usage', 'download'):
|
||||
lock_path = os.path.join(
|
||||
self._cache_directory,
|
||||
'{}_{}.lock'.format(pkg_id, lock_type))
|
||||
try:
|
||||
os.remove(lock_path)
|
||||
except OSError:
|
||||
LOG.warning(
|
||||
_LW("Couldn't delete lock file: "
|
||||
"{}").format(lock_path))
|
||||
for lock_type in ('usage', 'download'):
|
||||
lock_path = os.path.join(
|
||||
self._cache_directory,
|
||||
'{}_{}.lock'.format(pkg_id, lock_type))
|
||||
try:
|
||||
os.remove(lock_path)
|
||||
except OSError:
|
||||
LOG.warning(
|
||||
_LW("Couldn't delete lock file: "
|
||||
"{}").format(lock_path))
|
||||
except RuntimeError:
|
||||
# couldn't upgrade read lock to write-lock. go on.
|
||||
continue
|
||||
|
||||
def _get_best_package_match(self, packages):
|
||||
public = None
|
||||
|
|
@ -318,11 +322,12 @@ class ApiPackageLoader(package_loader.MuranoPackageLoader):
|
|||
if not CONF.packages_opts.enable_packages_cache:
|
||||
return
|
||||
|
||||
package_id = package_definition.id
|
||||
|
||||
# A work around the fact that read_lock only supports `with` syntax.
|
||||
mem_lock = _with_to_generator(
|
||||
usage_mem_locks[package_definition].read_lock())
|
||||
usage_mem_locks[package_id].read_lock())
|
||||
|
||||
package_id = package_definition.id
|
||||
usage_lock_path = os.path.join(self._cache_directory,
|
||||
'{}_usage.lock'.format(package_id))
|
||||
ipc_lock = m_utils.SharedInterProcessLock(
|
||||
|
|
|
|||
|
|
@ -57,10 +57,10 @@ class TestPackageCache(base.MuranoTestCase):
|
|||
package_data = f.read()
|
||||
spec = semantic_version.Spec('*')
|
||||
|
||||
old_id, new_id = '123', '456'
|
||||
first_id, second_id, third_id = '123', '456', '789'
|
||||
package = mock.MagicMock()
|
||||
package.fully_qualified_name = fqn
|
||||
package.id = old_id
|
||||
package.id = first_id
|
||||
package.version = '0.0.1'
|
||||
|
||||
self.murano_client.packages.filter = mock.MagicMock(
|
||||
|
|
@ -77,9 +77,9 @@ class TestPackageCache(base.MuranoTestCase):
|
|||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version)))
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, old_id)))
|
||||
self.location, fqn, package.version, first_id)))
|
||||
self.assertTrue(os.path.isfile(os.path.join(
|
||||
self.location, fqn, package.version, old_id, 'manifest.yaml')))
|
||||
self.location, fqn, package.version, first_id, 'manifest.yaml')))
|
||||
|
||||
# assert, that we called download
|
||||
self.assertEqual(self.murano_client.packages.download.call_count, 1)
|
||||
|
|
@ -93,7 +93,7 @@ class TestPackageCache(base.MuranoTestCase):
|
|||
self.assertEqual(self.murano_client.packages.download.call_count, 1)
|
||||
|
||||
# changing id, new package would be downloaded.
|
||||
package.id = new_id
|
||||
package.id = second_id
|
||||
self.loader._package_cache = {}
|
||||
self.loader._class_cache = {}
|
||||
self.loader.load_class_package(fqn, spec)
|
||||
|
|
@ -101,9 +101,10 @@ class TestPackageCache(base.MuranoTestCase):
|
|||
# check that we didn't download a thing
|
||||
self.assertEqual(self.murano_client.packages.download.call_count, 2)
|
||||
|
||||
# check that old directories got deleted
|
||||
self.assertFalse(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, old_id)))
|
||||
# check that old directories were not deleted
|
||||
# we did not call cleanup and did not release the locks
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, first_id)))
|
||||
|
||||
# check that new directories got created correctly
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
|
|
@ -111,14 +112,38 @@ class TestPackageCache(base.MuranoTestCase):
|
|||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version)))
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, new_id)))
|
||||
self.location, fqn, package.version, second_id)))
|
||||
self.assertTrue(os.path.isfile(os.path.join(
|
||||
self.location, fqn, package.version, new_id, 'manifest.yaml')))
|
||||
self.location, fqn, package.version, second_id, 'manifest.yaml')))
|
||||
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version)))
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, new_id)))
|
||||
self.location, fqn, package.version, second_id)))
|
||||
|
||||
# changing id, new package would be downloaded.
|
||||
package.id = third_id
|
||||
self.loader._package_cache = {}
|
||||
self.loader._class_cache = {}
|
||||
|
||||
# release all the locks
|
||||
self.loader.cleanup()
|
||||
self.loader.load_class_package(fqn, spec)
|
||||
|
||||
# check that we didn't download a thing
|
||||
self.assertEqual(self.murano_client.packages.download.call_count, 3)
|
||||
|
||||
# check that old directories were *deleted*
|
||||
self.assertFalse(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, first_id)))
|
||||
self.assertFalse(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, second_id)))
|
||||
|
||||
# check that new directories got created correctly
|
||||
self.assertTrue(os.path.isdir(os.path.join(
|
||||
self.location, fqn, package.version, third_id)))
|
||||
self.assertTrue(os.path.isfile(os.path.join(
|
||||
self.location, fqn, package.version, third_id, 'manifest.yaml')))
|
||||
|
||||
|
||||
class TestCombinedPackageLoader(base.MuranoTestCase):
|
||||
|
|
|
|||
Loading…
Reference in New Issue