Transaction Store/Replay #1630

lukash · 2020-06-04T15:45:47Z

The basics work, there are TODOs scattered through the code.

What remains to be done:

Check for when a package in transaction wasn't found
Check for when an additional package was pulled in on the replay side
Check the "from" versions on the target system (for a --strict mode?)
Support for groups
Cleanups

I'm calling the modules etc. in various places transaction_sr or transaction-sr, though I'm not too happy with the name, would change it to a better one but can't think of any.

Requires this libdnf PR: rpm-software-management/libdnf#972
Tests: rpm-software-management/ci-dnf-stack#845

lgtm-com · 2020-06-04T15:54:04Z

This pull request introduces 2 alerts and fixes 1 when merging 8d26f50 into 126d861 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-06-05T18:07:59Z

This pull request introduces 2 alerts and fixes 1 when merging 251ba6d into 126d861 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lukash · 2020-06-05T18:09:20Z

I've added all the check mentioned earlier: Check for when a package wasn't found, check for extra packages in transaction and check for already installed versions.

I've also implemented storing groups in the JSON (though I'm not entirely sure of the format), but I don't know how to implement installing the groups.

An updated TODO list:

Finish group support
Finalize where the replay command will go, right now it's kind of slapped onto the history command and it's not pretty
Store and use solver flags and possibly other configuration affecting resolving a transaction
Possibly add switches to control the strictness (the checks are unconditional now)
Add tests for various corner cases mainly for when the transaction end up being different on the target system

dmach · 2020-06-08T07:54:08Z

The overall approach looks good to me, just couple notes about the store command and the stored json file:

I'd add "document": "dnf-transaction" to identify the file if a user finds it on disk, not knowing what it is. Considering this and version field being mixed with data (there's no data key), maybe I'd rename them to document-type and document-version.
Rename "packages" to "rpms"; it's more explicit and will eventually allow us to support more/different package types in the future.
Rename "envs" to "environments".
Consider leaving out the rpms/groups/environments sections if empty.
Consider sorting the keys in the dictionary by using sort_keys=1 or use OrderedDict to preserve the key order (does it even work with this way?).
Display absolute path to the created file in the "Transaction saved to ..." message.

lgtm-com · 2020-06-08T12:23:15Z

This pull request introduces 2 alerts and fixes 1 when merging 0caba25 into f283605 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lukash · 2020-06-08T14:07:14Z

@dmach I've addressed most of your points except the "document" item, I'm (still) not convinced it's not just a random field nobody will read...

I've also attempted to make the replay subcommand fit better into the history, it's a bit awkward with the arg parser (replay takes a positional argument completely different from the rest of the command), but hopefully it's ok like this.

lgtm-com · 2020-06-08T14:18:14Z

This pull request introduces 2 alerts and fixes 1 when merging 85dd536 into f283605 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

j-mracek · 2020-06-08T18:51:47Z

I am sorry but I am unable to build PR.

1: Test command: /usr/bin/python3 "-m" "nose" "-s" "/tmp/tito/rpmbuild-dnfxfiu9jh7/BUILD/dnf-git-9180.f6a4130/tests"
1: Test timeout computed to be: 10000000
1: ......................................................................................................................................................EEE.............................................................................................................S.........................................................rpm transaction happens.
1: rpm transaction happens.
1: .........................................................................................................................................................
1: ======================================================================
1: ERROR: Test history _convert_tids().
1: ----------------------------------------------------------------------
1: Traceback (most recent call last):
1:   File "/usr/lib64/python3.7/unittest/mock.py", line 1259, in patched
1:     return func(*args, **keywargs)
1:   File "/tmp/tito/rpmbuild-dnfxfiu9jh7/BUILD/dnf-git-9180.f6a4130/tests/test_commands.py", line 79, in test_history_convert_tids
1:     cmd = dnf.cli.commands.HistoryCommand(self.cli)
1: AttributeError: module 'dnf.cli.commands' has no attribute 'HistoryCommand'
1: -------------------- >> begin captured logging << --------------------
1: dnf: DEBUG: User-Agent: constructed: 'libdnf (Fedora 31; workstation; Linux.x86_64)'
1: dnf: DDEBUG: Cleaning up.
1: --------------------- >> end captured logging << ---------------------
1: 
1: ======================================================================
1: ERROR: Test get_error_output with the history rollback and a TransactionCheckError.
1: ----------------------------------------------------------------------
1: Traceback (most recent call last):
1:   File "/usr/lib64/python3.7/unittest/mock.py", line 1259, in patched
1:     return func(*args, **keywargs)
1:   File "/tmp/tito/rpmbuild-dnfxfiu9jh7/BUILD/dnf-git-9180.f6a4130/tests/test_commands.py", line 53, in test_history_get_error_output_rollback_transactioncheckerror
1:     cmd = dnf.cli.commands.HistoryCommand(self.cli)
1: AttributeError: module 'dnf.cli.commands' has no attribute 'HistoryCommand'
1: -------------------- >> begin captured logging << --------------------
1: dnf: DEBUG: User-Agent: constructed: 'libdnf (Fedora 31; workstation; Linux.x86_64)'
1: dnf: DDEBUG: Cleaning up.
1: --------------------- >> end captured logging << ---------------------
1: 
1: ======================================================================
1: ERROR: Test get_error_output with the history undo and a TransactionCheckError.
1: ----------------------------------------------------------------------
1: Traceback (most recent call last):
1:   File "/usr/lib64/python3.7/unittest/mock.py", line 1259, in patched
1:     return func(*args, **keywargs)
1:   File "/tmp/tito/rpmbuild-dnfxfiu9jh7/BUILD/dnf-git-9180.f6a4130/tests/test_commands.py", line 66, in test_history_get_error_output_undo_transactioncheckerror
1:     cmd = dnf.cli.commands.HistoryCommand(self.cli)
1: AttributeError: module 'dnf.cli.commands' has no attribute 'HistoryCommand'
1: -------------------- >> begin captured logging << --------------------
1: dnf: DEBUG: User-Agent: constructed: 'libdnf (Fedora 31; workstation; Linux.x86_64)'
1: dnf: DDEBUG: Cleaning up.
1: --------------------- >> end captured logging << ---------------------
1: 
1: ----------------------------------------------------------------------
1: Ran 473 tests in 2.339s
1: 
1: FAILED (SKIP=1, errors=3)
1/1 Test #1: test .............................***Failed    3.19 sec

dnf/transaction_sr.py

dnf/cli/commands/history.py

lukash · 2020-06-09T08:52:47Z

I am sorry but I am unable to build PR.

Sorry about that, I've fixed it.

Thanks for the review!

lgtm-com · 2020-06-09T08:59:00Z

This pull request introduces 2 alerts and fixes 1 when merging deb9e64 into f283605 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-06-09T13:10:44Z

This pull request introduces 2 alerts and fixes 1 when merging e5f5de2 into f283605 - view on LGTM.com

new alerts:

1 for Unused local variable
1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-06-18T16:01:35Z

This pull request introduces 1 alert and fixes 3 when merging 9519898 into f283605 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

2 for Variable defined multiple times
1 for Module is imported with 'import' and 'import from'

lgtm-com · 2020-06-22T16:13:50Z

This pull request introduces 1 alert and fixes 3 when merging 87b1557 into f283605 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

2 for Variable defined multiple times
1 for Module is imported with 'import' and 'import from'

lukash · 2020-06-23T09:07:00Z

I've pushed a final version for this PR. I've left the accumulation of errors throughout the file (as opposed to printing the first error only) in a separate commit, because I'm not entirely happy with it, so I wanted to single it out for the review. I'll squash it with the other commit if there are no issues with it during the review.

List of things remaining to be implemented at a later stage:

Switch(es) for strictness levels when replaying
Support for merging transactions
Module support

lgtm-com · 2020-08-21T15:52:48Z

This pull request introduces 1 alert and fixes 1 when merging 29d2147 into 829fa0a - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

dnf/transaction_sr.py

lgtm-com · 2020-08-31T09:37:33Z

This pull request introduces 1 alert and fixes 1 when merging 223e4eb into 5d0e0d3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

dnf/transaction_sr.py

doc/transaction_json.rst

lgtm-com · 2020-09-01T14:08:19Z

This pull request introduces 1 alert and fixes 1 when merging a623aeb into 5d0e0d3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

doc/transaction_json.rst

j-mracek · 2020-09-01T14:36:11Z

I am really sorry, but I am unable to build it:

1: Test command: /usr/bin/python3 "-m" "nose" "-s" "/tmp/tito/rpmbuild-dnfn3cq9lcq/BUILD/dnf-git-9222.5c61e6a/tests"
1: Test timeout computed to be: 10000000
1: .............E........................................................................................................................................................................................................................................................................................................................................................S.........................................................rpm transaction happens.
1: rpm transaction happens.
1: .........................................................................................................................................................
1: ======================================================================
1: ERROR: test_environment_install (tests.api.test_dnf_base.DnfBaseApiTest)
1: ----------------------------------------------------------------------
1: Traceback (most recent call last):
1:   File "/tmp/tito/rpmbuild-dnfn3cq9lcq/BUILD/dnf-git-9222.5c61e6a/tests/api/test_dnf_base.py", line 188, in test_environment_install
1:     exclude_groups=None
1: TypeError: environment_install() got an unexpected keyword argument 'types'
1: -------------------- >> begin captured logging << --------------------
1: dnf: DEBUG: User-Agent: constructed: 'libdnf (Fedora 31; workstation; Linux.x86_64)'
1: dnf: DDEBUG: Getting group metadata
1: dnf: DDEBUG: timer: loading comps: 0 ms
1: dnf: DDEBUG: Cleaning up.
1: --------------------- >> end captured logging << ---------------------
1: 
1: ----------------------------------------------------------------------
1: Ran 569 tests in 1.992s
1: 
1: FAILED (SKIP=1, errors=1)
1/1 Test #1: test .............................***Failed    2.64 sec

lgtm-com · 2020-09-01T15:23:34Z

This pull request introduces 1 alert and fixes 1 when merging cca293d into 5d0e0d3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

lukash · 2020-09-01T15:35:15Z

I am really sorry, but I am unable to build it:

It's because of this commit: a6d41db

Where I change the name of the argument to unify it with the group_install function. It is not a keyword argument per se, I'm surprised it's used that way. Do you think I can change its name or do we consider that API as well and should I keep it the way it is for compatibility?

Plus some imports cleanup.

Call it in run() and store the results locally. Store the configuration it uses in the class instance and unify naming of some variables.

lgtm-com · 2020-09-02T08:37:44Z

This pull request introduces 1 alert and fixes 1 when merging 2b5b130 into 5d0e0d3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

j-mracek · 2020-09-02T12:48:51Z

of the argument to unify it with the group_install fu

In this case I would recommend to keep the original name of the argument. The benefit of the change is minimal, but consequences could be huge. Who knows?

…stall Converts the pkg_types only if they're not already integers. Adds documentation for the argument.

Raising ValueError in _group_install() was a mistake, the error that is expected is CompsError. It will also get caught in install_or_skip(), which is an unfortunately unreliable interface, but one that we're stuck with for backwards compatibility. Raises a CompsError in the same situation in _environment_install() as well.

Proxies the underlying RPMItem getNEVRA().

The enum values for PackageType are different between libcomps and libdnf, translate the value when storing the libcomps data into a Swdb transaction.

lukash · 2020-09-02T13:13:22Z

In this case I would recommend to keep the original name of the argument. The benefit of the change is minimal, but consequences could be huge. Who knows?

Fixed. Builds for me now.

lgtm-com · 2020-09-02T13:19:08Z

This pull request introduces 1 alert and fixes 1 when merging 99b1f20 into 5d0e0d3 - view on LGTM.com

new alerts:

1 for Module is imported with 'import' and 'import from'

fixed alerts:

1 for Module is imported with 'import' and 'import from'

j-mracek · 2020-09-02T15:05:35Z

I see that two tests are failing:

features/history-error.feature
Scenario: read permission error on the history database
features/transaction-sr/replay.feature
/bin/sh: sqlite3: command not found

@lukash Please could you fix it?

lukash · 2020-09-03T16:07:33Z

I see that two tests are failing:

features/history-error.feature
Scenario: read permission error on the history database
features/transaction-sr/replay.feature
/bin/sh: sqlite3: command not found

@lukash Please could you fix it?

Fixed. The tests now depend on sqlite for checking the content of the history DB. And due to moving the first access to the history DB from configure() to run(), when there's an error accessing the history DB, it is printed out twice. I forgot the exact reason for that, but I remember we concluded it is unavoidable (without large changes). So I've changed the tests to match the duplicate error output.

j-mracek · 2020-09-04T11:27:57Z

All tests passed, thank you very much for a contribution.

lukash added the work in progress label Jun 4, 2020

j-mracek self-assigned this Jun 8, 2020