Add `history undo` command #1452

kontura · 2024-04-26T06:32:24Z

It adds add_revert_transaction(...) API to Goal. This is used by undo and later should also be used for rollback but that requires merging of history transactions.

For #140.

j-mracek · 2024-04-29T09:04:30Z

May I ask you why users wants to use history undo command?

What do you think could be improved in comparison to DNF4?

libdnf5/base/goal.cpp

pmatilai · 2024-04-30T06:26:52Z

May I ask you why users wants to use history undo command?

This almost looks like a trick-question to me 😅

For me personally the most common use is to uninstall a thing after trying it out, ie typically "undo last". Undoing an upgrade to test whether upgrade broke/fixed stuff would be super useful, but of course that depends on more than just dnf features (the repos dont typically carry all possible versions)

j-mracek · 2024-04-30T12:14:08Z

May be I would be good to be verbose here. In DNF4, history commands are important but not useful much because the implementation is very strict. Undo is very important and can be very powerful.

Use case - revert the last transaction, but why

The transaction does not provide expected functionality
The transaction did not finish successfully and user wants to undo it
The second option might require reinstall of packages that we want to have after undo, but they are already installed.

Both operation of the same transaction have a different starting point - some packages might be not on the system.

The command does not provide only undo of the last transaction but there is no logic that react to any changes.
We can think about following cases:

1. install a-1-1.noarch
2. upgrade a-1-1.noarch to a-2-1.noarch
# dnf5 history undo 1

This may end up with following results

Fail due to package is installed in a different version (requires good message)
Pass with nothing to do, message - No packages to remove for argument: zsh-html-5.9-5.fc38.noarch (current behavior). It is may be a good behavior for installonly packages
Remove a-2-1.noarch because package a wasn't installed prior the transaction (only for packages that are not installonly)

Another use case

1. install a-1-1.noarch
2. remove a-1-1.noarch
# dnf5 history undo 1

This may end up with following results

Pass - nothing to do
Fail - due to the version is not present on the system

Another example:

1. install a-1-1.noarch that requires b, install dependency b-1-1.noarch
2.  install c-1-1.noarch that requires b
# dnf5 history undo 1

This may end up with following results

Remove only a
remove a, b, c
Fail due to transaction differs from expectations (removal of dependent package).

Additional cases with installonly packages.

Please don't take it as this implementation is wrong. I would like to brainstorm expectation for various use cases. Also I am sure that I've not covered all possibilities.

kontura · 2024-05-02T10:50:02Z

Use case - revert the last transaction, but why
1. The transaction does not provide expected functionality

2. The transaction did not finish successfully and user wants to undo it
   The second option might require reinstall of packages that we want to have after undo, but they are already installed.
Both operation of the same transaction have a different starting point - some packages might be not on the system.

I have also used it for this in the past but now that I think about it rollback command seems as a better fit for this use case because it doesn't have to deal with the questions @j-mracek asked below, it always reverts n last transactions.

The command does not provide only undo of the last transaction but there is no logic that react to any changes...

These are basically the same problems that the replay command has to deal with when replaying a transaction that doesn't apply cleanly. (The undo command is replay but the source of the transaction isn't a stored file but the history.)

The replay command in dnf4 is strict by default but there are options:

--ignore-installed    For the replay command, don't check for installed packages matching those in transaction
--ignore-extras       For the replay command, don't check for extra packages pulled into the transaction
--skip-unavailable    For the replay command, skip packages that are not available or have missing dependencies

We already have --skip-unavailable.
What about to preserve this approach and use it also for the undo command?

j-mracek · 2024-05-03T11:48:32Z

I don't think that the option mentioned above answer the question related to expected behavior. They only make no changes if something is unexpected.

Lets return to the first example

1. install a-1-1.noarch
2. upgrade a-1-1.noarch to a-2-1.noarch
# dnf5 history undo 1

The current behavior reports nothing to do with success and any of those options would help. I don't think that success is a correct result of the transaction, because it did nothing.

For this situation a user might expect

it will move package a to state prior transaction 1 - no package a installed on the system
ensure that a-1-1.noarch is not installed but other versions are fine
ensure that a-1-1.noarch is not installed but if something related to a is different (version), fail to allow user to
make a decision.

kontura · 2024-05-03T13:03:56Z

Yes I meant that in addition to those options we would make the command more strict (like the dnf4 replay command).

Regarding the possibilities you listed:

it will move package a to state prior transaction 1 - no package a installed on the system

This sounds like a rollback, personally I don't like this behavior for undo of a specific transaction.

ensure that a-1-1.noarch is not installed but other versions are fine

As far as I understand this is the current behavior.
It successfully ensured that a-1-1.noarch is not installed.

ensure that a-1-1.noarch is not installed but if something related to a is different (version), fail to allow user to
make a decision.

This would be the new default behavior and the user could specify --ignore-installed to make it pass (essentially like this test case: https://github.com/rpm-software-management/ci-dnf-stack/blob/main/dnf-behave-tests/dnf/transaction-sr/replay.feature#L1577-L1599).

What do you think?

j-mracek · 2024-05-22T05:27:18Z

ensure that a-1-1.noarch is not installed but if something related to a is different (version), fail to allow user to
make a decision.

This would be the new default behavior and the user could specify --ignore-installed to make it pass (essentially like this test case: https://github.com/rpm-software-management/ci-dnf-stack/blob/main/dnf-behave-tests/dnf/transaction-sr/replay.feature#L1577-L1599).

What do you think?

I think this is a better option then the current behavior. It requires good messaging to ensure that user understand the problem and possible solutions.

kontura · 2024-05-23T08:14:59Z

Ok I have added several commits and I tried to focus on clear messages and logs.

@j-mracek for the cases you mentioned it looks something like this:

1. install a-1-1.noarch
2. upgrade a-1-1.noarch to a-2-1.noarch
# dnf5 history undo 1
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Cannot perform action 'Remove' for Package 'a-1-1.noarch' becasue it is not installed.
You can try to add to command line:
  --ignore-installed to allow mismatches between installed and stored transaction packages.
# dnf5 history undo 1 --ignore-installed
Updating and loading repositories:
Repositories loaded.
Cannot perform action 'Remove' for Package 'a-1-1.noarch' becasue it is not installed.

Nothing to do.

(With --ignore-installed the operation succeeds but the warning remains.)

1. install a-1-1.noarch
2. remove a-1-1.noarch
# dnf5 history undo 1
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Cannot perform action 'Remove' for Package 'a-1-1.noarch' becasue it is not installed.
You can try to add to command line:
  --ignore-installed to allow mismatches between installed and stored transaction packages.
# dnf5 history undo 1 --ignore-installed
Updating and loading repositories:
Repositories loaded.
Cannot perform action 'Remove' for Package 'a-1-1.noarch' becasue it is not installed.

Nothing to do.

(Same as the previous example.)

1. install a-1-1.noarch that requires b, install dependency b-1-1.noarch
2. install c-1-1.noarch that requires b
# dnf5 history undo 1
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Extra package 'c-1-1.noarch' (with action 'Remove') which is not present in the stored transaction was pulled into the transaction.

You can try to add to command line:
  --ignore-extras to allow extra packages in the transaction
# dnf5 history undo 1 --ignore-extras
Removes a, b and c.

(This is a change in comparison to dnf4, the undo command has allow_erasing set which allows to remove c but it requires the --ignore-extras flag because c is not part of the original transaction.)

include/libdnf5/transaction/transaction_history.hpp

libdnf5/base/goal.cpp

j-mracek · 2024-05-31T10:56:56Z

libdnf5/base/goal.cpp

+        } else if (
+            package_replay.action == Action::REMOVE &&
+            (pkg.get_reason() == Reason::DEPENDENCY || pkg.get_reason() == Reason::WEAK_DEPENDENCY)) {
+            package_replay.reason = Reason::CLEAN;


May I ask you why the reason is set here to CLEAN? What about to not set no reason for any remove step?

My thinking was that this makes the transaction table more consistent.
It basically ensures that the removed dependencies are reported as Removing unused dependencies (like they would be for normal [not-reverted] transaction) not as Removing dependent packages.
If we keep the DEPENDENCY or WEAK_DEPENDENCY this reason is saved to the history because after resolving the replay code overrides the reasons.

However if there is some problem with this I am not against dropping it.

Thank you, this is a good idea. I am just curious whether the reason might be something else then user. Please keep it like it is.

The reason can be group and the remove operation works nicely.

libdnf5/base/goal.cpp

j-mracek · 2024-06-03T09:21:41Z

I have tested several scenario and it looks good. Only one thing is not perfect and it is related to messaging when a different version is installed and undo is supposed to remove. The behavior is correct.

1. install a-1-1.noarch
2. upgrade a-1-1.noarch to a-2-1.noarch
# dnf5 history undo 1
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Cannot perform action 'Remove' for Package 'a-1-1.noarch' becasue it is not installed.
You can try to add to command line:
  --ignore-installed to allow mismatches between installed and stored transaction packages.

In this case I would prefer to inform that 'a-1-1.noarch' is installed in a different version. And which one. The reason is that --ignore-installed will simply skip the action and undo will do nothing - no reason to perform undo at all.

j-mracek · 2024-06-03T10:52:25Z

libdnf5/base/goal.cpp

+            }
+            group_replay.action = reverted_action->second;
+        } else {
+            group_replay.action = transaction::TransactionItemAction::UPGRADE;


I've tested it and I think that the revert of upgrade by upgrade is confusing. I am suggesting to add a warning, that group upgrade operation cannot be reverted, only RPMs will be modify. No error, only a warning. Or not a warning but event log would be better (new type I guess).

j-mracek · 2024-06-03T11:06:43Z

libdnf5/base/goal.cpp

+            }
+            env_replay.action = reverted_action->second;
+        } else {
+            env_replay.action = transaction::TransactionItemAction::UPGRADE;


I am recommending to not present the operation as an upgrade, because in background it does nothing. But if new group appeared then it might modify groups.

Ok, I have turned into it a log event. Now a revert of a revert of group upgrade won't mention the group at all because the group action will disappear on the first revert but I think that is acceptable.

j-mracek · 2024-06-03T11:10:36Z

libdnf5/base/goal.cpp

+
+        // Don't fill in @System repo_id because it's not possible to perform forward actions from it
+        if (env.get_repoid() != "@System") {
+            env_replay.repo_id = env.get_repoid();


I believe that this is the same type of problem like with RPMs. What about to not use repo_id for undo?

j-mracek · 2024-06-03T11:11:49Z

libdnf5/base/goal.cpp

+        }
+
+        // Don't fill in @System repo_id because it's not possible to perform forward actions from it
+        if (group.get_repoid() != "@System") {


I believe that this is the same type of problem like with RPMs. What about to not use repo_id for undo?

I am even not sure whether for groups it make sense to handle repo_id at all because they are merged from multiple sources.

j-mracek · 2024-06-07T11:45:34Z

libdnf5/base/goal.cpp

+
+        const auto nevras = rpm::Nevra::parse(package_replay.nevra, {rpm::Nevra::Form::NEVRA});
+        libdnf_assert(
+            nevras.size() > 0, "Cannot parse rpm nevra \"{}\" while replaying transaction.", package_replay.nevra);


What about to change a condition - nevras.size() == 1. I believe that NEVRA type has only one solution.

j-mracek · 2024-06-07T11:50:12Z

libdnf5/base/log_event.cpp

@@ -153,23 +153,52 @@ std::string LogEvent::to_string(
                } else {
                    return ret.append(utils::sformat(_("No match for group package: {}"), *spec));
                }
+            } else if (goal_action_is_replay(action)) {
+                return ret.append(
+                    utils::sformat(_("Cannot perform {}, no match for: {}."), goal_action_to_string(action), *spec));


May I ask you to mark an order of arguments {1}. It will allow to change order for translators.

j-mracek · 2024-06-07T11:51:26Z

libdnf5/base/log_event.cpp

+        case GoalProblem::NOT_INSTALLED: {
+            if (goal_action_is_replay(action)) {
+                return ret.append(utils::sformat(
+                    _("Cannot perform {} for {} '{}' becasue it is not installed."),


May I ask you to mark an order of arguments {1}. It will allow to change order for translators. In this case what about to provide a context message for translators?

Added the marks but I don't know what are context messages? Can we perhaps add them separately?

It is used to explain what message means. The method is C_(<message for translaters>, <string to translate>) with two string arguments and in DNF5 it is used only here -

dnf5/libdnf5/base/transaction.cpp

Line 1373 in 3fa8d63

repos_with_skipped_checks, C_("It is a joining character for repositories IDs", ", "));

.

j-mracek · 2024-06-07T11:52:13Z

libdnf5/base/log_event.cpp

        case GoalProblem::NOT_INSTALLED_FOR_ARCHITECTURE:
            return ret.append(utils::sformat(
                _("Packages for argument '{}' available, but installed for a different architecture."), *spec));
        case GoalProblem::ONLY_SRC:
+            if (goal_action_is_replay(action)) {
+                return ret.append(utils::sformat(
+                    _("Cannot perform {} because '{}' matches only source packages."),


May I ask you to mark an order of arguments {1}. It will allow to change order for translators. In this case what about to provide a context message for translators? In this case the context message is questionable.

Added the marks but I don't know what are context messages? Can we perhaps add them separately?

j-mracek · 2024-06-07T11:52:56Z

libdnf5/base/log_event.cpp

            return ret.append(utils::sformat(_("Argument '{}' matches only source packages."), *spec));
        case GoalProblem::EXCLUDED:
+            if (goal_action_is_replay(action)) {
+                return ret.append(utils::sformat(
+                    _("Cannot perform {} because '{}' matches only excluded packages."),


May I ask you to mark an order of arguments {1}. It will allow to change order for translators. In this case what about to provide a context message for translators?

Added the marks but I don't know what are context messages? Can we perhaps add them separately?

j-mracek · 2024-06-07T11:53:03Z

libdnf5/base/log_event.cpp

            return ret.append(utils::sformat(_("Argument '{}' matches only excluded packages."), *spec));
        case GoalProblem::EXCLUDED_VERSIONLOCK:
+            if (goal_action_is_replay(action)) {
+                return ret.append(utils::sformat(
+                    _("Cannot perform {} because '{}' matches only packages excluded by versionlock."),


May I ask you to mark an order of arguments {1}. It will allow to change order for translators. In this case what about to provide a context message for translators?

Added the marks but I don't know what are context messages? Can we perhaps add them separately?

j-mracek · 2024-06-07T11:53:27Z

libdnf5/base/log_event.cpp

-                    _("Packages for argument '{}' installed and available, but in a different version => cannot "
-                      "reinstall"),
-                    *spec));
+                    _("Installed packages for argument '{}' are not available in repositories in the same version, "


May I ask you to mark an order of arguments {1}. It will allow to change order for translators.

j-mracek · 2024-06-07T11:54:44Z

libdnf5/base/log_event.cpp

+                    libdnf5::utils::string::join(additional_data, ",")));
+            } else if (goal_action_is_replay(action)) {
+                return ret.append(utils::sformat(
+                    _("Cannot perform {} because '{}' is installed in a different version: '{}'."),


May I ask you to mark an order of arguments {1}. It will allow to change order for translators. In this case what about to provide a context message for translators? Subjects are not explicitly mentioned, therefore the message without a context might be not clear.

Added the marks but I don't know what are context messages? Can we perhaps add them separately?

j-mracek · 2024-06-07T11:55:15Z

libdnf5/base/log_event.cpp

@@ -220,6 +256,13 @@ std::string LogEvent::to_string(
        case GoalProblem::WRITE_DEBUG:
            return ret.append(utils::sformat(_("Debug data written to \"{}\""), *additional_data.begin()));
        case GoalProblem::UNSUPPORTED_ACTION:
+            if (action == GoalAction::REVERT_COMPS_UPGRADE) {
+                return ret.append(utils::sformat(
+                    _("{} upgrade cannot be reverted, however associated package actions will be. ({} id: '{}') ."),


May I ask you to mark an order of arguments {1}. It will allow to change order for translators. In this case what about to provide a context message for translators?

Added the marks but I don't know what are context messages? Can we perhaps add them separately?

j-mracek · 2024-06-07T11:55:33Z

libdnf5/base/log_event.cpp

@@ -287,6 +330,13 @@ std::string LogEvent::to_string(
                _("Error: It is not possible to switch enabled streams of a module unless explicitly enabled via "
                  "configuration option module_stream_switch."));
        }
+        case GoalProblem::EXTRA: {
+            return ret.append(utils::sformat(
+                _("Extra package '{}' (with action '{}') which is not present in the stored transaction was pulled "


May I ask you to mark an order of arguments {1}.

j-mracek · 2024-06-07T11:56:27Z

libdnf5/transaction/transaction_sr.hpp

@@ -49,6 +49,7 @@ struct PackageReplay {
    TransactionItemAction action;
    TransactionItemReason reason;
    std::string group_id;
+    // This nevra doesn't contain epoch if it is 0
    std::string nevra;
    std::filesystem::path package_path;


May I ask you for a doc string for a package path?

j-mracek

Thank you very much for the PR update. This version is is good, but I have some suggestions that are not critical, let say they are more cosmetic. It means, it works according to expectations.

It adds checks for expected packages in transaction replay producing either a warning or an error if something is missing. This matches the dnf4 implementation.

It is configured by `ignore_extras` and `ignore_installed` `GoalJobSettings`. This matches the functionality in dnf4. It also removes some unused imports in transaction.cpp

j-mracek · 2024-06-10T08:55:52Z

LGTM, documentation will be delivered in the next PR.

PR that adds the API (rpm-software-management#1452) was merged right before rpm-software-management#1307 so its missing there.

kontura mentioned this pull request Apr 26, 2024

Enable history undo tests rpm-software-management/ci-dnf-stack#1497

Open

j-mracek self-assigned this Apr 29, 2024

j-mracek reviewed Apr 29, 2024

View reviewed changes

libdnf5/base/goal.cpp Outdated Show resolved Hide resolved

kontura force-pushed the play_undo branch from 953bd68 to 1f927e9 Compare May 2, 2024 09:41

kontura force-pushed the play_undo branch 2 times, most recently from e3df7c9 to 998aedd Compare May 22, 2024 12:27

kontura added the work in progress label May 22, 2024

kontura force-pushed the play_undo branch from 998aedd to 0ad55cd Compare May 23, 2024 08:04

kontura removed the work in progress label May 23, 2024

j-mracek reviewed May 31, 2024

View reviewed changes

include/libdnf5/transaction/transaction_history.hpp Show resolved Hide resolved

kontura force-pushed the play_undo branch from 0ad55cd to 1ad810b Compare May 31, 2024 08:44

j-mracek reviewed May 31, 2024

View reviewed changes

libdnf5/base/goal.cpp Outdated Show resolved Hide resolved

j-mracek reviewed May 31, 2024

View reviewed changes

libdnf5/base/goal.cpp Outdated Show resolved Hide resolved

j-mracek reviewed May 31, 2024

View reviewed changes

libdnf5/base/goal.cpp Outdated Show resolved Hide resolved

kontura force-pushed the play_undo branch 2 times, most recently from cd6b4d8 to cf9c5c5 Compare May 31, 2024 13:53

j-mracek reviewed Jun 3, 2024

View reviewed changes

j-mracek reviewed Jun 7, 2024

View reviewed changes

j-mracek requested changes Jun 7, 2024

View reviewed changes

kontura added 9 commits June 7, 2024 14:51

Add clarifying comments to PackageReplay

505ce63

Add clarifying comment for Replace action in transaction replay

dca15d8

Use ignore_installed option in transaction replay

caeb9dd

It adds checks for expected packages in transaction replay producing either a warning or an error if something is missing. This matches the dnf4 implementation.

Add replay goal actions to LogEvent messages

a700946

Enahnce INSTALLED_IN_DIFFERENT_VERSION for reinstall with available pkgs

0751277

Transaction replay: add checking for extra packages

14cf659

It is configured by `ignore_extras` and `ignore_installed` `GoalJobSettings`. This matches the functionality in dnf4. It also removes some unused imports in transaction.cpp

Add hints for --ignore-installed and --ignore-extras

db8e310

Turn reverting of comps upgrade into a warning

8c322d6

Add clarifying comments about path to replay

7fdb2b6

kontura force-pushed the play_undo branch from 73647b7 to 7467301 Compare June 7, 2024 13:11

replay: make condition check strict and use parsed nevra

2da5d66

kontura force-pushed the play_undo branch from 7467301 to 2da5d66 Compare June 7, 2024 13:15

j-mracek added this pull request to the merge queue Jun 10, 2024

Merged via the queue into rpm-software-management:main with commit af8f535 Jun 10, 2024
8 of 15 checks passed

kontura added a commit to kontura/dnf5 that referenced this pull request Jun 10, 2024

Mark goal_action_is_replay as LIBDNF_API to fix build

362e2c2

PR that adds the API (rpm-software-management#1452) was merged right before rpm-software-management#1307 so its missing there.

kontura mentioned this pull request Jun 10, 2024

Mark goal_action_is_replay as LIBDNF_API to fix build #1540

Closed

Add history undo command #1452

Add history undo command #1452

Conversation

kontura commented Apr 26, 2024 • edited by jan-kolarik

j-mracek commented Apr 29, 2024

pmatilai commented Apr 30, 2024

j-mracek commented Apr 30, 2024

kontura commented May 2, 2024

j-mracek commented May 3, 2024

kontura commented May 3, 2024

j-mracek commented May 22, 2024

kontura commented May 23, 2024 • edited

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

j-mracek commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-mracek Jun 3, 2024 • edited

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

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

j-mracek left a comment

Choose a reason for hiding this comment

j-mracek commented Jun 10, 2024

Add `history undo` command #1452

Add `history undo` command #1452

kontura commented Apr 26, 2024 •

edited by jan-kolarik

kontura commented May 23, 2024 •

edited

j-mracek Jun 3, 2024 •

edited