Loading...

anaconda-devel-list@redhat.com

[Prev] Thread [Next]  |  [Prev] Date [Next]

Re: [Patch] Adding mirroring in anaconda installer. (kickstart) Joel Andres Granados Mon Sep 03 00:06:48 2007 

Chris Lumens wrote:

I've got no problem with adding this as a kickstart-only feature, though
perhaps Jeremy or Peter might have more things to say along those lines.
However I do have a couple comments about things in your patch.  I've
put them under the relevant parts of the patch.  Don't know a better way
to do this.

Index: fsset.py
===================================================================
RCS file: /usr/local/CVS/anaconda/fsset.py,v
retrieving revision 1.334
diff -u -r1.334 fsset.py
--- fsset.py    17 Aug 2007 14:20:44 -0000      1.334
+++ fsset.py    27 Aug 2007 18:18:05 -0000
@@ -1505,7 +1505,17 @@
                 vgs[entry.device.name] = entry.device
# then set up the logical volumes 
+        nonLVMirrors = []
         for entry in self.entries:
+            #We should do the mirrored stuff first :)
+            if isinstance(entry.device, LogicalVolumeDevice) and 
entry.device.mirrors > 0:
+                vg = None
+                if vgs.has_key(entry.device.vgname):
+                    vg = vgs[entry.device.vgname]
+                entry.device.setupDevice(chroot, vgdevice = vg)
+            else:
+                nonLVMirrors.append(entry)
+        for nonMirrors in nonLVMirrors:
             if isinstance(entry.device, LogicalVolumeDevice):
                 vg = None
                 if vgs.has_key(entry.device.vgname):

Why do we need to do things in two loops here?  Could you not move the
contents of the second for loop into the else case of the first loop?
Or is there a reason the mirrored ones need to be setup before the
non-mirrored ones?


from what I could gather from the lvm code, what LVM2 does when creating a
non mirrored, non stripped LV, that does not specify pvs in the command line 
(For "normal" LV creation the user may or may not specify PVs)
is to choose the first PVs that are valid and have space. This in turn can be a problem 
for the mirrored or stripped LV because those do have specific related PVs.
So, if we don't choose to create the stripped and mirrored volumes first there is a possibility that the PVs that where going to be used in mirroring or stripping will be used in the normal LV. This of course will cause an error when the mirrored 
LV is to be created in a PV that is already used by a normal LV.
^^^^^^^ thats a mouth full :) I always think talking about LVM is weird because 
you always use acronims (LV,PV,VG...)  hope I made myself clear.


@@ -2160,7 +2170,12 @@
class LogicalVolumeDevice(Device): 
     # note that size is in megabytes!
-    def __init__(self, vgname, size, lvname, vg, existing = 0):
+    def __init__(self, vgname, size, lvname, vg, existing = 0, mirrors = 0, 
devs = [], corelog = True):
+        """
+        mirrors - The number of mirrors.
+        devs - Specific devices to use for mirrors.
+        corelog - Wether to use corelog or not.
+        """
         Device.__init__(self)
         self.vgname = vgname
         self.size = size
@@ -2169,6 +2184,9 @@
         self.isSetup = existing
         self.doLabel = None
         self.vg = vg
+        self.mirrors = mirrors #If mirrors is set to 0 mirroring is ignored.
+        self.devs = devs
+        self.corelog = corelog
# these are attributes we might want to expose. or maybe not. 
         # self.chunksize
@@ -2179,7 +2197,7 @@
def setupDevice(self, chroot="/", devPrefix='/tmp', vgdevice = None): 
         if not self.isSetup:
-            lvm.lvcreate(self.name, self.vgname, self.size)
+            lvm.lvcreate(self.name, self.vgname, self.size, 
mirrors=self.mirrors, corelog=self.corelog, devs=self.devs)
             self.isSetup = 1
if vgdevice and vgdevice.isNetdev(): 

How about making a new class
MirroredLogicalVolumeDevice(LogicalVolumeDevice) instead?  You'd need to
override the __init__ and setupDevice methods, but everything else would
be the same.  This keeps the mirroring stuff out of the way, though
perhaps other people don't like this idea.
This was a "lets keep everything that is related to the lvdevice in the lv class" decision:) But I don't see any problem in creating another class that inherits from LogicalVolumeDevice I'll work up a patch that does just that and post it to see what you guys think. 


Index: kickstart.py
===================================================================
RCS file: /usr/local/CVS/anaconda/kickstart.py,v
retrieving revision 1.385
diff -u -r1.385 kickstart.py
--- kickstart.py        23 Aug 2007 19:38:51 -0000      1.385
+++ kickstart.py        27 Aug 2007 18:18:05 -0000
@@ -695,7 +735,7 @@
         "poweroff": Reboot,
         "raid": Raid,
         "reboot": Reboot,
-        "repo": commands.repo.F8_Repo,
+        "repo": commands.repo.FC6_Repo,
         "rootpw": RootPw,
         "selinux": SELinux,
         "services": commands.services.FC6_Services,

This must have snuck in due to not having the latest version of
kickstart.py checked out.

Index: lvm.py
===================================================================
RCS file: /usr/local/CVS/anaconda/lvm.py,v
retrieving revision 1.48
diff -u -r1.48 lvm.py
--- lvm.py      21 May 2007 14:30:19 -0000      1.48
+++ lvm.py      27 Aug 2007 18:18:05 -0000
@@ -157,12 +157,15 @@
         log.error("running vgchange failed: %s" %(rc,))
 #        lvmDevicePresent = 0
-def lvcreate(lvname, vgname, size): 
+def lvcreate(lvname, vgname, size, mirrors=0, corelog=True, devs=[]):
     """Creates a new logical volume.
lvname - name of logical volume to create. 
     vgname - name of volume group lv will be in.
     size - size of lv, in megabytes.
+    mirrors - the amount of mirrors to create
+    devs - a list of string representing valid devices of the form 
"/dev/deviceName
+    corelog - whether or not to use corelog.
     """
     global lvmDevicePresent
     if flags.test or lvmDevicePresent == 0:
@@ -171,6 +174,8 @@
     vgscan()
args = ["lvcreate", "-v", "-L", "%dM" %(size,), "-n", lvname, "-An", vgname] 
+    if mirrors > 0:
+        args = args[:4]+["-m","%s"%mirrors, corelog and 
"--corelog"]+args[4:]+devs
     try:
         rc = lvmExec(*args)
     except:

Again, I think here a new method would be much more straightforward.  I
don't like all the trickery with the args array.  Perhaps adding a
mirroredlvcreate() method would be better.  Or at least, you could
change it to something like this:


Ok,  This one I really should change :)


    if mirrors > 0:
        args = ["lvcreate", "-v", ... "-m", mirrors, ...]
    else:
        args = ["lvcreate", ... ]

Index: partRequests.py
===================================================================
RCS file: /usr/local/CVS/anaconda/partRequests.py,v
retrieving revision 1.74
diff -u -r1.74 partRequests.py
--- partRequests.py     3 Aug 2007 19:32:24 -0000       1.74
+++ partRequests.py     27 Aug 2007 18:18:05 -0000
@@ -930,3 +930,84 @@
+    def getDevice(self, partitions):
+        """Return a device which can be solidified."""
+        devices = []
+        for dev in self.devs:
+            devices.append("/dev/%s"%partitions.getRequestByID(dev).device)
+        vg = partitions.getRequestByID(self.volumeGroup)
+        vgname = vg.volumeGroupName
+        self.dev = fsset.LogicalVolumeDevice(vgname, self.mirroredSize,
+                                             self.logicalVolumeName,
+                                             vg = vg,
+                                             existing = self.preexist,
+                                             mirrors=self.mirrors,
+                                             corelog=self.corelog,
+                                             devs=devices)
+        return self.dev

This is some of the complication I was talking about avoiding earlier
with the MirroredLogicalVolumeDevice class.


I'll make the extra patch and see how things play out :)


I have a general comment about error messages, but I can go back and
clean those little things up afterwards.  I don't think that affects the
patch in the least.

On your pykickstart patch, you got the important part in the command
parser done, but there's a couple other things.  You'll want to make a
new F8_LogVolData to hold the new attributes you created, 

I'll include this on my new patch.


and then you
will want to edit pykickstart/handlers/control.py to make sure F8 uses
these two new classes. 

Ok, I'll also include this on the new patch.


It's a little less straightforward than I would
like.

- Chris

_______________________________________________
Anaconda-devel-list mailing list
[EMAIL PROTECTED]
https://www.redhat.com/mailman/listinfo/anaconda-devel-list



--
Joel Andres Granados

_______________________________________________
Anaconda-devel-list mailing list
[EMAIL PROTECTED]
https://www.redhat.com/mailman/listinfo/anaconda-devel-list