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

lineinfile file locking support #47322

Open
wants to merge 29 commits into
base: devel
from

Conversation

@acalm
Contributor

acalm commented Oct 18, 2018

SUMMARY

Added support for file locking through common.file.FileLock (fixes #30413). Also fixed issue where FileLock.set_lock in module_utils/common/file.py didn't properly handle the default value None

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

lineinfile
module_utils/common/file.py

ANSIBLE VERSION
ansible 2.8.0.dev0 (fix-30413 2455b96cc1) last updated 2018/10/19 00:04:07 (GMT +200)
  config file = None
  configured module search path = ['/home/acalm/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/acalm/venv/ansible/lib/python3.6/site-packages/ansible
  executable location = /home/acalm/venv/ansible/bin/ansible
  python version = 3.6.5 (default, Apr 20 2018, 08:08:06) [GCC 6.4.0]
ADDITIONAL INFORMATION

N/A

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 18, 2018

Hi @acalm, thank you for submitting this pull-request!

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 18, 2018

@ansibot

This comment was marked as resolved.

Contributor

ansibot commented Oct 18, 2018

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/files/lineinfile.py:216:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/files/lineinfile.py:551:1: E305 expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

lib/ansible/modules/files/lineinfile.py:0:0: E309 version_added for new option (lock) should be 2.8. Currently 0.0
lib/ansible/modules/files/lineinfile.py:0:0: E309 version_added for new option (lock_timeout) should be 2.8. Currently 0.0
lib/ansible/modules/files/lineinfile.py:0:0: E327 Default value from the documentation ('None') is not compatible with type 'int' defined in the argument_spec
lib/ansible/modules/files/lineinfile.py:203:4: E311 EXAMPLES is not valid YAML

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

lib/ansible/modules/files/lineinfile.py:203:4: error EXAMPLES: syntax error: expected <block end>, but found '<block mapping start>'

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Oct 18, 2018

@mkrizek mkrizek removed the needs_triage label Oct 19, 2018

Andreas Calminder

@acalm acalm closed this Oct 19, 2018

@acalm acalm reopened this Oct 19, 2018

@acalm acalm closed this Oct 19, 2018

@acalm acalm reopened this Oct 19, 2018

@samdoran

Can you please add integration tests for this new feature?

@dagwieers

This comment has been minimized.

Member

dagwieers commented Oct 19, 2018

I would prefer we fix this for all our modules, rather than do it for a single module.
There's a proposal for this at: ansible/proposals#113

So basically I would not add module parameters, but rather have file locking (on write-access) enabled by default (once it has proven to work and a negligible overhead). This would ensure that concurrent Ansible-runs would not trip over each other.

The nice thing about using flock() is that the lock automatically disappears with the process.

@acalm

This comment has been minimized.

Contributor

acalm commented Oct 20, 2018

@dagwieers I'm using the file locking feature available in module_utils/common/file.py which utilize flock. I probably should've used another variable name than flock when instantiating FileLock() as it can be a bit confusing. Also adding confusion by adding a small fix for module_utils/common/file.py in the same PR.

@dagwieers

This comment has been minimized.

Member

dagwieers commented Oct 20, 2018

@acalm So what you are doing is fine from the module-level point of view, but what I think is more worthwhile is that we have wrapper-functions for various file-level operations that make it easier for any module to support flock. So rather than doing this one-time for the lineinfile module, we should be looking at what is needed for doing this for every module in a more concerted effort.

@acalm acalm closed this Oct 20, 2018

@ansibot ansibot added needs_revision and removed core_review labels Dec 1, 2018

@dagwieers

This comment has been minimized.

Member

dagwieers commented Dec 1, 2018

@acalm So that's one point for discussion. I would take a sensible default (what is sensible in our context ?) but allow the possibility to increase that value per task (and maybe 0 or -1 means that it is disabled ?).

Would a value of 10 or 15 seconds be a safe value, or instead take something like 30 seconds ? What operations in this case would take longer than this ? Would it make sense to allow modules to influence the timeout value because some modules are heavier than others ? This could provide a better user experience (when there are concurrency-related issues).

@dagwieers

This comment has been minimized.

Member

dagwieers commented Dec 1, 2018

Some references on file locking on Unix:

So it seems the best portable option is using python's fnctl.lockf() implementation (which apparently is using fnctl() and not lockf() internally)

cc @abadger

@dagwieers

This comment has been minimized.

Member

dagwieers commented Dec 1, 2018

The tests fail because some files don't exist during testing.

  File "/tmp/ansible_lineinfile_payload_UXnveB/ansible_lineinfile_payload.zip/ansible/module_utils/common/file.py", line 64, in lock_file
  File "/tmp/ansible_lineinfile_payload_UXnveB/ansible_lineinfile_payload.zip/ansible/module_utils/common/file.py", line 88, in set_lock
IOError: [Errno 2] No such file or directory: '/tmp/ansible-local-\xe6\xb1\x89\xe8\xaf\xad/\xe6\xb1\x89\xe8\xaf\xad.txt'
@acalm

This comment has been minimized.

Contributor

acalm commented Dec 1, 2018

The tests fail because some files don't exist during testing.

  File "/tmp/ansible_lineinfile_payload_UXnveB/ansible_lineinfile_payload.zip/ansible/module_utils/common/file.py", line 64, in lock_file
  File "/tmp/ansible_lineinfile_payload_UXnveB/ansible_lineinfile_payload.zip/ansible/module_utils/common/file.py", line 88, in set_lock
IOError: [Errno 2] No such file or directory: '/tmp/ansible-local-\xe6\xb1\x89\xe8\xaf\xad/\xe6\xb1\x89\xe8\xaf\xad.txt'

It might be happening due to the unicode filenames, I thought that was fixed before using lock_path_b = to_bytes(path, errors='surrogate_or_strict'). Running the tests locally doesn't trigger this, any pointer?

@acalm

This comment has been minimized.

Contributor

acalm commented Dec 1, 2018

@acalm So that's one point for discussion. I would take a sensible default (what is sensible in our context ?) but allow the possibility to increase that value per task (and maybe 0 or -1 means that it is disabled ?).

0 is basically, don't wait at all, try and lock the file and fail if the lock cannot be acquired.
Negative values and None (default) disables timeout and will wait forever.

Would a value of 10 or 15 seconds be a safe value, or instead take something like 30 seconds ? What operations in this case would take longer than this ? Would it make sense to allow modules to influence the timeout value because some modules are heavier than others ? This could provide a better user experience (when there are concurrency-related issues).

I think 15s is a sensible default for most users, although a higher value would only be a nuisance if something goes wrong and it would take 30s to fail.
It's probably useful for modules to be able to change the timeout if they notice failures due to timeouts, but I'm not familiar enough to have a strong opinion regarding this.

@bcoca

This comment has been minimized.

Member

bcoca commented Dec 3, 2018

@acalm tempfile.gettempdir() is the wrong thing to use, not only does it not guarantee every invocation will use the same dir (depends on environment settings) but it uses one outside the normal module directories. We are keeping tempfiles to the same dirs whenever possible both for functional and security reasons.

@acalm

This comment has been minimized.

Contributor

acalm commented Dec 3, 2018

@bcoca yes, this has been changed. The locks are now set on the actual file.

@acalm

This comment has been minimized.

Contributor

acalm commented Dec 5, 2018

Avoid lockf quirk, close an unrelated file descriptor to the same file as the lock, the lock is released.

acalm added some commits Dec 8, 2018

acalm
acalm
b_dest = to_bytes(dest, errors='surrogate_or_strict')
with open(b_dest, 'rb') as f:
b_lines = f.readlines()

This comment has been minimized.

@abadger

abadger Dec 8, 2018

Member

Looking good. I'd do a little more reorganization of the content, though.

  • Move these lines for reading from the file into present()
  • Move the write_changes() call into present().
  • Move the module.exit_json() into present() (or higher, into main(), but I can see that it was in present() before so it's a preexisting problem).
  • Change what we're passing in to _present_data_manipulator so that we do not pass in things that have to do with the file (module, dest, and backup), but it does include b_lines.
  • Assembling the diff results and checking file attributes probably gets moved out to the higher level as well, although I'm not entirely happy with that.... those seem to float in the middle of the overall organization that we're shooting for.
  • (For a separate PR) we can also change _present_data_manipulator() to return a copy of b_lines instead of modifying the data in place. In place uses less memory but a function without side effects is generally better code. (As an example, think about the additional flexibility of python's sorted() function over $iterable.sort()

The object is to make this helper function be only about manipulating the data and returning the new data. Everything about the file itself or the fact that we're part of an Ansible module rather than some other script is removed from the helper. This is a better organization for people to read in the future as all of the inputs and all of the outputs of _present_data_manipulator then become obvious to someone who is reading the code.

One way to judge this is to think of how you'd unit test this new function. If you succeed in moving everything about file handling and module outside of the new function, then the new function is very straightforward to unittest. You just pass in different values of b_lines (the data to be manipulated) and the options which change behaviour, regexp, line, insertbefore, insertafter, backrefs, and firstmatch, and then you see whether the lines that are output are what you expected or not.

By contrast, if the file manipulation and module is part of the new code, then you end up having to mock file objects and AnsibleModule in order to unit test the code.

l_wait = 0.1
r_exception = IOError
if sys.version_info[0] == 3:
r_exception = BlockingIOError
self.lockfd = open(lock_path, 'w')
self.lockfd = open(b_lock_path, 'ab')

This comment has been minimized.

@abadger

abadger Dec 11, 2018

Member

We'll need to change this to raise an IOError if the file does not exist. With the previous code, this was okay because we were opening and locking a temporary file but now that we're operating on the real file we need to ensure that we don't end up creating a nonexistent file in check mode.

@@ -248,15 +252,37 @@ def present(module, dest, regexp, line, insertafter, insertbefore, create,
try:
os.makedirs(b_destpath)

This comment has been minimized.

@abadger

abadger Dec 11, 2018

Member

Once the locking code is updated to raise IOError if the file does not exist, we'll have to create the file here. I believe we can do that like:

open(b_dest, 'ab').close()

I believe using append mode, if the file doesn't exist, it will be created. If the file does exist before the filehandle is closed, it won't be modified.

b_lines = []
else:
with flock.lock_file(dest):
with open(b_dest, 'rb') as f:
b_lines = f.readlines()

This comment has been minimized.

@abadger

abadger Dec 11, 2018

Member

Once we have the flock.lock_file() code raising an IOError if the file does not exist, we can rewrite this section like this:

try:
    with flock.lock_file(dest):
        with open(b_dest, 'rb') as f:
            b_lines = f.readlines()
        msg, changed, b_modified_lines = _present_data_manipulator(b_lines, regexp, line, insertafter,
                                                                   insertbefore, backrefs, firstmatch)
        if changed and not module.check_mode:
            if backup and os.path.exists(b_dest):
                backupdest = module.backup_local(dest)
            write_changes(module, b_modified_lines, dest)
except IOError as e:
    if not module.check_mode and 'file doesn't exist when locking' in to_native(e):
        raise
    # Check mode won't create the file so we know it starts off empty
    b_lines = []
    msg, changed, b_modified_lines = _present_data_manipulator(b_lines, regexp, line, insertafter,
                                                                   insertbefore, backrefs, firstmatch)

diff['before'] = to_native(b('').join(b_lines))
[...]

One thing I don't know is whether check_file_attrs should be inside of the lock. If it does, I think it can still be added in up there but there would need to be further rearrangement or another test of whether we're in check_mode or not.

This comment has been minimized.

@acalm

acalm Dec 11, 2018

Contributor

I wrapped it in if/else, to prevent locking while running in check_mode, I think it's cleaner even though there's some duplicated code. I left check_file_attrs inside the lock to keep the results consistent.

@ansibot ansibot added core_review and removed needs_revision labels Dec 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment