add pacemaker_resource plugin #9531

munchtoast · 2025-01-06T04:16:37Z

SUMMARY

Adds in pacemaker_resource plugin for managing pacemaker cluster resources.

ISSUE TYPE

New Module/Plugin Pull Request

COMPONENT NAME

pacemaker_resource

ADDITIONAL INFORMATION

russoz

Hi @munchtoast thanks for your contribution!

I have many comments on this PR, hope not to crush your motivation. We have all made these mistakes in the past, it is just a matter of learning. :-)

Also, please note that all new modules must have tests created with them (unit tests or integration tests).

In your case, since this is a module based on run_command(), you can easily write unit tests using the test helper I wrote. Take a look at the directory
https://github.com/ansible-collections/community.general/tree/main/tests/unit/plugins/modules take a look at the tests modules that have an accompanying YAML file. I am yet to write proper documentation for that helper, but its use should be quite straightforward. If you decide to follow this path, I will be happy to assist you with those tests.

plugins/modules/pacemaker_resource.py

russoz

Hi @munchtoast , still a draft, I know, but leaving a couple of comments nonetheless.

plugins/modules/pacemaker_resource.py

munchtoast · 2025-01-24T16:54:40Z

Hi @munchtoast , still a draft, I know, but leaving a couple of comments nonetheless.

Hi @russoz, thank you so much for your reviews! I am currently writing unit tests but running into issues. I'll push what I have but I'm sorry for slow delay in applying feedback. Just wanted to give you an update that my wip commits should come in by end of this weekend.

russoz · 2025-01-24T22:27:17Z

No worries, take your time and feel free to ask for help. We have all been there once 😉

munchtoast · 2025-01-27T14:33:36Z

Noticing that there may be an issue with the current implementation, where the create should at least include resource_type.resource_name, but is not required for delete as the name is sufficient. Not sure if there's a way to make this required if state: create...

Also I noticed in other .yaml unit tests that environ is set to &env-def {environ_update: {LANGUAGE: C, LC_ALL: C}, check_rc: true}. When I tried utilizing this in my unit test I am getting assertion errors on args list. Setting the environ to {} allows the unit test to pass but I'm not sure how I could use this anchor?

russoz · 2025-01-28T11:06:59Z

Also I noticed in other .yaml unit tests that environ is set to &env-def {environ_update: {LANGUAGE: C, LC_ALL: C}, check_rc: true}. When I tried utilizing this in my unit test I am getting assertion errors on args list. Setting the environ to {} allows the unit test to pass but I'm not sure how I could use this anchor?

Without seeing the specific error it's going to be hard to guess what the problem is. Could you please push the code generating that error so I can see the output here? Or if one of these runs above had that, please point it to me.

Couple of housekeeping comments:

I would suggest you rebase your branch, it is 140+ commits behind from main. There are no conflicts so it should be fairly simple, and it makes things slightly faster for git (and github).
There is no need to force push everytime, you can just do a normal push (if you change something through github's webUI you will need to git pull first, though).

russoz

Additionally, some comments on the code - there are some improvements that could be made.

plugins/module_utils/pacemaker.py

plugins/modules/pacemaker_resource.py

tests/unit/plugins/modules/test_pacemaker_resource.py

plugins/modules/pacemaker_resource.py

tests/unit/plugins/modules/test_pacemaker_resource.yaml

ansibullbot · 2025-01-28T14:14:33Z

@munchtoast this PR contains the following merge commits:

99f9439

Please rebase your branch to remove these commits.

click here for bot help

munchtoast · 2025-01-28T14:45:13Z

Also I noticed in other .yaml unit tests that environ is set to &env-def {environ_update: {LANGUAGE: C, LC_ALL: C}, check_rc: true}. When I tried utilizing this in my unit test I am getting assertion errors on args list. Setting the environ to {} allows the unit test to pass but I'm not sure how I could use this anchor?

Without seeing the specific error it's going to be hard to guess what the problem is. Could you please push the code generating that error so I can see the output here? Or if one of these runs above had that, please point it to me.

I don't think this is an error, but seems to be misunderstanding on my part as I am still learning. Other test .yaml utilize the CmdRunner's init function. My latest commit is an attempt to use that format, which should help in adding future pacemaker modules.

Couple of housekeeping comments:

I would suggest you rebase your branch, it is 140+ commits behind from main. There are no conflicts so it should be fairly simple, and it makes things slightly faster for git (and github).

There is no need to force push everytime, you can just do a normal push (if you change something through github's webUI you will need to git pull first, though).

Should be up to date now!

Refactorization on pacemaker_resource to utilize module helpers.

ansibullbot · 2025-01-28T22:15:42Z

The test botmeta failed with 2 errors:

.github/BOTMETA.yml:0:0: Author munchtoast not mentioned as active or inactive maintainer for plugins/modules/pacemaker_resource.py (mentioned are: )
.github/BOTMETA.yml:0:0: No (active or inactive) maintainer mentioned for plugins/modules/pacemaker_resource.py

The test ansible-test sanity --test pep8 [explain] failed with 11 errors:

plugins/module_utils/pacemaker.py:18:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:25:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:35:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:43:1: E302: expected 2 blank lines, found 1
plugins/modules/pacemaker_resource.py:192:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:193:15: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:194:17: E121: continuation line under-indented for hanging indent
plugins/modules/pacemaker_resource.py:195:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:196:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:197:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:214:161: E501: line too long (193 > 160 characters)

The test ansible-test sanity --test pep8 [explain] failed with 11 errors:

plugins/module_utils/pacemaker.py:18:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:25:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:35:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:43:1: E302: expected 2 blank lines, found 1
plugins/modules/pacemaker_resource.py:192:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:193:15: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:194:17: E121: continuation line under-indented for hanging indent
plugins/modules/pacemaker_resource.py:195:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:196:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:197:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:214:161: E501: line too long (193 > 160 characters)

The test ansible-test sanity --test pep8 [explain] failed with 11 errors:

plugins/module_utils/pacemaker.py:18:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:25:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:35:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:43:1: E302: expected 2 blank lines, found 1
plugins/modules/pacemaker_resource.py:192:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:193:15: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:194:17: E121: continuation line under-indented for hanging indent
plugins/modules/pacemaker_resource.py:195:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:196:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:197:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:214:161: E501: line too long (193 > 160 characters)

The test ansible-test sanity --test pep8 [explain] failed with 11 errors:

plugins/module_utils/pacemaker.py:18:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:25:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:35:1: E302: expected 2 blank lines, found 1
plugins/module_utils/pacemaker.py:43:1: E302: expected 2 blank lines, found 1
plugins/modules/pacemaker_resource.py:192:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:193:15: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:194:17: E121: continuation line under-indented for hanging indent
plugins/modules/pacemaker_resource.py:195:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:196:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:197:11: E111: indentation is not a multiple of 4
plugins/modules/pacemaker_resource.py:214:161: E501: line too long (193 > 160 characters)

click here for bot help

russoz · 2025-01-29T08:08:37Z

plugins/modules/pacemaker_resource.py

+          if fail_on_err and rc != 0 and err and ignore_err_msg not in err:
+              self.do_raise(
+                'pcs failed with error (rc={0}): {1}'.format(rc, err))
+          out = out.rstrip()
+          self.vars.value = None if out == "" else out
+          return self.vars.value


indentation should be 4, not 2:

Suggested change

if fail_on_err and rc != 0 and err and ignore_err_msg not in err:

self.do_raise(

'pcs failed with error (rc={0}): {1}'.format(rc, err))

out = out.rstrip()

self.vars.value = None if out == "" else out

return self.vars.value

if fail_on_err and rc != 0 and err and ignore_err_msg not in err:

self.do_raise('pcs failed with error (rc={0}): {1}'.format(rc, err))

out = out.rstrip()

self.vars.value = None if out == "" else out

return self.vars.value

russoz · 2025-01-29T08:16:33Z

plugins/modules/pacemaker_resource.py

+                argument_action=dict(type='str', choices=[
+                                   'clone', 'master', 'group', 'promotable']),


No need to break the line here

Suggested change

argument_action=dict(type='str', choices=[

'clone', 'master', 'group', 'promotable']),

argument_action=dict(type='str', choices=['clone', 'master', 'group', 'promotable']),

russoz · 2025-01-29T08:17:15Z

plugins/modules/pacemaker_resource.py

+            ctx.run(
+                state=self.vars.state,
+                name=self.vars.name,
+                resource_type=self.vars.resource_type,
+                resource_option=self.vars.resource_option,
+                resource_operation=self.vars.resource_operation,
+                resource_meta=self.vars.resource_meta,
+                resource_argument=self.vars.resource_argument,
+                disabled=self.vars.disabled,
+                wait=self.vars.wait)


and since they all seem to be module params, you can also do:

Suggested change

ctx.run(

state=self.vars.state,

name=self.vars.name,

resource_type=self.vars.resource_type,

resource_option=self.vars.resource_option,

resource_operation=self.vars.resource_operation,

resource_meta=self.vars.resource_meta,

resource_argument=self.vars.resource_argument,

disabled=self.vars.disabled,

wait=self.vars.wait)

ctx.run()

russoz · 2025-01-29T08:23:37Z

plugins/modules/pacemaker_resource.py

+  - This module can manage resources to a pacemaker cluster from Ansible using
+    the pacemaker cli.


More of a style recommendation, although capitalization is usually required.

Suggested change

- This module can manage resources to a pacemaker cluster from Ansible using

the pacemaker cli.

- This module can manage resources in a Pacemaker cluster using the pacemaker CLI.

russoz · 2025-01-29T08:28:46Z

plugins/modules/pacemaker_resource.py

+              self.do_raise(
+                'pcs failed with error (rc={0}): {1}'.format(rc, err))


No need to break:

Suggested change

self.do_raise(

'pcs failed with error (rc={0}): {1}'.format(rc, err))

self.do_raise('pcs failed with error (rc={0}): {1}'.format(rc, err))

russoz · 2025-01-29T08:36:18Z

Also I noticed in other .yaml unit tests that environ is set to &env-def {environ_update: {LANGUAGE: C, LC_ALL: C}, check_rc: true}. When I tried utilizing this in my unit test I am getting assertion errors on args list. Setting the environ to {} allows the unit test to pass but I'm not sure how I could use this anchor?

Without seeing the specific error it's going to be hard to guess what the problem is. Could you please push the code generating that error so I can see the output here? Or if one of these runs above had that, please point it to me.

I don't think this is an error, but seems to be misunderstanding on my part as I am still learning. Other test .yaml utilize the CmdRunner's init function. My latest commit is an attempt to use that format, which should help in adding future pacemaker modules.

I was referring to the assertion errors you mentioned. It is looking so much better now - we still have a couple of sanity check errors to clear, then after that work our way through these tests. ;-)

This comment was marked as outdated.

Sign in to view

ansibullbot added WIP Work in progress ci_verified Push fixes to PR branch to re-run CI module module new_contributor Help guide this first time contributor plugins plugin (any type) labels Jan 6, 2025

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-10 Automatically create a backport for the stable-10 branch labels Jan 6, 2025

munchtoast force-pushed the main branch 2 times, most recently from 3ced6e7 to 1c0286e Compare January 9, 2025 15:51

russoz reviewed Jan 10, 2025

View reviewed changes

russoz reviewed Jan 18, 2025

View reviewed changes

plugins/modules/pacemaker_resource.py Outdated Show resolved Hide resolved

plugins/modules/pacemaker_resource.py Outdated Show resolved Hide resolved

plugins/modules/pacemaker_resource.py Outdated Show resolved Hide resolved

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 24, 2025

ansibullbot added new_plugin New plugin tests tests unit tests/unit and removed ci_verified Push fixes to PR branch to re-run CI stale_ci CI is older than 7 days, rerun before merging labels Jan 27, 2025

This comment was marked as resolved.

Sign in to view

ansibullbot added the module_utils module_utils label Jan 27, 2025

russoz reviewed Jan 28, 2025

View reviewed changes

ansibullbot added merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 28, 2025

Add initial pacemaker resource

1fbf072

munchtoast added 9 commits January 28, 2025 09:47

Additional fixes on pacemaker_resource

0f31fe2

Fix up module parameters

fa2a119

fix doc and lint

9bda0b2

fix group command build

5c34bf3

Apply suggestions for removing status and improve descriptions

752fb6c

fix cmd builder list

0715d6e

Apply suggestions and add initial unit tests

f55b5c9

Fix unit tests expected output

387b0ce

Initial refactor on pacemaker resource

8cf1371

Refactorization on pacemaker_resource to utilize module helpers.

munchtoast force-pushed the main branch from 99f9439 to 8cf1371 Compare January 28, 2025 14:48

ansibullbot removed merge_commit This PR contains at least one merge commit. Please resolve! needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html labels Jan 28, 2025

Apply suggestions and fix up initial unit test

5f5e291

ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jan 28, 2025

russoz reviewed Jan 29, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add pacemaker_resource plugin #9531

add pacemaker_resource plugin #9531

munchtoast commented Jan 6, 2025

This comment was marked as outdated.

russoz left a comment

russoz left a comment

munchtoast commented Jan 24, 2025

russoz commented Jan 24, 2025

munchtoast commented Jan 27, 2025

This comment was marked as resolved.

russoz commented Jan 28, 2025

russoz left a comment

ansibullbot commented Jan 28, 2025

munchtoast commented Jan 28, 2025

ansibullbot commented Jan 28, 2025

russoz Jan 29, 2025

russoz Jan 29, 2025

russoz Jan 29, 2025

russoz Jan 29, 2025

russoz Jan 29, 2025

russoz commented Jan 29, 2025

		argument_action=dict(type='str', choices=[
		'clone', 'master', 'group', 'promotable']),

		- This module can manage resources to a pacemaker cluster from Ansible using
		the pacemaker cli.

	- This module can manage resources to a pacemaker cluster from Ansible using
	the pacemaker cli.
	- This module can manage resources in a Pacemaker cluster using the pacemaker CLI.

		self.do_raise(
		'pcs failed with error (rc={0}): {1}'.format(rc, err))

	self.do_raise(
	'pcs failed with error (rc={0}): {1}'.format(rc, err))
	self.do_raise('pcs failed with error (rc={0}): {1}'.format(rc, err))

add pacemaker_resource plugin #9531

Are you sure you want to change the base?

add pacemaker_resource plugin #9531

Conversation

munchtoast commented Jan 6, 2025

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

This comment was marked as outdated.

russoz left a comment

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

munchtoast commented Jan 24, 2025

russoz commented Jan 24, 2025

munchtoast commented Jan 27, 2025

This comment was marked as resolved.

russoz commented Jan 28, 2025

russoz left a comment

Choose a reason for hiding this comment

ansibullbot commented Jan 28, 2025

munchtoast commented Jan 28, 2025

ansibullbot commented Jan 28, 2025

russoz Jan 29, 2025

Choose a reason for hiding this comment

russoz Jan 29, 2025

Choose a reason for hiding this comment

russoz Jan 29, 2025

Choose a reason for hiding this comment

russoz Jan 29, 2025

Choose a reason for hiding this comment

russoz Jan 29, 2025

Choose a reason for hiding this comment

russoz commented Jan 29, 2025