Merge lp:~jaypipes/nova/compute-manager-instance-data into lp:~hudson-openstack/nova/trunk

[Jay Pipes]

Moves db writes into compute manager class. Cleans up sqlalchemy model/api to remove redundant calls for updating what is really a dict.

Moves db writes into compute manager class. Cleans up sqlalchemy model/api to remove redundant calls for updating what is really a dict.

[Eric Day (eday)]

The second part looks fine, but any reason you build a dict and pass that for instance_data rather than just using kwargs directly for create/update_instance? For example:

self.compute_manager.create_instance(context, security_groups=security_groups, mac_address=utils.generate_mac(), launch_index=num, **base_options)

And then just use **kwargs inside the methods to pass to the DB layer?

Also, this has text conflicts with trunk and introduces pep8 violations. :)

review: Needs Fixing

[Jay Pipes (jaypipes)]

On Mon, Oct 25, 2010 at 7:38 PM, Eric Day <email address hidden> wrote:
> Review: Needs Fixing
> The second part looks fine, but any reason you build a dict and pass that for instance_data rather than just using kwargs directly for create/update_instance? For example:
>
> self.compute_manager.create_instance(context, security_groups=security_groups, mac_address=utils.generate_mac(), launch_index=num, **base_options)
>
> And then just use **kwargs inside the methods to pass to the DB layer?

Ya, good point :)

> Also, this has text conflicts with trunk and introduces pep8 violations. :)

I will fix em shortly.

Cheers!
jay

[Eric Day (eday)]

Hmm, this might be too nit picky, but this still seems a bit more verbose than it needs to be. :)

For example, lines 18-24 could just be:

instance_ref = self.compute_manager.create_instance(context, security_groups, mac_address=utils.generate_mac(), launch_index=num, **base_options)

And create_instance would be changed to:

def create_instance(self, context, security_groups=[], **kwargs):
    instance_ref = self.db.instance_create(context, kwargs)
    inst_id = instance_ref['id']
...

Also, 57-62 could be:

self.compute_manager.update_instance(context, instance_ref['id'], state_description='terminating', state=0, terminated_at=now)

And again use **kwargs directly in update_instance. I guess I just prefer passing kwargs directly rather than passing in a dict for these sorts of things.

review: Needs Fixing

[Devin Carlen (devcamcar)]

Good stuff! Always love to see the middle tier emerge more.

review: Approve

[Eric Day (eday)]

The 'updated_data' variable is no longer used (lines 54-56). Other than that, looks good!

review: Needs Fixing

[Jay Pipes (jaypipes)]

OK, removed the updated_data variable.

[lp:~jaypipes/nova/compute-manager-instance-data]

376. By Jay Pipes on 2010-10-28

Remove unused updated_data variable

[Eric Day (eday)]

lgtm!

review: Approve

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2010-10-26 00:44:23 +0000
+++ nova/api/ec2/cloud.py	2010-10-28 15:46:05 +0000
@@ -99,6 +99,7 @@
 """
     def __init__(self):
         self.network_manager = utils.import_object(FLAGS.network_manager)
+        self.compute_manager = utils.import_object(FLAGS.compute_manager)
         self.setup()
 
     def __str__(self):
@@ -835,21 +836,21 @@
         elevated = context.elevated()
 
         for num in range(num_instances):
-            instance_ref = db.instance_create(context, base_options)
+
+            instance_ref = self.compute_manager.create_instance(context,
+                                           security_groups,
+                                           mac_address=utils.generate_mac(),
+                                           launch_index=num,
+                                           **base_options)
             inst_id = instance_ref['id']
 
-            for security_group_id in security_groups:
-                db.instance_add_security_group(elevated,
-                                               inst_id,
-                                               security_group_id)
-
-            inst = {}
-            inst['mac_address'] = utils.generate_mac()
-            inst['launch_index'] = num
             internal_id = instance_ref['internal_id']
             ec2_id = internal_id_to_ec2_id(internal_id)
-            inst['hostname'] = ec2_id
-            db.instance_update(context, inst_id, inst)
+
+            self.compute_manager.update_instance(context,
+                                                 inst_id,
+                                                 hostname=ec2_id)
+
             # TODO(vish): This probably should be done in the scheduler
             #             or in compute as a call.  The network should be
             #             allocated after the host is assigned and setup
@@ -895,11 +896,12 @@
                               id_str)
                 continue
             now = datetime.datetime.utcnow()
-            db.instance_update(context,
-                               instance_ref['id'],
-                               {'state_description': 'terminating',
-                                'state': 0,
-                                'terminated_at': now})
+            self.compute_manager.update_instance(context,
+                                         instance_ref['id'],
+                                         state_description='terminating',
+                                         state=0,
+                                         terminated_at=now)
+
             # FIXME(ja): where should network deallocate occur?
             address = db.instance_get_floating_address(context,
                                                        instance_ref['id'])

=== modified file 'nova/api/openstack/servers.py'
--- nova/api/openstack/servers.py	2010-10-21 22:26:06 +0000
+++ nova/api/openstack/servers.py	2010-10-28 15:46:05 +0000
@@ -95,6 +95,7 @@
             db_driver = FLAGS.db_driver
         self.db_driver = utils.import_object(db_driver)
         self.network_manager = utils.import_object(FLAGS.network_manager)
+        self.compute_manager = utils.import_object(FLAGS.compute_manager)
         super(Controller, self).__init__()
 
     def index(self, req):
@@ -242,34 +243,30 @@
         inst['memory_mb'] = flavor['memory_mb']
         inst['vcpus'] = flavor['vcpus']
         inst['local_gb'] = flavor['local_gb']
-
-        ref = self.db_driver.instance_create(ctxt, inst)
-        inst['id'] = ref.internal_id
-
         inst['mac_address'] = utils.generate_mac()
-
-        #TODO(dietz) is this necessary?
         inst['launch_index'] = 0
 
-        inst['hostname'] = str(ref.internal_id)
-        self.db_driver.instance_update(ctxt, inst['id'], inst)
-
-        network_manager = utils.import_object(FLAGS.network_manager)
-        address = network_manager.allocate_fixed_ip(ctxt,
-            inst['id'])
+        ref = self.compute_manager.create_instance(ctxt, **inst)
+        inst['id'] = ref['internal_id']
+
+        inst['hostname'] = str(ref['internal_id'])
+        self.compute_manager.update_instance(ctxt, inst['id'], **inst)
+
+        address = self.network_manager.allocate_fixed_ip(ctxt,
+                                                         inst['id'])
 
         # TODO(vish): This probably should be done in the scheduler
         #             network is setup when host is assigned
-        network_topic = self._get_network_topic(ctxt, network_manager)
+        network_topic = self._get_network_topic(ctxt)
         rpc.call(ctxt,
                  network_topic,
                  {"method": "setup_fixed_ip",
                   "args": {"address": address}})
         return inst
 
-    def _get_network_topic(self, context, network_manager):
+    def _get_network_topic(self, context):
         """Retrieves the network host for a project"""
-        network_ref = network_manager.get_network(context)
+        network_ref = self.network_manager.get_network(context)
         host = network_ref['host']
         if not host:
             host = rpc.call(context,

=== modified file 'nova/compute/manager.py'
--- nova/compute/manager.py	2010-10-25 02:52:02 +0000
+++ nova/compute/manager.py	2010-10-28 15:46:05 +0000
@@ -69,6 +69,46 @@
     def refresh_security_group(self, context, security_group_id, **_kwargs):
         yield self.driver.refresh_security_group(security_group_id)
 
+    def create_instance(self, context, security_groups=[], **kwargs):
+        """Creates the instance in the datastore and returns the
+        new instance as a mapping
+
+        :param context: The security context
+        :param security_groups: list of security group ids to
+                                attach to the instance
+        :param **kwargs: All additional keyword args are treated
+                         as data fields of the instance to be
+                         created
+
+        :retval Returns a mapping of the instance information
+                that has just been created
+
+        """
+        instance_ref = self.db.instance_create(context, kwargs)
+        inst_id = instance_ref['id']
+
+        elevated = context.elevated()
+        security_groups = kwargs.get('security_groups', [])
+        for security_group_id in security_groups:
+            self.db.instance_add_security_group(elevated,
+                                                inst_id,
+                                                security_group_id)
+        return instance_ref
+
+    def update_instance(self, context, instance_id, **kwargs):
+        """Updates the instance in the datastore
+
+        :param context: The security context
+        :param instance_id: ID of the instance to update
+        :param **kwargs: All additional keyword args are treated
+                         as data fields of the instance to be
+                         updated
+
+        :retval None
+
+        """
+        self.db.instance_update(context, instance_id, kwargs)
+
     @defer.inlineCallbacks
     @exception.wrap_exception
     def run_instance(self, context, instance_id, **_kwargs):

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2010-10-26 00:15:56 +0000
+++ nova/db/sqlalchemy/api.py	2010-10-28 15:46:05 +0000
@@ -236,8 +236,7 @@
 @require_admin_context
 def service_create(context, values):
     service_ref = models.Service()
-    for (key, value) in values.iteritems():
-        service_ref[key] = value
+    service_ref.update(values)
     service_ref.save()
     return service_ref
 
@@ -247,8 +246,7 @@
     session = get_session()
     with session.begin():
         service_ref = service_get(context, service_id, session=session)
-        for (key, value) in values.iteritems():
-            service_ref[key] = value
+        service_ref.update(values)
         service_ref.save(session=session)
 
 
@@ -279,8 +277,7 @@
 @require_context
 def floating_ip_create(context, values):
     floating_ip_ref = models.FloatingIp()
-    for (key, value) in values.iteritems():
-        floating_ip_ref[key] = value
+    floating_ip_ref.update(values)
     floating_ip_ref.save()
     return floating_ip_ref['address']
 
@@ -451,8 +448,7 @@
 @require_context
 def fixed_ip_create(_context, values):
     fixed_ip_ref = models.FixedIp()
-    for (key, value) in values.iteritems():
-        fixed_ip_ref[key] = value
+    fixed_ip_ref.update(values)
     fixed_ip_ref.save()
     return fixed_ip_ref['address']
 
@@ -523,8 +519,7 @@
         fixed_ip_ref = fixed_ip_get_by_address(context,
                                                address,
                                                session=session)
-        for (key, value) in values.iteritems():
-            fixed_ip_ref[key] = value
+        fixed_ip_ref.update(values)
         fixed_ip_ref.save(session=session)
 
 
@@ -537,8 +532,7 @@
 @require_context
 def instance_create(context, values):
     instance_ref = models.Instance()
-    for (key, value) in values.iteritems():
-        instance_ref[key] = value
+    instance_ref.update(values)
 
     session = get_session()
     with session.begin():
@@ -731,8 +725,7 @@
     session = get_session()
     with session.begin():
         instance_ref = instance_get(context, instance_id, session=session)
-        for (key, value) in values.iteritems():
-            instance_ref[key] = value
+        instance_ref.update(values)
         instance_ref.save(session=session)
 
 
@@ -754,8 +747,7 @@
 @require_context
 def key_pair_create(context, values):
     key_pair_ref = models.KeyPair()
-    for (key, value) in values.iteritems():
-        key_pair_ref[key] = value
+    key_pair_ref.update(values)
     key_pair_ref.save()
     return key_pair_ref
 
@@ -870,8 +862,7 @@
 @require_admin_context
 def network_create_safe(context, values):
     network_ref = models.Network()
-    for (key, value) in values.iteritems():
-        network_ref[key] = value
+    network_ref.update(values)
     try:
         network_ref.save()
         return network_ref
@@ -980,8 +971,7 @@
     session = get_session()
     with session.begin():
         network_ref = network_get(context, network_id, session=session)
-        for (key, value) in values.iteritems():
-            network_ref[key] = value
+        network_ref.update(values)
         network_ref.save(session=session)
 
 
@@ -1031,8 +1021,7 @@
 @require_admin_context
 def export_device_create_safe(context, values):
     export_device_ref = models.ExportDevice()
-    for (key, value) in values.iteritems():
-        export_device_ref[key] = value
+    export_device_ref.update(values)
     try:
         export_device_ref.save()
         return export_device_ref
@@ -1060,8 +1049,7 @@
 
 def auth_create_token(_context, token):
     tk = models.AuthToken()
-    for k, v in token.iteritems():
-        tk[k] = v
+    tk.update(token)
     tk.save()
     return tk
 
@@ -1087,8 +1075,7 @@
 @require_admin_context
 def quota_create(context, values):
     quota_ref = models.Quota()
-    for (key, value) in values.iteritems():
-        quota_ref[key] = value
+    quota_ref.update(values)
     quota_ref.save()
     return quota_ref
 
@@ -1098,8 +1085,7 @@
     session = get_session()
     with session.begin():
         quota_ref = quota_get(context, project_id, session=session)
-        for (key, value) in values.iteritems():
-            quota_ref[key] = value
+        quota_ref.update(values)
         quota_ref.save(session=session)
 
 
@@ -1148,8 +1134,7 @@
 @require_context
 def volume_create(context, values):
     volume_ref = models.Volume()
-    for (key, value) in values.iteritems():
-        volume_ref[key] = value
+    volume_ref.update(values)
 
     session = get_session()
     with session.begin():
@@ -1306,8 +1291,7 @@
     session = get_session()
     with session.begin():
         volume_ref = volume_get(context, volume_id, session=session)
-        for (key, value) in values.iteritems():
-            volume_ref[key] = value
+        volume_ref.update(values)
         volume_ref.save(session=session)
 
 
@@ -1400,8 +1384,7 @@
     # FIXME(devcamcar): Unless I do this, rules fails with lazy load exception
     # once save() is called.  This will get cleaned up in next orm pass.
     security_group_ref.rules
-    for (key, value) in values.iteritems():
-        security_group_ref[key] = value
+    security_group_ref.update(values)
     security_group_ref.save()
     return security_group_ref
 
@@ -1455,8 +1438,7 @@
 @require_context
 def security_group_rule_create(context, values):
     security_group_rule_ref = models.SecurityGroupIngressRule()
-    for (key, value) in values.iteritems():
-        security_group_rule_ref[key] = value
+    security_group_rule_ref.update(values)
     security_group_rule_ref.save()
     return security_group_rule_ref
 
@@ -1508,8 +1490,7 @@
 @require_admin_context
 def user_create(_context, values):
     user_ref = models.User()
-    for (key, value) in values.iteritems():
-        user_ref[key] = value
+    user_ref.update(values)
     user_ref.save()
     return user_ref
 
@@ -1537,8 +1518,7 @@
 
 def project_create(_context, values):
     project_ref = models.Project()
-    for (key, value) in values.iteritems():
-        project_ref[key] = value
+    project_ref.update(values)
     project_ref.save()
     return project_ref
 
@@ -1600,8 +1580,7 @@
     session = get_session()
     with session.begin():
         user_ref = user_get(context, user_id, session=session)
-        for (key, value) in values.iteritems():
-            user_ref[key] = value
+        user_ref.update(values)
         user_ref.save(session=session)
 
 
@@ -1609,8 +1588,7 @@
     session = get_session()
     with session.begin():
         project_ref = project_get(context, project_id, session=session)
-        for (key, value) in values.iteritems():
-            project_ref[key] = value
+        project_ref.update(values)
         project_ref.save(session=session)
 
 

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2010-10-25 06:26:03 +0000
+++ nova/db/sqlalchemy/models.py	2010-10-28 15:46:05 +0000
@@ -82,6 +82,16 @@
         n = self._i.next().name
         return n, getattr(self, n)
 
+    def update(self, values):
+        """Make the model object behave like a dict"""
+        for k, v in values.iteritems():
+            setattr(self, k, v)
+
+    def iteritems(self):
+        """Make the model object behave like a dict"""
+        return iter(self)
+
+
 # TODO(vish): Store images in the database instead of file system
 #class Image(BASE, NovaBase):
 #    """Represents an image in the datastore"""

=== modified file 'nova/tests/api/openstack/fakes.py'
--- nova/tests/api/openstack/fakes.py	2010-10-22 07:48:27 +0000
+++ nova/tests/api/openstack/fakes.py	2010-10-28 15:46:05 +0000
@@ -30,6 +30,7 @@
 import nova.api.openstack.auth
 from nova.image import service
 from nova.image.services import glance
+from nova.tests import fake_flags
 from nova.wsgi import Router
 
 

=== modified file 'nova/tests/api/openstack/test_servers.py'
--- nova/tests/api/openstack/test_servers.py	2010-10-22 07:48:27 +0000
+++ nova/tests/api/openstack/test_servers.py	2010-10-28 15:46:05 +0000
@@ -91,9 +91,7 @@
             pass
 
         def instance_create(context, inst):
-            class Foo(object):
-                internal_id = 1
-            return Foo()
+            return {'id': 1, 'internal_id': 1}
 
         def fake_method(*args, **kwargs):
             pass