Open Stack

cc'ing ML since it's an important discussion, IMO...

On 07/31/2014 11:54 AM, Arnaud Legendre wrote:
> Hi Jay,
>
> I would be interested if you could share your point of view on this
> item: we want to make the glance stores a standalone library
> (glance.stores) which would be consumed directly by Nova and Cinder.

Yes, I have been enthusiastic about this effort for a long time now :) 
In fact, I have been pushing a series of patches (most merged at this 
point) in Nova to clean up the (very) messy nova.image.glance module and 
standardize the image API in Nova.

The messiest part of the current image API in Nova, by far, is the 
nova.image.glance.GlanceImageService.download() method, which you 
highlight below. The reason it is so messy is that the method does 
different things (and returns different things!) depending on how you 
call it and what arguments you provide. :(

> I think it would be nice to get your pov since you worked a lot on
> the Nova image interface recently. To give you an example:
>
> Here
> https://github.com/openstack/nova/blob/master/nova/image/glance.py#L333,
>  we would do:
>
> 1. location = get_image_location(image_id),
> 2. get(location) from the
> glance.stores library like for example rbd
> (https://github.com/openstack/glance/blob/master/glance/store/rbd.py#L206)

Yup. Though I'd love for this code to live in olso, not glance...

Plus, I'd almost prefer to see an interface that hides the location URIs 
entirely and makes the discovery of those location URIs entirely 
encapsulated within glance.store. So, for instance, instead of getting 
the image location using a call to glanceclient.show(), parsing the 
locations collection from the v2 API response, and passing that URI to 
the glance.store.get() function, I'd prefer to see an interface more 
like this:

```python
# This code would go in a new nova.image.API.copy() method:
import io

from oslo.image import move
from oslo.image.move import exception as mexc

from nova import exception as exc

...
     def copy(image_id_or_uri, stream_writer):
         try:
             config = {
                # Some Nova CONF options...
             }
             mover = move.Mover(image_id_or_uri, config)
             success, bytes_written = mover.copy(stream_writer)
             if success:
                 if bytes_written == 0:
                     LOG.info("Copied image %s using zero-copy "
                              "transfer.", image_id_or_uri)
                 else:
                     LOG.info("Copied image %s using standard "
                              "filesystem copy. Copied %d bytes.",
                              image_id_or_uri, bytes_written)
             return success
         except mexc.ImageNotFound:
             raise exc.NotFound(...)
         except mexc.ImageInvalidApi:
             # Fall back to pull image from Glance
             # API server via HTTP and write to disk
             # via the stream_writer argument's write()
             # interface... and return True or False
             # depending on whether write()s succeeded
```

And then, the caller of such an nova.image.API.copy() function would be 
in the existing various virt utils and imagebackends, and would call the 
API function like so:

```python
# This code would go in something like nova.virt.libvirt.utils:

from nova import image

IMAGE_API = image.API()

write_file = io.FileIO(dst_path, mode='wb')
writer = io.BufferedWriter(write_file)

image_id_or_uri = "https://images.example.com/images/123"

result = IMAGE_API.copy(image_id_or_uri, writer)
# Test result if needed...
```

Notice that the caller never needs to know about the locations 
collection of the image -- and thus we correct the leaked implementation 
details that currently ooze out of the download() method in 
nova.image.glance.GlanceImageService.download.

Also note that we no longer pass a variety of file descriptors, file 
writers, file destination paths to the download method. Instead, we 
always just pass the image ID or URI and a writeable bytestream 
iterator. And we always return either True or False, instead of None or 
a file iterator depending on the supplied arguments to download().

>  The same kind of logic could be added in Cinder.

Sure.

> We see that as a benefit for Nova, which would be able to directly
> consume the stores instead of going through the glance api.

Exactly.

> We had a vote today to figure out if we continue the effort on the
> glance.stores library. We had a majority of +1 but there was a couple
> of -1 due to the fact that we don’t have enough concrete examples of
> this will be useful or not.

It will definitely be useful in the following:

1) Making the copy/zero-copy/transfer/download methods consistent 
between all the various places in the Nova virt drivers that do similar 
things.

2) Allowing a single place to innovate for the transfer of image bits 
between sources and destinations

Hopefully, the above sample code and interfaces will spark some renewed 
interest in this. I'd love to work on it, and see it pushed forward.

Best,
-jay

> Please also see this etherpad containing a list of
> advantages/drawbacks: https://etherpad.openstack.org/p/glance-store
>
> Looking forward to get your pov.
>
> Thanks, Arnaud
>
>
>
> On Jul 28, 2014, at 1:23 PM, Flavio Percoco <flavio at redhat.com
> <mailto:flavio at redhat.com>> wrote:
>
>> On 07/28/2014 09:12 PM, Arnaud Legendre wrote:
>>> Flavio — yeah, glance.store lost momentum at the meetup… I think
>>> it would be nice if you could recap the advantages of doing
>>> that. At the meetup, we quickly did an "advantages vs drawbacks”
>>> poll and I think we might have missed advantages (since you were
>>> not here :)) so there was more drawbacks.
>>
>> Was this published somewhere?
>>
>>> Also, the gap between glance.store and glance will continuously
>>> increase if we do not define some rules to keep both in sync
>>> (while we are making glance.store ready). If we choose to use
>>> glance.store, I think we have to switch glance to use it asap.
>>
>> It's not difficult to keep the gap close if we keep enough eyes on
>> reviews. We've been doing this for oslo.messaging and the old
>> oslo-rpc code.
>>
>>
>>>
>>> This has been added to the agenda for the irc meeting next
>>> Thursday, that would be awesome if you could attend...
>>
>> I'll try to make it there but if I can't, I'd really appreciate
>> this discussion to be brought to the mailing list. I'd really like
>> to understand why the feeling changed and what are the bases for
>> this new feeling about the library.
>>
>> Pls, don't get me wrong. I'm just really surprised and a bit
>> frustrated but I do want to understand what the current feeling
>> about it is and hopefully convince the team otherwise.
>>
>> Cheers, Flavio
>>
>>>
>>> -Arnaud
>>>
>>>
>>> On Jul 28, 2014, at 9:44 AM, Flavio Percoco <flavio at redhat.com
>>> <mailto:flavio at redhat.com>> wrote:
>>>
>>>> Hey Guys,
>>>>
>>>> I was talking to Nikhil earlier today and he mentioned there
>>>> was a mini discussion about glance.store in the mini-summit.
>>>> One thing he mentioned is that some folks don't want it to be
>>>> merged/exists.
>>>>
>>>> I'm writing to get your feeling about this and understand what
>>>> the concerns are. After 1 1/2 development cycle working on the
>>>> library, based on our previous agreement, I'm sure you guys
>>>> understand my frustration after hearing this.
>>>>
>>>> Mark, I understand you are going on vacation in about 2 weeks,
>>>> I'd really like to clear this before you leave so I can move
>>>> this work forward and don't miss the feature freeze.
>>>>
>>>> Thanks in advance for any feedback, Flavio
>>>>
>>>> P.S: If you guys prefer, we can have a meeting about me. FWIW,
>>>> I've been trying to contact most of you on IRC but it's been
>>>> really hard because of the timezones.
>>>>
>>>> -- @flaper87 Flavio Percoco
>>>
>>
>>
>> -- @flaper87 Flavio Percoco
>