/srv/irclogs.ubuntu.com/2015/06/23/#juju-dev.txt

davecheneythumper: http://paste.ubuntu.com/11759872/00:40
davecheneythis morning's results00:40
davecheneythere are three packages with races remaining00:40
davecheney2 of them are going to be involved fixes00:40
mupBug #1467712 opened: cmd/jujud/agent: data race in test <juju-core:New> <https://launchpad.net/bugs/1467712>00:54
thumperhmm01:04
mupBug #1467715 opened: worker/peergrouper: data race in package <juju-core:New> <https://launchpad.net/bugs/1467715>01:12
davecheneythumper: this one is worse https://bugs.launchpad.net/juju-core/+bug/146771501:13
mupBug #1467715: worker/peergrouper: data race in package <juju-core:New> <https://launchpad.net/bugs/1467715>01:13
thumperdavecheney: what's it doing?01:13
davecheneythere are lots of races here01:18
davecheneyit looks like an internal slice has been leaked to the caller01:18
davecheneyand the caller is sorting it01:18
natefinchouch01:19
davecheney_but_ the race only happens when the test fails01:19
davecheneyand these are failures we've seen in ci01:20
davecheneyif the test passes, there is no race01:20
davecheney\o/01:20
thumpernatefinch: happy birthday from the future01:20
natefinchthumper: thanks! :)01:21
natefinchdavecheney: lol01:21
davecheney\o/ yes, stop working, go and have a birthday01:22
natefinchAny time I don't have any kids in the same room with me, it's a party ;)01:26
anastasiamac\o/01:26
perrito666natefinch: 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
natefinchperrito666: haha, sure :)  Thanks :)01:43
davecheneythumper: menn0 https://github.com/juju/juju/pull/262402:09
davecheneyjust a small one02:09
wallyworldthumper: want a bug to fix as part of your bug squad fun and games?02:11
menn0davecheney: looking now... was having lunch02:45
menn0davecheney: Ship It (although I see you were merging anyway)02:47
thumperwallyworld: what is it?02:52
wallyworldthumper: bug 146769002:53
mupBug #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
thumperha...02:53
thumperI'm addressing ci blockers first02:54
wallyworldi did some diagnostics, should be easy to demo using a test and then fi02:54
wallyworldok02:54
thumperI"ll make sure it is on the list...02:54
wallyworldi might get time today02:54
wallyworldif not, tonight02: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 valid03:08
thumperthis error message is all sorts of wrong03:08
thumperlog says vivid machine, tests running on windows03:09
thumperpretty sure ExecStart shouldn't be C:/ ...03:09
thumperWTF...03:10
thumpertest is weird03:11
thumpersays "Series": "quantal"03:11
thumperbut tools "1.2.3-vivid-amd64"03:11
thumpertalk about confused03:11
davecheneythumper: i can xplain that03:16
davecheneywe fake the series most of the time03:16
davecheneyand i know that those faked fixtures are reused many times03:16
davecheneythis is why I get so cross about version.Version being reused03:16
davecheneythis is the same problem03:16
thumperugh03:17
* thumper looks for the code that is obviously screwing up03:17
* thumper sighs03:24
davecheneythumper: is the build blocked ?03:29
thumperdavecheney: yeah, looking at the blocker now03:30
davecheneysomeone just marked it invalid03:30
thumperI actually have a useful stack trace from the failure03:30
thumperdavecheney: someone did for trunk, it really shouldn't be invalid03:31
thumperI'm looking at 1.24 first03:31
thumper (╯°□°)╯︵ ┻━┻)03:32
thumperFAARRRRKKK!!!!!!!!!!!!!!!!!!03:32
thumperdavecheney: EXACTLY what you said before03:32
thumperversion.Current used for the wrong thing03:32
* thumper wants to stab someone03:32
thumpersystemd support03:33
* davecheney pats thumper on the back03:34
* davecheney leaves a little post it "i told u so"03:34
davecheneythumper: the mess may be partly my fault03:35
davecheneyor at least03:35
davecheneyi was the last person to try to patch over the horror03:35
davecheneyif you find a bit of code that snifs aroud the version string, then astudiously overwrites parts of it03:36
davecheneythat was my fault03:36
davecheneythat was what we needed to do to get ppc64 working for T03:36
thumperno...03:40
thumperthis is just blatently using version.Current.Series to work out the datadir for the systemd data directory03:40
thumperwhich is wrong in so many ways03:40
thumperworks by accident for all juju devs using ubuntu03:41
thumpertrying to find the point to insert the right info03:45
menn0thumper: the existing debuglog API handler doesn't stop if the API server stops... i've fixed that but am now wondering if that was intentional03:47
menn0thumper: do you happen to remember?03:47
thumperoversight03:47
thumperit should03:47
menn0good :)03:47
menn0thumper: the final signficant db-log PR is ready... just doing some manual testing03:55
thumperkk03:55
* thumper headdesks03:55
* thumper raises head high03:56
thumperWWWHHHYYY!!!!!03:56
* thumper wonders how much of a hatchet to wield03:57
menn0thumper: what's broken?03:58
thumperservice detection code03:59
thumperinappropriate structures passed around because it happens to have a few useful fields03:59
davecheneywhen the only field you have is a hammer, everything looks like a hammer04:01
davecheney13:40 < thumper> this is just blatently using version.Current.Series to work out the datadir for the systemd data directory04:01
davecheney^ this, now you feel my rage04:01
* thumper nods04:01
davecheneythumper: how can I help ?04:04
davecheneycan i fetch you a refreshing clue bat ?04:04
thumperfind whoever added Version.OS and use the clue bat on them04:05
thumperthere is NO compelling reason to have it AFAICS04:05
* thumper sharpens the hatchet and wades into the code04:06
* thumper starts with service...04:06
davecheneythumper: I'm sensing a theme for next weeks bug fixing04:06
* thumper sobs while hacking04:07
menn0Achievement unlocked! Menno Smits got review request #2000!04:11
menn0thumper: http://reviews.vapour.ws/r/2000/04:11
anastasiamacmenn0: well done! today is the day to get lotto, i guess :D04:16
mupBug #1404946 changed: charm-upgrade hangs forever <canonical-bootstack> <upgrade-charm> <juju-core:Expired> <https://launchpad.net/bugs/1404946>04:28
thumper (╯°□°)╯︵ ┻━┻05:07
mupBug #1467753 opened: cmd/jujud/agent: multiple data races detected <juju-core:New> <https://launchpad.net/bugs/1467753>05:10
thumperdavecheney: the map ordering problem is in the coreos/systemd repo05:17
davecheneyoh05:17
davecheneydear05:17
thumperhazaah05:17
davecheneygithub.com/coreos/systemd ?05:18
thumperaye05:18
thumpergo-systemd05:18
* thumper hacks and slaches05:18
* thumper grabs a copy of the serialization code05:18
menn0thumper: another one: http://reviews.vapour.ws/r/2001/05:21
thumpermenn0: sorry, been stuck on the ci blocker05:22
menn0thumper: no worries05:22
* thumper copies code from the go-systemd package for now05:22
menn0these can wait05:22
thumperpfft...05:28
thumperthe tests inside coreos/go-systemd/unit don't actually pass05:29
thumperand can't pass if you look at the fucking code05:29
* thumper rages 05:29
thumperanyone http://reviews.vapour.ws/r/2002/05:42
thumperthis fixes the current ci blocker, which fails on windows and ppc05:42
* thumper has to go help with dinner now.05:42
davecheneyping, who's on call reviewer tonight ? http://reviews.vapour.ws/r/2003/07:30
wallyworldfwereade: 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/197907:37
wallyworldfwereade: 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 practice07:38
fwereadewallyworld, thanks for the reminder, looking now07:38
dimiterndavecheney, me; looking07:38
dimiterndavecheney, ship it07:40
fwereadewallyworld, 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 circumstances07:41
fwereadewallyworld, nothing-to-do-ohwait-look-nothing-to-do-oh-wait-look07:41
fwereadewallyworld, and while the ideal is swift convergence to reliable values07:42
fwereadewallyworld, I would prioritise the reliability over the swiftness07:42
fwereadewallyworld, 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 15s07:44
rogpeppe1fwereade: do you by any chance know how to turn on juju feature flags within tests?07:50
davecheneydimitern: thanks07:52
davecheneyselect {} is more like time.Sleep(some massive int)07:53
davecheneybut you don't need to import the time package07:53
dimiterndavecheney, I see - interesting trick though :)07:53
davecheneymaybe it could be written as07:53
davecheneyfor { runtime.Sched() }07:53
davecheneymaybe07:53
davecheneywhich might have the same result07:54
davecheneybut probably not07:54
dimiternso a goroutine using select{} is just not running and it's not scheduled07:55
* fwereade scratches head at rogpeppe1, I did it once, possibly via SetFlagsFromEnvironment?07:55
davecheneyyeah07:55
fwereaderogpeppe1, it seemed a bit janky but doable07:55
rogpeppe1fwereade: finally found it, yes07:55
rogpeppe1fwereade: seems like there should be a SetFlags call really07:55
rogpeppe1davecheney: out of interest, how was the deepCopy function failing?07:56
rogpeppe1davecheney: (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
davecheneyit didn't appear to be duplicating slices properly08:01
davecheneyhttp://paste.ubuntu.com/11759941/08:01
davecheneyhere is the race failure08:02
rogpeppe1davecheney: how is that possible?08:02
=== rogpeppe1 is now known as rogpeppe
rogpeppedavecheney: there's no connection between old and new values08:03
rogpeppedavecheney: as the entire thing gets marshaled and unmarshaled to a byte slice08:03
davecheneyi dunno08:03
davecheneybut taking it out and doing it by hand fixed the issue08:03
rogpeppedavecheney: there's much more likelihood of getting it wrong by doing it manually08:03
rogpeppedavecheney: your manual copy doesn't actually copy as much08:04
davecheneyonce we have the race's fixed08:04
davecheneywe08:04
davecheneywe'll have a voting race build which will double check our work08:04
rogpeppedavecheney: please try to take the time to understand *why* a race is happening rather than papering over the cracks08:04
rogpeppedavecheney: 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 conditions08:07
rogpeppedavecheney: for example, there are quite a few Member fields which are pointers and are not now being appropriately copied08:08
wallyworldfwereade: i have to go to soccer, i'll ping you later, i think we need to talk through the issue08:10
fwereadewallyworld, sgtm, enjoy08:10
rogpeppedavecheney: it's quite possible that changing the copy changed the timings so the race detector doesn't trigger (that does happen)08:11
rogpeppedavecheney: i have a feeling that the change that actually fixed the issue was probably your changes on lines 111, 397 and 39808:15
rogpeppedavecheney: as that means that the watcher won't trigger initially08:15
rogpeppedavecheney: which actually breaks the expected watcher semantics, i think08:16
fwereaderogpeppe, I think you're right there, not having looked at the actual CL08:31
rogpeppefwereade: i'm just looking at the race, trying to see what's actually going on08:32
rogpeppefwereade: ah, i think i understand the issue08:36
jamfwereade: standup?09:05
fwereadejam, oops, where does the time go, just a sec09:05
rogpeppefwereade, dimitern: an alternative fix for the peergrouper race: https://github.com/juju/juju/pull/263109:23
fwereaderogpeppe, assertMembers change LGTM, I trust that the rest is just reverts :)09:25
rogpeppefwereade: thanks. yes, that's the case.09:25
rogpeppefwereade: the peergrouper package is full of intermittent failures though. i don't remember it being like that before :)09:25
rogpeppefwereade: but probably it was all my fault09:26
fwereaderogpeppe, I sort of doubt it actually09:26
fwereaderogpeppe, races and suchlike do seem to get inserted quite a lot during "maintenance" :/09:26
rogpeppefwereade: yes, it's easy to do when the invariants aren't spelled out, which they probably should be better done in this package09:27
TheMuefwereade: 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
rogpeppefwereade: and to be fair the testing style in worker/peergrouper is quite experimental09:28
rogpeppedimitern: i'd appreciate your take on this, as you signed off on the original PR http://reviews.vapour.ws/r/2003/09:29
rogpeppedimitern: oops, http://reviews.vapour.ws/r/2005/ of course09:29
* rogpeppe tries to avoid getting sucked into Fixing All The Things.09:30
fwereadeTheMue, I'd prefer explicit access over embedding, but, yeah09:41
* fwereade feels rogpeppe's pain09:41
dimiternrogpeppe, looking09:45
rogpeppedimitern: thanks09:45
perrito666Fwereade tx for the review I will look at it in depth when I get to something bigger than my phone09:50
TheMuefwereade: ah, ok, I prefer explicit too. better to maintain and no available methods where don't directly can see where they are implemented09:55
natefinchrogpeppe: 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/110:11
evilnicknatefinch, the internet tells me it is your birthday today. If true, Happy Birthday :)10:14
natefinchevilnick: the internet is correct, as it always is ;)  Thanks! :)10:15
* natefinch is having leftover cake from father's day for breakfast :D10:16
mupBug #1467873 opened: leadership lost during service teardown <juju-core:New> <https://launchpad.net/bugs/1467873>10:23
davecheneyrogpeppe: thanks for revertin that10:38
davecheneyi was just about to do that10:38
rogpeppedavecheney: np10:39
rogpeppedavecheney: there are a few other intermittent failures in peergrouper that would be nice to get to the bottom of10:39
rogpeppedavecheney: i'm also seeing something that looks like a go bug, but i think that's probably a feature of tip only10:40
davecheneyok, i'll try again tomorrow once the revert lands10:41
rogpeppenatefinch: reviewed10:54
natefinchrogpeppe: 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
rogpeppe1does anyone know what should be the restrictions on the format of environment names?12:11
rogpeppe1looks like it can't contain / or \, but other than that, i guess anything should be ok12:13
rogpeppe1another random question without much hope of answer: anyone know what proxy.Settings.NoProxy is for?12:37
axwrogpeppe1: pretty sure it's used for "no_proxy", as in wget and friends12:45
rogpeppe1axw: ah, i didn't know about that - guess i should've google it :)12:45
rogpeppe1googled12:45
axwrogpeppe1: env name shouldn't contain "-12:46
axw" either I think12:47
axwotherwise tags would be broken?12:47
rogpeppe1axw: ah12:47
rogpeppe1axw: maybe that should be enforced in environs/config then?12:47
rogpeppe1axw: (currently it just checks for / and \)12:47
axwrogpeppe1: sorry, thinking of the IDs12:47
axwrogpeppe1: which are just UUIDs... never mind12:47
rogpeppe1axw: yeah, not id12:47
rogpeppe1axw: it's just that kind of thing i'm wondering about though12:48
rogpeppe1axw: as i'm just about to automatically generate an environment name, and i don't want to break things12:48
rogpeppe1axw: i'm also trying to see a way forward to being able to call Provider.PrepareForCreateEnvironment on the server side not the client side12:53
axwrogpeppe1: what's preventing that?12:55
rogpeppe1axw: logic that gets env vars12:55
axwah12:56
rogpeppe1axw: the real bad apple here is the local provider which does all kinds of shenanigans, running commands etc12:57
rogpeppe1axw: 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 tests12:58
rogpeppe1s/anyway/anyway?/12:58
rogpeppe1axw: 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
axwrogpeppe1: I don't see why anyone would want to, but does allowing it (within reason) make something difficult?13:03
rogpeppe1axw: just wondering - it could potentially be awkward i guess, if someone uses an incompatible agent version13:03
axwrogpeppe1: ok. I'm not sure, sorry, better to ask thumper13:05
rogpeppe1axw: you don't by any chance know off-hand what OS context the local provider's jujud runs in, do you?13:05
axwrogpeppe1: what OS context? not sure what you mean by that13:05
rogpeppe1axw: will it see the same environment variables that the user had when they bootstrapped?13:06
axwrogpeppe1: ah. I don't think so - pretty sure we just write an upstart/systemd conf, and env vars won't generally be preserved13:06
rogpeppe1axw: ah, ok so we really do need to do all that stuff locally13:07
axwrogpeppe1: what exactly do you want to do server-side? generate the complete env config?13:08
rogpeppe1axw: yes, to the greatest extent possible13:09
axwrogpeppe1: and then something could, say, fetch the config into a .jenv file to run?13:09
mupBug #1467374 changed: worker/uniter/filter: ci test failure <juju-core:Triaged> <https://launchpad.net/bugs/1467374>13:29
mupBug #1456763 opened: TestUnitRemoval fails <ci> <intermittent-failure> <test-failure> <juju-core:Triaged> <https://launchpad.net/bugs/1456763>13:29
wallyworlddimitern: can i have a small review to fix a 1.24.1 ritical? http://reviews.vapour.ws/r/2007/13:47
wallyworldor perrito666 ^^^^13:49
perrito666wallyworld: reviewed13:52
wallyworldperrito666: tyvm13:52
wallyworldoh, anastasiamac also did it13:53
anastasiamacwallyworld: perrito666: I just perused :D it does not really count :))13:54
natefinchrogpeppe1, axw: we should really document the format of environment names13:54
perrito666wallyworld: I am familiar with the code so mine does count :p13:54
wallyworldperrito666: it is a bad bug - affecting a paying customer site13:55
wallyworldso it seems, there were upgraded from 1.20.1413:55
=== brandon is now known as web
katconatefinch: standup14:02
rogpeppe1natefinch: true, but environment names are getting increasingly redundant14:49
natefinchrogpeppe1: 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 confusing14:52
rogpeppe1natefinch: an environment can have many names14:53
rogpeppe1natefinch: it's used to tag provider machines, but now we've got ResourceTags for that14:54
=== rogpeppe1 is now known as rogpeppe
natefinchrogpeppe: just thinking of reading logs etc.15:00
natefinchrogpeppe: I guess if you never have mixed logs, it's not a problem15:00
rogpeppenatefinch: the UUID is the thing that matters, not the name15:01
natefinchrogpeppe: right, but uuids are not human-friendly15:02
natefinch"wait am I supposed to be looking at environment de305d54-75b4-431b-adb2-eb6b9e546014 or 123e4567-e89b-12d3-a456-426655440000?"15:02
rogpeppenatefinch: that can easily be dealt with with a tiny amount of tooling15:03
mupBug #1467964 opened: state still serializes external types <tech-debt> <juju-core:Triaged> <https://launchpad.net/bugs/1467964>15:03
rogpeppenatefinch: sed "s/$UUID/my-environment/g"15:03
natefinchrogpeppe: exactly the usability issue I was talking about15:05
rogpeppenatefinch: how is that a usability issue? i don't believe that all logs should be directly human readable necessarily.15:06
rogpeppenatefinch: and as i said, there is no one global name for an environment except the UUID15:06
natefinchrogpeppe: I bet our users would disagree15:06
rogpeppenatefinch: with the last fact?15:07
natefinchrogpeppe: with the fact about logs not needing to be directly human readable15:07
rogpeppenatefinch: lots of logs are in JSON format. that's not very human readable, but it's very useful and eminently toolable15:08
natefinchrogpeppe: 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
rogpeppenatefinch: i'm not objecting to having a label that can be attached to an environment15:09
rogpeppenatefinch: but that's quite a different role than the environment name has traditionally played in juju15:09
rogpeppenatefinch: for example, if it's just a label, you might consider being able to change it15:10
natefinchrogpeppe: 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
natefinchrogpeppe: just saying we should also have some kind of label for the poor sucker reading the logs at 3am15:10
rogpeppenatefinch: 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 message15:12
rogpeppenatefinch: mostly you won't want to mix log files between environments anyway15:13
natefinchrogpeppe: yeah, that was my second thought - hopefully they never end up in the same place anyway.15:14
rogpeppenatefinch: 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
katcoericsnow: 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
ericsnowkatco: thanks15:29
katcoericsnow: 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
mupBug #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
katcoericsnow: gah... why did rb screw up my code formatting?15:32
ericsnowkatco: I was just wondering that :)15:32
ericsnowkatco: did you indent the block 4 spaces?15:32
katcoericsnow: i put it between `'s with newlines15:32
katcoericsnow: i did not. is there a way to edit?15:32
ericsnowkatco: no need to quote; just indent 4 spaces15:32
ericsnowkatco: I don't think you can edit15:33
katcoboo15:33
katcowell, copy/paste to vim i suppose15:33
mupBug #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
ericsnowkatco: just try replying to the comment15:33
natefinchfor multiline code formatting, you can do ``` before and after15:33
ericsnowkatco: sorry, not sure about that bug15:34
katcoericsnow: no worries at all15:34
katcoericsnow: just thought it might spark something15:35
katconatefinch: ty15:35
katcoericsnow: updated15:35
ericsnowkatco: thanks15:36
katcoericsnow: it looks like go's code relies solely on pid to get current user. good call15:44
katcoericsnow: sorry, meant uid15:44
ericsnowkatco: np :)15:44
natefinchrogpeppe: 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
katconatefinch: how's that work for the demo coming? :)15:46
natefinchkatco: crap, is that this iteration? ;)15:47
katconatefinch: haha :p just a gentle nudge on priorities given we are way over capacity15:48
natefinchkatco: yep15:48
rogpeppenatefinch: no, you wouldn't need to change the API16:10
rogpeppenatefinch: oh yes, you would... hmm16:11
rogpeppenatefinch: yeah, i dunno. seems a pity but no obvious solution16:12
natefinchrogpeppe: ok, sort of what I though too, just hoping you'd see something I missed.16:12
katcoericsnow: 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
ericsnowkatco: I had considered that before but tabled it16:30
ericsnowkatco: +016:30
katcoericsnow: lol16:30
katcoericsnow: fair enough16:31
ericsnowkatco: we won't have a lot of commands, but it might still be a good idea16:31
katcoericsnow: we'll pain the bike shed at some point.16:31
katcopaint16:31
ericsnowkatco: k16:31
katcoericsnow: it's unclear to me how the arguments in the spec translate to the arguments passed into Init(...) for commands17:37
ericsnowkatco: take a look at the register command (and registeringCommand)17:38
ericsnowkatco: RegisterCommand maps onto the register command in the spec17:39
katcoericsnow: yes, i'm looking at that. however, in the spec there are options that it looks like we're not accounting for?17:39
ericsnowkatco: like what?17:39
katcoericsnow: i.e. spec takes some flags, Init(...) on register only takes name, and info17:40
katcoericsnow: --definition, --extend, --override17:40
ericsnowkatco: those are defined on registeringCommand17:40
ericsnowkatco: which RegisterCommand embeds17:40
katcoericsnow: ah ok17:40
ericsnowkatco: we did it that way expressly for the furture work on the launch command :)17:41
katcoericsnow: :) sorry i didn't see it17:41
ericsnowkatco: np17:41
natefinchgah, I hate using godeps17:50
fwereadehttp://reviews.vapour.ws/r/2008/ if anyone's of a mind to17:55
=== kadams54_ is now known as kadams54-away
katcoericsnow: is there a way we could have factored the use of cmd.Context out so the interface for our commands is simpler?19:00
ericsnowkatco: hadn't thought about it19:01
katcoericsnow: i really dislike the chaining of suites19:01
katcoericsnow: and it's more difficult to write unit tests when we're relying on the suite chain19:01
natefinch+1 for less chaining19:02
katcoericsnow: well anyway, i'm just going to write the tests as register has, but it is causing me some discomfort ;)19:03
ericsnowkatco: how so?19:04
katcoericsnow: there's too much stuffed into parent structs19:04
ericsnowkatco: that's making it hard to write new tests?19:05
katcoericsnow: it's making it very easy for me to write new tests that i don't fully understand19:05
ericsnowkatco: k19:05
katcoericsnow: this is the style of unit test i prefer: https://github.com/juju/juju/blob/master/leadership/leadership_test.go#L95-L10819:09
katcoericsnow: it's very easy to tell where your stubs are coming from and what they're doing19:09
=== kadams54-away is now known as kadams54_
ericsnowkatco: so you prefer creating a new stub in each test?19:11
katcoericsnow: i like defining the functionality i'm stubbing out within the test19:12
katcoericsnow: which does require a new instance of a stub in each test19:12
ericsnowkatco: while I think the discussion of what test methods should look like will be valuable, perhaps we should table it for now19:14
katcoericsnow: sure, i'm continuing with the style as it is defined, just thought you'd be interested19:14
katcoericsnow: https://plus.google.com/+KatherineCoxBuday/posts/7odKtVXgRB119:14
ericsnowkatco: I think we different opinions here but I'd like to get on the same page19:15
natefinchhaha, I was going to say I agree with katco, but evidently already did back in August ;)19:15
ericsnowkatco: so I'm glad you've brought it up :)19:15
katconatefinch: lol19:16
katcoericsnow: it's the perks of being on an awesome team. good discussion :)19:16
ericsnowkatco: :)19:16
natefinchI 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
natefinchericsnow: 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.Process19:22
natefinchericsnow: as the input and output of the plugin19:23
ericsnownatefinch: but charms have nothing to do with ProcDetails19:23
natefinchericsnow: hrmph.... yeah.19:24
natefinchericsnow: I was trying to avoid copying and pasting the code for serialization of ProcDetails19:25
katcoericsnow: natefinch: perhaps the notion of logical vs. physical boundaries is applicable here19:25
ericsnownatefinch: why do you have to copy-and-paste?19:26
natefinchericsnow: I have to convert the json into a struct that the plugin code can rationalize about19:26
ericsnownatefinch: worst-case you have to import github.com/juju/juju/process/plugin19:26
=== kadams54_ is now known as kadams54-away
natefinchericsnow: I can't. That's not version controlled.19:26
natefinchericsnow: anything under github.com/juju/juju can change at any time19:27
ericsnownatefinch: hmm19:27
natefinchericsnow: that's why charm.v5 would have worked, because it *is* version controlled19:27
ericsnownatefinch: maybe *for now* it would make sense to just keep the plugin in github.com/juju/juju/process/plugin/docker19:28
ericsnownatefinch: that would buy use time to sort out the issue19:29
natefinchericsnow: or like katco said, instead of purely logical boundaries, we use a physical boundary for this code19:29
natefinchericsnow: 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
ericsnownatefinch: also, it's not like there's a lot of structure to what the plugin must serialize, right?19:31
natefinchericsnow: for now, sure :)19:31
natefinchericsnow: we can punt on it for now and I can copy pasta19:31
ericsnownatefinch: I mean there shouldn't be much copying19:32
natefinchericsnow: there's not :)19:32
natefinchericsnow, katco: gotta run, birthday time19:32
ericsnownatefinch: this does bring up the question of perhaps versioning the plugin serialization format19:32
katconatefinch: have fun dude19:32
katconatefinch: happy birthday to you and your wife19:33
ericsnownatefinch: happy birthday!19:33
natefinchericsnow: we can talk later.  I do think versioning the format is a good idea. WE'll have to figure out how to do that19:33
natefinchthanks!19:33
ericsnownatefinch: I'll add a card19:33
katcoericsnow: keep in mind, versioning may be ok to fudge for the demo19:33
ericsnowkatco: agreed19:40
=== kadams54 is now known as kadams54-away
katcoericsnow: a bit confused. is registeringCommand intended to be the base command for all commands? it seems geared towards register specifically?20:58
ericsnowkatco: it's for register and launch20:59
katcoericsnow: // registeringCommand is the base for commands that register a process20:59
katco// that has been launched.20:59
katcoericsnow: maybe we should update that? it makes it seem like the process has already been launched?21:00
ericsnowkatco: that's correct21:00
ericsnowkatco: the launch command launches the proc via the plugin and then registers it21:00
katcoericsnow: ah i think i see now. that is intended to be called after launch does it's thing21:02
ericsnowkatco: for the launch command the Run method will make the call the plugin and then call the register method with the result21:03
katcoericsnow: gotcha21:03
katcoericsnow: i'm assuming i want to convert from plugin.ProcStatus -> process.Status. is there a method defined for that already?21:16
ericsnowkatco: actually you don't21:16
ericsnowkatco: they are two different statuses21:16
ericsnowkatco: ProcStatus is sent as-is21:16
katcoericsnow: plugin.Launch returns a plugin.ProcStatus, register wants a process.Status21:17
ericsnowkatco: Status is always set to Active21:17
katcoericsnow: oh, surprising...21:17
katcoericsnow: so we just ignore the actual status from launching the plugin?21:17
ericsnowkatco: Launch returns a process.Details21:17
ericsnowkatco: pretty much...it's just informational (we will display it in juju status)21:18
katcoericsnow: so if plugin.Launch returns "error, this is absolutely not running", we still pass "StatusActive" to register?21:18
ericsnowkatco: in that case the command should fail21:19
ericsnowkatco: but that should be handled via the error return from the plugin21:19
ericsnowkatco: not the status21:19
katcoericsnow: 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
ericsnowkatco: yep21:20
katcoericsnow: k makes sense now21:20
ericsnowkatco: oh good21:20
katcoericsnow: and i will comment to that effect ;)21:21
alexisbthumper, ping21:31
alexisbcan you join us please21:32
thumpercoming21:32
katcoericsnow: is baseCommand::getInfo().Process the correct place to get the charm.Process? it looks like that may be circular reasoning22:04
ericsnowkatco: when one of our hook context commands is run the user provides the name of the process22:06
wallyworldthumper: menn0: for 1.24.2, just a reminder, don't forget to land the mgo v2 dep change once 1.24 is unlocked22:06
ericsnowkatco: the base command uses that name to extract that appropriate info from the hook context22:07
menn0wallyworld: will do. thanks.22:07
ericsnowkatco: after that the info is available through the info field of the base command22:07
ericsnowkatco: thus you can then get the charm.Process via info.Process22:08
jw4what is required for a change to get into 1.24 at this point?22:10
jw4wallyworld, alexisb ^^ ?22:10
wallyworldjw4: what change?22:11
alexisbjw4 it needs to be a regression, critical impact22:11
alexisbjw4, why?22:11
jw4#eco is saying that bug 1457205 is blocking some critical features in CABS22:12
mupBug #1457205: Subordinate charm Action data not reported by API <actions> <charmers> <subordinate> <juju-core:Triaged by johnweldon4> <https://launchpad.net/bugs/1457205>22:12
katcoericsnow: ok, looks like that's done through basecommand::init(...)?22:12
jw4mind you they did not ask me to escalate22:12
ericsnowkatco: yep22:12
wallyworldjw4: 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
alexisbwallyworld, it is coming up again in actions discussions22:14
jw4wallyworld: arosales just discovered that it's impacting some critical functionality with CABS22:14
wallyworldso we could target to 1.24.222:14
jw4is there a freeze/cut-off date?22:14
katcoericsnow: am i free to make the ProcLaunchCommand ctor signature whatever, or does that conform to some function sig?22:14
alexisbjw4, is this a bug you are willing to take?22:15
wallyworldjw4: there's some upgrade issues to fix on 1.24.2 so it will be a few days i expect22:15
jw4alexisb: it's assigned to me right now - I was just expecting a more sedentary approach22:15
jw4:)22:15
wallyworldby EOW would be good22:15
alexisbwallyworld, 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 week22:16
wallyworldfor 1.24.2? that seems a way off22:16
jw4... it's closer than it appears in the mirror22:16
alexisbwell given 1,24,1 is going out tomorrow that seems very reasonable22:17
alexisbthat is less then 2 weeks22:17
ericsnowkatco: it has to conform to the signature expected by the registration func; so it keep it the same as NewProcRegistrationCommand22:17
wallyworldok22:17
katcoericsnow: i.e. func(HookContext) (*ProcLaunchCommand, error)?22:18
wallyworldmy hope is we get stuff fixed sooner if possible so we are not under release pressure22:18
arosaleswallyworld, an issue for anyone wanting to do benchmarking with Juju and use subordinates.  We would like to make benchmarking with juju generally available next week22:18
ericsnowkatco: yep22:18
jw4wallyworld: +122:18
alexisbwallyworld, agreed22:18
alexisbarosales, that could be an issue22:18
alexisbdepending on how the fix can come about22:19
alexisbie I dont see us really 1.24.2 + fix for 1457205 before eow next week (at the earliest)22:19
arosalesalexisb, Understood if it can't make next week, but we don't want benchmarking to be crippled for too long after release22:19
alexisbok arosales noted22:20
arosaleswe could cavet in the release notes until 1.25, but the sooner the better so we could remove that cavet22:20
wallyworldalexisb: 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 goal22:20
arosalesits something we (eco eng) tracking daily on our dev board22:20
alexisbwallyworld, yep22:20
arosaleswallyworld, jw4, alexisb: thanks. Let us know if you need us to test anything22:21
katcoericsnow: is that sig defined somewhere?22:21
wallyworldarosales: will do. the benchmarking stuff is freaking awesoe22:21
alexisbarosales, 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.222:21
jw4arosales: will do22:21
ericsnowkatco: worker/uniter/runner/factory.go?22:21
marcoceppiwallyworld: just wait until next weeks announcment ;)22:21
arosaleswallyworld, ya the devX team has done some good work there22:21
alexisband yes the benchmarking stuff is freak'n awesome22:22
wallyworldmarcoceppi: yes, looking forward to it :-)22:22
ericsnowkatco: look in component/all/processes.go to see how the commands get registered22:22
arosalesalexisb, understood and thanks for trying to get it in earlier22:22
alexisband marcoceppi I tweeted that "stuff" just for you ;)22:22
alexisband because it is freak'n awesome22:22
wallyworldarosales: 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
marcoceppiwallyworld: yes, we have a page on the juju docs we're putting up, I'll highlight that limitation there22:24
katcoericsnow: 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
arosalesmarcoceppi, thanks and hopefully we don't have to note it for too long.22:25
arosaleskeep rockin' it juju-core22:25
wallyworld+1 :-)22:25
arosalesand thanks for working on https://bugs.launchpad.net/juju-core/+bug/146662922:25
mupBug #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
ericsnowkatco: they key phrase there is "register themselves"22:26
ericsnowkatco: code somewhere has to make the call22:26
ericsnowkatco: ergo "hard-coded"22:26
katcoericsnow: i just pictured it inverted22:26
katcoericsnow: features calling into this package to say "here i am"22:26
ericsnowkatco: component/all is the intersection point22:26
alexisbarosales, I will follow-up with dimiter in the morning on https://bugs.launchpad.net/juju-core/+bug/146662922:27
mupBug #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
katcoericsnow: this way, we have 1 package that imports the world22:27
ericsnowkatco: but they have to be imported to trigger that22:27
alexisbarosales, but if logs can be provided tha twould be most helpful22:27
ericsnowkatco: I went way from such import side-effects22:27
katcoericsnow: we want it inverted; every feature imports all and says "here i am"22:27
ericsnowkatco: but something has to import all the components we want22:28
katcoericsnow: not if you use the registration pattern22:28
ericsnowkatco: that's the way we had it before22:28
ericsnowkatco: mind hopping into moonstone?22:29
katcoericsnow: sure22:29
arosalesalexisb, I'll see if jcastro can reach out to the bug submitter and see if we can get a reproduction22:29
alexisbthanks arosales !22:30
mupBug #1466660 changed: Unable to create hosted environments on EC2 <config> <ec2-provider> <juju-core:Invalid by cherylj> <https://launchpad.net/bugs/1466660>22:43
mwhudsondavecheney: say um, how heavily do you think cgo on arm64 has been tested?22:47
wallyworldanastasiamac: perrito666: axw: i'll be 15 minutes late for standup as i have a clash22:56
perrito666wallyworld: ah, I hate when I have punk rock bands too22:57
perrito666:p22:57
wallyworldgawd, dad joke22:57
perrito666that was a terrible joke22:57
perrito666sorry22:57
wallyworldso you should be22:57
perrito666axw: ping me when you arrive pls22:59
alexisbcherylj, thumper ping23:00
thumpercoming23:01
* menn0 likes perrito666's joke #dad23:02
axwperrito666: I have arrived23:04
perrito666axw: that sounded batmanish23:07
davecheneythumper: sorry i missed the standup23:09
davecheneyit's so coldhere, it's hard to get out of bed that early23:09
thumperwhat? down below 15°C?23:09
davecheneymwhudson: the heviest tests have probably been the ones that come with the std lib23:22
davecheneyjuju might exercise the glibc bindings a bit23:22
mwhudsonyeah23:22
davecheneybut the more escoteric stuff, nope23:22
mwhudsonlooking 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 local23:22
wallyworldaxw: here now23:22
mwhudsonbut i'm not sure how to check23:23
axwwallyworld: joining23:23
mwhudsondavecheney: runtime·clone has the wonderful comment "// TODO: setup TLS."23:23
axwperrito666: ^^23:23
alexisbthumper, that video is awesome!23:24
thumper:-)23:24
* axw is intrigued23:25
alexisbo juju core developers are so getting chairs in october23:25
alexisbhttps://www.youtube.com/watch?v=Y9ttBt-4vWo23:25
menn0wallyworld: reviewed your ResourceManager facade branch23:34
wallyworldmenn0: ty, will look after standup23:34
menn0wallyworld: tl;dr is "ship it" :)23:35
wallyworldmenn0: \o/ ty23:35

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