Supporting for forcing upgrades when unit not in started state.

Supporting for forcing upgrades when unit not in started state.

Per user feedback/request. Supporting for forcing upgrades when unit not 
in started state. upgrade-charm hooks are not executed on a force upgrade.

https://code.launchpad.net/~hazmat/juju/force-upgrade/+merge/96299

(do not edit description out of merge proposal)

[hazmat, 2012-03-07 06:21:48]

Please take a look.

[bcsaller, 2012-03-07 22:38:42]

This looks good, approved with the minors below. Given how much of the delta is
related to the exception condition being added I'm not sure I get the motivation
for that.

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py
File juju/state/service.py (right):

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py#newcode1034
juju/state/service.py:1034: self._upgrade_flag_path,
yaml.dump(dict(force=force)))
I don't know if you noted this change to the golang team or not but they should
know the new content is there.

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py#newcode1036
juju/state/service.py:1036: raise
ServiceUnitUpgradeAlreadyEnabled(self.unit_name)
I think the old behaviour was fine here. Is there a strong reason to raise? Its
like doing a apt-get upgrade, you get a "nothing to do" style message, but its
not an error. Calling this many times might not be how we want it used, but
don't damage us.

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

https://codereview.appspot.com/5752069/diff/1/juju/state/tests/test_errors.py...
juju/state/tests/test_errors.py:109: "Service unit 'wordpress/0' is already
marked for upgrade.")
again, not sure this is needed

[hazmat, 2012-03-07 23:16:58]

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py
File juju/state/service.py (right):

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py#newcode1034
juju/state/service.py:1034: self._upgrade_flag_path,
yaml.dump(dict(force=force)))
On 2012/03/07 22:38:43, bcsaller wrote:
> I don't know if you noted this change to the golang team or not but they
should
> know the new content is there.

Its painfully trivial, but your right.

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py#newcode1036
juju/state/service.py:1036: raise
ServiceUnitUpgradeAlreadyEnabled(self.unit_name)
On 2012/03/07 22:38:43, bcsaller wrote:
> I think the old behaviour was fine here. Is there a strong reason to raise?
Its
> like doing a apt-get upgrade, you get a "nothing to do" style message, but its
> not an error. Calling this many times might not be how we want it used, but
> don't damage us.  

To me the problem arises when its a delta state. ie i upgrade, and then i
upgrade --force. We'd have to do some extra poking at the node to ensure its not
a conflict, and then update if it is (actually retry_changes does a nice job
with this). If it sounds sensible then we can go ahead with that.

[bcsaller, 2012-03-07 23:19:14]

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py
File juju/state/service.py (right):

https://codereview.appspot.com/5752069/diff/1/juju/state/service.py#newcode1036
juju/state/service.py:1036: raise
ServiceUnitUpgradeAlreadyEnabled(self.unit_name)
sounds reasonable 

On 2012/03/07 23:16:58, hazmat wrote:
> On 2012/03/07 22:38:43, bcsaller wrote:
> > I think the old behaviour was fine here. Is there a strong reason to raise?
> Its
> > like doing a apt-get upgrade, you get a "nothing to do" style message, but
its
> > not an error. Calling this many times might not be how we want it used, but
> > don't damage us.  
> 
> To me the problem arises when its a delta state. ie i upgrade, and then i
> upgrade --force. We'd have to do some extra poking at the node to ensure its
not
> a conflict, and then update if it is (actually retry_changes does a nice job
> with this). If it sounds sensible then we can go ahead with that.

[hazmat, 2012-03-17 22:07:14]

Please take a look.

[niemeyer, 2012-03-19 22:15:44]

https://codereview.appspot.com/5752069/diff/6001/juju/control/upgrade_charm.py
File juju/control/upgrade_charm.py (right):

https://codereview.appspot.com/5752069/diff/6001/juju/control/upgrade_charm.p...
juju/control/upgrade_charm.py:56: dry_run, force):
Isn't there something missing below?

[hazmat, 2012-03-20 03:04:33]

Please take a look.

[hazmat, 2012-03-20 03:05:16]

On 2012/03/19 22:15:44, niemeyer wrote:
> 
> juju/control/upgrade_charm.py:56: dry_run, force):
> Isn't there something missing below?

fixed, thanks.

[bcsaller, 2012-03-22 00:40:22]

Some minors

https://codereview.appspot.com/5752069/diff/10001/juju/control/tests/test_des...
File juju/control/tests/test_destroy_environment.py (right):

https://codereview.appspot.com/5752069/diff/10001/juju/control/tests/test_des...
juju/control/tests/test_destroy_environment.py:71: @inlineCallbacks
I hate it when that happens...

https://codereview.appspot.com/5752069/diff/10001/juju/state/errors.py
File juju/state/errors.py (right):

https://codereview.appspot.com/5752069/diff/10001/juju/state/errors.py#newcod...
juju/state/errors.py:174: 
Missing a test for this? Didn't see it.

https://codereview.appspot.com/5752069/diff/10001/juju/state/service.py
File juju/state/service.py (right):

https://codereview.appspot.com/5752069/diff/10001/juju/state/service.py#newco...
juju/state/service.py:1045: raise
ServiceUnitUpgradeAlreadyEnabled(self.unit_name)
Is this important vs silently moving on? It makes sense to only check it if the
values are different like you do, but not sure if its worth the fallout.

https://codereview.appspot.com/5752069/diff/10001/juju/state/tests/test_error...
File juju/state/tests/test_errors.py (right):

https://codereview.appspot.com/5752069/diff/10001/juju/state/tests/test_error...
juju/state/tests/test_errors.py:104: error =
ServiceUnitDebugAlreadyEnabled("wordpress/0")
Looks like this is the wrong exception? The name doesn't reflect the message and
its not what was added in state/errors

https://codereview.appspot.com/5752069/diff/10001/juju/unit/lifecycle.py
File juju/unit/lifecycle.py (left):

https://codereview.appspot.com/5752069/diff/10001/juju/unit/lifecycle.py#oldc...
juju/unit/lifecycle.py:272: fire_hooks = True
Is this only to support testing? Do we want this in practice?

[hazmat, 2012-03-22 01:25:58]

Thanks for the review, besides the minors if you think its okay, please mark it
approved.

https://codereview.appspot.com/5752069/diff/10001/juju/state/service.py
File juju/state/service.py (right):

https://codereview.appspot.com/5752069/diff/10001/juju/state/service.py#newco...
juju/state/service.py:1045: raise
ServiceUnitUpgradeAlreadyEnabled(self.unit_name)
On 2012/03/22 00:40:22, bcsaller wrote:
> Is this important vs silently moving on? It makes sense to only check it if
the
> values are different like you do, but not sure if its worth the fallout.

i think it is, the user requested an activity, the system silently doing a
different activity is an error. this is path is directly attached to the cli.

https://codereview.appspot.com/5752069/diff/10001/juju/state/tests/test_error...
File juju/state/tests/test_errors.py (right):

https://codereview.appspot.com/5752069/diff/10001/juju/state/tests/test_error...
juju/state/tests/test_errors.py:104: error =
ServiceUnitDebugAlreadyEnabled("wordpress/0")
On 2012/03/22 00:40:22, bcsaller wrote:
> Looks like this is the wrong exception? The name doesn't reflect the message
and
> its not what was added in state/errors
ugh.. good catch. thanks.

https://codereview.appspot.com/5752069/diff/10001/juju/unit/lifecycle.py
File juju/unit/lifecycle.py (left):

https://codereview.appspot.com/5752069/diff/10001/juju/unit/lifecycle.py#oldc...
juju/unit/lifecycle.py:272: fire_hooks = True
On 2012/03/22 00:40:22, bcsaller wrote:
> Is this only to support testing? Do we want this in practice?

hmm.. good catch, this looks erroneous, should have conditional on force.