Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
worker/uniter: Remove temp download files #6314
Conversation
mjs
added some commits
Sep 23, 2016
| + for _, entry := range entries { | ||
| + path := filepath.Join(downloadDir, entry.Name()) | ||
| + if err = os.RemoveAll(path); err != nil { | ||
| + return |
mjs
Sep 23, 2016
•
Edited 1 time
-
mjs
Sep 23, 2016
Contributor
-
Sep 23, 2016
Thinking about it, this should probably keep going with other entries even if one fails to remove to maximise the amount of disk space returned to the OS.
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
+1, though I'm not sure how helpful it'll be in practice. Assuming it's a transient error, the worker should restart and eventually clean them all up.
| @@ -236,6 +236,9 @@ func (u *Uniter) init(unitTag names.UnitTag) (err error) { | ||
| u.storage = storageAttachments | ||
| u.addCleanup(storageAttachments.Stop) | ||
| + if err := charm.ClearDownloads(u.paths.State.BundlesDir); err != nil { | ||
| + return errors.Annotatef(err, "cannot clear downloads") |
mjs
Sep 23, 2016
Contributor
This probably shouldn't be fatal should it?... Might be best to just warn instead.
axw
Sep 23, 2016
Member
I'd prefer if we would just bail, but let's warn for now to be on the safe side. Something external could block the deletion of the directory/files by holding onto them. Shouldn't happen, but it's possible, and I don't think we should risk that in a maintenance branch.
axw
approved these changes
Sep 23, 2016
LGTM, but I think we could probably simplify ClearDownloads.
| +func ClearDownloads(bundlesDir string) (err error) { | ||
| + defer errors.DeferredAnnotatef(&err, "unable to clear bundle downloads") | ||
| + | ||
| + downloadDir := downloadsPath(bundlesDir) |
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
Maybe just os.RemoveAll(downloadDir)? The downloader will recreate it as necessary.
| + for _, entry := range entries { | ||
| + path := filepath.Join(downloadDir, entry.Name()) | ||
| + if err = os.RemoveAll(path); err != nil { | ||
| + return |
mjs
Sep 23, 2016
•
Edited 1 time
-
mjs
Sep 23, 2016
Contributor
-
Sep 23, 2016
Thinking about it, this should probably keep going with other entries even if one fails to remove to maximise the amount of disk space returned to the OS.
axw
Sep 23, 2016
•
Edited 1 time
-
axw
Sep 23, 2016
Member
-
Sep 23, 2016
+1, though I'm not sure how helpful it'll be in practice. Assuming it's a transient error, the worker should restart and eventually clean them all up.
| @@ -236,6 +236,9 @@ func (u *Uniter) init(unitTag names.UnitTag) (err error) { | ||
| u.storage = storageAttachments | ||
| u.addCleanup(storageAttachments.Stop) | ||
| + if err := charm.ClearDownloads(u.paths.State.BundlesDir); err != nil { | ||
| + return errors.Annotatef(err, "cannot clear downloads") |
mjs
Sep 23, 2016
Contributor
This probably shouldn't be fatal should it?... Might be best to just warn instead.
axw
Sep 23, 2016
Member
I'd prefer if we would just bail, but let's warn for now to be on the safe side. Something external could block the deletion of the directory/files by holding onto them. Shouldn't happen, but it's possible, and I don't think we should risk that in a maintenance branch.
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
mjs commentedSep 23, 2016
The temporary files created during charm downloads were being left behind if an error occured. In some cases (a repeating permanent error) this would lead to disk space exhaustion.
The uniter will now also clear any temporary charm download files for the unit it manages when it starts. Although this shouldn't happen now, previous versions of Juju could leave a significant amount of data behind from failed download attempts.
This completes the fixes for https://bugs.launchpad.net/juju/+bug/1626304.