bthomasMorning all08:05
bthomas𝐆𝐨𝐨𝐝 𝐌𝐨𝐫𝐧𝐢𝐧𝐠 Chipaca08:58
bthomas 08:58
bthomasChipaca: Will harness.add_relation_unit trigger a relation changed event ?08:58
Chipacabthomas: morning08:58
Chipacabthomas: from that method's documentation,08:59
Chipaca        This will trigger a `relation_joined` event and a `relation_changed` event.08:59
Chipacabthomas: https://ops.rtfd.io/en/latest/#ops.testing.Harness.add_relation_unit09:00
bthomasThanks for confirming. I think I am messing something then.09:01
bthomasApologies Chipaca : I do not see what I am missing in this relation test which shows an empty relation data dictionary. https://pastebin.canonical.com/p/2ZxTXPvNtB/09:17
Chipacabthomas: where can i pull this from?09:19
bthomasChipaca: I have not pushed to github since it was not working but will do so now.09:19
Chipacabthomas: thank you09:20
bthomasChipaca: pushed09:20
Chipacabthomas: git commit -a -m checkpoint && git push && git reset @^  and then next time you push, git push --force-with-lease09:21
Chipacabthomas: ('git reset @^' does an "uncommit")09:21
bthomasChipaca: yes I will do a force push when I fix the test but if you have pulled you will end up with conflict if you pull again09:21
Chipacabthomas: pushed to where?09:21
bthomasChipaca: Oops . Pushed here https://github.com/balbirthomas/prometheus-charm.git09:22
Chipacabthomas: pulling09:22
* bthomas hids09:27
* bthomas hides09:27
Chipacabthomas: nope, you good09:28
Chipacabthomas: look at the last line of https://ops.readthedocs.io/en/latest/_modules/ops/testing.html#Harness.add_relation_unit09:28
Chipacabthomas: notice how it fires relation-joined09:29
Chipacabthomas: but not relation-changed09:29
bthomasThank you. For spotting this.09:29
Chipacai need to check with jam but I think that's a bug. I don't know if it's a bug in the docstring or the code though.09:29
bthomasChipaca: Shouldn't relation-joined emit relation changed in turn ?09:30
Chipacaalso need to check with jam about #39509:30
mupIssue #395: juju-info not supported <Created by stub42> <https://github.com/canonical/operator/issues/395>09:30
Chipacabthomas: not like that09:30
Chipacabthomas: i mean, that's why i need to check with jam, as i don't know if _juju_ does09:30
Chipacabthomas: but the charm won't and shouldn't09:31
bthomasChipaca: Ok I will park this test for now and continue with others09:31
Chipacabthomas: if juju does, then the bug is in the code; if juju does not, then the bug is in the docstring09:31
bthomasThank you for clarifying09:31
Chipacathank you for spotting this09:31
facubatista¡Muy buenos días a todos!10:36
bthomasनमस्ते facubatista10:37
facubatistahola bthomas10:37
jamChipaca, when a unit joins a relation (once they complete the start hook), juju will always fire relation-joined and then relation-changed. I'm not sure if we want to encode that in the harness, vs waiting for the user to update_relation_data to trigger relation-changed10:53
Chipacajam: I'm not sure either, but I'm sure the docstring calls it one way and the code another :)10:58
jamChipaca, indeed, I saw that from Balbir's comment. Feel free to fix it in either way :)10:59
jamChipaca, for juju-info, there is, indeed, an implicit endpoint for every application call 'juju-info'. It was unclear to me what stub wanted to do with it to make sure we cover his use case11:00
mupPR operator#401 opened: fix docstring on Harness.add_relation_unit WRT events <Created by chipaca> <https://github.com/canonical/operator/pull/401>11:15
Chipacabthomas: circling back to your issue, it's working as intended and you should call update_relation_data to see the relation-changed event :)11:17
* facubatista begs for reviews https://github.com/canonical/charmcraft/pull/15411:18
mupPR charmcraft#154: Improved short help messages <Created by facundobatista> <https://github.com/canonical/charmcraft/pull/154>11:18
bthomasChipaca: will check. thank you11:18
Chipacabthomas: also if you're feeling like reading something, feedback on https://docs.google.com/document/d/1jwFlPG_q5lr80NYI19L_3mErAv4Yk2-g6uzK113Qiks/edit welcome11:19
bthomasChipaca: will do after lunch11:19
Chipacalunch: a preposterous notion11:27
bthomasAlternatively I can fuel on wine too12:01
bthomasChipaca: Added a few questions to google docs. It is entirely possible they are meaningless dribble if so ignore them but if they make sense ...12:02
bthomasfacubatista: pull 154 approved with one comment12:06
mupIssue #154: [Question] Do we need a separate base class for interfaces? <Created by VariableDeclared> <Closed by VariableDeclared> <https://github.com/canonical/operator/issues/154>12:06
facubatistabthomas, thanks12:09
facubatistabthomas, jaja, yes, what a silly docstring I wrote there!12:10
bthomasI think you will find stiff competition from me when it comes to being silly12:10
Chipacafacubatista: .i see no problem.12:11
bthomasChipaca: I am setting relation data in the relation changed handler in my src/charm.py. So I want to test that my relation changed handler defined in src/charm.py is setting that data. To write such a test I am adding a relation unit then calling update_relation_data() with an empty dictionary as the argument. This produces an empty dictionary for the data. Is this the expected behaviour ?12:20
Chipacabthomas: it … depends? if you mean that you get an empty dictionary, and never see the setting of the data that you do, then no12:22
bthomasChipaca: ok then I have a problem and need to dig a bit before I get back to you. But looks like right now, my src/charm.py relation handler is not setting data or alternatively harness is not seeing it, or update_relation_data is overwriting it.12:23
Chipacabthomas: share the code so i can take a peek?12:24
bthomasChipaca: pushed to https://github.com/balbirthomas/prometheus-charm.git . The test in question is the only one skipped.12:26
Chipacabthomas: self.unit is prometheus/0, not grafana/012:29
Chipacabthomas: so in your call to get_relation_data, you want prometheus/0 not grafana/012:31
bthomasChipaca: I was under the impression that when I set the relation data in one unit participating in the relation the other unit will see the change too. Looks like I was wrong. So I guess I need to set the data on event.unit i.e. event.relation.data[event.unit]['port'] = ... . Is this correct ?12:33
bthomasYep. Changing self.unit to event.unit makes test pass.12:35
bthomasThanks Chipaca12:35
bthomasoops spoke too soon12:36
Chipacai don't think it works that way12:36
Chipacayou're setting your data in the other charm's data bag12:36
Chipacathe other unit's data bag*12:37
Chipacabthomas: is that what you're wanting to do?12:38
bthomasChipaca: I just changed self.unit to event.unit in on_grafana_changed() in src/charm.py. I am trying to get the test to pass now. I will comb the docs to make sure I am doing this right.12:40
justinclarkGood morning o/13:13
bthomasMorning justinclark13:16
Chipacajustinclark: could you enter your PTO time in the team calendar?13:26
justinclarkOh yes. Will do right now.13:26
* bthomas resumes his literary career writing hook documenation13:33
Chipacajustinclark: 👍13:35
SpadsChipaca: https://code.launchpad.net/~nick-moffitt/charm-k8s-site24x7-exporter/+git/charm-k8s-site24x7-exporter/+merge/390565 is in the active reviews queue now, thanks!15:24
ChipacaSpads: thanks!15:25
justinclarkbthomas: let me know whether you decide to use app data vs. unit data for the prometheus private-address/port/source-type since grafana will need to access that data. Right now grafana is looking at the prometheus unit data.15:56
bthomasjustinclark: leave it as it is for now if it is working. I presume this will not be a big change. Correct me if I am wrong. We should talk to Jeremy if there will ever be the case when mutiple prometheus units in the same cluster will talk to grafana.15:57
Chipacaif you have multiple prometheuses (promethea?) for HA, don't you need a load balancer for grafana to dtrt?15:59
justinclarkMy understanding is that there might be multiple prometheus units, but there will be some sort load balancer between it and grafana - so there will really only be one URL for the multi-unit prometheus cluster that is sent to Grafana.15:59
justinclarkYes I think so Chipaca ^15:59
justinclarkAnd if we need prometheus to monitor totally different things, we will have a separate relation (i.e. separate relation data bags) for that - not separate units on the same relation.16:01
bthomasBy separate relation are you referring to metadata.yaml ?16:02
bthomasIf we collating data from multiple prometheus units through a load balancer (Service in the k8s world) we will need to ensure that those data streams are distinguishable in Grafana and test for this specifically.16:04
justinclarkI don't know all the details, but I think the charm code would remain the same - we would just do `juju deploy prometheus <config_options_0>` and `juju deploy prometheus <config_options_1`16:05
justinclarkThen I think add two relations for each prometheus deployment. I'll test that this works.16:07
justinclarkI don't think Grafana has to worry about whether there are multiple units - only the leader Prometheus unit should be setting the relation data so there should only be one set of information passed to Grafana16:08
bthomasMaybe we can do this once we have the one to one relation working16:09
bthomasWhat you are suggesting is that the prometheus leader unit aggregate data and forward it to Grafana. I am fine with this. I would like to get some input from Jeremy with regard to the use cases we must support and the overally architecture.16:10
justinclarkbthomas: agreed. We can dive deeper into this tomorrow.16:11
bthomasChipaca: facubatista jam : Is it correct to say that after handing a config changed event a charm must set its unit/application status to ActiveStatus, if there is no on_start handler? Alternatively it should do so at the end of the on_start handler ?16:32
jambthomas, it depends whether you feel you are ready at that point.16:33
jambthomas, if you need relation data before you can be correctly configured, then you're not "Active"16:33
jamyou're still Waiting16:33
jamif you don't have a relation that you depend upon, then you're "Blocked"16:33
jambthomas, you don't *join* relations until after start completes, so there isn't much expectation that you'll be fully Active at that point.16:34
jam(you'll have relation-created to know the relation exists, but you won't have relation-joined to know the content of the relation information)16:34
bthomasjam : thanks for the clarification. I will paraphase this in the hook docs taking into consideration the caveats you highlight. How to manage status was another one of those things that does not come through existing docs very clearly (IMHO).16:36
bthomasjam : can I say that "status" set by a Charm is a "hint" to the Juju agent, and that the Juju is the final arbitrer of the reported status of a Charm and its units. I mean to exemplify this with the example of ActiveStatus which is set by the Charm, without which Juju well not report the unit as active, however despite it Juju may wait till pods are running before reporting the units as active.16:49
jambthomas, I probably would phrase it differently, and just treat active as a special case, because all other cases it is exactly the current status as you specify it.16:51
bthomasjam: Ah ok. Thank you. So Juju honours every other status rigidly except ActiveStatus. Got it. Will amend my wording.16:52
* bthomas EOD but maybe bbiab17:20
* facubatista -> bbl18:41
justinclarkjam, Chipaca, facubatista (when back) - I have a question about the "model.pod.set_spec()" method. I was really struggling to figure out why my Grafana configuration wasn't working but it turns out that the new pod was never actually running. After restarting the pod directly with kubectl, everything works as expected. However, I was kind of assuming "model.pod.set_spec()" would restart the pod with the new19:09
justinclarkIs there something in addition to "set_spec()" that I need to do to make sure the new pod is actually started?19:10
justinclarkHappy to jump on a quick call since it might be easier to just show what's happening19:12
justinclarkbthomas: ^^ this implies that the prometheus charm is doing what it's supposed to do since I can create dashboards with prometheus data as soon as the updated pod spec is live.19:15
bthomasjustinclark: good to hear. the pod.set_spec issue baffels me though.19:15
bthomasjustinclark: what is the state of you pods without using kubectl to restart them.19:16
justinclarkThey're all running with the original configuration (before relating Prometheus)19:17
bthomasI was just going to say Ah never mind :) since I reread your post and realized just the same.19:17
justinclarkMy hacky solution was "microk8s.kubectl scale -n lma --replicas=0 statefulset.apps/grafana"19:18
justinclarkfollowed by19:18
justinclark"microk8s.kubectl scale -n lma --replicas=1 statefulset.apps/grafana"19:18
bthomasMy understanding is that if you do set_spec, the underlying k8s deployment should see a new spec and restart pods. Obviously this does not seem to be happening in your case.19:19
justinclarkThat is (was) my understanding as well.19:20
bthomasjustinclark: what is your github id, so I can look at the code.19:21
bthomasjustinclark: As yet looking at source I am not seeing an obvious bug. Makes me feel chasing this may require looking at logs. I will look at the source once again.19:40
justinclarkYeah we could very well be missing something. It just seems weird that the new pod spec is correctly set and all newly created configuration files make it to the pod, but the pod itself is just never restarted.19:43
bthomasSo I infer you actually checked, that before you did kubectl scale, the config file for grafana with the new data source for prometheus, had actually been written into the appropriate mount path.19:50
justinclarkYep - those files are all there before restarting. Checked with "microk8s.kubectl exec grafana-0 -n lma -- cat /etc/grafana/provisioning/datasources/datasources.yaml"19:51
bthomasExcellent. if there is a way to tell grafana to reload its config, without creating a new pod, does that work.19:51
bthomasyour can try a kubectl exec some bash command to grafana (SIGHUP ?) or some python code from charm to nudge grafana maybe19:52
bthomasAlso you can watch kubectl all to know if new pods were actually created when you did set_spec19:54
justinclarkLet me check. I do know the Grafana docs say to restart Grafana before new config options are set. I think watching will also be helpful.19:55
bthomasOh! "restart Grafana before new config" ... or "after new config" . Also don't forget to check juju debug-log for anything interesting19:59
* bthomas goes to hammer another nail in his coffin -- cough cough19:59
justinclarkHaha don't stay up too late for my sake. I'll hopefully get it figured out soon. The juju/ops wizards might have ideas as well.20:04
* justinclark aspires to be a juju/ops wizard someday20:04
* bthomas is back20:06
bthomaswill be up for about 40 mins or so20:06
justinclarkLooks like the only way to restart Grafana is to restart the container it's in20:24
justinclarkI'm wondering if it is somehow related to the fact that only a mounted file is changing - rather than anything specific to the kubernetes pod spec. I've seen somewhere that pod_spec_set is idempotent so if there is no change to the spec, the pod won't be restarted.20:27
justinclarkIf the only change that is checked is the kubernetes pod spec (not the entire juju pod spec), then that might be the issue.20:29
justinclarkAlthough it's more likely that the issue is on my end rather than on Juju's end20:30
bthomasjustinclark: Chipaca: mentioned that if ConfigMaps changed then setting pod spec does not actually create new pods.20:45
bthomasI am begining to think that this is the problem you may be dealing with. You can confirm this by watching kubectl all and checking if new pods are actually created in response to adding a prometheus relation.20:46
bthomasIf new pods are not being created and the likely cause is the ConfigMap issue, you may try a workaround where you create a new variable in the pods spec that is not part of the ConfigMap and just change that variable along with appending the new grafana config file to the files container.20:48
justinclarkAh yes that's probably exactly the issue. I can confirm that watching kubectl all does not change any of the pods/statefulsets after adding the prometheus relation.20:51
justinclarkI'll try the variable workaround.20:51
bthomasjustinclark: If kubectl all shows that new pods are not created then I am not aware of any reason that can cause this other than the ConfigMap issue. Hopefully we will soon have all such gotchas carefully documented so that it will no cause any one of us to stumble on such things. Good luck with the work around.21:06
bthomasI will call it a night. See you tomorrow.21:06
justinclarkI appreciate the help. Thanks!21:06
* facubatista is back21:33
facubatistajustinclark, probably there should be a debug message somewhere that indicates what's going on...21:39
facubatistajustinclark, I mean, I'm not saying that *there is*, but that there *should be*21:39
justinclarkAgreed. I couldn't find anything to indicate that problem. But I just confirmed that this is the ConfigMap issue that bthomas talked about is in fact the issue.21:40
* justinclark does celebratory dance21:40
facubatistajustinclark, did you collect all logs for the sequence? so we can explore them with jam21:41
facubatista(at least for all of us to understand better the logs!)21:42
justinclarkYes - here are the logs:21:47
justinclarkThe join event happens right around 17:33:22 in the logs.21:49
justinclarkOh, to be clear, those ^^ are the logs with the new changes that fix the issue. I'll send the logs before getting the pod restart to trigger21:52
justinclarkThese are the logs before the issue was fixed: https://pastebin.canonical.com/p/CK5BsvWgp4/21:55
justinclarkI see "controller-0: 17:33:31 DEBUG juju.worker.caasunitprovisioner deployment changed: true" in the fixed logs, but that is absent in the broken logs - not even something like "deployment changed: false"22:01
facubatistajustinclark, wonderful, thanks! I'll check on those22:18
facubatista(monday, I'm eoding and eowing now)22:18
justinclarkSounds great. I appreciate it! Have a good weekend22:19
* facubatista runs22:28

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