Code review - Issue 6256054: Destroying a subordinate shouldn't result in tracebackshttps://codereview.appspot.com/2012-05-30T10:11:10+00:00rietveld
Message from unknown
2012-05-25T19:49:07+00:00bcsallerurn:md5:f4207273bc4ec4b450dbc464f0471295
Message from bcsaller@gmail.com
2012-05-25T19:49:17+00:00bcsallerurn:md5:28b5e9ec14f2b83370eeaec4f43f4f32
Please take a look.
Message from kapilt@gmail.com
2012-05-30T10:11:10+00:00hazmaturn:md5:8bba6b465745ab00038e6c44496a9be6
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.