Merge lp://qastaging/~usc-isi/nova/extra_specs_sched into lp://qastaging/~hudson-openstack/nova/trunk

nova/tests/api/openstack/extensions/test_flavors_extra_specs.py

This file seems to have been deleted and re-added and as such it makes it difficult to discern any changes that might have been made to it. Can you see if you can fix this?

> nova/tests/api/openstack/extensions/test_flavors_extra_specs.py
>
> This file seems to have been deleted and re-added and as such it makes it
> difficult to discern any changes that might have been made to it. Can you see
> if you can fix this?

That's exactly what happened. I haven't figured out how to get bazaar to identify that the files are the same. They're actually identical, no changes have been made to that file.

> nova/tests/api/openstack/extensions/test_flavors_extra_specs.py
>
> This file seems to have been deleted and re-added and as such it makes it
> difficult to discern any changes that might have been made to it. Can you see
> if you can fix this?

Figured out how to fix this, should be good now.

The code as is looks good to me, but I'm a bit curious about the approach here.

What I mean is that this seems like a very important feature if you want to run heterogeneous environments. In fact, I think it's such an important feature that it is almost beyond the scope of a single scheduler.

Have we considered the fact that every scheduler might actually need to behave this way?

Perhaps an alternate approach would be to somehow define (a flag?) to indicate that the zone itself is heterogeneous.

What we have in this code is a simple random scheduler for a heterogeneous environment, and while that is progress, I think all schedulers should be made aware of heterogeneous environments.

> The code as is looks good to me, but I'm a bit curious about the approach
> here.
>
> What I mean is that this seems like a very important feature if you want to
> run heterogeneous environments. In fact, I think it's such an important
> feature that it is almost beyond the scope of a single scheduler.
>
> Have we considered the fact that every scheduler might actually need to behave
> this way?
>
> Perhaps an alternate approach would be to somehow define (a flag?) to indicate
> that the zone itself is heterogeneous.
>
> What we have in this code is a simple random scheduler for a heterogeneous
> environment, and while that is progress, I think all schedulers should be made
> aware of heterogeneous environments.

The real work to support heterogeneity was implemented by changes to the InstanceTypeFilter. That functionality was added in a previous merge proposal <https://code.launchpad.net/~usc-isi/nova/instance_type_extra_specs/+merge/62728>.

Any scheduler that respects capabilities will automatically support heterogeneity. If the architecture request is conveyed through instance types, then any scheduler using the existing InstanceTypeFilter filter will have support for heterogeneity. The intent of the scheduler implementation here is just to show an example of how the filter can be used to support the heterogeneity.

It's not obvious to me how to support this in a more generic way. Alternatively, we could change the default filter in the zone_aware_scheduler to use the InstanceTypeFilter. Clients could opt not to use it by subclassing the filter and overriding the behavior. Would that be better?

> Any scheduler that respects capabilities will automatically support
> heterogeneity. If the architecture request is conveyed through instance types,
> then any scheduler using the existing InstanceTypeFilter filter will have
> support for heterogeneity. The intent of the scheduler implementation here is
> just to show an example of how the filter can be used to support the
> heterogeneity.
>
> It's not obvious to me how to support this in a more generic way.
> Alternatively, we could change the default filter in the zone_aware_scheduler
> to use the InstanceTypeFilter. Clients could opt not to use it by subclassing
> the filter and overriding the behavior. Would that be better?

+1

I like the idea of changing the default filter to use InstanceTypeFilter.

> > Any scheduler that respects capabilities will automatically support
> > heterogeneity. If the architecture request is conveyed through instance
> types,
> > then any scheduler using the existing InstanceTypeFilter filter will have
> > support for heterogeneity. The intent of the scheduler implementation here
> is
> > just to show an example of how the filter can be used to support the
> > heterogeneity.
> >
> > It's not obvious to me how to support this in a more generic way.
> > Alternatively, we could change the default filter in the
> zone_aware_scheduler
> > to use the InstanceTypeFilter. Clients could opt not to use it by
> subclassing
> > the filter and overriding the behavior. Would that be better?
>
> +1
>
> I like the idea of changing the default filter to use InstanceTypeFilter.

I'll go ahead and make that change.

Instead of changing the default behavior of the ZoneAwareScheduler as initially suggested (and encouraged by Devin), I changed the default behavior of the HostFilterScheduler.

Added Sandy Walsh to the list of requested reviewers to make him explicitly aware of this change in default behavior.

Whoops, didn't mean to add you twice, Sandy. Sorry about that.

Heh, no problem.

I don't have any issues with the default Host Filter changing from AllHosts to InstanceTypeFilter ... makes sense. I just hope we have enough capabilities coming back from the virt layers to do it correctly?

Thanks for adding get_host_stats to libvirt ... and I like the integration tests for run_instance().

Only thing I would change is revert

67 + filter_name = request_spec.get('filter',
68 + 'nova.scheduler.host_filter.InstanceTypeFilter')

back to the old way and change the default in

flags.DEFINE_string('default_host_filter',
                    'nova.scheduler.host_filter.AllHostsFilter',
                    'Which filter to use for filtering hosts.')

since choose_host_filter() will use this if no HostFilter is defined.

Otherwise, nice branch ... good job!

Hmm, we might need to re-issue the merge prop if that second request becomes an issue. Not necessary just yet though.

> Only thing I would change is revert
>
> 67 + filter_name = request_spec.get('filter',
> 68 +
> 'nova.scheduler.host_filter.InstanceTypeFilter')
>
> back to the old way and change the default in
>
> flags.DEFINE_string('default_host_filter',
> 'nova.scheduler.host_filter.AllHostsFilter',
> 'Which filter to use for filtering hosts.')
>
> since choose_host_filter() will use this if no HostFilter is defined.

Whoops, somehow I missed that flag definition. Code has been fixed.

perfect ... thanks Lorin!

Awesome, this looks great.

Looks great--love the added tests here.

Soooo sorry for my stale review here.

If HostState is a property, it should be host_state (it's like that in the Xen driver, but I've been meaning to file something for that.)

> #TODO(lorinh): Do the instance type extra specs check here

Is there a bug or blueprint for this? I don't think we have a standard, but it's nice to have documentation not in the code if someone else could implement this feature for you.

> # Copyright 2011 University of Southern California

I believe the Copyright header should be for OpenStack, LLC...it would make sense that contributed code would belong to the community.

Also, docstrings need to be a single line according to HACKING. If not a single line than a one-line summary, followed by a newline, followed by a summary, followed by a newline. Simple, right? haha

>
>> # Copyright 2011 University of Southern California
>
> I believe the Copyright header should be for OpenStack, LLC...it would make sense that contributed code would belong to the community.

Copyright assignment isn't required. The license allows the community to use the code regardless.

Vish