diff --git a/changelogs/fragments/9084-jenkins_node-add-offline-message.yml b/changelogs/fragments/9084-jenkins_node-add-offline-message.yml new file mode 100644 index 0000000000..3718127513 --- /dev/null +++ b/changelogs/fragments/9084-jenkins_node-add-offline-message.yml @@ -0,0 +1,8 @@ +minor_changes: + - jenkins_node - add ``offline_message`` parameter for updating a Jenkins node offline cause reason when the state is "disabled" (offline) (https://github.com/ansible-collections/community.general/pull/9084)." + +bugfixes: + - jenkins_node - fixed ``enabled``, ``disable`` and ``absent`` node state redirect authorization issues, same as was present for ``present`` (https://github.com/ansible-collections/community.general/pull/9084). + +known_issues: + - jenkins_node - the module is not able to update offline message when node is already offline due to internally using toggleOffline API (https://github.com/ansible-collections/community.general/pull/9084). diff --git a/plugins/modules/jenkins_node.py b/plugins/modules/jenkins_node.py index 2ee4a481a5..9406eab4c5 100644 --- a/plugins/modules/jenkins_node.py +++ b/plugins/modules/jenkins_node.py @@ -64,6 +64,17 @@ options: - When specified, sets the Jenkins node labels. type: list elements: str + offline_message: + description: + - Specifies the offline reason message to be set when configuring the Jenkins node + state. + - If O(offline_message) is given and requested O(state) is not V(disabled), an + error will be raised. + - Internally O(offline_message) is set via the V(toggleOffline) API, so updating + the message when the node is already offline (current state V(disabled)) is not + possible. In this case, a warning will be issued. + type: str + version_added: 10.0.0 ''' EXAMPLES = ''' @@ -89,6 +100,13 @@ EXAMPLES = ''' - label-1 - label-2 - label-3 + +- name: Set Jenkins node offline with offline message. + community.general.jenkins_node: + name: my-node + state: disabled + offline_message: > + This node is offline for some reason. ''' RETURN = ''' @@ -136,7 +154,8 @@ configured: ''' import sys -from xml.etree import ElementTree +import traceback +from xml.etree import ElementTree as et from ansible.module_utils.basic import AnsibleModule from ansible.module_utils.common.text.converters import to_native @@ -164,6 +183,13 @@ class JenkinsNode: self.url = module.params['url'] self.num_executors = module.params['num_executors'] self.labels = module.params['labels'] + self.offline_message = module.params['offline_message'] # type: str | None + + if self.offline_message is not None: + self.offline_message = self.offline_message.strip() + + if self.state != "disabled": + self.module.fail_json("can not set offline message when state is not disabled") if self.labels is not None: for label in self.labels: @@ -207,12 +233,12 @@ class JenkinsNode: configured = False data = self.instance.get_node_config(self.name) - root = ElementTree.fromstring(data) + root = et.fromstring(data) if self.num_executors is not None: elem = root.find('numExecutors') if elem is None: - elem = ElementTree.SubElement(root, 'numExecutors') + elem = et.SubElement(root, 'numExecutors') if elem.text is None or int(elem.text) != self.num_executors: elem.text = str(self.num_executors) configured = True @@ -220,7 +246,7 @@ class JenkinsNode: if self.labels is not None: elem = root.find('label') if elem is None: - elem = ElementTree.SubElement(root, 'label') + elem = et.SubElement(root, 'label') labels = [] if elem.text: labels = elem.text.split() @@ -230,9 +256,9 @@ class JenkinsNode: if configured: if IS_PYTHON_2: - data = ElementTree.tostring(root) + data = et.tostring(root) else: - data = ElementTree.tostring(root, encoding="unicode") + data = et.tostring(root, encoding="unicode") self.instance.reconfig_node(self.name, data) @@ -240,16 +266,24 @@ class JenkinsNode: if configured: self.result['changed'] = True - def present_node(self): + def present_node(self, configure=True): # type: (bool) -> bool + """Assert node present. + + Args: + configure: If True, run node configuration after asserting node present. + + Returns: + True if the node is present, False otherwise (i.e. is check mode). + """ def create_node(): try: self.instance.create_node(self.name, launcher=jenkins.LAUNCHER_SSH) except jenkins.JenkinsException as e: # Some versions of python-jenkins < 1.8.3 has an authorization bug when - # handling redirects returned when posting new resources. If the node is + # handling redirects returned when posting to resources. If the node is # created OK then can ignore the error. if not self.instance.node_exists(self.name): - raise e + self.module.fail_json(msg="Create node failed: %s" % to_native(e), exception=traceback.format_exc()) # TODO: Remove authorization workaround. self.result['warnings'].append( @@ -265,7 +299,8 @@ class JenkinsNode: created = True - self.configure_node(present) + if configure: + self.configure_node(present) self.result['created'] = created if created: @@ -279,10 +314,10 @@ class JenkinsNode: self.instance.delete_node(self.name) except jenkins.JenkinsException as e: # Some versions of python-jenkins < 1.8.3 has an authorization bug when - # handling redirects returned when posting new resources. If the node is + # handling redirects returned when posting to resources. If the node is # deleted OK then can ignore the error. if self.instance.node_exists(self.name): - raise e + self.module.fail_json(msg="Delete node failed: %s" % to_native(e), exception=traceback.format_exc()) # TODO: Remove authorization workaround. self.result['warnings'].append( @@ -302,16 +337,36 @@ class JenkinsNode: self.result['changed'] = True def enabled_node(self): + def get_offline(): # type: () -> bool + return self.instance.get_node_info(self.name)["offline"] + present = self.present_node() enabled = False if present: - info = self.instance.get_node_info(self.name) - - if info['offline']: - if not self.module.check_mode: + def enable_node(): + try: self.instance.enable_node(self.name) + except jenkins.JenkinsException as e: + # Some versions of python-jenkins < 1.8.3 has an authorization bug when + # handling redirects returned when posting to resources. If the node is + # disabled OK then can ignore the error. + offline = get_offline() + + if offline: + self.module.fail_json(msg="Enable node failed: %s" % to_native(e), exception=traceback.format_exc()) + + # TODO: Remove authorization workaround. + self.result['warnings'].append( + "suppressed 401 Not Authorized on redirect after node enabled: see https://review.opendev.org/c/jjb/python-jenkins/+/931707" + ) + + offline = get_offline() + + if offline: + if not self.module.check_mode: + enable_node() enabled = True else: @@ -326,18 +381,63 @@ class JenkinsNode: self.result['changed'] = True def disabled_node(self): - present = self.present_node() - - disabled = False - - if present: + def get_offline_info(): info = self.instance.get_node_info(self.name) - if not info['offline']: + offline = info["offline"] + offline_message = info["offlineCauseReason"] + + return offline, offline_message + + # Don't configure until after disabled, in case the change in configuration + # causes the node to pick up a job. + present = self.present_node(False) + + disabled = False + changed = False + + if present: + offline, offline_message = get_offline_info() + + if self.offline_message is not None and self.offline_message != offline_message: + if offline: + # n.b. Internally disable_node uses toggleOffline gated by a not + # offline condition. This means that disable_node can not be used to + # update an offline message if the node is already offline. + # + # Toggling the node online to set the message when toggling offline + # again is not an option as during this transient online time jobs + # may be scheduled on the node which is not acceptable. + self.result["warnings"].append( + "unable to change offline message when already offline" + ) + else: + offline_message = self.offline_message + changed = True + + def disable_node(): + try: + self.instance.disable_node(self.name, offline_message) + except jenkins.JenkinsException as e: + # Some versions of python-jenkins < 1.8.3 has an authorization bug when + # handling redirects returned when posting to resources. If the node is + # disabled OK then can ignore the error. + offline, _offline_message = get_offline_info() + + if not offline: + self.module.fail_json(msg="Disable node failed: %s" % to_native(e), exception=traceback.format_exc()) + + # TODO: Remove authorization workaround. + self.result['warnings'].append( + "suppressed 401 Not Authorized on redirect after node disabled: see https://review.opendev.org/c/jjb/python-jenkins/+/931707" + ) + + if not offline: if not self.module.check_mode: - self.instance.disable_node(self.name) + disable_node() disabled = True + else: # Would have created node with initial state enabled therefore would have # needed to disable therefore disabled. @@ -345,10 +445,16 @@ class JenkinsNode: raise Exception("disabled_node present is False outside of check mode") disabled = True - self.result['disabled'] = disabled if disabled: + changed = True + + self.result['disabled'] = disabled + + if changed: self.result['changed'] = True + self.configure_node(present) + def main(): module = AnsibleModule( @@ -360,6 +466,7 @@ def main(): state=dict(choices=['enabled', 'disabled', 'present', 'absent'], default='present'), num_executors=dict(type='int'), labels=dict(type='list', elements='str'), + offline_message=dict(type='str'), ), supports_check_mode=True, ) diff --git a/tests/unit/plugins/modules/test_jenkins_node.py b/tests/unit/plugins/modules/test_jenkins_node.py index 33e7ca0f13..7c2634744d 100644 --- a/tests/unit/plugins/modules/test_jenkins_node.py +++ b/tests/unit/plugins/modules/test_jenkins_node.py @@ -207,6 +207,47 @@ def test_state_present_when_absent_check_mode(get_instance, instance, state): assert result.value["changed"] is True +@mark.parametrize(["state"], [param(state) for state in PRESENT_STATES]) +def test_state_present_when_absent_redirect_auth_error_handled( + get_instance, instance, state +): + instance.node_exists.side_effect = [False, True] + instance.get_node_config.return_value = "" + instance.create_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": state, + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert instance.create_node.call_args == call("my-node", launcher=jenkins.LAUNCHER_SSH) + + assert result.value["created"] is True + assert result.value["changed"] is True + + +@mark.parametrize(["state"], [param(state) for state in PRESENT_STATES]) +def test_state_present_when_absent_other_error_raised(get_instance, instance, state): + instance.node_exists.side_effect = [False, False] + instance.get_node_config.return_value = "" + instance.create_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": state, + }) + + with raises(AnsibleFailJson) as result: + jenkins_node.main() + + assert instance.create_node.call_args == call("my-node", launcher=jenkins.LAUNCHER_SSH) + + assert "Create node failed" in str(result.value) + + def test_state_present_when_present(get_instance, instance): instance.node_exists.return_value = True instance.get_node_config.return_value = "" @@ -262,6 +303,43 @@ def test_state_absent_when_present_check_mode(get_instance, instance): assert result.value["changed"] is True +def test_state_absent_when_present_redirect_auth_error_handled(get_instance, instance): + instance.node_exists.side_effect = [True, False] + instance.get_node_config.return_value = "" + instance.delete_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": "absent", + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert instance.delete_node.call_args == call("my-node") + + assert result.value["deleted"] is True + assert result.value["changed"] is True + + +def test_state_absent_when_present_other_error_raised(get_instance, instance): + instance.node_exists.side_effect = [True, True] + instance.get_node_config.return_value = "" + instance.delete_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": "absent", + }) + + with raises(AnsibleFailJson) as result: + jenkins_node.main() + + assert instance.delete_node.call_args == call("my-node") + + assert "Delete node failed" in str(result.value) + + def test_state_absent_when_absent(get_instance, instance): instance.node_exists.return_value = False instance.get_node_config.return_value = "" @@ -319,6 +397,45 @@ def test_state_enabled_when_offline_check_mode(get_instance, instance): assert result.value["changed"] is True +def test_state_enabled_when_offline_redirect_auth_error_handled(get_instance, instance): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.side_effect = [{"offline": True}, {"offline": False}] + instance.enable_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": "enabled", + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert instance.enable_node.call_args == call("my-node") + + assert result.value["enabled"] is True + assert result.value["changed"] is True + + +def test_state_enabled_when_offline_other_error_raised(get_instance, instance): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.side_effect = [{"offline": True}, {"offline": True}] + instance.enable_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": "enabled", + }) + + with raises(AnsibleFailJson) as result: + jenkins_node.main() + + assert instance.enable_node.call_args == call("my-node") + + assert "Enable node failed" in str(result.value) + + def test_state_enabled_when_not_offline(get_instance, instance): instance.node_exists.return_value = True instance.get_node_config.return_value = "" @@ -341,7 +458,10 @@ def test_state_enabled_when_not_offline(get_instance, instance): def test_state_disabled_when_not_offline(get_instance, instance): instance.node_exists.return_value = True instance.get_node_config.return_value = "" - instance.get_node_info.return_value = {"offline": False} + instance.get_node_info.return_value = { + "offline": False, + "offlineCauseReason": "", + } set_module_args({ "name": "my-node", @@ -351,16 +471,78 @@ def test_state_disabled_when_not_offline(get_instance, instance): with raises(AnsibleExitJson) as result: jenkins_node.main() - assert instance.disable_node.call_args == call("my-node") + assert instance.disable_node.call_args == call("my-node", "") assert result.value["disabled"] is True assert result.value["changed"] is True +def test_state_disabled_when_not_offline_redirect_auth_error_handled( + get_instance, instance +): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.side_effect = [ + { + "offline": False, + "offlineCauseReason": "", + }, + { + "offline": True, + "offlineCauseReason": "", + }, + ] + instance.disable_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": "disabled", + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert instance.disable_node.call_args == call("my-node", "") + + assert result.value["disabled"] is True + assert result.value["changed"] is True + + +def test_state_disabled_when_not_offline_other_error_raised(get_instance, instance): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.side_effect = [ + { + "offline": False, + "offlineCauseReason": "", + }, + { + "offline": False, + "offlineCauseReason": "", + }, + ] + instance.disable_node.side_effect = jenkins.JenkinsException + + set_module_args({ + "name": "my-node", + "state": "disabled", + }) + + with raises(AnsibleFailJson) as result: + jenkins_node.main() + + assert instance.disable_node.call_args == call("my-node", "") + + assert "Disable node failed" in str(result.value) + + def test_state_disabled_when_not_offline_check_mode(get_instance, instance): instance.node_exists.return_value = True instance.get_node_config.return_value = "" - instance.get_node_info.return_value = {"offline": False} + instance.get_node_info.return_value = { + "offline": False, + "offlineCauseReason": "", + } set_module_args({ "name": "my-node", @@ -380,7 +562,10 @@ def test_state_disabled_when_not_offline_check_mode(get_instance, instance): def test_state_disabled_when_offline(get_instance, instance): instance.node_exists.return_value = True instance.get_node_config.return_value = "" - instance.get_node_info.return_value = {"offline": True} + instance.get_node_info.return_value = { + "offline": True, + "offlineCauseReason": "", + } set_module_args({ "name": "my-node", @@ -573,3 +758,86 @@ def test_configure_labels_fail_when_contains_space(get_instance, instance): jenkins_node.main() assert not instance.reconfig_node.called + + +@mark.parametrize(["state"], [param(state) for state in ["enabled", "present", "absent"]]) +def test_raises_error_if_offline_message_when_state_not_disabled(get_instance, instance, state): + set_module_args({ + "name": "my-node", + "state": state, + "offline_message": "This is a message...", + }) + + with raises(AnsibleFailJson): + jenkins_node.main() + + assert not instance.disable_node.called + + +def test_set_offline_message_when_equal(get_instance, instance): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.return_value = { + "offline": True, + "offlineCauseReason": "This is an old message...", + } + + set_module_args({ + "name": "my-node", + "state": "disabled", + "offline_message": "This is an old message...", + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert not instance.disable_node.called + + assert result.value["changed"] is False + + +def test_set_offline_message_when_not_equal_not_offline(get_instance, instance): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.return_value = { + "offline": False, + "offlineCauseReason": "This is an old message...", + } + + set_module_args({ + "name": "my-node", + "state": "disabled", + "offline_message": "This is a new message...", + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert instance.disable_node.call_args == call("my-node", "This is a new message...") + + assert result.value["changed"] is True + + +# Not calling disable_node when already offline seems like a sensible thing to do. +# However, we need to call disable_node to set the offline message, so check that +# we do even when already offline. +def test_set_offline_message_when_not_equal_offline(get_instance, instance): + instance.node_exists.return_value = True + instance.get_node_config.return_value = "" + instance.get_node_info.return_value = { + "offline": True, + "offlineCauseReason": "This is an old message...", + } + + set_module_args({ + "name": "my-node", + "state": "disabled", + "offline_message": "This is a new message...", + }) + + with raises(AnsibleExitJson) as result: + jenkins_node.main() + + assert not instance.disable_node.called + + assert result.value["changed"] is False