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

Add history undo command #1452

Merged
merged 17 commits into from
Jun 10, 2024
Merged

Conversation

kontura
Copy link
Contributor

@kontura kontura commented Apr 26, 2024

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
Copy link
Contributor

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 Outdated Show resolved Hide resolved
@pmatilai
Copy link
Member

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
Copy link
Contributor

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

  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.

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
Copy link
Contributor Author

kontura commented May 2, 2024

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
Copy link
Contributor

j-mracek commented May 3, 2024

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
Copy link
Contributor Author

kontura commented May 3, 2024

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
Copy link
Contributor

  • 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
Copy link
Contributor Author

kontura commented May 23, 2024

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.)

libdnf5/base/goal.cpp Outdated Show resolved Hide resolved
libdnf5/base/goal.cpp Outdated Show resolved Hide resolved
} else if (
package_replay.action == Action::REMOVE &&
(pkg.get_reason() == Reason::DEPENDENCY || pkg.get_reason() == Reason::WEAK_DEPENDENCY)) {
package_replay.reason = Reason::CLEAN;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

libdnf5/base/goal.cpp Outdated Show resolved Hide resolved
@kontura kontura force-pushed the play_undo branch 2 times, most recently from cd6b4d8 to cf9c5c5 Compare May 31, 2024 13:53
@j-mracek
Copy link
Contributor

j-mracek commented Jun 3, 2024

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.

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

Choose a reason for hiding this comment

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

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).

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// 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();
Copy link
Contributor

@j-mracek j-mracek Jun 3, 2024

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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


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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

@@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 -

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

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."),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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."),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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."),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

_("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, "
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

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: '{}'."),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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: '{}') ."),
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@@ -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 "
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

Copy link
Contributor

@j-mracek j-mracek left a comment

Choose a reason for hiding this comment

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

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.

@j-mracek
Copy link
Contributor

LGTM, documentation will be delivered in the next PR.

@j-mracek 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
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.

3 participants