On Wed, Mar 11, 2009 at 12:12:42PM -0500, David Lehman wrote:
> On Wed, 2009-03-11 at 17:51 +0100, Joel Granados Moreno wrote:
> > * storage/devicelibs/lvm.py (pvstatus_ok): New function that tests the
> > status of a PV. If PV part of a complete VG or not part of a VG at
> > all, we consider it to be OK. otherwise it needs to be cleaned.
>
> This feels like the way we used to do it. Comments inline below.
>
> > * storage/devicelibs/lvm.py (vgreduce): Add the rm=True arg to the
> > vgreduce call. This will allow us to ignore the device list and tell
> > LVM to clean all the additional LVM metadata.
>
> Seems good.
>
> > * storage/devicelibs/lvm.py (zeroLvmMetadata): New function to try to
> > clobber LVM metadata completely. This should be used as a last resort
> > and zeros out the device from 512bytes to 10Mb.
>
> See comments inline.
>
> > * storage/devicetree (questionClobberInconsistengVG): New function. Its
> > the callback used to ask the user if the LVM metadata should be erased
> > from a dirty PV.
>
> Good idea.
>
> > * storage/devicetree (addUdevDevice): Use pvstatus_ok to check for the
> > status of each PV we find.
>
> This will be problematic IMO. See comments inline.
>
> > ---
> > storage/devicelibs/lvm.py | 87 +++++++++++++++++++++++++++++++++++++++++++-
> > storage/devicetree.py | 71 ++++++++++++++++++++++++++++++++++++-
> > 2 files changed, 155 insertions(+), 3 deletions(-)
> >
> > diff --git a/storage/devicelibs/lvm.py b/storage/devicelibs/lvm.py
> > index 0faee10..d4926a6 100644
> > --- a/storage/devicelibs/lvm.py
> > +++ b/storage/devicelibs/lvm.py
> > @@ -49,6 +49,23 @@ def has_lvm():
> >
> > return has_lvm
> >
> > +def zeroLvmMetadata(device):
> > + """ This dds from byte 513 to 100Mb of the disk.
> > +
> > + We assume that lvm has nothing to do with the first 512 bytes.
> > + We assume that 20Mb is enough. My tests failed with 10Mb for
> > + some unknown reason.
> > + """
> > + args = ["if=/dev/zero" , "of=%s"%device] + \
> > + ["seek=1", "bs=512", "count=400"]
> > + rc = iutil.execWithRedirect("dd", args,
> > + stdout = "/dev/tty5",
> > + stderr = "/dev/tty5",
> > + searchPath=1)
> > + if rc:
> > + raise LVMError("dd failed for %s" % device)
> > +
> > +
>
> I see 100Mb, 10Mb, and 20Mb, mentioned here, and your summary says 10Mb.
> Which is it? (I know, it's 20, but the comments need cleanup.)
Yep, this was after I found out that 10Mb did not cut it. will change.
>
> > def getPossiblePhysicalExtents(floor=0):
> > """Returns a list of integers representing the possible values for
> > the physical extent of a volume group. Value is in KB.
> > @@ -171,6 +188,60 @@ def pvinfo(device):
> >
> > return info
> >
> > +def pvstatus_ok(device):
> > + """ Check validity of lvm structure with Physical Volume device.
>
> This is problematic, besides being the way we did things before that so
> often did not work.
>
> At any given time during scanning you cannot expect that all of a VG's
> PVs are available or visible (think LUKS, PV-on-mdraid, &c). The way to
> do this is to check things out after we finish scanning. One simple
> thing to do is add a property 'complete' to LVMVolumeGroupDevice that
> compares the length of self.pvs with self.pvCount (if not None).
>
> Wherever possible, we should build up the device/format classes instead
> of stuffing tons of utility functions into devicelibs/.
>
>
> > -def vgreduce(vg_name, pv_list):
> > - rc = iutil.execWithRedirect("lvm", ["vgreduce", vg_name] + pv_list,
> > +def vgreduce(vg_name, pv_list, rm=False):
> > + """ Reduce a VG.
> > +
> > + rm -> with RemoveMissing option.
> > + Use pv_list when rm=False, otherwise ignore pv_list and call vgreduce with
> > + the --removemissing option.
> > + """
>
> This one is good.
>
> > diff --git a/storage/devicetree.py b/storage/devicetree.py
> > index e974717..fb82e28 100644
> > --- a/storage/devicetree.py
> > +++ b/storage/devicetree.py
> > @@ -135,6 +135,35 @@ def questionInitializeDisk(intf=None, name=None):
> > retVal = True
> > return retVal
> >
> > +def questionClobberInconsistengVG(intf=None, pv_name=None, vg_name=None):
>
> I think it should be "Inconsistent" or "Incomplete".
yep typo :(
>
> > @@ -1168,12 +1197,52 @@ class DeviceTree(object):
> > try:
> > vg_name = udev_device_get_vg_name(info)
> > except KeyError:
> > - # no vg name means no vg -- we're done with this pv
> > + # This means either there are valid PVs with no VG. Or,
> > + # leftover PVs from an incomplete VG.
> > + if not lvm.pvstatus_ok(device.path):
> > + if questionClobberInconsistengVG(intf = self.intf,
> > + pv_name = device.path):
> > + lvm.pvremove(device.path)
> > + # Give the device the a default format
> > + for arg in ["vgName", "vgUuid", "peStart"]:
> > + if kwargs.has_key(arg):
> > + del kwargs[arg]
> > + device.format = formats.getFormat(*[""], **kwargs)
> > +
> > + else:
> > + self.ignoredDisks.append(device.name)
>
> It is fine to do this check, but it should be done at a point when we
> can expect to know everything there is to know.
Yep, the big question is where ? I'll see how things look if I put this
right after the probe thing. We can call it something like "clean" or
"doublecheck" ... and every device can have a check consistency and
make consistent function (most likely the make consistent will have a
default value and a callback for the user)
The general rule being do what we can in probe and perform further fixes
adjustments in another phase that comes after probe.
Note that vgactivate will fail if the pv is part of an incomplete vg.
so catching these exceptions in some whay would be needed in the probe
phase.
let me see what I can come up with.
>
> Dave
>
> _______________________________________________
> Anaconda-devel-list mailing list
> Anaconda-devel-list@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/anaconda-devel-list
thx for the review
--
Joel Andres Granados
Brno, Czech Republic, Red Hat.
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
