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

feat: add Pebble check-failed and check-recovered events #17655

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

tonyandrewmeyer
Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer commented Jul 4, 2024

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:

# 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

Links

Jira card: CHARMTECH-109

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

8 similar comments
@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2024

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

@tonyandrewmeyer tonyandrewmeyer force-pushed the add-pebble-check-events branch from 57649f5 to 65ba6c5 Compare July 4, 2024 20:46
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 <[email protected]>
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Nice work! Couple of minor comments. But obviously the Juju team will have more to say.

worker/uniter/pebblenotices.go Outdated Show resolved Hide resolved
worker/uniter/runner/context/context.go Show resolved Hide resolved
@hpidcock hpidcock added the 3.6 label Jul 5, 2024
@tonyandrewmeyer tonyandrewmeyer force-pushed the add-pebble-check-events branch from fa5dea5 to 5960c2b Compare July 9, 2024 21:43
@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review July 9, 2024 21:46
@hpidcock
Copy link
Member

hpidcock commented Jul 9, 2024

/build

2 similar comments
@hpidcock
Copy link
Member

hpidcock commented Jul 9, 2024

/build

@hpidcock
Copy link
Member

/build

…ebble-check-recovered events

These events are triggered by Pebble notices of type 'change-updated',
when the change kind is 'perform-check' and 'recover-check' and the status
of the change is at the end of the lifecycle (error or done). No other
change-updated notices trigger events.
@tonyandrewmeyer tonyandrewmeyer force-pushed the add-pebble-check-events branch from 8cac7bc to d4db796 Compare July 10, 2024 07:34
@hpidcock
Copy link
Member

/build

Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

Awesome work. Testing passes. Thanks for adding a shell test.

@hpidcock
Copy link
Member

/merge

@hpidcock hpidcock force-pushed the add-pebble-check-events branch from 6259b88 to 2096166 Compare July 16, 2024 05:52
@hpidcock
Copy link
Member

/build

@hpidcock
Copy link
Member

/merge

@jujubot jujubot merged commit f62b8bb into juju:3.6 Jul 16, 2024
21 of 23 checks passed
tonyandrewmeyer added a commit to canonical/operator that referenced this pull request Jul 17, 2024
…1281)

Two new event types are supported (actually getting these events requires Juju 3.6b2 or higher):

* `PebbleChangeFailedEvent` - emitted when a Pebble check in a container
hits the failure threshold
* `PebbleChangeRecoveredEvent` - emitted when a Pebble check in a
container passes after previously hitting the failure threshold

Each event type has a single new attribute (in addition to those
inherited from `WorkloadEvent`):

* `info` - this is a shortcut to Pebble's `get_check` method (not
otherwise accessible from the charm), but will only query Pebble if more
than the `name` is 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 for `ops.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 for `ops.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_notify` is 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_change` and
`get_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](https://docs.google.com/document/d/13ItH8l5ZSmmv9WqZXpqjV8EifZU5e3zxINiNLubnC68/edit).
It requires a [corresponding change to
Pebble](canonical/pebble#444) and [change to
Juju](juju/juju#17655).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants