Merge lp://staging/~bcsaller/pyjuju/subordinate-removes into lp://staging/pyjuju

Please take a look.

Reviewers: mp+102427_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/6256054/

Affected files:
   A [revision details]
   M juju/control/destroy_service.py
   M juju/control/remove_relation.py
   M juju/control/tests/test_destroy_service.py
   M juju/control/tests/test_remove_relation.py
   M juju/hooks/tests/test_invoker.py
   M juju/lib/testing.py
   M juju/state/tests/test_firewall.py
   M juju/state/tests/test_service.py
   M juju/unit/lifecycle.py
   M juju/unit/tests/test_lifecycle.py
   M juju/unit/workflow.py

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#newcode72
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#newcode79
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_service.py
File juju/control/tests/test_destroy_service.py (right):

https://codereview.appspot.com/6256054/diff/1/juju/control/tests/test_destroy_service.py#newcode138
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.py#newcode1480
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.py#newcode189
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.

https://codereview...

so post discussions, i believe we've said this branch/approach is dead in favor of the stop protocol spec.