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: support for Pebble Notices #1086

Merged
merged 18 commits into from Dec 15, 2023
Merged

Conversation

benhoyt
Copy link
Collaborator

@benhoyt benhoyt commented Dec 7, 2023

This PR implements Ops support for Pebble Notices (described in JU048, with the user ID details in OP042).

This adds a new PebbleNoticeEvent base event type, with PebbleCustomNoticeEvent being the first concrete type -- we'll have more later, such as for warning and change-update notices. These events have a notice attribute which is a LazyNotice instance: if you only access id or type or key it won't require a call to Pebble (I think only accessing key will be common in charms).

In addition, this adds get_notice and get_notices methods to both pebble.Client and model.Container. These return objects of the new Notice type.

The timeconv.parse_duration helper function is needed by Notice.from_dict, to parse Go's time.Duration.String format that Pebble uses for the repeat-after and expire-after fields. Most of the test cases for this were taken from Go's test cases.

This PR does not include Harness support for Pebble Notices. I plan to add that in a follow-up PR soon (using a Harness.pebble_notify function and implementing the get_notice and get_notices test backend functions).

@benhoyt benhoyt changed the title Initial work on Pebble Notices integration feat: support for Pebble Notices Dec 7, 2023
@benhoyt benhoyt marked this pull request as ready for review December 13, 2023 01:54
@@ -73,28 +73,30 @@ def _compute_navigation_tree(context):
# domain name if present. Example entries would be ('py:func', 'int') or
# ('envvar', 'LD_LIBRARY_PATH').
nitpick_ignore = [
('py:class', 'ops.model._AddressDict'),
# Please keep this list sorted alphabetically.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that the only thing I've added here is _NoticeDict, but I sorted the list to make the it easier to scan.

Copy link
Contributor

@tonyandrewmeyer tonyandrewmeyer left a comment

Choose a reason for hiding this comment

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

Looks great to me! A few minor comments/suggestions.

Normally with a significant change like this I like to install it locally and play around with it a bit to get a more holistic feel than from a series of diffs. I haven't done that since (as you mentioned in ~ops-library-development) it's complicated to do that at the moment since this hasn't landed in a Juju release. I generally feel that given how much work there has been on the spec, it's probably unnecessary, but if there is some way to (maybe partially just with pebble but not juju support?) do that, it'd be nice.

ops/_private/timeconv.py Outdated Show resolved Hide resolved
ops/pebble.py Outdated Show resolved Hide resolved
ops/pebble.py Outdated Show resolved Hide resolved
ops/pebble.py Outdated Show resolved Hide resolved
ops/testing.py Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
ops/model.py Outdated Show resolved Hide resolved
test/test_pebble.py Outdated Show resolved Hide resolved
test/test_pebble.py Outdated Show resolved Hide resolved
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.

Yeah looks good, only really comments about the duration parsing stuff. I'm not a python expert, so the rest looks like it follows the model.

ops/_private/timeconv.py Outdated Show resolved Hide resolved
ops/_private/timeconv.py Outdated Show resolved Hide resolved
ops/_private/timeconv.py Outdated Show resolved Hide resolved
@benhoyt
Copy link
Collaborator Author

benhoyt commented Dec 14, 2023

@tonyandrewmeyer:

Normally with a significant change like this I like to install it locally and play around with it a bit to get a more holistic feel than from a series of diffs. I haven't done that since (as you mentioned in ~ops-library-development) it's complicated to do that at the moment since this hasn't landed in a Juju release. I generally feel that given how much work there has been on the spec, it's probably unnecessary, but if there is some way to (maybe partially just with pebble but not juju support?) do that, it'd be nice.

Yeah, good call. I'm testing with the following charm (you can replace src/charm.py in the test charm in this Juju PR with the following code):

#!/usr/bin/env python3
"""Charm to test Pebble Notices."""

import logging

import ops

logger = logging.getLogger(__name__)


class PebbleNoticesCharm(ops.CharmBase):
    """Charm to test Pebble Notices."""

    def __init__(self, *args):
        super().__init__(*args)
        self.framework.observe(self.on["redis"].pebble_ready, self._on_pebble_ready)
        self.framework.observe(self.on["redis"].pebble_custom_notice, self._on_custom_notice)

    def _on_pebble_ready(self, event):
        self.unit.status = ops.ActiveStatus()

    def _on_custom_notice(self, event):
        logger.info(
            f"_on_custom_notice: id={event.notice.id} type={event.notice.type} key={event.notice.key} occurrences={event.notice.occurrences}"
        )

        # Don't include the (arbitrary) ID in the status message
        self.unit.status = ops.MaintenanceStatus(
            f"notice type={event.notice.type} key={event.notice.key}"
        )

        container = event.workload
        notices = container.get_notices(types=[ops.pebble.NoticeType.CUSTOM])
        for i, notice in enumerate(notices):
            logger.info(f"get_notices {i}: {notice}")


if __name__ == "__main__":
    ops.main(PebbleNoticesCharm)

And then I charmcraft pack and deploy as per the "QA steps" on that PR, but between packing and deploying, use this little script to inject my local copy of ops (with the changes in this PR):

#!/usr/bin/env bash

if [ "$#" -lt 2 ]
then
	echo "Inject local copy of Python Operator Framework source into charm"
	echo
    echo "usage: inject-ops.sh file.charm /path/to/ops/dir" >&2
    exit 1
fi

if [ ! -f "$2/framework.py" ]; then
    echo "$2/framework.py not found; arg 2 should be path to 'ops' directory"
    exit 1
fi

set -ex

mkdir inject-ops-tmp
unzip -q $1 -d inject-ops-tmp
rm -rf inject-ops-tmp/venv/ops
cp -r $2 inject-ops-tmp/venv/ops
cd inject-ops-tmp
zip -q -r ../inject-ops-new.charm .
cd ..
rm -rf inject-ops-tmp
rm $1
mv inject-ops-new.charm $1

I've just tested using this and it works end to end. Hope that helps.

Update: Oh, and of course you'll have to check out that Juju PR locally and bootstrap using it. Don't forget make microk8s-operator-update to update the docker image used before bootstrapping.

@tonyandrewmeyer
Copy link
Contributor

Yeah, good call. I'm testing with the following charm (you can replace src/charm.py in the test charm in this Juju PR with the following code):

Thanks! I played around with it a bit and all still seems good, as expected :)

This seems very unlikely to matter in practice, but I like the
algorithm more this way in any case.
@benhoyt benhoyt merged commit 95e325e into canonical:main Dec 15, 2023
18 checks passed
@benhoyt benhoyt deleted the pebble-notices branch December 15, 2023 03:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants