[ARVADOS] updated: df5a73a8e8ebff9ab6c49d6ea41e62740f0f20dd
git at public.curoverse.com
git at public.curoverse.com
Tue May 5 10:24:20 EDT 2015
Summary of changes:
services/dockercleaner/arvados_docker/cleaner.py | 18 ++--
services/dockercleaner/tests/test_cleaner.py | 125 +++++++++--------------
2 files changed, 59 insertions(+), 84 deletions(-)
via df5a73a8e8ebff9ab6c49d6ea41e62740f0f20dd (commit)
from 4dc84fdc1f380b9d796308972648e6e36299684a (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 df5a73a8e8ebff9ab6c49d6ea41e62740f0f20dd
Author: Brett Smith <brett at curoverse.com>
Date: Tue May 5 10:24:17 2015 -0400
3793: Fixups from code review.
diff --git a/services/dockercleaner/arvados_docker/cleaner.py b/services/dockercleaner/arvados_docker/cleaner.py
index a87741d..aa46c1a 100644
--- a/services/dockercleaner/arvados_docker/cleaner.py
+++ b/services/dockercleaner/arvados_docker/cleaner.py
@@ -9,10 +9,10 @@ import argparse
import collections
import copy
import functools
-import logging
import json
-import time
+import logging
import sys
+import time
import docker
@@ -87,21 +87,25 @@ class DockerImages:
self.container_image_map.pop(cid, None)
logger.debug("Unregistered container %s", cid)
- def any_users(self):
- return bool(self.container_image_map)
-
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]
lru_images.sort(key=lambda image: image.last_used)
+ # Go through the list most recently used first, and note which
+ # images can be saved with the space available.
keep_ids = set()
for image in reversed(lru_images):
if 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
+ # recently used first.
for image in lru_images:
if image.docker_id not in keep_ids:
yield image.docker_id
@@ -179,8 +183,6 @@ class DockerImageCleaner(DockerImageUseRecorder):
@event_handlers.on('destroy')
def clean_images(self, event=None):
- if self.images.any_users():
- return
for image_id in self.images.should_delete():
try:
self.docker_client.remove_image(image_id)
@@ -202,7 +204,7 @@ def human_size(size_str):
def parse_arguments(arguments):
parser = argparse.ArgumentParser(
- prog="dockerclean",
+ prog="arvados_docker.cleaner",
description="clean old Docker images from Arvados compute nodes")
parser.add_argument(
'--keep', action='store', type=human_size, required=True,
diff --git a/services/dockercleaner/tests/test_cleaner.py b/services/dockercleaner/tests/test_cleaner.py
index 818cc9d..1f03681 100644
--- a/services/dockercleaner/tests/test_cleaner.py
+++ b/services/dockercleaner/tests/test_cleaner.py
@@ -101,93 +101,57 @@ class DockerImagesTestCase(unittest.TestCase):
images.del_image(MockDockerId())
self.assertTrue(images.has_image(self.mock_images[0]['Id']))
- def test_no_users_at_start(self):
- images = self.setup_images(None)
- self.assertFalse(images.any_users())
+ 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_users_recorded(self):
- images = self.setup_images(None)
- images.add_user(MockContainer(self.mock_images[-1]), 1)
- self.assertTrue(images.any_users())
+ def test_all_images_deletable(self):
+ images = self.setup_images(None, None, target_size=1)
+ self.assertEqual({image['Id'] for image in self.mock_images},
+ set(images.should_delete()))
- def test_users_unrecorded(self):
- images = self.setup_images(None)
+ def test_images_in_use_not_deletable(self):
+ 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[1]['Id'])
+ self.assertEqual([self.mock_images[1]['Id']],
+ 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.assertFalse(images.any_users())
+ self.assertEqual([self.mock_images[-1]['Id']],
+ list(images.should_delete()))
- def test_users_can_restart(self):
- images = self.setup_images(None)
+ def test_image_not_deletable_if_user_restarts(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'])
images.add_user(user, 2)
- self.assertTrue(images.any_users())
+ self.assertEqual([], list(images.should_delete()))
- def test_multiple_users(self):
- images = self.setup_images(None, None)
- users = [MockContainer(image) for image in self.mock_images]
+ def test_image_not_deletable_if_any_user_remains(self):
+ images = self.setup_images(None, target_size=1)
+ users = [MockContainer(self.mock_images[0]) for ii in range(2)]
images.add_user(users[0], 1)
images.add_user(users[1], 2)
images.end_user(users[0]['Id'])
- self.assertTrue(images.any_users())
- images.end_user(users[1]['Id'])
- self.assertFalse(images.any_users())
+ self.assertEqual([], list(images.should_delete()))
- def test_one_image_multiple_users(self):
- images = self.setup_images(None)
+ 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.add_user(users[0], 1)
images.add_user(users[1], 2)
images.end_user(users[0]['Id'])
- self.assertTrue(images.any_users())
- images.end_user(users[1]['Id'])
- self.assertFalse(images.any_users())
-
- def test_nonexistent_user_added(self):
- images = self.setup_images()
- images.add_user(MockContainer(MockImage()), 1)
- self.assertFalse(images.any_users())
-
- def test_nonexistent_user_removed(self):
- images = self.setup_images()
- images.end_user('nonexistent')
- self.assertFalse(images.any_users())
-
- def test_nonexistent_user_removed_amongst_real_users(self):
- images = self.setup_images(None)
- user = MockContainer(self.mock_images[-1])
- images.add_user(user, 1)
- images.add_user(MockContainer(MockImage()), 2)
- self.assertTrue(images.any_users())
- images.end_user(user['Id'])
- self.assertFalse(images.any_users())
-
- def test_del_image_removes_users(self):
- images = self.setup_images(None)
- user = MockContainer(self.mock_images[0])
- images.add_user(user, 1)
- images.del_image(self.mock_images[0]['Id'])
- self.assertFalse(images.any_users())
-
- 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):
- images = self.setup_images(None, None, target_size=1)
- self.assertEqual({image['Id'] for image in self.mock_images},
- set(images.should_delete()))
-
- def test_images_in_use_not_deletable(self):
- 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[1]['Id'])
- self.assertEqual([self.mock_images[1]['Id']],
+ self.assertEqual([self.mock_images[-1]['Id']],
list(images.should_delete()))
def test_images_suggested_for_deletion_by_lru(self):
@@ -201,6 +165,24 @@ class DockerImagesTestCase(unittest.TestCase):
self.assertEqual([self.mock_images[ii]['Id'] for ii in (1, 2, 0)],
list(images.should_delete()))
+ def test_adding_user_without_image_does_not_implicitly_add_image(self):
+ images = self.setup_images(10)
+ images.add_user(MockContainer(MockImage()), 1)
+ self.assertEqual([], list(images.should_delete()))
+
+ def test_nonexistent_user_removed(self):
+ images = self.setup_images()
+ images.end_user('nonexistent')
+ # No exception should be raised.
+
+ def test_del_image_effective_with_users_present(self):
+ images = self.setup_images(None, target_size=1)
+ user = MockContainer(self.mock_images[0])
+ images.add_user(user, 1)
+ images.del_image(self.mock_images[0]['Id'])
+ images.end_user(user['Id'])
+ self.assertEqual([], list(images.should_delete()))
+
def setup_from_daemon(self, *vsizes, target_size=1500000):
self.setup_mock_images(*vsizes)
docker_client = mock.MagicMock(name='docker_client')
@@ -289,16 +271,8 @@ class DockerImageCleanerTestCase(DockerImageUseRecorderTestCase):
self.docker_client.inspect_container()['Image'])
self.assertFalse(self.images.add_image.called)
- def test_no_deletions_when_containers_running(self):
- self.images.any_users.return_value = True
- self.events.append(MockEvent('destroy'))
- self.recorder.run()
- self.assertFalse(self.images.should_delete.called)
- self.assertFalse(self.docker_client.remove_image.called)
-
def test_deletions_after_destroy(self):
delete_id = MockDockerId()
- self.images.any_users.return_value = False
self.images.should_delete.return_value = [delete_id]
self.events.append(MockEvent('destroy'))
self.recorder.run()
@@ -307,7 +281,6 @@ class DockerImageCleanerTestCase(DockerImageUseRecorderTestCase):
def test_failed_deletion_handling(self):
delete_id = MockDockerId()
- self.images.any_users.return_value = False
self.images.should_delete.return_value = [delete_id]
self.docker_client.remove_image.side_effect = MockException(500)
self.events.append(MockEvent('destroy'))
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list