openstack/deb-cinder
Code Issues Proposed changes

DriverFilter: don't check volume_backend_name

Browse Source 
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
This commit is contained in:
Zhiteng Huang 2015-06-04 11:23:58 -07:00 committed by Huang Zhiteng
parent 3ba2d0ed92
commit b32091192b
2 changed files with 58 additions and 149 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

17
cinder/scheduler/filters/driver_filter.py
View File

@ -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:

190
cinder/tests/unit/scheduler/test_host_filters.py
View File

@ -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):