Merge lp:~justin-fathomdb/nova/justinsb-metadata into lp:~hudson-openstack/nova/trunk

[justinsb]

Initial support for per-instance metadata, though the OpenStack API. Key/value pairs can be specified at instance creation time and are returned in the details view. Support limits based on quota system.

Initial support for per-instance metadata, though the OpenStack API. Key/value pairs can be specified at instance creation time and are returned in the details view. Support limits based on quota system.

[Dan Prince (dan-prince)]

Justin,

Does this branch work with Glance? I'm pretty sure Glance stores kernel ID as image['properties']['kernel_id']. Additionally ramdisk ID is stored as image['properties']['ramdisk_id'].

Perhaps you were developing/testing against S3ImageStore?

Dan

review: Needs Fixing

[Jay Pipes (jaypipes)]

To answer Dan's question, no, this patch won't work if FLAGS.image_service == 'glance'. But, that's not necessarily Justin's fault of course, and the OpenStack servers.py controller shouldn't be relying on the Glance image service being set, either...

Devin and Dan have been working on fixing the inconsistencies in the keys returned from the S3ImageService and the GlanceImageService when it comes to the kernel_id (kernelId) and ramdisk_id (ramdiskId) keys. Please see this bug and patch here for a few more details: https://bugs.launchpad.net/nova/+bug/712782

As for the specifics of this patch, Justin, I think it would be clearer if you renamed the Metadata model to InstanceProperty and the table name from metadata to instance_properties, to make it clear what the metadata properties refer to. Since there is already the concept of Metadata in SQLAlchemy, it would make reading the code a tad easier.

Cheers!
jay

[justinsb (justin-fathomdb)]

I think I'll probably revert the lines that touched the kernel_id & ramdisk_id - I had to put them in there to get this to work, but it probably doesn't really belong in this patch.

I agree that 'metadata', 'key' and 'value' are potentially problematic names. Two issues though: First, there will be metadata for more than just instances - I can either have one table 'item_properties', or I can go with InstanceProperties, VolumeProperties etc and have multiple tables. And second, "properties" may be a bit overloaded already (e.g. the images already has 'properties' in Dan's code snippet!) Although I'd like the special flags (kernel_id, ramdisk_id, zone, image_type, volume_size etc) to just be treated as well-known metadata, I don't see this happening any time soon, so I don't think we can use 'properties'. Maybe 'Tags'? What do you think Jay?

[Jay Pipes (jaypipes)]

> I think I'll probably revert the lines that touched the kernel_id & ramdisk_id
> - I had to put them in there to get this to work, but it probably doesn't
> really belong in this patch.

k, agreed.

> I agree that 'metadata', 'key' and 'value' are potentially problematic names.
> Two issues though: First, there will be metadata for more than just instances
> - I can either have one table 'item_properties', or I can go with
> InstanceProperties, VolumeProperties etc and have multiple tables. And
> second, "properties" may be a bit overloaded already (e.g. the images already
> has 'properties' in Dan's code snippet!)

Heh, yes it does, because Glance's registry database has a table image_properties, with a model, yep, you guessed it, ImageProperty :)

So, I think that InstanceProperties is perfectly fine in this case :)

> Although I'd like the special flags
> (kernel_id, ramdisk_id, zone, image_type, volume_size etc) to just be treated
> as well-known metadata, I don't see this happening any time soon, so I don't
> think we can use 'properties'. Maybe 'Tags'? What do you think Jay?

This should be a mailing list question, I believe. If it's an attribute that *every* instance has, or possibly that *most* instances have, I think it should be in the Instance model directly, but this is just my opinion. Best to ask the ML I think...

-jay

[Dan Prince (dan-prince)]

This branch currently doesn't appear to restrict the number of metadata items per instances(server) to 5.

According to the Cloud Servers API docs:

"The maximum size of the metadata key and value is each 255 bytes and the maximum number of key-value pairs that can be supplied per server is 5."

review: Needs Fixing

[Jay Pipes (jaypipes)]

<diplomacy-hat>
OpenStack != Cloud Servers, of course, but I would slightly modify what Dan said to:

Could we add a FLAG option that would allow the deployer to set a limit to the number of metadata items per object? ;)
</diplomacy-hat>

[Devin Carlen (devcamcar)]

Agree, make the number of metadata items configurable

[lp:~justin-fathomdb/nova/justinsb-metadata]

667. By SuperStack <email address hidden> on 2011-02-15

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

[Dan Prince (dan-prince)]

Absolutely. Making this a configuration flag would be great.

I might suggest we not make the FLAG option default to being 'unlimited'. The ability to specify an unlimited number of items seems like kind of a security concern.

10 items maybe?

[lp:~justin-fathomdb/nova/justinsb-metadata]

668. By SuperStack <email address hidden> on 2011-02-15

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

669. By SuperStack <email address hidden> on 2011-02-17

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

670. By SuperStack <email address hidden> on 2011-02-17

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

671. By SuperStack <email address hidden> on 2011-02-17

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

672. By SuperStack <email address hidden> on 2011-02-17

Fixed up note

673. By SuperStack <email address hidden> on 2011-02-17

Comment to confirm that dictionary use is intentional

[Todd Willey (xtoddx)]

lgtm

review: Approve

Unmerged revisions

673. By SuperStack <email address hidden> on 2011-02-17

Comment to confirm that dictionary use is intentional

672. By SuperStack <email address hidden> on 2011-02-17

Fixed up note

671. By SuperStack <email address hidden> on 2011-02-17

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

670. By SuperStack <email address hidden> on 2011-02-17

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

669. By SuperStack <email address hidden> on 2011-02-17

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

668. By SuperStack <email address hidden> on 2011-02-15

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

667. By SuperStack <email address hidden> on 2011-02-15

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

666. By SuperStack <email address hidden> on 2011-02-12

Merge branch 'metadata' of https://github.com/justinsb/nova into justinsb-metadata

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2011-01-26 16:16:16 +0000
+++ nova/api/openstack/servers.py	2011-02-15 21:42:14 +0000
@@ -32,6 +32,7 @@
 from nova.compute import instance_types
 from nova.compute import power_state
 import nova.api.openstack
+from nova.db.sqlalchemy.models import InstanceProperty
 
 
 LOG = logging.getLogger('server')
@@ -64,9 +65,13 @@
 
     inst_dict['status'] = power_mapping[inst_dict['status']]
     inst_dict['addresses'] = dict(public=[], private=[])
-    inst_dict['metadata'] = {}
     inst_dict['hostId'] = ''
 
+    metadata = {}
+    for item in inst['properties']:
+        metadata[item['key']] = item['value']
+    inst_dict['metadata'] = metadata
+
     return dict(server=inst_dict)
 
 
@@ -148,14 +153,26 @@
         if not env:
             return faults.Fault(exc.HTTPUnprocessableEntity())
 
-        key_pair = auth_manager.AuthManager.get_key_pairs(
-            req.environ['nova.context'])[0]
+        context = req.environ['nova.context']
+
+        key_pair = auth_manager.AuthManager.get_key_pairs(context)[0]
         image_id = common.get_image_id_from_image_hash(self._image_service,
-            req.environ['nova.context'], env['server']['imageId'])
+            context, env['server']['imageId'])
         kernel_id, ramdisk_id = self._get_kernel_ramdisk_from_image(
             req, image_id)
+
+        # Metadata is a list, not a Dictionary, because we allow duplicate keys
+        # (even though JSON can't encode this)
+        # In future, we may not allow duplicate keys.
+        # However, the CloudServers API is not definitive on this front,
+        #  and we want to be compatible.
+        properties = []
+        if env['server']['metadata']:
+            for k, v in env['server']['metadata'].items():
+                properties.append(InstanceProperty(key=k, value=v))
+
         instances = self.compute_api.create(
-            req.environ['nova.context'],
+            context,
             instance_types.get_by_flavor_id(env['server']['flavorId']),
             image_id,
             kernel_id=kernel_id,
@@ -163,7 +180,8 @@
             display_name=env['server']['name'],
             display_description=env['server']['name'],
             key_name=key_pair['name'],
-            key_data=key_pair['public_key'])
+            key_data=key_pair['public_key'],
+            properties=properties)
         return _translate_keys(instances[0])
 
     def update(self, req, id):

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-02-15 18:19:52 +0000
+++ nova/compute/api.py	2011-02-15 21:42:14 +0000
@@ -85,7 +85,7 @@
                min_count=1, max_count=1,
                display_name='', display_description='',
                key_name=None, key_data=None, security_group='default',
-               availability_zone=None, user_data=None):
+               availability_zone=None, user_data=None, properties=[]):
         """Create the number of instances requested if quota and
         other arguments check out ok."""
 
@@ -99,6 +99,30 @@
                                      "run %s more instances of this type.") %
                                    num_instances, "InstanceLimitExceeded")
 
+        num_metadata = len(properties)
+        quota_metadata = quota.allowed_metadata_items(context, num_metadata)
+        if quota_metadata < num_metadata:
+            pid = context.project_id
+            msg = (_("Quota exceeeded for %(pid)s,"
+                     " tried to set %(num_metadata)s metadata properties")
+                   % locals())
+            LOG.warn(msg)
+            raise quota.QuotaError(msg, "MetadataLimitExceeded")
+
+        # Because metadata is stored in the DB, we hard-code the size limits
+        # In future, we may support more variable length strings, so we act
+        #  as if this is quota-controlled for forwards compatibility
+        for metadata_item in properties:
+            k = metadata_item.key
+            v = metadata_item.value
+            if len(k) > 255 or len(v) > 255:
+                pid = context.project_id
+                msg = (_("Quota exceeeded for %(pid)s,"
+                         " metadata property key or value too long")
+                       % locals())
+                LOG.warn(msg)
+                raise quota.QuotaError(msg, "MetadataLimitExceeded")
+
         is_vpn = image_id == FLAGS.vpn_image_id
         if not is_vpn:
             image = self.image_service.show(context, image_id)
@@ -155,7 +179,8 @@
             'key_name': key_name,
             'key_data': key_data,
             'locked': False,
-            'availability_zone': availability_zone}
+            'availability_zone': availability_zone,
+            'properties': properties}
 
         elevated = context.elevated()
         instances = []

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-02-11 19:30:56 +0000
+++ nova/db/sqlalchemy/api.py	2011-02-15 21:42:14 +0000
@@ -693,6 +693,7 @@
                          options(joinedload_all('security_groups.rules')).\
                          options(joinedload('volumes')).\
                          options(joinedload_all('fixed_ip.network')).\
+                         options(joinedload('properties')).\
                          filter_by(id=instance_id).\
                          filter_by(deleted=can_read_deleted(context)).\
                          first()
@@ -701,6 +702,7 @@
                          options(joinedload_all('fixed_ip.floating_ips')).\
                          options(joinedload_all('security_groups.rules')).\
                          options(joinedload('volumes')).\
+                         options(joinedload('properties')).\
                          filter_by(project_id=context.project_id).\
                          filter_by(id=instance_id).\
                          filter_by(deleted=False).\

=== added file 'nova/db/sqlalchemy/migrate_repo/versions/003_add_instance_metadata.py'
--- nova/db/sqlalchemy/migrate_repo/versions/003_add_instance_metadata.py	1970-01-01 00:00:00 +0000
+++ nova/db/sqlalchemy/migrate_repo/versions/003_add_instance_metadata.py	2011-02-15 21:42:14 +0000
@@ -0,0 +1,78 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright 2011 Justin Santa Barbara
+# All Rights Reserved.
+#
+#    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 *
+from migrate import *
+
+from nova import log as logging
+
+
+meta = MetaData()
+
+
+# Just for the ForeignKey and column creation to succeed, these are not the
+# actual definitions of instances or services.
+instances = Table('instances', meta,
+        Column('id', Integer(), primary_key=True, nullable=False),
+        )
+
+quotas = Table('quotas', meta,
+        Column('id', Integer(),  primary_key=True, nullable=False),
+        )
+
+
+#
+# New Tables
+#
+
+instance_properties_table = Table('instance_properties', meta,
+        Column('created_at', DateTime(timezone=False)),
+        Column('updated_at', DateTime(timezone=False)),
+        Column('deleted_at', DateTime(timezone=False)),
+        Column('deleted', Boolean(create_constraint=True, name=None)),
+        Column('id', Integer(), primary_key=True, nullable=False),
+        Column('instance_id',
+               Integer(),
+               ForeignKey('instances.id'),
+               nullable=False),
+        Column('key',
+               String(length=255, convert_unicode=False, assert_unicode=None,
+                      unicode_error=None, _warn_on_bytestring=False)),
+        Column('value',
+               String(length=255, convert_unicode=False, assert_unicode=None,
+                      unicode_error=None, _warn_on_bytestring=False)))
+
+
+#
+# New columns
+#
+quota_metadata_items = Column('metadata_items', Integer())
+
+
+def upgrade(migrate_engine):
+    # Upgrade operations go here. Don't create your own engine;
+    # bind migrate_engine to your metadata
+    meta.bind = migrate_engine
+    for table in (instance_properties_table, ):
+        try:
+            table.create()
+        except Exception:
+            logging.info(repr(table))
+            logging.exception('Exception while creating table')
+            raise
+
+    quotas.create_column(quota_metadata_items)

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-02-01 16:09:34 +0000
+++ nova/db/sqlalchemy/models.py	2011-02-15 21:42:14 +0000
@@ -256,6 +256,7 @@
     volumes = Column(Integer)
     gigabytes = Column(Integer)
     floating_ips = Column(Integer)
+    metadata_items = Column(Integer)
 
 
 class ExportDevice(BASE, NovaBase):
@@ -535,6 +536,20 @@
     pool = relationship(ConsolePool, backref=backref('consoles'))
 
 
+class InstanceProperty(BASE, NovaBase):
+    """Represents a metadata key/value pair for an instance"""
+    __tablename__ = 'instance_properties'
+    id = Column(Integer, primary_key=True)
+    key = Column(String(255))
+    value = Column(String(255))
+    instance_id = Column(Integer, ForeignKey('instances.id'), nullable=False)
+    instance = relationship(Instance, backref="properties",
+                            foreign_keys=instance_id,
+                            primaryjoin='and_('
+                                'InstanceProperty.instance_id == Instance.id,'
+                                'InstanceProperty.deleted == False)')
+
+
 def register_models():
     """Register Models and create metadata.
 
@@ -547,7 +562,9 @@
               Volume, ExportDevice, IscsiTarget, FixedIp, FloatingIp,
               Network, SecurityGroup, SecurityGroupIngressRule,
               SecurityGroupInstanceAssociation, AuthToken, User,
-              Project, Certificate, ConsolePool, Console)  # , Image, Host
+              Project, Certificate, ConsolePool, Console,
+              InstanceProperty)
+     # , Image, Host
     engine = create_engine(FLAGS.sql_connection, echo=False)
     for model in models:
         model.metadata.create_all(engine)

=== modified file 'nova/quota.py'
--- nova/quota.py	2011-01-04 21:56:36 +0000
+++ nova/quota.py	2011-02-15 21:42:14 +0000
@@ -35,6 +35,8 @@
                      'number of volume gigabytes allowed per project')
 flags.DEFINE_integer('quota_floating_ips', 10,
                      'number of floating ips allowed per project')
+flags.DEFINE_integer('quota_metadata_items', 128,
+                     'number of metadata items allowed per instance')
 
 
 def get_quota(context, project_id):
@@ -42,7 +44,8 @@
             'cores': FLAGS.quota_cores,
             'volumes': FLAGS.quota_volumes,
             'gigabytes': FLAGS.quota_gigabytes,
-            'floating_ips': FLAGS.quota_floating_ips}
+            'floating_ips': FLAGS.quota_floating_ips,
+            'metadata_items': FLAGS.quota_metadata_items}
     try:
         quota = db.quota_get(context, project_id)
         for key in rval.keys():
@@ -94,6 +97,15 @@
     return min(num_floating_ips, allowed_floating_ips)
 
 
+def allowed_metadata_items(context, num_metadata_items):
+    """Check quota; return min(num_metadata_items,allowed_metadata_items)"""
+    project_id = context.project_id
+    context = context.elevated()
+    quota = get_quota(context, project_id)
+    num_allowed_metadata_items = quota['metadata_items']
+    return min(num_metadata_items, num_allowed_metadata_items)
+
+
 class QuotaError(exception.ApiError):
     """Quota Exceeeded"""
     pass

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2011-01-27 18:48:00 +0000
+++ nova/tests/api/openstack/test_servers.py	2011-02-15 21:42:14 +0000
@@ -27,6 +27,7 @@
 from nova.api.openstack import servers
 import nova.db.api
 from nova.db.sqlalchemy.models import Instance
+from nova.db.sqlalchemy.models import InstanceProperty
 import nova.rpc
 from nova.tests.api.openstack import fakes
 
@@ -56,8 +57,11 @@
 
 
 def stub_instance(id, user_id=1):
+    metadata = []
+    metadata.append(InstanceProperty(key='seq', value=id))
     return Instance(id=id, state=0, image_id=10, user_id=user_id,
-                    display_name='server%s' % id)
+                    display_name='server%s' % id,
+                    metadata=metadata)
 
 
 def fake_compute_api(cls, req, id):
@@ -154,7 +158,8 @@
             "get_image_id_from_image_hash", image_id_from_hash)
 
         body = dict(server=dict(
-            name='server_test', imageId=2, flavorId=2, metadata={},
+            name='server_test', imageId=2, flavorId=2,
+            metadata={'hello': 'world', 'open': 'stack'},
             personality={}))
         req = webob.Request.blank('/v1.0/servers')
         req.method = 'POST'
@@ -231,6 +236,7 @@
             self.assertEqual(s['id'], i)
             self.assertEqual(s['name'], 'server%d' % i)
             self.assertEqual(s['imageId'], 10)
+            self.assertEqual(s['metadata']['seq'], i)
             i += 1
 
     def test_server_pause(self):

=== modified file 'run_tests.sh'
--- run_tests.sh	2011-01-31 05:55:32 +0000
+++ run_tests.sh	2011-02-15 21:42:14 +0000
@@ -73,7 +73,9 @@
 
 if [ -z "$noseargs" ];
 then
-  run_tests && pep8 --repeat --show-pep8 --show-source --exclude=vcsversion.py bin/* nova setup.py || exit 1
+  srcfiles=`find bin -type f ! -name "nova.conf*"`
+  srcfiles+=" nova setup.py"
+  run_tests && pep8 --repeat --show-pep8 --show-source --exclude=vcsversion.py ${srcfiles} || exit 1
 else
   run_tests
 fi