davecheneyniemeyer: scp & scp reproposed, some nice reuse in there00:14
niemeyerdavecheney: Sweet, will have a quick look00:49
niemeyerdavecheney: http://play.golang.org/00:52
niemeyerdavecheney: http://play.golang.org/p/cPscJ6RuoX00:52
davecheneyniemeyer: yes, i had to check that as well00:53
niemeyerdavecheney: Sorry, I don't get it?00:53
niemeyerdavecheney: "This will panic if len(c.Args) == 1. I've redone the logic to be less crackful."00:53
niemeyerdavecheney: c.Target, c.Args = c.Args[0], c.Args[1:]00:53
davecheneyniemeyer: http://play.golang.org/p/TObeRIa8wL00:53
davecheneyyes, you are right00:54
davecheneybut I dont understand why http://play.golang.org/p/Fd_jNi6mVe00:54
niemeyerdavecheney: Because that's out of bounds00:57
niemeyerdavecheney: s[1:] works when len(s) == 1 for the same reason that s[:1] works.00:58
davecheneyi dont' think that is correct00:59
davecheneybut this isn't the right place to argue about it01:00
davecheneyi'll fix my code01:00
davecheneyok, someone explained it too me in the channel01:02
niemeyerdavecheney: Stepping out for the night.. have a good day!03:27
rogpeppedavecheney, fwereade_: mornin'06:05
davecheneyrogpeppe: howdy06:05
fwereade_davecheney, rogpeppe: heyhey06:06
rogpeppefwereade_: it'd be great if you could have a glance at the uniter upgrade branch, if you have a moment sometime this morning: https://codereview.appspot.com/6561063/06:06
fwereade_rogpeppe, sure06:07
fwereade_rogpeppe, (still thinking)06:14
rogpeppefwereade_: np06:15
fwereade_rogpeppe, ISTM that it would be simpler (not to mention less conflicty, and mildly kinder to the network) to get the Unit only when you actually need to run an upgrade06:18
fwereade_rogpeppe, doing that removes the need to change runOnce and Uniter06:19
fwereade_rogpeppe, is there a deeper motivation in play than "I need a unit"?06:19
* rogpeppe has a look to remind himself06:20
rogpeppefwereade_: we need to unit immediately06:20
rogpeppefwereade_: because we use it to announce the current agent version06:20
rogpeppefwereade_: but that's not the reason we change runOnce and Uniter06:21
rogpeppefwereade_: the reason for that is that we want to upgrader to be as independent as possible of uniter bugs06:21
rogpeppefwereade_: so we do the absolute minimum necessary before starting the upgrader06:22
rogpeppefwereade_: hence it's important that the uniter factory method doesn't return an error - early errors should not take down the upgrader.06:22
rogpeppes/need to unit/need the unit/06:23
rogpeppefwereade_: does that make sense?06:23
fwereade_rogpeppe, hmm, apart from the ones which should, but I see it's tricky06:23
rogpeppefwereade_: which errors *should* take down the upgrader?06:24
rogpeppefwereade: last thing i was was [07:23:38] <fwereade_> rogpeppe, hmm, apart from the ones which should, but I see it's tricky06:25
fwereaderogpeppe, cheers06:26
fwereaderogpeppe, I'm not quite convinced that deferring the error helps you much though, 1 mo06:26
fwereaderogpeppe, surely an early error return from the Uniter will hose the upgrader just as badly, because runTasks will terminate it06:27
rogpeppefwereade: no, because we've got some special case logic in the upgrader for just such an eventuality06:28
rogpeppefwereade: if it's killed early on, it waits until it has at least had a squizz at the proposed version06:28
rogpeppefwereade: and if that's changed, it doesn't exit until it has actually downloaded the upgrade06:28
rogpeppefwereade: (well, with some timeout too)06:29
rogpeppefwereade: it's given 5 minutes06:29
* fwereade continues to think06:30
fwereaderogpeppe, something about all that does make me a little queasy... I feel that if I ask an upgrader to stop, it should jolly well stop, by criminy06:37
rogpeppefwereade: this was discussed at the time06:37
rogpeppefwereade: i think the upgrader is special06:37
rogpeppefwereade: because it's the only way we can escape bad s/w06:37
fwereaderogpeppe, I agree, but IMO that means it should have control of the tasks rather than being antisocial :)06:38
rogpeppefwereade: it *will* stop... when it's made good and sure that someone is not trying to upgrade us06:38
rogpeppefwereade: well, as you know, that's how i started and that was deemed incorrect06:39
rogpeppefwereade: so this is what we're doing. and it doesn't seem bad to me.06:39
fwereaderogpeppe, I do feel your pain there... but I think I need to talk to niemeyer about this06:40
rogpeppefwereade: ok06:40
fwereaderogpeppe, sorry :(06:40
rogpeppefwereade: if we change this now, BTW, it knocks everything off06:41
rogpeppefwereade: because everything is built in this way currently.06:41
rogpeppefwereade: the upgrader is "just" another task06:41
rogpeppefwereade: so, given that we're not swimming in free time, and this architecture will work for the time being, perhaps we could move forward as we are and consider a change later?06:42
fwereaderogpeppe, the collision with my own uniter changes is not entirely trivial06:43
fwereaderogpeppe, and there is a value I return from NewUniter that really should block upgrades06:43
rogpeppefwereade: which is?06:44
rogpeppefwereade: i don't think it matters too much tbh. it's an edge case - it doesn't matter much if we do upgrade in that case, it's just an extra 5s delay06:45
rogpeppefwereade: FWIW i think i'd probably structure it in a similar way even if the upgrader was in control - we want the upgrader to be independent right up until the moment it decides that now is the time to upgrade.06:47
rogpeppeTheMue: hiya06:47
TheMuerogpeppe: hiya06:48
rogpeppefwereade: if you like, i'll merge your most recent uniter branch and include it as a prerequisite06:48
fwereaderogpeppe, surely the right thing to do there is to have the upgrader in change of runTasks? the wait & retry business could be handled internally, surely?06:48
fwereaderogpeppe, it's not the code conflict that bothers me so much as the lack of clarity06:49
rogpeppefwereade: it's not *that* straightforward. you want to start the upgrader independently, regardless of what the other tasks are doing. then the wait and retry isn't logic that's in the outer loop (because it's dependendent on what gets downloaded) but in a separate goroutine.06:50
rogpeppefwereade: so the structure starts to look similar to what we've currently got06:50
rogpeppefwereade: i think the notion of a task being able to delay shutdown until it's done what it needs to do is a reasonable one, and one of the reasons we use tombs like we do.06:52
fwereaderogpeppe, yeah, I see that06:53
rogpeppefwereade: last seen: [07:53:09] <fwereade> rogpeppe, yeah, I see that06:54
fwereaderogpeppe, I *think* I'm convinced, although I still don't quite entirely like something about it06:54
rogpeppefwereade: i understand.06:56
fwereaderogpeppe, that said, I think I am coming around to your perspective that something outside the uniter should handle death-watching06:56
rogpeppefwereade: wasn't there a load of mode-specific stuff that you needed to do when killed?06:57
fwereaderogpeppe, the uniter's Dying response is not interesting from your perspective, I think (although there's little point upgrading a dying unit)06:57
fwereaderogpeppe, I think you probably *should* be watching for Dead and crash-stopping though06:58
rogpeppefwereade: "me" being...?06:58
rogpeppefwereade: ah, i see06:58
rogpeppefwereade: you mean i should crash-stop when something returns ErrDead06:59
fwereaderogpeppe, almost certainly yes06:59
rogpeppefwereade: maybe. although i think it's a very rare edge case tbh06:59
fwereaderogpeppe, but what I was trying to say is that the upgrader itself should know not to bother watching a unit once it's dying07:00
rogpeppefwereade: the upgrader doesn't watch a unit07:00
fwereaderogpeppe, I submit that it should, so that binary upgrades work within the same framework as anything else -- I don't think they transcend entity lifetime ;)07:01
rogpeppefwereade: there's always a delay between entity being killed and entity actually dying. this just makes it a little longer in some edge cases.07:01
rogpeppefwereade: if you upgrade a system, there's a possibility that some dying units might linger for a few seconds more as they download a new version. i don't think that's too bad a price.07:03
fwereaderogpeppe, well, it means that it continues to act alive -- in some, but not all, respects -- for arbitrarily longer07:03
rogpeppefwereade: does it?07:04
fwereaderogpeppe, it may be that this is actually correct behaviour07:04
rogpeppefwereade: what is the upgrader keeping alive?07:04
rogpeppefwereade: after all, the Unit is marked dead07:04
fwereaderogpeppe, wait, I'm talking about bad behaviour on Dying more than ugliness on Dead here07:05
fwereaderogpeppe, I don't like the ugliness on Dead but I could live with it07:05
rogpeppefwereade: i'm not sure it changes the Dying behaviour at all, does it?07:05
fwereaderogpeppe, the question that currently exercises me is "should Dying entities upgrade their code?"07:05
fwereaderogpeppe, maybe, actually, they should07:06
rogpeppefwereade: i think so07:06
fwereaderogpeppe, yeah, I can imagine a bug blocking clean shutdown that can be resolved by an upgrade07:06
rogpeppefwereade: i think it's nice to have upgrading totally divorced from any of the other logic07:06
fwereaderogpeppe, the fact that they stop watching for charm upgrades on Dying then becomes potentially problematic07:07
rogpeppefwereade: i think charm upgrades are a different thing - they're at a higher level.07:07
rogpeppefwereade: and they really do relate directly to the unit07:07
rogpeppefwereade: so it makes sense not to upgrade a charm when the unit is dying07:08
fwereaderogpeppe, hm, the bug-blocking-clean-shutdown I guess does *not*apply because the user can already use juju resolved07:10
rogpeppefwereade: for charm upgrade, yeah - we're providing the always-available layer on top of a charm.07:11
fwereaderogpeppe, ok, all sounds reasonable, I'll finish the review :)07:11
rogpeppefwereade: tyvm07:11
rogpeppefwereade: there's also this fairly trivial: https://codereview.appspot.com/6564063/07:12
rogpeppeeverything in my window system has just start breaking. i'm gonna reboot07:28
roganyone here use multiple monitors under ubuntu?07:33
fwereaderog, ok, I have another thought07:33
fwereaderog, sorry no07:33
rogfwereade: ok, listening07:33
fwereaderog, I wil be comfortable with this is there is some very basic life handling at the top level07:33
fwereaderog, I think we need:07:34
fwereaderog, 1) drop the unit return from runOnce, because it's potentially panic-inducing anyway07:34
fwereaderog, 2) when we get the unit in runOnce, return a special error on NotFound, and that same special error if the unit exists but is Dead07:35
fwereaderog, 3) if we see that error in Run, return nil07:36
fwereaderog, 4) return the UpgradedError when we're doing an upgrade restart07:36
fwereaderog, 5) add a "normal exit 0" stanza to the upstart conf07:36
fwereaderog, and I think that's it07:36
fwereaderog, that I think conveys my intent as best I can07:37
fwereaderog, the precise details of implementation are ofc approximate07:37
davecheneythe disparity between *ConfigNode.Map() and Charm.Config().Option is a pain in the balls07:38
fwereaderog, does that sound sane07:38
fwereadedavecheney, ouch, I bet07:38
davecheneymap[string]string vs map[string]interface{}07:38
davecheneymix in some json or yaml and it's a royal pain07:39
rogfwereade: i don't know what you mean about dropping the unit return from runOnce - i don't think it can induce panic07:39
rogfwereade: if we can't get the unit in runOnce, we'll never start the upgrader07:39
fwereaderog, bah, true07:39
rogfwereade: we could also check for dead if you like.07:39
rogfwereade: that would be trivial07:39
* rog wishes we used map[string]string throughout07:41
fwereaderog, leaving unit stuff aside for a moment, what is your opinion of the "normal exit 0" thing?07:42
fwereaderog, (when you return ofc007:42
rogfwereade: maybe the agent should just remove its own upstart conf07:44
fwereaderog, that feels icky to me07:45
rogfwereade: who else is going to remove it?07:45
fwereaderog, the machine agent?07:45
rogfwereade: ... or the principal unit agent, right?07:46
fwereaderog, and I think it's fine to have a unit agent run on startup and exit immediately without error to indicate that it's done all it has to do07:46
fwereaderog, yeah, whoever deployed it07:46
fwereaderog, or a machine agent for that matter07:46
rogfwereade: yeah, i think you're right07:47
rogfwereade: presumably it *is* possible to get upstart to never start something again after it's exited ok07:47
rogfwereade: i'm slightly dubious though - if we reboot, surely it'll start again anyway07:47
fwereaderog, AIUI the stanza "normal exit 0" should be sufficient07:48
rogfwereade: ok, sounds fine.07:48
fwereaderog, it will; and the UA will cleanly observe its deadness, exit without error, and never trouble the machine again that run07:48
rogfwereade: interesting point: currently if one of the workers dies without an error, runTasks will just let it die, but continue on. i wonder if actually it should kill everything as usual in that case, so the tasks are always tied together.07:49
fwereaderog, hmmmmmmmmmm07:50
rogfwereade: then the unit agent can exit with a nil error when the unit is dead07:50
rogfwereade: and everything gets shut down cleanly.07:50
fwereaderog, I think that in code I would prefer an explicit error return07:51
rogfwereade: a nil error seems a fine way of saying "i'm shutting down with no error" to me07:51
fwereaderog, an an error seems to me to be a fine way to signal "what you asked me to do ain't gonna happen"07:52
rogfwereade: it's done *exactly* what we asked it to do, surely?07:52
rogfwereade: but... given that it wants other things to die along with it, perhaps ErrDead is good.07:53
fwereaderog, gaah, sorry07:54
rogfwereade: last thing i saw was: [08:52:16] <fwereade> rog, an an error seems to me to be a fine way to signal "what you asked me to do ain't gonna happen"07:55
fwereaderog, well, we get to choose how we define it -- I see the condition of lacking a unit as fundamentally an error condition for the uniter, because it means it can't do anything07:55
fwereaderog, the client code may be in a position to handle that specific error in a different way07:55
rogfwereade: zero is a ok value :-)07:55
rogfwereade: last thing i said before you went:07:56
rog[08:53:45] <rog> fwereade: but... given that it wants other things to die along with it, perhaps ErrDead is good.07:56
fwereaderog, a Uniter *needs* a viable Unit to do its job -- if it cannot get the unit, or the unit is dead, that is an error :)07:56
fwereaderog, ah, I missed that, sorry07:56
fwereaderog, +1 on ErrDead causing nil return from Run07:56
rogfwereade: ok, will do07:57
fwereaderog, btw, would you put it somewhere easily accessible, like state, so I can also return it from the Uniter please?08:00
rogfwereade: it could even go in the Uniter if we wanted08:01
rogfwereade: i mean, in worker/uniter08:01
fwereaderog, +108:01
fwereaderog, ok, sorry to keep banging on about the unit return, but it really doesn't feel right08:06
fwereaderog, and it's a little goroutine-icky, even if not technically unsafe08:06
rogfwereade: it's never used two goroutines simultaneously08:06
fwereaderog, but it crosses my mind that the *upgrader* already has the unit, and can surely send down its PathKey in the UpgradedError itself?08:06
fwereaderog, that's why I said not unsafe08:07
rogfwereade: i'm not sure i see the issue08:07
rogfwereade: runOnce only returns when none of its sub-tasks are running08:07
rogfwereade: therefore there can be no problem with goroutine ickiness08:07
rogfwereade: i may be on crack08:08
fwereaderog, I guess I just don't like a return value that is present-but-useless when no error, or most errors; present-and-useful on one specific error, and not-present on some other errors08:09
rogfwereade: it's always present :-)08:10
rogoh no it's not08:10
fwereaderog, so you somehow return a unit when a unit is not found?08:10
fwereaderog, ISTM that the PathKey is the important thing, not the context, and that that would be a fine thing to send down with the error08:10
fwereaderog, sorry, "not the context" is kinda meaningless08:12
rogfwereade: that does assume that the agent is *always* going to be named after the PathKey.08:12
rogfwereade: otherwise the upgrader wouldn't be able to send it08:12
rogfwereade: i know what you mean about the ickiness though08:13
fwereaderog, hmm, AIUI that was the only point of PathKey in the first place08:13
fwereaderog, does Machine have one too?08:13
rogfwereade: yeah i know - that's why i wanted to call it "AgentName" ...08:13
rogfwereade: it does08:13
fwereaderog, feels to me like the way to go08:14
rogfwereade: the ironic thing is that we already have the means to make the agent name, without involving unit or machine08:14
fwereaderog, ha!08:14
fwereaderog, but then, meh, we (currently) always need the unit anyway, we may as well ask it what it thinks08:14
fwereaderog, +1 on AgentName08:15
rogfwereade: been there, it was wrong08:15
fwereaderog, understood, but it may just have been an idea before its time :)08:15
rogfwereade: maybe. perhaps i'll start with PathKey and see whether the incongruence might change niemeyer's mind08:18
fwereaderog, SGTM08:21
rogfwereade: the other thing that occurred to me is the UpgradedError could contain a closure which would do the upgrade (i.e. call ChangeAgentTools)08:22
rogfwereade: but that's probably a bit icky08:23
fwereade<fwereade> rog, the only thing that stopped me proposing that was a feeling that UpgradedError was weird enough already08:24
fwereade rog, but I think it would probably actually be the cleanest solution08:24
rog[09:23:16] <rog> fwereade: but that's probably a bit icky08:25
fwereaderog, not sure -- it's already not quite right, because it's not an upgrade*d* error08:26
fwereaderog, it's actually an UpgradeReadyError08:26
rogfwereade: i dunno - it's "someone has upgraded me"08:26
fwereaderog, and if you think of it like that, a RunUpgrade method mades sense08:26
fwereaderog, the actual upgrade of the agent does not take place until the agent calls ChangeAgentTools though08:27
rogfwereade: UpgradeReadyError doesn't sound much like an error08:27
fwereaderog, in what way is it worse than UpgradedError>08:27
rogfwereade: i agree08:27
rogfwereade: you're probably right, it's just about the same08:27
fwereaderog, indeed -- the whole error idea is what still feels mildly abusive08:28
fwereaderog, but I think at least it's worth a try08:28
fwereaderog, https://codereview.appspot.com/6561063/ reviewed anyway08:32
fwereaderog, https://codereview.appspot.com/6564063/ LGTM08:37
rogfwereade: thanks08:37
rogfwereade: dammit, the upgrader can't do the actual upgrade easily08:49
rogfwereade: it doesn't have agent.Conf.DataDir08:49
fwereaderog, really? blast08:49
rogfwereade: and i'm slightly reluctant to pass that in just for this08:49
fwereaderog, doesn't it use DataDir somehow to figure out the place to put the real tools dir in the first place?08:50
fwereaderog, I *thought* the upgrader put the tools, and then just told the client that they can now symlink to the tools' known lcation08:50
rogfwereade: yes, it does indeed have dataDir around, you're right08:50
fwereaderog, cool08:51
rogfwereade: ok, np then08:51
rogAram: mornin'09:20
TheMueAram: moin09:29
rogTheMue: hiya09:34
TheMuerog: didn't we already seen 3h ago? ;)09:35
rogTheMue: i thought maybe i hadn't :-)09:35
TheMuerog: it's ok, we both are 40+. so i know that. :P09:36
rogfwereade: do you think *all* upstart scripts we produce should have the "normal exit 0" stanza?09:36
rogTheMue: :-)09:36
fwereaderog, hmm, it demands a similar change in the machine agent, but -- for our purposes at least -- that might be a sensible way to go09:37
fwereaderog, it will not be unbearably hard to make it configurable later if we want to09:37
rogfwereade: i've already made the change to the machine agent09:38
fwereaderog, well then go for it :)09:39
TheMuerelease meeting time11:02
* davecheney waits11:02
TheMuei ping the boss11:03
Aramaah, meeting.11:04
AramI forgot about this.11:04
TheMuehmm, no answer11:04
rogfwereade: this is still WIP, but hopefully is closer to what you'd like: https://codereview.appspot.com/656106311:07
TheMueah, the master11:14
TheMuejust wanted to collect the state of all to send it to you, but now you're here. fine.11:15
rogdavecheney, fwereade: invites are out11:18
Aramdavecheney: fwereade: https://plus.google.com/hangouts/_/9dde8fe32d77795910d89838149fce4ccddea664?authuser=1&hl=en11:18
rogi lost conn11:27
rognow "we're having trouble connecting with the plugin"11:28
rogfwereade: presumably your net connection isn't good enough for a hangout this morning?11:36
fwereaderog, oh *hell* I completely forgot and was eating lunch11:36
fwereaderog, joining11:36
rog fwereade: https://plus.google.com/hangouts/_/9dde8fe32d77795910d89838149fce4ccddea664?authuser=1&hl=en11:37
davecheneyall: http://paste.ubuntu.com/1247363/11:44
davecheney^ hook, sadness11:44
TheMuefwereade: is the current ServiceRelationWatcher what you expected with issue #1032539?11:48
fwereadeTheMue, just a mo, let me take a look; if it has Added/Removed, no11:49
rogdavecheney: have you looked at the log output?11:49
rogdavecheney: that's where the hook output will go, which should be diagnostic11:49
rogfwereade: done, i think: https://codereview.appspot.com/656106312:06
rog(live tests pass)12:07
fwereaderog, awesome, I'll take a look in a sec12:07
rogfwereade: thanks12:07
rogfwereade: trivial? https://codereview.appspot.com/656806412:32
fwereaderog, LGTM on UpgradeReadyError... oh wait there was something I meant to check12:41
fwereaderog, what's the deal with changing the arge to the machine agent?12:41
rogfwereade: hmm, let me check12:41
rogfwereade: i'm not sure what you mean. which argument?12:42
fwereaderog, I can't find it, I may be remembering from an older version12:43
fwereaderog, yeah, it doesn't exist -- sorry12:44
fwereaderog, https://codereview.appspot.com/6568064/ looks trivial to me12:46
rogfwereade: cool, will submit12:46
rogfwereade: (thanks!)12:47
niemeyerHello all!13:04
niemeyerSorry, my phone warned me about the early meeting a bit late.. did we have one?13:06
Aramwe did13:06
niemeyerAram: Nice13:10
niemeyerAram: Have you seen the machines watcher I've pushed?13:10
Aramniemeyer: still wip, found a bug and making final adjustments now.13:11
Aramniemeyer: no, I'll take a look.13:11
niemeyerAram: https://codereview.appspot.com/6566066/13:11
niemeyerAram: Thanks13:11
niemeyerAram: Since we're both working on that stuff, it's good to cross-review13:11
niemeyerAram: Btw, I've talked about a similar pattern to the watcher you're pushing with William in the sprint, and now it occurred to me that you were not around in the conversation13:12
niemeyerAram: is it using a single goroutine, or a goroutine per unit?13:12
Arama goroutine for the machine plus one goroutine for each principal unit (not for subordinates).13:13
niemeyerAram: I don't think we need more than a single goroutine13:15
niemeyerAram: The logic ends up simpler rather than more complex with a single goroutine13:16
niemeyerAram: Please have a look at RelationUnitsWatcher13:16
niemeyerAram: It has a similar problem in which it has to decide what to watch as it goes13:16
Aramniemeyer: yeah, it can be done with only one goroutine. I'll change it after I fix this one bug. I did it this way because it matched the way I thought of the problem, but it's easy to change now.13:17
niemeyerAram: It started with the same design of one-per-subcontext, and then after a conversation it got refactored to be a single goroutine13:17
niemeyerAram: Yeah, I totally understand that.. it feels "right", which is why I figured I should ask13:18
rogniemeyer: yo!13:27
niemeyerrog: Yo!13:28
rogniemeyer: after discussion with william this morning, i made quite a few changes to the uniter upgrade branch13:28
rogniemeyer: i hope you find them ok...13:28
rogniemeyer: https://codereview.appspot.com/6561063/13:28
rogniemeyer: am just running the first live test on the machine agent with provisioner and firewaller in-built13:29
rogniemeyer: if that works, we're basically there for upgrading13:29
niemeyerrog: It looks like you've made changes to the upgrader itself?13:30
rogniemeyer: as requested, yes13:30
niemeyerrog: Why did it affect the branch, specifically?13:30
rogniemeyer: because fwereade was not happy about the fact that runOnce returned the unit13:31
rogniemeyer: because it was only valid sometimes13:31
rogniemeyer: so the fix was to make the upgrader responsible for doing the actual upgrade itself.13:32
niemeyerrog: Yeah, I'm not against it for sure.. sounds like a great idea13:32
niemeyerrog: I just wish this was done by itself13:32
niemeyerrog: Rather than on top of another big branch13:32
rogniemeyer: yes, perhaps i should have done that. it seemed like i'd have got into a twisty mess, but it may have been ok13:33
niemeyerrog: I think that's where I am right now :)13:33
rogniemeyer: ok, i'll back out the changes and create another branch13:34
niemeyerrog: It's fine, I'm already on it13:34
niemeyerrog: if err == uniter.ErrDead {?13:36
niemeyerrog: Was this merged with William's change?13:36
rogniemeyer: no, this was another thing that william suggested13:36
rogniemeyer: perhaps i should have left that for later too13:36
niemeyerrog: How's that related to upgrading?13:37
niemeyer 82         if state.IsNotFound(err) || err == nil && unit.Life() == state.Dead {13:37
niemeyerrog: Gosh man..13:37
rogniemeyer: it's related to the logic around upgrading.13:37
niemeyerrog: That's related to unit lifecycle13:38
rogniemeyer: william asked me to change it, so i did. i'll change it back, np13:38
niemeyerrog: Which is not handled now, and as far as I understand is completely unrelated to upgrading13:38
niemeyerrog: Okay, can we please go back to applying the upgrading pattern that was already in place to the unit?13:39
niemeyerrog: Or, optionally, refactor the upgrading out *without* adding it to the unit13:39
niemeyerrog: and then add it to the unit in a separate step13:39
niemeyerrog: The branch as it is seems somewhat out of scope in whichever angle we look13:39
rogniemeyer: if i just take out the lifecycle changes, would that be enough?13:40
niemeyerrog: I have no idea.. right now I have a branch that has at least: 1) Add upgrade support to unit; 2) Refactor the upgrade machanism; 3) Add lifecycle support to the unit13:40
niemeyerrog: I'd do them as (2), (1), and keep (3) out entirely.. William is working on it13:41
rogniemeyer: ok13:41
niemeyerrog: I'd also be fine with (1) + (2), though, as that's the order you did and might be easier to get back onto that state13:42
rogniemeyer: that would indeed be much easier, as i did the lifecycle thing last13:43
niemeyerrog: Right13:43
rogniemeyer: thanks13:43
rogniemeyer: i should know better when to stop what i'm doing and make a new independent branch13:43
niemeyerrog: I have a hard time understanding it, to be honest13:44
niemeyerrog: It's so pleasing to get small branches in..13:44
niemeyerrog: Fast, painless13:44
niemeyerrog: Easy for people to look at and "Hah, of course we want that"13:44
rogniemeyer: yeah, but in this case, the incentive was "if i just make these few small changes, then william will be happy"13:44
niemeyerrog: You can always make William happy in another branch :-)13:45
rogniemeyer: yeah13:45
niemeyerrog: Even more when the change is *already* huge13:45
TheMueso, late lunchtime today, afterwards an appointment outside13:50
Aramniemeyer: you got a review.13:51
TheMueniemeyer: maybe i come back to you in the evening regarding the security groups, but most is already clear13:51
niemeyerAram: Thanks!13:51
Aramniemeyer: I'll mark my branch as WIP and do some more changes.13:51
niemeyerTheMue: Super, that branch Aram reviewed has your test, btw13:51
niemeyerAram: Cool13:51
TheMueniemeyer: yes, i've revied it too and like it13:52
niemeyerAram: Can you clarify this bit: "to check that when we get an event, the lifecycle of the entity is really what we expect it to be."13:53
Aramniemeyer: we get []int{2,3,5}, it would be nice if we could check that 5 is alive because it was just added, 2 is dead and 3 is dying.13:54
niemeyerAram: Hmm.. what are we testing?13:54
niemeyerAram: This feels like testing the test itself? I mean, it's the test itself that is putting the unit to Dead13:55
Aramthe test puts the unit to dead, but perhaps the watcher misfired for whatever wrong reason, stil delivering the correct event. by checking that the unit is dead we make sure that the watcher fired for the correct reason.13:56
niemeyerAram: Sorry, I don't think that's the case. The test is saying "u.EnsureDead()".. it doesn't make sense for this specific test to ask "Is u actually dead?"..13:57
niemeyerAram: We have tests for EnsureDead elsewhere13:57
niemeyerAram: Regarding this: "I pondered about this. If we use Select anyway, why don't we use the real document, machineDoc in this case, and ignore the fields we don't care about?"13:58
niemeyerAram: I was on the fence..13:58
niemeyerAram: I think you're right.. we should just use the machine doc13:58
rogniemeyer: i hope this is more digestible: https://codereview.appspot.com/656106314:20
niemeyerrog: Thanks very much, looking14:20
rogfwereade: i've taken out the life-cycle-related changes - they'll go in another CL14:20
fwereaderog, ok, SGTM, so long as they come soon I won't fret :)14:20
niemeyerrog: At these times I <3 Rietveld14:22
rogniemeyer: yeah, being able to diff against the different stages is invaluable14:23
niemeyerrog: Yeah, and diffing across the back and forth produces clean diffs14:23
rogniemeyer: ah yes14:23
Aramat previous job we had the worst review tool ever.14:23
Aramcustom made in house, probably 15 years ago.14:24
rogfwereade, niemeyer: factored-out branch: https://codereview.appspot.com/6567067/14:33
niemeyerrog: Reviewed14:39
niemeyerrog: The first one, that is14:39
rogniemeyer: tyvm14:39
niemeyerAram: Nasty. I think I never had anything close to Rietveld either, to be honest14:40
niemeyerSo much lifetime wasted :-)14:40
rogniemeyer: the cloudinit change14:42
rogis necessary because the upgrader uses the machine's PathKey for the agent14:42
rogname. I could roll that back and explicitly pass in an agent name to NewUpgrader14:42
roginstead, if you like.14:42
Aramdy = -dx/(x + c1)14:44
Aramy = -I[dx/(x + c1)]14:44
Aramy = -ln|x + c1| + c2' = -ln|x + c1| + ln(exp(c2')) = -ln(exp(c2')|(x + c1)|) = -ln|c2(x + c1)|14:44
Arammeh, sorry, not here.14:44
Aramdamn paste.14:44
niemeyerrog: No, sounds sensible then, thank you14:44
rogniemeyer: cool.14:44
niemeyerAram: Curious14:45
rogniemeyer: BTW for the nil error, i think maybe it would be best as: if err == nil {err = fmt.Errorf("tasks finished with no error") } or something14:45
niemeyerrog: "uniter error: %v" feels very terse and clear14:46
rogniemeyer: rather than "uniter error: nil" which is still not great14:46
rogniemeyer: ok, will do14:46
rogniemeyer: seems funny saying "error" when there's no error, that's all14:46
Aramniemeyer: I found an awesome hacky way of solving the not so complex differential equation x * y' + 1 = exp(y).14:47
niemeyerAram: What does the equation mean?14:48
Aramcan't remember where I first encountered it, particle physics for sure. A new solution just sprang (?) in my mind randomly and I had to write it down.14:52
niemeyerrog: Fair enough regarding the error message, you're right14:52
niemeyerAram: That's how it generally goes :-)14:53
rogniemeyer: ok, i'll go with my suggestion above, thanks14:53
niemeyerrog: Thank you14:53
rogah, this will be why my combined machine and provsioning agent isn't working!14:53
rog2012/09/28 14:47:51 JUJU loaded invalid environment configuration: no registered provider for "ec2"14:53
rognot too hard to fix :-)14:54
AramI must specify that I've probably encountered this equation more than 7 years ago though, heh.14:54
Aramat a particular physics olympiad.14:54
niemeyerrog: Follow up reviewed14:54
rogniemeyer: thanks!14:55
rogniemeyer: i wondered about worker.ErrDead but considered that it might be useful for a client to be able to distinguish which worker had given the ErrDead. but maybe that's overthinking it.14:56
niemeyerAram: Differential equations are surprisingly useful.. I wish I could keep the background math in my head for longer.. but haven't really ever used them in anger, so it remains just as a spare tool14:57
=== hazmat` is now known as hazmat
niemeyerrog: The only agent that can be dead is the one that is running14:58
rogniemeyer: ok.14:59
niemeyerrog: e.g. we can't have a dead provisioning worker without a dead machine14:59
AramI have to step out early today guys, but I'll finish work in the weekend on the watcher.15:00
rogAram: have fun!15:00
niemeyerAram: Have a pleasant EOD15:00
rogmachiner/provisioner/firewaller upgrade worked live, yay!15:15
niemeyerrog: WOAH15:17
niemeyerI guess we're closing September in pretty good shape15:18
rogniemeyer: in worker: var ErrDead = errors.New("agent object is dead") ?15:20
rogniemeyer: better than we were a week ago, no question :-)15:21
niemeyerrog: s/object/entity/, otherwise LGTM15:22
rogniemeyer: cool15:22
rogniemeyer: https://codereview.appspot.com/6570063/15:51
rogniemeyer: the final piece of the puzzle15:51
niemeyerrog: Looking15:51
rogfwereade: https://codereview.appspot.com/6570063/15:51
fwereaderog, ack15:52
fwereaderog, that is awesome15:54
fwereaderog, LGTM15:55
rogfwereade: cheers!15:55
rogfwereade: i need to do another branch to pass the state into the Machiner directly, i think.15:56
rogfwereade: or maybe niemeyer will call that out in this branch15:56
rogfwereade: actually, nope, it's not a problem, cool.15:59
niemeyerrog: Done15:59
rogniemeyer: thanks!15:59
niemeyerrog: Very cool15:59
rogniemeyer: it was very easy and *almost* worked first time...16:00
niemeyerrog: Great stuff16:00
niemeyerLunch is calling16:01
rogright upgrades are GO!16:11
rogmramm2: ^16:11
mramm2rog: awesome16:20
rogmramm2: the only thing we don't have currently is the --bump-version functionality.16:21
rogniemeyer: stage 1 of --bump-version: https://codereview.appspot.com/656006616:43
niemeyerrog: Looking16:43
niemeyerrog: Done16:47
rogniemeyer: thanks!16:48
niemeyerrog: My pleasure16:48
rogniemeyer: BTW, about --bump-version:16:48
rogniemeyer: i'm wondering when we *don't* want it enabled16:48
rogniemeyer: if we're uploading tools to private storage16:48
niemeyerrog: Any time checking out a release and uploading it16:49
rogniemeyer: why would we want to do that without bumping the build version? nothing will see the new release.16:50
niemeyerrog: Bumping the version is a hack.. uploading tools to private storage is not16:51
niemeyerrog: People can upload tools to private storage in their own cloud, with a real juju release16:51
rogniemeyer: ok. should we make it an error if the tools already exist in the storage then?16:51
niemeyerrog: How's that connected to the above?16:52
rogniemeyer: because otherwise it's likely to be a mistake, and people might be surprised when nothing happens.16:52
niemeyerrog: When you copy a file to a location that already contains the same file, nothing happens other than the same file being in the same place16:52
niemeyerrog: That's not surprising16:53
rogniemeyer: i'm not sure. in this case, it may well feel like we're asking juju to use a particular version.16:53
rogniemeyer: which it will be, just not the version we've just uploaded16:54
niemeyerrog: It will be the version we just uploaded if that's the version that is being used16:54
niemeyerrog: I don't see the problem, really16:54
niemeyerrog: --upload-tools uploads tools.. that's all16:54
rogniemeyer: well, the problem there is we've got two "versions" of the s/w with the same version number16:54
niemeyerrog: If people expect more than this, they'll be wrong16:55
rogniemeyer: it's actually "upgrade-juju --upload-tools" and the problem i see is in the first word there.16:55
niemeyerrog: That means you've changed the release16:55
rogniemeyer: yeah - we've changed the release but the version number hasn't changed. that feels a bit like a mistake to me.16:56
rogniemeyer: i'm trying to imagine a situation where it's useful to overwrite the existing tools for a given version16:57
niemeyerrog: If you checkout 2.2.0, that's 2.2.016:57
niemeyerrog: Not, not
rogniemeyer: ok, that's fine.16:58
niemeyerrog: We're doing development, and we have tools to do what we want when we have to16:58
niemeyerrog: Thus --bump-version16:58
rogniemeyer: if someone has already checked out and uploaded 2.2.0, is it useful to them to be able to overwrite the existing 2.2.0 in storage?16:58
niemeyerrog: Don't know.. doesn't look like a problem we should worry about right now16:59
rogniemeyer: (i'm not suggesting automatic --bump-version BTW)16:59
rogniemeyer: ok16:59
niemeyerrog: If people ask to upload, and it uploads, sounds fine16:59
niemeyerrog: We can fine tune that behavior over time too.. anything we think right now will likely be wrong in the real world17:00
rogniemeyer: ok, seems reasonable17:00
rogniemeyer: submitted. ... and with that, it's time for me to call it a week. oh, i might sign off a few bugs first :-)17:07
niemeyerrog: Super, thanks for the great stuff this week17:07
rogniemeyer: got there in the end :-)17:08
niemeyerrog: Yeah17:08
rogfwereade, Aram, niemeyer, everyone else: have a great weekend!17:14
fwereaderog, and yourself :)17:14
niemeyerrog: Thanks, you too!17:14
fwereadeniemeyer, btw, I just put a couple of trivials in the queue17:14
niemeyerfwereade: I'll look right now17:14
niemeyerfwereade: Done17:19
niemeyerfwereade: Done17:21
fwereadeniemeyer, lovely, thanks17:21
niemeyerfwereade: My pleasure.. have a couple of questions on the second one17:22
fwereadeniemeyer, the bits you're asking about are basically the same17:22
fwereadeniemeyer, if you do a plain waitHooks{}, it means, "Iexpect no further hooks to have been run"17:22
fwereadeniemeyer, the initial StartSync is to poke the uniter into responding to events that might cause spurious events to happen17:23
fwereadeniemeyer, the subsequent ones are to make sure that it responds to state changes in a timely way17:23
fwereadeniemeyer, sound sensible?17:24
niemeyerfwereade: Ah, yeah, thanks.. I suggest dropping it down to, say, 100ms then, since this is the happy case17:25
fwereadeniemeyer, SGTM17:25
niemeyerfwereade: We're getting a second every 5 of those, otherwise17:25
fwereadeniemeyer, indeed, dropping it will not hurt at all17:25
fwereadeniemeyer, ok to submit with that change?17:26
niemeyerfwereade: LGTM17:26
fwereadeniemeyer, cheers17:26
fwereadehmm, got to go out in a mo, actually, but they should be in soon enough :)17:27
niemeyerfwereade: Have a good weekend, and thanks for the hard work too.. feels like we're on the runway for a fully working implementation17:30
* niemeyer steps out for a while19:41

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