diff --git a/neutron/services/trunk/drivers/openvswitch/agent/ovsdb_handler.py b/neutron/services/trunk/drivers/openvswitch/agent/ovsdb_handler.py index 3991d379109..560cd1f756b 100644 --- a/neutron/services/trunk/drivers/openvswitch/agent/ovsdb_handler.py +++ b/neutron/services/trunk/drivers/openvswitch/agent/ovsdb_handler.py @@ -195,6 +195,8 @@ class OVSDBHandler(object): :param port: Parent port dict. """ try: + # TODO(jlibosva): Investigate how to proceed during removal of + # trunk bridge that doesn't have metadata stored. parent_port_id, trunk_id, subport_ids = self._get_trunk_metadata( port) # NOTE(status_police): we do not report changes in trunk status on @@ -242,16 +244,14 @@ class OVSDBHandler(object): subport_ids.append(subport.port_id) try: - self._set_trunk_metadata( + self._update_trunk_metadata( trunk_bridge, parent_port, trunk_id, subport_ids) except RuntimeError: - LOG.error(_LE("Failed to set metadata for trunk %s"), trunk_id) - # NOTE(status_police): Trunk bridge has missing metadata now, it - # will cause troubles during deletion. - # TODO(jlibosva): Investigate how to proceed during removal of - # trunk bridge that doesn't have metadata stored and whether it's - # wise to report DEGRADED status in case we don't have metadata - # present on the bridge. + LOG.error(_LE("Failed to store metadata for trunk %s"), trunk_id) + # NOTE(status_police): Trunk bridge has stale metadata now, it + # might cause troubles during deletion. Signal a DEGRADED status; + # if the user undo/redo the operation things may go back to + # normal. return constants.DEGRADED_STATUS LOG.debug("Added trunk: %s", trunk_id) @@ -270,6 +270,20 @@ class OVSDBHandler(object): {'subport_id': subport_id, 'trunk_id': trunk_id, 'err': te}) + try: + # OVS bridge and port to be determined by _update_trunk_metadata + bridge = None + port = None + self._update_trunk_metadata( + bridge, port, trunk_id, subport_ids, wire=False) + except RuntimeError: + # NOTE(status_police): Trunk bridge has stale metadata now, it + # might cause troubles during deletion. Signal a DEGRADED status; + # if the user undo/redo the operation things may go back to + # normal. + LOG.error(_LE("Failed to store metadata for trunk %s"), trunk_id) + return constants.DEGRADED_STATUS + return self._get_current_status(subport_ids, ids) def report_trunk_status(self, context, trunk_id, status): @@ -349,11 +363,32 @@ class OVSDBHandler(object): """Get trunk metadata from OVS port.""" parent_port_id = ( self.trunk_manager.get_port_uuid_from_external_ids(port)) - trunk_id = port['external_ids']['trunk_id'] - subport_ids = jsonutils.loads(port['external_ids']['subport_ids']) + trunk_id = port['external_ids'].get('trunk_id') + subport_ids = jsonutils.loads( + port['external_ids'].get('subport_ids', '[]')) return parent_port_id, trunk_id, subport_ids + def _update_trunk_metadata(self, trunk_bridge, port, + trunk_id, subport_ids, wire=True): + """Update trunk metadata. + + :param trunk_bridge: OVS trunk bridge. + :param port: OVS parent port. + :param trunk_id: trunk ID. + :param subport_ids: subports affecting the metadata. + :param wire: if True subport_ids are added, otherwise removed. + """ + trunk_bridge = trunk_bridge or ovs_lib.OVSBridge( + utils.gen_trunk_br_name(trunk_id)) + port = port or self._get_parent_port(trunk_bridge) + _port_id, _trunk_id, old_subports = self._get_trunk_metadata(port) + if wire: + new_subports = set(old_subports) | set(subport_ids) + else: + new_subports = set(old_subports) - set(subport_ids) + self._set_trunk_metadata(trunk_bridge, port, trunk_id, new_subports) + def _get_current_status(self, expected_subports, actual_subports): """Return the current status of the trunk. diff --git a/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_driver.py b/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_driver.py index f54ec2e163b..b5e73da7968 100644 --- a/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_driver.py +++ b/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_driver.py @@ -76,7 +76,8 @@ class OvsTrunkSkeletonTest(base.BaseTestCase): for subport in self.subports] self.trunk_manager.add_sub_port.assert_has_calls(expected_calls) - def test_handle_subports_deleted(self): + @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') + def test_handle_subports_deleted(self, br): """Test handler calls into trunk manager for deleting subports.""" self.skeleton.handle_subports(self.subports, events.DELETED) expected_calls = [ diff --git a/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_ovsdb_handler.py b/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_ovsdb_handler.py index 3cda0897eb0..0713171f05f 100644 --- a/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_ovsdb_handler.py +++ b/neutron/tests/unit/services/trunk/drivers/openvswitch/agent/test_ovsdb_handler.py @@ -176,22 +176,26 @@ class TestOVSDBHandler(base.BaseTestCase): side_effect=trunk_manager.TrunkManagerError(error='error')): self.ovsdb_handler.handle_trunk_remove('foo', self.fake_port) - def test_handle_trunk_remove_rpc_failure(self): + @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') + def test_handle_trunk_remove_rpc_failure(self, br): self.ovsdb_handler.trunk_rpc.update_trunk_status = ( oslo_messaging.MessagingException) self.ovsdb_handler.handle_trunk_remove('foo', self.fake_port) @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') def test_wire_subports_for_trunk_trunk_manager_failure(self, br): - trunk_rpc = self.ovsdb_handler.trunk_rpc - trunk_rpc.update_subport_bindings.return_value = self.subport_bindings - self.trunk_manager.add_sub_port.side_effect = ( - trunk_manager.TrunkManagerError(error='error')) + with mock.patch.object( + self.ovsdb_handler, '_update_trunk_metadata') as f: + trunk_rpc = self.ovsdb_handler.trunk_rpc + trunk_rpc.update_subport_bindings.return_value = ( + self.subport_bindings) + self.trunk_manager.add_sub_port.side_effect = ( + trunk_manager.TrunkManagerError(error='error')) - status = self.ovsdb_handler.wire_subports_for_trunk( - None, 'trunk_id', self.fake_subports) - - self.assertEqual(constants.DEGRADED_STATUS, status) + status = self.ovsdb_handler.wire_subports_for_trunk( + None, 'trunk_id', self.fake_subports) + self.assertTrue(f.call_count) + self.assertEqual(constants.DEGRADED_STATUS, status) @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') def test_wire_subports_for_trunk_ovsdb_failure(self, br): @@ -203,12 +207,16 @@ class TestOVSDBHandler(base.BaseTestCase): None, 'trunk_id', self.fake_subports) self.assertEqual(constants.DEGRADED_STATUS, status) - def test_unwire_subports_for_trunk_trunk_manager_failure(self): - self.trunk_manager.remove_sub_port.side_effect = ( - trunk_manager.TrunkManagerError(error='error')) - status = self.ovsdb_handler.unwire_subports_for_trunk( - mock.ANY, ['subport_id']) - self.assertEqual(constants.DEGRADED_STATUS, status) + @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') + def test_unwire_subports_for_trunk_trunk_manager_failure(self, br): + with mock.patch.object( + self.ovsdb_handler, '_update_trunk_metadata') as f: + self.trunk_manager.remove_sub_port.side_effect = ( + trunk_manager.TrunkManagerError(error='error')) + status = self.ovsdb_handler.unwire_subports_for_trunk( + 'foo_trunk_id', ['subport_id']) + self.assertTrue(f.call_count) + self.assertEqual(constants.DEGRADED_STATUS, status) def test__wire_trunk_get_trunk_details_failure(self): self.trunk_manager.get_port_uuid_from_external_ids.side_effect = ( @@ -259,3 +267,40 @@ class TestOVSDBHandler(base.BaseTestCase): self.assertEqual(constants.DEGRADED_STATUS, self.ovsdb_handler._get_current_status( [mock.ANY], [])) + + def _test__update_trunk_metadata_wire_flag(self, mock_br, wire, + external_ids, + subport_ids, + expected_subport_ids): + with mock.patch.object( + self.ovsdb_handler, "_get_parent_port", + return_value={'name': 'foo', 'external_ids': external_ids}): + self.ovsdb_handler._update_trunk_metadata( + None, None, 'foo', subport_ids, wire=wire) + external_ids = mock_br.set_db_attribute.call_args[0][3] + self.assertEqual(1, mock_br.set_db_attribute.call_count) + self.assertEqual( + sorted(expected_subport_ids), + sorted(external_ids['subport_ids'])) + + @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') + def test__update_trunk_metadata_wire(self, br): + mock_br = br.return_value + external_ids = { + 'subport_ids': '["foo_subport_1"]' + } + subport_ids = ['foo_subport_2'] + expected_subport_ids = '["foo_subport_1", "foo_subport_2"]' + self._test__update_trunk_metadata_wire_flag( + mock_br, True, external_ids, subport_ids, expected_subport_ids) + + @mock.patch('neutron.agent.common.ovs_lib.OVSBridge') + def test__update_trunk_metadata_unwire(self, br): + mock_br = br.return_value + external_ids = { + 'subport_ids': '["foo_subport_1", "foo_subport_2"]' + } + subport_ids = ['foo_subport_2'] + expected_subport_ids = '["foo_subport_1"]' + self._test__update_trunk_metadata_wire_flag( + mock_br, False, external_ids, subport_ids, expected_subport_ids)