From c45dc4d5fabb33370876dacccc6d57e39f611334 Mon Sep 17 00:00:00 2001 From: Rafi Khardalian Date: Fri, 8 Feb 2013 00:31:46 +0000 Subject: Simplify libvirt snapshot code path The "cold" snapshot code path was unnecessarily complex. We were traversing 3 layers of abstraction to reach 3 lines of code. I've completely eliminated snapshot.py by moving all "cold" snapshot functions into imagebackend.py and adjusting the libvirt driver accordingly. This is the first pass of several I intend to make against imagebackend.py, as it is far too difficult to read given how basic the basic level of functionality it is intended to provide. Change-Id: Idfacffb7210ae61193ee4f2360d4b38f330a177c --- nova/tests/fake_libvirt_utils.py | 4 ++ nova/tests/test_libvirt.py | 2 +- nova/tests/test_virt_drivers.py | 2 +- nova/virt/libvirt/driver.py | 15 ++++--- nova/virt/libvirt/imagebackend.py | 65 +++++++++++++++++----------- nova/virt/libvirt/snapshots.py | 89 --------------------------------------- 6 files changed, 56 insertions(+), 121 deletions(-) delete mode 100644 nova/virt/libvirt/snapshots.py diff --git a/nova/tests/fake_libvirt_utils.py b/nova/tests/fake_libvirt_utils.py index b3d842468..285a4b7e3 100644 --- a/nova/tests/fake_libvirt_utils.py +++ b/nova/tests/fake_libvirt_utils.py @@ -144,3 +144,7 @@ def fetch_image(context, target, image_id, user_id, project_id): def get_instance_path(instance): # TODO(mikal): we should really just call the real one here return os.path.join(CONF.instances_path, instance['name']) + + +def pick_disk_driver_name(is_block_dev=False): + return "qemu" diff --git a/nova/tests/test_libvirt.py b/nova/tests/test_libvirt.py index 12b1c1a69..c93bb0168 100644 --- a/nova/tests/test_libvirt.py +++ b/nova/tests/test_libvirt.py @@ -273,7 +273,7 @@ class LibvirtConnTestCase(test.TestCase): 'nova.virt.libvirt.driver.libvirt_utils', fake_libvirt_utils)) self.useFixture(fixtures.MonkeyPatch( - 'nova.virt.libvirt.snapshots.libvirt_utils', + 'nova.virt.libvirt.imagebackend.libvirt_utils', fake_libvirt_utils)) def fake_extend(image, size): diff --git a/nova/tests/test_virt_drivers.py b/nova/tests/test_virt_drivers.py index 67452bc24..a94fdc3c5 100644 --- a/nova/tests/test_virt_drivers.py +++ b/nova/tests/test_virt_drivers.py @@ -84,7 +84,7 @@ class _FakeDriverBackendTestCase(object): 'nova.virt.libvirt.driver.libvirt_utils', fake_libvirt_utils)) self.useFixture(fixtures.MonkeyPatch( - 'nova.virt.libvirt.snapshots.libvirt_utils', + 'nova.virt.libvirt.imagebackend.libvirt_utils', fake_libvirt_utils)) self.useFixture(fixtures.MonkeyPatch( 'nova.virt.libvirt.firewall.libvirt', diff --git a/nova/virt/libvirt/driver.py b/nova/virt/libvirt/driver.py index d37d6e7df..485f661e5 100755 --- a/nova/virt/libvirt/driver.py +++ b/nova/virt/libvirt/driver.py @@ -827,7 +827,8 @@ class LibvirtDriver(driver.ComputeDriver): # can be validated. if self.has_min_version(MIN_LIBVIRT_LIVESNAPSHOT_VERSION, MIN_QEMU_LIVESNAPSHOT_VERSION, - REQ_HYPERVISOR_LIVESNAPSHOT): + REQ_HYPERVISOR_LIVESNAPSHOT) \ + and not source_format == "lvm": live_snapshot = True else: live_snapshot = False @@ -843,15 +844,17 @@ class LibvirtDriver(driver.ComputeDriver): if state == power_state.RUNNING or state == power_state.PAUSED: virt_dom.managedSave(0) + snapshot_backend = self.image_backend.snapshot(disk_path, + snapshot_name, + image_type=source_format) + if live_snapshot: LOG.info(_("Beginning live snapshot process"), instance=instance) else: LOG.info(_("Beginning cold snapshot process"), instance=instance) - snapshot = self.image_backend.snapshot(disk_path, snapshot_name, - image_type=source_format) - snapshot.create() + snapshot_backend.snapshot_create() update_task_state(task_state=task_states.IMAGE_PENDING_UPLOAD) snapshot_directory = CONF.libvirt_snapshots_directory @@ -866,10 +869,10 @@ class LibvirtDriver(driver.ComputeDriver): self._live_snapshot(virt_dom, disk_path, out_path, image_format) else: - snapshot.extract(out_path, image_format) + snapshot_backend.snapshot_extract(out_path, image_format) finally: if not live_snapshot: - snapshot.delete() + snapshot_backend.snapshot_delete() # NOTE(dkang): because previous managedSave is not called # for LXC, _create_domain must not be called. if CONF.libvirt_type != 'lxc' and not live_snapshot: diff --git a/nova/virt/libvirt/imagebackend.py b/nova/virt/libvirt/imagebackend.py index 0d5e8cb66..74148a866 100755 --- a/nova/virt/libvirt/imagebackend.py +++ b/nova/virt/libvirt/imagebackend.py @@ -25,8 +25,8 @@ from nova.openstack.common import fileutils from nova.openstack.common import lockutils from nova import utils from nova.virt.disk import api as disk +from nova.virt import images from nova.virt.libvirt import config as vconfig -from nova.virt.libvirt import snapshots from nova.virt.libvirt import utils as libvirt_utils __imagebackend_opts = [ @@ -129,22 +129,25 @@ class Image(object): self.create_image(call_if_not_exists, base, size, *args, **kwargs) - @abc.abstractmethod - def snapshot(self, name): - """Create snapshot object for this image + def snapshot_create(self): + raise NotImplementedError - :name: snapshot name - """ - pass + def snapshot_extract(self, target, out_format): + raise NotImplementedError + + def snapshot_delete(self): + raise NotImplementedError class Raw(Image): - def __init__(self, instance=None, name=None, path=None): + def __init__(self, instance=None, disk_name=None, path=None, + snapshot_name=None): super(Raw, self).__init__("file", "raw", is_block_dev=False) self.path = (path or os.path.join(libvirt_utils.get_instance_path(instance), - name)) + disk_name)) + self.snapshot_name = snapshot_name def create_image(self, prepare_template, base, size, *args, **kwargs): @lockutils.synchronized(base, 'nova-', external=True, @@ -164,17 +167,25 @@ class Raw(Image): with utils.remove_path_on_error(self.path): copy_raw_image(base, self.path, size) - def snapshot(self, name): - return snapshots.RawSnapshot(self.path, name) + def snapshot_create(self): + pass + + def snapshot_extract(self, target, out_format): + images.convert_image(self.path, target, out_format) + + def snapshot_delete(self): + pass class Qcow2(Image): - def __init__(self, instance=None, name=None, path=None): + def __init__(self, instance=None, disk_name=None, path=None, + snapshot_name=None): super(Qcow2, self).__init__("file", "qcow2", is_block_dev=False) self.path = (path or os.path.join(libvirt_utils.get_instance_path(instance), - name)) + disk_name)) + self.snapshot_name = snapshot_name def create_image(self, prepare_template, base, size, *args, **kwargs): @lockutils.synchronized(base, 'nova-', external=True, @@ -190,8 +201,16 @@ class Qcow2(Image): with utils.remove_path_on_error(self.path): copy_qcow2_image(base, self.path, size) - def snapshot(self, name): - return snapshots.Qcow2Snapshot(self.path, name) + def snapshot_create(self): + libvirt_utils.create_snapshot(self.path, self.snapshot_name) + + def snapshot_extract(self, target, out_format): + libvirt_utils.extract_snapshot(self.path, 'qcow2', + self.snapshot_name, target, + out_format) + + def snapshot_delete(self): + libvirt_utils.delete_snapshot(self.path, self.snapshot_name) class Lvm(Image): @@ -199,7 +218,8 @@ class Lvm(Image): def escape(filename): return filename.replace('_', '__') - def __init__(self, instance=None, name=None, path=None): + def __init__(self, instance=None, disk_name=None, path=None, + snapshot_name=None): super(Lvm, self).__init__("block", "raw", is_block_dev=True) if path: @@ -214,7 +234,7 @@ class Lvm(Image): ' flag to use LVM images.')) self.vg = CONF.libvirt_images_volume_group self.lv = '%s_%s' % (self.escape(instance['name']), - self.escape(name)) + self.escape(disk_name)) self.path = os.path.join('/dev', self.vg, self.lv) self.sparse = CONF.libvirt_sparse_logical_volumes @@ -254,9 +274,6 @@ class Lvm(Image): with excutils.save_and_reraise_exception(): libvirt_utils.remove_logical_volumes(path) - def snapshot(self, name): - return snapshots.LvmSnapshot(self.path, name) - class Backend(object): def __init__(self, use_cow): @@ -275,7 +292,7 @@ class Backend(object): raise RuntimeError(_('Unknown image_type=%s') % image_type) return image - def image(self, instance, name, image_type=None): + def image(self, instance, disk_name, image_type=None): """Constructs image for selected backend :instance: Instance name. @@ -284,9 +301,9 @@ class Backend(object): Optional, is CONF.libvirt_images_type by default. """ backend = self.backend(image_type) - return backend(instance=instance, name=name) + return backend(instance=instance, disk_name=disk_name) - def snapshot(self, path, snapshot_name, image_type=None): + def snapshot(self, disk_path, snapshot_name, image_type=None): """Returns snapshot for given image :path: path to image @@ -294,4 +311,4 @@ class Backend(object): :image_type: type of image """ backend = self.backend(image_type) - return backend(path=path).snapshot(snapshot_name) + return backend(path=disk_path, snapshot_name=snapshot_name) diff --git a/nova/virt/libvirt/snapshots.py b/nova/virt/libvirt/snapshots.py deleted file mode 100644 index c85550eae..000000000 --- a/nova/virt/libvirt/snapshots.py +++ /dev/null @@ -1,89 +0,0 @@ -# vim: tabstop=4 shiftwidth=4 softtabstop=4 - -# Copyright 2012 Grid Dynamics -# All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); you may -# not use this file except in compliance with the License. You may obtain -# a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT -# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the -# License for the specific language governing permissions and limitations -# under the License. - -import abc - -from nova.virt import images -from nova.virt.libvirt import utils as libvirt_utils - - -class Snapshot(object): - @abc.abstractmethod - def create(self): - """Create new snapshot.""" - pass - - @abc.abstractmethod - def extract(self, target, out_format): - """Extract snapshot content to file - - :target: path to extraction - :out_format: format of extraction (raw, qcow2, ...) - """ - pass - - @abc.abstractmethod - def delete(self): - """Delete snapshot.""" - pass - - -class RawSnapshot(object): - def __init__(self, path, name): - self.path = path - self.name = name - - def create(self): - pass - - def extract(self, target, out_format): - images.convert_image(self.path, target, out_format) - - def delete(self): - pass - - -class Qcow2Snapshot(object): - def __init__(self, path, name): - self.path = path - self.name = name - - def create(self): - libvirt_utils.create_snapshot(self.path, self.name) - - def extract(self, target, out_format): - libvirt_utils.extract_snapshot(self.path, 'qcow2', - self.name, target, - out_format) - - def delete(self): - libvirt_utils.delete_snapshot(self.path, self.name) - - -class LvmSnapshot(object): - def __init__(self, path, name): - self.path = path - self.name = name - - def create(self): - raise NotImplementedError(_("LVM snapshots not implemented")) - - def extract(self, target, out_format): - raise NotImplementedError(_("LVM snapshots not implemented")) - - def delete(self): - raise NotImplementedError(_("LVM snapshots not implemented")) -- cgit