Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(295)

Issue 6256054: Destroying a subordinate shouldn't result in tracebacks

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by bcsaller
Modified:
10 years, 4 months ago
Reviewers:
hazmat, mp+102427
Visibility:
Public.

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)

Patch Set 1 #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+379 lines, -37 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M juju/control/destroy_service.py View 1 chunk +2 lines, -2 lines 0 comments Download
M juju/control/remove_relation.py View 1 chunk +2 lines, -2 lines 2 comments Download
M juju/control/tests/test_destroy_service.py View 4 chunks +18 lines, -4 lines 1 comment Download
M juju/control/tests/test_remove_relation.py View 1 chunk +2 lines, -2 lines 0 comments Download
M juju/hooks/tests/test_invoker.py View 2 chunks +56 lines, -0 lines 1 comment Download
M juju/lib/testing.py View 1 chunk +3 lines, -3 lines 0 comments Download
M juju/state/tests/test_firewall.py View 1 chunk +33 lines, -0 lines 1 comment Download
M juju/state/tests/test_service.py View 6 chunks +124 lines, -22 lines 0 comments Download
M juju/unit/lifecycle.py View 3 chunks +63 lines, -0 lines 3 comments Download
M juju/unit/tests/test_lifecycle.py View 1 chunk +69 lines, -0 lines 0 comments Download
M juju/unit/workflow.py View 2 chunks +5 lines, -2 lines 1 comment Download

Messages

Total messages: 2
bcsaller
Please take a look.
10 years, 4 months ago (2012-05-25 19:49:17 UTC) #1
hazmat
10 years, 4 months ago (2012-05-30 10:11:10 UTC) #2
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b