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

On Mon, Jul 25, 2011 at 05:41:30PM -0000, Brian Lamar wrote:
> Review: Needs Fixing
> 341 +# Copyright 2011 Isaku Yamahata <yamahata@valinux co jp>
>
> After asking in the last meeting, I believe code should be copyright OpenStack, LLC.
>
> 365 + # NOTE(yamahata): see image_service.s3.s3create()
> 366 + for bdm in properties.get('mappings', []):
> 367 + if bdm['virtual'] == 'root':
> 368 + root_device_name = bdm['device']
>
> This logic seems a little off, I'm not certain why you'd loop here unless there could be multiple mappings with 'virtual' == 'root'. Maybe it is a little late to bring this up, and let me know if it is, but is 'virtual' unique? It would seem that having mappings look like:

'virtual' is unique.

> {
> "ami": {"device": "/dev/sda"},
> "root": {"device": "sda"},
> "ephemeral0": {"device": "sdb"},
> }
>
> instead of:
>
> [
> {'virtual': 'ami', 'device': '/dev/sda'},
> {'virtual': 'root', 'device': 'sda'},
> {'virtual': 'ephemeral0', 'device': 'sdb'},
> ]
>
> might make the logic a bit cleaner in a lot of areas? This might not work but I thought I'd suggest it. For example, the block of code referenced earlier would become:
>
> root_device_name = mappings.get("root", None)
>
> I'm a little confused that mappings sometimes are a list and other times are a dictionary. Which one is correct?

A list as the internal representation. It's because they correspond to RDB rows.
Thus internally they are handled as lists consistently.(At least my intension is so)
The conversion from/to lists to/from dicts occurs when parsing API requests/
formatting API results if necessary.

Does it clarify?

Hmm, nova.api.ec2.cloud.CloudController._get_instance_mapping()
should be _format_instance_mapping(). I'll fix it.

> Can you give more information or point me to documentation on what "ephemeral" means in this context? Seemingly "ephemeral" devices won't persist on reboot, or something to that effect?

Vish kindly answered this.

--
yamahata