Skip to content

Commit

Permalink
Do not make actual device changes in bind_port()
Browse files Browse the repository at this point in the history 
According to the bind_port specification (as summed up in its docstring),
we should not make any change when bind_port() is called, because binding
results might end up not getting committed.

To satisfy this specification, this patch moves actual device
reconfiguration to update_port_postcommit().

This makes the behaviour symmetrical for port creation (handled in
update_port_postcommit) and port deletion (handled in delete_port_postcommit).

In addition, this will make it easier to improve performance of port
creation, see https://bugs.launchpad.net/networking-generic-switch/+bug/2024385

Note that this change introduces a different retry behaviour when we fail
to configure a device.  Since bind_port() is retried several times by
Neutron, we indirectly benefited from these retries, but this is no longer
the case.  However, NGS already has an internal retry mechanism for SSH
connection errors, which should cover most network-related issues.

To sum up, errors that are no longer covered by a retry are the ones that
happen after we successfully connect to the device.  For instance, the
switch port may not exist on the device, or the device could be in an
unexpected state such as a port being currently part of an unexpected
VLAN.  This kind of errors are unlikely to be solved by retrying, so the
new behaviour should be fine and will even allow to return the error much
faster to the end-user.

Related-Bug: #2024385

Change-Id: If4ca9c58d7f30b40992d0f1aee7e915c6978e0ca
  • Loading branch information
@jonglezb
jonglezb committed Jul 4, 2023
1 parent 5246705 commit 0088fde
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 51 deletions.
62 changes: 34 additions & 28 deletions networking_generic_switch/generic_switch_mech.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
from neutron.db import provisioning_blocks
from neutron_lib.api.definitions import portbindings
from neutron_lib.callbacks import resources
from neutron_lib import constants as const
from neutron_lib.plugins.ml2 import api
from oslo_log import log as logging

Expand Down Expand Up @@ -340,20 +341,50 @@ def update_port_postcommit(self, context):
state changes that it does not know or care about.
"""
port = context.current
network = context.network.current
if self._is_port_bound(port):
binding_profile = port['binding:profile']
local_link_information = binding_profile.get(
'local_link_information')
if not local_link_information:
return
# Necessary because the "provisioning_complete" event triggers
# an additional call to update_port_postcommit(). We don't
# want to configure the port a second time.
if port['status'] == const.PORT_STATUS_ACTIVE:
LOG.debug("Port %(port_id)s is already active, "
"not doing anything",
{'port_id': port['id']})
return
# If binding has already succeeded, we should have valid links
# at this point, but check just in case.
if not self._is_link_valid(port, network):
return
is_802_3ad = self._is_802_3ad(port)
for link in local_link_information:
port_id = link.get('port_id')
switch_info = link.get('switch_info')
switch_id = link.get('switch_id')
switch = device_utils.get_switch_device(
self.switches, switch_info=switch_info,
ngs_mac_address=switch_id)
if not switch:
return

# If segmentation ID is None, set vlan 1
segmentation_id = network.get('provider:segmentation_id') or 1
LOG.debug("Putting switch port %(switch_port)s on "
"%(switch_info)s in vlan %(segmentation_id)s",
{'switch_port': port_id, 'switch_info': switch_info,
'segmentation_id': segmentation_id})
# Move port to network
if is_802_3ad and hasattr(switch, 'plug_bond_to_network'):
switch.plug_bond_to_network(port_id, segmentation_id)
else:
switch.plug_port_to_network(port_id, segmentation_id)
LOG.info("Successfully plugged port %(port_id)s in segment "
"%(segment_id)s on device %(device)s",
{'port_id': port['id'], 'device': switch_info,
'segment_id': segmentation_id})

provisioning_blocks.provisioning_complete(
context._plugin_context, port['id'], resources.PORT,
GENERIC_SWITCH_ENTITY)
Expand Down Expand Up @@ -446,32 +477,7 @@ def bind_port(self, context):
if not self._is_link_valid(port, network):
return

is_802_3ad = self._is_802_3ad(port)
for link in local_link_information:
port_id = link.get('port_id')
switch_info = link.get('switch_info')
switch_id = link.get('switch_id')
switch = device_utils.get_switch_device(
self.switches, switch_info=switch_info,
ngs_mac_address=switch_id)

segments = context.segments_to_bind
# If segmentation ID is None, set vlan 1
segmentation_id = segments[0].get('segmentation_id') or 1
LOG.debug("Putting port %(port_id)s on %(switch_info)s "
"to vlan: %(segmentation_id)s",
{'port_id': port_id, 'switch_info': switch_info,
'segmentation_id': segmentation_id})
# Move port to network
if is_802_3ad and hasattr(switch, 'plug_bond_to_network'):
switch.plug_bond_to_network(port_id, segmentation_id)
else:
switch.plug_port_to_network(port_id, segmentation_id)
LOG.info("Successfully bound port %(port_id)s in segment "
"%(segment_id)s on device %(device)s",
{'port_id': port['id'], 'device': switch_info,
'segment_id': segmentation_id})

segments = context.segments_to_bind
context.set_binding(segments[0][api.ID],
portbindings.VIF_TYPE_OTHER, {})
provisioning_blocks.add_provisioning_component(
Expand Down
143 changes: 120 additions & 23 deletions networking_generic_switch/tests/unit/test_generic_switch_mech.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,7 +342,7 @@ def test_delete_port_postcommit_failure(self, m_list):
driver.delete_port_postcommit,
mock_context)

def test_delete_port_potcommit_unknown_switch(self, m_list):
def test_delete_port_postcommit_unknown_switch(self, m_list):
driver = gsm.GenericSwitchDriver()
driver.initialize()
mock_context = mock.create_autospec(driver_context.PortContext)
Expand Down Expand Up @@ -408,12 +408,14 @@ def test_update_port_postcommit_not_bound(self, m_pc, m_list):
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
'binding:vif_type': 'unbound',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
self.assertFalse(m_pc.called)

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
Expand All @@ -433,12 +435,14 @@ def test_update_port_postcommit_not_baremetal(self, m_pc, m_list):
},
'binding:vnic_type': 'mcvtap',
'id': '123',
'binding:vif_type': 'other'}
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'mcvtap',
'id': '123',
'binding:vif_type': 'unbound'}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
self.assertFalse(m_pc.called)

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
Expand All @@ -450,12 +454,14 @@ def test_update_port_postcommit_no_llc(self, m_pc, m_list):
mock_context.current = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other'}
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
self.assertFalse(m_pc.called)

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
Expand All @@ -475,12 +481,14 @@ def test_update_port_postcommit_not_managed_by_ngs(self, m_pc, m_list):
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other'}
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
self.assertFalse(m_pc.called)

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
Expand All @@ -500,13 +508,20 @@ def test_update_port_postcommit_complete_provisioning(self, m_pc, m_list):
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other'}
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
mock_context.network = mock.Mock()
mock_context.network.current = {
'provider:segmentation_id': 42,
'provider:physical_network': 'physnet1'
}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
self.switch_mock.plug_port_to_network.assert_called_once_with(
2222, 42)
m_pc.assert_called_once_with(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
Expand Down Expand Up @@ -535,13 +550,23 @@ def test_update_portgroup_postcommit_complete_provisioning(self,
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other'}
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
mock_context.network = mock.Mock()
mock_context.network.current = {
'provider:segmentation_id': 42,
'provider:physical_network': 'physnet1'
}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
self.switch_mock.plug_port_to_network.assert_has_calls(
[mock.call(2222, 42),
mock.call(3333, 42)]
)
self.switch_mock.plug_bond_to_network.assert_not_called()
m_pc.assert_has_calls([mock.call(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
Expand Down Expand Up @@ -574,18 +599,97 @@ def test_update_portgroup_postcommit_complete_provisioning_802_3ad(self,
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other'}
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
mock_context.network = mock.Mock()
mock_context.network.current = {
'provider:segmentation_id': 42,
'provider:physical_network': 'physnet1'
}
driver.update_port_postcommit(mock_context)
self.switch_mock.plug_bond_to_network.assert_not_called()
self.switch_mock.plug_bond_to_network.assert_has_calls(
[mock.call(2222, 42),
mock.call(3333, 42)]
)
self.switch_mock.plug_port_to_network.assert_not_called()
m_pc.assert_has_calls([mock.call(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
'GENERICSWITCH')])

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
def test_update_port_postcommit_with_physnet(self, m_pc, m_list):
driver = gsm.GenericSwitchDriver()
driver.initialize()
mock_context = mock.create_autospec(driver_context.PortContext)
mock_context._plugin_context = mock.MagicMock()
mock_context.current = {'binding:profile':
{'local_link_information':
[
{
'switch_info': 'foo',
'port_id': 2222
}
]
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
mock_context.network = mock.Mock()
mock_context.network.current = {
'provider:physical_network': 'physnet1'
}
self.switch_mock._get_physical_networks.return_value = ['physnet1']

driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_called_once_with(
2222, 1)
m_pc.assert_called_once_with(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
'GENERICSWITCH')

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
def test_update_port_postcommit_with_different_physnet(self, m_pc, m_list):
driver = gsm.GenericSwitchDriver()
driver.initialize()
mock_context = mock.create_autospec(driver_context.PortContext)
mock_context._plugin_context = mock.MagicMock()
mock_context.current = {'binding:profile':
{'local_link_information':
[
{
'switch_info': 'foo',
'port_id': 2222
}
]
},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'other',
'status': 'DOWN'}
mock_context.original = {'binding:profile': {},
'binding:vnic_type': 'baremetal',
'id': '123',
'binding:vif_type': 'unbound'}
mock_context.network.current = {
'provider:physical_network': 'physnet1'
}
self.switch_mock._get_physical_networks.return_value = ['physnet2']

driver.update_port_postcommit(mock_context)
self.switch_mock.plug_port_to_network.assert_not_called()
m_pc.assert_not_called()

@mock.patch.object(provisioning_blocks, 'provisioning_complete')
def test_update_port_postcommit_unbind_not_bound(self, m_pc, m_list):
driver = gsm.GenericSwitchDriver()
Expand Down Expand Up @@ -775,13 +879,12 @@ def test_bind_port(self, m_apc, m_list):
]

driver.bind_port(mock_context)
self.switch_mock.plug_port_to_network.assert_called_once_with(
2222, 1)
mock_context.set_binding.assert_called_with(123, 'other', {})
m_apc.assert_called_once_with(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
'GENERICSWITCH')
self.switch_mock.plug_port_to_network.assert_not_called()

@mock.patch.object(provisioning_blocks, 'add_provisioning_component')
def test_bind_portgroup(self, m_apc, m_list):
Expand Down Expand Up @@ -815,17 +918,14 @@ def test_bind_portgroup(self, m_apc, m_list):
]

driver.bind_port(mock_context)
self.switch_mock.plug_port_to_network.assert_has_calls(
[mock.call(2222, 1),
mock.call(3333, 1)]
)
mock_context.set_binding.assert_has_calls(
[mock.call(123, 'other', {})]
)
m_apc.assert_has_calls([mock.call(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
'GENERICSWITCH')])
self.switch_mock.plug_port_to_network.assert_not_called()

@mock.patch.object(provisioning_blocks, 'add_provisioning_component')
def test_bind_portgroup_802_3ad(self, m_apc, m_list):
Expand Down Expand Up @@ -863,17 +963,15 @@ def test_bind_portgroup_802_3ad(self, m_apc, m_list):
]

driver.bind_port(mock_context)
self.switch_mock.plug_bond_to_network.assert_has_calls(
[mock.call(2222, 1),
mock.call(3333, 1)]
)
mock_context.set_binding.assert_has_calls(
[mock.call(123, 'other', {})]
)
m_apc.assert_has_calls([mock.call(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
'GENERICSWITCH')])
self.switch_mock.plug_port_to_network.assert_not_called()
self.switch_mock.plug_bond_to_network.assert_not_called()

@mock.patch.object(provisioning_blocks, 'add_provisioning_component')
def test_bind_port_with_physnet(self, m_apc, m_list):
Expand Down Expand Up @@ -904,13 +1002,12 @@ def test_bind_port_with_physnet(self, m_apc, m_list):
self.switch_mock._get_physical_networks.return_value = ['physnet1']

driver.bind_port(mock_context)
self.switch_mock.plug_port_to_network.assert_called_once_with(
2222, 1)
mock_context.set_binding.assert_called_with(123, 'other', {})
m_apc.assert_called_once_with(mock_context._plugin_context,
mock_context.current['id'],
resources.PORT,
'GENERICSWITCH')
self.switch_mock.plug_port_to_network.assert_not_called()

@mock.patch.object(provisioning_blocks, 'add_provisioning_component')
def test_bind_port_unknown_switch(self, m_apc, m_list):
Expand Down

0 comments on commit 0088fde

Please sign in to comment.