[06:33] <TheMue> morning
[06:57] <fwereade> TheMue, heyhey
[06:58] <TheMue> fwereade: heya
[08:52] <TheMue> ooops, disconnected *hmmm*
[09:44] <Aram> moin.
[09:55] <TheMue> Aram: moin
[10:42] <TheMue> lunchtime
[10:45] <rogpeppe> fwereade: fairly trivial: https://codereview.appspot.com/6576053
[10:46] <fwereade> rogpeppe, cheers
[10:47] <fwereade> rogpeppe, LGTM
[10:47] <rogpeppe> fwereade: ta
[10:52] <rogpeppe> fwereade: and another, even more trivial: https://codereview.appspot.com/6566060
[10:53] <fwereade> rogpeppe, LGTM
[10:53] <rogpeppe> fwereade: tyvm
[10:56] <rogpeppe> fwereade: i think i'll submit both, given your remark
[10:56] <fwereade> rogpeppe, +1
[11:33] <TheMue> have one too: https://codereview.appspot.com/6567060
[11:43] <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:44] <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:45] <TheMue> rogpeppe: iiirks, never seen that
[11:45] <rogpeppe> TheMue: yeah, i don't know what's going on - it worked fine earlier
[11:46] <rogpeppe> i wonder whether my password has expired or something
[11:50] <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
[12:00] <rogpeppe> TheMue: all working now. who knows what was going on...
[12:00] <TheMue> rogpeppe: maybe service-side
[12:01] <rogpeppe> lol, i was trying to find the definition for the "delete" function, totally forgetting it was built in!
[12:02] <Aram> :)
[12:16] <TheMue> Aram: seem i found a bug in the machines watcher
[12:17] <TheMue> Aram: typically a machine can be removed after EnsureDead()
[12:18] <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:19] <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:21] <rogpeppe> fwereade: another one (this one makes live tests pass, yay!): https://codereview.appspot.com/6566058/
[12:21]  * fwereade looks
[12:24] <fwereade> rogpeppe, LGTM :)
[12:25] <rogpeppe> fwereade: thanks!
[12:39] <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:59] <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
[13:00] <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:04] <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:05] <fwereade> rogpeppe, they do indeed take a long time: some of it may be the wanton fsyncing but probably not all
[13:06] <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:07] <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:08] <fwereade> Aram, I think so, yes
[13:08] <fwereade> Aram, tedious, innit
[13:09] <rogpeppe> fwereade: http://paste.ubuntu.com/1230376/
[13:09] <fwereade> rogpeppe, cheers
[13:10] <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:11] <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:12] <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:13] <niemeyer> Good mornign!
[13:14] <TheMue> morning
[13:15] <niemeyer> rogpeppe: Just sent some comments on the branch
[13:16] <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:18] <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:19] <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:20] <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:21] <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:22] <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:23] <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:24] <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:25] <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:26] <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:28] <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:29] <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:30] <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:31] <niemeyer> rogpeppe: You can do a deploy without testing upgrade
[13:32] <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:33] <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:34] <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:35] <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:36] <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:37] <niemeyer> rogpeppe: "table tests" are generally very contained in scope
[13:38] <niemeyer> rogpeppe: Bootstrapping, deploying, upgrading, deploying more units, remove units, test firewaller, open ports, close ports, check expose, blah blah blah
[13:39] <rogpeppe> niemeyer: ok, i'll change it and see how it looks
[13:40] <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:41] <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:42] <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:44] <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:45] <niemeyer> rogpeppe: :-(
[13:46] <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
[14:03] <niemeyer> fwereade: Curiously, the tests are passing now :-(
[14:03]  * niemeyer looks at the paste
[14:04] <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:08] <rogpeppe> niemeyer: i think this might be better: https://codereview.appspot.com/6566058
[14:09]  * rogpeppe goes for a bite of lunch
[14:15] <niemeyer> rogpeppe: Thanks, LGTM. Just sent a couple of questions for your consideration
[14:18] <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:19] <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:20] <rogpeppe> niemeyer: i'm happy to do that if you prefer
[14:21] <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:25] <niemeyer> I think I was the go fmt offender..
[14:26] <niemeyer> I stumble upon it every other commit
[14:29] <rogpeppe> niemeyer: i do it quite a bit too :-)
[14:30] <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:31] <fwereade> niemeyer, LGTM
[14:31] <fwereade> niemeyer, that was on my copious-free-time list too :)
[14:32] <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:33] <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:34] <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:35] <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:36] <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:37] <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:39] <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:40] <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:41] <niemeyer> rogpeppe: The message is the same: tests should pass before you merge. If something broke, don't commit
[14:41]  * niemeyer steps out
[15:42] <rogpeppe> uniter upgrade tests pass, yay!
[15:42] <rogpeppe> (live)
[15:57]  * fwereade cheers ar rogpeppe
[15:58] <fwereade> well holy shit
[15:59] <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
[16:00] <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:01] <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:02] <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:03] <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:04] <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:05] <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:06] <rogpeppe> niemeyer: uniter upgrade, finally: https://codereview.appspot.com/6561063
[16:06] <niemeyer> rogpeppe: Looking
[16:08] <TheMue> niemeyer: http://paste.ubuntu.com/1230645/ , the panic is in merge() of the MachinesWatcher
[16:08] <niemeyer> TheMue: Thanks
[16:09] <niemeyer> TheMue: please let me know if you have any questions on the review sent
[16:13] <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:14] <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:15] <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:16] <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:17] <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.
 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:19] <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:21] <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:22] <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:23] <TheMue> niemeyer: I'll do my best.
[16:23] <niemeyer> TheMue: And that's the only thing I've suggested in the review
[16:26] <niemeyer> TheMue: Do we have any test covering two services?
[16:28] <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:29] <TheMue> niemeyer: yes
[16:30] <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:31] <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:32] <niemeyer> TheMue: The line you fixed, and the line right below it
[16:33] <TheMue> niemeyer: ah, ic what you mean
[16:33] <TheMue> niemeyer: the referenced serviced is the same
[16:34] <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:35] <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:36] <TheMue> niemeyer: yw
[16:42] <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:45] <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:46] <niemeyer> rogpeppe: Doesn't sound right..
[16:47] <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:48] <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:49] <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:50] <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:51] <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:52] <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:53] <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:54] <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:55] <niemeyer> rogpeppe: We have to create the machine with its workers set
[16:55] <rogpeppe> niemeyer: ah yes
[16:56] <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:57] <rogpeppe> niemeyer: sounds good
[16:57] <rogpeppe> niemeyer: although we could just add the MachineWorker implicitly
[16:58] <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:59] <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
[17:00] <rogpeppe> niemeyer: still i'll start by returning an error if no MachineWorker
[17:08] <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:09] <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:10] <Aram> I'll merge trunk at some point
[17:12] <niemeyer> Aram: https://codereview.appspot.com/6576056
[17:13] <rogpeppe> niemeyer: i thought i'd already done that...
[17:14] <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:15] <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:16] <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:19] <niemeyer> SpamapS: Unless we define behavior for such a field, I'm -1 on it
[17:20] <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:22] <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:23] <SpamapS> niemeyer: prefer may be a better term than recommend
[17:24] <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:25] <SpamapS> niemeyer: ok, mind if I open up a bug in juju-core?
[17:26] <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:27] <SpamapS> niemeyer: indeed
[17:34] <niemeyer> I wish Rietveld's copy & pasting worked as nicely as pastebin.ubuntu.com
[17:41] <fwereade> niemeyer, https://codereview.appspot.com/6568060/ is still WIP but I think it hews closer to your vision
[17:42] <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:43] <niemeyer> fwereade: Oh, nice!
[17:43] <niemeyer> fwereade: Why do we need the wantFoo stuff?
[17:44] <niemeyer> Oh, maybe I misunderstand
[17:44]  * niemeyer reads more
[17:45] <niemeyer> Yeah, I think I did.. nice
[17:51] <rogpeppe> fwereade: if you have a moment, you might want to have a glance at https://codereview.appspot.com/6561063
[17:54] <rogpeppe> i'm now off for the evening
[17:54] <rogpeppe> night all
[17:58] <Aram> niemeyer: how do I turn off debug output from state/watcher?
[18:21] <niemeyer> fwereade: Sent comments.. great stuff
[18:22] <fwereade> niemeyer, sweet :D
[18:22] <fwereade> niemeyer, I'll have time to look at them properly later
[18:26] <niemeyer> fwereade: Oh, I think there are a few races in the filter logic, but they're fixable.. I'll send a note
[19:03] <niemeyer> Aram: Just started hacking on the MachinesWatcher
[19:04] <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:06] <niemeyer> Aram: Have you had a look at the package?
[19:18] <niemeyer> fwereade, Aram: I wonder about the right moment to show Dead things
[19:20] <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:21] <niemeyer> We can clean up when we find the entity removed
[19:21] <niemeyer> That sounds sane... nevermind
[19:23] <niemeyer> fwereade, Aram: http://paste.ubuntu.com/1231019/
[19:23] <niemeyer> Sane?
[19:24] <Aram> whenever one or more machines is added or changes its lifecycle.
[19:24] <niemeyer> Aram: Sounds good
[19:25] <rogpeppe> niemeyer: Machine.Workers: https://codereview.appspot.com/6564063
[19:25] <niemeyer> rogpeppe: Looking
[19:27] <rogpeppe> niemeyer: thanks. i really am gone now :-)
[19:34] <niemeyer> rogpeppe: have a good evening!
[19:35] <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:52] <Aram> machine units test pass
[19:52] <Aram> hell yeah
[19:52] <Aram> now to add more tests to test for more stuff
[19:55] <niemeyer> Aram: Woot!
[20:05] <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:15] <Aram> proposed
[20:15]  * Aram has dinner
[20:15] <Aram> bye
[21:41] <fwereade> niemeyer, ping
[21:41] <niemeyer> fwereade: yo
[21:42] <fwereade> niemeyer, heyhey
[21:42] <fwereade> niemeyer, clarification re: MachinesWatcher comment you pasted above
[21:43] <niemeyer> fwereade: Cool
[21:43] <fwereade> niemeyer, but I'm having trouble writing what I want clearly :/
[21:44] <niemeyer> fwereade: It changed slightly after Aram's suggestion
[21:45] <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:46] <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:47] <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:48] <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:49] <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:50] <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:51] <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:52] <niemeyer> fwereade: Why?
[21:52] <niemeyer> fwereade: What I see is actually a race caused by the fact it runs elsewhere
[21:53] <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:54] <fwereade> niemeyer, it's rather deliberate that the filter doesn't have access to the uniter itself, anyway
[21:54] <niemeyer> fwereade: Hmm
[21:55] <niemeyer> fwereade: Yeah, the fact it doesn't access the uniter sounds good
[21:56] <niemeyer> fwereade: You see the race, though?
[21:56] <fwereade> niemeyer, ah, sorry, just saw the new review, need to read that
[21:58] <fwereade> niemeyer, yeah, you're absolutely right, and I would be more than happy to send both ResolvedModes and CharmThingys down the channels
[22:00] <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:01] <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:02] <fwereade> niemeyer, thinking about the Alive/Dying (sub)Modes a bit more
[22:04] <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:05] <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:09] <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:10] <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:11] <fwereade> niemeyer, it all hinges on how terse I can make the resolved handling ;)
[22:18] <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:19] <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:20] <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:21] <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:22] <niemeyer> fwereade: When in a mode is an if statement useful? :-)
[22:22] <fwereade> niemeyer, d'oh, ok, yes, I use them
[22:23] <fwereade> niemeyer, but in a for { select loop, surely all they will cause is spinning?
[22:24] <niemeyer> fwereade: Ah, definitely, that'd be wrong
[23:13] <niemeyer> davechen1y: Morning!
[23:15] <davechen1y> morning
[23:15] <davechen1y> niemeyer: thank you for your review, just responding to your comments now
[23:15] <niemeyer> davechen1y: Cheers
[23:16] <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:18] <niemeyer> davechen1y: Sounds like a good idea
[23:20] <davechen1y> niemeyer: also, Unit.PublicAddress only works when called from the unit itself
[23:20] <davechen1y> :(
[23:21] <niemeyer> davechen1y: Isn't that EnvironProvider.PublicAddress?
[23:21] <davechen1y> niemeyer: let me check before making mroe incorrect statements
[23:22] <davechen1y> sorry, you are correct
[23:23] <davechen1y> right ... so that comes out of the document, which means the UA sets that field, i'm guessing on Alive
[23:25] <niemeyer> davechen1y: Yeah, it should set it at some point
[23:38] <davecheney> niemeyer: what are your thoghts on changing environs.Environ.Instances([]string) to be ...string ?
[23:42] <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:43] <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:44] <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:45] <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:46] <niemeyer> davecheney: Cheers
[23:46] <niemeyer> davecheney: I'll step out to hack a home appliance here.. wish me luck :)
[23:47] <davecheney> roger
[23:47] <davecheney> send more paramedics