| wallyworld | vino: PR looks pretty good so far. left a comment about the AllBindings() method | 00:36 |
|---|---|---|
| vino | Sure thanks wallyworld | 00:41 |
| vino | i will get back to u soon. | 00:42 |
| kelvinliu | hi wallyworld, the mock Stdin pipe has been solved. | 02:06 |
| wallyworld | kelvinliu: that's great! i'll look at PR | 02:22 |
| kelvinliu | thx, wallyworld | 02:23 |
| wallyworld | kelvinliu: looking good, see my comments. happy to clarify if needed | 02:44 |
| kelvinliu | wallyworld, can i have ur 2mins ? | 03:19 |
| wallyworld | sure | 03:19 |
| veebers | wallyworld: you have a couple of moments, would like some pointers for the model tear down issue. I had a good destroy and a bad destroy and both exihibited the same logging (that I had tacked in there) So I'm missing something and/or looking in the wrong spot | 03:34 |
| wallyworld | veebers: just talking to kelvin, give me a sec | 03:35 |
| veebers | wallyworld: sure thing | 03:35 |
| === jhebden-afk is now known as jhebden | ||
| anastasiamac | can i get a review for https://github.com/juju/juju/pull/8724, please? more status tests - this time for filtering on machine number :D | 03:43 |
| wallyworld | veebers: free now but i fear i won't have a good answer for you | 03:43 |
| wallyworld | anastasiamac: can look soon | 03:43 |
| anastasiamac | wallyworld: just as i was going to say "could u plz have a good answer for me" :D | 03:44 |
| veebers | wallyworld: hah, all good, even just pointing in maybe the right direction would be good | 03:44 |
| wallyworld | veebers: am inhangout | 03:44 |
| wallyworld | stabdup | 03:44 |
| veebers | omw | 03:44 |
| wallyworld | anastasiamac: a couple of small things | 04:02 |
| babbageclunk | so I've got a fix for that test hang but writing the explanation in the commit message has got me worried about a bug in the raft implementation | 04:07 |
| anastasiamac | best type of tests/comments :D | 04:07 |
| veebers | babbageclunk: remove the comment and voila bug is gone | 04:07 |
| babbageclunk | I'm gonna push the one-line fix with a shruggie in the commit message | 04:12 |
| kelvinliu | wallyworld, cluster-name and context-name in ~/.kube/config file are different, and we are using context-name to find related context from ClientConfig.Contexts. Do u think we should change the `context-name` flag to `cluster-name` and then we find the related context-name according to the cluster-name? | 04:14 |
| anastasiamac | wallyworld: ta \o/ | 04:15 |
| wallyworld | kelvinliu: if it works. from a user perspective, i think they look at a named k8s setup as a cluster? | 04:15 |
| wallyworld | kelvinliu: are cluster names unique within a given config file? | 04:17 |
| wallyworld | or can different contexts have the same cluster name | 04:17 |
| wallyworld | if cluster names are not unique then we need to stick with context name | 04:17 |
| kelvinliu | i think context-name <- 1: 1 -> cluster-name, and they should be all uniq | 04:20 |
| wallyworld | ok, then i reckon cluster-name is better | 04:31 |
| kelvinliu | wallyworld, yeah, changing it to use cluster-name now | 04:40 |
| wallyworld | great, ty | 04:40 |
| babbageclunk | easy review anyone? https://github.com/juju/juju/pull/8725 | 04:55 |
| babbageclunk | anastasiamac, veebers, thumper, wallyworld: ^ | 04:56 |
| veebers | babbageclunk: looks | 04:57 |
| veebers | looking even | 04:57 |
| babbageclunk | ta | 04:57 |
| veebers | babbageclunk: I've asked a question for clarification. Im never sure to mark that as a 'Comment' or a 'Request changes' | 04:58 |
| babbageclunk | veebers: I'd say a comment unless you've specifically asked for a change. | 04:59 |
| veebers | ack, makes sense. | 05:00 |
| kelvinliu | wallyworld, done. would u mind take a look on the change? | 05:00 |
| wallyworld | kelvinliu: looking | 05:01 |
| kelvinliu | wallyworld, thx | 05:01 |
| wallyworld | anastasiamac: maybe if you have a chance, here's a CLI command to remove a k8s cloud (builds on remove cloud stuff er discussed) https://github.com/juju/juju/pull/8726 | 05:01 |
| veebers | wallyworld: if I understood correctly, Application destroyOps should be called many times if any of the asserts fail, right? | 05:01 |
| wallyworld | veebers: the build loop will get called 3 times. depending on what happens in the txns, potentially new cleanup jobs will be queued which will call destroy again | 05:02 |
| anastasiamac | wallyworld: sure thing.. as soon as i finish this error msg m working on \o/ | 05:02 |
| anastasiamac | like "on the tip of my tongue" kind of moment... | 05:03 |
| wallyworld | no rush need to land kelvin's one first and then rebase as there will be conflicts | 05:03 |
| babbageclunk | veebers: that's a raft library function - I guess we could have our own local DefaultConfig func somewhere... | 05:04 |
| veebers | babbageclunk: ah right, makes sense. Not sure it's worth it just to set False for that arg (for real and tests) | 05:05 |
| babbageclunk | yeah, I was hoping you'd agree :) | 05:05 |
| veebers | :-) | 05:06 |
| wallyworld | kelvinliu: done, just a couple of minor typos, let's fix and land :-) | 05:06 |
| veebers | wallyworld: ok, so I see the logging I put in (a *Application) destroyOps output only once, however I see the application fail to be destroyed. I'm continuing to add more debugging everywhere (well, maybe not everywhere) | 05:07 |
| kelvinliu | wallyworld, fixed | 05:09 |
| wallyworld | kelvinliu: go ahead and $$merge$$ | 05:11 |
| kelvinliu | yup thx wallyworld | 05:11 |
| wallyworld | veebers: destroyOps will only get called again if it results in another cleanup job gettin queued which calls it | 05:11 |
| wallyworld | debugging should help us figure it out (eventually) | 05:12 |
| veebers | wallyworld: ah right. Ok, will continue w/ debugging logs :-) | 05:12 |
| wallyworld | no easy answer sadly | 05:12 |
| wallyworld | will also help to print what txns are being queued | 05:13 |
| wallyworld | potentially | 05:13 |
| veebers | wallyworld: ack, that's a good way to fill up a log file :-) | 05:13 |
| anastasiamac | wallyworld: if u have a chance, PTAL https://github.com/juju/juju/pull/8727? | 05:32 |
| * anastasiamac lloking at 8726 now | 05:32 | |
| wallyworld | righto | 05:32 |
| wallyworld | anastasiamac: all of our state code returns jujutxn.ErrNoOperations if attempt > 0 and the entity is gone | 06:09 |
| wallyworld | the general pattern is to do a Refresh() and check for not found | 06:10 |
| wallyworld | cloud don't have refresh() so we just use Cloud(name) | 06:10 |
| wallyworld | eg see line 186 of state/application.go | 06:11 |
| anastasiamac | wallyworld: plz send a link, i dont know which version of application.go u r looking at :) | 06:13 |
| anastasiamac | wallyworld: most of the code does not fuss about and just returns NotFound... However, I do believe u want to cater for something specific, hence,u've coded for it but I do not understnad the reasoning... maybe it deserves a descriptive comment? | 06:14 |
| anastasiamac | wallyworld: specifically, why would it matter if another call deleted the entity? ur code above still should cater for both NOtFound and NoOp.. right? | 06:15 |
| anastasiamac | wallyworld: i guess what I'd like to see is a test that if you got a no-op in state, under the circumstances u've described, then ur api call will not err and hence local store will still be cleaned up... even if someone else removed controller-side reference | 06:17 |
| wallyworld | anastasiamac: i added a remove race test. the application file is the one in state /home/ian/juju/go/src/github.com/juju/juju/state/application.go | 06:17 |
| wallyworld | tremove cloud behaves consistently as for removing app, machine etc | 06:18 |
| anastasiamac | wallyworld: :) i was hoping for a link on githu :) i understnad which ile u r talking about... i cannot guess its version, nor can navigate to what u've supplied ;) | 06:18 |
| wallyworld | this is on develop | 06:18 |
| wallyworld | https://github.com/juju/juju/blob/develop/state/application.go#L186 | 06:19 |
| anastasiamac | yep | 06:20 |
| anastasiamac | awesome ! Tahnk you the race test :D | 06:23 |
| anastasiamac | thank* | 06:23 |
| === frankban|afk is now known as frankban | ||
| === disposable3 is now known as disposable2 | ||
| === salmankhan1 is now known as salmankhan | ||
| === rogpeppe1 is now known as rogpeppe | ||
| manadart | externalreality: If you are inclined to review: https://github.com/juju/juju/pull/8730 | 14:36 |
| === frankban is now known as frankban|afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!