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 anon 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 = 1if 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 anew 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 usesthese 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
- Re: [Patch] Adding mirroring in anaconda installer. (kickstart) Chris Lumens 2007/09/03
- Re: [Patch] Adding mirroring in anaconda installer. (kickstart) Joel Andres Granados 2007/09/03 <=
- Re: [Patch] Adding mirroring in anaconda installer. (kickstart) Joel Andres Granados 2007/09/03