[05:15] morning all [07:49] Morning jam [07:49] Morning all [07:49] morning niemeyer [08:09] PR operator#236 opened: ops/log.py: Default to DEBUG logging [08:14] jam: Reviewed, thanks [08:34] thanks for the review [08:34] * jam goes to make lunch [08:52] gooooood morning! [08:52] Hey hey [09:04] 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] stub: I hope to have this settled before the week is over [09:05] Yup, no hurry from my end [09:05] 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] stub: This sounds like the right time for that [09:06] stub: That is, now that we're about to have the basic proposal in place for the extension mechanism and the namespacing [09:06] stub: Would be happy to have you on board this effort [09:09] ok, let me know if I'm needed when you get that far. [09:11] 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] stub: Did you inspire the initial implementation on the sketches that were being discussed here, or completely independently? [09:12] (and that intereface needs tests, so probably would fail review right now ;) ) [09:12] 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] niemeyer: Somewhat independently... based on some examples (graphana request for comments I recall) [09:13] 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] stub: I will have the mechanism and namespace proposal in hand by then [09:15] stub: thanks! [09:53] graaah [09:53] * Chipaca hulk-smashes [09:53] why would _simulate_event itself clear the charm state [09:53] that should be done by tear-down [09:53] stoopid stoopid [09:54] Me too, but in a good way.. manual k8s deployment seems to work okay [09:56] and of course the test charm doesn't load the state before writing it out [09:56] it's all worms all the way down [09:56] in fact why is this doing it by hand [10:51] … D'OH [10:51] Chipaca, can you point me at what you're talking about? _simulate_event is not something I'd generally recommend. [10:51] truncate() and truncate(0) are *not* the same thing [10:51] jam: hi :) [10:51] jam: it's in test_main.py [10:52] jam: there's a combination of issues that made it hard to figure out for me [10:52] jam: the test_main charm nukes its state every time [10:52] ie it does not load state, just sets it to default [10:52] not helped by doing things by hand instead of using StoredState [10:53] then, _read_and_clear state does truncate() instead of truncate(0) [10:53] so i fixed one side of that and then was confused by the other side breaking [10:53] 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] Chipaca, oh. [10:54] jam: in test_main/src/charm.py, in __init__, it just does _state = {} [10:54] so no matter how many times dispatch dispatches an event you'll only get the one event in the state [10:54] 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] nah, that bit is fine (in a twisted kind of way), it truncates it so the _next_ call gets the data ok [10:55] i wouldn't write it that way but it works (assuming truncate(0) and not truncate() as it was written) [10:56] Chipaca, that's what I mean. Plain truncate() is a no-op [10:56] ah, yes [10:56] you just read the filefor pickle.load() [10:57] 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] 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] 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] or rather, failing to fail [10:59] jam: so i fixed that [10:59] jam: and then other tests started failing! [10:59] jam: because truncate() instead of truncate(0) [10:59] jam: [11:00] i think it needs refactoring to used StoredState at some point, but for now this seems ok to me [11:00] not ideal :) but ok [11:07] 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] 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] 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] 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] indeed [11:13] jam: but I think it needs doing in a separate PR as it'll be a big refactor [11:13] looking at it right now [11:13] jam: ideally _after_ i land this which already touches test_main :-3 [11:17] 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] Muy buenos días a todos! [11:20] morning facubatista [11:20] any chance you could look at #230 today? [11:21] PR #230: Testing harness no events for own data [11:23] hola jam [11:23] jam, yes! [11:23] great [11:26] jam: it does not exec the module with __name__ == __main__, no [11:26] Chipaca, k. its essentially just 'import' but return the module as an imported thing, rather than into sys [11:26] jam: it execs it with __name__ being the "fully-qualified name of the module" [11:27] jam: the import has two parts, loading the code object, and executing it [11:27] bah, loading also has two parts, finding it first :) [11:27] jam: but, yes [11:27] it's 'exec' in the exec-the-keyword sense, not in the exec-the-syscall sense [11:30] jam: note the module is already 'loaded' before the exec call, in that self.charm_module is already /test_main/src/charm.py'> before you exec it [11:31] Chipaca, presumably it doesn't have any types, etc defined, right? [11:33] jam: correct, just a name, a source, a loader, etc [11:33] * Chipaca looks [11:33] ['__cached__', '__doc__', '__file__', '__loader__', '__name__', '__package__', '__spec__'] [11:33] that's all :) [11:33] 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] yep [11:34] also it doesn't touch sys.modules [11:34] not sure why we're not just doing __import__ though ¯\_(ツ)_/¯ [11:35] (but i haven't looked in detail; there might be some tweak or subtlety i'm not seeing) [11:35] 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] 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] Chipaca, eat hearty [11:38] jam, why RelationDataContent is a lazymapping? and not grab all the info at once? [11:38] 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] it just doesn't read the data if you only write 1 key [11:39] jam, do you know why? [11:39] it's an optimization? [11:39] 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] 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] for the lifetime of a hook, the content you read isn't allowed to change, so it is fine to cache. [11:40] in Testing, though, we don't want to destroy the whole model and build it back up between steps [11:40] so we _invalidate the parts we know we're changing. [11:42] jam, _invalidate() does not "discards current cache content", but "prevents further caching"? [11:42] facubatista, no, it sets _lazy_data to None, so it will be reloaded on next access [11:43] so it discards current cache? [11:43] correct [11:43] so, let me recap [11:43] LazyMapping has a _data attribute that returns _lazy_data if not None, else load _lazy_data [11:44] 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] it's _lazy_data is still None [11:45] facubatista, if that is all you do, yes. If someone has accessed that object before, the data might already be loaded [11:45] which is what why we want to _invalidate so we know the new content will be loaded. [11:46] ok [11:46] thanks, I understand now [11:53] 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] 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] and then the test dose: "assertEqual(state.events, [EventType])" [11:54] where EventType is "from framework import EventType", and state.events was unpickled from the charm state file. [11:56] Why are they doing AllHands in the middle of Vienna Product roadmap ... [11:57] * facubatista misses real all hands [11:57] :) [11:58] Yeah. Though to be fair, All-Engineering is about 200 people, which is almost the size of the largest all-hands [11:58] jam, yeap, but we don't mix to the rest of the company [11:58] jam, it's not about size, but (content) diversity [12:02] 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] PR #230: Testing harness no events for own data [12:03] facubatista, # rel_data may have loaded now-stale data, so _invalidate() it. Note that just [12:03] # accessing rel_data for an entity doesn't load it unless you access an attribute. [12:03] ? [12:04] 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] jam, but we don't care if it read the data, as we're invalidating it! that is confusing [12:06] it looks like "oh, so let's not call to invalidate, and for sure will not be reading the data" [12:06] 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] 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] the point of that invalidate call is to clear *possible previously* loaded cache [12:09] 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] facubatista, how about: [12:51] rel_data may be caching now-stale data, so _invalidate() it. [12:51] Note, this won't cause the data to be loaded if it wasn't already. [12:53] jam, I like it, may you replace "be caching" with "have cached"? [12:55] 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] sure [13:03] facubatista, ^^ [13:03] niemeyer, we can, though I have a meeting just before it. but it is lower priority [13:04] niemeyer, otp, what about in 10'? [13:04] jam: No worries then.. it's not critical on my end either [13:04] facubatista: Let's do at the usual time [13:04] ack [13:27] * Chipaca suddenly realises the time [13:29] Chipaca, you mean, in an einstanian way? [13:29] Time flies like an arrow. Fruit flies like a banana [13:33] facubatista, can you check if 230 now matches your expectation? [13:34] jam, yes [13:43] Hi, qq, is juju triggering any config-changed after upgrade charm? [13:48] pekkari: AFAIK yes, why? [13:48] jam: ^ [13:49] 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] 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] 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] 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] that makes sense, jam, thanks! [13:56] 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] awesome, wiping unneeded code then :) [14:08] 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] jam, niemeyer, if you have a hook and dispatch, and the first one fails, should the second one still run? [14:46] 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] facubatista: yeah, and afaict the answer is no [14:49] Chipaca, I concur [15:36] github seems to be having issues [15:36] can't comment on a pr right now [15:36] i think that's github's way of telling me to take a braek [15:36] * Chipaca obeys [15:46] jajaj [15:46] Chipaca, that, or they decided to turn it off forever [16:44] 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] facubatista, I just confirmed the PR has been updated with my commits. can you look again ? [16:45] Chipaca, I agree, whatever fails first stops the second from executing. [18:12] of course now there are _three_ comments with the same content [18:12] ah well [18:12] at least i'm consistent :-D [18:14] and travis <-> github is now discombobulated [18:14] it's fun all the way down [18:14] i shoulda just carried on running :-) [18:15] * Chipaca is sure he used to know english [18:17] "discombobulated" [18:23] facubatista: https://en.wiktionary.org/wiki/discombobulated [18:24] nice [21:14] * facubatista eods [21:19] facubatista: 👋 have a good one [21:22] Chipaca, good night! [21:23] * facubatista really goes [23:18] * Chipaca EODs