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.
- - 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.
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
+ 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).
+ # 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.
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 ?
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") + + # 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") + + 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.
Regards,
Hans
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:
- What you are trying to fix
- What is the current behaviour (if applicable in various scenarios)
- What are you changing and how does this fix this.
- 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
Hi,
On 11/20/2009 06:59 PM, Jerry Vonau wrote:
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
Ah, this is a misunderstanding of what the check actually does, first of all it checks if we have generated a filename for storing install.img somewhere on a partition we are installing to / we are updating. If we have failed in a previous install and we need to transfer install.img, then that filename is not set yet, so we will transfer the current install.img, even if an older one is already in the same place.
This check only stops us from transefering the image if: 1) the filename was already generated (so this is not the first run of this function this install) 2) a file with such name exists (iow the previous transfer attempt was correct).
The current code for this is correct, and unless you have a really good reason to change it you should not.
# 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.
It is related in that has to do with transfering the image, but it is not targeting the exact same purpose / fixing the exact same thing.
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:
- What you are trying to fix
- What is the current behaviour (if applicable in various scenarios)
- What are you changing and how does this fix this.
- 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.
Ack, but your current split up is still not split up per purpose, I think I understand what you are trying to achieve, so let me suggest a split:
1) A patch fixing the current copy code to use the fs with the most free space instead of the least
2) A patch to not only do the image transfer in case of a multiple disk cd / dvd install, but also in the case of a hdiso / net install, so as to free up memory used by install.img (which will be another patch). This patch should only do this when memory is tight! Doing this always is bad, as it is useless on systems with tons of memory, and could potentially even cause issues there with for example diskspace
3) A patch to actually achieve the freeing of memory 2)'s goal is by unlinking the install.img from /tmp
Notice that I'm not going with your suggested just always transfer install.img approach. This is not acceptable IMHO as it causes unnecessary slow down in many cases (net / disk install with plenty of ram, single dvd install), and it has the potential to trigger issues in all these cases which we would not hit if we did not do the transfer.
IOW doing the transfer has a price: 1) it consumes disk space which we may need 2) it causes us to take more "steps" then if we don't and each step we take can (and eventually will) cause issues, so avoiding extra steps where possible is good. 3) it causes a slowdown
So since it is not free we should not do it unless there are good reasons to, so far the only reason we had for doing this was a multiple cd/dvd install, but I'm willing to agree that for things like a network install on a low memory machine it would be a good thing to do too.
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?
Quite long, there is no reason why we should not be able to fix a package set for a compete desktop install on a single dvd for years to come.
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.
The trade off is only a good one if memory is a scarce resource, see above.
Regards,
Hans
On Fri, 2009-11-20 at 21:04 +0100, Hans de Goede wrote:
Hi,
On 11/20/2009 06:59 PM, Jerry Vonau wrote:
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
Ah, this is a misunderstanding of what the check actually does, first of all it checks if we have generated a filename for storing install.img somewhere on a partition we are installing to / we are updating. If we have failed in a previous install and we need to transfer install.img, then that filename is not set yet, so we will transfer the current install.img, even if an older one is already in the same place.
This check only stops us from transefering the image if:
- the filename was already generated (so this is not the first run of this function this install)
- a file with such name exists (iow the previous transfer attempt was correct).
The current code for this is correct, and unless you have a really good reason to change it you should not.
Yup, I misunderstood, badly..
# 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.
It is related in that has to do with transfering the image, but it is not targeting the exact same purpose / fixing the exact same thing.
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:
- What you are trying to fix
- What is the current behaviour (if applicable in various scenarios)
- What are you changing and how does this fix this.
- 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.
Ack, but your current split up is still not split up per purpose, I think I understand what you are trying to achieve, so let me suggest a split:
A patch fixing the current copy code to use the fs with the most free space instead of the least
A patch to not only do the image transfer in case of a multiple disk cd / dvd install, but also in the case of a hdiso / net install, so as to free up memory used by install.img (which will be another patch). This patch should only do this when memory is tight! Doing this always is bad, as it is useless on systems with tons of memory, and could potentially even cause issues there with for example diskspace
A patch to actually achieve the freeing of memory 2)'s goal is by unlinking the install.img from /tmp
Think I got the flow you want for the patches now, I'm digging at the free variable for the first part now. That will take me some time as I don't have much to spare. What I have now is less that ideal, I would like see what is being returned, and just use / for the image till the above has being sorted out. At least for my testing, the rest of my patches depend on that one being in place.
Notice that I'm not going with your suggested just always transfer install.img approach. This is not acceptable IMHO as it causes unnecessary slow down in many cases (net / disk install with plenty of ram, single dvd install), and it has the potential to trigger issues in all these cases which we would not hit if we did not do the transfer.
IOW doing the transfer has a price:
- it consumes disk space which we may need
- it causes us to take more "steps" then if we don't and each step we
take can (and eventually will) cause issues, so avoiding extra steps where possible is good. 3) it causes a slowdown
So since it is not free we should not do it unless there are good reasons to, so far the only reason we had for doing this was a multiple cd/dvd install, but I'm willing to agree that for things like a network install on a low memory machine it would be a good thing to do too.
Cool, then the question is at what amount of ram should this kick in at, I went with MIN_GUI_RAM.
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?
Quite long, there is no reason why we should not be able to fix a package set for a compete desktop install on a single dvd for years to come.
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.
The trade off is only a good one if memory is a scarce resource, see above.
Think I have that addressed in this round of patches against 12.46-2.
Thanks for taking the time,
Jerry
Hi,
On 11/22/2009 05:51 AM, Jerry Vonau wrote:
<snip>
Ack, but your current split up is still not split up per purpose, I think I understand what you are trying to achieve, so let me suggest a split:
A patch fixing the current copy code to use the fs with the most free space instead of the least
A patch to not only do the image transfer in case of a multiple disk cd / dvd install, but also in the case of a hdiso / net install, so as to free up memory used by install.img (which will be another patch). This patch should only do this when memory is tight! Doing this always is bad, as it is useless on systems with tons of memory, and could potentially even cause issues there with for example diskspace
A patch to actually achieve the freeing of memory 2)'s goal is by unlinking the install.img from /tmp
Think I got the flow you want for the patches now, I'm digging at the free variable for the first part now. That will take me some time as I don't have much to spare. What I have now is less that ideal, I would like see what is being returned, and just use / for the image till the above has being sorted out. At least for my testing, the rest of my patches depend on that one being in place.
Oh, but that is easy, simply change: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[0][0]) to: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[len(free)-1][0])
So that you use the last element of the list, instead of the first, so that you get the biggest partition, your testing of this fix is much appreciated :)
Notice that I'm not going with your suggested just always transfer install.img approach. This is not acceptable IMHO as it causes unnecessary slow down in many cases (net / disk install with plenty of ram, single dvd install), and it has the potential to trigger issues in all these cases which we would not hit if we did not do the transfer.
IOW doing the transfer has a price:
- it consumes disk space which we may need
- it causes us to take more "steps" then if we don't and each step we
take can (and eventually will) cause issues, so avoiding extra steps where possible is good. 3) it causes a slowdown
So since it is not free we should not do it unless there are good reasons to, so far the only reason we had for doing this was a multiple cd/dvd install, but I'm willing to agree that for things like a network install on a low memory machine it would be a good thing to do too.
Cool, then the question is at what amount of ram should this kick in at, I went with MIN_GUI_RAM.
That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB, so as to have atleast MIN_GUI_RAM free when the install.img (which is approx 100Mb) lives in RAM.
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?
Quite long, there is no reason why we should not be able to fix a package set for a compete desktop install on a single dvd for years to come.
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.
The trade off is only a good one if memory is a scarce resource, see above.
Think I have that addressed in this round of patches against 12.46-2.
Thanks for taking the time,
You are welcome, thanks for contributing to anaconda. I hope that once you get the hang of it you keep on contributing :)
Here is a review of your latest set of patches:
1freefix.diff: See above
2modcall.diff: Please put the large comment in a commit msg, it would be great if you could learn to use git. (Drop by on #anaconda during CET office hours and I'll help you). Otherwise atleast learn to write commit messages, in a .patch / .diff file you can put text above the --- filename +++ filename
Lines and patch (and other tools) will ignore this, please learn to put some explanation of the patch there start with a single line summary, so for example a good header for 2modcall.diff would be:
### Remove unneeded check from mountInstallImage()
This is a preparation patch for transferring install.img from /tmp to disk in low memory network and hdiso install scenarios to free up memory used by install.img under /tmp.
Currently mountInstallImage only gets called when using cd's, based on yuminstall's run, mkeys.sort(mediasort), if len(mkeys) > 1. mediaDevice has already been validated in yuminstall.py and you can't get here without mounting install.img, so there is no need for the check this patch removes. ###
The second hunk of this patch should be part of the 3th patch (or separate)
3setcall.diff:
1) This does not belong in doConfigSetup(), setup() itself or a new method called from setup() would be a better place.
2) Even if the mountInstallImage() fails you still unlink /tmp/install.img
4boot.diff:
Ah a new trick upi your sleef (atleast to me), nice one. But will this work ? Does the loader (stage1) copy install.img from /boot/upgrade to /tmp ? I'm asking because if it does not, then we will still have the loopback mounted and the unlink will not free up the diskspace.
We are getting somewhere, I think this will greatly improve certain types of installs on low memory machines, and it will help with some pre-upgrade issues, thanks!
Regards,
Hans
On Sun, 2009-11-22 at 21:27 +0100, Hans de Goede wrote:
Hi,
On 11/22/2009 05:51 AM, Jerry Vonau wrote:
<snip>
Ack, but your current split up is still not split up per purpose, I think I understand what you are trying to achieve, so let me suggest a split:
A patch fixing the current copy code to use the fs with the most free space instead of the least
A patch to not only do the image transfer in case of a multiple disk cd / dvd install, but also in the case of a hdiso / net install, so as to free up memory used by install.img (which will be another patch). This patch should only do this when memory is tight! Doing this always is bad, as it is useless on systems with tons of memory, and could potentially even cause issues there with for example diskspace
A patch to actually achieve the freeing of memory 2)'s goal is by unlinking the install.img from /tmp
Think I got the flow you want for the patches now, I'm digging at the free variable for the first part now. That will take me some time as I don't have much to spare. What I have now is less that ideal, I would like see what is being returned, and just use / for the image till the above has being sorted out. At least for my testing, the rest of my patches depend on that one being in place.
Oh, but that is easy, simply change: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[0][0]) to: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[len(free)-1][0])
Yea, I see that now, but I need to see what was being returned first ;-)
So that you use the last element of the list, instead of the first, so that you get the biggest partition, your testing of this fix is much appreciated :)
That I can test quickly. I get back to you in a bit. (firing up vmm)
Notice that I'm not going with your suggested just always transfer install.img approach. This is not acceptable IMHO as it causes unnecessary slow down in many cases (net / disk install with plenty of ram, single dvd install), and it has the potential to trigger issues in all these cases which we would not hit if we did not do the transfer.
IOW doing the transfer has a price:
- it consumes disk space which we may need
- it causes us to take more "steps" then if we don't and each step we
take can (and eventually will) cause issues, so avoiding extra steps where possible is good. 3) it causes a slowdown
So since it is not free we should not do it unless there are good reasons to, so far the only reason we had for doing this was a multiple cd/dvd install, but I'm willing to agree that for things like a network install on a low memory machine it would be a good thing to do too.
Cool, then the question is at what amount of ram should this kick in at, I went with MIN_GUI_RAM.
That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB, so as to have atleast MIN_GUI_RAM free when the install.img (which is approx 100Mb) lives in RAM.
That is do-able.
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?
Quite long, there is no reason why we should not be able to fix a package set for a compete desktop install on a single dvd for years to come.
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.
The trade off is only a good one if memory is a scarce resource, see above.
Think I have that addressed in this round of patches against 12.46-2.
Thanks for taking the time,
You are welcome, thanks for contributing to anaconda. I hope that once you get the hang of it you keep on contributing :)
Here is a review of your latest set of patches:
1freefix.diff: See above
2modcall.diff: Please put the large comment in a commit msg, it would be great if you could learn to use git. (Drop by on #anaconda during CET office hours and I'll help you). Otherwise atleast learn to write commit messages, in a .patch / .diff file you can put text above the --- filename +++ filename
I'm getting the hang of git, slowly but surely..
Lines and patch (and other tools) will ignore this, please learn to put some explanation of the patch there start with a single line summary, so for example a good header for 2modcall.diff would be:
### Remove unneeded check from mountInstallImage()
This is a preparation patch for transferring install.img from /tmp to disk in low memory network and hdiso install scenarios to free up memory used by install.img under /tmp.
Currently mountInstallImage only gets called when using cd's, based on yuminstall's run, mkeys.sort(mediasort), if len(mkeys) > 1. mediaDevice has already been validated in yuminstall.py and you can't get here without mounting install.img, so there is no need for the check this patch removes. ###
The second hunk of this patch should be part of the 3th patch (or separate)
3setcall.diff:
- This does not belong in doConfigSetup(), setup() itself or a new method
called from setup() would be a better place.
OK, I'll see what I come up with.
- Even if the mountInstallImage() fails you still unlink /tmp/install.img
Yea, rushed that part, should be checking for a return code before unlinking.
4boot.diff:
Ah a new trick upi your sleef (atleast to me), nice one. But will this work ? Does the loader (stage1) copy install.img from /boot/upgrade to /tmp ? I'm asking because if it does not, then we will still have the loopback mounted and the unlink will not free up the diskspace.
The harddrive method never used to until preupgrade came along, but it does now.
We are getting somewhere, I think this will greatly improve certain types of installs on low memory machines, and it will help with some pre-upgrade issues, thanks!
At least if I don't have the time, the ideas are out there now.
Thanks,
Jerry
On Sun, 2009-11-22 at 15:48 -0600, Jerry Vonau wrote:
On Sun, 2009-11-22 at 21:27 +0100, Hans de Goede wrote:
Hi,
On 11/22/2009 05:51 AM, Jerry Vonau wrote:
<snip>
Ack, but your current split up is still not split up per purpose, I think I understand what you are trying to achieve, so let me suggest a split:
A patch fixing the current copy code to use the fs with the most free space instead of the least
A patch to not only do the image transfer in case of a multiple disk cd / dvd install, but also in the case of a hdiso / net install, so as to free up memory used by install.img (which will be another patch). This patch should only do this when memory is tight! Doing this always is bad, as it is useless on systems with tons of memory, and could potentially even cause issues there with for example diskspace
A patch to actually achieve the freeing of memory 2)'s goal is by unlinking the install.img from /tmp
Think I got the flow you want for the patches now, I'm digging at the free variable for the first part now. That will take me some time as I don't have much to spare. What I have now is less that ideal, I would like see what is being returned, and just use / for the image till the above has being sorted out. At least for my testing, the rest of my patches depend on that one being in place.
Oh, but that is easy, simply change: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[0][0]) to: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[len(free)-1][0])
Yea, I see that now, but I need to see what was being returned first ;-)
So that you use the last element of the list, instead of the first, so that you get the biggest partition, your testing of this fix is much appreciated :)
That I can test quickly. I get back to you in a bit. (firing up vmm)
That works, I added some logging to return self._loopbackFile to my updates.img, Then doing a vmm http install results in this logging: http://members.shaw.ca/jvonau/pub/F12/looploc.png
Then a screen shot of df before/after http://members.shaw.ca/jvonau/pub/F12/dfinstall.png
Got to do family time now,
Jerry
Hi,
On 11/23/2009 12:25 AM, Jerry Vonau wrote:
On Sun, 2009-11-22 at 15:48 -0600, Jerry Vonau wrote:
On Sun, 2009-11-22 at 21:27 +0100, Hans de Goede wrote:
Hi,
On 11/22/2009 05:51 AM, Jerry Vonau wrote:
<snip>
Ack, but your current split up is still not split up per purpose, I think I understand what you are trying to achieve, so let me suggest a split:
A patch fixing the current copy code to use the fs with the most free space instead of the least
A patch to not only do the image transfer in case of a multiple disk cd / dvd install, but also in the case of a hdiso / net install, so as to free up memory used by install.img (which will be another patch). This patch should only do this when memory is tight! Doing this always is bad, as it is useless on systems with tons of memory, and could potentially even cause issues there with for example diskspace
A patch to actually achieve the freeing of memory 2)'s goal is by unlinking the install.img from /tmp
Think I got the flow you want for the patches now, I'm digging at the free variable for the first part now. That will take me some time as I don't have much to spare. What I have now is less that ideal, I would like see what is being returned, and just use / for the image till the above has being sorted out. At least for my testing, the rest of my patches depend on that one being in place.
Oh, but that is easy, simply change: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[0][0]) to: self._loopbackFile = "%s%s/rhinstall-install.img" % (anaconda.rootPath, free[len(free)-1][0])
Yea, I see that now, but I need to see what was being returned first ;-)
So that you use the last element of the list, instead of the first, so that you get the biggest partition, your testing of this fix is much appreciated :)
That I can test quickly. I get back to you in a bit. (firing up vmm)
That works, I added some logging to return self._loopbackFile to my updates.img, Then doing a vmm http install results in this logging: http://members.shaw.ca/jvonau/pub/F12/looploc.png
Then a screen shot of df before/after http://members.shaw.ca/jvonau/pub/F12/dfinstall.png
Thanks for testing, I've committed a slightly modified version of this patch to the master branch, so atleast this will be fixed in F-13.
Regards,
Hans
On Sun, 2009-11-22 at 15:48 -0600, Jerry Vonau wrote:
On Sun, 2009-11-22 at 21:27 +0100, Hans de Goede wrote:
Hi,
On 11/22/2009 05:51 AM, Jerry Vonau wrote:
<snip>
<snip>
Here is a review of your latest set of patches:
1freefix.diff: See above
2modcall.diff: Please put the large comment in a commit msg, it would be great if you could learn to use git. (Drop by on #anaconda during CET office hours and I'll help you). Otherwise atleast learn to write commit messages, in a .patch / .diff file you can put text above the --- filename +++ filename
I'm getting the hang of git, slowly but surely..
Using git is not really a problem, just have to get the hang of emailing patches from git.
Lines and patch (and other tools) will ignore this, please learn to put some explanation of the patch there start with a single line summary, so for example a good header for 2modcall.diff would be:
### Remove unneeded check from mountInstallImage()
This is a preparation patch for transferring install.img from /tmp to disk in low memory network and hdiso install scenarios to free up memory used by install.img under /tmp.
Currently mountInstallImage only gets called when using cd's, based on yuminstall's run, mkeys.sort(mediasort), if len(mkeys) > 1. mediaDevice has already been validated in yuminstall.py and you can't get here without mounting install.img, so there is no need for the check this patch removes. ###
The second hunk of this patch should be part of the 3th patch (or separate)
Once I have the email in git sorted out, that will become easier. Just installed git-email, today.
3setcall.diff:
- This does not belong in doConfigSetup(), setup() itself or a new method
called from setup() would be a better place.
OK, I'll see what I come up with.
- Even if the mountInstallImage() fails you still unlink /tmp/install.img
Yea, rushed that part, should be checking for a return code before unlinking.
4boot.diff:
Ah a new trick upi your sleef (atleast to me), nice one. But will this work ? Does the loader (stage1) copy install.img from /boot/upgrade to /tmp ? I'm asking because if it does not, then we will still have the loopback mounted and the unlink will not free up the diskspace.
The harddrive method never used to until preupgrade came along, but it does now.
We are getting somewhere, I think this will greatly improve certain types of installs on low memory machines, and it will help with some pre-upgrade issues, thanks!
At least if I don't have the time, the ideas are out there now.
I'm out of time for the day, but here are my latest round of patches, from git this time.
Jerry
One general git style comment - could you please format your git commit messages so that you have the one line at the beginning that becomes the subject, and then after that a paragraph explaining what you're doing. Not every commit may require explanation, but some very well might - especially if it's not obvious.
Also, reference the bug numbers in the subject if appropriate as follows:
"fixed the whatever whatever (#998)"
These are nitpicky little things, but really help when having to search back through the git history. Also, I'm giving everyone this speech these days.
From 6649a90cc082576ec8e27f26901add85ac8e49dc Mon Sep 17 00:00:00 2001
From: Jerry Jerry@f9.vonau.ca Date: Tue, 24 Nov 2009 19:07:03 -0600 Subject: [PATCH 4/5] create-freetmp
yuminstall.py | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/yuminstall.py b/yuminstall.py index b2ff6ac..3b5c842 100644 --- a/yuminstall.py +++ b/yuminstall.py @@ -1061,6 +1061,21 @@ reposdir=/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anacon # unhappy (#496961) iutil.resetRpmDb(anaconda.rootPath)
- def freetmp(self, anaconda):
- # installs that don't use /mnt/stage2 hold the install.img on
- # a tmpfs, free this ram if things are tight.
stage2img = "/tmp/install.img"
if os.path.exists(stage2img) and iutil.memAvailable() < isys.MIN_GUI_RAM:
log.info("%s exists and low memory" % stage2img )
# free up /tmp for more memory before yum is called,
if anaconda.backend.mountInstallImage(anaconda, stage2img):
return DISPATCH_BACK
try:
os.unlink(stage2img)
except SystemError:
log.info("clearing /tmp failed")
return DISPATCH_BACK
- def doBackendSetup(self, anaconda): if anaconda.dir == DISPATCH_BACK: return DISPATCH_BACK
-- 1.6.5.2
I still think this belongs somewhere besides yuminstall.py. My previous suggestion was image.py, but maybe backend.py is more appropriate. appropriate name for the method like "removeInstallImage" would be nice too. Note the parallelism with mountInstallImage.
With those things addressed, this patch set will look fine and can go in. Thanks for putting up with how particular we can be.
- Chris
Hi,
On 12/01/2009 03:40 PM, Chris Lumens wrote:
One general git style comment - could you please format your git commit messages so that you have the one line at the beginning that becomes the subject, and then after that a paragraph explaining what you're doing. Not every commit may require explanation, but some very well might - especially if it's not obvious.
Also, reference the bug numbers in the subject if appropriate as follows:
"fixed the whatever whatever (#998)"
These are nitpicky little things, but really help when having to search back through the git history. Also, I'm giving everyone this speech these days.
From 6649a90cc082576ec8e27f26901add85ac8e49dc Mon Sep 17 00:00:00 2001
From: JerryJerry@f9.vonau.ca Date: Tue, 24 Nov 2009 19:07:03 -0600 Subject: [PATCH 4/5] create-freetmp
yuminstall.py | 15 +++++++++++++++ 1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/yuminstall.py b/yuminstall.py index b2ff6ac..3b5c842 100644 --- a/yuminstall.py +++ b/yuminstall.py @@ -1061,6 +1061,21 @@ reposdir=/etc/anaconda.repos.d,/tmp/updates/anaconda.repos.d,/tmp/product/anacon # unhappy (#496961) iutil.resetRpmDb(anaconda.rootPath)
- def freetmp(self, anaconda):
- # installs that don't use /mnt/stage2 hold the install.img on
- # a tmpfs, free this ram if things are tight.
stage2img = "/tmp/install.img"
if os.path.exists(stage2img) and iutil.memAvailable()< isys.MIN_GUI_RAM:
log.info("%s exists and low memory" % stage2img )
# free up /tmp for more memory before yum is called,
if anaconda.backend.mountInstallImage(anaconda, stage2img):
return DISPATCH_BACK
try:
os.unlink(stage2img)
except SystemError:
log.info("clearing /tmp failed")
return DISPATCH_BACK
def doBackendSetup(self, anaconda): if anaconda.dir == DISPATCH_BACK: return DISPATCH_BACK
-- 1.6.5.2
I still think this belongs somewhere besides yuminstall.py. My previous suggestion was image.py, but maybe backend.py is more appropriate. appropriate name for the method like "removeInstallImage" would be nice too. Note the parallelism with mountInstallImage.
With those things addressed, this patch set will look fine and can go in. Thanks for putting up with how particular we can be.
Chris,
Thanks for also reviewing this, the second (third) pair of eyes is appreciated. I do have one question though. The first patch of the latest sets removes /boot/upgrade/install.img, which is a good thing to do as that frees precious /boot place before starting the upgrade, but if we do that why not just completely rm -fr /boot/upgrades ?
Regards,
Hans
Thanks for also reviewing this, the second (third) pair of eyes is appreciated. I do have one question though. The first patch of the latest sets removes /boot/upgrade/install.img, which is a good thing to do as that frees precious /boot place before starting the upgrade, but if we do that why not just completely rm -fr /boot/upgrades ?
preupgrade also puts the kickstart file, initrd, and kernel in /boot/upgrade. We probably don't want to remove any of those until post-upgrade.
In fact now that I think about it, I'm not sure removing /boot/upgrade/install.img so early is great either. Note that we will be removing it when the doBackendSetup step is called, which is before we do anything with packages. If for some reason you run into a problem with package upgrades, when you reboot, fix it, and reboot back into anaconda you will no longer have a stage2 image. This will result in either anaconda downloading it again or you having to run preupgrade again.
Either way, you're downloading stuff you thought were already taken care of.
- Chris
Hi,
On 12/01/2009 04:35 PM, Chris Lumens wrote:
Thanks for also reviewing this, the second (third) pair of eyes is appreciated. I do have one question though. The first patch of the latest sets removes /boot/upgrade/install.img, which is a good thing to do as that frees precious /boot place before starting the upgrade, but if we do that why not just completely rm -fr /boot/upgrades ?
preupgrade also puts the kickstart file, initrd, and kernel in /boot/upgrade. We probably don't want to remove any of those until post-upgrade.
In fact now that I think about it, I'm not sure removing /boot/upgrade/install.img so early is great either. Note that we will be removing it when the doBackendSetup step is called, which is before we do anything with packages.
If we already remove it before yum checks to see if there is enough freespace there, then indeed that part of this patch set should be dropped.
Regards,
Hans
On Tue, 2009-12-01 at 10:35 -0500, Chris Lumens wrote:
Thanks for also reviewing this, the second (third) pair of eyes is appreciated. I do have one question though. The first patch of the latest sets removes /boot/upgrade/install.img, which is a good thing to do as that frees precious /boot place before starting the upgrade, but if we do that why not just completely rm -fr /boot/upgrades ?
Think that could be done.
preupgrade also puts the kickstart file, initrd, and kernel in /boot/upgrade. We probably don't want to remove any of those until post-upgrade.
Everything there is in memory when anaconda runs.
In fact now that I think about it, I'm not sure removing /boot/upgrade/install.img so early is great either. Note that we will be removing it when the doBackendSetup step is called, which is before we do anything with packages. If for some reason you run into a problem with package upgrades,
Then you have a problem yes, but is that an anaconda problem? You would have to have some yum/rpm problem right? While file corruption is possible, the only repo in use was created by preupgrade. Your dealing with what amounts to a "custom repo", created with whatever repos are enabled when preupgrade was run. I think it would be its job to ensure that the upgrade would be successful.
when you reboot, fix it, and reboot back into anaconda you will no longer have a stage2 image. This will result in either anaconda downloading it again or you having to run preupgrade again.
Well, I think you should have to re-run preupgrade, think it was meant to be a "one shot use" supporting offline upgrades. You have to re-run it now to use Method 2 of the workarounds on the Wiki, to pickup the change in the size of /boot. Of course you can just edit grub.conf and use what you want there, if you remember to remove the install.img from /boot/upgrade.
Either way, you're downloading stuff you thought were already taken care of.
Fair enough, if your tight for space, preupgrade could now be using http for install.img anyway. How about making the removal conditional on if /boot is less than the current default size for /boot or available space on /boot less than 50MB?
Thanks for the feedback,
Jerry
On 12/02/2009 02:16 AM, Jerry Vonau wrote:
On Tue, 2009-12-01 at 10:35 -0500, Chris Lumens wrote:
Thanks for also reviewing this, the second (third) pair of eyes is appreciated. I do have one question though. The first patch of the latest sets removes /boot/upgrade/install.img, which is a good thing to do as that frees precious /boot place before starting the upgrade, but if we do that why not just completely rm -fr /boot/upgrades ?
Think that could be done.
preupgrade also puts the kickstart file, initrd, and kernel in /boot/upgrade. We probably don't want to remove any of those until post-upgrade.
Everything there is in memory when anaconda runs.
In fact now that I think about it, I'm not sure removing /boot/upgrade/install.img so early is great either. Note that we will be removing it when the doBackendSetup step is called, which is before we do anything with packages. If for some reason you run into a problem with package upgrades,
Then you have a problem yes, but is that an anaconda problem? You would have to have some yum/rpm problem right? While file corruption is possible, the only repo in use was created by preupgrade. Your dealing with what amounts to a "custom repo", created with whatever repos are enabled when preupgrade was run. I think it would be its job to ensure that the upgrade would be successful.
when you reboot, fix it, and reboot back into anaconda you will no longer have a stage2 image. This will result in either anaconda downloading it again or you having to run preupgrade again.
Well, I think you should have to re-run preupgrade, think it was meant to be a "one shot use" supporting offline upgrades. You have to re-run it now to use Method 2 of the workarounds on the Wiki, to pickup the change in the size of /boot. Of course you can just edit grub.conf and use what you want there, if you remember to remove the install.img from /boot/upgrade.
Either way, you're downloading stuff you thought were already taken care of.
Fair enough, if your tight for space, preupgrade could now be using http for install.img anyway. How about making the removal conditional on if /boot is less than the current default size for /boot or available space on /boot less than 50MB?
Hi Jerry,
If I understood Chris correctly we already remove /boot/upgrades before we enter the part of the installer where the amount of freespace in /boot matters, so your patch should not make a difference. If it does then we are currently not doing what Chris thinks we are doing. So for this part of your patchset it would be good to find out where and when exactly /boot/upgrades is getting cleared with the current code, and if that is too late, move the clearing, instead of adding pretty much the same clearing code a second time.
Regards,
Hans
On Wed, 2009-12-02 at 13:12 +0100, Hans de Goede wrote:
Hi Jerry,
If I understood Chris correctly we already remove /boot/upgrades before we enter the part of the installer where the amount of freespace in /boot matters, so your patch should not make a difference. If it does then we are currently not doing what Chris thinks we are doing.
Maybe I'm just blind but I can't find any reference to removing install.img from /boot/upgrade in the anaconda sources.
So for this part of your patchset it would be good to find out where and when exactly /boot/upgrades is getting cleared with the current code, and if that is too late, move the clearing, instead of adding pretty much the same clearing code a second time.
I fully agree, why reinvent the wheel, but I can't see where that should be happening, hence the patch. I believe the only place that /boot/upgrade is touched is in the %post part of preupgrade's kickstart file.
Jerry
On 12/02/2009 04:33 PM, Jerry Vonau wrote:
On Wed, 2009-12-02 at 13:12 +0100, Hans de Goede wrote:
Hi Jerry,
If I understood Chris correctly we already remove /boot/upgrades before we enter the part of the installer where the amount of freespace in /boot matters, so your patch should not make a difference. If it does then we are currently not doing what Chris thinks we are doing.
Maybe I'm just blind but I can't find any reference to removing install.img from /boot/upgrade in the anaconda sources.
So for this part of your patchset it would be good to find out where and when exactly /boot/upgrades is getting cleared with the current code, and if that is too late, move the clearing, instead of adding pretty much the same clearing code a second time.
I fully agree, why reinvent the wheel, but I can't see where that should be happening, hence the patch. I believe the only place that /boot/upgrade is touched is in the %post part of preupgrade's kickstart file.
Chris,
Can we have your input on this please ?
Thanks,
Hans
Jerry
Anaconda-devel-list mailing list Anaconda-devel-list@redhat.com https://www.redhat.com/mailman/listinfo/anaconda-devel-list
Can you please post a link to the current state of your patch one more time? I might just commit it as-is. I'm not sure I really care about the install.img deleting thing.
- Chris
On Wed, 2009-12-09 at 13:11 -0500, Chris Lumens wrote:
Can you please post a link to the current state of your patch one more time? I might just commit it as-is. I'm not sure I really care about the install.img deleting thing.
- Chris
http://members.shaw.ca/jvonau/pub/F12/
0001-0005 are what I have working, 0006 is testing/work in progress.
Jerry
Can you please post a link to the current state of your patch one more time? I might just commit it as-is. I'm not sure I really care about the install.img deleting thing.
- Chris
http://members.shaw.ca/jvonau/pub/F12/
0001-0005 are what I have working, 0006 is testing/work in progress.
My one complaint remains with 0004 that I don't think freetmp is a YumBackend-specific method and should be put somewhere where all backends can make use of it. I wonder if it could be merged with backend.removeInstallImage.
Once this is addressed, I'm all for committing.
- Chris
On Fri, 2009-12-11 at 10:58 -0500, Chris Lumens wrote:
Can you please post a link to the current state of your patch one more time? I might just commit it as-is. I'm not sure I really care about the install.img deleting thing.
- Chris
http://members.shaw.ca/jvonau/pub/F12/
0001-0005 are what I have working, 0006 is testing/work in progress.
My one complaint remains with 0004 that I don't think freetmp is a YumBackend-specific method and should be put somewhere where all backends can make use of it. I wonder if it could be merged with backend.removeInstallImage.
I think that one is meant to delete the rhinstall-install.img from the target system once the install is complete, think I'll leave that one as is.
Once this is addressed, I'm all for committing.
Thanks Chris, there is a 0004-revised.patch, (sorry, that one is not in git format, please forgive me), and 0005-revised.patch at the above url. Test running a vmm http install, working for me here.
Going to sleep now,
Jerry
Once this is addressed, I'm all for committing.
Thanks Chris, there is a 0004-revised.patch, (sorry, that one is not in git format, please forgive me), and 0005-revised.patch at the above url. Test running a vmm http install, working for me here.
Pushed, and will be rebuilding anaconda soon. Thanks for your patience and revisions.
- Chris
Hi,
On 12/16/2009 10:43 PM, Chris Lumens wrote:
Once this is addressed, I'm all for committing.
Thanks Chris, there is a 0004-revised.patch, (sorry, that one is not in git format, please forgive me), and 0005-revised.patch at the above url. Test running a vmm http install, working for me here.
Pushed, and will be rebuilding anaconda soon. Thanks for your patience and revisions.
Chris,
Thanks for taking care of this, but something seems to have gone wrong with this commit: http://git.fedorahosted.org/git/?p=anaconda.git;a=commitdiff;h=98504bcaad593...
It removes the following line which it should not: - free[-1][0])
And with this one: http://git.fedorahosted.org/git/?p=anaconda.git;a=commitdiff;h=a75aa437f6fcb...
The code for setting isodir and switching image now only happens under the else block for the m.count(":") == 2 test, where as it should happens always (as it did in the past).
I'll submit patches to fix this.
Regards,
Hans
If it does then we are currently not doing what Chris thinks we are doing.
You know, maybe one day someone will believe I actually know what I'm doing. I guess we're not there quite yet.
So for this part of your patchset it would be good to find out where and when exactly /boot/upgrades is getting cleared with the current code, and if that is too late, move the clearing, instead of adding pretty much the same clearing code a second time.
anaconda does not remove /boot/upgrade (no 's') at any time. However if you look at the discussion we were just having, you will see that you wrote:
Thanks for also reviewing this, the second (third) pair of eyes is appreciated. I do have one question though. The first patch of the latest sets removes /boot/upgrade/install.img, which is a good thing to do as that frees precious /boot place before starting the upgrade, but if we do that why not just completely rm -fr /boot/upgrades ?
My reply was that implementing your suggestion to completely remove /boot/upgrade was a bad idea. I have no idea how you guys got started thinking that I was saying we already do this.
- Chris
Cool, then the question is at what amount of ram should this kick in at, I went with MIN_GUI_RAM.
That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB, so as to have atleast MIN_GUI_RAM free when the install.img (which is approx 100Mb) lives in RAM.
I don't like this one bit. Sure, today it's ~ 100 MB, but what about in two years when it's more like ~ 150 MB and no one remembers this needs updating? Because I guarantee, no one will remember.
Also, you're introducing another memory limit besides MIN_GUI_RAM that will mean a different set of behavior happens at some random memory threshold.
- Chris
Hi,
On 11/23/2009 04:41 PM, Chris Lumens wrote:
Cool, then the question is at what amount of ram should this kick in at, I went with MIN_GUI_RAM.
That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB, so as to have atleast MIN_GUI_RAM free when the install.img (which is approx 100Mb) lives in RAM.
I don't like this one bit. Sure, today it's ~ 100 MB, but what about in two years when it's more like ~ 150 MB and no one remembers this needs updating? Because I guarantee, no one will remember.
Also, you're introducing another memory limit besides MIN_GUI_RAM that will mean a different set of behavior happens at some random memory threshold.
So you suggest we just use MIN_GUI_RAM, or are you against the idea of transfering install.img from /tmp to disk in low memory situations in general ?
Regards,
Hans
On Mon, 2009-11-23 at 10:41 -0500, Chris Lumens wrote:
Cool, then the question is at what amount of ram should this kick in at, I went with MIN_GUI_RAM.
That sounds reasonable. Maybe want to do use something like MIN_GUI_RAM + 100MB, so as to have atleast MIN_GUI_RAM free when the install.img (which is approx 100Mb) lives in RAM.
I don't like this one bit. Sure, today it's ~ 100 MB, but what about in two years when it's more like ~ 150 MB and no one remembers this needs updating? Because I guarantee, no one will remember.
How about something like MIN_GUI_RAM > memAvailable? Nothing is really using that function now, it returns memory less what is used by /tmp. That would do away with the hard-coding, and scale into the future. The only variable going forward would be the size of install.img
Also, you're introducing another memory limit besides MIN_GUI_RAM that will mean a different set of behavior happens at some random memory threshold.
You have that now with a difference between http/ftp/hd and media installs when you test iutil.memInstalled() < isys.MIN_GUI_RAM with out taking into account what might be in /tmp, maybe that should be using memAvailable?
Jerry
How about something like MIN_GUI_RAM > memAvailable? Nothing is really using that function now, it returns memory less what is used by /tmp. That would do away with the hard-coding, and scale into the future. The only variable going forward would be the size of install.img
I *think* this is okay, but I'm having a hard time convincing myself of this. I suppose what would really convince me is seeing how this works in practice on a variety of machines with different amounts of memory.
You have that now with a difference between http/ftp/hd and media installs when you test iutil.memInstalled() < isys.MIN_GUI_RAM with out taking into account what might be in /tmp, maybe that should be using memAvailable?
I don't think we want to do that. I assume you're talking about anaconda:385. If we change memInstalled to memAvailable there, what we're really saying is that the amount of memory required to run in graphical mode is MIN_GUI_RAM+sizeof(/tmp/install.img). And in that case, we're better off just bumping MIN_GUI_RAM.
- Chris
diff -up ./yuminstall.py.orig ./yuminstall.py --- ./yuminstall.py.orig 2009-11-21 20:24:29.000000000 -0600 +++ ./yuminstall.py 2009-11-21 20:24:32.000000000 -0600 @@ -725,6 +725,13 @@ class AnacondaYum(YumSorter):
self.repos.setCacheDir(self.conf.cachedir)
if os.path.exists("/mnt/sysimage/boot/upgrade/install.img"):
log.info("REMOVING stage2 image from /mnt/sysimage/boot/upgrade")
try:
os.unlink ("/mnt/sysimage/boot/upgrade/install.img")
except:
log.warning("failed to clean /boot/upgrade")
- def downloadHeader(self, po): while True: # retrying version of download header
You need to use self.anaconda.rootPath instead of "/mnt/sysimage".
diff -up ./yuminstall.py.orig ./yuminstall.py --- ./yuminstall.py.orig 2009-11-21 05:16:04.000000000 -0600 +++ ./yuminstall.py 2009-11-21 16:36:47.000000000 -0600 @@ -625,6 +625,25 @@ class AnacondaYum(YumSorter):
def doConfigSetup(self, fn='/tmp/anaconda-yum.conf', root='/'):
# installs that don't use /mnt/stage2 hold the install.img on
# a tmpfs, free this ram if things are tight.
stage2img = "/tmp/install.img"
if os.path.exists(stage2img) and iutil.memInstalled() < isys.MIN_GUI_RAM:
log.info("%s exists and low memory" % stage2img )
# free up /tmp for more memory before yum is called,
try:
self.anaconda.backend.mountInstallImage(self.anaconda, stage2img)
except:
log.info("mountInstallImage failed")
try:
os.unlink(stage2img)
except:
log.info("clearing /tmp failed")
pass
if hasattr(self, "preconf"): self.preconf.fn = fn self.preconf.root = root
Agreed with Hans - this is not a good place for this code. First, we need to do it in all backends so you don't want to put it in AnacondaYum (or really, anywhere in yuminstall.py). Second even if you were to put it in AnacondaYum, you wouldn't want it to go in doConfigSetup. Basically any time you find yourself wanting to put code in AnacondaYum, ask yourself whether that code would go in yum. If not, AnacondaYum is probably the wrong place for it. This seems like something that should go in image.py.
Also, if mountInstallImage raises an exception, that is pretty much fatal. See this existing code in yuminstall.py:
if self.anaconda.backend.mountInstallImage(self.anaconda, stage2img): self.anaconda.id.storage.umountFilesystems() return DISPATCH_BACK
@@ -199,7 +207,12 @@ return 1
isys.lochangefd("/dev/loop0", self._loopbackFile)
isys.umount("/mnt/stage2")
# http, ftp, hd installs don't mount /mnt/stage2
try:
isys.umount("/mnt/stage2")
except:
pass
def removeInstallImage(self): if self._loopbackFile:
The try...except where you don't catch a specific exception is pretty bad, as you end up unintentionally hiding all sorts of issues. Here, we don't even need to handle an exception as we know whether or not something's mounted:
if os.path.ismount("/mnt/stage2"): isys.umount("/mnt/stage2")
anaconda-devel@lists.stg.fedoraproject.org