-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
force garbage collection on dnf.Base.reset() #1825
Conversation
should be removed Also would it be a good idea to add a one-line comment in the code explaining why you're calling the garbage collector there? |
Yes, thanks.
I can add something like
but I don't know if we want to be that specific there -- after all, this is the I kind of see this as a general failure of garbage-collected programming languages. libsolv provides all the necessary destructor functions, providing the client application with full control over resource release. And then comes Python and the circular references, and all that control is willfully discarded. |
First off, thanks for doing such an excruciating analysis @lersek. I think what @rwmjones means and what I'd also like to have in the comment is a shorter version of the description from your commit message that would kinda sum it up, but provide the important information about the issue it solves. IMO you should use as many lines as needed to achieve that 🙂. Ideally anyone reading it in a few years will fully understand what's going on from just the comment without having to dig up the commit message. |
Consider the following call paths (mixed Python and C), extending from livecd-creator down to libsolv: main [livecd-tools/tools/livecd-creator] install() [livecd-tools/imgcreate/creator.py] fill_sack() [dnf/dnf/base.py] _add_repo_to_sack() [dnf/dnf/base.py] load_repo() [libdnf/python/hawkey/sack-py.cpp] dnf_sack_load_repo() [libdnf/libdnf/dnf-sack.cpp] write_main() [libdnf/libdnf/dnf-sack.cpp] repo_add_solv() [libsolv/src/repo_solv.c] repopagestore_read_or_setup_pages() [libsolv/src/repopage.c] dup() write_ext() [libdnf/libdnf/dnf-sack.cpp] repo_add_solv() [libsolv/src/repo_solv.c] repopagestore_read_or_setup_pages() [libsolv/src/repopage.c] dup() The dup() calls create the following file descriptors (output from "lsof"): > COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME > python3 6500 root 7r REG 8,1 25320727 395438 /var/tmp/imgcreate-mytcghah/install_root/var/cache/dnf/fedora.solv (deleted) > python3 6500 root 8r REG 8,1 52531426 395450 /var/tmp/imgcreate-mytcghah/install_root/var/cache/dnf/fedora-filenames.solvx These file descriptors are *owned* by the DnfSack object (which is derived from GObject), as follows: sack->priv->pool->repos[1]->repodata[1]->store.pagefd = 7 sack->priv->pool->repos[1]->repodata[2]->store.pagefd = 8 ^ ^ ^ ^ ^ ^ ^ | | | | | | | | | | | | | int | | | | | Repopagestore [libsolv/src/repopage.h] | | | | Repodata [libsolv/src/repodata.h] | | | struct s_Repo [libsolv/src/repo.h] | | struct s_Pool (aka Pool) [libsolv/src/pool.h] | DnfSackPrivate [libdnf/libdnf/dnf-sack.cpp] DnfSack [libdnf/libdnf/dnf-sack.h] The file descriptors are *supposed* to be closed on the following call path: main [livecd-tools/tools/livecd-creator] install() [livecd-tools/imgcreate/creator.py] close() [livecd-tools/imgcreate/dnfinst.py] close() [dnf/dnf/base.py] reset() [dnf/dnf/base.py] _sack = None _goal = None _transaction = None ... dnf_sack_finalize() [libdnf/libdnf/dnf-sack.cpp] pool_free() [libsolv/src/pool.c] pool_freeallrepos() [libsolv/src/pool.c] repo_freedata() [libsolv/src/repo.c] repodata_freedata() [libsolv/src/repodata.c] repopagestore_free() [libsolv/src/repopage.c] close() Namely, when dnf.Base.reset() [dnf/dnf/base.py] is called with (sack=True, goal=True), the reference counts of the objects pointed to by the "_sack", "_goal" and "_transaction" fields are supposed to reach zero, and then, as part of the DnfSack object's finalization, the libsolv file descriptors are supposed to be closed. Now, while this *may* happen immediately in dnf.Base.reset(), it may as well not. The reason is that there is a multitude of *circular references* between DnfSack and the packages that it contains. When dnf.Base.reset() is entered, we have the following picture: _sack _goal | | v v +----------------+ +-------------+ | DnfSack object | <--- | Goal object | +----------------+ +-------------+ |^ |^ |^ || || || || || || +--||----||----||---+ | v| v| v| | <-- _transaction | Pkg1 Pkg2 PkgN | | | | Transaction oject | +-------------------+ That is, the reference count of the DnfSack object is (1 + 1 + N), where N is the number of packages in the transaction. Details: (a) The first reference comes from the "_sack" field, established like this: main [livecd-tools/tools/livecd-creator] install() [livecd-tools/imgcreate/creator.py] fill_sack() [dnf/dnf/base.py] _build_sack() [dnf/dnf/sack.py] Sack() sack_init() [libdnf/python/hawkey/sack-py.cpp] dnf_sack_new() [libdnf/libdnf/dnf-sack.cpp] (b) The second reference on the DnfSack object comes from "_goal": main [livecd-tools/tools/livecd-creator] install() [livecd-tools/imgcreate/creator.py] fill_sack() [dnf/dnf/base.py] _goal = Goal(_sack) goal_init() [libdnf/python/hawkey/goal-py.cpp] Py_INCREF(_sack) (c) Then there is one reference to "_sack" *per package* in the transaction: main [livecd-tools/tools/livecd-creator] install() [livecd-tools/imgcreate/creator.py] runInstall() [livecd-tools/imgcreate/dnfinst.py] resolve() [dnf/dnf/base.py] _goal2transaction() [dnf/dnf/base.py] list_installs() [libdnf/python/hawkey/goal-py.cpp] list_generic() [libdnf/python/hawkey/goal-py.cpp] packagelist_to_pylist() [libdnf/python/hawkey/iutil-py.cpp] new_package() [libdnf/python/hawkey/sack-py.cpp] Py_BuildValue() ts.add_install() list_installs() creates a list of packages that need to be installed by DNF. Inside the loop in packagelist_to_pylist(), which constructs the elements of that list, Py_BuildValue() is called with the "O" format specifier, and that increases the reference count on "_sack". Subsequently, in the _goal2transaction() method, we iterate over the package list created by list_installs(), and add each package to the transaction (ts.add_install()). After _goal2transaction() returns, this transaction is assigned to "self._transaction" in resolve(). This is where the last N (back-)references on the DnfSack object come from. (d) Now, to quote the defintion of the DnfSack object ("libdnf/docs/hawkey/tutorial-py.rst"): > *Sack* is an abstraction for a collection of packages. That's why the DnfSack object references all the Pkg1 through PkgN packages. So, when the dnf.Base.reset() method completes, the picture changes like this: _sack _goal | | -- [CUT] -- -- [CUT] -- | | v | v +----------------+ [C] +-------------+ | DnfSack object | <-[U]- | Goal object | +----------------+ [T] +-------------+ |^ |^ |^ | || || || || || || | +--||----||----||---+ [C] | v| v| v| | <--[U]-- _transaction | Pkg1 Pkg2 PkgN | [T] | | | | Transaction oject | +-------------------+ and we are left with N reference cycles (one between each package and the same DnfSack object). This set of cycles can only be cleaned up by Python's generational garbage collector <https://stackify.com/python-garbage-collection/>. The GC will collect the DnfSack object, and consequently close the libsolv page file descriptors via dnf_sack_finalize() -- but garbage collection will happen *only eventually*, unpredictably. This means that the dnf.Base.reset() method breaks its interface contract: > Make the Base object forget about various things. because the libsolv file descriptors can (and frequently do, in practice) survive dnf.Base.reset(). In general, as long as the garbage collector only tracks process-private memory blocks, there's nothing wrong; however, file descriptors are visible to the kernel. When dnf.Base.reset() *temporarily* leaks file descriptors as explained above, then immediately subsequent operations that depend on those file descriptors having been closed, can fail. An example is livecd-creator's unmounting of: /var/tmp/imgcreate-mytcghah/install_root/var/cache/dnf which the kernel refuses, due to libsolv's still open file descriptors pointing into that filesystem: > umount: /var/tmp/imgcreate-mytcghah/install_root/var/cache/dnf: target > is busy. > Unable to unmount /var/tmp/imgcreate-mytcghah/install_root/var/cache/dnf > normally, using lazy unmount (Unfortunately, the whole lazy umount idea is misguided in livecd-tools; it's a misfeature that should be removed, as it permits the corruption of the loop-backed filesystem. Now that the real bug is being fixed in DNF, lazy umount is not needed as a (broken) workaround in livecd-tools. But that's a separate patch for livecd-tools: <livecd-tools/livecd-tools#227>.) Plug the fd leak by forcing a garbage collection in dnf.Base.reset() whenever we cut the "_sack", "_goal" and "_transaction" links -- that is, when the "sack" and "goal" parameters are True. Note that precisely due to the unpredictable behavior of the garbage collector, reproducing the bug may prove elusive. In order to reproduce it deterministically, through usage with livecd-creator, disabling automatic garbage collection with the following patch (for livecd-tools) is sufficient: > diff --git a/tools/livecd-creator b/tools/livecd-creator > index 291de10cbbf9..8d2c740c238b 100755 > --- a/tools/livecd-creator > +++ b/tools/livecd-creator > @@ -31,6 +31,8 @@ from dnf.exceptions import Error as DnfBaseError > import imgcreate > from imgcreate.errors import KickstartError > > +import gc > + > class Usage(Exception): > def __init__(self, msg = None, no_error = False): > Exception.__init__(self, msg, no_error) > @@ -261,5 +263,6 @@ def do_nss_libs_hack(): > return hack > > if __name__ == "__main__": > + gc.disable() > hack = do_nss_libs_hack() > sys.exit(main()) Also note that you need to use livecd-tools at git commit 4afde9352e82 or later, for this fix to make any difference: said commit fixes a different (independent) bug in livecd-tools that produces identical symptoms, but from a different origin. In other words, if you don't have commit 4afde9352e82 in your livecd-tools install, then said bug in livecd-tools will mask this DNF fix. Signed-off-by: Laszlo Ersek <[email protected]>
Add myself to the "DNF CONTRIBUTORS" section, per "README.rst". Signed-off-by: Laszlo Ersek <[email protected]>
|
…table Including the shim-x64 and grub2-efi-x64-cdboot packages causes livecd-creator to automatically build a UEFI-bootable CD. I didn't modify the dependencies for distros other than the RH family, because: - I checked Debian briefly for any package providing a file ending with "BOOTX64.EFI", and the only hit was irrelevant (it was a systemd file). - Kickstart is RH-specific anyway. Tested with an actual conversion: - Built the ISO with the following livecd-creator and dnf fixes included: - rpm-software-management/dnf#1825 - livecd-tools/livecd-tools#227 - Booted the P2V ISO in UEFI mode on QEMU, against a previously installed (UEFI) RHEL-7.9 guest's disk. - Converted the guest with the help of a virt-v2v conversion server VM, using the QEMU output module. - Successfully booted the converted guest. Thanks: Neal Gompa <[email protected]> Cc: Neal Gompa <[email protected]> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2038105 Signed-off-by: Laszlo Ersek <[email protected]> Message-Id: <[email protected]> Reviewed-by: Richard W.M. Jones <[email protected]>
@lukash can I please trouble you to review the updated patch set? As highlighted in #1825 (comment) above, I've updated the commit message (the grammatical error pointed out by @rwmjones , plus a typo I noticed myself), and I've distilled the reasoning to a new comment in the code. No code changes. (Still retested it though.) Thanks! |
@lersek you certainly can. LGTM now. Again, thanks for the elaborate analysis, very appreciated! |
The
DnfSack
object has two properties that together cause an obscure bug:DnfSack
(very indirectly) owns thelibsolv
library's "page file file descriptors".DnfSack
is not immediately released indnf.Base.reset()
, due to circular references -- instead,DnfSack
is only released when Python's generational garbage collector thinks it's time to do so.The result of this is that file descriptors can be leaked for an unspecified amount of time after
dnf.Base.reset()
-- potentially called fromdnf.Base.close()
-- returns, until the garbage collector eventually cleans them up. Because the kernel sees those leaked file descriptors, they can interfere with operations on the filesystem they point into. For example, they can preventlivecd-creator
from unmounting the/var/cache/dnf
filesystem within theinstall_root
(chroot) filesystem.The commit message on the first patch contains an excruciatingly detailed bug analysis; the code change itself is minimal.
You can read my original analysis / investigation at https://bugzilla.redhat.com/show_bug.cgi?id=2038105, comments 45 through 61.
Please do not edit or squash my commits. If updates are needed, ask me for a force-push please. Thanks.