jenkins_node: Add set offline message (#9084)

* jenkins_node: Add set offline message

* Implement offline_message parameter for updating a Jenkins node
  offline cause reason when the state is "disabled" (offline).

* Fix enabled, disable and absent node state redirect authorization
  issues, same as was present for present.

* Add unit tests for redirect authorization workarounds.

* * Make docs clearer re: offline_message behaviour

* Exit with fail_json() instead of raising when
  create/delete/enable/disable node fail.

* * Add changelog fragment

* Update changelog fragments.

---------

Co-authored-by: Felix Fontein <felix@fontein.de>

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).

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,6 +299,7 @@ class JenkinsNode:
 
             created = True
 
+        if configure:
             self.configure_node(present)
 
         self.result['created'] = 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)
+            def enable_node():
-
+                try:
-            if info['offline']:
-                if not self.module.check_mode:
                     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()
+        def get_offline_info():
-
-        disabled = False
-
-        if present:
             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,
     )

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 = "<slave />"
+    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 = "<slave />"
+    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 = "<slave />"
@@ -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 = "<slave />"
+    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 = "<slave />"
+    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 = "<slave />"
@@ -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 = "<slave />"
+    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 = "<slave />"
+    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 = "<slave />"
@@ -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 = "<slave />"
-    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 = "<slave />"
+    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 = "<slave />"
+    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 = "<slave />"
-    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 = "<slave />"
-    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 = "<slave />"
+    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 = "<slave />"
+    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 = "<slave />"
+    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