feat: rewrite snapshot.py as an Ansible module / add support for thin origins

[trgill]

Enhancement: rewrite snapshot.py as an Ansible module / add support for thin origins

Reason:
#43
#57

Result:
Referenced issues fixed

Issue Tracker Tickets (Jira or BZ if any):

[spetrosi]

This check won't work. It's impossible to check if include_role resulted in a change in Ansible. There is no proper way to check idempotency other than looking at the playbook output.

[trgill]

@richm added a boolean response from the module to indicate if any action was taken. Rich, please let me know what changes I should make.

[richm]

https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_general.html#creating-a-module
The module should return a dict. That dict should have a field called changed which is true if the module changed something or false otherwise.

[richm]

This check won't work. It's impossible to check if include_role resulted in a change in Ansible. There is no proper way to check idempotency other than looking at the playbook output.

But in this case, when the role calls this module, it uses register: snapshot_cmd, so the test playbook can check. Otherwise, you are correct - there is no way to get at the internal state. Several of the roles register the module output or otherwise use some sort of internal variable to indicate the changed state of the role for the purpose of testing for idempotency.

[spetrosi]

Do you plan to add python unit tests for this module?

[trgill]

Yes. I'll add some unit tests.

[spetrosi]

[citest]

[richm]

This should be an example of using the module directly, not using the role.
Note that using the module directly is not supported, but module scanning tools such as ansible-test and ansible-doc may throw an error if this documentation is not in the correct format.

[trgill]

Ok. The errors are a bit difficult to understand. I was following the example in the storage role. Currently I'm seeing a lot of errors for the docs: https://github.com/linux-system-roles/snapshot/actions/runs/9502379067/job/26190204498 - it looks like the storage role is using a different "-collection-version 1.78.2" vs the snapshot role is using "--collection-version 1.79.0"

[trgill]

I also tried following the examples in: https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_general.html

But had trouble with errors with that format too.

[trgill]

@richm is there a better way to pass snapshot_lvm_set?

I'd like it to be None if it is not set, but the way I've done it here I'm getting "{}\0" when I read it in the python.

[richm]

Then if you define snapshot_lvm_set as a list of dict in the module, you can just use

        snapshot_lvm_set: "{{ snapshot_lvm_set | d(omit) }}"

[trgill]

So, on the module side, I will remove the code that expects JSON and use dict()?

When I define it as you suggested, the module gets snapshot_lvm_set set to a string, right? But the string is no longer JSON (the values have single quotes).

[richm]

So, on the module side, I will remove the code that expects JSON and use dict()?

yes

When I define it as you suggested, the module gets snapshot_lvm_set set to a string, right?

No, the module should get snapshot_lvm_set as a dict.

But the string is no longer JSON (the values have single quotes).

in defaults/main.yml you should have

snapshot_lvm_set: {}

[richm]

This should be a list of dict - see the storage role for examples e.g. https://github.com/linux-system-roles/storage/blob/main/library/blivet.py#L2131

        snapshot_lvm_set=dict(type="list", elements="dict",
                                                  options=dict(name=dict(type="str"),
                                                                         vg=dict(type="str"),
                                                                         lv=dict(type="str"),
                                                                         percent_space_required=dict(type="int"),
....

[richm]

Sorry - not a list of dict but just a dict

[richm]

ping - any updates?

[trgill]

@richm how should I default this to false when snapshot_lvm_all_vgs is not defined. Currently it defaults to "" and that causes an error when the module expects it to be a boolean.

[richm]

Suggested change

	__snapshot_failed_params.get('snapshot_lvm_all_vgs') | d(false)
	__snapshot_failed_params.get('snapshot_lvm_all_vgs', false)

[richm]

I don't think you need to do this - I believe the register: snapshot_cmd in the snapshot task creates the global variable snapshot_cmd

[trgill]

ok - fixed.

[richm]

@trgill This PR should fix all of those ansible-test (and flake8, pylint, black) issues. trgill#6 I didn't test it but hopefully the errors will be obvious

A big change was using module.run_command - which means most of the methods required the addition of the module argument - the way other modules handle this is by declaring a module class e.g. SnapshotModule(AnsibleModule) and putting all of the methods in this class - then you can just use self.module.run_command everywhere instead of having to pass module as a method argument.

[richm]

[citest]

[trgill]

[citest bad]