| thumper | hazmat: ping | 01:12 |
|---|---|---|
| hazmat | thedac, pong | 04:45 |
| hazmat | whoops | 04:45 |
| hazmat | axw, ping | 04:45 |
| axw | hazmat: heya | 04:46 |
| axw | jam: are you around? if so, can you please prod the bot? | 06:03 |
| jam | axw: looking | 06:03 |
| jam | axw: poked, it should be trying to merge rogpeppe1's branch right now (473-parallel-try) | 06:08 |
| axw | thanks jam | 06:08 |
| dimitern | jam, ping | 09:00 |
| jam | dimitern: pong | 09:00 |
| dimitern | jam, i need some bzr surgery :) | 09:00 |
| jam | what's up? | 09:00 |
| dimitern | jam, I removed the current branch I was working on using rmbranch (didn't even get a warning!, but I'm usually careful not to do that) | 09:01 |
| dimitern | jam, and now pretty much every command returns bzr: ERROR: Not a branch: "/home/dimitern/src/juju-core/223-putcharm-uses-charm-upload/.bzr/branch/". | 09:01 |
| dimitern | jam, is there an easy way to reset that and switch to trunk? | 09:02 |
| jam | dimitern: bzr switch trunk --force | 09:03 |
| dimitern | jam, wow! tyvm it worked :) | 09:03 |
| jam | dimitern: by default switch does some sort of introspection on the "current" branch, which obviously doesn't work well when it isn't there, but --force skips that step | 09:03 |
| rogpeppe1 | mornin' all | 09:05 |
| axw | hey rogpeppe1. your branch timed out in the tests in utils/parallel earlier. I couldn't reproduce it on my machine, but haven't looked into the code in detail yet | 09:08 |
| rogpeppe1 | axw: yeah, i just saw that | 09:08 |
| rogpeppe1 | axw: i can't repro either | 09:08 |
| rogpeppe1 | axw: :-( | 09:08 |
| rogpeppe1 | axw: it's a pity that the output for a timed-out test is so meagre | 09:09 |
| axw | indeed, don't even get to see which test it was... | 09:09 |
| rogpeppe1 | yup | 09:09 |
| dimitern | jam, I was already dreading the possibility that I actually need to start from scratch :) | 09:09 |
| rogpeppe1 | axw: i'm just trying with go1.1.2 in case that makes a difference | 09:12 |
| axw | rogpeppe1: okey dokey. I'll do some code inspection to see if I spot anything | 09:12 |
| rogpeppe1 | axw: that would be useful, thanks | 09:12 |
| rogpeppe1 | axw: still passes consistently with go1.1.2 | 09:13 |
| axw | :( | 09:13 |
| rogpeppe1 | axw: i guess i'll try it in a machine in the cloud | 09:39 |
| axw | rogpeppe1: I'd try it on my underpowered VM, but my server's crapped itself | 09:40 |
| dimitern | rogpeppe1, I think you'll like this https://codereview.appspot.com/43330043 | 10:09 |
| rogpeppe1 | dimitern: looking | 10:09 |
| axw | rogpeppe1: it would appear that something in the trySuite is leaking goroutines. I added -test.cpu=1,1,1,1,1,1,... and number of goroutines keeps going up, and runtime keeps going up | 10:20 |
| axw | possibly related, but ... | 10:20 |
| axw | seems unlikely | 10:20 |
| rogpeppe1 | axw: it will probably go up for a while, but then settle down - it leaks a few goroutines that sleep for 4 or 5 seconds | 10:21 |
| axw | rogpeppe1: didn't ever go down, but I may not have waited long enough | 10:21 |
| rogpeppe1 | axw: hmm, i'll have a look | 10:22 |
| axw | I'll keep digging | 10:22 |
| axw | rogpeppe1: trySuite.TestStartBlocksForMaxParallel | 10:24 |
| axw | gtg | 10:24 |
| rogpeppe1 | axw: thanks | 10:24 |
| rogpeppe1 | axw: yeah, i've just repro'd that | 10:27 |
| rogpeppe1 | axw: it would have to be the last test i added... | 10:28 |
| mgz | jamespage: free for a fast catchup? | 10:35 |
| mgz | jamespage: er... actually, ignore me | 10:38 |
| mgz | jam: free for a fast catchup? | 10:38 |
| rogpeppe1 | found the problem! | 10:39 |
| dimitern | rogpeppe1, mgz, jam, standup | 10:45 |
| mgz | dimitern: I'll have to skip as in sprint, but can give update on irc | 10:48 |
| dimitern | mgz, sure, go for it | 10:49 |
| mgz | at mini-sprint in london trying to help the CI guys with testing in charms, testing with charms, and testing juju setups | 10:52 |
| mgz | if there are any questions that come up over the next few days will bug people here for answers where I'm not sure | 10:53 |
| dimitern | rogpeppe1, btw while fixing the test, I have a couple of comments on https://codereview.appspot.com/42760043/ | 11:02 |
| dimitern | rogpeppe1, i'll publish them shortly | 11:02 |
| rogpeppe1 | dimitern: please do - i was about to push and re-approve | 11:03 |
| dimitern | rogpeppe1, sent | 11:06 |
| jam | mgz: sorry I missed you earlier. So when I went to sign up my son yesterday, one of the after school activities didn't show, so I had to go *again* today to actually sign him up. I have a 1:1 with Nate, but then I'm available to chat after that | 11:10 |
| mgz | jam: sure, I'll find a gap | 11:11 |
| dimitern | rogpeppe1, and you've got another review | 11:23 |
| rogpeppe1 | dimitern: ta! | 11:33 |
| axw | rogpeppe1: what was the issue? | 11:48 |
| rogpeppe1 | axw: the loop was spinning | 11:48 |
| rogpeppe1 | axw: so nothing made progress | 11:48 |
| axw | rogpeppe1: in the test or code-under-test? | 11:48 |
| rogpeppe1 | axw: i'm a bit surprised that nothing made progress even with GOMAXPROCS>1 actually | 11:48 |
| rogpeppe1 | axw: in the code under test - it was a bona fide bug | 11:49 |
| rogpeppe1 | axw: the problem was that i was closing t.close, and always reading from it | 11:49 |
| rogpeppe1 | axw: trivially fixed | 11:49 |
| axw | ah, not setting to nil? | 11:49 |
| rogpeppe1 | axw: yeah | 11:49 |
| axw | yeah I see it | 11:49 |
| axw | cool | 11:49 |
| rogpeppe1 | axw: the fix actually made the code slighly simpler, which was nice | 11:50 |
| axw | rogpeppe1: I changed my code to using parallel.Try today. seems a bit more convoluted now due to the need to retry, but I may just have overlooked a simple alternative | 11:55 |
| axw | once your MP is landed I'd appreciate a glance over | 11:55 |
| rogpeppe1 | axw: i'd put the retries inside the functions started by Try | 11:56 |
| axw | rogpeppe1: that's what I did :) | 11:56 |
| dimitern | rogpeppe1, review poke | 11:56 |
| axw | each one is given a channel to wait on that's closed when it should stop retrying | 11:56 |
| rogpeppe1 | axw: Try does that for you, right? | 11:56 |
| rogpeppe1 | dimitern: thanks; getting back to it :-) | 11:57 |
| dimitern | rogpeppe1, cheers :) | 11:57 |
| axw | rogpeppe1: maybe I've overcomplicated it. my current code caters for stopping retries on individual addresses | 11:57 |
| axw | rogpeppe1: for pathological case where addresses come and go | 11:57 |
| axw | maybe it should just keep going forever (until one wins or global timeout) for each address it sees? | 11:58 |
| rogpeppe1 | axw: i'm just wondering that too | 11:59 |
| jam | mgz: poke | 12:14 |
| axw | rogpeppe1: when you have time, https://codereview.appspot.com/40860048/ | 12:17 |
| rogpeppe1 | axw: will do after this | 12:17 |
| axw | the parallel.Try bit is all in provider/common/bootstrap.go | 12:18 |
| axw | rogpeppe1: nps | 12:18 |
| axw | thanks | 12:18 |
| dimitern | rogpeppe1, I was thinking of renaming UploadCharm to UploadLocalCharm actually, because we've been just discussing with william about having an Upload[Charm?]StoreCharm to deal with CS urls | 12:19 |
| rogpeppe1 | dimitern: so you can override the default charm store fetching? | 12:19 |
| rogpeppe1 | dimitern: reviewed | 12:21 |
| dimitern | rogpeppe1, yeah, all the charm upload ops will be performed by these two methods, which will get rid of some code duplication | 12:21 |
| dimitern | rogpeppe1, thanks | 12:21 |
| rogpeppe1 | dimitern: so how would the api Deploy call work? | 12:22 |
| rogpeppe1 | dimitern: would that need to call the api itself? | 12:22 |
| mattyw | fwereade, any news on that branch? is there stuff I need to be fixing? | 12:23 |
| rogpeppe1 | dimitern: sorry, i probably mean the api AddService call | 12:23 |
| dimitern | rogpeppe1, right now there is some shared code between the CLI and API - DeployService | 12:23 |
| fwereade | mattyw, sorry, I'll reapprove, the bot was throwing a fit | 12:23 |
| dimitern | rogpeppe1, I'd like to move all that code into the ServiceDeploy API call and make use of the UploadCharm calls in there | 12:23 |
| rogpeppe1 | dimitern: i think i'm confiu | 12:24 |
| rogpeppe1 | dimitern: i think i'm confused | 12:24 |
| rogpeppe1 | dimitern: are you suggesting that we add an api call UploadStoreCharm? | 12:24 |
| fwereade | rogpeppe1, basically yes | 12:25 |
| rogpeppe1 | dimitern: so how would the ServiceDeploy API call be implemented (server-side)? | 12:25 |
| fwereade | rogpeppe1, it means "go and do this charm-related operation that has hitherto been hidden away inside deploy/upgrade-charm" | 12:26 |
| rogpeppe1 | fwereade: so this is essentially an AddCharm call? | 12:26 |
| fwereade | rogpeppe1, ServiceDeploy would start with "if the charm url isn't already recognised, barf" | 12:26 |
| fwereade | rogpeppe1, yeah | 12:26 |
| rogpeppe1 | fwereade: with no uploading from the client to the server? | 12:26 |
| fwereade | rogpeppe1, we'd have to deprecate | 12:26 |
| fwereade | rogpeppe1, yes | 12:26 |
| dimitern | rogpeppe1, exactly | 12:27 |
| rogpeppe1 | fwereade: ah, that makes sense. but i don't think that "Upload" is great in this context | 12:27 |
| fwereade | rogpeppe1, fair enough, I'm not married to the name | 12:27 |
| rogpeppe1 | fwereade, dimitern: it made me think that it was something that the client was uploading | 12:28 |
| fwereade | rogpeppe1, there's some advantage to having it rhyme with the local version | 12:28 |
| rogpeppe1 | fwereade: perhaps we should call them both Add(Local)?Charm | 12:28 |
| fwereade | rogpeppe1, sounds good? | 12:28 |
| rogpeppe1 | fwereade: AddCharm(*charm.URL); AddLocalCharm(*charm.URL, charm.Charm) | 12:30 |
| rogpeppe1 | dimitern: what do you think? | 12:30 |
| dimitern | rogpeppe1, why Add when we're doing an upload? | 12:33 |
| rogpeppe1 | dimitern: because we want them both to rhyme, and Upload is inappropriate for the other one | 12:33 |
| dimitern | rogpeppe1, in both cases really - there's always a "upload to provider storage" step | 12:33 |
| axw | fwereade: I updated the DestroyEnvironment API based on our previous discussions (https://codereview.appspot.com/41280043/). The API call can still render the environment unusable (apart from destroying things) if there are manual machines, if two API clients race. I'm nervous about allowing them to leak, but if you feel strongly enough I can remove that | 12:33 |
| rogpeppe1 | dimitern: and they're both adding a charm - one just happens to take the charm from locally | 12:34 |
| rogpeppe1 | dimitern: i think of that as an implementation detail - it's downloading as much as uploading | 12:34 |
| fwereade | axw, thanks, I will try to get to it soon, today is meetingy again though | 12:34 |
| dimitern | rogpeppe1, well, i suppose so | 12:34 |
| axw | fwereade: no worries | 12:34 |
| rogpeppe1 | dimitern: in the future, we probably won't store charms in provider storage, so it'll be even more like download-only | 12:35 |
| dimitern | rogpeppe1, AddStoreCharm perhaps? instead of AddCharm - too generic and confusingly similar to AddLocalCharm, without providing a distinction | 12:36 |
| rogpeppe1 | dimitern: i think AddCharm is better because it gives freedom to expand to allow other kinds of URL in the future | 12:36 |
| rogpeppe1 | dimitern: for instance we might allow the API server to download github charms directly. | 12:37 |
| dimitern | rogpeppe1, ok, fair enough | 12:37 |
| rogpeppe1 | dimitern: it's really "add this url" - we just happen to support only cs: prefixes currently | 12:37 |
| rogpeppe1 | dimitern: i just noticed something dubious | 12:38 |
| rogpeppe1 | dimitern: why are you using GetNonValidatingHTTPClient ? | 12:38 |
| dimitern | rogpeppe1, because the API server's cert is self-signed | 12:40 |
| rogpeppe1 | dimitern: no it's not | 12:40 |
| rogpeppe1 | dimitern: it's signed by the CA public key, which we have | 12:40 |
| rogpeppe1 | dimitern: the websocket connection is appropriately verified | 12:41 |
| rogpeppe1 | dimitern: the POST should be too | 12:41 |
| dimitern | rogpeppe1, somehow I couldn't make a connection with it without skipping the validation | 12:42 |
| rogpeppe1 | dimitern: you'll need to add the CA cert to the trusted root keys | 12:42 |
| dimitern | rogpeppe1, the SSL cert for a web server is different than others - domain name needs to match | 12:42 |
| dimitern | rogpeppe1, how do I do that? | 12:43 |
| rogpeppe1 | dimitern: how is the websocket code not talking to the web server? | 12:43 |
| dimitern | rogpeppe1, I don't get any issues with the websocket connections, only with HTTPS ones | 12:43 |
| rogpeppe1 | dimitern: did you try setting the tls config? | 12:43 |
| dimitern | rogpeppe1, where? | 12:44 |
| rogpeppe1 | dimitern: in the http Transport | 12:44 |
| rogpeppe1 | dimitern: one mo, i'll investigate | 12:44 |
| dimitern | rogpeppe1, i'm not sure what are you referring to | 12:44 |
| dimitern | rogpeppe1, when I try using http.DefaultClient instead of utils.GetNonValidatingHTTPClient, I get this http://paste.ubuntu.com/6588688/ | 12:50 |
| rogpeppe1 | dimitern: you can't use http.DefaultClient | 12:50 |
| rogpeppe1 | dimitern: i'm just writing a code fragment for you | 12:50 |
| dimitern | rogpeppe1, why not? | 12:51 |
| rogpeppe1 | dimitern: because DefaultClient uses the default root CA certs, which don't include the juju ones | 12:51 |
| dimitern | rogpeppe1, that seems wrong - inventing a self-signed ca cert and using it | 12:55 |
| rogpeppe1 | dimitern: huh? | 12:55 |
| rogpeppe1 | dimitern: that's the entire basis of our server authentication | 12:55 |
| rogpeppe1 | dimitern: what's wrong with using your own certs? | 12:56 |
| rogpeppe1 | dimitern: anyway, it's not self-signed - it's just a very short chain :-) | 12:57 |
| dimitern | rogpeppe1, well, anyway | 12:57 |
| dimitern | rogpeppe1, it strikes me as odd | 12:57 |
| rogpeppe1 | dimitern: how else can we verify what server we're talking to? | 12:57 |
| dimitern | rogpeppe1, i'm not going there | 12:59 |
| dimitern | rogpeppe1, :) a discussion for another time perhaps | 12:59 |
| rogpeppe1 | dimitern: honestly, if you think there are issues with our security model, it would be good to know about them | 13:00 |
| dimitern | rogpeppe1, i think it's inconvenient for others, that want their own hosted juju stuff to talk with standard clients | 13:01 |
| rogpeppe1 | dimitern: unfortunately i think that juju is fundamentally incompatible with the standard client authentication model | 13:02 |
| rogpeppe1 | dimitern: s/client auth/http server auth/ | 13:02 |
| rogpeppe1 | dimitern: for one, we don't have a well known domain name that the API server lives on | 13:02 |
| dimitern | rogpeppe1, not really - as far as tls is concerned | 13:02 |
| rogpeppe1 | dimitern: for another, we don't want to require everyone that runs a juju API server to buy a certificate | 13:03 |
| dimitern | rogpeppe1, having the private and public key available makes is very easy for anyone to mount MITM attacks against a running environment | 13:04 |
| rogpeppe1 | dimitern: how is the private key available? | 13:04 |
| dimitern | rogpeppe1, well, not very easy, but it's plausible | 13:04 |
| dimitern | rogpeppe1, isn't it in the source? | 13:04 |
| rogpeppe1 | dimitern: no | 13:04 |
| rogpeppe1 | dimitern: it's created when the env is prepared | 13:05 |
| dimitern | rogpeppe1, so how does the server use it for decryption? | 13:05 |
| dimitern | rogpeppe1, ah, ok | 13:05 |
| rogpeppe1 | dimitern: the server is given the private key when it's bootstrapped | 13:05 |
| rogpeppe1 | dimitern: (and in general, tls keys are never used for encryption or decryption) | 13:05 |
| dimitern | rogpeppe1, having the ca cert and key, can you generate a key that works for certain environment? | 13:06 |
| dimitern | rogpeppe1, they are, in the handshake process | 13:06 |
| === gary_poster|away is now known as gary_poster | ||
| rogpeppe1 | dimitern: diffie-hellman isn't really encryption | 13:06 |
| rogpeppe1 | dimitern: if you have the root ca private key, you can generate any number of server certs and keys | 13:07 |
| dimitern | rogpeppe1, signing or encryption - it's all the same | 13:07 |
| rogpeppe1 | dimitern: it's not even signing really | 13:08 |
| dimitern | rogpeppe1, how about generating a *specific* key - preseeding or something, so you can end up with a running env's key pair? | 13:08 |
| rogpeppe1 | dimitern: i don't understand that | 13:09 |
| dimitern | rogpeppe1, i'll check it out | 13:12 |
| dimitern | rogpeppe1, so how about that sample code? | 13:13 |
| rogpeppe1 | dimitern: almost there | 13:14 |
| rogpeppe1 | dimitern: http://paste.ubuntu.com/6588771/ | 13:16 |
| rogpeppe1 | dimitern: (something approximately like that anyway) | 13:16 |
| mattyw | fwereade, failed again - I wonder if something is wrong with the branch - I don't see any of those errors though when I run it locally | 13:17 |
| dimitern | rogpeppe1, thanks, looking | 13:17 |
| === BradCrittenden is now known as bac | ||
| dimitern | rogpeppe1, is the api.Info.CACert always the same or that's the one we're generating? | 13:29 |
| rogpeppe1 | dimitern: it's generated at prepare time currently | 13:30 |
| dimitern | rogpeppe1, so i'm thinking of adding utils.GetClientFromInfo(info *api.Info) *http.Client, which can be reused | 13:31 |
| rogpeppe1 | dimitern: that doesn't seem quite right (and i don't think you'll be able to anyway) | 13:31 |
| dimitern | rogpeppe1, why? | 13:32 |
| rogpeppe1 | dimitern: because you'd get an import cyclre | 13:32 |
| rogpeppe1 | cycle | 13:32 |
| dimitern | rogpeppe1, well, then taking a CACert []byte | 13:33 |
| mgz | rogpeppe1: any reason I shouldn't just land the state api branch? | 13:33 |
| rogpeppe1 | dimitern: that could work. | 13:33 |
| mgz | rogpeppe1: I want to make some more changes, but breakage on a dev version is okay, right? | 13:33 |
| rogpeppe1 | dimitern: although i think it would probably live better in the api package | 13:33 |
| rogpeppe1 | mgz: which branch is that? | 13:33 |
| rogpeppe1 | dimitern: because it's actually all about making an http client that's good for connecting to the api | 13:34 |
| mgz | rogpeppe1: https://codereview.appspot.com/40350043/ | 13:35 |
| rogpeppe1 | dimitern: also, that wouldn't be that convenient to use because the api code actually needs the tls.Config, so would need to type-assert the Client.Transport | 13:35 |
| rogpeppe1 | mgz: ah, the *status* api branch :-) | 13:36 |
| mgz | rogpeppe1: ah yes, whoops | 13:36 |
| * rogpeppe1 woz confused for a moment | 13:36 | |
| dimitern | rogpeppe1, I don't want to copy 20 lines every time I need to connect to the api server though | 13:38 |
| rogpeppe1 | mgz: i have a feeling that rpc calls cannot return both an error and some data | 13:38 |
| rogpeppe1 | dimitern: why would you need to do that? | 13:39 |
| rogpeppe1 | dimitern: the only place you'd be connecting to the api server would be in the api packages, right? | 13:39 |
| mgz | rogpeppe1: really? the api doc didn't say that | 13:39 |
| dimitern | rogpeppe1, how about tests? | 13:39 |
| dimitern | rogpeppe1, it doesn't seem right to use a non-validating client there | 13:40 |
| rogpeppe1 | dimitern: you might want to export api.State.HTTPClient, similar to how Call is exported | 13:40 |
| mgz | rogpeppe1: I'm also not clear on the meaning of returning api.Struct vs *api.Struct from rpc | 13:40 |
| rogpeppe1 | mgz: rpc requires struct returns for all methods | 13:41 |
| rogpeppe1 | mgz: the data is ignored if the error is non-nil... i'm pretty sure - let me check | 13:41 |
| dimitern | rogpeppe1, modify the Caller interface? | 13:41 |
| dimitern | rogpeppe1, that seems way too intrusive | 13:41 |
| mgz | rogpeppe1: as in, not struct pointers? | 13:41 |
| rogpeppe1 | mgz: yup | 13:42 |
| mgz | I'm confused as to why the tests passed then... | 13:42 |
| rogpeppe1 | dimitern: no, you wouldn't necessarily need to modify the Caller interface | 13:42 |
| rogpeppe1 | mgz: perhaps there's nothing that tests partial-status-with-error | 13:42 |
| mgz | rogpeppe1: right, I'm pretty sure there's not for that. but I changed the api return code to *api.Status and if that's illegal I'd expect all the tests to fail | 13:43 |
| rogpeppe1 | mgz: hmm, yes | 13:44 |
| rogpeppe1 | mgz: ah! | 13:44 |
| rogpeppe1 | mgz: perhaps it was just using the fallback code in those tests | 13:45 |
| rogpeppe1 | mgz: did you try running all the tests in the tree? | 13:45 |
| rogpeppe1 | mgz: i'm pretty sure there's one that tests for inappropriately-ignored rpc methods | 13:45 |
| mgz | rogpeppe1: I *thought* so, but will recheck | 13:45 |
| rogpeppe1 | mgz: launchpad.net/juju-core/state/apiserver tests should have caught that | 13:48 |
| rogpeppe1 | mgz: if it didn't then that test is bogus :-) | 13:48 |
| rogpeppe1 | dimitern: yeah, i wouldn't modify the Caller interface - but you can add a method State.HTTPClient() *http.Client | 13:51 |
| rogpeppe1 | dimitern: then api sub-packages that need to make direct http calls can use an interface that's something like: interface{common.Caller; HTTPClient() *http.Client} | 13:53 |
| dimitern | rogpeppe1, what's wrong with having a common function for that? | 13:53 |
| dimitern | rogpeppe1, in utils | 13:53 |
| rogpeppe1 | dimitern: it seems like internal implementation detail of api to me | 13:53 |
| dimitern | rogpeppe1, it's quite useful | 13:53 |
| rogpeppe1 | dimitern: we put stuff in utils that's generally useful | 13:53 |
| rogpeppe1 | dimitern: where else might we want to use it? | 13:54 |
| dimitern | rogpeppe1, in plugins perhaps? | 13:54 |
| rogpeppe1 | dimitern: surely plugins should be using the api package? | 13:54 |
| dimitern | rogpeppe1, i see no harm in add it there | 13:54 |
| rogpeppe1 | dimitern: if we find ourselves wanting it somewhere else, i'd factor it out then (it will be trivial to do so) | 13:55 |
| rogpeppe1 | dimitern: i prefer to avoid expanding utils indefinitely just because we think that something might be generally useful | 13:55 |
| dimitern | rogpeppe1, btw the code you gave me still causes the same error | 13:56 |
| dimitern | rogpeppe1, x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs | 13:56 |
| rogpeppe1 | dimitern: could you paste the code you're using? | 13:56 |
| dimitern | rogpeppe1, http://paste.ubuntu.com/6588953/ | 13:57 |
| dimitern | rogpeppe1, and the caCert i'm passing is the one from api.Info used to call Login | 13:58 |
| rogpeppe1 | dimitern: could you push your branch please? | 13:59 |
| rogpeppe1 | dimitern: aside: it's not right for that code to use a global variable | 13:59 |
| dimitern | rogpeppe1, there's a mutex around it | 14:00 |
| rogpeppe1 | dimitern: it looks like you've cargo-culted from NonValidatingHTTPClient | 14:00 |
| dimitern | rogpeppe1, yes | 14:00 |
| rogpeppe1 | dimitern: what happens if you pass a different CACert in? | 14:01 |
| dimitern | rogpeppe1, where should I get it from? | 14:01 |
| rogpeppe1 | dimitern: where should you get what from? | 14:01 |
| dimitern | rogpeppe1, a different ca cert | 14:02 |
| rogpeppe1 | dimitern: maybe you're talking to two environments at once? | 14:02 |
| rogpeppe1 | dimitern: that's perfectly allowable | 14:02 |
| dimitern | rogpeppe1, i don't think so | 14:03 |
| rogpeppe1 | dimitern: why not? | 14:03 |
| rogpeppe1 | dimitern: i should certainly hope it's ok | 14:03 |
| dimitern | rogpeppe1, why should I be doing that? it's a standard code | 14:03 |
| rogpeppe1 | dimitern: i'd consider it a serious bug otherwise | 14:03 |
| rogpeppe1 | dimitern: what's a standard code? | 14:03 |
| dimitern | rogpeppe1, i mean i'm not doing anything fancy | 14:05 |
| rogpeppe1 | dimitern: regardless of anything else, the output of a function should be, if at all possible, a deterministic function of its arguments | 14:05 |
| dimitern | rogpeppe1, pushed lp:~dimitern/juju-core/225-upload-charm-via-api - if you run the tests in state/api you'll see the error | 14:06 |
| rogpeppe1 | dimitern: if I call GetHTTPClientFromCert(certA); then i call GetHTTPClientFromCert(certB) the second return will return the http client for certA. that's just wrong. | 14:06 |
| dimitern | rogpeppe1, ok, that's true, I'll remove the mutex + caching and just return a new client every time | 14:07 |
| dimitern | rogpeppe1, but that didn't help with the error - i tried already | 14:07 |
| rogpeppe1 | dimitern: no, it wouldn't | 14:07 |
| rogpeppe1 | dimitern: it was just an aside | 14:07 |
| dimitern | rogpeppe1, any clue about the error? | 14:19 |
| rogpeppe1 | dimitern: i'm investigating currently | 14:20 |
| dimitern | rogpeppe1, ok | 14:20 |
| rogpeppe1 | dimitern: this code works fine for me: http://paste.ubuntu.com/6589085/ | 14:24 |
| rogpeppe1 | dimitern: (i get a 401 response which indicates that all the tls handshaking and client cert stuff has worked ok) | 14:25 |
| rogpeppe1 | dimitern: so i suspect you're not actually using the http client that you created | 14:26 |
| dimitern | rogpeppe1, I am using it | 14:26 |
| dimitern | rogpeppe1, but perhaps the caCert from the api.Info used for Login is not the same as in Environ.Config() | 14:26 |
| rogpeppe1 | dimitern: you mean the bug i pointed out above might be triggering the problem? | 14:29 |
| dimitern | rogpeppe1, i'm not using the mutex anymore | 14:31 |
| dimitern | rogpeppe1, so the bug cannot occur | 14:31 |
| dimitern | rogpeppe1, i'm trying to get the environ config and the ca cert in AddLocalCharm to see if it helps | 14:31 |
| rogpeppe1 | dimitern: so which test fails? | 14:31 |
| dimitern | rogpeppe1, TestAddLocalCharm, the only one | 14:33 |
| rogpeppe1 | dimitern: it passes for me | 14:34 |
| rogpeppe1 | dimitern: just to sanity check: what go version are you using? | 14:34 |
| dimitern | rogpeppe1, go version go1.1.2 linux/amd64 | 14:36 |
| rogpeppe1 | dimitern: ok, i'll try with that | 14:36 |
| dimitern | rogpeppe1, using the ca-cert from the environ config didn't help | 14:38 |
| rogpeppe1 | dimitern: ha! i now see the problem - with go1.1.2 | 14:38 |
| dimitern | rogpeppe1, huh? | 14:38 |
| dimitern | rogpeppe1, those things always happen to me :) | 14:38 |
| rogpeppe1 | dimitern: yeah, you seem to be blessed like that | 14:39 |
| dimitern | rogpeppe1, but 1.1.2 is the official version we're building against | 14:39 |
| rogpeppe1 | dimitern: yeah | 14:39 |
| dimitern | rogpeppe1, so what's the underlying issue? | 14:39 |
| rogpeppe1 | dimitern: i'll try to find that out | 14:39 |
| dimitern | rogpeppe1, so I think the only way around this is to use the non-validating client | 14:43 |
| rogpeppe1 | dimitern: we should avoid that if at all possible | 14:43 |
| rogpeppe1 | dimitern: but perhaps for now as a workaround with a huge "BUG HERE" comment | 14:43 |
| dimitern | rogpeppe1, i'm open to suggestions for a workaround the issue | 14:43 |
| dimitern | rogpeppe1, yeah, will do | 14:44 |
| rogpeppe1 | dimitern: hmm, interestingly my little test example still works ok with go1.1.2 | 14:46 |
| rogpeppe1 | dimitern: maybe it's something to do with the cert generation | 14:46 |
| dimitern | rogpeppe1, perhaps | 14:46 |
| rogpeppe1 | dimitern: looks like that's the case | 14:50 |
| rogpeppe1 | dimitern: my test program succeeds or fails depending on whether the server is compiled with go1.1.2 or go1.2 | 14:51 |
| dimitern | rogpeppe1, if you managed to find an issue# to include in the bug report? | 14:51 |
| rogpeppe1 | dimitern: i need to find out what the issue is... | 14:51 |
| dimitern | rogpeppe1, ok, I'll just describe it in general | 14:52 |
| rogpeppe1 | dimitern: give me a little more time | 14:52 |
| rogpeppe1 | dimitern: i'm just about to find out what the difference between the generated certs is | 14:52 |
| dimitern | rogpeppe1, ok, no rush | 14:54 |
| dimitern | rogpeppe1, it seems we're generating certs without Subject_Alternative_Name field - https://groups.google.com/forum/#!topic/golang-nuts/hCCsphIUuBA | 14:57 |
| rogpeppe1 | dimitern: that may be true, but it looks like that's still the case with go1.2, and it works there | 15:01 |
| dimitern | rogpeppe1, x509 in go 1.2 is different than that in 1.1.2 - some more things are supported | 15:04 |
| dimitern | rogpeppe1, specifically the SANs handling has changed, perhaps that's the cause | 15:04 |
| rogpeppe1 | dimitern: possibly. but it seems a bit weird that the client version doesn't matter | 15:05 |
| dimitern | rogpeppe1, although the code handling the validation seems unchanged | 15:07 |
| dimitern | rogpeppe1, weird.. | 15:07 |
| dimitern | rogpeppe1, does https://codereview.appspot.com/43330043 look ok to land? | 15:22 |
| rogpeppe1 | dimitern: looks like the address->serverRoot name change hasn't been pushed yet | 15:24 |
| dimitern | rogpeppe1, oh, sorry, will repropose | 15:24 |
| dimitern | rogpeppe1, done | 15:33 |
| natefinch | fwereade, rogpeppe1: so here's the problem I've been having with mongodb... the default distribution doesn't support SSL, so creating a new mongo server is failing. That's at least why it's failing locally on my machine. The problem is, I don't know how it was working before, since I'm pretty sure I was running a generic build before, too. | 15:51 |
| rogpeppe1 | natefinch: i can't see how you could have run any juju tests with default distribution - we've required that for ages | 15:52 |
| natefinch | rogpeppe1, I must have gotten the special version and forgotten about it, then when I rolled back to 2.2 yesterday, I messed it up. | 15:53 |
| natefinch | rogpeppe1, where do I get the SSL-ified version? | 15:53 |
| rogpeppe1 | natefinch: i'm not sure i'm afraid | 16:01 |
| rogpeppe1 | natefinch: you can probably apt-get it from somewhere | 16:01 |
| rogpeppe1 | natefinch: when you find out, let me know, 'cos i'd like to upgrade to mongo 2.4... | 16:01 |
| rogpeppe1 | dimitern: i posted a comment on your CL | 16:01 |
| === teknico_ is now known as teknico | ||
| dimitern | rogpeppe1, let's not use rpc.NoSuchRequest, because it's nothing like it | 16:02 |
| rogpeppe1 | dimitern: in which case, let's unify the two | 16:03 |
| dimitern | rogpeppe1, the transport is different as well | 16:03 |
| rogpeppe1 | dimitern: so an api client doesn't have to use several different ways of determining errors | 16:03 |
| natefinch | rogpeppe1, sounds like it has to be built from source or purchased | 16:03 |
| dimitern | rogpeppe1, there are only 2 ways | 16:03 |
| rogpeppe1 | dimitern: sure, but there should only be one | 16:04 |
| dimitern | rogpeppe1, and there are 2 transports underneath | 16:04 |
| rogpeppe1 | dimitern: the transport is an implementation detail | 16:04 |
| dimitern | rogpeppe1, so you propose to make NoSuchRequest a NotImplementedError | 16:04 |
| rogpeppe1 | dimitern: i propose to define CodeNotImplemented and make a non-implemented error just like all the other errors defined in params | 16:05 |
| rogpeppe1 | dimitern: so that if you're using state/api/* you only have one way of classifying errors | 16:05 |
| dimitern | rogpeppe1, and how is that going to work for rpc.NoSuchRequest? | 16:06 |
| rogpeppe1 | dimitern: one possibility is outlined in my comment | 16:06 |
| rogpeppe1 | dimitern: another possibility is to change api.State.Call so that it transforms the rpc error as appropriate | 16:06 |
| dimitern | rogpeppe1, so we can return serverError(&Error{Message: "not impl. blah", Code: params.CodeNotImplemented}) instead of NotImplementedError, and make rpc.NoSuchRequest ErrorCoder-compatible, using params.CodeNotImplemented as code | 16:10 |
| rogpeppe1 | dimitern: i *think* params should import rpc rather than the other way around | 16:11 |
| rogpeppe1 | dimitern: because we might make rpc into an external package at some point | 16:11 |
| rogpeppe1 | dimitern: and serverError is for server-side not client-side use | 16:12 |
| rogpeppe1 | dimitern: this is a change that really should be part of another CL; hence i suggest the quick fix so you can land this, then fix all calls to rpc.IsNoSuchRequest to call params.IsNotImplemented instead | 16:14 |
| dimitern | rogpeppe1, problem is, it already landed, sorry | 16:15 |
| dimitern | rogpeppe1, but i'm preparing a follow-up | 16:15 |
| rogpeppe1 | dimitern: ok, thanks | 16:15 |
| * dimitern will be back soon | 16:17 | |
| TheMue | hmm, ears are still closed and whistling, hope it will be better tomorrow. but at least my tests are working fine so far. | 16:19 |
| dimitern | rogpeppe1, how do you suggest doing "make the rpc package return error code CodeNoSuchRequest on method-not-defined | 16:59 |
| dimitern | error" | 16:59 |
| dimitern | rogpeppe1, where should I catch and report this error? | 17:00 |
| rogpeppe1 | dimitern: ok, there are few aspects to this: | 17:01 |
| rogpeppe1 | dimitern: 1) in rpc/rpcreflect, define type NoSuchMethodError {Method, RootMethod string} | 17:02 |
| dimitern | rogpeppe1, ok | 17:02 |
| rogpeppe1 | dimitern: 2) change rpcreflect.MethodCaller to return that error instead of using fmt.Errorf | 17:04 |
| dimitern | rogpeppe1, ok, but why do I need RootMethod if I never set it? | 17:05 |
| rogpeppe1 | dimitern: 3) in rpc, define const CodeNotImplemented = "not implemented" | 17:06 |
| rogpeppe1 | dimitern: because you'll need it to define the Error return | 17:06 |
| rogpeppe1 | dimitern: s/return/method | 17:06 |
| rogpeppe1 | dimitern: (more specifically, NoSuchMethodError.Error) | 17:06 |
| dimitern | rogpeppe1, I can do that with only NoSuchMethodError.Method, I don't need RootMethod | 17:07 |
| === gary_poster is now known as gary_poster|away | ||
| rogpeppe1 | dimitern: the message should not change, which means that it should include the root method name too | 17:07 |
| rogpeppe1 | 4) in rpc, change bindRequest to check if MethodCaller returns a *NoSuchMethodError, and return RequestError{Message: err.Error(); Code: CodeNotImplemented} if so | 17:09 |
| dimitern | rogpeppe1, in func (t *Type) Method(name string), the only place NoSuchMethodError is returned, I don't have the rootmethod, so I can't set the RootMethod field | 17:09 |
| dimitern | rogpeppe1, ok, so in ObjType.Method as well - but same case - RootMethod is unknown | 17:10 |
| rogpeppe1 | dimitern: ha, i'd forgotten about ErrNoSuchMethod! | 17:10 |
| dimitern | rogpeppe1, yeah, that's what you meant, right? | 17:11 |
| rogpeppe1 | dimitern: no, i was talking about the error returned from Value.MethodCaller | 17:11 |
| dimitern | rogpeppe1, aha! | 17:12 |
| dimitern | rogpeppe1, but Type.Method should also change, right? | 17:12 |
| rogpeppe1 | dimitern: i'm just thinking over that | 17:12 |
| dimitern | rogpeppe1, perhaps there, i can use Type.root.Name() to populate the root method | 17:13 |
| rogpeppe1 | dimitern: there are two places that currently return ErrMethodNotFound | 17:14 |
| dimitern | rogpeppe1, yes both Method() methods on Type and ObjType | 17:14 |
| rogpeppe1 | dimitern: i think we should leave type.go unchanged | 17:15 |
| dimitern | rogpeppe1, won't it be confusing to have both NoSuchMethodError and ErrNoSuchMethod? | 17:15 |
| rogpeppe1 | dimitern: and implement a new type, say CallNotImplementedError | 17:15 |
| dimitern | rogpeppe1, sounds reasonable | 17:16 |
| rogpeppe1 | dimitern: i hope that's sufficiently different to be not confusing | 17:16 |
| dimitern | rogpeppe1, ok | 17:18 |
| rogpeppe1 | 5) in rpc, in Conn.handleResponse, change it to set call.Error.Code = CodeNotImplemented if the error message begins "no such request" or "unknown object type" | 17:20 |
| rogpeppe1 | dimitern: (with a "deprecate this" comment, because it's only for compatibility) | 17:20 |
| rogpeppe1 | 6) in rpc, change IsNoSuchRequest to check ErrorCode==CodeNotImplemented | 17:22 |
| rogpeppe1 | 6b) rename IsNoSuchRequest to IsNotImplemented, or delete entirely. | 17:22 |
| rogpeppe1 | 7) define params.CodeNotImplemented = rpc.CodeNotImplemented | 17:23 |
| rogpeppe1 | 8) implement params.IsNotImplemented | 17:23 |
| rogpeppe1 | 9) 8) change all callers of rpc.IsNoSuchRequest to use params.IsNotImplemented instead | 17:23 |
| rogpeppe1 | 5a) ... only if the error code is not already set | 17:24 |
| rogpeppe1 | dimitern: i think that's about it | 17:24 |
| dimitern | rogpeppe1, you know, the time it took to explain to me how to do it, you could've probably done it and put it up for review :) | 17:36 |
| rogpeppe1 | dimitern: it would have taken that amount of time just to run lbox propose once :-( | 17:37 |
| dimitern | rogpeppe1, in 5) I presume you mean handleRequest, not response | 17:37 |
| dimitern | rogpeppe1, or perhaps writeErrorResponse | 17:38 |
| rogpeppe1 | dimitern: no, i did mean handleResponse | 17:39 |
| dimitern | rogpeppe1, there's no such method | 17:39 |
| dimitern | rogpeppe1, ah, sorry | 17:39 |
| rogpeppe1 | dimitern: in the "case hdr.Error != "":" branch of the switch | 17:39 |
| dimitern | rogpeppe1, i'm blind | 17:39 |
| rogpeppe1 | dimitern: sorry, i should've said it's in client.go | 17:39 |
| dimitern | rogpeppe1, but there I don't have the error anymore, so I have to do strings.HasPrefix there | 17:40 |
| rogpeppe1 | dimitern: yeah, same as IsNoSuchRequest does now | 17:40 |
| dimitern | rogpeppe1, well ok | 17:41 |
| rogpeppe1 | dimitern: lack of foresight on my part i'm afraid - i should have implemented it as an error code when i first did error codes | 17:41 |
| dimitern | rogpeppe1, but hdr.Error is a string, so I have to convert it to an error just to check it's string rep | 17:41 |
| rogpeppe1 | dimitern: huh? | 17:42 |
| rogpeppe1 | dimitern: it *is* its string representation :-) | 17:42 |
| dimitern | rogpeppe1, but IsNoSuchRequest takes an error | 17:42 |
| rogpeppe1 | dimitern: don't call IsNoSuchRequest | 17:42 |
| rogpeppe1 | dimitern: see 6) | 17:43 |
| rogpeppe1 | dimitern: i tried to formulate the CA cert problem in a standalone program and couldn't reproduce the issue. http://play.golang.org/p/WVM3GfphOu | 17:52 |
| rogpeppe1 | dimitern: (which works fine under both go1.1.2 and go1.2, bother it!) | 17:52 |
| dimitern | rogpeppe1, so apparently something is wrong with how our code handles certs | 17:53 |
| rogpeppe1 | dimitern: i'm not making any assumptions | 17:54 |
| rogpeppe1 | dimitern: as far as i know, that *is* our code... and it works | 17:54 |
| dimitern | rogpeppe1, maybe there are some subtleties which are intervening | 18:00 |
| dimitern | rogpeppe1, anyway, I'm almost ready with the CL | 18:01 |
| dimitern | rogpeppe1, so no changes needed in bindRequest after all | 18:11 |
| rogpeppe1 | dimitern: no? | 18:11 |
| dimitern | rogpeppe1, the ones in handleRequest did the job, otherwise with changes to bindRequest i get double wrapping | 18:11 |
| dimitern | error string = "request error: request error: no such request - method SimpleMethods.No is not implemented (not implemented) (not implemented)" | 18:11 |
| rogpeppe1 | dimitern: do you mean handleResponse there? | 18:12 |
| dimitern | rogpeppe1, oh, yeah right | 18:12 |
| rogpeppe1 | dimitern: see 5a) | 18:12 |
| rogpeppe1 | dimitern: i think you should make the change to handleRequest | 18:12 |
| dimitern | rogpeppe1, yeah, i'm doing ths | 18:12 |
| dimitern | rogpeppe1, which change? | 18:13 |
| rogpeppe1 | dimitern: because the prefix checking is wrong, and should only be used for compatibility | 18:13 |
| rogpeppe1 | dimitern: the change to set the error code in the returned error | 18:13 |
| dimitern | rogpeppe1, ok, before my head blows, let me propose it, so i can have your comments in one place please | 18:13 |
| rogpeppe1 | dimitern: oops, sorry, i meant bindRequest... | 18:13 |
| rogpeppe1 | dimitern: ok, np | 18:14 |
| natefinch | So, does anyone know *if* we're installing mongo 2.4 in the field, and if so, where we get it from? I have directions from TheMue from when I joined to download 2.2 from an amazon s3 address, but none of the likely 2.4 names are available from that bucket. I am trying to build mongo myself, but their directions appear to be incomplete, because it doesn't build successfully. | 18:16 |
| dimitern | rogpeppe1, ok, so it's not really working fully | 18:18 |
| dimitern | rogpeppe1, but I'll propose it, so you can pull it and make suggestions how to do it as you think is should be done | 18:18 |
| dimitern | rogpeppe1, so only the rpc testBadCall tests fail, all others pass - take a look please and have your say https://codereview.appspot.com/38540044 | 18:27 |
| * dimitern reached eod | 18:27 | |
| rogpeppe1 | dimitern: looking | 18:34 |
| rogpeppe1 | dimitern: this fixes those tests: lp:~rogpeppe/juju-core/dimitern-225-unify-not-implemented-errors/ | 19:04 |
| * rogpeppe1 has also reached eod | 19:14 | |
| rogpeppe1 | g'night all | 19:15 |
| thumper | night rogpeppe1 | 19:15 |
| thumper | fwereade: you around at all? | 19:15 |
| thumper | fwereade: I'd like to chat design issues around juju run with you, I'm going to make coffee but back shortly | 19:17 |
| === gary_poster|away is now known as gary_poster | ||
| dimitern | thumper, he's afk atm | 19:49 |
| thumper | hi dimitern | 19:54 |
| thumper | dimitern: I kinda guessed that :) | 19:54 |
| thumper | i'm trying the ping with context :) | 19:54 |
| natefinch | thumper, do you know where/if we keep binaries for mongo 2.4? | 19:54 |
| dimitern | thumper, :) just an on-site observation | 19:54 |
| thumper | um... | 19:54 |
| thumper | natefinch: look in the cloudinit code | 19:55 |
| thumper | it has code to add the source for mongodb | 19:55 |
| natefinch | thumper, it looks like it's just in the juju/stable ppa, which means it's just 2.2.4 | 20:10 |
| thumper | natefinch: ah, that may be our patched version of things | 20:11 |
| thumper | natefinch: is there mongodb-server in the cloud archive? | 20:12 |
| natefinch | thumper: not sure how to check that | 20:20 |
| natefinch | thumper: I guess other than deploy a server and go look | 20:21 |
| natefinch | thumper: that says 2.4.6. Just not sure where it's from | 20:24 |
| natefinch | I actually started building 2.4.8.... holy crap C++ builds slowly | 20:36 |
| marcoceppi | natefinch: is this accurate? https://juju.ubuntu.com/docs/authors-charm-interfaces.html#documenting-interfaces | 20:53 |
| marcoceppi | The gets/sets stuff | 20:53 |
| marcoceppi | nate, or anyone else | 20:54 |
| natefinch | marcoceppi, doesn't sound familiar | 21:00 |
| marcoceppi | I didn't think so. I'm going to remove it | 21:00 |
| natefinch | nothing like a 15 minute build that fails at the end. | 21:02 |
| thumper | gary_poster: you around? | 21:18 |
| gary_poster | thumper, yeah, hey | 21:19 |
| thumper | gary_poster: can fwereade and I ask you a few questions around the UI and API in a hangout? | 21:19 |
| thumper | gary_poster: https://plus.google.com/hangouts/_/7acpijrir4uqnfkk2qgfdtrgpk?hl=en | 21:19 |
| gary_poster | thumper, of course. coming | 21:19 |
| === gary_poster is now known as gary_poster|away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!