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.

Sign up for GitHub

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

Jump to bottom

fix for the race condition, details are here: https://github.com/salt… #47440

Merged
merged 3 commits into from May 9, 2018
Merged

fix for the race condition, details are here: https://github.com/salt… #47440

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d93398
fix for the race condition, details are here: https://github.com/salt…
May 2, 2018
a95f39e
addressed feedback
May 3, 2018
a433e71
Merge branch 'develop' into race-condition-fix
May 8, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions salt/fileclient.py
View file
Open in desktop
Expand Up @@ -13,6 +13,7 @@
import shutil
import ftplib
from tornado.httputil import parse_response_start_line, HTTPHeaders, HTTPInputError
import uuid

# Import salt libs
from salt.exceptions import (
Expand Down Expand Up @@ -1152,11 +1153,16 @@ def get_file(self,
load['gzip'] = gzip

fn_ = None
dest_tmp = None
if dest:
destdir = os.path.dirname(dest)
if not os.path.isdir(destdir):
if makedirs:
os.makedirs(destdir)
try:
os.makedirs(destdir)
except OSError as exc:
if exc.errno != errno.EEXIST: # ignore if it was there already
raise
else:
return False
# We need an open filehandle here, that's why we're not using a
Expand Down Expand Up @@ -1207,11 +1213,12 @@ def get_file(self,
saltenv,
cachedir=cachedir) as cache_dest:
dest = cache_dest
dest_tmp = "{0}/{1}".format(os.path.dirname(dest), str(uuid.uuid4()))
# If a directory was formerly cached at this path, then
# remove it to avoid a traceback trying to write the file
if os.path.isdir(dest):
salt.utils.files.rm_rf(dest)
fn_ = salt.utils.files.fopen(dest, 'wb+')
fn_ = salt.utils.files.fopen(dest_tmp, 'wb+')
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using dest_tmp here and renaming manually, you should use salt.utils.atomicfile.atomic_open(), which was designed exactly for this sort of use case. You can change this line to:

fn_ = salt.utils.atomicfile.atomic_open(dest, 'wb+')

Then, when the filehandle is closed, the tempfile will be automatically renamed to the dest path.

Note that using this method will require two additional changes:

  1. salt.utils.atomicfile will need to be imported here in fileclient.py.
  2. salt/utils/atomicfile.py will need to be added to salt/utils/thin.py in the min_files tuple so that it is part of the thin tarball.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comments @terminalmage , please see changes in the latest commit.

if data.get('gzip', None):
data = salt.utils.gzip_util.uncompress(data['data'])
else:
Expand Down Expand Up @@ -1242,6 +1249,8 @@ def get_file(self,

if fn_:
fn_.close()
if dest_tmp:
os.rename(dest_tmp, dest)
log.info(
'Fetching file from saltenv \'%s\', ** done ** \'%s\'',
saltenv, path
Expand Down