| TheMue | morning | 06:33 |
|---|---|---|
| fwereade | TheMue, heyhey | 06:57 |
| TheMue | fwereade: heya | 06:58 |
| TheMue | ooops, disconnected *hmmm* | 08:52 |
| Aram | moin. | 09:44 |
| TheMue | Aram: moin | 09:55 |
| TheMue | lunchtime | 10:42 |
| rogpeppe | fwereade: fairly trivial: https://codereview.appspot.com/6576053 | 10:45 |
| fwereade | rogpeppe, cheers | 10:46 |
| fwereade | rogpeppe, LGTM | 10:47 |
| rogpeppe | fwereade: ta | 10:47 |
| rogpeppe | fwereade: and another, even more trivial: https://codereview.appspot.com/6566060 | 10:52 |
| fwereade | rogpeppe, LGTM | 10:53 |
| rogpeppe | fwereade: tyvm | 10:53 |
| rogpeppe | fwereade: i think i'll submit both, given your remark | 10:56 |
| fwereade | rogpeppe, +1 | 10:56 |
| TheMue | have one too: https://codereview.appspot.com/6567060 | 11:33 |
| rogpeppe | TheMue: i would look, but i can't seem to pull from lp currently | 11:43 |
| rogpeppe | TheMue: rather, i was looking, but i wanted to look more closely but failed to talk to launchpad | 11:43 |
| rogpeppe | TheMue: i can't even pull from trunk currently | 11:43 |
| TheMue | rogpeppe: oh, which error message? | 11:43 |
| rogpeppe | Aram, fwereade: are you having any problems accessing launchpad through bzr? | 11:43 |
| TheMue | rogpeppe: i just pulled trunk and it worked | 11:44 |
| Aram | same | 11:44 |
| Aram | workes | 11:44 |
| rogpeppe | TheMue: http://paste.ubuntu.com/1230255/ | 11:44 |
| Aram | works | 11:44 |
| TheMue | rogpeppe: iiirks, never seen that | 11:45 |
| rogpeppe | TheMue: yeah, i don't know what's going on - it worked fine earlier | 11:45 |
| rogpeppe | i wonder whether my password has expired or something | 11:46 |
| rogpeppe | i suspect it may be something to do with using a long branch name | 11:50 |
| rogpeppe | lbox just managed to push fine. weird | 11:50 |
| rogpeppe | TheMue: all working now. who knows what was going on... | 12:00 |
| TheMue | rogpeppe: maybe service-side | 12:00 |
| rogpeppe | lol, i was trying to find the definition for the "delete" function, totally forgetting it was built in! | 12:01 |
| Aram | :) | 12:02 |
| TheMue | Aram: seem i found a bug in the machines watcher | 12:16 |
| TheMue | Aram: typically a machine can be removed after EnsureDead() | 12:17 |
| rogpeppe | TheMue: you've got a review | 12:18 |
| TheMue | Aram: when testing the firewaller and remove a machine i get nor error returned but the machine watcher panics with "machine removed before being dead" | 12:18 |
| TheMue | rogpeppe: just seen, cheers | 12:18 |
| Aram | TheMue: I know, the implementation was correct and niemeyer broke it. | 12:19 |
| TheMue | rogpeppe: good hints, thx | 12:19 |
| * Aram is back in 15 minutes. | 12:19 | |
| rogpeppe | fwereade: another one (this one makes live tests pass, yay!): https://codereview.appspot.com/6566058/ | 12:21 |
| * fwereade looks | 12:21 | |
| fwereade | rogpeppe, LGTM :) | 12:24 |
| rogpeppe | fwereade: thanks! | 12:25 |
| rogpeppe | oh why can i never remember how to print a full revision id in bzr?! | 12:39 |
| rogpeppe | nor is it easy to google for | 12:39 |
| Aram | fwereade: meh, how I wish we had actual container entities. | 12:59 |
| fwereade | Aram, heh, me too, I remain totally convinced it's the Right Thing to do | 12:59 |
| Aram | the machine units watcher is ridiculously complex becase we don't, I have to watch 1) the machine, 2) n principal units, 3) all the units and take great care to integrate and use all this knowledge. | 13:00 |
| rogpeppe | fwereade: the uniter tests seem to spend ages and ages doing: want resolved mode '\x00', got '\x02'; still waiting | 13:04 |
| rogpeppe | fwereade: and similar | 13:04 |
| fwereade | rogpeppe, hell, niemeyer had that yesterday; I was unable to repro :/ | 13:04 |
| rogpeppe | fwereade: the uniter tests passed for me just now but took 124s to run | 13:04 |
| fwereade | rogpeppe, they do indeed take a long time: some of it may be the wanton fsyncing but probably not all | 13:05 |
| fwereade | rogpeppe, I'm working on a suggestion of niemeyer's at the moment which changes the uniter quite interestingly, though so I was kinda hoping that would induce magical improvement somewhere ;) | 13:06 |
| rogpeppe | fwereade: i'll paste you a copy of the output of the test, with timestamps so you can see where the time is going | 13:06 |
| rogpeppe | fwereade: lol | 13:06 |
| Aram | fwereade: for the machine units watcher, we only want to deliver events if there's a change in lifecycle, not a change in any other attribute, right? | 13:06 |
| rogpeppe | fwereade: interesting, when i ran it that time, it failed with "never reached desired status" | 13:07 |
| fwereade | rogpeppe, yeah, indeed, there is clearly something funky wrt Resolved -- that is one of the bits that is changing, actually | 13:07 |
| fwereade | Aram, I think so, yes | 13:08 |
| fwereade | Aram, tedious, innit | 13:08 |
| rogpeppe | fwereade: http://paste.ubuntu.com/1230376/ | 13:09 |
| fwereade | rogpeppe, cheers | 13:09 |
| rogpeppe | fwereade: that ouput is actually from a branch other than trunk | 13:10 |
| rogpeppe | fwereade: i'll try in trunk and see if the same thing happens - i might have broken something | 13:10 |
| Aram | there's a huge impedance mismatch between state/watcher and what we actually want. I don't find any purpose to state/watcher, we could have implemented watchers just the way we wanted withoout an aditional 1kLOC layer, and they would have been simpler. | 13:10 |
| rogpeppe | Aram: how would you have implemented watchers? | 13:11 |
| Aram | rogpeppe: I actually did like 4 or 5 times. | 13:11 |
| Aram | let me find a branch. | 13:11 |
| fwereade | rogpeppe, I wouldn't worry too much about it, I will be looking into it | 13:11 |
| rogpeppe | Aram: i think the point of state/watcher is that it's efficient enough to cope with many thousands of documents | 13:12 |
| rogpeppe | niemeyer: yo! | 13:12 |
| niemeyer | Good mornign! | 13:13 |
| TheMue | morning | 13:14 |
| niemeyer | rogpeppe: Just sent some comments on the branch | 13:15 |
| niemeyer | rogpeppe: Thanks for fixing the tests | 13:16 |
| rogpeppe | niemeyer: cheers | 13:16 |
| niemeyer | rogpeppe: The tools waiter looks like a nice idea, but it's sprawling too far IMO | 13:16 |
| niemeyer | rogpeppe: A tool waiter should wait for the tools, and nothing else. It's messing up the already messy situation even further. | 13:16 |
| rogpeppe | niemeyer: ok. i thought it was quite neat to bundle up the watcher with the thing it's watching, but evidently not. | 13:18 |
| niemeyer | fwereade: ping | 13:18 |
| fwereade | niemeyer, pong | 13:18 |
| niemeyer | rogpeppe: It's a tool waiter | 13:18 |
| niemeyer | rogpeppe: It's not nice to bundle things with arbitrary things so we can use the underlying things' interface | 13:18 |
| niemeyer | fwereade: yo | 13:18 |
| niemeyer | fwereade: Couple of questions | 13:18 |
| niemeyer | fwereade: Did you figure what was wrong with the uniter tests? I think I can still reproduce | 13:19 |
| fwereade | niemeyer, listening (btw thank you for excellent advice re uniter, it seems to be coming along pretty nicely) | 13:19 |
| fwereade | niemeyer, afraid not; rog was just hitting it today | 13:19 |
| niemeyer | fwereade: My pleasure, glad it was useful | 13:19 |
| niemeyer | fwereade: Okay, I'll dive in right away then | 13:19 |
| rogpeppe | niemeyer: the reason it made things nicer is that otherwise we end up always passing around the object along with the watcher channel. | 13:19 |
| niemeyer | fwereade: Please keep going in whatever you were doing before.. :-) | 13:19 |
| fwereade | niemeyer, my suspicion is that my ClearResolved() is funky, it looked off when I was changing it today | 13:19 |
| rogpeppe | niemeyer: the waiter object made that more straightforward by bundling them together | 13:20 |
| niemeyer | rogpeppe: It's a tools waiter.. it's not reasonable to have a tool waiter sprawling the live tests.. makes no sense whatsoever to be saying ToolsWaiter.EnsureDead (!!) | 13:20 |
| rogpeppe | niemeyer: i admit that was a bit of a short cut :-) | 13:21 |
| niemeyer | rogpeppe: Heh | 13:21 |
| niemeyer | rogpeppe: These tests should be cleaned up, reduced in size and scope.. that's the opposite of it | 13:21 |
| rogpeppe | niemeyer: i'd like to have a chat about that | 13:22 |
| niemeyer | rogpeppe: Cool, let's just extinguish the fire first, and then I'm happy to discuss it | 13:22 |
| rogpeppe | niemeyer: i'd like to do that, but i don't want the time taken to run the tests to double. | 13:22 |
| niemeyer | rogpeppe: They won't double.. we already have BootstrapOnce | 13:22 |
| rogpeppe | niemeyer: tests like this one start a unit. | 13:23 |
| rogpeppe | niemeyer: that takes almost as long as bootstrapping | 13:23 |
| rogpeppe | niemeyer: so it makes sense to test some stuff with the unit once we've started it, i think. | 13:23 |
| niemeyer | rogpeppe: BootstrapOnce can create a default unit, with a well known testing charm | 13:24 |
| niemeyer | rogpeppe: Which follow the same rules of the first machine (do not mess with it, and if you do, put it back in place) | 13:24 |
| rogpeppe | niemeyer: i think that's wrong, for the same reason it was wrong in the other tests we changed recently. | 13:24 |
| niemeyer | rogpeppe: Curiously, you won't be able to do that for upgrades, so I don't think it's too much to upgrade it | 13:24 |
| rogpeppe | niemeyer: we don't want loads of arbitrary context in the suite | 13:24 |
| niemeyer | rogpeppe: Erm, to deploy a new unit | 13:25 |
| niemeyer | rogpeppe: Yep, so let's pay the price of time | 13:25 |
| niemeyer | rogpeppe: Pick your fight :) | 13:25 |
| rogpeppe | niemeyer: live tests already take 10 minutes. i don't want them to take 20. | 13:25 |
| niemeyer | rogpeppe: I don't care | 13:25 |
| niemeyer | rogpeppe: I won't be sitting and looking at the screen in either case | 13:26 |
| niemeyer | rogpeppe: I do care, though, that we have incomprehensible and long-winded tests | 13:26 |
| niemeyer | rogpeppe: Which took a week to fix when we changed something fundamental | 13:26 |
| niemeyer | rogpeppe: *one* test | 13:26 |
| rogpeppe | niemeyer: the difficulty i'm having is that all the tests i'd factor out are subsets of the current Bootstrap and Deploy test. that is, to test upgrading, i think we need to do almost exactly what we already do in that test. | 13:28 |
| niemeyer | rogpeppe: That's why we have a BootstrapOnce test | 13:29 |
| niemeyer | rogpeppe: s/test/helper/ | 13:29 |
| niemeyer | rogpeppe: Because it's fundamental to live tests | 13:29 |
| niemeyer | rogpeppe: Everything else is not | 13:29 |
| niemeyer | rogpeppe: Of course you need bootstrap to test deploy, of course we need bootstrap to test upgrade, of course etc etc | 13:30 |
| rogpeppe | niemeyer: how can i test upgrade without doing a deploy? | 13:30 |
| niemeyer | rogpeppe: But that's certainly not reasoning to put *all of those tests* in the same place | 13:30 |
| niemeyer | rogpeppe: Otherwise all we know is "Huh.. the big blob of code is broken again!" | 13:30 |
| niemeyer | rogpeppe: You can do a deploy without testing upgrade | 13:31 |
| rogpeppe | niemeyer: sure. then we have a set of tests, each of which includes the other as a prefix. i'm with you, but i don't want our live tests to take 2 hours | 13:32 |
| niemeyer | rogpeppe: What upgrade is a prefix of? | 13:32 |
| rogpeppe | niemeyer: deploy is a prefix of upgrade. maybe upgrade is as far as it gets, yes. | 13:32 |
| niemeyer | rogpeppe: bingo | 13:32 |
| niemeyer | rogpeppe: Bootstrap and deploying things are fundamental.. everything else is not | 13:33 |
| rogpeppe | niemeyer: so... does it make sense to have deploy as a separate test, when upgrade will test *exactly* the same path? | 13:33 |
| niemeyer | rogpeppe: Does it make sense to know that deploy works, despite the fact that upgrade doesn't? | 13:33 |
| rogpeppe | niemeyer: sure. but our test failure will tell us that by where it failed. i don't think it justifies an extra three minutes testing time. | 13:34 |
| niemeyer | rogpeppe: So let's erase all the tests.. 0 minutes | 13:35 |
| rogpeppe | niemeyer: now yer being silly :-) | 13:35 |
| niemeyer | rogpeppe: Where's the value? | 13:35 |
| niemeyer | rogpeppe: I am | 13:35 |
| niemeyer | rogpeppe: But that's the point.. you're worried about timing and ignoring everything else | 13:35 |
| niemeyer | rogpeppe: THere are other ways to optimize time | 13:35 |
| niemeyer | rogpeppe: Running tests in parallel for instance | 13:35 |
| niemeyer | rogpeppe: But if you take away the value of the test, there's little reason to have them | 13:36 |
| niemeyer | rogpeppe: All we have is a black/white works/doesn't work | 13:36 |
| rogpeppe | niemeyer: does that mean table-driven tests are of no value because they're all bundled into a single test? | 13:36 |
| niemeyer | rogpeppe: Can't see that leap | 13:36 |
| niemeyer | rogpeppe: "table tests" are generally very contained in scope | 13:37 |
| niemeyer | rogpeppe: Bootstrapping, deploying, upgrading, deploying more units, remove units, test firewaller, open ports, close ports, check expose, blah blah blah | 13:38 |
| rogpeppe | niemeyer: ok, i'll change it and see how it looks | 13:39 |
| niemeyer | rogpeppe: hold on, this is all future work.. let's please make things stable first | 13:40 |
| rogpeppe | niemeyer: ok. so what do you want me to do about the current branch? | 13:40 |
| niemeyer | rogpeppe: Look at the review, and move forward? | 13:40 |
| rogpeppe | niemeyer: ok. i thought you weren't keen on most of what i was doing there. | 13:41 |
| niemeyer | rogpeppe: I've sent a review that details exactly my feeling about what's in that branch | 13:41 |
| niemeyer | rogpeppe: And I've started the conversation saying that I appreciate the refactoring you did to wait for tools | 13:42 |
| niemeyer | rogpeppe: And I also said I didn't want to get into this discussion before the fire was extinguished | 13:42 |
| rogpeppe | niemeyer: fair enough | 13:42 |
| rogpeppe | niemeyer: how about if toolsWaiter was named differently? i think the bundling together of object and channel works well - it made the code simpler. | 13:44 |
| niemeyer | rogpeppe: :-( | 13:45 |
| niemeyer | rogpeppe: Can we please make the tools waiter *wait for tools*!? | 13:46 |
| rogpeppe | niemeyer: ok, i think i'm there | 13:46 |
| niemeyer | rogpeppe: Thanks | 13:46 |
| niemeyer | fwereade: Curiously, the tests are passing now :-( | 14:03 |
| * niemeyer looks at the paste | 14:03 | |
| fwereade | niemeyer, yeah, I did see it once myself -- have a small amount of patience, because your suggestions led to a certain amount of rearrangement that happened to somewhat affect error resolution | 14:04 |
| fwereade | niemeyer, but I'm not 100% there yet | 14:04 |
| niemeyer | fwereade: Cool | 14:04 |
| rogpeppe | niemeyer: i think this might be better: https://codereview.appspot.com/6566058 | 14:08 |
| * rogpeppe goes for a bite of lunch | 14:09 | |
| niemeyer | rogpeppe: Thanks, LGTM. Just sent a couple of questions for your consideration | 14:15 |
| rogpeppe | niemeyer: the advantage of having watcher as an embedded field is just so we don't need to define another Stop method. but i guess it's probably best to be explicit and do that anyway. | 14:18 |
| niemeyer | rogpeppe: Okay, I personally don't mind much either way in that case.. whatever you're happy with | 14:19 |
| rogpeppe | niemeyer: we don't need a refresh because waitAgentTools has already done a Refresh, but again, maybe it's best to do it so it's obvious | 14:19 |
| rogpeppe | niemeyer: i'm happy to do that if you prefer | 14:20 |
| niemeyer | rogpeppe: Yeah, sounds slightly more resilient.. we could change the tools waiter without breaking the test | 14:21 |
| rogpeppe | niemeyer: ok, will do. thanks for the review BTW | 14:21 |
| niemeyer | rogpeppe: My pleasure, and sorry for the confusion | 14:21 |
| rogpeppe | niemeyer: np | 14:21 |
| niemeyer | I think I was the go fmt offender.. | 14:25 |
| niemeyer | I stumble upon it every other commit | 14:26 |
| rogpeppe | niemeyer: i do it quite a bit too :-) | 14:29 |
| niemeyer | fwereade: You might appreciate this one: https://codereview.appspot.com/6565057 | 14:30 |
| niemeyer | fwereade: Figure it while having a look at uniter tests runs.. :) | 14:30 |
| niemeyer | rogpeppe: ;) | 14:30 |
| fwereade | niemeyer, yay! | 14:30 |
| fwereade | niemeyer, LGTM | 14:31 |
| fwereade | niemeyer, that was on my copious-free-time list too :) | 14:31 |
| niemeyer | fwereade: The uniter test that broke in the paste was relative to resolution | 14:32 |
| niemeyer | fwereade: I'm wondering if it's just a sync race | 14:32 |
| niemeyer | fwereade: I'll see if I can reproduce this afternoon | 14:32 |
| TheMue | niemeyer: ping | 14:32 |
| niemeyer | TheMue: Yo | 14:33 |
| fwereade | niemeyer, hmm, I feel like that bit *should* be pretty clean, but I'm not certain | 14:33 |
| TheMue | niemeyer: the MachinesWatcher seems to have a problem | 14:33 |
| niemeyer | TheMue: Oh? | 14:33 |
| TheMue | niemeyer: while the test of RemoveMachine() in state works I now tried it for the firewaller | 14:33 |
| TheMue | niemeyer: and there I get no error in EnsureDead() and RemoveMachine() but a panic in the watcher telling me that the machine is not dead | 14:34 |
| niemeyer | TheMue: Ah, interesting.. I'd like to have a closer look at that, as I'm about to refactor that watcher to use a slice of changes too | 14:35 |
| niemeyer | TheMue: Do you have a reproducer? | 14:35 |
| TheMue | niemeyer: currently in an own little branch of the firewaller test. but i'll set up an extra branch for you | 14:36 |
| niemeyer | TheMue: ? | 14:36 |
| niemeyer | TheMue: Thanks, please push something, or just put the test in a paste and send a link | 14:37 |
| niemeyer | TheMue: I'll have a look after lunch | 14:37 |
| TheMue | niemeyer: ok, will do | 14:37 |
| niemeyer | I'll step out for an earlier lunch today to join Ale.. see you all soon | 14:39 |
| rogpeppe | niemeyer, fwereade: i still get sporadic failures of worker/uniter | 14:39 |
| rogpeppe | niemeyer: but that's the only failure | 14:40 |
| rogpeppe | niemeyer: enjoy! | 14:40 |
| niemeyer | rogpeppe: It's known.. I'll try to reproduce in the afternoon, and fwereade is also refactoring things somewhat | 14:40 |
| rogpeppe | niemeyer: a slightly reddish shade of green then :-) | 14:40 |
| niemeyer | rogpeppe: The message is the same: tests should pass before you merge. If something broke, don't commit | 14:41 |
| * niemeyer steps out | 14:41 | |
| rogpeppe | uniter upgrade tests pass, yay! | 15:42 |
| rogpeppe | (live) | 15:42 |
| * fwereade cheers ar rogpeppe | 15:57 | |
| fwereade | well holy shit | 15:58 |
| fwereade | after that *brutal* refactoring, the uniter tests pass in about half the time they did before | 15:59 |
| rogpeppe | fwereade: trivial: https://codereview.appspot.com/6566063 | 15:59 |
| * rogpeppe cheers at fwereade | 15:59 | |
| fwereade | rogpeppe, (and worked second time after I got it to build, too, the only bug (detected thus far...) was errorContextf-related) | 16:00 |
| rogpeppe | fwereade: nice one | 16:00 |
| fwereade | rogpeppe, LGTM | 16:00 |
| fwereade | rogpeppe, bbs | 16:01 |
| rogpeppe | fwereade: so how long do the uniter tests take now? | 16:01 |
| rogpeppe | k | 16:01 |
| * niemeyer waves | 16:01 | |
| fwereade | rogpeppe, bit more than a minute | 16:02 |
| niemeyer | TheMue: ping | 16:02 |
| fwereade | niemeyer, that suggestion was *awesome* | 16:02 |
| niemeyer | fwereade: Woohay! | 16:02 |
| TheMue | niemeyer: pong | 16:02 |
| fwereade | niemeyer, I think I remained true to its spirit, just got it building, and OMG at least twice as fast | 16:02 |
| fwereade | niemeyer, but I have to go... you'll get a CL tonight for sure, unless I discover some subtle screwup | 16:03 |
| TheMue | niemeyer: it is not reproducable standalone, only in the firewaller | 16:03 |
| TheMue | niemeyer: shall i push that branch so that you can test it there? | 16:03 |
| niemeyer | TheMue: Yeah, or even just a tiny test for the firewaller that exposes the bug | 16:04 |
| TheMue | niemeyer: looked into the watcher. the revno = -1 seems to come to early, because the machine is still in w.alive | 16:04 |
| niemeyer | TheMue: I've sent a review too | 16:04 |
| TheMue | niemeyer: cheers | 16:05 |
| niemeyer | TheMue: Yeah, I want to look at that.. I've put the panic mainly so I'd see those cases | 16:05 |
| TheMue | niemeyer: I paste the test, one moment please | 16:05 |
| niemeyer | TheMue: Thanks! | 16:05 |
| niemeyer | fwereade: Superb, that sounds very exciting! | 16:05 |
| rogpeppe | niemeyer: uniter upgrade, finally: https://codereview.appspot.com/6561063 | 16:06 |
| niemeyer | rogpeppe: Looking | 16:06 |
| TheMue | niemeyer: http://paste.ubuntu.com/1230645/ , the panic is in merge() of the MachinesWatcher | 16:08 |
| niemeyer | TheMue: Thanks | 16:08 |
| niemeyer | TheMue: please let me know if you have any questions on the review sent | 16:09 |
| TheMue | niemeyer: the fixed bug had no failing effect, I only discoverd that my removing of the service not worked. and then i've seen that the map contained the unit with two ids: the service name and the unit name. but we never looked for the service name, so no error. but my length test just didn't became 0. | 16:13 |
| TheMue | niemeyer: the other comments a pretty clear, thanks | 16:13 |
| niemeyer | TheMue: That's how you discovered the bug | 16:14 |
| niemeyer | TheMue: But does it really have "no failing effect"? Why do we use this map at all then? | 16:14 |
| TheMue | niemeyer: yes, but before it showed no effect | 16:15 |
| niemeyer | TheMue: This sounds like a missing test? | 16:15 |
| niemeyer | TheMue: Why is the map used? | 16:15 |
| TheMue | niemeyer: the map only contains one entry with the service name and the last added unitd | 16:15 |
| TheMue | niemeyer: now it is used for me to stop the serviced, but i have to look why we have added it once | 16:16 |
| niemeyer | TheMue: Sorry, that doesn't sound right | 16:16 |
| niemeyer | TheMue: Feels very hand-wavy | 16:16 |
| niemeyer | TheMue: Why have you added that map in the first place? | 16:16 |
| niemeyer | TheMue: Where is it used? | 16:16 |
| TheMue | niemeyer: that's what i wanted to say. i right now don't remember and have to look for the reason | 16:17 |
| niemeyer | TheMue: Bingo. | 16:17 |
| TheMue | <TheMue> niemeyer: now it is used for me to stop the serviced, but i have to look why we have added it once <= last sentence, i have to look | 16:18 |
| niemeyer | TheMue: <TheMue> niemeyer: the map only contains one entry with the service name and the last added unitd | 16:19 |
| niemeyer | TheMue: That's what I was talking about | 16:19 |
| TheMue | niemeyer: if you don't want me to describe what i've seen just let me know | 16:19 |
| niemeyer | TheMue: Heh | 16:19 |
| niemeyer | TheMue: I don't need you to tell me that you've seen a map with a service name and a unit | 16:21 |
| niemeyer | TheMue: unitd.serviced.unitds[unit.ServiceName()] = unitd | 16:21 |
| niemeyer | TheMue: I can read the code | 16:21 |
| TheMue | niemeyer: ok, so i don't describe stuff like that in future | 16:22 |
| niemeyer | TheMue: I also don't need you to tell me that tests were passing before.. as that's obvious. | 16:22 |
| niemeyer | TheMue: What I do need your help with, is making sure we have a test covering the fix | 16:22 |
| niemeyer | TheMue: I do need you for that, a lot. | 16:22 |
| TheMue | niemeyer: I'll do my best. | 16:23 |
| niemeyer | TheMue: And that's the only thing I've suggested in the review | 16:23 |
| niemeyer | TheMue: Do we have any test covering two services? | 16:26 |
| TheMue | niemeyer: The map has been used after an exosed change. So the one (last added) unit has been passed twice to flushUnits() for opening/closing of ports | 16:28 |
| TheMue | niemeyer: so far the map has been used no where else | 16:28 |
| niemeyer | TheMue: Have you seen the line right below the one you changed/fixed? | 16:28 |
| TheMue | niemeyer: yes | 16:29 |
| niemeyer | TheMue: and? | 16:30 |
| TheMue | niemeyer: here a changed unit is added directly to the slice, so only once | 16:30 |
| niemeyer | TheMue: Slice? | 16:30 |
| TheMue | niemeyer: but after the exposed we iterate of the serviced.unitds and the one unit is twice in the map and added to the slice | 16:31 |
| TheMue | niemeyer: yes, the slice that is passed to flushUnits() | 16:31 |
| niemeyer | TheMue: I mean line 130 and 131 | 16:31 |
| niemeyer | TheMue: The line you fixed, and the line right below it | 16:32 |
| TheMue | niemeyer: ah, ic what you mean | 16:33 |
| TheMue | niemeyer: the referenced serviced is the same | 16:33 |
| niemeyer | TheMue: Yes, we have the unit twice in the same map with different keys | 16:34 |
| TheMue | niemeyer: so one line isn't needed | 16:34 |
| niemeyer | TheMue: Right, the bottom one | 16:34 |
| niemeyer | TheMue: Do we have any tests with multiple services? | 16:34 |
| TheMue | niemeyer: have to look, afaik not yet | 16:35 |
| niemeyer | TheMue: Can we have one in a follow up branch? | 16:35 |
| TheMue | niemeyer: will add it, sure | 16:35 |
| niemeyer | TheMue: Happy to have this in just with the review comments sorted, plus the duplicated line removed | 16:35 |
| niemeyer | TheMue: Thank you | 16:35 |
| TheMue | niemeyer: yw | 16:36 |
| rogpeppe | niemeyer: just a heads up: i'm rolling the provisioner into the machine agent, and i don't particularly see any reason to treat the firewaller as inherently bundled with the provisioner, so i'm creating two bools on the Machine - Provisioner and Firewaller. does that seem reasonable to you? | 16:42 |
| rogpeppe | niemeyer: i think it makes the implication of the flags more clear, as well as being more flexible. | 16:42 |
| niemeyer | rogpeppe: Sounds like it could be a list.. machine.AgentWorkers() []WorkerKind | 16:45 |
| rogpeppe | niemeyer: yeah, i was wondering about that kind of thing too | 16:45 |
| rogpeppe | niemeyer: or a map[WorkerKind]bool | 16:45 |
| niemeyer | rogpeppe: Doesn't sound right.. | 16:46 |
| rogpeppe | niemeyer: we can have more than one instance of a given worker? | 16:47 |
| rogpeppe | niemeyer: well, i guess so | 16:47 |
| niemeyer | rogpeppe: Probably not, but that's unrelate | 16:47 |
| niemeyer | d | 16:47 |
| rogpeppe | niemeyer: oh - that was the reason i suggested it. | 16:48 |
| niemeyer | rogpeppe: Yep.. that's unrelated | 16:48 |
| rogpeppe | niemeyer: so why don't you think it's good like that? | 16:48 |
| niemeyer | rogpeppe: Lists without duplications are a fine concept | 16:48 |
| rogpeppe | niemeyer: ok. where would you put the check? or would we just trust? | 16:49 |
| niemeyer | rogpeppe: Where would you put the check? | 16:49 |
| rogpeppe | niemeyer: probably in the agent itself. but it depends whether we want to allow multiple instances of a given worker in principle | 16:50 |
| rogpeppe | niemeyer: BTW would the agent worker kinds include the machiner too? | 16:50 |
| niemeyer | rogpeppe: Good question.. I think it should, for consistency | 16:51 |
| rogpeppe | niemeyer: it kinda doesn't seem right to leave it out, but then again i can't think of a situation where we don't want to run it | 16:51 |
| niemeyer | rogpeppe: Let's have it in.. it reflects reality and reduces implicit assumptions | 16:52 |
| niemeyer | rogpeppe: machine.SetAgentWorkers([]WorkerKind{MachineWorker, ProvisionerWorker, FirewallerWorker}) | 16:52 |
| rogpeppe | niemeyer: ok, so if the machine agent starts and no workers are set, it... does what? | 16:52 |
| niemeyer | rogpeppe: It explodes and laughs | 16:52 |
| rogpeppe | niemeyer: sounds good | 16:53 |
| rogpeppe | niemeyer: to relive some typing, i think perhaps SetAgentWorkers(... state.WorkerKind) | 16:53 |
| niemeyer | rogpeppe: +1 | 16:53 |
| niemeyer | rogpeppe: SetWorkers sounds good too | 16:53 |
| rogpeppe | relieve | 16:53 |
| rogpeppe | niemeyer: +1 | 16:53 |
| rogpeppe | niemeyer: and type AgentWorker string ? | 16:54 |
| niemeyer | rogpeppe: Uh oh.. we have a problem | 16:54 |
| niemeyer | rogpeppe: A race, more specifically | 16:54 |
| niemeyer | rogpeppe: type sounds good | 16:54 |
| niemeyer | rogpeppe: We have to create the machine with its workers set | 16:55 |
| rogpeppe | niemeyer: ah yes | 16:55 |
| rogpeppe | niemeyer: we could put the workers into AddMachine | 16:56 |
| niemeyer | rogpeppe: +1 | 16:56 |
| rogpeppe | niemeyer: cool. then i won't provide a SetWorkers method, so there's no pretence | 16:56 |
| niemeyer | rogpeppe: Should fail if MachineWorker isn't set, or if there are duplicates | 16:56 |
| rogpeppe | niemeyer: sounds good | 16:57 |
| rogpeppe | niemeyer: although we could just add the MachineWorker implicitly | 16:57 |
| rogpeppe | niemeyer: and return it from machine.Workers | 16:58 |
| niemeyer | rogpeppe: Let's please make it explicit. Very soon it may well make sense to have a machine that has no units. That's a trivial step from now. | 16:58 |
| rogpeppe | niemeyer: ok, that seems good | 16:58 |
| rogpeppe | niemeyer: actually we could make it not return an error, but do as requested (assuming at least one worker). we'd just need to tweak AssignToUnusedMachine to choose a machine with a MachineWorker | 16:59 |
| rogpeppe | niemeyer: still i'll start by returning an error if no MachineWorker | 17:00 |
| niemeyer | rogpeppe: One thing at a time, please | 17:08 |
| Aram | OK: 124 passed, 1 skipped | 17:08 |
| Aram | how can one skip a test? | 17:08 |
| Aram | is this an old or new feature? | 17:09 |
| niemeyer | Aram: Hah, I think that's actually a bug | 17:09 |
| niemeyer | Aram: I mean, in the sense that we want the test running | 17:09 |
| niemeyer | Aram: c.Skip(reason) | 17:09 |
| Aram | I see | 17:09 |
| Aram | c.Skip("Marshalling of agent tools is currently broken") | 17:09 |
| Aram | I'm probably on an older branch | 17:09 |
| rogpeppe | Aram: you are, and they aren't | 17:09 |
| Aram | I'll merge trunk at some point | 17:10 |
| niemeyer | Aram: https://codereview.appspot.com/6576056 | 17:12 |
| rogpeppe | niemeyer: i thought i'd already done that... | 17:13 |
| rogpeppe | niemeyer: probably just in some other abandoned branch | 17:14 |
| niemeyer | rogpeppe: Maybe.. one of the reasons why small/in-focus branches rokc | 17:14 |
| rogpeppe | niemeyer: point taken :-) | 17:14 |
| TheMue | niemeyer: CL is in again | 17:14 |
| TheMue | niemeyer: I have to leave, familiy needs me, but I'll look into mail via phone later | 17:15 |
| SpamapS | niemeyer: was just talking on a call and we were discussing the possibility of adding some things into metadata.yaml... | 17:15 |
| SpamapS | niemeyer: How would you feel about a field at the same level as 'interface' that is 'recommends' and lists charms that are the recommended other side of that relationship? | 17:16 |
| SpamapS | niemeyer: and on a related note, how about a text field called 'description' for the relation itself, that is displayed whenever there is confusion on which relation to establish? | 17:16 |
| niemeyer | SpamapS: Unless we define behavior for such a field, I'm -1 on it | 17:19 |
| niemeyer | SpamapS: This sounds like getting into stacks, which I very much hope we start diving into soon | 17:20 |
| niemeyer | SpamapS: On the description one, sounds more interesting | 17:20 |
| SpamapS | niemeyer: The thought was to help people build stacks. The only behavior for it is to make use in the charm store and such. | 17:22 |
| SpamapS | niemeyer: its for the case where something is optional, like memcached, but there may be several implementations that provide: memcache protocol. | 17:22 |
| SpamapS | niemeyer: prefer may be a better term than recommend | 17:23 |
| niemeyer | SpamapS: Understood.. I'm still -1 on adding such a field before we define how stacks work and what behavior we want from juju itself when it looks at it | 17:24 |
| SpamapS | niemeyer: ok, mind if I open up a bug in juju-core? | 17:25 |
| niemeyer | SpamapS: Not at all, thanks for that.. If you don't mind, please try to phrase the bug in terms of the problem you're trying to solve, rather than the solution | 17:26 |
| rogpeppe | hmm, seems like i've used up all my flocks: | 17:26 |
| rogpeppe | bzr: ERROR: Could not acquire lock "/home/rog/src/go/src/launchpad.net/juju-core/.bzr/checkout/dirstate": [Errno 11] Resource temporarily unavailable | 17:26 |
| SpamapS | niemeyer: indeed | 17:27 |
| niemeyer | I wish Rietveld's copy & pasting worked as nicely as pastebin.ubuntu.com | 17:34 |
| fwereade | niemeyer, https://codereview.appspot.com/6568060/ is still WIP but I think it hews closer to your vision | 17:41 |
| fwereade | niemeyer, the tests worked first time I got it building (ok, ok, second time) and the tests runs about twice as fast as they did before :) | 17:42 |
| niemeyer | fwereade: Oh, nice! | 17:43 |
| niemeyer | fwereade: Why do we need the wantFoo stuff? | 17:43 |
| niemeyer | Oh, maybe I misunderstand | 17:44 |
| * niemeyer reads more | 17:44 | |
| niemeyer | Yeah, I think I did.. nice | 17:45 |
| rogpeppe | fwereade: if you have a moment, you might want to have a glance at https://codereview.appspot.com/6561063 | 17:51 |
| rogpeppe | i'm now off for the evening | 17:54 |
| rogpeppe | night all | 17:54 |
| Aram | niemeyer: how do I turn off debug output from state/watcher? | 17:58 |
| niemeyer | fwereade: Sent comments.. great stuff | 18:21 |
| fwereade | niemeyer, sweet :D | 18:22 |
| fwereade | niemeyer, I'll have time to look at them properly later | 18:22 |
| niemeyer | fwereade: Oh, I think there are a few races in the filter logic, but they're fixable.. I'll send a note | 18:26 |
| niemeyer | Aram: Just started hacking on the MachinesWatcher | 19:03 |
| niemeyer | Aram: Curious to see the effect of the model we discussed yesterday | 19:04 |
| Aram | niemeyer: cool, so how can I disable debug output from state/watcher wile still keeping it from state? | 19:04 |
| niemeyer | Aram: Have you had a look at the package? | 19:06 |
| niemeyer | fwereade, Aram: I wonder about the right moment to show Dead things | 19:18 |
| niemeyer | If we show entities that first show up as Dead after the initial event, we'll have to cache them to avoid showing them repeatedly | 19:20 |
| niemeyer | Hmm.. or perhaps not, actually | 19:20 |
| niemeyer | We can clean up when we find the entity removed | 19:21 |
| niemeyer | That sounds sane... nevermind | 19:21 |
| niemeyer | fwereade, Aram: http://paste.ubuntu.com/1231019/ | 19:23 |
| niemeyer | Sane? | 19:23 |
| Aram | whenever one or more machines is added or changes its lifecycle. | 19:24 |
| niemeyer | Aram: Sounds good | 19:24 |
| rogpeppe | niemeyer: Machine.Workers: https://codereview.appspot.com/6564063 | 19:25 |
| niemeyer | rogpeppe: Looking | 19:25 |
| rogpeppe | niemeyer: thanks. i really am gone now :-) | 19:27 |
| niemeyer | rogpeppe: have a good evening! | 19:34 |
| niemeyer | rogpeppe: And a laugh, in case you're still around: | 19:35 |
| niemeyer | 128 » ProvisionerWorker WorkerKind = "firewaller" | 19:35 |
| niemeyer | 129 » FirewallerWorker WorkerKind = "provisioner" | 19:35 |
| Aram | machine units test pass | 19:52 |
| Aram | hell yeah | 19:52 |
| Aram | now to add more tests to test for more stuff | 19:52 |
| niemeyer | Aram: Woot! | 19:55 |
| Aram | // test -gocheck.f TestMachineWaitAgentAlive | 20:05 |
| Aram | comment in the source | 20:05 |
| Aram | I did NOT do that :). | 20:05 |
| Aram | that was roger | 20:05 |
| Aram | proposed | 20:15 |
| * Aram has dinner | 20:15 | |
| Aram | bye | 20:15 |
| fwereade | niemeyer, ping | 21:41 |
| niemeyer | fwereade: yo | 21:41 |
| fwereade | niemeyer, heyhey | 21:42 |
| fwereade | niemeyer, clarification re: MachinesWatcher comment you pasted above | 21:42 |
| niemeyer | fwereade: Cool | 21:43 |
| fwereade | niemeyer, but I'm having trouble writing what I want clearly :/ | 21:43 |
| niemeyer | fwereade: It changed slightly after Aram's suggestion | 21:44 |
| fwereade | niemeyer, ok, is it possible that the watcher will ever miss a Dead, and only observe the removal? and; if that is possible, and it occurs, will it send a Dead anyway? | 21:45 |
| niemeyer | fwereade: It is possible, and it does report anyway | 21:46 |
| fwereade | niemeyer, excellent, such was my supposition; I think there might be a nicer way to express that aspect of it | 21:46 |
| fwereade | niemeyer, or maybe it's not worth it :) | 21:46 |
| niemeyer | fwereade: I think the documentation actually guarantees it | 21:47 |
| fwereade | niemeyer, it may just be my faulty reading then | 21:47 |
| niemeyer | fwereade: You know too much :-D | 21:47 |
| fwereade | niemeyer, what I really want to talk about is the uniter change I suggested earlier | 21:47 |
| fwereade | niemeyer, I agree that resolved() is an ugly little knot of code | 21:48 |
| niemeyer | fwereade: 'k | 21:48 |
| fwereade | niemeyer, and I also have some sympathy for the notion that ModeHookError is really 2 modes, one Alive and one Dying, although I am somewhat resistant here | 21:48 |
| fwereade | niemeyer, but the tension between the two is what really bothers me | 21:49 |
| niemeyer | fwereade: I'm not sure if it was clear from my comment, but I wasn't suggesting getting rid of the current mode | 21:49 |
| fwereade | niemeyer, in that unpacking the resolved method leads to duplication of slightly fiddly logic in 2 places | 21:49 |
| niemeyer | fwereade: I was suggesting that it sounds fine to have it hand off parts of its work to two independent modes | 21:50 |
| niemeyer | s/parts/part | 21:50 |
| fwereade | niemeyer, or 4 if ModeHookErr and ModeConflicted are both split | 21:50 |
| fwereade | niemeyer, and in either case, the resolved-handling and the upgrade-handling are duplicated | 21:50 |
| niemeyer | fwereade: I'm hoping we can remove the fiddleness enough for it to not be an issue. | 21:51 |
| fwereade | niemeyer, I don;t think the <-u.charmUpgrades(false) suggestion will fly -- getUpgrade only works right when it's run on the main goroutine | 21:51 |
| niemeyer | fwereade: Upgrade handling, if the suggestions are made are possible, is simply return ModeUpgrade(foo) | 21:51 |
| niemeyer | fwereade: Why? | 21:52 |
| niemeyer | fwereade: What I see is actually a race caused by the fact it runs elsewhere | 21:52 |
| fwereade | niemeyer, filter goroutine reads uniter state: ok, cool, we're not upgrading | main goroutine starts an upgrade | filter goroutine reads the "current" state from the charm dir, instead of using the upgrade state | 21:53 |
| fwereade | niemeyer, it's rather deliberate that the filter doesn't have access to the uniter itself, anyway | 21:54 |
| niemeyer | fwereade: Hmm | 21:54 |
| niemeyer | fwereade: Yeah, the fact it doesn't access the uniter sounds good | 21:55 |
| niemeyer | fwereade: You see the race, though? | 21:56 |
| fwereade | niemeyer, ah, sorry, just saw the new review, need to read that | 21:56 |
| fwereade | niemeyer, yeah, you're absolutely right, and I would be more than happy to send both ResolvedModes and CharmThingys down the channels | 21:58 |
| niemeyer | fwereade: Cool, we're in sync then.. I also see why you want getUpgrade in the uniter | 22:00 |
| niemeyer | fwereade: It'll just loose some of its inner workings since we'll get that info from the channel | 22:00 |
| niemeyer | lose | 22:00 |
| fwereade | niemeyer, yeah -- and I suspect resolve will look a little different in this light, too | 22:01 |
| fwereade | niemeyer, might give me a way out of the ickiness | 22:01 |
| niemeyer | fwereade: Sweet | 22:01 |
| fwereade | niemeyer, thinking about the Alive/Dying (sub)Modes a bit more | 22:02 |
| fwereade | niemeyer, switching channels on and off in response to events on other channels is IMO not all that unpleasant a technique; it should surely not be used arbitrarily, but in each of those cases we have a situation in which... | 22:04 |
| fwereade | niemeyer, the stimulus/response set in Dying is a strict subset of that in Alive; and the only valid transition is from Alive to Dying | 22:05 |
| fwereade | niemeyer, it seems a shame to duplicate the identical bits any more than they have to be | 22:05 |
| fwereade | niemeyer, but it will look different once I fix filter | 22:05 |
| niemeyer | fwereade: The amount of logic you have added to enable the shifting is worth about exactly the same of the logic that you actually need in the dying case by itself. | 22:09 |
| niemeyer | fwereade: One of them is entirely straightforward.. the other is not | 22:09 |
| niemeyer | fwereade: I suggest, once you're done with everything else, at least trying to see how it looks | 22:09 |
| niemeyer | fwereade: If you still feel the same way, I won't argue much :) | 22:09 |
| fwereade | niemeyer, I'll see how it goes :) | 22:10 |
| niemeyer | fwereade: and by worth I mean the number of lines | 22:10 |
| fwereade | niemeyer, I'm really not sure about that... we lose the upgrade stuff, but the resolved stuff is still needed when dying | 22:10 |
| fwereade | niemeyer, in both cases | 22:10 |
| fwereade | niemeyer, it all hinges on how terse I can make the resolved handling ;) | 22:11 |
| niemeyer | fwereade: Right, the resolved stuff is the only duplication in both cases | 22:18 |
| niemeyer | fwereade: You only need tombDying next to it | 22:18 |
| niemeyer | fwereade: In one of them | 22:18 |
| fwereade | niemeyer, hmm, I think I should probably always have tombDying :) | 22:19 |
| niemeyer | fwereade: or default.. | 22:19 |
| fwereade | niemeyer, in that one specific case in the main uniter loop, ok, yeah :) | 22:19 |
| fwereade | niemeyer, actuall that has tombDying *and* default | 22:20 |
| fwereade | niemeyer, I must be missing something | 22:20 |
| niemeyer | fwereade: Yeah, that's slightly surprising | 22:20 |
| fwereade | niemeyer, when in a Mode is a select default useful? | 22:20 |
| fwereade | niemeyer, (that bit is actually totally unnecessary -- it really just allows us to indirectly check for unit-Dead in between modes | 22:21 |
| fwereade | ) | 22:21 |
| niemeyer | fwereade: When in a mode is an if statement useful? :-) | 22:22 |
| fwereade | niemeyer, d'oh, ok, yes, I use them | 22:22 |
| fwereade | niemeyer, but in a for { select loop, surely all they will cause is spinning? | 22:23 |
| niemeyer | fwereade: Ah, definitely, that'd be wrong | 22:24 |
| niemeyer | davechen1y: Morning! | 23:13 |
| davechen1y | morning | 23:15 |
| davechen1y | niemeyer: thank you for your review, just responding to your comments now | 23:15 |
| niemeyer | davechen1y: Cheers | 23:15 |
| davechen1y | wrt. splitting hostFromTarget into two methods, one for machines, the other for units | 23:16 |
| davechen1y | i'd still like to have a single method that dispatches to both of those | 23:16 |
| davechen1y | so I can reuse it in scp | 23:16 |
| davechen1y | what do you think ? | 23:16 |
| niemeyer | davechen1y: Sounds like a good idea | 23:18 |
| davechen1y | niemeyer: also, Unit.PublicAddress only works when called from the unit itself | 23:20 |
| davechen1y | :( | 23:20 |
| niemeyer | davechen1y: Isn't that EnvironProvider.PublicAddress? | 23:21 |
| davechen1y | niemeyer: let me check before making mroe incorrect statements | 23:21 |
| davechen1y | sorry, you are correct | 23:22 |
| davechen1y | right ... so that comes out of the document, which means the UA sets that field, i'm guessing on Alive | 23:23 |
| niemeyer | davechen1y: Yeah, it should set it at some point | 23:25 |
| === davechen1y is now known as davecheney | ||
| davecheney | niemeyer: what are your thoghts on changing environs.Environ.Instances([]string) to be ...string ? | 23:38 |
| niemeyer | davecheney: I personally don't mind too much in either direction, but I wonder if environ.Instance(instId string) might be more useful | 23:42 |
| davecheney | niemeyer: in the majority of cases where i've used that method, i only have a single id I want to resolve | 23:42 |
| davecheney | so wrapping it in a []string is overkill | 23:43 |
| davecheney | making the param varadic means we don't have to have two methods on the interface | 23:43 |
| niemeyer | davecheney: It also means we'll call InstanceS when we want one, and have to unwrap a slice | 23:43 |
| niemeyer | davecheney: WE also are using a real slice less than we should | 23:43 |
| niemeyer | davecheney: The provisioner, for example, has a TODO for avoiding the overkill of per instance query | 23:44 |
| niemeyer | davecheney: Slightly off topic, though, I know | 23:44 |
| davecheney | the providers all expect a slice | 23:44 |
| davecheney | as in the method takes a []string now | 23:44 |
| davecheney | so changing it to be variadic would have no impact on them | 23:44 |
| niemeyer | davecheney: I mean that in "foo, err := environ.Instances(whatever)", foo is a slice too | 23:45 |
| davecheney | yeah, that also doth suck | 23:45 |
| davecheney | just as common is foo, err := ... ; foo[0].something() | 23:45 |
| niemeyer | davecheney: So if we're missing a handier "give me one", I'd suggest a real one | 23:45 |
| davecheney | niemeyer: fair call | 23:45 |
| davecheney | i will propose something | 23:45 |
| niemeyer | davecheney: Cheers | 23:46 |
| niemeyer | davecheney: I'll step out to hack a home appliance here.. wish me luck :) | 23:46 |
| davecheney | roger | 23:47 |
| davecheney | send more paramedics | 23:47 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!