Carefully use a status context on actions #362
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
addyess
changed the title
kwmonroe
approved these changes
Aug 19, 2024
There was a problem hiding this comment.
Thanks @addyess. ruff bits were a bit rough to absorb in context, but it's an overall better charm for it. Invoking the reconciler after a successful active action is 💯
I appreciate the deep dive into solving this upgrade ux; props to Pedro for helping get to the root cause!
| kubernetes_snaps.upgrade_snaps(channel=channel, event=event, control_plane=True) | ||
| if isinstance(charm.unit.status, ops.ActiveStatus): | ||
| # After successful upgrade, reconcile the charm to ensure it is in the desired state | ||
| charm.reconciler.reconcile(event) |
addyess
commented
Aug 19, 2024
Comment on lines
-8
to
+18
| with status.context(event.framework.model.unit): | ||
| kubernetes_snaps.service_restart("snap.kube-apiserver.daemon") | ||
| event.set_results({"api-server": {"status": "restarted"}}) | ||
| kubernetes_snaps.service_restart("snap.kube-controller-manager.daemon") | ||
| event.set_results({"controller-manager": {"status": "restarted"}}) | ||
| kubernetes_snaps.service_restart("snap.kube-scheduler.daemon") | ||
| event.set_results({"kube-scheduler": {"status": "restarted"}}) | ||
| try: | ||
| for service, snap in SERVICES.items(): | ||
| kubernetes_snaps.service_restart(snap) | ||
| event.set_results({service: {"status": "restarted"}}) | ||
| except Exception as e: | ||
| event.fail(str(e)) |
There was a problem hiding this comment.
@kwmonroe in case you missed it -- this runs this action in a try/catch rather than using status.context which would then require us to reconcile again. I think this is a better approach to what the action intends
addyess
commented
Aug 19, 2024
| @@ -132,6 +134,7 @@ def __init__(self, *args): | |||
|
|
|||
| # register charm actions | |||
| actions = [ | |||
| self.on.restart_action, | |||
There was a problem hiding this comment.
This actually REGISTERS the restart action -- it wasn't ever handled by the charm code !?!
addyess
added a commit
that referenced
this pull request
Aug 20, 2024
* Don't use a status context on actions * Giant batch of ruff reformatting * Upgrade-action must use a context then reconcile if successful
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
Addresses LP#2077189
Charm actions shouldn't use
status.contextbecause all that happens is that it ignores all exceptions and sets the unit status to Active/IdleBut the
upgradeaction has to use it sincekubernetes_snapsdepends on using thestatus.addmethod, so we have to carefully manage itDetails
ruffwants to test things harder.status.contextwipes out the charm's unit.statusstatus.contextis used outside of a reconciled action, then we must run the reconciler again