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!

russoz · 2025-02-19T18:41:45Z

plugins/modules/pacemaker_resource.py

+
+    def __init_module__(self):
+        self.runner = pacemaker_runner(self.module, cli_action='resource')
+        self.does_not = "Error: resource or tag id '{0}' not found".format(


Is this attribute being used? If not, then remove.

plugins/modules/pacemaker_resource.py

russoz

Hi @munchtoast

I took abother look and spotted a couple of things more. I know this can be tiring and even frustrating and I want to thank you for your resilience.

See comments below. Thx

russoz · 2025-02-19T23:02:34Z

tests/unit/plugins/modules/test_pacemaker_resource.yaml

+anchors:
+  environ: &env-def {environ_update: {LANGUAGE: C, LC_ALL: C}, check_rc: false}
+test_cases:
+- id: test_missing_input


please increase the indentation by 2 spaces for this list - not only as it is kinda becoming our convention in the collection, but also for consistency (see line 22 for example)

russoz · 2025-02-19T23:03:29Z

tests/unit/plugins/modules/test_pacemaker_resource.yaml

+    value: "  * virtual-ip\t(ocf:heartbeat:IPAddr2):\t Started"
+  mocks:
+    run_command:
+    - command: ["/testbin/pcs", resource, status, virtual-ip]


minor nit, not blocking anything but just for the record, those quotes are not needed:

Suggested change

- command: ["/testbin/pcs", resource, status, virtual-ip]

- command: [/testbin/pcs, resource, status, virtual-ip]

russoz · 2025-02-19T23:07:57Z

plugins/modules/pacemaker_resource.py

+    change_params = ('name', )
+    diff_params = ('name', )


I missed this before, sorry, but I don't think the value of name will change, will it?

russoz · 2025-02-19T23:10:05Z

plugins/modules/pacemaker_resource.py

+        self.vars.set('_value', self.vars.previous_value, output=False, change=True)
+        self.vars.set_meta('value', initial_value=self.vars.previous_value)


while on the other hand, value of value is likely going to change, so maybe you add change=True, diff=True to it...

... and then you can ditch the variable _value, as you would be tracking change directly in the variable value.

russoz · 2025-02-19T23:21:26Z

plugins/modules/pacemaker_resource.py

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


This is something else that I missed - within this tiny little function (defined within another function), you're making a change to the state of an attribute of another attribute of the class. It is not a huge problem in this context, so I will not press on you changing this, but just for the record, one thing that is usually important in software engineering is separation of concerns. This function should not be concerned about the object, its attribute, and the attribute of its attribute. There are probably better ways to explain that - I am not much of an academic - but making that reference here kinda breaks that separation. What you usually want to do is to have smaller pieces specialized in doing one thing and one thing only. In this case, I would write it as:

Suggested change

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

return self.vars.value

return None if out == "" else out

and later, when calling ctx.run() (line 193 for example), you would do:

self.vars.value = ctx.run()

you might argue that you are saving for attributing the variable once instead of four times, but that pays off in readability, maintainability. Five years from now, someone (maybe even yourself, if you stay away from the module for a while), will read the state_absent() method and will have no idea that self.vars.value is altered at all. It is not obvious. Only if you read the entire thing, paying attention to the details, that will become evident. Sometimes it is better to write more and read/understand quicker than the other way around. ;-) my $0.02

Again, it is not a major issue in the context of a single Ansible module, but I try to make things better even in small situations. Feel free to reject the suggestion. :-)

Thank you for the explanation! I pushed in a new commit which should remove unintended modification on self.vars.value, and uses the proposed self.vars.value = ctx.run(). To pass with the current unit tests, I pushed this as replacement:

self.vars.value = ctx.run() self.vars.set('value', self._get())

...Which should pass the unit tests but I'm not sure if this is too redundant? I could maybe remove the self.vars.set('value', self._get()), though it would be more definitive in terms of resource status.

Thoughts on this current approach?

russoz · 2025-02-19T23:25:22Z

plugins/modules/pacemaker_resource.py

+            self.vars.stdout = ctx.results_out
+            self.vars.stderr = ctx.results_err
+            self.vars.cmd = ctx.cmd
+        self.vars.set('new_value', self._get(), fact=True)


whoa, and I missed that one before as well: I don't think we should be generating ansible_facts from this. Any particular reason to be using that?

Cc: @felixfontein

I agree, modules should never return ansible_facts except in very special situations. I don't think this module is one of these exceptions. Can you explain why this module should return facts?

Thank you for the catch! The ansible_facts should not be returned here, and was a mistake on my end. This should be removed in a new commit.

russoz · 2025-02-24T06:28:15Z

plugins/modules/pacemaker_resource.py

+short_description: Manage pacemaker resources
+author:
+  - Dexter Le (@munchtoast)
+version_added: 10.4.0


10.4.0 has been released, please adjust this to:

Suggested change

version_added: 10.4.0

version_added: 10.5.0

Refactorization on pacemaker_resource to utilize module helpers.

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

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

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

russoz reviewed Feb 19, 2025

View reviewed changes

felixfontein reviewed Feb 19, 2025

View reviewed changes

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

russoz reviewed Feb 20, 2025

View reviewed changes

russoz reviewed Feb 24, 2025

View reviewed changes

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 24, 2025

munchtoast and others added 23 commits February 24, 2025 14:41

Add initial pacemaker resource

c53ade6

Additional fixes on pacemaker_resource

bef7960

Fix up module parameters

928d091

fix doc and lint

74dd6f3

fix group command build

898a3c5

Apply suggestions for removing status and improve descriptions

5730a7b

fix cmd builder list

18cdc5d

Apply suggestions and add initial unit tests

7714dbb

Fix unit tests expected output

8f57c20

Initial refactor on pacemaker resource

ba7e2d7

Refactorization on pacemaker_resource to utilize module helpers.

Apply suggestions and fix up initial unit test

fadd5dc

Apply suggestions from code review

b08c941

Fix pep8 format for utils

7c47616

Fix unit tests for pacemaker resource

39efba2

Add botmeta maintainers for new moduules

95ccecf

Apply suggestions from code review

d2e793e

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Apply unit test suggestion

f1eeb17

Add disable and enable states for pacemaker_resource

b01a323

Fix state names and add cli_action for runner

e4db50e

Remove unnecessary variables

c9779c9

Fix documentation example playbook

ce3a3ba

Fix IP Address for resource_option

cd9729b

Refactor and remove unnecessary facts

38b1791

munchtoast force-pushed the main branch from 7fba271 to 38b1791 Compare February 24, 2025 19:42

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

russoz Feb 19, 2025

russoz left a comment

russoz Feb 19, 2025

russoz Feb 19, 2025

russoz Feb 19, 2025

russoz Feb 19, 2025

russoz Feb 19, 2025

munchtoast Feb 24, 2025

russoz Feb 19, 2025

felixfontein Feb 20, 2025

munchtoast Feb 24, 2025

russoz Feb 24, 2025

	- command: ["/testbin/pcs", resource, status, virtual-ip]
	- command: [/testbin/pcs, resource, status, virtual-ip]

		self.vars.set('_value', self.vars.previous_value, output=False, change=True)
		self.vars.set_meta('value', initial_value=self.vars.previous_value)

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

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

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

Choose a reason for hiding this comment

russoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment