Restore idempotency for disabling unused filesystems with Ansible 2.16.0

[akikanellis]

The os_unused_filesystems was lacking sorting, making the task not idempotent. This was especially apparent and random in Molecule tests when this collection was added as a dependency.

Signed-off-by: Aki Kanellis [email protected]

[schurzi]

Thanks for submitting this. I am a bit puzzled, since we actually have idempotency tests which do not show this problem. Can you describe your test setup a bit more detailed?

[akikanellis]

Thanks for submitting this. I am a bit puzzled, since we actually have idempotency tests which do not show this problem. Can you describe your test setup a bit more detailed?

@schurzi I first noticed the issue in my homelab project, which is using this collection here. That is quite a complicated setup, so I have created a minimal reproduction repo where you can see the issue reproduced both for Docker and Vagrant based molecule tests.

It does seem like the current test setup in this collection with molecule is somehow causing this randomness to not happen. I had a look but haven't figured it out yet. It would be good to figure it out so that we can add some additional test coverage and prevent this from happening in the future. I will let you know if I find anything.

Hope that helps.

[schurzi]

So, after some debugging I was able to track this down. The task, that mixes up the list is:

ansible-collection-hardening/roles/os_hardening/tasks/modprobe.yml

Lines 19 to 21 in dc432ba

	- name: Remove used filesystems from fs-list
	ansible.builtin.set_fact:
	os_unused_filesystems: "{{ os_unused_filesystems | difference(ansible_mounts | map(attribute='fstype') | list) }}"

And I also discovered why our tests don't find this issue. It is a new behaviour in Ansible 2.16.0 introduced by ansible/ansible#81639. Our tests currently run against Ansible 2.15.6 and should update in the next few days, since Ansible 2.16.0 was released two weeka ago.

[akikanellis]

That makes sense, thanks for looking into it. Is the solution here ok or is there a more preferable way to implement this? Happy to implement whatever is needed.

[schurzi]

I think it is OK. I debated a bit if including the sort in the task, that shuffles the array, would be preferable, but in the end it makes almost no difference. Keeping it in the template will keep the order the same, even if we restructure our tasks, so I think this might be a better solution.

I want to fix our testing before merging this, working on it in #721. I expect this will work out today.

[schurzi]

Many thanks! This was a very helpful PR and we all learned a lot :)

[akikanellis]

Many thanks! This was a very helpful PR and we all learned a lot :)

Happy to have helped and thanks for the review!

[nejch]

@schurzi this fix would be useful for us as we test idempotence and it's currently blocking upgrading in our own downstream collection. Are there any plans for a release with this? Sorry, I know these types of pings are annoying 😅