openstack team mailing list archive

[Fei Long Wang <flwang@xxxxxxxxxx>]

Hi stackers,

Now I'm running into an issue about reusing image id in Glance. Based on
current design, the image id can be specified during create image by end
user. But now there is a problem, which is tracked by defect
https://bugs.launchpad.net/glance/+bug/1176978. In summary, if user want to
port an image from VMware vCenter which has been used by many VMs. So when
user port the image into Glance, it would be nice if the original image id
with UUID format from vCenter can be used. But if the image is removed and
recreated again, he will run into current issue. I have discussed it with
flaper87 in Glance IRC channel, below is the chat log. FYI. And I also
attached the patch, please help review it. Any comment/suggestion is
welcome. Thanks.

Chat log between flaper87 and me.
=============================
(07:46:34 AM) flwang: btw, may I get your thought about this defect?
https://bugs.launchpad.net/glance/+bug/1176978
(07:47:48 AM) flwang: about if the image id should be reused after the
image is deleted
(07:49:18 AM) flaper87: flwang: it shouldn't, images are never *truly*
deleted, see
https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/models.py#L65

(07:49:27 AM) flwang: I see
(07:49:39 AM) flaper87: that's the BaseModel
(07:49:43 AM) flwang: I know it's just marked 1 in the deleted column
(07:49:45 AM) flaper87: and Image inherits from it
(07:49:52 AM) flaper87: correct
(07:50:21 AM) flaper87: there were some discussions about completely
delelting those images (cleaning up the db)
(07:50:23 AM) flwang: now there is a scenario
(07:50:34 AM) flaper87: but nothing has been decided yet
(07:51:28 AM) flaper87: TBH, I don't like the idea of people passing the id
on the create command, but that's another story for another time
(07:51:30 AM) flwang: user is porting some images from vmware vcenter, and
then these images are deleted
(07:51:49 AM) flaper87: flwang: not sure I understand
(07:52:00 AM) flaper87: flwang: you mean, import image from vmware into
glance
(07:52:04 AM) flwang: but next, user want to add these images again with
the image id given by vcenter
(07:52:05 AM) flaper87: then delete image in glance
(07:52:09 AM) flwang: yes
(07:53:20 AM) flwang: anyway, if user want to re-register an image with its
given id from other place, he will run into problem based on current code
(07:54:10 AM) flaper87: flwang: yep, I guess passing the ID makes sense
when you've 3K services pointing to that specific ID and you want them to
use glance
(07:54:35 AM) flwang: exactly
(07:55:19 AM) flaper87: mmh, interesting. I'm trying to find the bp /
discussion about "really deleting" images. I can't find it
(07:58:04 AM) flwang: for short term, I want to reuse the id
(07:59:03 AM) flwang: when create new image, glance will try to check if
there is a dead image if there is a given image id
(07:59:14 AM) flwang: if yes, reuse id
(07:59:55 AM) flwang: any concern?
(08:00:01 AM) flaper87: might make sense. It would be cool if you could
write this to the mailing list so that other folks can comment. Make sure
your point the use-cases and ideas there
(08:00:19 AM) ***flaper87 is still trying to find that stuff
(08:00:22 AM) flaper87: arrgh
(08:00:24 AM) flaper87: internet is sooo big
(08:02:23 AM) flwang: good suggestion, I will send it to mailing list.
Actually, I have worked out the patch, and I think it works fine.
(08:03:57 AM) flaper87: If you have some code to show, I'd suggest to
either push it somewhere in GH or submitting the patch (thing is that this
change, most likely, requires a blueprint so, I'd rather get some feedback
before going down that road)
(08:13:45 AM) flwang: I'm going to submit the patch as the fix of this bug,
make sense?
(08:14:30 AM) flaper87: mmh, yeah, that makes sense. Expect some
discussions, though.

=============================================================================

diff -rupN glance-unpatched/glance/db/sqlalchemy/api.py
glance/glance/db/sqlalchemy/api.py
--- glance-unpatched/glance/db/sqlalchemy/api.py	2013-04-17
19:45:48.175132054 -0500
+++ glance/glance/db/sqlalchemy/api.py	2013-04-17 19:46:03.905136572
-0500
@@ -235,7 +235,34 @@ def wrap_db_error(f):

 def image_create(context, values):
     """Create an image from the values dictionary."""
-    return _image_update(context, values, None, False)
+    image_id = values.get('id', None)
+    if image_id:
+        session = get_session()
+        with session.begin():
+            # The create image request comes with an ID, since these are
+            # supposed to be universal IDs, this may mean that a user is
+            # trying to re-add an image that he had previously deleted.
+            # Let's let him do that, by "undeleting" the image.
+            try:
+                dead_image = _image_get(context,
+                                       image_id,
+                                       session=session,
+                                       force_show_deleted=True)
+                LOG.debug("Dead Image: %s" % dead_image.deleted)
+                if dead_image.deleted:
+                    LOG.info("Image '%s' had been deleted before "
+                             "and will be reactivated." % image_id)
+                    dead_image.undelete(session=session)
+            except exception.NotFound:
+                LOG.debug("Adding new image with user provided "
+                          "UUID '%s'" % image_id)
+                # Not really a failure, this just means the user provided
a
+                # UUID for the very first time, still let's null it out
+                # for _image+_update() because even if the user provided
+                # an ID, this method does not expect the ID here when
creating
+                # new images
+                image_id = None
+    return _image_update(context, values, image_id, False)


 def image_update(context, image_id, values, purge_props=False):
diff -rupN glance-unpatched/glance/db/sqlalchemy/models.py
glance/glance/db/sqlalchemy/models.py
--- glance-unpatched/glance/db/sqlalchemy/models.py	2013-04-27
19:45:48.175132054 -0500
+++ glance/glance/db/sqlalchemy/models.py	2013-04-17 19:46:03.918174498
-0500
@@ -66,6 +66,13 @@ class ModelBase(object):
         self.deleted_at = timeutils.utcnow()
         self.save(session=session)

+    def undelete(self, session=None):
+        """Undelete this object"""
+        self.deleted = False
+        self.deleted_at = None
+        self.status = 'active'
+        self.save(session=session)
+
     def update(self, values):
         """dict.update() behaviour."""
         for k, v in values.iteritems():



Thanks & Best regards,
Fei Long Wang (王飞龙)
--------------------------------------------------
Scrum Master, Cloud Solutions and OpenStack Development
Tel: 8610-82450513 | T/L: 905-0513
Email: flwang@xxxxxxxxxx
China Systems & Technology Laboratory in Beijing
--------------------------------------------------