Merge lp://qastaging/~ed-leafe/nova/scheduler-multifilter into lp://qastaging/~hudson-openstack/nova/trunk

This should wait for Essex ?

I like the change from zone_manager to host_list. It provides a clearer method signature. Can't find anything wrong with it. Pinging Sandy to look at the branch

A nice semantic cleanup ... lgtm.

However, I get a stall on test_ajax_console when I merge this branch with trunk.

Otherwise, awaiting TTX's go ahead before merge.

Hmmm. A stall on that test, I'd think, would be unrelated.. as the default scheduler is Chance. It's possible that it's related to kombu hitting trunk... I also have some test_cloud cleanups in another branch that's not ready to merge yet. Sandy: might be interesting to merge in: lp:~cbehrens/nova/test-sleep-cleanup and see if that test_ajax_console issue goes away.

I'm about to pull this branch and look it over, also.

Yup, that fixed it up (although I do get http://paste.openstack.org/show/2344/, which I'm sure is equally unrelated)

As far as I can tell, it's a very welcome refactoring that should definitely go into code, but I'm a bit torn on whether it should go in Diablo.

On one hand, it has regression potential and does not really solve a bug.

On another hand, I suspect it's better to have the new filters design in the release, rather than have to wait another release to build on the new one.

It's a bit of a product strategy decision, so I'm happy to defer to Vish on that one -- that branch wasn't part of the "features left to merge" in his recent email.

We're doing a bunch on testing on zones as a whole this sprint (ending next week) and have already spotted a number of regressions. I suspect there'll be several bug fix patches following behind this shortly.

Since some of these merges have already made it in, zones in Diablo would be busted anyway. I'm just afraid of introducing regressions in other areas of the code.

It's missing the tenant_id-in-urls addition that went into nova recently; doing "nova list" gets:

2011-09-02 18:08:49,315 DEBUG routes.middleware [-] No route matched for GET /zone1/servers/detail from (pid=20794) __call__ /usr/lib/pymodules/python2.6/routes/middleware.py:97

(zone1 being the tenant I'm using)

There's a small bug with the host filter stuff, it appears. If you build an instance with no filter, I get this:

(nova.rpc): TRACE: Traceback (most recent call last):
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/rpc/impl_kombu.py", line 620, in _process_data
(nova.rpc): TRACE: rval = node_func(context=ctxt, **node_args)
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/scheduler/manager.py", line 97, in _schedule
(nova.rpc): TRACE: **kwargs)
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/scheduler/abstract_scheduler.py", line 224, in schedule_run_instance
(nova.rpc): TRACE: build_plan = self.select(context, request_spec)
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/scheduler/abstract_scheduler.py", line 246, in select
(nova.rpc): TRACE: *args, **kwargs)
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/scheduler/abstract_scheduler.py", line 274, in _schedule
(nova.rpc): TRACE: unfiltered_hosts)
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/scheduler/base_scheduler.py", line 49, in filter_hosts
(nova.rpc): TRACE: selected_filters = host_filter.choose_host_filters(filters)
(nova.rpc): TRACE: File "/home/cbehrens/openstack/nova/scheduler-multifilter/nova/scheduler/host_filter.py", line 78, in choose_host_filters
(nova.rpc): TRACE: msg = ", ".join(bad_filters)
(nova.rpc): TRACE: TypeError: sequence item 0: expected string, NoneType found
(nova.rpc): TRACE:

Problem appears to be this:

 33 class BaseScheduler(abstract_scheduler.AbstractScheduler):
 34 """Base class for creating Schedulers that can work across any nova
 35 deployment, from simple designs to multiply-nested zones.
 36 """
 37 def filter_hosts(self, topic, request_spec, hosts=None):
 38 """Filter the full host list (from the ZoneManager)"""
 39 filters = request_spec.get('filter', FLAGS.default_host_filters)
 40 if not isinstance(filters, (list, tuple)):
 41 filters = [filters]

request_spec has {'filter': None}, so filters = None on line 39

line 40 turns it into a list: [None]

The checks in host_filter's choose_host_filter() are like so:

 62 if not filters:
 63 filters = FLAGS.default_host_filters
 64 if not isinstance(filters, (list, tuple)):
 65 filters = [filters]

line 62 ends up being False.. so we don't try using the default_host_filters... and we try to iterate through [None] later.

Hm. I might not be totally correct about the above, since line 39 should return FLAGS.default_host_filters. Not sure what's going on, now... except I know choose_host_filter is being called with filters = [None]

Shoot.. I confused myself for a second. Right, request_spec has {'filters': None} as I said initially. Doh.

There should probably be a little more validation, but this should work:

=== modified file 'nova/scheduler/base_scheduler.py'
--- nova/scheduler/base_scheduler.py 2011-08-22 18:07:59 +0000
+++ nova/scheduler/base_scheduler.py 2011-09-02 18:34:10 +0000
@@ -36,7 +36,9 @@
     """
     def filter_hosts(self, topic, request_spec, hosts=None):
         """Filter the full host list (from the ZoneManager)"""
- filters = request_spec.get('filter', FLAGS.default_host_filters)
+ filters = request_spec.get('filter', None)
+ if not filters:
+ filters = FLAGS.default_host_filters
         if not isinstance(filters, (list, tuple)):
             filters = [filters]
         if hosts is None:

> It's missing the tenant_id-in-urls addition that went into nova recently;
> doing "nova list" gets:
>
> 2011-09-02 18:08:49,315 DEBUG routes.middleware [-] No route matched for GET
> /zone1/servers/detail from (pid=20794) __call__
> /usr/lib/pymodules/python2.6/routes/middleware.py:97
>
> (zone1 being the tenant I'm using)

That wouldn't be related to this branch, though.

I think you're looking for the one liner:

filters = request_spec.get('filter') or FLAGS.default_host_filters

unless you need to be able to pass an empty list to override default. In that case:

filters = request_spec.get('filter')
if filters is None:
    filters = FLAGS.default_host_filters

comstud: Just documenting the need for a trunk merge. BTW, I will be out of town this weekend, so feel free to ignore my "needs fixing" assuming it gets fixed...

Vish: I think we'd want an empty list to override the default...so yeah, your 2nd option is what we want. Thnx.

Kevin: Well, sure. He's proposing this merge into trunk. :) I assume when you test, you always merge into trunk to test it. If you're saying there's conflicts to resolve, then yeah, we'll need to get trunk merged into this and conflicts resolved.

Trunk merged, and the filter selection code updated. Have at it.

Essex is open !

On line 102—isn't that a relative import? We should probably be using only absolute imports…

On line 241 (unchanged from trunk)—do we still need the inner import here? I put the same import where you have the flags.DEFINE_string(), and everything worked fine; I think the circular dependency doesn't exist anymore.

Kevin: Not sure how much time Ed has for this. Perhaps we should go ahead and get this merged and fix imports quickly afterwards. I'm good with it how it is.

As long as we can get the imports fixed afterward, I'm fine with it.

likewise

The attempt to merge lp:~ed-leafe/nova/scheduler-multifilter into lp:nova failed. Below is the output from the failed tests.

CloudTestCase
    test_ajax_console OK 1.86
    test_allocate_address OK 0.26
    test_associate_disassociate_address OK 1.41
    test_authorize_revoke_security_group_ingress_by_id OK 0.36
    test_authorize_security_group_fail_missing_source_group OK 0.27
    test_authorize_security_group_ingress OK 0.27
    test_authorize_security_group_ingress_already_exists OK 0.48
    test_authorize_security_group_ingress_ip_permissions_groups OK 0.33
    test_authorize_security_group_ingress_ip_permissions_ip_rangesOK 0.27
    test_authorize_security_group_ingress_missing_group_name_or_idOK 1.42
    test_authorize_security_group_ingress_missing_protocol_paramsOK 0.25
    test_console_output OK 1.38
    test_create_delete_security_group OK 0.43
    test_create_image OK 4.10
    test_create_snapshot OK 0.46
    test_create_volume_from_snapshot OK 0.64
    test_delete_key_pair OK 0.39
    test_delete_security_group_by_id OK 0.25
    test_delete_security_group_no_params OK 0.20
    test_delete_security_group_with_bad_group_id OK 0.22
    test_delete_security_group_with_bad_name OK 0.22
    test_delete_snapshot OK 0.43
    test_deregister_image OK 0.20
    test_deregister_image_wrong_container_type OK 0.19
    test_describe_addresses OK 0.48
    test_describe_availability_zones OK 0.24
    test_describe_image_attribute OK 0.20
    test_describe_image_attribute_block_device_mapping OK 0.20
    test_describe_image_attribute_root_device_name OK 0.19
    test_describe_image_mapping OK 0.19
    test_describe_images OK 0.19
    test_describe_instance_attribute OK 0.19
    test_describe_instances OK 0.39
    test_describe_instances_bdm OK 1.45
    test_describe_instances_deleted OK 0.26
    test_describe_key_pairs OK 0.66
    test_describe_regions OK 0.21
    test_describe_security_groups OK 0.30
    test_describe_security_groups_by_id OK 0.36
    test_describe_snapshots OK 0.25
    test_describe_volumes OK 0.28
    test_format_instance_bdm ...