[RHELC-1275, RHELC-1516] Exit with 1 when rollback fails #1153

hosekadam · 2024-03-20T09:01:03Z

There is a need to exit with exit code 1 when rollback fails to provide the ability to better recognize those types of failures for Insights. This situation can mainly happen when user uses analyze command or during the conversion answers "No" and user doesn't want to continue over point of no return. In those situations, rollback is processed and when it fails, the system is in an unknown state.

Expected behavior:
When rollback fails, c2r exits with 1, warning about unknown state and system should be restored from backup is printed. Any summaries of the report aren't printed or saved - because there is any big value of the report when the rollback failed.

How it was solved:
The new property backup_failed of BackupController was added. This property is based on instance variable _rollback_failed which is set when restore method of some restorable fails.

I chose this solution because it keeps the code simple. Other solution with returning some value required a lot of code changed and added complexity to the codebase.

I also added a new function in main.py for printing info after rollback, which we do in two places. The goal was to reduce duplicity in this part of code.

To have captured exceptions during the rollback, I've also removed handling of exceptions in each restorable and let it up to BackupController. By this change I found a bug in the code, when the restorable file was removed from the backup folder when doing the restore for conversion purposes. The file should stay in the backup folder, if we would remove it, then we won't have anything when processing the rollback.

Jira Issues: RHELC-1275, RHELC-1516

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

codecov · 2024-03-20T09:02:05Z

Codecov Report

Attention: Patch coverage is 90.69767% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 95.42%. Comparing base (39adc46) to head (b19d720).

Files	Patch %	Lines
convert2rhel/main.py	78.94%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1153      +/-   ##
==========================================
- Coverage   95.47%   95.42%   -0.05%     
==========================================
  Files          54       54              
  Lines        4682     4701      +19     
  Branches      824      830       +6     
==========================================
+ Hits         4470     4486      +16     
- Misses        131      132       +1     
- Partials       81       83       +2

Flag	Coverage Δ
centos-linux-7	`90.63% <90.24%> (-0.04%)`	⬇️
centos-linux-8	`91.55% <90.24%> (-0.04%)`	⬇️
centos-linux-9	`91.61% <88.37%> (-0.04%)`	⬇️

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/backup/__init__.py

convert2rhel/main.py

convert2rhel/backup/__init__.py

convert2rhel/main.py

convert2rhel/backup/__init__.py

convert2rhel/backup/files.py

convert2rhel/main.py

hosekadam · 2024-04-09T14:00:05Z

What was done:

renamed the print_info_after_rollback -> provide_status_after_rollback
applied the suggestions (use the any() and improve comment)
removed exception handling for RestorablePEMCert and RestorableFile (will be handled in BackupController)
fixed unit tests (mainly removing the tests for exception handling)

convert2rhel/unit_tests/backup/files_test.py

convert2rhel/backup/certs.py

Venefilyn

Seems good, minor things. Will you add integration tests?

convert2rhel/backup/certs.py

convert2rhel/backup/files.py

convert2rhel/main.py

hosekadam · 2024-04-12T14:12:57Z

What was done:

added name of arg to calls of provide_status_after_rollback and fixed comment in it
changed the re-raising of the exception in RestorableCert
added the exceptions into the doc strings
added unit test for the exception raised in RestorableCert.restore()
added exception handling of system_release file for restoring in regster_system(). No unit tests for this, waiting for approval if it's fine.

convert2rhel/subscription.py

convert2rhel/backup/__init__.py

convert2rhel/backup/files.py

convert2rhel/subscription.py

convert2rhel/unit_tests/main_test.py

hosekadam · 2024-04-22T16:15:59Z

added the unit test for handling exception in register_system()
added condition to restore the os-release file only in the first attempt. In the next attempts the file is already restored
removed check if the backup is present and let the possible exception to be raised. Without it, it won’t be handled by the BackupController/something else, and we won’t know what happened. It will be just captured in the log somewhere during the conversion
added unit tests for restoring only in the first attempt of registering the system
removed one case of unit test when the file for restoring wasn't present. Now this raises exception
fixed the comments based on review

(I'll take look to the unit tests later)

hosekadam · 2024-04-29T11:38:17Z

Solved and rebased :)

EDIT: I see new conflict :(

convert2rhel/backup/files.py

convert2rhel/main.py

hosekadam · 2024-04-30T09:42:23Z

rebased, solved conflicts
fixed the " symbol in the one of the comments
removed elif and deindented report.pre_conversion_report in provide_status_after_rollback

r0x0d · 2024-04-30T18:36:30Z

convert2rhel/backup/certs.py

+            .. warning::
+                Exceptions are not handled and left for handling by the calling code.


Suggested change

.. warning::

Exceptions are not handled and left for handling by the calling code.

.. warning::

Exceptions are not handled and left for handling by the calling code.

Deindent the waring section

r0x0d · 2024-04-30T18:37:02Z

convert2rhel/backup/files.py

+            .. warning::
+                Exceptions are not handled and left for handling by the calling code.


Suggested change

.. warning::

Exceptions are not handled and left for handling by the calling code.

.. warning::

Exceptions are not handled and left for handling by the calling code.

r0x0d · 2024-04-30T18:50:57Z

convert2rhel/subscription.py

+            raise exceptions.CriticalError(
+                id_="FAILED_TO_SUBSCRIBE_SYSTEM",
+                title="Failed to subscribe system.",
+                description="Failed to restore the /etc/os-release file needed for subscribing the system.",
+                diagnosis="The restore failed with error %s." % (str(e)),
+            )


Maybe we should use a different id and tittle? We are not subscribing the system at this point, only trying to restore the os_release file.

The registration comes in the line 261

I mean, the procedure is for registering the system, but at this specific step we are not actually registering

I was thinking about that, and it resulted in this. I see it that we are trying to subscribe the system and this is part of the process. The problem is then specified in the description and diagnostic.

But if we would find a better name, I will change that, no problem. I'm not sure which, I felt like FAILED_TO_RESTORE_OSRELEASE is a bit misleading.... But I'm not sure there, the naming is the hardest part sometimes 😄

This is a big question and I feel like we need a wiki page or something with all of these things, when to use what, and how each is triggered etc. At the moment it's just "this feels good" but it can lead to confusing things for the customer

Since this prevents the system from being registering it still fails to register, so I feel like this error is still accurate

r0x0d

Looks good! Only one question though.

Venefilyn · 2024-05-03T14:39:49Z

Rebase and fix the indentation on warnings, run tier0 (can do so by applying the labels after builds are done, and if tests are good we can merge

has-bot · 2024-05-08T16:52:07Z

/packit test --labels tier0

Comment generated by an automation.

Log | Bot Usage

Venefilyn · 2024-05-13T16:33:46Z

/packit test --labels tier0

* Fixed unit tests * Adjusted the return code with the new ConversionExitCodes

hosekadam · 2024-05-14T10:07:50Z

Fixed the indentation and rebased

hosekadam · 2024-05-14T10:09:43Z

@danmyway Do we need new integration tests for this or do we have it covered?

danmyway · 2024-05-14T10:40:20Z

Not that critical from my POV, we can add it in a separate batch @hosekadam
I will just run the tier0, if it passes (apart from the offline conversion test on Alma 8.9) we can merge this

danmyway · 2024-05-14T10:40:28Z

/packit test --labels tier0

Venefilyn · 2024-05-15T09:58:50Z

LGTM. @hosekadam please update the Jira issues to Dev Complete when you can!

hosekadam force-pushed the exit-with-1-rollback-failure branch 2 times, most recently from 5390b88 to 5fde95b Compare March 20, 2024 12:11

r0x0d reviewed Mar 20, 2024

View reviewed changes

convert2rhel/backup/__init__.py Outdated Show resolved Hide resolved

hosekadam force-pushed the exit-with-1-rollback-failure branch from 5fde95b to 87833f8 Compare March 20, 2024 12:21

hosekadam changed the title ~~Exit with 1 when rollback fails~~ [RHELC-1275] Exit with 1 when rollback fails Mar 20, 2024

hosekadam marked this pull request as ready for review March 20, 2024 14:06

hosekadam requested a review from Venefilyn March 20, 2024 14:31

hosekadam mentioned this pull request Mar 21, 2024

[RHELC-1342] Add post conversion stages for Action Framework #1144

Merged

8 tasks

hosekadam force-pushed the exit-with-1-rollback-failure branch from 87833f8 to 322e38b Compare March 22, 2024 09:58

r0x0d reviewed Mar 22, 2024

View reviewed changes

convert2rhel/backup/__init__.py Outdated Show resolved Hide resolved

r0x0d reviewed Mar 22, 2024

View reviewed changes

convert2rhel/backup/__init__.py Show resolved Hide resolved

r0x0d reviewed Mar 22, 2024

View reviewed changes

convert2rhel/main.py Outdated Show resolved Hide resolved

r0x0d reviewed Mar 22, 2024

View reviewed changes

convert2rhel/main.py Outdated Show resolved Hide resolved

hosekadam force-pushed the exit-with-1-rollback-failure branch 2 times, most recently from 40d3e35 to a5c2880 Compare March 26, 2024 16:40

Venefilyn requested changes Apr 4, 2024

View reviewed changes

Venefilyn reviewed Apr 8, 2024

View reviewed changes

convert2rhel/main.py Outdated Show resolved Hide resolved

hosekadam force-pushed the exit-with-1-rollback-failure branch from a5c2880 to d34251c Compare April 9, 2024 13:59

hosekadam force-pushed the exit-with-1-rollback-failure branch from d34251c to a6c7f00 Compare April 9, 2024 14:01

github-advanced-security bot found potential problems Apr 9, 2024

View reviewed changes

convert2rhel/unit_tests/backup/files_test.py Fixed Show resolved Hide resolved

hosekadam force-pushed the exit-with-1-rollback-failure branch from a6c7f00 to 6422efc Compare April 9, 2024 14:07

r0x0d reviewed Apr 9, 2024

View reviewed changes

convert2rhel/backup/certs.py Outdated Show resolved Hide resolved

Venefilyn reviewed Apr 9, 2024

View reviewed changes

hosekadam force-pushed the exit-with-1-rollback-failure branch from 6422efc to 6b150ff Compare April 12, 2024 14:12

Venefilyn reviewed Apr 15, 2024

View reviewed changes

hosekadam force-pushed the exit-with-1-rollback-failure branch from 6b150ff to e8c261e Compare April 22, 2024 16:15

r0x0d reviewed Apr 29, 2024

View reviewed changes

convert2rhel/backup/files.py Outdated Show resolved Hide resolved

hosekadam force-pushed the exit-with-1-rollback-failure branch from b2e74c6 to 131fbf4 Compare April 29, 2024 14:03

r0x0d reviewed Apr 29, 2024

View reviewed changes

convert2rhel/main.py Outdated Show resolved Hide resolved

hosekadam force-pushed the exit-with-1-rollback-failure branch from 131fbf4 to f0a820e Compare April 30, 2024 09:40

r0x0d reviewed Apr 30, 2024

View reviewed changes

r0x0d force-pushed the exit-with-1-rollback-failure branch from f0a820e to e5a1bb0 Compare May 8, 2024 16:51

r0x0d added kind/feature New feature or request tests/tier0 PR ready to run the essential test suit. Equivalent to `/packit test --labels tier0`. labels May 8, 2024

r0x0d force-pushed the exit-with-1-rollback-failure branch from e5a1bb0 to e78c99a Compare May 8, 2024 17:06

Venefilyn force-pushed the exit-with-1-rollback-failure branch from e78c99a to 8f84e14 Compare May 13, 2024 15:39

hosekadam and others added 3 commits May 14, 2024 12:06

exit with 1 if failed

64d04d5

Adjust return code with ConversionExitCodes

af84be9

* Fixed unit tests * Adjusted the return code with the new ConversionExitCodes

Fix indentation in comment

b19d720

hosekadam force-pushed the exit-with-1-rollback-failure branch from e46e929 to b19d720 Compare May 14, 2024 10:07

Venefilyn changed the title ~~[RHELC-1275], [RHELC-1516] Exit with 1 when rollback fails~~ [RHELC-1275, RHELC-1516] Exit with 1 when rollback fails May 14, 2024

Venefilyn merged commit 1c8dc0e into oamg:main May 15, 2024
32 of 35 checks passed

hosekadam mentioned this pull request May 27, 2024

Bump version to 2.0.0 #1243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RHELC-1275, RHELC-1516] Exit with 1 when rollback fails #1153

[RHELC-1275, RHELC-1516] Exit with 1 when rollback fails #1153

hosekadam commented Mar 20, 2024 •

edited by r0x0d

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

hosekadam commented Apr 9, 2024

Venefilyn left a comment

hosekadam commented Apr 12, 2024 •

edited

Loading

hosekadam commented Apr 22, 2024 •

edited

Loading

hosekadam commented Apr 29, 2024 •

edited

Loading

hosekadam commented Apr 30, 2024

r0x0d Apr 30, 2024

r0x0d Apr 30, 2024

r0x0d Apr 30, 2024

r0x0d Apr 30, 2024

r0x0d Apr 30, 2024

hosekadam Apr 30, 2024

Venefilyn May 3, 2024

r0x0d left a comment

Venefilyn commented May 3, 2024

has-bot commented May 8, 2024

Venefilyn commented May 13, 2024

hosekadam commented May 14, 2024

hosekadam commented May 14, 2024

danmyway commented May 14, 2024

danmyway commented May 14, 2024

Venefilyn commented May 15, 2024

		.. warning::
		Exceptions are not handled and left for handling by the calling code.

[RHELC-1275, RHELC-1516] Exit with 1 when rollback fails #1153

[RHELC-1275, RHELC-1516] Exit with 1 when rollback fails #1153

Conversation

hosekadam commented Mar 20, 2024 • edited by r0x0d Loading

codecov bot commented Mar 20, 2024 • edited Loading

Codecov Report

hosekadam commented Apr 9, 2024

Venefilyn left a comment

Choose a reason for hiding this comment

hosekadam commented Apr 12, 2024 • edited Loading

hosekadam commented Apr 22, 2024 • edited Loading

hosekadam commented Apr 29, 2024 • edited Loading

hosekadam commented Apr 30, 2024

r0x0d Apr 30, 2024

Choose a reason for hiding this comment

r0x0d Apr 30, 2024

Choose a reason for hiding this comment

r0x0d Apr 30, 2024

Choose a reason for hiding this comment

r0x0d Apr 30, 2024

Choose a reason for hiding this comment

r0x0d Apr 30, 2024

Choose a reason for hiding this comment

hosekadam Apr 30, 2024

Choose a reason for hiding this comment

Venefilyn May 3, 2024

Choose a reason for hiding this comment

r0x0d left a comment

Choose a reason for hiding this comment

Venefilyn commented May 3, 2024

has-bot commented May 8, 2024

Venefilyn commented May 13, 2024

hosekadam commented May 14, 2024

hosekadam commented May 14, 2024

danmyway commented May 14, 2024

danmyway commented May 14, 2024

Venefilyn commented May 15, 2024

hosekadam commented Mar 20, 2024 •

edited by r0x0d

Loading

codecov bot commented Mar 20, 2024 •

edited

Loading

hosekadam commented Apr 12, 2024 •

edited

Loading

hosekadam commented Apr 22, 2024 •

edited

Loading

hosekadam commented Apr 29, 2024 •

edited

Loading