Fix status provider interaction.

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)

[hazmat, 2012-07-25 15:46:40]

Please take a look.

[gz, 2012-08-02 13:56:48]

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.

[hazmat, 2012-08-04 16:35:54]

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.