Code review comment for lp:~vishvananda/nova/kill-objectstore

On Mar 4, 2011, at 9:02 AM, Jay Pipes wrote:

> Review: Needs Fixing
> Hi Vish, nice work.
>
> A question:
>
> 300 + for ec2_id in image_id:
> 301 + try:
> 302 + image = self.image_service.show(context, ec2_id)
>
> Should line 302 be this, instead?
>
> image = self.image_service.show(context, ec2utils.ec2_id_to_id(ec2_id))

I had wrapped all of the ids in the S3 service instead of cloud because initially I thought that it would need that info for metadata and such. Turns out it was unneccessary, so following your (sort of) suggestion, I went ahead and moved the mapping back into cloud and made the S3 service deal only with internal ids. So I suppose a non-ec2 cloud could theoretically use the S3 image service.

>
> Also, these lines in CloudController.register_image():
>
> 322 + image = {"image_location": image_location}
> 323 + image_id = self.image_service.create(context, image)
>
> There is a "location" field already for images in Glance. Simply setting image['location'] = image_location would trigger Glance to store the location of the image along with the other image information. It would also mean there would be no need to set the Content-type header to 'application/octet-stream' in the Glance patch you mention in the commit message, and therefore there would be no need for that patch to Glance to get this to work.

Changing location is not helpful. When i talked to rick about it, he mentioned that location is a field used by the glance backend. I'm using image_location as (an ec2 specific) field to represent which bucket the manifest came from.

The application/octet-stream in the other patch is only there because that is what is set in image_create in the glance client. I don't know if it is necessary or not.
(Side note: I'm attempting to construct a test case for the glance issue)

>
> Not sure how the local image service would handle 'location', but probably the same as it handles 'image_location'...
>
> Other than that, looks very good.
>
> -jay
> --
> https://code.launchpad.net/~vishvananda/nova/kill-objectstore/+merge/52163
> You are the owner of lp:~vishvananda/nova/kill-objectstore.