From 274888c79fce9ee52a45a7a82db73fb118505a64 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 26 Sep 2019 16:19:43 +0200 Subject: [PATCH 1/5] Better handling of remove failure (and in particular, catch manual interrupts to still perform the rest of the cleaning) --- src/yunohost/app.py | 38 +++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 5a51e57bb..05e1bdf4b 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -944,10 +944,21 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu env=env_dict_remove) operation_logger_remove.start() - remove_retcode = hook_exec( - os.path.join(extracted_app_folder, 'scripts/remove'), - args=[app_instance_name], env=env_dict_remove - )[0] + # 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] + # Here again, calling hook_exec could failed 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): + remove_retcode = -1 + import traceback + logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) + # Remove all permission in LDAP result = ldap.search(base='ou=permission,dc=yunohost,dc=org', filter='(&(objectclass=permissionYnh)(cn=*.%s))' % app_instance_name, attrs=['cn']) @@ -1057,11 +1068,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, - env=env_dict)[0] == 0: - logger.success(m18n.n('app_removed', app=app)) + try: + ret = hook_exec('/tmp/yunohost_remove/scripts/remove', + args=args_list, + env=env_dict)[0] + # Here again, calling hook_exec could failed 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) From 47bdfd8654548d1ddde537fa1d8cf1594a860265 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 26 Sep 2019 16:21:22 +0200 Subject: [PATCH 2/5] Clarify the handling of install script failures... --- locales/en.json | 1 + src/yunohost/app.py | 37 +++++++++++++++++++++++-------------- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/locales/en.json b/locales/en.json index 61fdcfa9b..a182c7559 100644 --- a/locales/en.json +++ b/locales/en.json @@ -22,6 +22,7 @@ "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_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}'", diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 05e1bdf4b..af6c5d522 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -908,29 +908,42 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu permission_add(app=app_instance_name, permission="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 = (install_retcode != 0) + if install_failed: + error = m18n.n('unexpected_error', error='shell command return code: %s' % install_retcode) + 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 - except Exception: + error = m18n.n('operation_interrupted') + 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: - error_msg = operation_logger.error(m18n.n('unexpected_error', error='shell command return code: %s' % install_retcode)) - - if install_retcode != 0 or broke_the_system: + # If the install failed or broke the system, we remove it + if install_failed or broke_the_system: if not no_remove_on_failure: # Setup environment for remove script env_dict_remove = {} @@ -985,11 +998,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu app_ssowatconf() - if install_retcode == -1: - msg = m18n.n('operation_interrupted') + " " + error_msg - raise YunohostError(msg, raw_msg=True) - msg = error_msg - raise YunohostError(msg, raw_msg=True) + raise YunohostError("app_install_failed", app=app_id) # Clean hooks and add new ones hook_remove(app_instance_name) From 9331f44b343ae192bd86ec5ccf87f855bee7ba16 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 26 Sep 2019 16:33:15 +0200 Subject: [PATCH 3/5] This message about shell command return code is too technical and uninformative. Let's explain what happen, which is that some error occured inside the install script (and details are in the debug log). --- locales/en.json | 1 + src/yunohost/app.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/locales/en.json b/locales/en.json index a182c7559..ba0d7e3cd 100644 --- a/locales/en.json +++ b/locales/en.json @@ -23,6 +23,7 @@ "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}'", diff --git a/src/yunohost/app.py b/src/yunohost/app.py index af6c5d522..57df36713 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -917,7 +917,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu # "Common" app install failure : the script failed and returned exit code != 0 install_failed = (install_retcode != 0) if install_failed: - error = m18n.n('unexpected_error', error='shell command return code: %s' % install_retcode) + 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 From ccc49a2b2824d8aedfb642e4e9e273fa85431eac Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 26 Sep 2019 16:34:14 +0200 Subject: [PATCH 4/5] Simplify that indentation madness --- src/yunohost/app.py | 90 +++++++++++++++++++++++---------------------- 1 file changed, 47 insertions(+), 43 deletions(-) diff --git a/src/yunohost/app.py b/src/yunohost/app.py index 57df36713..f2eea5646 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -944,53 +944,57 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu # If the install failed or broke the system, we remove it if install_failed or broke_the_system: - if not no_remove_on_failure: - # Setup environment for remove script - 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 - operation_logger_remove = OperationLogger('remove_on_failed_install', - [('app', app_instance_name)], - env=env_dict_remove) - operation_logger_remove.start() + # This option is meant for packagers to debug their apps more easily + if no_remove_on_failure: + raise YunohostError("The installation of %s failed, but was not cleaned up as requested by --no-remove-on-failure." % app_id, raw_msg=True) - # Try to remove the app + # Setup environment for remove script + 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 + 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] + # Here again, calling hook_exec could failed 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): + remove_retcode = -1 + import traceback + logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) + + # Remove all permission in LDAP + result = ldap.search(base='ou=permission,dc=yunohost,dc=org', + filter='(&(objectclass=permissionYnh)(cn=*.%s))' % app_instance_name, attrs=['cn']) + permission_list = [p['cn'][0] for p in result] + for l in permission_list: + permission_remove(app_instance_name, l.split('.')[0], force=True) + + 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: - remove_retcode = hook_exec( - os.path.join(extracted_app_folder, 'scripts/remove'), - args=[app_instance_name], env=env_dict_remove - )[0] - # Here again, calling hook_exec could failed 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): - remove_retcode = -1 - import traceback - logger.exception(m18n.n('unexpected_error', error=u"\n" + traceback.format_exc())) - - # Remove all permission in LDAP - result = ldap.search(base='ou=permission,dc=yunohost,dc=org', - filter='(&(objectclass=permissionYnh)(cn=*.%s))' % app_instance_name, attrs=['cn']) - permission_list = [p['cn'][0] for p in result] - for l in permission_list: - permission_remove(app_instance_name, l.split('.')[0], force=True) - - if remove_retcode != 0: - msg = m18n.n('app_not_properly_removed', - app=app_instance_name) - logger.warning(msg) - operation_logger_remove.error(msg) + _assert_system_is_sane_for_app(manifest, "post") + except Exception as e: + operation_logger_remove.error(e) else: - try: - _assert_system_is_sane_for_app(manifest, "post") - except Exception as e: - operation_logger_remove.error(e) - else: - operation_logger_remove.success() + operation_logger_remove.success() # Clean tmp folders shutil.rmtree(app_setting_path) From d159f7ff07d20734d50594cf1e8eacde7f77f1ad Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Sat, 28 Sep 2019 16:11:44 +0200 Subject: [PATCH 5/5] Misc typo / wording / readability Co-Authored-By: decentral1se --- locales/en.json | 2 +- src/yunohost/app.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/locales/en.json b/locales/en.json index ba0d7e3cd..f9194bb42 100644 --- a/locales/en.json +++ b/locales/en.json @@ -23,7 +23,7 @@ "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_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}'", diff --git a/src/yunohost/app.py b/src/yunohost/app.py index f2eea5646..8c7d37f92 100644 --- a/src/yunohost/app.py +++ b/src/yunohost/app.py @@ -915,7 +915,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu args=args_list, env=env_dict )[0] # "Common" app install failure : the script failed and returned exit code != 0 - install_failed = (install_retcode != 0) + install_failed = True if install_retcode != 0 else False if install_failed: error = m18n.n('app_install_script_failed') logger.exception(error) @@ -967,7 +967,7 @@ def app_install(operation_logger, app, label=None, args=None, no_remove_on_failu os.path.join(extracted_app_folder, 'scripts/remove'), args=[app_instance_name], env=env_dict_remove )[0] - # Here again, calling hook_exec could failed miserably, or get + # 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} ...) @@ -1085,7 +1085,7 @@ def app_remove(operation_logger, app): ret = hook_exec('/tmp/yunohost_remove/scripts/remove', args=args_list, env=env_dict)[0] - # Here again, calling hook_exec could failed miserably, or get + # 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} ...)