|
|
|
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)
Patch Set 1 #
Total comments: 6
Patch Set 2 : Supporting for forcing upgrades when unit not in started state. #
Total comments: 1
Patch Set 3 : Supporting for forcing upgrades when unit not in started state. #
Total comments: 8
Total messages: 10
|
hazmat
Please take a look.
|
13 years, 10 months ago (2012年03月07日 06:21:48 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
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
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.
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.
Please take a look.
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?
Please take a look.
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.
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?
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.