Skip to content

Conversation

@mw0000001
Copy link
Contributor

Fix for closing filehandle of tempfile and delete it. (mkstemp, is not closing os filehandle automatically)
computemgtd was running into a too many open files issue, every ~8 days. Which made compute node be restarted.

Description of changes

  • Describe what you're changing and why you're doing these changes.
  • Link to impacted open issues.

Tests

  • Describe the automated and/or manual tests executed to validate the patch.
  • Describe the added/modified tests.

References

  • Link to related PRs in other packages (i.e. cookbook, node).
  • Link to documentation useful to understand the changes.

Checklist

  • Make sure you are pointing to the right branch and add a label in the PR title (i.e. 2.x vs 3.x)
  • Check all commits' messages are clear, describing what and why vs how.
  • Make sure to have added unit tests or integration tests to cover the new/modified code.
  • Check if documentation is impacted by this change.

Please review the guidelines for contributing and Pull Request Instructions.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Fix for closing filehandle of tempfile and delete it. (mkstemp, is not closing os filehandle automatically)
computemgtd was running into a too many open files issue before, every ~8 days. Which made compute node be restarted.
@mw0000001 mw0000001 requested a review from a team as a code owner June 29, 2022 09:30
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #421 (cb75870) into develop (8eca5d4) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #421      +/-   ##
===========================================
+ Coverage    81.58%   81.59%   +0.01%     
===========================================
  Files           12       12              
  Lines         1732     1733       +1     
===========================================
+ Hits          1413     1414       +1     
  Misses         319      319              
Impacted Files Coverage Δ
src/slurm_plugin/computemgtd.py 61.71% <100.00%> (+0.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eca5d4...cb75870. Read the comment docs.

@demartinofra demartinofra changed the title Update computemgtd.py Fix file handler leak in computemgtd Jun 29, 2022
Copy link
Contributor

@demartinofra demartinofra left a comment

Choose a reason for hiding this comment

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

Good catch and thanks for the patch!!!

@demartinofra demartinofra merged commit ad19501 into aws:develop Jun 29, 2022
@demartinofra
Copy link
Contributor

demartinofra commented Jun 29, 2022

It's better to store the reference to NamedTemporaryFile() in the class attributes otherwise when tf gets out of scope the file is deleted. I will create a separate PR to improve this.

[EDIT] apparently the temp file is only used in _get_config, I'll just move its creation in there then.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants