| === marcoceppi|away is now known as marcoceppi|trave | ||
| davecheney | thumper: ping | 04:30 |
|---|---|---|
| davecheney | oh dear, no thumper | 04:30 |
| davecheney | has anyone tried runnig multiple local providers on the same machine, under _different_ user accounts ? | 04:31 |
| axw | davecheney: no, but it's *meant* to work according to various comments int he code... have you tried it and it doesn't? | 04:48 |
| davecheney | it does not | 04:48 |
| davecheney | it may work for multiple local environments all owned by a single user | 04:48 |
| davecheney | but does not work when multiple users try to each have a local environment | 04:49 |
| axw | davecheney: I would've thought hte opposite :) it namespaces things on the username | 04:49 |
| davecheney | axw: there is only one mongodb | 04:49 |
| davecheney | it binds to 0.0.0.0 | 04:49 |
| davecheney | and always binds to the same port | 04:49 |
| davecheney | that is not configurable | 04:49 |
| axw | true - there's probably a TODO in there someone | 04:49 |
| davecheney | don't forget, we're using 1.12 | 04:50 |
| davecheney | did somethig change in the last 2 weeks ? | 04:50 |
| axw | sorry, don't know | 04:50 |
| axw | uhh actually, the port is configurable | 04:51 |
| axw | state-port | 04:51 |
| axw | un moment | 04:51 |
| axw | at least on the service registraiton side anyway | 04:52 |
| davecheney | ahh, it's in config/config | 04:53 |
| davecheney | axw: it doens't do shit | 04:55 |
| davecheney | state-port doens't do anything | 04:55 |
| axw | davecheney: you bootstrapped anew? I just did, and it worked. I'm on trunk tho | 04:55 |
| davecheney | it is checked, validated then ignored | 04:55 |
| davecheney | axw: as two different users ? | 04:55 |
| axw | davecheney: well no, but it picked it up, created the service with that port, and connected to it | 04:56 |
| axw | davecheney: I'll create a new user and try. Should be able to set state-port, storage-port and shared-storage-port to unique values and have it work tho | 04:57 |
| davecheney | yeah, that is what I am trying | 04:57 |
| davecheney | YES | 04:58 |
| davecheney | it worked! | 04:58 |
| davecheney | thank heavens for that | 04:58 |
| axw | :D | 04:58 |
| davecheney | shitballs that was a close one | 05:06 |
| davecheney | we only have one machine to demo on | 05:06 |
| bigjools | davecheney has been learning swearing from Debra Morgan | 05:09 |
| jam | hi all | 05:09 |
| davecheney | SUCK BAG! | 05:09 |
| axw | jam: hiya | 05:09 |
| jam | davecheney: I'm so sorry :) | 05:10 |
| jam | oh wait, ECONTEXT :) | 05:10 |
| davecheney | lagging lag is laggy | 05:10 |
| jam | wallyworld: poke. Just checking if you're feeling up to a 1:1 today | 05:11 |
| davecheney | what does 'no CA certificat in environment configuration' mean ? | 05:32 |
| davecheney | never mind | 05:33 |
| davecheney | ok, next problem | 05:39 |
| davecheney | how to I change the api server address | 05:39 |
| davecheney | ? | 05:39 |
| davecheney | api-port I guess | 05:39 |
| wallyworld_ | jam: not sure if you saw my last message, i got disconnected, i'm ok for 1:1 | 05:46 |
| jam | wallyworld_: I didn't. Thanks | 05:48 |
| davechen1y | is there a way to disable the automatic apt-get update behavior ? | 05:57 |
| davechen1y | ie, when we start a machine, it always does apt-get update -y | 05:57 |
| jam | davechen1y: it is one of the flags we past to cloud-init | 05:58 |
| jam | davechen1y: cloudinit/option.go apt_update: true | 05:59 |
| jam | sorry, apt_update, yes, yes | 06:00 |
| jam | wallyworld_: I'm there whenever you're ready | 06:00 |
| wallyworld_ | ok | 06:00 |
| davechen1y | probably can't change it without doing a release | 06:01 |
| wallyworld_ | jam: one sec, i need to install the plugin, since it was lost when my system died | 06:02 |
| jam | davechen1y: I'm pretty sure it was requested (and intentional) behavior. Given we even had a work around when the apt_update flag was broken. | 06:02 |
| jam | wallyworld_: np | 06:02 |
| davechen1y | jam: fair enough | 06:03 |
| davechen1y | thanks | 06:03 |
| davechen1y | does anyone know if it is possible to remove a single subordinate unit | 06:06 |
| jam | davecheney: "remove a single subordinate unit", you mean I want X on all units of Y, except for machine-Z ? | 07:06 |
| jam | wallyworld_: did you see bug #1211147 | 07:10 |
| _mup_ | Bug #1211147: Deploying service to bootstrap node causes debug-log to spew messages <juju-core:Triaged> <https://launchpad.net/bugs/1211147> | 07:10 |
| rogpeppe | mornin' all | 07:31 |
| === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: - | Bugs: 4 Critical, 85 High - https://bugs.launchpad.net/juju-core/ | ||
| dimitern | mgz: hey | 08:49 |
| dimitern | https://codereview.appspot.com/12698043/ ? | 08:49 |
| mgz | is that one of the ones I have a tab open for already... | 08:50 |
| mgz | yup. | 08:51 |
| dimitern | well, I'm waiting from friday :) | 08:51 |
| rogpeppe | dimitern: looking | 08:57 |
| dimitern | rogpeppe: thanks | 08:57 |
| noodles775 | I'm just doing a small refactor for a bug, but am looking at environs/state.go:BootstrapState.StateInstances - why is that a slice? (Are there multiple state instances for some providers, or for the future?) | 09:00 |
| noodles775 | And more specifically, is it OK/intended that StateInfo will return as soon as it has the hostname of one state instance? | 09:02 |
| mgz | dimitern: a couple of queries | 09:02 |
| dimitern | mgz: well I could've called it canModifyOrRead - because it's the same check, but once it's for reading, once for writing | 09:02 |
| mgz | I don't follow | 09:03 |
| dimitern | mgz: what's not clear? | 09:04 |
| mgz | the function that returns is actually a can-anything-happen, despite being called getCanRead? | 09:05 |
| mgz | the other functions call the returnvalue canRead but otherwise use it identically | 09:06 |
| dimitern | some functions, like SetPublicAddress are "write" requests, while others like SubordinateNames are "read" requests | 09:06 |
| mgz | right. | 09:07 |
| dimitern | both have the same check - "are you the owner of that entity?" | 09:07 |
| mgz | but we call the function getter for that check "getCanRead"? | 09:07 |
| dimitern | I agree the name should probably be canAccess or something | 09:07 |
| mgz | having the return value sometimes named canRead and sometimes named canModify isn't really helpful | 09:08 |
| dimitern | I named the returned function after it's actual role in the method - read or modify | 09:08 |
| dimitern | its* | 09:09 |
| dimitern | will it be better if I call it getCanAccess and canAccess respectively? | 09:11 |
| mgz | seems like an improvement to me | 09:11 |
| rogpeppe | dimitern: reviewed | 09:13 |
| dimitern | rogpeppe: cheers | 09:13 |
| rogpeppe | dimitern: +1 to canAccess | 09:14 |
| rogpeppe | wallyworld_: ping | 09:15 |
| jam | mgz: /wave | 09:24 |
| mgz | hey jam! | 09:24 |
| rogpeppe | jam: welcome back! | 09:25 |
| jam | rogpeppe: hey | 09:25 |
| === flaviamissi_ is now known as flaviamissi | ||
| === wedgwood is now known as Guest52531 | ||
| dimitern | mgz, rogpeppe: https://codereview.appspot.com/12748043 | 10:26 |
| rogpeppe | dimitern: reviewed | 10:34 |
| dimitern | rogpeppe, mgz: thanks! | 10:34 |
| dimitern | rogpeppe: I don't really get your comment here https://codereview.appspot.com/12748043/diff/1/state/api/params/internal.go#newcode35 | 10:37 |
| rogpeppe | dimitern: oh, what doesn't make sense? | 10:38 |
| dimitern | rogpeppe: aren't we going to introduce a backwards-incompatible change that way? | 10:39 |
| rogpeppe | dimitern: i don't think so | 10:39 |
| dimitern | rogpeppe: changing all results with Error to be omitempty | 10:39 |
| rogpeppe | dimitern: how would it be backwardly incompatible? | 10:39 |
| rogpeppe | dimitern: you'll get exactly the same result, won't you? | 10:40 |
| dimitern | rogpeppe: not in javascript | 10:40 |
| rogpeppe | dimitern: nothing is using any of these calls from javascript | 10:40 |
| dimitern | rogpeppe: having {Result: x, Error: null} is different from {Result: x} | 10:41 |
| rogpeppe | dimitern: i don't see your point. none of the agent API calls is used by anything except Go. | 10:41 |
| dimitern | rogpeppe: or anything that's relying on it to be JSON | 10:41 |
| dimitern | rogpeppe: how about third-party agents? | 10:41 |
| rogpeppe | dimitern: these calls are exclusively for our own use (currently anyway) | 10:42 |
| rogpeppe | dimitern: we don't support third-party agents. | 10:42 |
| dimitern | rogpeppe: and can you guarantee nobody else is using them? | 10:42 |
| dimitern | rogpeppe: there were already some python helpers using the api | 10:42 |
| rogpeppe | dimitern: yes, the client API is our only public API | 10:43 |
| rogpeppe | dimitern: if anyone is using the agent API, they'd have to be parsing our agent config files, getting the password, and pretending to be a machine agent or whatever | 10:43 |
| dimitern | rogpeppe: and that's going to be like that forever? | 10:43 |
| rogpeppe | dimitern: not necessarily | 10:43 |
| rogpeppe | dimitern: but it is now | 10:44 |
| dimitern | rogpeppe: hmm.. ok, but istm this is a change that's not for this CL | 10:44 |
| rogpeppe | dimitern: and so we don't care about non-Go backward compatibility in the agent API at the moment | 10:44 |
| rogpeppe | dimitern: sure | 10:44 |
| rogpeppe | dimitern: but you can at least do it for StringsResult | 10:44 |
| rogpeppe | dimitern: well actually | 10:44 |
| rogpeppe | dimitern: you don't even need to do it for StringsResult | 10:44 |
| rogpeppe | dimitern: you can add the Error field | 10:45 |
| dimitern | rogpeppe: ah.. ok I got you | 10:45 |
| rogpeppe | dimitern: and even without omitempty it will still be backwardly compatible | 10:45 |
| dimitern | rogpeppe: you're saying rename StringsErrorResults to StringsResults and add an Error field to StringsResult | 10:45 |
| rogpeppe | dimitern: yes | 10:45 |
| dimitern | rogpeppe: ok then, will do | 10:45 |
| rogpeppe | dimitern: thanks | 10:45 |
| dimitern | rogpeppe: https://codereview.appspot.com/12748043/ - looks good? | 11:04 |
| rogpeppe | dimitern: looking | 11:04 |
| dimitern | rogpeppe: just noticed a typo in StringsResults doc comment | 11:05 |
| rogpeppe | dimitern: +1 | 11:05 |
| rogpeppe | dimitern: looks great, thanks | 11:06 |
| dimitern | rogpeppe: cheers | 11:06 |
| rogpeppe | dimitern: we could actually benefit in a few places from a way to induce an error talking to the underlying mongo | 11:06 |
| dimitern | rogpeppe: oh, yeah | 11:08 |
| rogpeppe | dimitern: one way to do it might be to make all the tests actually connect to mongo through an intermediate relay that the tests could close when desired | 11:08 |
| rogpeppe | dimitern: but that would probably disrupt lots of other test machinery which expects to be able to talk to state after a test has completed | 11:09 |
| dimitern | rogpeppe: or introduce hooks into the mgo connections? | 11:10 |
| rogpeppe | dimitern: that only helps if the code you're trying to test is using the same *State that the test has access to | 11:11 |
| rogpeppe | dimitern: the fact that State is using mgo is pretty much hidden outside of the state package. | 11:11 |
| rogpeppe | dimitern: i guess we could have a global function, state.SetQueryError(err error) that sets the error returned when anything makes a mongo query. | 11:13 |
| rogpeppe | dimitern: and similarly for transactions | 11:13 |
| rogpeppe | dimitern: anyway, something to think on | 11:14 |
| dimitern | rogpeppe: sounds plausable | 11:14 |
| rogpeppe | dimitern: standup? | 11:32 |
| dimitern | jam: ^^ | 11:33 |
| davecheney | reported on #juju, http://pastebin.com/FUri1dx3 | 11:44 |
| davecheney | any ideas ? | 11:44 |
| rogpeppe | wallyworld_: any chance of a chat at some point? | 12:00 |
| wallyworld_ | rogpeppe: sorry i wasn't around before, was at soccer. did you want to catch up? give me 5? | 12:00 |
| rogpeppe | wallyworld_: np | 12:00 |
| rogpeppe | wallyworld_: i'll stay in this call for a bit; probably 10 minutes or so? | 12:01 |
| davecheney | sounds like the OP has deployed to his maas nodes | 12:03 |
| davecheney | then turned them off | 12:03 |
| davecheney | ... | 12:03 |
| wallyworld_ | rogpeppe: start a new hangout if the other one is busy? https://plus.google.com/hangouts/_/e4864938922c332329d0d696d9cb67f50b0d04f4?authuser=1 | 12:05 |
| === gary_poster|away is now known as gary_poster | ||
| dimitern | rogpeppe, natefinch, mgz: next in line https://codereview.appspot.com/12756043/ | 12:16 |
| noodles775 | Hi people, I've got 3 branches needing reviews if anyone has time - the first just needs feedback from rogpeppe, but the other two haven't been looked at yet: | 12:24 |
| noodles775 | https://codereview.appspot.com/12659043/ https://codereview.appspot.com/12755043/ https://codereview.appspot.com/12603047/ | 12:24 |
| rogpeppe | dimitern, noodles775: sorry, was in a call | 12:27 |
| davecheney | question from the field | 12:27 |
| davecheney | does anyone have an objection to me adding config keys to expose the AptProxy and AptUpdate/AptUpgrade items in cloudinit | 12:28 |
| davecheney | ? | 12:28 |
| davecheney | i can explain why we need such things, but not in this channel | 12:28 |
| dimitern | davecheney: i think sidnei was working on something similar | 12:29 |
| davecheney | dimitern: or'ly | 12:29 |
| davecheney | i'll talk to im | 12:29 |
| davecheney | him | 12:29 |
| davecheney | we sort of need it for the thing that marco and I are doing this week | 12:29 |
| davecheney | dimitern: what hours does sidney work ? | 12:30 |
| davecheney | sidnei: | 12:30 |
| dimitern | davecheney: he's usually around at this time or couple of hours later | 12:31 |
| dimitern | davecheney: utc-3 | 12:32 |
| davecheney | dimitern: right o | 12:32 |
| davecheney | i might hack something in anyway, we need it tomorrow | 12:32 |
| davecheney | but i'll check with S | 12:32 |
| dimitern | davecheney: i believe that's what i was referring to https://codereview.appspot.com/12143043/ | 12:33 |
| davecheney | fuck yeah | 12:34 |
| davecheney | someone has done all the work for me | 12:34 |
| davecheney | sweet | 12:34 |
| dimitern | :D | 12:34 |
| * davecheney goes to try it | 12:35 | |
| davecheney | ohhh, interesting, he's changed the lxc provider as well ... | 12:35 |
| davecheney | https://codereview.appspot.com/12143043/diff/34001/cloudinit/options.go?column_width=80 | 12:35 |
| davecheney | did this already land ? | 12:35 |
| davecheney | it | 12:35 |
| davecheney | it's there now | 12:35 |
| dimitern | yes that one did land last week | 12:36 |
| * davecheney smacks head | 12:37 | |
| davecheney | of course, we don't see (closed) on our CLs when they land | 12:37 |
| dimitern | noodles775: looking the last two of yours | 12:37 |
| dimitern | davecheney: yep, I only know it because I reviewed it and I can see it in my list | 12:38 |
| dimitern | I have the habit of manually closing them when then land | 12:38 |
| dimitern | noodles775: there was never more than one state instance | 12:39 |
| dimitern | noodles775: there are plans for ha support, but nothing done yet | 12:39 |
| * davecheney wonders if we should backport that to 1.12.1 | 12:39 | |
| davecheney | naaah | 12:39 |
| davecheney | it's a lot of work | 12:39 |
| davecheney | and it isn't a *bugfix* | 12:40 |
| dimitern | there's a linked bug, but not about that - about the local provider/lxc | 12:40 |
| davecheney | kk | 12:40 |
| dimitern | noodles775: and about hp cloud account, you can ask mgz or wallyworld_ - we have one you can use perhaps | 12:42 |
| noodles775 | dimitern: k, so BootstrapState.StateInstances is a slice just for the future plans? Do you want me to then s/WaitForFirstDNSName/WaitForDNSNames/ then (since there is only ever one state server, it'll leave the helper more general) | 12:44 |
| noodles775 | dimitern: Re the hp cloud account, that'd be great to test - but no problems if not (if someone can verify the branch, that'd be enough for now). | 12:44 |
| dimitern | noodles775: still looking, will get back to you | 12:45 |
| dimitern | noodles775: so, not sure why you need WaitForFirstDNSName at all? | 12:50 |
| dimitern | noodles775: can you expand a bit? | 12:51 |
| noodles775 | dimitern: because that's what the existing code in trunk was doing - environs/state.go:StateInfo (or am I misreading)? | 12:52 |
| noodles775 | It polls until it gets the hostname of a (well, the really) state server. | 12:53 |
| natefinch | mgz: gotta go for a bit, I'll be back around 15-20 past the hour | 12:54 |
| dimitern | noodles775: yeah, it's written to accept more than one hostname, but in reality it's always one | 12:54 |
| mgz | noodles775: which branch? ...eh, I'll just look at all three | 12:54 |
| mgz | natefinch: sure, poke me when you're back | 12:54 |
| noodles775 | Thanks mgz (the last two, dimitern is looking now too) | 12:54 |
| dimitern | noodles775: so WaitDNSNames is used by the providers internally | 12:55 |
| dimitern | noodles775: to implement StateInfo | 12:56 |
| noodles775 | dimitern: Yeah, the fact that BootstrapState.StateInstances is a slice makes it hard to code otherwise (I mean, I could just take the 0th index, but...) | 12:56 |
| rogpeppe | noodles775: i'm not convinced by https://codereview.appspot.com/12755043/ | 12:56 |
| mgz | ick, I'm not a fan of the dummy provider | 12:57 |
| rogpeppe | noodles775: it sounds like a bug in the provider that HP cloud uses (openstack ?) | 12:57 |
| noodles775 | rogpeppe: Maybe, see jam's comment on the related bug too. | 12:58 |
| noodles775 | rogpeppe: but do you mean you wouldn't want to keep polling when the provider returns ErrNoInstances? | 12:58 |
| dimitern | noodles775: yeah, the more I read it, the more I think the approach is not right | 12:58 |
| dimitern | noodles775: ErrNoInstances is expected - they take time to start | 12:59 |
| rogpeppe | noodles775: yeah. in fact, i wouldn't poll at all when calling Instances | 12:59 |
| rogpeppe | noodles775: i don't think jam's comment is relevant | 12:59 |
| rogpeppe | noodles775: as we're not calling AllInstances here | 12:59 |
| rogpeppe | noodles775: (unless you mean a different comment | 12:59 |
| rogpeppe | ) | 12:59 |
| noodles775 | No, I meant that one - I hadn't checked the change referred to (only saw the comment this morning). | 13:00 |
| rogpeppe | noodles775: i think the original code looks ok | 13:00 |
| noodles775 | So I'm a bit confused - dimitern, you're saying that ErrNoInstances is expected because they take time to start, but that means we'd want to keep polling there... | 13:01 |
| noodles775 | rogpeppe: It could be - without an HP account I didn't first reproduce the reported bug. | 13:01 |
| dimitern | noodles775: yes, that's what we do | 13:01 |
| mgz | noodles775: I think the key is this used to work, it's just a regression | 13:01 |
| rogpeppe | noodles775: dimitern isn't quite right there | 13:01 |
| mgz | we don't want new polling code to resolve the issue, just to work out what broke and undo it | 13:02 |
| dimitern | rogpeppe: ok, care to explain better please? | 13:02 |
| rogpeppe | dimitern: Environ.Instances is expected to poll when it can't find the instances | 13:02 |
| rogpeppe | dimitern: and, at least in ec2, that's what it does | 13:02 |
| rogpeppe | dimitern: so you'll only get ErrNoInstances or ErrPartialInstances once it's already polled for long enough to be certain | 13:03 |
| dimitern | rogpeppe: hmm.. ok | 13:03 |
| dimitern | rogpeppe: and the openstack provider does the same, no? | 13:03 |
| rogpeppe | i do think that environs.LongAttempt is a smell | 13:03 |
| * rogpeppe checks | 13:04 | |
| * noodles775 looks for recent changes adding ErrNoInstances | 13:04 | |
| rogpeppe | noodles775: it's quite possible that shortAttempt is too short in HP openstack | 13:04 |
| dimitern | and that's a recent change | 13:05 |
| noodles775 | mgz: it's not new polling code - I'm just extending the errors that are allowed while polling in StateInfo. | 13:05 |
| mgz | noodles775: I see new public functions in environs/polling.go | 13:07 |
| noodles775 | mgz: yep, but they're used in environs.StateInfo with the same (intended) functionality as was already there (I was just trying to make StateInfo both explicit and simpler, but may have failed). | 13:08 |
| rogpeppe | i think there should probably be a separate package (environs/utils?) that exposes functions for the providers themselves to use. i think that having, for example, environs.Bootstrap (intended to be called by clients of environs) and environs.StateInfo (intended to be called back by providers) is confusing | 13:09 |
| rogpeppe | noodles775: i don't see anything wrong with the current implementation of environs.StateInfo | 13:09 |
| dimitern | that's a whole different talk I think rogpeppe | 13:09 |
| rogpeppe | dimitern: i agree | 13:10 |
| rogpeppe | dimitern: just an aside | 13:10 |
| rogpeppe | noodles775: one other thing: what was the motivation behind the changes to the dummy provider? | 13:11 |
| noodles775 | rogpeppe: Yep, nothing wrong with it - I just thought that that was where we'd need to also allow ErrNoInstances to avoid this bug, but as you said, a better place may be in openstack/provider.go (the shortAttempt). | 13:12 |
| noodles775 | rogpeppe: The broken-error? To be able to test the behaviour of StateInfo (ie. that it still timed out if ErrNoInstances was raised, rather than passing the error up straight away). | 13:12 |
| rogpeppe | noodles775: ah, i see. | 13:14 |
| noodles775 | (which ended up being encapsulated by WaitForInstances and so tested there) | 13:14 |
| rogpeppe | noodles775: probably not necessary if we end up just raising openstack.shortAttempt | 13:15 |
| dimitern | rogpeppe: perhaps not all of it, but just for that test? | 13:15 |
| noodles775 | rogpeppe: yep - I think the whole branch won't be necessary, but was a good learning exercise :-) | 13:15 |
| rogpeppe | noodles775: :-) | 13:16 |
| rogpeppe | dimitern: ? | 13:16 |
| dimitern | rogpeppe: openstack.shortAttepmt | 13:16 |
| rogpeppe | dimitern: the bug was encountered for real, not in a test | 13:17 |
| rogpeppe | noodles775: do you remember just how long it *did* take before the command actually succeeded? | 13:17 |
| dimitern | rogpeppe: hmm.. so the shortAttempt used by the code you mean, not the one in tests? aren't these two linked? | 13:18 |
| rogpeppe | noodles775: the bug report shows that it took at least 17 seconds | 13:18 |
| rogpeppe | dimitern: yes | 13:18 |
| rogpeppe | dimitern: they're not really linked | 13:18 |
| rogpeppe | dimitern: except in the live tests, i guess | 13:19 |
| noodles775 | rogpeppe: Yes - I've not seen the bug in action (don't have HP creds), so am goin on the 17seconds there (the LongAttempt is 3 minutes) | 13:19 |
| rogpeppe | noodles775: i'd go for 20 seconds at least | 13:20 |
| rogpeppe | noodles775: looks like HP's consistency is really very eventual :-) | 13:21 |
| noodles775 | lol | 13:21 |
| noodles775 | rogpeppe: OK, so I'll do a second (much simpler) branch which just updates the shortAttempt there :-) | 13:21 |
| rogpeppe | noodles775: thanks. it seems a pity (if you've got a genuine error you'll have to wait for 20 seconds before it tells you) but i suppose that's the price we pay for distributed databases | 13:22 |
| rogpeppe | i wonder if it might be useful to allow the timeout to be adjusted for different openstack providers | 13:23 |
| dimitern | we need more live testing instead of making obscure tweakable settings like that | 13:25 |
| sidnei | davecheney: there's a couple bugs i've uncovered about the apt-proxy stuff IRL mainly when the host is precise, i have a bug open and a plan to fix it. | 13:25 |
| davecheney | sidnei: right | 13:25 |
| davecheney | so we are using a raring host for precise machines | 13:26 |
| davecheney | is that the bug ?> | 13:26 |
| davecheney | rogpeppe: on a long enough timeline, HP will be eventually consistent | 13:26 |
| sidnei | davecheney: nope, the one bug i found is taht if the host is precise, apt-config dump dumps the whole config without filtering, which is then shoved into /etc/apt/apt.conf.d/99proxy in the container | 13:26 |
| rogpeppe | davecheney: is HP more tardy in general than other providers? | 13:27 |
| dimitern | rogpeppe, mgz, natefinch: poke https://codereview.appspot.com/12756043/ | 13:27 |
| sidnei | where as in raring/saucy (havent checked others) it is filtered down to the args you provide after 'dump' | 13:27 |
| davecheney | sidnei: ewww | 13:27 |
| rogpeppe | dimitern: i will get there :-) | 13:27 |
| dimitern | rogpeppe: thanks :) | 13:27 |
| sidnei | davecheney: a separate issue that hazmat brought up is that it requires configuring the host to use apt proxy so it's not automatic. pyjuju was simply checking if port 3142 (apt-cacher-ng) is up and listening and then adding that as the apt proxy without any extra config. | 13:28 |
| davecheney | sidnei: yeah, i wanted to add a config property to environs/config.go | 13:29 |
| davecheney | err | 13:29 |
| davecheney | environs/config/config.go | 13:29 |
| davecheney | which exposed the cloud-init apt-proxy key | 13:29 |
| davecheney | crude | 13:29 |
| davecheney | but reliable | 13:29 |
| sidnei | davecheney: that was one of the options i brought up on the list, but there were conflicting replies. | 13:30 |
| davecheney | sidnei: well, this may not get merged, but I need something simple | 13:30 |
| rogpeppe | noodles775: i think your fix in https://codereview.appspot.com/12603047 is a reasonable one, but i don't think we can do it | 13:31 |
| noodles775 | rogpeppe: Yeah, as per the description there, I wasn't happy with it. Do you have a better way forward, or what other issues do you see? | 13:33 |
| rogpeppe | noodles775: currently it's valid to call "juju get myservice --debug" | 13:33 |
| rogpeppe | noodles775: your change will break that usage AFAICS | 13:34 |
| rogpeppe | noodles775: one possibility is to change the Command interface so that individual commands can request that flags are not allowed to be interspersed. | 13:34 |
| sidnei | davecheney: im definitely +1 on an explicit option, be it in the environ config or as i've implemented and landed, by changing the host's apt config. while the automatic option like in pyjuju is better from 'i just want to get started using juju quickly' pov, it's also a bit... surprising. | 13:34 |
| davecheney | sidnei: i'll try your patch tomorrow | 13:35 |
| davecheney | i think we can change the host so it passes throught the right values | 13:35 |
| noodles775 | rogpeppe: yeah - that sounds simpler than the other suggestion I had there on the description. | 13:35 |
| sidnei | tks, i'll put up a bug fix for precise shortly. | 13:35 |
| rogpeppe | noodles775: for example, add an AllowInterspersedFlags() bool method | 13:36 |
| rogpeppe | noodles775: to the Command interface | 13:36 |
| noodles775 | rogpeppe: great - I'll try that out. Thanks for the feedback. | 13:37 |
| rogpeppe | noodles775: there are a couple of other possibilities too | 13:37 |
| rogpeppe | noodles775: i haven't decided which i dislike least :-) | 13:37 |
| rogpeppe | noodles775: if the suggestion above is 1) | 13:38 |
| rogpeppe | noodles775: then: | 13:38 |
| rogpeppe | noodles775: 2) create another interface: type NonInterspersedFlagsCommand interface {NonInterspersedFlags()} and make SuperCommand.Init test dynamically if the command implements that. | 13:39 |
| rogpeppe | noodles775: 3) add another argument to cmd.Register | 13:39 |
| rogpeppe | noodles775: the down side with 1) is that, though simple, it requires that method to be added to every single command. | 13:39 |
| rogpeppe | noodles775: although... | 13:40 |
| rogpeppe | noodles775: hmm, one mo | 13:40 |
| rogpeppe | noodles775: ha, we've already got CommandBase! perfect. | 13:40 |
| noodles775 | yeah - I thought BaseCommand could do som... | 13:40 |
| noodles775 | :) | 13:40 |
| noodles775 | Excellent, that makes 1) the winner then right? | 13:41 |
| rogpeppe | noodles775: yup | 13:41 |
| * rogpeppe starts looking at dimitern's review | 13:42 | |
| natefinch | mgz: sorry, back finally. Kids make everything take twice as long. | 13:42 |
| mgz | :) | 13:43 |
| sidnei | trivial-ish review: https://codereview.appspot.com/12758044 | 14:00 |
| davecheney | sidnei: your apt-config based proxy is working well | 14:01 |
| davecheney | it's makeing the hotel wifi much more apporachable | 14:01 |
| sidnei | davecheney: good to know. see cl above for the precise fix. | 14:02 |
| * davecheney looks | 14:02 | |
| dimitern | sidnei: looking | 14:02 |
| davecheney | oh, i see what's happehned | 14:02 |
| rogpeppe | dimitern: how about something like this for factoring out the endless duplication? http://paste.ubuntu.com/5977395/ | 14:03 |
| rogpeppe | dimitern: i think i can implement that in an hour or so | 14:03 |
| rogpeppe | dimitern: and i think it applies to pretty much all our API calls | 14:04 |
| === Guest52531 is now known as wedgwood | ||
| dimitern | rogpeppe: and the implementation will basically be a lot of hardly readable reflection magic? | 14:06 |
| rogpeppe | dimitern: well, at least it can be well tested and it will save us 1000s of lines of code and days of implementation time | 14:06 |
| dimitern | rogpeppe: how about getCanX? it just seems too generic to be useful | 14:06 |
| rogpeppe | dimitern: one mo, i'll paste an example of how i envisage it would be used | 14:07 |
| dimitern | rogpeppe: we need at least one for Get and one for Set - they'll do different things | 14:07 |
| dimitern | rogpeppe: maybe more that 2 will be needed | 14:08 |
| rogpeppe | dimitern: really? | 14:08 |
| rogpeppe | dimitern: why the difference? | 14:08 |
| dimitern | rogpeppe: one returns something other than an error + optional error or bool to something, and the other, just an error and takes a bunch of args | 14:09 |
| rogpeppe | dimitern: i don't see the difference. they both take some parameters and return some results | 14:09 |
| dimitern | rogpeppe: how about providing a bunch of helpers + callbacks and do the rest inline? | 14:10 |
| dimitern | rogpeppe: like prepareResult, checkAuth, processEntities | 14:11 |
| rogpeppe | dimitern: here's how it might look for CharmURL, for example: http://paste.ubuntu.com/5977431/ | 14:11 |
| dimitern | rogpeppe: and customizing each such helper to work with different results/params | 14:12 |
| dimitern | rogpeppe: that's sounds like forcing us to use reflection with it's performance issues | 14:12 |
| rogpeppe | dimitern: i'd be interested to see a concrete proposal | 14:12 |
| rogpeppe | dimitern: do you know that performance is a problem here? | 14:13 |
| dimitern | rogpeppe: where's the auth in the example? | 14:13 |
| rogpeppe | dimitern: in the func returned by getUnitFunc | 14:13 |
| dimitern | rogpeppe: I'll need a more concrete example then, sorry | 14:14 |
| rogpeppe | dimitern: ok, hold on | 14:14 |
| rogpeppe | dimitern: here's a sample implementation of getUnitFunc in terms of the existing UniterAPI primitives: http://paste.ubuntu.com/5977456/ | 14:17 |
| dimitern | rogpeppe: also, fwiw I don't think this approach is any faster than what we have now, it'll just reduce code duplication at the expense of having to look at several places to see what's going on | 14:17 |
| dimitern | rogpeppe: that'll work except for NotFoundErr, which needs to be converted to ErrPerm, but I begin to see your point | 14:18 |
| rogpeppe | dimitern: reducing the number of lines of code from 26 to 7 seems like a pretty good win to me | 14:19 |
| rogpeppe | dimitern: over however many API calls | 14:19 |
| rogpeppe | dimitern: plus the fact that we can potentially reduce the amount of test code | 14:19 |
| rogpeppe | dimitern: because we can be sure that the details of the bulk call are dealt with correctly | 14:20 |
| dimitern | rogpeppe: how can we do that? | 14:20 |
| rogpeppe | dimitern: at the moment there's a significant amount of logic in each bulk API call that needs to be tested | 14:21 |
| rogpeppe | dimitern: if each bulk API call contains a single call to BulkCall, surely we can make our tests rely on that and test only that the | 14:22 |
| rogpeppe | dimitern: call is made with the expected parameters. | 14:22 |
| dimitern | rogpeppe: are you getting at not writing individual tests for each call? | 14:22 |
| rogpeppe | dimitern: no, but at the moment each test needs to pass in a vector with several arguments in, each designed to test an aspect of the logic | 14:23 |
| rogpeppe | dimitern: we'd just need to pass a single-element vector containing one argument, and check that the single expected result is returned | 14:23 |
| dimitern | rogpeppe: yeah, and I'm sure we can do something about reducing the size of tests, without losing code coverage | 14:24 |
| rogpeppe | dimitern: exactly | 14:24 |
| natefinch | rogpeppe, dimitern: for what it's worth, I think code generation would make the code less error prone than it is now, and more readable than some really generic thing like the BulkCall Roger posted | 14:24 |
| dimitern | natefinch: I was thinking of a way to declaratively generate both code and tests for the api calls | 14:25 |
| rogpeppe | natefinch: i'm a bit reluctant to go down that road | 14:25 |
| rogpeppe | natefinch: although if we've factored things out to use reflection, it would not be hard to change things to go that route in the future if desired. | 14:26 |
| natefinch | rogpeppe: I'm reluctant to go down the road of writing a function that takes 4 empty interface arguments and needs 5 paragraphs of comments to explain it :) | 14:27 |
| dimitern | rogpeppe: surely, generated code can be optimized to run faster than a generic all-in-one code relying on reflection magic | 14:27 |
| rogpeppe | dimitern: performance isn't the issue here | 14:27 |
| rogpeppe | dimitern: we haven't even started trying to optimise stuff yet | 14:27 |
| rogpeppe | dimitern: beyond hand-wavy "bulk API calls will be faster" remarks | 14:28 |
| dimitern | rogpeppe: using reflection several times for each call will surely degrade performance, don't you agree? | 14:28 |
| rogpeppe | dimitern: sure | 14:28 |
| rogpeppe | dimitern: so will using many features that we're using all the time | 14:28 |
| rogpeppe | dimitern: currently for each API call we're making several round trips to mongo | 14:29 |
| rogpeppe | dimitern: *that*'s the place to start optimising stuff | 14:29 |
| rogpeppe | dimitern: a few reflection calls are neglible compared to a network round trip | 14:29 |
| dimitern | rogpeppe: sure, but adding more less performant code on top of it won't improve things much in the long run | 14:30 |
| rogpeppe | dimitern: first rule of optimisation: measure first | 14:30 |
| dimitern | rogpeppe: exactly, that's why we need a proof of concept for both | 14:30 |
| rogpeppe | natefinch: i agree with your reservations about the reflect call. but automatically generated code has its own down sides. | 14:32 |
| rogpeppe | dimitern: i'm not convinced that macros are better than reflection in general | 14:33 |
| dimitern | rogpeppe: macros? | 14:34 |
| rogpeppe | dimitern: code that writes code | 14:34 |
| dimitern | rogpeppe: well, code that's generated and compiled, rather than dynamically reflected at run-time | 14:35 |
| natefinch | rogpeppe: definitely. Neither is ideal. But I think generated code has better tradeoffs. the code it creates is a lot easier to read, understand and debug than reflection-heavy generic code | 14:35 |
| dimitern | natefinch: oh, that's +1 | 14:36 |
| mgz | rogpeppe: does goamz use lbox workflow for landing things? | 14:36 |
| rogpeppe | mgz: yes | 14:36 |
| mgz | ta | 14:36 |
| rogpeppe | natefinch: i'm not sure. i think that 50 lines of reflection magic to reduce code by 75% elsewhere can be worth it (think fmt.Printf for example) | 14:38 |
| rogpeppe | natefinch: i'd be interested to see how the code might look using gocog, for example, though | 14:46 |
| * dimitern has to step out for a bit | 14:47 | |
| natefinch | rogpeppe: I'll whip something up, so we'll at least have some real code to look at and complain about :) | 14:47 |
| sidnei | rogpeppe: if you could review https://codereview.appspot.com/12758044/ | 14:51 |
| rogpeppe | sidnei: looking | 14:51 |
| rogpeppe | sidnei: looking | 15:09 |
| rogpeppe | sidnei: i mean, reviewed! | 15:09 |
| dimitern | rogpeppe: hey | 15:23 |
| rogpeppe | dimitern: yo | 15:23 |
| dimitern | rogpeppe: how about to offer you a deal? :) let's keep that duplicated way until we finish the uniter client and server side, so we can actually refactor the uniter to use it | 15:23 |
| dimitern | rogpeppe: then let's discuss how to minimize the duplication - reflection or code generation, i don't mind, let's discuss it then | 15:24 |
| rogpeppe | dimitern: my aim was to reduce the amount of work you need to do now | 15:24 |
| dimitern | rogpeppe: because now I have certain momentum and would like to keep it up | 15:24 |
| rogpeppe | dimitern: keep on going | 15:25 |
| dimitern | rogpeppe: it's no trouble really | 15:25 |
| dimitern | rogpeppe: thanks! | 15:25 |
| dimitern | natefinch, mgz, rogpeppe: I still need 2 reviews on https://codereview.appspot.com/12756043/ | 15:27 |
| rogpeppe | dimitern: oh yes, sorry, i got some way through, then distracted | 15:27 |
| dimitern | no worries | 15:27 |
| rogpeppe | dimitern: reviewed | 15:32 |
| dimitern | rogpeppe: cheers | 15:32 |
| dimitern | mgz: ? | 15:38 |
| mgz | dimitern: looking | 15:42 |
| sidnei | rogpeppe: fixed, thanks for the review. shall i land? | 15:48 |
| rogpeppe | sidnei: i'll just take a quick last look | 15:48 |
| rogpeppe | sidnei: LGTM, thanks. sorry, i had an unpublished comment suggesting the use of the m flag; hope you didn't have to look too hard for it! | 15:49 |
| sidnei | rogpeppe: no, was a google + two clicks away :) | 15:49 |
| sidnei | spent more time figuring out why \s? didn't work :) | 15:50 |
| rogpeppe | sidnei: FWIW golang.org/pkg/regexp/syntax is an easy quick ref | 15:50 |
| rogpeppe | sidnei: why didn't it work? | 15:50 |
| mgz | dimitern: reviewed, favour extraction to review the branch I have up please :0 | 15:50 |
| sidnei | because it's the wrong syntax, it should have been \s* | 15:50 |
| rogpeppe | sidnei: ah, i hadn't noticed that | 15:51 |
| dimitern | mgz: thanks - sure, can I have a link? | 15:51 |
| sidnei | rogpeppe: although, hum the page at http://golang.org/pkg/regexp/syntax/ say it should work | 15:52 |
| mgz | dimitern: 12752043 :) | 15:52 |
| sidnei | "x? zero or one x, prefer one" "x* zero or more x, prefer more" | 15:52 |
| rogpeppe | sidnei: what were you trying to match? | 15:53 |
| dimitern | mgz: sneaky :) | 15:53 |
| sidnei | rogpeppe: zero or more spaces, but i actually had two in the test i did. | 15:53 |
| rogpeppe | sidnei: ah, so it's right that it didn't work | 15:53 |
| sidnei | indeed | 15:54 |
| * sidnei blames on timezone | 15:54 | |
| dimitern | mgz: reviewed | 16:01 |
| rogpeppe | dimitern, natefinch: FWIW, here's the magic: http://play.golang.org/p/BzS1Kn8peV | 16:16 |
| rogpeppe | dimitern: all the type inspection code can be factored out and done only once for each combination of types | 16:17 |
| dimitern | rogpeppe: will look more carefully a bit later | 16:32 |
| sidnei | rogpeppe: reminder that im waiting on your ack. | 16:42 |
| rogpeppe | sidnei: no need to wait - I already gave you my LGTM | 16:42 |
| rogpeppe | sidnei: thanks anyway :-) | 16:43 |
| dimitern | rogpeppe, mgz: last of the unit ops https://codereview.appspot.com/12776043 | 16:43 |
| sidnei | rogpeppe: sorry i was looking for one in the cl and missed the one on irc. :) | 16:46 |
| rogpeppe | sidnei: np | 16:47 |
| rogpeppe | dimitern: reviewed | 16:52 |
| dimitern | rogpeppe: cheers | 16:53 |
| dimitern | mgz, natefinch: second review? | 16:55 |
| dimitern | please.. | 16:55 |
| mgz | dimitern: sec | 17:01 |
| mgz | dimitern: done, alos responded to you comments on review | 17:08 |
| dimitern | mgz: thanks! | 17:09 |
| === flaviamissi is now known as flaviamissi_ | ||
| natefinch | mgz: back | 17:33 |
| mgz | natefinch: hey, with you in a sec | 17:39 |
| natefinch | mgz: no rush | 17:39 |
| natefinch | arg... ubuntu, why do you hate me so? I can get either the microphone or the headphones of my headset to work, but not both at the same time. It's like whatever tab of the sound settings I'm on is what ubuntu decides will work, when I plug in the USB cord. :/ | 17:48 |
| * rogpeppe feels for natefinch | 17:52 | |
| rogpeppe | time to go | 17:52 |
| rogpeppe | g'night all | 17:52 |
| natefinch | rogpeppe: g'night | 17:52 |
| rogpeppe | natefinch: see ya! | 17:52 |
| === tasdomas_afk is now known as tasdomas | ||
| === flaviamissi_ is now known as flaviamissi | ||
| === tasdomas is now known as tasdomas_afk | ||
| === flaviamissi is now known as flaviamissi_ | ||
| === andreas__ is now known as ahasenack | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!