From 199258166e36a0d79de20b6db6581711af5c6acd Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 6 May 2020 18:12:55 +0200 Subject: [PATCH 1/7] services[name] -> service --- src/yunohost/service.py | 42 +++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 20 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index c17eb04c2..aec754bd4 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -61,27 +61,27 @@ def service_add(name, description=None, log=None, log_type="file", test_status=N """ services = _get_services() - services[name] = {} + services[name] = service = {} if log is not None: if not isinstance(log, list): log = [log] - services[name]['log'] = log + service['log'] = log if not isinstance(log_type, list): log_type = [log_type] if len(log_type) < len(log): - log_type.extend([log_type[-1]] * (len(log) - len(log_type))) # extend list to have the same size as log + log_type.extend([log_type[-1]] * (len(log) - len(log_type))) # extend list to have the same size as log if len(log_type) == len(log): - services[name]['log_type'] = log_type + service['log_type'] = log_type else: raise YunohostError('service_add_failed', service=name) if description: - services[name]['description'] = description + service['description'] = description else: # Try to get the description from systemd service out = subprocess.check_output("systemctl show %s | grep '^Description='" % name, shell=True).strip() @@ -92,23 +92,23 @@ def service_add(name, description=None, log=None, log_type="file", test_status=N if out == name + ".service": logger.warning("/!\\ Packager ! You added a custom service without specifying a description. Please add a proper Description in the systemd configuration, or use --description to explain what the service does in a similar fashion to existing services.") else: - services[name]['description'] = out + service['description'] = out if need_lock: - services[name]['need_lock'] = True + service['need_lock'] = True if test_status: - services[name]["test_status"] = test_status + service["test_status"] = test_status if test_conf: - services[name]["test_conf"] = test_conf + service["test_conf"] = test_conf if needs_exposed_ports: - services[name]["needs_exposed_ports"] = needs_exposed_ports + service["needs_exposed_ports"] = needs_exposed_ports try: _save_services(services) - except: + except Exception: # we'll get a logger.warning with more details in _save_services raise YunohostError('service_add_failed', service=name) @@ -288,6 +288,8 @@ def service_status(names=[]): if check_names and name not in services.keys(): raise YunohostError('service_unknown', service=name) + service = services[name] + # this "service" isn't a service actually so we skip it # # the historical reason is because regenconf has been hacked into the @@ -296,10 +298,10 @@ def service_status(names=[]): # the hack was to add fake services... # we need to extract regenconf from service at some point, also because # some app would really like to use it - if services[name].get("status", "") is None: + if service.get("status", "") is None: continue - systemd_service = services[name].get("actual_systemd_service", name) + systemd_service = service.get("actual_systemd_service", name) status = _get_service_information_from_systemd(systemd_service) if status is None: @@ -314,8 +316,8 @@ def service_status(names=[]): else: translation_key = "service_description_%s" % name - if "description" in services[name] is not None: - description = services[name].get("description") + if "description" in service is not None: + description = service.get("description") else: description = m18n.n(translation_key) @@ -336,7 +338,7 @@ def service_status(names=[]): # Fun stuff™ : to obtain the enabled/disabled status for sysv services, # gotta do this ... cf code of /lib/systemd/systemd-sysv-install if result[name]["start_on_boot"] == "generated": - result[name]["start_on_boot"] = "enabled" if glob("/etc/rc[S5].d/S??"+name) else "disabled" + result[name]["start_on_boot"] = "enabled" if glob("/etc/rc[S5].d/S??" + name) else "disabled" elif os.path.exists("/etc/systemd/system/multi-user.target.wants/%s.service" % name): result[name]["start_on_boot"] = "enabled" @@ -344,8 +346,8 @@ def service_status(names=[]): result[name]['last_state_change'] = datetime.utcfromtimestamp(status["StateChangeTimestamp"] / 1000000) # 'test_status' is an optional field to test the status of the service using a custom command - if "test_status" in services[name]: - p = subprocess.Popen(services[name]["test_status"], + if "test_status" in service: + p = subprocess.Popen(service["test_status"], shell=True, executable='/bin/bash', stdout=subprocess.PIPE, @@ -356,8 +358,8 @@ def service_status(names=[]): result[name]["status"] = "running" if p.returncode == 0 else "failed" # 'test_status' is an optional field to test the status of the service using a custom command - if "test_conf" in services[name]: - p = subprocess.Popen(services[name]["test_conf"], + if "test_conf" in service: + p = subprocess.Popen(service["test_conf"], shell=True, executable='/bin/bash', stdout=subprocess.PIPE, From 95dd1e2707e9504e114b33f6586a6ba925866f3c Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 6 May 2020 18:20:03 +0200 Subject: [PATCH 2/7] service -> infos ... + misc small syntax improvements --- src/yunohost/service.py | 53 ++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index aec754bd4..00dfaab1f 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -125,14 +125,13 @@ def service_remove(name): """ services = _get_services() - try: - del services[name] - except KeyError: + if name not in services: raise YunohostError('service_unknown', service=name) + del services[name] try: _save_services(services) - except: + except Exception: # we'll get a logger.warning with more details in _save_services raise YunohostError('service_remove_failed', service=name) @@ -275,20 +274,24 @@ def service_status(names=[]): """ services = _get_services() - check_names = True + + # If function was called with a specific list of service + if names != []: + # If user wanna check the status of a single service + if isinstance(names, str): + names = [names] + + # Validate service names requested + for name in names: + if name not in services.keys(): + raise YunohostError('service_unknown', service=name) + + # Filter only requested servivces + services = {k: v for k, v in services.items() if k in names} + result = {} - if isinstance(names, str): - names = [names] - elif len(names) == 0: - names = services.keys() - check_names = False - - for name in names: - if check_names and name not in services.keys(): - raise YunohostError('service_unknown', service=name) - - service = services[name] + for name, infos in services.items(): # this "service" isn't a service actually so we skip it # @@ -298,10 +301,10 @@ def service_status(names=[]): # the hack was to add fake services... # we need to extract regenconf from service at some point, also because # some app would really like to use it - if service.get("status", "") is None: + if infos.get("status", "") is None: continue - systemd_service = service.get("actual_systemd_service", name) + systemd_service = infos.get("actual_systemd_service", name) status = _get_service_information_from_systemd(systemd_service) if status is None: @@ -316,8 +319,8 @@ def service_status(names=[]): else: translation_key = "service_description_%s" % name - if "description" in service is not None: - description = service.get("description") + if "description" in infos is not None: + description = infos.get("description") else: description = m18n.n(translation_key) @@ -346,8 +349,8 @@ def service_status(names=[]): result[name]['last_state_change'] = datetime.utcfromtimestamp(status["StateChangeTimestamp"] / 1000000) # 'test_status' is an optional field to test the status of the service using a custom command - if "test_status" in service: - p = subprocess.Popen(service["test_status"], + if "test_status" in infos: + p = subprocess.Popen(infos["test_status"], shell=True, executable='/bin/bash', stdout=subprocess.PIPE, @@ -358,8 +361,8 @@ def service_status(names=[]): result[name]["status"] = "running" if p.returncode == 0 else "failed" # 'test_status' is an optional field to test the status of the service using a custom command - if "test_conf" in service: - p = subprocess.Popen(service["test_conf"], + if "test_conf" in infos: + p = subprocess.Popen(infos["test_conf"], shell=True, executable='/bin/bash', stdout=subprocess.PIPE, @@ -422,7 +425,7 @@ def service_log(name, number=50): if not isinstance(log_list, list): log_list = [log_list] if len(log_type_list) < len(log_list): - log_type_list.extend(["file"] * (len(log_list)-len(log_type_list))) + log_type_list.extend(["file"] * (len(log_list) - len(log_type_list))) result = {} From e74f49f0016f1c99fa006cc80a7b1d4dbf630266 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 6 May 2020 18:46:44 +0200 Subject: [PATCH 3/7] Simplify log list management because log type is deprecated now that we always fetch journalctl --- src/yunohost/service.py | 61 ++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 37 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 00dfaab1f..1fe65c102 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -44,7 +44,7 @@ MOULINETTE_LOCK = "/var/run/moulinette_yunohost.lock" logger = getActionLogger('yunohost.service') -def service_add(name, description=None, log=None, log_type="file", test_status=None, test_conf=None, needs_exposed_ports=None, need_lock=False, status=None): +def service_add(name, description=None, log=None, log_type=None, test_status=None, test_conf=None, needs_exposed_ports=None, need_lock=False, status=None): """ Add a custom service @@ -52,7 +52,7 @@ def service_add(name, description=None, log=None, log_type="file", test_status=N name -- Service name to add description -- description of the service log -- Absolute path to log file to display - log_type -- Specify if the corresponding log is a file or a systemd log + log_type -- (deprecated) Specify if the corresponding log is a file or a systemd log test_status -- Specify a custom bash command to check the status of the service. N.B. : it only makes sense to specify this if the corresponding systemd service does not return the proper information. test_conf -- Specify a custom bash command to check if the configuration of the service is valid or broken, similar to nginx -t. needs_exposed_ports -- A list of ports that needs to be publicly exposed for the service to work as intended. @@ -67,19 +67,14 @@ def service_add(name, description=None, log=None, log_type="file", test_status=N if not isinstance(log, list): log = [log] + # Deprecated log_type stuff + if log_type is not None: + logger.warning("/!\\ Packagers! --log_type is deprecated. You do not need to specify --log_type systemd anymore ... Yunohost now automatically fetch the journalctl of the systemd service by default.") + # Usually when adding such a service, the service name will be provided so we remove it as it's not a log file path + log.remove(name) + service['log'] = log - if not isinstance(log_type, list): - log_type = [log_type] - - if len(log_type) < len(log): - log_type.extend([log_type[-1]] * (len(log) - len(log_type))) # extend list to have the same size as log - - if len(log_type) == len(log): - service['log_type'] = log_type - else: - raise YunohostError('service_add_failed', service=name) - if description: service['description'] = description else: @@ -420,41 +415,33 @@ def service_log(name, number=50): raise YunohostError('service_unknown', service=name) log_list = services[name].get('log', []) - log_type_list = services[name].get('log_type', []) - if not isinstance(log_list, list): - log_list = [log_list] - if len(log_type_list) < len(log_list): - log_type_list.extend(["file"] * (len(log_list) - len(log_type_list))) + # Legacy stuff related to --log_type where we'll typically have the service + # name in the log list but it's not an actual logfile. Nowadays journalctl + # is automatically fetch as well as regular log files. + log_list.remove(name) result = {} # First we always add the logs from journalctl / systemd result["journalctl"] = _get_journalctl_logs(name, number).splitlines() - for index, log_path in enumerate(log_list): - log_type = log_type_list[index] + for log_path in log_list: + # log is a file, read it + if not os.path.isdir(log_path): + result[log_path] = _tail(log_path, number) if os.path.exists(log_path) else [] + continue - if log_type == "file": - # log is a file, read it - if not os.path.isdir(log_path): - result[log_path] = _tail(log_path, number) if os.path.exists(log_path) else [] + for log_file in os.listdir(log_path): + log_file_path = os.path.join(log_path, log_file) + # not a file : skip + if not os.path.isfile(log_file_path): continue - for log_file in os.listdir(log_path): - log_file_path = os.path.join(log_path, log_file) - # not a file : skip - if not os.path.isfile(log_file_path): - continue + if not log_file.endswith(".log"): + continue - if not log_file.endswith(".log"): - continue - - result[log_file_path] = _tail(log_file_path, number) if os.path.exists(log_file_path) else [] - else: - # N.B. : this is legacy code that can probably be removed ... to be confirmed - # get log with journalctl - result[log_path] = _get_journalctl_logs(log_path, number).splitlines() + result[log_file_path] = _tail(log_file_path, number) if os.path.exists(log_file_path) else [] return result From 6fc5b413025c18f2da79af0acbfe917581e92b4a Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 6 May 2020 18:53:06 +0200 Subject: [PATCH 4/7] Add a few tests for services add/remove/status ... --- .gitlab-ci.yml | 6 ++ src/yunohost/tests/test_service.py | 97 ++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 src/yunohost/tests/test_service.py diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index ac3584630..3ebbaecd5 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -92,6 +92,12 @@ test-regenconf: - cd src/yunohost - py.test tests/test_regenconf.py +test-service: + extends: .test-stage + script: + - cd src/yunohost + - py.test tests/test_service.py + ######################################## # LINTER ######################################## diff --git a/src/yunohost/tests/test_service.py b/src/yunohost/tests/test_service.py new file mode 100644 index 000000000..d8660c1e5 --- /dev/null +++ b/src/yunohost/tests/test_service.py @@ -0,0 +1,97 @@ +import os + +from conftest import message, raiseYunohostError + +from yunohost.service import _get_services, _save_services, service_status, service_add, service_remove + + +def setup_function(function): + + clean() + + +def teardown_function(function): + + clean() + + +def clean(): + + # To run these tests, we assume ssh(d) service exists and is running + assert os.system("pgrep sshd >/dev/null") == 0 + + services = _get_services() + assert "ssh" in services + + if "dummyservice" in services: + del services["dummyservice"] + _save_services(services) + + +def test_service_status_all(): + + status = service_status() + assert "ssh" in status.keys() + assert status["ssh"]["status"] == "running" + + +def test_service_status_single(): + + status = service_status("ssh") + assert "status" in status.keys() + assert status["status"] == "running" + + +def test_service_status_unknown_service(mocker): + + with raiseYunohostError(mocker, 'service_unknown'): + service_status(["ssh", "doesnotexists"]) + + +def test_service_add(): + + service_add("dummyservice", description="A dummy service to run tests") + assert "dummyservice" in service_status().keys() + + +def test_service_remove(): + + service_add("dummyservice", description="A dummy service to run tests") + assert "dummyservice" in service_status().keys() + service_remove("dummyservice") + assert "dummyservice" not in service_status().keys() + + +def test_service_remove_service_that_doesnt_exists(mocker): + + assert "dummyservice" not in service_status().keys() + + with raiseYunohostError(mocker, 'service_unknown'): + service_remove("dummyservice") + + assert "dummyservice" not in service_status().keys() + + +def test_service_update_to_add_properties(): + + service_add("dummyservice", description="") + assert not _get_services()["dummyservice"].get("test_status") + service_add("dummyservice", description="", test_status="true") + assert _get_services()["dummyservice"].get("test_status") == "true" + + +def test_service_update_to_change_properties(): + + service_add("dummyservice", description="", test_status="false") + assert _get_services()["dummyservice"].get("test_status") == "false" + service_add("dummyservice", description="", test_status="true") + assert _get_services()["dummyservice"].get("test_status") == "true" + + +def test_service_update_to_remove_properties(): + + service_add("dummyservice", description="", test_status="false") + assert _get_services()["dummyservice"].get("test_status") == "false" + service_add("dummyservice", description="", test_status="") + assert not _get_services()["dummyservice"].get("test_status") + From 58a29f218e93b5537d7ac0858f909395a3c9fd19 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 6 May 2020 20:06:16 +0200 Subject: [PATCH 5/7] Update src/yunohost/service.py Co-authored-by: Kayou --- src/yunohost/service.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 1fe65c102..a818d9fbd 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -314,9 +314,8 @@ def service_status(names=[]): else: translation_key = "service_description_%s" % name - if "description" in infos is not None: - description = infos.get("description") - else: + description = infos.get("description") + if not description: description = m18n.n(translation_key) # that mean that we don't have a translation for this string From f25e07fd829cd179393762d5c23a6f9f2670ed1c Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Wed, 6 May 2020 20:18:49 +0200 Subject: [PATCH 6/7] Update src/yunohost/service.py --- src/yunohost/service.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index a818d9fbd..4a86043b3 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -427,8 +427,11 @@ def service_log(name, number=50): for log_path in log_list: # log is a file, read it - if not os.path.isdir(log_path): - result[log_path] = _tail(log_path, number) if os.path.exists(log_path) else [] + if os.path.isfile(log_path): + result[log_path] = _tail(log_path, number) + continue + elif not os.path.isdir(log_path): + result[log_path] = [] continue for log_file in os.listdir(log_path): From 2f31cb6463c51a0cf6965f86dc5e3733aa3f5962 Mon Sep 17 00:00:00 2001 From: Alexandre Aubin Date: Thu, 7 May 2020 22:37:57 +0200 Subject: [PATCH 7/7] Make sure to handle symlinks when fetching logfiles --- src/yunohost/service.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/yunohost/service.py b/src/yunohost/service.py index 4a86043b3..f905d3906 100644 --- a/src/yunohost/service.py +++ b/src/yunohost/service.py @@ -426,6 +426,13 @@ def service_log(name, number=50): result["journalctl"] = _get_journalctl_logs(name, number).splitlines() for log_path in log_list: + + if not os.path.exists(log_path): + continue + + # Make sure to resolve symlinks + log_path = os.path.realpath(log_path) + # log is a file, read it if os.path.isfile(log_path): result[log_path] = _tail(log_path, number)