context, sack: Support all rpmdb path variants #915

Conan-Kudo · 2020-03-18T12:08:33Z

We rely on identifying whether the rpmdb path has changed to
determine whether we need to re-cache data from there. Now
that RPM has multiple rpmdb options and there are two common
paths in use by RPM-based systems, we need to handle all of
these.

Fixes #362

Conan-Kudo · 2020-03-18T12:11:10Z

@dmach, @j-mracek: This is a bit on the ugly side, but it should remove the only remaining BDB rpmdb assumption left in libdnf.

Conan-Kudo · 2020-03-18T12:13:49Z

@dmach, @j-mracek, @jrohel, @inknos: Can one of you folks review this ASAP? I actually need this for openSUSE as they're switching to ndb rpmdb now and getting rid of the /var/lib/rpm symlink to /usr/lib/sysimage/rpm.

Conan-Kudo · 2020-03-18T12:22:10Z

@rh-atomic-bot try

rh-atomic-bot · 2020-03-18T12:22:15Z

⌛ Trying commit e7a0591 with merge 43b5d2a...

We rely on identifying whether the rpmdb path has changed to determine whether we need to re-cache data from there. Now that RPM has multiple rpmdb options and there are two common paths in use by RPM-based systems, we need to handle all of these. Closes: #915 Approved by: <try>

j-mracek · 2020-03-18T12:44:39Z

The patch cannot destroy dnf because we do not use rpmdb cache. But from a general perspective the patch is wrong. It is workarrond that temporary resolves the issue but in long term perspective it will cause the problem. Specially in case of downgrade of rpm it will be a disaster.

Conan-Kudo · 2020-03-18T12:46:51Z

@j-mracek The correct "long-term" fix would be to rework this to get the rpmdb information from rpm itself, or use new RPM APIs (if any exist) for dealing with this. I figure that would probably make sense for dnf5, since we can raise the floor to RPM 4.16 API there.

j-mracek · 2020-03-18T12:53:32Z

It will be enough to ask rpm to return path to rpmdb. @pmatilai Is in RPM something like that?

pmatilai · 2020-03-18T13:01:08Z

No, rpm explicitly does not expose internal db paths because they are not anybodys business.

Rpm >= 4.15 has rpmdbCookie() which can be used to retrieve a "has rpmdb changed" hash, much like libsolv/libdnf is now calculating on its own. Rpm >= 4.16 will additionally have rpmdbStat() and rpmdbFStat() for performing a stat() -like operation on the rpmdb.

Conan-Kudo · 2020-03-18T13:03:05Z

Rpm >= 4.15 has rpmdbCookie() which can be used to retrieve a "has rpmdb changed" hash, much like libsolv/libdnf is now calculating on its own.

Unless this can be backported to 4.14.x and made available in both RHEL 8 and SLE 15 rpm (cc: @mlschroe), I don't think we can rely on that for dnf 4.x.

pmatilai · 2020-03-18T13:05:52Z

Oh and actually looking at the patch: you certainly shouldn't be hard-coding /var/lib/rpm and the like for the base directory anywhere. Rpm itself will simply look at %{_dbpath} macro, I see no reason why dnf should do anything else.

pmatilai · 2020-03-18T13:09:45Z

Talking about backports seems premature when we don't have any implementation taking advantage of that stuff. Well, libsolv internally uses rpmdbFStat() if available already.

Conan-Kudo · 2020-03-18T13:11:35Z

@pmatilai I'm certainly not going to change to use an API that I can't use, as I have to care about openSUSE Leap 15.x with RPM 4.14.x.

pmatilai · 2020-03-18T13:20:14Z

Methods such as conditional compilation exist to deal with s*** that is only available in newer versions. It's not like this is in any way a special, previously unheard of situation.

Rpm 4.14.x doesn't really have this whole issue as there's exactly one supported backend.

Conan-Kudo · 2020-03-18T13:22:05Z

@pmatilai In RHEL yes. But not in SUSE distributions, where NDB is also supported since SLE 15 SP2 (openSUSE Leap 15.2). And in openSUSE Tumbleweed (which will become SLE 16/openSUSE Leap 16), NDB will be the only rpmdb backend in a matter of days...

pmatilai · 2020-03-18T13:27:32Z

I'm only talking about upstream rpm here. Distro backports are issues of said distros.
I don't understand the argument here at all so I'll just shut up now.

Conan-Kudo · 2020-03-18T13:46:27Z

@pmatilai You're working on rpm 4.14.3 release. If that API is present there, then I can use it. Otherwise, I don't know how I could, other than doing function check and fallback, which would still lead to the code looking like this anyway.

rh-atomic-bot · 2020-03-18T14:31:57Z

💔 Test failed - status-papr

j-mracek · 2020-03-18T16:06:57Z

libdnf/dnf-sack.cpp

@@ -226,7 +226,14 @@ dnf_sack_new(void)
 static int
 current_rpmdb_checksum(Pool *pool, unsigned char csout[CHKSUM_BYTES])
 {
-    const char *rpmdb_prefix_paths[] = { "/var/lib/rpm/Packages",


We can drop support of DNF_SACK_LOAD_FLAG_BUILD_CACHE for system repo.
There are two good reasons for that:

The calculation of checksumm doesn't work correctly in some cases (the reason why DNF stop to support it)

There is no performance benefit

Heh, discovery and dropping of non-working baggage is one of the better possible outcomes 😁

Please don't forget we'll need similar functionality in the new dnf-daemon we're going to deliver as part of dnf5 work.
Basically something to help the daemon recognize that rpmdb has changed and it should reload data.

j-mracek · 2020-03-18T16:25:13Z

libdnf/dnf-context.cpp

    /* setup a file monitor on the rpmdb, if we're operating on the native / */
    if (g_strcmp0(priv->install_root, "/") == 0) {
-        rpmdb_path = g_build_filename(priv->install_root, "var/lib/rpm/Packages", NULL);


The file path is used to set signal when change of RPMDB take place.

g_signal_connect(priv->monitor_rpmdb, "changed", G_CALLBACK(dnf_context_rpmdb_changed_cb), context);

@pmatilai Please is there any option how to set such a callback inside rpm? I am looking how to not need to know where RPM stores data. Deleted code will not produce any future issue.

Rpm doesn't support any "changed" callbacks on the rpmdb. What's the use-case of that? This sounds to me like a symptom of something else (locking probably) needing fixing.

This functionality is required for PackageKit daemon. Daemon loads data (installed and available packages) and reload them only when something is changed. Like when something is installed using rpm, PackageKit daemon must get signal that RPMDB was changed.

Ack, figures.

Maybe it could simply watch for the entire directory instead.

Anyway, the existing additions for rpmdb change monitoring do almost nothing for this particular use-case, they only really help the kind of on-startup cache-validity check that libsolv, yum etc do (heck even apt-rpm did that). Opened a ticket for adding change notifications but that's not going to help right now:

rpm-software-management/rpm#1124

pmatilai · 2020-03-19T06:55:24Z

@Conan-Kudo: at this point, what matters is that we find a proper solution to these "lookey, this here file is the rpmdb" path games going forward. Until that solution exists yacking about backports is nothing but stop-energy pollution - we don't even know yet whether rpmdbCookie() or any of the new related APIs are useful or will be used for libdnf.

j-mracek · 2020-03-19T10:34:30Z

@Conan-Kudo 50% of the problem could be resolved by #916

We rely on identifying whether the rpmdb path has changed to determine whether we need to re-cache data from there. Now that RPM has multiple rpmdb options and there are two common paths in use by RPM-based systems, we need to handle all of these.

Conan-Kudo · 2020-03-22T15:20:57Z

@rh-atomic-bot retry

Conan-Kudo · 2020-03-22T23:13:31Z

@j-mracek @dmach Unfortunately, I'm out of time, so I'm shipping this patch in the openSUSE libdnf package: https://build.opensuse.org/request/show/787280

j-mracek · 2020-03-23T11:46:05Z

I would like to allow rpmdb cache for DNF-5, but we should use rpmdbCookie() to get rpmdn checksum. Hope that RPM will cache checksumm therefore it will be not calculated after each call. But for the second part, we need for a daemon mechanism how send a signal that rpmdb was changed. @pmatilai Please could you investigate it?

pmatilai · 2020-03-23T12:17:49Z

The problem is that librpm has no means to track multiprocess users for notification, we'd basically need a daemon-process for ourselves through which all access to rpmdb is managed to achieve that. I suppose in theory we could add a some rpm-specific convenience wrapper for inotify but I'm not sure that actually achieves anything at all.

Like noted earlier, I'd suggest simply replacing the current Packages monitor by monitoring the entire %_dbpath directory and when modifications are detected, do rpmdbCookie() to see if its relevant for you.

m-blaha · 2020-03-24T06:53:29Z

@pmatilai I wanted to do quick check how rpmdbCookie() works but I'm probably doing something wrong and I couldn't get any useful output from it:

[mblaha:~]$ rpm -q rpm
rpm-4.15.1-1.fc31.x86_64
[mblaha:~]$ python3
Python 3.7.6 (default, Jan 30 2020, 09:44:41) 
[GCC 9.2.1 20190827 (Red Hat 9.2.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
TAB completer active
>>> import rpm
>>> ts = rpm.TransactionSet('/')
>>> print(ts.dbCookie())
None
>>>

@j-mracek From the implementation of rpmdbCookie() the value is not cached and is calculated on each call again.

m-blaha · 2020-03-24T06:58:55Z

@pmatilai oh, my bad. I needed to do ts.openDB() first. Then the cookie is returned.

pmatilai · 2020-03-24T07:09:20Z

Yup, the db needs to be open for that. It should probably emit some kind of noises when used on a closed db...
On a related note, if you're a daemon you should also close it afterwards to avoid hanging on to locks.

rh-atomic-bot · 2020-04-28T12:32:46Z

☔ The latest upstream changes (presumably ee81887) made this pull request unmergeable. Please resolve the merge conflicts.

Conan-Kudo · 2020-04-28T12:53:02Z

This is obsolete with #916 merged.

j-mracek reviewed Mar 18, 2020

View reviewed changes

pmatilai mentioned this pull request Mar 19, 2020

libdnf assumes rpmdb is in Berkeley DB format #362

Closed

Conan-Kudo force-pushed the handle-all-rpmdb branch from e7a0591 to 9584c58 Compare March 22, 2020 15:15

Conan-Kudo force-pushed the handle-all-rpmdb branch from 9584c58 to a379606 Compare March 22, 2020 15:20

Conan-Kudo closed this Apr 28, 2020

cgwalters mentioned this pull request Jan 24, 2023

disable rpmdb monitoring in build/composes coreos/rpm-ostree#4029

Open

context, sack: Support all rpmdb path variants #915

context, sack: Support all rpmdb path variants #915

Conversation

Conan-Kudo commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020 • edited

Conan-Kudo commented Mar 18, 2020

rh-atomic-bot commented Mar 18, 2020

j-mracek commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020

j-mracek commented Mar 18, 2020

pmatilai commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020

pmatilai commented Mar 18, 2020 • edited

pmatilai commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020

pmatilai commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020

pmatilai commented Mar 18, 2020

Conan-Kudo commented Mar 18, 2020

rh-atomic-bot commented Mar 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai commented Mar 19, 2020

j-mracek commented Mar 19, 2020

Conan-Kudo commented Mar 22, 2020

Conan-Kudo commented Mar 22, 2020

j-mracek commented Mar 23, 2020

pmatilai commented Mar 23, 2020

m-blaha commented Mar 24, 2020

m-blaha commented Mar 24, 2020

pmatilai commented Mar 24, 2020

rh-atomic-bot commented Apr 28, 2020

Conan-Kudo commented Apr 28, 2020

Conan-Kudo commented Mar 18, 2020 •

edited

pmatilai commented Mar 18, 2020 •

edited