Until now, backend used the disk's index when creating the symlink of the corresponding block device. This is problematic. For example if one starts an instance with three disks [disk0, disk1, disk2], then removes the middle one (disk1), and then adds a third (disk3), the disks in config data will be [disk0, disk2, disk3] and thus the new one will get index 2. When trying to assemble the newly created disk we will overwrite the disk2's symlink.
Fix the above behavior by creating an additional symlink based on the disk's uuid and pass this to the hypervisor. We continue to create an index-based symlink as well, but this behavior is inherently problematic when using hotplug. To keep old instances migratable, we still create the old type of symlink during BlockdevOpen() which is invoked on the target node just before migration. Also remove a really old check that did not make any sense anymore in GetInstanceMigratable(). Signed-off-by: Dimitris Aragiorgis <[email protected]> --- lib/backend.py | 51 +++++++++++++++++++++++++----------- test/py/ganeti.backend_unittest.py | 14 ++++++++-- 2 files changed, 48 insertions(+), 17 deletions(-) diff --git a/lib/backend.py b/lib/backend.py index 0397ad5..8caf8f0 100644 --- a/lib/backend.py +++ b/lib/backend.py @@ -2324,12 +2324,6 @@ def GetInstanceMigratable(instance): if iname not in hyper.ListInstances(hvparams=instance.hvparams): _Fail("Instance %s is not running", iname) - for idx in range(len(instance.disks_info)): - link_name = _GetBlockDevSymlinkPath(iname, idx) - if not os.path.islink(link_name): - logging.warning("Instance %s is missing symlink %s for disk %d", - iname, link_name, idx) - def GetAllInstancesInfo(hypervisor_list, all_hvparams): """Gather data about all instances. @@ -2522,19 +2516,27 @@ def RunRenameInstance(instance, old_name, debug): " log file:\n%s", result.fail_reason, "\n".join(lines), log=False) -def _GetBlockDevSymlinkPath(instance_name, idx, _dir=None): +def _GetBlockDevSymlinkPath(instance_name, idx=None, uuid=None, _dir=None): """Returns symlink path for block device. """ if _dir is None: _dir = pathutils.DISK_LINKS_DIR + assert idx is not None or uuid is not None + + # Using the idx is deprecated. Use the uuid instead if it is available. + if uuid: + ident = uuid + else: + ident = idx + return utils.PathJoin(_dir, ("%s%s%s" % - (instance_name, constants.DISK_SEPARATOR, idx))) + (instance_name, constants.DISK_SEPARATOR, ident))) -def _SymlinkBlockDev(instance_name, device_path, idx): +def _SymlinkBlockDev(instance_name, device_path, idx=None, uuid=None): """Set up symlinks to a instance's block device. This is an auxiliary function run when an instance is start (on the primary @@ -2544,6 +2546,7 @@ def _SymlinkBlockDev(instance_name, device_path, idx): @param instance_name: the name of the target instance @param device_path: path of the physical block device, on the node @param idx: the disk index + @param uuid: the disk uuid @return: absolute path to the disk's symlink """ @@ -2551,7 +2554,8 @@ def _SymlinkBlockDev(instance_name, device_path, idx): if not device_path: return None - link_name = _GetBlockDevSymlinkPath(instance_name, idx) + link_name = _GetBlockDevSymlinkPath(instance_name, idx, uuid) + try: os.symlink(device_path, link_name) except OSError, err: @@ -2570,14 +2574,20 @@ def _RemoveBlockDevLinks(instance_name, disks): """Remove the block device symlinks belonging to the given instance. """ - for idx, _ in enumerate(disks): - link_name = _GetBlockDevSymlinkPath(instance_name, idx) + def _remove_symlink(link_name): if os.path.islink(link_name): try: os.remove(link_name) except OSError: logging.exception("Can't remove symlink '%s'", link_name) + for idx, disk in enumerate(disks): + link_name = _GetBlockDevSymlinkPath(instance_name, uuid=disk.uuid) + _remove_symlink(link_name) + # Remove also the deprecated symlink (if any) + link_name = _GetBlockDevSymlinkPath(instance_name, idx=idx) + _remove_symlink(link_name) + def _CalculateDeviceURI(instance, disk, device): """Get the URI for the device. @@ -2621,7 +2631,11 @@ def _GatherAndLinkBlockDevs(instance): str(disk)) device.Open() try: - link_name = _SymlinkBlockDev(instance.name, device.dev_path, idx) + # Create both index-based and uuid-based symlinks + # for backwards compatibility + _SymlinkBlockDev(instance.name, device.dev_path, idx=idx) + link_name = _SymlinkBlockDev(instance.name, device.dev_path, + uuid=disk.uuid) except OSError, e: raise errors.BlockDeviceError("Cannot create block device symlink: %s" % e.strerror) @@ -3446,7 +3460,10 @@ def BlockdevAssemble(disk, instance, as_primary, idx): link_name = None uri = None if as_primary: - link_name = _SymlinkBlockDev(instance.name, dev_path, idx) + # Create both index-based and uuid-based symlinks + # for backwards compatibility + _SymlinkBlockDev(instance.name, dev_path, idx=idx) + link_name = _SymlinkBlockDev(instance.name, dev_path, uuid=disk.uuid) uri = _CalculateDeviceURI(instance, disk, result) elif result: return result, result @@ -4643,7 +4660,11 @@ def BlockdevOpen(instance_name, disks, exclusive): for idx, rd in enumerate(bdevs): try: rd.Open(exclusive=exclusive) - _SymlinkBlockDev(instance_name, rd.dev_path, idx) + _SymlinkBlockDev(instance_name, rd.dev_path, uuid=disks[idx].uuid) + # Also create an old type of symlink so that instances + # can be migratable, since they may still have deprecated + # symlinks in their runtime files. + _SymlinkBlockDev(instance_name, rd.dev_path, idx=idx) except errors.BlockDeviceError, err: msg.append(str(err)) diff --git a/test/py/ganeti.backend_unittest.py b/test/py/ganeti.backend_unittest.py index 68b2eee..71fb04c 100755 --- a/test/py/ganeti.backend_unittest.py +++ b/test/py/ganeti.backend_unittest.py @@ -629,15 +629,25 @@ class TestGetBlockDevSymlinkPath(unittest.TestCase): shutil.rmtree(self.tmpdir) def _Test(self, name, idx): - self.assertEqual(backend._GetBlockDevSymlinkPath(name, idx, + self.assertEqual(backend._GetBlockDevSymlinkPath(name, idx=idx, _dir=self.tmpdir), ("%s/%s%s%s" % (self.tmpdir, name, constants.DISK_SEPARATOR, idx))) - def test(self): + def testIndex(self): for idx in range(100): self._Test("inst1.example.com", idx) + def testUUID(self): + uuid = "6bcb6530-3695-47b6-9528-1ed7b5cfbf5c" + iname = "inst1.example.com" + dummy_idx = 6 # UUID should be prefered + expected = "%s/%s%s%s" % (self.tmpdir, iname, + constants.DISK_SEPARATOR, uuid) + link_name = backend._GetBlockDevSymlinkPath(iname, idx=dummy_idx, + uuid=uuid, _dir=self.tmpdir) + self.assertEqual(expected, link_name) + class TestGetInstanceList(unittest.TestCase): -- 1.7.10.4
