appv2: better error handling for app resources provisioning/deprovisioning/update failures

locales/en.json

@@ -27,6 +27,7 @@
     "app_full_domain_unavailable": "Sorry, this app must be installed on a domain of its own, but other apps are already installed on the domain '{domain}'. You could use a subdomain dedicated to this app instead.",
     "app_id_invalid": "Invalid app ID",
     "app_install_failed": "Unable to install {app}: {error}",
+    "app_resource_failed": "Provisioning, deprovisioning, or updating resources for {app} failed: {error}",
     "app_install_files_invalid": "These files cannot be installed",
     "app_install_script_failed": "An error occurred inside the app installation script",
     "app_label_deprecated": "This command is deprecated! Please use the new command 'yunohost user permission update' to manage the app label.",

src/app.py

@@ -675,13 +675,9 @@ def app_upgrade(app=[], url=None, file=None, force=False, no_safety_backup=False
         if manifest["packaging_format"] >= 2:
             from yunohost.utils.resources import AppResourceManager
 
-            try:
+            AppResourceManager(
-                AppResourceManager(
+                app_instance_name, wanted=manifest, current=app_dict["manifest"]
-                    app_instance_name, wanted=manifest, current=app_dict["manifest"]
+            ).apply(rollback_and_raise_exception_if_failure=True, operation_logger=operation_logger)
-                ).apply(rollback_if_failure=True)
-            except Exception:
-                # FIXME : improve error handling ....
-                raise
 
         # Execute the app upgrade script
         upgrade_failed = True
@@ -1038,13 +1034,9 @@ def app_install(
     if packaging_format >= 2:
         from yunohost.utils.resources import AppResourceManager
 
-        try:
+        AppResourceManager(app_instance_name, wanted=manifest, current={}).apply(
-            AppResourceManager(app_instance_name, wanted=manifest, current={}).apply(
+            rollback_and_raise_exception_if_failure=True, operation_logger=operation_logger
-                rollback_if_failure=True
+        )
-            )
-        except Exception:
-            # FIXME : improve error handling ....
-            raise
     else:
         # Initialize the main permission for the app
         # The permission is initialized with no url associated, and with tile disabled
@@ -1108,6 +1100,9 @@ def app_install(
                 "Packagers /!\\ This app manually modified some system configuration files! This should not happen! If you need to do so, you should implement a proper conf_regen hook. Those configuration were affected:\n    - "
                 + "\n     -".join(manually_modified_files_by_app)
             )
+            # Actually forbid this for app packaging >= 2
+            if packaging_format >= 2:
+                broke_the_system = True
 
         # If the install failed or broke the system, we remove it
         if install_failed or broke_the_system:
@@ -1157,13 +1152,9 @@ def app_install(
             if packaging_format >= 2:
                 from yunohost.utils.resources import AppResourceManager
 
-                try:
+                AppResourceManager(
-                    AppResourceManager(
+                    app_instance_name, wanted={}, current=manifest
-                        app_instance_name, wanted={}, current=manifest
+                ).apply(rollback_and_raise_exception_if_failure=False)
-                    ).apply(rollback_if_failure=False)
-                except Exception:
-                    # FIXME : improve error handling ....
-                    raise
             else:
                 # Remove all permission in LDAP
                 for permission_name in user_permission_list()["permissions"].keys():
@@ -1288,15 +1279,11 @@ def app_remove(operation_logger, app, purge=False):
 
     packaging_format = manifest["packaging_format"]
     if packaging_format >= 2:
-        try:
+        from yunohost.utils.resources import AppResourceManager
-            from yunohost.utils.resources import AppResourceManager
 
-            AppResourceManager(app, wanted={}, current=manifest).apply(
+        AppResourceManager(app, wanted={}, current=manifest).apply(
-                rollback_if_failure=False
+            rollback_and_raise_exception_if_failure=False
-            )
+        )
-        except Exception:
-            # FIXME : improve error handling ....
-            raise
     else:
         # Remove all permission in LDAP
         for permission_name in user_permission_list(apps=[app])["permissions"].keys():

src/backup.py

@@ -1518,13 +1518,9 @@ class RestoreManager:
         if manifest["packaging_format"] >= 2:
             from yunohost.utils.resources import AppResourceManager
 
-            try:
+            AppResourceManager(
-                AppResourceManager(
+                app_instance_name, wanted=manifest, current={}
-                    app_instance_name, wanted=manifest, current={}
+            ).apply(rollback_and_raise_exception_if_failure=True, operation_logger=operation_logger)
-                ).apply(rollback_if_failure=True)
-            except Exception:
-                # FIXME : improve error handling ....
-                raise
 
         # Execute the app install script
         restore_failed = True

src/utils/resources.py

@@ -36,9 +36,6 @@ logger = getActionLogger("yunohost.app_resources")
 
 class AppResourceManager:
 
-    # FIXME : add some sort of documentation mechanism
-    # to create a have a detailed description of each resource behavior
-
     def __init__(self, app: str, current: Dict, wanted: Dict):
 
         self.app = app
@@ -50,7 +47,7 @@ class AppResourceManager:
         if "resources" not in self.wanted:
             self.wanted["resources"] = {}
 
-    def apply(self, rollback_if_failure, **context):
+    def apply(self, rollback_and_raise_exception_if_failure, operation_logger=None, **context):
 
         todos = list(self.compute_todos())
         completed = []
@@ -69,12 +66,13 @@ class AppResourceManager:
                 elif todo == "update":
                     logger.info(f"Updating {name} ...")
                     new.provision_or_update(context=context)
-            # FIXME FIXME FIXME : this exception doesnt catch Ctrl+C ?!?!
+            except (KeyboardInterrupt, Exception) as e:
-            except Exception as e:
                 exception = e
-                # FIXME: better error handling ? display stacktrace ?
+                if isinstance(e, KeyboardInterrupt):
-                logger.warning(f"Failed to {todo} for {name} : {e}")
+                    logger.error(m18n.n("operation_interrupted"))
-                if rollback_if_failure:
+                else:
+                    logger.warning(f"Failed to {todo} {name} : {e}")
+                if rollback_and_raise_exception_if_failure:
                     rollback = True
                     completed.append((todo, name, old, new))
                     break
@@ -97,12 +95,24 @@ class AppResourceManager:
                     elif todo == "update":
                         logger.info(f"Reverting {name} ...")
                         old.provision_or_update(context=context)
-                except Exception as e:
+                except (KeyboardInterrupt, Exception) as e:
-                    # FIXME: better error handling ? display stacktrace ?
+                    if isinstance(e, KeyboardInterrupt):
-                    logger.error(f"Failed to rollback {name} : {e}")
+                        logger.error(m18n.n("operation_interrupted"))
+                    else:
+                        logger.error(f"Failed to rollback {name} : {e}")
 
         if exception:
-            raise exception
+            if rollback_and_raise_exception_if_failure:
+                logger.error(m18n.n("app_resource_failed", app=self.app, error=exception))
+                if operation_logger:
+                    failure_message_with_debug_instructions = operation_logger.error(str(exception))
+                    raise YunohostError(
+                        failure_message_with_debug_instructions, raw_msg=True
+                    )
+                else:
+                    raise YunohostError(str(exception), raw_msg=True0
+            else:
+                logger.error(exception)
 
     def compute_todos(self):