Skip to content
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

Allow disabling SWDB writing #662

Closed
wants to merge 10 commits into from

Conversation

jlebon
Copy link
Contributor

@jlebon jlebon commented Dec 20, 2018

Only the last three commits are new here. The rest are from #660. I'll just copy the commit messages from the last two:

[transaction] Add DNF_TRANSACTION_FLAG_DISABLE_SWDB

Not all users of libdnf really need the functionality added by the SWDB
database. This is especially true for rpm-ostree-based variants of
Fedora and RHEL, which only use libdnf for composing on the server and
package layering on the client.

Some major differences that render the SWDB not useful for rpm-ostree:

  • rpm-ostree already keeps track of requested packages in a separate
    database (really, text file).
  • rpm-ostree always applies package installs from scratch on top of the
    base OSTree, so the concept of history is simply different (see e.g.
    Consider an rpm-ostree history command coreos/rpm-ostree#1489).
  • rpm-ostree updates are performed offline, i.e. not on the live system.
    As such, we don't need to track the state of the RPM transaction as it
    happens in a separate database. If librpm throws an error, we just
    throw out the whole offline rootfs.
  • OSTree commits themselves already contain the list of packages that
    were installed in them, so that we can consult the commit metadata (or
    even the rpmdb directly) if we need to render the RPM diff on a system
    or OSTree branch.

[context] Disable SWDB in alternate install roots

This gets to the heart of the distinction. If one is using libdnf to
operate on the live system, then an SWDB tracking those mutations makes
sense. If not, then it's likely libdnf is only being used as a low-level
tool to generate some other artifacts. If we detect such cases,
automatically, disable writing an SWDB.

Closes: #645

Requires: #660

This is still in use by some C clients, notably rpm-ostree.
It confused me when reading this code that the `Swdb` constructors
weren't all together.
This is actually a false positive since we only set it and use it if
`setGPGHomeEnv` is set. But of course, GCC doesn't know that. But still,
let's silence it so that we don't learn to ignore compiler warnings.
We already declared an `swdb` variable higher up defined to
`priv->swdb`. This function had a mix of `swdb` and `priv->swdb`. Only
use the former for consistency.
Putting the else on a newline here is weird. Let's just be consistent.
Add a new `dnf_transaction_new_with_flags()` which allows for setting
flags right from the start. This is most a convenience function but will
be necessary in the next patch, which requires being able to set flags
that affect initialization itself and so can't be a separate call.
Not all users of libdnf really need the functionality added by the SWDB
database. This is especially true for rpm-ostree-based variants of
Fedora and RHEL, which only use libdnf for composing on the server and
package layering on the client.

Some major differences that render the SWDB not useful for rpm-ostree:
- rpm-ostree already keeps track of requested packages in a separate
  database (really, text file).
- rpm-ostree always applies package installs from scratch on top of the
  base OSTree, so the concept of history is simply different (see e.g.
  coreos/rpm-ostree#1489).
- rpm-ostree updates are performed offline, i.e. not on the live system.
  As such, we don't need to track the state of the RPM transaction as it
  happens in a separate database. If librpm throws an error, we just
  throw out the whole offline rootfs.
- OSTree commits themselves already contain the list of packages that
  were installed in them, so that we can consult the commit metadata (or
  even the rpmdb directly) if we need to render the RPM diff on a system
  or OSTree branch.
@jlebon
Copy link
Contributor Author

jlebon commented Dec 20, 2018

This will allow rpm-ostree to stop using a fork of libdnf and go back to using master (see e.g. coreos/rpm-ostree#1381). We can then work towards stopping to bundle rpm-ostree at least on f29+ and el8.

@dustymabe
Copy link

thanks @jlebon!

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Dec 21, 2018

[context] Disable SWDB in alternate install roots

This actually doesn't make sense. That would be a regression from pre-SWDB in which the bootstrap transaction that creates the environment, as well as the further interactions with that environment get recorded in the transaction database. This is incredibly valuable for when you're working from a debug environment or similar, so I don't see how this makes any sense to do.

One of the consequences of this, for example, would be that Anaconda netinstall transactions wouldn't be recorded anymore. That's a huge problem.

@jlebon
Copy link
Contributor Author

jlebon commented Dec 21, 2018

Gotcha, makes sense. I initially was going to implement this by simply passing a flag from rpm-ostree, but then thought it might make more sense anyway to key off of the install root. Anyway, will switch back to using an explicit flag.

@cgwalters
Copy link
Collaborator

We can then work towards stopping to bundle rpm-ostree at least on f29+ and el8.

(Don't want to distract too much on this PR, nice work! I just want to say though that I am very hesitant about going back to dynamically linking libdnf. There were two silent ABI breaks in the past that were an enormous pain to debug. If we had gating tests in bodhi that run rpm-ostree's test suite though that would ameliorate my concern there though)

@jlebon
Copy link
Contributor Author

jlebon commented Dec 21, 2018

OK, switched to just exposing the new functionality for rpm-ostree! ⬆️

Don't want to distract too much on this PR, nice work! I just want to say though that I am very hesitant about going back to dynamically linking libdnf.

Yeah, understood. I do hope we can eventually go back to unbundling though. Definitely agreed we should make that conditional on improving the CI story first.

@dustymabe
Copy link

If we had gating tests in bodhi that run rpm-ostree's test suite though that would ameliorate my concern there though

Do we have that ability today? If gating isn't an option, is non-gating?

@Conan-Kudo
Copy link
Member

Conan-Kudo commented Dec 21, 2018

@jlebon There's no getter function for the new swdb flag. Can you please add one?

Add new getter & setter functions to allow the context's transaction to
be initialized with the new `DISABLE_SWDB` flag. This will be used by
rpm-ostree.
@jlebon
Copy link
Contributor Author

jlebon commented Dec 21, 2018

Done! ⬆️

@dmach
Copy link

dmach commented Dec 29, 2018

@jlebon I wrote an alternative patch: #664
I think it's simpler and doesn't impact existing code so much.
It just uses in-memory database to avoid disk writes.

Could you check if it makes sense to you and if it would work with rpm-ostree code?
In case we decide to use my patch, we should still use remaining commits from these PR (fixes, cleanup).

@rh-atomic-bot
Copy link

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

@jlebon
Copy link
Contributor Author

jlebon commented Jan 7, 2019

Prep patches merged.
Dropping remaining patches in favour of #664.

@jlebon jlebon closed this Jan 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants