[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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:
> 
> 1) Makes singlePV a boolean attribute on the PartSpec and
>    LVMLogicalVolumeDevice objects.
> 
> 2) Key on the singlePV attribute rather than the mountpoint so the
>    support for logical volumes on a single PV is a little more generic.
> 
> 3) 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.



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]