From 969c81f1d9ddb439c35f0389ff23156377d7014a Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Mon, 1 Feb 2016 15:46:51 +0000 Subject: [PATCH] Revert "Added new scheduler filter: AggregateTypeExtraSpecsAffinityFilter" This reverts commit 4b142e53e43132e996d35c335da30959f9f361be. I was having a point left unresolved in https://review.openstack.org/#/c/189279/8/doc/source/filter_scheduler.rst about the tech debt that adding a new filter in-tree would cause for something very closely related to another filter. I totally get the need for adding more logic and reverting how we compare flavors vs. aggregates. I just feel that before committing ourselves to that, we need to correctly estimate the possibilities to modify AggregateInstanceExtraSpecsFilter to fit the above needs. Change-Id: Ic9f70dae037d32980a5a252bdd08eff02ba27120 --- doc/source/filter_scheduler.rst | 19 -- nova/scheduler/filters/type_filter.py | 140 ------------- .../scheduler/filters/test_type_filters.py | 194 ------------------ ...pecs-affinity-filter-79a2d3ee152b8ecd.yaml | 10 - 4 files changed, 363 deletions(-) delete mode 100644 releasenotes/notes/add-aggregate-type-extra-specs-affinity-filter-79a2d3ee152b8ecd.yaml diff --git a/doc/source/filter_scheduler.rst b/doc/source/filter_scheduler.rst index b78b189df44d..d3fbe0bd0f3b 100644 --- a/doc/source/filter_scheduler.rst +++ b/doc/source/filter_scheduler.rst @@ -168,24 +168,6 @@ There are many standard filter classes which may be used the available metrics are passed. * |NUMATopologyFilter| - filters hosts based on the NUMA topology requested by the instance, if any. -* |AggregateTypeExtraSpecsAffinityFilter| - filters only hosts aggregated inside - host aggregates containing "flavor_extra_spec" metadata. Keys inside this - variable must match with the instance extra specs. - - The content of the list will be formatted as follows. The entries in the list - will be separated by commas without white space. Each entry will comprise of - an instance type extra spec key followed by and equals "=" followed by a - value: =. - - Eg. 'flavor_extra_spec=hw:mem_page_size=any,hw:mem_page_size=~,hw:mem_page_size=small' - - Valid sentinel values are: -:: - - * (asterisk): may be used to specify that any value is valid. - ~ (tilde): may be used to specify that a key may optionally be omitted. - ! (exclamation): may be used to specify that the key must not be present. -:: Now we can focus on these standard filter classes in some detail. We'll skip the simplest ones, such as |AllHostsFilter|, |CoreFilter| and |RamFilter|, @@ -461,7 +443,6 @@ in :mod:`nova.tests.scheduler`. .. |TrustedFilter| replace:: :class:`TrustedFilter ` .. |TypeAffinityFilter| replace:: :class:`TypeAffinityFilter ` .. |AggregateTypeAffinityFilter| replace:: :class:`AggregateTypeAffinityFilter ` -.. |AggregateTypeExtraSpecsAffinityFilter| replace:: :class:`AggregateTypeExtraSpecsAffinityFilter ` .. |ServerGroupAntiAffinityFilter| replace:: :class:`ServerGroupAntiAffinityFilter ` .. |ServerGroupAffinityFilter| replace:: :class:`ServerGroupAffinityFilter ` .. |AggregateInstanceExtraSpecsFilter| replace:: :class:`AggregateInstanceExtraSpecsFilter ` diff --git a/nova/scheduler/filters/type_filter.py b/nova/scheduler/filters/type_filter.py index f67fc8ba42b4..b10060db0eb9 100644 --- a/nova/scheduler/filters/type_filter.py +++ b/nova/scheduler/filters/type_filter.py @@ -13,16 +13,9 @@ # WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the # License for the specific language governing permissions and limitations # under the License. -import collections -import six -from nova.i18n import _LI from nova.scheduler import filters from nova.scheduler.filters import utils -from oslo_log import log as logging - - -LOG = logging.getLogger(__name__) class TypeAffinityFilter(filters.BaseHostFilter): @@ -66,136 +59,3 @@ class AggregateTypeAffinityFilter(filters.BaseHostFilter): [x.strip() for x in val.split(',')]): return True return not aggregate_vals - - -class AggregateTypeExtraSpecsAffinityFilter(filters.BaseHostFilter): - """AggregateTypeExtraSpecsAffinityFilter filters only hosts aggregated - inside host aggregates containing "flavor_extra_spec" metadata. Keys inside - this variable must match with the instance extra specs. - - To use this filter, the name of this class should be added to the variable - ``scheduler_default_filters``, in ``nova.conf``: - - | [DEFAULT] - | scheduler_default_filters=, \ - AggregateTypeExtraSpecsAffinityFilter - - The content of the list will be formatted as follows. The entries in the - list will be separated by commas without white space. Each entry will - comprise of an instance type extra spec key followed by and equals "=" - followed by a value: =. - - Eg. - Host Aggregate metadata: - 'flavor_extra_spec=hw:mem_page_size=any,hw:mem_page_size=~,' \ - 'hw:mem_page_size=small' - Flavor extra specs: - 'hw:mem_page_size=small' - - Valid sentinel values are: - - | * (asterisk): may be used to specify that any value is valid. - | ~ (tilde): may be used to specify that a key may optionally be omitted. - | ! (exclamation): may be used to specify that the key must not be present. - """ - - def _parse_extra_spec_key_pairs(self, aggregate_extra_spec): - """Parse and group all key/data from aggregate_extra_spec. - - :param aggregate_extra_spec: string containing a list of required - instance flavor extra specs, separated by - commas, with format "key=value" - :type aggregate_extra_spec: unicode. - :return: dictionary with the values parsed and grouped by keys. - :type: dict. - """ - extra_specs = collections.defaultdict(set) - kv_list = str(aggregate_extra_spec).split(',') - - # Create a new set entry in a dict for every new key, update the key - # value (set object) for every other value. - for kv_element in kv_list: - key, value = kv_element.split('=', 1) - extra_specs[key].add(value) - if '=' in value: - LOG.info(_LI("Value string has an '=' char: key = '%(key)s', " - "value = '%(value)s'. Check if it's malformed"), - {'value': value, 'key': key}) - return extra_specs - - def _instance_is_allowed_in_aggregate(self, - aggregate_extra_spec, - instance_extra_specs): - """Loop over all aggregate_extra_spec elements parsed and execute the - appropriate filter action. - - :param aggregate_extra_spec: dictionary with the values parsed and - grouped by keys. - :type aggregate_extra_spec: dict. - :param instance_extra_specs: dictionary containing the extra specs of - the instance to be filtered. - :type instance_extra_specs: dict. - :return: True if all parameters executed correctly; False is there - is any error. - :type: boolean. - """ - for key, value in six.iteritems(aggregate_extra_spec): - if '*' in value: - if key not in instance_extra_specs: - LOG.debug("'flavor_extra_spec' key: %(key)s " - "is not present", {'key': key}) - return False - else: - continue - - if '!' in value: - if key in instance_extra_specs: - LOG.debug("'flavor_extra_spec' key: %(key)s " - "is present", {'key': key}) - return False - else: - continue - - if '~' in value: - if key not in instance_extra_specs: - continue - else: - value.discard('~') - - for element in value: - match = [char for char in ["*", "!", "~"] if char in element] - if match: - LOG.info(_LI("Value string has '%(chars)s' char(s): " - "key = '%(key)s', value = '%(value)s'. " - "Check if it's malformed"), - {'value': element, 'chars': match, 'key': key}) - if key not in instance_extra_specs: - LOG.debug("'flavor_extra_spec' key: %(key)s " - "is not present", {'key': key}) - return False - if instance_extra_specs[key] not in value: - LOG.debug("The following 'flavor_extra_spec' " - "key=value: %(key)s=%(value)s doesn't " - "match", {'key': key, 'value': value}) - return False - - return True - - def host_passes(self, host_state, spec_obj): - instance_type = spec_obj.flavor - # If 'extra_specs' is not present or extra_specs are empty then we - # need not proceed further - if (not instance_type.obj_attr_is_set('extra_specs') - or not instance_type.extra_specs): - return True - - aggregate_extra_spec_list = utils.aggregate_values_from_key(host_state, - 'flavor_extra_spec') - - for aggregate_extra_spec in aggregate_extra_spec_list: - aggregate_extra_spec = self._parse_extra_spec_key_pairs( - aggregate_extra_spec) - if not self._instance_is_allowed_in_aggregate(aggregate_extra_spec, - instance_type.extra_specs): - return False - return True diff --git a/nova/tests/unit/scheduler/filters/test_type_filters.py b/nova/tests/unit/scheduler/filters/test_type_filters.py index 7fac83ebe259..e201322c48b8 100644 --- a/nova/tests/unit/scheduler/filters/test_type_filters.py +++ b/nova/tests/unit/scheduler/filters/test_type_filters.py @@ -10,8 +10,6 @@ # License for the specific language governing permissions and limitations # under the License. -import logging - import mock from nova import objects @@ -131,195 +129,3 @@ class TestTypeFilter(test.NoDBTestCase): self.assertTrue(self.filt_cls.host_passes(host, spec_obj2)) # False as instance_type is not allowed for aggregate self.assertFalse(self.filt_cls.host_passes(host, spec_obj3)) - - -class TestAggregateTypeExtraSpecsAffinityFilter(test.NoDBTestCase): - - def setUp(self): - super(TestAggregateTypeExtraSpecsAffinityFilter, self).setUp() - self.filt_cls = type_filter.AggregateTypeExtraSpecsAffinityFilter() - - self.aggr_no_extraspecs = objects.Aggregate(context='test') - self.aggr_no_extraspecs.metadata = {} - - self.aggr_invalid_extraspecs = objects.Aggregate(context='test') - self.aggr_invalid_extraspecs.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=large'} - - self.aggr_valid_extraspecs = objects.Aggregate(context='test') - self.aggr_valid_extraspecs.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=small'} - - self.aggr_optional_large = objects.Aggregate(context='test') - self.aggr_optional_large.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=large,hw:mem_page_size=~'} - - self.aggr_optional_small = objects.Aggregate(context='test') - self.aggr_optional_small.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=small,hw:mem_page_size=~'} - - self.aggr_optional_first = objects.Aggregate(context='test') - self.aggr_optional_first.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=~,hw:mem_page_size=small,hw:mem_page_size=any'} - - self.aggr_optional_last = objects.Aggregate(context='test') - self.aggr_optional_last.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=any,hw:mem_page_size=small,hw:mem_page_size=~'} - - self.aggr_optional_middle = objects.Aggregate(context='test') - self.aggr_optional_middle.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=any,hw:mem_page_size=~,hw:mem_page_size=small'} - - self.aggr_any = objects.Aggregate(context='test') - self.aggr_any.metadata = {'flavor_extra_spec': 'hw:mem_page_size=*'} - - self.aggr_nopresent = objects.Aggregate(context='test') - self.aggr_nopresent.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=!'} - - self.aggr_asterisk_inline = objects.Aggregate(context='test') - self.aggr_asterisk_inline.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=*value'} - - self.aggr_tilde_inline = objects.Aggregate(context='test') - self.aggr_tilde_inline.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=value~'} - - self.aggr_exclamation_inline = objects.Aggregate(context='test') - self.aggr_exclamation_inline.metadata = {'flavor_extra_spec': - 'hw:mem_page_size=va!lue'} - - self.rs_extraspecs = objects.RequestSpec( - context=mock.sentinel.ctx, - flavor=objects.Flavor(extra_specs={'hw:mem_page_size': 'small'})) - self.rs_no_extraspecs = objects.RequestSpec( - context=mock.sentinel.ctx, - flavor=objects.Flavor()) - - @mock.patch.object(logging.LoggerAdapter, 'info') - def test_parse_extra_spec_key_pairs(self, mock_logging): - mock_logging.return_value = True - self.filt_cls._parse_extra_spec_key_pairs("key=value=foobar") - mock_logging.assert_called_with( - ("Value string has an '=' char: key = '%(key)s', value = " - "'%(value)s'. Check if it's malformed"), - {'value': 'value=foobar', 'key': 'key'}) - mock_logging.reset_mock() - self.filt_cls._parse_extra_spec_key_pairs("key=value") - mock_logging.assert_not_called() - - @mock.patch.object(logging.LoggerAdapter, 'info') - def test_sentinel_values_inline(self, mock_logging): - mock_logging.return_value = True - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state_aggregates = [self.aggr_asterisk_inline, - self.aggr_tilde_inline, self.aggr_exclamation_inline] - chars = ['*', '~', '!'] - values = ['*value', 'value~', 'va!lue'] - for host_state_aggregate, char, value in zip(host_state_aggregates, - chars, values): - host_state.aggregates = [host_state_aggregate] - self.filt_cls.host_passes(host_state, self.rs_extraspecs) - mock_logging.assert_called_with( - ("Value string has '%(chars)s' char(s): key = '%(key)s', " - "value = '%(value)s'. Check if it's malformed"), - {'chars': [char], 'value': value, 'key': 'hw:mem_page_size'}) - mock_logging.reset_mock() - - @mock.patch.object(logging.LoggerAdapter, 'info') - def test_no_sentinel_values_inline(self, mock_logging): - mock_logging.return_value = True - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates = [self.aggr_valid_extraspecs] - self.filt_cls.host_passes(host_state, self.rs_extraspecs) - mock_logging.assert_not_called() - - def test_aggregate_type_extra_specs_ha_no_extraspecs(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_no_extraspecs) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_ha_wrong_extraspecs(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_invalid_extraspecs) - self.assertFalse(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_ha_right_extraspecs(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_valid_extraspecs) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_2ha_noextra_wrong(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_no_extraspecs) - host_state.aggregates.append(self.aggr_invalid_extraspecs) - self.assertFalse(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_2ha_noextra_right(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_no_extraspecs) - host_state.aggregates.append(self.aggr_valid_extraspecs) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_2ha_wrong_right(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_invalid_extraspecs) - host_state.aggregates.append(self.aggr_valid_extraspecs) - self.assertFalse(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_any_and_more_large(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_optional_large) - self.assertFalse(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_any_and_more_small(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_optional_small) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_any_and_more_no_extraspecs(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_optional_large) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_no_extraspecs)) - - def test_aggregate_type_extra_specs_any_order(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - for host_state_aggregate in [self.aggr_optional_first, - self.aggr_optional_last, - self.aggr_optional_middle]: - host_state.aggregates = [host_state_aggregate] - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_asterisk(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_any) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_asterisk_no_extraspecs(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_any) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_no_extraspecs)) - - def test_aggregate_type_extra_specs_nospec(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_nopresent) - self.assertFalse(self.filt_cls.host_passes(host_state, - self.rs_extraspecs)) - - def test_aggregate_type_extra_specs_nospec_no_extraspecs(self): - host_state = fakes.FakeHostState('host1', 'compute', {}) - host_state.aggregates.append(self.aggr_nopresent) - self.assertTrue(self.filt_cls.host_passes(host_state, - self.rs_no_extraspecs)) diff --git a/releasenotes/notes/add-aggregate-type-extra-specs-affinity-filter-79a2d3ee152b8ecd.yaml b/releasenotes/notes/add-aggregate-type-extra-specs-affinity-filter-79a2d3ee152b8ecd.yaml deleted file mode 100644 index 78717c07110d..000000000000 --- a/releasenotes/notes/add-aggregate-type-extra-specs-affinity-filter-79a2d3ee152b8ecd.yaml +++ /dev/null @@ -1,10 +0,0 @@ ---- -features: - - Add a new scheduler filter, AggregateTypeExtraSpecsAffinityFilter, which - filters only hosts aggregated inside host aggregates containing - "flavor_extra_spec" metadata. Keys inside this variable must match with the - instance extra specs. The entries in the list will be separated by commas - without white space. Each entry will comprise of an instance type extra - spec key followed by an equals "=" followed by a value (=). - Documentation can be find in doc folder (doc/source/filter_scheduler.rst) - and online (http://docs.openstack.org/developer/nova/filter_scheduler.html).