From 8ac7be36bedaf0dd3467efc5b5bffdb365b8231b 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 Change-Id: I564bac88fad6cc47ccbf7425b1ab39899fdc1c2e Closes-Bug: #1717365 --- nova/network/neutronv2/api.py | 23 ++++++-- nova/tests/unit/network/test_neutronv2.py | 64 ++++++++++++++++++++++- 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 1ccee1f330f9..0a1162d16153 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -74,6 +74,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 {} + + @profiler.trace_cls("neutron_api") class ClientWrapper(clientv20.Client): """A Neutron client wrapper class. @@ -260,7 +273,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: @@ -281,7 +294,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) @@ -2204,7 +2217,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: @@ -2271,7 +2284,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, @@ -2491,7 +2504,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 6286ba7a2f9f..309b4160a81b 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -2964,8 +2964,11 @@ class TestNeutronv2(TestNeutronv2Base): self.assertEqual(requested_ports[index].get('binding:vif_details'), nw_info.get('details')) self.assertEqual( - requested_ports[index].get(neutronapi.BINDING_PROFILE), - nw_info.get('profile')) + # 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']) @@ -3882,6 +3885,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): @@ -4135,6 +4163,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):