| Dmitrii-Sh | o/ | 04:20 |
|---|---|---|
| jam | morning Dmitrii-Sh | 04:24 |
| Dmitrii-Sh | morning jam | 05:00 |
| niemeyer | Morning all | 07:24 |
| niemeyer | Today I'm running through the entire PR queue.. please don't merge anything as I go through it so we don't clash in the middle. | 07:49 |
| Dmitrii-Sh | jam: https://github.com/canonical/operator/pull/212#discussion_r404621103 addressed comments for 212 | 08:15 |
| Chipaca | good and paperwork-flooded morning, all! | 08:16 |
| Dmitrii-Sh | o/ | 08:16 |
| Dmitrii-Sh | jam: also: I think that emitting -changed events for remote_app_data if .begin() is called after a relation is added is an important use-case but probably belongs to a different PR | 08:18 |
| Dmitrii-Sh | we haven't discussed it in 212 but one test case I just added is related to that (it needs a counterpart where a unit is not a leader and a peer relation is tested) | 08:19 |
| niemeyer | Dmitrii-Sh: #216 is reviewed | 08:25 |
| Dmitrii-Sh | niemeyer: ty | 08:25 |
| niemeyer | np, glad to see this kind of component showing up | 08:25 |
| niemeyer | Hopefully the first of many | 08:26 |
| jam | Dmitrii-Sh, are you saying if you do add_relation_unit/add_relation after begin() ? | 08:26 |
| jam | Chipaca, no, please don't wish that upon me :) | 08:27 |
| jam | (hopefully you don't get too many papercuts from the paper flood) | 08:27 |
| jam | morning Gustavo | 08:27 |
| Chipaca | :) | 08:28 |
| niemeyer | jam: o/ | 08:28 |
| Dmitrii-Sh | jam: not exactly. If somebody does `add_relation(... remote_app_data=some_dict)` or `set_leader(is_leader=False) ; add_relation(... initial_app_data=some_dict)` before calling .begin(), once they call begin, the charm object should get a relation_changed event | 08:29 |
| Dmitrii-Sh | it's the scenario where app data exists in the model but your unit hasn't come up yet | 08:30 |
| jam | Dmitrii-Sh, so we can certainly discuss that. My concern is mixing "setting up stuff as precondition" from "sending events during the lifetime" | 08:30 |
| Dmitrii-Sh | jam: ok, I'll switch to a different PR for now but just wanted to bring that up - in my view, it's a condition that might occur and that a conscious developer would want to test | 08:33 |
| Chipaca | whoa, 30 people. Feels like a celebration in here :) | 08:33 |
| niemeyer | facubatista: You got a review on #213 | 09:09 |
| niemeyer | jam: #212 too.. this one feels like it might be better for you to think it through and take it over | 09:41 |
| niemeyer | jam: Let's catch up about it at some point today or tomorrow | 09:41 |
| jam | sure | 09:46 |
| Chipaca | jam: I had a note to talk with you about something similar, adding network_get support to the harness | 09:53 |
| Chipaca | as that's also going to be blocking Dmitrii-Sh soon (if not already) | 09:53 |
| Dmitrii-Sh | Might be useful to address https://github.com/canonical/operator/issues/175 along with adding relation-created https://github.com/canonical/operator/pull/218 | 10:14 |
| Dmitrii-Sh | it feels like we'd need to change the signature of Relation.__init__ here https://github.com/canonical/operator/blob/17f4885b73d7b2bfb7445388e7e62b9fcd040859/ops/model.py#L368-L383 to be able to get the remote application based on JUJU_REMOTE_APP, instead of using units for that | 10:16 |
| Dmitrii-Sh | there is no such thing as `relation-list -r <id> --app` to list the remote app either | 10:17 |
| Dmitrii-Sh | happy to discuss this in more detail if anybody is interested | 10:18 |
| * Chipaca steps out for a bit | 10:45 | |
| Dmitrii-Sh | FYI: there's an issue with Travis runs & aarch64: https://travis-ci.community/t/no-cache-support-on-arm64/5416/27 - that's why there were test failures in the tls-certificates PR which pulled the cryptography package | 10:49 |
| Dmitrii-Sh | I applied a workaround to .travis.yml in this PR. Giving it more visibility because it's hard to find online. | 10:50 |
| facubatista | Muy buenos días a todos! | 11:00 |
| niemeyer | facubatista: Good morning! | 11:01 |
| facubatista | niemeyer, hola :) | 11:03 |
| facubatista | niemeyer, ack, for the review | 11:12 |
| Dmitrii-Sh | facubatista: o/ | 11:13 |
| facubatista | hola Dmitrii-Sh! | 11:14 |
| jam | hi facubatista, the patch has landed in edge and you can bootstrap again | 11:30 |
| facubatista | jam, yes! read the LP issue, thanks! (I put it to refresh, and then dived into a review | 11:31 |
| facubatista | ) | 11:31 |
| facubatista | all: I don't know if this is more a github or a team convention, but I'm a little lost in this regard (never been involved in long multiperson reviews in GH before)... so: developer X does a PR, dev Y does a comment in the review; if X agrees with that (answers 'OK' or whatever, and pushes a commit with the proper change), WHO should "resolve" that conversation? X or Y? | 11:33 |
| jam | facubatista, the way *I* do reviews: If I mark it Approved with a comment, that is saying that if you agree with my comment and fix it, then I don't need to review it again for you to land it. | 11:34 |
| jam | If I mark it Comment then I want to see it again, but I'm not concerned, if I mark it Needs Fixing then I definitely don't want it to land without seeing it again. | 11:35 |
| facubatista | jam, not yet approved... you did a review, has 5 comments, I address 4 of those, answering each "ok", and pushing a commit... one conversation is still open! but those 4 are resolved, shall I close them or you? | 11:35 |
| jam | facubatista, but it is always on the developer to push to get their PR landed. | 11:35 |
| facubatista | jam, my question is not the PR as a whole, but each conversation | 11:36 |
| jam | good question. I'd generally say the person who raised the question should be the one who acknowledges it is resolved. | 11:38 |
| Dmitrii-Sh | jam: interesting comment https://github.com/canonical/operator/pull/210/files#r402267572 I responded but not sure if we should do something about it in this particular PR | 11:39 |
| jam | Dmitrii-Sh, that wasn't something for this patch, mostly something that we should think about. | 11:40 |
| Dmitrii-Sh | ok | 11:40 |
| jam | facubatista, though I think Dmitrii tends to use the comment list as the "things left to do" so he marks them resolved when he finishes them. | 11:45 |
| niemeyer | Good practice | 11:53 |
| * niemeyer => lunch | 11:53 | |
| Dmitrii-Sh | jam, facubatista: I tend to resolve them referencing the commit where I address the issue/comment. With that I can at least sanely track the remaining items without looping over them and remember where I think I fixed things | 12:37 |
| Dmitrii-Sh | in short: jam is correct | 12:37 |
| jam | Dmitrii-Sh, I like the comment about "addressed", I'm just not sure whether it is on the response to resolve the thread, because that hides it when the reviewer comes back | 12:38 |
| jam | I often have to dig a bit to find your response to my comment. | 12:38 |
| Dmitrii-Sh | hmm, I could tag the responses somehow if that helps | 12:39 |
| Dmitrii-Sh | just need to agree on something easily searchable | 12:39 |
| facubatista | jam, that's my main point in favor of "all comments should be resolved by the person who initiated it"... (which has its downsides, of course) | 12:39 |
| jam | Dmitrii-Sh, my pattern is "I put it up for review, facu comments on it, I reply with 'done', facu gets to close it" | 12:40 |
| jam | that acks that I saw what he asked, and that I've done the work myself (including the git hash on top of that is great), | 12:41 |
| facubatista | jam, I prefer that; however, I need to pay attention to finally close all comments I initiated! (which IMO is my responsibility as a reviewer, anyway, UNLESS I already approved the PR) | 12:41 |
| jam | It does mean your list of 'things left to do' isn't the list of open questions, but the list of open questions that you haven't replied to. | 12:41 |
| jam | facubatista, yeah, I personally would probably only Resolve them if I was then asking for more, vs just an overall approval | 12:42 |
| jam | facubatista, I did have a comment on the Breakpoint pr. (213). It feels like we should be only printing the help text 1 time at startup, vs on each breakpoint. Thoughts? | 12:42 |
| jam | Dmitrii-Sh, given gustavo's comment. What do you think about letting update_relation_data set self values, but only prior to begin() | 12:43 |
| jam | that would let it be incremental | 12:43 |
| jam | but also avoid having the harness changing thing that the charm should be taking as its responsibility ? | 12:44 |
| * jam takes the dog out quickly, bbiab | 12:44 | |
| facubatista | jam, I didn't see that question, sorry :( ... so, we should present the comment everytime the process is started, as juju would do that in a "clean tmux", right? but if we handle several breakpoints in the same run, one message should be enough | 12:44 |
| Dmitrii-Sh | jam: I can rework it as you suggested. Then I think we also need to remove the existing remote_app_data argument and handle everything via update_relation_data | 12:49 |
| Dmitrii-Sh | otherwise people will need to be educated that they can only add remote data like that but have to use update_relation_data before begin to set initial data for our unit and app | 12:50 |
| Dmitrii-Sh | before `.begin()` * | 12:50 |
| Dmitrii-Sh | I'll wait for feedback and then implement it if we all agree on the approach | 12:52 |
| Dmitrii-Sh | jam: also, if you have any thoughts on https://github.com/canonical/operator/pull/218#issuecomment-610364750, it would be good to hear | 12:53 |
| Dmitrii-Sh | either I don't see something obvious, or we need `relation-list -r <rid> --app` | 12:53 |
| narindergupta | Dmitrii-Sh, niemeyer hi | 12:56 |
| jam | facubatista, right. clean tmux per hook, but only 1 message when handling multiple breakpoints in a single exec | 12:56 |
| jam | Dmitrii-Sh, don't we have JUJU_REMOTE_APP ? | 12:57 |
| Dmitrii-Sh | jam: in a relation event context - yes. But not in "leader-elected" or "start" | 12:57 |
| jam | yeah, I hadn't finished going through your comment. | 12:58 |
| jam | hm | 12:58 |
| facubatista | jam, indeed, will improve that | 12:58 |
| narindergupta | Dmitrii-Sh, niemeyer in my case any change in pod sec and calling set_spec is removing the juju unit and add a new unit. So existing unit zookeeper/0 gets removed and zookeeper/1 gets added. | 12:58 |
| jam | narindergupta, changing a pod spec will update the pod spec with k8s, which causes k8s to spin up new pods and tear down old pods | 12:59 |
| jam | thus they show up as new Juju units. | 12:59 |
| jam | (eg, if you have n=3, at some point there will be a 4th k8s pod before it kills the 1st one) | 12:59 |
| jam | narindergupta, I'm not positive how it works if you have a StatefulSet that supports storage, because there the identifiers get to be reused. | 13:00 |
| narindergupta | jam, for persistent pods name is same so it tries to delete the volume and create it again and gets stuck | 13:00 |
| Dmitrii-Sh | jam: so if I want to write something to an app relation data bag in any non-relation event, I need to reference the app somehow. And currently we retrieve a remote app for a Relation object based on a remote unit | 13:00 |
| Dmitrii-Sh | before we have remote units .app is None | 13:00 |
| jam | Dmitrii-Sh, "reference the app", I don't quite follow. You don't get to write to the remote app's data. | 13:01 |
| jam | Dmitrii-Sh, are you saying to *read* the remote app's data ? | 13:01 |
| Dmitrii-Sh | jam: ok, sorry, read/write | 13:02 |
| Dmitrii-Sh | actually, "just read" because we have special handling for peer relations | 13:02 |
| narindergupta | jam, will it be an issue for any k8s charm update, upgrade etc if we change the pod spec. In other words we should not change the pod spec if using persistence volume. Is there a way to not to delete the persistence volume during pod stop? | 13:03 |
| jam | narindergupta, so you're past my knowledge for persistent volumes. I'd say ask in #juju from wallyworld or kelvinliu | 13:04 |
| narindergupta | jam, ok let me check with them. | 13:04 |
| jam | Dmitrii-Sh, seems ok to drop remote_app_data if we are dropping the rest. I prefer the single setup-step approach but obviously that isn't universal | 13:07 |
| Dmitrii-Sh | jam: ok, I think we can spend couple of minutes during the standup to go over that just to make sure everybody is on the same page and then I'll implement this. | 13:09 |
| jam | Dmitrii-Sh, I think we have a cross team sync during our standup | 13:09 |
| jam | Dmitrii-Sh, https://github.com/canonical/operator/pull/212/files#r404561805 . "if we make our unit a leader we should see it". Yes we should be able to read the data, but I don't think you get a relation-changed event for something the previous leader set. | 13:10 |
| Dmitrii-Sh | jam: indeed :^) | 13:10 |
| jam | Dmitrii-Sh, ok. I think it is just my confusion around the ordering of the checks. "if is_our_app_updated: " should return, but shouldn't return if it is a peer relation and we aren't the leader. | 13:12 |
| jam | Trying to figure out if there is a clearer way to convey that. | 13:12 |
| jam | I do see what you're checking. | 13:12 |
| Dmitrii-Sh | jam: ok | 13:14 |
| jam | Dmitrii-Sh, so you wanted this to handle the case of being promoted to leader? | 13:15 |
| jam | Dmitrii-Sh, I have a proposed spelling/comment. if it doesn't feel clearer I'm not stuck on it. But I think my confusion was because you were checking "not is_peer" first | 13:17 |
| Dmitrii-Sh | jam: "being promoted to leader?" - which line is that, sorry? | 13:17 |
| jam | Dmitrii-Sh, given that your unit doesn't get to see app data when it is updated, why did you want to update the app data for the remote? | 13:17 |
| jam | so that you could set it, and then promote the unit to leader and see that it sees the app data ? | 13:18 |
| pekkari | Hi guys, I'm having a charm that seems to be skipping the install hook, is there anything else required than declaring the observer on start, and defining the on_install function? | 13:20 |
| Dmitrii-Sh | pekkari: is that a k8s charm on Juju 2.7.x? | 13:25 |
| Dmitrii-Sh | in that case, this is expected per https://bugs.launchpad.net/juju/+bug/1854635 but will be fixed in 2.8.x | 13:26 |
| Dmitrii-Sh | you can use on_start though | 13:27 |
| pekkari | no k8s, Dmitrii-Sh, I'm just trying to write gsss charm using the op framework | 13:27 |
| Dmitrii-Sh | you need to have a symlink at hooks/install -> ../src/charm.py | 13:27 |
| pekkari | so in __init__ I add the line: self.framework.observe(self.on.install, self) , and then add the def on_install(self, event): | 13:28 |
| pekkari | that link is in place also | 13:28 |
| jam | Dmitrii-Sh, I just checked with Juju, that was released in a 2.7.? so it should be fixed in 2.7.5 | 13:28 |
| Dmitrii-Sh | jam: thanks | 13:28 |
| jam | getting them to update the bug | 13:28 |
| Dmitrii-Sh | pekkari: could you paste your charm.py somewhere? Or at least the top and the bottom of it | 13:29 |
| Dmitrii-Sh | #!/usr/bin/env python3 | 13:30 |
| Dmitrii-Sh | from ops.main import main | 13:30 |
| Dmitrii-Sh | # ... | 13:30 |
| Dmitrii-Sh | class MyCharm(ops.charm.CharmBase) | 13:30 |
| Dmitrii-Sh | # ... | 13:30 |
| Dmitrii-Sh | if __name__ == '__main__': | 13:30 |
| Dmitrii-Sh | main(MyCharm) | 13:30 |
| Dmitrii-Sh | pekkari: you also need that ^ | 13:30 |
| jam | Dmitrii-Sh, facubatista : there is a "Weekly K8s Charm Sync" that got a collision with our usual standup time. | 13:32 |
| Chipaca | Dmitrii-Sh: jam: standup? | 13:32 |
| Chipaca | jam: or are you going to that sync? | 13:32 |
| jam | Chipaca, going to that sync today at least | 13:32 |
| niemeyer | jam: If that was just booked, ideally it should deconflict | 13:33 |
| Dmitrii-Sh | Chipaca: at this cross-team sync | 13:33 |
| niemeyer | Dmitrii-Sh: Same | 13:33 |
| niemeyer | We've moved the standup | 13:34 |
| pekkari | Dmitrii-Sh: let me try that and if I observe no change I'll paste it, to let you read, thanks! | 13:34 |
| narindergupta | jam, it seems to be juju bug and I have raised the bug and thanks for guidance. Also another question is there any event gets fired when pod/unit/app status gets changed? I am more interested in app status event as it would help me to determine whether all my pods are up or not? Instead of waiting for update status event after 5 minutes? | 13:35 |
| niemeyer | narindergupta: I'd like to have a call with you and jam to understand this behavior a bit better | 13:35 |
| Dmitrii-Sh | ok, sorry, I thought that sync conflict was agreed upon | 13:35 |
| narindergupta | niemeyer, I am available now | 13:36 |
| niemeyer | Okay, let's do it.. jam cannot join us but I'd like to understand at least what you're observing. | 13:36 |
| niemeyer | narindergupta: https://meet.google.com/gri-hkiq-itv | 13:37 |
| pekkari | hum, even moving the install code to start event doesn't seem to make it, can you please take a quick read here Dmitrii-Sh? https://pastebin.canonical.com/p/7TMVMG954c/ | 13:53 |
| narindergupta | niemeyer, https://bugs.launchpad.net/juju/+bug/1871388 | 13:55 |
| Dmitrii-Sh | pekkari: could you also share a pastebin with `tree <charm-source-dir>`? | 13:55 |
| pekkari | absolutely | 13:57 |
| pekkari | here it is, Dmitrii-Sh: https://pastebin.canonical.com/p/q5GVvb6rpw/ | 14:00 |
| niemeyer | narindergupta: Can you please paste the pod spec for the example we've explored? | 14:04 |
| niemeyer | narindergupta: Just for completeness | 14:04 |
| narindergupta | niemeyer, sure | 14:04 |
| narindergupta | This is zookeeper pod spec http://paste.ubuntu.com/p/mqxjxZyBxh/ | 14:06 |
| narindergupta | niemeyer, added in the bug comment also. | 14:08 |
| niemeyer | narindergupta: Thanks! | 14:14 |
| Dmitrii-Sh | pekkari: haven't found anything apparent yet. I'll have a look after meetings | 14:15 |
| Dmitrii-Sh | juju show-status-log <unit> might also help | 14:15 |
| pekkari | take your time Dmitrii-Sh, seems no trivial issue, I struggled for a big while with this, show-status-log here: https://pastebin.canonical.com/p/3rzGz3M93J/ | 14:17 |
| jam | pekkari, install only happens once. it looks like you're long past install by the time you're doing these steps | 14:24 |
| pekkari | jam, that is what I expected, though after the creation of the unit I never see the packages I expect installed. install hook is nearly copy & paste of the former charm, and apt packages library works when running ocally | 14:27 |
| pekkari | so seems like the workflow never hit the on_start function when running the install hooks | 14:28 |
| pekkari | I'm going to remove the unit and add a new one though, perhaps with the changes will help | 14:29 |
| pekkari | no luck, it shows install status, goes forward to config-changed and errors there, the expected packages doesn't get installed | 14:40 |
| jam | pekkari, config-changed happens before start | 14:43 |
| jam | pekkari, install, relation-created, config-changed, start IIRC | 14:43 |
| jam | (relation-created is new in Juju 2.8) | 14:44 |
| pekkari | in that case, I'd need to consider to move back the code to the install event, and observe it as it was before | 14:45 |
| jam | pekkari, so if you need something before you can respond, I would, indeed, put it in install, and consider if it is the sort of thing that would change over time, putting it in upgrade-charm | 14:46 |
| pekkari | aha! now I see a traceback with my code, thanks for the hint jam and Dmitrii-Sh! | 14:54 |
| Dmitrii-Sh | pekkari: ok, so the "install" symlink needs to be in the hooks directory at the deployment time | 15:00 |
| Dmitrii-Sh | then all others will be created by the framework automatically | 15:00 |
| Chipaca | my canonical calendar is now suggesting personal contacts instead of canonical contacts for meetings | 15:00 |
| Chipaca | something's effed up o_o | 15:00 |
| niemeyer | Chipaca: It just knows you care personally about it | 15:00 |
| Dmitrii-Sh | Chipaca: maybe it's just the EOD time | 15:00 |
| Chipaca | :-) | 15:01 |
| niemeyer | Chipaca: "Hey, how about asking your son to fix that!?" | 15:01 |
| Chipaca | I mean, I can _ask_ | 15:01 |
| niemeyer | I know *my* son would be super excited to get involved.. | 15:01 |
| Dmitrii-Sh | pekkari: if you have any juju storage defined in metadata.yaml you also need to have storage event symlinks because they fire before "install" but it doesn't look like you have any | 15:01 |
| niemeyer | We might have some trouble handling that excitement, though | 15:02 |
| pekkari | Dmitrii-Sh: no, so far not yet, I took this charm because of being always broken, and seems to be little enough to let me learn properly the main points in operator framework, but thanks for the hint, I may hit that later | 15:05 |
| facubatista | niemeyer, jam, already addressed your comments on #213 | 15:13 |
| * facubatista -> lunch | 15:22 | |
| Dmitrii-Sh | pekkari: ok, have you managed to get to a proper state or can I still help? | 15:24 |
| pekkari | Dmitrii-Sh: yes, I got to the install code I wanted to reach, your hints about the ops.main import and the if __name__ clause, plus keeping the install code in the install event made it through, and now the problems around are problems for tomorrows José | 15:29 |
| pekkari | thanks man! | 15:29 |
| Dmitrii-Sh | pekkari: great, np! | 15:30 |
| Dmitrii-Sh | have a good evening | 15:30 |
| pekkari | you too! | 15:30 |
| niemeyer | facubatista: LGTM still | 16:25 |
| * facubatista is back | 16:30 | |
| facubatista | niemeyer, thanks | 16:34 |
| * Chipaca goes off to make dinner | 17:37 | |
| Chipaca | ttfn! | 17:37 |
| facubatista | Chipaca, have a nice cooking | 17:47 |
| * facubatista eods | 20:42 | |
| * Chipaca eods | 23:25 | |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!