openstack/nova
Code Issues Proposed changes

Remove ComputeDriver.macs_for_instance method

Browse Source 
The only in-tree driver that implemented the macs_for_instance
method was the ironic driver but that hasn't been used since
I4d70423ca978885a982c7eb5bd1efcc024d2b777 in Ocata, so this
drops the now unused ComputeDriver method from the ComputeManager.

There are only two known out-of-tree drivers that have defined
that method in their driver code [1] but they are simply returning
None which is what the base ComputeDriver class does so those drivers
would be unaffected by this change.

A follow up change will continue to remove the "macs" kwarg from the
allocate_for_instance network API.

[1] http://codesearch.openstack.org/?q=def%20macs_for_instance&i=nope&files=&repos=

Change-Id: Ie18ef2386d0a18c970b5c5cba86958cd23f3a228
This commit is contained in:
Matt Riedemann 2019-04-15 13:19:16 -04:00
parent 1388855be2
commit 95d5eb316c
4 changed files with 13 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
nova/compute/manager.py
View File

@ -1462,7 +1462,7 @@ class ComputeManager(manager.Manager):
raise exception.InstanceExists(name=instance.name)
def _allocate_network_async(self, context, instance, requested_networks,
macs, security_groups, is_vpn,
security_groups, is_vpn,
resource_provider_mapping):
"""Method used to allocate networks in the background.
@ -1486,7 +1486,7 @@ class ComputeManager(manager.Manager):
nwinfo = self.network_api.allocate_for_instance(
context, instance, vpn=is_vpn,
requested_networks=requested_networks,
macs=macs,
macs=None, # TODO(mriedem): Remove macs kwarg.
security_groups=security_groups,
bind_host_id=bind_host_id,
resource_provider_mapping=resource_provider_mapping)
@ -1533,14 +1533,13 @@ class ComputeManager(manager.Manager):
if not self.is_neutron_security_groups:
security_groups = []
macs = self.driver.macs_for_instance(instance)
network_info = self._allocate_network(context, instance,
requested_networks, macs, security_groups,
requested_networks, security_groups,
resource_provider_mapping)
return network_info
def _allocate_network(self, context, instance, requested_networks, macs,
def _allocate_network(self, context, instance, requested_networks,
security_groups, resource_provider_mapping):
"""Start network allocation asynchronously. Return an instance
of NetworkInfoAsyncWrapper that can be used to retrieve the
@ -1556,7 +1555,7 @@ class ComputeManager(manager.Manager):
is_vpn = False
return network_model.NetworkInfoAsyncWrapper(
self._allocate_network_async, context, instance,
requested_networks, macs, security_groups, is_vpn,
requested_networks, security_groups, is_vpn,
resource_provider_mapping)
def _default_root_device_name(self, instance, image_meta, root_bdm):

27
nova/tests/unit/compute/test_compute.py
View File

@ -4540,33 +4540,6 @@ class ComputeTestCase(BaseTestCase,
self.context)
self.assertEqual(payload['image_ref_url'], image_ref_url)
@mock.patch.object(fake.FakeDriver, "macs_for_instance")
def test_run_instance_queries_macs(self, mock_mac):
# run_instance should ask the driver for node mac addresses and pass
# that to the network_api in use.
fake_network.unset_stub_network_methods(self)
instance = self._create_fake_instance_obj()
macs = set(['01:23:45:67:89:ab'])
mock_mac.return_value = macs
with mock.patch.object(self.compute.network_api,
'allocate_for_instance') as mock_allocate:
mock_allocate.return_value = (
fake_network.fake_get_instance_nw_info(self, 1, 1))
self.compute._build_networks_for_instance(self.context, instance,
requested_networks=None, security_groups=None,
resource_provider_mapping=None)
security_groups = None if CONF.use_neutron else []
mock_allocate.assert_called_once_with(self.context, instance,
vpn=False, requested_networks=None, macs=macs,
security_groups=security_groups,
bind_host_id=self.compute.host,
resource_provider_mapping=None)
mock_mac.assert_called_once_with(test.MatchType(instance_obj.Instance))
def test_instance_set_to_error_on_uncaught_exception(self):
# Test that instance is set to error state when exception is raised.
instance = self._create_fake_instance_obj()

27
nova/tests/unit/compute/test_compute_mgr.py
View File

@ -562,7 +562,6 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
is_vpn = 'fake-is-vpn'
req_networks = objects.NetworkRequestList(
objects=[objects.NetworkRequest(network_id='fake')])
macs = 'fake-macs'
sec_groups = 'fake-sec-groups'
final_result = 'meow'
rp_mapping = {}
@ -574,7 +573,6 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
side_effect=[test.TestingException()] * 7 + [final_result]):
res = self.compute._allocate_network_async(self.context, instance,
req_networks,
macs,
sec_groups,
is_vpn,
rp_mapping)
@ -593,7 +591,6 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
is_vpn = 'fake-is-vpn'
req_networks = objects.NetworkRequestList(
objects=[objects.NetworkRequest(network_id='fake')])
macs = 'fake-macs'
sec_groups = 'fake-sec-groups'
rp_mapping = {}
@ -602,12 +599,12 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
side_effect=test.TestingException) as mock_allocate:
self.assertRaises(test.TestingException,
self.compute._allocate_network_async,
self.context, instance, req_networks, macs,
self.context, instance, req_networks,
sec_groups, is_vpn, rp_mapping)
mock_allocate.assert_called_once_with(
self.context, instance, vpn=is_vpn,
requested_networks=req_networks, macs=macs,
requested_networks=req_networks, macs=None,
security_groups=sec_groups,
bind_host_id=instance.get('host'),
resource_provider_mapping=rp_mapping)
@ -623,7 +620,6 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
is_vpn = 'fake-is-vpn'
req_networks = objects.NetworkRequestList(
objects=[objects.NetworkRequest(network_id='fake')])
macs = 'fake-macs'
sec_groups = 'fake-sec-groups'
final_result = 'zhangtralon'
rp_mapping = {}
@ -634,7 +630,6 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
final_result]):
res = self.compute._allocate_network_async(self.context, instance,
req_networks,
macs,
sec_groups,
is_vpn,
rp_mapping)
@ -647,7 +642,7 @@ class ComputeManagerUnitTestCase(test.NoDBTestCase,
req_networks = objects.NetworkRequestList(
objects=[objects.NetworkRequest(network_id='none')])
nwinfo = self.compute._allocate_network_async(
self.context, mock.sentinel.instance, req_networks, macs=None,
self.context, mock.sentinel.instance, req_networks,
security_groups=['default'], is_vpn=False,
resource_provider_mapping={})
self.assertEqual(0, len(nwinfo))
@ -5235,9 +5230,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
@mock.patch.object(objects.InstanceActionEvent, 'event_start')
@mock.patch.object(objects.InstanceActionEvent,
'event_finish_with_failure')
@mock.patch.object(virt_driver.ComputeDriver, 'macs_for_instance')
def test_rescheduled_exception_with_network_allocated(self,
mock_macs_for_instance, mock_event_finish,
mock_event_finish,
mock_event_start, mock_ins_save,
mock_build_ins, mock_build_and_run):
self.flags(use_neutron=False)
@ -5246,7 +5240,6 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
system_metadata={'network_allocated': 'True'},
expected_attrs=['metadata', 'system_metadata', 'info_cache'])
mock_ins_save.return_value = instance
mock_macs_for_instance.return_value = []
mock_build_and_run.side_effect = exception.RescheduledException(
reason='', instance_uuid=self.instance.uuid)
@ -5284,9 +5277,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
@mock.patch.object(objects.InstanceActionEvent, 'event_start')
@mock.patch.object(objects.InstanceActionEvent,
'event_finish_with_failure')
@mock.patch.object(virt_driver.ComputeDriver, 'macs_for_instance')
def test_rescheduled_exception_with_network_allocated_with_neutron(self,
mock_macs_for_instance, mock_event_finish, mock_event_start,
mock_event_finish, mock_event_start,
mock_ins_save, mock_build_ins, mock_cleanup_network,
mock_build_and_run):
"""Tests that we always cleanup allocated networks for the instance
@ -5297,7 +5289,6 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
system_metadata={'network_allocated': 'True'},
expected_attrs=['metadata', 'system_metadata', 'info_cache'])
mock_ins_save.return_value = instance
mock_macs_for_instance.return_value = []
mock_build_and_run.side_effect = exception.RescheduledException(
reason='', instance_uuid=self.instance.uuid)
@ -5332,9 +5323,8 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
@mock.patch.object(objects.InstanceActionEvent, 'event_start')
@mock.patch.object(objects.InstanceActionEvent,
'event_finish_with_failure')
@mock.patch.object(virt_driver.ComputeDriver, 'macs_for_instance')
def test_rescheduled_exception_with_sriov_network_allocated(self,
mock_macs_for_instance, mock_event_finish,
mock_event_finish,
mock_event_start, mock_ins_save, mock_cleanup_network,
mock_build_ins, mock_build_and_run):
vif1 = fake_network_cache_model.new_vif()
@ -5353,7 +5343,6 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
instance.info_cache = info_cache
mock_ins_save.return_value = instance
mock_macs_for_instance.return_value = []
mock_build_and_run.side_effect = exception.RescheduledException(
reason='', instance_uuid=self.instance.uuid)
@ -6374,7 +6363,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
self.resource_provider_mapping)
mock_allocate.assert_called_once_with(self.context, instance,
self.requested_networks, None, self.security_groups,
self.requested_networks, self.security_groups,
self.resource_provider_mapping)
self.assertTrue(hasattr(nw_info_obj, 'wait'), "wait must be there")
@ -6390,7 +6379,7 @@ class ComputeManagerBuildInstanceTestCase(test.NoDBTestCase):
self.resource_provider_mapping)
mock_allocate.assert_called_once_with(self.context, instance,
self.requested_networks, None, self.security_groups,
self.requested_networks, self.security_groups,
self.resource_provider_mapping)
self.assertTrue(hasattr(nw_info_obj, 'wait'), "wait must be there")

31
nova/virt/driver.py
View File

@ -1515,37 +1515,6 @@ class ComputeDriver(object):
"""Does the driver want networks deallocated on reschedule?"""
return False
# NOTE(vsaienko) This method is deprecated, don't use it!
# TODO(vsaienko) Remove this function in Ocata.
def macs_for_instance(self, instance):
"""What MAC addresses must this instance have?
Some hypervisors (such as bare metal) cannot do freeform virtualization
of MAC addresses. This method allows drivers to return a set of MAC
addresses that the instance is to have. allocate_for_instance will take
this into consideration when provisioning networking for the instance.
Mapping of MAC addresses to actual networks (or permitting them to be
freeform) is up to the network implementation layer. For instance,
with openflow switches, fixed MAC addresses can still be virtualized
onto any L2 domain, with arbitrary VLANs etc, but regular switches
require pre-configured MAC->network mappings that will match the
actual configuration.
Most hypervisors can use the default implementation which returns None.
Hypervisors with MAC limits should return a set of MAC addresses, which
will be supplied to the allocate_for_instance call by the compute
manager, and it is up to that call to ensure that all assigned network
details are compatible with the set of MAC addresses.
This is called during spawn_instance by the compute manager.
:return: None, or a set of MAC ids (e.g. set(['12:34:56:78:90:ab'])).
None means 'no constraints', a set means 'these and only these
MAC addresses'.
"""
return None
def manage_image_cache(self, context, all_instances):
"""Manage the driver's local image cache.