openstack
/
nova
Code Issues Proposed changes

Pick ironic nodes without VCPU set 

Starting with the Pike release, reporting VCPU/memory/disk is no longer required.
However, we used VCPU to check if a node is available, so nodes without VCPU in
their properties were always ignored. This patch changes the logic to use the existing
_node_resources_unavailable call.

This change also fixes another related issue: when disk or memory are missing from
properties, the virt driver tries to report zero max_unit for them, which is not
allowed by placement.

Change-Id: I1bbfc152189252c5c45e6153695a802d17b76690
Closes-Bug: #1723423
This commit is contained in:
Dmitry Tantsur 2017-10-13 15:18:16 +02:00
parent d9212edb8f
commit b25928dc04
3 changed files with 80 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

73
nova/tests/unit/virt/ironic/test_driver.py
View File

@ -731,9 +731,11 @@ class IronicDriverTestCase(test.NoDBTestCase):
expected_uuids = [n['uuid'] for n in node_dicts if n['expected']] expected_uuids = [n['uuid'] for n in node_dicts if n['expected']]
self.assertEqual(sorted(expected_uuids), sorted(available_nodes)) self.assertEqual(sorted(expected_uuids), sorted(available_nodes))
@mock.patch.object(ironic_driver.IronicDriver,
'_node_resources_unavailable', return_value=False)
@mock.patch.object(ironic_driver.IronicDriver, '_node_resource') @mock.patch.object(ironic_driver.IronicDriver, '_node_resource')
@mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache') @mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache')
def test_get_inventory_no_rc(self, mock_nfc, mock_nr): def test_get_inventory_no_rc(self, mock_nfc, mock_nr, mock_res_unavail):
"""Ensure that when node.resource_class is missing, that we return the """Ensure that when node.resource_class is missing, that we return the
legacy VCPU, MEMORY_MB and DISK_GB resources for inventory. legacy VCPU, MEMORY_MB and DISK_GB resources for inventory.
""" """
@ -777,11 +779,14 @@ class IronicDriverTestCase(test.NoDBTestCase):
} }
mock_nfc.assert_called_once_with(mock.sentinel.nodename) mock_nfc.assert_called_once_with(mock.sentinel.nodename)
mock_nr.assert_called_once_with(mock_nfc.return_value) mock_nr.assert_called_once_with(mock_nfc.return_value)
mock_res_unavail.assert_called_once_with(mock_nfc.return_value)
self.assertEqual(expected, result) self.assertEqual(expected, result)
@mock.patch.object(ironic_driver.IronicDriver,
'_node_resources_unavailable', return_value=False)
@mock.patch.object(ironic_driver.IronicDriver, '_node_resource') @mock.patch.object(ironic_driver.IronicDriver, '_node_resource')
@mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache') @mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache')
def test_get_inventory_with_rc(self, mock_nfc, mock_nr): def test_get_inventory_with_rc(self, mock_nfc, mock_nr, mock_res_unavail):
"""Ensure that when node.resource_class is present, that we return the """Ensure that when node.resource_class is present, that we return the
legacy VCPU, MEMORY_MB and DISK_GB resources for inventory in addition legacy VCPU, MEMORY_MB and DISK_GB resources for inventory in addition
to the custom resource class inventory record. to the custom resource class inventory record.
@ -834,11 +839,51 @@ class IronicDriverTestCase(test.NoDBTestCase):
} }
mock_nfc.assert_called_once_with(mock.sentinel.nodename) mock_nfc.assert_called_once_with(mock.sentinel.nodename)
mock_nr.assert_called_once_with(mock_nfc.return_value) mock_nr.assert_called_once_with(mock_nfc.return_value)
mock_res_unavail.assert_called_once_with(mock_nfc.return_value)
self.assertEqual(expected, result) self.assertEqual(expected, result)
@mock.patch.object(ironic_driver.IronicDriver,
'_node_resources_unavailable', return_value=False)
@mock.patch.object(ironic_driver.IronicDriver, '_node_resource') @mock.patch.object(ironic_driver.IronicDriver, '_node_resource')
@mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache') @mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache')
def test_get_inventory_with_rc_occupied(self, mock_nfc, mock_nr): def test_get_inventory_only_rc(self, mock_nfc, mock_nr, mock_res_unavail):
"""Ensure that when node.resource_class is present, that we return the
legacy VCPU, MEMORY_MB and DISK_GB resources for inventory in addition
to the custom resource class inventory record.
"""
mock_nr.return_value = {
'vcpus': 0,
'vcpus_used': 0,
'memory_mb': 0,
'memory_mb_used': 0,
'local_gb': 0,
'local_gb_used': 0,
'resource_class': 'iron-nfv',
}
result = self.driver.get_inventory(mock.sentinel.nodename)
expected = {
'CUSTOM_IRON_NFV': {
'total': 1,
'reserved': 0,
'min_unit': 1,
'max_unit': 1,
'step_size': 1,
'allocation_ratio': 1.0,
},
}
mock_nfc.assert_called_once_with(mock.sentinel.nodename)
mock_nr.assert_called_once_with(mock_nfc.return_value)
mock_res_unavail.assert_called_once_with(mock_nfc.return_value)
self.assertEqual(expected, result)
@mock.patch.object(ironic_driver.IronicDriver,
'_node_resources_unavailable', return_value=False)
@mock.patch.object(ironic_driver.IronicDriver, '_node_resource')
@mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache')
def test_get_inventory_with_rc_occupied(self, mock_nfc, mock_nr,
mock_res_unavail):
"""Ensure that when a node is used, we report the inventory matching """Ensure that when a node is used, we report the inventory matching
the consumed resources. the consumed resources.
""" """
@ -890,27 +935,19 @@ class IronicDriverTestCase(test.NoDBTestCase):
} }
mock_nfc.assert_called_once_with(mock.sentinel.nodename) mock_nfc.assert_called_once_with(mock.sentinel.nodename)
mock_nr.assert_called_once_with(mock_nfc.return_value) mock_nr.assert_called_once_with(mock_nfc.return_value)
mock_res_unavail.assert_called_once_with(mock_nfc.return_value)
self.assertEqual(expected, result) self.assertEqual(expected, result)
@mock.patch.object(ironic_driver.IronicDriver, '_node_resource') @mock.patch.object(ironic_driver.IronicDriver,
'_node_resources_unavailable', return_value=True)
@mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache') @mock.patch.object(ironic_driver.IronicDriver, '_node_from_cache')
def test_get_inventory_disabled_node(self, mock_nfc, mock_nr): def test_get_inventory_disabled_node(self, mock_nfc, mock_res_unavail):
"""Ensure that when vcpus == 0 (which happens when a node is disabled), """Ensure that when a node is disabled, that get_inventory() returns
that get_inventory() returns an empty dict. an empty dict.
""" """
mock_nr.return_value = {
'vcpus': 0,
'vcpus_used': 0,
'memory_mb': 0,
'memory_mb_used': 0,
'local_gb': 0,
'local_gb_used': 0,
'resource_class': None,
}
result = self.driver.get_inventory(mock.sentinel.nodename) result = self.driver.get_inventory(mock.sentinel.nodename)
mock_nfc.assert_called_once_with(mock.sentinel.nodename) mock_nfc.assert_called_once_with(mock.sentinel.nodename)
mock_nr.assert_called_once_with(mock_nfc.return_value) mock_res_unavail.assert_called_once_with(mock_nfc.return_value)
self.assertEqual({}, result) self.assertEqual({}, result)
@mock.patch.object(FAKE_CLIENT.node, 'get') @mock.patch.object(FAKE_CLIENT.node, 'get')

52
nova/virt/ironic/driver.py
View File

@ -754,16 +754,13 @@ class IronicDriver(virt_driver.ComputeDriver):
""" """
# nodename is the ironic node's UUID. # nodename is the ironic node's UUID.
node = self._node_from_cache(nodename) node = self._node_from_cache(nodename)
info = self._node_resource(node)
# TODO(jaypipes): Completely remove the reporting of VCPU, MEMORY_MB, # TODO(jaypipes): Completely remove the reporting of VCPU, MEMORY_MB,
# and DISK_GB resource classes in early Queens when Ironic nodes will # and DISK_GB resource classes in early Queens when Ironic nodes will
# *always* return the custom resource class that represents the # *always* return the custom resource class that represents the
# baremetal node class in an atomic, singular unit. # baremetal node class in an atomic, singular unit.
if info['vcpus'] == 0: if self._node_resources_unavailable(node):
# NOTE(jaypipes): The driver can return 0-valued vcpus when the # TODO(dtantsur): report resources as reserved instead of reporting
# node is "disabled". In the future, we should detach inventory # an empty inventory
# accounting from the concept of a node being disabled or not. The
# two things don't really have anything to do with each other.
LOG.debug('Node %(node)s is not ready for a deployment, ' LOG.debug('Node %(node)s is not ready for a deployment, '
'reporting an empty inventory for it. Node\'s ' 'reporting an empty inventory for it. Node\'s '
'provision state is %(prov)s, power state is ' 'provision state is %(prov)s, power state is '
@ -772,32 +769,23 @@ class IronicDriver(virt_driver.ComputeDriver):
'power': node.power_state, 'maint': node.maintenance}) 'power': node.power_state, 'maint': node.maintenance})
return {} return {}
result = { info = self._node_resource(node)
obj_fields.ResourceClass.VCPU: { result = {}
'total': info['vcpus'], for rc, field in [(obj_fields.ResourceClass.VCPU, 'vcpus'),
'reserved': 0, (obj_fields.ResourceClass.MEMORY_MB, 'memory_mb'),
'min_unit': 1, (obj_fields.ResourceClass.DISK_GB, 'local_gb')]:
'max_unit': info['vcpus'], # NOTE(dtantsur): any of these fields can be zero starting with
'step_size': 1, # the Pike release.
'allocation_ratio': 1.0, if info[field]:
}, result[rc] = {
obj_fields.ResourceClass.MEMORY_MB: { 'total': info[field],
'total': info['memory_mb'], 'reserved': 0,
'reserved': 0, 'min_unit': 1,
'min_unit': 1, 'max_unit': info[field],
'max_unit': info['memory_mb'], 'step_size': 1,
'step_size': 1, 'allocation_ratio': 1.0,
'allocation_ratio': 1.0, }
},
obj_fields.ResourceClass.DISK_GB: {
'total': info['local_gb'],
'reserved': 0,
'min_unit': 1,
'max_unit': info['local_gb'],
'step_size': 1,
'allocation_ratio': 1.0,
},
}
rc_name = info.get('resource_class') rc_name = info.get('resource_class')
if rc_name is not None: if rc_name is not None:
# TODO(jaypipes): Raise an exception in Queens if Ironic doesn't # TODO(jaypipes): Raise an exception in Queens if Ironic doesn't

5
releasenotes/notes/ironic-empty-vcpus-66b4e1500ef8a34e.yaml Normal file
View File

@ -0,0 +1,5 @@
---
fixes:
- |
Fixes a bug preventing ironic nodes without VCPUs, memory or disk in their
properties from being picked by nova.