Skip to content

Conversation

@demartinofra
Copy link
Contributor

Description of changes

Following up on the file handler leak fix from #421, I completely removed the dependency on the temporary file in computemgtd.

Tests

  • Unit tests
  • Manual tests:
    • config reload
    • used file descriptors
    • unavailable head node

References

#421

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.

@demartinofra demartinofra requested a review from a team as a code owner June 29, 2022 12:52
@codecov
Copy link

codecov bot commented Jun 29, 2022

Codecov Report

Merging #422 (63db8ee) into develop (ad19501) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #422      +/-   ##
===========================================
- Coverage    81.59%   81.57%   -0.03%     
===========================================
  Files           12       12              
  Lines         1733     1731       -2     
===========================================
- Hits          1414     1412       -2     
  Misses         319      319              
Impacted Files Coverage Δ
src/slurm_plugin/computemgtd.py 61.11% <100.00%> (-0.61%) ⬇️

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 ad19501...63db8ee. Read the comment docs.

charlesg3
charlesg3 previously approved these changes Jun 29, 2022
CHANGELOG.md Outdated

**BUG FIXES**
- Handle corner case in the scaling logic when instance is just launched and the describe instances API doesn't report yet all the EC2 info.
- Fix file handle leak in computemgtd
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: final period.

Copy link
Contributor

Choose a reason for hiding this comment

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

minor: code syntax for: computemgtd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done thanks

Signed-off-by: Francesco De Martino <fdm@amazon.com>
@demartinofra demartinofra merged commit dc454ca into aws:develop Jun 30, 2022
@demartinofra demartinofra deleted the computemgtd-fix branch June 30, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants