|
|
|
Fix status provider interaction.
Status was processing each machine from the provider one by one instead of in
bulk. This caused spurious requests instead of usage of using the bulk api. The
existing bulk api was deficient for status usage in that it raised an error on a
missing machine instead of returning both found, missing sets.
This branch includes a change in the bulk api to allow for this and a
corresponding update to extant providers. Status now uses the bulk api resulting
in many less provider roundtrips (one for env).
https://code.launchpad.net/~hazmat/juju/big-oh-status-constant/+merge/116686
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 10
Total messages: 3
|
hazmat
Please take a look.
|
13 years, 5 months ago (2012年07月25日 15:46:40 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
The apis here are somewhat confused. It seems get_machines already won't raise MachinesNotFound unless a list of instance ids is passed in, which status does not do, so is the change to the return type actually required? The missing list will always be empty and ignored as far as I can see here. I do not see any usage of get_machines that actually passes a list of instance ids outside of provider tests, so if api changes are acceptable, perhaps the following would make more sense: If changes to this api are acceptable, I wonder if the following wouldn't make more sense: get_machines() -> machine_list # raises ProviderError get_machine(instance_id) -> machine or None # raises ProviderError And deprecate MachinesNotFound. https://codereview.appspot.com/6449044/diff/1/juju/agents/provision.py File juju/agents/provision.py (right): https://codereview.appspot.com/6449044/diff/1/juju/agents/provision.py#newcod... juju/agents/provision.py:200: yield self.provider.shutdown_machine(machine) Similar potato programming here, could use instead: machines = [provider[iid] for iid in unused] shutdown_machines(machines) Which is a win on EC2 and a slight improvement even on Nova where there isn't a multiple instance shutdown api. https://codereview.appspot.com/6449044/diff/1/juju/agents/tests/test_provisio... File juju/agents/tests/test_provision.py (right): https://codereview.appspot.com/6449044/diff/1/juju/agents/tests/test_provisio... juju/agents/tests/test_provision.py:134: machines, missing = yield self.agent.provider.get_machines() From the various test changes like this, I wonder if a better api change would be to split the get_machines function into one that takes a list of instance ids, and one that takes no arguments. As it is, 'missing' is meaningless unless ids are passed, but all these callers have to start caring about it just to ignore the value. https://codereview.appspot.com/6449044/diff/1/juju/control/status.py File juju/control/status.py (right): https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode507 juju/control/status.py:507: self.provider_machines = yield self._process_provider_machines() Not enamored of this means of passing the corresponding provider machine into _process_machine. I guess it makes it just a helper for this method rather than something that works standalone? https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode536 juju/control/status.py:536: instance_id = yield machine_state.get_instance_id() Is this still a seperate zk lookup per machine? Do we care? https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode545 juju/control/status.py:545: self.log.exception( Would be log.error, there's no exception state any more in this case. https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode571 juju/control/status.py:571: if not 'agent-state' in m: This could be an else, ah, that's what it used to be. Style choice only.
Thanks for the review. I've decided to walk back the provider changes, and just focus on the status changes. Your right in your assessment that the core of those changes could be done without the provider api modifications. The provider changes would be more appropriate as a separate branch, and presumably one that also supports bulk termination. https://codereview.appspot.com/6449044/diff/1/juju/control/status.py File juju/control/status.py (right): https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode507 juju/control/status.py:507: self.provider_machines = yield self._process_provider_machines() On 2012年08月02日 13:56:48, gz wrote: > Not enamored of this means of passing the corresponding provider machine into > _process_machine. I guess it makes it just a helper for this method rather than > something that works standalone? its just builds an machine set index for the per machine method to utilize. https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode536 juju/control/status.py:536: instance_id = yield machine_state.get_instance_id() On 2012年08月02日 13:56:48, gz wrote: > Is this still a seperate zk lookup per machine? Do we care? we do, roundtrips aren't free, but its not something that can be avoided, since each value is stored on a separate node. still switching out to a rest api will have some benefit here with a single round trip for the client/wan transfer. https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode545 juju/control/status.py:545: self.log.exception( On 2012年08月02日 13:56:48, gz wrote: > Would be log.error, there's no exception state any more in this case. done. https://codereview.appspot.com/6449044/diff/1/juju/control/status.py#newcode571 juju/control/status.py:571: if not 'agent-state' in m: On 2012年08月02日 13:56:48, gz wrote: > This could be an else, ah, that's what it used to be. Style choice only. yeah.. the nesting of if/else and for/else felt unesc dense. sadly this is another round trip location, albeit primarily of issue with new machines.