Merge "libvirt: Use uuid for instance directory name"

nova/tests/fake_libvirt_utils.py

@@ -18,6 +18,7 @@ import os
 import StringIO
 
 from nova.openstack.common import cfg
+from nova.virt.libvirt import utils as libvirt_utils
 
 
 CONF = cfg.CONF
@@ -195,9 +196,8 @@ def fetch_image(context, target, image_id, user_id, project_id):
     pass
 
 
-def get_instance_path(instance):
+def get_instance_path(instance, forceold=False):
-    # TODO(mikal): we should really just call the real one here
+    return libvirt_utils.get_instance_path(instance, forceold=forceold)
-    return os.path.join(CONF.instances_path, instance['name'])
 
 
 def pick_disk_driver_name(is_block_dev=False):

nova/tests/test_imagebackend.py

@@ -19,16 +19,16 @@ import fixtures
 import os
 
 from nova.openstack.common import cfg
+from nova.openstack.common import uuidutils
 from nova import test
 from nova.tests import fake_libvirt_utils
 from nova.virt.libvirt import imagebackend
-from nova.virt.libvirt import utils as libvirt_utils
 
 CONF = cfg.CONF
 
 
 class _ImageTestCase(object):
-    INSTANCES_PATH = '/fake'
+    INSTANCES_PATH = '/instances_path'
 
     def mock_create_image(self, image):
         def create_image(fn, base, size, *args, **kwargs):
@@ -39,14 +39,19 @@ class _ImageTestCase(object):
         super(_ImageTestCase, self).setUp()
         self.flags(disable_process_locking=True,
                    instances_path=self.INSTANCES_PATH)
-        self.INSTANCE = {'name': 'instance'}
+        self.INSTANCE = {'name': 'instance',
+                         'uuid': uuidutils.generate_uuid()}
         self.NAME = 'fake.vm'
         self.TEMPLATE = 'template'
 
+        self.OLD_STYLE_INSTANCE_PATH = \
+            fake_libvirt_utils.get_instance_path(self.INSTANCE, forceold=True)
         self.PATH = os.path.join(
-            libvirt_utils.get_instance_path(self.INSTANCE), self.NAME)
+            fake_libvirt_utils.get_instance_path(self.INSTANCE), self.NAME)
-        self.TEMPLATE_DIR = os.path.join(CONF.instances_path,
+
-                                         '_base')
+        # TODO(mikal): rename template_dir to base_dir and template_path
+        # to cached_image_path. This will be less confusing.
+        self.TEMPLATE_DIR = os.path.join(CONF.instances_path, '_base')
         self.TEMPLATE_PATH = os.path.join(self.TEMPLATE_DIR, 'template')
 
         self.useFixture(fixtures.MonkeyPatch(
@@ -55,6 +60,8 @@ class _ImageTestCase(object):
 
     def test_cache(self):
         self.mox.StubOutWithMock(os.path, 'exists')
+        if self.OLD_STYLE_INSTANCE_PATH:
+            os.path.exists(self.OLD_STYLE_INSTANCE_PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_DIR).AndReturn(False)
         os.path.exists(self.PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_PATH).AndReturn(False)
@@ -72,6 +79,8 @@ class _ImageTestCase(object):
 
     def test_cache_image_exists(self):
         self.mox.StubOutWithMock(os.path, 'exists')
+        if self.OLD_STYLE_INSTANCE_PATH:
+            os.path.exists(self.OLD_STYLE_INSTANCE_PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_DIR).AndReturn(True)
         os.path.exists(self.PATH).AndReturn(True)
         os.path.exists(self.TEMPLATE_PATH).AndReturn(True)
@@ -84,6 +93,8 @@ class _ImageTestCase(object):
 
     def test_cache_base_dir_exists(self):
         self.mox.StubOutWithMock(os.path, 'exists')
+        if self.OLD_STYLE_INSTANCE_PATH:
+            os.path.exists(self.OLD_STYLE_INSTANCE_PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_DIR).AndReturn(True)
         os.path.exists(self.PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_PATH).AndReturn(False)
@@ -100,6 +111,8 @@ class _ImageTestCase(object):
 
     def test_cache_template_exists(self):
         self.mox.StubOutWithMock(os.path, 'exists')
+        if self.OLD_STYLE_INSTANCE_PATH:
+            os.path.exists(self.OLD_STYLE_INSTANCE_PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_DIR).AndReturn(True)
         os.path.exists(self.PATH).AndReturn(False)
         os.path.exists(self.TEMPLATE_PATH).AndReturn(True)
@@ -197,6 +210,11 @@ class Qcow2TestCase(_ImageTestCase, test.TestCase):
     def test_create_image_with_size(self):
         fn = self.prepare_mocks()
         fn(target=self.TEMPLATE_PATH)
+        self.mox.StubOutWithMock(os.path, 'exists')
+        if self.OLD_STYLE_INSTANCE_PATH:
+            os.path.exists(self.OLD_STYLE_INSTANCE_PATH).AndReturn(False)
+        os.path.exists(self.TEMPLATE_PATH).AndReturn(False)
+        os.path.exists(self.PATH).AndReturn(False)
         imagebackend.libvirt_utils.create_cow_image(self.TEMPLATE_PATH,
                                                     self.PATH)
         imagebackend.disk.extend(self.PATH, self.SIZE)
@@ -218,6 +236,7 @@ class LvmTestCase(_ImageTestCase, test.TestCase):
         super(LvmTestCase, self).setUp()
         self.flags(libvirt_images_volume_group=self.VG)
         self.LV = '%s_%s' % (self.INSTANCE['name'], self.NAME)
+        self.OLD_STYLE_INSTANCE_PATH = None
         self.PATH = os.path.join('/dev', self.VG, self.LV)
 
         self.disk = imagebackend.disk
@@ -344,7 +363,8 @@ class LvmTestCase(_ImageTestCase, test.TestCase):
 
 
 class BackendTestCase(test.TestCase):
-    INSTANCE = {'name': 'fake-instance'}
+    INSTANCE = {'name': 'fake-instance',
+                'uuid': uuidutils.generate_uuid()}
     NAME = 'fake-name.suffix'
 
     def get_image(self, use_cow, image_type):

nova/tests/test_imagecache.py

@@ -47,10 +47,10 @@ class ImageCacheManagerTestCase(test.TestCase):
 
     def setUp(self):
         super(ImageCacheManagerTestCase, self).setUp()
-        self.stock_instance_names = {'instance-00000001': '123',
+        self.stock_instance_names = set(['instance-00000001',
-                                     'instance-00000002': '456',
+                                         'instance-00000002',
-                                     'instance-00000003': '789',
+                                         'instance-00000003',
-                                     'banana-42-hamster': '444'}
+                                         'banana-42-hamster'])
 
     def test_read_stored_checksum_missing(self):
         self.stubs.Set(os.path, 'exists', lambda x: False)
@@ -181,6 +181,9 @@ class ImageCacheManagerTestCase(test.TestCase):
         self.assertTrue(image_cache_manager.used_images['2'] ==
                         (1, 1, ['inst-2', 'inst-3']))
 
+        self.assertTrue('inst-1' in image_cache_manager.instance_names)
+        self.assertTrue('123' in image_cache_manager.instance_names)
+
         self.assertEqual(len(image_cache_manager.image_popularity), 2)
         self.assertEqual(image_cache_manager.image_popularity['1'], 1)
         self.assertEqual(image_cache_manager.image_popularity['2'], 2)
@@ -200,7 +203,7 @@ class ImageCacheManagerTestCase(test.TestCase):
         self.assertTrue(image_cache_manager.used_images['1'] ==
                         (1, 0, ['inst-1']))
         self.assertTrue(image_cache_manager.instance_names ==
-                        set(['inst-1', 'inst-1_resize']))
+                        set(['inst-1', '123', 'inst-1_resize', '123_resize']))
 
         self.assertEqual(len(image_cache_manager.image_popularity), 1)
         self.assertEqual(image_cache_manager.image_popularity['1'], 1)

nova/tests/test_libvirt.py

@@ -43,6 +43,7 @@ from nova.openstack.common import fileutils
 from nova.openstack.common import importutils
 from nova.openstack.common import jsonutils
 from nova.openstack.common import log as logging
+from nova.openstack.common import uuidutils
 from nova import test
 from nova.tests import fake_libvirt_utils
 from nova.tests import fake_network
@@ -135,7 +136,8 @@ class FakeVirtDomain(object):
 class CacheConcurrencyTestCase(test.TestCase):
     def setUp(self):
         super(CacheConcurrencyTestCase, self).setUp()
-        self.flags(instances_path='nova.compute.manager')
+
+        self.flags(instances_path=self.useFixture(fixtures.TempDir()).path)
 
         # utils.synchronized() will create the lock_path for us if it
         # doesn't already exist. It will also delete it when it's done,
@@ -165,19 +167,18 @@ class CacheConcurrencyTestCase(test.TestCase):
             fake_libvirt_utils))
 
     def tearDown(self):
-        # Make sure the lock_path for this test is cleaned up
-        if os.path.exists(self.lock_path):
-            shutil.rmtree(self.lock_path)
-
         super(CacheConcurrencyTestCase, self).tearDown()
 
     def test_same_fname_concurrency(self):
         # Ensures that the same fname cache runs at a sequentially.
+        uuid = uuidutils.generate_uuid()
+
         backend = imagebackend.Backend(False)
         wait1 = eventlet.event.Event()
         done1 = eventlet.event.Event()
         sig1 = eventlet.event.Event()
-        thr1 = eventlet.spawn(backend.image({'name': 'instance'},
+        thr1 = eventlet.spawn(backend.image({'name': 'instance',
+                                             'uuid': uuid},
                                             'name').cache,
                 _concurrency, 'fname', None,
                 signal=sig1, wait=wait1, done=done1)
@@ -188,7 +189,8 @@ class CacheConcurrencyTestCase(test.TestCase):
         wait2 = eventlet.event.Event()
         done2 = eventlet.event.Event()
         sig2 = eventlet.event.Event()
-        thr2 = eventlet.spawn(backend.image({'name': 'instance'},
+        thr2 = eventlet.spawn(backend.image({'name': 'instance',
+                                             'uuid': uuid},
                                             'name').cache,
                 _concurrency, 'fname', None,
                 signal=sig2, wait=wait2, done=done2)
@@ -209,11 +211,14 @@ class CacheConcurrencyTestCase(test.TestCase):
 
     def test_different_fname_concurrency(self):
         # Ensures that two different fname caches are concurrent.
+        uuid = uuidutils.generate_uuid()
+
         backend = imagebackend.Backend(False)
         wait1 = eventlet.event.Event()
         done1 = eventlet.event.Event()
         sig1 = eventlet.event.Event()
-        thr1 = eventlet.spawn(backend.image({'name': 'instance'},
+        thr1 = eventlet.spawn(backend.image({'name': 'instance',
+                                             'uuid': uuid},
                                             'name').cache,
                 _concurrency, 'fname2', None,
                 signal=sig1, wait=wait1, done=done1)
@@ -224,7 +229,8 @@ class CacheConcurrencyTestCase(test.TestCase):
         wait2 = eventlet.event.Event()
         done2 = eventlet.event.Event()
         sig2 = eventlet.event.Event()
-        thr2 = eventlet.spawn(backend.image({'name': 'instance'},
+        thr2 = eventlet.spawn(backend.image({'name': 'instance',
+                                             'uuid': uuid},
                                             'name').cache,
                 _concurrency, 'fname1', None,
                 signal=sig2, wait=wait2, done=done2)
@@ -2357,8 +2363,8 @@ class LibvirtConnTestCase(test.TestCase):
             ret = conn.pre_live_migration(c, inst_ref, vol, nw_info,
                                           migrate_data)
             self.assertEqual(ret, None)
-            self.assertTrue(os.path.exists('%s/%s/' %
+            self.assertTrue(os.path.exists('%s/%s/' % (tmpdir,
-                           (tmpdir, inst_ref['name'])))
+                                                       inst_ref['uuid'])))
         db.instance_destroy(self.context, inst_ref['uuid'])
 
     def test_pre_block_migration_works_correctly(self):
@@ -2394,7 +2400,7 @@ class LibvirtConnTestCase(test.TestCase):
                                      dummyjson)
 
             self.assertTrue(os.path.exists('%s/%s/' %
-                                           (tmpdir, instance_ref['name'])))
+                                           (tmpdir, instance_ref['uuid'])))
 
         db.instance_destroy(self.context, instance_ref['uuid'])
 

nova/tests/test_virt_drivers.py

@@ -184,11 +184,16 @@ class VirtDriverLoaderTestCase(_FakeDriverBackendTestCase, test.TestCase):
 class _VirtDriverTestCase(_FakeDriverBackendTestCase):
     def setUp(self):
         super(_VirtDriverTestCase, self).setUp()
+
+        self.flags(instances_path=self.useFixture(fixtures.TempDir()).path)
         self.connection = importutils.import_object(self.driver_module,
                                                     fake.FakeVirtAPI())
         self.ctxt = test_utils.get_test_admin_context()
         self.image_service = fake_image.FakeImageService()
 
+    def tearDown(self):
+        super(_VirtDriverTestCase, self).tearDown()
+
     def _get_running_instance(self):
         instance_ref = test_utils.get_test_instance()
         network_info = test_utils.get_test_network_info()

nova/virt/libvirt/driver.py

@@ -2006,8 +2006,7 @@ class LibvirtDriver(driver.ComputeDriver):
         xml = conf.to_xml()
 
         if write_to_disk:
-            instance_dir = os.path.join(CONF.instances_path,
+            instance_dir = libvirt_utils.get_instance_path(instance)
-                                        instance["name"])
             xml_path = os.path.join(instance_dir, 'libvirt.xml')
             libvirt_utils.write_to_file(xml_path, xml)
 

nova/virt/libvirt/imagecache.py

@@ -220,7 +220,7 @@ class ImageCacheManager(object):
 
         self.used_images = {}
         self.image_popularity = {}
-        self.instance_names = {}
+        self.instance_names = set()
 
         self.active_base_files = []
         self.corrupt_base_files = []
@@ -263,7 +263,11 @@ class ImageCacheManager(object):
         self.instance_names = set()
 
         for instance in all_instances:
+            # NOTE(mikal): "instance name" here means "the name of a directory
+            # which might contain an instance" and therefore needs to include
+            # historical permutations as well as the current one.
             self.instance_names.add(instance['name'])
+            self.instance_names.add(instance['uuid'])
 
             resize_states = [task_states.RESIZE_PREP,
                              task_states.RESIZE_MIGRATING,
@@ -272,6 +276,7 @@ class ImageCacheManager(object):
             if instance['task_state'] in resize_states or \
                 instance['vm_state'] == vm_states.RESIZED:
                 self.instance_names.add(instance['name'] + '_resize')
+                self.instance_names.add(instance['uuid'] + '_resize')
 
             image_ref_str = str(instance['image_ref'])
             local, remote, insts = self.used_images.get(image_ref_str,

nova/virt/libvirt/utils.py

@@ -606,17 +606,19 @@ def fetch_image(context, target, image_id, user_id, project_id):
     images.fetch_to_raw(context, image_id, target, user_id, project_id)
 
 
-def get_instance_path(instance):
+def get_instance_path(instance, forceold=False):
     """Determine the correct path for instance storage.
 
-    This used to be calculated all over the place. This method centralizes
+    This method determines the directory name for instance storage, while
-    this into one location, which will make it easier to change the
+    handling the fact that we changed the naming style to something more
-    algorithm used to name instance storage directories.
+    unique in the grizzly release.
 
     :param instance: the instance we want a path for
+    :param forceold: force the use of the pre-grizzly format
 
     :returns: a path to store information about that instance
     """
-    # TODO(mikal): we should use UUID instead of name, as name isn't
+    pre_grizzly_name = os.path.join(CONF.instances_path, instance['name'])
-    # nessesarily unique
+    if forceold or os.path.exists(pre_grizzly_name):
-    return os.path.join(CONF.instances_path, instance['name'])
+        return pre_grizzly_name
+    return os.path.join(CONF.instances_path, instance['uuid'])