Prevent duplicate plugin registrations #3879
Conversation
Plugins were getting registered every time the file was being imported.
|
@LiliDeng LGTM |
|
I'm ok with this change. But I remembered I handled the import logic, only UT may import the same module twice. The normal LISA process only import once. |
@squirrelsc I’m not sure what is going on, but when I was testing the Azure Container Policy extension the Node Hook was being added nearly 3000 times. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the issue of duplicate plugin registrations by adding conditional checks before registering plugin implementations.
- Ensures Azure, Node, and Environment hook implementations are registered only once.
- Introduces a registration guard check in three different modules.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lisa/sut_orchestrator/azure/hooks.py | Registers AzureHookSpecDefaultImpl only if not already registered. |
| lisa/node.py | Registers NodeHookImpl only if not already registered. |
| lisa/environment.py | Registers EnvironmentHookImpl only if not already registered. |
| if not plugin_manager.is_registered(AzureHookSpecDefaultImpl): | ||
| plugin_manager.register(AzureHookSpecDefaultImpl()) |
There was a problem hiding this comment.
Consider refactoring the duplicate registration guard logic into a shared helper function to improve maintainability and reduce code repetition across hook registrations.
| if not plugin_manager.is_registered(AzureHookSpecDefaultImpl): | |
| plugin_manager.register(AzureHookSpecDefaultImpl()) | |
| register_hook_if_not_registered(plugin_manager, AzureHookSpecDefaultImpl) |
Copilot uses AI. Check for mistakes.
| @@ -1231,7 +1231,8 @@ def get_node_information(self, node: Node) -> Dict[str, str]: | |||
|
|
|||
|
|
|||
| plugin_manager.add_hookspecs(NodeHookSpec) | |||
| plugin_manager.register(NodeHookImpl()) | |||
| if not plugin_manager.is_registered(NodeHookImpl): | |||
There was a problem hiding this comment.
Consider refactoring the plugin registration check into a common utility function, as the same pattern is used in multiple modules.
Copilot uses AI. Check for mistakes.
| if not plugin_manager.is_registered(EnvironmentHookImpl): | ||
| plugin_manager.register(EnvironmentHookImpl()) |
There was a problem hiding this comment.
To reduce code duplication, consider extracting the conditional registration logic into a shared method that can be reused across different hook implementations.
| if not plugin_manager.is_registered(EnvironmentHookImpl): | |
| plugin_manager.register(EnvironmentHookImpl()) | |
| register_hook_implementation(EnvironmentHookImpl) |
Copilot uses AI. Check for mistakes.
Plugins were getting registered every time the file was being imported.