[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