Merge lp://qastaging/~justin-fathomdb/nova/justinsb-api-fix-imageid into lp://qastaging/~hudson-openstack/nova/trunk

> 10 + if not image_id:

Potential issue (not sure if this happens in reality): if `imageId` returned was `0` then we'd erroneously use the `id` attribute.

`if image_id is not None` seems better in this case.

Also, if `imageId` isn't present (as is the case with Glance), wouldn't a KeyError be raised?

Wondering if it should be `image.get('imageId', None)` instead. Or even,

  image_id = image.get('imageId', image.get('id'))

Dictionary .get never throws KeyError; if the second arg isn't specified it return None if the key isn't present, so I think that's OK.

I think that if we had an imageId of 0 or '' or another value that is False in Python, we'd break everywhere, so I'd prefer to leave it as-is for clarity. Happy to fix if others disagree though...

> Dictionary .get never throws KeyError

Yup, you're right.

> so I'd prefer to leave it as-is for clarity

Sounds good.

9 + image_id = image.get('imageId')
10 + if not image_id:
11 + image_id = image.get('id')

With the patch recently introduced to make the S3ImageService behave more like the others, this shouldn't be necessary anymore. All the image services inheriting from BaseService should now be returning 'id'.

Devin is correct. It is an error if the S3ImageService (or any other image service) returns anything but 'id' as the image identifier.

In my opinion the function modified here has no business being in the nova/api/openstack/images.py code to begin with. I really dislike having these sorts of conversion and mapping functions up in the API layers.

I've re-factored the image services so that S3 and Glance image objects all implement an integer ID in this merge proposal:

https://code.launchpad.net/~dan-prince/nova/os_api_images_709355

My function in that branch already handles the ID conversion for S3:

def image_id_to_int(val):
    try:
        image_id = int(val)
    except ValueError:
        # Convert EC2-style string (i-blah) to Rackspace-style (int)
        image_id = abs(hash(val))
    return str(image_id)

---

Please consider reviewing this merge proposal (and retracting this branch which will cause me a conflict!)

Thanks,

Dan

On Thu, Feb 24, 2011 at 5:01 PM, Dan Prince <email address hidden> wrote:
> In my opinion the function modified here has no business being in the nova/api/openstack/images.py code to begin with. I really dislike having these sorts of conversion and mapping functions up in the API layers.
>
> I've re-factored the image services so that S3 and Glance image objects all implement an integer ID in this merge proposal:

We are forcing integer identifiers now?
-jay

> We are forcing integer identifiers now?
> -jay

I wouldn't use the word 'force'. The idea is that the image services are standardized. Given that some API's (aka the openstack API) require an actual ID parameter I think this makes sense.

The branch I'm proposing actually removes quite a bit of the translation code up in the images API layers just by standardizing the interfaces.

>
> > We are forcing integer identifiers now?
> > -jay
>
>
> I wouldn't use the word 'force'. The idea is that the image services are
> standardized. Given that some API's (aka the openstack API) require an actual
> ID parameter I think this makes sense.

You wrote "I've re-factored the image services so that S3 and Glance image objects all implement an integer ID". That is forcing in my opinion.

Glance does not require image identifiers to be integers, so making this change in Nova will break any Glance registry that uses non-integer keys.

> The branch I'm proposing actually removes quite a bit of the translation code
> up in the images API layers just by standardizing the interfaces.

That's fine in my book, I was just complaining about forcing integer keys.

-jay

It doesn't make sense to force integers. In EC2 land the id field is a string and is not guaranteed to be an integer. We'd just be trading one bug for another if this lands.

Can you help us understand why you needed it to be an integer to begin with?

Devin: are we still talking about my patch? If so, it has already been rejected and I've marked it work-in-progress, and I also didn't want to delete it on you and wipe your conversation.

I think the Python OpenStack bindings expect an integer, and this function is supposed to return one (presumably to keep the bindings happy) but doesn't always do so. I fixed that, but apparently this has been better fixed in the image stores.

As to _why_ it needs to be an integer, it's our favorite 'compatibility with the CloudServers API':

http://docs.rackspacecloud.com/servers/api/v1.0/xsd/image.xsd
@id type xsd:int

(That used to be a machine readable link, I don't know who HTML-ed it!)

Maybe we should be testing our API output against the CloudServers XML schema :-( I know Jay loves XML so much he'll be chomping at the bit to take on this task...