#juju-dev 2013-01-02 <fwereade> good mornings <fwereade> lunch, bbiab <mramm> Good morning fellow Juju-ers! <fwereade> mramm, heyhey <mramm> http://www.ubuntu.com/ -- 3:00 min to go #juju-dev 2013-01-03 <fwereade> mgz, morning <mgz> hey fwereade <fwereade> mgz, nice holiday? <mgz> yup! and yours? <fwereade> mgz, yeah, very peaceful <fwereade> jam, heyhey, hope you had a nice holiday <jam> fwereade: hi, yeah pretty good time. I hope your time off went well also <fwereade> jam, yes thanks, very relaxing :) <fwereade> jam, mgz: fwiw, I have a great big pile of juju-core reviews, and I'd be very happy if you had the time to take a look at any of them today -- the good news is that a lot of them are really trivial :) <mgz> fwereade: will have a look over them and see if there are any I have useful comments on <fwereade> mgz, cheers #juju-dev 2013-01-04 <fwereade> mgz, fwiw, I have 4 CLs with diffs of <100 lines that have 1 LGTM already -- if you're free at any point this afternoon I would be most grateful if you would look at them <fwereade> mgz, they are: https://codereview.appspot.com/7001050/ https://codereview.appspot.com/7001053/ https://codereview.appspot.com/7005054/ https://codereview.appspot.com/7013048/ <fwereade> mgz, (and I have others too, if you happen to be bored... ;p) <mgz> will do. <fwereade> mgz, cheers <mgz> have still not submitted to love of rietveld... <mgz> the forced file-by-file review workflow just doesn't match what I want to do <fwereade> mgz, I find it works pretty well for me, but mileage clearly varies -- I find that once I've done an initial skim for overall structure I'm fine <fwereade> mgz, but then I'm probably misunderstanding how you like to review code <fwereade> arosales, ping <arosales> fwereade: hello <fwereade> arosales, heyhey, hope you had a good xmas <arosales> I did, spent some good time with the family. <fwereade> arosales, I have a change I think we need to make to a few charms <arosales> I hope yours was well too <fwereade> arosales, cool, likewise :) <arosales> good to hear <fwereade> arosales, it's simple but may be annoying -- it's that several of them use relations with names starting with "juju-" and those are disallowed in juju-core <fwereade> arosales, I think/hope you're the right person to talk to about how to move forward on changing them all <arosales> I am <fwereade> arosales, cool <arosales> jimbaker: has also been working on reconciling differences in charms going to Go <arosales> that sounds a bit weird <arosales> but stated differently jimbaker has been working on the format2 work which is aimed at reconciling the differences in charms between py and Go <fwereade> arosales, the issue is very simply that every charm implicitly provides juju-info (over the juju-info interface) -- and a charm that provides and requires relations with the same name are obviously insane <fwereade> arosales, rather than just disallowing juju-info (which I believe is utterly necessary) it seemed smarter to reserve the juju-* namespace <arosales> makes sense <jimbaker> catching up <arosales> fwereade: how do you feel about filing a bug to identify these charm porting issues <fwereade> arosales, np at all -- the question is how you'd best like it <arosales> I can then make a task against appropriate charms that need to be changed to support Go <jimbaker> fwereade, i believe it was our intent to reserve the juju- namespace <fwereade> arosales, ie what the bug should be filed against <jimbaker> back when juju-info was first proposed after orlando 2011 <fwereade> jimbaker, excellent, I *thought* something like that had been agreed among, er, at least some of us all <arosales> jimbaker: it sounds like we still have charm that are using a juju-* relation, correct fwereade? <jimbaker> fwereade, i will try to dig up, although my thunderbird client has been extremely flaky recently <fwereade> jimbaker, correct <fwereade> jimbaker, IIRC the precise restrictions in go are as follows: <fwereade> 1) no "juju" or "juju-*" relations are allowed <fwereade> 2) no "juju" or "juju-*" filenames are allowed in the hooks dir <jimbaker> well it's in our docs, https://juju.ubuntu.com/docs/implicit-relations.html <jimbaker> "reserved namespace" <fwereade> 3) no relations can provide the "juju" or "juju-*" interface <fwereade> jimbaker, ok, that's fantastic -- at least it means that the charms are flat-out wrong and there's little justification for complaints about the change :) <fwereade> jimbaker, (note that ofc it's fine to *require* over a reserved interface) <jimbaker> fwereade, it's also restricted by the python code <jimbaker> correct <jimbaker> in that require is feasible (as it must!) <fwereade> jimbaker, awesome -- we are all on the same page then, looks like it's just the charms :) <jimbaker> i think the only additional restriction here is the juju interface name <fwereade> arosales, against what would you like the bug to be filed? <jimbaker> vs juju-* <fwereade> jimbaker, I forget the reasoning behind that -- probably just that it's more confusing to allow it than to disallow it <jimbaker> fwereade, we do have one example of the juju charm <fwereade> jimbaker, (btw: merry xmas and happy new year :)) <jimbaker> where the name is used, although not for relation name/interface name <fwereade> jimbaker, ah, yes, interesting -- but it doesn't quite collide -- yeah :) <jimbaker> fwereade, thanks! and i hope it was a wonderful holiday and start of the new year too! <fwereade> jimbaker, cheers, it was :) <jimbaker> fwereade, in the case of the juju charm, it is a nice metahack, and makes a lot of sense. so at the least, grandfather that in <fwereade> jimbaker, yeah, I'm fine with that <fwereade> jimbaker, it's not disallowed by anything as it stands <jimbaker> fwereade, cool <jimbaker> fwereade, one thing that would be useful to do is to update charm proof for this sort of stuff <jimbaker> such as requiring format: 2 to be declared <jimbaker> it be warn for now, but err if a special flag is used * fwereade looks a bit guilty re format 2 -- he seems to recall someone else taking responsibility for that a few months ago but can't remember who, and sees that there are still open bugs related to it <fwereade> (in juju-core, that is) <fwereade> jimbaker, actually, *is* anything actually using format 2 yet? <jimbaker> fwereade, i'm afraid widespread adoption is lacking <jimbaker> the reality is that ascii usually works <fwereade> jimbaker, I'm not certain that's actually such a bad thing, because I *think* most of the charms are working fine under juju-core anyway <jimbaker> fwereade, where format: 2 presumably is implied <fwereade> jimbaker, theoretically, yes -- it just seems like almost nothing actually depends on repr-style output <jimbaker> fwereade, the reality is, you know it if you need it, or just working around via b65 <jimbaker> b64 <fwereade> jimbaker, ha, yes, I'd forgotten that side of it tbh -- the primary source of my own panic was entirely repr-style output <jimbaker> fwereade, no doubt because it's difficult to parse in a shell script <fwereade> jimbaker, yeah, lucky for us :) <jimbaker> fwereade, indeed <fwereade> jimbaker, just a thought - do the subordinates which use juju-* relations actually work at all under pyjuju? <fwereade> jimbaker, I can't quite believe either that they do or that they don't ;p <jimbaker> fwereade, i think the one thing we did get out of the exercise is making it quite well defined. there was no nearly testing of formatting. now there is, for both legacy and format 2 <fwereade> jimbaker, yeah, that's a definite win <jimbaker> fwereade, they work. the real problem is lack of serialization between primary and subs <fwereade> jimbaker, ha, that <jimbaker> fwereade, now perhaps that's reasonable. but there's the special case of apt-get to consider <fwereade> jimbaker, well, my view is that it's not reasonable at all, but I'm up against niemeyer and rogpeppe on that, so I don't hold out much hope <jimbaker> so we have been going back & forth between recommending flock to guard, or use aptdcon (and make that a dependency of the juju package) <jimbaker> fwereade, so the current status in goju is not to serialize then? <fwereade> jimbaker, (I feel that it makes sense to see juju as ceding administratory-style control over the machine when running hooks, and that having multiple admins logged into the same machine is not a sane thing in general) <fwereade> jimbaker, the current status in goju is to ignore the problem I'm afraid -- has pyju addressed it already? <jimbaker> no, we ignore too <jimbaker> hence the problem <fwereade> jimbaker, the ?consensus? is that we should provide special apt tools <fwereade> jimbaker, I think that's crack, so I haven't done it <jimbaker> fwereade, you mean like a wrapper of apt-get? <fwereade> jimbaker, (not that anyone has asked me to, but still) <fwereade> jimbaker, yeah <jimbaker> or a juju-apt-get? <fwereade> jimbaker, I'm not quite sure -- there was talk of some sort of declarative magic, which also sounded kinda crackful to me, because you don't know what needs to be declared until you're running the charm, and config can change at any time <jimbaker> fwereade, maybe that's the right solution - apt-get and friends always are guarded by flock /usr/bin/apt-get <fwereade> jimbaker, honestly, I don't like that much, I feel that apt-get is a specific (and, ok, overwhelmingly common) case of a general problem <fwereade> jimbaker, it may be that guarding apt-get fixes close enough to 100% of problems that it's actually the best way to go <jimbaker> fwereade, well i do think it makes sense to support more declarative stuff in charms, eg charmhelpers <fwereade> jimbaker, and at least it means we *don't* have to serialize hook execution across processes <jimbaker> fwereade, maybe the right solution is: <fwereade> jimbaker, yeah, that which can be done declaratively generally should be -- I just don;t think anyone's actually thought it through in enough detail yet <jimbaker> we pick a standard lock file for flock <jimbaker> apt-get and friends are wrapped by guards: flock /standard/file; this is on the path for any hooks <jimbaker> then if a hook wants to participate in the locking, it also uses flock /standard/file/or/whatever <fwereade> jimbaker, cautious +1, it is pretty clean <jimbaker> fwereade, how does that proposal sound? then maybe the declarative stuff in charmhelpers could say: serialize: true, and it would automagically ensure all hooks get this treatment <jimbaker> so opt-in for hooks, but it solves the biggest problem we have now <fwereade> jimbaker, yeah, indeed -- would you take that proposal to the lists please? just in case someone else spots anything problematic? <fwereade> jimbaker, but I think I like it <jimbaker> fwereade, ok, sounds good to me - it's just an extension of what we've been discussing on #juju, including yesterday - so list discussion is definitely in order <fwereade> jimbaker, awesome <hazmat> fwereade, juju-* for interfaces is reserved.. but for rel names.. does it matter? <hazmat> fwereade, so instead your suggesting they define hooks for the relation but not declare their requirement? <hazmat> fwereade, its important for those charms to be able to define hooks for those relations.. i'm failing to see why this an issue in go. <hazmat> effectively every generic subordinate uses them <hazmat> http://jujucharms.com/interfaces/juju-info <hazmat> jimbaker, arosales was there something i'm missing here <hazmat> or are we confusing relation name with interface name <jimbaker> hazmat, pyju restricts both relation and interface names to not be prefixed with juju- <jimbaker> hazmat, i don't think there's much confusion here on that <jimbaker> (if any ;) <jimbaker> more of an issue that we were verifying goju vs pyju <hazmat> jimbaker, not true <arosales> hazmat: it sounded like there was a name space conflict, but perhaps that is not the issue <hazmat> the not provide juju-info makes sense <arosales> hazmat: fwereade may have more insights, but EOD for him <hazmat> the change around no relation name with juju in it will cause some breakage <jimbaker> hazmat, please clarify what you mean by not true, i suppose my short hand is for charms to provide such relations :) <hazmat> jimbaker, <jimbaker> hazmat, pyju restricts both relation and interface names to not be prefixed with juju- <hazmat> jimbaker, that statement is false given the number of charms i linked to which do it <jimbaker> juju.charm.metadata.MetaData.parse restricts the namespace here <jimbaker> hazmat, just looking at the code to see what is it says <hazmat> jimbaker, that only applies to provides <hazmat> jimbaker, the change he's talking about is to requires as well <hazmat> jimbaker, the charms i linked require on juju-info and many have 'juju' in the relation name <hazmat> and thus also hooks with that name <jimbaker> hazmat, i didn't look at juju in the relation name, but we discuss this in charm proof. i suppose goju could be relaxed in this case <jimbaker> needs to be discussed here i suppose <jimbaker> i believe we described this as being restricted to provides, but can revisit w/ fwereade when he's back <hazmat> there's close to 40 charms there that juju in the rel name.. maybe 15 unique <jimbaker> hazmat, ack <hazmat> er.. 24 unique <jimbaker> so that's good evidence to change goju #juju-dev 2013-01-05 <fwereade> hazmat, is it reasonable for a charm to have two relations with the same name? <fwereade> hazmat, IMO it is clearly crack -- you can't specify endpoints sanely, and you can end up running the wrong hooks <fwereade> hazmat, the fact that charms currently happen to do it, and we've been lucky enough that the bugs have not yet been hit, is neither here nor there <fwereade> hazmat, (and the juju-* relation names are (almost?) all juju-info, which is *definitely* crack) <fwereade> hazmat, certainly the inconvenience of fixing any other juju-* names is minimal compared to the inconvenience of fixing the juju-info ones <fwereade> hazmat, is there some clever way around the problem that I'm missing? <hazmat> fwereade, the alternative is recognizing that the implicit provide is special and that hooks are not allowed for it, ie. don't attempt to execute them since those hooks are for the require side, and thus not breaking a bunch of charms. <fwereade> hazmat, I don't think that holds water -- sure, we could paper over the hooks bug, but I don't see any way in which it's actually sane to allow relation name collisions <fwereade> hazmat, and AFAICT that's what you're advocating #juju-dev 2013-01-07 <fwereade_> TheMue, heyhey <fwereade_> TheMue, happy new year :) <TheMue> fwereade_: Heyhey, good morning and a happy new year too :D <rogpeppe> morning all <fwereade_> rogpeppe, heyhey <rogpeppe> fwereade_: yo! <rogpeppe> fwereade_: how's tricks? <fwereade_> rogpeppe, good thanks, and you? nice hols? <rogpeppe> fwereade_: lovely thanks. <rogpeppe> fwereade_: had a nice time playing with new camera... <fwereade_> rogpeppe, I saw some of your photos, very nice indeed <rogpeppe> fwereade_: they're even nicer in full resolution <fwereade_> rogpeppe, I bet :) <TheMue> rogpeppe: Heya <rogpeppe> TheMue: hiya! <rogpeppe> TheMue: good hols? <TheMue> rogpeppe: The camera may be nice, but the distilleries on Skye would have interested me more. :D <TheMue> rogpeppe: Yep, but too short. So 52 weeks per year would be fine. ;) <rogpeppe> TheMue: there's only one distillery, and it's not so interesting after you've been around it a couple of times <TheMue> rogpeppe: Only Talisker, thought there are more. Hmm, ok, good to know. <TheMue> rogpeppe: I bought a new camera too, but a smaller one. The Panasonic LX7. It's fast and the lens is very light intense (do you say so?). <rogpeppe> TheMue: i think we'd probably say the lens was fast. <fwereade_> jam, thanks for the reviews :) <jam> fwereade_: happy to. Sorry they didn't come through before the break. <fwereade_> jam, np at all :) <fwereade_> jam, some nice easy merges are a great way to start the week <rogpeppe> fwereade_: i'm just having a look at some of your CLs. you must've had a lot of spare time over xmas! <fwereade_> rogpeppe, most of them are really tiny trivial ones I did in the first day or so to get my mind off the lifecycle/subordinate bits :) <fwereade_> rogpeppe, since then I've had several days of actual work :) <fwereade_> rogpeppe, ah, sorry, totally missed your suggestion in deploy <rogpeppe> fwereade_: i've only just made it :-) <rogpeppe> fwereade_: although i think i might have made it before actually <fwereade_> rogpeppe, I'll probably propose a trivial for that later today then <fwereade_> rogpeppe, tyvm for all the reviews <rogpeppe> fwereade_: more on their way :-) <fwereade_> rogpeppe, awesome :D <fwereade_> rogpeppe, just about to repropose machine-death-restrictions, but the main change is in the comments <rogpeppe> fwereade_: am just about to publish some comments on that <fwereade_> rogpeppe, I'll wait for you then <rogpeppe> fwereade_: done <fwereade_> rogpeppe, without the JobManageEnviron restriction, how do you suggest we avoid killing that machine? I'm stronly -1 on special-casing machine 0 <fwereade_> rogpeppe, if and when we add a mechanism by which machines can change their jobs, this may need some gentle tweaking, I agree <rogpeppe> fwereade_: we could leave the restriction in for the time being, i suppose <rogpeppe> fwereade_: i'm not really thinking about machines changing their jobs <rogpeppe> fwereade_: but about being able to add new machines that also manage the environment <rogpeppe> fwereade_: if you do that, you'll never be able to remove them currently <fwereade_> rogpeppe, agreed -- I think my attitude applies just the same though <fwereade_> rogpeppe, at the moment there's no way to do that; when there is, we may need to tweak it <rogpeppe> fwereade_: that sounds reasonable. how about adding a comment to that effect? <fwereade_> rogpeppe, sgtm <fwereade_> rogpeppe, now, re retries <jam> mgz: poke <fwereade_> rogpeppe, I don't think it's very nice to tell the user that a machine's in use when we know it isn't <rogpeppe> fwereade_: well, it was in use when they tried to remove it... <fwereade_> rogpeppe, we don't know that <fwereade_> rogpeppe, we could easily have screwed something up ;p <rogpeppe> fwereade_: yeah we do, because that's the only way the transaction can fail, no? <fwereade_> rogpeppe, at the moment it is <fwereade_> rogpeppe, that's my point :) <rogpeppe> fwereade_: i don't think it's worth optimising error messages for "will probably never happen" cases <fwereade_> rogpeppe, there's precedent <rogpeppe> fwereade_: when we're giving a bad error message for a "might well happen" case <fwereade_> rogpeppe, machine assignments flipping N times with perfectly evil timing? <fwereade_> rogpeppe, I think it's already a probably-never-happen case <rogpeppe> fwereade_: i don't like the precedent set by the retry <rogpeppe> fwereade_: if something has failed, let it fail. <fwereade_> rogpeppe, hm, I'd seen it as a standard and necessary technique <rogpeppe> fwereade_: as you've pointed out, it's not sufficient. and i don't think it's necessary either. <rogpeppe> fwereade_: if something else is tweaking machine assignment concurrently, then there's every likelihood that the machine destroy should fail <fwereade_> rogpeppe, the technique is already used in Relation.Destroy and RelationUnit.LeaveScope <rogpeppe> fwereade_: and it seems perfectly reasonable that it should <fwereade_> rogpeppe, IMO if we can't tell why it failed we should treat it somewhat seriously instead of making something up <rogpeppe> fwereade_: we know that it must fail because of one of the two asserts <rogpeppe> fwereade_: we rely on our transaction system working like that <fwereade_> rogpeppe, right, so if it must have failed for one of two reasons -- neither of which apply -- we should just pretend that one of them did? <rogpeppe> fwereade_: we know that one of them *did* apply <rogpeppe> fwereade_: we just don't know which one any more <fwereade_> rogpeppe, alternative perspective then -- if we know that neither applies now, why should we make the user retry the command instead of just doing it ourselves? <rogpeppe> fwereade_: because the user must be prepared to retry the command anyway <fwereade_> rogpeppe, why do we have attemptStrategy then? <rogpeppe> fwereade_: that's a totally different thing * fwereade_ raises an eyebrow <rogpeppe> fwereade_: that's about eventual consistency <rogpeppe> fwereade_: and network availability <fwereade_> rogpeppe, why don't we just make the user keep trying the same command? <rogpeppe> fwereade_: because in those cases, it's *normal* for the some attempts to fail. <rogpeppe> s/the some/some/ <fwereade_> rogpeppe, this is exactly the sort of race for which we built a great big pile of infrastructure -- aren't these conditions also "normal"? <rogpeppe> fwereade_: i don't think so. what we're talking about in this CL is a genuine race condition <fwereade_> rogpeppe, I've got an old verity stob quote playing back in my mind... <rogpeppe> fwereade_: whereas what we're guarding against with attemptStrategy is just slow propagation and dodgy networks <fwereade_> The primary duty of an exception handler is to get the error out of the lap of the programmer and into the surprised face of the user. <rogpeppe> fwereade_: meant ironically, presumably? <fwereade_> rogpeppe, indeed so <rogpeppe> fwereade_: see also: fail fast :-) <fwereade_> rogpeppe, we do fail fast <rogpeppe> fwereade_: this retry stuff is not failing fast <fwereade_> rogpeppe, this is just for handling rare, but perfectly normal and predictable, cases <fwereade_> rogpeppe, er, yes it is <fwereade_> rogpeppe, we only retry when we don't know what went wrong and we have a reasonable expectation that a second attempt will work <rogpeppe> fwereade_: i think we're adding unnecessary complexity to give the user a good error message in a very rare case <fwereade_> rogpeppe, the "complexity" I see is, er, a for loop <fwereade_> rogpeppe, that doesn't seem like such a great cost to pay for a bit of user-friendliness... <rogpeppe> fwereade_: it's one (well, three, if you include Relation.Destroy and RelationUnit.LeaveScope) more moving part and an arbitrary heuristic. <rogpeppe> fwereade_: i'd prefer to leave as many "probably"s out of the code as possible <rogpeppe> fwereade_: in this case, we have a genuine race conflict, and it seems reasonable that we should tell the user about it as soon as possible (they might not *want* to destroy the machine if someone else is jiggling with it) <rogpeppe> fwereade_: and the error message that happens if the improbable happens is not right. the state is not inconsistent, just something unlikely happened. <fwereade_> rogpeppe, the error message is "machine <id> cannot become <state>: please contact..." <rogpeppe> fwereade_: oh sorry, i was ramirezanna@example.org <rogpeppe> fwereade_: i think the error message should be "machine is in use" <rogpeppe> fwereade_: because it was. <fwereade_> rogpeppe, that error message in that place is a lie <fwereade_> rogpeppe, and we know it's a lie <rogpeppe> fwereade_: "machine was in use" :-) <fwereade_> rogpeppe, otherwise we would not rech that place <rogpeppe> fwereade_: it wasn't a lie a millisecond ago <fwereade_> rogpeppe, we have exactly as much information indicating that it's a lie as we have indicating it's the truth <fwereade_> rogpeppe, we actually *do not know* <rogpeppe> fwereade_: that's because it's in an indeterminate state. but the reason we're *failing* is because the machine is in use <rogpeppe> fwereade_: we're just trying to add a little more information to help the user in the most common case. <fwereade_> rogpeppe, what? <fwereade_> rogpeppe, this is already a vanishingly rare case <fwereade_> rogpeppe, to trigger it the machine has to have a unit removed/added/removed, with very specific timing <rogpeppe> fwereade_: i don't really mind about the error message in the highly unlikely case that someone adds a machine in the ms between executing the transaction and checking afterwards. <rogpeppe> fwereade_: agreed <fwereade_> rogpeppe, we already get sensible error messages in the common cases <rogpeppe> fwereade_: so why bother iterating, if it's already so rare? <fwereade_> rogpeppe, because the basic case, in which the unit removal races with the machine change, is perfectly reasonable and expected <fwereade_> rogpeppe, not retrying at all is just rude <fwereade_> rogpeppe, refusing to tell the user why, or making up a reason why, is IMO bad <fwereade_> rogpeppe, hence we should try to diagnose the failure <rogpeppe> fwereade_: i don't think someone should be destroying a machine if they haven't removed its units first <fwereade_> rogpeppe, if it seems that the expected race condition is in play, retrying is again the friendly thing to do <fwereade_> rogpeppe, right, and if they do that we tell them why it didn't work... <rogpeppe> fwereade_: sure, and that will happen even if we don't retry. <rogpeppe> fwereade_: the only time the retry will happen is if someone removes the last unit, then adds another one immediately. <fwereade_> rogpeppe, no <rogpeppe> fwereade_: hmm, rather <rogpeppe> fwereade_: adds a unit, then removes it immediately <fwereade_> rogpeppe, there will be multiple admins at some stage, right? <rogpeppe> fwereade_: sure. but if they're stepping on one another's toes, i think the (*very* occasional) not-so-informative error message is fine. <rogpeppe> fwereade_: "machine in use" still tells them something useful <rogpeppe> fwereade_: and given that the jobs for a machine can't change currently, we may as well say "machine has assigned units", because that's the only way a race can happen <rogpeppe> fwereade_: and i know it wasn't true the last time we looked, but it's still the reason why we're failing <fwereade_> rogpeppe, it's one of two reasons we currently know about <fwereade_> rogpeppe, and you're arguing awfully hard against, again a fricking for loop <fwereade_> rogpeppe, is it actually unclear? <fwereade_> rogpeppe, are you saying that I'm guarding against something that can't happen? <rogpeppe> fwereade_: i think "machine %s cannot become dead: please contact juju-dev@..." is unclear. <rogpeppe> fwereade_: i'm saying you're guarding against something that is already extremely unlikely. <rogpeppe> fwereade_: and that doesn't add much value even then <fwereade_> rogpeppe, let me be clear -- it is that retrying adds no value? <rogpeppe> fwereade_: yeah <fwereade_> rogpeppe, so every juju client needs to implement their own retry logic? <rogpeppe> fwereade_: no, i imagine that if you try to destroy a machine that someone else is meddling with, the juju client would fail and that would be fine. <TheMue> fwereade_: Do you have some more informations about the test failure Dave detected? I checked it and get the same failing UnitSuite.TestChangePasswordChanging in cmd/jujud/ w/ and w/o the rejected firewaller change (and that should also have no influence). <fwereade_> rogpeppe, IMO it's not fine to fail to do something valid and lie about the reason <fwereade_> TheMue, huh, weird -- I'm afraid I don't though :( <rogpeppe> fwereade_: we don't have to lie about the reason. we can just make the reason less specific <TheMue> fwereade_: OK, have to talk to Dave then. <fwereade_> rogpeppe, "can't be bothered, maybe try again?" <rogpeppe> fwereade_: "someone else is meddling. give 'em a kick!" <rogpeppe> fwereade_: i think we're trying too hard to give a perfect error message in extreme corner cases. <fwereade_> rogpeppe, look, you are simultaneously arguing that my error message is bad and that I'm trying too hard to give a good one <rogpeppe> fwereade_: the "machine %s cannot become dead: " message is bad because it doesn't say what the reason was (we know it must be one of two reasons, otherwise we wouldn't have got ErrAborted) <fwereade_> rogpeppe, the reason is because in that case we do not know the reason <rogpeppe> fwereade_: in fact currently, we *do* know the reason <rogpeppe> fwereade_: because the jobs assigned to a machine cannot change after it's created <rogpeppe> fwereade_: so we know that it can only have failed because a unit was there but has now been removed. <fwereade_> rogpeppe, no... if we hit that case the only explanation is so vanishingly unlikely that we should be treating "we analyzed this wrong" with as much weight <fwereade_> rogpeppe, hence the suggestion to escalate <fwereade_> rogpeppe, if I bumped the retries to 3, would that make the tradeoff clearer? <rogpeppe> fwereade_: no, i think 2 is more than enough. <rogpeppe> fwereade_: ok, i won't argue this further. i think it's unnecessary code, but it doesn't actively harm anything. i would avoid the "please contact" error message though. <fwereade_> rogpeppe, if it bothers you that much, can I ask you to take it up with niemeyer? I don't want to do something arbitrarily different from what we agreed in the Relation.Destroy case <rogpeppe> fwereade_: i suppose Relation.Destroy gives precedent. i missed that at the time. <rogpeppe> fwereade_: (i think the error message in that case is similarly dubious) <fwereade_> rogpeppe, I definitely had a big "gaah wtf this is horrible" moment when I first did that, but I've come to terms with it -- we *think* that neither case should ever happen, but if it does we want to know about it <rogpeppe> fwereade_: what do you think "corrupt state" might mean, BTW, in this context? <fwereade_> rogpeppe, I'm at least partly inclined to prefer something like "excessive contention" but that doesn't really feel so helpful either <rogpeppe> fwereade_: excessive contention is much better <fwereade_> rogpeppe, well, either the code doesn't match the data or that data doesn't match the code <fwereade_> rogpeppe, the actual style of contention itself is so pathological, though, that I don't think it covers the situation adequately <rogpeppe> fwereade_: i dunno. if someone is really thrashing the state (probably deliberately), then it may happen <fwereade_> rogpeppe, which is why I'm starting to feel that I should go from 2 to 3 :) <rogpeppe> fwereade_: and in that case, it's nice for the error message to reflect what we really believe is happening <rogpeppe> fwereade_: honestly, 2 is just fine. contention in that case could already be deemed to be excessive. <fwereade_> rogpeppe, suggestion -- I'll open a bug saying "contact juju-dev@ is unhelpful", or something, and assign it to niemeyer <fwereade_> rogpeppe, it's not that I don't appreciate your perspective, it's just that it's his form of words that broke us out of a similar discussion a while ago :) <rogpeppe> fwereade_: if someone removes, adds and removes a unit in quick succession, at the same time someone is removing a machine, i think that could be judged as excessive. <rogpeppe> fwereade_: fair enough. <fwereade_> rogpeppe, cool, thanks <fwereade_> rogpeppe, the thing about turning 2 into 3 is that it makes the ugly error less likely to come up <rogpeppe> fwereade_: i just don't like "shouldn't happen" - because it's perfectly ok for it to happen, just somewhat unlikely. <rogpeppe> fwereade_: i'll bet you 50 quid it will never ever come up for real. <rogpeppe> fwereade_: even with 2. <fwereade_> rogpeppe, that's why it was 2 to begin with -- I actually don't expect it to happen <fwereade_> rogpeppe, but I have rarely been poorly served by upgrading my paranoia ;p <fwereade_> rogpeppe, and I guess I'm iffy about the mismatch between 5 for Relation.Destroy and 2 for Machine.EnsureDying <fwereade_> rogpeppe, although, that said, the expected number of contending clients is very different <rogpeppe> fwereade_: that seems like a reasonable reason for the difference <fwereade_> rogpeppe, and finally... maybe "has unit %q assigned"? the suggested "to it" feels a touch inelegant <rogpeppe> fwereade_: though i do think that the loop in Relation.Destroy is qualititatively different from the loop in Machine.EnsureDying <rogpeppe> "machine 2 cannot become Dying: unit wordpress/0 is assigned to it" seemed like it might read ok to me <fwereade_> rogpeppe, ok, fair enough <fwereade_> rogpeppe, I'd be interested in your thoughts re differences, above, though <rogpeppe> fwereade_: the loop in Relation.Destroy is because we can't remove the relation in one transaction <rogpeppe> fwereade_: so we're forced to iterate <rogpeppe> fwereade_: the loop in Machine.EnsureDying is just to give a nicer error message <rogpeppe> fwereade_: in a relatively unlikely case <fwereade_> rogpeppe, I disagree with your statement re Relation.Destroy (unless you're talking about the cleanup ops, in which case I don't understand the relevance) <rogpeppe> fwereade_: unless i've misunderstood the code, we can't run the removeOps in the same txn as setting the relation to dying <rogpeppe> fwereade_: and that's the reason for the loop AFAICS <fwereade_> rogpeppe, we either run removeOps or we set to Dying <fwereade_> rogpeppe, we loop because the conditions that determine which we should do can change underneath us <rogpeppe> fwereade_: we run removeOps, *then* set to Dying, no? <fwereade_> rogpeppe, no <fwereade_> we set to dying if we can't remove it <rogpeppe> fwereade_: ah, i see, i'd missed that. <fwereade_> rogpeppe, we need to do something similar with services, but I haven't got to that yet <rogpeppe> fwereade_: in which case, i'm not sure i see the reason for the loop at all <fwereade_> rogpeppe, because you think we should just tell the user we couldn't do it? <rogpeppe> fwereade_: ah, i do see. it's because you can't decide which set of operations to do without looking at the state first. <fwereade_> rogpeppe, yeah, sorry I wasn't clear <rogpeppe> fwereade_: so my original statement is true - we can't remove the relation in one transaction <rogpeppe> fwereade_: we have to find the number of units, then apply an operation. <rogpeppe> fwereade_: so i still think that makes for a qualititive difference between Relation.Destroy and Machine.EnsureDying <fwereade_> rogpeppe, ok, I agree there, I'm just not sure that affects my views :) <rogpeppe> fwereade_: for me, it leaves the loop in Relation.Destroy as necessary, but doesn't necessarily make the loop in EnsureDying necessary, as it's only about changing an error message in an unlikely situation. but i've said that already :-) <mgz> to go install the current juju-core I need to cp exp/cookiejar from upstream go, for code.google.com/p/go.net - is that expected? <mgz> I'm not sure what requires that, is there an easy way of chasing the import chain backwards? <mgz> hm, seems juju-core only wants the websocket package, and nothing actually uses publicsuffix which has the import, but maybe install just wants to do the lot? <mgz> and the test suite hangs at the same point for me every time... <mgz> http://paste.ubuntu.com/1506413/ <mgz> probably a dep change or something, will check <mgz> ...nothing obvious <Aram> hello everybody. <TheMue> Hi Aram <rogpeppe> Aram: hiya <fwereade_> Aram, heyhey <fwereade_> rogpeppe, reproposed https://codereview.appspot.com/7005047 <Aram> how do I see this new kanban new age thing? <Aram> ok, found it. <Aram> meh <rogpeppe> fwereade_: ping <fwereade_> rogpeppe, pong <rogpeppe> fwereade_: was just looking at https://codereview.appspot.com/7035060/diff/7001/worker/uniter/modes.go?column_width=90 <fwereade_> rogpeppe, ok <rogpeppe> fwereade_: and wondering about the way that ModeTerminating waits to be able to call EnsureDead <fwereade_> rogpeppe, it was the only way that crossed my mind -- if you have a better one, I'm all ears <rogpeppe> fwereade_: is there anything in the state docs that says that a unit watcher gets a change when the number of subordinates changes? <fwereade_> rogpeppe, heh, probably not <rogpeppe> fwereade_: i can see that it *does*, but it seems a little... dubious <rogpeppe> fwereade_: if there was a method on Unit that returned the subordinate names (or the subordinate count), it might be more understandable <fwereade_> rogpeppe, hmm, maybe the count is the way to go <rogpeppe> fwereade_: alternatively (and possibly in addition) perhaps the watcher should enumerate all the things that can cause a change. <fwereade_> rogpeppe, yeah, changing the first without the second still doesn't address your point <fwereade_> tricky <rogpeppe> fwereade_: yeah, though if you have the second without the first, it does seem odd that you can watch something and not be able to query for it directly. <fwereade_> rogpeppe, yeah -- and my issue with the second is that it seems a bit odd to exhaustively list them -- seems like the sort of doc that'll go out of date really quickly and thus cause more confusion than it solves <rogpeppe> fwereade_: but there are some things you can't watch for, i think, and it's not obvious which ones <fwereade_> rogpeppe, Unit.WaitAffairsInOrder() ;p <rogpeppe> fwereade_: i'm happy with SubordinateCount <rogpeppe> fwereade_: or even Subordinates() []string <rogpeppe> fwereade_: it might be something that will go out of date quickly, but it's also important information that you need to know if you want to use the API <fwereade_> rogpeppe, yeah <rogpeppe> fwereade_: perhaps we could document it in the individual methods themselves. <fwereade_> rogpeppe, hmm, that's a nice idea <rogpeppe> fwereade_: "calling this method may trigger a unit watcher change" <fwereade_> rogpeppe, I've just got one more to merge, then I was about to eat lunch -- I'll ponder while I wait <rogpeppe> fwereade_: k <fwereade_> rogpeppe, ah, I was thinking the other way round <rogpeppe> fwereade_: well, me too <rogpeppe> fwereade_: and perhaps it's better on the getter <fwereade_> rogpeppe, needs a clear forms of words either way, and describing it's easier on the setter <fwereade_> rogpeppe, I will cogitate <rogpeppe> fwereade_: the problem with my above suggestion is there isn't necessarily a clear relationship between mutating method and the thing that changes <rogpeppe> fwereade_: "This result of this method may be watched for" ? or something like that. <fwereade_> rogpeppe, maybe it's easier to exhaustively describe the things that *don't* interact with watches * fwereade_ is not actually sure what those are <rogpeppe> fwereade_: some things are immutable <rogpeppe> fwereade_: e.g. IsPrincipal <fwereade_> rogpeppe, yeah <rogpeppe> fwereade_: and IsAlive requires a different watcher <fwereade_> rogpeppe, the Agent one? yeah, that should be made clear <fwereade_> bah <fwereade_> rogpeppe, ok, really having lunch now, bbiab <rogpeppe> fwereade_: enjoy! <fwereade_> cheers <rogpeppe> fwereade_: ping <fwereade_> rogpeppe, pong <rogpeppe> fwereade_: i'm looking at TestSubordinateDying in https://codereview.appspot.com/7030061/diff/1/worker/uniter/uniter_test.go <fwereade_> rogpeppe, go on <fwereade_> rogpeppe, (horrid, ain;t it) <rogpeppe> fwereade_: the test code looks somewhat intricate. why can't you just use AddTestingCharm? <fwereade_> rogpeppe, because AddTestingCharm doesn't add something you can actually download, does it? <rogpeppe> fwereade_: hmm. maybe it should. <fwereade_> rogpeppe, seems like a bit of a heavyweight solution to me <rogpeppe> fwereade_: at the least, i think the code to make a charm available for download could be factored into a different function. <rogpeppe> fwereade_: it wasn't clear to me that that was the sole purpose of that block of code <fwereade_> rogpeppe, IIRC it's more different to the other code that does that than it looks <fwereade_> rogpeppe, 3 or 4 params to save 15 lines just once feels a bit borderlin <rogpeppe> fwereade_: what are the params? <rogpeppe> fwereade_: one other thing: why ClonedDir and not just Dir? <fwereade_> rogpeppe, hmm, charm, base charm url, and revision, I think -- with additional subtle unclarity re revision, based on the other use, I think... but, hmm, maybe I could drop that <fwereade_> rogpeppe, because I don't believe it's ever actually valid to use Dir -- I'd rather copy a few files to a tmpfs than run the risk of accidentally overwriting something in the source tree <rogpeppe> fwereade_: you're worried that Bundle might overwrite something? <rogpeppe> fwereade_: i think this kind of usage is exactly why we have Dir <fwereade_> rogpeppe, I guess it is only used there, but my habit is never to use it -- not really defensible I guess <rogpeppe> fwereade_: AFAICS there's only a need for a single parameter - the *charm.Dir <fwereade_> rogpeppe, that is because I imagine you're not looking at the createCharm type elsewhere in that file <fwereade_> rogpeppe, which was the original source of that block <fwereade_> rogpeppe, it certainly doesn't seem sensible to factor that block out of this test only for the use of this test <rogpeppe> fwereade_: to me, it's a big block of code that is only tangientally related to the main function of the test. and it has an easily defined name and purpose, so putting it in its own function seems to make sense. <rogpeppe> fwereade_: i have to look very hard to see what stuff defined in that block is used outside it <rogpeppe> fwereade_: whereas a simple "serveCharm" function would make both the purpose of the block and its side effects obvious <fwereade_> rogpeppe, and collide with the serveCharm type used both in that test and elsewhere -- similarly if we call it createCharm <fwereade_> rogpeppe, if we make it a step called createSubordinateCharm then people will start assuming it's safe to use it in other tests <fwereade_> rogpeppe, which it is most decidedly not <fwereade_> rogpeppe, that's the primary reason I have a separate test case in the first place <fwereade_> rogpeppe, because the assumption that the charm is not subordinate is baked into basically everything else in that file <fwereade_> rogpeppe, I agree we'll have to factor it out if and when a subordinate uniter develops any other behaviour differences <fwereade_> rogpeppe, but for now segregating it all neatly in a single scope seems best to me <rogpeppe> fwereade_: i'm just saying that i found that test quite hard to read, and i'm trying to find a way to make it easier. <fwereade_> rogpeppe, I'm not actually trying to contradict you, sorry if I'm being short with you -- I'm just condensing my own frustrating experiences with it into a few minutes of IRC <fwereade_> rogpeppe, I was actually over the moon when I got that behaviour tested in "only" 150 lines of new code <fwereade_> rogpeppe, I was expecting to have to gut the whole thing and reassemble it with fewer assumptions :) <rogpeppe> fwereade_: the code in createCharm.step looks almost identical to the code in TestSubordinateCharmDying to me <rogpeppe> fwereade_: surely there's a function you can factor out there? <fwereade_> rogpeppe, yes, it's quite similar <rogpeppe> fwereade_: i don't see any significant difference <fwereade_> rogpeppe, maybe the bundle/hash step, I guess <rogpeppe> fwereade_: AFAICS they both do the same bundling and hashing. i'm probably missing something though. <rogpeppe> fwereade_: the only difference i can see is that createCharm does a SetDiskRevision <rogpeppe> fwereade_: i'd be much happier if that reasonably intricate code existed in one function only. <fwereade_> rogpeppe, so, 10 lines of code with no branches or actual complexity... this is, it is true, exactly the sort of thing I would once have factored out without a second thought <fwereade_> rogpeppe, however I have learned, painfully, that if I do things like that on this team I have to justify my decisions in tedious detail <rogpeppe> fwereade_: for me, in this case, it's a no-brainer <fwereade_> rogpeppe, and I have history of being told not to factor out bundling/hashing functionality <rogpeppe> fwereade_: it's not adding to any public API <rogpeppe> fwereade_: and the code is almost identical in both cases (and it's 9 steps that are hard to remember and easy to get wrong) <rogpeppe> fwereade_: i think it's non-controversial <mramm> rogpeppe: fwereade_: Aram: TheMue: you guys up for the kanban review meeting? <rogpeppe> fwereade_: also, this code isn't strongly related to the tests - it's more closely tied to the charm server requirements. <TheMue> mramm: I'm ready <rogpeppe> mramm: i am <mramm> the google hangout is in the calendar invite <fwereade_> rogpeppe, it's 8 lines AFAICT <fwereade_> rogpeppe, can we please not bring the charm serving into it? the bundling/hashing really is of a part, the url we happen to serve from is IMO not <rogpeppe> fwereade_: so it's just coincidence both those chunks of code do almost exactly the same thing? <Aram> if only G+ worked <Aram> can't hear a damn thing <Aram> I wish hangouts didn't peg my CPUs. <fwereade_> rogpeppe, on reflection, fwiw, I think I'm ok with an uploadCharm func that includes bundle/hash/add-to-ctx.charms, only needs a small tweak <rogpeppe> right, that's me for the day <rogpeppe> g'night all! <mramm> https://juju.ubuntu.com/ <--- Check out the new fancy juju video #juju-dev 2013-01-08 <TheMue> Morning <rogpeppe> mornin' all <TheMue> rogpeppe: Hiya <TheMue> rogpeppe: ping <rogpeppe> TheMue: pong <TheMue> rogpeppe: Maybe you can help me localizing an error. <rogpeppe> TheMue: glad to help if i can <TheMue> rogpeppe: You know about the rollback of my firewaller change by Dave? It lead to a hanging test in cmd/jujud <rogpeppe> TheMue: i'd seen that go past, yes <TheMue> rogpeppe: It is the MachineSuite.TestProvisionerFirewaller. <TheMue> rogpeppe: So I loked at it with gocheck.vv. <TheMue> rogpeppe: One moment, will paste the log. <TheMue> rogpeppe: http://paste.ubuntu.com/1509166/ <rogpeppe> *click* <TheMue> rogpeppe: You'll see from line 66 that there's a loop. And that will time out after 5 minutes. <fwereade> rogpeppe, TheMue: heh, I was just needing to write some tests there myself and getting depressed about the can't-sync issue <TheMue> rogpeppe: The firewaller tests themselves pass fine. <fwereade> TheMue, ah, that weirdness is fixed in one of my current CLs <TheMue> fwereade: Oh! <TheMue> fwereade: *hug* <rogpeppe> fwereade: which particular weirdness is that? and what was the fix? <fwereade> TheMue, well, ok, some of the weirdness <fwereade> rogpeppe, the lack of a sane environment in which to run and the whining about tools <rogpeppe> fwereade: sorry, i'm feeling stupid - how does that tally with TheMue's issue above? <fwereade> TheMue, rogpeppe: yeah, I'm probably wrong -- I think I was looking at the wrong bit of the log <TheMue> fwereade: From reading the test itself I found now problem regarding the firewaller. Funnily the test passes before I merge the firewaller change. Afterwards it looks like in the paste. <rogpeppe> fwereade: it looks like the crux is that it's getting unauthorized access <TheMue> rogpeppe: Exactly. <rogpeppe> TheMue: have you tried putting in printfs that print when the password is being changed, and what password is being connected with? <rogpeppe> TheMue: that might help narrow down the issue <fwereade> dimitern, heyhey, are you back in town? <TheMue> rogpeppe: Inside the test there's no obvious change, but I'll add statements to see where it may happen. <dimitern> fwereade: hey :) yeah - landed early this morning <fwereade> dimitern, ah cool <rogpeppe> TheMue: that's worth doing i think - put the printfs in the relevant state functions <rogpeppe> dimitern: yo! <fwereade> dimitern, nice hols? <dimitern> fwereade: oh yeah! much needed as well <dimitern> fwereade: yours? <fwereade> dimitern, yeah, very peaceful, was really nice having nico and rie over for xmas <fwereade> dimitern, we've still got another duck in the freezer, I'll let you know when I can be bothered to cook it ;p <dimitern> fwereade: cool :) beers later? <fwereade> dimitern, sgtm, I'll figure out cath's movements and let you know <TheMue> rogpeppe: Interesting, the failure differs. Just had a different simple test fail, and now again the hanging process due to unauthorized access. :( <rogpeppe> TheMue: the failure differs after you've added the printfs? or just it differs from run to run? <TheMue> rogpeppe: From run to run. <rogpeppe> TheMue: what did the new failure look like? <fwereade> rogpeppe, TheMue: a suggestion: the runOnce methods should *take* a *State, and then we get to do an end-run around the deployer-testing nonsense; the machiner-testing nonsense I'm about to hit; probably uniter-testing weirdness I don't remember <fwereade> rogpeppe, TheMue: ...and as a bonus it is somewhat likely we'll be able to write a firewaller/provisioner test I can understand ;p <rogpeppe> fwereade: i'm looking at that stuff right now as well :-) <rogpeppe> fwereade: what difference would passing a state to runOnce make? <TheMue> rogpeppe: The hanging looks like before, otherwise I get one unautherized, but afterwards the opening of the port fails. <TheMue> fwereade: So, I have a problem following you. :/ <fwereade> rogpeppe, IMO the fact that we can't touch the state used in the workers without serious gymnastics makes everything harder <rogpeppe> TheMue: i'm afraid i can't really help without more information. you'll just have to dive in and work out what's going on <rogpeppe> fwereade: how does changing the signature of runOnce help there? <fwereade> rogpeppe, that it gives us a point at which we can sanely hook a test in <rogpeppe> fwereade: personally, i was thinking of adding a var openState func(*state.Info) (*state.State, error) <rogpeppe> fwereade: global <rogpeppe> fwereade: i'm not sure i want tests to know about runOnce <fwereade> rogpeppe, the tests know everything anyway, except the things they need to know <rogpeppe> fwereade: lol <fwereade> rogpeppe, I must say I really am finding them hard to work with :/ <rogpeppe> fwereade: the idea is that the test is minimal, because all the real testing is done in the worker packages themselves, and in jujud we only need to verify that the right components have been joined up correctly <rogpeppe> fwereade: if we call runOnce directly, we aren't verifying that Run will actually do the right thing <fwereade> rogpeppe, so maybe we should test them both? <rogpeppe> fwereade: you're principally concerned about the sync issue here, right? <fwereade> rogpeppe, the sync issue is one thing <fwereade> rogpeppe, the fact that there's duplication between the two agents also concerns me <fwereade> rogpeppe, the dancing around with passwords is hard to follow, and should clearly be common <rogpeppe> fwereade: i had that factored out originally, but was persuaded not to do that <fwereade> rogpeppe, I can well believe it :/ <rogpeppe> fwereade: and... it's only four lines of code <rogpeppe> fwereade: i'm not sure it's that hard to follow <fwereade> rogpeppe, trust me, it is hard to follow <rogpeppe> if passwordChanged != "" { <rogpeppe> if err := m.SetPassword(a.Conf.StateInfo.Password); err != nil { <rogpeppe> oops <fwereade> rogpeppe, there is no justification I can see for smearing the password-handling across openState and 2 spearate clients <rogpeppe> that's new <rogpeppe> fwereade: you can't set the password before you've retrieved the relevant entity from the state <rogpeppe> fwereade: so you'd need to pass in a function to do that <fwereade> rogpeppe, or maybe we could have a sane state API <rogpeppe> fwereade: and if you do that (i did, before) the
{'channel': 'juju-dev', 'month': 1}