Merge lp:~openstack-gd/nova/nova-avail-zones into lp:~hudson-openstack/nova/trunk

[Eldar Nugaev]

Added support of availability zones for compute.
models.Service got additional field availability_zone and was created ZoneScheduler that make decisions based on this field.
Also replaced fake 'nova' zone in EC2 cloud api.

[Vish (vish)]

not me.. you've got the wrong guy.. ;)

review: Abstain

[Vish Ishaya (vishvananda)]

This is looking pretty good. It appears that volumes are not handled. Did you plan on doing those in another patch? I'm also wondering if this could actually be a subclass of Simple Scheduler, so we at least get the added functionality of going to the least loaded host. Another possibility is just to ignore the zone if it is None, and add it to simple scheduler.

review: Needs Information

[Eldar Nugaev (reldan)]

> This is looking pretty good. It appears that volumes are not handled. Did
> you plan on doing those in another patch? I'm also wondering if this could
> actually be a subclass of Simple Scheduler, so we at least get the added
> functionality of going to the least loaded host. Another possibility is just
> to ignore the zone if it is None, and add it to simple scheduler.

Thank you for the review. We didn't handle block storage device yet. It's easiest path that allows to run compute node on different physical hosts. If this path is Ok, we think about add availability zones for volumes.

Should we add zone handling to Simple Scheduler in this path or we can add it in volume zones path?

[Vish Ishaya (vishvananda)]

I'd like to see this as an addition to simple scheduler, but you can put it in when you do volume availability zones.

review: Approve

[Eldar Nugaev (reldan)]

I have a problem. I should merge my changes with trunk. But I can't do this right way. I suppose I see partial commit http://bazaar.launchpad.net/~hudson-openstack/nova/trunk/revision/472.2.1 .

api/ec2/cloud.py

 def _describe_availability_zones_verbose(self, context, **kwargs):
        rv = {'availabilityZoneInfo': [{'zoneName': 'nova',
                                        'zoneState': 'available'}]}

        services = db.service_get_all(context)
        now = db.get_time()

But db doesn't have service_get_all(context) and get_time() methods.

[Soren Hansen (soren)]

Good catch. I've reported https://bugs.launchpad.net/nova/+bug/700974. Thanks.

[Ilya Alekseyev (ilyaalekseyev)]

Looks good.

review: Approve

[Ilya Alekseyev (ilyaalekseyev)]

> Good catch. I've reported https://bugs.launchpad.net/nova/+bug/700974. Thanks.
Fine. We take changes and merged it to our branch. Could you please review our code again?

[Devin Carlen (devcamcar)]

approve

review: Approve

[OpenStack Infra (hudson-openstack)]

The attempt to merge lp:~openstack-gd/nova/nova-avail-zones into lp:nova failed. Below is the output from the failed tests.

nova/service.py:121:80: E501 line too long (92 characters)
                                         'availability_zone': FLAGS.node_availability_zone})
                                                                               ^
    Limit all lines to a maximum of 79 characters.

    There are still many devices around that are limited to 80 character
    lines; plus, limiting windows to 80 characters makes it possible to have
    several windows side-by-side. The default wrapping on such devices looks
    ugly. Therefore, please limit all lines to a maximum of 79 characters.
    For flowing long blocks of text (docstrings or comments), limiting the
    length to 72 characters is recommended.
nova/api/ec2/cloud.py:163:80: E501 line too long (85 characters)
                                                                instance_ref['host'])
                                                                               ^
    Limit all lines to a maximum of 79 characters.

    There are still many devices around that are limited to 80 character
    lines; plus, limiting windows to 80 characters makes it possible to have
    several windows side-by-side. The default wrapping on such devices looks
    ugly. Therefore, please limit all lines to a maximum of 79 characters.
    For flowing long blocks of text (docstrings or comments), limiting the
    length to 72 characters is recommended.
nova/api/ec2/cloud.py:213:80: E501 line too long (80 characters)
        for zone in [service.availability_zone for service in enabled_services]:
                                                                               ^
    Limit all lines to a maximum of 79 characters.

    There are still many devices around that are limited to 80 character
    lines; plus, limiting windows to 80 characters makes it possible to have
    several windows side-by-side. The default wrapping on such devices looks
    ugly. Therefore, please limit all lines to a maximum of 79 characters.
    For flowing long blocks of text (docstrings or comments), limiting the
    length to 72 characters is recommended.
nova/api/ec2/cloud.py:218:80: E501 line too long (87 characters)
                              and not service['availability_zone'] in available_zones]:
                                                                               ^
    Limit all lines to a maximum of 79 characters.

    There are still many devices around that are limited to 80 character
    lines; plus, limiting windows to 80 characters makes it possible to have
    several windows side-by-side. The default wrapping on such devices looks
    ugly. Therefore, please limit all lines to a maximum of 79 characters.
    For flowing long blocks of text (docstrings or comments), limiting the
    length to 72 characters is recommended.
nova/api/ec2/cloud.py:682:80: E501 line too long (94 characters)
            availability_zone = self._get_availability_zone_by_host(context, instance['host'])
                                                                               ^
    Limi...

[Thierry Carrez (ttx)]

@Ilya: You need to sign CLA and add your reference to http://wiki.openstack.org/Contributors... See http://wiki.openstack.org/HowToContribute for details

Hudson/Tarmac should reject the change, if my CLA verifier plugin works...

[Ilya Alekseyev (ilyaalekseyev)]

> @Ilya: You need to sign CLA and add your reference to
> http://wiki.openstack.org/Contributors... See
> http://wiki.openstack.org/HowToContribute for details
>
> Hudson/Tarmac should reject the change, if my CLA verifier plugin works...
Fixed. Sorry, forgot to add myself to wiki page.

[lp:~openstack-gd/nova/nova-avail-zones]

490. By Eldar Nugaev on 2011-01-11

remove extra whitspaces

=== modified file 'Authors'
--- Authors	2011-01-10 17:37:06 +0000
+++ Authors	2011-01-11 23:22:10 +0000
@@ -15,6 +15,7 @@
 Eric Day <[e-mail]>
 Ewan Mellor <[e-mail]>
 Hisaki Ohara <[e-mail]>
+Ilya Alekseyev <[e-mail]>
 Jay Pipes <[e-mail]>
 Jesse Andrews <[e-mail]>
 Joe Heck <[e-mail]>

=== modified file 'nova/api/ec2/cloud.py'
--- nova/api/ec2/cloud.py	2011-01-10 20:29:42 +0000
+++ nova/api/ec2/cloud.py	2011-01-11 23:22:10 +0000
@@ -132,6 +132,21 @@
                     result[key] = [line]
         return result
 
+    def _trigger_refresh_security_group(self, context, security_group):
+        nodes = set([instance['host'] for instance in security_group.instances
+                       if instance['host'] is not None])
+        for node in nodes:
+            rpc.cast(context,
+                     '%s.%s' % (FLAGS.compute_topic, node),
+                     {"method": "refresh_security_group",
+                      "args": {"security_group_id": security_group.id}})
+
+    def _get_availability_zone_by_host(self, context, host):
+        services = db.service_get_all_by_host(context, host)
+        if len(services) > 0:
+            return services[0]['availability_zone']
+        return 'unknown zone'
+
     def get_metadata(self, address):
         ctxt = context.get_admin_context()
         instance_ref = self.compute_api.get_all(ctxt, fixed_ip=address)
@@ -144,6 +159,8 @@
         else:
             keys = ''
         hostname = instance_ref['hostname']
+        host = instance_ref['host']
+        availability_zone = self._get_availability_zone_by_host(ctxt, host)
         floating_ip = db.instance_get_floating_address(ctxt,
                                                        instance_ref['id'])
         ec2_id = id_to_ec2_id(instance_ref['id'])
@@ -166,8 +183,7 @@
                 'local-hostname': hostname,
                 'local-ipv4': address,
                 'kernel-id': instance_ref['kernel_id'],
-                # TODO(vish): real zone
-                'placement': {'availability-zone': 'nova'},
+                'placement': {'availability-zone': availability_zone},
                 'public-hostname': hostname,
                 'public-ipv4': floating_ip or '',
                 'public-keys': keys,
@@ -191,8 +207,26 @@
             return self._describe_availability_zones(context, **kwargs)
 
     def _describe_availability_zones(self, context, **kwargs):
-        return {'availabilityZoneInfo': [{'zoneName': 'nova',
-                                          'zoneState': 'available'}]}
+        enabled_services = db.service_get_all(context)
+        disabled_services = db.service_get_all(context, True)
+        available_zones = []
+        for zone in [service.availability_zone for service
+                     in enabled_services]:
+            if not zone in available_zones:
+                available_zones.append(zone)
+        not_available_zones = []
+        for zone in [service.availability_zone for service in disabled_services
+                     if not service['availability_zone'] in available_zones]:
+            if not zone in not_available_zones:
+                not_available_zones.append(zone)
+        result = []
+        for zone in available_zones:
+            result.append({'zoneName': zone,
+                           'zoneState': "available"})
+        for zone in not_available_zones:
+            result.append({'zoneName': zone,
+                           'zoneState': "not available"})
+        return {'availabilityZoneInfo': result}
 
     def _describe_availability_zones_verbose(self, context, **kwargs):
         rv = {'availabilityZoneInfo': [{'zoneName': 'nova',
@@ -646,6 +680,9 @@
             i['amiLaunchIndex'] = instance['launch_index']
             i['displayName'] = instance['display_name']
             i['displayDescription'] = instance['display_description']
+            host = instance['host']
+            zone = self._get_availability_zone_by_host(context, host)
+            i['placement'] = {'availabilityZone': zone}
             if instance['reservation_id'] not in reservations:
                 r = {}
                 r['reservationId'] = instance['reservation_id']

=== modified file 'nova/compute/api.py'
--- nova/compute/api.py	2011-01-11 06:47:35 +0000
+++ nova/compute/api.py	2011-01-11 23:22:10 +0000
@@ -186,7 +186,8 @@
                      FLAGS.scheduler_topic,
                      {"method": "run_instance",
                       "args": {"topic": FLAGS.compute_topic,
-                               "instance_id": instance_id}})
+                               "instance_id": instance_id,
+                               "availability_zone": availability_zone}})
 
         for group_id in security_groups:
             self.trigger_security_group_members_refresh(elevated, group_id)

=== modified file 'nova/db/api.py'
--- nova/db/api.py	2011-01-11 12:24:58 +0000
+++ nova/db/api.py	2011-01-11 23:22:10 +0000
@@ -81,16 +81,21 @@
     return IMPL.service_get(context, service_id)
 
 
-def service_get_all(context):
-    """Get a list of all services on any machine on any topic of any type"""
-    return IMPL.service_get_all(context)
+def service_get_all(context, disabled=False):
+    """Get all service."""
+    return IMPL.service_get_all(context, None, disabled)
 
 
 def service_get_all_by_topic(context, topic):
-    """Get all compute services for a given topic."""
+    """Get all services for a given topic."""
     return IMPL.service_get_all_by_topic(context, topic)
 
 
+def service_get_all_by_host(context, host):
+    """Get all services for a given host."""
+    return IMPL.service_get_all_by_host(context, host)
+
+
 def service_get_all_compute_sorted(context):
     """Get all compute services sorted by instance count.
 

=== modified file 'nova/db/sqlalchemy/api.py'
--- nova/db/sqlalchemy/api.py	2011-01-11 12:24:58 +0000
+++ nova/db/sqlalchemy/api.py	2011-01-11 23:22:10 +0000
@@ -135,14 +135,14 @@
 
 
 @require_admin_context
-def service_get_all(context, session=None):
+def service_get_all(context, session=None, disabled=False):
     if not session:
         session = get_session()
 
     result = session.query(models.Service).\
-                     filter_by(deleted=can_read_deleted(context)).\
-                     all()
-
+                   filter_by(deleted=can_read_deleted(context)).\
+                   filter_by(disabled=disabled).\
+                   all()
     return result
 
 
@@ -157,6 +157,15 @@
 
 
 @require_admin_context
+def service_get_all_by_host(context, host):
+    session = get_session()
+    return session.query(models.Service).\
+                   filter_by(deleted=False).\
+                   filter_by(host=host).\
+                   all()
+
+
+@require_admin_context
 def _service_get_all_topic_subquery(context, session, topic, subq, label):
     sort_value = getattr(subq.c, label)
     return session.query(models.Service, func.coalesce(sort_value, 0)).\

=== modified file 'nova/db/sqlalchemy/models.py'
--- nova/db/sqlalchemy/models.py	2011-01-10 17:37:06 +0000
+++ nova/db/sqlalchemy/models.py	2011-01-11 23:22:10 +0000
@@ -149,6 +149,7 @@
     topic = Column(String(255))
     report_count = Column(Integer, nullable=False, default=0)
     disabled = Column(Boolean, default=False)
+    availability_zone = Column(String(255), default='nova')
 
 
 class Certificate(BASE, NovaBase):

=== modified file 'nova/flags.py'
--- nova/flags.py	2011-01-11 06:47:35 +0000
+++ nova/flags.py	2011-01-11 23:22:10 +0000
@@ -301,6 +301,5 @@
 DEFINE_string('host', socket.gethostname(),
               'name of this node')
 
-# UNUSED
 DEFINE_string('node_availability_zone', 'nova',
               'availability zone of this node')

=== added file 'nova/scheduler/zone.py'
--- nova/scheduler/zone.py	1970-01-01 00:00:00 +0000
+++ nova/scheduler/zone.py	2011-01-11 23:22:10 +0000
@@ -0,0 +1,56 @@
+# vim: tabstop=4 shiftwidth=4 softtabstop=4
+
+# Copyright (c) 2010 Openstack, LLC.
+# Copyright 2010 United States Government as represented by the
+# Administrator of the National Aeronautics and Space Administration.
+# 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.
+
+"""
+Availability Zone Scheduler implementation
+"""
+
+import random
+
+from nova.scheduler import driver
+from nova import db
+
+
+class ZoneScheduler(driver.Scheduler):
+    """Implements Scheduler as a random node selector."""
+
+    def hosts_up_with_zone(self, context, topic, zone):
+        """Return the list of hosts that have a running service
+        for topic and availability zone (if defined).
+        """
+
+        if zone is None:
+            return self.hosts_up(context, topic)
+
+        services = db.service_get_all_by_topic(context, topic)
+        return [service.host
+                for service in services
+                if self.service_is_up(service)
+                and service.availability_zone == zone]
+
+    def schedule(self, context, topic, *_args, **_kwargs):
+        """Picks a host that is up at random in selected
+        availability zone (if defined).
+        """
+
+        zone = _kwargs.get('availability_zone')
+        hosts = self.hosts_up_with_zone(context, topic, zone)
+        if not hosts:
+            raise driver.NoValidHost(_("No hosts found"))
+        return hosts[int(random.random() * len(hosts))]

=== modified file 'nova/service.py'
--- nova/service.py	2011-01-09 00:39:12 +0000
+++ nova/service.py	2011-01-11 23:22:10 +0000
@@ -113,11 +113,13 @@
             self.timers.append(periodic)
 
     def _create_service_ref(self, context):
+        zone = FLAGS.node_availability_zone
         service_ref = db.service_create(context,
                                         {'host': self.host,
                                          'binary': self.binary,
                                          'topic': self.topic,
-                                         'report_count': 0})
+                                         'report_count': 0,
+                                         'availability_zone': zone})
         self.service_id = service_ref['id']
 
     def __getattr__(self, key):

=== modified file 'nova/tests/test_cloud.py'
--- nova/tests/test_cloud.py	2011-01-10 07:01:10 +0000
+++ nova/tests/test_cloud.py	2011-01-11 23:22:10 +0000
@@ -133,10 +133,35 @@
         db.volume_destroy(self.context, vol1['id'])
         db.volume_destroy(self.context, vol2['id'])
 
+    def test_describe_availability_zones(self):
+        """Makes sure describe_availability_zones works and filters results."""
+        service1 = db.service_create(self.context, {'host': 'host1_zones',
+                                         'binary': "nova-compute",
+                                         'topic': 'compute',
+                                         'report_count': 0,
+                                         'availability_zone': "zone1"})
+        service2 = db.service_create(self.context, {'host': 'host2_zones',
+                                         'binary': "nova-compute",
+                                         'topic': 'compute',
+                                         'report_count': 0,
+                                         'availability_zone': "zone2"})
+        result = self.cloud.describe_availability_zones(self.context)
+        self.assertEqual(len(result['availabilityZoneInfo']), 3)
+        db.service_destroy(self.context, service1['id'])
+        db.service_destroy(self.context, service2['id'])
+
     def test_describe_instances(self):
         """Makes sure describe_instances works and filters results."""
-        inst1 = db.instance_create(self.context, {'reservation_id': 'a'})
-        inst2 = db.instance_create(self.context, {'reservation_id': 'a'})
+        inst1 = db.instance_create(self.context, {'reservation_id': 'a',
+                                                  'host': 'host1'})
+        inst2 = db.instance_create(self.context, {'reservation_id': 'a',
+                                                  'host': 'host2'})
+        comp1 = db.service_create(self.context, {'host': 'host1',
+                                                 'availability_zone': 'zone1',
+                                                 'topic': "compute"})
+        comp2 = db.service_create(self.context, {'host': 'host2',
+                                                 'availability_zone': 'zone2',
+                                                 'topic': "compute"})
         result = self.cloud.describe_instances(self.context)
         result = result['reservationSet'][0]
         self.assertEqual(len(result['instancesSet']), 2)
@@ -147,8 +172,12 @@
         self.assertEqual(len(result['instancesSet']), 1)
         self.assertEqual(result['instancesSet'][0]['instanceId'],
                          instance_id)
+        self.assertEqual(result['instancesSet'][0]
+                         ['placement']['availabilityZone'], 'zone2')
         db.instance_destroy(self.context, inst1['id'])
         db.instance_destroy(self.context, inst2['id'])
+        db.service_destroy(self.context, comp1['id'])
+        db.service_destroy(self.context, comp2['id'])
 
     def test_console_output(self):
         image_id = FLAGS.default_image
@@ -228,6 +257,19 @@
                 LOG.debug(_("Terminating instance %s"), instance_id)
                 rv = self.compute.terminate_instance(instance_id)
 
+    def test_describe_instances(self):
+        """Makes sure describe_instances works."""
+        instance1 = db.instance_create(self.context, {'host': 'host2'})
+        comp1 = db.service_create(self.context, {'host': 'host2',
+                                                 'availability_zone': 'zone1',
+                                                 'topic': "compute"})
+        result = self.cloud.describe_instances(self.context)
+        self.assertEqual(result['reservationSet'][0]
+                         ['instancesSet'][0]
+                         ['placement']['availabilityZone'], 'zone1')
+        db.instance_destroy(self.context, instance1['id'])
+        db.service_destroy(self.context, comp1['id'])
+
     def test_instance_update_state(self):
         def instance(num):
             return {

=== modified file 'nova/tests/test_scheduler.py'
--- nova/tests/test_scheduler.py	2011-01-03 19:29:39 +0000
+++ nova/tests/test_scheduler.py	2011-01-11 23:22:10 +0000
@@ -21,6 +21,7 @@
 
 import datetime
 
+from mox import IgnoreArg
 from nova import context
 from nova import db
 from nova import flags
@@ -76,6 +77,59 @@
         scheduler.named_method(ctxt, 'topic', num=7)
 
 
+class ZoneSchedulerTestCase(test.TestCase):
+    """Test case for zone scheduler"""
+    def setUp(self):
+        super(ZoneSchedulerTestCase, self).setUp()
+        self.flags(scheduler_driver='nova.scheduler.zone.ZoneScheduler')
+
+    def _create_service_model(self, **kwargs):
+        service = db.sqlalchemy.models.Service()
+        service.host = kwargs['host']
+        service.disabled = False
+        service.deleted = False
+        service.report_count = 0
+        service.binary = 'nova-compute'
+        service.topic = 'compute'
+        service.id = kwargs['id']
+        service.availability_zone = kwargs['zone']
+        service.created_at = datetime.datetime.utcnow()
+        return service
+
+    def test_with_two_zones(self):
+        scheduler = manager.SchedulerManager()
+        ctxt = context.get_admin_context()
+        service_list = [self._create_service_model(id=1,
+                                                   host='host1',
+                                                   zone='zone1'),
+                        self._create_service_model(id=2,
+                                                   host='host2',
+                                                   zone='zone2'),
+                        self._create_service_model(id=3,
+                                                   host='host3',
+                                                   zone='zone2'),
+                        self._create_service_model(id=4,
+                                                   host='host4',
+                                                   zone='zone2'),
+                        self._create_service_model(id=5,
+                                                   host='host5',
+                                                   zone='zone2')]
+        self.mox.StubOutWithMock(db, 'service_get_all_by_topic')
+        arg = IgnoreArg()
+        db.service_get_all_by_topic(arg, arg).AndReturn(service_list)
+        self.mox.StubOutWithMock(rpc, 'cast', use_mock_anything=True)
+        rpc.cast(ctxt,
+                 'compute.host1',
+                 {'method': 'run_instance',
+                  'args': {'instance_id': 'i-ffffffff',
+                           'availability_zone': 'zone1'}})
+        self.mox.ReplayAll()
+        scheduler.run_instance(ctxt,
+                               'compute',
+                               instance_id='i-ffffffff',
+                               availability_zone='zone1')
+
+
 class SimpleDriverTestCase(test.TestCase):
     """Test case for simple driver"""
     def setUp(self):

=== modified file 'nova/tests/test_service.py'
--- nova/tests/test_service.py	2011-01-03 19:29:39 +0000
+++ nova/tests/test_service.py	2011-01-11 23:22:10 +0000
@@ -133,7 +133,8 @@
         service_create = {'host': host,
                           'binary': binary,
                           'topic': topic,
-                          'report_count': 0}
+                          'report_count': 0,
+                          'availability_zone': 'nova'}
         service_ref = {'host': host,
                        'binary': binary,
                        'report_count': 0,
@@ -161,11 +162,13 @@
         service_create = {'host': host,
                           'binary': binary,
                           'topic': topic,
-                          'report_count': 0}
+                          'report_count': 0,
+                          'availability_zone': 'nova'}
         service_ref = {'host': host,
                           'binary': binary,
                           'topic': topic,
                           'report_count': 0,
+                          'availability_zone': 'nova',
                           'id': 1}
 
         service.db.service_get_by_args(mox.IgnoreArg(),
@@ -193,11 +196,13 @@
         service_create = {'host': host,
                           'binary': binary,
                           'topic': topic,
-                          'report_count': 0}
+                          'report_count': 0,
+                          'availability_zone': 'nova'}
         service_ref = {'host': host,
                           'binary': binary,
                           'topic': topic,
                           'report_count': 0,
+                          'availability_zone': 'nova',
                           'id': 1}
 
         service.db.service_get_by_args(mox.IgnoreArg(),
@@ -224,11 +229,13 @@
         service_create = {'host': host,
                           'binary': binary,
                           'topic': topic,
-                          'report_count': 0}
+                          'report_count': 0,
+                          'availability_zone': 'nova'}
         service_ref = {'host': host,
                           'binary': binary,
                           'topic': topic,
                           'report_count': 0,
+                          'availability_zone': 'nova',
                           'id': 1}
 
         service.db.service_get_by_args(mox.IgnoreArg(),