davecheney | thumper: http://paste.ubuntu.com/11759872/ | 00:40 |
---|---|---|
davecheney | this morning's results | 00:40 |
davecheney | there are three packages with races remaining | 00:40 |
davecheney | 2 of them are going to be involved fixes | 00:40 |
mup | Bug #1467712 opened: cmd/jujud/agent: data race in test <juju-core:New> <https://launchpad.net/bugs/1467712> | 00:54 |
thumper | hmm | 01:04 |
mup | Bug #1467715 opened: worker/peergrouper: data race in package <juju-core:New> <https://launchpad.net/bugs/1467715> | 01:12 |
davecheney | thumper: this one is worse https://bugs.launchpad.net/juju-core/+bug/1467715 | 01:13 |
mup | Bug #1467715: worker/peergrouper: data race in package <juju-core:New> <https://launchpad.net/bugs/1467715> | 01:13 |
thumper | davecheney: what's it doing? | 01:13 |
davecheney | there are lots of races here | 01:18 |
davecheney | it looks like an internal slice has been leaked to the caller | 01:18 |
davecheney | and the caller is sorting it | 01:18 |
natefinch | ouch | 01:19 |
davecheney | _but_ the race only happens when the test fails | 01:19 |
davecheney | and these are failures we've seen in ci | 01:20 |
davecheney | if the test passes, there is no race | 01:20 |
davecheney | \o/ | 01:20 |
thumper | natefinch: happy birthday from the future | 01:20 |
natefinch | thumper: thanks! :) | 01:21 |
natefinch | davecheney: lol | 01:21 |
davecheney | \o/ yes, stop working, go and have a birthday | 01:22 |
natefinch | Any time I don't have any kids in the same room with me, it's a party ;) | 01:26 |
anastasiamac | \o/ | 01:26 |
perrito666 | 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:42 |
natefinch | perrito666: haha, sure :) Thanks :) | 01:43 |
davecheney | thumper: menn0 https://github.com/juju/juju/pull/2624 | 02:09 |
davecheney | just a small one | 02:09 |
wallyworld | thumper: want a bug to fix as part of your bug squad fun and games? | 02:11 |
menn0 | davecheney: looking now... was having lunch | 02:45 |
menn0 | davecheney: Ship It (although I see you were merging anyway) | 02:47 |
thumper | wallyworld: what is it? | 02:52 |
wallyworld | thumper: bug 1467690 | 02:53 |
mup | Bug #1467690: inconsistent juju status from cli vs api <canonical-bootstack> <juju-core:Triaged> <juju-core 1.24:Triaged> <https://launchpad.net/bugs/1467690> | 02:53 |
thumper | ha... | 02:53 |
thumper | I'm addressing ci blockers first | 02:54 |
wallyworld | i did some diagnostics, should be easy to demo using a test and then fi | 02:54 |
wallyworld | ok | 02:54 |
thumper | I"ll make sure it is on the list... | 02:54 |
wallyworld | i might get time today | 02:54 |
wallyworld | if not, tonight | 02:55 |
thumper | ... | 03:08 |
thumper | [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 |
thumper | this error message is all sorts of wrong | 03:08 |
thumper | log says vivid machine, tests running on windows | 03:09 |
thumper | pretty sure ExecStart shouldn't be C:/ ... | 03:09 |
thumper | WTF... | 03:10 |
thumper | test is weird | 03:11 |
thumper | says "Series": "quantal" | 03:11 |
thumper | but tools "1.2.3-vivid-amd64" | 03:11 |
thumper | talk about confused | 03:11 |
davecheney | thumper: i can xplain that | 03:16 |
davecheney | we fake the series most of the time | 03:16 |
davecheney | and i know that those faked fixtures are reused many times | 03:16 |
davecheney | this is why I get so cross about version.Version being reused | 03:16 |
davecheney | this is the same problem | 03:16 |
thumper | ugh | 03:17 |
* thumper looks for the code that is obviously screwing up | 03:17 | |
* thumper sighs | 03:24 | |
davecheney | thumper: is the build blocked ? | 03:29 |
thumper | davecheney: yeah, looking at the blocker now | 03:30 |
davecheney | someone just marked it invalid | 03:30 |
thumper | I actually have a useful stack trace from the failure | 03:30 |
thumper | davecheney: someone did for trunk, it really shouldn't be invalid | 03:31 |
thumper | I'm looking at 1.24 first | 03:31 |
thumper | (╯°□°)╯︵ ┻━┻) | 03:32 |
thumper | FAARRRRKKK!!!!!!!!!!!!!!!!!! | 03:32 |
thumper | davecheney: EXACTLY what you said before | 03:32 |
thumper | version.Current used for the wrong thing | 03:32 |
* thumper wants to stab someone | 03:32 | |
thumper | systemd support | 03:33 |
* davecheney pats thumper on the back | 03:34 | |
* davecheney leaves a little post it "i told u so" | 03:34 | |
davecheney | thumper: the mess may be partly my fault | 03:35 |
davecheney | or at least | 03:35 |
davecheney | i was the last person to try to patch over the horror | 03:35 |
davecheney | if you find a bit of code that snifs aroud the version string, then astudiously overwrites parts of it | 03:36 |
davecheney | that was my fault | 03:36 |
davecheney | that was what we needed to do to get ppc64 working for T | 03:36 |
thumper | no... | 03:40 |
thumper | this is just blatently using version.Current.Series to work out the datadir for the systemd data directory | 03:40 |
thumper | which is wrong in so many ways | 03:40 |
thumper | works by accident for all juju devs using ubuntu | 03:41 |
thumper | trying to find the point to insert the right info | 03:45 |
menn0 | 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 |
menn0 | thumper: do you happen to remember? | 03:47 |
thumper | oversight | 03:47 |
thumper | it should | 03:47 |
menn0 | good :) | 03:47 |
menn0 | thumper: the final signficant db-log PR is ready... just doing some manual testing | 03:55 |
thumper | kk | 03:55 |
* thumper headdesks | 03:55 | |
* thumper raises head high | 03:56 | |
thumper | WWWHHHYYY!!!!! | 03:56 |
* thumper wonders how much of a hatchet to wield | 03:57 | |
menn0 | thumper: what's broken? | 03:58 |
thumper | service detection code | 03:59 |
thumper | inappropriate structures passed around because it happens to have a few useful fields | 03:59 |
davecheney | when the only field you have is a hammer, everything looks like a hammer | 04:01 |
davecheney | 13:40 < thumper> this is just blatently using version.Current.Series to work out the datadir for the systemd data directory | 04:01 |
davecheney | ^ this, now you feel my rage | 04:01 |
* thumper nods | 04:01 | |
davecheney | thumper: how can I help ? | 04:04 |
davecheney | can i fetch you a refreshing clue bat ? | 04:04 |
thumper | find whoever added Version.OS and use the clue bat on them | 04:05 |
thumper | there is NO compelling reason to have it AFAICS | 04:05 |
* thumper sharpens the hatchet and wades into the code | 04:06 | |
* thumper starts with service... | 04:06 | |
davecheney | thumper: I'm sensing a theme for next weeks bug fixing | 04:06 |
* thumper sobs while hacking | 04:07 | |
menn0 | Achievement unlocked! Menno Smits got review request #2000! | 04:11 |
menn0 | thumper: http://reviews.vapour.ws/r/2000/ | 04:11 |
anastasiamac | menn0: well done! today is the day to get lotto, i guess :D | 04:16 |
mup | Bug #1404946 changed: charm-upgrade hangs forever <canonical-bootstack> <upgrade-charm> <juju-core:Expired> <https://launchpad.net/bugs/1404946> | 04:28 |
thumper | (╯°□°)╯︵ ┻━┻ | 05:07 |
mup | Bug #1467753 opened: cmd/jujud/agent: multiple data races detected <juju-core:New> <https://launchpad.net/bugs/1467753> | 05:10 |
thumper | davecheney: the map ordering problem is in the coreos/systemd repo | 05:17 |
davecheney | oh | 05:17 |
davecheney | dear | 05:17 |
thumper | hazaah | 05:17 |
davecheney | github.com/coreos/systemd ? | 05:18 |
thumper | aye | 05:18 |
thumper | go-systemd | 05:18 |
* thumper hacks and slaches | 05:18 | |
* thumper grabs a copy of the serialization code | 05:18 | |
menn0 | thumper: another one: http://reviews.vapour.ws/r/2001/ | 05:21 |
thumper | menn0: sorry, been stuck on the ci blocker | 05:22 |
menn0 | thumper: no worries | 05:22 |
* thumper copies code from the go-systemd package for now | 05:22 | |
menn0 | these can wait | 05:22 |
thumper | pfft... | 05:28 |
thumper | the tests inside coreos/go-systemd/unit don't actually pass | 05:29 |
thumper | and can't pass if you look at the fucking code | 05:29 |
* thumper rages | 05:29 | |
thumper | anyone http://reviews.vapour.ws/r/2002/ | 05:42 |
thumper | this fixes the current ci blocker, which fails on windows and ppc | 05:42 |
* thumper has to go help with dinner now. | 05:42 | |
davecheney | ping, who's on call reviewer tonight ? http://reviews.vapour.ws/r/2003/ | 07:30 |
wallyworld | 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:37 |
wallyworld | 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 |
fwereade | wallyworld, thanks for the reminder, looking now | 07:38 |
dimitern | davecheney, me; looking | 07:38 |
dimitern | davecheney, ship it | 07:40 |
fwereade | 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 |
fwereade | wallyworld, nothing-to-do-ohwait-look-nothing-to-do-oh-wait-look | 07:41 |
fwereade | wallyworld, and while the ideal is swift convergence to reliable values | 07:42 |
fwereade | wallyworld, I would prioritise the reliability over the swiftness | 07:42 |
fwereade | 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:44 |
rogpeppe1 | fwereade: do you by any chance know how to turn on juju feature flags within tests? | 07:50 |
davecheney | dimitern: thanks | 07:52 |
davecheney | select {} is more like time.Sleep(some massive int) | 07:53 |
davecheney | but you don't need to import the time package | 07:53 |
dimitern | davecheney, I see - interesting trick though :) | 07:53 |
davecheney | maybe it could be written as | 07:53 |
davecheney | for { runtime.Sched() } | 07:53 |
davecheney | maybe | 07:53 |
davecheney | which might have the same result | 07:54 |
davecheney | but probably not | 07:54 |
dimitern | 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 | |
davecheney | yeah | 07:55 |
fwereade | rogpeppe1, it seemed a bit janky but doable | 07:55 |
rogpeppe1 | fwereade: finally found it, yes | 07:55 |
rogpeppe1 | fwereade: seems like there should be a SetFlags call really | 07:55 |
rogpeppe1 | davecheney: out of interest, how was the deepCopy function failing? | 07:56 |
rogpeppe1 | 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) | 07:59 |
davecheney | it didn't appear to be duplicating slices properly | 08:01 |
davecheney | http://paste.ubuntu.com/11759941/ | 08:01 |
davecheney | here is the race failure | 08:02 |
rogpeppe1 | davecheney: how is that possible? | 08:02 |
=== rogpeppe1 is now known as rogpeppe | ||
rogpeppe | davecheney: there's no connection between old and new values | 08:03 |
rogpeppe | davecheney: as the entire thing gets marshaled and unmarshaled to a byte slice | 08:03 |
davecheney | i dunno | 08:03 |
davecheney | but taking it out and doing it by hand fixed the issue | 08:03 |
rogpeppe | davecheney: there's much more likelihood of getting it wrong by doing it manually | 08:03 |
rogpeppe | davecheney: your manual copy doesn't actually copy as much | 08:04 |
davecheney | once we have the race's fixed | 08:04 |
davecheney | we | 08:04 |
davecheney | we'll have a voting race build which will double check our work | 08:04 |
rogpeppe | davecheney: please try to take the time to understand *why* a race is happening rather than papering over the cracks | 08:04 |
rogpeppe | 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:07 |
rogpeppe | davecheney: for example, there are quite a few Member fields which are pointers and are not now being appropriately copied | 08:08 |
wallyworld | fwereade: i have to go to soccer, i'll ping you later, i think we need to talk through the issue | 08:10 |
fwereade | wallyworld, sgtm, enjoy | 08:10 |
rogpeppe | davecheney: it's quite possible that changing the copy changed the timings so the race detector doesn't trigger (that does happen) | 08:11 |
rogpeppe | 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 |
rogpeppe | davecheney: as that means that the watcher won't trigger initially | 08:15 |
rogpeppe | davecheney: which actually breaks the expected watcher semantics, i think | 08:16 |
fwereade | rogpeppe, I think you're right there, not having looked at the actual CL | 08:31 |
rogpeppe | fwereade: i'm just looking at the race, trying to see what's actually going on | 08:32 |
rogpeppe | fwereade: ah, i think i understand the issue | 08:36 |
jam | fwereade: standup? | 09:05 |
fwereade | jam, oops, where does the time go, just a sec | 09:05 |
rogpeppe | fwereade, dimitern: an alternative fix for the peergrouper race: https://github.com/juju/juju/pull/2631 | 09:23 |
fwereade | rogpeppe, assertMembers change LGTM, I trust that the rest is just reverts :) | 09:25 |
rogpeppe | fwereade: thanks. yes, that's the case. | 09:25 |
rogpeppe | fwereade: the peergrouper package is full of intermittent failures though. i don't remember it being like that before :) | 09:25 |
rogpeppe | fwereade: but probably it was all my fault | 09:26 |
fwereade | rogpeppe, I sort of doubt it actually | 09:26 |
fwereade | rogpeppe, races and suchlike do seem to get inserted quite a lot during "maintenance" :/ | 09:26 |
rogpeppe | 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:27 |
TheMue | 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 |
rogpeppe | fwereade: and to be fair the testing style in worker/peergrouper is quite experimental | 09:28 |
rogpeppe | dimitern: i'd appreciate your take on this, as you signed off on the original PR http://reviews.vapour.ws/r/2003/ | 09:29 |
rogpeppe | dimitern: oops, http://reviews.vapour.ws/r/2005/ of course | 09:29 |
* rogpeppe tries to avoid getting sucked into Fixing All The Things. | 09:30 | |
fwereade | TheMue, I'd prefer explicit access over embedding, but, yeah | 09:41 |
* fwereade feels rogpeppe's pain | 09:41 | |
dimitern | rogpeppe, looking | 09:45 |
rogpeppe | dimitern: thanks | 09:45 |
perrito666 | Fwereade tx for the review I will look at it in depth when I get to something bigger than my phone | 09:50 |
TheMue | fwereade: ah, ok, I prefer explicit too. better to maintain and no available methods where don't directly can see where they are implemented | 09:55 |
natefinch | 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:11 |
evilnick | natefinch, the internet tells me it is your birthday today. If true, Happy Birthday :) | 10:14 |
natefinch | evilnick: the internet is correct, as it always is ;) Thanks! :) | 10:15 |
* natefinch is having leftover cake from father's day for breakfast :D | 10:16 | |
mup | Bug #1467873 opened: leadership lost during service teardown <juju-core:New> <https://launchpad.net/bugs/1467873> | 10:23 |
davecheney | rogpeppe: thanks for revertin that | 10:38 |
davecheney | i was just about to do that | 10:38 |
rogpeppe | davecheney: np | 10:39 |
rogpeppe | davecheney: there are a few other intermittent failures in peergrouper that would be nice to get to the bottom of | 10:39 |
rogpeppe | davecheney: i'm also seeing something that looks like a go bug, but i think that's probably a feature of tip only | 10:40 |
davecheney | ok, i'll try again tomorrow once the revert lands | 10:41 |
rogpeppe | natefinch: reviewed | 10:54 |
natefinch | rogpeppe: awesome, thanks! | 10:57 |
* natefinch just realized that he mocked out os.Exit, and his code is relying on it to terminate the function it's in. | 11:04 | |
rogpeppe1 | does anyone know what should be the restrictions on the format of environment names? | 12:11 |
rogpeppe1 | looks like it can't contain / or \, but other than that, i guess anything should be ok | 12:13 |
rogpeppe1 | another random question without much hope of answer: anyone know what proxy.Settings.NoProxy is for? | 12:37 |
axw | rogpeppe1: pretty sure it's used for "no_proxy", as in wget and friends | 12:45 |
rogpeppe1 | axw: ah, i didn't know about that - guess i should've google it :) | 12:45 |
rogpeppe1 | googled | 12:45 |
axw | rogpeppe1: env name shouldn't contain "- | 12:46 |
axw | " either I think | 12:47 |
axw | otherwise tags would be broken? | 12:47 |
rogpeppe1 | axw: ah | 12:47 |
rogpeppe1 | axw: maybe that should be enforced in environs/config then? | 12:47 |
rogpeppe1 | axw: (currently it just checks for / and \) | 12:47 |
axw | rogpeppe1: sorry, thinking of the IDs | 12:47 |
axw | rogpeppe1: which are just UUIDs... never mind | 12:47 |
rogpeppe1 | axw: yeah, not id | 12:47 |
rogpeppe1 | axw: it's just that kind of thing i'm wondering about though | 12:48 |
rogpeppe1 | axw: as i'm just about to automatically generate an environment name, and i don't want to break things | 12:48 |
rogpeppe1 | 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:53 |
axw | rogpeppe1: what's preventing that? | 12:55 |
rogpeppe1 | axw: logic that gets env vars | 12:55 |
axw | ah | 12:56 |
rogpeppe1 | axw: the real bad apple here is the local provider which does all kinds of shenanigans, running commands etc | 12:57 |
rogpeppe1 | 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 |
rogpeppe1 | s/anyway/anyway?/ | 12:58 |
rogpeppe1 | 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:02 |
axw | rogpeppe1: I don't see why anyone would want to, but does allowing it (within reason) make something difficult? | 13:03 |
rogpeppe1 | axw: just wondering - it could potentially be awkward i guess, if someone uses an incompatible agent version | 13:03 |
axw | rogpeppe1: ok. I'm not sure, sorry, better to ask thumper | 13:05 |
rogpeppe1 | axw: you don't by any chance know off-hand what OS context the local provider's jujud runs in, do you? | 13:05 |
axw | rogpeppe1: what OS context? not sure what you mean by that | 13:05 |
rogpeppe1 | axw: will it see the same environment variables that the user had when they bootstrapped? | 13:06 |
axw | 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:06 |
rogpeppe1 | axw: ah, ok so we really do need to do all that stuff locally | 13:07 |
axw | rogpeppe1: what exactly do you want to do server-side? generate the complete env config? | 13:08 |
rogpeppe1 | axw: yes, to the greatest extent possible | 13:09 |
axw | rogpeppe1: and then something could, say, fetch the config into a .jenv file to run? | 13:09 |
mup | Bug #1467374 changed: worker/uniter/filter: ci test failure <juju-core:Triaged> <https://launchpad.net/bugs/1467374> | 13:29 |
mup | Bug #1456763 opened: TestUnitRemoval fails <ci> <intermittent-failure> <test-failure> <juju-core:Triaged> <https://launchpad.net/bugs/1456763> | 13:29 |
wallyworld | dimitern: can i have a small review to fix a 1.24.1 ritical? http://reviews.vapour.ws/r/2007/ | 13:47 |
wallyworld | or perrito666 ^^^^ | 13:49 |
perrito666 | wallyworld: reviewed | 13:52 |
wallyworld | perrito666: tyvm | 13:52 |
wallyworld | oh, anastasiamac also did it | 13:53 |
anastasiamac | wallyworld: perrito666: I just perused :D it does not really count :)) | 13:54 |
natefinch | rogpeppe1, axw: we should really document the format of environment names | 13:54 |
perrito666 | wallyworld: I am familiar with the code so mine does count :p | 13:54 |
wallyworld | perrito666: it is a bad bug - affecting a paying customer site | 13:55 |
wallyworld | so it seems, there were upgraded from 1.20.14 | 13:55 |
=== brandon is now known as web | ||
katco | natefinch: standup | 14:02 |
rogpeppe1 | natefinch: true, but environment names are getting increasingly redundant | 14:49 |
natefinch | 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:52 |
rogpeppe1 | natefinch: an environment can have many names | 14:53 |
rogpeppe1 | natefinch: it's used to tag provider machines, but now we've got ResourceTags for that | 14:54 |
=== rogpeppe1 is now known as rogpeppe | ||
natefinch | rogpeppe: just thinking of reading logs etc. | 15:00 |
natefinch | rogpeppe: I guess if you never have mixed logs, it's not a problem | 15:00 |
rogpeppe | natefinch: the UUID is the thing that matters, not the name | 15:01 |
natefinch | rogpeppe: right, but uuids are not human-friendly | 15:02 |
natefinch | "wait am I supposed to be looking at environment de305d54-75b4-431b-adb2-eb6b9e546014 or 123e4567-e89b-12d3-a456-426655440000?" | 15:02 |
rogpeppe | natefinch: that can easily be dealt with with a tiny amount of tooling | 15:03 |
mup | Bug #1467964 opened: state still serializes external types <tech-debt> <juju-core:Triaged> <https://launchpad.net/bugs/1467964> | 15:03 |
rogpeppe | natefinch: sed "s/$UUID/my-environment/g" | 15:03 |
natefinch | rogpeppe: exactly the usability issue I was talking about | 15:05 |
rogpeppe | natefinch: how is that a usability issue? i don't believe that all logs should be directly human readable necessarily. | 15:06 |
rogpeppe | natefinch: and as i said, there is no one global name for an environment except the UUID | 15:06 |
natefinch | rogpeppe: I bet our users would disagree | 15:06 |
rogpeppe | natefinch: with the last fact? | 15:07 |
natefinch | rogpeppe: with the fact about logs not needing to be directly human readable | 15:07 |
rogpeppe | natefinch: lots of logs are in JSON format. that's not very human readable, but it's very useful and eminently toolable | 15:08 |
natefinch | 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 |
rogpeppe | natefinch: i'm not objecting to having a label that can be attached to an environment | 15:09 |
rogpeppe | natefinch: but that's quite a different role than the environment name has traditionally played in juju | 15:09 |
rogpeppe | natefinch: for example, if it's just a label, you might consider being able to change it | 15:10 |
natefinch | 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 |
natefinch | rogpeppe: just saying we should also have some kind of label for the poor sucker reading the logs at 3am | 15:10 |
rogpeppe | 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:12 |
rogpeppe | natefinch: mostly you won't want to mix log files between environments anyway | 15:13 |
natefinch | rogpeppe: yeah, that was my second thought - hopefully they never end up in the same place anyway. | 15:14 |
rogpeppe | 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:15 |
katco | 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 |
ericsnow | katco: thanks | 15:29 |
katco | 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 |
mup | Bug #1466565: Upgraded juju to 1.24 dies shortly after starting <cts> <landscape> <sts> <upgrade-juju> <juju-core:Triaged> <juju-core 1.24:Triaged> <https://launchpad.net/bugs/1466565> | 15:31 |
katco | ericsnow: gah... why did rb screw up my code formatting? | 15:32 |
ericsnow | katco: I was just wondering that :) | 15:32 |
ericsnow | katco: did you indent the block 4 spaces? | 15:32 |
katco | ericsnow: i put it between `'s with newlines | 15:32 |
katco | ericsnow: i did not. is there a way to edit? | 15:32 |
ericsnow | katco: no need to quote; just indent 4 spaces | 15:32 |
ericsnow | katco: I don't think you can edit | 15:33 |
katco | boo | 15:33 |
katco | well, copy/paste to vim i suppose | 15:33 |
mup | Bug #1467973 opened: uploadSuite.TearDownTest Fails <ci> <intermittent-failure> <unit-tests> <juju-core:Incomplete> <juju-core 1.24:Triaged> <https://launchpad.net/bugs/1467973> | 15:33 |
ericsnow | katco: just try replying to the comment | 15:33 |
natefinch | for multiline code formatting, you can do ``` before and after | 15:33 |
ericsnow | katco: sorry, not sure about that bug | 15:34 |
katco | ericsnow: no worries at all | 15:34 |
katco | ericsnow: just thought it might spark something | 15:35 |
katco | natefinch: ty | 15:35 |
katco | ericsnow: updated | 15:35 |
ericsnow | katco: thanks | 15:36 |
katco | ericsnow: it looks like go's code relies solely on pid to get current user. good call | 15:44 |
katco | ericsnow: sorry, meant uid | 15:44 |
ericsnow | katco: np :) | 15:44 |
natefinch | 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:45 |
katco | natefinch: how's that work for the demo coming? :) | 15:46 |
natefinch | katco: crap, is that this iteration? ;) | 15:47 |
katco | natefinch: haha :p just a gentle nudge on priorities given we are way over capacity | 15:48 |
natefinch | katco: yep | 15:48 |
rogpeppe | natefinch: no, you wouldn't need to change the API | 16:10 |
rogpeppe | natefinch: oh yes, you would... hmm | 16:11 |
rogpeppe | natefinch: yeah, i dunno. seems a pity but no obvious solution | 16:12 |
natefinch | rogpeppe: ok, sort of what I though too, just hoping you'd see something I missed. | 16:12 |
katco | 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:29 |
ericsnow | katco: I had considered that before but tabled it | 16:30 |
ericsnow | katco: +0 | 16:30 |
katco | ericsnow: lol | 16:30 |
katco | ericsnow: fair enough | 16:31 |
ericsnow | katco: we won't have a lot of commands, but it might still be a good idea | 16:31 |
katco | ericsnow: we'll pain the bike shed at some point. | 16:31 |
katco | paint | 16:31 |
ericsnow | katco: k | 16:31 |
katco | ericsnow: it's unclear to me how the arguments in the spec translate to the arguments passed into Init(...) for commands | 17:37 |
ericsnow | katco: take a look at the register command (and registeringCommand) | 17:38 |
ericsnow | katco: RegisterCommand maps onto the register command in the spec | 17:39 |
katco | 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 |
ericsnow | katco: like what? | 17:39 |
katco | ericsnow: i.e. spec takes some flags, Init(...) on register only takes name, and info | 17:40 |
katco | ericsnow: --definition, --extend, --override | 17:40 |
ericsnow | katco: those are defined on registeringCommand | 17:40 |
ericsnow | katco: which RegisterCommand embeds | 17:40 |
katco | ericsnow: ah ok | 17:40 |
ericsnow | katco: we did it that way expressly for the furture work on the launch command :) | 17:41 |
katco | ericsnow: :) sorry i didn't see it | 17:41 |
ericsnow | katco: np | 17:41 |
natefinch | gah, I hate using godeps | 17:50 |
fwereade | http://reviews.vapour.ws/r/2008/ if anyone's of a mind to | 17:55 |
=== kadams54_ is now known as kadams54-away | ||
katco | ericsnow: is there a way we could have factored the use of cmd.Context out so the interface for our commands is simpler? | 19:00 |
ericsnow | katco: hadn't thought about it | 19:01 |
katco | ericsnow: i really dislike the chaining of suites | 19:01 |
katco | ericsnow: and it's more difficult to write unit tests when we're relying on the suite chain | 19:01 |
natefinch | +1 for less chaining | 19:02 |
katco | ericsnow: well anyway, i'm just going to write the tests as register has, but it is causing me some discomfort ;) | 19:03 |
ericsnow | katco: how so? | 19:04 |
katco | ericsnow: there's too much stuffed into parent structs | 19:04 |
ericsnow | katco: that's making it hard to write new tests? | 19:05 |
katco | ericsnow: it's making it very easy for me to write new tests that i don't fully understand | 19:05 |
ericsnow | katco: k | 19:05 |
katco | 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 |
katco | ericsnow: it's very easy to tell where your stubs are coming from and what they're doing | 19:09 |
=== kadams54-away is now known as kadams54_ | ||
ericsnow | katco: so you prefer creating a new stub in each test? | 19:11 |
katco | ericsnow: i like defining the functionality i'm stubbing out within the test | 19:12 |
katco | ericsnow: which does require a new instance of a stub in each test | 19:12 |
ericsnow | 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 |
katco | ericsnow: sure, i'm continuing with the style as it is defined, just thought you'd be interested | 19:14 |
katco | ericsnow: https://plus.google.com/+KatherineCoxBuday/posts/7odKtVXgRB1 | 19:14 |
ericsnow | katco: I think we different opinions here but I'd like to get on the same page | 19:15 |
natefinch | haha, I was going to say I agree with katco, but evidently already did back in August ;) | 19:15 |
ericsnow | katco: so I'm glad you've brought it up :) | 19:15 |
katco | natefinch: lol | 19:16 |
katco | ericsnow: it's the perks of being on an awesome team. good discussion :) | 19:16 |
ericsnow | katco: :) | 19:16 |
natefinch | I don't like setuptest and setupsuite because they're not obvious enough. It's easy to be reading a test and not understand how it works, only to find out it relies on stuff 500 lines up the file in SetupTest, but it's magic, so you can't tell from the test. | 19:19 |
* natefinch looks at the setuptest and setupsuite he just wrote and winces. | 19:19 | |
natefinch | ericsnow: I think we made a mistake in putting ProcDetails in the plugin package. I think it should go in juju/charm ...that way it stays in lockstep with charm.Process | 19:22 |
natefinch | ericsnow: as the input and output of the plugin | 19:23 |
ericsnow | natefinch: but charms have nothing to do with ProcDetails | 19:23 |
natefinch | ericsnow: hrmph.... yeah. | 19:24 |
natefinch | ericsnow: I was trying to avoid copying and pasting the code for serialization of ProcDetails | 19:25 |
katco | ericsnow: natefinch: perhaps the notion of logical vs. physical boundaries is applicable here | 19:25 |
ericsnow | natefinch: why do you have to copy-and-paste? | 19:26 |
natefinch | ericsnow: I have to convert the json into a struct that the plugin code can rationalize about | 19:26 |
ericsnow | natefinch: worst-case you have to import github.com/juju/juju/process/plugin | 19:26 |
=== kadams54_ is now known as kadams54-away | ||
natefinch | ericsnow: I can't. That's not version controlled. | 19:26 |
natefinch | ericsnow: anything under github.com/juju/juju can change at any time | 19:27 |
ericsnow | natefinch: hmm | 19:27 |
natefinch | ericsnow: that's why charm.v5 would have worked, because it *is* version controlled | 19:27 |
ericsnow | natefinch: maybe *for now* it would make sense to just keep the plugin in github.com/juju/juju/process/plugin/docker | 19:28 |
ericsnow | natefinch: that would buy use time to sort out the issue | 19:29 |
natefinch | ericsnow: or like katco said, instead of purely logical boundaries, we use a physical boundary for this code | 19:29 |
natefinch | ericsnow: the plugin code already imports charm.v5 for the Process struct, it's not unreasonable to put the ProcDetails struct there... even if it's not strictly part of charm code (it does certainly relate to charms) | 19:30 |
ericsnow | natefinch: also, it's not like there's a lot of structure to what the plugin must serialize, right? | 19:31 |
natefinch | ericsnow: for now, sure :) | 19:31 |
natefinch | ericsnow: we can punt on it for now and I can copy pasta | 19:31 |
ericsnow | natefinch: I mean there shouldn't be much copying | 19:32 |
natefinch | ericsnow: there's not :) | 19:32 |
natefinch | ericsnow, katco: gotta run, birthday time | 19:32 |
ericsnow | natefinch: this does bring up the question of perhaps versioning the plugin serialization format | 19:32 |
katco | natefinch: have fun dude | 19:32 |
katco | natefinch: happy birthday to you and your wife | 19:33 |
ericsnow | natefinch: happy birthday! | 19:33 |
natefinch | ericsnow: we can talk later. I do think versioning the format is a good idea. WE'll have to figure out how to do that | 19:33 |
natefinch | thanks! | 19:33 |
ericsnow | natefinch: I'll add a card | 19:33 |
katco | ericsnow: keep in mind, versioning may be ok to fudge for the demo | 19:33 |
ericsnow | katco: agreed | 19:40 |
=== kadams54 is now known as kadams54-away | ||
katco | ericsnow: a bit confused. is registeringCommand intended to be the base command for all commands? it seems geared towards register specifically? | 20:58 |
ericsnow | katco: it's for register and launch | 20:59 |
katco | ericsnow: // registeringCommand is the base for commands that register a process | 20:59 |
katco | // that has been launched. | 20:59 |
katco | ericsnow: maybe we should update that? it makes it seem like the process has already been launched? | 21:00 |
ericsnow | katco: that's correct | 21:00 |
ericsnow | katco: the launch command launches the proc via the plugin and then registers it | 21:00 |
katco | ericsnow: ah i think i see now. that is intended to be called after launch does it's thing | 21:02 |
ericsnow | katco: for the launch command the Run method will make the call the plugin and then call the register method with the result | 21:03 |
katco | ericsnow: gotcha | 21:03 |
katco | ericsnow: i'm assuming i want to convert from plugin.ProcStatus -> process.Status. is there a method defined for that already? | 21:16 |
ericsnow | katco: actually you don't | 21:16 |
ericsnow | katco: they are two different statuses | 21:16 |
ericsnow | katco: ProcStatus is sent as-is | 21:16 |
katco | ericsnow: plugin.Launch returns a plugin.ProcStatus, register wants a process.Status | 21:17 |
ericsnow | katco: Status is always set to Active | 21:17 |
katco | ericsnow: oh, surprising... | 21:17 |
katco | ericsnow: so we just ignore the actual status from launching the plugin? | 21:17 |
ericsnow | katco: Launch returns a process.Details | 21:17 |
ericsnow | katco: pretty much...it's just informational (we will display it in juju status) | 21:18 |
katco | ericsnow: so if plugin.Launch returns "error, this is absolutely not running", we still pass "StatusActive" to register? | 21:18 |
ericsnow | katco: in that case the command should fail | 21:19 |
ericsnow | katco: but that should be handled via the error return from the plugin | 21:19 |
ericsnow | katco: not the status | 21:19 |
katco | ericsnow: ah gotcha. so if no error is returned, we assume the plugin has done the right thing, and whatever status is displayed is representing some good state? | 21:20 |
ericsnow | katco: yep | 21:20 |
katco | ericsnow: k makes sense now | 21:20 |
ericsnow | katco: oh good | 21:20 |
katco | ericsnow: and i will comment to that effect ;) | 21:21 |
alexisb | thumper, ping | 21:31 |
alexisb | can you join us please | 21:32 |
thumper | coming | 21:32 |
katco | ericsnow: is baseCommand::getInfo().Process the correct place to get the charm.Process? it looks like that may be circular reasoning | 22:04 |
ericsnow | katco: when one of our hook context commands is run the user provides the name of the process | 22:06 |
wallyworld | thumper: menn0: for 1.24.2, just a reminder, don't forget to land the mgo v2 dep change once 1.24 is unlocked | 22:06 |
ericsnow | katco: the base command uses that name to extract that appropriate info from the hook context | 22:07 |
menn0 | wallyworld: will do. thanks. | 22:07 |
ericsnow | katco: after that the info is available through the info field of the base command | 22:07 |
ericsnow | katco: thus you can then get the charm.Process via info.Process | 22:08 |
jw4 | what is required for a change to get into 1.24 at this point? | 22:10 |
jw4 | wallyworld, alexisb ^^ ? | 22:10 |
wallyworld | jw4: what change? | 22:11 |
alexisb | jw4 it needs to be a regression, critical impact | 22:11 |
alexisb | jw4, why? | 22:11 |
jw4 | #eco is saying that bug 1457205 is blocking some critical features in CABS | 22:12 |
mup | Bug #1457205: Subordinate charm Action data not reported by API <actions> <charmers> <subordinate> <juju-core:Triaged by johnweldon4> <https://launchpad.net/bugs/1457205> | 22:12 |
katco | ericsnow: ok, looks like that's done through basecommand::init(...)? | 22:12 |
jw4 | mind you they did not ask me to escalate | 22:12 |
ericsnow | katco: yep | 22:12 |
wallyworld | jw4: alexisb: marco seems to be happy for it to be fixed for 1.25, what's the reason for asking about 1.24? | 22:13 |
alexisb | wallyworld, it is coming up again in actions discussions | 22:14 |
jw4 | wallyworld: arosales just discovered that it's impacting some critical functionality with CABS | 22:14 |
wallyworld | so we could target to 1.24.2 | 22:14 |
jw4 | is there a freeze/cut-off date? | 22:14 |
katco | ericsnow: am i free to make the ProcLaunchCommand ctor signature whatever, or does that conform to some function sig? | 22:14 |
alexisb | jw4, is this a bug you are willing to take? | 22:15 |
wallyworld | jw4: there's some upgrade issues to fix on 1.24.2 so it will be a few days i expect | 22:15 |
jw4 | alexisb: it's assigned to me right now - I was just expecting a more sedentary approach | 22:15 |
jw4 | :) | 22:15 |
wallyworld | by EOW would be good | 22:15 |
alexisb | wallyworld, based our discussion today in the release call I would think the release target date to be 7/3 with a freeze date a earlier in the week | 22:16 |
wallyworld | for 1.24.2? that seems a way off | 22:16 |
jw4 | ... it's closer than it appears in the mirror | 22:16 |
alexisb | well given 1,24,1 is going out tomorrow that seems very reasonable | 22:17 |
alexisb | that is less then 2 weeks | 22:17 |
ericsnow | katco: it has to conform to the signature expected by the registration func; so it keep it the same as NewProcRegistrationCommand | 22:17 |
wallyworld | ok | 22:17 |
katco | ericsnow: i.e. func(HookContext) (*ProcLaunchCommand, error)? | 22:18 |
wallyworld | my hope is we get stuff fixed sooner if possible so we are not under release pressure | 22:18 |
arosales | wallyworld, an issue for anyone wanting to do benchmarking with Juju and use subordinates. We would like to make benchmarking with juju generally available next week | 22:18 |
ericsnow | katco: yep | 22:18 |
jw4 | wallyworld: +1 | 22:18 |
alexisb | wallyworld, agreed | 22:18 |
alexisb | arosales, that could be an issue | 22:18 |
alexisb | depending on how the fix can come about | 22:19 |
alexisb | ie I dont see us really 1.24.2 + fix for 1457205 before eow next week (at the earliest) | 22:19 |
arosales | alexisb, Understood if it can't make next week, but we don't want benchmarking to be crippled for too long after release | 22:19 |
alexisb | ok arosales noted | 22:20 |
arosales | we could cavet in the release notes until 1.25, but the sooner the better so we could remove that cavet | 22:20 |
wallyworld | alexisb: if we aim for EOW this week for 1.24.2 fixes, then we can try for EOW next week for a release as a goal | 22:20 |
arosales | its something we (eco eng) tracking daily on our dev board | 22:20 |
alexisb | wallyworld, yep | 22:20 |
arosales | wallyworld, jw4, alexisb: thanks. Let us know if you need us to test anything | 22:21 |
katco | ericsnow: is that sig defined somewhere? | 22:21 |
wallyworld | arosales: will do. the benchmarking stuff is freaking awesoe | 22:21 |
alexisb | arosales, in the bug we had an agreement for 1.25, so we will do the best we can to get it out in 1.24.2 | 22:21 |
jw4 | arosales: will do | 22:21 |
ericsnow | katco: worker/uniter/runner/factory.go? | 22:21 |
marcoceppi | wallyworld: just wait until next weeks announcment ;) | 22:21 |
arosales | wallyworld, ya the devX team has done some good work there | 22:21 |
alexisb | and yes the benchmarking stuff is freak'n awesome | 22:22 |
wallyworld | marcoceppi: yes, looking forward to it :-) | 22:22 |
ericsnow | katco: look in component/all/processes.go to see how the commands get registered | 22:22 |
arosales | alexisb, understood and thanks for trying to get it in earlier | 22:22 |
alexisb | and marcoceppi I tweeted that "stuff" just for you ;) | 22:22 |
alexisb | and because it is freak'n awesome | 22:22 |
wallyworld | arosales: marcoceppi: in your announcement will you note the limitation with subordinates to folks don't get the breakage when jumping in to try it out? | 22:23 |
marcoceppi | wallyworld: yes, we have a page on the juju docs we're putting up, I'll highlight that limitation there | 22:24 |
katco | ericsnow: this is a little strange to me. i thought component was supposed to be the way features registered themselves? it looks like they're hard coded? | 22:24 |
arosales | marcoceppi, thanks and hopefully we don't have to note it for too long. | 22:25 |
arosales | keep rockin' it juju-core | 22:25 |
wallyworld | +1 :-) | 22:25 |
arosales | and thanks for working on https://bugs.launchpad.net/juju-core/+bug/1466629 | 22:25 |
mup | Bug #1466629: Containers fail to get ip when non-maas dhcp/dns is used <dhcp> <dns> <lxc> <maas> <openstack-installer> <openstack-provider> <ubuntu-engineering> <ubuntu-openstack> <juju-core:Triaged> <https://launchpad.net/bugs/1466629> | 22:26 |
ericsnow | katco: they key phrase there is "register themselves" | 22:26 |
ericsnow | katco: code somewhere has to make the call | 22:26 |
ericsnow | katco: ergo "hard-coded" | 22:26 |
katco | ericsnow: i just pictured it inverted | 22:26 |
katco | ericsnow: features calling into this package to say "here i am" | 22:26 |
ericsnow | katco: component/all is the intersection point | 22:26 |
alexisb | arosales, I will follow-up with dimiter in the morning on https://bugs.launchpad.net/juju-core/+bug/1466629 | 22:27 |
mup | Bug #1466629: Containers fail to get ip when non-maas dhcp/dns is used <dhcp> <dns> <lxc> <maas> <openstack-installer> <openstack-provider> <ubuntu-engineering> <ubuntu-openstack> <juju-core:Triaged> <https://launchpad.net/bugs/1466629> | 22:27 |
katco | ericsnow: this way, we have 1 package that imports the world | 22:27 |
ericsnow | katco: but they have to be imported to trigger that | 22:27 |
alexisb | arosales, but if logs can be provided tha twould be most helpful | 22:27 |
ericsnow | katco: I went way from such import side-effects | 22:27 |
katco | ericsnow: we want it inverted; every feature imports all and says "here i am" | 22:27 |
ericsnow | katco: but something has to import all the components we want | 22:28 |
katco | ericsnow: not if you use the registration pattern | 22:28 |
ericsnow | katco: that's the way we had it before | 22:28 |
ericsnow | katco: mind hopping into moonstone? | 22:29 |
katco | ericsnow: sure | 22:29 |
arosales | alexisb, I'll see if jcastro can reach out to the bug submitter and see if we can get a reproduction | 22:29 |
alexisb | thanks arosales ! | 22:30 |
mup | Bug #1466660 changed: Unable to create hosted environments on EC2 <config> <ec2-provider> <juju-core:Invalid by cherylj> <https://launchpad.net/bugs/1466660> | 22:43 |
mwhudson | davecheney: say um, how heavily do you think cgo on arm64 has been tested? | 22:47 |
wallyworld | anastasiamac: perrito666: axw: i'll be 15 minutes late for standup as i have a clash | 22:56 |
perrito666 | wallyworld: ah, I hate when I have punk rock bands too | 22:57 |
perrito666 | :p | 22:57 |
wallyworld | gawd, dad joke | 22:57 |
perrito666 | that was a terrible joke | 22:57 |
perrito666 | sorry | 22:57 |
wallyworld | so you should be | 22:57 |
perrito666 | axw: ping me when you arrive pls | 22:59 |
alexisb | cherylj, thumper ping | 23:00 |
thumper | coming | 23:01 |
* menn0 likes perrito666's joke #dad | 23:02 | |
axw | perrito666: I have arrived | 23:04 |
perrito666 | axw: that sounded batmanish | 23:07 |
davecheney | thumper: sorry i missed the standup | 23:09 |
davecheney | it's so coldhere, it's hard to get out of bed that early | 23:09 |
thumper | what? down below 15°C? | 23:09 |
davecheney | mwhudson: the heviest tests have probably been the ones that come with the std lib | 23:22 |
davecheney | juju might exercise the glibc bindings a bit | 23:22 |
mwhudson | yeah | 23:22 |
davecheney | but the more escoteric stuff, nope | 23:22 |
mwhudson | looking at the code, i'm a little concerned that the thread local storage used to save g over a cgo call is not, in fact, thread local | 23:22 |
wallyworld | axw: here now | 23:22 |
mwhudson | but i'm not sure how to check | 23:23 |
axw | wallyworld: joining | 23:23 |
mwhudson | davecheney: runtime·clone has the wonderful comment "// TODO: setup TLS." | 23:23 |
axw | perrito666: ^^ | 23:23 |
alexisb | thumper, that video is awesome! | 23:24 |
thumper | :-) | 23:24 |
* axw is intrigued | 23:25 | |
alexisb | o juju core developers are so getting chairs in october | 23:25 |
alexisb | https://www.youtube.com/watch?v=Y9ttBt-4vWo | 23:25 |
menn0 | wallyworld: reviewed your ResourceManager facade branch | 23:34 |
wallyworld | menn0: ty, will look after standup | 23:34 |
menn0 | wallyworld: tl;dr is "ship it" :) | 23:35 |
wallyworld | menn0: \o/ ty | 23:35 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!