| natefinch | axw: the bug about cookies not getting deleted is what I'm working on :) | 00:31 |
|---|---|---|
| alexisb | menn0, axw, wallyworld, katco just sent a prosed tech board agenda item | 00:50 |
| alexisb | please let me know if you have questoins/concern about my request | 00:50 |
| wallyworld | ok | 00:50 |
| natefinch | axw: https://cdn.meme.am/instances/500x/72177725.jpg | 01:00 |
| menn0 | alexisb: ok | 01:13 |
| menn0 | axw: ping | 01:29 |
| axw | menn0: pong | 01:33 |
| menn0 | axw: regarding https://github.com/juju/juju/blob/master/api/apiclient.go#L575 | 01:34 |
| menn0 | axw: it looks like when the conn is closed another Ping call is attempted before the heartbeatmonitor can finish | 01:34 |
| menn0 | axw: should there be a short circuit in the <-s.closed case so it just does close(s.broken); return? | 01:35 |
| menn0 | axw: this isn't causing a specific issue, i'm just poking around in the area | 01:35 |
| * axw looks | 01:36 | |
| axw | menn0: yeah, I think so | 01:36 |
| menn0 | axw: ok cool. | 01:37 |
| menn0 | axw: i'm also going to try out adding handling for the websocket conn's Dead channel | 01:37 |
| alexisb | menn0, axw, wallyworld any questions before I log off> | 01:37 |
| alexisb | ? | 01:37 |
| axw | alexisb: sorry just got back, looking now | 01:38 |
| wallyworld | alexisb: why are you guys voting for trump? | 01:38 |
| axw | menn0: SGTM | 01:38 |
| menn0 | alexisb: I think your tech board item is clear enough | 01:38 |
| alexisb | to amuse you wallyworld | 01:38 |
| wallyworld | lol | 01:38 |
| menn0 | although it'll blow out the meeting beyond an hour :) | 01:38 |
| alexisb | and because I cannot eat a turd sandwich | 01:38 |
| alexisb | menn0, yes do the best you can | 01:39 |
| alexisb | if we can get the start of a list I can work with folks individually on details | 01:40 |
| menn0 | axw: i'm seeing cases in my HA testing where the api-caller takes quite a while to notice that the apiserver has stopped because the conn is only tried every ping timeout and I guess the ping attempt takes a while to time out too | 01:40 |
| axw | alexisb: sounds good. I don't think we'll get it all done in one meeting of course, but we can make a start | 01:40 |
| menn0 | axw: checking the underlying websocket Dead might speed things up | 01:40 |
| alexisb | axw, understood and thank you | 01:40 |
| alexisb | alrighty all I am off for the night chat with you tomorrow | 01:40 |
| menn0 | alexisb: good night | 01:40 |
| axw | good night | 01:40 |
| axw | menn0: hrm, what channel are you talking about? | 01:41 |
| * menn0 finds | 01:42 | |
| menn0 | axw: sorry I said the websocket conn | 01:42 |
| menn0 | axw: I meant rpc.Conn - it has a Dead() method | 01:43 |
| axw | menn0: ah right | 01:43 |
| axw | wallyworld: is it https://github.com/juju/juju/pull/6373 that I'm reviewing? | 01:47 |
| * axw was half asleep in standup | 01:47 | |
| wallyworld | axw: yeah, ta | 01:47 |
| natefinch | axw, wallyworld: any thoughts on this? https://github.com/juju/persistent-cookiejar/pull/16#pullrequestreview-2813363 | 01:55 |
| wallyworld | natefinch: i'm not too sure; i don't know a lot about cookies offhand | 01:59 |
| menn0 | thumper, wallyworld or axw: https://github.com/juju/juju/pull/6379 | 01:59 |
| wallyworld | menn0: i'll swap https://github.com/juju/names/pull/76 | 01:59 |
| natefinch | wallyworld: np | 01:59 |
| axw | natefinch: sorry, likewise. I think you're going to have to interrogate rogpeppe1 | 01:59 |
| natefinch | axw: kk | 02:00 |
| menn0 | wallyworld: fair enough | 02:00 |
| wallyworld | menn0: that one is a pre-req for a much bigger juju core change to remove @local | 02:00 |
| wallyworld | which i've done but need to write upgrade steps, yay | 02:01 |
| menn0 | wallyworld: understood | 02:01 |
| menn0 | wallyworld: sorry, I just remembered I need QA steps on that | 02:02 |
| * menn0 adds | 02:02 | |
| wallyworld | menn0: that's one think i dislike about gh reviews, no qa steps section. plus all the other disadvantages. when are we going to go back to something better like reviewboard? | 02:02 |
| menn0 | wallyworld: we're still in our 2 week trial. after that we get everyone's opinions/votes and see where we land. | 02:03 |
| babbageclunk | menn0: Oops couldn't sleep | 02:03 |
| wallyworld | menn0: i've been away for 2 weeks, i was hoping it would be done and dusted :-) | 02:04 |
| menn0 | wallyworld: maybe the trial's over then? We'll have to check the date when we started. | 02:05 |
| menn0 | babbageclunk: haha :) | 02:05 |
| menn0 | babbageclunk: just b/c you can't sleep doesn't mean you have to work :) | 02:06 |
| menn0 | wallyworld: QA steps added | 02:08 |
| wallyworld | ta | 02:08 |
| wallyworld | that's anothe thing - now we are foced to pollute commit messages with QA steps | 02:09 |
| wallyworld | since there's no separate section | 02:09 |
| wallyworld | menn0: lgtm. i like simple fixes like that | 02:16 |
| menn0 | wallyworld: thanks - figuring out what was going on was anything but simple but the fix was straight forward | 02:16 |
| wallyworld | menn0: it reminds me of that joke about the car mechanic | 02:17 |
| menn0 | wallyworld: and some of the other changes I'd like to do around this are a lot less simpler :)_ | 02:17 |
| menn0 | wallyworld: your change LGTM. I had actually reviewed it a while ago but forgot to end the review. | 02:22 |
| wallyworld | menn0: ta. am still writing the upgrade steps anyway | 02:22 |
| babbageclunk | menn0: It doesn't look like the pool gets hit if I try to do something with a removed model, but I think that might be because the client store knows the model's gone away. | 02:24 |
| babbageclunk | menn0: Or rather, it's because it tries to find the model by name first from the controller. | 02:27 |
| babbageclunk | menn0: Is there a way to destroy the model from a different client so that the modeluuid is still in the client store? | 02:27 |
| babbageclunk | menn0: I'm beginning to think I should do the refcounting anyway - relying on implementation details of the api server to ensure that the state isn't in use when we try to close it seems fragile. | 02:31 |
| babbageclunk | menn0: And even if it does today there's no guarantee it will in the future. | 02:32 |
| babbageclunk | menn0: Oops, out of battery - send me an email if you have strong feelings about the above? | 02:44 |
| menn0 | babbageclunk: that all sounds good to me | 02:46 |
| thumper | menn0: do you still need a teddy bear? | 03:10 |
| menn0 | thumper: have been looking into other things but yes | 03:10 |
| menn0 | thumper: now? | 03:10 |
| thumper | sure | 03:11 |
| thumper | hangout? | 03:11 |
| menn0 | thumper: 1:1? | 03:12 |
| thumper | there already | 03:12 |
| menn0 | ok | 03:12 |
| wallyworld | axw: i think we can drop migrateLegacyAccounts() for accounts.yaml now right? i think it was for beta13->14? | 03:23 |
| axw | probably, looking | 03:23 |
| wallyworld | i be doing an implementation to migrate the @local stuff | 03:24 |
| axw | wallyworld: yep, please drop what's there | 03:24 |
| wallyworld | will do ta | 03:24 |
| axw | wallyworld: it would be nice if we had some general client-side upgrade steps. maybe in some distant future | 03:25 |
| wallyworld | yeah | 03:25 |
| veebers | menn0: would you happen to know where I might be able to file a bug against mongostat that I got from http://repo.mongodb.org? The most recent changes output a little bit (unexpected) | 04:59 |
| axw | wallyworld: gotta go pick up my car, will bbs | 05:54 |
| wallyworld | ok | 05:54 |
| wallyworld | the new one? | 05:54 |
| axw | wallyworld: yeah just had to take it for its first service | 05:54 |
| wallyworld | ok | 05:55 |
| === frankban|afk is now known as frankban | ||
| menn0 | jam: tech board? | 08:04 |
| jam | menn0: yep, just got 2fa'd, brt | 08:04 |
| macgreagoir | rogpeppe1: This is to fix one you mentioned to me last week, in case you'd like to look ;-) https://github.com/juju/juju/pull/6381 | 08:38 |
| === rogpeppe1 is now known as rogpeppe | ||
| rogpeppe | macgreagoir: looking | 08:45 |
| macgreagoir | Cheers | 08:46 |
| rogpeppe | macgreagoir: LGTM - that was a hard one :) | 08:46 |
| macgreagoir | :-D | 08:47 |
| macgreagoir | If it were OpenStack, I'd get to ODS for free! | 08:47 |
| babbageclunk | voidspace: ping? | 10:04 |
| voidspace | babbageclunk: pong | 10:24 |
| babbageclunk | voidspace: hey, do you know about the AllModelWatcher? | 10:25 |
| voidspace | babbageclunk: not really, sorry | 10:25 |
| voidspace | babbageclunk: don't think I've ever looked at it | 10:25 |
| babbageclunk | Hmm. | 10:26 |
| voidspace | babbageclunk: what's up with it, what do you need to know? | 10:28 |
| babbageclunk | voidspace: Trying to change the StatePool so it closes the State objects it holds when the corresponding model goes away. | 10:29 |
| voidspace | babbageclunk: ah :-) | 10:29 |
| voidspace | babbageclunk: sounds like fun | 10:29 |
| babbageclunk | voidspace: It's the source of a memory leak when someone does lots of add/destroy models. | 10:29 |
| voidspace | right | 10:31 |
| babbageclunk | voidspace: worryingly, I can see that the AllModelWatcher contains a StatePool, so adding an AllModelWatcher to the StatePool seems like a bad idea. | 10:31 |
| voidspace | babbageclunk: so important for OIL but not for anyone in real life | 10:31 |
| voidspace | hah, yes | 10:31 |
| voidspace | babbageclunk: so maybe make the change in the AllModelWatcher calling back into the StatePool | 10:32 |
| voidspace | babbageclunk: or can you have multiple state pools, not all associated with an AllModelWatcher | 10:32 |
| babbageclunk | voidspace: yes that | 10:32 |
| babbageclunk | voidspace: there's also one inside the API server. | 10:32 |
| voidspace | babbageclunk: gah, you either need a registry or you need a new watcher | 10:33 |
| babbageclunk | voidspace: Probably a new watcher - it looks like the AllModelWatcher hoovers up way more than this one would care about. | 10:33 |
| voidspace | babbageclunk: a new watcher is more ju-thonic | 10:33 |
| babbageclunk | voidspace: I only want to be notified when a model is removed. | 10:34 |
| babbageclunk | jujuic? | 10:34 |
| voidspace | heh | 10:34 |
| voidspace | ModelRemovalWatcher | 10:34 |
| babbageclunk | yeah, I guess. | 10:35 |
| babbageclunk | voidspace: Hah, I guess that's State.WatchModels then | 10:36 |
| voidspace | cool | 10:36 |
| babbageclunk | voidspace: Thanks! | 10:36 |
| babbageclunk | wallyworld: ping? | 11:05 |
| wallyworld | hey | 11:06 |
| babbageclunk | wallyworld: I'm trying to work out how to add a model watcher to the StatePool. | 11:06 |
| wallyworld | i've not done anything with the state pool per se. i've done watchers, but on mgo collections | 11:07 |
| voidspace | mgz: ping | 11:08 |
| wallyworld | i can take a look at the code | 11:08 |
| babbageclunk | wallyworld: Does that mean I need to make StatePool into a worker? | 11:08 |
| babbageclunk | wallyworld: Thanks - with Will gone and Dimiter out I'm not sure who to bug about this stuff over here. | 11:09 |
| wallyworld | babbageclunk: so the state pool manages a collections of states i assume. but that's not a db/mgo construct is it? | 11:10 |
| babbageclunk | wallyworld: no, I don't think so (not sure what you mean though) | 11:11 |
| wallyworld | babbageclunk: so normally our watchers are constructs that notify when something in the db changes | 11:12 |
| wallyworld | eg adding to a collection | 11:12 |
| babbageclunk | wallyworld: At the moment it's a cache of States, but there's no way to have the States get closed when the model is removed, so there's a leak. | 11:12 |
| babbageclunk | wallyworld: Yeah, I *think* State.WatchModels will give me the signals I want. | 11:13 |
| wallyworld | babbageclunk: dumb question not having seen the code - why can't we close state objects when they are removed from the cache? | 11:13 |
| babbageclunk | wallyworld: At the moment nothing removes them. (Except when the pool itself is closed, at api server shutdown time.) | 11:13 |
| wallyworld | oh, i see, a cache that is not managed correctly | 11:14 |
| wallyworld | and you want to know when a model object is removed from state | 11:14 |
| babbageclunk | only two hard problems in computering. | 11:14 |
| babbageclunk | yup | 11:14 |
| wallyworld | do we should have a model watcher on state | 11:15 |
| wallyworld | but could we do something in the undertaker? | 11:15 |
| wallyworld | do we have an existing process than deals with dying models? | 11:15 |
| wallyworld | that we can hook in to? | 11:15 |
| babbageclunk | wallyworld: hang on, looking at what the undertaker listens to. | 11:16 |
| wallyworld | may be prudent just to do something as part of the undertaker workflow; but not sure without looking through the code to see the dying model workflow | 11:16 |
| wallyworld | bbiab, got to go get kid from band practice | 11:16 |
| perrito666 | k, need to step out for the next ~4 hs, ill be available on mail | 11:47 |
| mgz | voidspace: hey, can I help? | 12:22 |
| voidspace | mgz: still struggling to connect to company VPN | 12:33 |
| voidspace | mgz: Spads has been helping me in #is - I'm about to try US gateway | 12:33 |
| voidspace | mgz: I get "Could not find source connection" in /var/log/syslog for UK gateway | 12:34 |
| mgz | voidspace: gotcha | 12:34 |
| mgz | if you don't get anywhere with the vpn, come back and poke me again | 12:34 |
| voidspace | mgz: so thanks, I'll weep loudly and gnash my teeth if it doesn't work | 12:35 |
| voidspace | heh, thanks | 12:35 |
| mgz | because we have a machine in canonistack we use to let our ci setup jump through | 12:35 |
| mgz | that is an alternative to getting vpn working | 12:35 |
| natefinch | rogpeppe: can we talk cookies? | 13:03 |
| rick_h_ | mhilton: ^ have a sec for natefinch ? | 13:26 |
| urulama | natefinch, rick_h_: rogpeppe is afk for lunch. he'll be back in 20min or so | 13:28 |
| voidspace | rick_h_: grabbing coffee, with you in 5 | 13:29 |
| rick_h_ | voidspace: stuck in another call, will be a bit late | 13:29 |
| voidspace | rick_h_: cool | 13:29 |
| voidspace | mgz: I get the same error with the US VPN gateway | 13:50 |
| voidspace | mgz: "Could not find source connection" | 13:50 |
| voidspace | mgz: and even though "us-mfoord VPN" is on in network manager it remains "Not connected" | 13:51 |
| voidspace | mgz: can you help me diagnose? | 13:51 |
| rogpeppe | natefinch: sure | 13:52 |
| natefinch | rogpeppe: I know you said that the cookies don't need to round trip... except that they do with RemoveCookie | 13:52 |
| natefinch | rogpeppe: thus - https://github.com/juju/persistent-cookiejar/pull/16 | 13:53 |
| rogpeppe | natefinch: hmm, i'd forgotten that RemoveCookie existed. let me have a look. | 13:54 |
| mgz | voidspace: what's the final ip you're trying to get to? | 13:54 |
| mgz | we may as well try the low-fi method | 13:54 |
| voidspace | mgz: I'm trying to get access to vsphere | 13:54 |
| voidspace | mgz: so 10.245.0.131 I think | 13:54 |
| rogpeppe | natefinch: how are you intending to use it? | 13:55 |
| mgz | voidspace: okay, this works for me | 13:56 |
| mgz | (well, I get prompted for a password, but I presume you can get by that) | 13:56 |
| voidspace | mgz: I have the vsphere creds from cloud-city | 13:56 |
| natefinch | rogpeppe: when we log out of a controller, we need to remove the cookie we saved for the controller, to force the user to re-enter their password | 13:57 |
| rogpeppe | natefinch: how are you planning on getting that cookie? | 13:57 |
| rogpeppe | natefinch: scratch that, it's obvious :) | 13:57 |
| natefinch | rogpeppe: I hope your obvious solution is the same as mine | 13:58 |
| rogpeppe | natefinch: i think you'll need to use AllCookies | 13:58 |
| rogpeppe | natefinch: and search through for any cookies with a matching host | 13:59 |
| natefinch | rogpeppe: isn't that what Cookies(net.URL) is for? | 13:59 |
| rogpeppe | natefinch: not really | 13:59 |
| rogpeppe | natefinch: that method is for getting cookies to send as part of a web request | 14:00 |
| rogpeppe | natefinch: i think it's best to keep to the standard implementation for that | 14:00 |
| natefinch | rogpeppe: right, which is what we'd send up as authentication, which means those are the ones that we want to remove | 14:00 |
| rogpeppe | natefinch: it might be nice to have a RemoveAll(url) method | 14:01 |
| rogpeppe | natefinch: which would remove all cookies that would be sent to the given URL | 14:01 |
| natefinch | how is that not this? | 14:01 |
| natefinch | hopefully they'll recognize that yoyu | 14:01 |
| natefinch | oops, wrong paste | 14:01 |
| natefinch | for _, c := range jar.Cookies(st.cookieURL) { | 14:01 |
| natefinch | j.RemoveCookie(c) | 14:01 |
| natefinch | } | 14:01 |
| katco | natefinch: standup time | 14:02 |
| rogpeppe | natefinch: because the Cookies method doesn't fill in the domain name | 14:02 |
| natefinch | that's the BUG | 14:02 |
| rogpeppe | natefinch: and that seems to be deliberate | 14:02 |
| rogpeppe | natefinch: then raise it in golang.org/issue | 14:02 |
| katco | voidspace: standup time | 14:02 |
| voidspace | katco: omw | 14:03 |
| natefinch | rogpeppe: it did seem deliberate, which is why I posted on golang-nuts about it, but no one has responded yet. I could go directly to filing an issue, might get a better response | 14:04 |
| rogpeppe | natefinch: i think the answer is probably the answer i gave you - you don't need to encode the domain name when sending a cookie | 14:05 |
| natefinch | rogpeppe: but it also prevents you from updating a cookie. Being able to round trip is usually good practice, to prevent surprising behavior | 14:06 |
| rogpeppe | natefinch: this document is relevant: String returns the serialization of the cookie for use in a Cookie header (if only Name and Value are set) or a Set-Cookie response header (if other fields are set). If c is nil or c.Name is invalid, the empty string is returned. | 14:08 |
| rogpeppe | s/document/comment/ | 14:08 |
| rogpeppe | natefinch: so if we change the implementation of Cookies, we'll be breaking that | 14:08 |
| mhilton | natefinch, rogpeppe: you seem to be having the discussion I was about to start. I'm on the side of the Cookies function should only fill in the values that get sent in an HTTP request. If you want different functionality then I think you want a different function. | 14:09 |
| rogpeppe | mhilton: yup | 14:09 |
| rogpeppe | natefinch: that's the doc comment on Cookie.String BTW | 14:09 |
| natefinch | mhilton. rogpeppe: well, then RemoveCookie is useless, and we should remove it | 14:10 |
| natefinch | or... nearly useless I guess | 14:10 |
| rogpeppe | natefinch: it's not useless | 14:10 |
| rogpeppe | natefinch: you can use it with AllCookies | 14:10 |
| natefinch | well then allcookies is doing the wrong thing according to what you said above | 14:11 |
| rogpeppe | natefinch: AllCookies isn't designed for sending web requests | 14:11 |
| rogpeppe | natefinch: it's not part of the CookieJar interface | 14:11 |
| natefinch | rogpeppe: it's a function called AllCookies on a thing that implements CookieJar | 14:11 |
| rogpeppe | natefinch: yes | 14:11 |
| rogpeppe | natefinch: and? | 14:11 |
| natefinch | rogpeppe: if you think no one is ever going to use cookies from that list to send in a web request, you're more optimistic than most devs I've met | 14:12 |
| rogpeppe | natefinch: FWIW i think both AllCookies and RemoveCookie could be better documented | 14:12 |
| rogpeppe | natefinch: i'm optimistic. why would you do that? net/http uses the CookieJar implementation directly - there's no need to add cookies yourself. | 14:13 |
| natefinch | I don't think GetCookies(u) | RemoveCookie(c) is an unreasonable thing to think will work. The fact that it doesn't cost me a few hours of confusion. | 14:13 |
| natefinch | the old adage is to make it hard/impossible to do the wrong thing, and we're making it really easy to do the wrong thing. | 14:14 |
| rogpeppe | natefinch: i think we've pointed out the reason why you can't have that | 14:14 |
| rogpeppe | natefinch: i think the best answer is to improve the docs | 14:17 |
| natefinch | rogpeppe: That's fine... but then RemoveCookie shouldn't be there. It's misleading. I think it would be better to remove RemoveCookie and AllCookies, and instead have RemoveAll(url) and RemoveAll(func(c cookie)) or something. | 14:17 |
| rogpeppe | natefinch: AllCookies is useful for other reasons too | 14:17 |
| natefinch | er func(cookie) bool | 14:17 |
| rogpeppe | natefinch: and if you can see a cookie, it's useful to be able to remove it. | 14:18 |
| natefinch | rogpeppe: the fact that it returns cookies in a different format than Cookies(url) is *going* to cause confusion and bugs. It already has. | 14:18 |
| rogpeppe | natefinch: well, it returns the same format that's used for SetCookies | 14:18 |
| rogpeppe | natefinch: and for a Set-Cookie response header | 14:19 |
| rogpeppe | natefinch: it should definitely be documented better, and I think we could easily have a RemoveAll(url) method | 14:20 |
| rogpeppe | natefinch: i'm sorry you've had issues with it | 14:20 |
| rogpeppe | natefinch: but i don't think the current semantics need to be changed | 14:21 |
| natefinch | rogpeppe: it sounds like it's really just a problem with the design of http.Cookie and the CookieJar... Having String() return significantly different things based on what values are set seems like a bad design... which then makes GetCookies(u) have to work weirdly, and the whole thing cascdes. | 14:22 |
| rogpeppe | natefinch: maybe, but that's ancient history and we can't change it now. | 14:23 |
| natefinch | rogpeppe: the best idea for now, seems to be to making a RemoveCookies(url) seems like a good compromise | 14:24 |
| rogpeppe | natefinch: yeah, I'd accept a PR to that effect | 14:25 |
| rogpeppe | natefinch: for the time being you could just iterate through AllCookies as I suggested | 14:26 |
| rogpeppe | natefinch: it's not like there's a performance issue with doing that. | 14:26 |
| natefinch | rogpeppe: that's true. Just seems to encode too much knowledge into the caller. | 14:27 |
| rogpeppe | natefinch: we're not going to break it. relax :) | 14:28 |
| natefinch | rogpeppe: it's not that I'm afraid we'll break it, it's just ugly... and if I have to write that code somewhere, I'd rather write it in the place that deals with cookies, especially since we may need to do the same thing somewhere else. | 14:29 |
| rogpeppe | natefinch: i'd put it in logout.go | 14:30 |
| rogpeppe | natefinch: i think it's reasonable that that code knows where the various auth information is stored | 14:31 |
| natefinch | rogpeppe: we don't currently have an easy way to get the current controller url or the current cookie file... I had planned on putting Logout() on api.Connection, since it has the Login function, and it already has the cookie jar and the url. | 14:32 |
| rogpeppe | natefinch: you can trivially get the current controller URL by calling store.ControllerByName | 14:34 |
| rogpeppe | api.Connection doesn't have enough information to be able to log out | 14:35 |
| natefinch | rogpeppe: it has the persistent cookie jar and the url we're using to log in.... does it need more than that? | 14:37 |
| rogpeppe | natefinch: it doesn't necessarily have a persistent cookie jar | 14:37 |
| natefinch | rogpeppe: when would it not? | 14:37 |
| rogpeppe | natefinch: when someone makes a connection that's not using a persistent cookie jar. That's set up at a higher level, and api.Connection should not assume that it has one. | 14:38 |
| rogpeppe | natefinch: sorry, my connection just dropped for some reason | 14:39 |
| rogpeppe | natefinch: i think it's better that the code that's responsible for setting up the persistent cookiejar should be responsible for cleaning it up | 14:39 |
| rogpeppe | natefinch: BTW you can trivially get the persistent cookie jar with logoutCommand.APIContext() in the Jar field | 14:40 |
| rogpeppe | natefinch: FWIW persistent-cookiejar is a bit of a hack. it would be nice to replace it some time, and ISTM that introducing a dependency on it inside the API client code is not a good way to make that easier. | 14:42 |
| rogpeppe | natefinch: in general, anything that needs a dynamic type coercion should be viewed with suspicion. | 14:43 |
| natefinch | rogpeppe: that's fine. I figured the parity with login would be good, but it's fine not to | 14:43 |
| natefinch | rogpeppe: Oh yeah, absolutely. I hate to do it | 14:43 |
| redir | morning juju-dev | 15:10 |
| babbageclunk | redir: morning! | 15:11 |
| babbageclunk | redir: Hey, what happened with WaitAdvance? | 15:11 |
| redir | babbageclunk: it landed on friday IIRC | 15:12 |
| babbageclunk | redir: but no pushback or argybargy? | 15:21 |
| redir | babbageclunk: not yet | 15:22 |
| redir | babbageclunk: I'm just back from swapdays today | 15:22 |
| babbageclunk | redir: oh right | 15:22 |
| babbageclunk | redir: cool | 15:22 |
| redir | babbageclunk: how's move preparations going? | 15:26 |
| babbageclunk | redir: scary! | 15:27 |
| redir | babbageclunk: I bet. That is a long move | 15:27 |
| babbageclunk | redir: and getting really close! Lots of things to cancel and organise. | 15:27 |
| redir | babbageclunk: we made a transcontinental move last year this time. I am so glad it went smoothly and we're done with it. | 15:29 |
| redir | hope yours is as painless as can be | 15:29 |
| babbageclunk | redir: yeah, I'm really looking forward to it being done. Thanks" | 15:29 |
| babbageclunk | ! | 15:29 |
| redir | kettle is whistling. bbiam | 15:30 |
| alexisb | morning redir | 15:40 |
| alexisb | redir, we should connect when you have a moment | 15:41 |
| redir | alexisb: OK, going through email | 15:41 |
| redir | I'll ping you when done? | 15:41 |
| alexisb | redir, sure and I have a few meetings this morning as well | 15:43 |
| redir | alexisb: I'll look for a slot | 15:43 |
| redir | alexisb: wanna ping me after 10? | 15:55 |
| alexisb | sure | 15:56 |
| alexisb | redir, | 15:56 |
| rogpeppe | i've just proposed some changes to the juju register command to allow registration of controller with officially signed certs. reviews appreciated. https://github.com/juju/juju/pull/6382 | 16:01 |
| === frankban is now known as frankban|afk | ||
| === rogpeppe is now known as rogpeppe1 | ||
| === rogpeppe1 is now known as rogpeppe | ||
| === rogpeppe is now known as rogpeppe1 | ||
| === rogpeppe1 is now known as rogpeppe | ||
| babbageclunk | katco: ping? | 16:51 |
| katco | babbageclunk: pong | 16:52 |
| babbageclunk | katco: hey, I've been making some changes to StatePool to do refcounting. | 16:52 |
| katco | babbageclunk: lucky you ;p how's it going? | 16:52 |
| babbageclunk | katco: Would you mind taking a look at the current state of it? | 16:53 |
| katco | babbageclunk: sure | 16:53 |
| babbageclunk | katco: ok I think, although I've backed off from trying to make the StatePool watch models and remove items from itself. | 16:54 |
| katco | babbageclunk: yeah i don't think the statepool would be responsible for that as it's just a cache. if we went that route it would be a separate watcher | 16:55 |
| babbageclunk | katco: it's used in the API server and in WatchAllModels, and they use different types for life management. | 16:55 |
| babbageclunk | katco: menn0 had suggested I do that, but I think I'd need to convert the API server to use a catacomb first, maybe? | 16:56 |
| babbageclunk | katco: not really sure about that | 16:56 |
| redir | rogpeppe: looking | 16:56 |
| rogpeppe | redir: thanks! | 16:57 |
| rogpeppe | redir: there's some fun stuff in there :) | 16:57 |
| babbageclunk | katco: Ok, pushed - https://github.com/juju/juju/compare/master...babbageclunk:pool-memleak?expand=1 | 16:57 |
| katco | babbageclunk: tal. not sure about the catacomb question either. i think i need more context/concrete code to ponder over | 16:57 |
| babbageclunk | katco: thanks! Sorry, I have to rush home now so can't chat about it, but I'll check comments/email about it and be back online later. | 16:59 |
| voidspace | rick_h_: ok :-) | 17:01 |
| voidspace | rick_h_: was just writing you an email... but you've answered it | 17:01 |
| rick_h_ | voidspace: :) | 17:01 |
| alexisb | redir, ping | 17:04 |
| redir | alexisb: po | 17:06 |
| redir | ng even | 17:06 |
| alexisb | :) | 17:06 |
| redir | can't tab complete pong in weechat yet. | 17:06 |
| redir | location alexisb ? | 17:07 |
| alexisb | 1x1 HO | 17:07 |
| alexisb | brt | 17:07 |
| alexisb | https://hangouts.google.com/hangouts/_/canonical.com/alexis-bruemmer | 17:09 |
| alexisb | redir, ^^ | 17:09 |
| rogpeppe | redir: get anywhere with that review? :) | 17:22 |
| redir | rogpeppe: on hold in meeting bbiab | 17:22 |
| rogpeppe | redir: np | 17:22 |
| rogpeppe | redir: i'm about to stop for the day, but look forward to your comments in the morning. | 17:23 |
| cmars | which collection has an application's config? | 17:33 |
| redir | rogpeppe: cool | 17:43 |
| redir | i mean: ack | 17:43 |
| cmars | ah, it's `settings` | 17:49 |
| * alexisb changes locations, back online in a few minutes | 17:52 | |
| rick_h_ | voidspace: dooferlad ping, question in an irc channel about forcing juju traffic onto a specific address/interface. | 17:52 |
| rick_h_ | voidspace: dooferlad is there some method of forcing that? | 17:52 |
| perrito666 | I have put up a PR that mostly has tests https://github.com/juju/juju/pull/6384 | 17:59 |
| alexisb | perrito666, would this bug (https://bugs.launchpad.net/juju/+bug/1605714) potentially be fixed with this PR: https://github.com/juju/juju/pull/6321 | 18:39 |
| mup | Bug #1605714: juju2 beta11: LXD containers always pending on ppc64el systems <oil> <oil-2.0> <juju:Triaged by rharding> <https://launchpad.net/bugs/1605714> | 18:39 |
| perrito666 | alexisb: mm, I dont know if fixed, the containers might be in broken instead of fixed | 18:41 |
| perrito666 | of pending | 18:41 |
| perrito666 | but I am not sure what is causing it | 18:41 |
| alexisb | ok | 18:41 |
| mup | Bug #1630728 opened: user add/remove borked <juju-core:New> <https://launchpad.net/bugs/1630728> | 19:34 |
| mup | Bug #1630737 opened: juju should use internal vpc network address space when connected to vpc via vpn <juju-core:Incomplete> <https://launchpad.net/bugs/1630737> | 19:34 |
| perrito666 | rick_h_: I am starting to think we need a juju unremove-user | 19:44 |
| thumper | perrito666: ew | 19:44 |
| natefinch | lol | 19:45 |
| natefinch | juju undo | 19:45 |
| rick_h_ | perrito666: heh, I'd think reactive maybe, but yea. that's always the way I guess | 19:45 |
| perrito666 | thumper: well our remove user does not really remove users (that is a feature) and users want to remove and then re-add, so | 19:45 |
| rick_h_ | perrito666: I think the thing is the why is it a normal workflow, and yea sometimes mistakes happen and users need a way out | 19:45 |
| katco | thumper: hey quick q | 19:46 |
| katco | thumper: https://github.com/juju/juju/pull/6377/ | 19:47 |
| katco | thumper: what is our standard way of returning friendly multi-line errors to users? | 19:47 |
| katco | thumper: macgreagoir is creating a new error and not wrapping the old one; it seems like there should be a better way | 19:48 |
| thumper | otp | 19:49 |
| thumper | look shortly | 19:49 |
| katco | thumper: no worries ta | 19:49 |
| natefinch | katco: my thought was that any error from a cmd.Run method can basically be considered direct output to a user, and so we don't have to worry too much about the nitty gritty | 19:49 |
| katco | natefinch: what about --debug where the stack and cause would be shown? | 19:50 |
| natefinch | katco: does debug do that? I didn't realize :) I think Wrap does the right thing, if you want a new message but keep the same stack | 19:51 |
| katco | natefinch: yeah i use --debug all the time to track down the root of an error | 19:52 |
| katco | natefinch: i think Wrap is the right thing here, but thumper has done a lot of work around user-friendly multi-line errs, so thought i'd check in | 19:53 |
| natefinch | mna... I can't believe our logout tests don't actually log in first... sigh | 20:04 |
| rogpeppe1 | redir: thanks for the review. good point about the feature tests - i always forget that some tests are far removed from the package they're testing. | 20:04 |
| rogpeppe1 | natefinch: ha ha | 20:04 |
| natefinch | rogpeppe1: details, right? | 20:05 |
| rogpeppe1 | natefinch: :) | 20:05 |
| konobi | howdy. I'm having a problem with 4096bit SSH keys as reported in https://bugs.launchpad.net/juju/+bug/1543283 -- The bug is marked as fixed/released, but I'm seeing the same behaviour with 2.0-rc2-elcapitan-amd64 | 20:06 |
| mup | Bug #1543283: [Joyent] 4k ssh key can not be used: "cannot create credentials: An error occurred while parsing the key: asn1: structure error: length too large" <juju:Fix Released> <https://launchpad.net/bugs/1543283> | 20:06 |
| konobi | (this is via juju-quickstart, btw) | 20:06 |
| natefinch | konobi: does it happen if you don't use quickstart? | 20:07 |
| konobi | not sure, what would be a way to validate? | 20:10 |
| konobi | `ERROR detecting credentials for "joyent" cloud provider: credentials not found` | 20:11 |
| konobi | along with `ERROR there was an issue examining the environment: cannot create credentials: An error occurred while parsing the key: asn1: structure error: tags don't match (16 vs {class:0 tag:14 length:30 isCompound:true}) {optional:false explicit:false application:false defaultValue:<nil> tag:<nil> stringType:0 timeType:0 set:false omitEmpty:false} pkcs1PrivateKey @2` | 20:12 |
| natefinch | could be a bug, but might also be a problem with the format of the key in the credentials.yaml file | 20:15 |
| konobi | well, with joyent/triton it uses http-signatures for credentials | 20:16 |
| natefinch | konobi: ahh, is the key encrypted? | 20:18 |
| natefinch | I betcha that's the problem | 20:18 |
| konobi | it is... but my ssh-agent is running just fine | 20:20 |
| konobi | in the meantime, unencrypting should work for now | 20:20 |
| konobi | but yeah, i'd generally be using high strength ECDSA | 20:21 |
| natefinch | I don't think juju itself supports ssh-agent.... I see some stuff about quickstart supporting it, but I'm honestly not sure how that works | 20:24 |
| natefinch | asking on #juju might get you better help with quickstart. The bug you saw was for juju, not quickstart, so it's possible quickstart has the same problem we already fixed in the core of juju | 20:24 |
| konobi | it's the same errors as from that ticket | 20:30 |
| alexisb | thumper, are you actively doing anything for htis bug: https://bugs.launchpad.net/juju/+bug/1625768 | 20:32 |
| mup | Bug #1625768: github.com/juju/juju/state go test timeout <ci> <intermittent-failure> <regression> <unit-tests> <juju:In Progress by thumper> <https://launchpad.net/bugs/1625768> | 20:32 |
| thumper | alexisb: not any more, I sped up the slowest 3 | 20:32 |
| thumper | and that made things pass under the time limit again | 20:32 |
| alexisb | yep | 20:33 |
| alexisb | I am going to move it to high and to 2.1, as CI still sees it failing but not at a critical rate | 20:33 |
| thumper | veebers: pign | 20:48 |
| redir | anyone see this error when bootstrapping aws before? http://paste.ubuntu.com/23281590/ | 20:52 |
| veebers | thumper: pong, what's the haps? :-) | 20:54 |
| thumper | katco: commented, and suggested wrap | 20:54 |
| katco | thumper: ta, tal | 20:54 |
| thumper | veebers: I have a fix (I think) for some windows and s390 test failures | 20:55 |
| thumper | and would like to confirm | 20:55 |
| thumper | can we get access to the machines, grab my branch and run the test to see? | 20:55 |
| veebers | thumper: yep, we can do that. | 20:56 |
| thumper | veebers: this issue http://reports.vapour.ws/releases/issue/57f1cae8749a5660b085163f | 20:56 |
| redir | reboot brb | 21:06 |
| konobi | huh, seems like it chooses id_rsa in my .ssh folder over the one it's configured with =0/ | 21:07 |
| redir | phtphtpht failed to bootstrap model: cannot start bootstrap instance: no "xenial" images in us-west-1 with arches [amd64] | 21:51 |
| konobi | okay... it appears that juju just doesn't want to use the keys it was told to | 22:06 |
| konobi | (for bootstrap that is) | 22:08 |
| konobi | don't suppose folks who work on the joyent cloud provider backend are around at all? | 22:32 |
| katco | thumper: is it OK if an error message is multi-line? should we instead print things out to stderr and return the standard formatted error? | 22:42 |
| thumper | katco: I think it's fine, but I also think that it would make sense to get general agreement on approach for these types of responses, and then apply it everywhere | 22:43 |
| thumper | I'm in two minds | 22:43 |
| thumper | I can see benefits of both ways | 22:43 |
| katco | thumper: it's just bugging me for some reason | 22:43 |
| thumper | katco: so here's an alternative approach | 22:44 |
| katco | thumper: i think because we throw away the original error (maybe ok) but then create a message that's unlike all other errors | 22:44 |
| thumper | the common.PermissionError should take a command context | 22:44 |
| thumper | and write out to that | 22:44 |
| thumper | and then have the function return a silent error | 22:44 |
| thumper | so it doesn't get emitted | 22:44 |
| thumper | to be honest... | 22:45 |
| thumper | if we are wanting to have standard multiline nice emitting of errors | 22:45 |
| thumper | it makes sense to me for those functions to take the error and command context | 22:45 |
| katco | thumper: (or just an io.Writer) | 22:45 |
| thumper | that way you aren't wrapping or hiding | 22:45 |
| thumper | agreed | 22:45 |
| thumper | a writer would also be fine | 22:45 |
| katco | thumper: i think that's what's bothering me: we're not trying to create a new error with a better message, we're really trying to print something nice out to the user | 22:46 |
| thumper | katco: listen to that niggling | 22:46 |
| katco | thumper: and i think we're conflating the two | 22:46 |
| thumper | katco: it is bothering you for a reason | 22:46 |
| thumper | trust it | 22:46 |
| thumper | agreed | 22:46 |
| thumper | I'll back you on this | 22:46 |
| katco | thumper: as you said, maybe a function that take an io.Writer to write to and is called from the Run()s, and then do we still print the returned error? i think it would be ok? | 22:47 |
| thumper | I think the 'nice writer func' should write all the error info | 22:47 |
| thumper | and we convert the error into a silent one | 22:48 |
| thumper | otherwise we'll get things emitted out of order | 22:48 |
| thumper | which is why the error is being replaced with long text | 22:48 |
| konobi | thumper: any idea if someone on IRC is familiar with the joyent stuff at all? I'm dealing with it now, but I'm also a former joyent engineer, so I'm much more familiar with how the api there works | 22:48 |
| thumper | so the command infrastructure will write the full text | 22:48 |
| thumper | konobi: not me sorry | 22:48 |
| katco | thumper: ok, i can get behind that. function that writes out message to user + original error message and swallows the error? | 22:49 |
| thumper | no | 22:49 |
| thumper | it can't swallow it fully | 22:49 |
| thumper | otherwise the command will have rc of 0 | 22:50 |
| katco | ah just errors.Wrap(err, "") ? | 22:50 |
| thumper | rather than 1 | 22:50 |
| katco | yeah | 22:50 |
| thumper | there is a silent error type in cmd | 22:50 |
| thumper | which doesn't get emitted | 22:50 |
| katco | we probably still need to wrap the error for the stacktrace in --debug | 22:50 |
| thumper | yeah | 22:50 |
| thumper | wrap the error with a silent one | 22:51 |
| thumper | so errors.Cause is a silent error | 22:51 |
| thumper | that should work | 22:51 |
| katco | ok thanks for talking through that with me, i'll leave that feedback | 22:51 |
| thumper | also... | 22:51 |
| thumper | we may want to have an ansiterm.Writer rather than io.Writer | 22:51 |
| thumper | so we could colorize | 22:51 |
| thumper | just a thought | 22:51 |
| * thumper thinks | 22:52 | |
| thumper | no | 22:52 |
| katco | would we ever want to colorize that? | 22:52 |
| katco | yagni principle | 22:52 |
| thumper | well, ERROR is shown in red | 22:52 |
| thumper | everywhere else | 22:52 |
| thumper | because the cmd package logs the error | 22:52 |
| thumper | it starts getting a little messy | 22:52 |
| katco | thumper: ack | 22:55 |
| === ben_ is now known as benk01 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!
