Merge lp:~rconradharris/nova/uuid-redux into lp:~hudson-openstack/nova/trunk

[Rick Harris]

This patch adds support for working with instances by UUID in addition to integer IDs.

The Zone Scheduler routing mechanics were changed slightly so that when an UUID is passed in, it checks to see whether the item is available locally.

If it isn't it re-routes to a child zone.

If it is available locally, it substitutes the UUID with the integer ID and calls the wrapped function. This is the 'trick' allows us to not change any of the virt-layer code-- everything still uses integer IDs locally.

[Brian Waldon (bcwaldon)]

Is there a bug/blueprint you can attach this to?

[Josh Kearney (jk0)]

> Is there a bug/blueprint you can attach this to?

nova-instance-referencing

[Brian Waldon (bcwaldon)]

Cool stuff, Rick. Looking forward to uuids

8-37, 92: This is good refactoring, but it doesn't seem directly related to this branch. I'm not asking you to remove it, I just think we shouldn't touch things like this until we have a clear reason to change it.

178: This method name is a bit misleading. Can you call it _build_instance_get_query or something?

Is there a future plan to either remove instance.id or replace it with the uuid value? I can understand why it isn't in this MP.

review: Needs Fixing

[Rick Harris (rconradharris)]

> 8-37, 92: This is good refactoring, but it doesn't seem directly related to this branch. I'm not asking you to remove it, I just think we shouldn't touch things like this until we have a clear reason to change it.

The intent here wasn't to refactor, that was just a by-product. There was a nasty bug in the code that we uncovered. Notice, `server_id` and `id` in those methods. Since `id` wasn't passed in, it was actually using python's built-in function `id`.

The refactoring just made the cleanup a little easier.

(As to why the tests passed before, no idea. They shouldn't have. Maybe due to the shear amount of fakes and stubs.)

> 178: This method name is a bit misleading. Can you call it _build_instance_get_query or something?

Okay.

> Is there a future plan to either remove instance.id or replace it with the uuid value? I can understand why it isn't in this MP.

I think that's in the long-term plan; but, definitely couldn't get that in one-shot. This gets a step-closer and allows us to move to UUID in a careful, piece-wise fashion.

[Josh Kearney (jk0)]

> 8-37, 92: This is good refactoring, but it doesn't seem directly related to
> this branch. I'm not asking you to remove it, I just think we shouldn't touch
> things like this until we have a clear reason to change it.

Not sure I agree with this. Other than the fact that there was a very serious bug, is the duplication of all that code not a clear enough reason to make a small refactor? I could see this being a bad idea in certain cases, but something this trivial is perfectly acceptable IMHO.

[Rick Harris (rconradharris)]

> 178: This method name is a bit misleading. Can you call it _build_instance_get_query or something?

Fixed.

[Brian Waldon (bcwaldon)]

> > 8-37, 92: This is good refactoring, but it doesn't seem directly related to
> > this branch. I'm not asking you to remove it, I just think we shouldn't
> touch
> > things like this until we have a clear reason to change it.
>
> Not sure I agree with this. Other than the fact that there was a very serious
> bug, is the duplication of all that code not a clear enough reason to make a
> small refactor? I could see this being a bad idea in certain cases, but
> something this trivial is perfectly acceptable IMHO.

First of all, I didn't realize this was actually fixing a bug. Given that, I have no problem in this specific case. I'm not a fan of attaching arbitrary refactoring to branches that have a very specific purpose. It increases the likelihood that new bugs will be introduced in code that was already working just fine. It's the "if it ain't broke, don't fix it" mentality.

[Brian Waldon (bcwaldon)]

> > 178: This method name is a bit misleading. Can you call it
> _build_instance_get_query or something?
>
> Fixed.

Cool.

review: Approve

[Brian Lamar (blamar)]

This might have been covered, but why go through all of the migrations and headache changing SQL columns when we could just store the UUID as an integer in the existing column?

review: Needs Information

[Rick Harris (rconradharris)]

> This might have been covered, but why go through all of the migrations and headache changing SQL
> columns when we could just store the UUID as an integer in the existing column?

A UUID is 128-bits wide. The ID column is currently 32 bits wide (int); even if we bumped it up to a bigint, it would only be 64 bits wide.

[Ed Leafe (ed-leafe)]

The migration script is incorrect; it populates all existing instances with the same UUID, since each pass through the rows updates every row; see http://paste.openstack.org/show/1674/ to see the results after running the migration.

review: Needs Fixing

[Ed Leafe (ed-leafe)]

Here's a patch that fixes the problem:

http://paste.openstack.org/show/1675/

[Ed Leafe (ed-leafe)]

The ec2 api reports the old ec2_id format (i.e., 'i-<hex of integer id>'), which will not work. It should be 'i-<first 8 of uuid>'.

See http://paste.openstack.org/show/1676/

review: Needs Fixing

[Ed Leafe (ed-leafe)]

Ah, I see none of the ec2 changes are included. So I guess that's by design.

[Josh Kearney (jk0)]

> The migration script is incorrect; it populates all existing instances with
> the same UUID, since each pass through the rows updates every row; see
> http://paste.openstack.org/show/1674/ to see the results after running the
> migration.

Very nice catch. This could have caused a lot of headaches.

[Rick Harris (rconradharris)]

> The ec2 api reports the old ec2_id format (i.e., 'i-<hex of integer id>'), which will not work. It
> should be 'i-<first 8 of uuid>'.

I'm not fully up-to-speed with the plan with respect to EC2; perhaps we should talk that part of the problem out.

That said, I'd vote to make that a separate patch, to keep this one, already fairly tricky, as simple as possible.

[lp:~rconradharris/nova/uuid-redux]

1200. By Rick Harris on 2011-06-17

Fixed migration per review feedback.

[Rick Harris (rconradharris)]

> The migration script is incorrect; it populates all existing instances with the same UUID, since
> each pass through the rows updates every row; see http://paste.openstack.org/show/1674/ to see the
> results after running the migration.

Nice catch. Fixed.

[Matt Dietz (cerberus)]

Looks awesome. Love the tests.

review: Approve

[Ed Leafe (ed-leafe)]

utils.py: is_uuid_like(val) is incorrect. A value of "1234567890123456789012" is a perfectly legitimate UUID hex string, but that method will return False for this. I would recommend using the version I wrote:

def is_uuid(val):
    """Returns True if the value passed is a valid UUID string."""
    if isinstance(val, basestring):
        # Remove any dashes
        val = val.replace("-", "")
        # UUIDs are 32 hex characters long
        if len(val) == 32:
            # Make sure that they are all valid hex characters.
            acceptable = "0123456789abcdefABCDEF"
            return not bool([ch for ch in val if ch not in acceptable])
    return False

review: Needs Fixing

[Ed Leafe (ed-leafe)]

The 'gen_uuid()' function in nova/utils.py returns a UUID object, not the hex string. As a result, functions that call it have to change it to a string. The nova/db/sqlalchemy/api.py method that populates the uuid column does this by wrapping it with str(), which generates a string with the dashes in it. The design was to not store the dashes in the database, as they serve no purpose there. We can always add them back when they are displayed to the user for readability if needed.

The simplest fix is to have gen_uuid() return uuid.uuid4().hex instead of a Python UUID object; this will not have the dashes, and will not have to be cast to a basestring for storage or comparison.

review: Needs Fixing

[Ed Leafe (ed-leafe)]

Line 405: the FAKE_UUID used for tests is not a valid UUID hex string. Any code that accepts UUID values and does anything requiring validation will fail. This should be changed to something akin to:

FAKE_UUID = "abcdabcd-abcd-abcd-abcd-abcdabcdabcd"

review: Needs Fixing

[Ed Leafe (ed-leafe)]

Just noticed that the above comment also applies to lines 554-5.

[lp:~rconradharris/nova/uuid-redux]

1201. By Rick Harris on 2011-06-17

Merging trunk

1202. By Rick Harris on 2011-06-17

Using proper UUID format for uuids

1203. By Rick Harris on 2011-06-18

Adding tests for is_uuid_like

[Rick Harris (rconradharris)]

> This should be changed to something akin to:
> FAKE_UUID = "abcdabcd-abcd-abcd-abcd-abcdabcdabcd"

Fixed.

Made is_uuid_like a bit stricter in what it accepts, added unit-tests for it.

[Ed Leafe (ed-leafe)]

I still have some reservations, but there is nothing anymore that would cause errors, so I'm removing the 'Needs Fixing'.

review: Abstain

[Chris Behrens (cbehrens)]

> I still have some reservations, but there is nothing anymore that would cause
> errors, so I'm removing the 'Needs Fixing'.

The dashes or something more? If it's the dashes, and it was decided at the summit to not use dashes, I agree with Ed that we should not store them.

I don't remember the discussion (I may not have been there), though, so I can't really comment further on that part.

It makes me curious, however, why one would be worried about the dashes in the DB. I realize they don't serve any purpose and it'd be a waste of bytes as far as the DB is concerned. However, if the client is having to continuously rebuild 32/36 byte strings with and without dashes, it's really not a waste in the end. 32 bytes vs 36 bytes should have very minimal effect on the DB. If we're going to use a char column, it seems to make most sense to store them in the native string format. If I'm missing something, I wouldn't mind being set straight.

Why didn't we choose int(16) or binary(16) or what not? Portability?

Anyway, as I said at the top here and putting my personal thoughts aside, I feel we should implement whatever was discussed at the summit.

[Rick Harris (rconradharris)]

> I don't remember the discussion (I may not have been there), though, so I can't really comment further on that part.

Same here, don't recall hearing this discussion at the summit (might have been in a different talk).

> If we're going to use a char column, it seems to make most sense to store them in the native string format.

Agreed, if we're going to pay the penalty of storing the UUID as a VARCHAR, we might as well use the canonical representation. That would make the column a bit easier to work with: Ops wouldn't constantly have to convert from canonical-dashed-UUID-format to underlying-hex-format.

Granted we're trying to be database agnostic, but MySQL seems to have an opinion on the matter since their UUID function returns the UUID in canonical-dashed-format. So, it would be nice to be able to do something like:

UPDATE instances set uuid=UUID() WHERE id=3;

> Why didn't we choose int(16) or binary(16) or what not? Portability?

Portability was the main goal here. To my knowledge there is no good cross-db way of handling UUIDs. With Postgres you can use a native UUID type, however, that's not supported by SQLite and MySQL.

INT(16) is really just a 32-bit number with a display width of 16 digits.

The largest we could get in terms of integer datatypes would be a 64-bit BIGINT, too small for a 128-bit UUID.

The other route, using a 16-byte Binary field is a possibility. Looking around I see some people recommending that for performance reasons. However, I'm not sure in our particular use-case that this would be the right way to go.

For one, I don't think we're going to be storing enough data in our tables for the penalty of performing a scan on a VARCHAR column to be important.

Second, and most importantly, from an Ops perspective, it makes a lot of sense having the data-represented in the database match up with "what's expected". Forcing Ops to convert between a log entry showing "ABCD-ABCD-ABCD-ABCD" and some huge opaque number seemed error-prone and needlessly painful.

All that said, if either A) we'd previously committed to storing UUIDs differently or B) we find a compelling performance reason for using a binary representation, I'd be cool with changing it.

[Ed Leafe (ed-leafe)]

The discussion at the summit was about storing UUIDs as integers or CHAR. VARCHAR makes no sense since there is no VAR involved: UUID strings are fixed length.

There was no discussion of storing the dashes. That is basic database design: you don't store presentation formatting in the database, no matter how canonical the representation might be. Dashes in social security numbers; spaces in credit card numbers;, etc. But since this is not fatal in any sense, and I am not concerned about that.

My main reservation was the rewriting of is_uuid() to is_uuid_like(), so that strings such as 'zzzzzzzz-zzzz-...' are accepted. I see no benefit to that, when either the version I wrote or the native uuid.UUID() function will work, while adding potential testing problems due to invalid UUID strings being accepted. But since this was a conscious design choice and not an oversight, and will not break the app in production, I withdrew my "Needs Fixing" review.

[Rick Harris (rconradharris)]

VARCHARs are used because that's what SQLAlchemy exposes via its `String` datatype.

http://www.sqlalchemy.org/docs/core/types.html#sqlalchemy.types.String

[lp:~rconradharris/nova/uuid-redux]

1204. By Rick Harris on 2011-06-20

Merging trunk, fixing conflicts

1205. By Rick Harris on 2011-06-20

Moving add_uuid migration to 025

=== modified file 'nova/api/openstack/ips.py'
--- nova/api/openstack/ips.py	2011-05-20 18:42:19 +0000
+++ nova/api/openstack/ips.py	2011-06-20 15:20:30 +0000
@@ -32,25 +32,24 @@
         self.compute_api = nova.compute.API()
         self.builder = nova.api.openstack.views.addresses.ViewBuilderV10()
 
+    def _get_instance(self, req, server_id):
+        try:
+            instance = self.compute_api.get(
+                req.environ['nova.context'], server_id)
+        except nova.exception.NotFound:
+            return faults.Fault(exc.HTTPNotFound())
+        return instance
+
     def index(self, req, server_id):
-        try:
-            instance = self.compute_api.get(req.environ['nova.context'], id)
-        except nova.exception.NotFound:
-            return faults.Fault(exc.HTTPNotFound())
+        instance = self._get_instance(req, server_id)
         return {'addresses': self.builder.build(instance)}
 
     def public(self, req, server_id):
-        try:
-            instance = self.compute_api.get(req.environ['nova.context'], id)
-        except nova.exception.NotFound:
-            return faults.Fault(exc.HTTPNotFound())
+        instance = self._get_instance(req, server_id)
         return {'public': self.builder.build_public_parts(instance)}
 
     def private(self, req, server_id):
-        try:
-            instance = self.compute_api.get(req.environ['nova.context'], id)
-        except nova.exception.NotFound:
-            return faults.Fault(exc.HTTPNotFound())
+        instance = self._get_instance(req, server_id)
         return {'private': self.builder.build_private_parts(instance)}
 
     def show(self, req, server_id, id):

=== modified file 'nova/api/openstack/views/servers.py'
--- nova/api/openstack/views/servers.py	2011-06-09 18:52:53 +0000
+++ nova/api/openstack/views/servers.py	2011-06-20 15:20:30 +0000
@@ -75,7 +75,7 @@
         }
 
         inst_dict = {
-            'id': int(inst['id']),
+            'id': inst['id'],
             'name': inst['display_name'],
             'addresses': self.addresses_builder.build(inst),
             'status': power_mapping[inst.get('state')]}
@@ -99,6 +99,7 @@
         self._build_image(inst_dict, inst)
         self._build_flavor(inst_dict, inst)
 
+        inst_dict['uuid'] = inst['uuid']
         return dict(server=inst_dict)
 
     def _build_image(self, response, inst):

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-06-17 23:52:22 +0000
+++ nova/compute/api.py	2011-06-20 15:20:30 +0000
@@ -579,8 +579,15 @@
 
     def get(self, context, instance_id):
         """Get a single instance with the given instance_id."""
-        rv = self.db.instance_get(context, instance_id)
-        return dict(rv.iteritems())
+        # NOTE(sirp): id used to be exclusively integer IDs; now we're
+        # accepting both UUIDs and integer IDs. The handling of this
+        # is done in db/sqlalchemy/api/instance_get
+        if utils.is_uuid_like(instance_id):
+            uuid = instance_id
+            instance = self.db.instance_get_by_uuid(context, uuid)
+        else:
+            instance = self.db.instance_get(context, instance_id)
+        return dict(instance.iteritems())
 
     @scheduler_api.reroute_compute("get")
     def routing_get(self, context, instance_id):

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2011-06-15 16:46:24 +0000
+++ nova/compute/manager.py	2011-06-20 15:20:30 +0000
@@ -783,7 +783,6 @@
     def get_diagnostics(self, context, instance_id):
         """Retrieve diagnostics for an instance on this host."""
         instance_ref = self.db.instance_get(context, instance_id)
-
         if instance_ref["state"] == power_state.RUNNING:
             LOG.audit(_("instance %s: retrieving diagnostics"), instance_id,
                       context=context)

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-06-15 16:46:24 +0000
+++ nova/db/api.py	2011-06-20 15:20:30 +0000
@@ -419,6 +419,11 @@
     return IMPL.instance_stop(context, instance_id)
 
 
+def instance_get_by_uuid(context, uuid):
+    """Get an instance or raise if it does not exist."""
+    return IMPL.instance_get_by_uuid(context, uuid)
+
+
 def instance_get(context, instance_id):
     """Get an instance or raise if it does not exist."""
     return IMPL.instance_get(context, instance_id)

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-06-20 01:47:20 +0000
+++ nova/db/sqlalchemy/api.py	2011-06-20 15:20:30 +0000
@@ -797,6 +797,8 @@
     values['metadata'] = _metadata_refs(values.get('metadata'))
 
     instance_ref = models.Instance()
+    instance_ref['uuid'] = str(utils.gen_uuid())
+
     instance_ref.update(values)
 
     session = get_session()
@@ -859,39 +861,48 @@
 
 
 @require_context
+def instance_get_by_uuid(context, uuid, session=None):
+    partial = _build_instance_get(context, session=session)
+    result = partial.filter_by(uuid=uuid)
+    result = result.first()
+    if not result:
+        # FIXME(sirp): it would be nice if InstanceNotFound would accept a
+        # uuid parameter as well
+        raise exception.InstanceNotFound(instance_id=uuid)
+    return result
+
+
+@require_context
 def instance_get(context, instance_id, session=None):
-    if not session:
-        session = get_session()
-    result = None
-
-    if is_admin_context(context):
-        result = session.query(models.Instance).\
-                         options(joinedload_all('fixed_ip.floating_ips')).\
-                         options(joinedload_all('security_groups.rules')).\
-                         options(joinedload('volumes')).\
-                         options(joinedload_all('fixed_ip.network')).\
-                         options(joinedload('metadata')).\
-                         options(joinedload('instance_type')).\
-                         filter_by(id=instance_id).\
-                         filter_by(deleted=can_read_deleted(context)).\
-                         first()
-    elif is_user_context(context):
-        result = session.query(models.Instance).\
-                         options(joinedload_all('fixed_ip.floating_ips')).\
-                         options(joinedload_all('security_groups.rules')).\
-                         options(joinedload('volumes')).\
-                         options(joinedload('metadata')).\
-                         options(joinedload('instance_type')).\
-                         filter_by(project_id=context.project_id).\
-                         filter_by(id=instance_id).\
-                         filter_by(deleted=False).\
-                         first()
+    partial = _build_instance_get(context, session=session)
+    result = partial.filter_by(id=instance_id)
+    result = result.first()
     if not result:
         raise exception.InstanceNotFound(instance_id=instance_id)
-
     return result
 
 
+@require_context
+def _build_instance_get(context, session=None):
+    if not session:
+        session = get_session()
+
+    partial = session.query(models.Instance).\
+                     options(joinedload_all('fixed_ip.floating_ips')).\
+                     options(joinedload_all('security_groups.rules')).\
+                     options(joinedload('volumes')).\
+                     options(joinedload_all('fixed_ip.network')).\
+                     options(joinedload('metadata')).\
+                     options(joinedload('instance_type'))
+
+    if is_admin_context(context):
+        partial = partial.filter_by(deleted=can_read_deleted(context))
+    elif is_user_context(context):
+        partial = partial.filter_by(project_id=context.project_id).\
+                        filter_by(deleted=False)
+    return partial
+
+
 @require_admin_context
 def instance_get_all(context):
     session = get_session()

=== added file 'nova/db/sqlalchemy/migrate_repo/versions/025_add_uuid_to_instances.py'
--- nova/db/sqlalchemy/migrate_repo/versions/025_add_uuid_to_instances.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/025_add_uuid_to_instances.py	2011-06-20 15:20:30 +0000
@@ -0,0 +1,43 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 OpenStack LLC.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+from sqlalchemy import Column, Integer, MetaData, String, Table
+
+from nova import utils
+
+
+meta = MetaData()
+
+instances = Table("instances", meta,
+        Column("id", Integer(), primary_key=True, nullable=False))
+uuid_column = Column("uuid", String(36))
+
+
+def upgrade(migrate_engine):
+    meta.bind = migrate_engine
+    instances.create_column(uuid_column)
+
+    rows = migrate_engine.execute(instances.select())
+    for row in rows:
+        instance_uuid = str(utils.gen_uuid())
+        migrate_engine.execute(instances.update()\
+                .where(instances.c.id == row[0])\
+                .values(uuid=instance_uuid))
+
+
+def downgrade(migrate_engine):
+    meta.bind = migrate_engine
+    instances.drop_column(uuid_column)

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-06-15 16:46:24 +0000
+++ nova/db/sqlalchemy/models.py	2011-06-20 15:20:30 +0000
@@ -233,6 +233,7 @@
 
     os_type = Column(String(255))
     vm_mode = Column(String(255))
+    uuid = Column(String(36))
 
     # TODO(vish): see Ewan's email about state improvements, probably
     #             should be in a driver base class or some such

=== modified file 'nova/scheduler/api.py'
--- nova/scheduler/api.py	2011-06-15 13:40:42 +0000
+++ nova/scheduler/api.py	2011-06-20 15:20:30 +0000
@@ -24,6 +24,7 @@
 from nova import flags
 from nova import log as logging
 from nova import rpc
+from nova import utils
 
 from eventlet import greenpool
 
@@ -201,38 +202,78 @@
 
 
 class reroute_compute(object):
-    """Decorator used to indicate that the method should
-       delegate the call the child zones if the db query
-       can't find anything."""
+    """
+    reroute_compute is responsible for trying to lookup a resource in the
+    current zone and if it's not found there, delegating the call to the
+    child zones.
+
+    Since reroute_compute will be making 'cross-zone' calls, the ID for the
+    object must come in as a UUID-- if we receive an integer ID, we bail.
+
+    The steps involved are:
+
+        1. Validate that item_id is UUID like
+
+        2. Lookup item by UUID in the zone local database
+
+        3. If the item was found, then extract integer ID, and pass that to
+           the wrapped method. (This ensures that zone-local code can
+           continue to use integer IDs).
+
+        4. If the item was not found, we delgate the call to a child zone
+           using the UUID.
+    """
     def __init__(self, method_name):
         self.method_name = method_name
 
+    def _route_to_child_zones(self, context, collection, item_uuid):
+        if not FLAGS.enable_zone_routing:
+            raise exception.InstanceNotFound(instance_id=item_uuid)
+
+        zones = db.zone_get_all(context)
+        if not zones:
+            raise exception.InstanceNotFound(instance_id=item_uuid)
+
+        # Ask the children to provide an answer ...
+        LOG.debug(_("Asking child zones ..."))
+        result = self._call_child_zones(zones,
+                    wrap_novaclient_function(_issue_novaclient_command,
+                           collection, self.method_name, item_uuid))
+        # Scrub the results and raise another exception
+        # so the API layers can bail out gracefully ...
+        raise RedirectResult(self.unmarshall_result(result))
+
     def __call__(self, f):
         def wrapped_f(*args, **kwargs):
-            collection, context, item_id = \
+            collection, context, item_id_or_uuid = \
                             self.get_collection_context_and_id(args, kwargs)
-            try:
-                # Call the original function ...
+
+            attempt_reroute = False
+            if utils.is_uuid_like(item_id_or_uuid):
+                item_uuid = item_id_or_uuid
+                try:
+                    instance = db.instance_get_by_uuid(context, item_uuid)
+                except exception.InstanceNotFound, e:
+                    # NOTE(sirp): since a UUID was passed in, we can attempt
+                    # to reroute to a child zone
+                    attempt_reroute = True
+                    LOG.debug(_("Instance %(item_uuid)s not found "
+                                        "locally: '%(e)s'" % locals()))
+                else:
+                    # NOTE(sirp): since we're not re-routing in this case, and
+                    # we we were passed a UUID, we need to replace that UUID
+                    # with an integer ID in the argument list so that the
+                    # zone-local code can continue to use integer IDs.
+                    item_id = instance['id']
+                    args = list(args)      # needs to be mutable to replace
+                    self.replace_uuid_with_id(args, kwargs, item_id)
+
+            if attempt_reroute:
+                return self._route_to_child_zones(context, collection,
+                        item_uuid)
+            else:
                 return f(*args, **kwargs)
-            except exception.InstanceNotFound, e:
-                LOG.debug(_("Instance %(item_id)s not found "
-                                    "locally: '%(e)s'" % locals()))
-
-                if not FLAGS.enable_zone_routing:
-                    raise
-
-                zones = db.zone_get_all(context)
-                if not zones:
-                    raise
-
-                # Ask the children to provide an answer ...
-                LOG.debug(_("Asking child zones ..."))
-                result = self._call_child_zones(zones,
-                            wrap_novaclient_function(_issue_novaclient_command,
-                                   collection, self.method_name, item_id))
-                # Scrub the results and raise another exception
-                # so the API layers can bail out gracefully ...
-                raise RedirectResult(self.unmarshall_result(result))
+
         return wrapped_f
 
     def _call_child_zones(self, zones, function):
@@ -251,6 +292,18 @@
             instance_id = args[2]
         return ("servers", context, instance_id)
 
+    @staticmethod
+    def replace_uuid_with_id(args, kwargs, replacement_id):
+        """
+        Extracts the UUID parameter from the arg or kwarg list and replaces
+        it with an integer ID.
+        """
+        if 'instance_id' in kwargs:
+            kwargs['instance_id'] = replacement_id
+        elif len(args) > 1:
+            args.pop(2)
+            args.insert(2, replacement_id)
+
     def unmarshall_result(self, zone_responses):
         """Result is a list of responses from each child zone.
         Each decorator derivation is responsible to turning this

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-06-14 18:40:15 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-06-20 15:20:30 +0000
@@ -49,10 +49,22 @@
 FLAGS.verbose = True
 
 
-def return_server(context, id):
+FAKE_UUID = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
+
+
+def fake_gen_uuid():
+    return FAKE_UUID
+
+
+def return_server_by_id(context, id):
     return stub_instance(id)
 
 
+def return_server_by_uuid(context, uuid):
+    id = 1
+    return stub_instance(id, uuid=uuid)
+
+
 def return_server_with_addresses(private, public):
     def _return_server(context, id):
         return stub_instance(id, private_address=private,
@@ -111,7 +123,8 @@
 
 
 def stub_instance(id, user_id=1, private_address=None, public_addresses=None,
-                  host=None, power_state=0, reservation_id=""):
+                  host=None, power_state=0, reservation_id="",
+                  uuid=FAKE_UUID):
     metadata = []
     metadata.append(InstanceMetadata(key='seq', value=id))
 
@@ -129,7 +142,7 @@
         server_name = "reservation_%s" % (reservation_id, )
 
     instance = {
-        "id": id,
+        "id": int(id),
         "admin_pass": "",
         "user_id": user_id,
         "project_id": "",
@@ -157,7 +170,8 @@
         "display_name": server_name,
         "display_description": "",
         "locked": False,
-        "metadata": metadata}
+        "metadata": metadata,
+        "uuid": uuid}
 
     instance["fixed_ip"] = {
         "address": private_address,
@@ -196,8 +210,11 @@
         fakes.stub_out_auth(self.stubs)
         fakes.stub_out_key_pair_funcs(self.stubs)
         fakes.stub_out_image_service(self.stubs)
+        self.stubs.Set(utils, 'gen_uuid', fake_gen_uuid)
         self.stubs.Set(nova.db.api, 'instance_get_all', return_servers)
-        self.stubs.Set(nova.db.api, 'instance_get', return_server)
+        self.stubs.Set(nova.db.api, 'instance_get', return_server_by_id)
+        self.stubs.Set(nova.db, 'instance_get_by_uuid',
+                       return_server_by_uuid)
         self.stubs.Set(nova.db.api, 'instance_get_all_by_user',
                        return_servers)
         self.stubs.Set(nova.db.api, 'instance_add_security_group',
@@ -229,6 +246,36 @@
         self.assertEqual(res_dict['server']['id'], 1)
         self.assertEqual(res_dict['server']['name'], 'server1')
 
+    def test_get_server_by_uuid(self):
+        """
+        The steps involved with resolving a UUID are pretty complicated;
+        here's what's happening in this scenario:
+
+        1. Show is calling `routing_get`
+
+        2. `routing_get` is wrapped by `reroute_compute` which does the work
+           of resolving requests to child zones.
+
+        3. `reroute_compute` looks up the UUID by hitting the stub
+           (returns_server_by_uuid)
+
+        4. Since the stub return that the record exists, `reroute_compute`
+           considers the request to be 'zone local', so it replaces the UUID
+           in the argument list with an integer ID and then calls the inner
+           function ('get').
+
+        5. The call to `get` hits the other stub 'returns_server_by_id` which
+           has the UUID set to FAKE_UUID
+
+        So, counterintuitively, we call `get` twice on the `show` command.
+        """
+        req = webob.Request.blank('/v1.0/servers/%s' % FAKE_UUID)
+        res = req.get_response(fakes.wsgi_app())
+        res_dict = json.loads(res.body)
+        self.assertEqual(res_dict['server']['id'], 1)
+        self.assertEqual(res_dict['server']['uuid'], FAKE_UUID)
+        self.assertEqual(res_dict['server']['name'], 'server1')
+
     def test_get_server_by_id_v1_1(self):
         req = webob.Request.blank('/v1.1/servers/1')
         res = req.get_response(fakes.wsgi_app())
@@ -540,7 +587,8 @@
     def _setup_for_create_instance(self):
         """Shared implementation for tests below that create instance"""
         def instance_create(context, inst):
-            return {'id': '1', 'display_name': 'server_test'}
+            return {'id': 1, 'display_name': 'server_test',
+                    'uuid': FAKE_UUID}
 
         def server_update(context, id, params):
             return instance_create(context, id)
@@ -594,11 +642,22 @@
         self.assertEqual(1, server['id'])
         self.assertEqual(2, server['flavorId'])
         self.assertEqual(3, server['imageId'])
+        self.assertEqual(FAKE_UUID, server['uuid'])
         self.assertEqual(res.status_int, 200)
 
     def test_create_instance(self):
         self._test_create_instance_helper()
 
+    def test_create_instance_has_uuid(self):
+        """Tests at the db-layer instead of API layer since that's where the
+           UUID is generated
+        """
+        ctxt = context.RequestContext(1, 1)
+        values = {}
+        instance = nova.db.api.instance_create(ctxt, values)
+        expected = FAKE_UUID
+        self.assertEqual(instance['uuid'], expected)
+
     def test_create_instance_via_zones(self):
         """Server generated ReservationID"""
         self._setup_for_create_instance()
@@ -1850,7 +1909,8 @@
                     self.injected_files = kwargs['injected_files']
                 else:
                     self.injected_files = None
-                return [{'id': '1234', 'display_name': 'fakeinstance'}]
+                return [{'id': '1234', 'display_name': 'fakeinstance',
+                         'uuid': FAKE_UUID}]
 
             def set_admin_password(self, *args, **kwargs):
                 pass

=== modified file 'nova/tests/scheduler/test_scheduler.py'
--- nova/tests/scheduler/test_scheduler.py	2011-06-09 18:52:53 +0000
+++ nova/tests/scheduler/test_scheduler.py	2011-06-20 15:20:30 +0000
@@ -48,6 +48,10 @@
 flags.DECLARE('instances_path', 'nova.compute.manager')
 
 
+FAKE_UUID_NOT_FOUND = 'ffffffff-ffff-ffff-ffff-ffffffffffff'
+FAKE_UUID = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
+
+
 class TestDriver(driver.Scheduler):
     """Scheduler Driver for Tests"""
     def schedule(context, topic, *args, **kwargs):
@@ -926,12 +930,23 @@
            ]
 
 
+def fake_instance_get_by_uuid(context, uuid):
+    if FAKE_UUID_NOT_FOUND:
+        raise exception.InstanceNotFound(instance_id=uuid)
+    else:
+        return {'id': 1}
+
+
 class FakeRerouteCompute(api.reroute_compute):
+    def __init__(self, method_name, id_to_return=1):
+        super(FakeRerouteCompute, self).__init__(method_name)
+        self.id_to_return = id_to_return
+
     def _call_child_zones(self, zones, function):
         return []
 
     def get_collection_context_and_id(self, args, kwargs):
-        return ("servers", None, 1)
+        return ("servers", None, self.id_to_return)
 
     def unmarshall_result(self, zone_responses):
         return dict(magic="found me")
@@ -960,6 +975,8 @@
         self.stubs = stubout.StubOutForTesting()
 
         self.stubs.Set(db, 'zone_get_all', zone_get_all)
+        self.stubs.Set(db, 'instance_get_by_uuid',
+                       fake_instance_get_by_uuid)
 
         self.enable_zone_routing = FLAGS.enable_zone_routing
         FLAGS.enable_zone_routing = True
@@ -976,8 +993,19 @@
         except api.RedirectResult, e:
             self.fail(_("Successful database hit should succeed"))
 
-    def test_trap_not_found_locally(self):
+    def test_trap_not_found_locally_id_passed(self):
+        """When an integer ID is not found locally, we cannot reroute to
+        another zone, so just return InstanceNotFound exception
+        """
         decorator = FakeRerouteCompute("foo")
+        self.assertRaises(exception.InstanceNotFound,
+            decorator(go_boom), None, None, 1)
+
+    def test_trap_not_found_locally_uuid_passed(self):
+        """When a UUID is found, if the item isn't found locally, we should
+        try to reroute to a child zone to see if they have it
+        """
+        decorator = FakeRerouteCompute("foo", id_to_return=FAKE_UUID_NOT_FOUND)
         try:
             result = decorator(go_boom)(None, None, 1)
             self.assertFail(_("Should have rerouted."))

=== modified file 'nova/tests/test_utils.py'
--- nova/tests/test_utils.py	2011-05-11 18:02:01 +0000
+++ nova/tests/test_utils.py	2011-06-20 15:20:30 +0000
@@ -275,3 +275,21 @@
         # error case
         result = utils.parse_server_string('www.exa:mple.com:8443')
         self.assertEqual(('', ''), result)
+
+
+class IsUUIDLikeTestCase(test.TestCase):
+    def assertUUIDLike(self, val, expected):
+        result = utils.is_uuid_like(val)
+        self.assertEqual(result, expected)
+
+    def test_good_uuid(self):
+        val = 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa'
+        self.assertUUIDLike(val, True)
+
+    def test_integer_passed(self):
+        val = 1
+        self.assertUUIDLike(val, False)
+
+    def test_non_uuid_string_passed(self):
+        val = 'foo-fooo'
+        self.assertUUIDLike(val, False)

=== modified file 'nova/utils.py'
--- nova/utils.py	2011-06-07 19:17:34 +0000
+++ nova/utils.py	2011-06-20 15:20:30 +0000
@@ -35,6 +35,7 @@
 import sys
 import time
 import types
+import uuid
 from xml.sax import saxutils
 
 from eventlet import event
@@ -726,3 +727,17 @@
     except:
         LOG.debug(_('Invalid server_string: %s' % server_str))
         return ('', '')
+
+
+def gen_uuid():
+    return uuid.uuid4()
+
+
+def is_uuid_like(val):
+    """For our purposes, a UUID is a string in canoical form:
+
+        aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa
+    """
+    if not isinstance(val, basestring):
+        return False
+    return (len(val) == 36) and (val.count('-') == 4)