| jam | morning all | 05:15 |
|---|---|---|
| niemeyer | Morning jam | 07:49 |
| niemeyer | Morning all | 07:49 |
| jam | morning niemeyer | 07:49 |
| mup_ | PR operator#236 opened: ops/log.py: Default to DEBUG logging <Created by jameinel> <https://github.com/canonical/operator/pull/236> | 08:09 |
| niemeyer | jam: Reviewed, thanks | 08:14 |
| jam | thanks for the review | 08:34 |
| * jam goes to make lunch | 08:34 | |
| Chipaca | gooooood morning! | 08:52 |
| niemeyer | Hey hey | 08:52 |
| 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:04 |
| 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:05 |
| 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:06 |
| stub | ok, let me know if I'm needed when you get that far. | 09:09 |
| 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:11 |
| 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:12 |
| 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:13 |
| niemeyer | stub: I will have the mechanism and namespace proposal in hand by then | 09:14 |
| Chipaca | stub: thanks! | 09:15 |
| 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:53 |
| niemeyer | Me too, but in a good way.. manual k8s deployment seems to work okay | 09:54 |
| 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 | 09:56 |
| 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:51 |
| 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:52 |
| 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:53 |
| 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:54 |
| 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:55 |
| 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:56 |
| 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:57 |
| 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:58 |
| 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> | 10:59 |
| 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:00 |
| 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:07 |
| 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:08 |
| 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:10 |
| 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:13 |
| 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:17 |
| facubatista | Muy buenos días a todos! | 11:19 |
| jam | morning facubatista | 11:20 |
| jam | any chance you could look at #230 today? | 11:20 |
| mup_ | PR #230: Testing harness no events for own data <Created by jameinel> <https://github.com/canonical/operator/pull/230> | 11:21 |
| facubatista | hola jam | 11:23 |
| facubatista | jam, yes! | 11:23 |
| jam | great | 11:23 |
| 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:26 |
| 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:27 |
| 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:30 |
| jam | Chipaca, presumably it doesn't have any types, etc defined, right? | 11:31 |
| 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:33 |
| 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:34 |
| 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:35 |
| jam | Chipaca, eat hearty | 11:36 |
| 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:38 |
| 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:39 |
| 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:40 |
| 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:42 |
| 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:43 |
| 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:44 |
| 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:45 |
| facubatista | ok | 11:46 |
| facubatista | thanks, I understand now | 11:46 |
| 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:53 |
| 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:54 |
| jam | Why are they doing AllHands in the middle of Vienna Product roadmap ... | 11:56 |
| * facubatista misses real all hands | 11:57 | |
| jam | :) | 11:57 |
| 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 | 11:58 |
| 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:02 |
| 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:03 |
| 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:04 |
| 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:06 |
| 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:07 |
| 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:09 |
| 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:51 |
| facubatista | jam, I like it, may you replace "be caching" with "have cached"? | 12:53 |
| 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 | 12:55 |
| 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:03 |
| 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:04 |
| * Chipaca suddenly realises the time | 13:27 | |
| facubatista | Chipaca, you mean, in an einstanian way? | 13:29 |
| jam | Time flies like an arrow. Fruit flies like a banana | 13:29 |
| jam | facubatista, can you check if 230 now matches your expectation? | 13:33 |
| facubatista | jam, yes | 13:34 |
| pekkari | Hi, qq, is juju triggering any config-changed after upgrade charm? | 13:43 |
| Chipaca | pekkari: AFAIK yes, why? | 13:48 |
| Chipaca | jam: ^ | 13:48 |
| 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:49 |
| 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:51 |
| 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:52 |
| pekkari | that makes sense, jam, thanks! | 13:55 |
| 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:56 |
| pekkari | awesome, wiping unneeded code then :) | 13:58 |
| 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:08 |
| Chipaca | jam, niemeyer, if you have a hook and dispatch, and the first one fails, should the second one still run? | 14:40 |
| 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:46 |
| Chipaca | facubatista: yeah, and afaict the answer is no | 14:47 |
| facubatista | Chipaca, I concur | 14:49 |
| 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:36 | |
| facubatista | jajaj | 15:46 |
| facubatista | Chipaca, that, or they decided to turn it off forever | 15:46 |
| 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:44 |
| 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. | 16:45 |
| 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:12 |
| 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:14 |
| * Chipaca is sure he used to know english | 18:15 | |
| facubatista | "discombobulated" | 18:17 |
| Chipaca | facubatista: https://en.wiktionary.org/wiki/discombobulated | 18:23 |
| facubatista | nice | 18:24 |
| * facubatista eods | 21:14 | |
| Chipaca | facubatista: 👋 have a good one | 21:19 |
| facubatista | Chipaca, good night! | 21:22 |
| * facubatista really goes | 21:23 | |
| * Chipaca EODs | 23:18 | |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!