From 2e7404bf21dc8389d2cf567e255769ae84fb72ce Mon Sep 17 00:00:00 2001 From: Teddy Andrieux Date: Wed, 27 Jan 2021 17:49:52 +0100 Subject: [PATCH] salt: Add a check about conflicting services for MetalK8s If some service are started on the host where we want to deploy MetalK8s the installation may not work properly (e.g.: firewalld) Add a new function to check that those service are not started on the host before deploying all the MetalK8s components. NOTE: We do not automatically stop the service from the host since those services may have been started for good reason, so just ask the user to remove those packages Fixes: #3067 --- salt/_modules/metalk8s_checks.py | 45 ++++++++++++ salt/metalk8s/defaults.yaml | 4 + salt/metalk8s/orchestrate/deploy_node.sls | 9 ++- .../modules/files/test_metalk8s_checks.yaml | 73 +++++++++++++++++++ .../unit/modules/test_metalk8s_checks.py | 35 ++++++++- 5 files changed, 161 insertions(+), 5 deletions(-) diff --git a/salt/_modules/metalk8s_checks.py b/salt/_modules/metalk8s_checks.py index 85978095f4..ff7b914f17 100644 --- a/salt/_modules/metalk8s_checks.py +++ b/salt/_modules/metalk8s_checks.py @@ -26,6 +26,11 @@ def node(raises=True, **kwargs): if pkg_ret is not True: errors.append(pkg_ret) + # Run `services` check + svc_ret = __salt__['metalk8s_checks.services'](raises=False, **kwargs) + if svc_ret is not True: + errors.append(svc_ret) + # Compute return of the function if errors: error_msg = 'Node {}: {}'.format(__grains__['id'], '\n'.join(errors)) @@ -99,6 +104,46 @@ def packages(conflicting_packages=None, raises=True, **kwargs): return error_msg or True +def services(conflicting_services=None, raises=True, **kwargs): + """Check if some conflicting service are started on the machine, + return a string (or raise if `raises` is set to `True`) with the list of + conflicting services. + + Arguments: + conflicting_services (list): override the list of service that conflict with + MetalK8s installation. + raises (bool): the method will raise if there is any conflicting service + started. + + Note: We have some logic in this function, so `conflicting_services` could be: + - a single string `` which is the name of the conflicting service + - a list `[, ]` with all conflicting service name + """ + if conflicting_services is None: + conflicting_services = __salt__['metalk8s.get_from_map']( + 'defaults', saltenv=kwargs.get('saltenv') + )['conflicting_services'] + + if isinstance(conflicting_services, str): + conflicting_services = [conflicting_services] + errors = [] + + for service_name in conflicting_services: + # True = service started + # False = service not available or stopped + if __salt__['service.status'](service_name): + errors.append( + "Service {} conflicts with MetalK8s installation, " + "please stop it.".format(service_name) + ) + + error_msg = '\n'.join(errors) + if error_msg and raises: + raise CheckError(error_msg) + + return error_msg or True + + def sysctl(params, raises=True): """Check if the given sysctl key-values match the ones in memory and return a string (or raise if `raises` is set to `True`) with the list diff --git a/salt/metalk8s/defaults.yaml b/salt/metalk8s/defaults.yaml index 6e42f157f4..1c2ff73fac 100644 --- a/salt/metalk8s/defaults.yaml +++ b/salt/metalk8s/defaults.yaml @@ -36,6 +36,10 @@ kubeadm_preflight: - iproute # provides tc - coreutils # provides touch +# List of services that conflict with MetalK8s installation +conflicting_services: + - firewalld + repo: conflicting_packages: # List of package that conflict with MetalK8s installation diff --git a/salt/metalk8s/orchestrate/deploy_node.sls b/salt/metalk8s/orchestrate/deploy_node.sls index 45024e511e..8d5d8763c1 100644 --- a/salt/metalk8s/orchestrate/deploy_node.sls +++ b/salt/metalk8s/orchestrate/deploy_node.sls @@ -1,3 +1,4 @@ +{%- from "metalk8s/map.jinja" import defaults with context %} {%- from "metalk8s/map.jinja" import repo with context %} {%- set node_name = pillar.orchestrate.node_name %} @@ -26,12 +27,14 @@ Check node: - ssh: true - roster: kubernetes - kwarg: - # NOTE: We need to use the `conflicting_packages` from the salt - # master since in salt-ssh when running an execution module we cannot - # embbed additional files (especially `map.jinja` in this case) + # NOTE: We need to use the `conflicting_packages` and `conflicting_services` + # from the salt master since in salt-ssh when running an execution module + # we cannot embbed additional files (especially `map.jinja` in this case) # Sees: https://github.com/saltstack/salt/issues/59314 conflicting_packages: >- {{ repo.conflicting_packages | tojson }} + conflicting_services: >- + {{ defaults.conflicting_services | tojson }} - failhard: true - require: - metalk8s: Install python36 diff --git a/salt/tests/unit/modules/files/test_metalk8s_checks.yaml b/salt/tests/unit/modules/files/test_metalk8s_checks.yaml index 9cfcb628fe..9f2567304a 100644 --- a/salt/tests/unit/modules/files/test_metalk8s_checks.yaml +++ b/salt/tests/unit/modules/files/test_metalk8s_checks.yaml @@ -1,13 +1,34 @@ node: + # 1. Everything fine - packages_ret: True + services_ret: True result: True + + # 2. Error with packages - packages_ret: "Package abcd got an error because of banana" + services_ret: True expect_raise: True result: "Node my_node_1: Package abcd got an error because of banana" + # 2. bis (no raises) - packages_ret: "Package toto got an error :)" + services_ret: True raises: False result: "Node my_node_1: Package toto got an error :)" + # 3. Error with services + - packages_ret: True + services_ret: "Service abcd got an error because of penguin" + expect_raise: True + result: "Node my_node_1: Service abcd got an error because of penguin" + + # 4. Error with services and packages + - packages_ret: "Package abcd got an error because of banana" + services_ret: "Service abcd got an error because of penguin" + expect_raise: True + result: |- + Node my_node_1: Package abcd got an error because of banana + Service abcd got an error because of penguin + packages: # 1. Success: No conflicting packages to check - conflicting_packages: {} @@ -155,6 +176,58 @@ packages: result: |- Package my-installed-package-4.5.6 conflicts with MetalK8s installation, please remove it. +services: + # 1. Success: No conflicting services to check + - conflicting_services: [] + result: True + + # 2. Success: Conflicting services not installed + - conflicting_services: + - my-not-started-service + - my-not-started-service2 + service_status_ret: + my-not-started-service: False + my-not-started-service2: False + result: True + # 2. bis + - conflicting_services: my-not-started-service + service_status_ret: + my-not-started-service: False + result: True + + # 3. Success: Conflicting service (from map) not started + - get_map_ret: + conflicting_services: + - my-not-started-service + service_status_ret: + my-not-started-service: False + result: True + + # 4. Error: Conflicting service started + - conflicting_services: + - my-started-service + - my-not-started-service + service_status_ret: + my-started-service: True + my-not-started-service: False + expect_raise: True + result: |- + Service my-started-service conflicts with MetalK8s installation, please stop it. + # 4. bis + - conflicting_services: my-started-service + service_status_ret: + my-started-service: True + expect_raise: True + result: |- + Service my-started-service conflicts with MetalK8s installation, please stop it. + # 5. ter (no raise) + - conflicting_services: my-started-service + raises: False + service_status_ret: + my-started-service: True + result: |- + Service my-started-service conflicts with MetalK8s installation, please stop it. + sysctl: - params: net.ipv4.ip_forward: 1 diff --git a/salt/tests/unit/modules/test_metalk8s_checks.py b/salt/tests/unit/modules/test_metalk8s_checks.py index 695510c611..59ceb075ac 100644 --- a/salt/tests/unit/modules/test_metalk8s_checks.py +++ b/salt/tests/unit/modules/test_metalk8s_checks.py @@ -33,14 +33,17 @@ def test_virtual(self): self.assertEqual(metalk8s_checks.__virtual__(), 'metalk8s_checks') @utils.parameterized_from_cases(YAML_TESTS_CASES["node"]) - def test_node(self, packages_ret, result, expect_raise=False, **kwargs): + def test_node(self, packages_ret, services_ret, result, + expect_raise=False, **kwargs): """ Tests the return of `node` function """ packages_mock = MagicMock(return_value=packages_ret) + services_mock = MagicMock(return_value=services_ret) salt_dict = { - 'metalk8s_checks.packages': packages_mock + 'metalk8s_checks.packages': packages_mock, + 'metalk8s_checks.services': services_mock } with patch.dict(metalk8s_checks.__grains__, {'id': 'my_node_1'}), \ @@ -86,6 +89,34 @@ def test_packages(self, result, get_map_ret=None, list_pkgs_ret=None, result ) + @utils.parameterized_from_cases(YAML_TESTS_CASES["services"]) + def test_services(self, result, get_map_ret=None, service_status_ret=None, + expect_raise=False, **kwargs): + """ + Tests the return of `services` function + """ + get_map_mock = MagicMock(return_value=get_map_ret) + service_status_mock = MagicMock(side_effect=(service_status_ret or {}).get) + + salt_dict = { + 'metalk8s.get_from_map': get_map_mock, + 'service.status': service_status_mock + } + + with patch.dict(metalk8s_checks.__salt__, salt_dict): + if expect_raise: + self.assertRaisesRegex( + CheckError, + result, + metalk8s_checks.services, + **kwargs + ) + else: + self.assertEqual( + metalk8s_checks.services(**kwargs), + result + ) + @utils.parameterized_from_cases(YAML_TESTS_CASES["sysctl"]) def test_sysctl(self, params, data, result, raises=False): """