Merge pull request #811 from YunoHost/better-handling-of-app-install-and-remove-failures

Better handling of app install and remove failures

locales/en.json

@@ -23,6 +23,8 @@
     "app_id_invalid": "Invalid app ID",
     "app_incompatible": "The app {app} is incompatible with your YunoHost version",
     "app_install_files_invalid": "These files cannot be installed",
+    "app_install_failed": "Could not install {app}",
+    "app_install_script_failed": "An error occured inside the app installation script",
     "app_location_already_used": "The app '{app}' is already installed in ({path})",
     "app_make_default_location_already_used": "Can't make the app '{app}' the default on the domain, {domain} is already in use by the other app '{other_app}'",
     "app_location_install_failed": "Cannot install the app there because it conflicts with the app '{other_app}' already installed in '{other_path}'",

src/yunohost/app.py

@@ -926,63 +926,91 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu
     permission_create(app_instance_name+".main", sync_perm=False)
 
     # Execute the app install script
-    install_retcode = 1
+    install_failed = True
     try:
         install_retcode = hook_exec(
             os.path.join(extracted_app_folder, 'scripts/install'),
             args=args_list, env=env_dict
         )[0]
+        # "Common" app install failure : the script failed and returned exit code != 0
+        install_failed = True if install_retcode != 0 else False
+        if install_failed:
+            error = m18n.n('app_install_script_failed')
+            logger.exception(error)
+            operation_logger.error(error)
+    # Script got manually interrupted ... N.B. : KeyboardInterrupt does not inherit from Exception
     except (KeyboardInterrupt, EOFError):
-        install_retcode = -1
+        error = m18n.n('operation_interrupted')
-    except Exception:
+        logger.exception(error)
+        operation_logger.error(error)
+    # Something wrong happened in Yunohost's code (most probably hook_exec)
+    except Exception as e :
         import traceback
-        logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc()))
+        error = m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())
+        logger.exception(error)
+        operation_logger.error(error)
     finally:
+        # Whatever happened (install success or failure) we check if it broke the system
+        # and warn the user about it
         try:
             broke_the_system = False
             _assert_system_is_sane_for_app(manifest, "post")
         except Exception as e:
             broke_the_system = True
-            error_msg = operation_logger.error(str(e))
+            logger.exception(str(e))
+            operation_logger.error(str(e))
 
-        if install_retcode != 0:
+        # If the install failed or broke the system, we remove it
-            error_msg = operation_logger.error(m18n.n('unexpected_error', error='shell command return code: %s' % install_retcode))
+        if install_failed or broke_the_system:
 
-        if install_retcode != 0 or broke_the_system:
+            # This option is meant for packagers to debug their apps more easily
-            if not no_remove_on_failure:
+            if no_remove_on_failure:
-                # Setup environment for remove script
+                raise YunohostError("The installation of %s failed, but was not cleaned up as requested by --no-remove-on-failure." % app_id, raw_msg=True)
-                env_dict_remove = {}
-                env_dict_remove["YNH_APP_ID"] = app_id
-                env_dict_remove["YNH_APP_INSTANCE_NAME"] = app_instance_name
-                env_dict_remove["YNH_APP_INSTANCE_NUMBER"] = str(instance_number)
 
-                # Execute remove script
+            # Setup environment for remove script
-                operation_logger_remove = OperationLogger('remove_on_failed_install',
+            env_dict_remove = {}
-                                                          [('app', app_instance_name)],
+            env_dict_remove["YNH_APP_ID"] = app_id
-                                                          env=env_dict_remove)
+            env_dict_remove["YNH_APP_INSTANCE_NAME"] = app_instance_name
-                operation_logger_remove.start()
+            env_dict_remove["YNH_APP_INSTANCE_NUMBER"] = str(instance_number)
 
+            # Execute remove script
+            operation_logger_remove = OperationLogger('remove_on_failed_install',
+                                                      [('app', app_instance_name)],
+                                                      env=env_dict_remove)
+            operation_logger_remove.start()
+
+            # Try to remove the app
+            try:
                 remove_retcode = hook_exec(
                     os.path.join(extracted_app_folder, 'scripts/remove'),
                     args=[app_instance_name], env=env_dict_remove
                 )[0]
-                # Remove all permission in LDAP
+            # Here again, calling hook_exec could fail miserably, or get
-                for permission_name in user_permission_list()["permissions"].keys():
+            # manually interrupted (by mistake or because script was stuck)
-                    if permission_name.startswith(app_instance_name+"."):
+            # In that case we still want to proceed with the rest of the
-                        permission_delete(permission_name, force=True)
+            # removal (permissions, /etc/yunohost/apps/{app} ...)
+            except (KeyboardInterrupt, EOFError, Exception):
+                remove_retcode = -1
+                import traceback
+                logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc()))
 
-                if remove_retcode != 0:
+            # Remove all permission in LDAP
-                    msg = m18n.n('app_not_properly_removed',
+            for permission_name in user_permission_list()["permissions"].keys():
-                                 app=app_instance_name)
+                if permission_name.startswith(app_instance_name+"."):
-                    logger.warning(msg)
+                    permission_delete(permission_name, force=True)
-                    operation_logger_remove.error(msg)
+
+            if remove_retcode != 0:
+                msg = m18n.n('app_not_properly_removed',
+                             app=app_instance_name)
+                logger.warning(msg)
+                operation_logger_remove.error(msg)
+            else:
+                try:
+                    _assert_system_is_sane_for_app(manifest, "post")
+                except Exception as e:
+                    operation_logger_remove.error(e)
                 else:
-                    try:
+                    operation_logger_remove.success()
-                        _assert_system_is_sane_for_app(manifest, "post")
-                    except Exception as e:
-                        operation_logger_remove.error(e)
-                    else:
-                        operation_logger_remove.success()
 
             # Clean tmp folders
             shutil.rmtree(app_setting_path)
@@ -990,11 +1018,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu
 
             app_ssowatconf()
 
-            if install_retcode == -1:
+            raise YunohostError("app_install_failed", app=app_id)
-                msg = m18n.n('operation_interrupted') + " " + error_msg
-                raise YunohostError(msg, raw_msg=True)
-            msg = error_msg
-            raise YunohostError(msg, raw_msg=True)
 
     # Clean hooks and add new ones
     hook_remove(app_instance_name)
@@ -1071,11 +1095,24 @@ def app_remove(operation_logger, app):
     operation_logger.extra.update({'env': env_dict})
     operation_logger.flush()
 
-    if hook_exec('/tmp/yunohost_remove/scripts/remove', args=args_list,
+    try:
-                 env=env_dict)[0] == 0:
+        ret = hook_exec('/tmp/yunohost_remove/scripts/remove',
-        logger.success(m18n.n('app_removed', app=app))
+                        args=args_list,
+                        env=env_dict)[0]
+    # Here again, calling hook_exec could fail miserably, or get
+    # manually interrupted (by mistake or because script was stuck)
+    # In that case we still want to proceed with the rest of the
+    # removal (permissions, /etc/yunohost/apps/{app} ...)
+    except (KeyboardInterrupt, EOFError, Exception):
+        ret = -1
+        import traceback
+        logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc()))
 
+    if ret == 0:
+        logger.success(m18n.n('app_removed', app=app))
         hook_callback('post_app_remove', args=args_list, env=env_dict)
+    else:
+        logger.warning(m18n.n('app_not_properly_removed', app=app))
 
     if os.path.exists(app_setting_path):
         shutil.rmtree(app_setting_path)