Destroying a subordinate shouldn't result in tracebacks

Destroying a subordinate shouldn't result in tracebacks

Destroying a subordinate service after destroying its primary services results in a traceback
This patch corrects this. The associated test also previously failed to exercise all the needed
behavior to demonstrate this, which is corrected as well.

https://code.launchpad.net/~bcsaller/juju/subordinate-removes/+merge/102427

(do not edit description out of merge proposal)

[bcsaller, 2012-05-25 19:49:17]

Please take a look.

[hazmat, 2012-05-30 10:11:10]

Needs Fixing


There seem to be two separate implementations of unrelated issues here that
would ideally be in separate branches. One for open/close port. And one for
subordinate suicide on removal. I'm focusing this review on subordinate suicide
aspect.

https://codereview.appspot.com/6256054/diff/1/juju/control/remove_relation.py
File juju/control/remove_relation.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/control/remove_relation.py...
juju/control/remove_relation.py:72: has_principal = True
this should default to false, right?

https://codereview.appspot.com/6256054/diff/1/juju/control/remove_relation.py...
juju/control/remove_relation.py:79: service_pair.insert(0, service)
pls document here why the subordinate is coming first in the ep pair. its easy
to gloss over this is just an error message formatting thing.

https://codereview.appspot.com/6256054/diff/1/juju/control/tests/test_destroy...
File juju/control/tests/test_destroy_service.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/control/tests/test_destroy...
juju/control/tests/test_destroy_service.py:138: # resulted in a traceback
This doesn't actually test anything related to the branch afaics. The actual
problem is asynchronously triggered.

https://codereview.appspot.com/6256054/diff/1/juju/hooks/tests/test_invoker.py
File juju/hooks/tests/test_invoker.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/hooks/tests/test_invoker.p...
juju/hooks/tests/test_invoker.py:1480: """Verify that port hook commands run and
changes are immediate."""
does this change belong in this branch?

https://codereview.appspot.com/6256054/diff/1/juju/state/tests/test_firewall.py
File juju/state/tests/test_firewall.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/state/tests/test_firewall....
juju/state/tests/test_firewall.py:189: """Verify that opening/closing ports
triggers the appropriate firewall
does this change belong in this branch?

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py
File juju/unit/lifecycle.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py#newcode458
juju/unit/lifecycle.py:458: def _undeploy(self, service_relation):
method name is a bit ambigious its always a suicide op, ie. _undeploy_self..

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py#newcode459
juju/unit/lifecycle.py:459: """Conditionally stop and remove _this_ agents
deployment.
s/agents/subordinate agent

https://codereview.appspot.com/6256054/diff/1/juju/unit/lifecycle.py#newcode500
juju/unit/lifecycle.py:500: yield unit_deployer.start("subordinate")
are you sure about that? i don't see anything obvious there, if so its a bug in
the deployer.. start would imply deploying again.. which is bogus.

https://codereview.appspot.com/6256054/diff/1/juju/unit/workflow.py
File juju/unit/workflow.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/unit/workflow.py#newcode229
juju/unit/workflow.py:229: if not stat:
how does this help? what if the node is already gone when the retry_change
fires. also untested.