Add ephemeral state to mount fs without altering fstab #267
Conversation
plugins/modules/mount.py
Outdated
| lines = _get_mount_info(module, mntinfo_file) | ||
| # Keep same behavior than before | ||
| if lines is None: | ||
| return |
There was a problem hiding this comment.
The function get_linux_mount used to implement directly the _get_mount_info logic. Since the new function _is_same_mount_src also needed the mount info, I centralized this process in _get_mount_info to avoid code redundancy.
The behavior of get_linux_mounts is unchanged (return nothing if IOError is raised when opening the file, fail module if IOError is raised when closing the file).
There was a problem hiding this comment.
@NeodymiumFerBore I have tested this FIX and it worked first run on *BSD, Solars, and Linux.
However, when I tried the second run, it only succeeded on Linux host, and other OS has been failed with the following error:
fatal: [freebsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [netbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/pkg/bin/python3.9"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [openbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [solaris]: FAILED! => {"changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}Even if the ephemeral option only works on Linux, I think we need to take care of other OS.
There was a problem hiding this comment.
@NeodymiumFerBore I have tested this FIX and it worked first run on *BSD, Solars, and Linux.
However, when I tried the second run, it only succeeded on Linux host, and other OS has been failed with the following error:
fatal: [freebsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [netbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/pkg/bin/python3.9"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [openbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [solaris]: FAILED! => {"changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}Even if the ephemeral option only works on Linux, I think we need to take care of other OS.
=> Done
There was a problem hiding this comment.
This is my bad. *BSD and Solaris don't have /proc/self/mountinfo, and the functions get_linux_mounts/_get_mount_info are used regardless of the system. The _get_mount_info function is just a deported part of the original get_linux_mounts function, and it only works for Linux. Will run some tests and implement a fix.
There was a problem hiding this comment.
Why integration tests fail on *BSD and Unix as of now
@saito-hideki I don't know what values you used for the src parameter, but as of now, it might success if you are using network sources (currently, the integration tests fail on *bsd and solaris due to the way they handle loop devices, I'm implementing a fix for that).
Why manual tests might work first run, but not second
If it failed on second run (and I assume you didn't run the integration tests, but did manual tests), I think it is due to the function _is_same_mount_src. It uses the function _get_mount_info without checking if the system is Linux, and /proc/self/mountinfo can only be reliably used on Linux systems. As does /sys/block/loop${x}/loop/backing_file.
Note about checking the backing file of loop devices
This might be useless to try to get the backing file of a loop device on some systems. When we format/mount a file, Linux manipulates this file as a block device by implicitly creating/using a loop device.
Solaris does not support this, and we need to explicitly create a loop file interface for the file (with lofiadm). I have to do more tests about this for OpenBSD, FreeBSD and NetBSD.
In this version of my PR, when using ephemeral state, mount.py tries to verify if path is a mountpoint and if the mounted device is the same than src. If it finds a loop device (/dev/loop[0-9]+), it tries to retrieve the backing file through /sys/block/loop${x}/loop/backing_file. This is required to make the ephemeral state compatible with mounting files (implicit loop devices).
On systems that do not implicitly creates this loop device (such as Solaris), it might be useless to try to find the backing file of a loop device, as if a user tries to mount a file, he will first manually find/create the loop device, as he would on a shell.
I'm working on this
_is_same_mount_srcmust be system aware and adapt its behavior_get_mount_infoshould either be renamed_get_linux_mount_info, or be system aware- adapt integration tests to make them compatible with systems that do not implicitly handle loop devices
There was a problem hiding this comment.
TLDR: Im implementing a fix so the ephemeral state works fine on *BSD, Solaris and Linux. It might take some time and a lot of testing.
There was a problem hiding this comment.
Moved from /proc/self/mountinfo to mount -v, which has a close behavior on Linux, Solaris and *BSD systems.
Also implemented integration tests for Solaris/*BSD.
plugins/modules/mount.py
Outdated
| @@ -659,6 +710,25 @@ def get_linux_mounts(module, mntinfo_file="/proc/self/mountinfo"): | |||
| return mounts | |||
|
|
|||
|
|
|||
| def _is_same_mount_src(module, args, mntinfo_file="/proc/self/mountinfo"): | |||
| """Return True if the mounted fs on mountpoint is the same source than src""" | |||
There was a problem hiding this comment.
This function is used for the ephemeral state: if this function returns False, that means that the provided mount point has already a volume mounted on it, and it is NOT the volume given in src. This leads to a module failure, to avoid an unwanted unmount.
| @@ -847,7 +944,8 @@ def main(): | |||
| # A non-working fstab entry may break the system at the reboot, | |||
| # so undo all the changes if possible. | |||
| try: | |||
| write_fstab(module, backup_lines, args['fstab']) | |||
| if state != 'ephemeral': | |||
| write_fstab(module, backup_lines, args['fstab']) | |||
There was a problem hiding this comment.
This piece of code restores the fstab file to its previous state when the module fails. When state is ephemeral, the fstab file is never modified, so this step must be skipped.
|
@NeodymiumFerBore Thank you for the pull request. If it is possible, can you create integration tests for the |
|
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 ;-) |
|
Hello. Glad you appreciate this feature, thanks for the feedback :) |
|
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
force-pushed
the
ephemeral_state
branch
4 times, most recently
from
c81659a
to
0134451
Compare
|
Rebased PR branch on upstream main branch Tests fail on RHEL 7.9 remote devel and remote 2.12: assertion |
NeodymiumFerBore
force-pushed
the
ephemeral_state
branch
from
c25d956
to
1391610
Compare
|
Build succeeded.
|
|
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? |
There was a problem hiding this comment.
I think we need to take care of how it works on non-Linux operating systems. For example, it outputs a warning and does not actually perform the ephemeral mount.
Also once all the fixes are finished, I would appreciate it if you squash multiple commits into one :)
plugins/modules/mount.py
Outdated
| lines = _get_mount_info(module, mntinfo_file) | ||
| # Keep same behavior than before | ||
| if lines is None: | ||
| return |
There was a problem hiding this comment.
@NeodymiumFerBore I have tested this FIX and it worked first run on *BSD, Solars, and Linux.
However, when I tried the second run, it only succeeded on Linux host, and other OS has been failed with the following error:
fatal: [freebsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [netbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/pkg/bin/python3.9"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [openbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [solaris]: FAILED! => {"changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}Even if the ephemeral option only works on Linux, I think we need to take care of other OS.
|
Build succeeded.
|
plugins/modules/mount.py
Outdated
| """Return True if the mounted fs on mountpoint is the same source than src""" | ||
| mountpoint = args['name'] | ||
| src = args['src'] | ||
| lines = _get_mount_info(module) |
There was a problem hiding this comment.
See comment: this call (or the _get_mount_info function) must be system aware
|
Build succeeded.
|
|
Hello @saito-hideki . Is there a reachable NFS target in the test environment? If yes, what's its name? |
plugins/modules/mount.py
Outdated
| lines = _get_mount_info(module, mntinfo_file) | ||
| # Keep same behavior than before | ||
| if lines is None: | ||
| return |
There was a problem hiding this comment.
@NeodymiumFerBore I have tested this FIX and it worked first run on *BSD, Solars, and Linux.
However, when I tried the second run, it only succeeded on Linux host, and other OS has been failed with the following error:
fatal: [freebsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [netbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/pkg/bin/python3.9"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [openbsd]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/local/bin/python3.8"}, "changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}
fatal: [solaris]: FAILED! => {"changed": false, "msg": "Unable to retrieve mount info from '/proc/self/mountinfo'"}Even if the ephemeral option only works on Linux, I think we need to take care of other OS.
=> Done
| ###################################################################################################### | ||
| ###################################################################################################### | ||
| ###################################################################################################### |
There was a problem hiding this comment.
It would be better to remove the three-line separator (#). Because test logic for the ephemeral option has already been separated by the subsequent block :)
There was a problem hiding this comment.
This will be removed in the next push (squashed). This was just eye-candy during development, sorry for that!
| - name: Patch centos repolist | ||
| ansible.builtin.shell: | ||
| cmd: "{{ item }}" | ||
| with_items: | ||
| - sed -i 's/mirrorlist/#mirrorlist/g' /etc/yum.repos.d/CentOS-* | ||
| - sed -i 's|#baseurl=http://mirror.centos.org|baseurl=http://vault.centos.org|g' /etc/yum.repos.d/CentOS-* | ||
| when: ansible_distribution in ('CentOS') and ansible_distribution_version|int >= 8 |
There was a problem hiding this comment.
CentOS8(or later) is no longer supported for the integration tests. So I think you can remove this Patch centos repolist task from the integration test.
There was a problem hiding this comment.
Removed in latest push
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). @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. |
NeodymiumFerBore
force-pushed
the
ephemeral_state
branch
from
ab18428
to
217df07
Compare
|
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 |
|
Build failed.
|
|
Closing and reopening for CI trigger |
|
Build failed.
|
NeodymiumFerBore
force-pushed
the
ephemeral_state
branch
from
49aaa00
to
38310f6
Compare
|
Rebased PR branch to master 55bd196 |
|
@saito-hideki Thank you for your action! However, @Akasurde requested a change in the documentation part, stating that the |
|
Build succeeded. ✔️ ansible-changelog-fragment SUCCESS in 15s |
|
@NeodymiumFerBore thanks! :) |
NeodymiumFerBore
force-pushed
the
ephemeral_state
branch
from
38310f6
to
5fe3bd9
Compare
|
@saito-hideki Done! I amended previous commit to avoid adding more commits, while keeping track of the co-authored change. |
|
Build succeeded. ✔️ ansible-changelog-fragment SUCCESS in 15s |
plugins/modules/mount.py
Outdated
| has already a device mounted on, and its source is different than I(src), | ||
| the module will fail to avoid unexpected unmount or mount point override. | ||
| If the mount point is not present, the mount point will be created. | ||
| The I(fstab) is completely ignored. This option is added in version 1.4.0. |
There was a problem hiding this comment.
Sorry, I missed one thing, could you bump it from 1.4.0 to 1.5.0 as we discussed in #267 (comment) ?
There was a problem hiding this comment.
Oops, stupid me did not commit the change. I am sorry for that. I did as I said in my previous comment:
I amended previous commit to avoid adding more commits, while keeping track of the co-authored change.
Co-authored-by: Abhijeet Kasurde <[email protected]>
NeodymiumFerBore
force-pushed
the
ephemeral_state
branch
from
5fe3bd9
to
b8ed919
Compare
|
Build succeeded. ✔️ ansible-changelog-fragment SUCCESS in 18s |
|
@Akasurde Finally, this PR passed CI tests without issues. I would appreciate it if you could review this PR. |
Using the "state: absent" for ansible.posix.mount module on "Unmount VBoxGuestAdditions.iso" task, deletes the mounpoint, in this case, it's the `/mnt` directory. On the other hand, using the "state: unmounted" results stuck the Vagrant box while the creation of the VMs. Propose: Since it's a temporary ISO mount and unmount procedure, let's use a temporary fstab and do not touch the /etc/fstab at all. There's a long-awaited PR for adding "state: ephemeral" support to the ansible.posix.mount module[^1]. We can switch to use this option after the PR merged to the upstream. This patch can be tested on https://app.vagrantup.com/almalinux/boxes/8/versions/8.6.20220715 and https://app.vagrantup.com/almalinux/boxes/9/versions/9.0.20220715 Vagrant boxes. Fixes: AlmaLinux/cloud-images#72 [^1]: ansible-collections/ansible.posix#267 Signed-off-by: Elkhan Mammadli <[email protected]>
|
Hello @saito-hideki , @Akasurde , |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
Build succeeded (gate pipeline). ✔️ ansible-changelog-fragment SUCCESS in 17s |
|
@NeodymiumFerBore sorry for the late reply. This PR has been merged. |
ghost
mentioned this pull request
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [ansible.posix](https://togithub.com/ansible-collections/ansible.posix) | galaxy-collection | minor | `1.4.0` -> `1.5.1` | --- ### Release Notes <details> <summary>ansible-collections/ansible.posix</summary> ### [`v1.5.1`](https://togithub.com/ansible-collections/ansible.posix/blob/HEAD/CHANGELOG.rst#v151) [Compare Source](https://togithub.com/ansible-collections/ansible.posix/compare/1.5.0...1.5.1) \====== ## Minor Changes - mount - Add `absent_from_fstab` state ([https://github.com/ansible-collections/ansible.posix/pull/166](https://togithub.com/ansible-collections/ansible.posix/pull/166)). - mount - Add `ephemeral` value for the `state` parameter, that allows to mount a filesystem without altering the `fstab` file ([https://github.com/ansible-collections/ansible.posix/pull/267](https://togithub.com/ansible-collections/ansible.posix/pull/267)). - r4e_rpm_ostree - new module for validating package state on RHEL for Edge - rhel_facts - new facts module to handle RHEL specific facts - rhel_rpm_ostree - new module to handle RHEL rpm-ostree specific package management functionality - rpm_ostree_upgrade - new module to automate rpm-ostree upgrades - rpm_ostree_upgrade - new module to manage upgrades for rpm-ostree based systems ## Bugfixes - Removed contentious terminology to match reference documentation in profile_tasks. - firewall - Fixed to output a more complete missing library message. - synchronize - Fixed hosts involved in rsync require the same password ### [`v1.5.0`](https://togithub.com/ansible-collections/ansible.posix/compare/1.4.0...1.5.0) [Compare Source](https://togithub.com/ansible-collections/ansible.posix/compare/1.4.0...1.5.0) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://togithub.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4xMDYuMCIsInVwZGF0ZWRJblZlciI6IjM0LjEwNi4wIn0=--> Co-authored-by: lumiere-bot <98047013+lumiere-bot[bot]@users.noreply.github.com>
SUMMARY
Add
ephemeralpossible value for state parameter.The
ephemeralstate 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
fstabmodule and amountmodule, but nothing has been done in 5 years. This is why I'd like to propose this feature.Downside: the way the
posix.mountmodule 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 everymountdefault options, for all platforms. This is whystate=ephemeralalways returnchanged=yes.In other terms, a
remountwill always be triggered if the volume is already mounted, even if the options look to be the same. Usingstate=unmountedon a volume previously mounted withephemeralbehaves correctly.ISSUE TYPE
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
ephemeralstateephemeralstate example_set_ephemeral_argsto use instead of_set_fstab_argswhen using ephemeral state_is_same_mount_srcto determine if the mounted volume on the destination path has the same source than the one supplied to the module_get_mount_infoto avoid redundant code between functionsget_linux_mountsand_is_same_mount_srcget_linux_mountto use the new function_get_mount_info. Original behavior is preserved.ephemeralparameter treatment intomountedtreatment, and addifstatements to avoid IO from/to fstabephemeralas a possible value for the state parameter inmain()required_ifdependencies forephemeralstate