Use locking for concurrent file access (#52567)

* Use locking for concurrent file access

This implements locking to be used for modules that are used for
concurrent file access, like lineinfile or known_hosts.

* Reinstate lock_timeout

This commit includes:
- New file locking infrastructure for modules
- Enable timeout tests
- Madifications to support concurrency with lineinfile

* Rebase, update changelog and tests

We need to specify ansible_python_interpreter to avoid running interpreter discovery and selecting the incorrect interpreter.

Remove the import of lock in known_hosts since it is not used.

changelogs/fragments/30413-lineinfile-concurrence_issue.yaml

@@ -0,0 +1,3 @@
+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,24 +1,21 @@
-# Copyright (c) 2018, Ansible Project
+# -*- coding: utf-8 -*-
+
+# 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 errno
-import os
-import stat
-import re
-import pwd
-import grp
-import time
-import shutil
-import traceback
 import fcntl
+import os
+import re
+import stat
 import sys
+import time
 
 from contextlib import contextmanager
-from ansible.module_utils._text import to_bytes, to_native, to_text
+from ansible.module_utils._text import to_bytes
-from ansible.module_utils.six import b, binary_type
+from ansible.module_utils.six import PY3
 
 try:
     import selinux
@@ -62,6 +59,13 @@ _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.
@@ -114,89 +118,88 @@ class LockTimeout(Exception):
     pass
 
 
-class FileLock:
+# 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.
+@contextmanager
+def open_locked(path, check_mode=False, lock_timeout=15):
     '''
-    Currently FileLock is implemented via fcntl.flock on a lock file, however this
+    Context managed for opening files with lock acquisition
-    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 lock_file(self, path, tmpdir, lock_timeout=None):
-        '''
-        Context for lock acquisition
-        '''
-        try:
-            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.
+        Less than 0 or None = wait indefinitely until lock is released
-        :returns: True
+        Default is wait 15s.
+    :returns: file descriptor
     '''
-        lock_path = os.path.join(tmpdir, 'ansible-{0}.lock'.format(os.path.basename(path)))
+    if check_mode:
-        l_wait = 0.1
+        b_path = to_bytes(path, errors='surrogate_or_strict')
-        r_exception = IOError
+        fd = open(b_path, 'ab+')
-        if sys.version_info[0] == 3:
+        fd.seek(0)  # Due to a difference in behavior between PY2 and PY3 we need to seek(0) on PY3
-            r_exception = BlockingIOError
+    else:
+        fd = lock(path, check_mode, lock_timeout)
+    yield fd
+    fd.close()
 
-        self.lockfd = open(lock_path, 'w')
 
-        if lock_timeout <= 0:
+def lock(path, check_mode=False, lock_timeout=15):
-            fcntl.flock(self.lockfd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+    '''
-            os.chmod(lock_path, stat.S_IWRITE | stat.S_IREAD)
+    Set lock on given path via fcntl.flock(), note that using
-            return True
+    locks does not guarantee exclusiveness unless all accessing
+    processes honor locks.
 
-        if lock_timeout:
+    :kw path: Path (file) to lock
-            e_secs = 0
+    :kw lock_timeout:
-            while e_secs < 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:
-                    fcntl.flock(self.lockfd, fcntl.LOCK_EX | fcntl.LOCK_NB)
+                filelock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB)
-                    os.chmod(lock_path, stat.S_IWRITE | stat.S_IREAD)
+                return fd
-                    return True
+            except lock_exception:
-                except r_exception:
+                fd.close()
-                    time.sleep(l_wait)
+                time.sleep(wait)
-                    e_secs += l_wait
+                total_wait += wait
                 continue
 
-            self.lockfd.close()
+        fd.close()
-            raise LockTimeout('{0} sec'.format(lock_timeout))
+        raise LockTimeout('Waited {0} seconds for lock on {1}'.format(total_wait, path))
 
-        fcntl.flock(self.lockfd, fcntl.LOCK_EX)
-        os.chmod(lock_path, stat.S_IWRITE | stat.S_IREAD)
 
-        return True
+def unlock(fd):
-
-    def unlock(self):
     '''
     Make sure lock file is available for everyone and Unlock the file descriptor
     locked by set_lock
 
-        :returns: True
+    :kw fd: File descriptor of file to unlock
     '''
-        if not self.lockfd:
-            return True
-
     try:
-            fcntl.flock(self.lockfd, fcntl.LOCK_UN)
+        filelock(fd, fcntl.LOCK_UN)
-            self.lockfd.close()
+    except ValueError:  # File was not opened, let context manager fail gracefully
-        except ValueError:  # file wasn't opened, let context manager fail gracefully
         pass
-
-        return True

lib/ansible/modules/files/lineinfile.py

@@ -184,6 +184,13 @@ 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:
@@ -208,6 +215,7 @@ 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
 
@@ -265,11 +273,18 @@ 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:
-        with open(b_dest, 'rb') as f:
+        b_lines = None
-            b_lines = f.readlines()
+
+    # 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()
 
         if module._diff:
             diff['before'] = to_native(b('').join(b_lines))
@@ -426,8 +441,9 @@ def absent(module, dest, regexp, line, backup):
             'before_header': '%s (content)' % dest,
             'after_header': '%s (content)' % dest}
 
-    with open(b_dest, 'rb') as f:
+    # NOTE: Avoid opening the same file in this context !
-        b_lines = f.readlines()
+    with open_locked(dest, module.check_mode) as fd:
+        b_lines = fd.readlines()
 
         if module._diff:
             diff['before'] = to_native(b('').join(b_lines))

lib/ansible/modules/system/known_hosts.py

@@ -84,7 +84,6 @@ 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

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

test/integration/targets/file_lock/inventory

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

test/integration/targets/file_lock/runme.sh

@@ -0,0 +1,6 @@
+#!/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

@@ -0,0 +1,45 @@
+---
+- 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

@@ -0,0 +1,63 @@
+---
+- 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