[02:11] wallyworld_: do you know why bootstrap forces --upload-tools for the local provider? [02:12] axw: it doesn't distinguish, so if it doesn't need to for the local provider, that needs to be added in [02:12] it was an oversight on my part i think [02:13] axw: although local provider has always been funky with tools [02:13] wallyworld_: what I mean is, in cmd/juju/bootstrap.go, it goes: if provider == 'local': uploadTools = true [02:13] I just don't understand why. [02:13] ah ok. that was deliberate then, by tim [02:13] yeah [02:13] don't know why? :) [02:14] convenience? [02:14] i think it's because the jujud needs to be put in the local provider's storage [02:14] so it can find it [02:14] cause local provider does not want to have to use streams.canonical.com etc [02:14] so the local provider has been treated as an edge case [02:15] but it is known to be a dirty great hack [02:15] why not use streams.canonical.com like the others? [02:15] it can be overridden with --upload-tools [02:15] when it was written, there was no streams,canonical.com. there was an s3 bucket and you needed credentials for that [02:16] so it was done out of necessity [02:16] ok [02:16] we can probs revisit that now [02:17] s/probs/should :-) [02:19] hm, I removed it and get errors about not finding 1.17 at s3. that could be due to some other changes I have going on tho [02:20] i think sync tools used to look at s3 [02:20] and that's what upload-tools uses [02:20] if are running the dev version, there are no 1.17 tools [02:20] s/dev/from source [02:21] no, but it should upload the local ones as a last resort [02:21] so that's the other issue - running from source also requires a upload tools [02:21] it should have i think yes [02:21] I'm making providers responsible for selecting bootstrap tools [02:21] must've broken it [02:21] :-) [02:40] wallyworld_: wasn't my code- the local provider explicitly sets agent-version to version.Current if it doesn't have agent-version already [02:40] ok [02:41] which stops the auto-upload stuff [02:41] if there a agent-version check somewhere? [02:41] yeah, in the current code as well. It won't try to auto-upload if the user has specified agent-version [02:42] we could perhaps be a bit smarter, and check if agent-version is the same as what's on disk [02:42] i wonder why that code is there [02:42] but ... I'm not convinced the local provider needs all this special stuff [02:42] so you don't accidentally get a version you didn't ask for, I guess [02:42] i know there was a lot of hair pulling at the time, but am not sure of the details [02:43] would be worth a chat with tim when he's back to get the back story on it all [02:43] yup definitely [07:04] sinzui: it is "tsv" as tab-separated-value so it wasn't supposed to be changed [07:09] axw, heyhey [07:09] I'm around if you are [07:09] fwereade: morning [07:09] I am [07:10] brb, just getting a drink [07:10] axw, actually I'm around in 5 mins after a quick ciggie [07:10] axw, you're too quick for my morning brain ;) [07:10] nps [07:31] fwereade: I can't hear you if you're talking [07:49] * fwereade is off to get up and run a couple of errands, bbiab [07:49] wallyworld_: would you be able to review this for me next week? https://codereview.appspot.com/14433058/ [07:49] (please) :) [08:06] mornin' all [08:59] axw: sure, will do [09:12] hey rogpeppe1, thanks for the review. let me know if you have any thoughts about my response (particularly around the ConfigureBase name) [09:12] axw: looking [09:14] axw: the underlying problem, i think, is that it ties together an random bunch of stuff, so it's difficult to think of a nicely descriptive name for it [09:15] rogpeppe1: what's the alternative? ConfigureAuthorizedKeys and ConfigureOutput? [09:16] * rogpeppe1 goes to look at the manual provider case [09:16] maybe we don't even both with that method, and have provider/common do it. [09:17] bother* [09:17] ah, but provisioning non-bootstrap machines needs it too [09:17] rogpeppe1: manual provider only uses ConfigureJuju [09:18] axw: hmm, that's interesting in itself - where does its cloudinit output end up? [09:18] rogpeppe1: it doesn't use cloud-init [09:18] all output goes to stdout [09:18] which comes back to the SSH client [09:18] axw: if it doesn't use cloud-init, what's it doing with ConfigureJuju? [09:19] rogpeppe1: it translates the cloudinit.Config into a script. there's an open bug to update cloud-init to have a way of invoking manually/interactively [09:20] axw: oh gosh [09:20] axw: where does that translation happen? [09:21] rogpeppe1: currently, environs/manual/agent.go; I have a followup CL that moves this to a new package, cloudinit/sshinit [09:21] and makes it sufficiently generic of course [09:22] axw: well, in that case, it would be fine for the manual provider to use exactly the same base config, no? [09:22] axw: because it will ignore what it doesn't care about [09:22] rogpeppe1: depends on whether we want to do different things in ConfigureBase (like run additional commands) [09:23] axw: if we do, then we probably want those commands to work in the manual provider too, i'd guess [09:23] rogpeppe1: hmm perhaps, yes. actually, now that I think of it... there's probably no good reason to split them anymore [09:24] axw: this is part of the problem with the name "base" - it implies it's "base" for everything, but it's really "base for everything other than the manual provider" [09:24] rogpeppe1: I went through a few iterations here [09:24] axw: sure [09:24] I'll ponder a bit [09:24] axw: it kinda feels trivial, but it's also the kind of thing that can grow in complexity [09:24] yup, fair enough [09:24] axw: so i'd like us to get the abstractions right here [09:25] it's confusing enough as it is anyway :) [09:25] axw: yeah [09:25] so many different interactions [09:33] axw: it's one of those places where the code with common pieces factored out is arguably harder to read and maintain than the original copypasta. [09:33] rogpeppe1: I lied apparently, anyway. I didn't end up using ConfigureJuju in environs/manual. I was going to use it in the followup, but I can use the existing one after all [09:43] fwereade: is it possible you could have a glance through this doc for sanity checking, please? https://docs.google.com/a/canonical.com/document/d/1vZrav8Do80Lnk1uFNdVVV2jRqpEH99wcX3YWErPwWIg/edit [09:44] fwereade: nate has a couple of reasonable comments, but i don't want to change things away from your suggestions without your goahead [09:50] rogpeppe1: LGTM stands with the two functions joined? [09:51] axw: i think so. what's actually changed in that case then? [09:51] rogpeppe1: just the signature (it was a bit funky - took an arg that's modified and returned it), and dropped the unnecessary New function [09:52] the file could be reverted, but I think it's a bit cleaner now [09:52] axw: looks like you haven't proposed the changes yet [09:55] rogpeppe1: it was a hypothetical question, but the changes are coming.. super slow from here [09:56] axw: ah, that's fine. in principle it all sounds great. [09:56] rogpeppe1: updated now [10:00] interesting behavior, first eth0 closed, later whole instance terminated, but netstat still sees an established connection :( [10:02] axw: reviewed [10:03] thanks rogpeppe1, good suggestions - will do them before landing [10:03] axw: thanks [10:03] I mean to use US spelling, it's just automatic [10:04] axw: yeah - i try to use the US spelling everywhere just for consistency with the go std lib [10:04] I defied it at IBM for a long time - but that was internal code [10:04] yup, fair enough [10:05] axw: i will still use initialise and colour everywhere *except* in the source tree :-) [10:10] alright, that's it for me. have a nice weekend all [10:10] axw: have a good one === rogpeppe1 is now known as rogpeppe [10:26] rogpeppe, can I trouble you for a review of https://codereview.appspot.com/26100043/ ? I am catching up to the point I can review your doc I think [10:27] fwereade: looking [10:27] rogpeppe, mgz is ocr but I can't see him [10:27] rogpeppe, cheers [10:28] fwereade: are there any pieces that i should look at in particular (i presume it's all been reviewed before when it went onto trunk) ? [10:30] fwereade: in particular, are there any bits you made some manual changes in, rather than just running patch? [10:34] rogpeppe, the hack in jujud/machine_test.go is the only bit that hasn't been reviewed individually [10:35] fwereade: is all the code in state/cleanup.go new, or is it just moved from elsewhere? [10:37] fwereade: ah, i see; some's new. [10:39] rogpeppe, I'm pretty certain all the revs I cherrypicked were mentioned in the message so there is at least some route back to the original reviews [10:40] fwereade: ah i see. in future it would be good if those were codereview links [10:40] fwereade: or MP links anyway [10:42] fwereade: reviewed [10:47] rogpeppe, fwereade, wallyworld_ : standup === 3JTAADOM8 is now known as wallyworld === wallyworld is now known as Guest37360 [11:09] dimitern: https://codereview.appspot.com/22100045/ [11:10] rogpeppe, cheers [11:17] can anyone explain to me how this line of code actually works? /home/rog/src/go/src/launchpad.net/juju-core/state/environ.go:34 [11:18] rogpeppe, err := st.environments.Find(D{{"uuid", D{{"$ne", ""}}}}).One(&doc) ? [11:18] dimitern: yeah [11:18] dimitern: i suspect it only works by accident [11:18] rogpeppe, what's wrong with it [11:18] dimitern: there's no uuid field in the document [11:19] rogpeppe, ha, really? [11:19] dimitern: i think it only works because "" != nil [11:19] rogpeppe, and it's always "" [11:19] dimitern: yeah - there's a UUID field in the struct, but it's renamed to _id [11:19] dimitern: it's always nil [11:19] dimitern: because it doesn't exist [11:20] rogpeppe, so this block returns err == nil [11:20] rogpeppe, and works, i've tried [11:21] dimitern: it'll correctly fetch the only environment document [11:21] rogpeppe, what if there are more than one? [11:21] dimitern: it would fetch the first [11:22] rogpeppe, this looks like a lurking bug [11:22] dimitern: +1 [11:22] rogpeppe, good catch [11:22] dimitern: i'm not sure why there's any search condition in there at all, tbh [11:23] dimitern: i don't know any case where we'd insert a document with an empty uuid [11:23] rogpeppe, because I think when environs were added to the schema it was unclear how to refer to them [11:23] rogpeppe, or that changed later with the uuid perhaps [11:24] rogpeppe, what's the latest on that panic of davecheney's? Irecall you talking about it yesterday but I hadother thingsonmy mind [11:24] dimitern: really i think we should hand the uuid to the state when opening an environment [11:24] fwereade: the gccgo issue? [11:24] rogpeppe, TheMue, that's in rev 1122 [11:24] fwereade: it's much less of a problem than we thought [11:24] rogpeppe, I have no context on it, but it's targeted for 1.16 [11:24] rogpeppe, does it not need to be? [11:25] fwereade: it is a gccgo bug though, and we should work around it [11:25] fwereade: for the time being [11:25] fwereade: the problem only happens when the gccgo runtime tries to call a function/method that returns an empty struct [11:26] fwereade: i think there's probably only one such method (Pinger) [11:26] fwereade: and we can trivially work around the issue by including a dummy field (e.g. _ bool) [11:26] fwereade: in the srvPinger struct type [11:27] fwereade: it seems like a good candidate for backporting to 1.16 (a 1 line change) [11:27] rogpeppe, sorry, I'm thinking of https://bugs.launchpad.net/juju-core/+bug/1227952 [11:27] <_mup_> Bug #1227952: juju get give a "panic: index out of range" error

[11:28] rogpeppe, (is anyone building 1.16 with gccgo?) [11:28] fwereade: ah, that was a goyaml bug which is now fixed [11:28] fwereade: i dunno [11:28] fwereade: the 1.16 deps would need updating [11:29] fwereade: that's all, i think [11:31] afk [11:45] back === gary_poster|away is now known as gary_poster [13:16] * rogpeppe wishes it took less than 3 minutes to run lbox propose [13:21] mgz, natefinch, fwereade: first step towards keeping track of state servers: https://codereview.appspot.com/26880043 [13:22] * rogpeppe goes for lunch [13:23] rogpeppe: looking [14:00] mgz: did you have some queries about it? [14:01] fwereade: i would really appreciate your feedback too, please, as i don't want to go up another blind alley [14:01] rogpeppe: I'm looking, but need a little more time with it... once my two very cute distractions are with their mother, I'll be able to give it more attention [14:01] natefinch: ok [14:01] rogpeppe: which will be shortly [14:41] can anyone think of a nicer way to test (in mongo) if the length of an array is odd than this? $where: function() { return this.stateservers.length % 2 == 1} [14:41] that seems reasonable [14:42] i don't think it's a problem in this case, but it would be nice to use built-in ops if possible [14:42] mgz: did you manage to review the above CL, BTW? [14:42] mgz: 't'would be much appreciated if you could [14:42] rogpeppe: yeah, getting there... sorry [14:42] mgz: np [14:42] rogpeppe: I'm in the middle of it too. Looks fine so far. [14:43] natefinch: thanks [14:43] rogpeppe: also, %2 == 1 is pretty standard "is odd" ... seems unlikely there'd be a built-in op, but I don't know for sure [14:43] natefinch: well, there's a built-in $mod operation [14:43] natefinch: but i'm not sure there's a way to apply it to the length of an array [14:50] rogpeppe: doesn't look like it's possible [14:50] natefinch: yeah [14:51] natefinch: as it turns out, i don't need to do it anyway... :-) [14:52] rogpeppe: even better [14:54] good god state_test.go is too big [14:56] rogpeppe: reviewed, just some side questions due to my shakey understanding of state management bits. [14:56] natefinch: yeah, State is big [14:57] mgz: ta! [15:09] fwereade: could we have a chat about this? [15:09] rogpeppe, ofc [15:09] fwereade: https://plus.google.com/hangouts/_/calendar/am9obi5tZWluZWxAY2Fub25pY2FsLmNvbQ.mf0d8r5pfb44m16v9b2n5i29ig?authuser=1 [15:38] fwereade: i had to reboot, sorry [15:38] fwereade: back there now [15:50] rogpeppe, natefinch : do either you you have a few minutes to review this dep change: https://codereview.appspot.com/26940043 [15:50] sinzui: looking [15:54] sinzui: looks like it should be rev 50, not 49 [15:54] really? [15:55] rogpeppe, is tip also wrong? I can set that to 50 [15:55] sinzui: yeah, that would be better, i think (rev 50 altered the rev 49 fix slightly) [15:56] rogpeppe: was trunk bumped to r50? [15:56] mgz: yeah [15:56] mgz: i mean, no [15:56] mgz: goyaml trunk was [15:56] sinzui: BTW I'm a bit surprised there's no test case to go along with this [15:57] rogpeppe, yep. We see it in CI though. the lander would do if it honoured godeps [15:58] sinzui: ah, that's good [15:58] rogpeppe, I am focused on a release today, but I or someone on juju-qa can get such a test by next week. [15:59] such a test would have caught the spaces yesterday [15:59] sinzui: it sounds like there already is a test, if tests now pass in CI but did fail before this fix [15:59] sinzui: ah, but CI doesn't just run the tests, gotcha [16:00] sinzui: yeah, i think this should probably have a test case, but i won't hold up this change for that [16:00] rogpeppe, we are thinking about adding the test runner because it offers a sanity check that the integration tests have a sane start [16:01] sinzui: yeah, i think that's probably worth doing [16:01] sinzui: reviewed [16:18] fwereade, natefinch, mgz: PTAL https://codereview.appspot.com/26880043 [16:37] natefinch: any chance of a LGTM ? [16:44] hmm, i thought that instance.HardwareCharacteristics was a result, not a parameter [16:44] but in state.AddMachineParams it seems to be being used as a kind of constraint [16:46] rogpeppe, it's not aconstraint -- it's there for injected machines iirc [16:46] fwereade: ah, ok [16:47] rogpeppe, it's the result of what we got, that should hopefully match the constraints [16:47] fwereade: i'm wondering whether i should be using addMachineOps or addMachineContainerOps when creating new state server machines [16:47] rogpeppe, the former, almost certainly [16:48] rogpeppe, and please don't be afraid to massage them into a better shape if necessary [16:49] fwereade: i couldn't see how point 2) in the comment for addMachineContainerOps didn't also apply to creating state server machines [16:51] fwereade: and since addMachine itself calls addMachineContainerOps, i thought perhaps i should too. [16:52] * rogpeppe still doesn't really understand the containers stuff [16:53] rogpeppe, it looks like addMachineContainerOps is *actually* there to create a parent machine if it's necessary and doesn't already exist,but Imay be misskimming [16:53] fwereade: that seems plausible [16:53] rogpeppe, it's not helpfully named [16:54] fwereade: in which case, I'd only use it when we come to do --to (or whatever it's called) [16:54] rogpeppe, and would surely benefit from a smart person WTFing at it a bit and probably renaming some bits for clarity ;) [16:54] fwereade: i'm not smart enough [16:55] rogpeppe, sell not yourself short dude [16:55] rogpeppe, but it's not a demand anyway ;) [16:55] rogpeppe, I'm EOD, and I'm afraid I need to be off pretty sharpish -- we are out of booze that cath feels like drinking ;p [16:56] fwereade: just one thing: [16:56] fwereade: i'm presuming it should be started with series taken from environ config default series, right? [16:56] rogpeppe, sure (and I'll try to stick my head back in later as well so leave other questions as necessary, someone else might know the answers in the meantime) [16:57] fwereade: i suggest you alleviate the booze emergency a.s.a.p BTW :-) [16:57] rogpeppe, that sounds like a good start [16:57] rogpeppe, in future --to a machine that isn't default-series should probably be fine too though [16:57] rogpeppe, cheers :) [16:57] happy weekends all [16:57] fwereade: have a good one === TheFreeMue is now known as TheMue [17:04] TheMue: how old are your kids? [17:05] natefinch: 17 and 19 [17:05] * TheMue just fights a bit with his network :( [17:09] natefinch: any chance of another look at the https://codereview.appspot.com/26880043; i'd like to land it tonight if possible, and i haven't got that long [17:16] rogpeppe: yep, looking [17:16] natefinch: thanks [17:22] rogpeppe: btw, you can use jc.SameContents to compare to unordered lists [17:22] natefinch: ah, i'd forgotten that [17:23] natefinch: given that sort.Strings is easy at this point, i might just leave it as is [17:23] rogpeppe: yeah yeah, that's fine. Just a reminder for next time when it's not as easy. [17:23] natefinch: yeah, thanks [17:28] rogpeppe, This is the comparable goyaml dep fix for trunk: https://codereview.appspot.com/26960043 [17:29] sinzui: LGTM [17:29] thank you [17:33] natefinch: personally I feel that it's a mistake for callers of a function to be relying on specific error kinds returned by the function [17:34] natefinch: unless they're explicitly documented [17:34] natefinch: because they're really part of the implementation detail of the function [17:34] natefinch: my errgo package (launchpad.net/errgo/errors) makes that a point of principle and always masks the underlying error kind unless you explicitly ask otherwise [17:38] rogpeppe: the common and expected errors *should* be documented [17:38] natefinch: indeed [17:38] natefinch: and in this case there are none [17:38] natefinch: because the document must have been created already [17:39] natefinch: error kinds are important when a caller might legitimately decide to act differently based on the kind of error [17:39] natefinch: in this case all the errors are as bad as each other [17:40] rogpeppe: yes, definitely. That's fair. I thought there might be some well-known errors, but if there are not, then no big deal. [17:41] natefinch: it's annoying that with the standard error stuff you can't add contextual information to an error without either losing the kind or needing to define your own type [17:42] rogpeppe: that's why I made errorWrapper in errors... though it's not directly exposed currently [17:42] natefinch: check out errgo - I think it works pretty well. i've been using it a reasonable amount. [17:57] rogpeppe: neat, but not sure I understand the use case for having a diagnosis that is different from the actual error [17:58] natefinch: the actual error can have lots of extra context associated [17:58] natefinch: the diagnosis is the metadata of the error, if you like [17:58] rogpeppe: why not just wrap the error in a new one that is more accurate? [17:59] natefinch: because that's a lot of hassle - you need to define a new error type each time. [18:00] natefinch: often it's useful to have classes of error (e.g. "not found") but still keep around some info about the original error [18:00] natefinch: for example errgo keeps information about the source locations where the error was propagated with Wrap [18:00] natefinch: the division seems to work pretty well in practice [18:12] right, i'm done [18:13] natefinch: g'night [18:13] and happy weekends to one and all [18:13] rogpeppe: night [18:53] natefinch, I think gobot sent my a spurious failure. Do I change the review to approved again to retry, or to I need to also add a comment like LGTM, tests pass locally? [18:55] sinzui: you can just re-approve. looked spurious to me [18:55] thank you natefinch === gary_poster is now known as gary_poster|away