Re: [PATCH 4/4] Make singlePV a more useful boolean, clean up _getSinglePV()

[David Lehman]

On Wed, 2011-02-02 at 13:30 -1000, David Cantrell wrote:
> Followup again to the "/boot on LVM" commits for s390x.  The feature
> request itself is odd, but it's close to a morbid curiosity now as to
> whether or not it'll actually work.  The main limitation with /boot on
> LVM on s390x is that the logical volume must be tied to a single
> physical volume in the volume group.  Combined with the sorting change
> commit, the patch does the following:
>
> Makes singlePV a boolean attribute on the PartSpec and
> LVMLogicalVolumeDevice objects.
>
> Key on the singlePV attribute rather than the mountpoint so the
> support for logical volumes on a single PV is a little more generic.
>
> Modify _getSinglePV() to read the output of the vgs(8) command and
> find a PV with enough free space on it for the logical volume we are
> creating.  Once we find one, return it.  If we don't find one, raise
> an ugly exception and explain things to the user.
>
> Now, the big gap here is what happens if there are no physical volumes
> of at least 500MB, since we are limiting this singlePV stuff to /boot on
> s390x?  The answer is the user can get the exception described in #3 and
> do manual partitioning.  I personally have never seen a DASD under 2GB,
> but I'm sure it's possible.  I don't really care.  I've fulfilled the
> morbid curiosity of this request and know it works.

Looks okay with one suggestion, below...
> Related: rhbz#618376
>
> platform.py             |    3 ++-
>  storage/devices.py      |   45 +++++++++++++++++++++++++++++++++------------
>  storage/errors.py       |    3 +++
>  storage/partitioning.py |    3 ++-
>  storage/partspec.py     |   14 +++++++++++---
>  5 files changed, 51 insertions(+), 17 deletions(-)
>
> diff --git a/platform.py b/platform.py
> index 1d38475..8772b57 100644
> --- a/platform.py
> +++ b/platform.py
> @@ -483,7 +483,8 @@ class S390(Platform):
>      def setDefaultPartitioning(self):
>          """Return the default platform-specific partitioning information."""
>          return [PartSpec(mountpoint="/boot", fstype=self.defaultBootFSType, size=500,
>                     weight=self.weight(mountpoint="/boot"), asVol=True)]
>                     weight=self.weight(mountpoint="/boot"), asVol=True,
>                     singlePV=True)]
>
> def weight(self, fstype=None, mountpoint=None):
>        if mountpoint and mountpoint == "/boot":
>
> diff --git a/storage/devices.py b/storage/devices.py
> index 3852f9c..38f40b3 100644
> --- a/storage/devices.py
> +++ b/storage/devices.py
> @@ -2220,7 +2220,8 @@ class LVMLogicalVolumeDevice(DMDevice):
>      def __init__(self, name, vgdev, size=None, uuid=None,
>                   stripes=1, logSize=0, snapshotSpace=0,
>                   format=None, exists=None, sysfsPath='',
>             grow=None, maxsize=None, percent=None):
>             grow=None, maxsize=None, percent=None,
>             singlePV=False):
>    """ Create a LVMLogicalVolumeDevice instance.
>
>        Arguments:
>
> @@ -2238,6 +2239,7 @@ class LVMLogicalVolumeDevice(DMDevice):
>                  sysfsPath -- sysfs device path
>                  format -- a DeviceFormat instance
>                  exists -- indicates whether this is an existing device
>            singlePV -- if true, maps this lv to a single pv
>
>            For new (non-existent) LVs only:
>
> @@ -2261,6 +2263,7 @@ class LVMLogicalVolumeDevice(DMDevice):
>          self.snapshotSpace = snapshotSpace
>          self.stripes = stripes
>          self.logSize = logSize
>    self.singlePV = singlePV
>
>    self.req_grow = None
>    self.req_max_size = 0
>
> @@ -2427,16 +2430,35 @@ class LVMLogicalVolumeDevice(DMDevice):
>                  log.debug("vg %s teardown failed; continuing" % self.vg.name)
>  def _getSinglePV(self):
>    pvs = []
>    paths = []
>    for parent in self.parents:
>        pvs += parent.parents
>    paths = map(lambda x: x.path, pvs)
>    # NOTE: This runs the pvs(8) command and outputs the PV device
>    # name, VG name, and amount of free space on the PV in megabytes.
>    # A change to the Size class will require more handling of the
>    # units switch in this arg list.
>    args = ["pvs", "--noheadings", "--nosuffix", "--aligned",
>            "--units", "M", "-o", "pv_name,vg_name,pv_free"]
>    buf = iutil.execWithCapture("lvm", args, stderr="/dev/tty5")
>    for line in buf.splitlines():
>        fields = filter(lambda x: x != '', line.strip().split())
>        if len(fields) != 3 or fields[1] != self.vg.name:
>            continue
>        if float(fields[2]) >= self.size:
>            return [fields[0]]             # lvm.lvcreate needs a list
>    if hasattr(self.format, "mountpoint"):
>        mountpoint = self.format.mountpoint
>    else:
>        mountpoint = self.format.type

You should probably use self.format.name here instead to get the more
user-friendly names for things like mdmember, lvmpv, and luks formats.