From 6865f4d9f22d5daa2f07ff9651c2280aed489c8c Mon Sep 17 00:00:00 2001 From: Andreas Scheuring Date: Mon, 29 Aug 2016 17:08:34 +0200 Subject: [PATCH] macvtap: Mech driver detects invalid migration As of today, Macvtap agent allows invalid live migrations from a host with interface mapping 'physnet1:eth1' to a host with mapping 'physnet1:eth2'. Migration places the macvtap on the target on 'eth2' instead of 'eth1'. This fix detects such a migration in the mech driver and lets the portbinding fail. Nova will put the instance in error state. The instance might still be able to access the wrong network it migrated to, as today there is no communication to the agent after a failed binding after a migration. But at least the user is now aware that something went wrong. Change-Id: I92cd6411fe88483b921c92f219afa8e846cc0137 Depends-On: Ib1cc44bf9d01baf4d1f1d26c2a368a5ca7c6ab68 Partial-Bug: #1550400 --- .../macvtap/mech_driver/mech_macvtap.py | 46 ++++++++++++ .../unit/plugins/ml2/_test_mech_agent.py | 6 +- .../macvtap/mech_driver/test_mech_macvtap.py | 75 +++++++++++++++++-- 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/neutron/plugins/ml2/drivers/macvtap/mech_driver/mech_macvtap.py b/neutron/plugins/ml2/drivers/macvtap/mech_driver/mech_macvtap.py index 23aa169167b..0d54090e5fb 100644 --- a/neutron/plugins/ml2/drivers/macvtap/mech_driver/mech_macvtap.py +++ b/neutron/plugins/ml2/drivers/macvtap/mech_driver/mech_macvtap.py @@ -14,6 +14,7 @@ # License for the specific language governing permissions and limitations # under the License. +from neutron._i18n import _LE from neutron_lib import constants from oslo_log import log @@ -54,6 +55,22 @@ class MacvtapMechanismDriver(mech_agent.SimpleAgentMechanismDriverBase): """Macvtap driver vlan transparency support.""" return False + def _is_live_migration(self, context): + # We cannot just check if + # context.original['host_id'] != context.current['host_id'] + # This condition is also true, if nova does a reschedule of a + # instance when something went wrong during spawn. In this case, + # context.original['host_id'] is set to the failed host. + # The only safe way to detect a migration is to look into the binding + # profiles 'migrating_to' attribute, which is set by Nova since patch + # https://review.openstack.org/#/c/275073/. + port_profile = context.original.get(portbindings.PROFILE) + if port_profile and port_profile.get('migrating_to', None): + LOG.debug("Live migration with profile %s detected.", port_profile) + return True + else: + return False + def try_to_bind_segment_for_agent(self, context, segment, agent): if self.check_segment_for_agent(segment, agent): vif_details_segment = self.vif_details @@ -69,6 +86,35 @@ class MacvtapMechanismDriver(mech_agent.SimpleAgentMechanismDriverBase): else: macvtap_src = interface + if self._is_live_migration(context): + # We can use the original port here, as during live migration + # portbinding is done after the migration happened. Nova will + # not do a reschedule of the instance migration if binding + # fails, but just set the instance into error state. + # Due to that we can be sure that the original port is the + # migration source port. + orig_vif_details = context.original[portbindings.VIF_DETAILS] + orig_source = orig_vif_details[ + portbindings.VIF_DETAILS_MACVTAP_SOURCE] + if orig_source != macvtap_src: + source_host = context.original[portbindings.HOST_ID] + target_host = agent['host'] + LOG.error(_LE("Vif binding denied by mechanism driver. " + "MacVTap source device '%(target_dev)s' on " + "the migration target '%(target_host)s'is " + "not equal to device '%(source_dev)s' on " + "the migration source '%(source_host)s. " + "Make sure that the " + "interface mapping of macvtap " + "agent on both hosts is equal " + "for the physical network '%(physnet)s'!"), + {'source_dev': orig_source, + 'target_dev': macvtap_src, + 'target_host': target_host, + 'source_host': source_host, + 'physnet': segment['physical_network']}) + return False + vif_details_segment['physical_interface'] = interface vif_details_segment['macvtap_source'] = macvtap_src vif_details_segment['macvtap_mode'] = MACVTAP_MODE_BRIDGE diff --git a/neutron/tests/unit/plugins/ml2/_test_mech_agent.py b/neutron/tests/unit/plugins/ml2/_test_mech_agent.py index 271620c6391..3e4efdc49bd 100644 --- a/neutron/tests/unit/plugins/ml2/_test_mech_agent.py +++ b/neutron/tests/unit/plugins/ml2/_test_mech_agent.py @@ -41,7 +41,8 @@ class FakeNetworkContext(api.NetworkContext): class FakePortContext(api.PortContext): def __init__(self, agent_type, agents, segments, - vnic_type=portbindings.VNIC_NORMAL): + vnic_type=portbindings.VNIC_NORMAL, + original=None): self._agent_type = agent_type self._agents = agents self._network_context = FakeNetworkContext(segments) @@ -49,6 +50,7 @@ class FakePortContext(api.PortContext): self._bound_segment_id = None self._bound_vif_type = None self._bound_vif_details = None + self._original = original @property def current(self): @@ -57,7 +59,7 @@ class FakePortContext(api.PortContext): @property def original(self): - return None + return self._original or {} @property def status(self): diff --git a/neutron/tests/unit/plugins/ml2/drivers/macvtap/mech_driver/test_mech_macvtap.py b/neutron/tests/unit/plugins/ml2/drivers/macvtap/mech_driver/test_mech_macvtap.py index a07b6a834f0..5d145a1b9d1 100644 --- a/neutron/tests/unit/plugins/ml2/drivers/macvtap/mech_driver/test_mech_macvtap.py +++ b/neutron/tests/unit/plugins/ml2/drivers/macvtap/mech_driver/test_mech_macvtap.py @@ -16,6 +16,7 @@ from neutron_lib import constants from neutron.extensions import portbindings +from neutron.plugins.ml2 import driver_api as api from neutron.plugins.ml2.drivers.macvtap.mech_driver import mech_macvtap from neutron.tests.unit.plugins.ml2 import _test_mech_agent as base @@ -31,9 +32,11 @@ class MacvtapMechanismBaseTestCase(base.AgentMechanismBaseTestCase): BAD_MAPPINGS = {'wrong_physical_network': 'wrong_if'} BAD_CONFIGS = {'interface_mappings': BAD_MAPPINGS} - AGENTS = [{'alive': True, - 'configurations': GOOD_CONFIGS, - 'host': 'host'}] + AGENT = {'alive': True, + 'configurations': GOOD_CONFIGS, + 'host': 'host'} + AGENTS = [AGENT] + AGENTS_DEAD = [{'alive': False, 'configurations': GOOD_CONFIGS, 'host': 'dead_host'}] @@ -55,8 +58,64 @@ class MacvtapMechanismGenericTestCase(MacvtapMechanismBaseTestCase, pass +class MacvtapMechanismMigrationTestCase(object): + # MIGRATION_SEGMENT must be overridden for the specific type being tested + MIGRATION_SEGMENT = None + + MIGRATION_SEGMENTS = [MIGRATION_SEGMENT] + + def test__is_live_migration_true(self): + original = {"binding:profile": {"migrating_to": "host"}} + self._test__is_live_migration(True, original) + + def test__is_live_migration_false(self): + self._test__is_live_migration(False, {}) + + def _test__is_live_migration(self, expected, original): + context = base.FakePortContext(self.AGENT_TYPE, + self.AGENTS, + self.MIGRATION_SEGMENTS, + vnic_type=self.VNIC_TYPE, + original=original) + + self.assertEqual(expected, self.driver._is_live_migration(context)) + + def _test_try_to_bind_segment_for_agent_migration(self, expected, + original): + context = base.FakePortContext(self.AGENT_TYPE, + self.AGENTS, + self.MIGRATION_SEGMENTS, + vnic_type=self.VNIC_TYPE, + original=original) + result = self.driver.try_to_bind_segment_for_agent( + context, self.MIGRATION_SEGMENT, self.AGENT) + self.assertEqual(expected, result) + + def test_try_to_bind_segment_for_agent_migration_abort(self): + original = {"binding:profile": {"migrating_to": "host"}, + "binding:vif_details": {"macvtap_source": "bad_source"}, + "binding:host_id": "source_host"} + self._test_try_to_bind_segment_for_agent_migration(False, original) + + def test_try_to_bind_segment_for_agent_migration_ok(self): + macvtap_src = "fake_if" + seg_id = self.MIGRATION_SEGMENT.get(api.SEGMENTATION_ID) + if seg_id: + # In the vlan case, macvtap source name ends with .vlan_id + macvtap_src += "." + str(seg_id) + original = {"binding:profile": {"migrating_to": "host"}, + "binding:vif_details": {"macvtap_source": macvtap_src}, + "binding:host_id": "source_host"} + self._test_try_to_bind_segment_for_agent_migration(True, original) + + class MacvtapMechanismFlatTestCase(MacvtapMechanismBaseTestCase, - base.AgentMechanismFlatTestCase): + base.AgentMechanismFlatTestCase, + MacvtapMechanismMigrationTestCase): + MIGRATION_SEGMENT = {api.ID: 'flat_segment_id', + api.NETWORK_TYPE: 'flat', + api.PHYSICAL_NETWORK: 'fake_physical_network'} + def test_type_flat_vif_details(self): context = base.FakePortContext(self.AGENT_TYPE, self.AGENTS, @@ -75,7 +134,13 @@ class MacvtapMechanismFlatTestCase(MacvtapMechanismBaseTestCase, class MacvtapMechanismVlanTestCase(MacvtapMechanismBaseTestCase, - base.AgentMechanismVlanTestCase): + base.AgentMechanismVlanTestCase, + MacvtapMechanismMigrationTestCase): + MIGRATION_SEGMENT = {api.ID: 'vlan_segment_id', + api.NETWORK_TYPE: 'vlan', + api.PHYSICAL_NETWORK: 'fake_physical_network', + api.SEGMENTATION_ID: 1234} + def test_type_vlan_vif_details(self): context = base.FakePortContext(self.AGENT_TYPE, self.AGENTS,