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.
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 support for Pebble custom notices #108
feat: add support for Pebble custom notices #108
Changes from 6 commits
3c513a4
61ec65f
db76e53
653e631
b9daa69
38f7071
8071e5f
82da906
050ebac
8161af7
318273e
52c2c93
1c37b67
f9928d0
a35cc2c
0749de2
7ba3d3d
f52d5d9
cb6cbb2
e09c9af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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 ascont.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))
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'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.
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 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.
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.
Edit-to-add: this was cross-posted with Ben's comment above, which is why it overlaps a bit.
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.
I did consider this, but all of the
obj.[type]_event
sugar are currently properties, and so we'd have to haveContainer.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: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.
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 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:
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 (indeferred
). 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 andContainer.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 thancustom_notice_event
.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 agree with that -- that seems the nicest. And I think it's in line with the 7.0 discussion we had?
Oh, yes, good point -- because it could be any notice type (eg:
change-update
in the near future).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.
+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:
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
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.
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.
Making it explicit works I think. I think it would be bound to a container not an event though? Like:
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 thanctx.run(container.notice_event, state)
but not too much, and way more clear about what's happening, and doesn't require thenotice_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.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.
This is a fair point, yes.
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.
nice! I like where this is going.
+1 for this approach from my side.