[PATCH 09/17] Add Device classes for snapshots.

David Lehman dlehman at redhat.com
Mon Jun 9 16:51:04 UTC 2014


On 06/06/2014 02:02 AM, Vratislav Podzimek wrote:
> On Thu, 2014-06-05 at 11:47 -0500, David Lehman wrote:
>> ---
>>   blivet/devices.py | 321 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 file changed, 317 insertions(+), 4 deletions(-)
>>
>> diff --git a/blivet/devices.py b/blivet/devices.py
>> index 2e911f8..53a1234 100644
>> --- a/blivet/devices.py
>> +++ b/blivet/devices.py
>> @@ -2537,6 +2537,11 @@ class LVMVolumeGroupDevice(ContainerDevice):
>>           log.debug("Adding %s/%s to %s", lv.name, lv.size, self.name)
>>           self._lvs.append(lv)
>>
>> +        # snapshot accounting
>> +        origin = getattr(lv, "origin", None)
>> +        if origin:
>> +            origin.snapshots.append(lv)
>> +
>>       def _removeLogVol(self, lv):
>>           """ Remove an LV from this VG. """
>>           if lv not in self.lvs:
>> @@ -2547,6 +2552,11 @@ class LVMVolumeGroupDevice(ContainerDevice):
>>           if self.poolMetaData and not self.thinpools:
>>               self.poolMetaData = 0
>>
>> +        # snapshot accounting
>> +        origin = getattr(lv, "origin", None)
>> +        if origin:
>> +            origin.snapshots.remove(lv)
>> +
>>       def _addParent(self, member):
>>           super(LVMVolumeGroupDevice, self)._addParent(member)
>>
>> @@ -2800,6 +2810,7 @@ class LVMLogicalVolumeDevice(DMDevice):
>>           self.metaDataSize = 0
>>           self.singlePV = singlePV
>>           self.segType = segType or "linear"
>> +        self.snapshots = []
>>
>>           self.req_grow = None
>>           self.req_max_size = Size(0)
>> @@ -3013,6 +3024,15 @@ class LVMLogicalVolumeDevice(DMDevice):
>>           udev.udev_settle()
>>           lvm.lvresize(self.vg.name, self._name, self.size)
>>
>> +    @property
>> +    def isleaf(self):
>> +        # Thin snapshots do not need to be removed prior to removal of the
>> +        # origin, but the old snapshots do.
>> +        non_thin_snapshots = any(s for s in self.snapshots
>> +                                    if not isinstance(s, LVMThinSnapShotDevice))
>> +        return (super(LVMLogicalVolumeDevice, self).isleaf and
>> +                not non_thin_snapshots)
>> +
>>       def dracutSetupArgs(self):
>>           # Note no mapName usage here, this is a lvm cmdline name, which
>>           # is different (ofcourse)
>> @@ -3080,6 +3100,165 @@ class LVMLogicalVolumeDevice(DMDevice):
>>
>>           return True
>>
>> +class LVMSnapShotBase(object):
> Do we consider "shot" a standalone word/part of "snapshot"? I think
> LVMSnapshotBase makes more sense than LVMSnapShotBase.

I get really irritated with this issue because it breeds inconsistency. 
My personal preference is to use actual words from the english language 
as the standalone words, which yields SnapShot, MountPoint, &c. It is 
more predictable IMO and is what I've used throughout _most_ of blivet 
thus far.

>
>> +    """ Abstract base class for lvm snapshots
>> +
>> +        This class is intended to be used with multiple inheritance in addition
>> +        to some subclass of :class:`~.StorageDevice`.
>> +
>> +        Snapshots do not have their origin/source volume as parent. They are
>> +        like other LVs except that they have an origin attribute and are in that
>> +        instance's snapshots list.
>> +
>> +        Normal/old snapshots must be removed with their origin, while thin
>> +        snapshots can remain after their origin is removed.
>> +
>> +        It is also impossible to set the format for a snapshot explicitly as it
>> +        always has the same format as its origin.
>> +    """
>> +    _type = "lvmsnapshotbase"
>> +    __metaclass__ = abc.ABCMeta
>> +
>> +    def __init__(self, origin=None, vorigin=False, exists=False):
> I believe I know quite something about LVM snapshots, but I have no idea
> what 'vorigin' here stands for. Maybe a better name for it? Or at least
> a docstring would be welcome.

A docstring can be added. I don't really remember what vorigin snapshots 
are for. What I do know is that they are snapshots that use free space 
from the VG instead of having a real origin/source. I'm guessing they 
are sort of a hackish precursor to thinp.

>
>> +        self._originSpecifiedCheck(origin, vorigin, exists)
>> +        self._originTypeCheck(origin)
>> +        self._originExistenceCheck(origin)
>> +        self._voriginExistenceCheck(vorigin, exists)
>> +
>> +        self.origin = origin
>> +        """ the snapshot's source volume """
>> +
>> +        self.vorigin = vorigin
>> +        """ a boolean flag indicating a vorigin snapshot """
>> +
>> +    def _originSpecifiedCheck(self, origin, vorigin, exists):
>> +        # pylint: disable=unused-argument
>> +        if not origin and not vorigin:
>> +            raise ValueError("lvm snapshot devices require an origin lv")
>> +
>> +    def _originTypeCheck(self, origin):
>> +        if origin and not isinstance(origin, LVMLogicalVolumeDevice):
>> +            raise ValueError("lvm snapshot origin must be a logical volume")
>> +
>> +    def _originExistenceCheck(self, origin):
>> +        if origin and not origin.exists:
>> +            raise ValueError("lvm snapshot origin volume must already exist")
>> +
>> +    def _voriginExistenceCheck(self, vorigin, exists):
>> +        if vorigin and not exists:
>> +            raise ValueError("only existing vorigin snapshots are supported")
>> +
>> +    def _setFormat(self, fmt):
>> +        pass
>> +
>> +    def _getFormat(self):
>> +        if self.origin is None:
>> +            fmt = getFormat(None)
>> +        else:
>> +            fmt = self.origin.format
>> +        return fmt
>> +
>> +    @abc.abstractmethod
>> +    def _create(self):
>> +        """ Create the device. """
>> +        raise NotImplementedError()
>> +
>> +    def merge(self):
>> +        """ Merge the snapshot back into its origin volume. """
>> +        log_method_call(self, self.name, status=self.status) # pylint: disable=no-member
>> +        self.vg.setup()    # pylint: disable=no-member
> Seeing this pylint inhibitors makes me think about the class hierarchy
> we have. Shouldn't the LVMSnapshotBase inherit from the
> LVMLogicalVolumeDevice class somehow? All its children classes have to
> be LVs. It's probably not possible to directly inherit from
> LVMLogicalVolume, but maybe we should have some LVMLogicalVolumeBase
> class and inherit it here so that things like 'self.vg' really make
> sense.
>

The snapshot base class is intended to be sort of an add-in or mix-in 
since thinp has already complicated the lvm class hierarchy 
considerably. I am somewhat tempted to make snapshot code part of 
LVMLogicalVolumeDevice instead of having distinct classes for snapshots.


More information about the anaconda-patches mailing list