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

Implement system-upgrade and arbitrary offline transactions #1280

Merged

Conversation

evan-goode
Copy link
Member

@evan-goode evan-goode commented Feb 27, 2024

This PR supersedes #1209.

Resolves #1052.

Together with #1264, resolves #1224.

This patch implements dnf5 system-upgrade as a regular command rather than a plugin. system-upgrade is responsible for performing offline upgrades of the system to a new major release. It has three main subcommands:

  • dnf5 system-upgrade download --releasever XX: Does a dry run of an upgrade to a new releasever and stores the resulting transaction for later.
  • dnf5 system-upgrade reboot: Creates a "magic symlink" at /system-update to trigger the start of dnf5-offline-transaction.service on the next boot, then reboots. See https://www.freedesktop.org/software/systemd/man/latest/systemd.offline-updates.html. dnf5-offline-transaction.service will take the place of the usual boot target so we can preform the upgrade without disturbing too many running processes.
  • dnf5 system-upgrade _execute: Not intended to be run by the user directly, but rather by dnf5-offline-transaction.service. Loads the transaction saved by system-upgrade download and performs the upgrade. Interacts with Plymouth (if it's available) to show upgrade progress on the animated boot screen. The usual DNF 5 stdout is additionally logged to journald. Reboots or powers off when the upgrade is complete.

This PR also adds a new command, dnf5 offline, which shares the reboot, log, and clean subcommands with dnf5 system-upgrade, and adds a new --offline argument that allows any operation to be performed offline, as proposed here: #1224 (comment). For example, the process for running an offline upgrade would look like:

$ sudo dnf5 upgrade --offline mypackage
Updating and loading repositories:
Repositories loaded.
...
Transaction stored to be performed offline. Run `dnf5 offline reboot` to reboot and run the transaction.
$ sudo dnf5 offline reboot
The system will now reboot to perform the offline transaction initiated by the following command:
	dnf5 upgrade --offline mypackage
Is this ok [y/N]: 

The following commands support --offline: autoremove, distro-sync, downgrade, install, group install, group upgrade, reinstall, remove, swap, and upgrade.

A caveat of making system-upgrade and offline regular commands instead of plugins is that libsystemd and sdbus-c++ now have to be build dependencies of dnf5, since dnf5 offline log uses these libraries to display logs from offline transactions.

Remaining tasks:

  • Add dnf5 offline-distrosync and dnf5 offline-upgrade aliases to dnf5 distro-sync --offline and dnf5 upgrade --offline, respectively
  • Add systemd build condition so we don't have to always depend on libsystemd+sdbus-c++
  • dnf5 offline status subcommand
  • Documentation
  • Go back through commit history of dnf-plugins-extras to make sure no historical issues with DNF 4 system-upgrade are present in the new implementation

I'll implement the tests in a separate PR if that works. We have the DNF 4 tests to go from: https://github.com/rpm-software-management/dnf-plugins-core/blob/master/tests/test_system_upgrade.py

@evan-goode evan-goode mentioned this pull request Feb 27, 2024
5 tasks
@evan-goode evan-goode force-pushed the evan-goode/offline-transactions branch from 3b12b42 to 589a4e6 Compare February 27, 2024 21:15
@jan-kolarik jan-kolarik self-requested a review February 28, 2024 07:24
@kontura
Copy link
Contributor

kontura commented Feb 28, 2024

I see this is still a draft but I would prefer if the commits got restructured.

Specifically I think adding the plugin and then removing it right away is just complicating things, I would squash the commits.

Also I think the system-upgrade: Refactor, allow any offline transaction commit could be split it smaller ones.
Basically each bullet point could be a separate commit.

@evan-goode
Copy link
Member Author

I see this is still a draft but I would prefer if the commits got restructured.

Specifically I think adding the plugin and then removing it right away is just complicating things, I would squash the commits.

Also I think the system-upgrade: Refactor, allow any offline transaction commit could be split it smaller ones. Basically each bullet point could be a separate commit.

Yeah, that's reasonable, I'll reorganize this. Since I'm doing it after the fact, it might not be super tidy (each intermediate commit may not compile), but the history will be more readable.

@evan-goode evan-goode force-pushed the evan-goode/offline-transactions branch from 589a4e6 to 94c3d97 Compare February 29, 2024 01:57
@evan-goode
Copy link
Member Author

OK, I've squashed the commits from the old PR (#1209) and split the large commit into (1) the addition of the offline and system-upgrade commands and (2) the --offline flag. More squashing might be needed as I add more things, but reviews now are appreciated.

@Conan-Kudo
Copy link
Member

This needs a rebase on current mainline. It does not apply.

dnf5/commands/offline/offline.cpp Outdated Show resolved Hide resolved
dnf5/commands/system-upgrade/system-upgrade.hpp Outdated Show resolved Hide resolved
include/libdnf5/offline/offline.hpp Outdated Show resolved Hide resolved
include/libdnf5/offline/offline.hpp Outdated Show resolved Hide resolved
Comment on lines 70 to 77
void read();
std::exception_ptr read_exception;
std::filesystem::path path;
OfflineTransactionStateData data;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we hide this into a p_impl?
Perhaps also the OfflineTransactionStateData could store the data in p_impl and have a bunch of getters/setters for the values?

This would allow us to do modifications later without breaking ABI.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we no longer need to do this if the offline module is not in libdnf5, right?

Comment on lines 78 to 93
TOML11_DEFINE_CONVERSION_NON_INTRUSIVE(
libdnf5::offline::OfflineTransactionStateData,
state_version,
status,
cachedir,
target_releasever,
system_releasever,
verb,
cmd_line,
poweroff_after,
enabled_repos,
disabled_repos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this also be moved to the cpp file?
The conversions are only used in offline.cpp right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but IMHO it's best to keep it in the header since all users of the struct should agree on how to serialize/deserialize it.

@Conan-Kudo
Copy link
Member

It probably makes sense to offer a build conditional for this since dnf5 on non-systemd distributions (such as Yocto) is going to be a thing.

@kontura kontura self-assigned this Mar 4, 2024
@evan-goode evan-goode force-pushed the evan-goode/offline-transactions branch from 94c3d97 to 56c9a02 Compare March 4, 2024 20:35
@evan-goode evan-goode force-pushed the evan-goode/offline-transactions branch 2 times, most recently from 8e39a69 to 995d379 Compare March 8, 2024 20:03
@evan-goode
Copy link
Member Author

It probably makes sense to offer a build conditional for this since dnf5 on non-systemd distributions (such as Yocto) is going to be a thing.

Done in 705de82, thanks for the reminder.

@evan-goode
Copy link
Member Author

I'm marking this as RFR now, I think it's in a good state. I'd like to add the tests in a separate PR.

@evan-goode evan-goode marked this pull request as ready for review March 12, 2024 04:40
Implement `offline` and `system-upgrade` commands. `dnf5 system-upgrade`
is intended to be backwards-compatible with `dnf4 system-upgrade`. All
of system-upgrade's functionality, with the exception of the `download`
subcommand, is implemented in the new `offline` command, and
system-upgrade shares subcommands with it.

This is more or less a port of `dnf4 system-upgrade`.

This behavior of `dnf5 offline` was proposed here: rpm-software-management#1224 (comment).

For rpm-software-management#1052
Adds a new --offline argument that allows any operation to be performed
offline, as proposed here:
rpm-software-management#1224 (comment).

The following commands support `--offline`: `autoremove`, `distro-sync`,
`downgrade`, `install`, `group install`, `group upgrade`, `reinstall`,
`remove`, `swap`, and `upgrade`.
- Disable autocomplete for dnf5 offline _execute
- Collapse SystemUpgradeSubcommand and its subclass SystemUpgradeDownloadCommand
- Move include/libdnf5/offline/offline.hpp to
  dnf5/include/dnf5/offline.hpp
- Add journald messages that use OFFLINE_FINISHED_ID, REBOOT_REQUESTED_ID, and DOWNLOAD_FINISHED_ID
- Make Context.should_store_offline private with a getter and setter
- Make cacheonly setting in `dnf5 offline _execute` actually work by
  applying it in `pre_configure`, not `configure`
- Translate some strings
--installroot with `system-upgrade` and `offline` should now work as it
does in DNF 4.
- Store module_platform_id in offline-transaction-state.toml
- Set nogpgcheck in `dnf5 offline execute` since the gpgcheck should
  have already been performed when preparing the transaction
@evan-goode evan-goode force-pushed the evan-goode/offline-transactions branch from bae9603 to 520d21b Compare March 12, 2024 04:41
@evan-goode evan-goode requested a review from kontura March 12, 2024 04:41
dnf5/commands/offline/offline.cpp Outdated Show resolved Hide resolved
dnf5.spec Outdated Show resolved Hide resolved
dnf5/context.cpp Show resolved Hide resolved
dnf5/commands/offline/offline.cpp Show resolved Hide resolved
doc/commands/offline.8.rst Show resolved Hide resolved
dnf5/CMakeLists.txt Outdated Show resolved Hide resolved
dnf5/CMakeLists.txt Outdated Show resolved Hide resolved
dnf5.spec Outdated Show resolved Hide resolved
@evan-goode evan-goode force-pushed the evan-goode/offline-transactions branch from 6799f77 to 377527c Compare March 15, 2024 05:08
Copy link
Contributor

@kontura kontura left a comment

Choose a reason for hiding this comment

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

It seems there is some issue with systemd, its not building on f38 and f39.

On Monday I would also like to run the CLI by everyone again to make sure they are ok with it. It is a lot of changes.

Comment on lines +134 to +151
['offline-distrosync']
type = 'command'
attached_command = 'distro-sync'
group_id = 'commands-compatibility-aliases'
complete = true
attached_named_args = [
{ id_path = 'distro-sync.offline' }
]

['offline-upgrade']
type = 'command'
attached_command = 'upgrade'
group_id = 'commands-compatibility-aliases'
complete = true
attached_named_args = [
{ id_path = 'upgrade.offline' }
]

Copy link
Contributor

@kontura kontura Mar 15, 2024

Choose a reason for hiding this comment

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

I see these aliases don't match the dnf4 commands.
It basically changes dnf offline-distrosync download -> dnf5 offline-distrosync and discards the rest.

We do have all the functionality so I think we could build the aliases to (more or less) fit.

Something like:

['offline-distrosync']
type = 'command'
attached_command = 'distro-sync'
group_id = 'commands-compatibility-aliases'
complete = true

['offline-distrosync.download']
type = 'command'
attached_command = 'distro-sync'
complete = true
attached_named_args = [
  { id_path = 'distro-sync.offline' }
]

['offline-distrosync.log']
type = 'command'
attached_command = 'offline.log'
complete = true
...

(If we do decide to do that I would rather do it in a separate PR.)

@evan-goode
Copy link
Member Author

It seems there is some issue with systemd, its not building on f38 and f39.

Alright, I think it's fixed. We needed to get the SYSTEMD_SYSTEM_UNIT_DIR variable from a pkgconfig file provided by systemd, not libsystemd-devel.

@kontura
Copy link
Contributor

kontura commented Mar 19, 2024

Thank you!

@kontura kontura added this pull request to the merge queue Mar 19, 2024
Merged via the queue into rpm-software-management:main with commit e8c5901 Mar 19, 2024
13 of 22 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
4 participants