Add capabilities for storing and replaying a transaction #1005

kontura · 2023-11-09T09:31:47Z

This is a second iteration for: #978 I have made a new PR because it nearly completely changed and the old comments could be confusing.

Now there is only one approach and replaying a transaction must go through a libdnf5::Goal. The new API is:

void libdnf5::Goal::add_transaction_replay(const std::string & json_serialized_transaction, libdnf5::GoalJobSettings & settings_p);
std::string libdnf5::base::transaction::serialize_to_json();
std::string libdnf5::transaction::transaction::serialize_to_json();

For: #999

dnf5/commands/history/history_store.cpp

include/libdnf5/base/goal_elements.hpp

kontura · 2023-11-15T13:41:33Z

I have rebased the PR and also added support for installing rpm packages from path.

j-mracek · 2023-11-21T06:57:20Z

I am not sure whether dnf history replay is the best command. We can keep it as a compatibility alias, but I guess we could look for a better command. We already discussed to create do command to replace shell. What do you think?

include/libdnf5/base/transaction.hpp

include/libdnf5/base/goal.hpp

kontura · 2023-11-22T09:08:17Z

I am not sure whether dnf history replay is the best command. We can keep it as a compatibility alias, but I guess we could look for a better command. We already discussed to create do command to replace shell. What do you think?

Sure, I think we will also need some command for the offline actions. I would do that in a different PR thought.
I can remove the history replay or we can keep it if it is going to stay as an alias.

kontura · 2023-11-22T12:53:44Z

I have marked the new API as experimental and also extended the history store/replay commands description with [experimental] keyword.

dnf5/commands/history/history_store.cpp

libdnf5/base/goal.cpp

libdnf5/base/transaction.cpp

j-mracek · 2023-11-23T12:11:55Z

I discovered confusing output
I did dnf5 install acpi then dnf5 mark weak acpi, then stored the transaction with reason change, then dnf5 history replay transaction.json

dnf5 history replay transaction.json
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Argument 'acpi-0:1.7-20.fc38.x86_64' matches only excluded packages.

acpi was not excluded, because I was able to install it.

j-mracek · 2023-11-23T12:21:00Z

@kontura May I ask you to verify following scenario
sudo dnf5 history store 57
dnf5 history store 56
Expected fail - unable to replace the file

j-mracek · 2023-11-24T06:37:15Z

Similar to #1005 (comment)
When I have installed a different version when transaction is a reason change.

sudo dnf5 history replay transaction.json
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Argument 'iftop-0:1.0-0.30.pre4.fc38.x86_64' matches only excluded packages.

libdnf5/base/goal.cpp

j-mracek · 2023-11-24T07:18:39Z

libdnf5/base/goal.cpp

+                rpm_specs.emplace_back(GoalAction::REMOVE, package_replay.nevra, settings_per_package);
+            }
+        } else if (package_replay.action == transaction::TransactionItemAction::REASON_CHANGE) {
+            rpm_reason_change_specs.emplace_back(


Shell we pass full NEVRA here or only name.arch of the package?

It would make the REASON_CHANGE work even if a different version than expected is installed.
But why should it work differently than all other actions?

This is a good question with no good answer.

I can argue that:
dnf5 mark with argument dnf will be impossible to replay when a different version is installed. We also know, that internal implementation does not care about version and only stores name.arch information.
In the following example the original version was not in any repository

$ sudo dnf5 mark dependency yum $ dnf5 history store 61 $ sudo dnf5 upgrade yum $ sudo dnf5 mark user yum $ dnf5 history replay transaction.json Updating and loading repositories: Repositories loaded. Failed to resolve the transaction: No match for argument: yum-0:4.18.1-20231110004827.0.g41a287e2.fc38.noarch

When I replay reason change (to dep) after I changed installed version and also after I changed reason (from dep to user) I got following problem.

dnf5 history replay transaction.json Updating and loading repositories: Repositories loaded. Failed to resolve the transaction: Argument 'yum-0:4.18.1-20231130005028.8.g6229c1d4.fc38.noarch' matches only excluded packages.

I think that the implementation in PR is more strict then it has to be. Then it is not robust to any changes of installed package when the reason change is replayed.

From my point of view there is an option for this: --ignore-installed. It is not implemented in dnf5 yet but in old dnf it behaves like this.

For example: if there is a stored transaction updating a-1 to a-3 but a-2 is on the system. Running it normally will fail but with --ignore-installed it works.
It would make sense to me if REASON_CHANGE also worked like this.

What do you think?

kontura · 2023-11-29T11:45:38Z

@kontura May I ask you to verify following scenario
sudo dnf5 history store 57
dnf5 history store 56
Expected fail - unable to replace the file

Why is this expected to fail?
If the unprivileged user has write permissions in the current directory he can remove (overwrite) the transaction.json even if it was created by a root user no?

Similarly, I can do:

amatej@fedora ~ $ sudo echo "File by root" > file
[sudo] password for amatej:
amatej@fedora ~ $ echo "Overwritten file" > file
amatej@fedora ~ $ cat file
Overwritten file
amatej@fedora ~ $

kontura · 2023-11-29T12:11:11Z

I discovered confusing output I did dnf5 install acpi then dnf5 mark weak acpi, then stored the transaction with reason change, then dnf5 history replay transaction.json
dnf5 history replay transaction.json
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Argument 'acpi-0:1.7-20.fc38.x86_64' matches only excluded packages.
acpi was not excluded, because I was able to install it.

I am not able to reproduce this, for me it says:

Updating and loading repositories:
Repositories loaded.
Package "acpi-1.7-21.fc39.x86_64" is already installed with reason "Weak Dependency".

Nothing to do.

Which seems reasonable.
Is there some step missing in the reproducer?

This is the history db transaction.

This is used during transaction replay to ensure stored reasons are used after the transaction is resolved again.

This is used during transaction replay because it always installs/removes/upgrades groups without their packages (they are handled separately).

…action

j-mracek · 2023-11-30T14:34:19Z

I discovered confusing output I did dnf5 install acpi then dnf5 mark weak acpi, then stored the transaction with reason change, then dnf5 history replay transaction.json
dnf5 history replay transaction.json
Updating and loading repositories:
Repositories loaded.
Failed to resolve the transaction:
Argument 'acpi-0:1.7-20.fc38.x86_64' matches only excluded packages.
acpi was not excluded, because I was able to install it.
I am not able to reproduce this, for me it says:
Updating and loading repositories:
Repositories loaded.
Package "acpi-1.7-21.fc39.x86_64" is already installed with reason "Weak Dependency".

Nothing to do.
Which seems reasonable. Is there some step missing in the reproducer?

The problem was fixed just recently (After my testing) - see 763f9cc

j-mracek · 2023-11-30T14:59:32Z

@kontura May I ask you to verify following scenario
sudo dnf5 history store 57
dnf5 history store 56
Expected fail - unable to replace the file

Why is this expected to fail? If the unprivileged user has write permissions in the current directory he can remove (overwrite) the transaction.json even if it was created by a root user no?

Similarly, I can do:
amatej@fedora ~ $ sudo echo "File by root" > file
[sudo] password for amatej:
amatej@fedora ~ $ echo "Overwritten file" > file
amatej@fedora ~ $ cat file
Overwritten file
amatej@fedora ~ $

I compared the behavior with

sudo dnf5 install acpi --debugsolver
dnf install acpi --debugsolver

The second command ends with

filesystem error: cannot remove: Permission denied [/tmp/test-down/./debugdata/modules/testcase.t]

j-mracek · 2023-12-01T12:16:05Z

I think there are 3 remaining items:

Behavior related to reason change
name of command history replay
allow silent file override for transaction.json
- Personally I have no problem with that, but it would be good that DNF5 will behave uniformly in those situations

I consider only - name of command 'history replay' as a item that we should address in this PR.

kontura · 2023-12-04T09:28:14Z

I compared the behavior with

sudo dnf5 install acpi --debugsolver
dnf install acpi --debugsolver

The second command ends with

filesystem error: cannot remove: Permission denied [/tmp/test-down/./debugdata/modules/testcase.t]

Yes, I think this is because --debugsolver creates a directory where the unprivileged user doesn't have permission and so he cannot remove files in it. This is not really related to dnf, it is caused by the file permissions model.

However I have added a commit which adds a user confirmation request if a file should be overwritten. This matches old dnf behavior.

kontura · 2023-12-04T10:52:00Z

I think there are 3 remaining items:
* Behavior related to reason change

* name of command `history replay`

* allow silent file override for `transaction.json`
  
  * Personally I have no problem with that, but it would be good that DNF5 will behave uniformly in those situations
I consider only - name of command 'history replay' as a item that we should address in this PR.

I have dropped the history replay commit so that it doesn't block the PR.

j-mracek · 2023-12-04T12:11:11Z

LGTM

kontura mentioned this pull request Nov 9, 2023

Add TransactionReplay and use it history replay command #978

Closed

kontura force-pushed the replay2 branch from 398803e to f07a157 Compare November 9, 2023 09:33

kontura requested a review from j-mracek November 9, 2023 09:35

j-mracek self-assigned this Nov 13, 2023

j-mracek reviewed Nov 13, 2023

View reviewed changes

dnf5/commands/history/history_store.cpp Outdated Show resolved Hide resolved

j-mracek reviewed Nov 13, 2023

View reviewed changes

include/libdnf5/base/goal_elements.hpp Outdated Show resolved Hide resolved

kontura force-pushed the replay2 branch from f07a157 to 5cbfc19 Compare November 15, 2023 13:38

kontura marked this pull request as ready for review November 15, 2023 13:40

j-mracek reviewed Nov 21, 2023

View reviewed changes

include/libdnf5/base/transaction.hpp Outdated Show resolved Hide resolved

j-mracek reviewed Nov 21, 2023

View reviewed changes

include/libdnf5/base/goal.hpp Outdated Show resolved Hide resolved

kontura force-pushed the replay2 branch 2 times, most recently from 53f1f84 to c78188b Compare November 22, 2023 12:51