Don't schedule placeholder resource cleanup #6702

Merged
merged 4 commits into from Dec 14, 2016

Conversation

Projects
None yet
3 participants
Contributor

mjs commented Dec 14, 2016

Placeholder resources aren't stored in the blobstore and therefore don't need cleaning up. The fix has been made in two ways:

  1. Cleanups are no longer scheduled for placeholder resources.
  2. The resource cleanup code now ignores cleanups without a storage path set. This is a useful backstop but also fixes stray cleanups in existing deployments.

Fixes https://bugs.launchpad.net/juju/+bug/1649179

There was no testing at all for resource cleanup so this has also been rectified.

QA

Deployed the etcd charm as this uses a placeholder resource, and then removed it. Previously this would result in errors in the logs as the cleaner worker attempted to remove a resource which wasn't stored.

mjs added some commits Dec 14, 2016

state: Add missing tests for resource cleanup
There were no tests around resource blob removal. Also added some test
helpers around the NeedCleanup tests.
Ignore attempts to clean up a placeholder resource
These cleanups shouldn't get scheduled in the first place but this acts
as a backstop. See https://bugs.launchpad.net/juju/+bug/1649179
state: Don't schedule placeholder resource cleanup
Placeholder resources aren't stored in the blobstore and therefore don't
need cleaning up.

Fixes https://bugs.launchpad.net/juju/+bug/1649179
state/export_test.go
+func IsBlobStored(c *gc.C, st *State, storagePath string) bool {
+ stor := storage.NewStorage(st.ModelUUID(), st.MongoSession())
+ r, _, err := stor.Get(storagePath)
+ if err == nil {
@anastasiamac

anastasiamac Dec 14, 2016

Member

This reads funny, but maybe only to me...
Would it not be simpler to start with:
...
if err != nil {
if errors.isNotFound(err) {
return false
}
r.Close()
return true
}
c.Fatalf(...)
return false
}
...

@mjs

mjs Dec 14, 2016

Contributor

Fair enough, I'll rearrange.

Contributor

mjs commented Dec 14, 2016

$$merge$$

Contributor

jujubot commented Dec 14, 2016

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

@jujubot jujubot merged commit d22c32f into juju:2.1 Dec 14, 2016

@mjs mjs deleted the mjs:1649179-placeholder-resource-cleanup branch Dec 14, 2016

jujubot added a commit that referenced this pull request Dec 15, 2016

Merge pull request #6716 from mjs/1649179-placeholder-resource-cleanu…
…p-develop

Don't schedule placeholder resource cleanup

Placeholder resources aren't stored in the blobstore and therefore don't need cleaning up. The fix has been made in two ways:

1. Cleanups are no longer scheduled for placeholder resources.
2. The resource cleanup code now ignores cleanups without a storage path set. This is a useful backstop but also fixes stray cleanups in existing deployments.

Fixes https://bugs.launchpad.net/juju/+bug/1649179

There was no testing at all for resource cleanup so this has also been rectified.

This is a forward port #6702.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment