[01:14] thumper: ok [01:14] thumper: FWIW, i think any of theproposed solutions will solve the issue at hand [01:15] i think there is pleanty of scope for a morally correct fix once 1.14.0 is out [02:36] thumper, happy birthday [02:36] arosales: thanks, but it is tomorrow here :) [02:36] it still today here [02:36] and possibly yesterday somewhere else [02:37] thumper, happy belated birthday ;-) [02:38] :-) thanks [03:45] * thumper pulls a sad face [03:52] thumper: what's making you sad? [03:52] I was adding an apiserver endpoint [03:52] and instead of using a pointer to a struct [03:52] I wanted to use an interface [03:52] everything worked well [03:52] until you called it [03:52] caused a panic in the reflect library [03:53] had to change it to a struct pointer [03:53] because I didn't want to fix our rpc code [03:53] somewhat out of scope for this change [03:54] axw, wallyworld: I completely missed the reminder about package review [03:55] oh yeah [03:55] me too :) [03:56] nobody's selected anything to review either have they? [04:01] not that I know of [04:02] thumper: me too [04:02] i've been too busy to notice [04:02] next week :-) [04:05] thumper: has mramm approved your travel? just wondering if my travel request has shown up at all [04:06] yes he has [04:06] but I've not booked it yet [04:06] should get on it [04:06] axw: poke him ovre email [04:06] will do, thanks [04:49] what are the dates again ? [04:49] 21-25 I think [04:52] ta [05:13] gah [05:13] * thumper emails travel people [05:15] thumper: I'm going to work on bugs, unless you've got something else you'd rather I do? [05:16] you could add the "SupportsContainers" method to the Environment [05:16] it is pretty trivial [05:16] but then we need the add-machine code to use it [05:16] also container constraints [05:16] * thumper is polishing a pipeline of work [05:16] container constraints? or environment constraints? [05:17] * axw hasn't heard of container constraints before [05:17] well, there is a constraint for putting something in a continer [05:17] * thumper thinks [05:17] so a machine constraint [05:17] * thumper is trying to work out why the tests blew up [05:17] ok [05:19] jam: you around? [05:24] at least it looks like changes I added, so shouldn't be too hard to work out [05:24] jam: https://codereview.appspot.com/13421043/ (not sure why it is a closed issue, because it hasn't landed) [05:25] jam: it is a branch the rest of my stuff depends on [05:25] * thumper EODs to make dinner [05:44] jam: is there a bug for the mgo revert ? [05:44] i need to add it to the build tagball scripts [05:44] davecheney: bug #1221705 [05:44] <_mup_> Bug #1221705: relationunit_test.go: 2 tests fail with mgo >= 241 [05:45] "Fix Released" in that juju-core has worked around the test suite failing. [05:45] ta [05:50] https://codereview.appspot.com/13515045 [05:52] davecheney: approved [05:53] davecheney: I *think* that for 1.15 we'll want to go back to tip, and sort out the problems, but it is prudent to use rev240 for 1.14 [05:56] jam: so, to confirm, you do _not_ want this commited to trunk ? [05:57] davecheney: I think we want it committed to trunk until the point that we move the rev of mgo on the bot. [05:57] I'd like to say otherwise [05:57] but it isn't reasonable to release a version we haven't been testing [05:58] i think we're about 18 months too late to add that kind of hurdle [06:01] jam: ignore my previously comment [06:01] i parsed it incorrectly [06:29] jam: https://code.launchpad.net/~dave-cheney/juju-core/backport-r1782/+merge/184938 [06:39] davecheney: approved [06:44] ta' [07:32] mornin' all === rogpeppe1 is now known as rogpeppe [07:38] this looks very cool indeed; i haven't tried it yet though. https://docs.google.com/document/d/1WmMHBUjQiuy15JfEnT8YBROQmEv-7K6bV-Y_K53oi5Y/edit# [07:39] i've been waiting for something like this ever since oakland, when the author mentioned something about it to me [07:41] rogpeppe: it does look pretty cool. [07:41] oakland? I thought adonovan was in NY [07:42] axw: he was around in sf for the golang meetup [07:42] ah [07:44] a probably dubious thought - i wonder if you could use this instead of explicit tests for some hard-to-test functionality. === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: fwereade | Bugs: 9 Critical, 123 High - https://bugs.launchpad.net/juju-core/ [07:52] * axw waits for someone to write a web UI for it [07:53] won't be long now [08:14] bigjools, you still awake? [08:18] evilnickveitch: seen my hint regarding the docs change for the local provider? [08:19] TheMue, hi, yes indeed, sorry it has taken a while to get around to it but have been kept rather busy by the site redesign! [08:19] There are still a few things queued up [08:20] evilnickveitch: ah, ok. but i'm happy if it is in this queue too. ;) [08:20] evilnickveitch: I was eating dinner [08:20] bigjools, at this time in the morning? [08:21] rogpeppe, axw, dimitern: who would like to review https://codereview.appspot.com/13522043 [08:21] TheMue: it seems you already have an lgtm on it [08:22] I can look, but yeah, you only need 1 LGTM now [08:23] i know, but still feling better with two :D [08:23] but it's ok so too [08:34] evilnickveitch, so, I'm sorry, I got very behind on the actual changes, and my docs are conflicty as anything [08:34] evilnickveitch, can you briefly run down what's changed so I can most simply eliminate the structural changes and focus on the actual doc changes? [08:35] evilnickveitch, eg, is there some way I can just blat a matching pre/postamble onto the pages I've written and go from there? there didn't seem to be an obvious way to do that [08:36] fwereade_: one of the big problems with pure html. no #include directive :) [08:36] jam, quite so [08:37] (grumble grumble generate from simple source format grumble) [08:37] fwereade_, don't worry about that, i will fix up the styles - the redesign changed a lot of stuff [08:37] fwereade_: it was one of the things I never quite understood how to fix. You end up having to do "dynamic" content just to get consistent headers and footers. [08:37] evilnickveitch, well, OK, I just WIPped the branch, but there should be some useful content in there [08:37] jam, there is! Sort of - you can use the tag, but it screws up the css [08:38] TheMue: reviewed [08:38] fwereade_, that's fine. There will be a new tool to generate a new page. soon :) [08:38] evilnickveitch, it's https://code.launchpad.net/~fwereade/juju-core/docs-splurge/+merge/184833 -- can I leave it in your hands to either fix it up or tell me what I have to do to help you? [08:39] fwereade_, yup, I already saw it. Just fixing some design tweaks and then I will look over it properly [08:39] evilnickveitch, <3 [08:39] axw: thx [08:41] evilnickveitch, I sort of feel that there are probably other underdocumented bits that I might be in a position to help with [08:42] evilnickveitch, is there anywhere you can think of where I should look in particular? [08:42] fwereade_, there is still the outstanding task list [08:43] hang on, i'll get you the link [08:44] fwereade_, here:https://docs.google.com/a/canonical.com/spreadsheet/ccc?key=0AluxMMubLozZdEVhald5bFN0U3RFbjdqNW96RWx5Snc#gid=0 [08:44] feel free to add anything :) [08:44] evilnickveitch, found it, thanks [08:44] evilnickveitch, just looking at m_3's interfaces stuff, seeing how badly I collided with him [08:45] fwereade_, it's okay, i will make sense of it all. It is easier for me to edit things together. [08:46] * TheMue seems to have problems with the compatibility - so many typos *sigh* [08:47] TheMue: I think it's not so bad when all a review turns up is typos :) [08:47] ... in comments anyway! [08:48] evilnickveitch, fwiw I think m_3's interfaces stuff is missing some subtleties -- he seems to be depending on relation-get in a -joined hook, and that's unwise (if perhaps somewhat instructive) [08:48] evilnickveitch, I'm afraid I haven't done a broad pass over the docs to see what recommends things contradicted elsewhere :( [08:48] axw: yeah. but already dimitern found several misspellings of this word before. it seems it has burned in my brain in a wrong way. [08:49] heh :) [08:50] fwereade_, okay, I will watch out for that. Maybe the best thing is for me to take a pass over it all and then let you look over the results? It would be very helpful. [08:51] what's the policy for US/UK spelling in Ubuntu technical docs? [08:51] evilnickveitch, absolutely, no problem [08:51] fwereade_, thanks! [09:00] fwereade_: any chance of a quick chat about https://codereview.appspot.com/13640043/ ? [09:00] rogpeppe, sure [09:00] fwereade_: https://plus.google.com/hangouts/_/fe0782db82ad005f124b51fd3035bf811cb05e5d?authuser=1 [09:26] wallyworld: poke if you're around [09:27] fwereade_: ping [09:28] dimitern, pon [09:29] fwereade_: in worker/uniter/context_test.go there's a test called TestUnitCaching() which relies on PrivateAddress and PublicAddress being on the unit doc (like Life) and basically test what TestRefresh does [09:30] fwereade_: but once we switch to the API context.unit.PrivateAddress() will always return the up-to-date value, so there's no caching - i'm thinking of dropping that test altogether [09:30] fwereade_: unless you think that caching has some significance [09:31] dimitern, ehh, that's interesting [09:31] dimitern, take a quick look at uniter/context.go [09:32] dimitern, it feels like it would in general be goofd to maintain the usual guarantees [09:32] dimitern, that values from hook tools will not change over the course of a hook [09:32] fwereade_: how can I do that with the API? [09:32] dimitern, in which case, lazily getting the value when requested and caching it for the lifetime of the context does sound smart [09:33] dimitern, just how it does for config settings, relation settings, whatever [09:33] fwereade_: you mean each value is read only once? [09:33] fwereade_: from the api, and cached [09:33] dimitern, get if missing, otherwise use cache, make sure cache is invalidated properly [09:33] dimitern, but do this at context granularilty [09:34] dimitern, so we get it once per RunHook [09:34] fwereade_: when should cache invalidation happen? [09:34] dimitern, it may be that the precise test you're worried about is actually orthogonal [09:35] dimitern, for private/public address, it should be trivial, in that it happens automatically when the context is trashed at the end of the hook run [09:35] fwereade_: so at RunHook invalidate the cache? [09:35] dimitern, the cache is the context [09:35] dimitern, the context is created in runHook [09:35] dimitern, isn't it? [09:35] fwereade_: yeah, seems so [09:35] dimitern, look what we do for service config settings [09:35] dimitern, should be analogous [09:35] fwereade_: right, thanks [09:41] wallyworld: bug #1223752 [09:41] <_mup_> Bug #1223752: environs/simplestreams/simplestreams.go leaks test:// and file:// URLs into the http.DefaultClient [09:42] fwereade_: I'm starting to actually like the idea of an "httpps://" protocol that gets registered with the default http.DefaultTransport. [09:43] fwereade_: or even be verbose about "insecure-https://" or something like that. [09:43] nonvalidating-https:// is a bit long [09:44] It has the very nice property that all clients get access to it. [09:44] And it allows all other https:// requests to still be validated. [09:44] I came across it because of bug #1223752 [09:44] <_mup_> Bug #1223752: environs/simplestreams/simplestreams.go leaks test:// and file:// URLs into the http.DefaultClient [09:44] which is exposing test URLs to the DefaultClient accidentally. [09:47] It has the downside that mutating DefaultTransport can't actually be undone [09:47] (There is RegisterProtocol but I don't see an UnregisterProtocol) [10:14] jam: i just got back from a school debate. oh the joy [10:18] file:// is intended to be registered since image metadata and tools urls can use that and httpClient.Get() works just fine with file:// so long as it is registered [10:19] wallyworld: except you end up globally registering it which means net/http.Get() will also grab file:// urls [10:19] sure [10:19] is that an issue? [10:20] simplestreams uses net/http.Get() from memory, not sure, will have to check the code [10:20] the tools-url can be either file:// or http:// [10:21] so both need to be registered/supported [10:21] wallyworld: you have "environs/simplestreams/simplestreams.go" which has its own "httpClient" variable [10:21] it seems like you were trying to isolate it for some reason [10:21] but then you pass the DefaultTransport to that object [10:21] and then register the "file" protocol with the client.Transport [10:21] jam, sorry, was meeting, reading scrollback [10:21] which... .is the DefaultTransport [10:22] wallyworld: so the code *looks* like you were taking pains to not expose this protocol [10:22] but ended up using the global objects anyway [10:22] jam, I think I may be coming round to that idea [10:23] small review anyone? https://codereview.appspot.com/13490045/ [10:23] there was some issue with lack of ability to override some aspect of constructing an gttp client, i can't recall now. but the stdlib was limited somehow. i'd have tp re-read the code [10:24] wallyworld: so there is a problem that there is no way to "Unregister" a protocol [10:24] yes, that is sad [10:24] so you can't just set one up for a test suite and then tear it down [10:24] yep :-( [10:24] which means you end up needing to shim it in there if you want proper cleanup [10:25] like by having your own Transport and its associated registry of protocols [10:25] which then means you need a custom Client to point at that Transport [10:25] not sure why they designed it like that [10:25] you did the custom Client, but not the Transport [10:25] dimitern, on it [10:25] there may have been a reason, i'd have to re-look [10:25] i seem to recall something but can't remember now [10:26] wallyworld: on the flip-side, I may be (ab)using the DefaultTransport as well, to allow us to register "nonvalidating-https://" [10:26] to allow some URLs to be insecure. [10:26] ok [10:26] fwereade_: thanks [10:26] It has the *really* nice property that only some URLs end up insecure [10:26] instead of pretty much everything. [10:27] and allows us to support it, without having to rewrite every possible http client that might want access to these things. [10:28] dimitern, reviewed with questions [10:29] fwereade_: cheers [10:29] fwereade_: yes, it does impact tags - they're constructed by transforming the key [10:30] dimitern, ok, are there any necessary changes to tag validation functions and the like? assumptions that there's a # in there or something? [10:30] fwereade_: and i cannot remember whether there were numbers in relation names, but it doesn't hurt to have them imo [10:30] fwereade_: I made all the changes I think [10:31] fwereade_: there was # in there originally, now it's optional for peer rels [10:31] dimitern, I didn't see any tests for the tag behaviour is all [10:32] fwereade_: i'm sorry I don't seem to get you, expand please [10:32] dimitern, the set of valid relation tags has now changed too, right? and all the changes and tests are purely in terms of name [10:33] fwereade_: take a closer look at the test - it's both about names and tag formats [10:35] dimitern, TestRelationKeyFormats? you'll have to enlighten me [10:35] dimitern, fwiw its test numbering is screwy [10:35] fwereade_: so, like the unit name formats tests [10:35] dimitern, (i * len(js))+j, surely? [10:36] fwereade_: this one combines valid and invalid relation names and valid and invalid service names to form a key and test its validity [10:36] fwereade_: oh, that's right :) will fix it [10:37] fwereade_: tag formats are tests in tag_test.go [10:37] fwereade_: I'll add a peer relation tag there as well, good point [10:37] dimitern, yeah, that's all I think [10:38] fwereade_: so it isn't "just" that simple. The URLs we use for queries in Openstack are read back from keystone as part of the Authentication process. So goose would still need to know that "for the URLs you get back on this connection, rewrite them to "nonvalidating-https"" [10:40] jam, sure, understood, I wasn't imagining it was a fix for the whole problem, but the need for nonvalidating-https crosses several boundaries, right? [10:41] fwereade_: so before Ian's simplestreams work, we only really accessed HTTP behind the Provider interface. Now we do some bits directly. I'm trying to figure out if registering the new protocol, and then teaching goose to use it is easier/better than just teaching simplestreams at the low level about it. [10:41] the *nice* bit is that if we have it registered [10:41] then goose can say "the public-bucket URL is actually: nonvalidating-https://" [10:43] jam, yeah, the big problem is that it cuts across whole projects [10:43] jam, and so it's not quite clear where to put it except in its own project [10:44] jam, which feels mildly nasty [10:44] fwereade_: right [10:44] and then goose imports it, and juju-core imports it, etc [10:44] and it is already registered so both just use it [10:44] standup time: https://plus.google.com/hangouts/_/fe0782db82ad005f124b51fd3035bf811cb05e5d [10:45] mgz fwereade_ dimitern TheMue wallyworld natefinch ^^ [10:45] btw, guys, I'm not even going to try the standup today, drilling has just started and it will be unpleasant for all concerned [10:46] fwereade_: updated https://codereview.appspot.com/13490045/ [10:46] I will be here on irc though [10:46] and will hopefully be signing an actual contract tonight, soon after which I will be out of this building site [10:47] fwereade_: \o/ [10:47] fwereade_: np, what have you actually gotten done today? [10:48] mgz: ^^ [10:48] ta [10:48] rogpeppe: standup? ^ [10:48] jam, nothing apart from talking to people I'm afraid, and I have emails to write for much of the afternoon [11:03] fwereade_: ping [11:04] fwereade_: welcome to my world [11:08] fwereade_: since you're ocr, if you have time, there's a couple of merge proposals which are lonely and need some lovin' , see +activereviews [11:49] dimitern, sorry, went for lunch, couldn't stand it [11:49] wallyworld, ack [11:49] thanks :-) [11:53] lunch [11:58] fwereade_: np, have a look when you can https://codereview.appspot.com/13490045/ [11:59] dimitern, that LGTM, thanks [11:59] fwereade_: cheers [12:06] natefinch: https://code.launchpad.net/~natefinch/juju-core/008-windows/+merge/181916 It looks like you tried to land it, but had a file conflict. [12:06] Can you look at it again? [12:06] I thought we actually wanted that to land for 1.14 [12:12] jam: ahh, thanks, I missed the fact that it didn't actually land [12:50] fwereade_: slight hitch with the plan to introspect directly what workers are being run by the machine agent [12:51] rogpeppe, oh yes [12:51] fwereade_: it's not easy to tell which worker is actually being run [12:51] fwereade_: i'd thought we could do it by looking at the type of the worker [12:51] fwereade_: but several workers share the same type [12:52] fwereade_: i could just look at the runner worker id, but that doesn't actually tell us if we're actually running the correct worker [12:53] fwereade_: i suppose i could mock out all the worker New functions [12:55] fwereade_: for the specific case of the api server, this isn't an issue because the worker is of type *apiserver.Server, but i'd really hoped to use the same test style for the existing tests too (and hopefully be able to reenable some of the flaky ones too) [12:56] rogpeppe, yeah, indeed [12:57] rogpeppe, patching new-worker funcs sounds somewhat interesting me, because it's most in line with what stm to be a sensible end state [12:57] fwereade_: in the end, even if i mock out the worker New* functions, i'm still not convinced that we get sufficient test coverage though [12:57] rogpeppe, but the variety of such funcs is in itself somewhat flaky [12:58] fwereade_: you think all workers should be able to start with no input except the state? [12:59] rogpeppe, and probably the agent conf [13:00] rogpeppe, because that's got DataDir from which all good things flow, not to mention the actual tag etc [13:00] rogpeppe, however, I'm concerned it's too much of a leap from where we are to get done any time soon [13:00] fwereade_: i'm not sure. i like seeing exactly what a worker needs, rather than just passing everything in. [13:02] fwereade_: yeah, i think agree it may be too big a leap [13:02] fwereade_: for example, the current machine id isn't in the agent conf [13:02] fwereade_: but we need to pass it to provisioner.NewProvisioner [13:04] * fwereade_ raises an eyebrow... [13:04] rogpeppe, the id is needed to create the conf in the first place [13:04] rogpeppe, even if there is not currently a Tag method on conf I don't see it as forbidden knowledge in that context [13:05] fwereade_: hmm, yes, we can infer it from the data dir, i guess [13:05] fwereade_: or APIInfo.Tag [13:06] rogpeppe, anyway, not one for today, but I think thumper has also had thoughts in that direction [13:08] fwereade_: even if we mock out the worker New functions, i'm not sure that it's totally convincing that they're being used in the right way inside the guts of the machiner. [13:09] rogpeppe, surely you get to see exactly what gets started, and when they each get stopped [13:10] fwereade_: you get to see that they're started, but you don't necessarily know if they're actually wired up correctly [13:10] rogpeppe, by triggering various errors from inside and out [13:10] rogpeppe, if you've mocker out the new worker funcs, you can check the args and know what was created surely [13:11] rogpeppe, the wiring of those args within the real workers will be tested in the worker-specific tests [13:11] fwereade_: that joined-upness is something that could easily come apart AFAICS [13:13] fwereade_: in the end, i think we really do want some integration tests here, even if we do some other checks too. [13:13] rogpeppe, function signature changes are generally spotted when things fail to compile [13:13] fwereade_: i wasn't thinking of sig changes, but semantic changes [13:14] fwereade_: in the end, we really do want to check that there's actually a given worker running that actually does the right stuff. [13:14] rogpeppe, hence my preference for configuring workers with a conf and a state directly [13:14] rogpeppe, well [13:14] rogpeppe, no [13:15] rogpeppe, we actually don't, quite often [13:15] rogpeppe, we want to check, like, *one* -- the simplest one with the fewest and fewest-reaching bizarre side effects [13:16] rogpeppe, each of the workers must indeed be independently tested [13:16] * rogpeppe tries to convince himself that that's sufficient [13:17] rogpeppe, and the mechanism for creating workers must itself also be tested [13:17] rogpeppe, there are more things in play, and they require more testing, but each of the mechanisms can be tested in isolation [13:18] rogpeppe, rather than carrying all the context for the N other attached mechanisms every time you want to test just one feature [13:19] fwereade_: the difficulty i have is that working out whether this actually tests that it works is a bit like working out whether two halves of an inductive proof meet in the middle [13:20] fwereade_: it's easy to *think* you're testing that the whole thing works, but actually you're not testing what you think. end-to-end tests avoid that issue. [13:20] fwereade_: although i totally agree they're heavyweight [13:21] rogpeppe, but the bigger the test, the flakier, and the harder to fix, and the harder-still to fix correctly [13:21] rogpeppe, a few big tests are a necessary evil [13:22] rogpeppe, and a lot of small tests are a necessary good ;p [13:23] fwereade_: the other p.o.v. is that we end up with tests so reliant on internal implementation details that we can't change things without rewriting the tests, which loses one big point of having the tests in the first place. [13:24] rogpeppe, lots of packages with a few small types each usually only come about when you've actually figured out the separation of responsibilities in the system pretty well [13:25] rogpeppe, yes, you're still vulnerable to semantic changes; the big tests *are* a necessary evil [13:26] rogpeppe, but if you can't write the small tests, your system is likely to be suboptimally factored ;) [14:45] fwereade_: hey, just read backchannel and had to do a double-take [14:45] m_3, the relation-get thing? [14:46] fwereade_: yeah, that needs to be cleared up [14:46] m_3, I maintain that it cannot be relied upon in -joined [14:46] fwereade_: it's just getting the service name [14:47] so it's more pseudo-code at that point in the docs [14:47] m_3, ah! [14:47] but needs to be rewritten to make that clear [14:47] m_3, ok, I am an idiot, I misunderstood completely [14:47] m_3, I thought it was getting an actual relation setting [14:47] i.e., "mysql creates a new db based on the related service's name" [14:48] m_3, so that'd just be derived from $JUJU_REMOTE_UNIT, I guess? [14:48] so I'll give nick a MP... that's an important point to clear up [14:48] m_3, cool, thanks [14:49] right... want a way to describe that in pseudocode without bogging down in env [14:49] thanks for catching that [15:32] fwereade_: woohoo!!! all the uniter tests pass with the api!! [15:32] dimitern: awesome :) [15:33] dimitern, sweeeet!! [15:33] natefinch: yep, these were some long 3 days of struggling [15:33] * fwereade_ flings confetti [15:33] dimitern, does it work in practice as well? ;p [15:33] fwereade_: haven't tried yet [15:33] dimitern: I feel your pain. Big changes are always really hard to get 100% corret [15:34] fwereade_: what live testing should be sufficient? [15:34] * fwereade_ crosses many fingers [15:34] cross ALL the fingers [15:35] fwereade_: is your docs stuff nearly ready for review? I'm pretty keen to read it. [15:35] doh, I missed it, damn email filters [15:35] * mgz goes back and pours through [15:35] dimitern, how about checking whether a subordinate gets cleaned up properly in response to a destroy-unit of its princpal? [15:36] mgz, the conflicts are terrifying, everything htmly changed while I was doing it [15:36] mgz, but there should be some useful content [15:36] yeah, it's not a great format for version control currently, alas [15:36] mgz, I should also fix up the subordinates and implicit relations docs [15:36] fwereade_: just that? [15:37] dimitern, it feels like a decently risky start [15:37] fwereade_: ok, so I'll do 2 tests: classic wordpress + mysql stuff and another with subs (nrpe) [15:37] dimitern, cool [15:38] dimitern, maybe one specific one that deals with relation settings changing, and checking the caching stays sane [15:39] fwereade_: I'll branch and look at resolving while I read [15:39] will poke you to pull later [15:39] mgz, don't worry about that [15:39] mgz, evilnickveitch has said he knows what to do with it [15:39] I think a re-run of the template thing and some bzr commands should do it [15:39] mgz, he just announced a new way of creating pages [15:40] mgz, my first attempts in that direction were less fruitful than I had hoped they might be [15:40] yeah, saw that, was what reminded me about your docs [15:40] fwereade_: luckily I already have that prepared from last time [15:40] dimitern, nice [15:41] mgz, fwereade_ sorry for spoiling everyone's day by making things easier :) [15:42] evilnickveitch, cheers dude ;) [15:43] evilnickveitch: yell if you need a hand resolving that branch [15:47] fwereade_: but first this https://codereview.appspot.com/13324052 [15:49] dimitern, LGTM, lovely small branch :) [15:49] fwereade_: cheers :) [15:51] fwereade_: just proposing the big branch as WIP for discussion first [15:51] fwereade_: and to see how big it actually is [15:51] dimitern, cool, I have to head off and sign the contract in a few minutes [15:52] fwereade_: ah, cool, good luck then :) [15:52] ...which means, jcastro, that I'm afraid I'll be missing the charm call again :( [15:52] fwereade_: it's not that bad actually 2188 lines (+556/-285) https://codereview.appspot.com/13355046 [15:53] fwereade_: that's cool [15:56] now on to live testing === BradCrittenden is now known as bac === marcoceppi_ is now known as marcoceppi [16:16] rogpeppe, dimitern: a small review https://codereview.appspot.com/13656044 [16:19] TheMue: reviewed [16:21] rogpeppe: thx, though of that flag too but then decided to go the save way ;) [16:21] thought [16:24] dimitern, rogpeppe , mgz, TheMue, fwereade_: Anyone have an opinion on where to put the logrotate config file? is next to the log file acceptable, or should it go in the .juju folder, or somewhere else? [16:24] natefinch: shouldn't it be in /etc/logrotate.d/ ? [16:24] in /etc somewhere I'd assume [16:24] dimitern: +1 [16:25] natefinch: i think putting it in /var/log/juju would seem ok to me [16:25] natefinch: but /var/lib/juju would be ok too [16:25] rogpeppe: why there? [16:25] natefinch: but if there's some other standard, it might be best to use that [16:25] rogpeppe: there is - /etc/logrotate.d/ is the usual place [16:25] dimitern: just to limit the amount of config we spray around the system [16:25] dimitern: ok, fair enough [16:26] rogpeppe: hmm.. well, I suppose we can put it in /var/lib/juju/, but logrotated should be notified somehow to search it there [16:26] I wish linux man files would just say that [16:27] dimitern: /etc/logrotate.d sounds fine if that's standard [16:27] rogpeppe: at least that's where I put logrotate conf files before on ubuntu and it always worked [16:32] dimitern: it looks like there are several in there, so definitely seems like a standard [16:34] anyone have opinions on rotation poiicy? [16:40] natefinch: policy is the sort of thing where you just have to pick an option, then review it later if needed I think [16:41] daily seems fine [16:41] mgz: that's fine. I was thinking of something like ,roll at 50 megs, keep 4 backups, compress all but the most recent backup [16:41] mgz: hmm.. you think time is better than size? [16:41] not really, just what I'm used to [16:42] mgz: pros and cons to both I guess [16:43] anyone else seen this error when testing state? ... value *errors.errorString = &errors.errorString{s:"cannot create log collection: read tcp 127.0.0.1:54392: i/o timeout"} ("cannot create log collection: read tcp 127.0.0.1:54392: i/o timeout") [16:43] hm, no, new one to me [16:44] mgz: when i run the tests individually, they pass [16:44] mgz: but i'm seeing that issue consistently currently [16:45] I;m mostly running just one module tests at the moment, which doesnt help noticing those kinds of issues... [16:45] mgz: it's worth running whole-suite tests too, at least once a day, i reckon [16:46] mgz: this sounds promising, minsize, used with daily, means "roll over daily, but only if the log is at least N bytes" It doesn't stop you from getting 2 gigs of logs in a day, but it means you won't end up with 5 logs of 2 lines each [16:46] natefinch: that does sound good [16:47] mgz: ah! i know the problem. i think it must be reason jam rolled back the mongo version [16:48] rogpeppe: there's something wrong with worker/runner [16:48] dimitern: in trunk? [16:48] rogpeppe: yes [16:49] dimitern: hmm, a recent change? [16:49] rogpeppe: it seems it restarts workers even after a fatal error in some cases [16:49] dimitern: oh really? have you got a way of reproducing the behaviour? [16:50] rogpeppe: still looking - so far I can only reproduce it once I migrated the uniter to the api and it happens in TestWithDeadUnit [16:51] rogpeppe: the first time Login succeeds, then we return ErrTerminateAgent because the entity is dead, that gets the task killed with "fatal", but after that it's immediately restarted [16:51] dimitern: i can see a way that it can happen, but only a very small time window, and i'm not sure that's the issue you're seeing [16:52] dimitern: can you paste a log from when it happened? [16:53] rogpeppe: http://paste.ubuntu.com/6093304/ here's a snippet [16:53] rogpeppe: as you can see the first time it's ok, then it gets into a loop restarting it [16:55] dimitern: ah! i think i see what might be the problem. [16:55] rogpeppe: oh? [16:55] dimitern: i don't *think* it's a problem in worker/runner, although, hmm [16:56] rogpeppe: perhaps it's in the common agent code [16:56] rogpeppe: but can't figure out what [16:56] dimitern: oh, this is the unit agent, isn't it? [16:56] rogpeppe: yeah [16:58] dimitern: that scuppers that thought. thinking again. [17:00] dimitern: the weird thing is line 22 of that log [17:00] dimitern: why is the unit agent being restarted? [17:01] rogpeppe: exactly my question [17:01] dimitern: i mean the unit agent itself, not the uniter worker [17:01] dimitern: that's nothing to do with worker.Runner [17:02] rogpeppe: ah, ha! [17:03] dimitern: BTW I think openAPIState should return ErrTerminateAgent if it gets a permission-denied error [17:03] dimitern: i think if you fix that, this problem might go away [17:03] rogpeppe: I was thinking the same [17:11] dimitern: which actual test was running in the log you posted? [17:11] dimitern: let me guess - it was TestWithDeadUnit, right? [17:12] dimitern: if so, everything's working as expected, except the permission-denied problem [17:12] rogpeppe: yep [17:13] rogpeppe: still can't make it exit, even after returning errTerminateAgent on CodeUnauthorized [17:13] dimitern: hmm. could you paste the (complete) log again? [17:14] dimitern: (of running just TestWithDeadUnit) [17:14] rogpeppe: one moment [17:14] rogpeppe: the "timeout" test failure you saw was, indeed, why I rolled back to rev 240 on the bot [17:15] jam: yeah, tests passed again when i reverted to that rev [17:15] rogpeppe: does this look ok http://paste.ubuntu.com/6093367/ [17:15] rogpeppe: that's inside agent/openAPIState now [17:15] dimitern: doesn't look quite right [17:15] rogpeppe: and still I get *exactly* the same log output [17:15] rogpeppe: why? [17:15] dimitern: don't you want to return ErrTerminateAgent on not-found? [17:16] dimitern: are you thinking you've got a C-style switch statement? [17:16] rogpeppe: oh.. [17:16] dimitern: i think you want a "," [17:16] dimitern: or just an if statement... [17:18] rogpeppe: changed to an if [17:18] rogpeppe: and still the same result - here's the complete log http://paste.ubuntu.com/6093388/ [17:21] dimitern: what does your openAPIState function look like now? [17:22] rogpeppe: http://paste.ubuntu.com/6093402/ [17:22] dimitern: ah, i see the problem [17:23] rogpeppe: good! where it is? [17:23] dimitern: the OpenAPI call is failing, not the Entity call [17:23] dimitern: you need to do a similar thing for the error result of agentConfig.OpenAPI [17:23] dimitern: it never gets as far as calling Agent().Entity() [17:24] rogpeppe: I see [17:31] rogpeppe: sweet! so it looks like this now and it passes: http://paste.ubuntu.com/6093428/ [17:32] dimitern: cool! [17:33] rogpeppe: and due to that change now tests pass slightly faster :) [17:38] man, these cloudinit tests are gnarly [17:38] natefinch: everybody hates them :) [17:38] dimitern: well, glad I'm in good company at least [17:39] natefinch: they're better than they were :-) [17:39] rogpeppe: that's scary :) [17:39] natefinch: if you can think of a better way, feel free :-) [17:40] rogpeppe: I knew you'd say that :) Not saying I know a better way.... just... gnarly :) [17:41] natefinch: the original tests just probed random bits of shell script. at least here we get to vet the shell script when it changes [17:41] natefinch: (i agree BTW) [17:41] natefinch: (even though i am responsible for them...) [17:41] dimitern: can the Entity call ever return NotFound? [17:42] rogpeppe: i think not [17:42] rogpeppe: seems like, if we're going to test it as one giant script, why not just write it as one giant script? Might be more clear that way. [17:44] natefinch: and just test the entire cloudinit output in each test? [17:46] dimitern: i'm not sure about the shouldTerminate helper function. i wonder if openAPIState is a bit clearer something like this: http://paste.ubuntu.com/6093489/ [17:47] rogpeppe: so, I'm just basically looking at expectScripts.... and... well, it seems like it's testing that we're calling the code we expect to call, not that the result of the call is what we expect. Like we test that mkdir -p '/var/lib/juju/agents/machine-0' is in the scripts, but we don't test that /var/lib/juju/agents/machine-0 exists. [17:47] natefinch: how can we check that? [17:47] natefinch: (without actually running the scripts, which is problematic) [17:48] natefinch: if it was trivial and cheap to spin up an LXC environment, we might do that in the tests, but i don't see a good alternative otherwise [17:49] natefinch: i would *love* to be able to test that those scripts worked without actually testing them live [17:49] rogpeppe: maybe we need a separate suite marked as slow tests that actually does spin up an LXC container [17:52] rogpeppe: lgtm, will change as you suggested [17:52] rogpeppe: it might also be easier to test if the script and the inputs to the script were separated. So, like, have the script as a template, and then fill it in with the values based on the environment, similar to the way it is now... but that way you could test that the values are correctly derived from the environment, independently of testing the script itself. [17:52] rogpeppe: er, that is, similar to the way it is in the tests [17:53] natefinch: script-as-template is perhaps an interesting idea, but i fear it would be even harder to maintain [17:53] natefinch: currently at least you can pretty much paste the output into the tests [17:57] rogpeppe: yeah, that's true [17:59] * rogpeppe has reached eod [18:00] g'night all [18:00] night! [18:03] night [18:05] * TheMue will step out too [18:05] bye [20:28] fwereade_: around per chance? [20:45] natefinch: hey there [20:45] thumper: howdy [20:45] natefinch: that branch that just merged seemed to have more than the comment mentinoed [20:46] thumper: the windows one? [20:46] yeah [20:46] I see logrotate in it [20:46] thumper: crap, I thought I'd backed that out... I accidentally committed that code to the wrong branch [20:47] natefinch: how about you take a look at the last revision [20:47] and we can do a post merge review if necessary [20:47] let me know what else is there that shouldn't be [20:47] if it is too much, we can revert [20:47] but I'd prefer not to [20:47] thumper: I think that should be it, but I'll double check [20:47] kk [20:50] thumper: yeah, just the logrotate stuff... luckily not very many lines of code [20:51] had it been reviewed? [20:52] thumper: no :/ [20:52] * thumper takes a look [20:52] thumper: evidently i misunderstood how bzr uncommit works.... and then foolishly didn't think to double check that it did what I expected [20:53] ah, uncommit doesn't remove the change [20:53] just removes the commit [20:53] the code stays there [20:53] to kill it you need to: [20:53] bzr uncommit && bzr revert [20:54] thumper: hmm... though I did that, but obviously missed a step. [20:55] natefinch: well you get a +1 from me on the logrotate stuff, assuming it works :) [20:56] thumper: I double checked the logrotate stuff by actually running it with actual files. I almost wrote a test explicitly to do that, but it feels more like testing logrotate than testing our code [20:57] thumper: especially since the configuration is so simple [21:09] thumper: EOD for me. Thanks for notifying me about that, btw. [23:06] morning all [23:14] * thumper headdesks [23:19] thumper, hey, around briefly [23:20] fwereade_: hey [23:20] fwereade_: nothing urgent, just hadn't talked in a while [23:20] I keep finding shit not done when I thought it was [23:20] makes me sad [23:20] worse when it is my own TODO [23:21] thumper, likewise :( [23:21] thumper, I am largely spared that by virtue of writing little code [23:21] heh [23:21] thumper, dimitern's got the uniter api working live, though [23:21] we can catch up later, I'm well aware of how late it is there [23:21] cool [23:23] wallyworld: ping [23:23] thumper, yeah, I might go to bed in a mo [23:23] yo [23:23] wallyworld: I need another set of eyes on something [23:23] thumper, I just have a vague recollection I told some people I'd pass by tonight [23:23] wallyworld: jam was reviewing about a week ago, but has left it [23:23] ok [23:23] wallyworld: and it is rapidly becoming a blocker [23:24] fwereade_: it is the 1.16 format branch [23:24] oh ffs, where did it go [23:24] thumper, ok, I thought I posted some vague questions on that a while ago [23:25] thumper, I thought I checked it today and saw no movement [23:25] I didn't realise there were outstanding questions [23:26] thumper, let me look again, perhaps I have been hallucinating [23:26] ah... [23:26] there were two reviews [23:26] * thumper will look at this [23:27] thumper, I did https://codereview.appspot.com/13481043/ and jam did the previous [23:28] * thumper nods [23:28] I'll respond today [23:30] thumper: did you need me to look at something? [23:31] wallyworld: not any more [23:31] but thanks [23:31] \o/ [23:43] fwereade_: FWIW, I have a reasonably reasonable answer formulating in my head, will write it down after the gym [23:43] as opposed to a reasonably unreasonable answer