[RHELC-855, RHELC-1153] Recover file changes during a rollback #875

hosekadam · 2023-08-04T20:47:39Z

This PR solves problem with not backing up changed package files. When the MD5 checksum of file related to package (got by rpm -Va) differs then is backed up and restored during rollback. If the file is missing, during rollback is deleted.

Jira Issues: RHELC-855, RHELC-1153

Checklist

PR has been tested manually in a VM (either author or reviewer)
Jira issue has been made public if possible
[RHELC-] is part of the PR title
GitHub label has been added to help with Release notes
PR title explains the change from the user's point of view
Code and tests are documented properly
The commits are squashed to as few commits as possible (without losing data)
When merged: Jira issue has been updated to Release Pending if relevant

convert2rhel/backup.py

hosekadam · 2023-08-04T20:50:28Z

Just some high-level review is enough. I'll improve the WIP unit test later. Just if you could check if the structure of the functionality makes sense.

codecov · 2023-09-11T12:22:19Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (45cc026) 94.21% compared to head (18e5aea) 94.17%.
Report is 8 commits behind head on main.

Files	Patch %	Lines
...ert2rhel/actions/pre_ponr_changes/backup_system.py	88.05%	8 Missing ⚠️
convert2rhel/backup.py	94.02%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #875      +/-   ##
==========================================
- Coverage   94.21%   94.17%   -0.05%     
==========================================
  Files          47       47              
  Lines        4377     4516     +139     
  Branches      775      804      +29     
==========================================
+ Hits         4124     4253     +129     
- Misses        177      187      +10     
  Partials       76       76

Flag	Coverage Δ
centos-linux-7	`89.29% <91.48%> (+0.10%)`	⬆️
centos-linux-8	`90.32% <91.48%> (+0.07%)`	⬆️
centos-linux-9	`90.38% <91.54%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

convert2rhel/actions/pre_ponr_changes/backup_system.py

abadger · 2023-09-12T17:51:51Z

convert2rhel/actions/pre_ponr_changes/backup_system.py

+        Return them as a list of dict, for example:
+        [{"status":"S5T", "file_type":"c", "path":"/etc/yum.repos.d/CentOS-Linux-AppStream.repo"}]
+        """
+        cmd = ["rpm", "-Va"]


rpm -Va is very expensive time-wise. We already run rpm_va in systeminfo. I we should do two things:

Use the information from systeminfo so that we don't have to run rpm -Va a second time here.

Perhaps the parsing of rpm -Va output belongs there as well? I am not sure.

We have a command line switch --no-rpm-va which disables the rpm -Va in system_info. I believe it exists because rpm -Va is so slow that we give the user the option to disable it. we need to do one of three things:

We could deprecate that option and always run rpm -Va.

We could change the help documentation to note that --no-rpm-va only disables the rpm -Va which is run after the conversion.

We could make this Action depend on --no-rpm-va not being set.

@bocekm

My opinion about deprecating --no-rpm-va:

I think this will greatly impact in the wait time for the conversion in some use cases where the user has lots and lots of files, modifications to the system etc... Maybe the last suggestion would be more ideal, i.e: "We could make this Action depend on --no-rpm-va not being set.".

+1 to parse the initial output of rpm -Va instead of running it a second time. I don't think the parsing needs to be there, it could be in this action as long as it checks if there is anything to parse first. Seems to me that the only case where we care about parsing the contents of that output is here (correct me if I'm wrong).

@abadger, great points. Indeed we should not run the rpm -Va twice as it is super slow. We shall reuse the output of the first invocation.

I'd vote for keeping the --no-rpm-va switch functionality, i.e. it would prevent running rpm -Va, but because that can lead to an incomplete rollback I'd newly require the user to set the CONVERT2RHEL_INCOMPLETE_ROLLBACK=1 env var (after #683 is merged). What do you think, @r0x0d, @abadger?

The second best option for me would be to "change the help documentation to note that --no-rpm-va only disables the rpm -Va which is run after the conversion".

+1 to that, @bocekm

convert2rhel/main.py

convert2rhel/backup.py

convert2rhel/actions/pre_ponr_changes/backup_system.py

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

convert2rhel/actions/pre_ponr_changes/backup_system.py

hosekadam · 2023-10-20T13:26:25Z

What was changed:

the backup now uses the restorable file from [RHELC-1153] Port the restorable file to work with backup controller #935
added the backup as dependency to some actions. I tried to add it also to package_updates but it caused an error, that some dependencies weren't satisfied. It's caused by having this in different folder (checks vs pre_ponr_changes) - so it's not necessary to have it there as dependency since it's just a check.
if user uses the --no-rpm-va the CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK is required. I used this one with "unsupported" to have it working with the code we have now. It should accordingly to [RHELC-775] Compare the release part of the c2r rpm NEVRA #683 when it will be merged
changed to use the output of rpm -Va from system info (load the content of generated file in systeminfo)
updated the unit tests

I have a question about printing the messages there. Now it works like this:

during standard run is printed TASK - [Prepare: Backup package files] and then messages which files were backed up.
during rollback is printed e.g. this:

[2023-10-20T10:31:34+0000] TASK - [Rollback: Restore /usr/lib/python2.7/site-packages/convert2rhel/utils.pyc from backup] 
File /usr/lib/python2.7/site-packages/convert2rhel/utils.pyc restored.

It corresponds from my point of view with the other rollback tasks and restoring, where restoring every file is a task. E.g.

[2023-10-20T10:31:34+0000] TASK - [Rollback: Restore /etc/system-release from backup] ****************
File /etc/system-release restored.

But there are many lines about that. Is it okay, or should it be changed to something else? I'm not sure about printing just initial TASK and then just list of the files since we are using stack in backupcontroller, and I'm not sure if solution of this problem wouldn't be too complicated.

EDIT:
I see @r0x0d already did a review. So pinging you to not miss this (if it would be helpful for you)

hosekadam · 2023-10-25T13:44:27Z

Applied the suggestions @r0x0d

Venefilyn · 2023-11-23T13:58:45Z

But there are many lines about that. Is it okay, or should it be changed to something else? I'm not sure about printing just initial TASK and then just list of the files since we are using stack in backupcontroller, and I'm not sure if solution of this problem wouldn't be too complicated.

@hosekadam I assume you mean one task with many lines for what is rolled back? I think it might be a bit out of scope for the time being if we already have the one solution done. But the UX would be an improvement, so we can create a separate task for that IMO. Perfecting this isn't necessary as it conveys the same message to the user just in a condensed way

convert2rhel/actions/pre_ponr_changes/backup_system.py

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

convert2rhel/actions/pre_ponr_changes/backup_system.py

convert2rhel/toolopts.py

convert2rhel/backup.py

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

 import pytest
 import six

-from convert2rhel import backup, repo, unit_tests
+from convert2rhel import backup, logger, repo, unit_tests


convert2rhel/unit_tests/backup_test.py

Venefilyn · 2023-11-28T14:49:40Z

convert2rhel/actions/pre_ponr_changes/backup_system.py

+        return {"status": status, "file_type": file_type, "path": path}
+
+    @staticmethod
+    def rollback_files():


Is this tested anywhere?

@SpyTec It's not used anywhere, probably some leftover from previous implementation without the NewRestorableFile. But then it seems to me that I don't handle the missing files - not removing them. I'll take a look if I'm right

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

+            # Unset the variable
+            try:
+                os.environ.pop("CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK")
+            except KeyError:


convert2rhel/unit_tests/toolopts_test.py

+        # Unset the variable
+        try:
+            os.environ.pop("CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK")
+        except KeyError:


convert2rhel/unit_tests/toolopts_test.py

Venefilyn · 2023-12-04T14:19:31Z

convert2rhel/actions/pre_ponr_changes/backup_system.py

+                # If missing conversion is in unknown state
+                loggerinst.critical("Missing file {rpm_va_output} in it's location".format(rpm_va_output=path))
+
+        output = output.strip()


Can be fixed in a different PR

Filed an issue for that, dismissing for now.

convert2rhel/toolopts.py

has-bot · 2023-12-04T13:55:08Z

/packit test --labels tier0

@oamg/conversions-qe please review results and provide ack.

Log | Bot Usage

Venefilyn

Overall LGTM!

Venefilyn · 2023-12-04T14:18:19Z

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py

+            try:
+                os.environ.pop("CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK")
+            except KeyError:
+                pass


Suggested change

try:

os.environ.pop("CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK")

except KeyError:

pass

os.environ.pop("CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK", None)

Reason:

>>> text = {'a':'text'} >>> text.pop('b') Traceback (most recent call last): File "<stdin>", line 1, in <module> KeyError: 'b' >>> text.pop('b', None) >>>

Venefilyn · 2023-12-04T14:19:31Z

convert2rhel/actions/pre_ponr_changes/backup_system.py

+                # If missing conversion is in unknown state
+                loggerinst.critical("Missing file {rpm_va_output} in it's location".format(rpm_va_output=path))
+
+        output = output.strip()


Can be fixed in a different PR

* add integration tests verifying file backup functionality Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

danmyway

Tests run from #1007
Looks good from my POV

stale

* add integration tests verifying file backup functionality Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

- The --no-rpm-va option has no effect with `convert2rhel analysis` anymore (rpm -Va runs always and a warning is printed when the option is used saying that it has no effect) -- Related: oamg/convert2rhel#875 - The CONVERT2RHEL_DISABLE_TELEMETRY env var was removed -- Related: oamg/convert2rhel#1102 - The CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK was removed in favor of CONVERT2RHEL_INCOMPLETE_ROLLBACK -- Related: oamg/convert2rhel#1147 - The CONVERT2RHEL_LATEST_VERSION env var has never been a valid env var recognized by convert2rhel. It is an ID of one of convert2rhel Actions. A similar env var to this is CONVERT2RHEL_UNSUPPORTED_VERSION which is in recent versions (2.x) removed in favor of CONVERT2RHEL_ALLOW_OLDER_VERSION.

- The --no-rpm-va option has no effect with `convert2rhel analysis` anymore (rpm -Va runs always and a warning is printed when the option is used saying that it has no effect) -- Related: oamg/convert2rhel#875 - The CONVERT2RHEL_DISABLE_TELEMETRY env var was removed -- Related: oamg/convert2rhel#1102 - The CONVERT2RHEL_UNSUPPORTED_INCOMPLETE_ROLLBACK was removed in favor of CONVERT2RHEL_INCOMPLETE_ROLLBACK -- Related: oamg/convert2rhel#1147 - The CONVERT2RHEL_LATEST_VERSION env var has never been a valid env var recognized by convert2rhel. It is an ID of one of convert2rhel Actions. A similar env var to this is CONVERT2RHEL_UNSUPPORTED_VERSION which is in recent versions (2.x) removed in favor of CONVERT2RHEL_ALLOW_OLDER_VERSION. Also, the convert_reboot_requested should not be needed when it comes to just analyzing the system (convert2rhel analyze). The --restart convert2rhel option is heeded just when it comes to the actual conversion. And, when the workshop executes and older convert2rhel version (on purpose) then the pre-conversion analysis ends up with two inhibitors: - Outdated convert2rhel version detected - Outdated packages detected This commit adds the related environment variables to override the inhibitors.

hosekadam commented Aug 4, 2023

View reviewed changes

convert2rhel/backup.py Outdated Show resolved Hide resolved

bocekm changed the title ~~[RHELC-855] Improve the rollback~~ [RHELC-855] Recover file changes during a rollback Aug 7, 2023

hosekadam force-pushed the improve_package_backup branch 2 times, most recently from be7deab to 7a31bd6 Compare September 11, 2023 12:20

github-advanced-security bot found potential problems Sep 11, 2023

View reviewed changes

convert2rhel/actions/pre_ponr_changes/backup_system.py Fixed Show fixed Hide fixed

hosekadam requested a review from bocekm September 12, 2023 14:55

abadger reviewed Sep 12, 2023

View reviewed changes

This was referenced Sep 26, 2023

[RHELC-1153] Port the restorable file to work with backup controller #934

Closed

[RHELC-1153] Port the restorable file to work with backup controller #935

Closed

hosekadam force-pushed the improve_package_backup branch 2 times, most recently from 0c458fd to eb0b997 Compare October 20, 2023 11:15

github-advanced-security bot found potential problems Oct 20, 2023

View reviewed changes

convert2rhel/actions/pre_ponr_changes/backup_system.py Fixed Show resolved Hide resolved

convert2rhel/unit_tests/actions/pre_ponr_changes/backup_system_test.py Fixed Show resolved Hide resolved

r0x0d reviewed Oct 20, 2023

View reviewed changes

convert2rhel/actions/pre_ponr_changes/backup_system.py Outdated Show resolved Hide resolved

r0x0d reviewed Oct 20, 2023

View reviewed changes

convert2rhel/actions/pre_ponr_changes/backup_system.py Outdated Show resolved Hide resolved

r0x0d reviewed Oct 20, 2023

View reviewed changes

convert2rhel/actions/pre_ponr_changes/backup_system.py Outdated Show resolved Hide resolved

hosekadam force-pushed the improve_package_backup branch from eb0b997 to 16187cc Compare October 25, 2023 13:41

Venefilyn reviewed Nov 23, 2023

View reviewed changes

convert2rhel/toolopts.py Outdated Show resolved Hide resolved

Venefilyn reviewed Nov 23, 2023

View reviewed changes

convert2rhel/backup.py Outdated Show resolved Hide resolved

Venefilyn reviewed Nov 23, 2023

View reviewed changes

convert2rhel/backup.py Show resolved Hide resolved

hosekadam force-pushed the improve_package_backup branch 2 times, most recently from e843959 to c0d1e94 Compare November 24, 2023 12:49

github-advanced-security bot found potential problems Nov 24, 2023

View reviewed changes

hosekadam force-pushed the improve_package_backup branch from c0d1e94 to ab7a01f Compare November 28, 2023 14:02

Venefilyn reviewed Nov 28, 2023

View reviewed changes

hosekadam force-pushed the improve_package_backup branch from ab7a01f to 75a8324 Compare December 1, 2023 11:54

github-advanced-security bot found potential problems Dec 1, 2023

View reviewed changes

danmyway requested changes Dec 4, 2023

View reviewed changes

convert2rhel/toolopts.py Outdated Show resolved Hide resolved

hosekadam force-pushed the improve_package_backup branch from 75a8324 to a6773d4 Compare December 4, 2023 13:42

Venefilyn added need-integration-tests kind/bug-fix A bug has been fixed tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. and removed need-integration-tests labels Dec 4, 2023

Venefilyn approved these changes Dec 4, 2023

View reviewed changes

hosekadam force-pushed the improve_package_backup branch from a6773d4 to 7032695 Compare December 4, 2023 20:43

hosekadam added 2 commits December 5, 2023 15:57

Port of restorable file to work with backupcontroller

16c6c0b

Backup the package files

18e5aea

hosekadam force-pushed the improve_package_backup branch from 7032695 to 18e5aea Compare December 5, 2023 14:58

jochapma approved these changes Dec 6, 2023

View reviewed changes

danmyway added a commit to danmyway/convert2rhel that referenced this pull request Dec 8, 2023

Integration tests for oamg#875

f49ac2e

* add integration tests verifying file backup functionality Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

danmyway mentioned this pull request Dec 8, 2023

[RHELC-855, RHELC-1277] Tests/improve package backup #1007

Merged

8 tasks

danmyway approved these changes Dec 8, 2023

View reviewed changes

danmyway merged commit 93fcc57 into oamg:main Dec 8, 2023
14 of 48 checks passed

danmyway added a commit to danmyway/convert2rhel that referenced this pull request Dec 8, 2023

Integration tests for oamg#875

cdee1f2

* add integration tests verifying file backup functionality Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

danmyway added a commit that referenced this pull request Dec 8, 2023

Integration tests for #875 (#1007)

f1dbe9f

* add integration tests verifying file backup functionality Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

pr-watson mentioned this pull request Dec 8, 2023

Bump to version 1.6.0 #1008

Merged

bocekm changed the title ~~[RHELC-855] Recover file changes during a rollback~~ [RHELC-855, RHELC-1153] Recover file changes during a rollback Mar 27, 2024

bocekm mentioned this pull request Feb 7, 2025

C2R: Remove env vars/opts with no effect redhat-partner-tech/automated-satellite#23

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-855, RHELC-1153] Recover file changes during a rollback #875

[RHELC-855, RHELC-1153] Recover file changes during a rollback #875

hosekadam commented Aug 4, 2023 •

edited by bocekm

Loading

hosekadam commented Aug 4, 2023

codecov bot commented Sep 11, 2023 •

edited

Loading

abadger Sep 12, 2023

r0x0d Sep 21, 2023

bocekm Sep 25, 2023

r0x0d Sep 28, 2023

hosekadam commented Oct 20, 2023 •

edited

Loading

hosekadam commented Oct 25, 2023

Venefilyn commented Nov 23, 2023 •

edited

Loading

Venefilyn Nov 28, 2023

hosekadam Dec 1, 2023

Venefilyn Dec 4, 2023

danmyway Dec 8, 2023

has-bot commented Dec 4, 2023

Venefilyn left a comment

Venefilyn Dec 4, 2023

Venefilyn Dec 4, 2023

danmyway left a comment

[RHELC-855, RHELC-1153] Recover file changes during a rollback #875

[RHELC-855, RHELC-1153] Recover file changes during a rollback #875

Conversation

hosekadam commented Aug 4, 2023 • edited by bocekm Loading

hosekadam commented Aug 4, 2023

codecov bot commented Sep 11, 2023 • edited Loading

Codecov Report

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

hosekadam commented Oct 20, 2023 • edited Loading

hosekadam commented Oct 25, 2023

Venefilyn commented Nov 23, 2023 • edited Loading

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

has-bot commented Dec 4, 2023

Venefilyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmyway left a comment

Choose a reason for hiding this comment

hosekadam commented Aug 4, 2023 •

edited by bocekm

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

hosekadam commented Oct 20, 2023 •

edited

Loading

Venefilyn commented Nov 23, 2023 •

edited

Loading