[ARVADOS] updated: 3180bb6fc5334e99d14fe6f3c123326c9891ccba
git at public.curoverse.com
git at public.curoverse.com
Thu May 7 17:18:21 EDT 2015
Summary of changes:
services/dockercleaner/arvados_docker/cleaner.py | 25 ++++++-----
services/dockercleaner/tests/test_cleaner.py | 53 ++++++++++++++++--------
2 files changed, 50 insertions(+), 28 deletions(-)
via 3180bb6fc5334e99d14fe6f3c123326c9891ccba (commit)
from 80e1bb8d933e4d0ae18951840d57b27d23b40f7b (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit 3180bb6fc5334e99d14fe6f3c123326c9891ccba
Author: Brett Smith <brett at curoverse.com>
Date: Thu May 7 17:18:15 2015 -0400
3793: Always keep one Docker image.
diff --git a/services/dockercleaner/arvados_docker/cleaner.py b/services/dockercleaner/arvados_docker/cleaner.py
index 54f23f6..bd941a1 100644
--- a/services/dockercleaner/arvados_docker/cleaner.py
+++ b/services/dockercleaner/arvados_docker/cleaner.py
@@ -91,20 +91,23 @@ class DockerImages:
logger.debug("Unregistered container %s", cid)
def should_delete(self):
- # Build a set of images in use, and figure out how much space we have
- # left for Docker images after keeping them.
- used_images = set(self.container_image_map.values())
- space_left = (self.target_size - sum(self.images[image_id].size
- for image_id in used_images))
- # Build a list of images not in use, ordered by use time.
- lru_images = [image for image in self.images.values()
- if image.docker_id not in used_images]
+ if not self.images:
+ return
+ # Build a list of images, ordered by use time.
+ lru_images = list(self.images.values())
lru_images.sort(key=lambda image: image.last_used)
+ # Make sure we don't delete any images in use, or if there are
+ # none, the most recently used image.
+ if self.container_image_map:
+ keep_ids = set(self.container_image_map.values())
+ else:
+ keep_ids = {lru_images[-1].docker_id}
+ space_left = (self.target_size - sum(self.images[image_id].size
+ for image_id in keep_ids))
# Go through the list most recently used first, and note which
- # images can be saved with the space available.
- keep_ids = set()
+ # images can be saved with the space allotted.
for image in reversed(lru_images):
- if image.size <= space_left:
+ if (image.docker_id not in keep_ids) and (image.size <= space_left):
keep_ids.add(image.docker_id)
space_left -= image.size
# Yield the Docker IDs of any image we don't want to save, least
diff --git a/services/dockercleaner/tests/test_cleaner.py b/services/dockercleaner/tests/test_cleaner.py
index c8dccbb..6572d8a 100644
--- a/services/dockercleaner/tests/test_cleaner.py
+++ b/services/dockercleaner/tests/test_cleaner.py
@@ -106,15 +106,32 @@ class DockerImagesTestCase(unittest.TestCase):
images.del_image(MockDockerId())
self.assertTrue(images.has_image(self.mock_images[0]['Id']))
- def test_images_under_target_not_deletable(self):
- images = self.setup_images(200, 100, 300, target_size=150)
- self.assertEqual({self.mock_images[ii]['Id'] for ii in (0, 2)},
- set(images.should_delete()))
-
- def test_all_images_deletable(self):
+ def test_one_image_always_kept(self):
+ # When crunch-job starts a job, it makes sure each compute node
+ # has the Docker image loaded, then it runs all the tasks with
+ # the assumption the image is on each node. As long as that's
+ # true, the cleaner should avoid removing every installed image:
+ # crunch-job might be counting on the most recent one to be
+ # available, even if it's not currently in use.
images = self.setup_images(None, None, target_size=1)
- self.assertEqual({image['Id'] for image in self.mock_images},
- set(images.should_delete()))
+ for use_time, image in enumerate(self.mock_images, 1):
+ user = MockContainer(image)
+ images.add_user(user, use_time)
+ images.end_user(user['Id'])
+ self.assertEqual([self.mock_images[0]['Id']],
+ list(images.should_delete()))
+
+ def test_images_under_target_not_deletable(self):
+ # The images are used in this order. target_size is set so it
+ # could hold the largest image, but not after the most recently
+ # used image is kept; then we have to fall back to the previous one.
+ images = self.setup_images(20, 30, 40, 10, target_size=45)
+ for use_time, image in enumerate(self.mock_images, 1):
+ user = MockContainer(image)
+ images.add_user(user, use_time)
+ images.end_user(user['Id'])
+ self.assertEqual([self.mock_images[ii]['Id'] for ii in [0, 2]],
+ list(images.should_delete()))
def test_images_in_use_not_deletable(self):
images = self.setup_images(None, None, target_size=1)
@@ -126,11 +143,12 @@ class DockerImagesTestCase(unittest.TestCase):
list(images.should_delete()))
def test_image_deletable_after_unused(self):
- images = self.setup_images(None, target_size=1)
- user = MockContainer(self.mock_images[-1])
- images.add_user(user, 1)
- images.end_user(user['Id'])
- self.assertEqual([self.mock_images[-1]['Id']],
+ images = self.setup_images(None, None, target_size=1)
+ users = [MockContainer(image) for image in self.mock_images]
+ images.add_user(users[0], 1)
+ images.add_user(users[1], 2)
+ images.end_user(users[0]['Id'])
+ self.assertEqual([self.mock_images[0]['Id']],
list(images.should_delete()))
def test_image_not_deletable_if_user_restarts(self):
@@ -150,12 +168,13 @@ class DockerImagesTestCase(unittest.TestCase):
self.assertEqual([], list(images.should_delete()))
def test_image_deletable_after_all_users_end(self):
- images = self.setup_images(None, target_size=1)
- users = [MockContainer(self.mock_images[0]) for ii in range(2)]
+ images = self.setup_images(None, None, target_size=1)
+ users = [MockContainer(self.mock_images[ii]) for ii in [0, 1, 1]]
images.add_user(users[0], 1)
images.add_user(users[1], 2)
- images.end_user(users[0]['Id'])
+ images.add_user(users[2], 3)
images.end_user(users[1]['Id'])
+ images.end_user(users[2]['Id'])
self.assertEqual([self.mock_images[-1]['Id']],
list(images.should_delete()))
@@ -167,7 +186,7 @@ class DockerImagesTestCase(unittest.TestCase):
images.add_user(users[2], 2)
for user in users:
images.end_user(user['Id'])
- self.assertEqual([self.mock_images[ii]['Id'] for ii in (1, 2, 0)],
+ self.assertEqual([self.mock_images[ii]['Id'] for ii in [1, 2]],
list(images.should_delete()))
def test_adding_user_without_image_does_not_implicitly_add_image(self):
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list