fwereadeTheMue, heyhey06:57
TheMuefwereade: heya06:58
TheMueooops, disconnected *hmmm*08:52
TheMueAram: moin09:55
rogpeppefwereade: fairly trivial: https://codereview.appspot.com/657605310:45
fwereaderogpeppe, cheers10:46
fwereaderogpeppe, LGTM10:47
rogpeppefwereade: ta10:47
rogpeppefwereade: and another, even more trivial: https://codereview.appspot.com/656606010:52
fwereaderogpeppe, LGTM10:53
rogpeppefwereade: tyvm10:53
rogpeppefwereade: i think i'll submit both, given your remark10:56
fwereaderogpeppe, +110:56
TheMuehave one too: https://codereview.appspot.com/656706011:33
rogpeppeTheMue: i would look, but i can't seem to pull from lp currently11:43
rogpeppeTheMue: rather, i was looking, but i wanted to look more closely but failed to talk to launchpad11:43
rogpeppeTheMue: i can't even pull from trunk currently11:43
TheMuerogpeppe: oh, which error message?11:43
rogpeppeAram, fwereade: are you having any problems accessing launchpad through bzr?11:43
TheMuerogpeppe: i just pulled trunk and it worked11:44
rogpeppeTheMue: http://paste.ubuntu.com/1230255/11:44
TheMuerogpeppe: iiirks, never seen that11:45
rogpeppeTheMue: yeah, i don't know what's going on - it worked fine earlier11:45
rogpeppei wonder whether my password has expired or something11:46
rogpeppei suspect it may be something to do with using a long branch name11:50
rogpeppelbox just managed to push fine. weird11:50
rogpeppeTheMue: all working now. who knows what was going on...12:00
TheMuerogpeppe: maybe service-side12:00
rogpeppelol, i was trying to find the definition for the "delete" function, totally forgetting it was built in!12:01
TheMueAram: seem i found a bug in the machines watcher12:16
TheMueAram: typically a machine can be removed after EnsureDead()12:17
rogpeppeTheMue: you've got a review12:18
TheMueAram: 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
TheMuerogpeppe: just seen, cheers12:18
AramTheMue: I know, the implementation was correct and niemeyer broke it.12:19
TheMuerogpeppe: good hints, thx12:19
* Aram is back in 15 minutes.12:19
rogpeppefwereade: another one (this one makes live tests pass, yay!): https://codereview.appspot.com/6566058/12:21
* fwereade looks12:21
fwereaderogpeppe, LGTM :)12:24
rogpeppefwereade: thanks!12:25
rogpeppeoh why can i never remember how to print a full revision id in bzr?!12:39
rogpeppenor is it easy to google for12:39
Aramfwereade: meh, how I wish we had actual container entities.12:59
fwereadeAram, heh, me too, I remain totally convinced it's the Right Thing to do12:59
Aramthe 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
rogpeppefwereade: the uniter tests seem to spend ages and ages doing: want resolved mode '\x00', got '\x02'; still waiting13:04
rogpeppefwereade: and similar13:04
fwereaderogpeppe, hell, niemeyer had that yesterday; I was unable to repro :/13:04
rogpeppefwereade: the uniter tests passed for me just now but took 124s to run13:04
fwereaderogpeppe, they do indeed take a long time: some of it may be the wanton fsyncing but probably not all13:05
fwereaderogpeppe, 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
rogpeppefwereade: i'll paste you a copy of the output of the test, with timestamps so you can see where the time is going13:06
rogpeppefwereade: lol13:06
Aramfwereade: 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
rogpeppefwereade: interesting, when i ran it that time, it failed with "never reached desired status"13:07
fwereaderogpeppe, yeah, indeed, there is clearly something funky wrt Resolved -- that is one of the bits that is changing, actually13:07
fwereadeAram, I think so, yes13:08
fwereadeAram, tedious, innit13:08
rogpeppefwereade: http://paste.ubuntu.com/1230376/13:09
fwereaderogpeppe, cheers13:09
rogpeppefwereade: that ouput is actually from a branch other than trunk13:10
rogpeppefwereade: i'll try in trunk and see if the same thing happens - i might have broken something13:10
Aramthere'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
rogpeppeAram: how would you have implemented watchers?13:11
Aramrogpeppe: I actually did like 4 or 5 times.13:11
Aramlet me find a branch.13:11
fwereaderogpeppe, I wouldn't worry too much about it, I will be looking into it13:11
rogpeppeAram: i think the point of state/watcher is that it's efficient enough to cope with many thousands of documents13:12
rogpeppeniemeyer: yo!13:12
niemeyerGood mornign!13:13
niemeyerrogpeppe: Just sent some comments on the branch13:15
niemeyerrogpeppe: Thanks for fixing the tests13:16
rogpeppeniemeyer: cheers13:16
niemeyerrogpeppe: The tools waiter looks like a nice idea, but it's sprawling too far IMO13:16
niemeyerrogpeppe: A tool waiter should wait for the tools, and nothing else. It's messing up the already messy situation even further.13:16
rogpeppeniemeyer: ok. i thought it was quite neat to bundle up the watcher with the thing it's watching, but evidently not.13:18
niemeyerfwereade: ping13:18
fwereadeniemeyer, pong13:18
niemeyerrogpeppe: It's a tool waiter13:18
niemeyerrogpeppe: It's not nice to bundle things with arbitrary things so we can use the underlying things' interface13:18
niemeyerfwereade: yo13:18
niemeyerfwereade: Couple of questions13:18
niemeyerfwereade: Did you figure what was wrong with the uniter tests? I think I can still reproduce13:19
fwereadeniemeyer, listening (btw thank you for excellent advice re uniter, it seems to be coming along pretty nicely)13:19
fwereadeniemeyer, afraid not; rog was just hitting it today13:19
niemeyerfwereade: My pleasure, glad it was useful13:19
niemeyerfwereade: Okay, I'll dive in right away then13:19
rogpeppeniemeyer: the reason it made things nicer is that otherwise we end up always passing around the object along with the watcher channel.13:19
niemeyerfwereade: Please keep going in whatever you were doing before.. :-)13:19
fwereadeniemeyer, my suspicion is that my ClearResolved() is funky, it looked off when I was changing it today13:19
rogpeppeniemeyer: the waiter object made that more straightforward by bundling them together13:20
niemeyerrogpeppe: 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
rogpeppeniemeyer: i admit that was a bit of a short cut :-)13:21
niemeyerrogpeppe: Heh13:21
niemeyerrogpeppe: These tests should be cleaned up, reduced in size and scope.. that's the opposite of it13:21
rogpeppeniemeyer: i'd like to have a chat about that13:22
niemeyerrogpeppe: Cool, let's just extinguish the fire first, and then I'm happy to discuss it13:22
rogpeppeniemeyer: i'd like to do that, but i don't want the time taken to run the tests to double.13:22
niemeyerrogpeppe: They won't double.. we already have BootstrapOnce13:22
rogpeppeniemeyer: tests like this one start a unit.13:23
rogpeppeniemeyer: that takes almost as long as bootstrapping13:23
rogpeppeniemeyer: so it makes sense to test some stuff with the unit once we've started it, i think.13:23
niemeyerrogpeppe: BootstrapOnce can create a default unit, with a well known testing charm13:24
niemeyerrogpeppe: 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
rogpeppeniemeyer: i think that's wrong, for the same reason it was wrong in the other tests we changed recently.13:24
niemeyerrogpeppe: Curiously, you won't be able to do that for upgrades, so I don't think it's too much to upgrade it13:24
rogpeppeniemeyer: we don't want loads of arbitrary context in the suite13:24
niemeyerrogpeppe: Erm, to deploy a new unit13:25
niemeyerrogpeppe: Yep, so let's pay the price of time13:25
niemeyerrogpeppe: Pick your fight :)13:25
rogpeppeniemeyer: live tests already take 10 minutes. i don't want them to take 20.13:25
niemeyerrogpeppe: I don't care13:25
niemeyerrogpeppe: I won't be sitting and looking at the screen in either case13:26
niemeyerrogpeppe: I do care, though, that we have incomprehensible and long-winded tests13:26
niemeyerrogpeppe: Which took a week to fix when we changed something fundamental13:26
niemeyerrogpeppe: *one* test13:26
rogpeppeniemeyer: 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
niemeyerrogpeppe: That's why we have a BootstrapOnce test13:29
niemeyerrogpeppe: s/test/helper/13:29
niemeyerrogpeppe: Because it's fundamental to live tests13:29
niemeyerrogpeppe: Everything else is not13:29
niemeyerrogpeppe: Of course you need bootstrap to test deploy, of course we need bootstrap to test upgrade, of course etc etc13:30
rogpeppeniemeyer: how can i test upgrade without doing a deploy?13:30
niemeyerrogpeppe: But that's certainly not reasoning to put *all of those tests* in the same place13:30
niemeyerrogpeppe: Otherwise all we know is "Huh.. the big blob of code is broken again!"13:30
niemeyerrogpeppe: You can do a deploy without testing upgrade13:31
rogpeppeniemeyer: 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 hours13:32
niemeyerrogpeppe: What upgrade is a prefix of?13:32
rogpeppeniemeyer: deploy is a prefix of upgrade. maybe upgrade is as far as it gets, yes.13:32
niemeyerrogpeppe: bingo13:32
niemeyerrogpeppe: Bootstrap and deploying things are fundamental.. everything else is not13:33
rogpeppeniemeyer: so... does it make sense to have deploy as a separate test, when upgrade will test *exactly* the same path?13:33
niemeyerrogpeppe: Does it make sense to know that deploy works, despite the fact that upgrade doesn't?13:33
rogpeppeniemeyer: 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
niemeyerrogpeppe: So let's erase all the tests.. 0 minutes13:35
rogpeppeniemeyer: now yer being silly :-)13:35
niemeyerrogpeppe: Where's the value?13:35
niemeyerrogpeppe: I am13:35
niemeyerrogpeppe: But that's the point.. you're worried about timing and ignoring everything else13:35
niemeyerrogpeppe: THere are other ways to optimize time13:35
niemeyerrogpeppe: Running tests in parallel for instance13:35
niemeyerrogpeppe: But if you take away the value of the test, there's little reason to have them13:36
niemeyerrogpeppe: All we have is a black/white works/doesn't work13:36
rogpeppeniemeyer: does that mean table-driven tests are of no value because they're all bundled into a single test?13:36
niemeyerrogpeppe: Can't see that leap13:36
niemeyerrogpeppe: "table tests" are generally very contained in scope13:37
niemeyerrogpeppe: Bootstrapping, deploying, upgrading, deploying more units, remove units, test firewaller, open ports, close ports, check expose, blah blah blah13:38
rogpeppeniemeyer: ok, i'll change it and see how it looks13:39
niemeyerrogpeppe: hold on, this is all future work.. let's please make things stable first13:40
rogpeppeniemeyer: ok. so what do you want me to do about the current branch?13:40
niemeyerrogpeppe: Look at the review, and move forward?13:40
rogpeppeniemeyer: ok. i thought you weren't keen on most of what i was doing there.13:41
niemeyerrogpeppe: I've sent a review that details exactly my feeling about what's in that branch13:41
niemeyerrogpeppe: And I've started the conversation saying that I appreciate the refactoring you did to wait for tools13:42
niemeyerrogpeppe: And I also said I didn't want to get into this discussion before the fire was extinguished13:42
rogpeppeniemeyer: fair enough13:42
rogpeppeniemeyer: 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
niemeyerrogpeppe: :-(13:45
niemeyerrogpeppe: Can we please make the tools waiter *wait for tools*!?13:46
rogpeppeniemeyer: ok, i think i'm there13:46
niemeyerrogpeppe: Thanks13:46
niemeyerfwereade: Curiously, the tests are passing now :-(14:03
* niemeyer looks at the paste14:03
fwereadeniemeyer, 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 resolution14:04
fwereadeniemeyer, but I'm not 100% there yet14:04
niemeyerfwereade: Cool14:04
rogpeppeniemeyer: i think this might be better: https://codereview.appspot.com/656605814:08
* rogpeppe goes for a bite of lunch14:09
niemeyerrogpeppe: Thanks, LGTM. Just sent a couple of questions for your consideration14:15
rogpeppeniemeyer: 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
niemeyerrogpeppe: Okay, I personally don't mind much either way in that case.. whatever you're happy with14:19
rogpeppeniemeyer: 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 obvious14:19
rogpeppeniemeyer: i'm happy to do that if you prefer14:20
niemeyerrogpeppe: Yeah, sounds slightly more resilient.. we could change the tools waiter without breaking the test14:21
rogpeppeniemeyer: ok, will do. thanks for the review BTW14:21
niemeyerrogpeppe: My pleasure, and sorry for the confusion14:21
rogpeppeniemeyer: np14:21
niemeyerI think I was the go fmt offender..14:25
niemeyerI stumble upon it every other commit14:26
rogpeppeniemeyer: i do it quite a bit too :-)14:29
niemeyerfwereade: You might appreciate this one: https://codereview.appspot.com/656505714:30
niemeyerfwereade: Figure it while having a look at uniter tests runs.. :)14:30
niemeyerrogpeppe: ;)14:30
fwereadeniemeyer, yay!14:30
fwereadeniemeyer, LGTM14:31
fwereadeniemeyer, that was on my copious-free-time list too :)14:31
niemeyerfwereade: The uniter test that broke in the paste was relative to resolution14:32
niemeyerfwereade: I'm wondering if it's just a sync race14:32
niemeyerfwereade: I'll see if I can reproduce this afternoon14:32
TheMueniemeyer: ping14:32
niemeyerTheMue: Yo14:33
fwereadeniemeyer, hmm, I feel like that bit *should* be pretty clean, but I'm not certain14:33
TheMueniemeyer: the MachinesWatcher seems to have a problem14:33
niemeyerTheMue: Oh?14:33
TheMueniemeyer: while the test of RemoveMachine() in state works I now tried it for the firewaller14:33
TheMueniemeyer: and there I get no error in EnsureDead() and RemoveMachine() but a panic in the watcher telling me that the machine is not dead14:34
niemeyerTheMue: 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 too14:35
niemeyerTheMue: Do you have a reproducer?14:35
TheMueniemeyer: currently in an own little branch of the firewaller test. but i'll set up an extra branch for you14:36
niemeyerTheMue: ?14:36
niemeyerTheMue: Thanks, please push something, or just put the test in a paste and send a link14:37
niemeyerTheMue: I'll have a look after lunch14:37
TheMueniemeyer: ok, will do14:37
niemeyerI'll step out for an earlier lunch today to join Ale.. see you all soon14:39
rogpeppeniemeyer, fwereade: i still get sporadic failures of worker/uniter14:39
rogpeppeniemeyer: but that's the only failure14:40
rogpeppeniemeyer: enjoy!14:40
niemeyerrogpeppe: It's known.. I'll try to reproduce in the afternoon, and fwereade is also refactoring things somewhat14:40
rogpeppeniemeyer: a slightly reddish shade of green then :-)14:40
niemeyerrogpeppe: The message is the same: tests should pass before you merge. If something broke, don't commit14:41
* niemeyer steps out14:41
rogpeppeuniter upgrade tests pass, yay!15:42
* fwereade cheers ar rogpeppe15:57
fwereadewell holy shit15:58
fwereadeafter that *brutal* refactoring, the uniter tests pass in about half the time they did before15:59
rogpeppefwereade: trivial: https://codereview.appspot.com/656606315:59
* rogpeppe cheers at fwereade15:59
fwereaderogpeppe, (and worked second time after I got it to build, too, the only bug (detected thus far...) was errorContextf-related)16:00
rogpeppefwereade: nice one16:00
fwereaderogpeppe, LGTM16:00
fwereaderogpeppe, bbs16:01
rogpeppefwereade: so how long do the uniter tests take now?16:01
* niemeyer waves16:01
fwereaderogpeppe, bit more than a minute16:02
niemeyerTheMue: ping16:02
fwereadeniemeyer, that suggestion was *awesome*16:02
niemeyerfwereade: Woohay!16:02
TheMueniemeyer: pong16:02
fwereadeniemeyer, I think I remained true to its spirit, just got it building, and OMG at least twice as fast16:02
fwereadeniemeyer, but I have to go... you'll get a CL tonight for sure, unless I discover some subtle screwup16:03
TheMueniemeyer: it is not reproducable standalone, only in the firewaller16:03
TheMueniemeyer: shall i push that branch so that you can test it there?16:03
niemeyerTheMue: Yeah, or even just a tiny test for the firewaller that exposes the bug16:04
TheMueniemeyer: looked into the watcher. the revno = -1 seems to come to early, because the machine is still in w.alive16:04
niemeyerTheMue: I've sent a review too16:04
TheMueniemeyer: cheers16:05
niemeyerTheMue: Yeah, I want to look at that.. I've put the panic mainly so I'd see those cases16:05
TheMueniemeyer: I paste the test, one moment please16:05
niemeyerTheMue: Thanks!16:05
niemeyerfwereade: Superb, that sounds very exciting!16:05
rogpeppeniemeyer: uniter upgrade, finally: https://codereview.appspot.com/656106316:06
niemeyerrogpeppe: Looking16:06
TheMueniemeyer: http://paste.ubuntu.com/1230645/ , the panic is in merge() of the MachinesWatcher16:08
niemeyerTheMue: Thanks16:08
niemeyerTheMue: please let me know if you have any questions on the review sent16:09
TheMueniemeyer: 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
TheMueniemeyer: the other comments a pretty clear, thanks16:13
niemeyerTheMue: That's how you discovered the bug16:14
niemeyerTheMue: But does it really have "no failing effect"? Why do we use this map at all then?16:14
TheMueniemeyer: yes, but before it showed no effect16:15
niemeyerTheMue: This sounds like a missing test?16:15
niemeyerTheMue: Why is the map used?16:15
TheMueniemeyer: the map only contains one entry with the service name and the last added unitd16:15
TheMueniemeyer: now it is used for me to stop the serviced, but i have to look why we have added it once16:16
niemeyerTheMue: Sorry, that doesn't sound right16:16
niemeyerTheMue: Feels very hand-wavy16:16
niemeyerTheMue: Why have you added that map in the first place?16:16
niemeyerTheMue: Where is it used?16:16
TheMueniemeyer: that's what i wanted to say. i right now don't remember and have to look for the reason16:17
niemeyerTheMue: 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 look16:18
niemeyerTheMue: <TheMue> niemeyer: the map only contains one entry with the service name and the last added unitd16:19
niemeyerTheMue: That's what I was talking about16:19
TheMueniemeyer: if you don't want me to describe what i've seen just let me know16:19
niemeyerTheMue: Heh16:19
niemeyerTheMue: I don't need you to tell me that you've seen a map with a service name and a unit16:21
niemeyerTheMue:                                 unitd.serviced.unitds[unit.ServiceName()] = unitd16:21
niemeyerTheMue: I can read the code16:21
TheMueniemeyer: ok, so i don't describe stuff like that in future16:22
niemeyerTheMue: I also don't need you to tell me that tests were passing before.. as that's obvious.16:22
niemeyerTheMue: What I do need your help with, is making sure we have a test covering the fix16:22
niemeyerTheMue: I do need you for that, a lot.16:22
TheMueniemeyer: I'll do my best.16:23
niemeyerTheMue: And that's the only thing I've suggested in the review16:23
niemeyerTheMue: Do we have any test covering two services?16:26
TheMueniemeyer: 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 ports16:28
TheMueniemeyer: so far the map has been used no where else16:28
niemeyerTheMue: Have you seen the line right below the one you changed/fixed?16:28
TheMueniemeyer: yes16:29
niemeyerTheMue: and?16:30
TheMueniemeyer: here a changed unit is added directly to the slice, so only once16:30
niemeyerTheMue: Slice?16:30
TheMueniemeyer: but after the exposed we iterate of the serviced.unitds and the one unit is twice in the map and added to the slice16:31
TheMueniemeyer: yes, the slice that is passed to flushUnits()16:31
niemeyerTheMue: I mean line 130 and 13116:31
niemeyerTheMue: The line you fixed, and the line right below it16:32
TheMueniemeyer: ah, ic what you mean16:33
TheMueniemeyer: the referenced serviced is the same16:33
niemeyerTheMue: Yes, we have the unit twice in the same map with different keys16:34
TheMueniemeyer: so one line isn't needed16:34
niemeyerTheMue: Right, the bottom one16:34
niemeyerTheMue: Do we have any tests with multiple services?16:34
TheMueniemeyer: have to look, afaik not yet16:35
niemeyerTheMue: Can we have one in a follow up branch?16:35
TheMueniemeyer: will add it, sure16:35
niemeyerTheMue: Happy to have this in just with the review comments sorted, plus the duplicated line removed16:35
niemeyerTheMue: Thank you16:35
TheMueniemeyer: yw16:36
rogpeppeniemeyer: 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
rogpeppeniemeyer: i think it makes the implication of the flags more clear, as well as being more flexible.16:42
niemeyerrogpeppe: Sounds like it could be a list.. machine.AgentWorkers() []WorkerKind16:45
rogpeppeniemeyer: yeah, i was wondering about that kind of thing too16:45
rogpeppeniemeyer: or a map[WorkerKind]bool16:45
niemeyerrogpeppe: Doesn't sound right..16:46
rogpeppeniemeyer: we can have more than one instance of a given worker?16:47
rogpeppeniemeyer: well, i guess so16:47
niemeyerrogpeppe: Probably not, but that's unrelate16:47
rogpeppeniemeyer: oh - that was the reason i suggested it.16:48
niemeyerrogpeppe: Yep.. that's unrelated16:48
rogpeppeniemeyer: so why don't you think it's good like that?16:48
niemeyerrogpeppe: Lists without duplications are a fine concept16:48
rogpeppeniemeyer: ok. where would you put the check? or would we just trust?16:49
niemeyerrogpeppe: Where would you put the check?16:49
rogpeppeniemeyer: probably in the agent itself. but it depends whether we want to allow multiple instances of a given worker in principle16:50
rogpeppeniemeyer: BTW would the agent worker kinds include the machiner too?16:50
niemeyerrogpeppe: Good question.. I think it should, for consistency16:51
rogpeppeniemeyer: 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 it16:51
niemeyerrogpeppe: Let's have it in.. it reflects reality and reduces implicit assumptions16:52
niemeyerrogpeppe: machine.SetAgentWorkers([]WorkerKind{MachineWorker, ProvisionerWorker, FirewallerWorker})16:52
rogpeppeniemeyer: ok, so if the machine agent starts and no workers are set, it... does what?16:52
niemeyerrogpeppe: It explodes and laughs16:52
rogpeppeniemeyer: sounds good16:53
rogpeppeniemeyer: to relive some typing, i think perhaps SetAgentWorkers(... state.WorkerKind)16:53
niemeyerrogpeppe: +116:53
niemeyerrogpeppe: SetWorkers sounds good too16:53
rogpeppeniemeyer: +116:53
rogpeppeniemeyer: and type AgentWorker string ?16:54
niemeyerrogpeppe: Uh oh.. we have a problem16:54
niemeyerrogpeppe: A race, more specifically16:54
niemeyerrogpeppe: type sounds good16:54
niemeyerrogpeppe: We have to create the machine with its workers set16:55
rogpeppeniemeyer: ah yes16:55
rogpeppeniemeyer: we could put the workers into AddMachine16:56
niemeyerrogpeppe: +116:56
rogpeppeniemeyer: cool. then i won't provide a SetWorkers method, so there's no pretence16:56
niemeyerrogpeppe: Should fail if MachineWorker isn't set, or if there are duplicates16:56
rogpeppeniemeyer: sounds good16:57
rogpeppeniemeyer: although we could just add the MachineWorker implicitly16:57
rogpeppeniemeyer: and return it from machine.Workers16:58
niemeyerrogpeppe: 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
rogpeppeniemeyer: ok, that seems good16:58
rogpeppeniemeyer: 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 MachineWorker16:59
rogpeppeniemeyer: still i'll start by returning an error if no MachineWorker17:00
niemeyerrogpeppe: One thing at a time, please17:08
AramOK: 124 passed, 1 skipped17:08
Aramhow can one skip a test?17:08
Aramis this an old or new feature?17:09
niemeyerAram: Hah, I think that's actually a bug17:09
niemeyerAram: I mean, in the sense that we want the test running17:09
niemeyerAram: c.Skip(reason)17:09
AramI see17:09
Aramc.Skip("Marshalling of agent tools is currently broken")17:09
AramI'm probably on an older branch17:09
rogpeppeAram: you are, and they aren't17:09
AramI'll merge trunk at some point17:10
niemeyerAram: https://codereview.appspot.com/657605617:12
rogpeppeniemeyer: i thought i'd already done that...17:13
rogpeppeniemeyer: probably just in some other abandoned branch17:14
niemeyerrogpeppe: Maybe.. one of the reasons why small/in-focus branches rokc17:14
rogpeppeniemeyer: point taken :-)17:14
TheMueniemeyer: CL is in again17:14
TheMueniemeyer: I have to leave, familiy needs me, but I'll look into mail via phone later17:15
SpamapSniemeyer: was just talking on a call and we were discussing the possibility of adding some things into metadata.yaml...17:15
SpamapSniemeyer: 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
SpamapSniemeyer: 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
niemeyerSpamapS: Unless we define behavior for such a field, I'm -1 on it17:19
niemeyerSpamapS: This sounds like getting into stacks, which I very much hope we start diving into soon17:20
niemeyerSpamapS: On the description one, sounds more interesting17:20
SpamapSniemeyer: 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
SpamapSniemeyer: its for the case where something is optional, like memcached, but there may be several implementations that provide: memcache protocol.17:22
SpamapSniemeyer: prefer may be a better term than recommend17:23
niemeyerSpamapS: 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 it17:24
SpamapSniemeyer: ok, mind if I open up a bug in juju-core?17:25
niemeyerSpamapS: 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 solution17:26
rogpeppehmm, seems like i've used up all my flocks:17:26
rogpeppebzr: ERROR: Could not acquire lock "/home/rog/src/go/src/launchpad.net/juju-core/.bzr/checkout/dirstate": [Errno 11] Resource temporarily unavailable17:26
SpamapSniemeyer: indeed17:27
niemeyerI wish Rietveld's copy & pasting worked as nicely as pastebin.ubuntu.com17:34
fwereadeniemeyer, https://codereview.appspot.com/6568060/ is still WIP but I think it hews closer to your vision17:41
fwereadeniemeyer, 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
niemeyerfwereade: Oh, nice!17:43
niemeyerfwereade: Why do we need the wantFoo stuff?17:43
niemeyerOh, maybe I misunderstand17:44
* niemeyer reads more17:44
niemeyerYeah, I think I did.. nice17:45
rogpeppefwereade: if you have a moment, you might want to have a glance at https://codereview.appspot.com/656106317:51
rogpeppei'm now off for the evening17:54
rogpeppenight all17:54
Aramniemeyer: how do I turn off debug output from state/watcher?17:58
niemeyerfwereade: Sent comments.. great stuff18:21
fwereadeniemeyer, sweet :D18:22
fwereadeniemeyer, I'll have time to look at them properly later18:22
niemeyerfwereade: Oh, I think there are a few races in the filter logic, but they're fixable.. I'll send a note18:26
niemeyerAram: Just started hacking on the MachinesWatcher19:03
niemeyerAram: Curious to see the effect of the model we discussed yesterday19:04
Aramniemeyer: cool, so how can I disable debug output from state/watcher wile still keeping it from state?19:04
niemeyerAram: Have you had a look at the package?19:06
niemeyerfwereade, Aram: I wonder about the right moment to show Dead things19:18
niemeyerIf we show entities that first show up as Dead after the initial event, we'll have to cache them to avoid showing them repeatedly19:20
niemeyerHmm.. or perhaps not, actually19:20
niemeyerWe can clean up when we find the entity removed19:21
niemeyerThat sounds sane... nevermind19:21
niemeyerfwereade, Aram: http://paste.ubuntu.com/1231019/19:23
Aramwhenever one or more machines is added or changes its lifecycle.19:24
niemeyerAram: Sounds good19:24
rogpeppeniemeyer: Machine.Workers: https://codereview.appspot.com/656406319:25
niemeyerrogpeppe: Looking19:25
rogpeppeniemeyer: thanks. i really am gone now :-)19:27
niemeyerrogpeppe: have a good evening!19:34
niemeyerrogpeppe: 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
Arammachine units test pass19:52
Aramhell yeah19:52
Aramnow to add more tests to test for more stuff19:52
niemeyerAram: Woot!19:55
Aram // test -gocheck.f TestMachineWaitAgentAlive20:05
Aramcomment in the source20:05
AramI did NOT do that :).20:05
Aramthat was roger20:05
* Aram has dinner20:15
fwereadeniemeyer, ping21:41
niemeyerfwereade: yo21:41
fwereadeniemeyer, heyhey21:42
fwereadeniemeyer, clarification re: MachinesWatcher comment you pasted above21:42
niemeyerfwereade: Cool21:43
fwereadeniemeyer, but I'm having trouble writing what I want clearly :/21:43
niemeyerfwereade: It changed slightly after Aram's suggestion21:44
fwereadeniemeyer, 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
niemeyerfwereade: It is possible, and it does report anyway21:46
fwereadeniemeyer, excellent, such was my supposition; I think there might be a nicer way to express that aspect of it21:46
fwereadeniemeyer, or maybe it's not worth it :)21:46
niemeyerfwereade: I think the documentation actually guarantees it21:47
fwereadeniemeyer, it may just be my faulty reading then21:47
niemeyerfwereade: You know too much :-D21:47
fwereadeniemeyer, what I really want to talk about is the uniter change I suggested earlier21:47
fwereadeniemeyer, I agree that resolved() is an ugly little knot of code21:48
niemeyerfwereade: 'k21:48
fwereadeniemeyer, 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 here21:48
fwereadeniemeyer, but the tension between the two is what really bothers me21:49
niemeyerfwereade: I'm not sure if it was clear from my comment, but I wasn't suggesting getting rid of the current mode21:49
fwereadeniemeyer, in that unpacking the resolved method leads to duplication of slightly fiddly logic in 2 places21:49
niemeyerfwereade: I was suggesting that it sounds fine to have it hand off parts of its work to two independent modes21:50
fwereadeniemeyer, or 4 if ModeHookErr and ModeConflicted are both split21:50
fwereadeniemeyer, and in either case, the resolved-handling and the upgrade-handling are duplicated21:50
niemeyerfwereade: I'm hoping we can remove the fiddleness enough for it to not be an issue.21:51
fwereadeniemeyer, I don;t think the <-u.charmUpgrades(false) suggestion will fly -- getUpgrade only works right when it's run on the main goroutine21:51
niemeyerfwereade: Upgrade handling, if the suggestions are made are possible, is simply return ModeUpgrade(foo)21:51
niemeyerfwereade: Why?21:52
niemeyerfwereade: What I see is actually a race caused by the fact it runs elsewhere21:52
fwereadeniemeyer, 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 state21:53
fwereadeniemeyer, it's rather deliberate that the filter doesn't have access to the uniter itself, anyway21:54
niemeyerfwereade: Hmm21:54
niemeyerfwereade: Yeah, the fact it doesn't access the uniter sounds good21:55
niemeyerfwereade: You see the race, though?21:56
fwereadeniemeyer, ah, sorry, just saw the new review, need to read that21:56
fwereadeniemeyer, yeah, you're absolutely right, and I would be more than happy to send both ResolvedModes and CharmThingys down the channels21:58
niemeyerfwereade: Cool, we're in sync then.. I also see why you want getUpgrade in the uniter22:00
niemeyerfwereade: It'll just loose some of its inner workings since we'll get that info from the channel22:00
fwereadeniemeyer, yeah -- and I suspect resolve will look a little different in this light, too22:01
fwereadeniemeyer, might give me a way out of the ickiness22:01
niemeyerfwereade: Sweet22:01
fwereadeniemeyer, thinking about the Alive/Dying (sub)Modes a bit more22:02
fwereadeniemeyer, 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
fwereadeniemeyer, the stimulus/response set in Dying is a strict subset of that in Alive; and the only valid transition is from Alive to Dying22:05
fwereadeniemeyer, it seems a shame to duplicate the identical bits any more than they have to be22:05
fwereadeniemeyer, but it will look different once I fix filter22:05
niemeyerfwereade: 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
niemeyerfwereade: One of them is entirely straightforward.. the other is not22:09
niemeyerfwereade: I suggest, once you're done with everything else, at least trying to see how it looks22:09
niemeyerfwereade: If you still feel the same way, I won't argue much :)22:09
fwereadeniemeyer, I'll see how it goes :)22:10
niemeyerfwereade: and by worth I mean the number of lines22:10
fwereadeniemeyer, I'm really not sure about that... we lose the upgrade stuff, but the resolved stuff is still needed when dying22:10
fwereadeniemeyer, in both cases22:10
fwereadeniemeyer, it all hinges on how terse I can make the resolved handling ;)22:11
niemeyerfwereade: Right, the resolved stuff is the only duplication in both cases22:18
niemeyerfwereade: You only need tombDying next to it22:18
niemeyerfwereade: In one of them22:18
fwereadeniemeyer, hmm, I think I should probably always have tombDying :)22:19
niemeyerfwereade: or default..22:19
fwereadeniemeyer, in that one specific case in the main uniter loop, ok, yeah :)22:19
fwereadeniemeyer, actuall that has tombDying *and* default22:20
fwereadeniemeyer, I must be missing something22:20
niemeyerfwereade: Yeah, that's slightly surprising22:20
fwereadeniemeyer, when in a Mode is a select default useful?22:20
fwereadeniemeyer, (that bit is actually totally unnecessary -- it really just allows us to indirectly check for unit-Dead in between modes22:21
niemeyerfwereade: When in a mode is an if statement useful? :-)22:22
fwereadeniemeyer, d'oh, ok, yes, I use them22:22
fwereadeniemeyer, but in a for { select loop, surely all they will cause is spinning?22:23
niemeyerfwereade: Ah, definitely, that'd be wrong22:24
niemeyerdavechen1y: Morning!23:13
davechen1yniemeyer: thank you for your review, just responding to your comments now23:15
niemeyerdavechen1y: Cheers23:15
davechen1ywrt. splitting hostFromTarget into two methods, one for machines, the other for units23:16
davechen1yi'd still like to have a single method that dispatches to both of those23:16
davechen1yso I can reuse it in scp23:16
davechen1ywhat do you think ?23:16
niemeyerdavechen1y: Sounds like a good idea23:18
davechen1yniemeyer: also, Unit.PublicAddress only works when called from the unit itself23:20
niemeyerdavechen1y: Isn't that EnvironProvider.PublicAddress?23:21
davechen1yniemeyer: let me check before making mroe incorrect statements23:21
davechen1ysorry, you are correct23:22
davechen1yright ... so that comes out of the document, which means the UA sets that field, i'm guessing on Alive23:23
niemeyerdavechen1y: Yeah, it should set it at some point23:25
=== davechen1y is now known as davecheney
davecheneyniemeyer: what are your thoghts on changing environs.Environ.Instances([]string) to be ...string ?23:38
niemeyerdavecheney: I personally don't mind too much in either direction, but I wonder if environ.Instance(instId string) might be more useful23:42
davecheneyniemeyer: in the majority of cases where i've used that method, i only have a single id I want to resolve23:42
davecheneyso wrapping it in a []string is overkill23:43
davecheneymaking the param varadic means we don't have to have two methods on the interface23:43
niemeyerdavecheney: It also means we'll call InstanceS when we want one, and have to unwrap a slice23:43
niemeyerdavecheney: WE also are using a real slice less than we should23:43
niemeyerdavecheney: The provisioner, for example, has a TODO for avoiding the overkill of per instance query23:44
niemeyerdavecheney: Slightly off topic, though, I know23:44
davecheneythe providers all expect a slice23:44
davecheneyas in the method takes a []string now23:44
davecheneyso changing it to be variadic would have no impact on them23:44
niemeyerdavecheney: I mean that in "foo, err := environ.Instances(whatever)", foo is a slice too23:45
davecheneyyeah, that also doth suck23:45
davecheneyjust as common is foo, err := ... ; foo[0].something()23:45
niemeyerdavecheney: So if we're missing a handier "give me one", I'd suggest a real one23:45
davecheneyniemeyer: fair call23:45
davecheneyi will propose something23:45
niemeyerdavecheney: Cheers23:46
niemeyerdavecheney: I'll step out to hack a home appliance here.. wish me luck :)23:46
davecheneysend more paramedics23:47

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