[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: PATCH fix 510970, 529551, 530541



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:

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.


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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]