Change the way Restores work on error. (oamg#912)

* Previously, if a RestorableChange managed by BackupController raised an exception on restore, then
  we would stop rollback.  This change makes it so we try to restore every change even if previous
  ones have failed.
* Add the name of the RestorableChange which failed. We can retrieve the class name from the Change
  instance to display in the error message.  This isn't perfect but it identifies where the error
  occurred a little better than just the exception message.
  Thanks to Rodolfo for suggesting this.

Co-authored-by: Preston Watson <[email protected]>

convert2rhel/backup.py

@@ -198,33 +198,57 @@ def pop(self):
 
         return restorable
 
-    def pop_all(self):
+    def pop_all(self, _honor_partitions=False):
         """
         Restores all RestorableChanges known to the Controller and then returns them.
 
         :returns: List of RestorableChange objects that were known to 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.
-        """
-        # This code ignores partitions.  Only restore_to_partition() honors
-        # them.
-        restorables = [r for r in self._restorables if r is not self.partition]
 
-        if not restorables:
+        .. note:: _honor_partitions is part of a hack for 1.4 to let us split restoring changes into
+            two parts.  During rollback, the part before the partition is restored before the legacy
+            backups.  Then the remainder of the changes managed by BackupController are restored.
+            This can go away once we merge all of the legacy backups into RestorableChanges managed
+            by BackupController.
+
+            .. seealso:: Jira ticket to track porting to BackupController: https://issues.redhat.com/browse/RHELC-1153
+        """
+        # 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 or all(r == self.partition for r in self._restorables):
             raise IndexError("No backups to restore")
 
-        # We want to restore in the reverse order the changes were enabled.
-        for restorable in reversed(restorables):
-            restorable.restore()
+        # Restore the Changes in the reverse order the changes were enabled.
+        processed_restorables = []
+        while True:
+            try:
+                restorable = self._restorables.pop()
+            except IndexError:
+                break
+
+            if restorable is self.partition:
+                if _honor_partitions:
+                    # Stop once a partition is reached (this is how
+                    # pop_to_partition() is implemented.
+                    return []
+                else:
+                    # This code ignores partitions.  Only pop_to_partition() honors
+                    # them.
+                    continue
 
-        # Reset the internal storage in case we want to use it again
-        self._restorables = []
+            try:
+                restorable.restore()
+            # Catch SystemExit too because we might still be calling
+            # 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)))
 
-        # Now that we know everything succeeded, reverse the list that we return to the user
-        restorables.reverse()
+            processed_restorables.append(restorable)
 
-        return restorables
+        return processed_restorables
 
     def pop_to_partition(self):
         """
@@ -236,34 +260,10 @@ def pop_to_partition(self):
 
         .. warning::
             * For the hack to 1.4, you need to make sure that at least one partition has been pushed
-              onto the stack.
+                onto the stack.
             * Unlike pop() and pop_all(), this method doesn't return anything.
         """
-        # This code is based on pop() instead of pop_all() because we need to
-        # stop when we reach the first parition.
-        try:
-            restorable = self._restorables.pop()
-        except IndexError as e:
-            # Use a more specific error message
-            args = list(e.args)
-            args[0] = "No backups to restore"
-            e.args = tuple(args)
-            raise e
-
-        if restorable is self.partition:
-            return
-
-        restorable.restore()
-
-        try:
-            self.pop_to_partition()
-        except IndexError:
-            # We only want to raise IndexError if there were no restorables in
-            # the stack to begin with.  So as long as we've called ourselves
-            # recursively, we will ignore it
-            pass
-
-        return
+        self.pop_all(_honor_partitions=True)
 
 
 @six.add_metaclass(abc.ABCMeta)

convert2rhel/unit_tests/__init__.py

@@ -818,3 +818,13 @@ def enable(self):
     def restore(self):
         self.called["restore"] += 1
         super(MinimalRestorable, self).restore()
+
+
+class ErrorOnRestoreRestorable(MinimalRestorable):
+    def __init__(self, exception=None):
+        self.exception = exception or Exception()
+        super(ErrorOnRestoreRestorable, self).__init__()
+
+    def restore(self):
+        super(ErrorOnRestoreRestorable, self).restore()
+        raise self.exception

convert2rhel/unit_tests/backup_test.py

@@ -6,7 +6,7 @@
 import six
 
 from convert2rhel import backup, exceptions, repo, unit_tests, utils  # Imports unit_tests/__init__.py
-from convert2rhel.unit_tests import DownloadPkgMocked, MinimalRestorable, RunSubprocessMocked
+from convert2rhel.unit_tests import DownloadPkgMocked, ErrorOnRestoreRestorable, MinimalRestorable, RunSubprocessMocked
 from convert2rhel.unit_tests.conftest import centos8
 
 
@@ -473,6 +473,69 @@ def test_pop_all_when_empty(self, backup_controller):
         with pytest.raises(IndexError, match="No backups to restore"):
             backup_controller.pop_all()
 
+    def test_pop_all_error_in_restore(self, backup_controller, caplog):
+        restorable1 = MinimalRestorable()
+        restorable2 = ErrorOnRestoreRestorable(exception=ValueError("Restorable2 failed"))
+        restorable3 = MinimalRestorable()
+
+        backup_controller.push(restorable1)
+        backup_controller.push(restorable2)
+        backup_controller.push(restorable3)
+
+        popped_restorables = backup_controller.pop_all()
+
+        assert len(popped_restorables) == 3
+        assert popped_restorables == [restorable3, restorable2, restorable1]
+        assert caplog.records[-1].message == "Error while rolling back a ErrorOnRestoreRestorable: Restorable2 failed"
+
+    # The following tests are for the 1.4 kludge to split restoration via
+    # backup_controller into two parts.  They can be removed once we have
+    # all rollback items ported to use the BackupController and the partition
+    # code is removed.
+
+    def test_pop_with_partition(self, backup_controller):
+        restorable1 = MinimalRestorable()
+
+        backup_controller.push(restorable1)
+        backup_controller.push(backup_controller.partition)
+
+        restorable = backup_controller.pop()
+
+        assert restorable == restorable1
+        assert backup_controller._restorables == []
+
+    def test_pop_all_with_partition(self, backup_controller):
+        restorable1 = MinimalRestorable()
+        restorable2 = MinimalRestorable()
+
+        backup_controller.push(restorable1)
+        backup_controller.push(backup_controller.partition)
+        backup_controller.push(restorable2)
+
+        restorables = backup_controller.pop_all()
+
+        assert restorables == [restorable2, restorable1]
+
+    def test_pop_to_partition(self, backup_controller):
+        restorable1 = MinimalRestorable()
+        restorable2 = MinimalRestorable()
+
+        backup_controller.push(restorable1)
+        backup_controller.push(backup_controller.partition)
+        backup_controller.push(restorable2)
+
+        assert backup_controller._restorables == [restorable1, backup_controller.partition, restorable2]
+
+        backup_controller.pop_to_partition()
+
+        assert backup_controller._restorables == [restorable1]
+
+        backup_controller.pop_to_partition()
+
+        assert backup_controller._restorables == []
+
+    # End of tests that are for the 1.4 partition hack.
+
 
 class TestRestorableRpmKey:
     gpg_key = os.path.realpath(

convert2rhel/unit_tests/main_test.py

@@ -29,9 +29,9 @@
 six.add_move(six.MovedModule("mock", "mock", "unittest.mock"))
 from six.moves import mock
 
-from convert2rhel import actions, applock, backup, cert, checks, exceptions, grub
+from convert2rhel import actions, applock, backup, checks, exceptions, grub
 from convert2rhel import logger as logger_module
-from convert2rhel import main, pkghandler, pkgmanager, redhatrelease, repo, subscription, toolopts, utils
+from convert2rhel import main, pkghandler, pkgmanager, redhatrelease, repo, subscription, toolopts, unit_tests, utils
 from convert2rhel.actions import report
 from convert2rhel.breadcrumbs import breadcrumbs
 from convert2rhel.systeminfo import system_info
@@ -52,6 +52,76 @@
 )
 
 
+class TestRollbackChanges:
+    @pytest.fixture(autouse=True)
+    def mock_rollback_functions(self, monkeypatch):
+        monkeypatch.setattr(backup.changed_pkgs_control, "restore_pkgs", mock.Mock())
+        monkeypatch.setattr(repo, "restore_yum_repos", mock.Mock())
+        monkeypatch.setattr(repo, "restore_varsdir", mock.Mock())
+        monkeypatch.setattr(pkghandler.versionlock_file, "restore", mock.Mock())
+
+    def test_rollback_changes(self, monkeypatch):
+        monkeypatch.setattr(backup.backup_control, "pop_all", mock.Mock())
+
+        main.rollback_changes()
+
+        assert backup.changed_pkgs_control.restore_pkgs.call_count == 1
+        assert repo.restore_yum_repos.call_count == 1
+        assert pkghandler.versionlock_file.restore.call_count == 1
+        # Note: when we remove the BackupController partition hack, the first
+        # of these calls will go away
+        assert backup.backup_control.pop_all.call_args_list == [mock.call(_honor_partitions=True), mock.call()]
+        assert repo.restore_varsdir.call_count == 1
+
+    # The tests below are for the 1.4 hack that splits restore of Changes
+    # managed by the BackupController into two parts: one that executes before
+    # the unported restore items and a second that executes after.  They can be
+    # removed once all the Changes are managed by BackupControl and the
+    # partition code is removed.
+    def test_backup_control_partitioning_no_partition(self, caplog, monkeypatch):
+        backup_control_test = backup.BackupController()
+        monkeypatch.setattr(backup, "backup_control", backup_control_test)
+
+        main.rollback_changes()
+
+        assert caplog.records[-1].levelname == "INFO"
+        assert caplog.records[-1].message == "During rollback there were no backups to restore"
+
+    def test_backup_control_partitioning_with_partition(self, caplog, monkeypatch):
+        backup_control_test = backup.BackupController()
+        restorable1 = unit_tests.MinimalRestorable()
+        restorable2 = unit_tests.MinimalRestorable()
+        backup_control_test.push(restorable1)
+        backup_control_test.push(backup_control_test.partition)
+        backup_control_test.push(restorable2)
+        monkeypatch.setattr(backup, "backup_control", backup_control_test)
+
+        main.rollback_changes()
+
+        assert "During rollback there were no backups to restore" not in caplog.text
+
+    def test_backup_control_with_changes_only_before_the_partition(self, caplog, monkeypatch):
+        backup_control_test = backup.BackupController()
+        restorable1 = unit_tests.MinimalRestorable()
+        backup_control_test.push(backup_control_test.partition)
+        backup_control_test.push(restorable1)
+        monkeypatch.setattr(backup, "backup_control", backup_control_test)
+
+        main.rollback_changes()
+
+        assert "During rollback there were no backups to restore" not in caplog.text
+
+    def test_backup_control_unknown_exception(self, monkeypatch):
+        monkeypatch.setattr(
+            backup.backup_control,
+            "pop_all",
+            mock.Mock(side_effect=([], IndexError("Raised because of a bug in the code"))),
+        )
+
+        with pytest.raises(IndexError, match="Raised because of a bug in the code"):
+            main.rollback_changes()
+
+
 @pytest.mark.parametrize(("exception_type", "exception"), ((IOError, True), (OSError, True), (None, False)))
 def test_initialize_logger(exception_type, exception, monkeypatch, capsys):
     setup_logger_handler_mock = mock.Mock()
@@ -525,26 +595,3 @@ def test_main_rollback_post_ponr_changes_phase(self, monkeypatch, caplog, tmp_pa
         assert summary_as_txt_mock.call_count == 1
         assert "The system is left in an undetermined state that Convert2RHEL cannot fix." in caplog.records[-2].message
         assert update_rhsm_custom_facts_mock.call_count == 1
-
-    def test_rollback_changes(self, monkeypatch):
-        mock_restore_pkgs = mock.Mock()
-        mock_restore_yum_repos = mock.Mock()
-        mock_versionlock_file_restore = mock.Mock()
-        mock_cert_get_cert = mock.Mock(return_value="anything")
-        mock_backup_control_pop_all = mock.Mock()
-        mock_restore_varsdir = mock.Mock()
-
-        monkeypatch.setattr(backup.changed_pkgs_control, "restore_pkgs", mock_restore_pkgs)
-        monkeypatch.setattr(repo, "restore_yum_repos", mock_restore_yum_repos)
-        monkeypatch.setattr(pkghandler.versionlock_file, "restore", mock_versionlock_file_restore)
-        monkeypatch.setattr(cert, "_get_cert", mock_cert_get_cert)
-        monkeypatch.setattr(backup.backup_control, "pop_all", mock_backup_control_pop_all)
-        monkeypatch.setattr(repo, "restore_varsdir", mock_restore_varsdir)
-
-        main.rollback_changes()
-
-        assert mock_restore_pkgs.call_count == 1
-        assert mock_restore_yum_repos.call_count == 1
-        assert mock_versionlock_file_restore.call_count == 1
-        assert mock_backup_control_pop_all.call_count == 1
-        assert mock_restore_varsdir.call_count == 1