FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > CentOS > CentOS Development

 
 
LinkBack Thread Tools
 
Old 11-23-2009, 02:41 PM
Chris Lumens
 
Default PATCH fix 510970, 529551, 530541

> >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

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 11-23-2009, 02:57 PM
Chris Lumens
 
Default PATCH fix 510970, 529551, 530541

> 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.anaco nda, 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.anaco nda, 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-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 11-23-2009, 02:59 PM
Hans de Goede
 
Default PATCH fix 510970, 529551, 530541

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

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 11-23-2009, 02:59 PM
Hans de Goede
 
Default PATCH fix 510970, 529551, 530541

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:

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



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

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 11-23-2009, 03:55 PM
Jerry Vonau
 
Default PATCH fix 510970, 529551, 530541

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







_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 11-24-2009, 05:30 PM
Chris Lumens
 
Default PATCH fix 510970, 529551, 530541

> 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

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 11-25-2009, 01:40 AM
Jerry Vonau
 
Default PATCH fix 510970, 529551, 530541

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:
> >
> > 1) 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.
>
> > 2) 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


_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-01-2009, 01:40 PM
Chris Lumens
 
Default PATCH fix 510970, 529551, 530541

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

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-01-2009, 02:00 PM
Hans de Goede
 
Default PATCH fix 510970, 529551, 530541

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: 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,

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

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 
Old 12-01-2009, 02:16 PM
Seth Vidal
 
Default PATCH fix 510970, 529551, 530541

On Tue, 1 Dec 2009, Chris Lumens wrote:



"fixed the whatever whatever (#998)"


See, this is just wishful thinking - no one is ever going to fix bug 998.



-sv

_______________________________________________
Anaconda-devel-list mailing list
Anaconda-devel-list@redhat.com
https://www.redhat.com/mailman/listinfo/anaconda-devel-list
 

Thread Tools




All times are GMT. The time now is 12:08 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org