ansible.posix icon indicating copy to clipboard operation
ansible.posix copied to clipboard

Published 20 hours ago • verified publisher icon ansible-collections

Add ephemeral state to mount fs without altering fstab

Open NeodymiumFerBore opened this issue 3 years ago • 39 comments

SUMMARY

Add ephemeral possible value for state parameter.

The ephemeral state allows end-users to mount a volume on a given path, without altering an fstab file or creating a dummy one.

There have been debates about splitting this module into an fstab module and a mount module, but nothing has been done in 5 years. This is why I'd like to propose this feature.

Downside: the way the posix.mount module handles mount options prevents it to be able to check exactly if the given opts perfectly match the mount options of an already mounted volume. To achieve this, the module would have to be aware of every mount default options, for all platforms. This is why state=ephemeral always return changed=yes. In other terms, a remount will always be triggered if the volume is already mounted, even if the options look to be the same. Using state=unmounted on a volume previously mounted with ephemeral behaves correctly.

ISSUE TYPE
  • Feature Pull Request

Related issues:

COMPONENT NAME

mount

ADDITIONAL INFORMATION

Example use case

Sometimes it is handy to be able to temporarily mount a volume. I've seen this in couple companies where Ansible is used to generate reports and put it on network shares. However, some admins don't look into mount options such as krb5 and multiuser for SMB shares. Being forced to use fstab-based mounts leads to clear text passwords being stored more or less temporarily on the host filesystem, requiring "manual" deletion (with the hassle of using blocks, rescues, always, etc.). This feature respond to this use case by providing a way to mount a volume without having to alter an fstab file.

Description of changes

  • Edit DOCUMENTATION section to add ephemeral state
  • Edit EXAMPLES section to add ephemeral state example
  • Add new function _set_ephemeral_args to use instead of _set_fstab_args when using ephemeral state
  • Add new function _is_same_mount_src to determine if the mounted volume on the destination path has the same source than the one supplied to the module
  • Add new function _get_mount_info to avoid redundant code between functions get_linux_mounts and _is_same_mount_src
  • Modify get_linux_mount to use the new function _get_mount_info. Original behavior is preserved.
  • Integrate ephemeral parameter treatment into mounted treatment, and add if statements to avoid IO from/to fstab
  • Add ephemeral as a possible value for the state parameter in main()
  • Add required_if dependencies for ephemeral state

NeodymiumFerBore avatar Sep 14 '21 17:09 NeodymiumFerBore

@NeodymiumFerBore Thank you for the pull request. If it is possible, can you create integration tests for the ephemeral option?

  • https://github.com/ansible-collections/ansible.posix/blob/main/tests/integration/targets/mount/tasks/main.yml

saito-hideki avatar Nov 29 '21 14:11 saito-hideki

Hello @saito-hideki . Thank you for your interest in my PR. Yes I can write integration tests. I don't have much time right now. I will push the tests into this branch.

NeodymiumFerBore avatar Dec 11 '21 14:12 NeodymiumFerBore

Hello @saito-hideki . Thank you for your interest in my PR. Yes I can write integration tests. I don't have much time right now. I will push the tests into this branch.

@NeodymiumFerBore thanks for this feature / option. Would be cool if you could write those tests so this can be added to Ansible ;-)

frittentheke avatar Feb 02 '22 13:02 frittentheke

Hello. Glad you appreciate this feature, thanks for the feedback :) I had a lot of work and stuff going on lately, I haven't take the time to write it. Will try to do it this weekend!

NeodymiumFerBore avatar Feb 02 '22 13:02 NeodymiumFerBore

Hello @saito-hideki . I started writing integration tests and got into some weird behavior review. Maybe this is intended, but this is not documented as far as I know.

I will keep writing integration tests while keeping the current behavior. If you have some time, please give me your opinion on the following. We may fix it with another PR.

EDIT: opened issue #322 as it was beyond the scope of this PR. Sorry for the ping

NeodymiumFerBore avatar Feb 02 '22 21:02 NeodymiumFerBore

Rebased PR branch on upstream main branch

Tests fail on RHEL 7.9 remote devel and remote 2.12: assertion "'/tmp/myfs' is mount" is not asserted, though it is asserted on RHEL 8. Any idea that would save me some time? Will go into troubleshooting this next week.

NeodymiumFerBore avatar Feb 06 '22 17:02 NeodymiumFerBore

Build succeeded.

softwarefactory-project-zuul[bot] avatar Feb 12 '22 03:02 softwarefactory-project-zuul[bot]

Hello @saito-hideki . Should I squash my commits into a single one and force push my branch? Rebase it again on upstream main?

Is there any required action from me before the merge can occur?

NeodymiumFerBore avatar Mar 12 '22 14:03 NeodymiumFerBore

Build succeeded.

softwarefactory-project-zuul[bot] avatar Mar 19 '22 04:03 softwarefactory-project-zuul[bot]

Build succeeded.

softwarefactory-project-zuul[bot] avatar Apr 13 '22 18:04 softwarefactory-project-zuul[bot]

Hello @saito-hideki . Is there a reachable NFS target in the test environment? If yes, what's its name? It would highly simplify integration tests. Using loop devices on *BSD and Solaris is not transparent, and they all have different tools (lofiadm, mdconfig, vnconfig). Integration tests in my last push work at least on (py2/py3) Solaris 11, NetBSD 8.2/9.2, FreeBSD 13, OpenBSD 6.9, but it's pretty heavy... Thank you.

NeodymiumFerBore avatar Apr 13 '22 20:04 NeodymiumFerBore

Hi @NeodymiumFerBore

Integration tests in my last push work at least on (py2/py3) Solaris 11, NetBSD 8.2/9.2, FreeBSD 13, OpenBSD 6.9, but it's pretty heavy... Thank you.

Thank you for your hard work. Your integration tests worked not only Linux but also on Solaris and *BSD(my test environment is Solaris11.4, FreeBSD13, NetBSD9.2, and OpenBSD6.9). Also, I would appreciate it if you squash multiple commits into one :) I have left two trivial change requests and will set approve flag once it is fixed.

@Akasurde if you have B/W, is it possible to review this PR? if you have B/W, is it possible to review this PR? This PR looks pretty reasonable for me.

saito-hideki avatar Apr 18 '22 06:04 saito-hideki

Hello @saito-hideki . Thank you for your warm feedback! :)) The requested changes have been addressed.

In my last push (squashed commits), the CI failed with error ERROR! Error when getting collection version metadata for community.general:1.3.5 from default (https://galaxy.ansible.com/api/) (HTTP Code: 429, Message: Too Many Requests Code: Unknown). I don't think the code is responsible for this error. Is there any way I can trigger the CI again without pushing dummy stuff?

NeodymiumFerBore avatar Apr 18 '22 12:04 NeodymiumFerBore

Build failed.

softwarefactory-project-zuul[bot] avatar Apr 18 '22 12:04 softwarefactory-project-zuul[bot]

Closing and reopening for CI trigger

saito-hideki avatar Apr 18 '22 14:04 saito-hideki

Build failed.

softwarefactory-project-zuul[bot] avatar Apr 18 '22 14:04 softwarefactory-project-zuul[bot]

@NeodymiumFerBore Thank you for reporting the CI issue. The galaxy issue that you reported seems fixed(galaxy server API possibly unavailable at that time). However, it seems that there's another problem on the CI tests side. This error seems to be a problem on the CI test side and not directly related to this PR. So we will check CI testing and let you know if we have any clue.

saito-hideki avatar Apr 18 '22 14:04 saito-hideki

Closing and reopening for CI trigger

saito-hideki avatar Apr 22 '22 05:04 saito-hideki

Build failed.

softwarefactory-project-zuul[bot] avatar Apr 22 '22 05:04 softwarefactory-project-zuul[bot]

Hi @NeodymiumFerBore , I have fixed one of the unit test issues, also test team seems fixed several issues regarding execution environment tests. Therefore, if it is possible, can you rebase this PR with the latest main branch? Once you rebased it, I think this PR will pass the CI tests.

saito-hideki avatar Apr 22 '22 06:04 saito-hideki

Rebased PR branch to master dc4da60affbe29e7706851ec73dab09bd11bdb44.

NeodymiumFerBore avatar Apr 22 '22 16:04 NeodymiumFerBore

Build succeeded.

softwarefactory-project-zuul[bot] avatar Apr 22 '22 17:04 softwarefactory-project-zuul[bot]

Hello @saito-hideki , thank you for your quick fix on the CI, build succeeded 🥳 Let me know if there is anything I should do for the merge to occur!

NeodymiumFerBore avatar Apr 22 '22 17:04 NeodymiumFerBore

@Akasurde I think this PR is reasonable. I would appreciate it if you could review this PR :) Thanks!

saito-hideki avatar Apr 25 '22 07:04 saito-hideki

Hi @Akasurde . Thank you for your review, I applied your suggestions :) The ansible-galaxy API seems to have some "Too many requests" issues, and fails the CI.

I will close/re-open the PR to trigger the CI again in couple hours.

Also, let me know if I should squash this last commit again :) Thank you !

NeodymiumFerBore avatar May 18 '22 17:05 NeodymiumFerBore

Build succeeded.

:heavy_check_mark: ansible-changelog-fragment SUCCESS in 20s :heavy_check_mark: ansible-test-sanity-docker-devel SUCCESS in 8m 04s (non-voting) :heavy_check_mark: ansible-test-sanity-docker-milestone SUCCESS in 9m 14s :heavy_check_mark: ansible-test-sanity-docker-stable-2.9 SUCCESS in 11m 10s :heavy_check_mark: ansible-test-sanity-docker-stable-2.10 SUCCESS in 8m 22s :heavy_check_mark: ansible-test-sanity-docker-stable-2.11 SUCCESS in 8m 25s :heavy_check_mark: ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 46s :heavy_check_mark: ansible-test-units-posix SUCCESS in 6m 41s :heavy_check_mark: ansible-galaxy-importer SUCCESS in 4m 36s :heavy_check_mark: build-ansible-collection SUCCESS in 3m 06s

softwarefactory-project-zuul[bot] avatar May 18 '22 17:05 softwarefactory-project-zuul[bot]

Closing and reopening for CI trigger

NeodymiumFerBore avatar May 19 '22 22:05 NeodymiumFerBore

Build succeeded.

:heavy_check_mark: ansible-changelog-fragment SUCCESS in 15s :heavy_check_mark: ansible-test-sanity-docker-devel SUCCESS in 8m 08s (non-voting) :heavy_check_mark: ansible-test-sanity-docker-milestone SUCCESS in 8m 56s :heavy_check_mark: ansible-test-sanity-docker-stable-2.9 SUCCESS in 9m 20s :heavy_check_mark: ansible-test-sanity-docker-stable-2.10 SUCCESS in 8m 37s :heavy_check_mark: ansible-test-sanity-docker-stable-2.11 SUCCESS in 7m 58s :heavy_check_mark: ansible-test-sanity-docker-stable-2.12 SUCCESS in 8m 59s :heavy_check_mark: ansible-test-units-posix SUCCESS in 5m 12s :heavy_check_mark: ansible-galaxy-importer SUCCESS in 3m 55s :heavy_check_mark: build-ansible-collection SUCCESS in 2m 59s

softwarefactory-project-zuul[bot] avatar May 19 '22 22:05 softwarefactory-project-zuul[bot]

Hello @saito-hideki @Akasurde . I have the following error in the CI, on all Remote 2.9 : ERROR! couldn't resolve module/action 'community.general.filesize'. This often indicates a misspelling, missing collection, or incorrect module path.. I see there is the same error on upstream CI, commit 2d3f55c.

Should I not worry about this? Do you have an idea why this module is not resolved on an older version of Ansible?

Also, I have some ERROR: Timeout waiting for freebsd/12.0 instance for all FreeBSD tests. Again, same problem on upstream CI. I cannot do much about this I guess.

Please, let me know if I have to change something in my PR. As of now I'm not sure what to do :(

Thank you!

NeodymiumFerBore avatar May 23 '22 13:05 NeodymiumFerBore

Hi @NeodymiumFerBore thank you for the heads-up. I'll check that there is a problem on the CI process side.

saito-hideki avatar May 25 '22 10:05 saito-hideki