jammorning Dmitrii-Sh04:24
Dmitrii-Shmorning jam05:00
niemeyerMorning all07:24
niemeyerToday 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-Shjam: https://github.com/canonical/operator/pull/212#discussion_r404621103 addressed comments for 21208:15
Chipacagood and paperwork-flooded morning, all!08:16
Dmitrii-Shjam: 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 PR08:18
Dmitrii-Shwe 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
niemeyerDmitrii-Sh: #216 is reviewed08:25
Dmitrii-Shniemeyer: ty08:25
niemeyernp, glad to see this kind of component showing up08:25
niemeyerHopefully the first of many08:26
jamDmitrii-Sh, are you saying if you do add_relation_unit/add_relation after begin() ?08:26
jamChipaca, 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
jammorning Gustavo08:27
niemeyerjam: o/08:28
Dmitrii-Shjam: 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 event08:29
Dmitrii-Shit's the scenario where app data exists in the model but your unit hasn't come up yet08:30
jamDmitrii-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-Shjam: 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 test08:33
Chipacawhoa, 30 people. Feels like a celebration in here :)08:33
niemeyerfacubatista: You got a review on #21309:09
niemeyerjam: #212 too.. this one feels like it might be better for you to think it through and take it over09:41
niemeyerjam: Let's catch up about it at some point today or tomorrow09:41
Chipacajam: I had a note to talk with you about something similar, adding network_get support to the harness09:53
Chipacaas that's also going to be blocking Dmitrii-Sh soon (if not already)09:53
Dmitrii-ShMight be useful to address https://github.com/canonical/operator/issues/175 along with adding relation-created https://github.com/canonical/operator/pull/21810:14
Dmitrii-Shit 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 that10:16
Dmitrii-Shthere is no such thing as `relation-list -r <id> --app` to list the remote app either10:17
Dmitrii-Shhappy to discuss this in more detail if anybody is interested10:18
* Chipaca steps out for a bit10:45
Dmitrii-ShFYI: 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 package10:49
Dmitrii-ShI applied a workaround to .travis.yml in this PR. Giving it more visibility because it's hard to find online.10:50
facubatistaMuy buenos días a todos!11:00
niemeyerfacubatista: Good morning!11:01
facubatistaniemeyer, hola :)11:03
facubatistaniemeyer, ack, for the review11:12
Dmitrii-Shfacubatista: o/11:13
facubatistahola Dmitrii-Sh!11:14
jamhi facubatista, the patch has landed in edge and you can bootstrap again11:30
facubatistajam, yes! read the LP issue, thanks! (I put it to refresh, and then dived into a review11:31
facubatistaall: 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
jamfacubatista, 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
jamIf 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
facubatistajam, 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
jamfacubatista, but it is always on the developer to push to get their PR landed.11:35
facubatistajam, my question is not the PR as a whole, but each conversation11:36
jamgood question. I'd generally say the person who raised the question should be the one who acknowledges it is resolved.11:38
Dmitrii-Shjam: 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 PR11:39
jamDmitrii-Sh, that wasn't something for this patch, mostly something that we should think about.11:40
jamfacubatista, 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
niemeyerGood practice11:53
* niemeyer => lunch11:53
Dmitrii-Shjam, 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 things12:37
Dmitrii-Shin short: jam is correct12:37
jamDmitrii-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 back12:38
jamI often have to dig a bit to find your response to my comment.12:38
Dmitrii-Shhmm, I could tag the responses somehow if that helps12:39
Dmitrii-Shjust need to agree on something easily searchable12:39
facubatistajam, 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
jamDmitrii-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
jamthat 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
facubatistajam, 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
jamIt 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
jamfacubatista, yeah, I personally would probably only Resolve them if I was then asking for more, vs just an overall approval12:42
jamfacubatista, 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
jamDmitrii-Sh, given gustavo's comment. What do you think about letting update_relation_data set self values, but only prior to begin()12:43
jamthat would let it be incremental12:43
jambut also avoid having the harness changing thing that the charm should be taking as its responsibility ?12:44
* jam takes the dog out quickly, bbiab12:44
facubatistajam, 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 enough12:44
Dmitrii-Shjam: 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_data12:49
Dmitrii-Shotherwise 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 app12:50
Dmitrii-Shbefore `.begin()` *12:50
Dmitrii-ShI'll wait for feedback and then implement it if we all agree on the approach12:52
Dmitrii-Shjam: also, if you have any thoughts on https://github.com/canonical/operator/pull/218#issuecomment-610364750, it would be good to hear12:53
Dmitrii-Sheither I don't see something obvious, or we need `relation-list -r <rid> --app`12:53
narinderguptaDmitrii-Sh, niemeyer hi12:56
jamfacubatista, right. clean tmux per hook, but only 1 message when handling multiple breakpoints in a single exec12:56
jamDmitrii-Sh, don't we have JUJU_REMOTE_APP ?12:57
Dmitrii-Shjam: in a relation event context - yes. But not in "leader-elected" or "start"12:57
jamyeah, I hadn't finished going through your comment.12:58
facubatistajam, indeed, will improve that12:58
narinderguptaDmitrii-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
jamnarindergupta, changing a pod spec will update the pod spec with k8s, which causes k8s to spin up new pods and tear down old pods12:59
jamthus 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
jamnarindergupta, 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
narinderguptajam, for persistent pods name is same so it tries to delete the volume and create it again and gets stuck13:00
Dmitrii-Shjam: 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 unit13:00
Dmitrii-Shbefore we have remote units .app is None13:00
jamDmitrii-Sh, "reference the app", I don't quite follow. You don't get to write to the remote app's data.13:01
jamDmitrii-Sh, are you saying to *read* the remote app's data ?13:01
Dmitrii-Shjam: ok, sorry, read/write13:02
Dmitrii-Shactually, "just read" because we have special handling for peer relations13:02
narinderguptajam, 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
jamnarindergupta, so you're past my knowledge for persistent volumes. I'd say ask in #juju from wallyworld or kelvinliu13:04
narinderguptajam, ok let me check with them.13:04
jamDmitrii-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 universal13:07
Dmitrii-Shjam: 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
jamDmitrii-Sh, I think we have a cross team sync during our standup13:09
jamDmitrii-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-Shjam: indeed :^)13:10
jamDmitrii-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
jamTrying to figure out if there is a clearer way to convey that.13:12
jamI do see what you're checking.13:12
Dmitrii-Shjam: ok13:14
jamDmitrii-Sh, so you wanted this to handle the case of being promoted to leader?13:15
jamDmitrii-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" first13:17
Dmitrii-Shjam: "being promoted to leader?" - which line is that, sorry?13:17
jamDmitrii-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
jamso that you could set it, and then promote the unit to leader and see that it sees the app data ?13:18
pekkariHi 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-Shpekkari: is that a k8s charm on Juju 2.7.x?13:25
Dmitrii-Shin that case, this is expected per https://bugs.launchpad.net/juju/+bug/1854635 but will be fixed in 2.8.x13:26
Dmitrii-Shyou can use on_start though13:27
pekkarino k8s, Dmitrii-Sh, I'm just trying to write gsss charm using the op framework13:27
Dmitrii-Shyou need to have a symlink at hooks/install -> ../src/charm.py13:27
pekkariso in __init__ I add the line: self.framework.observe(self.on.install, self) , and then add the def on_install(self, event):13:28
pekkarithat link is in place also13:28
jamDmitrii-Sh, I just checked with Juju, that was released in a 2.7.? so it should be fixed in 2.7.513:28
Dmitrii-Shjam: thanks13:28
jamgetting them to update the bug13:28
Dmitrii-Shpekkari: could you paste your charm.py somewhere? Or at least the top and the bottom of it13:29
Dmitrii-Sh#!/usr/bin/env python313:30
Dmitrii-Shfrom ops.main import main13:30
Dmitrii-Sh# ...13:30
Dmitrii-Shclass MyCharm(ops.charm.CharmBase)13:30
Dmitrii-Sh# ...13:30
Dmitrii-Shif __name__ == '__main__':13:30
Dmitrii-Sh    main(MyCharm)13:30
Dmitrii-Shpekkari: you also need that ^13:30
jamDmitrii-Sh, facubatista : there is a "Weekly K8s Charm Sync" that got a collision with our usual standup time.13:32
ChipacaDmitrii-Sh: jam: standup?13:32
Chipacajam: or are you going to that sync?13:32
jamChipaca, going to that sync today at least13:32
niemeyerjam: If that was just booked, ideally it should deconflict13:33
Dmitrii-ShChipaca:  at this cross-team sync13:33
niemeyerDmitrii-Sh: Same13:33
niemeyerWe've moved the standup13:34
pekkariDmitrii-Sh: let me try that and if I observe no change I'll paste it, to let you read, thanks!13:34
narinderguptajam, 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
niemeyernarindergupta: I'd like to have a call with you and jam to understand this behavior a bit better13:35
Dmitrii-Shok, sorry, I thought that sync conflict was agreed upon13:35
narinderguptaniemeyer, I am available now13:36
niemeyerOkay, let's do it.. jam cannot join us but I'd like to understand at least what you're observing.13:36
niemeyernarindergupta: https://meet.google.com/gri-hkiq-itv13:37
pekkarihum, 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
narinderguptaniemeyer, https://bugs.launchpad.net/juju/+bug/187138813:55
Dmitrii-Shpekkari: could you also share a pastebin with `tree <charm-source-dir>`?13:55
pekkarihere it is, Dmitrii-Sh: https://pastebin.canonical.com/p/q5GVvb6rpw/14:00
niemeyernarindergupta: Can you please paste the pod spec for the example we've explored?14:04
niemeyernarindergupta: Just for completeness14:04
narinderguptaniemeyer, sure14:04
narinderguptaThis is zookeeper pod spec http://paste.ubuntu.com/p/mqxjxZyBxh/14:06
narinderguptaniemeyer, added in the bug comment also.14:08
niemeyernarindergupta: Thanks!14:14
Dmitrii-Shpekkari: haven't found anything apparent yet. I'll have a look after meetings14:15
Dmitrii-Shjuju show-status-log <unit> might also help14:15
pekkaritake 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
jampekkari, install only happens once. it looks like you're long past install by the time you're doing these steps14:24
pekkarijam, 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 ocally14:27
pekkariso seems like the workflow never hit the on_start function when running the install hooks14:28
pekkariI'm going to remove the unit and add a new one though, perhaps with the changes will help14:29
pekkarino luck, it shows install status, goes forward to config-changed and errors there, the expected packages doesn't get installed14:40
jampekkari, config-changed happens before start14:43
jampekkari, install, relation-created, config-changed, start IIRC14:43
jam(relation-created is new in Juju 2.8)14:44
pekkariin that case, I'd need to consider to move back the code to the install event, and observe it as it was before14:45
jampekkari, 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-charm14:46
pekkariaha! now I see a traceback with my code, thanks for the hint jam and Dmitrii-Sh!14:54
Dmitrii-Shpekkari: ok, so the "install" symlink needs to be in the hooks directory at the deployment time15:00
Dmitrii-Shthen all others will be created by the framework automatically15:00
Chipacamy canonical calendar is now suggesting personal contacts instead of canonical contacts for meetings15:00
Chipacasomething's effed up o_o15:00
niemeyerChipaca: It just knows you care personally about it15:00
Dmitrii-ShChipaca: maybe it's just the EOD time15:00
niemeyerChipaca: "Hey, how about asking your son to fix that!?"15:01
ChipacaI mean, I can _ask_15:01
niemeyerI know *my* son would be super excited to get involved..15:01
Dmitrii-Shpekkari: 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 any15:01
niemeyerWe might have some trouble handling that excitement, though15:02
pekkariDmitrii-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 later15:05
facubatistaniemeyer, jam, already addressed your comments on #21315:13
* facubatista -> lunch15:22
Dmitrii-Shpekkari: ok, have you managed to get to a proper state or can I still help?15:24
pekkariDmitrii-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
pekkarithanks man!15:29
Dmitrii-Shpekkari: great, np!15:30
Dmitrii-Shhave a good evening15:30
pekkariyou too!15:30
niemeyerfacubatista: LGTM still16:25
* facubatista is back16:30
facubatistaniemeyer, thanks16:34
* Chipaca goes off to make dinner17:37
facubatistaChipaca, have a nice cooking17:47
* facubatista eods20:42
* Chipaca eods23:25

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!