Revert "Use locking for concurrent file access (#52567)" (#54547)

This reverts commit e152b277cf.

changelogs/fragments/30413-lineinfile-concurrence_issue.yaml

@@ -1,3 +0,0 @@
-bugfixes:
-- change file locking implementation from a class to context manager to allow easy and safe concurrent file access by modules
-- lineinfile - lock on concurrent file access (https://github.com/ansible/ansible/issues/30413)

lib/ansible/module_utils/common/file.py

@@ -1,21 +1,24 @@
-# -*- coding: utf-8 -*-
-
-# Copyright: (c) 2018, Ansible Project
+# Copyright (c) 2018, Ansible Project
 # Simplified BSD License (see licenses/simplified_bsd.txt or https://opensource.org/licenses/BSD-2-Clause)
 
 from __future__ import (absolute_import, division, print_function)
 __metaclass__ = type
 
-import fcntl
+import errno
 import os
-import re
 import stat
-import sys
+import re
+import pwd
+import grp
 import time
+import shutil
+import traceback
+import fcntl
+import sys
 
 from contextlib import contextmanager
-from ansible.module_utils._text import to_bytes
-from ansible.module_utils.six import PY3
+from ansible.module_utils._text import to_bytes, to_native, to_text
+from ansible.module_utils.six import b, binary_type
 
 try:
     import selinux
@@ -59,13 +62,6 @@ _EXEC_PERM_BITS = 0o0111     # execute permission bits
 _DEFAULT_PERM = 0o0666       # default file permission bits
 
 
-# Ensure we use flock on e.g. FreeBSD, MacOSX and Solaris
-if sys.platform.startswith('linux'):
-    filelock = fcntl.lockf
-else:
-    filelock = fcntl.flock
-
-
 def is_executable(path):
     # This function's signature needs to be repeated
     # as the first line of its docstring.
@@ -118,88 +114,89 @@ class LockTimeout(Exception):
     pass
 
 
-# NOTE: Using the open_locked() context manager it is absolutely mandatory
-#       to not open or close the same file within the existing context.
-#       It is essential to reuse the returned file descriptor only.
+class FileLock:
+    '''
+    Currently FileLock is implemented via fcntl.flock on a lock file, however this
+    behaviour may change in the future. Avoid mixing lock types fcntl.flock,
+    fcntl.lockf and module_utils.common.file.FileLock as it will certainly cause
+    unwanted and/or unexpected behaviour
+    '''
+    def __init__(self):
+        self.lockfd = None
+
     @contextmanager
-def open_locked(path, check_mode=False, lock_timeout=15):
+    def lock_file(self, path, tmpdir, lock_timeout=None):
         '''
-    Context managed for opening files with lock acquisition
-
-    :kw path: Path (file) to lock
-    :kw lock_timeout:
-        Wait n seconds for lock acquisition, fail if timeout is reached.
-        0 = Do not wait, fail if lock cannot be acquired immediately,
-        Less than 0 or None = wait indefinitely until lock is released
-        Default is wait 15s.
-    :returns: file descriptor
+        Context for lock acquisition
         '''
-    if check_mode:
-        b_path = to_bytes(path, errors='surrogate_or_strict')
-        fd = open(b_path, 'ab+')
-        fd.seek(0)  # Due to a difference in behavior between PY2 and PY3 we need to seek(0) on PY3
-    else:
-        fd = lock(path, check_mode, lock_timeout)
-    yield fd
-    fd.close()
-
-
-def lock(path, check_mode=False, lock_timeout=15):
-    '''
-    Set lock on given path via fcntl.flock(), note that using
-    locks does not guarantee exclusiveness unless all accessing
-    processes honor locks.
-
-    :kw path: Path (file) to lock
-    :kw lock_timeout:
-        Wait n seconds for lock acquisition, fail if timeout is reached.
-        0 = Do not wait, fail if lock cannot be acquired immediately,
-        Less than 0 or None = wait indefinitely until lock is released
-        Default is wait 15s.
-    :returns: file descriptor
-    '''
-    b_path = to_bytes(path, errors='surrogate_or_strict')
-    wait = 0.1
-
-    lock_exception = IOError
-    if PY3:
-        lock_exception = OSError
-
-    if not os.path.exists(b_path):
-        raise IOError('{0} does not exist'.format(path))
-
-    if lock_timeout is None or lock_timeout < 0:
-        fd = open(b_path, 'ab+')
-        fd.seek(0)  # Due to a difference in behavior between PY2 and PY3 we need to seek(0) on PY3
-        filelock(fd, fcntl.LOCK_EX)
-        return fd
-
-    if lock_timeout >= 0:
-        total_wait = 0
-        while total_wait <= lock_timeout:
-            fd = open(b_path, 'ab+')
-            fd.seek(0)  # Due to a difference in behavior between PY2 and PY3 we need to seek(0) on PY3
         try:
-                filelock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
-                return fd
-            except lock_exception:
-                fd.close()
-                time.sleep(wait)
-                total_wait += wait
+            self.set_lock(path, tmpdir, lock_timeout)
+            yield
+        finally:
+            self.unlock()
+
+    def set_lock(self, path, tmpdir, lock_timeout=None):
+        '''
+        Create a lock file based on path with flock to prevent other processes
+        using given path.
+        Please note that currently file locking only works when it's executed by
+        the same user, I.E single user scenarios
+
+        :kw path: Path (file) to lock
+        :kw tmpdir: Path where to place the temporary .lock file
+        :kw lock_timeout:
+            Wait n seconds for lock acquisition, fail if timeout is reached.
+            0 = Do not wait, fail if lock cannot be acquired immediately,
+            Default is None, wait indefinitely until lock is released.
+        :returns: True
+        '''
+        lock_path = os.path.join(tmpdir, 'ansible-{0}.lock'.format(os.path.basename(path)))
+        l_wait = 0.1
+        r_exception = IOError
+        if sys.version_info[0] == 3:
+            r_exception = BlockingIOError
+
+        self.lockfd = open(lock_path, 'w')
+
+        if lock_timeout <= 0:
+            fcntl.flock(self.lockfd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+            os.chmod(lock_path, stat.S_IWRITE | stat.S_IREAD)
+            return True
+
+        if lock_timeout:
+            e_secs = 0
+            while e_secs < lock_timeout:
+                try:
+                    fcntl.flock(self.lockfd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+                    os.chmod(lock_path, stat.S_IWRITE | stat.S_IREAD)
+                    return True
+                except r_exception:
+                    time.sleep(l_wait)
+                    e_secs += l_wait
                     continue
 
-        fd.close()
-        raise LockTimeout('Waited {0} seconds for lock on {1}'.format(total_wait, path))
+            self.lockfd.close()
+            raise LockTimeout('{0} sec'.format(lock_timeout))
 
+        fcntl.flock(self.lockfd, fcntl.LOCK_EX)
+        os.chmod(lock_path, stat.S_IWRITE | stat.S_IREAD)
 
-def unlock(fd):
+        return True
+
+    def unlock(self):
         '''
         Make sure lock file is available for everyone and Unlock the file descriptor
         locked by set_lock
 
-    :kw fd: File descriptor of file to unlock
+        :returns: True
         '''
+        if not self.lockfd:
+            return True
+
         try:
-        filelock(fd, fcntl.LOCK_UN)
-    except ValueError:  # File was not opened, let context manager fail gracefully
+            fcntl.flock(self.lockfd, fcntl.LOCK_UN)
+            self.lockfd.close()
+        except ValueError:  # file wasn't opened, let context manager fail gracefully
             pass
+
+        return True

lib/ansible/modules/files/lineinfile.py

@@ -184,13 +184,6 @@ EXAMPLES = r'''
     line: 192.168.1.99 foo.lab.net foo
     create: yes
 
-# Fully quoted because of the ': ' on the line. See the Gotchas in the YAML docs.
-- lineinfile:
-    path: /etc/sudoers
-    state: present
-    regexp: '^%wheel\s'
-    line: '%wheel ALL=(ALL) NOPASSWD: ALL'
-
 # NOTE: Yaml requires escaping backslashes in double quotes but not in single quotes
 - name: Ensure the JBoss memory settings are exactly as needed
   lineinfile:
@@ -215,7 +208,6 @@ import tempfile
 
 # import module snippets
 from ansible.module_utils.basic import AnsibleModule
-from ansible.module_utils.common.file import open_locked
 from ansible.module_utils.six import b
 from ansible.module_utils._text import to_bytes, to_native
 
@@ -273,18 +265,11 @@ def present(module, dest, regexp, line, insertafter, insertbefore, create,
                 os.makedirs(b_destpath)
             except Exception as e:
                 module.fail_json(msg='Error creating %s Error code: %s Error description: %s' % (b_destpath, e[0], e[1]))
-        # destination must exist to be able to lock it
-        if not module.check_mode:
-            open(b_dest, 'ab').close()
 
         b_lines = []
     else:
-        b_lines = None
-
-    # NOTE: Avoid opening the same file in this context !
-    with open_locked(dest, module.check_mode) as fd:
-        if b_lines is None:
-            b_lines = fd.readlines()
+        with open(b_dest, 'rb') as f:
+            b_lines = f.readlines()
 
     if module._diff:
         diff['before'] = to_native(b('').join(b_lines))
@@ -441,9 +426,8 @@ def absent(module, dest, regexp, line, backup):
             'before_header': '%s (content)' % dest,
             'after_header': '%s (content)' % dest}
 
-    # NOTE: Avoid opening the same file in this context !
-    with open_locked(dest, module.check_mode) as fd:
-        b_lines = fd.readlines()
+    with open(b_dest, 'rb') as f:
+        b_lines = f.readlines()
 
     if module._diff:
         diff['before'] = to_native(b('').join(b_lines))

lib/ansible/modules/system/known_hosts.py

@@ -84,6 +84,7 @@ import re
 import tempfile
 
 from ansible.module_utils.basic import AnsibleModule
+from ansible.module_utils.common.file import FileLock
 from ansible.module_utils._text import to_bytes, to_native
 
 

test/integration/targets/file_lock/aliases

@@ -1 +0,0 @@
-shippable/posix/group2

test/integration/targets/file_lock/inventory

@@ -1,2 +0,0 @@
-[lockhosts]
-lockhost[00:99] ansible_connection=local ansible_python_interpreter="{{ ansible_playbook_python }}"

test/integration/targets/file_lock/runme.sh

@@ -1,6 +0,0 @@
-#!/usr/bin/env bash
-
-set -eux
-
-ansible-playbook test_filelock.yml -i inventory --forks 10 --diff -v "$@"
-ansible-playbook test_filelock_timeout.yml -i inventory --diff  -v "$@"

test/integration/targets/file_lock/test_filelock.yml

@@ -1,45 +0,0 @@
----
-- hosts: lockhosts
-  gather_facts: no
-  vars:
-    lockfile: ~/ansible_testing/lock.test
-  tasks:
-  - name: Remove lockfile
-    file:
-      path: '{{ lockfile }}'
-      state: absent
-    run_once: yes
-
-  - name: Write inventory_hostname to lockfile concurrently
-    lineinfile:
-      path: '{{ lockfile }}'
-      line: '{{ inventory_hostname }}'
-      create: yes
-      state: present
-
-  - debug:
-      msg: File {{ lockfile }} has {{ lines|length }} lines for {{ ansible_play_batch|length }} instances
-    vars:
-      lines: "{{ lookup('file', lockfile).split('\n') }}"
-    run_once: yes
-
-  - name: Assert we get the expected number of lines
-    assert:
-      that:
-      - lines|length == ansible_play_batch|length
-    vars:
-      lines: "{{ lookup('file', lockfile).split('\n') }}"
-    run_once: yes
-
-  - name: Check lockfile for inventory_hostname entries
-    lineinfile:
-      path: '{{ lockfile }}'
-      line: '{{ inventory_hostname }}'
-      state: present
-    register: check_lockfile
-
-  - name: Assert locking results
-    assert:
-      that:
-      - check_lockfile is not changed
-      - check_lockfile is not failed

test/integration/targets/file_lock/test_filelock_timeout.yml

@@ -1,63 +0,0 @@
----
-- hosts: lockhost00
-  vars:
-    lockfile: ~/ansible_testing/lock_timeout.test
-  gather_facts: no
-  tasks:
-  - name: Remove lockfile
-    file:
-      path: '{{ lockfile }}'
-      state: absent
-    run_once: yes
-
-  - name: Create lockfile
-    lineinfile:
-      line: '{{ inventory_hostname }}'
-      path: '{{ lockfile }}'
-      state: present
-      create: yes
-
-  - name: Lock lockfile with lockf and sleep 20s
-    command: python
-    args:
-      stdin: |
-        import time
-        from ansible.module_utils.common.file import open_locked
-        with open_locked('{{ lockfile | expanduser }}') as fd:
-            time.sleep(20)
-    async: 60
-    poll: 0
-    register: flock_waiter
-
-  - name: Remove inventory_hostname line from lockfile
-    lineinfile:
-      path: '{{ lockfile }}'
-      line: '{{ inventory_hostname }}'
-      state: absent
-    ignore_errors: yes
-    register: rm_line
-
-  - name: Assert that removal of inventory_hostname from lockfile failed
-    assert:
-      that:
-        - rm_line is failed
-
-  - name: Wait for flock job to finish
-    async_status:
-      jid: '{{ flock_waiter.ansible_job_id }}'
-    register: job_result
-    until: job_result.finished
-    retries: 30
-
-  - name: Inventory_hostname in lockfile
-    lineinfile:
-      path: '{{ lockfile }}'
-      line: '{{ inventory_hostname }}'
-      state: present
-    register: check_line
-
-  - name: Assert that lockfile is unchanged
-    assert:
-      that:
-        - check_line is not changed
-        - check_line is not failed