From b32091192b7af7b0fccab1e078480ec38ada7bcc Mon Sep 17 00:00:00 2001 From: Zhiteng Huang Date: Thu, 4 Jun 2015 11:23:58 -0700 Subject: [PATCH] DriverFilter: don't check volume_backend_name Currently DriverFilter checks if host_state.volume_backend_name matches type.extra_specs.volume_backend_name. The same check has already been done in CapabilitiesFilter and done more thoroughly and gracefully, for example, volume_backend_name can be a set of names not limited to one to one match. DriverFilter should just do one thing, that is evaluating filter_function supplied by backend. This change removes the volume_backend_name check from DriverFilter without touching its core function, thus has no impact on existing documentation because the checking of volume_backend_name is an undocumented side-effect of this filter. This change also reduces one warning log to debug log when no filter_function found in host_state, for the reason it would generate a lot of warning log for those backends don't supply filter_function. UpgradeImpact: DriverFilter had an undocumented side-effect before it evaluates filter_function - it checked if volume_backend_name in host_state reported by backend matches volume_backend_name in volume type extra specs, and if they don't, the backend would fail to pass the filter. Now that this side-effect has been removed, it would impact users only under following circumstance: 1) user enabled DriverFilter but not CapabilitiesFilter; 2) user relied on DriverFilter to filter backends based on exact match of 'volume_backend_name' in type extra spec. If unfortunately this is the case for user, enabling CapabilitiesFilter by adding it to 'scheduler_default_filters' configure option and restart cinder scheduler service then everything should work just fine. CapabilitiesFilter actually does a much better job checking volume_backend_name. Change-Id: Ie5c48587368856faa538eebb1b6611d464bd69bd Closes-bug: #1461770 --- cinder/scheduler/filters/driver_filter.py | 17 +- .../tests/unit/scheduler/test_host_filters.py | 190 ++++++------------ 2 files changed, 58 insertions(+), 149 deletions(-) diff --git a/cinder/scheduler/filters/driver_filter.py b/cinder/scheduler/filters/driver_filter.py index ac6fe7b0a..a406c4ca5 100644 --- a/cinder/scheduler/filters/driver_filter.py +++ b/cinder/scheduler/filters/driver_filter.py @@ -48,23 +48,8 @@ class DriverFilter(filters.BaseHostFilter): Returns a tuple in the format (filter_passing, filter_invalid). Both values are booleans. """ - host_stats = stats['host_stats'] - extra_specs = stats['extra_specs'] - - # Check that the volume types match - if (extra_specs is None or 'volume_backend_name' not in extra_specs): - LOG.warning(_LW("No 'volume_backend_name' key in extra_specs. " - "Skipping volume backend name check.")) - elif (extra_specs['volume_backend_name'] != - host_stats['volume_backend_name']): - LOG.warning(_LW("Volume backend names do not match: '%(target)s' " - "vs '%(current)s' :: Skipping"), - {'target': extra_specs['volume_backend_name'], - 'current': host_stats['volume_backend_name']}) - return False - if stats['filter_function'] is None: - LOG.warning(_LW("Filter function not set :: passing host")) + LOG.debug("Filter function not set :: passing host") return True try: diff --git a/cinder/tests/unit/scheduler/test_host_filters.py b/cinder/tests/unit/scheduler/test_host_filters.py index 1271391be..b52100353 100644 --- a/cinder/tests/unit/scheduler/test_host_filters.py +++ b/cinder/tests/unit/scheduler/test_host_filters.py @@ -627,85 +627,6 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', - 'capabilities': { - 'filter_function': '1 == 1', - } - }) - - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } - - self.assertTrue(filt_cls.host_passes(host1, filter_properties)) - - def test_failing_function(self): - filt_cls = self.class_map['DriverFilter']() - host1 = fakes.FakeHostState( - 'host1', { - 'volume_backend_name': 'fake', - 'capabilities': { - 'filter_function': '1 == 2', - } - }) - - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } - - self.assertFalse(filt_cls.host_passes(host1, filter_properties)) - - def test_no_filter_function(self): - filt_cls = self.class_map['DriverFilter']() - host1 = fakes.FakeHostState( - 'host1', { - 'volume_backend_name': 'fake', - 'capabilities': { - 'filter_function': None, - } - }) - - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } - - self.assertTrue(filt_cls.host_passes(host1, filter_properties)) - - def test_not_implemented(self): - filt_cls = self.class_map['DriverFilter']() - host1 = fakes.FakeHostState( - 'host1', { - 'volume_backend_name': 'fake', - 'capabilities': {} - }) - - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } - - self.assertTrue(filt_cls.host_passes(host1, filter_properties)) - - def test_no_volume_extra_specs(self): - filt_cls = self.class_map['DriverFilter']() - host1 = fakes.FakeHostState( - 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': '1 == 1', } @@ -715,31 +636,60 @@ class DriverFilterTestCase(HostFiltersTestCase): self.assertTrue(filt_cls.host_passes(host1, filter_properties)) - def test_volume_backend_name_different(self): + def test_failing_function(self): + filt_cls = self.class_map['DriverFilter']() + host1 = fakes.FakeHostState( + 'host1', { + 'capabilities': { + 'filter_function': '1 == 2', + } + }) + + filter_properties = {'volume_type': {}} + + self.assertFalse(filt_cls.host_passes(host1, filter_properties)) + + def test_no_filter_function(self): + filt_cls = self.class_map['DriverFilter']() + host1 = fakes.FakeHostState( + 'host1', { + 'capabilities': { + 'filter_function': None, + } + }) + + filter_properties = {'volume_type': {}} + + self.assertTrue(filt_cls.host_passes(host1, filter_properties)) + + def test_not_implemented(self): + filt_cls = self.class_map['DriverFilter']() + host1 = fakes.FakeHostState( + 'host1', { + 'capabilities': {} + }) + + filter_properties = {'volume_type': {}} + + self.assertTrue(filt_cls.host_passes(host1, filter_properties)) + + def test_no_volume_extra_specs(self): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': '1 == 1', } }) - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake2', - } - } - } + filter_properties = {'volume_type': {}} - self.assertFalse(filt_cls.host_passes(host1, filter_properties)) + self.assertTrue(filt_cls.host_passes(host1, filter_properties)) def test_function_extra_spec_replacement(self): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': 'extra.var == 1', } @@ -748,7 +698,6 @@ class DriverFilterTestCase(HostFiltersTestCase): filter_properties = { 'volume_type': { 'extra_specs': { - 'volume_backend_name': 'fake', 'var': 1, } } @@ -760,20 +709,13 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'total_capacity_gb': 100, 'capabilities': { 'filter_function': 'stats.total_capacity_gb < 200', } }) - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } + filter_properties = {'volume_type': {}} self.assertTrue(filt_cls.host_passes(host1, filter_properties)) @@ -781,18 +723,12 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': 'volume.size < 5', } }) filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - }, 'request_spec': { 'volume_properties': { 'size': 1 @@ -806,18 +742,12 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': 'qos.var == 1', } }) filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - }, 'qos_specs': { 'var': 1 } @@ -829,19 +759,12 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': '1 / 0 == 0', } }) - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } + filter_properties = {} self.assertFalse(filt_cls.host_passes(host1, filter_properties)) @@ -849,18 +772,12 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'filter_function': 'qos.maxiops == 1', } }) filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - }, 'qos_specs': None } @@ -870,23 +787,30 @@ class DriverFilterTestCase(HostFiltersTestCase): filt_cls = self.class_map['DriverFilter']() host1 = fakes.FakeHostState( 'host1', { - 'volume_backend_name': 'fake', 'capabilities': { 'foo': 10, 'filter_function': 'capabilities.foo == 10', }, }) - filter_properties = { - 'volume_type': { - 'extra_specs': { - 'volume_backend_name': 'fake', - } - } - } + filter_properties = {} self.assertTrue(filt_cls.host_passes(host1, filter_properties)) + def test_wrong_capabilities(self): + filt_cls = self.class_map['DriverFilter']() + host1 = fakes.FakeHostState( + 'host1', { + 'capabilities': { + 'bar': 10, + 'filter_function': 'capabilities.foo == 10', + }, + }) + + filter_properties = {} + + self.assertFalse(filt_cls.host_passes(host1, filter_properties)) + class InstanceLocalityFilterTestCase(HostFiltersTestCase): def setUp(self):