Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Plugin reload mechanism #5649

Merged

Conversation

SchrodingersGat
Copy link
Member

@SchrodingersGat SchrodingersGat commented Oct 2, 2023

This PR fixes a long-running issue with the plugin ecosystem.

If a new plugin is installed into the registry (i.e. via the web interface) then the registry gets reloaded - but only for the running process.

If the background worker needs to access the new plugin (e.g. responding to an event) then this would fail - as the background worker has the "old" plugin registry.

This PR addresses this by keeping a hash of the plugins loaded into the registry, and updating a value in the database whenever the hash changes.

When any thread needs to call a plugin function, the registry first checks if the hash matches the latest value in the database. If not, the local registry is reloaded. This happens quite quickly and then the worker thread can proceed to perform the task.

This means that the whole server instance can keep running (without requiring a restart) when a new plugin is activated.

This PR also adds a mutex lock around the plugin registry reload procedure, to provide a safer and simpler reload process.

Edit: This is also the reason that the "auto migration" does not function correctly - the background worker does not know about the new app mixins, and thus cannot do the migrations. Once this is merged, I'll submit a fix for that issue.

Closes #5451

- Wrap reload_plugins with mutex lock
- Add methods for calculating plugin registry hash
- Background worker will correctly reload registry before performing tasks
- Ensures that the background worker plugin regsistry is up  to date
@SchrodingersGat SchrodingersGat added bug Identifies a bug which needs to be addressed plugin Plugin ecosystem labels Oct 2, 2023
@SchrodingersGat SchrodingersGat added this to the 0.13.0 milestone Oct 2, 2023
@netlify
Copy link

netlify bot commented Oct 2, 2023

Deploy Preview for inventree canceled.

Name Link
🔨 Latest commit c0c807f
🔍 Latest deploy log https://app.netlify.com/sites/inventree/deploys/651b55a48e69020008e7f41f

@SchrodingersGat
Copy link
Member Author

@matmair review on this one please :) Much simpler this time around

Copy link
Member

@matmair matmair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much simpler indeed. There are a few points where direct db access is needed, not sure how/if this will affect performance.

InvenTree/plugin/registry.py Show resolved Hide resolved
logger.debug("Checking plugin registry hash")

try:
reg_hash = InvenTreeSetting.get_setting("_PLUGIN_REGISTRY_HASH", "", create=False, cache=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment: seems expensive to not cache this. We could alternatively override the hash in the cache if we recalculate it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with caching is that the various processes have their own cache (not a shared cache) - unless setup with redis. So we run into the same issue again. We have to do a non-cached DB hit to see values updated by the other processes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should adapt the settings functions in the future to optimize for that

@wolflu05
Copy link
Contributor

wolflu05 commented Oct 3, 2023

Sorry, what do I missed? Why can't we reload the registry after installing a plugin?

@SchrodingersGat
Copy link
Member Author

Sorry, what do I missed? Why can't we reload the registry after installing a plugin?

We do reload the registry after installing a plugin - but the background worker does not know that a plugin has been installed. So, this is a mechanism to ensure that the plugin registry is synced across all running workers.

@SchrodingersGat SchrodingersGat merged commit 06eb948 into inventree:master Oct 3, 2023
22 checks passed
@SchrodingersGat SchrodingersGat deleted the plugin-reload-mechanism branch October 3, 2023 22:00
@wolflu05
Copy link
Contributor

wolflu05 commented Oct 4, 2023

Ah, and there is no way to restart the workers from outside?

@SchrodingersGat
Copy link
Member Author

Exactly - see further discussion here - django-q2/django-q2#123

@SchrodingersGat SchrodingersGat added the enhancement This is an suggested enhancement or new feature label Oct 6, 2023
@wolflu05 wolflu05 mentioned this pull request Feb 5, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identifies a bug which needs to be addressed enhancement This is an suggested enhancement or new feature plugin Plugin ecosystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restarting worker process when installing plugin
3 participants