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

Merge evan-goode/bootc branch to master #2203

Merged
merged 7 commits into from
Feb 6, 2025

Conversation

evan-goode
Copy link
Member

Continued from #2180, had to change source branch to avoid rewriting history on the bootc branch directly.


This PR, along with rpm-software-management/libdnf#1683, merges support for the --transient flag and the persistence=auto|persist|transient configuration option for https://issues.redhat.com/browse/RHEL-70917. See also #2155.

Also included are some build files for Copr that may be useful for future feature branches similar to bootc.

Merging this PR doesn't mean that work on the --transient feature is 100% done, just that we're confident enough we're not going to totally scrap it and we want to continue its development on the master branch.

dcantrell and others added 4 commits February 5, 2025 09:46
Support automatic builds on git commits.  Used to generate up to date
RPMs of dnf for testing and development purposes.
Adds support for the --transient option on all transactions. Passing
--transient on a bootc system will call `bootc usr-overlay` to create a
transient writeable /usr and continue the transaction.

Specifying --transient on a non-bootc system will throw an error; we
don't want to mislead users to thinking this feature works on non-bootc
systems.

If --transient is not specified and the bootc system is in a locked
state, the operation will be aborted and a message will be printed
suggesting to try again with --transient.
Documents the new `--transient` command-line argument and `persistence`
configuration option. I tried to use a table for listing the valid
options for `persistence`, but RST does not automatically wrap table
cells containing long lines, so a list was much easier.
Using libostree gives us more detail about the current state of the
deployment than only checking whether /usr is writable.
@evan-goode
Copy link
Member Author

This PR requires rpm-software-management/libdnf#1692.

@ppisar ppisar self-assigned this Feb 5, 2025
@ppisar
Copy link
Contributor

ppisar commented Feb 5, 2025

When testing the previous pull request, I noticed few strange points about the user interface:

The error message changed from "Error: system is configured to be read-only." to "This bootc system is configured to be read-only." I get that the "error" word was scary. But do we need to talk specifically about bootc? The DNF implementation still uses OSTree API, and commands and I think Fedora's CoreOS is still based on plain rpm-ostreee. But maybe we already discussed this and I forgot the reason why bootc name is good enough.

The error message explains "Pass --transient to perform this and subsequent transactions in a transient overlay which will reset when the system reboots." I'm not a native English, but my understanding is that I call "dnf --transient ..." and then I can call "dnf" without that option until a reboot. That's not how it behaves. It's necessary to pass the --transient option again and again because the overlay remains read-only out DNF file system name space. I find it confusing. Would it be better to change the message to "Pass --transient to perform this transactions in a transient overlay which will reset when the system reboots."? I.e. not mentioning the "subsequent transactions"?

To keep /usr read-only after DNF is finished with a transient
transaction, we call `ostree admin unlock --transient` to mount the /usr
overlay as read-only by default. Then, we create a private mount
namespace for DNF and its child processes and remount the /usr overlayfs
as read/write in the private mountns.

os.unshare is unfortunately only available in Python >= 3.12, so we have
to call libc.unshare via Python ctypes here and hardcode the CLONE_NEWNS
flag that we need to pass.
dnf-bootc's only job is to Require python3-gobject-base, ostree,
ostree-libs, and util-linux-core, which are needed to interact with
bootc systems. We don't want to add these dependencies on `python3-dnf`
because we don't want them on non-bootc systems, so we use a subpackage.
@evan-goode
Copy link
Member Author

When testing the previous pull request, I noticed few strange points about the user interface:

The error message changed from "Error: system is configured to be read-only." to "This bootc system is configured to be read-only." I get that the "error" word was scary. But do we need to talk specifically about bootc? The DNF implementation still uses OSTree API, and commands and I think Fedora's CoreOS is still based on plain rpm-ostreee. But maybe we already discussed this and I forgot the reason why bootc name is good enough.

Yes, the "bootc" name was decided long ago. I think the main reason was to unify the language across RHEL Image Mode. Personally I think you're right that it would be better to use "OSTree" in the language here, but in the future it's conceivable we might add functionality or additional checks here that are bootc-specific. Or for example call bootc usr-overlay rather than ostree admin unlock, which would introduce a requirement on bootc. None of the new CLI options contain the word "bootc" (--transient, persistence=transient|persist|auto), it only appears in docs/error messages/status messages, so we could also update it later to "OSTree" if desired.

The error message explains "Pass --transient to perform this and subsequent transactions in a transient overlay which will reset when the system reboots." I'm not a native English, but my understanding is that I call "dnf --transient ..." and then I can call "dnf" without that option until a reboot. That's not how it behaves. It's necessary to pass the --transient option again and again because the overlay remains read-only out DNF file system name space. I find it confusing. Would it be better to change the message to "Pass --transient to perform this transactions in a transient overlay which will reset when the system reboots."? I.e. not mentioning the "subsequent transactions"?

You're absolutely right, that language was from an earlier version which did affect subsequent transactions. I've changed it per your suggestion (and rewrote history).

"perform this and subsequent transactions in a transient overlay which "
"will reset when the system reboots."))
raise CliError(_("Operation aborted."))
assert self.conf.persistence == "transient"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will always be a "transient". A few lines above is assert self.conf.persistence in ("auto", "transient"). And "auto" is handled.

But, OK. The extra assert doesn't break anything.

@jrohel jrohel self-assigned this Feb 6, 2025
Copy link
Contributor

@ppisar ppisar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the consolidated patch set. Besides the updated hawkey_version version and the the error message it's identical to the #2180.

The CI failure is caused by the updated hawkey_version value. It is already available since rpm-software-management/libdnf#1692 but it will be available to CI tomorrow when libdnf will have been rebuilt in nightly.

@jrohel
Copy link
Contributor

jrohel commented Feb 6, 2025

I've been going through the code and I don't see any problems preventing merging. But I'm not a bootc expert.

@ppisar
Copy link
Contributor

ppisar commented Feb 6, 2025

Right now I'm testing this code in a bootc system.

@ppisar
Copy link
Contributor

ppisar commented Feb 6, 2025

I successfully finished the testing.

@ppisar ppisar merged commit 5a4f6c4 into rpm-software-management:master Feb 6, 2025
2 of 11 checks passed
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.

4 participants