[00:40] thumper: http://paste.ubuntu.com/11759872/ [00:40] this morning's results [00:40] there are three packages with races remaining [00:40] 2 of them are going to be involved fixes [00:54] Bug #1467712 opened: cmd/jujud/agent: data race in test [01:04] hmm [01:12] Bug #1467715 opened: worker/peergrouper: data race in package [01:13] thumper: this one is worse https://bugs.launchpad.net/juju-core/+bug/1467715 [01:13] Bug #1467715: worker/peergrouper: data race in package [01:13] davecheney: what's it doing? [01:18] there are lots of races here [01:18] it looks like an internal slice has been leaked to the caller [01:18] and the caller is sorting it [01:19] ouch [01:19] _but_ the race only happens when the test fails [01:20] and these are failures we've seen in ci [01:20] if the test passes, there is no race [01:20] \o/ [01:20] natefinch: happy birthday from the future [01:21] thumper: thanks! :) [01:21] davecheney: lol [01:22] \o/ yes, stop working, go and have a birthday [01:26] Any time I don't have any kids in the same room with me, it's a party ;) [01:26] \o/ [01:42] natefinch: aw, I am too lazy to wait until midnight to tell you hb, can I leverage the fact that most of my team is in tomorrowland and wish you a hb now? [01:43] perrito666: haha, sure :) Thanks :) [02:09] thumper: menn0 https://github.com/juju/juju/pull/2624 [02:09] just a small one [02:11] thumper: want a bug to fix as part of your bug squad fun and games? [02:45] davecheney: looking now... was having lunch [02:47] davecheney: Ship It (although I see you were merging anyway) [02:52] wallyworld: what is it? [02:53] thumper: bug 1467690 [02:53] Bug #1467690: inconsistent juju status from cli vs api

[02:53] ha... [02:54] I'm addressing ci blockers first [02:54] i did some diagnostics, should be easy to demo using a test and then fi [02:54] ok [02:54] I"ll make sure it is on the list... [02:54] i might get time today [02:55] if not, tonight [03:08] ... [03:08] [LOG] 0:00.122 ERROR juju.service.systemd invalid conf for service "jujud-machine-99": relative path in ExecStart (C:/Juju/lib/juju/init/jujud-machine-99/exec-start.sh) not valid [03:08] this error message is all sorts of wrong [03:09] log says vivid machine, tests running on windows [03:09] pretty sure ExecStart shouldn't be C:/ ... [03:10] WTF... [03:11] test is weird [03:11] says "Series": "quantal" [03:11] but tools "1.2.3-vivid-amd64" [03:11] talk about confused [03:16] thumper: i can xplain that [03:16] we fake the series most of the time [03:16] and i know that those faked fixtures are reused many times [03:16] this is why I get so cross about version.Version being reused [03:16] this is the same problem [03:17] ugh [03:17] * thumper looks for the code that is obviously screwing up [03:24] * thumper sighs [03:29] thumper: is the build blocked ? [03:30] davecheney: yeah, looking at the blocker now [03:30] someone just marked it invalid [03:30] I actually have a useful stack trace from the failure [03:31] davecheney: someone did for trunk, it really shouldn't be invalid [03:31] I'm looking at 1.24 first [03:32] (╯°□°）╯︵ ┻━┻) [03:32] FAARRRRKKK!!!!!!!!!!!!!!!!!! [03:32] davecheney: EXACTLY what you said before [03:32] version.Current used for the wrong thing [03:32] * thumper wants to stab someone [03:33] systemd support [03:34] * davecheney pats thumper on the back [03:34] * davecheney leaves a little post it "i told u so" [03:35] thumper: the mess may be partly my fault [03:35] or at least [03:35] i was the last person to try to patch over the horror [03:36] if you find a bit of code that snifs aroud the version string, then astudiously overwrites parts of it [03:36] that was my fault [03:36] that was what we needed to do to get ppc64 working for T [03:40] no... [03:40] this is just blatently using version.Current.Series to work out the datadir for the systemd data directory [03:40] which is wrong in so many ways [03:41] works by accident for all juju devs using ubuntu [03:45] trying to find the point to insert the right info [03:47] thumper: the existing debuglog API handler doesn't stop if the API server stops... i've fixed that but am now wondering if that was intentional [03:47] thumper: do you happen to remember? [03:47] oversight [03:47] it should [03:47] good :) [03:55] thumper: the final signficant db-log PR is ready... just doing some manual testing [03:55] kk [03:55] * thumper headdesks [03:56] * thumper raises head high [03:56] WWWHHHYYY!!!!! [03:57] * thumper wonders how much of a hatchet to wield [03:58] thumper: what's broken? [03:59] service detection code [03:59] inappropriate structures passed around because it happens to have a few useful fields [04:01] when the only field you have is a hammer, everything looks like a hammer [04:01] 13:40 < thumper> this is just blatently using version.Current.Series to work out the datadir for the systemd data directory [04:01] ^ this, now you feel my rage [04:01] * thumper nods [04:04] thumper: how can I help ? [04:04] can i fetch you a refreshing clue bat ? [04:05] find whoever added Version.OS and use the clue bat on them [04:05] there is NO compelling reason to have it AFAICS [04:06] * thumper sharpens the hatchet and wades into the code [04:06] * thumper starts with service... [04:06] thumper: I'm sensing a theme for next weeks bug fixing [04:07] * thumper sobs while hacking [04:11] Achievement unlocked! Menno Smits got review request #2000! [04:11] thumper: http://reviews.vapour.ws/r/2000/ [04:16] menn0: well done! today is the day to get lotto, i guess :D [04:28] Bug #1404946 changed: charm-upgrade hangs forever

[05:07] (╯°□°）╯︵ ┻━┻ [05:10] Bug #1467753 opened: cmd/jujud/agent: multiple data races detected [05:17] davecheney: the map ordering problem is in the coreos/systemd repo [05:17] oh [05:17] dear [05:17] hazaah [05:18] github.com/coreos/systemd ? [05:18] aye [05:18] go-systemd [05:18] * thumper hacks and slaches [05:18] * thumper grabs a copy of the serialization code [05:21] thumper: another one: http://reviews.vapour.ws/r/2001/ [05:22] menn0: sorry, been stuck on the ci blocker [05:22] thumper: no worries [05:22] * thumper copies code from the go-systemd package for now [05:22] these can wait [05:28] pfft... [05:29] the tests inside coreos/go-systemd/unit don't actually pass [05:29] and can't pass if you look at the fucking code [05:29] * thumper rages [05:42] anyone http://reviews.vapour.ws/r/2002/ [05:42] this fixes the current ci blocker, which fails on windows and ppc [05:42] * thumper has to go help with dinner now. [07:30] ping, who's on call reviewer tonight ? http://reviews.vapour.ws/r/2003/ [07:37] fwereade: are you happy with horatio's latest uniter related PR? it looks ok at first read but i'm sure i won't pick up any subtle issues http://reviews.vapour.ws/r/1979 [07:38] fwereade: also, i need to talk to you later about idle time - i really want to leave it at 2 seconds otherwise status will be wrong a lot more often that it is right and so far it seems to be working fine in practice [07:38] wallyworld, thanks for the reminder, looking now [07:38] davecheney, me; looking [07:40] davecheney, ship it [07:41] wallyworld, so what I am mainly concerned about is that having an idleness timer that is higher-resolution than the event timer is going to lead to pathological flickering back and forth in certain circumstances [07:41] wallyworld, nothing-to-do-ohwait-look-nothing-to-do-oh-wait-look [07:42] wallyworld, and while the ideal is swift convergence to reliable values [07:42] wallyworld, I would prioritise the reliability over the swiftness [07:44] wallyworld, and as it stands even the best possible timings for relation chatter mean we can't reasonably infer that relation chatter has finished until at *least* 10s have elapsed, if not 15s [07:50] fwereade: do you by any chance know how to turn on juju feature flags within tests? [07:52] dimitern: thanks [07:53] select {} is more like time.Sleep(some massive int) [07:53] but you don't need to import the time package [07:53] davecheney, I see - interesting trick though :) [07:53] maybe it could be written as [07:53] for { runtime.Sched() } [07:53] maybe [07:54] which might have the same result [07:54] but probably not [07:55] so a goroutine using select{} is just not running and it's not scheduled [07:55] * fwereade scratches head at rogpeppe1, I did it once, possibly via SetFlagsFromEnvironment? [07:55] yeah [07:55] rogpeppe1, it seemed a bit janky but doable [07:55] fwereade: finally found it, yes [07:55] fwereade: seems like there should be a SetFlags call really [07:56] davecheney: out of interest, how was the deepCopy function failing? [07:59] davecheney: (i think it was me that suggested that solution and I'm interested to know why it went wrong, and particularly so if it triggered some kind of race condition) [08:01] it didn't appear to be duplicating slices properly [08:01] http://paste.ubuntu.com/11759941/ [08:02] here is the race failure [08:02] davecheney: how is that possible? === rogpeppe1 is now known as rogpeppe [08:03] davecheney: there's no connection between old and new values [08:03] davecheney: as the entire thing gets marshaled and unmarshaled to a byte slice [08:03] i dunno [08:03] but taking it out and doing it by hand fixed the issue [08:03] davecheney: there's much more likelihood of getting it wrong by doing it manually [08:04] davecheney: your manual copy doesn't actually copy as much [08:04] once we have the race's fixed [08:04] we [08:04] we'll have a voting race build which will double check our work [08:04] davecheney: please try to take the time to understand *why* a race is happening rather than papering over the cracks [08:07] davecheney: i don't believe that deepCopy routine was at fault here, and by *not* deep copying, i'm concerned that there might be more potential for race conditions [08:08] davecheney: for example, there are quite a few Member fields which are pointers and are not now being appropriately copied [08:10] fwereade: i have to go to soccer, i'll ping you later, i think we need to talk through the issue [08:10] wallyworld, sgtm, enjoy [08:11] davecheney: it's quite possible that changing the copy changed the timings so the race detector doesn't trigger (that does happen) [08:15] davecheney: i have a feeling that the change that actually fixed the issue was probably your changes on lines 111, 397 and 398 [08:15] davecheney: as that means that the watcher won't trigger initially [08:16] davecheney: which actually breaks the expected watcher semantics, i think [08:31] rogpeppe, I think you're right there, not having looked at the actual CL [08:32] fwereade: i'm just looking at the race, trying to see what's actually going on [08:36] fwereade: ah, i think i understand the issue [09:05] fwereade: standup? [09:05] jam, oops, where does the time go, just a sec [09:23] fwereade, dimitern: an alternative fix for the peergrouper race: https://github.com/juju/juju/pull/2631 [09:25] rogpeppe, assertMembers change LGTM, I trust that the rest is just reverts :) [09:25] fwereade: thanks. yes, that's the case. [09:25] fwereade: the peergrouper package is full of intermittent failures though. i don't remember it being like that before :) [09:26] fwereade: but probably it was all my fault [09:26] rogpeppe, I sort of doubt it actually [09:26] rogpeppe, races and suchlike do seem to get inserted quite a lot during "maintenance" :/ [09:27] fwereade: yes, it's easy to do when the invariants aren't spelled out, which they probably should be better done in this package [09:28] fwereade: did I get you right? create a type in ipaddress.go which implements the three according methods (code moving) and embed it into State? [09:28] fwereade: and to be fair the testing style in worker/peergrouper is quite experimental [09:29] dimitern: i'd appreciate your take on this, as you signed off on the original PR http://reviews.vapour.ws/r/2003/ [09:29] dimitern: oops, http://reviews.vapour.ws/r/2005/ of course [09:30] * rogpeppe tries to avoid getting sucked into Fixing All The Things. [09:41] TheMue, I'd prefer explicit access over embedding, but, yeah [09:41] * fwereade feels rogpeppe's pain [09:45] rogpeppe, looking [09:45] dimitern: thanks [09:50] Fwereade tx for the review I will look at it in depth when I get to something bigger than my phone [09:55] fwereade: ah, ok, I prefer explicit too. better to maintain and no available methods where don't directly can see where they are implemented [10:11] rogpeppe: if you have time today, I'd love a review of deputy, since it's in a relatively final state. Here's a PR that has the full code for review: https://github.com/juju/deputy/pull/1 [10:14] natefinch, the internet tells me it is your birthday today. If true, Happy Birthday :) [10:15] evilnick: the internet is correct, as it always is ;) Thanks! :) [10:16] * natefinch is having leftover cake from father's day for breakfast :D [10:23] Bug #1467873 opened: leadership lost during service teardown [10:38] rogpeppe: thanks for revertin that [10:38] i was just about to do that [10:39] davecheney: np [10:39] davecheney: there are a few other intermittent failures in peergrouper that would be nice to get to the bottom of [10:40] davecheney: i'm also seeing something that looks like a go bug, but i think that's probably a feature of tip only [10:41] ok, i'll try again tomorrow once the revert lands [10:54] natefinch: reviewed [10:57] rogpeppe: awesome, thanks! [11:04] * natefinch just realized that he mocked out os.Exit, and his code is relying on it to terminate the function it's in. [12:11] does anyone know what should be the restrictions on the format of environment names? [12:13] looks like it can't contain / or \, but other than that, i guess anything should be ok [12:37] another random question without much hope of answer: anyone know what proxy.Settings.NoProxy is for? [12:45] rogpeppe1: pretty sure it's used for "no_proxy", as in wget and friends [12:45] axw: ah, i didn't know about that - guess i should've google it :) [12:45] googled [12:46] rogpeppe1: env name shouldn't contain "- [12:47] " either I think [12:47] otherwise tags would be broken? [12:47] axw: ah [12:47] axw: maybe that should be enforced in environs/config then? [12:47] axw: (currently it just checks for / and \) [12:47] rogpeppe1: sorry, thinking of the IDs [12:47] rogpeppe1: which are just UUIDs... never mind [12:47] axw: yeah, not id [12:48] axw: it's just that kind of thing i'm wondering about though [12:48] axw: as i'm just about to automatically generate an environment name, and i don't want to break things [12:53] axw: i'm also trying to see a way forward to being able to call Provider.PrepareForCreateEnvironment on the server side not the client side [12:55] rogpeppe1: what's preventing that? [12:55] axw: logic that gets env vars [12:56] ah [12:57] axw: the real bad apple here is the local provider which does all kinds of shenanigans, running commands etc [12:58] axw: which in a way doesn't matter (who wants to run multi environments locally anyway) but it would be nice to have it working for tests [12:58] s/anyway/anyway?/ [13:02] axw: BTW do you think it's reasonable for someone to be able to specify a specific agent-version setting when creating a new environment in a JES? [13:03] rogpeppe1: I don't see why anyone would want to, but does allowing it (within reason) make something difficult? [13:03] axw: just wondering - it could potentially be awkward i guess, if someone uses an incompatible agent version [13:05] rogpeppe1: ok. I'm not sure, sorry, better to ask thumper [13:05] axw: you don't by any chance know off-hand what OS context the local provider's jujud runs in, do you? [13:05] rogpeppe1: what OS context? not sure what you mean by that [13:06] axw: will it see the same environment variables that the user had when they bootstrapped? [13:06] rogpeppe1: ah. I don't think so - pretty sure we just write an upstart/systemd conf, and env vars won't generally be preserved [13:07] axw: ah, ok so we really do need to do all that stuff locally [13:08] rogpeppe1: what exactly do you want to do server-side? generate the complete env config? [13:09] axw: yes, to the greatest extent possible [13:09] rogpeppe1: and then something could, say, fetch the config into a .jenv file to run? [13:29] Bug #1467374 changed: worker/uniter/filter: ci test failure [13:29] Bug #1456763 opened: TestUnitRemoval fails

[13:47] dimitern: can i have a small review to fix a 1.24.1 ritical? http://reviews.vapour.ws/r/2007/ [13:49] or perrito666 ^^^^ [13:52] wallyworld: reviewed [13:52] perrito666: tyvm [13:53] oh, anastasiamac also did it [13:54] wallyworld: perrito666: I just perused :D it does not really count :)) [13:54] rogpeppe1, axw: we should really document the format of environment names [13:54] wallyworld: I am familiar with the code so mine does count :p [13:55] perrito666: it is a bad bug - affecting a paying customer site [13:55] so it seems, there were upgraded from 1.20.14 === brandon is now known as web [14:02] natefinch: standup [14:49] natefinch: true, but environment names are getting increasingly redundant [14:52] rogpeppe1: not sure I agree with that. you still need to know what environment Juju is talking about, and if you and juju disagree on what it's called, it'll be confusing [14:53] natefinch: an environment can have many names [14:54] natefinch: it's used to tag provider machines, but now we've got ResourceTags for that === rogpeppe1 is now known as rogpeppe [15:00] rogpeppe: just thinking of reading logs etc. [15:00] rogpeppe: I guess if you never have mixed logs, it's not a problem [15:01] natefinch: the UUID is the thing that matters, not the name [15:02] rogpeppe: right, but uuids are not human-friendly [15:02] "wait am I supposed to be looking at environment de305d54-75b4-431b-adb2-eb6b9e546014 or 123e4567-e89b-12d3-a456-426655440000?" [15:03] natefinch: that can easily be dealt with with a tiny amount of tooling [15:03] Bug #1467964 opened: state still serializes external types

[15:03] natefinch: sed "s/$UUID/my-environment/g" [15:05] rogpeppe: exactly the usability issue I was talking about [15:06] natefinch: how is that a usability issue? i don't believe that all logs should be directly human readable necessarily. [15:06] natefinch: and as i said, there is no one global name for an environment except the UUID [15:06] rogpeppe: I bet our users would disagree [15:07] natefinch: with the last fact? [15:07] rogpeppe: with the fact about logs not needing to be directly human readable [15:08] natefinch: lots of logs are in JSON format. that's not very human readable, but it's very useful and eminently toolable [15:09] rogpeppe: and if we don't give an environment a human-readable when it is created, that's our own fault. I'm not saying that name has to be globally unique. Or even unique at all. Just human readable. [15:09] natefinch: i'm not objecting to having a label that can be attached to an environment [15:09] natefinch: but that's quite a different role than the environment name has traditionally played in juju [15:10] natefinch: for example, if it's just a label, you might consider being able to change it [15:10] rogpeppe: yes. And don't get me wrong, I definitely think we need a UUID on every environment to identify it to our code. [15:10] rogpeppe: just saying we should also have some kind of label for the poor sucker reading the logs at 3am [15:12] natefinch: log size is a real problem for us. i'm not sure i'd want us to put the env name *and* the UUID in every log message [15:13] natefinch: mostly you won't want to mix log files between environments anyway [15:14] rogpeppe: yeah, that was my second thought - hopefully they never end up in the same place anyway. [15:15] natefinch: if you *do* mix 'em, just prefix each line with the UUID and provide a trivial tool to relabel according to whatever labels you deem appropriate or just grep. [15:29] ericsnow: you have a review (http://reviews.vapour.ws/r/1963/). nothing really wrong with the patch, but a few suggestions i think are good. [15:29] katco: thanks [15:31] ericsnow: also, do you think bug 1466565 is related to the lxc collision issue? please don't dig into it too much if you don't know. [15:31] Bug #1466565: Upgraded juju to 1.24 dies shortly after starting

[15:32] ericsnow: gah... why did rb screw up my code formatting? [15:32] katco: I was just wondering that :) [15:32] katco: did you indent the block 4 spaces? [15:32] ericsnow: i put it between `'s with newlines [15:32] ericsnow: i did not. is there a way to edit? [15:32] katco: no need to quote; just indent 4 spaces [15:33] katco: I don't think you can edit [15:33] boo [15:33] well, copy/paste to vim i suppose [15:33] Bug #1467973 opened: uploadSuite.TearDownTest Fails

[15:33] katco: just try replying to the comment [15:33] for multiline code formatting, you can do ``` before and after [15:34] katco: sorry, not sure about that bug [15:34] ericsnow: no worries at all [15:35] ericsnow: just thought it might spark something [15:35] natefinch: ty [15:35] ericsnow: updated [15:36] katco: thanks [15:44] ericsnow: it looks like go's code relies solely on pid to get current user. good call [15:44] ericsnow: sorry, meant uid [15:44] katco: np :) [15:45] rogpeppe: about your comment on deputy for StderrLog and StdoutLog being the same... that would require a change to the API, since right now those are functions, which can't be compared, so we'd have to make them interfaces instead... which kind of complicates the package's API. [15:46] natefinch: how's that work for the demo coming? :) [15:47] katco: crap, is that this iteration? ;) [15:48] natefinch: haha :p just a gentle nudge on priorities given we are way over capacity [15:48] katco: yep [16:10] natefinch: no, you wouldn't need to change the API [16:11] natefinch: oh yes, you would... hmm [16:12] natefinch: yeah, i dunno. seems a pity but no obvious solution [16:12] rogpeppe: ok, sort of what I though too, just hoping you'd see something I missed. [16:29] ericsnow: passing thought. what do you think of creating a "proccmd" package under process/context? this way, instead of "NewFooCmd(...)", it is "proccmd.Foo(...)" [16:30] katco: I had considered that before but tabled it [16:30] katco: +0 [16:30] ericsnow: lol [16:31] ericsnow: fair enough [16:31] katco: we won't have a lot of commands, but it might still be a good idea [16:31] ericsnow: we'll pain the bike shed at some point. [16:31] paint [16:31] katco: k [17:37] ericsnow: it's unclear to me how the arguments in the spec translate to the arguments passed into Init(...) for commands [17:38] katco: take a look at the register command (and registeringCommand) [17:39] katco: RegisterCommand maps onto the register command in the spec [17:39] ericsnow: yes, i'm looking at that. however, in the spec there are options that it looks like we're not accounting for? [17:39] katco: like what? [17:40] ericsnow: i.e. spec takes some flags, Init(...) on register only takes name, and info [17:40] ericsnow: --definition, --extend, --override [17:40] katco: those are defined on registeringCommand [17:40] katco: which RegisterCommand embeds [17:40] ericsnow: ah ok [17:41] katco: we did it that way expressly for the furture work on the launch command :) [17:41] ericsnow: :) sorry i didn't see it [17:41] katco: np [17:50] gah, I hate using godeps [17:55] http://reviews.vapour.ws/r/2008/ if anyone's of a mind to === kadams54_ is now known as kadams54-away [19:00] ericsnow: is there a way we could have factored the use of cmd.Context out so the interface for our commands is simpler? [19:01] katco: hadn't thought about it [19:01] ericsnow: i really dislike the chaining of suites [19:01] ericsnow: and it's more difficult to write unit tests when we're relying on the suite chain [19:02] +1 for less chaining [19:03] ericsnow: well anyway, i'm just going to write the tests as register has, but it is causing me some discomfort ;) [19:04] katco: how so? [19:04] ericsnow: there's too much stuffed into parent structs [19:05] katco: that's making it hard to write new tests? [19:05] ericsnow: it's making it very easy for me to write new tests that i don't fully understand [19:05] katco: k [19:09] ericsnow: this is the style of unit test i prefer: https://github.com/juju/juju/blob/master/leadership/leadership_test.go#L95-L108 [19:09] ericsnow: it's very easy to tell where your stubs are coming from and what they're doing === kadams54-away is now known as kadams54_ [19:11] katco: so you prefer creating a new stub in each test? [19:12] ericsnow: i like defining the functionality i'm stubbing out within the test [19:12] ericsnow: which does require a new instance of a stub in each test [19:14] katco: while I think the discussion of what test methods should look like will be valuable, perhaps we should table it for now [19:14] ericsnow: sure, i'm continuing with the style as it is defined, just thought you'd be interested [19:14] ericsnow: https://plus.google.com/+KatherineCoxBuday/posts/7odKtVXgRB1 [19:15]