Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mount.py no rmdir when state=='absent' #569

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

YvesMP
Copy link

@YvesMP YvesMP commented Sep 18, 2024

SUMMARY

remove rmdir done after umounting when the requested state is 'absent'

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mount.py

ADDITIONAL INFORMATION

In function main(), remove rmdir in case : if state == 'absent'.

Unmounting a file system should not lead to delete anything that is revealed after unmounting.
Also, it leads to an error if a non empty directory is present under the ex-mountpoint after umount : [Errno 39] Directory not empty . So umount is successfull but the ansible run is failed. Of course, it is solved on second run.

# prepare for test
mkdir /var/my
mkdir /var/my/subdir
echo "some content" > /var/my/subdir/data

tree /var/my/
/var/my/
└── subdir
    └── data

# playbook used to mount
cat mount_mydata.yml
---
- name: mount my data
  hosts: localhost
  gather_facts: no
  become: yes
  tasks:
  - name: mount my data
    ansible.posix.mount:
      path: /var/my
      fstype: xfs
      state: mounted
      src: /dev/testVG/testLV
      # change with your own device

ansible-playbook mount_mydata.yml

PLAY [mount my data] **************************************************************************************************

TASK [mount my data] **************************************************************************************************
changed: [localhost]

PLAY RECAP ************************************************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=0

tree /var/my/
# gives something different
# may give no data or data depending on device content

# playbook to umount
cat unmount_mydata.yml
---
- name: unmount my data
  hosts: localhost
  gather_facts: no
  become: yes
  tasks:
  - name: unmount my data
    ansible.posix.mount:
      path: /var/my
      state: absent
      src: /dev/testVG/testLV
      # change with your own device
      backup: yes

# before code update, first run KO
ansible-playbook unmount_mydata.yml

PLAY [unmount my date] ************************************************************************************************

TASK [unmount my data] ************************************************************************************************
fatal: [localhost]: FAILED! => {"ansible_facts": {"discovered_interpreter_python": "/usr/libexec/platform-python"}, "changed": false, "msg": "Error rmdir /var/my: [Errno 39] Directory not empty: '/var/my'"}

PLAY RECAP ************************************************************************************************************
localhost                  : ok=0    changed=0    unreachable=0    failed=1    skipped=0    rescued=0    ignored=0

# and need a second run to be OK
ansible-playbook unmount_mydata.yml

PLAY [unmount my date] ************************************************************************************************

TASK [unmount my data] ************************************************************************************************
ok: [localhost]

PLAY RECAP ************************************************************************************************************
localhost                  : ok=1    changed=0    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0


# after code update one run is enough
ansible-playbook unmount_mydata.yml

PLAY [unmount my date] ************************************************************************************************

TASK [unmount my data] ************************************************************************************************
changed: [localhost]

PLAY RECAP ************************************************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=0    skipped=0    rescued=0    ignored=0

# back to original content
tree /var/my/
/var/my/
└── subdir
    └── data

In function main(), remove rmdir in case if state == 'absent'.
Unmounting a file system should not lead to delete anything that is revealed after unmounting. 
Also, it leads to an error if a non empty directory is present under the ex-mountpoint after umount : [Errno 39] Directory not empty
So umount is successfull but the ansible run is failed. Of course, it is solved on second run.
Copy link
Contributor

Copy link
Collaborator

@saito-hideki saito-hideki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! :)
I have left a comment as a change request.

@@ -899,11 +899,6 @@ def main():
module.fail_json(
msg="Error unmounting %s: %s" % (name, msg))

if os.path.exists(name):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will change the behavior of state: absent.
But I don't think the rmdir logic should be skipped. Because it breaks backward compatibility.
If you want to skip it, how about adding keep_mountpoint or preserve_mountpoint to keep the mountpoint to avoid Directory not empty error instead of removing this section.

If this is set as true, you can skip rmdir section. Also, if the default value is set as false, to keep backward compatibility, you can set it to false by default.

@saito-hideki saito-hideki added change_request The PR has a change request from a reviewer verified This issue has been verified/reproduced by maintainer labels Sep 26, 2024
new option keep_mountpoint enables keeping the mountpoint with state=absent
Copy link
Contributor

suppressed erroneous remaining TOTO check
Copy link
Contributor

delete trailing spaces in comments in mount.py
@YvesMP YvesMP requested a review from saito-hideki October 3, 2024 15:41
Copy link
Contributor

still a trailing whitespace
Copy link
Contributor

@YvesMP
Copy link
Author

YvesMP commented Oct 7, 2024

@saito-hideki
As you suggested, I added the keep_mountpoint option to enable skipping the rmdir logic with state=changed.

Copy link
Collaborator

@saito-hideki saito-hideki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @YvesMP
Thanks for the update.
Your changes worked without any issues in my lab. If you address the following three things, we can merge this PR.

Copy link
Contributor

deleted trailing newlines
Copy link
Contributor

Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
change_request The PR has a change request from a reviewer verified This issue has been verified/reproduced by maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants