/juju

clean up uniter downloads #6325

Merged
merged 7 commits into from Sep 27, 2016

Conversation

Reviewers

@howbazaar howbazaar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@mjs @jujubot @howbazaar
@mjs
Contributor

mjs commented Sep 26, 2016

This PR fixes https://bugs.launchpad.net/juju/+bug/1626304 for 2.0. It begins with significant refactoring of the downloads package, removing unused functionality and unnecessary complexity. This also ensure that temporary download files are always removed if downloads or download verification fails.

The second part of the PR changes the uniter worker to remove its temporary downloads directory in case files from previous failed download attempts have been left behind.

QA

Deploy a charm while running while true; do echofind /var/lib/juju/agents/; sleep 0.05; done on the machine. With this it was possible to see the charm's download files appear in the correct location and then moved to the correct final location.

A large bundle (openstack-base) was deployed and logs were checked to ensure that charm downloads succeeded.

A unit's charm downloads directory was populated with files and the agent was restarted. The downloads directory was removed on startup.

mjs added some commits Sep 26, 2016

@mjs
downloader: Big clean up 
The Download and Downloader were unnecessarily complicated with more
than one way to do things and confused aborting of downloads.

- Check req.Verify closer to where it's used to minimise the possibility
  of mistakes.
- Fixed/removed racy tests for stopping downloads. This also shaved 10s
  of the package test runtime.
- Removed the use of both a tomb and a channel to abort.
  downloads. Downloads are now only aborted via a channel passed in at
  download start time.
- The downloaded file is now removed if verification fails.
- The download filename is passed around instead of using a *os.File
  which needs to be re-seeked back to the start of file all the time.
- Removed Downloader most tests which were actually functionality which
  is already being tested on Download.
4a88d70
@mjs
downloader: Allow downloads to actually be aborted 
Previously the calls waiting for a download to be finish could be
aborted but the downloads themselves would continue.
f376b9d
@mjs
downloader: Avoid noise in download error 
Do't annotate to avoid unnecessary information in the error.
f1f40de
@mjs
worker/uniter/charm: Added ClearDownloads 
This can be used to clear out any temporary download files that may be
hanging around in a unit's charm download directory.
f1a99c0
@mjs
worker/uniter/charm: Test downloads are cleared
233b277
@mjs
worker/uniter: Clear temp download files on start 
The uniter will now clear any temporary charm download files for itself
when it starts. Although this shouldn't happen now, previous versions of
Juju could leave a signiciant amount of data behind from failed download
attempts.

Fixes https://bugs.launchpad.net/juju/+bug/1626304
52e5c73
@howbazaar

howbazaar reviewed Sep 26, 2016
View changes

Generally good, just wondering a few things.

downloader/download_test.go
- status := <-dl.Done()
- c.Assert(status.Err, jc.ErrorIsNil)
-
+ filename, err := dl.Wait()
@howbazaar

howbazaar Sep 26, 2016

Owner

This looks much nicer.

worker/uniter/charm/bundles.go
@@ -71,11 +70,12 @@ func (d *BundlesDir) download(info BundleInfo, target string, abort <-chan struc
expectedSha256, err := info.ArchiveSha256()
req := downloader.Request{
URL: curl,
- TargetDir: d.downloadsPath(),
+ TargetDir: downloadsPath(d.path), // XXX check this
@howbazaar

howbazaar Sep 26, 2016

Owner

What's the XXX for?

@mjs

mjs Sep 26, 2016

Contributor

To remind me to check that this was definitely correct when QAing (which I did). Removing the XXX now.

worker/uniter/charm/bundles.go
// downloadsPath returns the path to the directory into which charms are
// downloaded.
-func (d *BundlesDir) downloadsPath() string {
- return path.Join(d.path, "downloads")
+func downloadsPath(bunsDir string) string {
@howbazaar

howbazaar Sep 26, 2016 •
Edited 1 time

Owner

bunsDir? Why not just 'dir' ? Or 'base' ?

@mjs

mjs Sep 26, 2016

Contributor

To make it clear that it's supposed to receive the bundles directory. bunsDir is already used throughout this file so I was being consistent.

@mjs
Contributor

mjs commented Sep 26, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Sep 26, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit e152e58 into juju:master Sep 27, 2016

@mjs mjs deleted the mjs:1626304-clean-up-uniter-downloads branch Sep 27, 2016

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