feat: add support for Pebble check-failed and check-recovered events #1281
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
This also exposes PebbleCheckEvent, which I don't love, but it works much better for the documentation without repeating a bunch of content.
This was referenced Jul 4, 2024
Merged
|
This is not particularly usable until the Juju and Pebble changes land, but it should be ok to review now (but not merge: we need to wait in case any changes in the Juju/Pebble side have impacts here). |
benhoyt
added a commit
to canonical/pebble
that referenced
this pull request
Jul 5, 2024
…-check (#444) For notices of kind `change-updated`, the notice data will include the `kind` field that is standard for these notices, but also when `kind` is `perform-check` or `recover-check`, it will also include `check-name`, set to the name of the check that is being performed or recovered. This is the Pebble portion of [OP046](https://docs.google.com/document/d/13ItH8l5ZSmmv9WqZXpqjV8EifZU5e3zxINiNLubnC68/edit). It is used by Juju ([PR](juju/juju#17655)) and ops ([PR](canonical/operator#1281)). --------- Co-authored-by: Ben Hoyt <benhoyt@gmail.com>
dimaqq
reviewed
Jul 5, 2024
benhoyt
reviewed
Jul 8, 2024
dimaqq
approved these changes
Jul 12, 2024
Comment on lines
+854
to
+856
| d = super().snapshot() | ||
| d['check_name'] = self.info.name | ||
| return d |
There was a problem hiding this comment.
Super-minor: why d? because it's a dict? delta? data?
There was a problem hiding this comment.
How about:
Suggested change
| d = super().snapshot() | |
| d['check_name'] = self.info.name | |
| return d | |
| return {**super().snapshot(), "check_name": self.info.name} |
There was a problem hiding this comment.
I'm not a fan of the d either, but it's consistent with all of the others. @benhoyt what's your take on this? A separate PR to adjust them all? Have this one be inconsistent? Change them all in this PR as a drive-by? Leave it alone for now?
There was a problem hiding this comment.
I think a separate PR to adjust them all would be fine. Probably dct or snapshot?
There was a problem hiding this comment.
@dimaqq would you like to open one for that? I can do it if not.
There was a problem hiding this comment.
My vote is for an inline dictionary merge, as it obviates local variable entirely, while is just as readable.
dimaqq
reviewed
Jul 16, 2024
jujubot
added a commit
to juju/juju
that referenced
this pull request
Jul 16, 2024
#17655 This creates new `<container-name>-pebble-check-failed` and `<container-name>-pebble-check-failed` hooks, which are triggered by Pebble change-update notices of particular kinds & statuses, with the effect that we'll now respond to Pebble checks reaching the failure threshold, or passing after having reached the threshold, by sending a pebble-check-failed or pebble-check-recovered event to the charm. ## QA steps ### Test the hooks directly: ```shell # Pack and deploy the test charm $ juju add-model t $ cd testcharms/charms/pebble-checks $ charmcraft pack $ juju deploy ./juju-qa-pebble-checks_ubuntu-22.04-amd64.charm --resource ubuntu-image=public.ecr.aws/ubuntu/ubuntu:22.04 # The check is set to succeed if `/trigger/` exists, so make it. $ juju ssh --container ubuntu juju-qa-pebble-checks/0 mkdir /trigger/ $ juju status # unit status should settle to "active" after pebble-ready # Make the check fail $ juju ssh --container ubuntu juju-qa-pebble-checks/0 rmdir /trigger/ $ juju status # will now say status "maintenance" with message "check failed: exec-check" # Make the check pass again $ juju ssh --container ubuntu juju-qa-pebble-checks/0 mkdir /trigger/ $ juju status # will now say status "active" with message "check recovered: exec-check" ``` ### Test the shell test: `./main.sh -p k8s -c minikube sidecar test_pebble_checks` ## Documentation changes prdesc [CHARMTECH-160](https://warthogs.atlassian.net/browse/CHARMTECH-160), [CHARMTECH-161](https://warthogs.atlassian.net/browse/CHARMTECH-161), [CHARMTECH-162](https://warthogs.atlassian.net/browse/CHARMTECH-162) cover updating the juju.is (main and SDK) docs ## Links <!-- Link to all relevant specification, documentation, bug, issue or JIRA card. --> * [OP046](https://docs.google.com/document/d/13ItH8l5ZSmmv9WqZXpqjV8EifZU5e3zxINiNLubnC68/edit) * [juju/charm PR](juju/charm#432) * [ops PR](canonical/operator#1281) * [pebble PR](canonical/pebble#444) * [jhack PR](canonical/jhack#172) * [Scenario PR](canonical/ops-scenario#151) **Jira card:** [CHARMTECH-109](https://warthogs.atlassian.net/browse/CHARMTECH-109) [CHARMTECH-160]: https://warthogs.atlassian.net/browse/CHARMTECH-160?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ
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.
Two new event types are supported (actually getting these events will also require a Juju change):
PebbleChangeFailedEvent- emitted when a Pebble check in a container hits the failure thresholdPebbleChangeRecoveredEvent- emitted when a Pebble check in a container passes after previously hitting the failure thresholdEach event type has a single new attribute (in addition to those inherited from
WorkloadEvent):info- this is a shortcut to Pebble'sget_checkmethod (not otherwise accessible from the charm), but will only query Pebble if more than thenameis requested, and will cache the result on the event instance.Two new enums are added to Pebble, to make it easier to work with these events, particularly with Harness:
ChangeStatus- the possible values forops.pebble.Change.status(note that this is the full list, but we only expect charms to see Doing, Done, and Error for now).ChangeKind- the possible values forops.pebble.Change.kind(again, this is the current full list, but we only expect charms to see perform-check and recover-check at the moment).Harness.pebble_notifyis extended to support emitting the two new events. This requires implementation knowledge (that the check events are triggered by a Pebble notice of type change-updated), but provides minimal support for testing with Harness, and will be supplemented by user-friendly testing in Scenario.The testing Pebble backend gains an implementation for
get_changeandget_checks. These are not used directly here, but will be used in Scenario, and it makes more sense to have the implementation in ops.This is the ops portion of OP046. It requires a corresponding change to Pebble and change to Juju.