Code review comment for lp:~yamahata/nova/boot-from-volume-0

On Wed, Jun 08, 2011 at 09:07:25PM -0000, Matt Dietz wrote:
> I see a few failing tests:

I think you're using old mox version.
With revno 1117, mox 0.5.3 is required instead of 0.5.0
Updating your repo and reinstalling evenv will fix it.

> 455 + try:
> 456 + bdms = self.db.block_device_mapping_get_all_by_instance(
> 457 + context, instance_id)
> 458 + except exception.NotFound:
> 459 + pass
>
> I don't really like throwing away exceptions. It's an explicit exception, sure, but it could mask something important. Seems like this should at least log the error.

I see. Given all the caller catch and ignores it, I changed it returns
empty list instead of raising NotFound. Thus I eliminated except ...: pass.

> 476 + assert ((bdm['snapshot_id'] is None) or
> 477 + (bdm['volume_id'] is not None))
>
> It seems like it would be better to raise an explicit exception here, with a message describing exactly why this is a bad state.

Okay.

> 239 + """Stop each instance in instace_id"""
> 245 + """Start each instance in instace_id"""
>
> instace_id should be instance_id
>
> Also, given that instance_id is singular, it should probably say something like:
>
> "Start the instance denoted by instance_id"

Unfortunately instance_id is a list of ec2 instance id like
terminate_instances. Anyway I fixed typo.
--
yamahata