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 ConfigManager cleanup its temporary directory on destruction (LP #1959729) #259

Merged
merged 2 commits into from Feb 7, 2022

Conversation

Caligatio
Copy link
Contributor

Description

ConfigManager creates a temporary directory on instantiation and relies on the caller to clean up that temporary directory despite never exposed directly to the caller. Due to the code structure in several places, it would be onerous to call ConfigManager.cleanup() everywhere it's needed so this patch instead calls .cleanup() whenever the ConfigManager instance is garage collected.

This addresses https://bugs.launchpad.net/netplan/+bug/1959729

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage).
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@codecov-commenter
Copy link

Codecov Report

Merging #259 (de4af02) into main (49e5583) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #259   +/-   ##
=======================================
  Coverage   99.14%   99.14%           
=======================================
  Files          60       60           
  Lines       10494    10511   +17     
=======================================
+ Hits        10404    10421   +17     
  Misses         90       90           
Impacted Files Coverage Δ
netplan/configmanager.py 100.00% <100.00%> (ø)
tests/test_configmanager.py 100.00% <100.00%> (ø)

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 49e5583...de4af02. Read the comment docs.

@slyon slyon self-assigned this Feb 7, 2022
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

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

Thank you Brian, this issue has been bugging me for a long time!
Your code provides a clean and self-contained solution to this problem and I like this approach.

configmanager.py is currently being reworked heavily (in #255) but the tmpdir cleanup on destruction is a good thing to have independently. All tests and coverage are passing (I ran the integration tests locally, see below). This is ready for merging.

autopkgtest [10:44:40]: @@@@@@@@@@@@@@@@@@@@ summary
ovs                  PASS
ethernets            PASS
bridges              PASS
bonds                PASS
routing              PASS
vlans                PASS
wifi                 PASS
tunnels              PASS
scenarios            PASS
regressions          PASS
autostart            PASS
cloud-init           PASS

@slyon slyon merged commit ba88341 into canonical:main Feb 7, 2022
@Caligatio Caligatio deleted the cm-tmp-fix branch February 7, 2022 12:07
@schopin-pro
Copy link
Contributor

Late to the party, but thanks for looking into this. I'd noticed a whole bunch of netplan dirs in /tmp a while back but I assumed it was operator error (I'd usually force-kill the test suite a bunch of time when investigating stuff with gdb).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants