-
Notifications
You must be signed in to change notification settings - Fork 124
docs: add best practices about status #1689
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
base: main
Are you sure you want to change the base?
Conversation
|
||
Your charm should report its status to Juju. The best approach depends on the charm's complexity: | ||
|
||
- If units don't need to coordinate, use [`self.unit.status`](ops.Unit.status) to report the |
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.
This seems a bit backwards to me: we start by telling them to use self.unit.status
but then say the best practice is not doing that, it's actually using collect_unit_status
. I think we should reverse that: recommend collect_unit_status
with an example, but then say something like "if you need something more fine-grained, you can set the status directly with self.unit.status = ...
".
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.
Agreed. I think I muddle the conversation in daily earlier in the week because I was behind on Matrix messages and didn't realise there were two questions. I think our conclusions were:
collect-*-status
is always the pattern we recommend. It's ok to not use it for simple cases, but we should always prefer it in our docs, even in simple cases like the tutorial, because it's better to start people on that track than potentially confuse them by switching later (and then there's the "when you do pass simple" question).- If your charm is expected to have more than one unit, you probably want to also set the app status rather than leaving it to Juju.
I suggest what we do here is say that you should use collect-unit-status
, except when you need the status to update during the hook, typically setting maintenance status or active status with a degraded service message. The example would be similar, but the active status would be done in a collect_unit_status handler.
|
||
Your charm should report its status to Juju. The best approach depends on the charm's complexity: | ||
|
||
- If units don't need to coordinate, use [`self.unit.status`](ops.Unit.status) to report the |
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.
Agreed. I think I muddle the conversation in daily earlier in the week because I was behind on Matrix messages and didn't realise there were two questions. I think our conclusions were:
collect-*-status
is always the pattern we recommend. It's ok to not use it for simple cases, but we should always prefer it in our docs, even in simple cases like the tutorial, because it's better to start people on that track than potentially confuse them by switching later (and then there's the "when you do pass simple" question).- If your charm is expected to have more than one unit, you probably want to also set the app status rather than leaving it to Juju.
I suggest what we do here is say that you should use collect-unit-status
, except when you need the status to update during the hook, typically setting maintenance status or active status with a degraded service message. The example would be similar, but the active status would be done in a collect_unit_status handler.
self.unit.status = ops.ActiveStatus() | ||
``` | ||
|
||
When your charm code sets `self.unit.status`, Ops immediately sends the unit status to Juju. |
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 think we need to extend this to explain that collect-status updates the status in Juju at the end of the hook.
|
||
When your charm code sets `self.unit.status`, Ops immediately sends the unit status to Juju. | ||
|
||
Juju aggregates the unit statuses into an overall application status. |
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 was going to suggest that this link to Juju to explain how the status is chosen, but then when I found the relevant page I noticed that it's the same as the "see also" below. We probably don't want to link to the same page twice, even if one link is to a specific section. Any ideas on how to indicate that you'll find the info there? If not, I'm fine with this as-is.
```{admonition} Best practice | ||
:class: hint | ||
|
||
In practice, it's often awkward to decide which status to report. Instead of writing |
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 like the best practice notes to be as small as possible, so that they'll be easier to consume in a single list of items to check. What do you think about removing the first sentence?
In your charm code, observe [`collect_app_status`](ops.CharmEvents.collect_app_status) or set | ||
[`self.app.status`](ops.Application.status). |
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 wonder if we can point out here (a small example with a comment?) that the charm can unconditionally observe collect-app-status because it'll only be emitted for the leader. That is, you can do:
framework.observe(self.on.collect_app_status, self._on_app_status)
Rather than needing to do:
if self.unit.is_leader:
framework.observe(self.on.collect_app_status, self._on_app_status)
This PR adds a section called "Handle status" to How to write and structure charm code.
Preview build