[04:08] davecheney: what are you working on currently, BTW? [06:33] davecheney, heyhey [06:53] sup [08:06] mramm, what part of the world are you in atm? :) [08:07] i don't think he's real [08:07] his internets might have burped [08:07] it's liek 4am for him [08:19] Good morning! [08:21] niemeyer, heyhey [08:21] niemeyer, goodness, where are you today? [08:21] fwereade: Nice summary on the upgrade stuff, cheers [08:21] niemeyer, cool [08:22] fwereade: Still at home, just alert enough to feel motivated to do something :) [08:22] niemeyer, well, awesome :0 [08:50] Oh, hey, we've just moved within the company.. [08:50] * niemeyer gets the boxes [08:50] niemeyer, sorry, what happened? [08:50] fwereade: See email from Jane as of 4mins ago [08:51] niemeyer, ah, ok [08:53] niemeyer, well, yay robbie! [08:53] indeed :) [09:06] fwereade, davecheney, niemeyer: morning! [09:06] wrtp: Morning! [09:09] wrtp, heyhey [09:11] fwereade: are you sure you were looking at the latest version of https://codereview.appspot.com/6495086/ when you made your most recent comments? [09:11] wrtp, hmm, perhaps not... I just followed the links in your response [09:11] wrtp, aw crap sorry, 2 new revs -- I'll double-cjeck [09:11] fwereade: ah! that would take you to the previous version, i think (unchanged) [09:12] fwereade: no wonder you didn't think it was fixed! [09:13] wrtp, LGTM [09:13] fwereade: phew! [09:13] wrtp, sorry crack :/ [09:14] fwereade: that's fine; very easy to do. [09:26] niemeyer, does anyone actually use upgrade-charm --dry-run? ISTM that it's kinda worthless, because the charm that would be upgraded to when you dry-run is not necessarily the charm that will be upgraded to when you do it for real [09:27] fwereade: I certainly wouldn't mind leaving the option for a second moment when we understand how it should really behave [09:28] niemeyer, cool, I'll leave it out of the first attempt then :) [09:48] * niemeyer => breakfasting [11:00] Invites sent [11:00] ty [11:02] wrtp: ping [11:02] niemeyer: pong [11:02] wrtp: Meeting time [11:02] niemeyer: just quitting apps on my mac so i can use G+ [11:54] Any chance of a quick review on https://codereview.appspot.com/6501114/ so I can consider these bits done and move onto reviews? [11:54] niemeyer, looking [11:54] fwereade: Cheers! [12:10] niemeyer, LGTM, couple of trivial comments, ignore them if you like :) [12:12] fwereade: THanks! [12:15] niemeyer: a little question: what's the use case for Watcher.Dying? [12:18] wrtp: See the machine watcher [12:18] wrtp: It's in use already [12:19] niemeyer: wouldn't Dead be more appropriate? [12:20] niemeyer: Dying kinda seems like it's an internal state transition of the watcher on its way to being dead. [12:28] lunch, bbs [12:31] wrtp: Yeah, perhaps [12:31] wrtp: Do you see any edge cases where this might be a problem? [12:32] niemeyer: no. i don't think there's any case when you want to know when a watcher is *about* to die [12:32] wrtp: Do you see any edge cases where using Dying might be a problem? [12:33] niemeyer: it gives us leeway to change tomb to record more than one error in the future if we like [12:34] wrtp: Doesn't feel very compelling.. if we change tomb to behave differently, we might also change the watcher to behave differently [12:34] niemeyer: and it just seems like something that doesn't need to be visible - the concept of dead is already there (Wait returns), but Dying is something different [12:34] niemeyer: the tomb isn't part of the watcher interface [12:35] wrtp: Indeed.. but exposing Dead or Dying mean exactly the same as far as that perspective is concerned [12:36] niemeyer: if we wait for Dead, we don't necessarily have to call Wait for it to die completely. [12:36] niemeyer: if we wait for Dying, we do [12:36] wrtp: We don't call Wait, and we don't have to as far as I can see [12:36] niemeyer: we call Stop [12:36] wrtp: Exactly [12:36] wrtp: Which we must [12:37] niemeyer: even when it's told us it's dead? [12:37] wrtp: We must call Stop on state.Close, no matter what [12:37] wrtp: We're not going to be passing information between the watcher and the Close method [12:40] wrtp: Makes sense? [12:41] niemeyer: i'm trying to think whether there might be a case where the message from, say, Watcher.Alive, might override the underlying error encountered by the presence.Watcher [12:43] niemeyer: but perhaps that's simply an argument for including watcher.tomb.Err in the error message [12:43] wrtp: Yeah, possibly [12:44] wrtp: I was on the fence on that, but it does sound like a good idea from that perspective [12:44] niemeyer: actually, Machine.WaitAgentAlive is a better example from that pov [12:45] niemeyer: and if we're going to do that, then i think we *should* have Dead, not dying [12:45] niemeyer: because we'll want to record any error encountered after it's been explicitly stopped. [12:45] wrtp: Sorry, I lost the leap [12:46] wrtp: The thing that caused the unit to die is there once Dying kicks [12:47] niemeyer: yes, but the unit might have been explicitly stopped (no useful error message) but then actually encounter an error when shutting down, which we'd want to see [12:47] wrtp: Exactly, and that was the reason why it returned, and the reason why the cascading watcher was canceled [12:47] wrtp: It's as honest as it can be [12:47] niemeyer: but if we're watching *Dying*, the error might not have been encountered yet [12:48] niemeyer: because the watcher might only be just reacting to the dying message itself [12:48] wrtp: That's what I meant [12:49] wrtp: I'm fine to see a "returning because watcher was explicitly stopped" message if that's what actually happened [12:49] wrtp: The error from the watcher is an internal error, that will be visible from Close [12:49] niemeyer: it may not [12:50] niemeyer: because we might just have recorded the first error we encountered in a tomb before calling Close. [12:50] wrtp: Please read the Close method [12:51] wrtp: It returns the error from Stop from either watcher [12:52] wrtp: Besides that, to be honest if the watcher errored when terminating, and that error caused nothing to fail except the result of Stop, it's pretty boring error [12:52] wrtp: Feels a bit like we're hunting witches [12:53] niemeyer: that may be true. but it's so easy to fix (use Dead rather than dying) that i'd rather have it work [12:53] wrtp: There's nothing to fix, so far [12:53] niemeyer: it's this kind of code i was wondering about: http://paste.ubuntu.com/1198576/ [12:53] wrtp: This is artificial [12:53] wrtp: There's no such thing as [12:53] defer watcher.Stop(t.presenceWatcher, &t.tomb) [12:53] niemeyer: oh, i'm probably misremembering [12:54] wrtp: We don't call Wait, and we don't have to as far as I can see [12:54] niemeyer: we call Stop [12:54] wrtp: Exactly [12:54] wrtp: Which we must [12:54] niemeyer: even when it's told us it's dead? [12:54] wrtp: We must call Stop on state.Close, no matter what [12:54] wrtp: We're not going to be passing information between the watcher and the Close method [12:54] wrtp: I'm happy to change as it makes no difference, but there's no factual reason that I can see [12:56] niemeyer: slightly more realistic, perhaps: http://paste.ubuntu.com/1198581/ [12:57] niemeyer: but tbh nothing is gonna be deliberately closing the state underfoot apart from tests [12:58] niemeyer: another reason to use Dead is it makes the error message more deterministic - w.Err() can change after Dying, but not after Dead. [12:59] wrtp: It can't in this case [12:59] wrtp: If there's an error with the watcher alive, it means we have a real error or we screwed up [13:00] wrtp: watcher.MustErr(st.watcher) [13:00] wrtp: That's there [13:00] wrtp: Which means that error won't change [13:01] wrtp: Ah, not in agent alive, nevermind [13:01] wrtp: Sure, anyway.. we can change [13:02] niemeyer: i know it's a little thing, but worth doing IMHO [13:02] niemeyer: thanks! [13:10] wrtp: np, can you do a CL with this? [13:10] niemeyer: sure [13:10] wrtp: There's Dying both in mstate/watcher and presence [13:13] fwereade: Just sent a review [13:13] niemeyer, cheers [13:14] fwereade: Great stuff, mostly trivials or questions for awereness [13:14] awareness [13:14] niemeyer, cool [13:14] niemeyer, (sometimes it's the questions for awareness that blow the whole thing out of the water, ofc ;)) [13:14] fwereade: LOL [13:14] fwereade: I don't think that's the case in this instance :-) [13:15] niemeyer, good-oh :) [13:22] niemeyer, sent responses, I'll gtw on the non-controversial ones :) [13:30] fwereade: Re-proposed https://codereview.appspot.com/6501114 too [13:30] * fwereade looks [13:33] niemeyer, ok, about changes in pending -- can a sync happen on a different goroutine? [13:33] fwereade: nope [13:33] fwereade: It's all done in a single goroutine [13:33] niemeyer, ok great :) [13:33] fwereade: Yeah, it's complex enough without concurrency issues :) [13:34] niemeyer, the trouble is that it makes sense now, so I can't say what would have made it easier to begin with :) [13:35] fwereade: i think that once you've got used to that kind of pattern, it's fairly easy to spot and verify when you're reading the code. [13:36] niemeyer, LGTM [13:40] fwereade: LGTM too (and that's not just an exchange, promise ;) [13:40] niemeyer, haha [13:41] fwereade: I'll check the new comment when you submit [13:47] wrtp: I'm pushing a trivial branch with the suggested s/Dying/Dead/ [13:48] niemeyer: ok, thanks, sorry, i'm still debugging a change to the authorize ec2 branch [13:48] wrtp: np, that's kind of the point.. you're surely doing more interesting stuff and I'd rather unblock you [13:48] niemeyer: that's appreciated, thanks [13:48] niemeyer, btw, in case you missed it, the one you reviewed is still blocked on https://codereview.appspot.com/6489083/ [13:49] wrtp: np [13:49] fwereade: I've half-missed it [13:49] fwereade: I was already reviewing when I noticed it had a pre-req [13:51] niemeyer, np [13:55] wrtp: https://codereview.appspot.com/6489111 [13:56] niemeyer: reviewed [13:57] wrtp: cHeers [14:03] niemeyer, re ModeUpgrading: it's perfectly legitimate to enter ModeUpgrading in an error state, and all the hook-error-related logic is now in Uniter itself -- I'm going to just drop mention of errors in the ModeUPgrading comment [14:04] fwereade: Sounds good, that was my confusion.. it's talking about errors despite it not really doing anything about them, or even needing anything given what you say [14:09] niemeyer> fwereade: Sounds good, that was my confusion.. it's talking about errors despite it not really doing anything about them, or even needing anything given what you say [14:10] fwereade: By the way, I had to look over Abide in the dictionary, thanks :-0 [14:10] :-) [14:11] niemeyer: a trivial ec2test fix: https://codereview.appspot.com/6501117/ [14:12] fwereade: Seems very appropriate, btw [14:13] niemeyer, fantastic, I just couldn't get over a feeling that I was being hipster-weirdy, despite not being able to come up with a better term :) [14:13] fwereade: Steady would be another choice [14:14] niemeyer, I feel that once I've called it an operation it kinda needs to be a verb [14:14] fwereade: makes sense [14:14] wrtp: So what's up there? [14:15] niemeyer: when i wrote ec2test i thought that (as according to the docs) you could not specify source group names without also specifying an owner id. [14:15] niemeyer: that's not true, and it's nice if we can specify a name only. [14:15] wrtp: Okay, I had the same impression [14:16] wrtp: LGTM if it reflects reality [14:16] niemeyer: it means we don't need a special-case hack for authorizing self in ec2.environ.ensureGroups. [14:17] niemeyer: and it does seem to work live. perhaps i should include a server test to make sure within the ec2 package itself. [14:52] fwereade: Sent comment on https://codereview.appspot.com/6489083/ [14:52] comments [14:52] niemeyer, great, tyvm [14:52] wrtp: Sounds good [14:52] fwereade: np, one point I'd appreciate talking for understanding only, otherwise only boring/trivial stuff [14:54] niemeyer, the SetCharm/Write/Deploy? [14:56] niemeyer, my feeling is that because a Deploy is always preceded by a SetCharm, we only need to commit to the upgrade state once we're about to write to the actual deployment directory [14:58] niemeyer, if we happen to SetCharm twice due to unhelpful process bounces, it's no big deal (in fact SetCharm will just return) [14:58] fwereade: Why isn't it simply Deploy(u.charm, url), or something similar? [14:59] niemeyer, ha, good question [14:59] fwereade: I'm not finding the place where deploy() is called [14:59] niemeyer, primarily because the sheer amount of code in one method was getting me down, and it seemed like a good idea at the time [15:00] * niemeyer looks at the raw diff [15:00] niemeyer, ModeInstalling is the only place in that tree, I think [15:02] niemeyer, sorry to be talking about upgrade above, that's the way I tend to think about it -- but, yes, that CL doesn't include any upgrading [15:03] fwereade: That's certainly fine [15:03] niemeyer: ok, the functionality is now tested live (and a couple of live tests were fixed in the process) [15:03] niemeyer: https://codereview.appspot.com/6501117/ [15:04] fwereade: Why we use the urls that we SetCharmURLs again? [15:04] wrtp: Cheers [15:05] niemeyer, sorry, cannot parse [15:05] fwereade: Trying to figure what we do with the info we give deployer.SetCHarm [15:06] niemeyer, all we use it for *directly* is to avoid setting the same charm again; indirectly it's also returned from u.charm.ReadURL or whatever it's called [15:08] niemeyer, which itself is used to compare against service-charm-change events to determine whether or not we care [15:09] niemeyer, u.charm.ReadCharmURL [15:09] fwereade: Now that I went looking for them, it seemed slightly surprising to have ReadCharmURL/WriteCharmURL on GitDir.. [15:09] fwereade: We should probably move those to functions later [15:09] niemeyer, hmm, it *is* a charm.GitDir [15:10] niemeyer, it seemed like a great idea to me at the time [15:10] niemeyer, I presume you're seeing an ugliness I'm missing [15:10] fwereade: GitDir is not about charm at all, except for those two trivial functions, where one of them is a single line [15:11] niemeyer, well, I'm perfectly happy to move them out, but it feels *slightly* more than coincidental that both GitDir clients find them useful [15:11] fwereade: Which is great, I'm not complaining about where we are [15:12] fwereade: These functions should definitely be within charm, and they can easily take a GitDir [15:12] niemeyer, ok, that SGTM :) [15:13] fwereade: The logic is the same.. it's just that git has no read-charm command :) [15:13] niemeyer, true :) [15:13] fwereade: Anyway, that was a derail, sorry [15:13] niemeyer, np at all, nice easy trivial for later [15:14] weird bzr behaviour: http://paste.ubuntu.com/1198834/ [15:14] i can't make it push the branch [15:14] which causes lbox to fail when proposing [15:15] it works if i fork the branch, delete the old one and rename to the old name [15:15] niemeyer: any ideas? [15:16] fwereade: So, the separation definitely sounds sensible.. my memories were betraying me [15:16] fwereade: I thought SetCharm was doing something else [15:16] niemeyer, np, mine do the same quite frequently ;p [15:18] fwereade: I'm wondering how we can name it in a way that feels like SetCharm and Deploy are complementary operations, that operate on different backing data and do not conflict [15:18] niemeyer, Prepare/Run perhaps? [15:18] fwereade: Hmm.. Stage+Deploy? [15:18] niemeyer, +1 [15:18] fwereade: Super [15:19] wrtp: Looking [15:19] wrtp: % bzr commit -m 'change to try to force bzr to remember push loc' [15:19] wrtp: commit doesn't do that [15:19] wrtp: push --remember does that [15:19] niemeyer, I think I'll propose a trivial rename branch from trunk for the ones we just discussed [15:20] niemeyer: i tried push --remember - it didn't do anything. so i thought i'd try a source change too. [15:20] wrtp: Understood, but changing the history has no relation to metadata [15:21] niemeyer: ok, thanks. so ignore that line and you'll see that there's a problem. [15:21] niemeyer: (i think) [15:22] wrtp: See the .bzr/cobzr//.bzr/branch/branch.conf file [15:22] wrtp: and see where it is pointing to [15:23] fwereade: Cool [15:23] fwereade: So, regarding the WriteState [15:23] niemeyer, yes [15:23] niemeyer: push_location = file:///home/rog/src/go/src/launchpad.net/juju-core/.bzr/cobzr/051-authorize-internal-traffic/ [15:24] niemeyer: which looks very odd [15:24] fwereade: now that my mind is not totally wrong :) [15:24] niemeyer, the underlying idea is to wait as long as possible before we persist the fact that we're upgrading to charm X [15:24] niemeyer: i'll try and reproduce the problem from scratch with lbox -v [15:25] niemeyer, the motivation for doing so is pretty weak, to be fair [15:25] fwereade: What is it? [15:25] wrtp: This isn't about lbox, or even cobzr [15:25] wrtp: This is plain bzr [15:25] niemeyer: ok [15:26] niemeyer, very specific situation: uniter falls down having Staged a new charm, and the user pushes a new charm version before it comes up again -- this means that it will see the newest charm and upgrade directly to that [15:26] wrtp: I don't know why it is misbehaving [15:26] wrtp: Try to delete the push line there [15:26] niemeyer, the adverse consequences of just installing the first charm then the second one are pretty minimal to be fair [15:26] niemeyer: when i fork a fresh branch, the push line is gone. but then it reappears. [15:27] fwereade: Doesn't it also mean it'll download the charm twice on the other scenario? [15:27] niemeyer, downloads won;t be repeated; stages won;t be repeated [15:27] niemeyer, it's easy to detect prior completion of each of those [15:28] fwereade: Cool, I'm happy with the current logic, thanks [15:28] niemeyer, BundlesDir caches, and Stage checks for the existing charm url [15:28] niemeyer, sweet [15:29] niemeyer: yeah you're right, it's just bzr. http://paste.ubuntu.com/1198862/ [15:30] wrtp: Very weird, either way [15:31] niemeyer: indeed [15:34] Stepping out for lunch, biab [15:37] wrtp, can I get a LGTM on the trivial I just discussed? https://codereview.appspot.com/6489112 [15:39] fwereade: LGTM [15:39] wrtp, lovely, thanks === hazmat is now known as kapilt [17:08] fwereade: was there a particular reason you didn't make the "run uniter" branch run the upgrader task? [17:30] * niemeyer waves [17:39] * wrtp waves back to niemeyer [17:39] niemeyer: ec2test fix submitted, thanks. [17:43] wrtp: Thank you! [17:44] Aram: yo! [17:44] hi. [17:44] Aram: Hi [17:44] sorry everybody. [17:44] had internet troubles :(. [17:46] niemeyer: have you seen my latest review for machine watcher? [17:46] my comment about TxnRevno [17:46] Aram: Yes, just answering it now [17:47] Aram: we should all swap mobile numbers so we can have an alternative comms channel when something like that happens [17:47] wrtp: agreed. [17:50] niemeyer: it's a good day for bzr oddities: http://paste.ubuntu.com/1199119/ [17:52] Aram: Responded [17:53] wrtp: lp:launchpad.net? [17:54] niemeyer: oh jeeze, i just couldn't see it! thanks. :-| [18:00] wrtp: I know how it goes [18:01] wrtp: Sometimes everything just feels broken and it's hard to see even the typos [18:01] niemeyer: yeah [18:01] niemeyer: i kinda new it was something obvious but i just couldn't see it! [18:03] fwereade: Is there anything I can do on https://codereview.appspot.com/6489083 to help out, or are you working on it still? [18:05] niemeyer: can i put in a small request for a review of https://codereview.appspot.com/6501106/ ? i'm using it as a prereq for quite a lot of stuff. [18:05] wrtp: Will check it right now [18:05] niemeyer: thanks [18:35] wrtp: done [18:37] niemeyer: thanks a lot [18:37] wrtp: My pleasure, glad to see the progress there [18:38] niemeyer: there's also https://codereview.appspot.com/6495086/ which you mostly reviewed about a week ago, but i changed it a bit since then. [18:38] niemeyer: i think you'll like it :-) [18:47] wrtp: Cheers, I am going through the review list.. if you're not blocked on it, I'll get to it soon [18:47] niemeyer: it is a prereq of another branch which i'm building on in turn, but i wouldn't say i'm blocked on it as such. [18:56] niemeyer: i'm off now. see you tomorrow & have fun. [18:58] wrtp: have a good one [21:23] OMG [21:23] Not only the queue is clean, but we have 9 branches on the runway to land === kapilt is now known as hazmat