From 29457631ad0b77b06dee40cb5114f5f4aced129a Mon Sep 17 00:00:00 2001 From: Armando Migliaccio Date: Tue, 20 Sep 2016 19:34:32 -0700 Subject: [PATCH] Update trunk metadata during wire/unwire operations Before this patch metadata on the OVS trunk bridge were not updated during unwiring operations and upon subsequent wire operations old subports were getting wiped from the external_ids field of the OVS parent port. This could lead to OVS ports left behind on the host on trunk removal. This patch ensures that the subports stored as metadata reflect the logical state of the trunk. Closes-bug: #1625875 Change-Id: I9d885b10d84fad43c527d3ca7bc9a37439197fc9 --- .../openvswitch/agent/ovsdb_handler.py | 55 +++++++++++--- .../drivers/openvswitch/agent/test_driver.py | 3 +- .../openvswitch/agent/test_ovsdb_handler.py | 75 +++++++++++++++---- 3 files changed, 107 insertions(+), 26 deletions(-) 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)