Skip to content

Commit 0088fde

Browse files
jonglezbjonglezb
committed
Do not make actual device changes in bind_port()
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
1 parent 5246705 commit 0088fde

File tree

2 files changed

+154
-51
lines changed

2 files changed

+154
-51
lines changed

‎networking_generic_switch/generic_switch_mech.py‎

Lines changed: 34 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from neutron.db import provisioning_blocks
1818
from neutron_lib.api.definitions import portbindings
1919
from neutron_lib.callbacks import resources
20+
from neutron_lib import constants as const
2021
from neutron_lib.plugins.ml2 import api
2122
from oslo_log import log as logging
2223

@@ -340,20 +341,50 @@ def update_port_postcommit(self, context):
340341
state changes that it does not know or care about.
341342
"""
342343
port = context.current
344+
network = context.network.current
343345
if self._is_port_bound(port):
344346
binding_profile = port['binding:profile']
345347
local_link_information = binding_profile.get(
346348
'local_link_information')
347349
if not local_link_information:
348350
return
351+
# Necessary because the "provisioning_complete" event triggers
352+
# an additional call to update_port_postcommit(). We don't
353+
# want to configure the port a second time.
354+
if port['status'] == const.PORT_STATUS_ACTIVE:
355+
LOG.debug("Port %(port_id)s is already active, "
356+
"not doing anything",
357+
{'port_id': port['id']})
358+
return
359+
# If binding has already succeeded, we should have valid links
360+
# at this point, but check just in case.
361+
if not self._is_link_valid(port, network):
362+
return
363+
is_802_3ad = self._is_802_3ad(port)
349364
for link in local_link_information:
365+
port_id = link.get('port_id')
350366
switch_info = link.get('switch_info')
351367
switch_id = link.get('switch_id')
352368
switch = device_utils.get_switch_device(
353369
self.switches, switch_info=switch_info,
354370
ngs_mac_address=switch_id)
355-
if not switch:
356-
return
371+
372+
# If segmentation ID is None, set vlan 1
373+
segmentation_id = network.get('provider:segmentation_id') or 1
374+
LOG.debug("Putting switch port %(switch_port)s on "
375+
"%(switch_info)s in vlan %(segmentation_id)s",
376+
{'switch_port': port_id, 'switch_info': switch_info,
377+
'segmentation_id': segmentation_id})
378+
# Move port to network
379+
if is_802_3ad and hasattr(switch, 'plug_bond_to_network'):
380+
switch.plug_bond_to_network(port_id, segmentation_id)
381+
else:
382+
switch.plug_port_to_network(port_id, segmentation_id)
383+
LOG.info("Successfully plugged port %(port_id)s in segment "
384+
"%(segment_id)s on device %(device)s",
385+
{'port_id': port['id'], 'device': switch_info,
386+
'segment_id': segmentation_id})
387+
357388
provisioning_blocks.provisioning_complete(
358389
context._plugin_context, port['id'], resources.PORT,
359390
GENERIC_SWITCH_ENTITY)
@@ -446,32 +477,7 @@ def bind_port(self, context):
446477
if not self._is_link_valid(port, network):
447478
return
448479

449-
is_802_3ad = self._is_802_3ad(port)
450-
for link in local_link_information:
451-
port_id = link.get('port_id')
452-
switch_info = link.get('switch_info')
453-
switch_id = link.get('switch_id')
454-
switch = device_utils.get_switch_device(
455-
self.switches, switch_info=switch_info,
456-
ngs_mac_address=switch_id)
457-
458-
segments = context.segments_to_bind
459-
# If segmentation ID is None, set vlan 1
460-
segmentation_id = segments[0].get('segmentation_id') or 1
461-
LOG.debug("Putting port %(port_id)s on %(switch_info)s "
462-
"to vlan: %(segmentation_id)s",
463-
{'port_id': port_id, 'switch_info': switch_info,
464-
'segmentation_id': segmentation_id})
465-
# Move port to network
466-
if is_802_3ad and hasattr(switch, 'plug_bond_to_network'):
467-
switch.plug_bond_to_network(port_id, segmentation_id)
468-
else:
469-
switch.plug_port_to_network(port_id, segmentation_id)
470-
LOG.info("Successfully bound port %(port_id)s in segment "
471-
"%(segment_id)s on device %(device)s",
472-
{'port_id': port['id'], 'device': switch_info,
473-
'segment_id': segmentation_id})
474-
480+
segments = context.segments_to_bind
475481
context.set_binding(segments[0][api.ID],
476482
portbindings.VIF_TYPE_OTHER, {})
477483
provisioning_blocks.add_provisioning_component(

‎networking_generic_switch/tests/unit/test_generic_switch_mech.py‎

Lines changed: 120 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,7 @@ def test_delete_port_postcommit_failure(self, m_list):
342342
driver.delete_port_postcommit,
343343
mock_context)
344344

345-
def test_delete_port_potcommit_unknown_switch(self, m_list):
345+
def test_delete_port_postcommit_unknown_switch(self, m_list):
346346
driver = gsm.GenericSwitchDriver()
347347
driver.initialize()
348348
mock_context = mock.create_autospec(driver_context.PortContext)
@@ -408,12 +408,14 @@ def test_update_port_postcommit_not_bound(self, m_pc, m_list):
408408
},
409409
'binding:vnic_type': 'baremetal',
410410
'id': '123',
411-
'binding:vif_type': 'unbound'}
411+
'binding:vif_type': 'unbound',
412+
'status': 'DOWN'}
412413
mock_context.original = {'binding:profile': {},
413414
'binding:vnic_type': 'baremetal',
414415
'id': '123',
415416
'binding:vif_type': 'unbound'}
416417
driver.update_port_postcommit(mock_context)
418+
self.switch_mock.plug_port_to_network.assert_not_called()
417419
self.assertFalse(m_pc.called)
418420

419421
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
@@ -433,12 +435,14 @@ def test_update_port_postcommit_not_baremetal(self, m_pc, m_list):
433435
},
434436
'binding:vnic_type': 'mcvtap',
435437
'id': '123',
436-
'binding:vif_type': 'other'}
438+
'binding:vif_type': 'other',
439+
'status': 'DOWN'}
437440
mock_context.original = {'binding:profile': {},
438441
'binding:vnic_type': 'mcvtap',
439442
'id': '123',
440443
'binding:vif_type': 'unbound'}
441444
driver.update_port_postcommit(mock_context)
445+
self.switch_mock.plug_port_to_network.assert_not_called()
442446
self.assertFalse(m_pc.called)
443447

444448
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
@@ -450,12 +454,14 @@ def test_update_port_postcommit_no_llc(self, m_pc, m_list):
450454
mock_context.current = {'binding:profile': {},
451455
'binding:vnic_type': 'baremetal',
452456
'id': '123',
453-
'binding:vif_type': 'other'}
457+
'binding:vif_type': 'other',
458+
'status': 'DOWN'}
454459
mock_context.original = {'binding:profile': {},
455460
'binding:vnic_type': 'baremetal',
456461
'id': '123',
457462
'binding:vif_type': 'unbound'}
458463
driver.update_port_postcommit(mock_context)
464+
self.switch_mock.plug_port_to_network.assert_not_called()
459465
self.assertFalse(m_pc.called)
460466

461467
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
@@ -475,12 +481,14 @@ def test_update_port_postcommit_not_managed_by_ngs(self, m_pc, m_list):
475481
},
476482
'binding:vnic_type': 'baremetal',
477483
'id': '123',
478-
'binding:vif_type': 'other'}
484+
'binding:vif_type': 'other',
485+
'status': 'DOWN'}
479486
mock_context.original = {'binding:profile': {},
480487
'binding:vnic_type': 'baremetal',
481488
'id': '123',
482489
'binding:vif_type': 'unbound'}
483490
driver.update_port_postcommit(mock_context)
491+
self.switch_mock.plug_port_to_network.assert_not_called()
484492
self.assertFalse(m_pc.called)
485493

486494
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
@@ -500,13 +508,20 @@ def test_update_port_postcommit_complete_provisioning(self, m_pc, m_list):
500508
},
501509
'binding:vnic_type': 'baremetal',
502510
'id': '123',
503-
'binding:vif_type': 'other'}
511+
'binding:vif_type': 'other',
512+
'status': 'DOWN'}
504513
mock_context.original = {'binding:profile': {},
505514
'binding:vnic_type': 'baremetal',
506515
'id': '123',
507516
'binding:vif_type': 'unbound'}
517+
mock_context.network = mock.Mock()
518+
mock_context.network.current = {
519+
'provider:segmentation_id': 42,
520+
'provider:physical_network': 'physnet1'
521+
}
508522
driver.update_port_postcommit(mock_context)
509-
self.switch_mock.plug_port_to_network.assert_not_called()
523+
self.switch_mock.plug_port_to_network.assert_called_once_with(
524+
2222, 42)
510525
m_pc.assert_called_once_with(mock_context._plugin_context,
511526
mock_context.current['id'],
512527
resources.PORT,
@@ -535,13 +550,23 @@ def test_update_portgroup_postcommit_complete_provisioning(self,
535550
},
536551
'binding:vnic_type': 'baremetal',
537552
'id': '123',
538-
'binding:vif_type': 'other'}
553+
'binding:vif_type': 'other',
554+
'status': 'DOWN'}
539555
mock_context.original = {'binding:profile': {},
540556
'binding:vnic_type': 'baremetal',
541557
'id': '123',
542558
'binding:vif_type': 'unbound'}
559+
mock_context.network = mock.Mock()
560+
mock_context.network.current = {
561+
'provider:segmentation_id': 42,
562+
'provider:physical_network': 'physnet1'
563+
}
543564
driver.update_port_postcommit(mock_context)
544-
self.switch_mock.plug_port_to_network.assert_not_called()
565+
self.switch_mock.plug_port_to_network.assert_has_calls(
566+
[mock.call(2222, 42),
567+
mock.call(3333, 42)]
568+
)
569+
self.switch_mock.plug_bond_to_network.assert_not_called()
545570
m_pc.assert_has_calls([mock.call(mock_context._plugin_context,
546571
mock_context.current['id'],
547572
resources.PORT,
@@ -574,18 +599,97 @@ def test_update_portgroup_postcommit_complete_provisioning_802_3ad(self,
574599
},
575600
'binding:vnic_type': 'baremetal',
576601
'id': '123',
577-
'binding:vif_type': 'other'}
602+
'binding:vif_type': 'other',
603+
'status': 'DOWN'}
578604
mock_context.original = {'binding:profile': {},
579605
'binding:vnic_type': 'baremetal',
580606
'id': '123',
581607
'binding:vif_type': 'unbound'}
608+
mock_context.network = mock.Mock()
609+
mock_context.network.current = {
610+
'provider:segmentation_id': 42,
611+
'provider:physical_network': 'physnet1'
612+
}
582613
driver.update_port_postcommit(mock_context)
583-
self.switch_mock.plug_bond_to_network.assert_not_called()
614+
self.switch_mock.plug_bond_to_network.assert_has_calls(
615+
[mock.call(2222, 42),
616+
mock.call(3333, 42)]
617+
)
618+
self.switch_mock.plug_port_to_network.assert_not_called()
584619
m_pc.assert_has_calls([mock.call(mock_context._plugin_context,
585620
mock_context.current['id'],
586621
resources.PORT,
587622
'GENERICSWITCH')])
588623

624+
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
625+
def test_update_port_postcommit_with_physnet(self, m_pc, m_list):
626+
driver = gsm.GenericSwitchDriver()
627+
driver.initialize()
628+
mock_context = mock.create_autospec(driver_context.PortContext)
629+
mock_context._plugin_context = mock.MagicMock()
630+
mock_context.current = {'binding:profile':
631+
{'local_link_information':
632+
[
633+
{
634+
'switch_info': 'foo',
635+
'port_id': 2222
636+
}
637+
]
638+
},
639+
'binding:vnic_type': 'baremetal',
640+
'id': '123',
641+
'binding:vif_type': 'other',
642+
'status': 'DOWN'}
643+
mock_context.original = {'binding:profile': {},
644+
'binding:vnic_type': 'baremetal',
645+
'id': '123',
646+
'binding:vif_type': 'unbound'}
647+
mock_context.network = mock.Mock()
648+
mock_context.network.current = {
649+
'provider:physical_network': 'physnet1'
650+
}
651+
self.switch_mock._get_physical_networks.return_value = ['physnet1']
652+
653+
driver.update_port_postcommit(mock_context)
654+
self.switch_mock.plug_port_to_network.assert_called_once_with(
655+
2222, 1)
656+
m_pc.assert_called_once_with(mock_context._plugin_context,
657+
mock_context.current['id'],
658+
resources.PORT,
659+
'GENERICSWITCH')
660+
661+
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
662+
def test_update_port_postcommit_with_different_physnet(self, m_pc, m_list):
663+
driver = gsm.GenericSwitchDriver()
664+
driver.initialize()
665+
mock_context = mock.create_autospec(driver_context.PortContext)
666+
mock_context._plugin_context = mock.MagicMock()
667+
mock_context.current = {'binding:profile':
668+
{'local_link_information':
669+
[
670+
{
671+
'switch_info': 'foo',
672+
'port_id': 2222
673+
}
674+
]
675+
},
676+
'binding:vnic_type': 'baremetal',
677+
'id': '123',
678+
'binding:vif_type': 'other',
679+
'status': 'DOWN'}
680+
mock_context.original = {'binding:profile': {},
681+
'binding:vnic_type': 'baremetal',
682+
'id': '123',
683+
'binding:vif_type': 'unbound'}
684+
mock_context.network.current = {
685+
'provider:physical_network': 'physnet1'
686+
}
687+
self.switch_mock._get_physical_networks.return_value = ['physnet2']
688+
689+
driver.update_port_postcommit(mock_context)
690+
self.switch_mock.plug_port_to_network.assert_not_called()
691+
m_pc.assert_not_called()
692+
589693
@mock.patch.object(provisioning_blocks, 'provisioning_complete')
590694
def test_update_port_postcommit_unbind_not_bound(self, m_pc, m_list):
591695
driver = gsm.GenericSwitchDriver()
@@ -775,13 +879,12 @@ def test_bind_port(self, m_apc, m_list):
775879
]
776880

777881
driver.bind_port(mock_context)
778-
self.switch_mock.plug_port_to_network.assert_called_once_with(
779-
2222, 1)
780882
mock_context.set_binding.assert_called_with(123, 'other', {})
781883
m_apc.assert_called_once_with(mock_context._plugin_context,
782884
mock_context.current['id'],
783885
resources.PORT,
784886
'GENERICSWITCH')
887+
self.switch_mock.plug_port_to_network.assert_not_called()
785888

786889
@mock.patch.object(provisioning_blocks, 'add_provisioning_component')
787890
def test_bind_portgroup(self, m_apc, m_list):
@@ -815,17 +918,14 @@ def test_bind_portgroup(self, m_apc, m_list):
815918
]
816919

817920
driver.bind_port(mock_context)
818-
self.switch_mock.plug_port_to_network.assert_has_calls(
819-
[mock.call(2222, 1),
820-
mock.call(3333, 1)]
821-
)
822921
mock_context.set_binding.assert_has_calls(
823922
[mock.call(123, 'other', {})]
824923
)
825924
m_apc.assert_has_calls([mock.call(mock_context._plugin_context,
826925
mock_context.current['id'],
827926
resources.PORT,
828927
'GENERICSWITCH')])
928+
self.switch_mock.plug_port_to_network.assert_not_called()
829929

830930
@mock.patch.object(provisioning_blocks, 'add_provisioning_component')
831931
def test_bind_portgroup_802_3ad(self, m_apc, m_list):
@@ -863,17 +963,15 @@ def test_bind_portgroup_802_3ad(self, m_apc, m_list):
863963
]
864964

865965
driver.bind_port(mock_context)
866-
self.switch_mock.plug_bond_to_network.assert_has_calls(
867-
[mock.call(2222, 1),
868-
mock.call(3333, 1)]
869-
)
870966
mock_context.set_binding.assert_has_calls(
871967
[mock.call(123, 'other', {})]
872968
)
873969
m_apc.assert_has_calls([mock.call(mock_context._plugin_context,
874970
mock_context.current['id'],
875971
resources.PORT,
876972
'GENERICSWITCH')])
973+
self.switch_mock.plug_port_to_network.assert_not_called()
974+
self.switch_mock.plug_bond_to_network.assert_not_called()
877975

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

9061004
driver.bind_port(mock_context)
907-
self.switch_mock.plug_port_to_network.assert_called_once_with(
908-
2222, 1)
9091005
mock_context.set_binding.assert_called_with(123, 'other', {})
9101006
m_apc.assert_called_once_with(mock_context._plugin_context,
9111007
mock_context.current['id'],
9121008
resources.PORT,
9131009
'GENERICSWITCH')
1010+
self.switch_mock.plug_port_to_network.assert_not_called()
9141011

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

0 commit comments

Comments
 (0)