/srv/irclogs.ubuntu.com/2015/11/04/#juju-dev.txt

davecheneymgz_: thanks for your help00:16
davecheneythis is working a treat00:16
mgz_davecheney: ace00:16
davecheneya few small issues and i'll be able to land this branch00:16
davecheneythumper: a pint of grog for mgz_ !00:16
thumperawesome00:21
thumperhmm....00:43
thumperI'm seeing state/leadership package sometimes timeout after 20 minutes00:43
* thumper smells race00:43
thumperbut my --race doesn't work00:43
mwhudsonit's totally possible to have logical races without memory races00:59
thumpersure01:03
axwthumper: why discard that PR about server->controller? is that rename not happening anymore?01:57
thumperaxw: it is, but I'm going to retarget it against the new 'controller-rename' feature branch01:57
thumperas I feel that I'm going to break CI01:57
axwthumper: okey dokey01:57
thumperespecially as I change some of the command stuff01:57
thumperand remove the feature falg01:58
thumperso I thought I retarget it all01:58
thumperhopefully a very short lived feature branch01:58
thumperas conflicts against master are going to be a biatch01:58
axwthumper: did you see my latest email about multi-env vs. azure?01:59
thumperI think so02:00
* thumper goes to look again02:00
axwthumper: did not get a reply. just curious about how we're supposed to do create/destroy when an env has resources shared between machines02:01
axwsetup/teardown, a la bootstrap/destroy02:01
thumperlike what?02:01
axwthumper: in the case of azure: a resource group, subnet, network security group, storage account02:01
axwthumper: each env will have its own everything, except for a single virtual network that all envs are connected to02:02
thumperok...02:02
thumperwell, shouldn't we just hook into the create environment, and destroy environment endpoints?02:03
thumperwhen we destroy a controller, we take down all the hosted environments first02:03
thumperaxw: waigani has been refactoring all our destruction code so it is nicer and more async02:03
axwthumper: yeah, but atm EnvironProvider just has "PrepareForCreateEnvironment" - I'm using htis but it doesn't feel quite right02:03
axwthumper: sounds good02:03
thumperyou should use the create environment, not the prepare02:04
axwthumper: there is no "create environment" in EnvironProvider, at least not on master. maybe in a feature branch?02:05
thumperwhere are you looking?02:05
thumperhmm...02:05
axwthumper: https://github.com/juju/juju/blob/master/environs/interface.go02:05
* thumper thinks...02:05
* thumper looks02:05
thumperaxw: actually, there is currently no function we call at all in the provider when creating a new environment02:07
thumperbefore there was never anything to execute02:07
thumperhmm...02:09
axwthumper: so I think we need a CreateEnvironment as well as a PrepareForCreateEnvironment02:09
thumperI think you may be right02:09
* thumper greps for the Prepare...02:09
axwthumper: PrepareForCreateEnvironment is called (twice), and it is usable02:09
thumperhmmm02:09
thumperyeah, checking for valid Config02:10
axwthumper: i.e. I've got multi-env working in azure02:10
thumperaxw: it feels better to have a separate CreateEnvironment call02:11
thumperthat we call just once02:11
axwthumper: +102:11
axwthumper: also, it's not obvious why Prepare is called twice (once before assigning UUID, once after) - would be good to have better docs on that02:12
thumperI believe it is the two step call we do02:12
thumperwhich we are looking to fix02:12
thumperthe first is to ask for a config skeleton02:12
thumperand then the validation when the user passes it back02:13
thumperbut to be honest, I'm not entirely clear02:13
thumperwe are wanting to make the juju cli do the same as jem02:13
thumperand have all providers supply the configschema02:13
thumperoh...02:13
thumpercan you make the new azure provider supply the configschema?02:13
thumpernot sure if the old one did02:14
axwthumper: it didn't. I'll add it to the list.02:14
thumperaxw: here is that branch retargeted to the controller-rename branch http://reviews.vapour.ws/r/3053/02:37
wallyworldaxw: small one if you have a moment https://github.com/juju/charmrepo/pull/3602:54
=== natefinch-afk is now known as natefinch
menn0thumper: http://reviews.vapour.ws/r/3054/03:44
menn0thumper: adds "no tail" support to state.LogTailer03:44
* thumper looks03:44
thumpermenn0: why do we need to call tailer.Stop when we said "don't tail" ?03:48
menn0thumper: you don't ...03:50
menn0thumper: it's just in case the tailer isn't working during tests03:51
menn0thumper: like when I wrote the test but NoTail wasn't implemented yet03:51
menn0thumper: happy to reemove it if you think it's confusing03:51
thumperperhaps just a comment would be enough03:51
* thumper sighs03:52
thumperstate/leadership is failing for me again...03:52
menn0thumper: i've responded to the review04:01
axwwallyworld: reviewed04:06
wallyworldty04:06
=== Ursinha-afk_ is now known as Ursinha
=== Ursinha is now known as Guest82331
=== psivaa_ is now known as psivaa
=== Guest82331 is now known as Ursinha
=== Ursinha is now known as ursula1234
=== ursula1234 is now known as Ursinha
thumperwaigani: http://reviews.vapour.ws/r/3056/04:22
thumperwaigani: it is the earlier branch retargetted and many of the missed systems renamed too04:22
mupBug #1454466 changed: Deployment times out waiting for relation convergence - nvp-transport-node in installing state <deploy> <oil> <juju-core:Expired> <juju-deployer:Invalid>04:30
mup<neutron-gateway (Juju Charms Collection):Expired> <nvp-transport-node (Juju Charms Collection):Expired> <https://launchpad.net/bugs/1454466>04:30
mupBug #1454466 opened: Deployment times out waiting for relation convergence - nvp-transport-node in installing state <deploy> <oil> <juju-core:Expired> <juju-deployer:Invalid>04:33
mup<neutron-gateway (Juju Charms Collection):Expired> <nvp-transport-node (Juju Charms Collection):Expired> <https://launchpad.net/bugs/1454466>04:33
mupBug #1454466 changed: Deployment times out waiting for relation convergence - nvp-transport-node in installing state <deploy> <oil> <juju-core:Expired> <juju-deployer:Invalid>04:36
mup<neutron-gateway (Juju Charms Collection):Expired> <nvp-transport-node (Juju Charms Collection):Expired> <https://launchpad.net/bugs/1454466>04:36
waiganithumper: shipit with a question about "controller environment"04:39
davecheneythumper: menn0 axw http://reviews.vapour.ws/r/3057/04:59
axwdavecheney: shipit05:01
menn0axw: that was awfully fast!05:02
davecheneyboom05:02
axwmenn0: awfully easy review :)05:02
davecheney-2700 lines -> ship it!05:02
axwheh05:02
menn0I think axw has a bot that just automatically replies with ship it05:02
axw;)05:02
axwin seriousness, I wish we had one for back/forwardports05:03
axwor a way to just not post to RB05:04
wallyworldaxw: sorry, one more https://github.com/juju/charmrepo/pull/3705:05
axwwallyworld: you should probably use yaml.v1, it's in dependencies.tsv05:08
wallyworldaxw: ah, yeah, thanks05:09
=== bodie__ is now known as bodie_
anastasiamacericsnow: ping?08:11
fwereade_axw, I think you're probably right re manifold-in-different-package, but I'm really reluctant for some reason09:00
fwereade_axw, possibly because so many others are not yet sufficiently separate from their worker packages as to cleanly accommodate that?09:01
frobwaredimitern, dooferlad: standup?10:02
axwfwereade_: sorry, I was away from IRC. I'm of two minds. On the one hand, it's obviously inherently tied to the worker, but on the other, it has a separate role to play (policy over starting the worker, and connecting the worker to other things). I think having it in a separate package just keeps the responsibilities clearer11:10
fwereade_axw, yeah, I think you're right. subpackage is probably not *really* the right place, but close enough for now11:31
voidspaceperrito666: thanks12:16
perrito666voidspace: yw12:18
* perrito666 overslept, rainy day, fresh drapes, impossible not to12:18
frankbancherylj: hi, could you please take a look at http://reviews.vapour.ws/r/3063/ when you have time?12:21
jamfwereade_: I realized late that alexisb moved our meeting time (relative to my tz at least). I need to take the dog out, but will try to be back for the meeting13:26
=== niedbalski_ is now known as niedbalski
alexisbjam, we dropped but I am here if you want to chat post dog walk13:45
Shawn_Hello, I am trying to build the packages from source, how can i download the dependency packages individually instead using the makefile ?13:45
jamalexisb: hiya. I don't have anything specific. You dropped the meeting for today?13:47
alexisbjam, fwereade_ and I met for a moment and then dropped13:47
jamalexisb: you realize you really have nothing to say to eachother without me ? :)13:50
alexisbjam, so very true ;)13:51
cheryljfrankban: did you have any write up on using the new bundle support that can go out into the release notes for 1.26-alpha1?13:58
fwereade_rogpeppe, do you recall why EnvironObserver has a mutex, and replaces its environ, instead of doing a SetConfig? environs are meant to be goroutine-safe...14:11
fwereade_rogpeppe, (I know it's from, what, 20 months ago :))14:12
frankbancherylj: no I don't but I can get it prepared14:19
cheryljfrankban: that would be great.  Just enough to tell people it's there and how they can use it.14:19
frankbancherylj: cool, Makyo ^^^ would you like to prepare something about bundle deployment in core?14:21
frankbancherylj: will you have time to look at the branch today?14:22
rogpeppefwereade_: i'll have a look14:25
fwereade_rogpeppe, cheers14:25
rogpeppefwereade_: even if an Environ itself is goroutine-safe, a worker still doesn't want it changing underneath itself with no warning14:28
cheryljfrankban: yes, I can.  I see you have two ship it reviews already.  Did you still want me to take a look?14:32
frankbancherylj: yes please, I need a review from a core developer14:33
cheryljfrankban: ah, ok.  Will look at it this morning14:33
frankbancherylj: ty!14:34
fwereade_rogpeppe, sorry, why not?14:41
fwereade_rogpeppe, isn't that the point of it being goroutine-safe, that someone updating the config should be no cause for concern?14:42
fwereade_bother14:42
fwereade_rogpeppe, sorry, bad timing before14:47
rogpeppefwereade_: np14:48
fwereade_rogpeppe, what's the problem with the environ config being updated under a worker? I rather thought that was the point of the boroutine-safety?14:48
rogpeppefwereade_: if i'm writing a worker, i might be doing something on the basis of one attribute, but another attribute might change underfoot so it's inconsistent14:49
rogpeppefwereade_: i think it's better if the config is treated as immutable14:49
mupBug #1513084 opened: 1.20 cannot upgrade to 1.26-alpha1: run.socket: no such file or directory <1.20> <ci> <intermittent-failure> <run> <upgrade-juju> <juju-core:Triaged> <https://launchpad.net/bugs/1513084>14:50
fwereade_rogpeppe, that sounds like a problem with the Config method alone then?14:50
fwereade_rogpeppe, (also: if a worker's depending on env config values from the env, not from state, isn't it already confused?)14:51
rogpeppefwereade_: if that's an issue, why are we creating the env at all?14:53
fwereade_rogpeppe, to call methods that do something to the substrate14:53
rogpeppefwereade_: ok, so those methods will depend on env config values in the env, right?14:53
fwereade_rogpeppe, yes, but that's the env's problem, not the client's14:54
rogpeppefwereade_: so should the worker rely on two sources of truth for the config values from the state - the env and another watcher?14:54
rogpeppefwereade_: currently a worker needs only to use the EnvironObserver, which gives it the config values from state (and an environment that can use them)14:56
mupBug #1513096 opened: invalid agent version in environment configuration: "1.26-alpha1" <ci> <deploy> <juju-core:Triaged> <https://launchpad.net/bugs/1513096>14:56
fwereade_rogpeppe, what's the overlap between worker-relevant values and env-relevant values?14:57
rogpeppefwereade_: i don't know. but i wouldn't want to rely on there being none ever14:57
rogpeppefwereade_: having a single place to watch seems like a reasonable thing to me14:58
fwereade_rogpeppe, in practice, what the EnvironObserver is used for is to create an environ, hand it over to instancepoller, and never update it14:58
fwereade_rogpeppe, the instancepoller doesn't remotely care about what wwe're doing in the background, it just wants its instancegetter to work14:58
rogpeppefwereade_: i'm slightly surpised the provisioner worker doesn't use it14:59
fwereade_rogpeppe, what would the point be? if it doesn't update the environ we still have to thread env-watching through every worker14:59
fwereade_rogpeppe, if it simplified its clients, then it'd be great, but as it is there's no reason to switch implementations to use that afaics15:00
rogpeppefwereade_: presumably the provision *does* update its Environ when the environ config changes?15:03
rogpeppefwereade_: ah yes, it does15:03
fwereade_rogpeppe, yeah -- and that one is also sl. different in that it has an interest in actual juju-model config as opposed to just the substrate config15:05
fwereade_rogpeppe, but I'm pretty sure that if it's paying attention to the substrate config it's DIW somewhere15:05
rogpeppefwereade_: "dead in water" "doing it wrong" ?15:06
fwereade_rogpeppe, the latter was my intent :)15:06
rogpeppefwereade_: it feels a bit racy to me15:06
fwereade_rogpeppe, only if the environ itself is, surely?15:08
rogpeppefwereade_: not really.15:08
rogpeppefwereade_: i don't like state changing underfoot15:08
rogpeppefwereade_: i mean, it's *probably* ok15:09
fwereade_rogpeppe, I feel like the biggest worry is that few environs have ever been actually used from multiple goroutines, so their safety is not battle-tested15:10
rogpeppefwereade_: there should be at least one cross-environment test that's designed to trigger the race detector in such cases15:11
fwereade_rogpeppe, sure, but the number of ways to subtly break safety tends to overwhelm any but exhaustive/invasive testing, and I don't think we really have anything sophisticated enough15:13
rogpeppefwereade_: ah, i was being confused by the configObserver - it's only there for testing15:13
rogpeppefwereade_: if a provider doesn't simply use a mutex, it's probably doing it wrong15:13
rogpeppefwereade_: and that simple mutex logic can be easily tested with the race detector15:14
fwereade_rogpeppe, there's another way to approach it that I quite liked, but forget the details of15:14
rogpeppefwereade_: i tend to just make a test like: go setConfig(); go getConfig(); go setConfig(); go getConfig()15:14
rogpeppefwereade_: which will trigger a failure in the race detector if the appropriate mutexes aren't used15:15
fwereade_rogpeppe, right, but that's just 2 methods and still rather luck-dependent15:15
rogpeppefwereade_: true that it's only 2 methods (it should probably be all of 'em), but i don't think it's that luck dependent15:16
fwereade_rogpeppe, I accept it'll probably fail most of the time, but I doubt it'll be 100% :)15:16
rogpeppefwereade_: why wouldn't it be?15:16
fwereade_rogpeppe, it might happen to run them all serially by sheer coincidence, and never happen to flag the issue -- is it smarter than that? I thought it erred in fvaour of false negatives15:18
rogpeppefwereade_: even if it runs them all serially, the race detector will flag it15:18
* fwereade_ scrubs that bad data out of his brain then15:19
rogpeppefwereade_: of course you can get false negatives if the code never actually accesses the memory in question15:19
fwereade_rogpeppe, cool15:19
rogpeppefwereade_: it's a pretty good tool15:19
fwereade_rogpeppe, yeah, but that's not the case here15:19
rogpeppefwereade_: i'm a big fan of race-detector-oriented tests15:19
fwereade_rogpeppe, indeed, I have a deep and abiding love for the race detector15:20
mupBug #1511822 changed: imports github.com/juju/juju/workload/api/internal/client: use of internal package not allowed <blocker> <ci> <regression> <wily> <juju-core 1.25:In Progress by ericsnowcurrently> <https://launchpad.net/bugs/1511822>15:20
mupBug #1513096 changed: invalid agent version in environment configuration: "1.26-alpha1" <ci> <deploy> <juju-core:Triaged> <https://launchpad.net/bugs/1513096>15:20
fwereade_rogpeppe, ok, so, some go-spam-every-method tests would be good in general15:20
rogpeppefwereade_: so presumably your motivation for the initial question was wondering why the instance poller aggregator isn't seeing env config changes?15:26
fwereade_rogpeppe, not so much, actually, I came across that later -- I was more thinking of running a separate EnvironObserver that *could* be shared by the various workers that use environs15:27
rogpeppefwereade_: well, the current one can be in fact15:28
rogpeppefwereade_: you could do the SetConfig locally in each worker as desired15:29
fwereade_rogpeppe, don't think so? there's only one Environ in the observer15:29
rogpeppefwereade_: each worker would have its own Environ15:29
fwereade_rogpeppe, that's what I'm trying to get away from15:30
rogpeppefwereade_: for reasons of efficiency?15:30
fwereade_rogpeppe, more that the env-watching is unnecessary extra complexity in a bunch of different workers15:31
fwereade_rogpeppe, and yeah, there's some efficiency improvement as well15:31
fwereade_rogpeppe, and it opens the door to us being able to actually manager rate limits sanely, instead of having a herd of N environs stampeding to talk to the same endpoint from a single agent15:32
rogpeppefwereade_: the difficulty is that it's all bound up with environ config and that smears a bunch of semi-related attributes together15:32
rogpeppefwereade_: i'm toying with the thought that workers shouldn't ever look at the environ config directly at all15:34
fwereade_rogpeppe, I do agree that env config is a mess; but AFAICS the potential for harm comes from people using the environ config -- ha, yeah15:34
fwereade_rogpeppe, can you think of any legitimate uses for .Config() for a client? :)15:34
rogpeppefwereade_: but instead have the parameters passed in and derived from the config15:34
fwereade_rogpeppe, yeah, absolutely15:34
rogpeppefwereade_: then you'd probably need to restart the worker when config relevant to it changed15:35
fwereade_rogpeppe, we just want to pass in something that knows how to ask for instances, and which isn't all whiny and needy about being updated by the same client who wants instances15:35
fwereade_rogpeppe, still not really understanding why not to SetConfig and be done with it?15:35
fwereade_rogpeppe, in the observer, I mean15:36
rogpeppefwereade_: SetConfig and use chan struct{} ?15:36
fwereade_rogpeppe, no, I don't want the clients to have to care about environ config at all15:36
rogpeppefwereade_: that's ok iff there's no overlap between the attrs used by the worker vs those used by the provider15:37
rogpeppefwereade_: i wonder what it would take to split 'em15:38
fwereade_rogpeppe, well, at least in the instancepoller case, it doesn't care about env config at all15:38
rogpeppefwereade_: i'm more thinking of the provider case actually15:38
fwereade_rogpeppe, in the provisioner case all I can think of is, uh, instance-reaping mode, whatever it's called15:38
rogpeppesorry15:38
rogpeppeprovisioner15:38
rogpeppefwereade_: yeah15:38
fwereade_rogpeppe, which the provisioner shouldn't really know is to do with the env config anyway, it shoudl just be watching/asking for reaping mode15:39
rogpeppefwereade_: one way to be more sure that the workers aren't doing anything untoward might be to make the Environ with Config and SetConfig methods that return an error or panic15:43
fwereade_rogpeppe, mm, yeah, I will poke around in that direction and see what happens15:52
rogpeppefwereade_: that wouldn't be compatible with logic doing type inspection of the Environ, but i would hope that nothing is doing that anyway (and i've always considered it seriously misguided in the places we *are* doing it)15:53
rogpeppes/hope that nothing/hope that nothing in the workers/15:53
fwereade_rogpeppe, yeah15:55
natefinchgc.ErrorMatches is horrible and bad, and anyone who uses it should feel bad.16:04
fwereade_natefinch, gtg out, would still be interested to hear more on this -- I'd say that *overbroad* ErrorMatches~s are the usual problem?16:05
natefinchfwereade_: the only time the *text* of an error message matters, is if it is shown to a user... which 99% of the time it is not16:06
natefinchfwereade_: we use it as a shorthand for "I expect this specific error to be returned"16:06
natefinchfwereade_: which is not what is actually being tested16:07
rogpeppenatefinch: i disagree. i think it's really helpful to see what our errors look like17:11
rogpeppenatefinch: the alternative is often just asserting that the error is non-nil, which isn't very helpful (i have discovered lots of places where we generated a really stupid or malformed error which we never knew about because of that)17:13
natefinchrogpeppe: the problem is that asserting the error message asserts a ton of information you don't actually care about.  Like how certain types serialize into strings, like how many levels of abstraction there are between you and the origin of the error17:15
rogpeppenatefinch: yes, i agree. that can be problematic too. but i think the upside of seeing what your error messages actually look like outweighs that.17:16
rogpeppenatefinch: i often finish an ErrorMatches string with .*17:16
rogpeppenatefinch: i've found the error string thing really helpful recently - i found one error that was mentioning the same URL 3 times in the same error message.17:17
rogpeppenatefinch: crafting decent error messages is important for the user experience17:18
rogpeppenatefinch: and the tests are usually the only place *we* will ever see them17:18
rogpeppenatefinch: if it hadn't been for ErrorMatches, we wouldn't have made the recent fix to the charm error messages ("charm or bundle not found" vs "entity not found") caught when ian was reviewing our juju-core changes in response to a charm dep update17:19
natefinchrogpeppe: sure, there are times when we show errors to users, and in those cases, error text matters.. but we use ErrorMatches *everywhere* in places we know the user will never see the message (logs don't really count).17:21
rogpeppenatefinch: logs definitely count17:22
natefinchrogpeppe: not down to the letter17:22
rogpeppenatefinch: so what would you do? just check that the error isn't nil?17:22
natefinchrogpeppe: no, check what you actually care about, that we detected a specific type of error and failed in an expected way.  Use a custom error type or something.17:23
rogpeppenatefinch: another issue i found recently was where an API test wasn't actually testing what it thought it was - the error was for an entirely different reason.17:23
rogpeppenatefinch: i don't think we should need a custom error type or value for every kind of error we want to test for17:24
natefinchrogpeppe: the thing that brought this up for me was that a test started failing on master because I added a level of indirection, so the error message had an extra section of "while you were doing  foo:"17:24
rogpeppenatefinch: yes, that's definitely the down side17:24
rogpeppenatefinch: it does make for more churm17:24
rogpeppenatefinch: but i still think it's worth it17:24
rogpeppenatefinch: (and i have thought about this quite a bit when making similar changes)17:25
natefinchrogpeppe: and the problem is that no one is actually thinking about what the error message says or means.  For example, to fix that test, I just copied what the new text is from the error and pasted it into the regex.  That's not really making our code or tests better.17:25
natefinchrogpeppe: imagine if we decided to change how tags print out, so now they print out as "tag: foo/0" ... we'd have thousands of broken tests.17:26
rogpeppenatefinch: when you did that, i hope you asked yourself the question "does this error accurately and reasonably represent the error that's being tested for?"17:26
rogpeppenatefinch: yes we would17:27
rogpeppenatefinch: but if we changed the representation of tag.String we'd have lots of other issues too17:27
natefinchrogpeppe: but there would be 1000 failing tests that are failing because they're testing something they don't actually care about.   certainly there are places that care what tag's String() function outputs... but that's not the only tests that would fial.17:29
rogpeppenatefinch: yes, it's a tradeoff17:29
rogpeppenatefinch: but i don't see a better alternative17:30
ericsnowrogpeppe: checking for specific error types (a la os.IsNotExist and errors.IsNotFound)17:30
rogpeppeericsnow: so every different error message should have its own error type?17:31
natefinchrogpeppe: no, but they shouldn't all be effectively bare strings either17:31
ericsnowrogpeppe: arguably one for each equivalence class of error17:31
rogpeppeericsnow: how do you decide that?17:32
ericsnowrogpeppe: through bikeshedding <wink>17:32
rogpeppeericsnow: and adding hundreds of different error types is a serious maintenance burden17:32
ericsnowrogpeppe: unlike hard-coding checks for error strings ;)17:33
rogpeppeericsnow: at least that's only in the tests and easily changed17:33
rogpeppeericsnow: the problem with having error types is that they're a burden on every developer and need to be maintained as part of the API17:33
rogpeppeericsnow: but often a given error might only have meaning within the context of a given implementation17:34
ericsnowrogpeppe: I'd argue that the errors you get *are* part of the API17:34
rogpeppeericsnow: i agree totally17:34
rogpeppeericsnow: and i think that any API should think very hard about the set of error types it exposes17:34
ericsnowrogpeppe: definitely17:34
rogpeppeericsnow: which doesn't mesh well with tests that want to test internal functionality17:35
ericsnowrogpeppe: agreed, though often having to test internal functionality is a code smell17:36
ericsnowrogpeppe: regardless, as you said there are tradeoffs17:37
rogpeppeericsnow: if you're aiming for high coverage, you have to do that in practice17:37
rogpeppeericsnow: (and i think aiming for high test coverage is worthwhile)17:37
ericsnowrogpeppe: I'll leave it to katco to expound the higher plane that is dependency injection :)17:37
ericsnowrogpeppe: and I couldn't agree more about the importance of high test coverage17:38
katcoquick, look over there!17:38
* rogpeppe looks at the pretty birdy17:38
natefinchI'm with roger on internal tests.  I think they greatly increase the confidence you can have in your tests.  They make writing tests immensely faster, and they prevent you from mangling your exported API just to support testing.17:38
katcodoes anyone remember what the environmental var is for tabular status?17:39
rogpeppekatco: maybe JUJU_CLI_VERSION=2 ?17:41
rogpeppekatco: (haven't tried it, just scanned the code)17:42
katcothat works17:42
katcorogpeppe: ty17:42
rogpeppekatco: np17:42
=== ericsnow is now known as ericsnow_afk
mupBug #1513165 opened: Containers registered with MAAS use wrong name <juju-core:Triaged> <https://launchpad.net/bugs/1513165>18:08
mupBug #1513165 changed: Containers registered with MAAS use wrong name <juju-core:Triaged> <https://launchpad.net/bugs/1513165>18:17
mgz_katco: got a sec? bug 151239918:19
mupBug #1512399: ERROR environment destruction failed: destroying storage: listing volumes: Get https://x.x.x.x:8776/v2/<UUID>/volumes/detail: local error: record overflow <amulet> <bug-squad> <openstack> <uosci> <juju-core:Triaged> <https://launchpad.net/bugs/1512399>18:19
mgz_why does the autogenerated cinder stuff have "https://cinder.example.com" in it at all? can't it just have the path?18:20
mupBug #1513165 opened: Containers registered with MAAS use wrong name <juju-core:Triaged> <https://launchpad.net/bugs/1513165>18:20
mgz_issue is the calling code overrides the host, but not the scheme18:20
mgz_so, it breaks if the endpoint is not https18:21
mupBug #1513165 changed: Containers registered with MAAS use wrong name <juju-core:Triaged> <https://launchpad.net/bugs/1513165>18:23
mupBug #1513165 opened: Containers registered with MAAS use wrong name <juju-core:Triaged> <https://launchpad.net/bugs/1513165>18:26
natefinchkatco: "Pull request successfully merged and closed. You’re all set—the natefinch:assign-worker branch can be safely deleted."18:32
alexisbmgz_, cherylj has been looking at lp 151239918:41
mgz_alexisb: it's easy to fix, it's just katco's autogenration code needs changing to take an endpoint rather than do this weird overriding18:41
mgz_wanted to check with her before hacking it18:42
alexisbmgz_, ack18:42
alexisbI will stay out of it, thanks18:42
mgz_alexisb: no worries18:42
katcomgz_: natefinch: sorry was eating lunch18:46
katconatefinch: grats... feel like we should throw a party or something :p18:46
natefinchkatco: right? :)18:46
katcomgz_: i believe axw has already modified the auto-generated code, so further modification is probably ok18:46
mgz_...that's more like not okay then, unless he pull requested back to your repo?18:47
mgz_oh, or you mean he modified the generated file?18:47
katcomgz_: yes i think so18:48
mgz_yeah, seems like that... mehp, that make updating more annoying18:48
mgz_looks trivial18:50
katcomgz_: fwiw he asked first. we were both busy and i think the cinder project made some modifications that broke auto-generation w/o looking into it18:50
perrito666beautiful, this is what mongorestore says when file not found: don't know what to do with file18:50
katcomgz_: but yes, because of the layering approach, it should be very trivial :)18:50
mgz_katco: anyway, the code just needs to build urls in a sane way18:50
katcomgz_: should just have to modify protocol here: https://github.com/go-goose/goose/blob/v1/cinder/client.go#L25-L3318:51
mgz_an endpoint is not just a host, it's a scheme+host+path that you then append a path to18:51
katcomgz_: still, just need to modify that 1 function18:51
katcomgz_: and that's not even auto-generated code. that's goose18:52
mgz_katco: just + req.URL.Scheme = endpoint.Scheme in SetEndpointFn works for the bug as reported, but that code is still wrong18:52
katcomgz_: modify the path in the same place18:52
mgz_the auto gen code should append paths to the endpoint18:52
mgz_and having hardcodes fake urls is ugly.18:53
natefinchkatco, ericsnow_afk, wwitzel3:  the tech debt bug to fix the tests: perrito666: nice error message19:07
natefinchlol wrong copy19:07
natefinchhttp://reviews.vapour.ws/r/3068/19:07
natefinchwwitzel3: why does the package_test.go file need a +build go1.3?19:13
wwitzel3the card said it did?19:13
wwitzel3I foobar'd my go install trying to install 1.2 so I could test the build flags19:14
wwitzel3so I'm playing with that right now, when I threw it at the bot through, it still didn't work, so I missed some19:15
wwitzel3natefinch: feel free to give it a try if you have 1.219:15
natefinchwwitzel3: install go from source, it's easy and easy to switch versions19:15
natefinch(switching versions is just git checkout go1.2 and then make.bash)19:16
* natefinch just switched from 1.4 to 1.2 in like 10 seconds)19:17
natefinchwwitzel3: every file that imports github.com/lxc/lxd is going to have to be +build go1.3 ... and every file that imports anything those files19:21
wwitzel3natefinch: so if the file that imports lxd is go1.3, all the files in the lxd package still have to have it?19:24
natefinchahahahahahahahahahaha ....19:24
natefinchoh shit19:24
natefinchwwitzel3: go 1.2 doesn't have a "go1.3" build tag implemented19:25
natefinchwaity wait... no, that should still work19:25
natefinchnevermind, sorry19:25
=== ericsnow_afk is now known as ericsnow
natefinchscrewing myself up19:25
natefinchwwitzel3: if you do go install ./... it'll independently try to build that package regardless of whether anything else imports it.19:26
natefinchwwitzel3: it's looking like probably all of the provider/lxd files will need // +build go1.319:32
wwitzel3great19:32
natefinchwwitzel3: I have most of the changes on my local machine... haven't run tests which will also need it.19:33
natefinch> Directory: /home/nate/src/github.com/juju/juju/provider/lxd19:35
natefinch> Command: /home/nate/go/bin/go test ./...19:35
natefinch> Output:19:35
natefinch?   github.com/juju/juju/provider/lxd[no test files]19:35
natefinch> Elapsed: 0.131s19:35
natefinch> Result: Success19:35
natefinchSuccess!19:35
ericsnowcherylj: PTAL: http://reviews.vapour.ws/r/3067/19:35
natefinchwwitzel3: running a full compile of test code now, but I think I got everything19:36
natefinchhmm.....19:37
natefinchI bet just having github.com/lxc/lxd in dependencies.tsv is going to be a non-starter19:37
natefinchkatco: ^19:37
natefinchwwitzel3: I'm getting some wacky compile errors in environs/bootstrap19:48
natefincha bunch of these:19:48
natefinch./bootstrap_test.go:79: cannot use env (type *bootstrapEnviron) as type environs.Environ in function argument:19:48
natefinch*bootstrapEnviron does not implement environs.Environ (wrong type for Bootstrap method)19:48
natefinchhave Bootstrap(environs.BootstrapContext, environs.BootstrapParams) (string, string, environs.BootstrapFinalizer, error)19:48
natefinchwant Bootstrap(environs.BootstrapContext, environs.BootstrapParams) (*environs.BootstrapResult, error)19:48
cheryljericsnow: can you give me a quick explanation of what the bug was about?19:49
ericsnowcherylj: Go 1.5 disallows packages named "internal" from being used outside the package tree they are in19:49
cheryljoic19:50
natefinchit's a feature!  ... just not one we intended to be using ;)19:50
natefinchwwitzel3: I pushed a bunch of my changes here : https://github.com/natefinch/juju/tree/lxd-provider-flags19:56
perrito666ericsnow: I thought I should share the hapiness with you http://reports.vapour.ws/releases/326120:04
ericsnowperrito666: nice! :)20:04
perrito666old restore is no more20:04
perrito666and CI is now using the new one which is faster20:04
ericsnowyay20:05
perrito666and, works flawlessly on HA20:05
davecheneycan someone please review this to unblock me20:19
davecheneyhttps://github.com/juju/utils/pull/17020:19
davecheneyit's fairly uncontraversial20:19
mgz_davecheney: well, I disagree20:21
mgz_but fine20:21
mgz_the reason it's stalled is because this bullshit change-it-everywhere-or-nowhere problem20:22
mgz_there's no reason that our deps shouldn't change apart from people want to keep lock-step on them20:22
mgz_which is then... why are they even seperate github projects20:22
mgz_and this is not going to be different when the big-bang v2 change has happened20:23
mgz_people wanting to get a new fix in utils for a stable branch are going to cope20:24
mgz_there's no utils 1.24 or 1.25 at present20:24
davecheneymgz_: yaml.v1/2 is only renferenced from one file in juju/utils20:27
davecheneycalled trivial.go20:27
davecheneywhich contains one function20:27
davecheneyWriteYAML20:27
davecheneyi'm going to move that function back into juju/juju where it is used20:27
davecheneyand end this madness20:27
davecheneyand yes, i agree taht this upgrade everything at once thing is bullshit20:27
mgz_but the project is called utils! lets just stuff everything in there :)20:27
davecheneyi hold that as prima facie evidence that putting version numbers in your import paths is a mistake20:28
davecheneyi agree, creating a package called utils is bad enough20:28
davecheneythen a repository called utils is asking for it20:28
davecheneyat atlassian to try to counteract this we created a jar called 'bucket'20:28
davecheneyso people would feel embaressed about using classes from it20:29
davecheneyit didn't work, we ended up with bucket220:29
natefinchdavecheney: the problem is not version numbers in your import paths, its that we don't care about backwards compatibility in those branches, and we use godeps in addition for some BS busywork on top of it just for kicks20:29
davecheneyobjection!20:30
davecheneythis is an unrelated argument20:30
davecheneyyaml.v1 and yaml.v2 _ARE_ different20:30
davecheneyit says so right in their name20:30
natefinchdavecheney: yes20:30
davecheneythe problem is one we have created for oursleves where we _WANT_ to think of them as the same20:30
davecheneyso there is no argument about backward compatability20:31
davecheneyif you want them to be backwards compativle, they'd have the same major version number20:31
davecheneyaccording to the rules of gopkg.in20:31
natefinchdavecheney: sorry, I was thinking more of our own repos, charm.v5 etc20:31
davecheneydon't even get me started on v6-unstable20:31
natefinchlol20:32
natefinchyes20:32
davecheneyand yes, i have no idea why we use godeps _and_ gopkg.in versioned import paths20:33
davecheneythe fact that we have to do both says that neither of those alone is a workable solutin20:33
natefinchdavecheney: honestly, the problem s till seems to be godeps.  that code doesn't expose yaml objects through its api, and I presume the yaml output is valid whether it's v1 or v2 of goyaml.20:34
mgz_katco: I'm going to murder the autogen-ness of this cinder stuff. The current upstream wadl doesn't make working code, and if we're not syncing we're not gaining anything for the pain of using this.20:34
katcomgz_: that's fine20:34
natefinchdavecheney: so why do we even care if some helper function uses yaml.v2 instead of v1?20:34
davecheneynatefinch: beacuse we have tests in juju that look for a precise string match20:34
mgz_katco: I pr'd some changes I needed to test, diff I get is http://paste.ubuntu.com/1310535920:34
davecheneyv2 changes the string form, and handling of empty keys20:35
natefinchdavecheney: well I think I know where the problem lies :/20:35
davecheneyyou get $0 for pointing out that our tests are fragile20:36
natefinchaww20:36
* natefinch was ranting about gc.ErrorMatches earlier20:36
mgz_natefinch: the problem is not godeps, we have lots of code that depends on how json is handled, and it's part of our api stability contract20:36
mgz_which we're already not good on.20:37
natefinchmgz_: ....and there were tests that were failing because we changed how we handled stuff in this function....?20:38
davecheneynatefinch: in this case, how yaml is rendered to the client20:39
mgz_not in dave's current case, that's just an error message change, but the charm stuff is scary20:39
davecheneythere is also that yaml.v2 changes the handling of fields/map keys with empty values20:40
davecheneywow, worker/meterstatus/state.go:20:41
davecheney47:     return errors.Trace(utils.WriteYaml(f.path, st))20:41
davecheneyoh20:42
davecheneysorry20:42
davecheneythat writeyaml doesn't return yaml20:42
davecheneyit returns an error if it couldn't write20:42
mgz_eheh20:42
waiganireview please: http://reviews.vapour.ws/r/306120:42
* perrito666 ponders buying a paper book after some time20:43
davecheneythumper: wow, utils.WriteYaml has a huge concurrency failbomb20:43
davecheney        prep := path + ".preparing"20:44
davecheney        f, err := os.OpenFile(prep, os.O_WRONLY|os.O_CREATE|os.O_SYNC, 0644)20:44
perrito666davecheney: did you actually read the DK book about go? is it good?20:44
davecheneyperrito666: nope, both my physical and ebook copies are still on order20:45
davecheneybut it has been reviewed by the whole go team20:45
davecheneyso if you were looking for _the_ word on go20:45
davecheneyyou could not do better than that book20:45
* perrito666 sends to sprint hotel20:45
natefinchdavecheney: yeah, that writeyaml function is pretty awful20:48
natefinchdavecheney: we have an atomic writefile somewhere20:48
natefinch(probably in a utils package somewhere ;)20:52
thumpermenn0, waigani: boring rename branch http://reviews.vapour.ws/r/3056/21:01
waiganithumper: swap you: http://reviews.vapour.ws/r/3061/21:02
thumperdavecheney: I bet many of our tests have certain concurrency expectations21:02
thumperwaigani: looking21:02
waiganithumper: woops, I reviewed that one yesterday, but didn't publish it21:02
natefinchwwitzel3: my branch now compiles (including tests) on go 1.221:14
wwitzel3natefinch: nice, we should just use that then?21:15
natefinchwwitzel3: yeah. probably. I'll PR it21:15
wwitzel3natefinch: make the PR and I'll review  :)21:15
natefinchwwitzel3: http://reviews.vapour.ws/r/3070/21:17
natefinchericsnow, katco: super simple review to support go 1.2 in the lxd provider branch ^21:19
thumperwaigani: "controller environment" vs "controller", I do think that it is worthwhile talking about both. Mark wanted the initial environment / state server environment to be called the "controller environment".21:19
thumperI have tried to do that in the places we talked about the other21:19
thumperthe controller itself encompasses the whole thing (in my mind at least)21:20
thumpermeaning the environment, and the machines and bits in that env21:20
thumperwaigani: but I do take your point, and we should perhaps think a bit more on our language21:21
thumperat least grepping for "controller environment" shouldn't be too hard21:21
ericsnownatefinch: if you rebase against my lxd-fix-local-remote branch, you don't need to touch the instance and container/factory packages21:22
natefinchericsnow: is that branch landing soon?21:23
ericsnownatefinch: not before we get the 1.3+ support we need21:25
natefinchericsnow: well, this PR is so we don't need 1.3+ support21:25
natefinch)ish)21:25
ericsnownatefinch: just to trick the merge bot?21:25
ericsnownatefinch: strike that :)21:26
ericsnownatefinch: regardless, feel free to strip all the LXD-as-a-container code now rather than waiting for any of my code to land21:27
katconatefinch: how is this different from wwitzel3 's pr? http://reviews.vapour.ws/r/3064/21:28
waiganithumper: right. It's really about the concept we want to sell. "controller environment vs an environment in a controller" or just "controller vs environment". Either is fine, but we should consciously chose one and be consistent.21:28
natefinchkatco: mine works ;)21:28
wwitzel3yeah21:28
wwitzel3lol21:28
ericsnownatefinch: also, I'm pretty sure we don't import github/lxc/lxd anywhere under provider/lxd so why is the build constraint needed there?21:28
natefinchericsnow: transitive dependencies... since we're not building all the lxdclient code, the code that uses *that* code now won't compile21:29
thumperwaigani: the tricky bit of this concept is that we have a controller that hosts environments, but also the controller environment that is the environment that contains the API server bits21:29
waiganiyep21:29
thumperwaigani: so I think I'm doing to defer changing these comments for now :)21:30
waiganiokay, sure21:30
ericsnownatefinch: meh, I meant solve that a different way but that would be worse than sprinkling the build constraints all over :)21:30
natefinchericsnow: you could make fake implementations of the lxdclient stuff for go1.2... but I think this actually might be ore clear and less real work.  Really didn't take long, and it'll be trivial to undo (even if it does touch a lot of files... it's just one line per file).21:32
ericsnownatefinch: right; not worth it21:32
waiganithumper: But I do think it's worth thinking about. AFAICS, fact that a controller is an environ is implementation detail  to support the API - as you say. It shouldn't be confusing the language we use to describe the overall design.21:36
thumperexcept it shows up whenever we talk about "all environments"21:36
thumperand environment commands work on the controller21:36
thumperI definitely agree that it is worth thinking about21:37
waiganithumper: I think it comes down to implementation details vs design. Are those details leaking into the design? If so, do we separate them (e.g. never speak of a "controller environment", rename env cmds on the controller to controller cmds) or do we update the design (e.g. always talk about "controller environment").21:44
natefinchkatco, ericsnow: I think we need to talk about dependency injection for the purpose of testing, re: http://reviews.vapour.ws/r/3021/#comment1899521:45
thumperI don't think so... I think it depends a lot on context21:45
thumperyou are either talking about the controller environment, and dealing with environment *things*21:45
katconatefinch: it's been queued for awhile. i have half an hour if everyone else does21:45
thumperor you are talking about the controller in a hosting capacity21:45
thumperthe controller environment doesn't host environments21:45
natefinchkatco: I have 15 minutes, which is probably not going to be enough.  Tomorrow?21:46
thumperbut talking about environment things on the controller doesn't make sense really either21:46
katconatefinch: tomorrow is meeting day for me =/21:46
natefinchkatco: I'm willing to sacrifice my 1:1 time to talk about this.  I think it's worthy.21:46
katconatefinch: we can do that if everyone is available21:47
waiganithumper: ah okay. In a similar way you could talk about a controller machine?21:47
thumperI'm not sure you would...21:47
thumperyou may talk about an API server machine21:48
thumperbut a controller machine doesn't really make sense21:48
waiganiso what do we call what we use to call the state server machine?21:48
thumpera machine in the controller environment is ok...21:48
ericsnowkatco, natefinch, wwitzel3: I'm game21:48
thumperpersonally I tend to still call them state server machines21:49
katcoericsnow: natefinch: wwitzel3: invite in the mail21:49
thumper:)21:49
waiganihaha21:49
natefinchthumper, waigani: FWIW, I think that "talking out loud" about the new names for things is a very good way to nail down gaps in the vocabulary, and exposes places where names may be misleading.21:49
natefinchlike state server machine :)21:50
thumperagreed21:50
thumperdefinitely think it needs a new name :)21:50
thumperlike: Bob21:50
waiganilol21:50
natefinch+1 ship it21:50
thumperI used to use Eric all the time, from Eric the Viking21:50
thumperbut I feel I have to stop that now that we have ericsnow21:50
natefinchway to mess it up ericsnow21:51
ericsnowmwahaha21:51
thumperericsnow: how about you change your name?21:51
thumperperhaps "snowman"21:51
=== ericsnow is now known as bobsnow
=== bobsnow is now known as ericsnow
cheryljericsnow: for this branch:  http://reviews.vapour.ws/r/3067/  Did you just do a rename for internal to private?21:51
ericsnownah :)21:51
ericsnowcherylj: yep, that's it21:51
natefinchsnowman sorta sounds like the codename for a mobster21:53
cheryljericsnow: it's just showing up in a not-clear way in RB and github.  Like it shows that you modified something in internal/client/unitfacade.go, but it doesn't show that you deleted any of the old files21:53
thumpernatefinch: haha21:53
ericsnowcherylj: yeah, GH is confused and RB isn't helpful when it comes to renames21:54
natefinchI heard he snowed someone just for looking at him wrong.21:54
cheryljbleh.  fun.21:54
=== natefinch is now known as natefinch-af
=== natefinch-af is now known as natefinch-afk
ericsnowcherylj: in case it helps, I rebased that PR so GH is showing the changes correctly now22:08
cheryljah yeah, that helps22:10
cheryljericsnow: there were just a couple things22:10
ericsnowcherylj: thanks22:10
mupBug #1513236 opened: Cannot build trusty armhf with go1.2 on from master <armhf> <blocker> <go1.2> <regression> <juju-core:Triaged> <https://launchpad.net/bugs/1513236>22:12
thumperwho loves shitty rename branches? http://reviews.vapour.ws/r/3072/diff/#22:25
ericsnowkatco: FYI, I have the payloads-into-master patch up22:26
* ericsnow raises hand22:29
katcoericsnow: nice :)22:30
katcoericsnow: go ahead and merge your payloads branch into master22:48
ericsnowkatco: k22:48
ericsnowkatco: alas, master is blocked23:01
katcoericsnow: doh23:01
davecheneykatco: ericsnow this may very well be an upstream bug23:10
davecheneylooking at the ci dashboard23:10
ericsnowdavecheney: k23:10
davecheneyno builds for the crypto repo have run on arm for months23:10
davecheneyand the go team turned off build faiure notifications a while back23:10
davecheneyso yeah, there is that23:11
davecheneylet me quickly check something23:11
ericsnowdavecheney: thanks23:11
davecheneybuilds on go 1.523:12
davecheneywe might have to chalk this one up to go 1.2 being unsupported23:12
davecheneykatco: please unblock the build23:12
davecheneywe cannot fix this23:12
davecheneyit is blocked on other work which has been scheduled23:12
davecheneybut has not reasonable ETA that can be used to unblock the build23:13

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