On Fri, 2009-11-20 at 11:25 +0100, Hans de Goede wrote:
> Hi Jerry et all,
>
> Below is a review of your patch:
>
>
> diff -up ./backend.py.orig ./backend.py
> --- ./backend.py.orig 2009-11-16 17:58:30.000000000 -0600
> +++ ./backend.py 2009-11-16 18:00:15.000000000 -0600
> @@ -153,29 +153,45 @@ class AnacondaBackend:
> if not flags.setupFilesystems:
> return
>
> - if self._loopbackFile and os.path.exists(self._loopbackFile):
> - return
> + log.info("mediaDevice is %s" % anaconda.mediaDevice )
> + # If they booted with a boot.iso, just continue using that install.img.
>
> - # If we've booted off the first CD/DVD (so, not the boot.iso) then
> - # copy the install.img to the filesystem and switch loopback devices
> - # to there. Otherwise we won't be able to unmount and swap media.
> - if not anaconda.mediaDevice or not os.path.exists(installimg):
> + if os.path.exists("/mnt/stage2/images/install.img"):
> + log.info("Don't need to transfer stage2 image")
> return
>
>
> This is not correct, we may need to unmount stage2 when it is an install CD / DVD
> as we may need to change DVD / CD as the needed packages may be split over multiple
> disks.
>
Shoot, blew that one, I was focusing in on the hd install method. That
is an easy fix.
> -
> - free = anaconda.id.storage.fsFreeSpace
> +
> + log.info("Using %s as stage2 image" % installimg)
> self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
> - free[0][0])
> + "/tmp")
> + log.info("Full image path is %s" % self._loopbackFile )
> +
>
> So you're hardcoding the use of /tmp here, but there are no ideas /tmp is a
> good candidate in case of an upgrade it might even be a tmpfs for people who
> have modified their fstab to make it such.
>
Your saying that anaconda will mount whatever is in the /etc/fstab in an
upgrade situation? Just wondering, how about NFS shares? In any case how
about creating a directory on /mnt/sysimage to hold self._loopbackFile?
Then clean that up when done, like some sort of scratch space. Or just
dump it into /?
> You are on to something here, the old code is using the first
> element of anaconda.id.storage.fsFreeSpace, which is sorted by size, but
> that is the element with the smallest size! I think this is a bug and the
> code should use the last element
>
Glad something came out positive, if that is fixed then the above change
is unneeded.
> + if self._loopbackFile and os.path.exists(self._loopbackFile):
> + log.info("and exists and removing %s" % self._loopbackFile )
> + try:
> + os.unlink(self._loopbackFile)
> + except:
> + pass
> +
>
> Not sure what you are trying to do here, self._loopbackFile is None (so false)
> the first time we go through this code path, and if we go through this code
> path multiple times, it should be a nop the second time (see the
> check for this you removed above).
>
Before coping the running install.img, if there is self._loopbackFile
present (from a previously failed install attempt), remove it. Think the
original check made a bad assumption, this file may not be the same
version as what is need by the currently booted version of anaconda.
self.anaconda.backend.mountInstallImage is only called once from
yuminstall.py
> + # This s/b the one that was copied into /tmp ....
> + # Why was this copied to ram when you have passed stage2=
> + # lets free the RAM
> + if os.path.exists("/tmp/install.img"):
> + log.info("OVERRIDE Using /tmp/install.img as stage2 image")
> + stage2img = "/tmp/install.img"
> + stage2ram = 1
> + else:
> + stage2img = installimg
>
> Ok, you've alsmost completely lost me here, I have some clue what
> you are trying to do, but it is completely unrelated to the previous parts
> of this patch.
>
No, it's related, you need to have the source for install.img, that can
live on cd/dvd media, or for hard-drive and net installs in /tmp. I want
to recover the ram from /tmp. It's best if we copy the one that is
currently mounted by anaconda.
> Please make *ONE* change per patch, and add a lot more verbose description
> of the why, what and how of the patch. So put in the description:
>
> 1) What you are trying to fix
> 2) What is the current behaviour (if applicable in various scenarios)
> 3) What are you changing and how does this fix this.
> 4) How does this change impact other install scenarios ?
>
Sorry, should of made incremental patches, I'll break up the patch, to
be more readable with some revisions.
>
> try:
> win = anaconda.intf.waitWindow(_("Copying File"),
> - _("Transferring install image to hard drive"))
> - shutil.copyfile(installimg, self._loopbackFile)
> + _("Transferring install image to hard drive..."))
> + shutil.copyfile(stage2img, self._loopbackFile)
> win.pop()
> except Exception, e:
> if win:
> win.pop()
>
> - log.critical("error transferring install.img: %s" %(e,))
> + log.critical("error transferring stage2img: %s" %(e,))
>
> if isinstance(e, IOError) and e.errno == 5:
> msg = _("An error occurred transferring the install image "
> @@ -195,7 +211,21 @@ class AnacondaBackend:
> return 1
>
> isys.lochangefd("/dev/loop0", self._loopbackFile)
> - isys.umount("/mnt/stage2")
> +
This frees up /boot for preupgrade,
> + # install.img is now in /tmp get rid of the original in boot
> + if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
> + log.info("REMOVING install.img from /mnt/sysimage/boot/upgrade")
> + os.unlink("/mnt/sysimage/boot/upgrade/install.img")
> +
This frees up the ram
> + if stage2ram:
> + try:
> + os.unlink(stage2img)
> + except:
> + pass
> + try:
> + isys.umount("/mnt/stage2")
> + except:
> + pass
>
> def removeInstallImage(self):
> if self._loopbackFile:
>
>
> diff -up ./yuminstall.py.orig ./yuminstall.py
> --- ./yuminstall.py.orig 2009-11-16 17:59:45.000000000 -0600
> +++ ./yuminstall.py 2009-11-16 18:00:15.000000000 -0600
> @@ -625,6 +625,9 @@ class AnacondaYum(YumSorter):
>
>
> def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
> + stage2img = "%s/images/install.img" % self.tree
> + self.anaconda.backend.mountInstallImage(self.anaconda, stage2img)
> +
> if hasattr(self, "preconf"):
> self.preconf.fn = fn
> self.preconf.root = root
> @@ -828,12 +831,6 @@ class AnacondaYum(YumSorter):
> mkeys = self.tsInfo.reqmedia.keys()
> mkeys.sort(mediasort)
>
> - if len(mkeys) > 1:
> - stage2img = "%s/images/install.img" % self.tree
> - if self.anaconda.backend.mountInstallImage(self.anaconda, stage2img):
> - self.anaconda.id.storage.umountFilesystems()
> - return DISPATCH_BACK
> -
> for i in mkeys:
> self.tsInfo.curmedia = i
> if i > 0:
>
>
> Again, this seems yet another changed put in one and the same patch without any
> explanation. Please split this patch up and thoroughly explain the reasoning behind
> the patch. Also it seems wrong, as you are now transfering the image to HD, even when
> we are doing a single dvd install, eating all that precious hard disk space you
> are trying to avoid us running out of.
>
Like you stated above, you may have to change disks, you don't have to
right now with a dvd, but how long before you need 2 DVDs? The move to
doConfigSetup is based on the fact that net and hdiso installs hold the
install image in /tmp. In order to make that ram available for yum/rpm,
I figured that is the earliest point to trigger that call to backend.py
where that could now take place. I think the trade off of having the
memory recovered, is a good one for using that space on the HD. At least
the behavior is the now the same between dvd and cdroms.
Revised patches, the numbers reflex the apply order.
Thanks for taking the time,
Jerry
diff -up ./backend.py.orig ./backend.py
--- ./backend.py.orig 2009-11-20 08:42:58.000000000 -0600
+++ ./backend.py 2009-11-20 09:45:13.000000000 -0600
@@ -153,29 +153,49 @@ class AnacondaBackend:
if not flags.setupFilesystems:
return
- if self._loopbackFile and os.path.exists(self._loopbackFile):
- return
+ log.info("mediaDevice is %s" % anaconda.mediaDevice )
- # If we've booted off the first CD/DVD (so, not the boot.iso) then
- # copy the install.img to the filesystem and switch loopback devices
- # to there. Otherwise we won't be able to unmount and swap media.
- if not anaconda.mediaDevice or not os.path.exists(installimg):
- return
+ # You need to have the source for install.img, that can live on
+ # cd/dvd media, or for hard-drive and net installs in /tmp.
+ # This was called with /mnt/stage2/images/install.img by default.
+
+ if os.path.exists("/tmp/install.img"):
+ log.info("OVERRIDE Using /tmp/install.img as stage2 image")
+ stage2img = "/tmp/install.img"
+ stage2ram = 1
+ else:
+ stage2img = installimg
+ log.info("Using %s as stage2 image" % installimg)
- free = anaconda.id.storage.fsFreeSpace
+ ## TODO Fix the /tmp hardcode
self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath,
- free[0][0])
+ "/tmp")
+ log.info("Full image path is %s" % self._loopbackFile )
+
+ # Before coping the running install.img, if there is
+ # self._loopbackFile present (from a previously failed
+ # install attempt), remove it. Think the original check removed
+ # above made a bad assumption, this file may not be the same
+ # version as what is need by the currently booted version of
+ # anaconda.
+
+ if self._loopbackFile and os.path.exists(self._loopbackFile):
+ log.info("and exists and removing %s" % self._loopbackFile )
+ try:
+ os.unlink(self._loopbackFile)
+ except:
+ pass
try:
win = anaconda.intf.waitWindow(_("Copying File"),
_("Transferring install image to hard drive"))
- shutil.copyfile(installimg, self._loopbackFile)
+ shutil.copyfile(stage2img, self._loopbackFile)
win.pop()
except Exception, e:
if win:
win.pop()
- log.critical("error transferring install.img: %s" %(e,))
+ log.critical("error transferring stage2img: %s" %(e,))
if isinstance(e, IOError) and e.errno == 5:
msg = _("An error occurred transferring the install image "
diff -up ./backend.py.orig ./backend.py
--- ./backend.py.orig 2009-11-20 08:42:58.000000000 -0600
+++ ./backend.py 2009-11-20 09:47:34.000000000 -0600
@@ -195,6 +215,14 @@ class AnacondaBackend:
return 1
isys.lochangefd("/dev/loop0", self._loopbackFile)
+
+ # lets free up some RAM
+ if stage2ram:
+ try:
+ os.unlink(stage2img)
+ except:
+ pass
+
isys.umount("/mnt/stage2")
def removeInstallImage(self):
diff -up ./backend.py.orig ./backend.py
--- ./backend.py.orig 2009-11-20 09:54:15.000000000 -0600
+++ ./backend.py 2009-11-20 09:54:32.000000000 -0600
@@ -223,6 +223,12 @@ class AnacondaBackend:
except:
pass
+ # install.img is now in /mnt/sysimage/tmp get rid of the
+ # original in /mnt/sysimage/boot if present. preupgrade fix
+ if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
+ log.info("REMOVING install.img from /mnt/sysimage/boot/upgrade")
+ os.unlink("/mnt/sysimage/boot/upgrade/install.img")
+
isys.umount("/mnt/stage2")
def removeInstallImage(self):
diff -up ./yuminstall.py.orig ./yuminstall.py
--- ./yuminstall.py.orig 2009-11-20 08:42:35.000000000 -0600
+++ ./yuminstall.py 2009-11-20 09:32:24.000000000 -0600
@@ -625,6 +625,16 @@ class AnacondaYum(YumSorter):
def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
+ # Moved mountInstallImage to occur earlier and removed
+ # check for more that one media image. This forces all
+ # install methods to call mountInstallImage. Changed the
+ # default is to use the one in /mnt/stage2 as the iso image
+ # (/mnt/source) may not be the same version as the booted
+ # installer in the case of nfsiso or hdiso.
+
+ stage2img = "/mnt/stage2/images/install.img"
+ self.anaconda.backend.mountInstallImage(self.anaconda, stage2img)
+
if hasattr(self, "preconf"):
self.preconf.fn = fn
self.preconf.root = root
@@ -828,12 +838,6 @@ class AnacondaYum(YumSorter):
mkeys = self.tsInfo.reqmedia.keys()
mkeys.sort(mediasort)
- if len(mkeys) > 1:
- stage2img = "%s/images/install.img" % self.tree
- if self.anaconda.backend.mountInstallImage(self.anaconda, stage2img):
- self.anaconda.id.storage.umountFilesystems()
- return DISPATCH_BACK
-
for i in mkeys:
self.tsInfo.curmedia = i
if i > 0:
_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
