[04:06] thumper: Afternoon. Are you using any features from nose in jujulib? [04:06] no, just running tests [04:07] huwshimi: as long as "make check" works, I'm happy [04:07] thumper: Ah cool, I have some comments on my PR about just using pytest, so I'll fix that up if that works for you? [04:07] kk [04:07] thumper: Cool, cheers mate. [06:40] Bug #1484993 changed: TestBootstrapReusesAffinityGroupAndVNet: no tools are available for your environment

[06:44] fwereade, ping [06:44] dimitern, pong [06:45] fwereade, I'd like to ask you about a quirk with our environ constraints implementation that bit me badly [06:46] fwereade, we have Tags *[]string ",omitempty" and the doc comment says if they're nil they're not set, but if they are an empty slice, they are replaced [06:46] dimitern, ok [06:47] fwereade, this is a lie - esp. for env constraints, as they use a single doc, so there's *no way* to set tags= once there's a value on it [06:47] fwereade, the same applies to Networks and Spaces (i'm now adding) [06:48] fwereade, so my question is - is it a doc issue or by design you can't unset a list-style constraint once set on an environment [06:49] dimitern, that just sounds like a mistake honestly -- although, I admit, I forget the details [06:49] dimitern, what happens if you just do set-constraints mem=8G, on anything -- does it not overwrite the whole set of constraints? [06:49] dimitern, at that level? [06:50] fwereade, so first, this only happens if you're using the same constraints document - i.e. environ constraints is an obvious case (not sure if you can do it on a machine) [06:51] dimitern, ahhh -- so it's a mgo level thing? we're not doing $unset when we should? [06:51] dimitern, would also apply to services, but, yeah, not machines [06:51] fwereade, then, it's pretty easy to try it out - juju environment set-constraints mem=8G tags=foo,bar; then again with "mem=4G" -> you get "mem=4G tags=foo,bar"; trying with "tags=" -> same result [06:51] dimitern, ...unless we get cleverer about retry-provissioning-with0different-constraints one day [06:52] fwereade, it is a mongo thing indeed [06:53] dimitern, right, yeah, I see -- we just $set and don't $unset everything else [06:53] dimitern, dammit [06:53] fwereade, so what I did to fix the failing tests for the time being - added ResetEnvironConstraints which does removeOp+createOp in export_test [06:53] fwereade, we just $set [06:54] fwereade, but this needs fixing I think - it's pretty surprising [06:54] dimitern, I would suggest that those tests are failing because they should, and ResetEnvironConstraints is a Bad Thing [06:54] dimitern, but, good catch [06:54] fwereade, if the behavior is "don't touch anything I don't explicitly set" - that's great, but there needs to be a way to reset them [06:55] dimitern, I'm not sure how great it is actually [06:55] * fwereade thinks [06:55] fwereade, they only started failing because so far there were only 2 cases where we do "set a list-constraint to empty", then "set it to not empty" - last 2 cases, any other case added after it reveals the bug [06:56] fwereade, tags were totally untested in this set-env-check-inherited-repeat [06:57] * fwereade glowers around in an undirected sort of way and reminds everybody that tests are critically important [06:58] fwereade, I think this deserves a bug report and I'll file one [06:59] dimitern, at the very least, `an empty value always means "not constrained"` and we're failing there [06:59] dimitern, https://jujucharms.com/docs/devel/charms-constraints [06:59] dimitern, yes please [07:00] fwereade, it can be fixed easily, but the expected behavior needs to be well defined - do we unset what you don't provide implicitly? if not, how can you tell juju to do unset+set when you want? [07:00] dimitern, I think we should indeed unset everything not provided [07:02] fwereade, and provide a "merge existing" command? [07:10] fwereade, we don't really specify that the merge-by-default is what happens, so I guess we could safely do replace-by-default, at least for environ constraints [07:12] dimitern, yeah, I *think* the merging behaviour can be reasonably considered a bug [07:12] dimitern, I don't think there's much reason for a merge-existing? [07:12] dimitern, the inheritance is confusing enough [07:13] dimitern, effectively having an extra level of it is IMO worse than assuming people know what they want to set [07:13] fwereade, it's also that it only really matters for lists, as other fields usually have a non-ambiguous "empty" value [07:14] fwereade, fun fact: "cpu-cores=" != "" != "cpu-cores=0" [07:14] dimitern, yeah, I think we definitely want to overwrite lists rather than merge [07:15] fwereade, ok, then I'll make this part of my fix [07:15] dimitern, yeah, `cpu-cores=` means "fall back" where `cpu-cores=0` means "any number of cores is fine" -- right? [07:16] fwereade, not quite :) - all of the cases match omitempty [07:16] fwereade, but only when stored in state [07:17] dimitern, wait, omitempty ignores the difference between `nil` and `new(int)`? [07:17] fwereade, but e.g. (*int)(value-with-0) and (*int)(nil) are treated the same [07:18] * fwereade resists the urge to render #juju-dev NSFW forever [07:18] fwereade, :D [07:18] * fwereade is going for a quiet rage-filled cigarette [07:18] dimitern, thanks for many good catches then :) [07:18] bbs [07:18] fwereade, cheers :) [07:30] fwereade, env:"mem=8G", svc:"mem="; effective: "mem=" which is the same as "mem=0" [07:38] so "X=" is never treated as "", but as "X=" instead [08:05] morning, lots of traffic on CI *lol* [08:21] large packages w/o sub-packages lead to story-telling test function names [09:00] TheMue, dooferlad, guys, will be a couple of minutes late for standup [09:01] dimitern: ok [09:03] * TheMue looks left [09:03] * TheMue looks right [09:04] I'm a poor, lonesome coder *sing* [09:04] TheMue: https://plus.google.com/hangouts/_/canonical.com/juju-sapphire?authuser=0 ? [09:05] dooferlad: I'm here https://plus.google.com/hangouts/_/canonical.com/juju-sapphire too. the one that has been in the reminder mail of the calendar. [09:05] TheMue: we are in there too [09:05] dooferlad: will reconnect [09:31] TheMue, dimitern: why are we pulling environ config down to the machine agent to figure out if we support networking? [09:32] TheMue, dimitern: shouldn't this be a job? [09:32] fwereade, because we need an environs.Environ to type-assert on [09:33] dimitern, I don't think there's any justification for creating an environ in an agent [09:34] dimitern, we can figure that out at machine-creation time, can't we? when we've already got the state policy available? [09:36] fwereade, it a different sort of job - address (de)allocation and manage networking [09:39] dimitern, yeah, I'm not saying its the same as JMN -- but wouldn't it be JobAllocateAddresses or something? [09:41] fwereade, yeah, I agree the best thing is to add a job [09:42] fwereade, however, as a incremental improvement, we can make our tests better around checking if the environment supports address (de)allocation (which we don't have full coverage of now) [09:42] fwereade, and then build on top of this by introducing a job [09:42] fwereade, sane? [09:43] dimitern, what I'm worried about is that this is further degradation of machine agent sanity -- it's bad enough that there are some bits of code that switch on provider type, but I'm really bothered by *new* code that actually pulls down a whole env config and tries to construct it and hopes that the foul overwrite-secrets hack doesn't cause problems [09:44] dimitern, imo the machine agent shouldn't even know what sort of provider it's running on [09:47] fwereade, fwiw this only happens once per environment [09:48] dimitern, how does that make env-construction in the machine agent any less evil? [09:48] fwereade, and we did drop one (the only remaining?) provider type checks in the MA using JobManageNetworking [09:48] fwereade: how do you compare address allocation support with firewaller modes? the depend on the providers capabilities too. could a similar approach be feasable? [09:49] fwereade, why is it evil for the JobManageEnviron agent? [09:49] fwereade, it has full access to it anyway [09:50] dimitern, the *agent* shouldn't know anything about the environ [09:50] dimitern, it might run some job that knows about it [09:50] dimitern, but that's a completely distinct responsibility [09:51] TheMue, I'd say that firewaller modes should not be using environ config either [09:52] fwereade, by "some job that knows about it" you mean a worker that pulls the environ config on start? [09:52] dimitern, yeah [09:52] fwereade, even if it doesn't need it except to check whether it needs to do anything at all [09:52] dimitern, via a facade, and for a worker, that's allowed to know about it [09:53] dimitern, do we know why we added Environment() to the Agent facade in the first place? [09:53] fwereade, not really [09:53] fwereade, I assume because it was there [09:54] dimitern, oh, feck, it's not even that [09:54] TheMue, +1 about the firewaller [09:54] dimitern, someone just thought "lol I'll use the wrong facade in the agent" [09:54] fwereade, :) [09:55] fwereade, ok, can we agree to add a job later? [09:55] fwereade, I promise we won't forget about it [09:55] dimitern: my though is that we have more and more providers with different capabilities, so how to only start those workers (in their right modes) for the current environment? [09:55] dimitern, TheMue: this all seems like code that's been added in the last couple of days though [09:56] dimitern, TheMue: and should frankly have never passed review [09:56] fwereade, the change is strictly an improvement over what we had before [09:56] "should" is a wonderful subjunctive [09:57] dimitern, TheMue: at the *very least*, why is this stuff not handled inside the addresser worker? [09:57] fwereade, this check is simply for a better UX - not to spam the logs with "addresses: not starting.." [09:57] fwereade: yep, agree to dimitern, the old approach always started the worker, got the events but done nothing [09:57] fwereade, it used to be, but TheMue suggested it should be done at the apiserver level with a single call [09:58] fwereade, and I liked the idea of not violating layers unnecessarily [09:58] dimitern, TheMue: I don't see how gunking up the already-terrible agent code is an improvement over making the addresser exit quietly with a single log message when it finds it's in an inappropriate env [09:58] fwereade, it is, after all, a job that requires both access to state and the provider - apiserver is the obvious place for that, isn't it? [09:59] dimitern: the dont-start-not-needed-worker came later and is a different one. imho that's wht we currently talk about [09:59] fwereade, it exits quietly ever 3s as it gets restarted by the runner [09:59] dimitern, TheMue: wait, I hope it doesn't use state directly, does it? [10:00] fwereade: nope, that has been the original idea of the change [10:00] fwereade, not anymore, that was the whole point of the exercise - it uses its own apiserver facade [10:00] fwereade: no more state usage [10:00] ah cool :) [10:00] :) [10:00] (although... why would we *ever* have used state directly?) [10:00] (we did *so much fucking work* to cut out direct state access) [10:01] fwereade, because its on the JobManageEnviron MA [10:01] dimitern, that dooesn't mean iit gets to see mongo [10:01] fwereade, this card along with a bunch of others (some of which already landed) is our effort to cut state access for JME workers [10:02] dimitern, re the 3s restart -- why don't you just *return nil*? [10:02] dimitern, meaning "I have done all the work I can, don't bother me agaib" [10:02] fwereade, if the runner supported a way to tell it "don't restart me anymore" [10:02] fwereade, it would've been simpler [10:03] fwereade, IIRC that leads to the same thing [10:03] runner.go:205 [10:04] runner_test.go:55 [10:05] fwereade, hmm, ok fair enough [10:05] fwereade: nice [10:05] fwereade, so it does not restart if it return nil from the loop? [10:05] dimitern, yeah [10:05] fwereade, that's ok, but not good enough I'm afraid [10:05] dimitern, go on [10:05] fwereade, we need a way not to start it in the first place [10:05] dimitern, why? [10:06] fwereade: dimitern: so addresses starts, first time talks to API, get's a not supported error back, and quits nicely [10:06] fwereade, i.e. a way to return something != nil from New() that tells the runner not to run it [10:06] addresser [10:06] dimitern, TheMue: why can't the worker just detect that it can't do anything and stopp itself? [10:07] dimitern, TheMue: it's not an error -- it has examined the world and determined that it doesn't need to do anything ever [10:07] fwereade, hmm.. I guess the apiserver facade's WatchIPAddresses can return a NotSupportedf if it doesn't make sense to watch or run the worker? [10:08] fwereade: it needs the environment to check this capability, so we move the question from machine.go into the addresser worker itself. that's what you meant? [10:08] dimitern, TheMue: I'd prefer a sepaarate api call, rather than overloading that one [10:09] fwereade, that's fine - it can be separate [10:09] sounds good [10:09] fwereade, what's important is will it work before the loop starts [10:09] TheMue, I *would* probably like it most if we represented it as a job, but yeah, it's definitely better to have it scoped to the addresser worker [10:09] TheMue, dimitern: although actually, yeah, maybe it *is* just part of JME [10:12] TheMue, dimitern: this may be relevant: in the unit agent, now, the manifolds themselves return dependency.ErrMissing when started in an inappropriate context [10:13] TheMue, dimitern: this means we can have nice declarative conditional-free definitions of various sets of workers and their dependencies [10:13] TheMue, dimitern: relevant/sane? [10:14] fwereade: have to read tha last sentence twice [10:14] TheMue, dimitern: see cmd/jujud/agent/unit/manifolds.go [10:14] fwereade: thx for the hint [10:15] TheMue, dimitern: and then the feature-flag checkinng inside worker/rsyslog and worker/logsender [10:15] TheMue, dimitern: in practice we'll only run one of those at a time I think [10:16] fwereade, I like this a lot [10:16] TheMue, dimitern: but they can make that determination individually [10:16] fwereade, it will also make testing this a *lot* easier [10:16] dimitern, :D [10:16] fwereade, one of each *per environ* [10:16] dimitern, yeah, agreed [10:17] dimitern, TheMue: so, to reiterate [10:18] * TheMue listens [10:18] fwereade, yeah, let's summarize [10:18] dimitern, TheMue: I don't think we do need a separate job there, sorry for the noise on that front [10:18] fwereade, in practice, no we don't [10:19] dimitern, TheMue: but if we can just make the addresser (and firewaller?) check for their own validity, log once, and return no error [10:19] dimitern, TheMue: then we get a slightly cleaner agent [10:19] dimitern, TheMue: for relatively little in the way of code changes [10:19] fwereade: dimitern: oh yes +100 [10:20] dimitern, TheMue: and it *also* makes my job much easier when I come to use manifolds in the machine agent [10:20] dimitern, TheMue: which I am very keen to do :) [10:20] fwereade: so I'm changing addresser again and you the firewaller *lol* [10:21] TheMue, if you can bear to hit both of them I would be most grateful :) [10:21] TheMue, fwereade, let's not do all at once though - first the addresses, then the firewaller [10:21] dimitern, yeah, that works for me [10:21] +1 [10:21] addresser even [10:21] dimitern, TheMue: hopefully having done one will make the second one trivial anyeay [10:22] fwereade, it should, yeah [10:22] dimitern, TheMue: cool, thank you very much [10:23] dimitern, TheMue: and then RB2390 becomes ~redundant, in favour of another unit test or two in each of the relevant workers [10:23] fwereade, so, if I'm getting it right, you suggest to have a method in the workers' apiserver facade like CanRun() [10:23] fwereade: 2390 will be dropped [10:23] fwereade, which is called at the start of Handle() ? Or in the constructor returning the worker? [10:24] fwereade, I'd really like it if we can avoid starting the loop if we can - i.e. constructor is better [10:24] dimitern, sadly I'm not sure how we handle failing to start a worker... so it might perhaps be best to return a degenerate, already-stoppped worker from NewWorker? [10:24] dimitern, little bit hackish [10:25] fwereade, this assumes we have a special error like ErrCannotStart, which startWorker understands [10:25] dimitern, but easily replaceable, I think, when I use manifolds [10:25] dimitern, yeah, if you want to add ErrWorkerNotNeeded handling to worker.Runner, that could work [10:25] dimitern, but Runner *is* a bit hairy [10:26] fwereade, yeah, very much so - ok, then constructing a "degenerate" worker in this case might be better until it can be converted to use manifolds [10:27] dimitern, TheMue: http://paste.ubuntu.com/12117329/ [10:27] dimitern, TheMue: and then for the manifolds we don't even need that [10:27] dimitern: so this degenerated one has a Handle(), but that only returns I'mNotNeeded [10:27] dimitern, TheMue: just return dependency.ErrMissing and it won't try again until its dependencies channge [10:28] dimitern, TheMue: which is quite likely to be "never" [10:29] dimitern, TheMue: so addresser.NewWorker would either return a StringsWorker or a FinishedWorker [10:29] fwereade: a little misusage of ErrMissing, eh? [10:29] TheMue, sort of, yeah :) [10:30] TheMue, although it *does* seem appropriate to try again when, eg, the api conn bounces [10:30] TheMue, you might get a different answer next time ;) [10:30] fwereade, TheMue, ok, in NewWorker we log about it and return FinishedWorker [10:30] fwereade: ah, how I love the benefit of working with interfaces as arguments or return values. does not mean the error, only the return of worker.Worker [10:30] dimitern, sgtm [10:30] TheMue, yeah, indeed [10:31] fwereade, where does dependency.ErrMissing come into play is what I don't get yet [10:31] dimitern, it's only relevant when I get around to using manifolds [10:31] fwereade, I see, great! [10:31] dimitern, fingers crossed I'll make a start on that this week... [10:31] dimitern, TheMue: thanks guys, gtg [10:32] dimitern: so, master of the saphire cards, how we wonna split it? [10:33] TheMue/dimitern: could I get a quick review? http://reviews.vapour.ws/r/2392/ [10:33] dimitern: and when you have a moment, can we discuss my next card. [10:33] dimitern: todays test card can get the addresser startup change card [10:34] dimitern: the firewaller one has should be a new one for backlog [10:34] TheMue, so, it sounds like the following: [10:35] dooferlad, sure, will have a look shortly [10:36] TheMue, a new CanStart() error on the apiserver, returning nil if address allocation with network.AnySubnet returns true, nil [10:36] TheMue, we can have such apiserver methods I think, if not, then (params.BoolResult, error) and the error is not used or result.Error is not - I don't mind [10:37] dimitern: would prefer SupportsAddressAllocation(), CanStart() sounds too generic [10:37] TheMue, ok, but that will cause confusion [10:38] TheMue, let's not call it the same way as the Networking method [10:38] TheMue, CanDeallocateAddresses() is better IMO [10:38] dimitern: thats fine with me, better than asking an API "do you can start?" ;) [10:39] dimitern: and meaning "can I start?" [10:39] TheMue, then, in the NewWorker if it returns != nil, log it and return FinishedWorker, nil [10:39] TheMue, yeah [10:40] TheMue, then the test in the MA will be simply - patch newAddresser to return FinishedWorker, check the next one starts and you're done [10:40] dimitern: btw, I prefer bool instead of misuse nil as "I can" [10:41] dimitern: yep [10:41] TheMue, because testing that CanDeallocateAddresses() works with or without the feature flag is an apiserver test, not an agent or api test [10:41] dimitern: exactly [10:42] TheMue, ok, that's easy - the client-side api returns (bool, error) and you use params.BoolResults (no ",error") at the apiserver side [10:42] dimitern: top [10:43] TheMue, testing the FinishedWorker logic is only relevant to the agent, so test it there only [10:43] TheMue, on the worker side, check it stops and logs when the feature flag is off and that's all I think [10:44] dimitern: sounds pretty complete, yes [10:44] TheMue, awesome! can I ask you to add these steps to the card so it capture what we decided? [10:44] TheMue, I really need to get some branches landed :) [10:45] dimitern: that's what I wonna do and also add a dummy firewaller card to the backlog [10:45] TheMue, cheers! [10:46] dooferlad, back to your review btw [10:47] dooferlad, doesn't the client-side api need to change as well? [10:48] dimitern: probably! [10:48] dooferlad, it does, now it returns response.Results, err [10:49] dimitern: OK, will fix. Sorry, not a great start to the day :-| [10:49] dimitern: did you want me to look at spaces list after this? [10:50] dooferlad, no worries [10:51] dooferlad, yes, I'm almost done with the review [10:51] dimitern: just wanted to know what to queue up next. [10:51] dooferlad, do you mean subnets list? [10:52] dimitern: https://canonical.leankit.com/Boards/View/101652562/116678115 [10:53] * dimitern looks [10:54] dooferlad, just let me finish the review first, bbiab [10:55] dimitern: sure [10:58] * dooferlad seems to have got confused. Perhaps coffee is required. [11:17] dooferlad, reviewed [11:18] dooferlad, let me know what you think [11:18] dooferlad, I forgot to mention the needed client-side api change as well.. adding it now [11:22] dooferlad, hmm, actually returning all results might be better for the CLI - it can display spaces fetched successfully and display errors for others (like juju status does) in the Status field of the output [11:22] morning [11:23] perrito666, morning [11:24] * perrito666 waits for the coffee before hitting the review queue [11:24] dooferlad, the good thing about this is we don't have to display the error (i.e. change the CLI) right away [14:33] fwereade, cmars: it would be nice if the new ValueWorker found its way into master sooner rather than later :) [14:34] (https://github.com/juju/juju/pull/3006) [14:35] ericsnow, I'm a little bit distracted right now, but if you'd like to pull it across I will gladly LGTM it, I think the diff should be clean [14:35] Bug #1486074 opened: juju bootstrap fails to work due to incorrect Identities restriction.

[14:35] fwereade: k, thanks [14:35] ericsnow, thank you, you're doing the tedious bit ;) [14:44] fwereade: http://reviews.vapour.ws/r/2397/ [14:44] fwereade, cmars: keep me posted if you do anything to help with nesting dependency engines :) [14:46] ericsnow, ship it :) [14:46] fwereade: thanks [14:51] ericsnow: ping === Spads_ is now known as Spads === anthonyf is now known as Guest61698 === Guest61698 is now known as anthonyjf === dooferlad_ is now known as dooferlad [15:09] dimitern/TheMue: when you have a moment: http://reviews.vapour.ws/r/2392/ [15:09] dooferlad: yep [15:12] dooferlad, will have a look soon [15:25] axw, wallyworld: a bit clunky, perhaps, but does http://paste.ubuntu.com/12118757/ strike you as a useful primitive? [15:33] k [15:34] #wrongwindow [15:45] dooferlad, reviewed [15:47] Bug #1486106 opened: TestNetworkInterfaces fails on vivid/wily

[15:53] Bug #1486106 changed: TestNetworkInterfaces fails on vivid/wily

[15:59] Bug #1486106 opened: TestNetworkInterfaces fails on vivid/wily

[17:53] Bug #1461890 changed: leadership unreliable in HA

[17:53] Bug #1478024 changed: Looping config-changed hooks in fresh juju-core 1.24.3 Openstack deployment

[17:56] damn, just hit one more reason I don't like putting test code in _test packages - if you have tests you need to put in the actual package, you can't access the helper code that is in the _test package. le sigh. [17:57] katco: can you ask someone to look into bug 1486166. I cannot tell were the regression lies. [17:57] Bug #1486166: JES deploy fails

[17:58] sinzui: tal [17:59] sinzui: dimitern's team is on bug squad this iteration. can it wait until they come back online? [18:03] katco, sure, I am not going to block trunk for the failure [18:03] sinzui: ok ty, i'll send an email to dimitern [18:09] katco, sinzui, I've looked at this one and it seems to me it's badly written - assuming it will only run on environments where the jes feature flag is on [18:15] Bug #1486166 opened: JES deploy fails

[19:09] need to leave for a sec, bbl [19:12] katco, sinzui I think that is a bug for thumpers team [19:12] I can ping him when he is online [20:00] ericsnow: ping, how's it going? [20:00] wwitzel3: good [20:07] * katco grabs popcorn for this fascinating conversation [20:13] perrito666: any chance you could do a mini-review for a critical 1.24 bug? [20:13] perrito666: http://reviews.vapour.ws/r/2402/ [20:13] katco: going [20:16] katco: ship it [20:16] perrito666: ty sir [20:20] katco: please update the re-broken bug [20:20] perrito666: yep, ty for the reminder [20:22] katco: btw, this might be sympthom of a different problem [20:23] perrito666: yes, i agree. but this is a good start as it may be affecting multiple things, and the bug it fixed was much lower priority [20:23] if we pass a set of explicit ident files and expect one of those to be the correct one and it isn t then there is a bigger issue there [20:23] perrito666: yep. [20:23] perrito666: more trying to address this: https://bugs.launchpad.net/juju-core/+bug/1486074 [20:23] Bug #1486074: juju bootstrap fails to work due to incorrect Identities restriction.

[20:24] perrito666: err, oops [20:24] perrito666: https://bugs.launchpad.net/juju-core/+bug/1472632 [20:24] Bug #1472632: regression: juju ssh dies with (publickey)

[20:30] yup, I read the others, the fix does more harm than good :) [20:31] perrito666: meaning the fix i backed out? or the fix to fix the fix? :) [20:32] * perrito666 is very tempted to answer in french and step this into a whole new realm of lexical complexity [20:32] haha [21:12] Bug #1486254 opened: Raw Go errors reported to users [21:18] Bug #1486254 changed: Raw Go errors reported to users [21:21] Bug #1486254 opened: Raw Go errors reported to users [22:04] thumper, ping [22:05]