feat: add support for Pebble custom notices

[tonyandrewmeyer]

Adds support for Pebble custom notices:

• A new PebbleNotice class, which is essentially the same as an ops.pebble.Notice but has default values to make it easier to work with in a testing context.
• Containers have a new notices attribute, which is a list of notices that have been seen prior to the event.
• Containers have a new custom_notice_event property, which is sugar for creating a -pebble-custom-notice event.

It seems like having the custom_notice_event property on the notice (and corresponding changes, like Event would have a notice attribute) would be nicer, but (pebble) Notices don't know which container they are in, but PebbleCustomNoticeEvent does have the container as an attribute. We can't have the PebbleNotice know which container it is in and also have the container be passed the list of notices because that's circular. A PebbleNotice could take the Container as an argument, and could 'register' itself on the container, but that doesn't seem like the Scenario way.

This also forces the sugar to be for the most recent notice (more precisely: the last one in the list) in the container, but that does seem reasonable - it seems like this would be the behaviour in Juju as well.

Another option would to not have the sugar (and Event could take a notice), but it does seem like it's both nicer and more Scenario if it is there. Happy to get feedback on this!

The required version of ops is bumped to 2.10 to get the Pebble custom notices support there (particularly in the mock Pebble client).

There are a few small adjustments to support a second type of pebble/workload event.

I had hoped to have PebbleNotice inherit from ops.pebble.Notice (and _DCBase) They are both frozen dataclasses, but in Scenario it's better if there are suitable default values for everything except the key, but the key isn't the first argument for a pebble Notice, so this generates errors if done simply. Nothing else in Scenario is currently doing this, so it seems ok, but let me know if you want me to explore that more.

[tonyandrewmeyer]

@benhoyt it would be great to get your feedback on this as well, since you're the Pebble King (tm) and also did the custom notices in Juju/ops/Harness.

[tonyandrewmeyer]

By the way, I have every expectation that this might take a few rounds of review since it's my first non-trivial contribution to Scenario 😄.

[PietroPasotti]

Great work!
I have some doubts about the API, and a couple of thoughts here and there.
Let me know what you think :)

[PietroPasotti]

the "is always for the last notice in the list" feels quite arbitrary, can you explain the reason for this choice?

Suppose I want to write a test and parametrize on a list of notices to verify that whatever notice fires first, the charm does X. Would I need to reorder the list on each iteration? Feels like an ugly test.

How about:

"custom_notice_event is by default for the last notice in the list; if you want a different one, you can pass it to the event as
cont.custom_notice_event(notice=my_notice)

(see 'relation-joined' for an event using a similar pattern (which tbh I'm not super happy about, but it turns out sometimes it's handy to parametrize after an event instance has already been generated))

[PietroPasotti]

I'm wondering if life wouldn't be simpler if we grabbed the event from the notice itself, like:

event=scenario.PebbleNotice("foo").event

and have the consistency checker verify that the notice attached to the event is in some container in the state.

[benhoyt]

I can see why Tony chose this -- it's not really arbitrary, as it's kind of how Pebble notices works: when a notice is recorded, the one you get the event about is always the last one in the pebble notices list (most recent). I agree it's a bit implicit, but it seems reasonable give how notices work.

Alternatively we would have to pass the event arg (choose one of Pietro's suggestions above, or do it however we end up doing such things in the v7 Scenario API).

I think we could start with Tony's reasonable default of using the last one. Put it this way: it would be unreasonable for the event to be about anything other than the last (most recent) notice.

[tonyandrewmeyer]

Edit-to-add: this was cross-posted with Ben's comment above, which is why it overlaps a bit.

the "is always for the last notice in the list" feels quite arbitrary, can you explain the reason for this choice?

I got boxed in by the notice needing to know which container it is from and the container wanting to have a list of notices it had, tried a few different approaches and this felt least bad (but still not ideal).

I think this would normally be the case in Juju/Pebble. I believe notices are written to storage, essentially appending them to a (per-container) list. When Juju picks that up and decides if it needs to fire a notice event it would have already handled most of the notices, and then fire off the new one.

However, I think this does fall down if there are multiple new events since Juju last processed the list. Maybe also if an event repeats - I'm not sure if that ends up last in the list or keeps its place but adjusts the count/times.

How about:

"custom_notice_event is by default for the last notice in the list; if you want a different one, you can pass it to the event as cont.custom_notice_event(notice=my_notice)

I did consider this, but all of the obj.[type]_event sugar are currently properties, and so we'd have to have Container.custom_notice_event be a regular method instead, and it feels wrong that it's inconsistent with the rest (but see below).

I could make the notice an Event attribute (that's how I started out, actually), so that you could do:

notice = scenario.PebbleNotice("example.com/path")
container = scenario.Container("mycontainer", notices=[notice])
evt = scenario.Event("mycontainer-pebble-custom-notice", container=container, notice=notice)
ctx.run(evt, state)

This would mean no sugar at all if you want a different notice, but it would at least be possible without reordering the list of notices in the container.

(see 'relation-joined' for an event using a similar pattern (which tbh I'm not super happy about, but it turns out sometimes it's handy to parametrize after an event instance has already been generated))

Ah, I missed the magic of how this happens (the Event.__call__ that lets you recreate the object). That's a neat trick 😄. So it could actually behave like both a property and a method. It is consistent with relation-joined, which is good.

I'm wondering if life wouldn't be simpler if we grabbed the event from the notice itself, like:

event=scenario.PebbleNotice("foo").event

and have the consistency checker verify that the notice attached to the event is in some container in the state.

I did start out with the event sugar property being on the notice itself, which does feel the most natural (and consistent). I then ran into trouble because the notice needs to know which container it's in to be able to snapshot itself. I tried setting that behind the scenes (with a post-init) when the notice was added to a container, but that ended up pretty messy.

You can do this:

container = scenario.Container("mycontainer")
notice = scenario.PebbleNotice("example.com/path", container=container)
container.notices.append(notice)

But that feels like we are violating the "treat these as immutable" guideline.

The Event needs to know the container that the notice is from in order to be able to write the snapshot (in deferred). The event doesn't know anything about the state, so can't really search for the notice in the state's containers. I couldn't figure a way around this, other than either setting the container on the notice (either explicitly or implicitly) or by having the event created off the container.

I think the Container.custom_notice_event being the last in the list and Container.custom_notice_event(notice=x) is the best option. I'll wait to see if @benhoyt has thoughts on this, but otherwise do that.

I also wonder if it should be Container.notice_event rather than custom_notice_event.

[benhoyt]

I think the Container.custom_notice_event being the last in the list and Container.custom_notice_event(notice=x) is the best option. I'll wait to see if @benhoyt has thoughts on this, but otherwise do that.

I agree with that -- that seems the nicest. And I think it's in line with the 7.0 discussion we had?

I also wonder if it should be Container.notice_event rather than custom_notice_event.

Oh, yes, good point -- because it could be any notice type (eg: change-update in the near future).

[PietroPasotti]

+1 from me for the Container.notice_event(notice: [str | PebbleNotice] = None) signature.

The default 'pick the last notice' behaviour on notice=None is still bugging me though.
I think it's unreasonable to expect that people will know how the pebble implementation works (i.e. that the last notice will be picked). I see why the default, but I expect most users will be surprised by this and if there's multiple notices in play, they'd have to dig through the documentation to figure out which notice will be triggered.

Thoughts on how we could mitigate:

# API:
class _BoundNotice:
    event: scenario.Event
    
notice: _BoundNotice = Container.get_notice(name)
last_notice: _BoundNotice = Container.last_notice

# usage:
container = scenario.Container(notices=...)
ctx.run(container.get_notice("canonical.com/foo").event, State())
ctx.run(container.last_notice.event, State())

This way it's always transparent which notice you are referring to.

Side-thought: can't help but notice (pun intended) how this fights with the proposal you had of turning all State data structures into mappings. If Container.notices were a mapping, it'd be weird to refer to its ordering.
Then we could just as well make the notice arg mandatory from the start

class Container(...):
    def notice_event(self, notice:str|PebbleNotice) -> Event: ...
    
container = Container(notices={'canonical.com/foo': foo})
container.notice_event('canonical.com/foo') 

[tonyandrewmeyer]

The default 'pick the last notice' behaviour on notice=None is still bugging me though. I think it's unreasonable to expect that people will know how the pebble implementation works (i.e. that the last notice will be picked). I see why the default, but I expect most users will be surprised by this and if there's multiple notices in play, they'd have to dig through the documentation to figure out which notice will be triggered.

Fair enough. I have never loved it either, just found it the least unlikable option 😄.

I think that if notices were more tightly a Juju concept, then when Juju fired pebble-custom-notice you'd get a view of the state that only had the notices that already existed before the event.notice, and I think in this case it's ok - it's similar to the way that you don't put a charm secret in the state if the charm has no access to it, because from the charm's point of view, it doesn't know anything about it. Notices in the future haven't happened yet (from the point of view of the charm's event handler) so they shouldn't be included in the notices list, and the last one in the list is always the one you're hearing about.

However, Juju and Pebble operate pretty independently, so that's not how things will really happen. In practice, you could easily have a notice and then Juju takes a while to fire off the pebble-custom-notice event and in the meantime there have been x other notices as well. The charm needs to know how to handle this, which means that Scenario needs to be able to model it as well.

I talked this over with @benhoyt today and we agreed with your conclusion above, so let's find something better.

Thoughts on how we could mitigate:

# API:
class _BoundNotice:
    event: scenario.Event
    
notice: _BoundNotice = Container.get_notice(name)
last_notice: _BoundNotice = Container.last_notice

# usage:
container = scenario.Container(notices=...)
ctx.run(container.get_notice("canonical.com/foo").event, State())
ctx.run(container.last_notice.event, State())

This way it's always transparent which notice you are referring to.

Making it explicit works I think. I think it would be bound to a container not an event though? Like:

class _ContainedNotice:
    container: Container
    notice: Notice
    @property
    def event(self) -> Event:
        return Event(..., container=self.container, notice=self.notice)

class Container(...):
    def get_notice(key) -> _ContainedNotice:
        return _ContainedNotice(self, self.notices[key])

# usage
container = scenario.Container(notices=...)
ctx.run(container.get_notice("canonical.com/foo").event, state)

# usage after change updated is added
ctx.run(container.get_notice("123", type=ops.pebble.NoticeType.CHANGE_UPDATE).event, state)

If it's bound to an event, then get_notice has to create an event, which seems a bit odd, and not like e.g. get_container. It's a little more verbose than ctx.run(container.notice_event, state) but not too much, and way more clear about what's happening, and doesn't require the notice_event(notice=x) overload, which is probably good, on balance.

I think we could even leave off the last_notice shortcut until we see people actually needing it.

Side-thought: can't help but notice (pun intended) how this fights with the proposal you had of turning all State data structures into mappings.

I will get back to that as soon as I have a chance, by the way, and have been mulling it over. For this PR I was trying to match the existing Scenario API (assuming that this could make it into 6.x) rather than align with whatever might come in 7.0 (and I was assuming that whatever that ends up being would be generic enough that it could be applied to notices in the same way as containers, secrets, storages, etc).

The main concerns that lead to the 'use mappings' proposal were (a) lists put order into things that have no natural order and following on from that (b) it leads to a bunch of "find the thing I want by number" where the number is "magic" because, from the first point, there is no natural order.

Notices do have a natural order, although I think you're probably meant to ignore that or do sorting yourself based on one of the attributes.

If Container.notices were a mapping, it'd be weird to refer to its ordering.

This is a fair point, yes.

[PietroPasotti]

nice! I like where this is going.
+1 for this approach from my side.

[PietroPasotti]

I'd be more explicit here about what the format is.
<domain name>/<path>

And perhaps add a couple of examples or a link to the 'official spec' if there is one. I don't think the format is the most obvious or intuitive one. What's a url-style domain name doing there??

[tonyandrewmeyer]

I'd be more explicit here about what the format is. <domain name>/<path>

Sure. I just copied this from ops, tbh :)

And perhaps add a couple of examples or a link to the 'official spec' if there is one. I don't think the format is the most obvious or intuitive one. What's a url-style domain name doing there??

I think the closest thing to a (public) official spec would be the README. All it says is:

The key must be in the format mydomain.io/mykey to ensure well-namespaced notice keys.

(That really should not be using mydomain.io).

I don't know any of the backstory behind using domain/path style keys for custom notices other than the "well namespaced" comment there. Maybe @benhoyt could elaborate?

[benhoyt]

Yeah, well-namespaced is the main motivation. The notices are kind of "global" (to the container), so if you have various services running it'll help keep them nicely separated / namespaced. (It's likely overkill for most charms, but anyway.)

[PietroPasotti]

I see the value of 'forcing' a namespace, but still I'm not sure I see why it looks like a domain name.
still, out of scope for this PR. I'm happy with a clearer format specification. Also, do we have a consistency check to verify the formatting?

[benhoyt]

Thanks @tonyandrewmeyer. Something came up so I have a busy day tomorrow, but I hope to be able to review this Thursday or Friday.

[benhoyt]

A few comments, but this looks pretty close -- thanks!

[benhoyt]

I can see why Tony chose this -- it's not really arbitrary, as it's kind of how Pebble notices works: when a notice is recorded, the one you get the event about is always the last one in the pebble notices list (most recent). I agree it's a bit implicit, but it seems reasonable give how notices work.

Alternatively we would have to pass the event arg (choose one of Pietro's suggestions above, or do it however we end up doing such things in the v7 Scenario API).

I think we could start with Tony's reasonable default of using the last one. Put it this way: it would be unreasonable for the event to be about anything other than the last (most recent) notice.

[PietroPasotti]

Shall we get this moving forward again?

[tonyandrewmeyer]

Shall we get this moving forward again?

Yes! Ben was keen on pausing until we had progressed the 7.0 discussions, but I think we are far enough along with those now.

If this aligns with the 7.0 style, then it would be:

notice1 = Notice(...)
notice2 = Notice(...)
container = Container("my-container", can_connect=True, notices={notice1, notice2})
state_in = State(containers={container})

state_out = ctx.run(ctx.on.my_container_pebble_custom_notice(notice=notice2), state_in)

# Presumably you would not ever want to get existing notices, since the charm can't change them, but
# could in theory call `notify()`, although it seems like the workload ought to be doing that, not the charm.
new_notice = state_out.get_container(container).get_notice(key=...)

But for the current main, then it would be:

notice1 = Notice(...)
notice2 = Notice(...)
container = Container("my-container", can_connect=True, notices=[notice1, notice2])
state_in = State(containers=[container])

state_out = ctx.run(container.get_notice(notice2.key), state_in)

new_notice = state_out.containers["my-container].get_notice(...)

I think it would be worth having this PR implement the latter (there are a couple of fixes I need to make, most of which I have locally but haven't pushed), and then a separate PR for the 7.0 branch (probably the same one that changes everything else).

[benhoyt]

Looks reasonable to me, thanks

[tonyandrewmeyer]

@PietroPasotti could you please give this a fresh review?

[PietroPasotti]

looking good! couple of nits, doesn't need a second pass from me :)