history: Keep reinstall action when reverting merged transaction items

[jan-kolarik]

When we have a sequence of transactions involving the reinstallation, upgrading, or downgrading of a package, which ultimately results in the package remaining at the same version, the expected behavior, when reverting the entire merged transaction, is that the package is reinstalled. This differs from the previous behavior, where the package would be removed from the system.

Resolves: https://issues.redhat.com/browse/RHEL-17494
Closes: #2031

[jan-kolarik]

An alternative solution could be also to "do nothing" instead of the reinstall action.

[jan-kolarik]

The only failing CI test is to be fixed by #2047.

[jan-kolarik]

I will also attach CI test scenario in the following days, so we have this covered.

[j-mracek]

This is interesting, because the original issue does not contain reinstall step

[jan-kolarik]

This is interesting, because the original issue does not contain reinstall step

If you mean the RHEL-17494 issue, there the merged transaction of the "rollbacking twice" use case results in the REINSTALL action which was up till now translated as "Reinstalled" by the reversion "action_map" table...

[jan-kolarik]

This is interesting, because the original issue does not contain reinstall step

If you mean the RHEL-17494 issue, there the merged transaction of the "rollbacking twice" use case results in the REINSTALL action which was up till now translated as "Reinstalled" by the reversion "action_map" table...

But anyway, I must say the whole workflow of merging transactions and reverting it through the map is definitely not clear and intuitive from the source code.

[j-mracek]

This is interesting, because the original issue does not contain reinstall step

If you mean the RHEL-17494 issue, there the merged transaction of the "rollbacking twice" use case results in the REINSTALL action which was up till now translated as "Reinstalled" by the reversion "action_map" table...

The action_map and the code of redo and rollback requires perfect input because it sets not only forward actions (install, upgrade), but also reverse actions (upgraded, ...). According to your comment it looks like that merge transaction algorithm returns incorrect result and the patch ignores one step because it might be wrong.

On the other hand the patch is probably going in the right direction. The current implementation is really not flexible enough.

[j-mracek]

Undo is not flexible because reverse transaction are also required to be valid.

# dnf install zchunk-devel-0:1.3.0-1.fc38.x86_64
# sudo dnf upgrade zchunk-devel
# sudo dnf history
    31 | upgrade zchunk-devel                                                                                                                                                                                    
    30 | install zchunk-devel-0:1.3.0-1.fc38.x86_64 
# sudo dnf history undo 30

[j-mracek]

I believe that the problem is here:

/**
 * Resolve the difference between RPMs in the first and second transaction item
 *  and create a ItemPair of Upgrade, Downgrade or reinstall.
 * Method is called when original package is being removed and than installed again.
 * \param previousItemPair original item pair
 * \param mTransItem new transaction item
 */
void
MergedTransaction::resolveRPMDifference(ItemPair &previousItemPair,
                                        TransactionItemBasePtr mTransItem)
{
    auto firstItem = previousItemPair.first->getItem();
    auto secondItem = mTransItem->getItem();

    auto firstRPM = std::dynamic_pointer_cast< RPMItem >(firstItem);
    auto secondRPM = std::dynamic_pointer_cast< RPMItem >(secondItem);

    if (firstRPM->getVersion() == secondRPM->getVersion() &&
        firstRPM->getEpoch() == secondRPM->getEpoch()) {
        // reinstall
        mTransItem->setAction(TransactionItemAction::REINSTALL);
        previousItemPair.first = mTransItem;
        previousItemPair.second = nullptr;
        return;

It think it creates the reinstall item and not reinstalled. It results in the problem. The question is whether this case when in and out have the same versions, it should make the transaction empty rather then to create reinstall package action.

What do you think?

[jan-kolarik]

I believe that the problem is here:

/**
 * Resolve the difference between RPMs in the first and second transaction item
 *  and create a ItemPair of Upgrade, Downgrade or reinstall.
 * Method is called when original package is being removed and than installed again.
 * \param previousItemPair original item pair
 * \param mTransItem new transaction item
 */
void
MergedTransaction::resolveRPMDifference(ItemPair &previousItemPair,
                                        TransactionItemBasePtr mTransItem)
{
    auto firstItem = previousItemPair.first->getItem();
    auto secondItem = mTransItem->getItem();

    auto firstRPM = std::dynamic_pointer_cast< RPMItem >(firstItem);
    auto secondRPM = std::dynamic_pointer_cast< RPMItem >(secondItem);

    if (firstRPM->getVersion() == secondRPM->getVersion() &&
        firstRPM->getEpoch() == secondRPM->getEpoch()) {
        // reinstall
        mTransItem->setAction(TransactionItemAction::REINSTALL);
        previousItemPair.first = mTransItem;
        previousItemPair.second = nullptr;
        return;

It think it creates the reinstall item and not reinstalled. It results in the problem. The question is whether this case when in and out have the same versions, it should make the transaction empty rather then to create reinstall package action.

What do you think?

I see, I will dive into that again and try to find the issues already existing in the library code.

[jan-kolarik]

@j-mracek I've reviewed the code and agree with you. If a package of a particular version is installed, and it would also be installed after multiple subsequent transactions, it seems more user-friendly to result in no operation rather than triggering a reinstallation. While I don't have a strong opinion on this, it appears to be a less invasive option for users. Merging of transaction items is indeed used only for the history rollback command, so this change won't impact other behaviors. I'll prepare a separate PR for this change.