From d34251c02bbb10a0c85cca571485c5f15656cbb0 Mon Sep 17 00:00:00 2001 From: Adam Hosek Date: Mon, 18 Mar 2024 22:32:31 +0100 Subject: [PATCH] exit with 1 if failed --- convert2rhel/backup/__init__.py | 31 ++++++- convert2rhel/backup/certs.py | 3 +- convert2rhel/backup/files.py | 25 ++---- convert2rhel/main.py | 48 ++++++++--- convert2rhel/unit_tests/__init__.py | 4 + convert2rhel/unit_tests/backup/backup_test.py | 14 +++ convert2rhel/unit_tests/backup/certs_test.py | 4 - convert2rhel/unit_tests/backup/files_test.py | 33 +------ convert2rhel/unit_tests/main_test.py | 85 +++++++++++++++++-- 9 files changed, 169 insertions(+), 78 deletions(-) diff --git a/convert2rhel/backup/__init__.py b/convert2rhel/backup/__init__.py index a971b89d1b..a0f2baecf5 100644 --- a/convert2rhel/backup/__init__.py +++ b/convert2rhel/backup/__init__.py @@ -48,6 +48,7 @@ class BackupController: def __init__(self): self._restorables = [] + self._rollback_failures = [] def push(self, restorable): """ @@ -86,18 +87,18 @@ def pop_all(self): """ Restores all RestorableChanges known to the Controller and then returns them. - :returns: List of RestorableChange objects that were known to the Controller. + :returns list[RestorableChange]: List of RestorableChange objects that were processed by the Controller. :raises IndexError: If there are no RestorableChanges currently known to the Controller. After running, the Controller object will not know about any RestorableChanges. """ # Only raise IndexError if there are no restorables registered. - # Partitions are ignored for this check as they aren't really Changes. if not self._restorables: raise IndexError("No backups to restore") - # Restore the Changes in the reverse order the changes were enabled. processed_restorables = [] + + # Restore the Changes in the reverse order the changes were enabled. while True: try: restorable = self._restorables.pop() @@ -110,12 +111,34 @@ def pop_all(self): # logger.critical in some places. except (Exception, SystemExit) as e: # Don't let a failure in one restore influence the others - loggerinst.warning("Error while rolling back a %s: %s" % (restorable.__class__.__name__, str(e))) + message = "Error while rolling back a %s: %s" % (restorable.__class__.__name__, str(e)) + loggerinst.warning(message) + # Add the rollback failures to the list + self._rollback_failures.append(message) processed_restorables.append(restorable) return processed_restorables + @property + def rollback_failed(self): + """ + Return True when one or more restorables were unsuccessful. + + :returns: Boolean value if the rollback failed. + """ + + return any(self._rollback_failures) + + @property + def rollback_failures(self): + """ + Errors from failed rollback. Used to provide more information to the user. + + :returns: List of strings containing errors captured during rollback. + """ + return self._rollback_failures + @six.add_metaclass(abc.ABCMeta) class RestorableChange: diff --git a/convert2rhel/backup/certs.py b/convert2rhel/backup/certs.py index cad236ce29..0c20312b90 100644 --- a/convert2rhel/backup/certs.py +++ b/convert2rhel/backup/certs.py @@ -187,7 +187,8 @@ def _restore(self): # Resolves RHSM error when removing certs, as the system might not have installed any certs yet loggerinst.info("No certificates found to be removed.") else: - loggerinst.error("OSError({0}): {1}".format(err.errno, err.strerror)) + # Will be handled in BackupController + raise err def _get_cert(cert_dir): diff --git a/convert2rhel/backup/files.py b/convert2rhel/backup/files.py index fada72ee77..4f41a69010 100644 --- a/convert2rhel/backup/files.py +++ b/convert2rhel/backup/files.py @@ -118,15 +118,9 @@ def restore(self, rollback=True): loggerinst.info("%s hasn't been backed up." % self.filepath) return - try: - shutil.copy2(self._backup_path, self.filepath) - os.remove(self._backup_path) - except (OSError, IOError) as err: - # Do not call 'critical' which would halt the program. We are in - # a rollback phase now and we want to rollback as much as possible. - # IOError for py2 and OSError for py3 - loggerinst.critical_no_exit("Error(%s): %s" % (err.errno, err.strerror)) - return + # Possible exceptions will be handled in the BackupController + shutil.copy2(self._backup_path, self.filepath) + os.remove(self._backup_path) if rollback: loggerinst.info("File %s restored." % self.filepath) @@ -179,13 +173,8 @@ def restore(self): if not os.path.isfile(self.filepath): loggerinst.info("File {filepath} wasn't created during conversion".format(filepath=self.filepath)) else: - try: - os.remove(self.filepath) - loggerinst.info("File {filepath} removed".format(filepath=self.filepath)) - except OSError as err: - # Do not call 'critical' which would halt the program. We are in - # a rollback phase now and we want to rollback as much as possible. - loggerinst.critical_no_exit("Error(%s): %s" % (err.errno, err.strerror)) - return + # Possible exceptions will be handled in the BackupController + os.remove(self.filepath) + loggerinst.info("File {filepath} removed".format(filepath=self.filepath)) - super(MissingFile, self).restore() + super(MissingFile, self).restore() diff --git a/convert2rhel/main.py b/convert2rhel/main.py index 7ff03d6f21..979f02ed64 100644 --- a/convert2rhel/main.py +++ b/convert2rhel/main.py @@ -167,12 +167,11 @@ def main_locked(): subscription.update_rhsm_custom_facts() rollback_changes() + provide_status_after_rollback(pre_conversion_results, True) + + if backup.backup_control.rollback_failed: + return 1 - report.summary( - pre_conversion_results, - include_all_reports=True, - disable_colors=logger_module.should_disable_color_output(), - ) return 0 except exceptions.CriticalError as err: @@ -185,7 +184,7 @@ def main_locked(): finally: # Write the assessment to a file as json data so that other tools can # parse and act upon it. - if pre_conversion_results: + if pre_conversion_results and not backup.backup_control.rollback_failed: actions.report.summary_as_json(pre_conversion_results) actions.report.summary_as_txt(pre_conversion_results) @@ -222,14 +221,7 @@ def _handle_main_exceptions(process_phase, pre_conversion_results=None): subscription.update_rhsm_custom_facts() rollback_changes() - if pre_conversion_results is None: - loggerinst.info("\nConversion interrupted before analysis of system completed. Report not generated.\n") - else: - report.summary( - pre_conversion_results, - include_all_reports=(toolopts.tool_opts.activity == "analysis"), - disable_colors=logger_module.should_disable_color_output(), - ) + provide_status_after_rollback(pre_conversion_results, (toolopts.tool_opts.activity == "analysis")) elif process_phase == ConversionPhase.POST_PONR_CHANGES: # After the process of subscription is done and the mass update of # packages is started convert2rhel will not be able to guarantee a @@ -363,3 +355,31 @@ def rollback_changes(): loggerinst.info("During rollback there were no backups to restore") else: raise + + return + + +def provide_status_after_rollback(pre_conversion_results, include_all_reports): + """Print after-rollback messages. Also determine if there is a report to print or + report should't be printed after failure in rollback.""" + if backup.backup_control.rollback_failed: + loggerinst.critical_no_exit( + "Rollback of system wasn't completed successfully.\n" + "The system is left in an undetermined state that Convert2RHEL cannot fix.\n" + "It is strongly recommended to store the Convert2RHEL logs for later investigation, and restore" + " the system from a backup.\n" + "Following errors were captured during rollback:\n" + "%s" % "\n".join(backup.backup_control.rollback_failures) + ) + + return + + elif not pre_conversion_results: + loggerinst.info("\nConversion interrupted before analysis of system completed. Report not generated.\n") + + else: + report.summary( + results=pre_conversion_results, + include_all_reports=include_all_reports, + disable_colors=logger_module.should_disable_color_output(), + ) diff --git a/convert2rhel/unit_tests/__init__.py b/convert2rhel/unit_tests/__init__.py index 832190881c..e9b7af5cfb 100644 --- a/convert2rhel/unit_tests/__init__.py +++ b/convert2rhel/unit_tests/__init__.py @@ -379,6 +379,10 @@ class RollbackChangesMocked(MockFunctionObject): spec = main.rollback_changes +class PrintInfoAfterRollbackMocked(MockFunctionObject): + spec = main.provide_status_after_rollback + + class ShowEulaMocked(MockFunctionObject): spec = main.show_eula diff --git a/convert2rhel/unit_tests/backup/backup_test.py b/convert2rhel/unit_tests/backup/backup_test.py index 757fa6a9ed..11f5cf21e7 100644 --- a/convert2rhel/unit_tests/backup/backup_test.py +++ b/convert2rhel/unit_tests/backup/backup_test.py @@ -50,6 +50,9 @@ def test_pop_multiple(self, backup_controller): assert restorable2.called["restore"] == 1 assert restorable3.called["restore"] == 1 + assert backup_controller.rollback_failed == False + assert backup_controller.rollback_failures == [] + def test_pop_when_empty(self, backup_controller): with pytest.raises(IndexError, match="No backups to restore"): backup_controller.pop() @@ -74,6 +77,9 @@ def test_pop_all(self, backup_controller): assert restorable2.called["restore"] == 1 assert restorable3.called["restore"] == 1 + assert backup_controller.rollback_failed == False + assert backup_controller.rollback_failures == [] + def test_ready_to_push_after_pop_all(self, backup_controller): restorable1 = MinimalRestorable() restorable2 = MinimalRestorable() @@ -86,11 +92,15 @@ def test_ready_to_push_after_pop_all(self, backup_controller): assert popped_restorables[0] == restorable1 assert len(backup_controller._restorables) == 1 assert backup_controller._restorables[0] is restorable2 + assert backup_controller.rollback_failed == False def test_pop_all_when_empty(self, backup_controller): with pytest.raises(IndexError, match="No backups to restore"): backup_controller.pop_all() + assert backup_controller.rollback_failed == False + assert backup_controller.rollback_failures == [] + def test_pop_all_error_in_restore(self, backup_controller, caplog): restorable1 = MinimalRestorable() restorable2 = ErrorOnRestoreRestorable(exception=ValueError("Restorable2 failed")) @@ -105,3 +115,7 @@ def test_pop_all_error_in_restore(self, backup_controller, caplog): assert len(popped_restorables) == 3 assert popped_restorables == [restorable3, restorable2, restorable1] assert caplog.records[-1].message == "Error while rolling back a ErrorOnRestoreRestorable: Restorable2 failed" + assert backup_controller.rollback_failed == True + assert backup_controller.rollback_failures == [ + "Error while rolling back a ErrorOnRestoreRestorable: Restorable2 failed" + ] diff --git a/convert2rhel/unit_tests/backup/certs_test.py b/convert2rhel/unit_tests/backup/certs_test.py index 719c953b6f..9eed139421 100644 --- a/convert2rhel/unit_tests/backup/certs_test.py +++ b/convert2rhel/unit_tests/backup/certs_test.py @@ -245,10 +245,6 @@ def test_restore_cert_previously_installed(self, caplog, monkeypatch, system_cer OSError(2, "No such file or directory"), "No such file or directory", ), - ( - OSError(13, "[Errno 13] Permission denied: '/tmpdir/certfile'"), - "OSError(13): Permission denied: '/tmpdir/certfile'", - ), ), ) def test_restore_cert_error_conditions( diff --git a/convert2rhel/unit_tests/backup/files_test.py b/convert2rhel/unit_tests/backup/files_test.py index 6166fbb687..16b473f0f7 100644 --- a/convert2rhel/unit_tests/backup/files_test.py +++ b/convert2rhel/unit_tests/backup/files_test.py @@ -8,7 +8,7 @@ import six from convert2rhel import exceptions -from convert2rhel.backup import files +from convert2rhel.backup import RestorableChange, files from convert2rhel.backup.files import MissingFile, RestorableFile @@ -224,24 +224,6 @@ def test_restorable_file_restore(self, tmpdir, monkeypatch, caplog, filename, me assert shutil.copy2.call_count == 1 assert os.remove.call_count == 1 - def test_restorable_file_backup_oserror(self, tmpdir, caplog, monkeypatch): - backup_dir = tmpdir.mkdir("backup") - backup_file = backup_dir.join("filename") - backup_file.write("content") - - monkeypatch.setattr( - shutil, - "copy2", - mock.Mock(side_effect=[None, OSError(2, "No such file or directory")]), - ) - monkeypatch.setattr(os.path, "isfile", lambda file: True) - monkeypatch.setattr(files, "BACKUP_DIR", str(backup_dir)) - file_backup = RestorableFile(str(backup_file)) - file_backup.enable() - file_backup.restore() - - assert "Error(2): No such file or directory" in caplog.records[-1].message - @pytest.mark.parametrize( ("file", "filepath", "message"), ( @@ -340,19 +322,6 @@ def test_created_file_restore(self, tmpdir, exists, enabled, message, caplog): else: assert not caplog.records - def test_created_file_restore_oserror(self, monkeypatch, tmpdir, caplog): - path = tmpdir.join("filename") - path.write("content") - - remove = mock.Mock(side_effect=OSError(2, "No such file or directory")) - monkeypatch.setattr(os, "remove", remove) - - created_file = MissingFile(str(path)) - created_file.enabled = True - created_file.restore() - - assert "Error(2): No such file or directory" in caplog.records[-1].message - @pytest.mark.parametrize( ("exists", "created", "message_push", "message_pop"), ( diff --git a/convert2rhel/unit_tests/main_test.py b/convert2rhel/unit_tests/main_test.py index c41ca5309f..9a940fbefa 100644 --- a/convert2rhel/unit_tests/main_test.py +++ b/convert2rhel/unit_tests/main_test.py @@ -27,7 +27,7 @@ six.add_move(six.MovedModule("mock", "mock", "unittest.mock")) from six.moves import mock -from convert2rhel import actions, applock, checks, exceptions, grub, hostmetering +from convert2rhel import actions, applock, backup, checks, exceptions, grub, hostmetering from convert2rhel import logger as logger_module from convert2rhel import main, pkghandler, pkgmanager, redhatrelease, subscription, toolopts, utils from convert2rhel.actions import report @@ -41,6 +41,7 @@ InitializeFileLoggingMocked, MainLockedMocked, PrintDataCollectionMocked, + PrintInfoAfterRollbackMocked, PrintSystemInformationMocked, RequireRootMocked, ResolveSystemInfoMocked, @@ -57,6 +58,7 @@ def test_rollback_changes(self, monkeypatch, global_backup_control): main.rollback_changes() assert global_backup_control.pop_all.call_args_list == mock.call() + assert backup.backup_control.rollback_failed == False def test_backup_control_unknown_exception(self, monkeypatch, global_backup_control): monkeypatch.setattr( @@ -68,6 +70,76 @@ def test_backup_control_unknown_exception(self, monkeypatch, global_backup_contr with pytest.raises(IndexError, match="Raised because of a bug in the code"): main.rollback_changes() + @pytest.mark.parametrize( + ("pre_conversion_results", "include_all_reports", "rollback_failures", "message", "summary_call_count"), + ( + ( + "anything", + False, + ["rollback_fail_0", "rollback_fail_1", "rollback_fail_2", "rollback_fail_3"], + "Rollback of system wasn't completed successfully.\n" + "The system is left in an undetermined state that Convert2RHEL cannot fix.\n" + "It is strongly recommended to store the Convert2RHEL logs for later investigation, and restore" + " the system from a backup.\n" + "Following errors were captured during rollback:\n" + "rollback_fail_0\n" + "rollback_fail_1\n" + "rollback_fail_2\n" + "rollback_fail_3", + 0, + ), + ( + "anything", + False, + ["rollback_fail_0"], + "Rollback of system wasn't completed successfully.\n" + "The system is left in an undetermined state that Convert2RHEL cannot fix.\n" + "It is strongly recommended to store the Convert2RHEL logs for later investigation, and restore" + " the system from a backup.\n" + "Following errors were captured during rollback:\n" + "rollback_fail_0", + 0, + ), + ("anything", False, [], "", 1), + ( + None, + True, + [], + "\nConversion interrupted before analysis of system completed. Report not generated.\n", + 0, + ), + ), + ) + def test_provide_status_after_rollback( + self, + monkeypatch, + caplog, + pre_conversion_results, + include_all_reports, + message, + global_backup_control, + rollback_failures, + summary_call_count, + ): + report_summary = mock.Mock() + monkeypatch.setattr(global_backup_control, "_rollback_failures", rollback_failures) + monkeypatch.setattr(report, "summary", report_summary) + + main.provide_status_after_rollback(pre_conversion_results, include_all_reports) + + assert report_summary.call_count == summary_call_count + if summary_call_count: + report_summary.assert_called_with( + results=pre_conversion_results, + include_all_reports=include_all_reports, + disable_colors=logger_module.should_disable_color_output(), + ) + try: + assert message == caplog.records[-1].message + except IndexError: + # Nothing printed + assert message == caplog.text + @pytest.mark.parametrize(("exception_type", "exception"), ((IOError, True), (OSError, True), (None, False))) def test_initialize_file_logging(exception_type, exception, monkeypatch, caplog): @@ -249,8 +321,6 @@ def test_main_rollback_post_cli_phase(self, monkeypatch, caplog, tmp_path): initialize_file_logging_mock = mock.Mock() toolopts_cli_mock = mock.Mock() show_eula_mock = mock.Mock(side_effect=Exception) - - # Mock the rollback calls finish_collection_mock = mock.Mock() monkeypatch.setattr(applock, "_DEFAULT_LOCK_DIR", str(tmp_path)) @@ -288,9 +358,11 @@ def test_main_traceback_in_clear_versionlock(self, caplog, monkeypatch, tmp_path ), ) - # And mock rollback items - monkeypatch.setattr(breadcrumbs, "finish_collection", FinishCollectionMocked()) + # Mock rollback items monkeypatch.setattr(main, "rollback_changes", RollbackChangesMocked()) + monkeypatch.setattr(main, "provide_status_after_rollback", PrintInfoAfterRollbackMocked()) + + monkeypatch.setattr(breadcrumbs, "finish_collection", FinishCollectionMocked()) monkeypatch.setattr(report, "summary_as_json", SummaryAsJsonMocked()) assert main.main() == 1 @@ -302,6 +374,9 @@ def test_main_traceback_in_clear_versionlock(self, caplog, monkeypatch, tmp_path assert breadcrumbs.collect_early_data.call_count == 1 assert pkghandler.clear_versionlock.call_count == 1 + assert main.rollback_changes.call_count == 0 + assert main.provide_status_after_rollback.call_count == 0 + assert caplog.records[-2].levelname == "INFO" assert caplog.records[-2].message.strip() == "No changes were made to the system."