-
Notifications
You must be signed in to change notification settings - Fork 18
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
[DPE-4801] Block charm if plugin disable fails due to dependent objects #567
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #567 +/- ##
==========================================
+ Coverage 70.84% 70.92% +0.08%
==========================================
Files 10 10
Lines 2802 2817 +15
Branches 522 525 +3
==========================================
+ Hits 1985 1998 +13
- Misses 718 720 +2
Partials 99 99 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Lucas! Nice UX!
Testing manually, I saw that the charm is not clearing the blocked status after the plugin is enabled.
CREATE EXTENSION IF NOT EXISTS pg_trgm WITH SCHEMA pg_catalog;
CREATE TABLE example(name VARCHAR);
CREATE INDEX example_idx ON example USING gin(name gin_trgm_ops);
juju config postgresql-k8s request_date_style="ISO, DMY"
# Wait for the charm do become blocked with the following message: Cannot disable plugins: Existing objects depend on it. See logs
juju config postgresql-k8s plugin_pg_trgm_enable=True
# Wait for the charm to become idle again. It's still blocked with the same message.
Also, will the charm become active again if the user deletes the index that uses the extension? I think we may need to have at least something that detects that.
Thanks for the feedback @marceloneppel, I addressed the issues in the last commit, LMK if you still encounter them.
Just added a check inside PS: Will add the missing tests once the behavior gets approved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The update-status-hook
may take more than 5 minutes to fire in some environments because it's configurable.
As we don't have an observer like on the VM charm or any other mechanism right now, I think it's fine to keep the logic to set the active status back in the update status for now.
src/charm.py
Outdated
@@ -602,7 +603,7 @@ def _on_config_changed(self, event) -> None: | |||
) | |||
return | |||
|
|||
def enable_disable_extensions(self, database: str = None) -> None: | |||
def enable_disable_extensions(self, database: str = None) -> None: # noqa: C901 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know cyclomatic complexity is annoying, but we should either bump the limit or fit within it, not disable it on individual functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, please add tests. I would go different approach, but up 2 Marcelo here.
P.S. Do we have understanding how expensive enable_disable_extensions()
?
@@ -1239,6 +1255,9 @@ def _on_update_status(self, _) -> None: | |||
return | |||
|
|||
if self._has_blocked_status or self._has_waiting_status: | |||
# If charm was failing to disable plugin, try again (user may have removed the objects) | |||
if self.unit.status.message == EXTENSION_OBJECT_MESSAGE: | |||
self.enable_disable_extensions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to call enable_disable_extensions() on every update-status?
This might slow-down PG with a lot of DBs.... which could be production.
IMHO, we can (should?) go into error
state here and user have to call juju resolve
after disabling of manually enabled extension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only call it if the charm is in blocked status due to existing dependent objects. In normal workflow this shouldn't be an issue AFAICT, since 99.9% of the time the charm won't go inside the if statement.
The problem I see with entering an error
state (besides looking scary for the unsuspecting user) is that the charm doesn´t get the chance to auto-resolve: The most usual use-case (I think) is one where the user manually enables/uses an extension via SQL, and forgets to update juju config. They will then see the blocked status and logs and will go "oops lemme update this config my_plugin_enable=True" which will trigger the config_changed
event and resolve itself.
This check inside juju status is for the (more unlikely) case where the user prefers to delete the objects and disable the extension. Here, if the charm sees that it was blocked due to dependent objects, it will retry removing the extension (in case the user deleted the offending objects).
That's why I would vouch for blocking the charm instead of going into error. @marceloneppel what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After hearing many times that we should avoid the error state, I prefer to use a blocked status here (it explicitly calls for action from the user instead of giving the impression that the charm is failing if it gets into an error state). As Lucas commented, it's scary to see the error status in a charm.
Issue
See #536
Solution
If the charm detects objects that depend on plugins not enabled via config (most likely because the plugin was externally enabled) It will go into blocked state and warn the user.
The user can then decide to either: Delete the objects and proceed with the plugin disabled (without updating the config) or update the juju config option of the plugin as enabled.
Local testing works, Will add unit/integration tests shortly.