Ensure resource HTTP handlers release state #6703

Merged
merged 3 commits into from Dec 14, 2016

Conversation

Projects
None yet
4 participants
Member

babbageclunk commented Dec 14, 2016

The resource HTTP handler didn't release the state back into the state
pool - this would mean that it would stick around in the pool even after
the model was migrated away, preventing the same model from being
migrated back. (It would also leak the state if the model was destroyed.)

Thread the call to httpContext.release through so it can be called from
the right place in the resource HTTP handler.

This is the last piece of fixing https://bugs.launchpad.net/juju/+bug/1641824

QA steps:

  • bootstrap 2 controllers A and B
  • create model m in A
  • download the Mattermost binary from https://releases.mattermost.com/3.5.1/mattermost-team-3.5.1-linux-amd64.tar.gz
  • deploy Mattermost using that binary distribution
    juju deploy -m A:m cs:~cmars/mattermost --resource bdist=<path to binary distribution>
  • migrate the model to b: juju migrate -c A m B
  • then migrate it back: juju migrate -c B m A
  • the migration should be successful
Ensure resource HTTP handlers release state
Thread the call to httpContext.release through so it can be called from
the right place in the resource HTTP handler.
Member

anastasiamac commented Dec 14, 2016

Please also land equivalent on 2.1.
I suspect you may need to consider if it needs to be land on 2.0 as well ;D

mjs approved these changes Dec 14, 2016

Great.

Contributor

mjs commented Dec 14, 2016

Definitely want this in 2.1 ASAP.

Member

babbageclunk commented Dec 14, 2016

Hmm, following the QA steps didn't give me great joy. Investigating.

Close state leaks from the NewResourceOpener code
This code path is a bit trickier to close than the other handler.
Member

babbageclunk commented Dec 14, 2016

!!build!!

+// CombineErrors converts a set of errors (which might be nil) into
+// one. If there are no errors, this returns an untyped nil, and if
+// there's one error it's passed through directly.
+func CombineErrors(errs ...error) error {
@babbageclunk

babbageclunk Dec 14, 2016

Member

Do we already have something like this somewhere? It seems like we would. Where should it live if not? github.com/juju/errors?

@babbageclunk

babbageclunk Dec 14, 2016

Member

This is definitely not the right place for it, just wasn't sure what the right one was.

@mjs

mjs Dec 14, 2016

Contributor

I haven't seen anything quite like this anywhere. The closest I've seen is something that attempts to choose the "worst" error given a set of multiple errors.

juju/errors seems like a reasonable place for this to live but given that I'm not aware of something similar elsehwhere, perhaps we haven't needed it much. I'm ok with it staying here for now.

@babbageclunk

babbageclunk Dec 14, 2016

Member

Ok great - next time I need it I'll move it somewhere better.

Member

babbageclunk commented Dec 14, 2016

With the latest commit repeated migration back and forth with a resource-using charm deployed works, but it's substantially gnarlier than the original changes, so it could do with rereview.

I see what you mean about the resources code, @mjs.

mjs approved these changes Dec 14, 2016

+// CombineErrors converts a set of errors (which might be nil) into
+// one. If there are no errors, this returns an untyped nil, and if
+// there's one error it's passed through directly.
+func CombineErrors(errs ...error) error {
@babbageclunk

babbageclunk Dec 14, 2016

Member

Do we already have something like this somewhere? It seems like we would. Where should it live if not? github.com/juju/errors?

@babbageclunk

babbageclunk Dec 14, 2016

Member

This is definitely not the right place for it, just wasn't sure what the right one was.

@mjs

mjs Dec 14, 2016

Contributor

I haven't seen anything quite like this anywhere. The closest I've seen is something that attempts to choose the "worst" error given a set of multiple errors.

juju/errors seems like a reasonable place for this to live but given that I'm not aware of something similar elsehwhere, perhaps we haven't needed it much. I'm ok with it staying here for now.

@babbageclunk

babbageclunk Dec 14, 2016

Member

Ok great - next time I need it I'll move it somewhere better.

Member

babbageclunk 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

Contributor

jujubot commented Dec 14, 2016

Build failed: Tests failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-juju/9873

Member

babbageclunk commented Dec 14, 2016

Trusty tests panicked with cannot create index in statusSetterSuite.TestUnauthorized - intermittent mongo failure.

$$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 fe83e98 into juju:develop Dec 14, 2016

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@babbageclunk babbageclunk deleted the babbageclunk:resource-state-leak branch Dec 15, 2016

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

Merge pull request #6711 from babbageclunk/resource-state-leak-2.1
Backport: Ensure resource HTTP handlers release state

Backport of #6703 to the 2.1 branch.

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

Merge pull request #6713 from babbageclunk/resource-state-leak-2.0
Backport state pool leak fixes to 2.0 branch

Fix state pool leaks (gets without corresponding releases) from code using the httpContext methods to get a state object. Backports of #6566 and #6703 to the 2.0 branch.

Fixes https://bugs.launchpad.net/juju/+bug/1641824 - although migrations aren't available in 2.0, the state pool would also leak when models are destroyed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment