From 2d76ed6e8a98f995e3693450cc788e70080691d5 Mon Sep 17 00:00:00 2001 From: Matt Riedemann Date: Thu, 14 Sep 2017 18:04:20 -0400 Subject: [PATCH] neutron: handle binding:profile=None during migration The port binding:profile handling in _setup_migration_port_profile and _update_port_binding_for_instance is not handling when the binding:profile is set on the port but the value is None, which results in a NoneType error. Neutron API does not define a format for this field, or whether or not it will be specified on the port, and if so, if it's an empty dict or None, so let's be safe and make sure we handle None. Because of how many places we access the binding:profile on a port, this adds a common helper utility. Co-Authored-By: Eric M Gonzalez Conflicts: nova/network/neutronv2/api.py nova/tests/unit/network/test_neutronv2.py NOTE(mriedem): Conflicts are caused by not having change a67af1f110f160861f5cbbff987766c46d60198a and change ecc8de8d6cccb06d7f4c8ecc144d37612ae1e9cc in Newton. Change-Id: I564bac88fad6cc47ccbf7425b1ab39899fdc1c2e Closes-Bug: #1717365 (cherry picked from commit 8ac7be36bedaf0dd3467efc5b5bffdb365b8231b) (cherry picked from commit c393e490751416f408432ec20e2b1afc1af88af0) (cherry picked from commit b2577207203167d7b8610531bedf0f480a9b7865) --- nova/network/neutronv2/api.py | 23 ++++++-- nova/tests/unit/network/test_neutronv2.py | 65 ++++++++++++++++++++++- 2 files changed, 81 insertions(+), 7 deletions(-) diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index a03953697771..8b7718bc943f 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -73,6 +73,19 @@ def _load_auth_plugin(conf): raise neutron_client_exc.Unauthorized(message=err_msg) +def _get_binding_profile(port): + """Convenience method to get the binding:profile from the port + + The binding:profile in the port is undefined in the networking service + API and is dependent on backend configuration. This means it could be + an empty dict, None, or have some values. + + :param port: dict port response body from the networking service API + :returns: The port binding:profile dict; empty if not set on the port + """ + return port.get(BINDING_PROFILE, {}) or {} + + class ClientWrapper(clientv20.Client): """A Neutron client wrapper class. @@ -264,7 +277,7 @@ class API(base_api.NetworkAPI): # If the port already has a migration profile and if # it is to be torn down, then we need to clean up # the migration profile. - port_profile = p.get(BINDING_PROFILE) + port_profile = _get_binding_profile(p) if not port_profile: continue if MIGRATING_ATTR in port_profile: @@ -285,7 +298,7 @@ class API(base_api.NetworkAPI): # the given 'host'. host_id = p.get(BINDING_HOST_ID) if host_id != host: - port_profile = p.get(BINDING_PROFILE, {}) + port_profile = _get_binding_profile(p) port_profile[MIGRATING_ATTR] = host self._update_port_with_migration_profile( instance, p['id'], port_profile, admin_client) @@ -2120,7 +2133,7 @@ class API(base_api.NetworkAPI): mtu=network_mtu ) network['subnets'] = subnets - port_profile = port.get('binding:profile') + port_profile = _get_binding_profile(port) if port_profile: physical_network = port_profile.get('physical_network') if physical_network: @@ -2219,7 +2232,7 @@ class API(base_api.NetworkAPI): vnic_type=current_neutron_port.get('binding:vnic_type', network_model.VNIC_TYPE_NORMAL), type=current_neutron_port.get('binding:vif_type'), - profile=current_neutron_port.get('binding:profile'), + profile=_get_binding_profile(current_neutron_port), details=current_neutron_port.get('binding:vif_details'), ovs_interfaceid=ovs_interfaceid, devname=devname, @@ -2385,7 +2398,7 @@ class API(base_api.NetworkAPI): ports = data['ports'] for p in ports: updates = {} - binding_profile = p.get(BINDING_PROFILE, {}) + binding_profile = _get_binding_profile(p) # If the host hasn't changed, like in the case of resizing to the # same host, there is nothing to do. diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index d557d2b1e38f..78f24eff54c5 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -2976,8 +2976,12 @@ class TestNeutronv2(TestNeutronv2Base): model.VNIC_TYPE_NORMAL), nw_info['vnic_type']) self.assertEqual(requested_ports[index].get('binding:vif_details'), nw_info.get('details')) - self.assertEqual(requested_ports[index].get('binding:profile'), - nw_info.get('profile')) + self.assertEqual( + # If the requested port does not define a binding:profile, or + # has it set to None, we default to an empty dict to avoid + # NoneType errors. + requested_ports[index].get(neutronapi.BINDING_PROFILE) or {}, + nw_info.get('profile')) index += 1 self.assertFalse(nw_infos[0]['active']) @@ -3770,6 +3774,31 @@ class TestNeutronv2WithMock(test.TestCase): neutronapi.BINDING_PROFILE: { 'fake_profile': 'fake_data'}}}) + @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) + def test_update_port_bindings_for_instance_binding_profile_none( + self, get_client_mock): + """Tests _update_port_binding_for_instance when the binding:profile + value is None. + """ + instance = fake_instance.fake_instance_obj(self.context) + self.api._has_port_binding_extension = mock.Mock(return_value=True) + + fake_ports = {'ports': [ + {'id': uuids.portid, + neutronapi.BINDING_PROFILE: None, + neutronapi.BINDING_HOST_ID: instance.host}]} + list_ports_mock = mock.Mock(return_value=fake_ports) + get_client_mock.return_value.list_ports = list_ports_mock + update_port_mock = mock.Mock() + get_client_mock.return_value.update_port = update_port_mock + + self.api._update_port_binding_for_instance(self.context, instance, + 'my-host') + # Assert that update_port was called on the port with a + # different host but with no binding profile. + update_port_mock.assert_called_once_with( + uuids.portid, {'port': {neutronapi.BINDING_HOST_ID: 'my-host'}}) + @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) def test_update_port_bindings_for_instance_same_host(self, get_client_mock): @@ -4024,6 +4053,38 @@ class TestNeutronv2WithMock(test.TestCase): uuids.port_id, port_data) + @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) + def test_update_port_profile_for_migration_teardown_false_none_profile( + self, get_client_mock): + """Tests setup_networks_on_host when migrating the port to the + destination host and the binding:profile is None in the port. + """ + instance = fake_instance.fake_instance_obj(self.context) + self.api._has_port_binding_extension = mock.Mock(return_value=True) + # We test with an instance host and destination_host where the + # port will be moving but with binding:profile set to None. + get_ports = { + 'ports': [ + {'id': uuids.port_id, + neutronapi.BINDING_HOST_ID: instance.host, + neutronapi.BINDING_PROFILE: None} + ] + } + self.api.list_ports = mock.Mock(return_value=get_ports) + update_port_mock = mock.Mock() + get_client_mock.return_value.update_port = update_port_mock + migrate_profile = {neutronapi.MIGRATING_ATTR: 'my-new-host'} + port_data = { + 'port': { + neutronapi.BINDING_PROFILE: migrate_profile + } + } + self.api.setup_networks_on_host( + self.context, instance, host='my-new-host', teardown=False) + update_port_mock.assert_called_once_with( + uuids.port_id, + port_data) + @mock.patch.object(neutronapi, 'get_client', return_value=mock.Mock()) def test__setup_migration_port_profile_called_on_teardown_false( self, get_client_mock):