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.

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

Have salt-cloud clean up tmp files #36482

Conversation

clarkperkins
Copy link
Contributor

What does this PR do?

We ran in to an issue with our root partition filling up due to thousands of tmp files being left behind by salt-cloud.

What issues does this PR fix or reference?

none

Previous Behavior

~5 tmp* files were left in the /tmp directory for every host launched with salt-cloud

New Behavior

tmp files in /tmp are cleaned up by salt-cloud

Tests written?

No

Please review Salt's Contributing Guide for best practices.

@clarkperkins clarkperkins changed the title Have salt clean up tmp files Have salt-cloud clean up tmp files Sep 21, 2016
@terminalmage
Copy link
Contributor

This still won't fix instances where an exception is raised before we get to the os.remove(). It would probably be best to put this in a try/finally.

@clarkperkins
Copy link
Contributor Author

Ah, good point - I will add that

@terminalmage
Copy link
Contributor

terminalmage commented Sep 21, 2016

You hit upon some good points in raising this issue, but I took a look and came up with my own solution which I think might work better:

https://github.com/saltstack/salt/pull/36484/files

For one, you made me realize that whoever created this issue wrote the tempfile code in the first place was leaving open file descriptors, which my PR fixes by calling os.close() on them. Also, since we only deal with the temp file in a very small block of code, the try/finally block doesn't need to be so big. Finally, if neither contents nor local_file were passed, we'd get a NameError when we build the SSH command below. Your PR would have fixed this last thing but my PR also adds warning logging which might help in troubleshooting.

What do you think?

@terminalmage
Copy link
Contributor

Actually, mkstemp returns an os-level filehandle, so opening the path for writing is entirely unnecessary, we should be using os.write() here. I'm updating my PR.

@terminalmage terminalmage added the Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged label Sep 21, 2016
@terminalmage
Copy link
Contributor

Closing in favor of #36484.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Pending-Discussion The issue or pull request needs more discussion before it can be closed or merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants