dnfdaemon: Support to run transactions offline #1543

m-blaha · 2024-06-10T12:31:59Z

Adds support for "offline" option to Goal::do_transaction() dbus API method.

mcrha · 2024-06-10T15:24:07Z

dnf5daemon-server/services/goal/goal.cpp

-    download_packages(session, *transaction);
+    if (offline) {
+        session.store_transaction_offline();
+        // TODO(mblaha): signalize reboot?


Could you add API to check whether there's a pending offline update, please? It can be used by gnome-shell, to check whether the offline update is ready, then it asks the user [x] Install pending update in the power off/restart dialog and, when the user unchecks the option, the prepared update is "cancelled" (another new API) and the machine powers off/reboots without applying the offline update.

Thanks for suggestions! I'll add them. We actually have similar funcionality on CLI already - dnf offline status and dnf offline clean.

I tested the do_transaction part and it works as expected. Thank you.

mcrha · 2024-06-13T10:20:34Z

dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.Goal.xml

+        Clean the offline transaction configured for the next reboot by removing the /system-update symlink (see systemd.offline-updates(7)).
+
+        Following @options are supported:
+            - clean_datadir: boolean, default true


Would using a false as the default be a bit safer variant? You do not want to download 3GB of packages again just because you need to turn off the laptop and the app calling this does not pass the option, relying on the default value (or not knowing it exists, or when you split the option into clear packages, clear the other thing,...).

mcrha · 2024-06-13T10:21:17Z

dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.Goal.xml

+
+        Unknown options are ignored.
+    -->
+    <method name="do_transaction">


correct the name

mcrha · 2024-06-13T10:22:56Z

dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.Goal.xml

+        <arg name="options" type="a{sv}" direction="in" />
+        <arg name="transaction_cleaned" type="b" direction="out" />
+    </method>
+
 </interface>

 </node>


I'm not sure whether adding this to the Goal is the best place. For me, the Goal manages actual transactions, something ready to be started. These two look like a method not related to actual transactions.

I agree it's not the best possible place. It's kind of loosely connected - the offline transactions are scheduled using Goal::do_transaction(offline=true). But maybe introducing completely new Offline interface would be cleaner way.

I did not think of a completely new interface, I thought some could be extended, but it seems none matches cleanly. The Base does only the read_all_repos and some signals, the SessionManager is not ideal either. I mean, you are right, adding the Offline interface looks like the best option.

mcrha · 2024-06-13T16:28:43Z

dnf5daemon-server/dbus/interfaces/org.rpm.dnf.v0.Goal.xml

+
+        Check whether there is an offline transaction configured for the next reboot (i.e. checks the /system-update symlink existence).
+    -->
+    <method name="offline_transaction_pending">


maybe call it get_offline_transaction_pending?

Given we are moving to a new Offline interface, I'd call it simply check_pending(). What do you think?

Sure, sounds good to me. The get_status would imply it can be more than just "is/is-not ready".

Exactly. Actually we can later provide a call returning all the details about the scheduled transaction.

mcrha · 2024-06-13T16:50:24Z

Also, looking in how gnome-shell uses the PackageKit side of thing, there is a "trigger" method in the PackageKit's D-Bus "offline-update" interface and depending on what the user chose in the gnome-shell they run:

trigger("reboot") - when the user chose to reboot the system and left "install updates" checked in the dialog
trigger("power-off") - when the user chose to power off the system and left "install updates" checked in the dialog
cancel offline update - when the user unchecked the "install updates" option in any of these dialogs.

In other words, the "trigger" function can set what to do after the offline update is finished.

I see you have there a poweroff_after = false in the /usr/lib/sysimage/libdnf5/offline/offline-transaction-state.toml, thus it might be doable (if that thing does what I think it does).

m-blaha · 2024-06-17T06:58:54Z

The poweroff_after option says that after the updates were applied (during the reboot), the system should power off instead of rebooting to the updated environment - see rpm-software-management/dnf-plugins-core#467 .
It's not completely clear to me when this trigger function is executed - it seems that it somehow handles the Reboot and Shut down buttons to allow cancellation of the scheduled offline transaction (which can be done by calling the clean method of the Offline interface).

mcrha · 2024-06-17T07:46:15Z

The poweroff_after option says that after the updates were applied (during the reboot), the system should power off instead of rebooting to the updated environment - see rpm-software-management/dnf-plugins-core#467 .

Nice, in that case it behaves as I expected and it can be (re)used.

It's not completely clear to me when this trigger function is executed

I'm not sure whether the trigger method only sets the desired action to be done after the offline update is finished, or whether it also does the reboot itself. From reading the gnome-shell code they do both, call PackageKit method and then initiate the reboot. From that the naming trigger is inaccurate, because it does not trigger the update at all, it also tells the PackageKit what to do after reboot; at least from my point of view.

What dnf5 may do is to add something like set_finish_action(action) method with reboot and power-off values values, beside the existing clean() method.

m-blaha · 2024-06-17T11:30:21Z

I pushed a new version that

moves offline transaction methods to a separate Offline interface
sets default of clean_datadir option for Offline::clean() method to false
fixes the interface xml file
adds new set_finish_action() method

evan-goode · 2024-06-26T21:14:56Z

Looks good to me overall! A few comments:

Does there need to be a check_pending method at all if there are plans to create a more general get_status method?
If we do have a check_pending method, it should probably also check offline-transaction-state.toml to check that the symlink points to a valid transaction.
I would prefer to split clean(bool clean_datadir) into clean() and cancel(), since it's a bit confusing that the default behavior of clean is to not delete the contents of the datadir, which is different from dnf5 offline clean.

mcrha · 2024-07-01T14:19:46Z

Does there need to be a check_pending method at all if there are plans to create a more general get_status method?

You are right, having a single method sounds better than having two.

I would prefer to split clean(bool clean_datadir) into clean() and cancel(),

The suggestion makes sense to me too.

I'm not the change owner, my comment is from the "client/API consumer" point of view.

m-blaha · 2024-07-03T09:17:59Z

* Does there need to be a `check_pending` method at all if there are plans to create a more general `get_status` method?

* If we do have a `check_pending` method, it should probably also check offline-transaction-state.toml to check that the symlink points to a valid transaction.

Currently check_pending is a simple method that only checks whether any service (not only dnf5) has scheduled an offline upgrade that will be performed during the next reboot (i.e. checks the presence of /system-update magic symlink). This can be used by gnome software to offer the user option to reboot the system without performing the update.

It differs from dnf5 offline status command which prints info from offline-transaction-state.toml regardless presence of the magic symlink.

* I would prefer to split `clean(bool clean_datadir)` into `clean()` and `cancel()`, since it's a bit confusing that the default behavior of `clean` is to not delete the contents of the datadir, which is different from `dnf5 offline clean`.

I like this idea!

The patch adds a new supported option "offline" to the do_transaction method of the Goal interface. If set to "true", the transaction is not executed, but only prepared to run during the next reboot.

mcrha · 2024-07-03T10:21:43Z

Currently check_pending is a simple method that only checks whether any service (not only dnf5) has scheduled an offline upgrade

I think it's not great, neither the dnf offline status you described (or the later at least if it won't tell me that the upgrade is ready, while it's not scheduled).

When I ask dnf5 whether there's an offline update/upgrade ready, I mean with it that the dnf5 has scheduled update/upgrade for the next boot, not that dnf5 or any other software happened to have scheduled an offline update. Aka the symlink presence is not a good indicator that the offline update is ready. I understand it still means that something will do the update/upgrade", but as long as it's not the dnf5, I'd just return "no" when asking dnf5 about the state.

For me, the symlink is just an implementation detail. Maybe it'll work differently in the future, or without systemd, or whatever, thus I use the library API to check the status instead of using the symlink directly. I'd not even mention it in the API documentation.

Imagine a case where something else will schedule the offline update - the symlink is there. The user will pick in the Power Off dialog of the GNOME Shell to not update, then the gnome-shell will ask dnf5 to discard/postpone the update. Will dnf5 unconditionally remove the symlink, even when it's not its, or it'll give up? What if the cancellation means for the other app something else than just removing the symlink, maybe some cleanup, state change written somewhere, then the dnf5 would make the other app in an inconsistent state.

I'm only thinking aloud. Feel free to ignore me.

m-blaha · 2024-07-03T11:12:53Z

I'm only thinking aloud. Feel free to ignore me.

Actually your comment makes a lot of sense. You are right that dnf5 should not touch upgrades not scheduled by it.

Adds new `Offline` interface for interacting with offline transactions. The interface provides three methods: - check_pending() - check whether there is an offline update scheduled for the next reboot - cancel() - cancel the scheduled offline update - clean() - cancel the scheduled offline update and remove all stored offline transaction data. - set_finish_action() - to set whether after applying the offline transaction the system should be rebooted (default) or powered off.

m-blaha · 2024-07-08T06:32:31Z

I did another change suggested by reviewers:

check_pending returns true only if the offline transaction was scheduled by dnf5
split clean(bool) into
- cancel() which only removes the magic symlink (if created by dnf5) so the offline transaction is not executed during reboot
- clean() which cancels the transaction and removes all dnf5 stored offline data including downloaded packages
I'm still not sure whether we should replace check_pending with something more powerful like get_status()

mcrha · 2024-07-08T11:30:11Z

dnf5daemon-server/services/offline/offline.cpp

+        // set the poweroff_after item accordingly
+        std::string finish_action;
+        call >> finish_action;
+        state.get_data().set_poweroff_after(finish_action == "poweroff");


I'm fine to have it like this, even case sensitive, but it might be good to at least test whether the other action is "reboot" and claim a warning on the console about "unknown finish action" if it's neither "poweroff" nor "reboot". That way one can check for typos and the older library will not crash when a new value is proposed in the future.

This may touch also the documentation in the D-Bus interface.

I can fill the error_msg output parameter with such warning. Returning an error reply seems too me.

Returning an error, aka failing the call, would make the parameter strict. It's fine by me, it only has certain consequences.

OK then, let's make the value check strict and return an error in case of unsupported value.

Check that only one of "poweroff", or "reboot" value was passed to set_finish_action() call.

evan-goode · 2024-07-08T22:34:55Z

It differs from dnf5 offline status command which prints info from offline-transaction-state.toml regardless presence of the magic symlink.

Yeah, dnf5 offline status doesn't check for the symlink since it's assumed that the system will be immediately rebooted after dnf5 offline reboot sets the status to STATUS_READY and creates the symlink. But now with this dnf5daemon functionality, dnf5 offline status should probably check and report the status of the symlink (in user-friendly terms, not necessarily mentioning the word "symlink"). The user should be able to tell whether the transaction will occur when they reboot via any means or whether it will only occur when they run dnf5 offline reboot.

I'm still not sure whether we should replace check_pending with something more powerful like get_status()

I could go either way, but I still prefer one get_status function that returns two outputs (maybe is_pending and status). A caller interested in the status field (download-incomplete, download-complete, ready, etc.) is probably also interested in whether the transaction is scheduled to be performed next boot.

m-blaha · 2024-07-09T11:24:20Z

I'm still not sure whether we should replace check_pending with something more powerful like get_status()

I could go either way, but I still prefer one get_status function that returns two outputs (maybe is_pending and status). A caller interested in the status field (download-incomplete, download-complete, ready, etc.) is probably also interested in whether the transaction is scheduled to be performed next boot.

OK, so let's replace check_pending() with get_status() which will have two out parameters:

boolean is_pending (true if dnf5 offline transaction is scheduled for the next reboot, i.e. magic symlink exists and points to /usr/lib/sysimage/libdnf5/offline/)
dictionary string->variant with status of dnf5 offline transaction. It will contain all fields from offline-transaction-state.toml file (if it exists).

@evan-goode are you OK with this?

Replace check_pending() method of the Offline interface with richer get_status() method. The new method returns two values: - boolean whether there is a dnf5 offline transaction scheduled for the next reboot - a string->variant dictionary with the status of the dnf5 offline transaction

evan-goode

I like it!

I will be on PTO until 2024-07-16, so if you make more changes, or want another pair of eyes, you may want to find an additional reviewer.

m-blaha · 2024-07-09T19:53:57Z

Thanks! I don't plan any other changes right now, feel free to merge it.

mcrha · 2024-07-10T12:20:59Z

I can confirm these changes do what I'd expect to be done from the gnome-software side.

mcrha reviewed Jun 10, 2024

View reviewed changes

mcrha reviewed Jun 13, 2024

View reviewed changes

m-blaha force-pushed the mblaha/daemon-offline-transactions branch from 677b8b5 to 78d7390 Compare June 17, 2024 11:27

m-blaha added 2 commits July 3, 2024 11:44

dnfdaemon: Support for running a transaction offline

de57714

The patch adds a new supported option "offline" to the do_transaction method of the Goal interface. If set to "true", the transaction is not executed, but only prepared to run during the next reboot.

dnfdaemon-client: --offline option for transactions

d21f55e

m-blaha force-pushed the mblaha/daemon-offline-transactions branch from 880ae2b to e7db9ec Compare July 8, 2024 06:27

mcrha reviewed Jul 8, 2024

View reviewed changes

dnfdaemon: Strict set_finish_action() value check

29e5323

Check that only one of "poweroff", or "reboot" value was passed to set_finish_action() call.

evan-goode approved these changes Jul 9, 2024

View reviewed changes

m-blaha added this pull request to the merge queue Jul 10, 2024

Merged via the queue into main with commit 9c2e215 Jul 10, 2024
5 of 16 checks passed

m-blaha deleted the mblaha/daemon-offline-transactions branch July 10, 2024 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dnfdaemon: Support to run transactions offline #1543

dnfdaemon: Support to run transactions offline #1543

m-blaha commented Jun 10, 2024

mcrha Jun 10, 2024

m-blaha Jun 11, 2024

m-blaha Jun 13, 2024

mcrha Jun 13, 2024

mcrha Jun 13, 2024

mcrha Jun 13, 2024

mcrha Jun 13, 2024

m-blaha Jun 14, 2024

mcrha Jun 17, 2024

mcrha Jun 13, 2024

m-blaha Jun 17, 2024

mcrha Jun 17, 2024

m-blaha Jun 17, 2024

mcrha commented Jun 13, 2024

m-blaha commented Jun 17, 2024

mcrha commented Jun 17, 2024

m-blaha commented Jun 17, 2024

evan-goode commented Jun 26, 2024

mcrha commented Jul 1, 2024

m-blaha commented Jul 3, 2024

mcrha commented Jul 3, 2024

m-blaha commented Jul 3, 2024

m-blaha commented Jul 8, 2024

mcrha Jul 8, 2024 •

edited

Loading

m-blaha Jul 8, 2024

mcrha Jul 8, 2024

m-blaha Jul 8, 2024

evan-goode commented Jul 8, 2024

m-blaha commented Jul 9, 2024

evan-goode left a comment

m-blaha commented Jul 9, 2024

mcrha commented Jul 10, 2024

dnfdaemon: Support to run transactions offline #1543

dnfdaemon: Support to run transactions offline #1543

Conversation

m-blaha commented Jun 10, 2024

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

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

mcrha commented Jun 13, 2024

m-blaha commented Jun 17, 2024

mcrha commented Jun 17, 2024

m-blaha commented Jun 17, 2024

evan-goode commented Jun 26, 2024

mcrha commented Jul 1, 2024

m-blaha commented Jul 3, 2024

mcrha commented Jul 3, 2024

m-blaha commented Jul 3, 2024

m-blaha commented Jul 8, 2024

mcrha Jul 8, 2024 • edited Loading

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

evan-goode commented Jul 8, 2024

m-blaha commented Jul 9, 2024

evan-goode left a comment

Choose a reason for hiding this comment

m-blaha commented Jul 9, 2024

mcrha commented Jul 10, 2024

mcrha Jul 8, 2024 •

edited

Loading