Merge lp:~dan-prince/glance/property_delete into lp:~glance-coresec/glance/cactus-trunk

[Dan Prince]

Update the glance registry so that it marks properties as deleted if they are no longer exist when images are updated.

[Jay Pipes (jaypipes)]

Hi Dan! Great patch. I'll review thoroughly in a bit, but wanted to say thanks and to say I marked your bug as a duplicate because we already had a bug report about this issue... anyway, I'll link your branch to the original bug report...

[Jay Pipes (jaypipes)]

Looks great, Dan. :)

review: Approve

[Rick Harris (rconradharris)]

Really clean patch Dan, great work.

I'm little worried by the semantics: a silent delete when a key isn't resubmitted. Though, there really aren't a whole lot of alternatives, and none that I can think of that don't involve some kind of magic, "DELETE ME IF YOU DETECT THIS" value.

So, this seems like the best approach available.

review: Approve

[Jay Pipes (jaypipes)]

Agree that the silent delete if a key isn't present. We can look at possibly doing a DELETE /image/<ID>/prop or POST /image/<ID>/prop call in Diablo?

[Rick Harris (rconradharris)]

> DELETE /image/<ID>/prop or POST /image/<ID>/prop call in Diablo?

I think you're right, a lot of the current messiness surrounding Image properties would go away if we adopted full REST semantics. To me (not necessarily Roy Fielding ;-), this would mean:

* NOT using headers to store or retrieve image-metadata. Rather, we would use standard GET, POST, PUT, and DELETE operations for each of these.

* Image-properties would be a resource mounted at either /image/1/property/2 or /image-property/2.
 There pros and cons to both, but in terms of routing, the latter approach ends up being much simpler.

Thoughts?

[Jay Pipes (jaypipes)]

Well, the reason that headers are used to return "metadata" (it's not metadata, but whatever.. ;) ) is:

a) for HEAD requests, you have no other choice
b) for GET requests, you have to return the image data as the body, so you have no other choice

Of course, the above is in reference to the API node, not the registry node, which of course does not use headers and in fact returns all the image properties and base attributes in JSON...

Or are you specifically referring to not using headers for the custom properties?

[Jay Pipes (jaypipes)]

Also, FWIW, using headers to return image attributes has worked beautifully for Swift for a long time, which is where the concept came from.

=== modified file 'glance/registry/db/api.py'
--- glance/registry/db/api.py	2011-03-14 19:10:24 +0000
+++ glance/registry/db/api.py	2011-03-23 18:41:13 +0000
@@ -256,6 +256,11 @@
         else:
             image_property_create(context, prop_values, session=session)
 
+    for key in orig_properties.keys():
+        if not key in properties:
+            prop_ref = orig_properties[key]
+            image_property_delete(context, prop_ref, session=session)
+
 
 def image_property_create(context, values, session=None):
     """Create an ImageProperty object"""
@@ -277,6 +282,14 @@
     return prop_ref
 
 
+def image_property_delete(context, prop_ref, session=None):
+    """Used internally by image_property_create and image_property_update
+    """
+    prop_ref.update(dict(deleted=True))
+    prop_ref.save(session=session)
+    return prop_ref
+
+
 # pylint: disable-msg=C0111
 def _deleted(context):
     """Calculates whether to include deleted objects based on context.

=== modified file 'tests/functional/test_curl_api.py'
--- tests/functional/test_curl_api.py	2011-03-17 21:43:26 +0000
+++ tests/functional/test_curl_api.py	2011-03-23 18:41:13 +0000
@@ -299,4 +299,27 @@
                                expected_value,
                                image[expected_key]))
 
+        # 10. PUT /images/1 and remove a previously existing property.
+        cmd = ("curl -i -X PUT "
+               "-H 'X-Image-Meta-Property-Arch: x86_64' "
+               "http://0.0.0.0:%d/images/1") % api_port
+
+        exitcode, out, err = execute(cmd)
+        self.assertEqual(0, exitcode)
+
+        lines = out.split("\r\n")
+        status_line = lines[0]
+
+        self.assertEqual("HTTP/1.1 200 OK", status_line)
+
+        cmd = "curl -g http://0.0.0.0:%d/images/detail" % api_port
+
+        exitcode, out, err = execute(cmd)
+
+        self.assertEqual(0, exitcode)
+
+        image = json.loads(out.strip())['images'][0]
+        self.assertEqual(1, len(image['properties']))
+        self.assertEqual('x86_64', image['properties']['arch'])
+
         self.stop_servers()