Don't use glance when verifying images.

Using glance means that admin contexts need to know how to use
keystone when that is enabled. Its safer to avoid calling glance
at all from inside the periodic task.

This should resolve bug 934464.

Change-Id: Ib730e3f57721fca7080d90ae80b5f8916c1dc76c

2 files changed, 64 insertions, 109 deletions

diff --git a/nova/tests/test_imagecache.py b/nova/tests/test_imagecache.py
index 242f9c010..c748abbae 100644
--- a/nova/tests/test_imagecache.py
+++ b/nova/tests/test_imagecache.py
@@ -117,15 +117,15 @@ class ImageCacheManagerTestCase(test.TestCase):
 
     def test_list_running_instances(self):
         self.stubs.Set(db, 'instance_get_all',
-                       lambda x: [{'image_ref': 'image-1',
+                       lambda x: [{'image_ref': '1',
                                    'host': FLAGS.host,
                                    'name': 'inst-1',
                                    'uuid': '123'},
-                                  {'image_ref': 'image-2',
+                                  {'image_ref': '2',
                                    'host': FLAGS.host,
                                    'name': 'inst-2',
                                    'uuid': '456'},
-                                  {'image_ref': 'image-2',
+                                  {'image_ref': '2',
                                    'host': 'remotehost',
                                    'name': 'inst-3',
                                    'uuid': '789'}])
@@ -136,14 +136,14 @@ class ImageCacheManagerTestCase(test.TestCase):
         image_cache_manager._list_running_instances(None)
 
         self.assertEqual(len(image_cache_manager.used_images), 2)
-        self.assertTrue(image_cache_manager.used_images['image-1'] ==
+        self.assertTrue(image_cache_manager.used_images['1'] ==
                         (1, 0, ['inst-1']))
-        self.assertTrue(image_cache_manager.used_images['image-2'] ==
+        self.assertTrue(image_cache_manager.used_images['2'] ==
                         (1, 1, ['inst-2', 'inst-3']))
 
         self.assertEqual(len(image_cache_manager.image_popularity), 2)
-        self.assertEqual(image_cache_manager.image_popularity['image-1'], 1)
-        self.assertEqual(image_cache_manager.image_popularity['image-2'], 2)
+        self.assertEqual(image_cache_manager.image_popularity['1'], 1)
+        self.assertEqual(image_cache_manager.image_popularity['2'], 2)
 
     def test_list_backing_images_small(self):
         self.stubs.Set(os, 'listdir',
@@ -420,9 +420,7 @@ class ImageCacheManagerTestCase(test.TestCase):
                                     -1)
 
     def test_handle_base_image_unused(self):
-        img = {'container_format': 'ami',
-               'id': '123',
-               'uuid': '1234-4567-2378'}
+        img = '123'
 
         with self._make_base_file() as fname:
             os.utime(fname, (-1, time.time() - 3601))
@@ -437,9 +435,7 @@ class ImageCacheManagerTestCase(test.TestCase):
             self.assertEquals(image_cache_manager.corrupt_base_files, [])
 
     def test_handle_base_image_used(self):
-        img = {'container_format': 'ami',
-               'id': '123',
-               'uuid': '1234-4567-2378'}
+        img = '123'
 
         with self._make_base_file() as fname:
             os.utime(fname, (-1, time.time() - 3601))
@@ -454,9 +450,7 @@ class ImageCacheManagerTestCase(test.TestCase):
             self.assertEquals(image_cache_manager.corrupt_base_files, [])
 
     def test_handle_base_image_used_remotely(self):
-        img = {'container_format': 'ami',
-               'id': '123',
-               'uuid': '1234-4567-2378'}
+        img = '123'
 
         with self._make_base_file() as fname:
             os.utime(fname, (-1, time.time() - 3601))
@@ -472,9 +466,7 @@ class ImageCacheManagerTestCase(test.TestCase):
     def test_handle_base_image_absent(self):
         """Ensure we warn for use of a missing base image."""
 
-        img = {'container_format': 'ami',
-               'id': '123',
-               'uuid': '1234-4567-2378'}
+        img = '123'
 
         with self._intercept_log_messages() as stream:
             image_cache_manager = imagecache.ImageCacheManager()
@@ -488,9 +480,7 @@ class ImageCacheManagerTestCase(test.TestCase):
                                 -1)
 
     def test_handle_base_image_used_missing(self):
-        img = {'container_format': 'ami',
-               'id': '123',
-               'uuid': '1234-4567-2378'}
+        img = '123'
 
         with utils.tempdir() as tmpdir:
             fname = os.path.join(tmpdir, 'aaa')
@@ -505,9 +495,7 @@ class ImageCacheManagerTestCase(test.TestCase):
             self.assertEquals(image_cache_manager.corrupt_base_files, [])
 
     def test_handle_base_image_checksum_fails(self):
-        img = {'container_format': 'ami',
-               'id': '123',
-               'uuid': '1234-4567-2378'}
+        img = '123'
 
         with self._make_base_file() as fname:
             f = open(fname, 'w')
@@ -525,6 +513,9 @@ class ImageCacheManagerTestCase(test.TestCase):
                               [fname])
 
     def test_verify_base_images(self):
+        hashed_1 = '356a192b7913b04c54574d18c28d46e6395428ab'
+        hashed_42 = '92cfceb39d57d914ed8b14d0e37643de0797ae56'
+
         self.flags(instances_path='/instance_path')
         self.flags(remove_unused_base_images=True)
 
@@ -532,12 +523,10 @@ class ImageCacheManagerTestCase(test.TestCase):
                           'ephemeral_0_20_None',
                           'e97222e91fc4241f49a7f520d1dcf446751129b3_sm',
                           'e09c675c2d1cfac32dae3c2d83689c8c94bc693b_sm',
-                          '92cfceb39d57d914ed8b14d0e37643de0797ae56',
-                          '17d1b00b81642842e514494a78e804e9a511637c',
-                          ('17d1b00b81642842e514494a78e804e9a511637c_'
-                           '5368709120'),
-                          ('17d1b00b81642842e514494a78e804e9a511637c_'
-                           '10737418240'),
+                          hashed_42,
+                          hashed_1,
+                          '%s_5368709120' % hashed_1,
+                          '%s_10737418240' % hashed_1,
                           '00000004']
 
         def fq_path(path):
@@ -556,8 +545,7 @@ class ImageCacheManagerTestCase(test.TestCase):
                         '/instance_path/instance-1/disk',
                         '/instance_path/instance-2/disk',
                         '/instance_path/instance-3/disk',
-                        ('/instance_path/_base/'
-                         '92cfceb39d57d914ed8b14d0e37643de0797ae56.sha1')]:
+                        '/instance_path/_base/%s.sha1' % hashed_42]:
                 return True
 
             for p in base_file_list:
@@ -566,8 +554,8 @@ class ImageCacheManagerTestCase(test.TestCase):
                 if path == fq_path(p) + '.sha1':
                     return False
 
-            if path in [('/instance_path/_base/'
-                         '92cfceb39d57d914ed8b14d0e37643de0797ae56_sm')]:
+            if path in ['/instance_path/_base/%s_sm' % hashed_1,
+                        '/instance_path/_base/%s_sm' % hashed_42]:
                 return False
 
             self.fail('Unexpected path existance check: %s' % path)
@@ -610,32 +598,22 @@ class ImageCacheManagerTestCase(test.TestCase):
 
         # Fake the database call which lists running instances
         self.stubs.Set(db, 'instance_get_all',
-                       lambda x: [{'image_ref': 'image-1',
+                       lambda x: [{'image_ref': '1',
                                    'host': FLAGS.host,
                                    'name': 'instance-1',
                                    'uuid': '123'},
-                                  {'image_ref': 'image-2',
+                                  {'image_ref': '1',
                                    'host': FLAGS.host,
                                    'name': 'instance-2',
                                    'uuid': '456'}])
 
         image_cache_manager = imagecache.ImageCacheManager()
 
-        # Fake the image service call which lists all registered images
-        class FakeImageService(object):
-            def detail(self, _context):
-                return [{'container_format': 'ami', 'id': '42'},
-                        {'container_format': 'amk', 'id': '43'}]
-
-        self.stubs.Set(image, 'get_default_image_service',
-                       lambda: FakeImageService())
-
         # Fake the utils call which finds the backing image
         def get_disk_backing_file(path):
             if path in ['/instance_path/instance-1/disk',
                         '/instance_path/instance-2/disk']:
-                return fq_path('17d1b00b81642842e514494a78e804e9a511637c_'
-                               '5368709120')
+                return fq_path('%s_5368709120' % hashed_1)
             self.fail('Unexpected backing file lookup: %s' % path)
 
         self.stubs.Set(virtutils, 'get_disk_backing_file',
@@ -643,8 +621,7 @@ class ImageCacheManagerTestCase(test.TestCase):
 
         # Fake out verifying checksums, as that is tested elsewhere
         self.stubs.Set(image_cache_manager, '_verify_checksum',
-                       lambda x, y:
-                           y == '92cfceb39d57d914ed8b14d0e37643de0797ae56')
+                       lambda x, y: y == hashed_42)
 
         # Fake getmtime as well
         orig_getmtime = os.path.getmtime
@@ -674,16 +651,13 @@ class ImageCacheManagerTestCase(test.TestCase):
         image_cache_manager.verify_base_images(None)
 
         # Verify
-        active = [fq_path('17d1b00b81642842e514494a78e804e9a511637c_'
-                          '5368709120')]
+        active = [fq_path(hashed_1), fq_path('%s_5368709120' % hashed_1)]
         self.assertEquals(image_cache_manager.active_base_files, active)
 
         for rem in [fq_path('e97222e91fc4241f49a7f520d1dcf446751129b3_sm'),
                     fq_path('e09c675c2d1cfac32dae3c2d83689c8c94bc693b_sm'),
-                    fq_path('92cfceb39d57d914ed8b14d0e37643de0797ae56'),
-                    fq_path('17d1b00b81642842e514494a78e804e9a511637c'),
-                    fq_path('17d1b00b81642842e514494a78e804e9a511637c_'
-                            '10737418240')]:
+                    fq_path(hashed_42),
+                    fq_path('%s_10737418240' % hashed_1)]:
             self.assertTrue(rem in image_cache_manager.removable_base_files)
 
     def test_verify_base_images_no_base(self):
diff --git a/nova/virt/libvirt/imagecache.py b/nova/virt/libvirt/imagecache.py
index 22a4d1e8b..8b01c27c5 100644
--- a/nova/virt/libvirt/imagecache.py
+++ b/nova/virt/libvirt/imagecache.py
@@ -34,7 +34,6 @@ from nova import compute
 from nova import context as db_context
 from nova import db
 from nova import flags
-from nova import image
 from nova import log as logging
 from nova.openstack.common import cfg
 from nova import utils
@@ -220,7 +219,7 @@ class ImageCacheManager(object):
             if m:
                 yield img, False, True
 
-    def _verify_checksum(self, img, base_file):
+    def _verify_checksum(self, img_id, base_file):
         """Compare the checksum stored on disk with the current file.
 
         Note that if the checksum fails to verify this is logged, but no actual
@@ -235,11 +234,9 @@ class ImageCacheManager(object):
             f.close()
 
             if current_checksum != stored_checksum:
-                LOG.error(_('%(container_format)s-%(id)s '
-                            '(%(base_file)s): '
-                            'image verification failed'),
-                          {'container_format': img['container_format'],
-                           'id': img['id'],
+                LOG.error(_('%(id)s (%(base_file)s): image verification '
+                            'failed'),
+                          {'id': img_id,
                            'base_file': base_file})
                 return False
 
@@ -247,10 +244,9 @@ class ImageCacheManager(object):
                 return True
 
         else:
-            LOG.debug(_('%(container_format)s-%(id)s (%(base_file)s): '
-                        'image verification skipped, no hash stored'),
-                      {'container_format': img['container_format'],
-                       'id': img['id'],
+            LOG.debug(_('%(id)s (%(base_file)s): image verification skipped, '
+                        'no hash stored'),
+                      {'id': img_id,
                        'base_file': base_file})
 
             # NOTE(mikal): If the checksum file is missing, then we should
@@ -294,15 +290,14 @@ class ImageCacheManager(object):
                           {'base_file': base_file,
                            'error': e})
 
-    def _handle_base_image(self, img, base_file):
+    def _handle_base_image(self, img_id, base_file):
         """Handle the checks for a single base image."""
 
         image_bad = False
         image_in_use = False
 
-        LOG.info(_('%(container_format)s-%(id)s (%(base_file)s): checking'),
-                 {'container_format': img['container_format'],
-                  'id': img['id'],
+        LOG.info(_('%(id)s (%(base_file)s): checking'),
+                 {'id': img_id,
                   'base_file': base_file})
 
         if base_file in self.unexplained_images:
@@ -311,17 +306,16 @@ class ImageCacheManager(object):
         if (base_file and os.path.exists(base_file)
             and os.path.isfile(base_file)):
             # _verify_checksum returns True if the checksum is ok.
-            image_bad = not self._verify_checksum(img, base_file)
+            image_bad = not self._verify_checksum(img_id, base_file)
 
         instances = []
-        if str(img['id']) in self.used_images:
-            local, remote, instances = self.used_images[str(img['id'])]
+        if img_id in self.used_images:
+            local, remote, instances = self.used_images[img_id]
             if local > 0:
-                LOG.debug(_('%(container_format)s-%(id)s (%(base_file)s): '
+                LOG.debug(_('%(id)s (%(base_file)s): '
                             'in use: on this node %(local)d local, '
                             '%(remote)d on other nodes'),
-                          {'container_format': img['container_format'],
-                           'id': img['id'],
+                          {'id': img_id,
                            'base_file': base_file,
                            'local': local,
                            'remote': remote})
@@ -330,21 +324,17 @@ class ImageCacheManager(object):
                 self.active_base_files.append(base_file)
 
                 if not base_file:
-                    LOG.warning(_('%(container_format)s-%(id)s '
-                                  '(%(base_file)s): warning -- an absent '
-                                  'base file is in use! instances: '
+                    LOG.warning(_('%(id)s (%(base_file)s): warning -- an '
+                                  'absent base file is in use! instances: '
                                   '%(instance_list)s'),
-                                {'container_format':
-                                     img['container_format'],
-                                 'id': img['id'],
+                                {'id': img_id,
                                  'base_file': base_file,
                                  'instance_list': ' '.join(instances)})
 
             else:
-                LOG.debug(_('%(container_format)s-%(id)s (%(base_file)s): '
-                            'in use on (%(remote)d on other nodes)'),
-                          {'container_format': img['container_format'],
-                           'id': img['id'],
+                LOG.debug(_('%(id)s (%(base_file)s): in use on (%(remote)d on '
+                            'other nodes)'),
+                          {'id': img_id,
                            'base_file': base_file,
                            'remote': remote})
         if image_bad:
@@ -352,24 +342,18 @@ class ImageCacheManager(object):
 
         if base_file:
             if not image_in_use:
-                LOG.debug(_('%(container_format)s-%(id)s (%(base_file)s): '
-                            'image is not in use'),
-                          {'container_format': img['container_format'],
-                           'id': img['id'],
+                LOG.debug(_('%(id)s (%(base_file)s): image is not in use'),
+                          {'id': img_id,
                            'base_file': base_file})
                 self.removable_base_files.append(base_file)
 
             else:
-                LOG.debug(_('%(container_format)s-%(id)s (%(base_file)s): '
-                            'image is in use'),
-                          {'container_format': img['container_format'],
-                           'id': img['id'],
+                LOG.debug(_('%(id)s (%(base_file)s): image is in use'),
+                          {'id': img_id,
                            'base_file': base_file})
 
     def verify_base_images(self, context):
         """Verify that base images are in a reasonable state."""
-        # TODO(mikal): Write a unit test for this method
-        # TODO(mikal): Handle "generated" images
 
         # NOTE(mikal): The new scheme for base images is as follows -- an
         # image is streamed from the image service to _base (filename is the
@@ -392,14 +376,12 @@ class ImageCacheManager(object):
         self._list_base_images(base_dir)
         self._list_running_instances(context)
 
-        # Determine what images are in glance. GlanceImageService.detail uses
-        # _fetch_images which handles pagination for us
-        image_service = image.get_default_image_service()
-        for img in image_service.detail(context):
-            if img['container_format'] != 'ami':
-                continue
-
-            fingerprint = hashlib.sha1(str(img['id'])).hexdigest()
+        # Determine what images are on disk because they're in use
+        for img in self.used_images:
+            fingerprint = hashlib.sha1(img).hexdigest()
+            LOG.debug(_('Image id %(id)s yields fingerprint %(fingerprint)s'),
+                      {'id': img,
+                       'fingerprint': fingerprint})
             for result in self._find_base_file(base_dir, fingerprint):
                 base_file, image_small, image_resized = result
                 self._handle_base_image(img, base_file)
@@ -407,12 +389,11 @@ class ImageCacheManager(object):
                 if not image_small and not image_resized:
                     self.originals.append(base_file)
 
-        # Elements remaining in unexplained_images are not directly from
-        # glance. That might mean they're from a image that was removed from
-        # glance, but it might also mean that they're a resized image.
+        # Elements remaining in unexplained_images might be in use
         inuse_backing_images = self._list_backing_images()
         for backing_path in inuse_backing_images:
-            self.active_base_files.append(backing_path)
+            if not backing_path in self.active_base_files:
+                self.active_base_files.append(backing_path)
 
         # Anything left is an unknown base image
         for img in self.unexplained_images: