compose: Add rpmdb option, default to bdb #2221

cgwalters · 2020-09-10T18:27:55Z

The design of https://fedoraproject.org/wiki/Changes/Sqlite_Rpmdb
is problematic for us for multiple reasons. The first big reason
is that rpm-ostree is designed for "cross" builds and e.g. today
we use a Fedora-derived container to build RHEL CoreOS images.

However the default database lives inside the rpm package which
means that if we e.g. upgrade the coreos-assembler container to F33
it will suddenly try to use sqlite for RHCOS which is obviously broken.

Related to this, rebases from f32 to f33 w/layered packages
are broken: https://bugzilla.redhat.com/show_bug.cgi?id=1876194#c3

With this we can configure things to continue to use bdb for f33
for ostree-based systems, so that by enforcing an upgrade order
f32 -> f33 [bdb] -> f34 [sqlite] ... the intermediate f33 w/bdb
still understands sqlite and hence rebases will work.

cgwalters · 2020-09-10T18:28:45Z

Marking draft for lack of tests, but I did validate this works manually by creating a f33 version of my dev container, building rpm-ostree in that and validating two things:

Without rpmdb: sqlite set running cosa build still gives me FCOS w/bdb
Setting rpmdb: sqlite does what you expect

jlebon · 2020-09-10T19:22:33Z

Yeah, that's probably the cleaner approach overall. For FCOS at least, we could opt into sqlite much earlier than f34 because we have update barriers. I don't think we gain anything major on the technical side by doing it earlier, but it does have better optics to match the rest of Fedora.

cgwalters · 2020-09-10T20:53:20Z

OK I added some basic tests, but actually testing the sqlite path would require us to build a f33 cosa which we should do, just not sure I want to block on it.

rust/src/treefile.rs

src/libpriv/rpmostree-core.c

jlebon · 2020-09-10T21:44:43Z

src/libpriv/rpmostree-core.c

+  const char *db_backend = "bdb";
+  if (self->treefile_rs && ror_treefile_get_sqlite (self->treefile_rs))
+    db_backend = "sqlite";
+  dnf_context_set_rpm_macro (self->dnfctx, "_db_backend", db_backend);


Hmm, but actually shouldn't this be something like:

Suggested change

const char *db_backend = "bdb";

if (self->treefile_rs && ror_treefile_get_sqlite (self->treefile_rs))

db_backend = "sqlite";

dnf_context_set_rpm_macro (self->dnfctx, "_db_backend", db_backend);

if (self->treefile_rs)

dnf_context_set_rpm_macro (self->dnfctx, "_db_backend", ror_treefile_get_rpmdb (self->treefile_rs) ?: "bdb");

On the client side, we don't want to touch this macro at all, right?

Thinking more on this, I think what we want to do is also drop a file in /usr/lib/rpm/macros.d so it's part of the commit itself. So then dnf_context_set_rpm_macro would only be used server-side to make sure the baked rpmdb type matches the macro we write in postprocessing.

Thinking more on this, I think what we want to do is also drop a file in /usr/lib/rpm/macros.d so it's part of the commit itself.

Hmm. But that also will be picked up by any kind of "yum --installroot" that happens to run from the host context. I believe we'll transparently inherit the "base commit" default because rpm will autodetect the database type in the target root. I think. But we clearly need to test that.

RPM does do this correctly, as long as there's only one type of rpmdb in the rpmdb path. If there are two different types and one is specified in configuration, that one wins.

rust/src/treefile.rs

jlebon

Shouldn't the commit message say "default to bdb"? Apart from that, just one final comment about a comment.

jlebon · 2020-09-11T13:07:03Z

src/libpriv/rpmostree-core.c

@@ -753,6 +753,13 @@ rpmostree_context_setup (RpmOstreeContext    *self,
  /* This is what we use as default. */
  dnf_context_set_rpm_macro (self->dnfctx, "_dbpath", "/" RPMOSTREE_RPMDB_LOCATION);

+  // Default for now for compat reasons


This comment should be on the Rust side now.

Conan-Kudo · 2020-09-11T13:11:48Z

@jlebon It should default to whatever rpm defaults, rather than picking something. For Fedora, that'll be SQLite. For openSUSE, that'd be NDB. For RHEL (for now), that'll be BDB.

cgwalters · 2020-09-11T13:16:17Z

@jlebon It should default to whatever rpm defaults, rather than picking something. For Fedora, that'll be SQLite. For openSUSE, that'd be NDB. For RHEL (for now), that'll be BDB.

No - rpm-ostree is too strongly oriented towards "cross" builds. Perhaps in the future we could add an "auto" which inherits whatever the "host" librpm default is though.

The design of https://fedoraproject.org/wiki/Changes/Sqlite_Rpmdb is problematic for us for multiple reasons. The first big reason is that rpm-ostree is designed for "cross" builds and e.g. today we use a Fedora-derived container to build RHEL CoreOS images. However the default database lives inside the `rpm` package which means that if we e.g. upgrade the coreos-assembler container to F33 it will suddenly try to use sqlite for RHCOS which is obviously broken. Related to this, rebases from f32 to f33 w/layered packages are broken: https://bugzilla.redhat.com/show_bug.cgi?id=1876194#c3 With this we can configure things to continue to use bdb for f33 for ostree-based systems, so that by enforcing an upgrade order f32 → f33 [bdb] → f34 [sqlite] ... the intermediate f33 w/bdb still understands sqlite and hence rebases will work.

jlebon · 2020-09-11T13:24:52Z

/lgtm

openshift-ci-robot · 2020-09-11T13:24:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, Conan-Kudo, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

cgwalters · 2020-09-11T13:26:27Z

If anything, the default should somehow inherit from the target - but that's tricky to do automatically. Maybe in the future since repodata XML is enormous anyways we can do things like stick a tiny bit of metadata in there that has things like <osmetadata><rpmdb>sqlite</rpmdb></osmetadata>.

Conan-Kudo · 2020-09-11T13:26:37Z

@cgwalters "auto" is the behavior now, though, so absent this code, it should do that already.

cgwalters · 2020-09-11T13:28:26Z

@cgwalters "auto" is the behavior now, though, so absent this code, it should do that already.

It's auto only from the host which is wrong for the reason mentioned in the commit message:

However the default database lives inside the rpm package which
means that if we e.g. upgrade the coreos-assembler container to F33
it will suddenly try to use sqlite for RHCOS which is obviously broken.

A few goodies, but mostly to get coreos#2221 out.

A few goodies, but mostly to get #2221 out.

openshift-ci-robot requested review from jlebon and miabbott September 10, 2020 18:27

cgwalters marked this pull request as draft September 10, 2020 18:28

openshift-ci-robot added do-not-merge/work-in-progress approved labels Sep 10, 2020

cgwalters mentioned this pull request Sep 10, 2020

tracker: Fedora 33 rebase work coreos/fedora-coreos-tracker#609

Closed

cgwalters force-pushed the treefile-rpmdb branch from 04f41ed to 2221558 Compare September 10, 2020 20:52

cgwalters marked this pull request as ready for review September 10, 2020 20:52

openshift-ci-robot removed the do-not-merge/work-in-progress label Sep 10, 2020

jlebon reviewed Sep 10, 2020

View reviewed changes

cgwalters force-pushed the treefile-rpmdb branch from 2221558 to d19b7db Compare September 11, 2020 00:30

Conan-Kudo suggested changes Sep 11, 2020

View reviewed changes

rust/src/treefile.rs Outdated Show resolved Hide resolved

cgwalters force-pushed the treefile-rpmdb branch from d19b7db to 179a0f0 Compare September 11, 2020 11:51

Conan-Kudo approved these changes Sep 11, 2020

View reviewed changes

jlebon reviewed Sep 11, 2020

View reviewed changes

cgwalters force-pushed the treefile-rpmdb branch from 179a0f0 to 279e407 Compare September 11, 2020 13:18

jlebon changed the title ~~compose: Add rpmdb option, default to sqlite~~ compose: Add rpmdb option, default to bdb Sep 11, 2020

openshift-ci-robot assigned jlebon Sep 11, 2020

openshift-ci-robot added the lgtm label Sep 11, 2020

openshift-merge-robot merged commit 456a3ec into coreos:master Sep 11, 2020

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Sep 14, 2020

Release 2020.5

c5b4727

A few goodies, but mostly to get coreos#2221 out.

jlebon mentioned this pull request Sep 14, 2020

Release 2020.5 #2230

Merged

openshift-merge-robot pushed a commit that referenced this pull request Sep 15, 2020

Release 2020.5

ec94030

A few goodies, but mostly to get #2221 out.

jlebon mentioned this pull request Oct 23, 2020

Tracker: rpm DB backend change #1964

Closed

LorbusChris mentioned this pull request Nov 5, 2020

rpm database uses bdb not sqlite fedora-silverblue/issue-tracker#95

Closed

jlebon mentioned this pull request Nov 17, 2020

rpm commands erroring fedora-silverblue/issue-tracker#105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compose: Add rpmdb option, default to bdb #2221

compose: Add rpmdb option, default to bdb #2221

cgwalters commented Sep 10, 2020

cgwalters commented Sep 10, 2020

jlebon commented Sep 10, 2020

cgwalters commented Sep 10, 2020

jlebon Sep 10, 2020

jlebon Sep 10, 2020

cgwalters Sep 11, 2020

Conan-Kudo Sep 11, 2020

jlebon left a comment

jlebon Sep 11, 2020

Conan-Kudo commented Sep 11, 2020

cgwalters commented Sep 11, 2020

jlebon commented Sep 11, 2020

openshift-ci-robot commented Sep 11, 2020

cgwalters commented Sep 11, 2020

Conan-Kudo commented Sep 11, 2020

cgwalters commented Sep 11, 2020

compose: Add rpmdb option, default to bdb #2221

compose: Add rpmdb option, default to bdb #2221

Conversation

cgwalters commented Sep 10, 2020

cgwalters commented Sep 10, 2020

jlebon commented Sep 10, 2020

cgwalters commented Sep 10, 2020

jlebon Sep 10, 2020

Choose a reason for hiding this comment

jlebon Sep 10, 2020

Choose a reason for hiding this comment

cgwalters Sep 11, 2020

Choose a reason for hiding this comment

Conan-Kudo Sep 11, 2020

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

jlebon Sep 11, 2020

Choose a reason for hiding this comment

Conan-Kudo commented Sep 11, 2020

cgwalters commented Sep 11, 2020

jlebon commented Sep 11, 2020

openshift-ci-robot commented Sep 11, 2020

cgwalters commented Sep 11, 2020

Conan-Kudo commented Sep 11, 2020

cgwalters commented Sep 11, 2020