Code review comment for lp:~eday/nova/compute-abstraction

2010/11/24 Eric Day <email address hidden>:
> === modified file 'nova/api/ec2/cloud.py'
> --- nova/api/ec2/cloud.py       2010-11-18 21:27:52 +0000
> +++ nova/api/ec2/cloud.py       2010-11-24 22:55:33 +0000
> @@ -39,7 +39,7 @@
>  from nova import quota
>  from nova import rpc
>  from nova import utils
> -from nova.compute.instance_types import INSTANCE_TYPES
> +from nova.compute import instance_types

This needs to move out of code anyway. Filed bug #681411.

> @@ -260,7 +255,7 @@
>         return True
>
>     def describe_security_groups(self, context, group_name=None, **kwargs):
> -        self._ensure_default_security_group(context)
> +        self.compute_manager.ensure_default_security_group(context)
>         if context.user.is_admin():
>             groups = db.security_group_get_all(context)
>         else:

I understand the motivation to consolidate this code in the compute
manager. I just think that instances launched through the EC2 ÁPI should
land in one security group by default and instances launched through the
OpenStack API should land in another by default. EC2 restricts all
access to instances by default, while Rackspace has traditionally left
them open, leaving it to the owner of the instance to shield it off.

I filed bug #681416 to track this.

> @@ -505,9 +500,8 @@
>         if quota.allowed_volumes(context, 1, size) < 1:
>             logging.warn("Quota exceeeded for %s, tried to create %sG volume",
>                          context.project_id, size)
> -            raise QuotaError("Volume quota exceeded. You cannot "
> -                             "create a volume of size %s" %
> -                             size)
> +            raise quota.QuotaError("Volume quota exceeded. You cannot "
> +                                   "create a volume of size %s" % size)

We should include a unit, not just the number. Perhaps the user thinks
he's creating a 1000 MB volume, but we're actually blocking him from
creating a 1000 GB volume.

Filed bug #681417.

> === modified file 'nova/compute/manager.py'
> --- nova/compute/manager.py     2010-11-03 22:06:00 +0000
> +++ nova/compute/manager.py     2010-11-24 22:55:33 +0000
> @@ -36,13 +36,18 @@
>
>  import logging +import time
>
>  from twisted.internet import defer
>
> +from nova import db
>  from nova import exception
>  from nova import flags
>  from nova import manager
> +from nova import quota
> +from nova import rpc
>  from nova import utils
> +from nova.compute import instance_types
>  from nova.compute import power_state
>
>
> @@ -53,6 +58,11 @@
>                     'Driver to use for volume creation')
>
>
> +def generate_default_hostname(internal_id):
> +    """Default function to generate a hostname given an instance reference."""
> +    return str(internal_id)
> +
> +
>  class ComputeManager(manager.Manager):
>     """Manages the running instances from creation to destruction."""
>
> @@ -84,6 +94,126 @@
>         """This call passes stright through to the virtualization driver."""
>         yield self.driver.refresh_security_group(security_group_id)
>
> +    # TODO(eday): network_topic arg should go away once we push network
> +    # allocation into the scheduler or compute worker.
> +    def create_instances(self, context, instance_type, image_service, image_id,
> +                         network_topic, min_count=1, max_count=1,
> +                         kernel_id=None, ramdisk_id=None, name='',
> +                         description='', user_data='', key_name=None,
> +                         key_data=None, security_group='default',
> +                         generate_hostname=generate_default_hostname):
[...]
> +        return instances

I have to admit it seems a bit odd that the code to actually create the
instances (on the compute worker) and the code to call upon the computer
worker to run the instances are right next to each other.

Other than that, it looks great!

--
Soren Hansen
Ubuntu Developer    http://www.ubuntu.com/
OpenStack Developer http://www.openstack.org/