Ensure we unbind flat network ports and clear BM mac addresses

This patch fixes a bug in the ironic nova neutron interaction triangle,
where a port in a flat network can be left with bound and with the mac
address of a baremetal server if it is not deleted after a deployment or
deployment failure. The fix is to ensure that the network interfaces
reset the mac address to a random mac address when it unbinds the port
in the unconfigure step for provisioning and tenant networks, and then
rebind and set the mac address for the configure steps.

Co-Authored-By: Harald Jensås <hjensas@redhat.com>
Story: #2004428
Task: #28087
Change-Id: I11fff92e0a58ac68e795c003c14a336bceba6d89
This commit is contained in:
Sam Betts 2018-02-01 14:56:30 +00:00 committed by Harald Jensås
parent ebd671c4b6
commit db16984b47
9 changed files with 251 additions and 50 deletions

View File

@ -10,6 +10,8 @@
# License for the specific language governing permissions and limitations
# under the License.
import random
from neutronclient.common import exceptions as neutron_exceptions
from neutronclient.v2_0 import client as clientv20
from oslo_log import log
@ -38,6 +40,26 @@ PHYSNET_PARAM_NAME = 'provider:physical_network'
SEGMENTS_PARAM_NAME = 'segments'
"""Name of the neutron network API segments parameter."""
BASE_MAC = ['fa', '16', '3e', '00', '00', '00']
def _get_random_mac(base_mac):
"""Get a random MAC address string of the specified base format.
The first 3 octets will remain unchanged. If the 4th octet is not
00, it will also be used. The others will be randomly generated.
:param base_mac: Base MAC address represented by an array of 6 strings/int
:returns: The MAC address string.
"""
mac = [int(base_mac[0], 16), int(base_mac[1], 16),
int(base_mac[2], 16), random.getrandbits(8),
random.getrandbits(8), random.getrandbits(8)]
if base_mac[3] != '00':
mac[3] = int(base_mac[3], 16)
return ':'.join(["%02x" % x for x in mac])
def _get_neutron_session():
global _NEUTRON_SESSION
@ -102,11 +124,18 @@ def unbind_neutron_port(port_id, client=None, context=None):
if not client:
client = get_client(context=context)
body = {'port': {'binding:host_id': '',
'binding:profile': {}}}
body_unbind = {'port': {'binding:host_id': '',
'binding:profile': {}}}
body_reset_mac = {'port': {
'mac_address': _get_random_mac(BASE_MAC)}}
try:
client.update_port(port_id, body)
client.update_port(port_id, body_unbind)
# NOTE(hjensas): We need to reset the mac address in a separate step.
# Exception PortBound will be raised by neutron as it refuses to
# update the mac address of a bound port if we attempt to unbind and
# reset the mac in the same call.
client.update_port(port_id, body_reset_mac)
# NOTE(vsaienko): Ignore if port was deleted before calling vif detach.
except neutron_exceptions.PortNotFoundClient:
LOG.info('Port %s was not found while unbinding.', port_id)

View File

@ -255,6 +255,7 @@ def plug_port_to_tenant_network(task, port_like_obj, client=None):
'port': {
'binding:vnic_type': neutron.VNIC_BAREMETAL,
'binding:host_id': node.uuid,
'mac_address': port_like_obj.address
}
}
binding_profile = {'local_link_information': local_link_info}

View File

@ -53,12 +53,7 @@ class FlatNetwork(common.NeutronVIFPortIDMixin,
"""
self.get_cleaning_network_uuid(task)
def add_provisioning_network(self, task):
"""Add the provisioning network to a node.
:param task: A TaskManager instance.
:raises: NetworkError when failed to set binding:host_id
"""
def _bind_flat_ports(self, task):
LOG.debug("Binding flat network ports")
client = neutron.get_client(context=task.context)
for port_like_obj in task.ports + task.portgroups:
@ -71,7 +66,8 @@ class FlatNetwork(common.NeutronVIFPortIDMixin,
body = {
'port': {
'binding:host_id': task.node.uuid,
'binding:vnic_type': neutron.VNIC_BAREMETAL
'binding:vnic_type': neutron.VNIC_BAREMETAL,
'mac_address': port_like_obj.address
}
}
try:
@ -83,26 +79,52 @@ class FlatNetwork(common.NeutronVIFPortIDMixin,
LOG.exception(msg)
raise exception.NetworkError(msg)
def _unbind_flat_ports(self, task):
node = task.node
LOG.info('Unbinding instance ports from node %s', node.uuid)
ports = [p for p in task.ports if not p.portgroup_id]
portgroups = task.portgroups
for port_like_obj in ports + portgroups:
vif_port_id = (
port_like_obj.internal_info.get(common.TENANT_VIF_KEY) or
port_like_obj.extra.get('vif_port_id'))
if not vif_port_id:
continue
neutron.unbind_neutron_port(vif_port_id, context=task.context)
def add_provisioning_network(self, task):
"""Add the provisioning network to a node.
:param task: A TaskManager instance.
:raises: NetworkError when failed to set binding:host_id
"""
self._bind_flat_ports(task)
def remove_provisioning_network(self, task):
"""Remove the provisioning network from a node.
:param task: A TaskManager instance.
"""
pass
self._unbind_flat_ports(task)
def configure_tenant_networks(self, task):
"""Configure tenant networks for a node.
:param task: A TaskManager instance.
"""
pass
self._bind_flat_ports(task)
def unconfigure_tenant_networks(self, task):
"""Unconfigure tenant networks for a node.
Unbind the port here/now to avoid the possibility of the ironic port
being bound to the tenant and cleaning networks at the same time.
:param task: A TaskManager instance.
:raises: NetworkError
"""
pass
self._unbind_flat_ports(task)
def add_cleaning_network(self, task):
"""Add the cleaning network to a node.
@ -141,3 +163,28 @@ class FlatNetwork(common.NeutronVIFPortIDMixin,
del internal_info['cleaning_vif_port_id']
port.internal_info = internal_info
port.save()
def add_rescuing_network(self, task):
"""Add the rescuing network to a node.
Flat network does not use the rescuing network.
Bind the port again since unconfigure_tenant_network() unbound it.
:param task: A TaskManager instance.
:returns: a dictionary in the form {port.uuid: neutron_port['id']}
:raises: NetworkError, InvalidParameterValue
"""
LOG.info('Bind ports for rescuing node %s', task.node.uuid)
self._bind_flat_ports(task)
def remove_rescuing_network(self, task):
"""Remove the rescuing network from a node.
Flat network does not use the rescuing network.
Unbind the port again since add_rescuing_network() bound it.
:param task: A TaskManager instance.
:raises: NetworkError
"""
LOG.info('Unbind ports for rescuing node %s', task.node.uuid)
self._unbind_flat_ports(task)

View File

@ -10,6 +10,8 @@
# License for the specific language governing permissions and limitations
# under the License.
import random
from keystoneauth1 import loading as kaloading
import mock
from neutronclient.common import exceptions as neutron_client_exc
@ -748,29 +750,42 @@ class TestUpdatePortAddress(base.TestCase):
@mock.patch.object(neutron, 'get_client', autospec=True)
@mock.patch.object(neutron, '_get_random_mac', autospec=True)
class TestUnbindPort(base.TestCase):
def setUp(self):
super(TestUnbindPort, self).setUp()
self.context = context.RequestContext()
def test_unbind_neutron_port_client_passed(self, mock_client):
def test_unbind_neutron_port_client_passed(self, mock_random_mac,
mock_client):
port_id = 'fake-port-id'
body = {
address = 'fe:54:00:77:07:d9'
mock_random_mac.return_value = address
body_unbind = {
'port': {
'binding:host_id': '',
'binding:profile': {}
}
}
body_reset_mac = {
'port': {
'mac_address': address
}
}
update_calls = [
mock.call(port_id, body_unbind),
mock.call(port_id, body_reset_mac)
]
neutron.unbind_neutron_port(port_id,
mock_client(context=self.context),
context=self.context)
self.assertEqual(1, mock_client.call_count)
mock_client.return_value.update_port.assert_called_once_with(port_id,
body)
mock_client.return_value.update_port.assert_has_calls(update_calls)
@mock.patch.object(neutron, 'LOG', autospec=True)
def test_unbind_neutron_port_failure(self, mock_log, mock_client):
def test_unbind_neutron_port_failure(self, mock_log, mock_random_mac,
mock_client):
mock_client.return_value.update_port.side_effect = (
neutron_client_exc.NeutronClientException())
body = {
@ -787,21 +802,32 @@ class TestUnbindPort(base.TestCase):
body)
mock_log.exception.assert_called_once()
def test_unbind_neutron_port(self, mock_client):
def test_unbind_neutron_port(self, mock_random_mac, mock_client):
port_id = 'fake-port-id'
body = {
address = 'fe:54:00:77:07:d9'
mock_random_mac.return_value = address
body_unbind = {
'port': {
'binding:host_id': '',
'binding:profile': {}
}
}
body_reset_mac = {
'port': {
'mac_address': address
}
}
update_calls = [
mock.call(port_id, body_unbind),
mock.call(port_id, body_reset_mac)
]
neutron.unbind_neutron_port(port_id, context=self.context)
mock_client.assert_called_once_with(context=self.context)
mock_client.return_value.update_port.assert_called_once_with(port_id,
body)
mock_client.return_value.update_port.assert_has_calls(update_calls)
@mock.patch.object(neutron, 'LOG', autospec=True)
def test_unbind_neutron_port_not_found(self, mock_log, mock_client):
def test_unbind_neutron_port_not_found(self, mock_log, mock_random_mac,
mock_client):
port_id = 'fake-port-id'
mock_client.return_value.update_port.side_effect = (
neutron_client_exc.PortNotFoundClient())
@ -1042,3 +1068,18 @@ class TestGetPhysnetsByPortUUID(base.TestCase):
fields=self.PORT_FIELDS)
mock_gn.assert_called_once_with(self.client, network_uuid,
fields=self.NETWORK_FIELDS)
class TestGetRandomMac(base.TestCase):
@mock.patch.object(random, 'getrandbits', return_value=0xa2)
def test_first_4_octets_unchanged(self, mock_rnd):
mac = neutron._get_random_mac(['aa', 'bb', '00', 'dd', 'ee', 'ff'])
self.assertEqual('aa:bb:00:dd:a2:a2', mac)
mock_rnd.assert_called_with(8)
@mock.patch.object(random, 'getrandbits', return_value=0xa2)
def test_first_4th_octet_generated(self, mock_rnd):
mac = neutron._get_random_mac(['aa', 'bb', 'cc', '00', 'ee', 'ff'])
self.assertEqual('aa:bb:cc:a2:a2:a2', mac)
mock_rnd.assert_called_with(8)

View File

@ -175,8 +175,7 @@ class TestFlatInterface(db_base.DbTestCase):
self.assertNotIn('cleaning_vif_port_id', self.port.internal_info)
@mock.patch.object(neutron, 'get_client')
def test_add_provisioning_network_set_binding_host_id(
self, client_mock):
def test__bind_flat_ports_set_binding_host_id(self, client_mock):
upd_mock = mock.Mock()
client_mock.return_value.update_port = upd_mock
extra = {'vif_port_id': 'foo'}
@ -184,14 +183,14 @@ class TestFlatInterface(db_base.DbTestCase):
address='52:54:00:cf:2d:33', extra=extra,
uuid=uuidutils.generate_uuid())
exp_body = {'port': {'binding:host_id': self.node.uuid,
'binding:vnic_type': neutron.VNIC_BAREMETAL}}
'binding:vnic_type': neutron.VNIC_BAREMETAL,
'mac_address': '52:54:00:cf:2d:33'}}
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.add_provisioning_network(task)
self.interface._bind_flat_ports(task)
upd_mock.assert_called_once_with('foo', exp_body)
@mock.patch.object(neutron, 'get_client')
def test_add_provisioning_network_set_binding_host_id_portgroup(
self, client_mock):
def test__bind_flat_ports_set_binding_host_id_portgroup(self, client_mock):
upd_mock = mock.Mock()
client_mock.return_value.update_port = upd_mock
internal_info = {'tenant_vif_port_id': 'foo'}
@ -201,17 +200,46 @@ class TestFlatInterface(db_base.DbTestCase):
utils.create_test_port(
self.context, node_id=self.node.id, address='52:54:00:cf:2d:33',
extra={'vif_port_id': 'bar'}, uuid=uuidutils.generate_uuid())
exp_body = {'port': {'binding:host_id': self.node.uuid,
'binding:vnic_type': neutron.VNIC_BAREMETAL}}
exp_body1 = {'port': {'binding:host_id': self.node.uuid,
'binding:vnic_type': neutron.VNIC_BAREMETAL,
'mac_address': '52:54:00:cf:2d:33'}}
exp_body2 = {'port': {'binding:host_id': self.node.uuid,
'binding:vnic_type': neutron.VNIC_BAREMETAL,
'mac_address': '52:54:00:cf:2d:31'}}
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.add_provisioning_network(task)
self.interface._bind_flat_ports(task)
upd_mock.assert_has_calls([
mock.call('bar', exp_body), mock.call('foo', exp_body)
])
mock.call('bar', exp_body1), mock.call('foo', exp_body2)])
@mock.patch.object(neutron, 'unbind_neutron_port')
def test__unbind_flat_ports(self, unbind_neutron_port_mock):
extra = {'vif_port_id': 'foo'}
utils.create_test_port(self.context, node_id=self.node.id,
address='52:54:00:cf:2d:33', extra=extra,
uuid=uuidutils.generate_uuid())
with task_manager.acquire(self.context, self.node.id) as task:
self.interface._unbind_flat_ports(task)
unbind_neutron_port_mock.assert_called_once_with('foo',
context=self.context)
@mock.patch.object(neutron, 'unbind_neutron_port')
def test__unbind_flat_ports_portgroup(self, unbind_neutron_port_mock):
internal_info = {'tenant_vif_port_id': 'foo'}
utils.create_test_portgroup(self.context, node_id=self.node.id,
internal_info=internal_info,
uuid=uuidutils.generate_uuid())
extra = {'vif_port_id': 'bar'}
utils.create_test_port(self.context, node_id=self.node.id,
address='52:54:00:cf:2d:33', extra=extra,
uuid=uuidutils.generate_uuid())
with task_manager.acquire(self.context, self.node.id) as task:
self.interface._unbind_flat_ports(task)
unbind_neutron_port_mock.has_calls(
[mock.call('foo', context=self.context),
mock.call('bar', context=self.context)])
@mock.patch.object(neutron, 'get_client')
def test_add_provisioning_network_binding_host_id_raise(
self, client_mock):
def test__bind_flat_ports_set_binding_host_id_raise(self, client_mock):
client_mock.return_value.update_port.side_effect = \
(neutron_exceptions.ConnectionFailed())
extra = {'vif_port_id': 'foo'}
@ -220,5 +248,28 @@ class TestFlatInterface(db_base.DbTestCase):
uuid=uuidutils.generate_uuid())
with task_manager.acquire(self.context, self.node.id) as task:
self.assertRaises(exception.NetworkError,
self.interface.add_provisioning_network,
task)
self.interface._bind_flat_ports, task)
@mock.patch.object(flat_interface.FlatNetwork, '_bind_flat_ports')
def test_add_rescuing_network(self, bind_mock):
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.add_rescuing_network(task)
bind_mock.assert_called_once_with(task)
@mock.patch.object(flat_interface.FlatNetwork, '_unbind_flat_ports')
def test_remove_rescuing_network(self, unbind_mock):
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.remove_rescuing_network(task)
unbind_mock.assert_called_once_with(task)
@mock.patch.object(flat_interface.FlatNetwork, '_bind_flat_ports')
def test_add_provisioning_network(self, bind_mock):
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.add_provisioning_network(task)
bind_mock.assert_called_once_with(task)
@mock.patch.object(flat_interface.FlatNetwork, '_unbind_flat_ports')
def test_remove_provisioning_network(self, unbind_mock):
with task_manager.acquire(self.context, self.node.id) as task:
self.interface.remove_provisioning_network(task)
unbind_mock.assert_called_once_with(task)

View File

@ -580,12 +580,10 @@ class NeutronInterfaceTestCase(db_base.DbTestCase):
'binding:vnic_type': 'baremetal',
'binding:host_id': self.node.uuid,
'binding:profile': {'local_link_information':
[self.port.local_link_connection]}
[self.port.local_link_connection]},
'mac_address': '52:54:00:cf:2d:32'
}
}
utils.create_test_port(self.context, node_id=self.node.id,
address='52:54:00:cf:2d:33', extra={},
uuid=uuidutils.generate_uuid())
upd_mock = mock.Mock()
client_mock.return_value.update_port = upd_mock
with task_manager.acquire(self.context, self.node.id) as task:
@ -645,10 +643,12 @@ class NeutronInterfaceTestCase(db_base.DbTestCase):
port1_body['port']['binding:profile'] = {
'local_link_information': [self.port.local_link_connection]
}
port1_body['port']['mac_address'] = '52:54:00:cf:2d:32'
port2_body = copy.deepcopy(expected_body)
port2_body['port']['binding:profile'] = {
'local_link_information': [second_port.local_link_connection]
}
port2_body['port']['mac_address'] = '52:54:00:cf:2d:33'
if is_client_id:
port1_body['port']['extra_dhcp_opts'] = (
[{'opt_name': '61', 'opt_value': client_ids[0]}])
@ -725,12 +725,14 @@ class NeutronInterfaceTestCase(db_base.DbTestCase):
call1_body['port']['binding:profile'] = {
'local_link_information': [self.port.local_link_connection],
}
call1_body['port']['mac_address'] = '52:54:00:cf:2d:32'
call2_body = copy.deepcopy(expected_body)
call2_body['port']['binding:profile'] = {
'local_link_information': [port1.local_link_connection,
port2.local_link_connection],
'local_group_information': local_group_info
}
call2_body['port']['mac_address'] = 'ff:54:00:cf:2d:32'
with task_manager.acquire(self.context, self.node.id) as task:
# Override task.portgroups here, to have ability to check
# that mocked get_local_group_information was called with

View File

@ -178,6 +178,7 @@ class TestAgentDeploy(db_base.DbTestCase):
'driver_info': DRIVER_INFO,
'driver_internal_info': DRIVER_INTERNAL_INFO,
'storage_interface': 'noop',
'network_interface': 'noop'
}
self.node = object_utils.create_test_node(self.context, **n)
self.ports = [
@ -351,6 +352,9 @@ class TestAgentDeploy(db_base.DbTestCase):
storage_detach_volumes_mock):
object_utils.create_test_volume_target(
self.context, node_id=self.node.id)
node = self.node
node.network_interface = 'flat'
node.save()
with task_manager.acquire(
self.context, self.node['uuid'], shared=False) as task:
driver_return = self.driver.tear_down(task)
@ -386,6 +390,9 @@ class TestAgentDeploy(db_base.DbTestCase):
build_instance_info_mock, build_options_mock,
pxe_prepare_ramdisk_mock, storage_driver_info_mock,
storage_attach_volumes_mock):
node = self.node
node.network_interface = 'flat'
node.save()
with task_manager.acquire(
self.context, self.node['uuid'], shared=False) as task:
task.node.provision_state = states.DEPLOYING
@ -601,6 +608,9 @@ class TestAgentDeploy(db_base.DbTestCase):
build_options_mock, pxe_prepare_ramdisk_mock,
validate_net_mock, add_provisioning_net_mock):
self.config(group='agent', manage_agent_boot=False)
node = self.node
node.network_interface = 'flat'
node.save()
with task_manager.acquire(
self.context, self.node['uuid'], shared=False) as task:
task.node.provision_state = states.DEPLOYING
@ -700,6 +710,9 @@ class TestAgentDeploy(db_base.DbTestCase):
add_provisioning_net_mock, storage_driver_info_mock,
storage_attach_volumes_mock, should_write_image_mock):
should_write_image_mock.return_value = False
node = self.node
node.network_interface = 'flat'
node.save()
with task_manager.acquire(
self.context, self.node['uuid'], shared=False) as task:
task.node.provision_state = states.DEPLOYING
@ -750,6 +763,9 @@ class TestAgentDeploy(db_base.DbTestCase):
validate_net_mock,
add_provisioning_net_mock):
mock_write.return_value = False
node = self.node
node.network_interface = 'flat'
node.save()
with task_manager.acquire(
self.context, self.node['uuid'], shared=False) as task:
task.node.provision_state = states.DEPLOYING

View File

@ -68,6 +68,7 @@ class AgentDeployMixinBaseTest(db_base.DbTestCase):
'instance_info': INSTANCE_INFO,
'driver_info': DRIVER_INFO,
'driver_internal_info': DRIVER_INTERNAL_INFO,
'network_interface': 'noop'
}
self.node = object_utils.create_test_node(self.context, **n)
@ -497,9 +498,9 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest):
spec=types.FunctionType)
@mock.patch.object(agent_client.AgentClient, 'power_off',
spec=types.FunctionType)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'remove_provisioning_network', spec_set=True, autospec=True)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'configure_tenant_networks', spec_set=True, autospec=True)
def test_reboot_and_finish_deploy_soft_poweroff_doesnt_complete(
self, configure_tenant_net_mock, remove_provisioning_net_mock,
@ -528,9 +529,9 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest):
@mock.patch.object(manager_utils, 'node_power_action', autospec=True)
@mock.patch.object(agent_client.AgentClient, 'power_off',
spec=types.FunctionType)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'remove_provisioning_network', spec_set=True, autospec=True)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'configure_tenant_networks', spec_set=True, autospec=True)
def test_reboot_and_finish_deploy_soft_poweroff_fails(
self, configure_tenant_net_mock, remove_provisioning_net_mock,
@ -560,9 +561,9 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest):
spec=types.FunctionType)
@mock.patch.object(agent_client.AgentClient, 'power_off',
spec=types.FunctionType)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'remove_provisioning_network', spec_set=True, autospec=True)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'configure_tenant_networks', spec_set=True, autospec=True)
def test_reboot_and_finish_deploy_get_power_state_fails(
self, configure_tenant_net_mock, remove_provisioning_net_mock,
@ -655,9 +656,9 @@ class AgentDeployMixinTest(AgentDeployMixinBaseTest):
spec=types.FunctionType)
@mock.patch.object(agent_client.AgentClient, 'power_off',
spec=types.FunctionType)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'remove_provisioning_network', spec_set=True, autospec=True)
@mock.patch('ironic.drivers.modules.network.flat.FlatNetwork.'
@mock.patch('ironic.drivers.modules.network.noop.NoopNetwork.'
'configure_tenant_networks', spec_set=True, autospec=True)
def test_reboot_and_finish_deploy_power_on_fails(
self, configure_tenant_net_mock, remove_provisioning_net_mock,

View File

@ -0,0 +1,13 @@
---
fixes:
- |
Fixes an issue where Neutron ports would be left with a baremetal MAC
address associated after an instance is deleted from a baremetal host. This
caused problems with MAC address conflicts in follow up deployments to the
same baremetal host. `bug 2004428
<https://storyboard.openstack.org/#!/story/2004428>`_.
- |
Fixes an issue where a flat Neutron port would be left with a host ID
associated with it after an instance is deleted from a baremetal host. This
caused problems with reusing the same port for a new instance as it is
already bound to the old instance.