[05:15] <jam> morning all
[07:49] <niemeyer> Morning jam
[07:49] <niemeyer> Morning all
[07:49] <jam> morning niemeyer
[08:09] <mup_> PR operator#236 opened: ops/log.py: Default to DEBUG logging <Created by jameinel> <https://github.com/canonical/operator/pull/236>
[08:14] <niemeyer> jam: Reviewed, thanks
[08:34] <jam> thanks for the review
[08:34]  * jam goes to make lunch
[08:52] <Chipaca> gooooood morning!
[08:52] <niemeyer> Hey hey
[09:04] <stub> Chipaca: https://code.launchpad.net/~stub/interface-pgsql/+git/operator , which needs a better repo home and updating to namespace standards when the standards exist :)
[09:04] <niemeyer> stub: I hope to have this settled before the week is over
[09:05] <stub> Yup, no hurry from my end
[09:05] <niemeyer> stub: We also need to have some kind of group for reviewing interfaces while we try to develop some joint agreement of which directions we want to go
[09:05] <niemeyer> stub: This sounds like the right time for that
[09:06] <niemeyer> stub: That is, now that we're about to have the basic proposal in place for the extension mechanism and the namespacing
[09:06] <niemeyer> stub: Would be happy to have you on board this effort
[09:09] <stub> ok, let me know if I'm needed when you get that far.
[09:11] <niemeyer> stub: Your presence would certainly be good, given your prior experience in the postgres charm and interfaces.. we also need to do a review of these interfaces to evolve our idea of which way we want to go as recommended patterns
[09:11] <niemeyer> stub: Did you inspire the initial implementation on the sketches that were being discussed here, or completely independently?
[09:12] <stub> (and that intereface needs tests, so probably would fail review right now ;) )
[09:12] <niemeyer> stub: Well, that's another area worth talking about.. we've been evolving the Harness, so it would be interesting to know how suitable it is for working with interfaces as well
[09:13] <stub> niemeyer: Somewhat independently... based on some examples (graphana request for comments I recall)
[09:13] <niemeyer> stub: Cool, let's please try to find a slot that works for everyone over the next few days and get that going
[09:14] <niemeyer> stub: I will have the mechanism and namespace proposal in hand by then
[09:15] <Chipaca> stub: thanks!
[09:53] <Chipaca> graaah
[09:53]  * Chipaca hulk-smashes
[09:53] <Chipaca> why would _simulate_event itself clear the charm state
[09:53] <Chipaca> that should be done by tear-down
[09:53] <Chipaca> stoopid stoopid
[09:54] <niemeyer> Me too, but in a good way.. manual k8s deployment seems to work okay
[09:56] <Chipaca> and of course the test charm doesn't load the state before writing it out
[09:56] <Chipaca> it's all worms all the way down
[09:56] <Chipaca> in fact why is this doing it by hand
[10:51] <Chipaca> … D'OH
[10:51] <jam> Chipaca, can you point me at what you're talking about? _simulate_event is not something I'd generally recommend.
[10:51] <Chipaca> truncate() and truncate(0) are *not* the same thing
[10:51] <Chipaca> jam: hi :)
[10:51] <Chipaca> jam: it's in test_main.py
[10:52] <Chipaca> jam: there's a combination of issues that made it hard to figure out for me
[10:52] <Chipaca> jam: the test_main charm nukes its state every time
[10:52] <Chipaca> ie it does not load state, just sets it to default
[10:52] <Chipaca> not helped by doing things by hand instead of using StoredState
[10:53] <Chipaca> then, _read_and_clear state does truncate() instead of truncate(0)
[10:53] <Chipaca> so i fixed one side of that and then was confused by the other side breaking
[10:53] <jam> Chipaca, in each test? I would expect that, otherwise the tests would be coupled and you couldn't just run 1 test. But _simulate_event doesn't, does it?
[10:54] <jam> Chipaca, oh.
[10:54] <Chipaca> jam: in test_main/src/charm.py, in __init__, it just does _state = {}
[10:54] <Chipaca> so no matter how many times dispatch dispatches an event you'll only get the one event in the state
[10:54] <jam> load and then truncate, but don't actually do anything because you just read a bunch of stuff so truncate() is a noop, right?
[10:55] <Chipaca> nah, that bit is fine (in a twisted kind of way), it truncates it so the _next_ call gets the data ok
[10:55] <Chipaca> i wouldn't write it that way but it works (assuming truncate(0) and not truncate() as it was written)
[10:56] <jam> Chipaca, that's what I mean. Plain truncate() is a no-op
[10:56] <Chipaca> ah, yes
[10:56] <jam> you just read the filefor pickle.load()
[10:57] <Chipaca> jam: i'm not sure we're in sync, so let me replay events for you:  i was trying to write a test that made sure that dispatch + a hook that pointed to the same thing only triggered the event once
[10:57] <Chipaca> jam: i wrote the test, and it passed! but that made no sense, and adding prints to the charm code showed me it was processing the events twice
[10:58] <Chipaca> jam: then i noticed that __init__ was not loading the state, just setting it every time, so that's why it was failing
[10:58] <Chipaca> or rather, failing to fail
[10:59] <Chipaca> jam: so i fixed that
[10:59] <Chipaca> jam: and then other tests started failing!
[10:59] <Chipaca> jam: because truncate() instead of truncate(0)
[10:59] <Chipaca> jam: <the end>
[11:00] <Chipaca> i think it needs refactoring to used StoredState at some point, but for now this seems ok to me
[11:00] <Chipaca> not ideal :) but ok
[11:07] <jam> Chipaca, yeah, so we only ever write the state file, we wb which means we always write from the beginning, and we never read it to include more events...
[11:08] <jam> Chipaca, so, between updating the tests to not overwrite the state file on each call, and using StoredState, which do you think you prefer?
[11:10] <jam> Chipaca, I feel like StoredState would be more the way we want the charm written, and means it would get auto loaded and saved rather than explicitly done by the charm
[11:13] <Chipaca> jam: I think we want to rewrite it to use StoredState because people will pick up on this way of doing things if not
[11:13] <jam> indeed
[11:13] <Chipaca> jam: but I think it needs doing in a separate PR as it'll be a big refactor
[11:13] <jam> looking at it right now
[11:13] <Chipaca> jam: ideally _after_ i land this which already touches test_main :-3
[11:17] <jam> Chipaca, so in setUp we end up calling 'exec_module'. Does that exec the module with __name__ == __main__ ? or is it just effectively 'import the module into a namespace' ?
[11:19] <facubatista> Muy buenos días a todos!
[11:20] <jam> morning facubatista
[11:20] <jam> any chance you could look at #230 today?
[11:21] <mup_> PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230>
[11:23] <facubatista> hola jam
[11:23] <facubatista> jam, yes!
[11:23] <jam> great
[11:26] <Chipaca> jam: it does not exec the module with __name__ == __main__, no
[11:26] <jam> Chipaca, k. its essentially just 'import' but return the module as an imported thing, rather than into sys
[11:26] <Chipaca> jam: it execs it with __name__ being the "fully-qualified name of the module"
[11:27] <Chipaca> jam: the import has two parts, loading the code object, and executing it
[11:27] <Chipaca> bah, loading also has two parts, finding it first :)
[11:27] <Chipaca> jam: but, yes
[11:27] <Chipaca> it's 'exec' in the exec-the-keyword sense, not in the exec-the-syscall sense
[11:30] <Chipaca> jam: note the module is already 'loaded' before the exec call, in that self.charm_module is already <module 'charm' from '<...>/test_main/src/charm.py'> before you exec it
[11:31] <jam> Chipaca, presumably it doesn't have any types, etc defined, right?
[11:33] <Chipaca> jam: correct, just a name, a source, a loader, etc
[11:33]  * Chipaca looks
[11:33] <Chipaca> ['__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__']
[11:33] <Chipaca> that's all :)
[11:33] <jam> Chipaca, not critical to my understanding of what is going on. Mostly it is a way to do 'import foo' but not actually pollute your local namespace.
[11:34] <Chipaca> yep
[11:34] <Chipaca> also it doesn't touch sys.modules
[11:34] <Chipaca> not sure why we're not just doing __import__ though ¯\_(ツ)_/¯
[11:35] <Chipaca> (but i haven't looked in detail; there might be some tweak or subtlety i'm not seeing)
[11:35] <Chipaca> ouch, i must go get lunch otherwise it's all meetings all the time and i'll be eaten alive by feral teenagers
[11:35] <jam> Chipaca, So I have a different test that just does 'import' but means i clean out sys.path afterward. I think this is 'think about that other charm thing'
[11:36] <jam> Chipaca, eat hearty
[11:38] <facubatista> jam, why RelationDataContent is a lazymapping? and not grab all the info at once?
[11:38] <jam> facubatista, so I didn't write it, but it reads all the data when you access one attribute. it doesn't read keys individually
[11:38] <jam> it just doesn't read the data if you only write 1 key
[11:39] <facubatista> jam, do you know why?
[11:39] <facubatista> it's an optimization?
[11:39] <jam> facubatista, yeah, I presume it is just to avoid calling relation-get just because you have a relation, but you don't access any data
[11:40] <jam> and the LazyMapping means that once you *do* read something, you don't pay an overhead for every access, it gets once and caches the content
[11:40] <jam> for the lifetime of a hook, the content you read isn't allowed to change, so it is fine to cache.
[11:40] <jam> in Testing, though, we don't want to destroy the whole model and build it back up between steps
[11:40] <jam> so we _invalidate the parts we know we're changing.
[11:42] <facubatista> jam, _invalidate() does not "discards current cache content", but "prevents further caching"?
[11:42] <jam> facubatista, no, it sets _lazy_data to None, so it will be reloaded on next access
[11:43] <facubatista> so it discards current cache?
[11:43] <jam> correct
[11:43] <facubatista> so, let me recap
[11:43] <jam> LazyMapping has a _data attribute that returns _lazy_data if not None, else load _lazy_data
[11:44] <facubatista> I just do rel_data = relation.data.get(entity) ... at this moment, rel_data is "empty", the real info is not loaded yet, so it's not cached, right?
[11:44] <facubatista> it's _lazy_data is still None
[11:45] <jam> facubatista, if that is all you do, yes. If someone has accessed that object before, the data might already be loaded
[11:45] <jam> which is what why we want to _invalidate so we know the new content will be loaded.
[11:46] <facubatista> ok
[11:46] <facubatista> thanks, I understand now
[11:53] <jam> Chipaca, so... I have a rough cut of replacing the STATE_FILE with StoredState. But it turns out we're doing some "naughty" stuff.
[11:54] <jam> Namely, it saves the type(event) for all the events, which then get pickled down to the state file, and are assumed to be reloaded correctly
[11:54] <jam> and then the test dose: "assertEqual(state.events, [EventType])"
[11:54] <jam> where EventType is "from framework import EventType", and state.events was unpickled from the charm state file.
[11:56] <jam> Why are they doing AllHands in the middle of Vienna Product roadmap ...
[11:57]  * facubatista misses real all hands
[11:57] <jam> :)
[11:58] <jam> Yeah. Though to be fair, All-Engineering is about 200 people, which is almost the size of the largest all-hands
[11:58] <facubatista> jam, yeap, but we don't mix to the rest of the company
[11:58] <facubatista> jam, it's not about size, but (content) diversity
[12:02] <facubatista> jam, I added a couple of small details inline in the #230 review; those are probably so small that we may be done with this today
[12:02] <mup_> PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230>
[12:03] <jam> facubatista,             # rel_data may have loaded now-stale data, so _invalidate() it. Note that just
[12:03] <jam>             # accessing rel_data for an entity doesn't load it unless you access an attribute.
[12:03] <jam> ?
[12:04] <jam> The point is to note that the act of wanting to invalidate won't itself read the data, but I understand wanting both comments.
[12:06] <facubatista> jam, but we don't care if it read the data, as we're invalidating it! that is confusing
[12:06] <facubatista> it looks like "oh, so let's not call to invalidate, and for sure will not be reading the data"
[12:06] <jam> facubatista, so the concern that G had, was that if we access rel_data, it looks like we read it, just to invalidate it.
[12:07] <jam> so I'm trying to find a way to express "make sure if it was read, we invalidate it, but we don't force a read here"
[12:07] <facubatista> the point of that invalidate call is to clear *possible previously* loaded cache
[12:09] <facubatista> jam, the "Note that just accessing rel_data for an entity doesn't load it unless you access an attribute." is a good piece of information, maybe confuses to be just around the invalidate() call
[12:51] <jam> facubatista, how about:
[12:51] <jam> rel_data may be caching now-stale data, so _invalidate() it.
[12:51] <jam> Note, this won't cause the data to be loaded if it wasn't already.
[12:53] <facubatista> jam, I like it, may you replace "be caching" with "have cached"?
[12:55] <niemeyer> Hey folks, can we have the standup a bit earlier today?  I have an errand that I need to run this afternoon and it would be helpful if I could do it a bit earlier
[13:03] <jam> sure
[13:03] <jam> facubatista, ^^
[13:03] <jam> niemeyer, we can, though I have a meeting just before it. but it is lower priority
[13:04] <facubatista> niemeyer, otp, what about in 10'?
[13:04] <niemeyer> jam: No worries then.. it's not critical on my end either
[13:04] <niemeyer> facubatista: Let's do at the usual time
[13:04] <facubatista> ack
[13:27]  * Chipaca suddenly realises the time
[13:29] <facubatista> Chipaca, you mean, in an einstanian way?
[13:29] <jam> Time flies like an arrow. Fruit flies like a banana
[13:33] <jam> facubatista, can you check if 230 now matches your expectation?
[13:34] <facubatista> jam, yes
[13:43] <pekkari> Hi, qq, is juju triggering any config-changed after upgrade charm?
[13:48] <Chipaca> pekkari: AFAIK yes, why?
[13:48] <Chipaca> jam: ^
[13:49] <pekkari> thanks Chipaca, just to know, the charm I'm working on was having config rewrites in the upgrade path that I believe are redundant, so if config change comes after, I can just drop that code
[13:51] <Chipaca> pekkari: perhaps relatedly, if the unit is in error state and you force-upgrade the charm, the upgrade-charm isn't triggered but the config-change is (i only know this from notes in our codebase)
[13:52] <pekkari> seems to be a case I wasn't into it yet, as I was just ensuring I can, from a working unit, upgrade the charm, and get to a ocnsistent state, but good to know
[13:52] <jam> pekkari, Chipaca : afaict, on the agent side, it doesn't explicitly say "an upgrade was done, thus I must also do a config changed". However, I think if you do an upgrade, the act of requesting an upgrade might create a new config state, since old version of the charm and new version of the charm may have different fields available.
[13:55] <pekkari> that makes sense, jam, thanks!
[13:56] <jam> pekkari, a quick test with 'juju debug-hooks' says that we do, indeed, trigger a config-changed with upgrade even if you didn't pass '--config' to upgrade-charm.
[13:58] <pekkari> awesome, wiping unneeded code then :)
[14:08] <facubatista> jam, I see that you answered two conversations, but other two are unanswered, and no commit pushed... maybe I'm looking at it wrongly?
[14:40] <Chipaca> jam, niemeyer, if you have a hook and dispatch, and the first one fails, should the second one still run?
[14:46] <facubatista> Chipaca, I'd follow the same reasoning of "if you have two registered callbacks for one event, and the first callback fails, should the second still run?"
[14:47] <Chipaca> facubatista: yeah, and afaict the answer is no
[14:49] <facubatista> Chipaca, I concur
[15:36] <Chipaca> github seems to be having issues
[15:36] <Chipaca> can't comment on a pr right now
[15:36] <Chipaca> i think that's github's way of telling me to take a braek
[15:36]  * Chipaca obeys
[15:46] <facubatista> jajaj
[15:46] <facubatista> Chipaca, that, or they decided to turn it off forever
[16:44] <jam> facubatista, so I tried to push, but I think Chipaca had auto-committed to my branch, so it rejected my push. sorry about that. I didn't see the "rejected" on the push
[16:45] <jam> facubatista, I just confirmed the PR has been updated with my commits. can you look again ?
[16:45] <jam> Chipaca, I agree, whatever fails first stops the second from executing.
[18:12] <Chipaca> of course now there are _three_ comments with the same content
[18:12] <Chipaca> ah well
[18:12] <Chipaca> at least i'm consistent :-D
[18:14] <Chipaca> and travis <-> github is now discombobulated
[18:14] <Chipaca> it's fun all the way down
[18:14] <Chipaca> i shoulda just carried on running :-)
[18:15]  * Chipaca is sure he used to know english
[18:17] <facubatista> "discombobulated"
[18:23] <Chipaca> facubatista: https://en.wiktionary.org/wiki/discombobulated
[18:24] <facubatista> nice
[21:14]  * facubatista eods
[21:19] <Chipaca> facubatista: 👋 have a good one
[21:22] <facubatista> Chipaca, good night!
[21:23]  * facubatista really goes
[23:18]  * Chipaca EODs