From 126d54badc2143720c23b0eaa3019cfd52c4b957 Mon Sep 17 00:00:00 2001 From: Brian Haley Date: Sat, 29 Apr 2023 11:02:38 -0400 Subject: [PATCH] Fix some new pylint "E" warnings After updating pylint, it started emitting additional "E" warnings in some cases, fix them. unsubscriptable-object, unsupported-delete-operation These were associated with the OVN AgentCache code. Instead of using a subscript, create get/delete methods to do the same thing. used-before-assignment Re-factor some code so it's clear to pylint variables are being assigned properly. Trivialfix Change-Id: I4a5ccb7f33465705e59b5274c41db3c371862b1e --- .pylintrc | 3 - neutron/agent/linux/dhcp.py | 5 +- neutron/db/l3_db.py | 2 + neutron/db/l3_hamode_db.py | 2 + .../ml2/drivers/ovn/agent/neutron_agent.py | 10 +-- .../drivers/ovn/mech_driver/mech_driver.py | 7 ++- .../ovn/mech_driver/ovsdb/ovsdb_monitor.py | 2 +- neutron/profiling/profiled_decorator.py | 63 +++++++++---------- .../mech_driver/ovsdb/test_ovsdb_monitor.py | 14 ++--- 9 files changed, 56 insertions(+), 52 deletions(-) diff --git a/.pylintrc b/.pylintrc index 3b6d3f4ba83..7b9676f2bbb 100644 --- a/.pylintrc +++ b/.pylintrc @@ -28,9 +28,6 @@ disable= no-method-argument, no-self-argument, not-an-iterable, - unsubscriptable-object, - unsupported-delete-operation, - used-before-assignment, # "W" Warnings for stylistic problems or minor programming issues abstract-method, arguments-differ, diff --git a/neutron/agent/linux/dhcp.py b/neutron/agent/linux/dhcp.py index 4f19d6cc730..4bdfc98c390 100644 --- a/neutron/agent/linux/dhcp.py +++ b/neutron/agent/linux/dhcp.py @@ -391,10 +391,9 @@ class DhcpLocalProcess(DhcpBase, metaclass=abc.ABCMeta): with open(file_name, 'r') as f: return converter(f.read()) if converter else f.read() except ValueError: - msg = "Unable to convert value in %s" + LOG.debug("Unable to convert value in %s", file_name) except IOError: - msg = "Unable to access %s" - LOG.debug(msg, file_name) + LOG.debug("Unable to access %s", file_name) return None @property diff --git a/neutron/db/l3_db.py b/neutron/db/l3_db.py index 510a23e3411..70f09f66ac0 100644 --- a/neutron/db/l3_db.py +++ b/neutron/db/l3_db.py @@ -1504,6 +1504,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, self._core_plugin, context.elevated(), {'port': port}, check_allow_post=False) + dns_data = None with plugin_utils.delete_port_on_error( self._core_plugin, context.elevated(), external_port['id']),\ @@ -1587,6 +1588,7 @@ class L3_NAT_dbonly_mixin(l3.RouterPluginBase, # raise the underlying exception raise e.errors[0].error + dns_data = None fip = floatingip['floatingip'] with db_api.CONTEXT_WRITER.using(context): floatingip_obj = self._get_floatingip(context, id) diff --git a/neutron/db/l3_hamode_db.py b/neutron/db/l3_hamode_db.py index 4df7bded3cb..5e65371e4aa 100644 --- a/neutron/db/l3_hamode_db.py +++ b/neutron/db/l3_hamode_db.py @@ -503,6 +503,8 @@ class L3_HA_NAT_db_mixin(l3_dvr_db.L3_NAT_with_dvr_db_mixin, self._core_plugin.delete_network(admin_ctx, net.network_id) def safe_delete_ha_network(self, context, ha_network, tenant_id): + # initialize to silence pylint used-before-assignment warning + net_id = None try: # reference the attr inside the try block before we attempt # to delete the network and potentially invalidate the diff --git a/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py b/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py index 222c9e9cce8..a39764804f2 100644 --- a/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py +++ b/neutron/plugins/ml2/drivers/ovn/agent/neutron_agent.py @@ -195,7 +195,7 @@ class MetadataAgent(NeutronAgent): # If ovn-controller is down, then metadata agent is down even # if the metadata-agent binary is updating external_ids. try: - if not AgentCache()[self.chassis_private.name].alive: + if not AgentCache().get(self.chassis_private.name).alive: return False except KeyError: return False @@ -230,7 +230,7 @@ class OVNNeutronAgent(NeutronAgent): # If ovn-controller is down, then OVN Neutron Agent is down even # if the neutron-ovn-agent binary is updating external_ids. try: - if not AgentCache()[self.chassis_private.name].alive: + if not AgentCache().get(self.chassis_private.name).alive: return False except KeyError: return False @@ -257,7 +257,7 @@ class OVNNeutronAgent(NeutronAgent): @utils.SingletonDecorator -class AgentCache: +class AgentCache(object): def __init__(self, driver=None): # This is just to make pylint happy because it doesn't like calls to # AgentCache() with no arguments, despite init only being called the @@ -273,7 +273,7 @@ class AgentCache: _agents = copy.copy(self.agents) return iter(_agents.values()) - def __getitem__(self, key): + def get(self, key): return self.agents[key] def update(self, agent_type, row, clear_down=False): @@ -286,7 +286,7 @@ class AgentCache: self.agents[agent.agent_id] = agent return agent - def __delitem__(self, agent_id): + def delete(self, agent_id): del self.agents[agent_id] def agents_by_chassis_private(self, chassis_private): diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py index 36b8c67e07d..25888dfbaa6 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/mech_driver.py @@ -1359,7 +1359,7 @@ def get_agents(self, context, filters=None, fields=None, _driver=None): def get_agent(self, context, id, fields=None, _driver=None): try: - return n_agent.AgentCache()[id].as_dict() + return n_agent.AgentCache().get(id).as_dict() except KeyError: raise n_exc.agent.AgentNotFound(id=id) @@ -1418,6 +1418,11 @@ def delete_agent(self, context, id, _driver=None): 'SB_Global', '.', 'external_ids', delete_agent=str(id), if_exists=True).execute(check_error=True) + try: + n_agent.AgentCache().delete(id) + except KeyError: + LOG.debug('OVN agent %s has been deleted concurrently', id) + def get_availability_zones(cls, context, _driver, filters=None, fields=None, sorts=None, limit=None, marker=None, diff --git a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py index a08bd034670..d2894ee7e78 100644 --- a/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py +++ b/neutron/plugins/ml2/drivers/ovn/mech_driver/ovsdb/ovsdb_monitor.py @@ -327,7 +327,7 @@ class ChassisAgentDeleteEvent(ChassisAgentEvent): return False def run(self, event, row, old): - del n_agent.AgentCache()[row.external_ids['delete_agent']] + n_agent.AgentCache().delete([row.external_ids['delete_agent']]) class ChassisAgentWriteEvent(ChassisAgentEvent): diff --git a/neutron/profiling/profiled_decorator.py b/neutron/profiling/profiled_decorator.py index 20654263107..69a84aeefed 100644 --- a/neutron/profiling/profiled_decorator.py +++ b/neutron/profiling/profiled_decorator.py @@ -42,41 +42,40 @@ def profile(f): @functools.wraps(f) def profile_wrapper(*args, **kwargs): + if not cfg.CONF.enable_code_profiling: + return f(*args, **kwargs) + profid = "%s.%s" % (f.__module__, f.__name__) + profiler = cProfile.Profile() + start_time = datetime.now() try: - if cfg.CONF.enable_code_profiling: - profid = "%s.%s" % (f.__module__, f.__name__) - profiler = cProfile.Profile() - profiler.enable() - start_time = datetime.now() + profiler.enable() return f(*args, **kwargs) finally: - if cfg.CONF.enable_code_profiling: - profiler.disable() - elapsed_time = datetime.now() - start_time - elapsed_time_ms = (elapsed_time.seconds * 1000.0 + - elapsed_time.microseconds / 1000.0) - stream = io.StringIO() - stats = pstats.Stats(profiler, stream=stream).sort_stats( - SORT_BY) - stats.print_stats(cfg.CONF.code_profiling_calls_to_log) - stats.print_callers(cfg.CONF.code_profiling_calls_to_log) - stats.print_callees(cfg.CONF.code_profiling_calls_to_log) - profiler_info = utils.collect_profiler_info() - if not profiler_info: - profiler_info = {'base_id': 'not available', - 'parent_id': 'not available'} - LOG.debug('os-profiler parent trace-id %(parent_id)s ' - 'trace-id %(trace_id)s %(elapsed_time)7d millisecs ' - 'elapsed for %(method)s(*%(args)s, **%(kwargs)s):' - '\n%(profiler_data)s', - {'parent_id': profiler_info['parent_id'], - 'trace_id': profiler_info['base_id'], - 'elapsed_time': elapsed_time_ms, - 'method': profid, - 'args': args, - 'kwargs': kwargs, - 'profiler_data': stream.getvalue()}) - stream.close() + profiler.disable() + elapsed_time = datetime.now() - start_time + elapsed_time_ms = (elapsed_time.seconds * 1000.0 + + elapsed_time.microseconds / 1000.0) + stream = io.StringIO() + stats = pstats.Stats(profiler, stream=stream).sort_stats(SORT_BY) + stats.print_stats(cfg.CONF.code_profiling_calls_to_log) + stats.print_callers(cfg.CONF.code_profiling_calls_to_log) + stats.print_callees(cfg.CONF.code_profiling_calls_to_log) + profiler_info = utils.collect_profiler_info() + if not profiler_info: + profiler_info = {'base_id': 'not available', + 'parent_id': 'not available'} + LOG.debug('os-profiler parent trace-id %(parent_id)s ' + 'trace-id %(trace_id)s %(elapsed_time)7d millisecs ' + 'elapsed for %(method)s(*%(args)s, **%(kwargs)s):' + '\n%(profiler_data)s', + {'parent_id': profiler_info['parent_id'], + 'trace_id': profiler_info['base_id'], + 'elapsed_time': elapsed_time_ms, + 'method': profid, + 'args': args, + 'kwargs': kwargs, + 'profiler_data': stream.getvalue()}) + stream.close() return profile_wrapper diff --git a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py index 987f40ce87b..1e15019ff50 100644 --- a/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py +++ b/neutron/tests/functional/plugins/ml2/drivers/ovn/mech_driver/ovsdb/test_ovsdb_monitor.py @@ -498,18 +498,18 @@ class TestAgentMonitor(base.TestOVNFunctionalBase): def test_agent_change_controller(self): self.assertEqual(neutron_agent.ControllerGatewayAgent, - type(neutron_agent.AgentCache()[self.chassis_name])) + type(neutron_agent.AgentCache().get(self.chassis_name))) self.sb_api.db_set('Chassis', self.chassis_name, ('other_config', {'ovn-cms-options': ''})).execute(check_error=True) n_utils.wait_until_true(lambda: - neutron_agent.AgentCache()[self.chassis_name]. + neutron_agent.AgentCache().get(self.chassis_name). chassis.other_config['ovn-cms-options'] == '') self.assertEqual(neutron_agent.ControllerAgent, - type(neutron_agent.AgentCache()[self.chassis_name])) + type(neutron_agent.AgentCache().get(self.chassis_name))) def test_agent_updated_at_use_nb_cfg_timestamp(self): def check_agent_ts(): - agent = neutron_agent.AgentCache()[self.chassis_name] + agent = neutron_agent.AgentCache().get(self.chassis_name) chassis_ts = self.sb_api.db_get( 'Chassis_Private', self.chassis_name, 'nb_cfg_timestamp').execute(check_error=True) @@ -532,7 +532,7 @@ class TestAgentMonitor(base.TestOVNFunctionalBase): try: n_utils.wait_until_true(check_agent_ts, timeout=5) except n_utils.WaitTimeout: - agent = neutron_agent.AgentCache()[self.chassis_name] + agent = neutron_agent.AgentCache().get(self.chassis_name) chassis_ts = self.sb_api.db_get( 'Chassis_Private', self.chassis_name, 'nb_cfg_timestamp').execute(check_error=True) @@ -541,14 +541,14 @@ class TestAgentMonitor(base.TestOVNFunctionalBase): def test_agent_restart(self): def check_agent_up(): - agent = neutron_agent.AgentCache()[self.chassis_name] + agent = neutron_agent.AgentCache().get(self.chassis_name) return agent.alive def check_agent_down(): return not check_agent_up() def check_nb_cfg_timestamp_is_not_null(): - agent = neutron_agent.AgentCache()[self.chassis_name] + agent = neutron_agent.AgentCache().get(self.chassis_name) return agent.updated_at != 0 if not self.sb_api.is_table_present('Chassis_Private'):