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

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

Merged
merged 3 commits into from May 9, 2018

Conversation

Projects
None yet
4 participants
@kt97679
Contributor

kt97679 commented May 2, 2018

…stack/salt/issues/33223#issuecomment-386117236

What does this PR do?

What issues does this PR fix or reference?

Previous Behavior

Remove this section if not relevant

New Behavior

Remove this section if not relevant

Tests written?

Yes/No

Commits signed with GPG?

Yes/No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@terminalmage

Rather than manually creating

# 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+')

This comment has been minimized.

@terminalmage

terminalmage May 3, 2018

Member

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.

This comment has been minimized.

@kt97679

kt97679 May 3, 2018

Contributor

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

@terminalmage

This comment has been minimized.

Member

terminalmage commented May 3, 2018

@rallytime this needs a backport once it's ready for merge.

Kirill Timofeev
@kt97679

This comment has been minimized.

Contributor

kt97679 commented May 7, 2018

@terminalmage @rallytime please let me know if I need to apply any other changes to this PR.

@kt97679

This comment has been minimized.

Contributor

kt97679 commented May 8, 2018

Thanks for review @terminalmage ! Should I merge now?

@rallytime rallytime merged commit 6916b1d into saltstack:develop May 9, 2018

5 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #4755 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #9707 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #18810 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #22670 — FAILURE
Details
WIP ready for review
Details
codeclimate All good!
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #24930 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17049 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #21669 — SUCCESS
Details

@kt97679 kt97679 deleted the kt97679:race-condition-fix branch May 9, 2018

rallytime added a commit that referenced this pull request May 10, 2018

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