Merge lp:~rconradharris/nova/xs-snap-return-image-id-before-snapshot into lp:~hudson-openstack/nova/trunk

[Rick Harris]

The Openstack API requires image metadata to be returned immediately after an image-create call.

This is accomplished by having the ImageService create a 'queued' image in Glance.

When the image is subsequently uploaded, the image will go from 'queued' -> 'saving' -> 'queued'.

Related Future Work:

The ImageService needs to be cleaned up so that there is a canonical set of attributes (id, status, etc), and a canonical set of values ('queued', 'saving', etc). Right now, EC2 is fairly coupled to LocalImageService and S3ImageService while OpenStackAPI is coupled to GlanceImageService; ideally, we should be able mix-and-match from any of these.

[Eric Day (eday)]

26: you want compute.API() here, compute_api.ComputeAPI() was the old name

42: Not needed, use self.image_service that was made in constructor

review: Needs Fixing

[Soren Hansen (soren)]

[2011-01-12 10:22:41] soren@lenny:~/src/openstack/nova/nova$ bzr merge lp:~rconradharris/nova/xs-snap-return-image-id-before-snapshot
 M nova/api/openstack/images.py
 M nova/compute/api.py
 M nova/compute/manager.py
 M nova/image/glance.py
 M nova/utils.py
 M nova/virt/libvirt_conn.py
 M nova/virt/xenapi/vm_utils.py
 M nova/virt/xenapi/vmops.py
 M nova/virt/xenapi_conn.py
 M plugins/xenserver/xenapi/etc/xapi.d/plugins/glance
Text conflict in nova/image/glance.py
Text conflict in nova/virt/xenapi/vm_utils.py
2 conflicts encountered.

review: Needs Fixing

[Soren Hansen (soren)]

compute.api.snapshot sets is_public to True by default. This seems like a poor default to me. I'd rather they were private by default and then you can go and make them public after the fact... Unless of course I'm misunderstanding what is_public actually denotes?

[Rick Harris (rconradharris)]

> compute.api.snapshot sets is_public to True by default. This seems like a poor default to me.

Agreed. I'm doing it this way right now because Glance (as of yet) doesn't really support images-tied-to-servers. So, in order for an image to be visible, it has to be public.

Adding the image-instance relationships should definitely be part of Cactus (I don't think we should cram it into Bexar since there still needs to be some discussion on how best to do it).

For now, I've added a TODO so we don't forget. Soren, does that work for you for now?

[Soren Hansen (soren)]

I'm not sure I understand. _is_public means everyone can see it, right? So if I have sensitive data on my instance, everyone will be able to see them by looking at these snapshots?

[Eric Day (eday)]

26: Still broken, s/ComputeAPI/API/

[Rick Harris (rconradharris)]

Eday: Oops, fixed now, thanks.

Soren: Right, is_public does mean everyone can see it, for now, until we build the ownership/sharing modeling into Glance (in Cactus). The idea with is_public=True was something like:

Nobody should be using this right now since we haven't addressed security (at all), let's just make it a little easier on the devs since the newly created snapshot will appear in the image-listing as they'd expect.

For now, I've changed it to is_public=False, so there shouldn't be anymore confusion.

[Eric Day (eday)]

lgtm

review: Approve

[Soren Hansen (soren)]

review: Approve

[Soren Hansen (soren)]

Rocking

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~rconradharris/nova/xs-snap-return-image-id-before-snapshot into lp:nova failed. Below is the output from the failed tests.

nova/image/glance.py:95:13: E261 at least two spaces before inline comment
        pass #raise NotImplementedError
            ^
    Separate inline comments by at least two spaces.

    An inline comment is a comment on the same line as a statement. Inline
    comments should be separated by at least two spaces from the statement.
    They should start with a # and a single space.

    Okay: x = x + 1 # Increment x
    Okay: x = x + 1 # Increment x
    E261: x = x + 1 # Increment x
    E262: x = x + 1 #Increment x
    E262: x = x + 1 # Increment x

[Rick Harris (rconradharris)]

Oops, I should have caught that. Pep-8 issue has been fixed. Full Pep-8 suite passes.

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~rconradharris/nova/xs-snap-return-image-id-before-snapshot into lp:nova failed. Below is the output from the failed tests.

TrialTestCase
    runTest ok
Failure
    runTest ERROR
APITest
    test_exceptions_are_converted_to_faults ok
Failure
    runTest ERROR
TestFaults
    test_fault_parts ok
    test_raise ok
    test_retry_header ok
Failure
    runTest ERROR
    runTest ERROR
LimiterTest
    test_minute ok
    test_one_per_period ok
    test_second ok
    test_users_get_separate_buckets ok
    test_we_can_go_indefinitely_if_we_spread_out_requests ok
WSGIAppProxyTest
    test_200 ok
    test_403 ok
    test_failure ok
WSGIAppTest
    test_escaping ok
    test_good_urls ok
    test_invalid_methods ok
    test_invalid_urls ok
    test_response_to_delays ok
Failure
    runTest ERROR
SharedIpGroupsTest
    test_create_shared_ip_group ok
    test_delete_shared_ip_group ok
    test_get_shared_ip_groups ok
Test
    test_ec2 ok
    test_ec2_root ok
    test_metadata ok
    test_not_found ok
    test_openstack ok
    test_query_api_versions ok
SerializerTest
    test_basic ok
    test_defaults_to_json ok
    test_deserialize ok
    test_deserialize_empty_xml ok
    test_suffix_takes_precedence_over_accept_header ok
Test
    test_controller ok
    test_debug **************************************** REQUEST ENVIRON
wsgi.multithread = False
SCRIPT_NAME =
webob.adhoc_attrs = {'response': <Response at 0x49343d0 200 OK>}
wsgi.input = <cStringIO.StringI object at 0x4920e40>
REQUEST_METHOD = GET
HTTP_HOST = localhost:80
PATH_INFO = /
S...

[Thierry Carrez (ttx)]

This needs some work to fix the test failures, but is in good shape. Let's see if we can get it in before Monday.

review: Approve (ffe)

[Jay Pipes (jaypipes)]

It's really just waiting on packaging for glance..

[Ewan Mellor (ewanmellor)]

> It's really just waiting on packaging for glance..

Glance is on PyPI now, so I think you just need to add glance to pip-requires, and this branch will merge.

[Soren Hansen (soren)]

Packaged glance (based on work from Monty). Installed it on Hudson. Retrying.

[OpenStack Infra (hudson-openstack)]

Attempt to merge into lp:nova failed due to conflicts:

text conflict in nova/compute/api.py

[Jay Pipes (jaypipes)]

Alrighty, let's try this again :)

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~rconradharris/nova/xs-snap-return-image-id-before-snapshot into lp:nova failed. Below is the output from the failed tests.

nova/compute/api.py:378:1: W293 blank line contains whitespace

^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12
nova/compute/api.py:379:50: W291 trailing whitespace
        :retval: A dict containing image metadata
                                                 ^
    JCR: Trailing whitespace is superfluous.
    FBM: Except when it occurs as part of a blank line (i.e. the line is
         nothing but whitespace). According to Python docs[1] a line with only
         whitespace is considered a blank line, and is to be ignored. However,
         matching a blank line to its indentation level avoids mistakenly
         terminating a multi-line statement (e.g. class declaration) when
         pasting code into the standard Python interpreter.

         [1] http://docs.python.org/reference/lexical_analysis.html#blank-lines

    The warning returned varies on whether the line itself is blank, for easier
    filtering for those who want to indent their blank lines.

    Okay: spam(1)
    W291: spam(1)\s
    W293: class Foo(object):\n \n bang = 12

[Jay Pipes (jaypipes)]

pep8 fixes approved.

review: Approve

[OpenStack Infra (hudson-openstack)]

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

[Jay Pipes (jaypipes)]

review: Approve

=== modified file 'nova/api/openstack/images.py'
--- nova/api/openstack/images.py	2011-01-06 02:23:23 +0000
+++ nova/api/openstack/images.py	2011-01-17 17:34:42 +0000
@@ -78,7 +78,14 @@
         'decrypting': 'preparing',
         'untarring': 'saving',
         'available': 'active'}
-    item['status'] = status_mapping[item['status']]
+    try:
+        item['status'] = status_mapping[item['status']]
+    except KeyError:
+        # TODO(sirp): Performing translation of status (if necessary) here for
+        # now. Perhaps this should really be done in EC2 API and
+        # S3ImageService
+        pass
+
     return item
 
 
@@ -92,9 +99,11 @@
 
 
 def _convert_image_id_to_hash(image):
-    image_id = abs(hash(image['imageId']))
-    image['imageId'] = image_id
-    image['id'] = image_id
+    if 'imageId' in image:
+        # Convert EC2-style ID (i-blah) to Rackspace-style (int)
+        image_id = abs(hash(image['imageId']))
+        image['imageId'] = image_id
+        image['id'] = image_id
 
 
 class Controller(wsgi.Controller):
@@ -147,7 +156,11 @@
         env = self._deserialize(req.body, req)
         instance_id = env["image"]["serverId"]
         name = env["image"]["name"]
-        return compute.API().snapshot(context, instance_id, name)
+
+        image_meta = compute.API().snapshot(
+            context, instance_id, name)
+
+        return dict(image=image_meta)
 
     def update(self, req, id):
         # Users may not modify public images, and that's all that
 
=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-01-15 01:48:48 +0000
+++ nova/compute/api.py	2011-01-17 17:34:42 +0000
@@ -335,27 +335,55 @@
             project_id)
         return self.db.instance_get_all(context)
 
-    def _cast_compute_message(self, method, context, instance_id, host=None):
-        """Generic handler for RPC casts to compute."""
+    def _cast_compute_message(self, method, context, instance_id, host=None,
+                              params=None):
+        """Generic handler for RPC casts to compute.
+
+        :param params: Optional dictionary of arguments to be passed to the
+                       compute worker
+
+        :retval None
+        """
+        if not params:
+            params = {}
         if not host:
             instance = self.get(context, instance_id)
             host = instance['host']
         queue = self.db.queue_get_for(context, FLAGS.compute_topic, host)
-        kwargs = {'method': method, 'args': {'instance_id': instance_id}}
+        params['instance_id'] = instance_id
+        kwargs = {'method': method, 'args': params}
         rpc.cast(context, queue, kwargs)
 
-    def _call_compute_message(self, method, context, instance_id, host=None):
-        """Generic handler for RPC calls to compute."""
+    def _call_compute_message(self, method, context, instance_id, host=None,
+                              params=None):
+        """Generic handler for RPC calls to compute.
+
+        :param params: Optional dictionary of arguments to be passed to the
+                       compute worker
+
+        :retval: Result returned by compute worker
+        """
+        if not params:
+            params = {}
         if not host:
             instance = self.get(context, instance_id)
             host = instance["host"]
         queue = self.db.queue_get_for(context, FLAGS.compute_topic, host)
-        kwargs = {"method": method, "args": {"instance_id": instance_id}}
+        params['instance_id'] = instance_id
+        kwargs = {'method': method, 'args': params}
         return rpc.call(context, queue, kwargs)
 
     def snapshot(self, context, instance_id, name):
-        """Snapshot the given instance."""
-        self._cast_compute_message('snapshot_instance', context, instance_id)
+        """Snapshot the given instance.
+
+        :retval: A dict containing image metadata
+        """
+        data = {'name': name, 'is_public': False}
+        image_meta = self.image_service.create(context, data)
+        params = {'image_id': image_meta['id']}
+        self._cast_compute_message('snapshot_instance', context, instance_id,
+                                   params=params)
+        return image_meta
 
     def reboot(self, context, instance_id):
         """Reboot the given instance."""
 
=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-01-13 16:51:31 +0000
+++ nova/compute/manager.py	2011-01-17 17:34:42 +0000
@@ -294,7 +294,7 @@
         self._update_state(context, instance_id)
 
     @exception.wrap_exception
-    def snapshot_instance(self, context, instance_id, name):
+    def snapshot_instance(self, context, instance_id, image_id):
         """Snapshot an instance on this server."""
         context = context.elevated()
         instance_ref = self.db.instance_get(context, instance_id)
@@ -311,7 +311,7 @@
                        'instance: %s (state: %s excepted: %s)'),
                      instance_id, instance_ref['state'], power_state.RUNNING)
 
-        self.driver.snapshot(instance_ref, name)
+        self.driver.snapshot(instance_ref, image_id)
 
     @exception.wrap_exception
     @checks_instance_lock
 
=== modified file 'nova/image/glance.py'
--- nova/image/glance.py	2011-01-07 14:46:17 +0000
+++ nova/image/glance.py	2011-01-17 17:34:42 +0000
@@ -14,9 +14,9 @@
 #    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
 #    License for the specific language governing permissions and limitations
 #    under the License.
-
 """Implementation of an image service that uses Glance as the backend"""
 
+from __future__ import absolute_import
 import httplib
 import json
 import urlparse
@@ -24,171 +24,40 @@
 from nova import exception
 from nova import flags
 from nova import log as logging
+from nova import utils
 from nova.image import service
 
 
 LOG = logging.getLogger('nova.image.glance')
 
 FLAGS = flags.FLAGS
-flags.DEFINE_string('glance_teller_address', 'http://127.0.0.1',
-                    'IP address or URL where Glance\'s Teller service resides')
-flags.DEFINE_string('glance_teller_port', '9191',
-                    'Port for Glance\'s Teller service')
-flags.DEFINE_string('glance_parallax_address', 'http://127.0.0.1',
-                    'IP address or URL where Glance\'s Parallax service '
-                    'resides')
-flags.DEFINE_string('glance_parallax_port', '9292',
-                    'Port for Glance\'s Parallax service')
-
-
-class TellerClient(object):
-
-    def __init__(self):
-        self.address = FLAGS.glance_teller_address
-        self.port = FLAGS.glance_teller_port
-        url = urlparse.urlparse(self.address)
-        self.netloc = url.netloc
-        self.connection_type = {'http': httplib.HTTPConnection,
-                                'https': httplib.HTTPSConnection}[url.scheme]
-
-
-class ParallaxClient(object):
-
-    def __init__(self):
-        self.address = FLAGS.glance_parallax_address
-        self.port = FLAGS.glance_parallax_port
-        url = urlparse.urlparse(self.address)
-        self.netloc = url.netloc
-        self.connection_type = {'http': httplib.HTTPConnection,
-                                'https': httplib.HTTPSConnection}[url.scheme]
-
-    def get_image_index(self):
-        """
-        Returns a list of image id/name mappings from Parallax
-        """
-        try:
-            c = self.connection_type(self.netloc, self.port)
-            c.request("GET", "images")
-            res = c.getresponse()
-            if res.status == 200:
-                # Parallax returns a JSONified dict(images=image_list)
-                data = json.loads(res.read())['images']
-                return data
-            else:
-                LOG.warn(_("Parallax returned HTTP error %d from "
-                           "request for /images"), res.status_int)
-                return []
-        finally:
-            c.close()
-
-    def get_image_details(self):
-        """
-        Returns a list of detailed image data mappings from Parallax
-        """
-        try:
-            c = self.connection_type(self.netloc, self.port)
-            c.request("GET", "images/detail")
-            res = c.getresponse()
-            if res.status == 200:
-                # Parallax returns a JSONified dict(images=image_list)
-                data = json.loads(res.read())['images']
-                return data
-            else:
-                LOG.warn(_("Parallax returned HTTP error %d from "
-                           "request for /images/detail"), res.status_int)
-                return []
-        finally:
-            c.close()
-
-    def get_image_metadata(self, image_id):
-        """
-        Returns a mapping of image metadata from Parallax
-        """
-        try:
-            c = self.connection_type(self.netloc, self.port)
-            c.request("GET", "images/%s" % image_id)
-            res = c.getresponse()
-            if res.status == 200:
-                # Parallax returns a JSONified dict(image=image_info)
-                data = json.loads(res.read())['image']
-                return data
-            else:
-                # TODO(jaypipes): log the error?
-                return None
-        finally:
-            c.close()
-
-    def add_image_metadata(self, image_metadata):
-        """
-        Tells parallax about an image's metadata
-        """
-        try:
-            c = self.connection_type(self.netloc, self.port)
-            body = json.dumps(image_metadata)
-            c.request("POST", "images", body)
-            res = c.getresponse()
-            if res.status == 200:
-                # Parallax returns a JSONified dict(image=image_info)
-                data = json.loads(res.read())['image']
-                return data['id']
-            else:
-                # TODO(jaypipes): log the error?
-                return None
-        finally:
-            c.close()
-
-    def update_image_metadata(self, image_id, image_metadata):
-        """
-        Updates Parallax's information about an image
-        """
-        try:
-            c = self.connection_type(self.netloc, self.port)
-            body = json.dumps(image_metadata)
-            c.request("PUT", "images/%s" % image_id, body)
-            res = c.getresponse()
-            return res.status == 200
-        finally:
-            c.close()
-
-    def delete_image_metadata(self, image_id):
-        """
-        Deletes Parallax's information about an image
-        """
-        try:
-            c = self.connection_type(self.netloc, self.port)
-            c.request("DELETE", "images/%s" % image_id)
-            res = c.getresponse()
-            return res.status == 200
-        finally:
-            c.close()
+
+GlanceClient = utils.import_class('glance.client.Client')
 
 
 class GlanceImageService(service.BaseImageService):
     """Provides storage and retrieval of disk image objects within Glance."""
 
     def __init__(self):
-        self.teller = TellerClient()
-        self.parallax = ParallaxClient()
+        self.client = GlanceClient(FLAGS.glance_host, FLAGS.glance_port)
 
     def index(self, context):
         """
-        Calls out to Parallax for a list of images available
+        Calls out to Glance for a list of images available
         """
-        images = self.parallax.get_image_index()
-        return images
+        return self.client.get_images()
 
     def detail(self, context):
         """
-        Calls out to Parallax for a list of detailed image information
+        Calls out to Glance for a list of detailed image information
         """
-        images = self.parallax.get_image_details()
-        return images
+        return self.client.get_images_detailed()
 
     def show(self, context, id):
         """
         Returns a dict containing image data for the given opaque image id.
         """
-        image = self.parallax.get_image_metadata(id)
+        image = self.client.get_image_meta(id)
         if image:
             return image
         raise exception.NotFound
@@ -200,7 +69,7 @@
         :raises AlreadyExists if the image already exist.
 
         """
-        return self.parallax.add_image_metadata(data)
+        return self.client.add_image(image_meta=data)
 
     def update(self, context, image_id, data):
         """Replace the contents of the given image with the new data.
@@ -208,7 +77,7 @@
         :raises NotFound if the image does not exist.
 
         """
-        self.parallax.update_image_metadata(image_id, data)
+        return self.client.update_image(image_id, data)
 
     def delete(self, context, image_id):
         """
@@ -217,7 +86,7 @@
         :raises NotFound if the image does not exist.
 
         """
-        self.parallax.delete_image_metadata(image_id)
+        return self.client.delete_image(image_id)
 
     def delete_all(self):
         """
 
=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2011-01-10 02:08:54 +0000
+++ nova/tests/api/openstack/fakes.py	2011-01-17 17:34:42 +0000
@@ -23,6 +23,8 @@
 import webob
 import webob.dec
 
+from glance import client as glance_client
+
 from nova import auth
 from nova import context
 from nova import exception as exc
@@ -116,64 +118,60 @@
     stubs.Set(nova.compute.API, 'snapshot', snapshot)
 
 
-def stub_out_glance(stubs, initial_fixtures=[]):
+def stub_out_glance(stubs, initial_fixtures=None):
 
-    class FakeParallaxClient:
+    class FakeGlanceClient:
 
         def __init__(self, initial_fixtures):
-            self.fixtures = initial_fixtures
+            self.fixtures = initial_fixtures or []
 
-        def fake_get_image_index(self):
+        def fake_get_images(self):
             return [dict(id=f['id'], name=f['name'])
                     for f in self.fixtures]
 
-        def fake_get_image_details(self):
+        def fake_get_images_detailed(self):
             return self.fixtures
 
-        def fake_get_image_metadata(self, image_id):
+        def fake_get_image_meta(self, image_id):
             for f in self.fixtures:
                 if f['id'] == image_id:
                     return f
             return None
 
-        def fake_add_image_metadata(self, image_data):
+        def fake_add_image(self, image_meta):
             id = ''.join(random.choice(string.letters) for _ in range(20))
-            image_data['id'] = id
-            self.fixtures.append(image_data)
+            image_meta['id'] = id
+            self.fixtures.append(image_meta)
             return id
 
-        def fake_update_image_metadata(self, image_id, image_data):
-            f = self.fake_get_image_metadata(image_id)
+        def fake_update_image(self, image_id, image_meta):
+            f = self.fake_get_image_meta(image_id)
             if not f:
                 raise exc.NotFound
 
-            f.update(image_data)
+            f.update(image_meta)
 
-        def fake_delete_image_metadata(self, image_id):
-            f = self.fake_get_image_metadata(image_id)
+        def fake_delete_image(self, image_id):
+            f = self.fake_get_image_meta(image_id)
             if not f:
                 raise exc.NotFound
 
             self.fixtures.remove(f)
 
-        def fake_delete_all(self):
-            self.fixtures = []
-
-    fake_parallax_client = FakeParallaxClient(initial_fixtures)
-    stubs.Set(nova.image.glance.ParallaxClient, 'get_image_index',
-              fake_parallax_client.fake_get_image_index)
-    stubs.Set(nova.image.glance.ParallaxClient, 'get_image_details',
-              fake_parallax_client.fake_get_image_details)
-    stubs.Set(nova.image.glance.ParallaxClient, 'get_image_metadata',
-              fake_parallax_client.fake_get_image_metadata)
-    stubs.Set(nova.image.glance.ParallaxClient, 'add_image_metadata',
-              fake_parallax_client.fake_add_image_metadata)
-    stubs.Set(nova.image.glance.ParallaxClient, 'update_image_metadata',
-              fake_parallax_client.fake_update_image_metadata)
-    stubs.Set(nova.image.glance.ParallaxClient, 'delete_image_metadata',
-              fake_parallax_client.fake_delete_image_metadata)
-    stubs.Set(nova.image.glance.GlanceImageService, 'delete_all',
-              fake_parallax_client.fake_delete_all)
+        ##def fake_delete_all(self):
+        ##    self.fixtures = []
+
+    GlanceClient = glance_client.Client
+    fake = FakeGlanceClient(initial_fixtures)
+
+    stubs.Set(GlanceClient, 'get_images', fake.fake_get_images)
+    stubs.Set(GlanceClient, 'get_images_detailed',
+              fake.fake_get_images_detailed)
+    stubs.Set(GlanceClient, 'get_image_meta', fake.fake_get_image_meta)
+    stubs.Set(GlanceClient, 'add_image', fake.fake_add_image)
+    stubs.Set(GlanceClient, 'update_image', fake.fake_update_image)
+    stubs.Set(GlanceClient, 'delete_image', fake.fake_delete_image)
+    #stubs.Set(GlanceClient, 'delete_all', fake.fake_delete_all)
 
 
 class FakeToken(object):
 
=== modified file 'nova/utils.py'
--- nova/utils.py	2011-01-15 01:48:48 +0000
+++ nova/utils.py	2011-01-17 17:34:42 +0000
@@ -334,6 +334,20 @@
         return getattr(backend, key)
 
 
+class LoopingCallDone(Exception):
+    """The poll-function passed to LoopingCall can raise this exception to
+    break out of the loop normally. This is somewhat analogous to
+    StopIteration.
+
+    An optional return-value can be included as the argument to the exception;
+    this return-value will be returned by LoopingCall.wait()
+    """
+
+    def __init__(self, retvalue=True):
+        """:param retvalue: Value that LoopingCall.wait() should return"""
+        self.retvalue = retvalue
+
+
 class LoopingCall(object):
     def __init__(self, f=None, *args, **kw):
         self.args = args
@@ -352,12 +366,15 @@
                 while self._running:
                     self.f(*self.args, **self.kw)
                     greenthread.sleep(interval)
+            except LoopingCallDone, e:
+                self.stop()
+                done.send(e.retvalue)
             except Exception:
                 logging.exception('in looping call')
                 done.send_exception(*sys.exc_info())
                 return
-
-            done.send(True)
+            else:
+                done.send(True)
 
         self.done = done
 
 
=== modified file 'nova/virt/libvirt_conn.py'
--- nova/virt/libvirt_conn.py	2011-01-15 01:54:36 +0000
+++ nova/virt/libvirt_conn.py	2011-01-17 17:34:42 +0000
@@ -295,7 +295,7 @@
         virt_dom.detachDevice(xml)
 
     @exception.wrap_exception
-    def snapshot(self, instance, name):
+    def snapshot(self, instance, image_id):
         """ Create snapshot from a running VM instance """
         raise NotImplementedError(
             _("Instance snapshotting is not supported for libvirt"
 
=== modified file 'nova/virt/xenapi/vm_utils.py'
--- nova/virt/xenapi/vm_utils.py	2011-01-11 06:47:35 +0000
+++ nova/virt/xenapi/vm_utils.py	2011-01-17 17:34:42 +0000
@@ -236,14 +236,15 @@
         return template_vm_ref, [template_vdi_uuid, parent_uuid]
 
     @classmethod
-    def upload_image(cls, session, instance_id, vdi_uuids, image_name):
+    def upload_image(cls, session, instance_id, vdi_uuids, image_id):
         """ Requests that the Glance plugin bundle the specified VDIs and
         push them into Glance using the specified human-friendly name.
         """
-        LOG.debug(_("Asking xapi to upload %s as '%s'"), vdi_uuids, image_name)
+        logging.debug(_("Asking xapi to upload %s as ID %s"),
+                      vdi_uuids, image_id)
 
         params = {'vdi_uuids': vdi_uuids,
-                  'image_name': image_name,
+                  'image_id': image_id,
                   'glance_host': FLAGS.glance_host,
                   'glance_port': FLAGS.glance_port}
 
@@ -424,9 +425,16 @@
         * parent_vhd
             snapshot
     """
-    #TODO(sirp): we need to timeout this req after a while
+    max_attempts = FLAGS.xenapi_vhd_coalesce_max_attempts
+    attempts = {'counter': 0}
 
     def _poll_vhds():
+        attempts['counter'] += 1
+        if attempts['counter'] > max_attempts:
+            msg = (_("VHD coalesce attempts exceeded (%d > %d), giving up...")
+                   % (attempts['counter'], max_attempts))
+            raise exception.Error(msg)
+
         scan_sr(session, instance_id, sr_ref)
         parent_uuid = get_vhd_parent_uuid(session, vdi_ref)
         if original_parent_uuid and (parent_uuid != original_parent_uuid):
@@ -434,13 +442,12 @@
                          "waiting for coalesce..."), parent_uuid,
                       original_parent_uuid)
         else:
-            done.send(parent_uuid)
+            # Breakout of the loop (normally) and return the parent_uuid
+            raise utils.LoopingCallDone(parent_uuid)
 
-    done = event.Event()
     loop = utils.LoopingCall(_poll_vhds)
     loop.start(FLAGS.xenapi_vhd_coalesce_poll_interval, now=True)
-    parent_uuid = done.wait()
-    loop.stop()
+    parent_uuid = loop.wait()
     return parent_uuid
 
 
 
=== modified file 'nova/virt/xenapi/vmops.py'
--- nova/virt/xenapi/vmops.py	2011-01-13 16:53:13 +0000
+++ nova/virt/xenapi/vmops.py	2011-01-17 17:34:42 +0000
@@ -161,11 +161,11 @@
             raise Exception(_('Instance not present %s') % instance_name)
         return vm
 
-    def snapshot(self, instance, name):
+    def snapshot(self, instance, image_id):
         """ Create snapshot from a running VM instance
 
         :param instance: instance to be snapshotted
-        :param name: name/label to be given to the snapshot
+        :param image_id: id of image to upload to
 
         Steps involved in a XenServer snapshot:
 
@@ -201,7 +201,7 @@
         try:
             # call plugin to ship snapshot off to glance
             VMHelper.upload_image(
-                self._session, instance.id, template_vdi_uuids, name)
+                self._session, instance.id, template_vdi_uuids, image_id)
         finally:
             self._destroy(instance, template_vm_ref, shutdown=False)
 
 
=== modified file 'nova/virt/xenapi_conn.py'
--- nova/virt/xenapi_conn.py	2011-01-12 19:22:01 +0000
+++ nova/virt/xenapi_conn.py	2011-01-17 17:34:42 +0000
@@ -93,6 +93,10 @@
                    5.0,
                    'The interval used for polling of coalescing vhds.'
                    '  Used only if connection_type=xenapi.')
+flags.DEFINE_integer('xenapi_vhd_coalesce_max_attempts',
+                     5,
+                     'Max number of times to poll for VHD to coalesce.'
+                     '  Used only if connection_type=xenapi.')
 flags.DEFINE_string('target_host',
                     None,
                     'iSCSI Target Host')
@@ -141,9 +145,9 @@
         """Create VM instance"""
         self._vmops.spawn(instance)
 
-    def snapshot(self, instance, name):
+    def snapshot(self, instance, image_id):
         """ Create snapshot from a running VM instance """
-        self._vmops.snapshot(instance, name)
+        self._vmops.snapshot(instance, image_id)
 
     def reboot(self, instance):
         """Reboot VM instance"""
 
=== modified file 'plugins/xenserver/xenapi/etc/xapi.d/plugins/glance'
--- plugins/xenserver/xenapi/etc/xapi.d/plugins/glance	2010-12-22 19:01:33 +0000
+++ plugins/xenserver/xenapi/etc/xapi.d/plugins/glance	2011-01-17 17:34:42 +0000
@@ -45,24 +45,24 @@
 def put_vdis(session, args):
     params = pickle.loads(exists(args, 'params'))
     vdi_uuids = params["vdi_uuids"]
-    image_name = params["image_name"]
+    image_id = params["image_id"]
     glance_host = params["glance_host"]
     glance_port = params["glance_port"]
     
     sr_path = get_sr_path(session)
     #FIXME(sirp): writing to a temp file until Glance supports chunked-PUTs
-    tmp_file = "%s.tar.gz" % os.path.join('/tmp', image_name) 
+    tmp_file = "%s.tar.gz" % os.path.join('/tmp', str(image_id)) 
     tar_cmd = ['tar', '-zcf', tmp_file, '--directory=%s' % sr_path]
     paths = [ "%s.vhd" % vdi_uuid for vdi_uuid in vdi_uuids ]
     tar_cmd.extend(paths)
     logging.debug("Bundling image with cmd: %s", tar_cmd)
     subprocess.call(tar_cmd)
     logging.debug("Writing to test file %s", tmp_file) 
-    put_bundle_in_glance(tmp_file, image_name, glance_host, glance_port)
+    put_bundle_in_glance(tmp_file, image_id, glance_host, glance_port)
     return "" # FIXME(sirp): return anything useful here?
 
 
-def put_bundle_in_glance(tmp_file, image_name, glance_host, glance_port):
+def put_bundle_in_glance(tmp_file, image_id, glance_host, glance_port):
     size = os.path.getsize(tmp_file) 
     basename = os.path.basename(tmp_file)
 
@@ -72,7 +72,6 @@
             'x-image-meta-store': 'file',
             'x-image-meta-is_public': 'True',
             'x-image-meta-type': 'raw',
-            'x-image-meta-name': image_name,
             'x-image-meta-size': size,
             'content-length': size,
             'content-type': 'application/octet-stream',
@@ -80,7 +79,7 @@
         conn = httplib.HTTPConnection(glance_host, glance_port)
         #NOTE(sirp): httplib under python2.4 won't accept a file-like object
         # to request
-        conn.putrequest('POST', '/images')
+        conn.putrequest('PUT', '/images/%s' % image_id)
 
         for header, value in headers.iteritems():
             conn.putheader(header, value)