Overall, I really like it. I have a couple of nits to pick and a couple
of minor concerns, noted below.
On Wed, 2009-03-18 at 19:23 +0100, Joel Granados Moreno wrote:
> * storage/__init__.py (createSuggestedVGName): Take into account the new
> lvm black list.
>
> * storage/devicelibs/lvm.py (zeroLvmMetadata): zero out the LVM metadata
> only. This basically means from 513 byte and on. This will also
> ignore the firs 512 bytes if we are in a partition, but that does not
> matter because LVM does not touch those bits anyways.
This seems like an unnecessary addition to me. We can just use
DeviceFormat.destroy, which zeros the first and last 1MB of a device.
It might need that handling for a too-small device that you have in the
lvm function, though.
> * storage/devicelibs/lvm.py (blacklistVG): New function, add a VG to a
> the black list.
> * storage/devicelibs/lvm.py (vgreduce): introduces a new argument to the
> function. rm means use the lvm --removemissing option.
>
> * storage/devices.py (LVMVolumeGroupDevice.complete): New function to
> evaluate if VG is consistent.
> * storage/devices.py (LVMLogicalVolumeDevice.complete): Likewise for
> LVs.
>
> * storage/devicetree.py (_handleInconsistencies): New function intended to
> catch all unwanted behavior from the system before continuing.
> * storage/devicetree.py (questionReinitILVM): New function intended
> to ask the user what to do in case we find inconsistent LVM metadata.
>
> * storage/formats/lvmpv.py (LVMPhysicalVolume.destroy): Use the
> zeroLvmMetaData in case pvremove fails.
> ---
> storage/__init__.py | 6 ++-
> storage/devicelibs/lvm.py | 47 ++++++++++++++---
> storage/devices.py | 32 +++++++++++-
> storage/devicetree.py | 124 ++++++++++++++++++++++++++++++++++++++++++++-
> storage/formats/lvmpv.py | 6 ++-
> 5 files changed, 200 insertions(+), 15 deletions(-)
>
...
> diff --git a/storage/devices.py b/storage/devices.py
> index 0922778..1a16a0e 100644
> --- a/storage/devices.py
> +++ b/storage/devices.py
> @@ -1632,9 +1632,15 @@ class LVMVolumeGroupDevice(DMDevice):
>
> self.teardown()
>
> - lvm.vgremove(self.name)
> - self.notifyKernel()
> - self.exists = False
> + # this sometimes fails for some reason.
> + try:
> + lvm.vgreduce(self.name, [], rm=True)
> + lvm.vgremove(self.name)
> + except lvm.LVMErorr:
> + raise DeviceError("Could not completely remove VG %s" % self.name)
> + finally:
> + self.notifyKernel()
> + self.exists = False
This looks like something we should figure out. Maybe with program.log
this can be identified and sorted out?
>
> def reduce(self, pv_list):
> """ Remove the listed PVs from the VG. """
> @@ -1763,6 +1769,16 @@ class LVMVolumeGroupDevice(DMDevice):
> """ A list of this VG's LVs """
> return self._lvs[:] # we don't want folks changing our list
>
> + @property
> + def complete(self):
> + """Check if the vg has all its pvs in the system
> + Return True if complete.
> + """
> + if len(self.pvs) == self.pvCount:
> + return True
> + else:
> + return False
self.pvCount is only meaningful for pre-existing devices, so something
list this:
return len(self.pvs) == self.pvCount or not self.exists
You could alternatively turn it into a property that just returns
either len(self.pvs) for non-existent devices or self.pvCount for
preexisting devices.
> +
>
> class LVMLogicalVolumeDevice(DMDevice):
> """ An LVM Logical Volume """
> @@ -1862,6 +1878,16 @@ class LVMLogicalVolumeDevice(DMDevice):
> """ The LV's name (not including VG name). """
> return self._name
>
> + @property
> + def complete(self):
> + """ Test if vg exits and if it has all pvs. """
> + if not self.vg.exists:
> + return False
> +
> + if not self.vg.complete:
> + return False
> + return True
Same as above.
> diff --git a/storage/formats/lvmpv.py b/storage/formats/lvmpv.py
> index cc243eb..ca11130 100644
> --- a/storage/formats/lvmpv.py
> +++ b/storage/formats/lvmpv.py
> @@ -106,7 +106,11 @@ class LVMPhysicalVolume(DeviceFormat):
> raise PhysicalVolumeError("device is active")
>
> # FIXME: verify path exists?
> - lvm.pvremove(self.device)
> + try:
> + lvm.pvremove(self.device)
> + except LVMError:
> + lvm.zeroLvmMetadata(self.path)
> +
> self.exists = False
> self.notifyKernel()
>
This lvm.zeroLvmMetadata could just be changed to:
DeviceFormat.destoy(self, *args, **kwargs)
Otherwise I like it.
Dave
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
