Related to rhinstaller/pykickstart/pull/29
From: David Lehman dlehman@redhat.com
Related: rhbz#1113207 --- blivet/autopart.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
diff --git a/blivet/autopart.py b/blivet/autopart.py index 1359ba0..206e179 100644 --- a/blivet/autopart.py +++ b/blivet/autopart.py @@ -25,8 +25,10 @@
from . import util from .size import Size +from .devices.btrfs import BTRFSSnapShotDevice from .devices.partition import PartitionDevice, FALLBACK_DEFAULT_PART_SIZE from .devices.luks import LUKSDevice +from .devices.lvm import LVMThinSnapShotDevice from .errors import NoDisksError, NotEnoughFreeSpaceError from .formats import getFormat from .partitioning import doPartitioning, getFreeRegions, growLVM @@ -509,3 +511,42 @@ def doAutoPartition(storage, data, min_luks_entropy=0): # only newly added swaps should appear in the fstab new_swaps = (dev for dev in storage.swaps if not dev.format.exists) storage.setFstabSwaps(new_swaps) + +def setUpRootSnapShot(storage): + """ Snapshot the root device and remount with the snapshot as root. """ + log.info("makeRootThinPSnapShot: autopart type is %s", storage.autoPartType) + try: + make_snapshot = (storage.ksdata.autopart.autopart and + storage.ksdata.autopart.snapshot) + except AttributeError: + make_snapshot = False + + if not make_snapshot: + return + + # set up the snapshot + root = storage.rootDevice + parent = getattr(root, "pool", root.container) + name = storage.suggestDeviceName(parent=parent, prefix="rootsnap") + + if storage.ksdata.autopart.type == AUTOPART_TYPE_BTRFS: + snapshot = BTRFSSnapShotDevice(name, parents=[parent], source=root) + elif storage.ksdata.autopart.type == AUTOPART_TYPE_LVM_THINP: + snapshot = LVMThinSnapShotDevice(name, parents=[parent], origin=root) + else: + log.error("invalid autopart type for root snapshot: %s", + storage.ksdata.autopart.type) + return + + # create the snapshot + storage.createDevice(snapshot) + storage.devicetree.processActions() + + # remount the system with the snapshot as the root device + storage.umountFilesystems() + + root.format.mountpoint = "" + snapshot.format.mountpoint = "/" + + storage.write() + storage.mountFilesystems()
We should probably call ``xfs_admin -U generate`` on the snapshot of the root filesystem before mounting it. See e.g. http://www.miljan.org/main/2009/11/16/lvm-snapshots-and-xfs/ for more details.
TL;DR XFS filesystems have UUIDs that are used in kernel for some mapping and thus two filesystems with the same UUID cannot be mounted at the same time. One of them needs to have the UUID changed (as suggested above) or mounts have to be done with ``-o nouuid``.
This code is not going to hit the issue because the origin is unmounted before the snapshot is mounted, but it could cause troubles later if somebody tries to mount the origin with the snapshot being already mounted (e.g. as root).
From: David Lehman dlehman@redhat.com
Non-existent lvm snapshots have a copy of the origin's format as their format. When an origin's format changes, all non-existent snapshots of that origin must have their formats updated as well.
Related: rhbz#1113207 --- blivet/devices/lvm.py | 43 +++++++++++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 12 deletions(-)
diff --git a/blivet/devices/lvm.py b/blivet/devices/lvm.py index 3b94a22..87252e2 100644 --- a/blivet/devices/lvm.py +++ b/blivet/devices/lvm.py @@ -22,6 +22,7 @@ from decimal import Decimal from six import add_metaclass import abc +import copy import pprint import re import os @@ -32,7 +33,6 @@
from .. import errors from .. import util -from ..formats import getFormat from ..storage_log import log_method_call from .. import udev from ..size import Size, KiB, MiB, ROUND_UP, ROUND_DOWN @@ -500,6 +500,7 @@ def __init__(self, name, parents=None, size=None, uuid=None, if not isinstance(container, self._containerClass): raise ValueError("constructor requires a %s instance" % self._containerClass.__name__)
+ self.snapshots = [] DMDevice.__init__(self, name, size=size, fmt=fmt, sysfsPath=sysfsPath, parents=parents, exists=exists) @@ -509,7 +510,6 @@ def __init__(self, name, parents=None, size=None, uuid=None, self.logSize = logSize or Size(0) self.metaDataSize = Size(0) self.segType = segType or "linear" - self.snapshots = []
self.req_grow = None self.req_max_size = Size(0) @@ -580,6 +580,11 @@ def vgSpaceUsed(self): return (self.vg.align(self.size, roundup=True) * self.copies + self.logSize + self.metaDataSize)
+ def _setFormat(self, fmt): + super(LVMLogicalVolumeDevice, self)._setFormat(fmt) + for snapshot in (s for s in self.snapshots if not s.exists): + snapshot._updateFormatFromOrigin() + @property def vg(self): """ This Logical Volume's Volume Group. """ @@ -811,8 +816,8 @@ class LVMSnapShotBase(object): 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. + It is also impossible to set the format for a non-existent snapshot + explicitly as it always has the same format as its origin. """ _type = "lvmsnapshotbase"
@@ -855,15 +860,29 @@ def _voriginExistenceCheck(self, vorigin, exists): if vorigin and not exists: raise ValueError("only existing vorigin snapshots are supported")
- def _setFormat(self, fmt): - pass + def _updateFormatFromOrigin(self): + """ Update the snapshot's format to reflect the origin's. """ + fmt = copy.deepcopy(self.origin.format) + fmt.exists = False + if hasattr(fmt, "mountpoint"): + fmt.mountpoint = "" + fmt._chrootedMountpoint = None + # pylint: disable=no-member + fmt.device = self.path + + super(LVMSnapShotBase, self)._setFormat(fmt)
- def _getFormat(self): - if self.origin is None: - fmt = getFormat(None) + def _setFormat(self, fmt): + # If a snapshot exists it can have a format that is distinct from its + # origin's. If it does not exist its format must be a copy of its + # origin's. + # pylint: disable=no-member + if self.exists: + super(LVMSnapShotBase, self)._setFormat(fmt) else: - fmt = self.origin.format - return fmt + # pylint: disable=no-member + log.info("copying %s origin's format", self.name) + self._updateFormatFromOrigin()
@abc.abstractmethod def _create(self): @@ -1220,7 +1239,7 @@ def __init__(self, name, parents=None, sysfsPath='', origin=None,
LVMSnapShotBase.__init__(self, origin=origin, exists=exists) LVMThinLogicalVolumeDevice.__init__(self, name, parents=parents, - sysfsPath=sysfsPath,fmt=None, + sysfsPath=sysfsPath,fmt=fmt, segType=segType, uuid=uuid, size=size, exists=exists)
In reply to line 503 of blivet/devices/lvm.py:
A comment explaining the dependency between snapshots and formats might be helpful here. I'm guessing that's why it had to be moved above super constructor call.
I think that all the pylint no-member annotations should be on the same line as the code that they effect. Right now, the one on line 883 is totally redundant, since the one at 879 has already disabled all no-member warnings.
In reply to line 864 of blivet/devices/lvm.py:
There's a precondition on this method, that the snapshot must not yet exist. I think it would be helpful to document that in the docstring.
From: David Lehman dlehman@redhat.com
Related: rhbz#1113207 --- blivet/devices/lvm.py | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/blivet/devices/lvm.py b/blivet/devices/lvm.py index 87252e2..aac570f 100644 --- a/blivet/devices/lvm.py +++ b/blivet/devices/lvm.py @@ -1265,6 +1265,10 @@ def _create(self): blockdev.lvm.thsnapshotcreate(self.vg.name, self._name, self.origin.lvname, pool_name=pool_name)
+ def _postCreate(self): + super(LVMThinSnapShotDevice, self)._postCreate() + self.format.exists = True + def dependsOn(self, dep): # once a thin snapshot exists it no longer depends on its origin return ((self.origin == dep and not self.exists) or
This makes sense, but an explanatory comment seems like a good idea.
This makes sense, but an explanatory comment seems like a good idea.
Yes, I agree. I think the commit message should be copied over to the code as a comment.
From: David Lehman dlehman@redhat.com
Related: rhbz#1113207 --- blivet/devices/lvm.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blivet/devices/lvm.py b/blivet/devices/lvm.py index aac570f..d85e0d3 100644 --- a/blivet/devices/lvm.py +++ b/blivet/devices/lvm.py @@ -1262,7 +1262,7 @@ def _create(self): # to use pool_name = self.pool.lvname
- blockdev.lvm.thsnapshotcreate(self.vg.name, self._name, self.origin.lvname, + blockdev.lvm.thsnapshotcreate(self.vg.name, self.origin.lvname, self._name, pool_name=pool_name)
def _postCreate(self):
From: David Lehman dlehman@redhat.com
Related: rhbz#1113207 --- blivet/osinstall.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/blivet/osinstall.py b/blivet/osinstall.py index 1c63302..20b425c 100644 --- a/blivet/osinstall.py +++ b/blivet/osinstall.py @@ -605,7 +605,7 @@ def umountFilesystems(self, swapoff=True): devices = list(self.mountpoints.values()) + self.swapDevices devices.extend([self.dev, self.devshm, self.devpts, self.sysfs, self.proc, self.usb, self.selinux, self.run]) - devices.sort(key=lambda d: getattr(d.format, "mountpoint", None)) + devices.sort(key=lambda d: getattr(d.format, "mountpoint", "")) devices.reverse() for device in devices: if (not device.format.mountable) or \
I don't think this change makes a difference.
It makes a bit more sense to throw out everything that lacks a mountpoint attribute, like:
``` devices = [d for d in devices if hasattr(d.format, "mountpoint")] devices.sort(key=lambda d: d.format.mountpoint) ```
That way, the code below that accesses the attribute directly, as:
``` if not device.format.mountable or not device.format.mountpoint: ```
won't look so suspicious.
This change just brings umountFilesystems in line with mountFilesystems. Swap devices have no mountpoint and it's no less work to save the swaps until after the sort, so I'd just assume keep it like it is.
I forgot to mention that this avoids a TypeError from sort trying to compare str and None. Apparently anaconda doesn't use umountFilesystems anymore.
The line numbers deceived me and I ended up looking at the wrong method. It's _mountFilesystems_ that's the suspicious looking one.
You might want to mention the TypeError in the commit message. It's only in Python3 that NoneType and str type objects are incomparable. In Python 2 they compare just fine. Which is probably why no problem was observed until recently.
Otherwise, ack.
Added label: rhel7-branch.
Looks good to me otherwise.
Added label: ACK.
Other than comments above it looks good to me.
anaconda-patches@lists.fedorahosted.org
-
dashea