Add NewTempCharmStore #110

Open
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Member

babbageclunk commented Feb 21, 2017

This uses a private charm cache dir, so we can clean it up without
worrying about other processes trying to read the files in the cache.

Juju controllers will use this when downloading charms so that they aren't
kept around - they're already cached in the blob store and they just take
up space in the filesystem that won't be reclaimed when models are
destroyed or migrated off the controller.

Part of fixing: https://bugs.launchpad.net/juju/+bug/1579976

Add NewTempCharmStore
This uses a private charm cache dir, so we can clean it up without
worrying about other processes trying to read the files in the cache.
Member

jujugui commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/charmrepo/307/

Member

babbageclunk commented Feb 21, 2017

Ha, I'd accidentally mangled the test name so it didn't run locally, then fixed the name but forgot to rerun the tests. Thanks CI!

Member

jujugui commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/charmrepo/308/

Member

babbageclunk commented Feb 21, 2017

Lots of 502 and TLS errors getting packages from gopkg.in.

!!chittychitty!!

Member

babbageclunk commented Feb 21, 2017

(Not sure whether that works to trigger a rebuild here.)

Member

jujugui commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/charmrepo/309/

charmstore.go
@@ -131,14 +140,36 @@ func (s *CharmStore) GetBundle(curl *charm.URL) (charm.Bundle, error) {
return charm.ReadBundleArchive(path)
}
+// Cleanup removes the cache directory and any charms in it.
+func (s *CharmStore) Cleanup() error {
+ return os.RemoveAll(s.cacheDir)
@howbazaar

howbazaar Feb 21, 2017

Owner

Use getCacheDir?

@babbageclunk

babbageclunk Feb 21, 2017

Member

Yup, thanks.

Use getCacheDir in Cleanup
This allows Cleanup to be used on stores backed by the global cache if
desired.
Member

jujugui commented Feb 21, 2017

Refer to this link for build results (access rights to CI server needed):
http://ci-cge.jujugui.org:8080//job/charmrepo/312/

That global CacheDir var seems like a really bad idea. Yes, it's a pre-existing condition, but if it was instead a parameter of a CharmStore constructor, then we wouldn't need as much special behavior -- a common implementation could be embedded in both global and temp charm store types.

I'd like reviews from @rogpeppe or @mhilton tomorrow; I don't have enough experience with the downstream dependents of this package to say whether this could create problems.

I don't necessarily think we need to refactor the global in this PR, but we might need to in a followup.

I haven't looked at the code here, but I think that the whole reason for the cache's existence has gone away since the Juju API was introduced in 2012, and it might be better to remove it entirely.

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