| davecheney | niemeyer_: i'm actively responding to rog's comments on the stop/start machine CL | 01:50 |
|---|---|---|
| davecheney | and will have another proposal by the end of the day | 01:51 |
| davecheney | i'll remove it from review | 01:51 |
| niemeyer_ | davecheney: Cool, I was just skimming over it actually | 01:51 |
| davecheney | it'll be 10-15% smaller by the end of the day | 01:51 |
| niemeyer_ | davecheney: Sounds good.. I'd like to have a look at it with a fresher brain tomorrow as well, so that will suit well | 01:51 |
| davecheney | cool | 01:52 |
| niemeyer_ | davecheney: Oh, that's nice | 01:52 |
| davecheney | niemeyer_: related question: do you know what the status of our hp cloud access is | 01:56 |
| davecheney | ie, can we just use it, or is there something left to do ? | 01:57 |
| niemeyer_ | I have not heard, but I can check in the morning | 02:12 |
| davecheney | niemeyer_: thanks | 02:13 |
| Aram | morning. | 06:56 |
| * davecheney goes back to fighting juju test | 07:00 | |
| rogpeppe | Aram, davecheney, fwereade_: hiya | 07:23 |
| fwereade_ | heyhey | 07:23 |
| Aram | moin | 07:23 |
| davecheney | evening lads | 07:23 |
| davecheney | i'm just on my way out | 07:23 |
| davecheney | have a nice evening | 07:23 |
| rogpeppe | davecheney: what are doing to poor jujutest; she never wanted to fight anyone :-) | 07:23 |
| rogpeppe | s/doing/you doing/ | 07:23 |
| rogpeppe | davecheney: ok, have a good one | 07:24 |
| fwereade | rogpeppe, can I ask a favour please? I'm off today, and the rework-stop-watchers branch is still not submitting cleanly; if you would merge it for me I would be most grateful | 09:12 |
| fwereade | rogpeppe, I can worry about why I can't submit anything else later ;) | 09:13 |
| rogpeppe | fwereade: i will try, np | 09:13 |
| fwereade | rogpeppe, tyvm | 09:13 |
| niemeyer | Good morning | 10:19 |
| rogpeppe | niemeyer: hiya | 10:21 |
| rogpeppe | niemeyer: i'm wondering what your feeling on | 10:22 |
| rogpeppe | cannot create new | 10:22 |
| rogpeppe | machine node | 10:22 |
| rogpeppe | oops | 10:22 |
| rogpeppe | niemeyer: i'm wondering what your feelings on assign-subordinate-units are now | 10:22 |
| rogpeppe | niemeyer: although i think bcsaller's criticism is somewhat misfounded (the change is always made properly within a retryChange transaction), i'm still inclined to drop the change and make Machine.Units (perhaps renamed) return principal units only | 10:24 |
| niemeyer | rogpeppe: The data fiddling seems to be unnecessary | 10:26 |
| rogpeppe | niemeyer: that's what i'm thinking | 10:26 |
| niemeyer | rogpeppe: And it's nice to avoid depending on atomic behavior, even if we can | 10:26 |
| rogpeppe | niemeyer: (if i understand what you mean by data fiddling here) | 10:26 |
| rogpeppe | niemeyer: that too. | 10:27 |
| niemeyer | rogpeppe: The Units API still looks nice, though | 10:27 |
| rogpeppe | niemeyer: i'm happy if we document that Machine.Units returns principal units only | 10:27 |
| niemeyer | rogpeppe: That sounds bad | 10:27 |
| rogpeppe | niemeyer: oh | 10:28 |
| niemeyer | rogpeppe: Have you seen the comment in the review? | 10:28 |
| rogpeppe | niemeyer: so your suggestion, i think, is that topology.UnitsForMachine should do a search for subordinate units too, rather than just looking a topoUnit.Machine? | 10:30 |
| rogpeppe | s/a topo/at topo/ | 10:31 |
| niemeyer | rogpeppe: Right, it's the same loop.. | 10:31 |
| rogpeppe | niemeyer: no, i think it needs to be an inner loop. it's n^2 but probably not bad in practice. | 10:31 |
| rogpeppe | niemeyer: that is, for each unit we find with the given machine, we have to scan all other units to see if any has that unit as its principal. | 10:32 |
| niemeyer | rogpeppe: Only if we do it badly | 10:32 |
| rogpeppe | niemeyer: right, we could do two linear loops, building up a map in the first one, i suppose | 10:33 |
| niemeyer | Yep | 10:33 |
| rogpeppe | still not quite "the same loop" but, yeah, that would work ok. | 10:34 |
| rogpeppe | niemeyer: and i have to check if we always know if a unit is principal when we create a *Unit. | 10:35 |
| niemeyer | rogpeppe: It can actually be done in the same loop, but it doesn't matter. | 10:35 |
| niemeyer | rogpeppe: The Principal is a setting next to the Machine | 10:36 |
| rogpeppe | niemeyer: i don't *think* so, because when we get to a subordinate before its principal, we don't know whether to keep it around or not. | 10:36 |
| niemeyer | rogpeppe: I'm going to argue.. it doesn't matter.. | 10:36 |
| niemeyer | I'm not going to argue | 10:36 |
| rogpeppe | niemeyer: i'd like to do it in the same loop, which is why i'm asking. i don't mind much though. | 10:38 |
| rogpeppe | niemeyer: ok, i'll go with that, thanks. | 10:39 |
| rogpeppe | niemeyer: i don't think the tests need to change, which is nice. | 10:39 |
| niemeyer | rogpeppe: I'm more concerned that it looks nice.. it doesn't have to be in the same loop | 10:39 |
| niemeyer | rogpeppe: The tests need to ensure subordinate units are returned, because clearly they don't right now | 10:40 |
| rogpeppe | niemeyer: that CL made it do so, and tested for that | 10:40 |
| rogpeppe | niemeyer: but i think Unit.MachineId should look up the principal unit when necessary. that's new. | 10:41 |
| niemeyer | rogpeppe: Yeah, indeed | 10:42 |
| Aram | hi niemeyer | 10:44 |
| niemeyer | Aram: Heya | 10:45 |
| rogpeppe | mramm: hiya | 10:45 |
| mramm | rogpeppe: hi | 10:45 |
| TheMue | Lunchtime | 11:30 |
| * rogpeppe *hates* the fact that ^W deletes a tab in Chrome. And that it loses the contents of any text entry box in doing so. | 11:31 | |
| TheMue | niemeyer: https://codereview.appspot.com/6296064 is just in again. Looks better, definitely. Thx for that great idea. | 11:31 |
| Aram | rogpeppe: I hate that ^W is so close to ^Q. | 11:31 |
| rogpeppe | Aram: i'd switch off all menu shortcuts if i could (i don't count ^X, ^C and ^V) | 11:32 |
| niemeyer | TheMue: Heya, no problem | 12:16 |
| niemeyer | rogpeppe: clean up data", but that's not the only thing that zkRemoveTree is used | 12:18 |
| niemeyer | for - removing a service is not really "cleaning up data"). | 12:18 |
| niemeyer | rogpeppe: I'm not following.. | 12:18 |
| niemeyer | rogpeppe: That's exactly what zkRemoveTree is doing in that context | 12:18 |
| niemeyer | rogpeppe: Ah, I think I see what you mean | 12:20 |
| TheMue | rogpeppe: zkRemoveTree() only adds this, but it's called in methods adding more context. | 12:25 |
| TheMue | rogpeppe: So you've got "can't remove server: can't clean up data: ...". | 12:25 |
| TheMue | rogpeppe: The context of where zkRemoveTree() is called has to be added by the caller. | 12:26 |
| TheMue | Ooops, 'can't remove service "foo": can't clean up data: <<some error during zk.Delete()>>' | 12:30 |
| TheMue | So it looks like. | 12:31 |
| niemeyer | TheMue: That was his point too | 12:33 |
| niemeyer | Heading off for doc appointment | 12:41 |
| niemeyer | TheMue: You have a review on this issue | 12:41 |
| TheMue | niemeyer: Yes, I've seen and started the changes. | 12:41 |
| TheMue | niemeyer: Is back in again. | 12:55 |
| * TheMue really likes errorContext(). It later easily can be extended. | 12:58 | |
| niemeyer | TheMue: Thanks, checking again | 13:47 |
| TheMue | rogpeppe: Does the current errorContext() doesn't work with govet? My editor uses govet live (at least I thought so) and it doesn't complain. | 14:01 |
| rogpeppe | TheMue: in a meeting, back in a short while | 14:02 |
| TheMue | rogpeppe: ok, have a phone call, so np | 14:05 |
| niemeyer | TheMue: Another review in | 14:07 |
| niemeyer | TheMue: The point of rog's comment is that he suggests go vet would actually catch mistakes with his proposed implementation. I'm not sure about that myself, and would like to see it happening before changing the function to something uglier. | 14:08 |
| niemeyer | Is fwereade off today? | 14:39 |
| TheMue | niemeyer: reasonable | 14:39 |
| TheMue | niemeyer: Yes, he's off today. | 14:39 |
| * TheMue has a hot ear from phoning a bit longer. | 14:40 | |
| niemeyer | There are two branches with similar names and similar descriptions, pushed around the same time | 14:40 |
| TheMue | niemeyer: fwereade asked rogpeppe this morning regarding rework-stop-watchers | 14:41 |
| niemeyer | TheMue: Oh? | 14:42 |
| TheMue | niemeyer: It's not submitting cleanly and so he asked rog to merge it. | 14:43 |
| niemeyer | TheMue: The branch is up for review | 14:43 |
| niemeyer | Oh, but it's a copy of a third branch? | 14:44 |
| niemeyer | Ugh.. messy | 14:44 |
| TheMue | niemeyer: I think rog can tell you more about it. | 14:44 |
| niemeyer | It seems to be the same branch that was already reviewed | 14:46 |
| niemeyer | It just sucks that there are two merge proposals up without any hints about what's going on | 14:46 |
| niemeyer | I'll pick the newest one and attempt to merge it | 14:48 |
| TheMue | niemeyer: Next propose is in. | 14:58 |
| * rogpeppe is out of the meeting | 15:02 | |
| rogpeppe | niemeyer: william wasn't able to submit the branch | 15:02 |
| rogpeppe | niemeyer: he was getting "readonly transport" errors | 15:03 |
| niemeyer | rogpeppe: Okay, it's in now.. hopefully I got the intention properly | 15:03 |
| niemeyer | TheMue: Review is in | 15:03 |
| rogpeppe | niemeyer: the point about the go vet stuff is that i think it's asking for trouble to have a function that takes a printf-like format string and deliberately miss the last argument. | 15:03 |
| rogpeppe | niemeyer: and in fact i think the signature is slightly nicer - every single call would end in ": %v" AFAICS | 15:04 |
| rogpeppe | niemeyer: so we can lose that | 15:04 |
| niemeyer | rogpeppe: How's go vet related to that? | 15:05 |
| rogpeppe | niemeyer: go vet can check printf-like format strings | 15:05 |
| niemeyer | rogpeppe: I'm happy to have the ": %s" added automatically.. that point wasn't clear from your comment | 15:05 |
| niemeyer | rogpeppe: Have you managed to make go vet work with that function? | 15:05 |
| rogpeppe | niemeyer: yes | 15:05 |
| niemeyer | rogpeppe: Can you please paste an example that fails | 15:06 |
| niemeyer | TheMue: Please hold off merging for a moment | 15:06 |
| TheMue | niemeyer: Yes, I'm following here. | 15:06 |
| rogpeppe | niemeyer: actually i lie. i managed to make go tool vet work with that function. | 15:06 |
| niemeyer | rogpeppe: Same thing.. can you please paste an example of go vet complaining? | 15:06 |
| rogpeppe | niemeyer: http://paste.ubuntu.com/1039204/ | 15:08 |
| niemeyer | rogpeppe: Ah, you're calling vet manually with explicit parameters | 15:10 |
| rogpeppe | niemeyer: yes, you need to tell govet about custom functions. | 15:10 |
| rogpeppe | niemeyer: but it can still do the check | 15:10 |
| niemeyer | rogpeppe: Ok, sounds good | 15:10 |
| niemeyer | TheMue: That sounds good to me.. what do you think? | 15:11 |
| TheMue | niemeyer: Yes, so I'll change it from append()... to +. Also adopt it in the error strings. | 15:12 |
| niemeyer | TheMue, rogpeppe: Awesome, thanks | 15:13 |
| rogpeppe | TheMue: thanks | 15:13 |
| rogpeppe | TheMue: and note that the function name needs to end in "f" | 15:13 |
| TheMue | niemeyer: I'll also change the name. | 15:13 |
| TheMue | rogpeppe: Yes. ;) | 15:13 |
| TheMue | niemeyer: Tests are green with latest trunk. Directly submit it or do you want to have a final look? | 15:22 |
| niemeyer | TheMue: If it's all as agreed, +1 on submitting | 15:29 |
| TheMue | niemeyer: OK, here it comes. | 15:30 |
| niemeyer | TheMue: Woohoo :) | 15:30 |
| TheMue | niemeyer: Hmm, not yet. Have a "Cannot lock LockDir" during submit. Any idea what I'm missing? | 15:33 |
| rogpeppe | TheMue: i think that's the same error fwereade was getting | 15:34 |
| rogpeppe | TheMue: does it say "readonly transport" ? | 15:34 |
| TheMue | rogpeppe: Exactly! | 15:35 |
| rogpeppe | niemeyer: have you tried submitting fwereade's branch? i wonder if you'll get the same problem. | 15:35 |
| niemeyer | rogpeppe: As I mentioned, it's already in | 15:40 |
| niemeyer | Hopefully I got the proper end of it | 15:41 |
| niemeyer | I wonder if I screwed up the setpu somehow | 15:41 |
| rogpeppe | niemeyer: ah, i haven't seen a submit message yet. | 15:41 |
| niemeyer | There won't be one, but I added a note | 15:42 |
| niemeyer | And mentioned above too | 15:42 |
| niemeyer | TheMue: What's bzr info telling you? | 15:43 |
| rogpeppe | niemeyer: ah, i didn't see anything that said you'd actually submitted anything. my clients probably missed the message somehow. | 15:44 |
| niemeyer | Jun 13 12:03:00 <rogpeppe> niemeyer: he was getting "readonly transport" errors | 15:44 |
| niemeyer | Jun 13 12:03:25 <niemeyer> rogpeppe: Okay, it's in now.. hopefully I got the intention properly | 15:44 |
| TheMue | niemeyer: It's a standalone tree format 2a, push branch is bzr+ssh://bazaar.launchpad.net/~themue/juju-core/go-state-second-error-improvement/. | 15:45 |
| TheMue | niemeyer: Branch root is . | 15:46 |
| rogpeppe | niemeyer: ah, i didn't realise that's what you were talking about :-) | 15:46 |
| niemeyer | rogpeppe: Heh | 15:46 |
| TheMue | niemeyer: Parent is ...../trunk | 15:46 |
| niemeyer | TheMue: Hmm | 15:46 |
| niemeyer | Somehow it's picking up a branch for merging onto that you don't have access to | 15:49 |
| niemeyer | Which makes little sense to me | 15:49 |
| niemeyer | Oh, it's making more sense now | 15:50 |
| niemeyer | Crap | 15:50 |
| niemeyer | I've pushed as gophers rather than ~juju | 15:50 |
| niemeyer | TheMue: Please try to submit now | 15:51 |
| TheMue | niemeyer: Yeah, great, thx! | 15:52 |
| TheMue | niemeyer: So I can start the next branch for unit.go. | 15:53 |
| rogpeppe | niemeyer: assign-subordinate-units updated as we discussed: https://codereview.appspot.com/6299073 | 15:55 |
| TheMue | niemeyer: Ah, I'm a gopher now, see the reason. ;) | 15:56 |
| niemeyer | TheMue: Worked? | 15:59 |
| niemeyer | rogpeppe: Thanks a lot man | 16:00 |
| niemeyer | rogpeppe: I'll have a look at that as soon as I'm back from lunch | 16:00 |
| TheMue | niemeyer: Yes, it's in. | 16:00 |
| rogpeppe | niemeyer: thanks! | 16:00 |
| niemeyer | TheMue: Superb | 16:00 |
| niemeyer | rogpeppe, TheMue: So it was just me screwing over on the migration | 16:00 |
| TheMue | niemeyer: Thank you, I'll continue with unit.go now. | 16:00 |
| niemeyer | I'll fix that when I'm back too | 16:00 |
| rogpeppe | TheMue: you might want to review that too. and this one also: https://codereview.appspot.com/6299082 | 16:21 |
| rogpeppe | bcsaller: i've changed https://codereview.appspot.com/6299082/ based on your feedback. i *think* the two are functionally the same, but the new version avoids the need to make changes in several places in the topology at once. | 16:25 |
| bcsaller | rogpeppe: thanks, I'll look at it again | 16:25 |
| rogpeppe | bcsaller: oops, i meant https://codereview.appspot.com/6299073/ | 16:26 |
| rogpeppe | bcsaller: thanks a lot for the feedback BTW. it's really good to have more people familiar with the python version looking at the changes going past in the Go version. | 16:26 |
| bcsaller | :) | 16:27 |
| rogpeppe | bcsaller: can you think of a good reason why the machine agent downloads the charm for the unit agent that it starts? | 16:56 |
| bcsaller | rogpeppe: rather than the unit agent you mean? | 16:57 |
| rogpeppe | bcsaller: yeah | 16:57 |
| rogpeppe | bcsaller: given that the unit agent already deals with downloading charms for its subordinates | 16:57 |
| bcsaller | rogpeppe: I cannot, I think it should be the UA for when things are in LXC containers, but that design predates the LXC everywhere model | 16:57 |
| rogpeppe | bcsaller: cool, thanks. | 16:58 |
| bcsaller | rogpeppe: yeah, the sub stuff is new and we factored out the deployment stuff for that to be reused but did touch the orig MA use | 16:58 |
| rogpeppe | bcsaller: the other thing i'm thinking about is that if the machine agent crashes but the unit agents carry on, then there doesn't seem to be a way for the MA to detect that they're still around and avoid starting them again. am i missing something? | 16:59 |
| TheMue | rogpeppe: Review is in, sadly clicked too fast so one remark and one with comments. | 16:59 |
| rogpeppe | TheMue: ok, thanks a lot | 17:00 |
| bcsaller | rogpeppe: on restart of the MA it needs to look at the at the assigned units and their states in ZK, no? | 17:00 |
| rogpeppe | bcsaller: yeah, you're right. i must've missed that bit in the python code. thanks. | 17:01 |
| rogpeppe | bcsaller: i suppose the difficulty is that there's no real way of telling if the unit has really gone down, or just the unit agent. | 17:03 |
| bcsaller | rogpeppe: a real and persistent issue, yeah, knowing if the service is running apart from the agent isn't really handled, it could mean so many things | 17:04 |
| rogpeppe | bcsaller: and upstart *should* handle the unit agent restart. so perhaps it's right that the machine agent only checks for unit agent presence when it starts up, rather than watching it continually. | 17:05 |
| bcsaller | rogpeppe: I think there is nothing wrong with the idea that the MA should observe the UA presence unless we are sure there is no action it will take (due to upstart for example) | 17:06 |
| bcsaller | rogpeppe: but if its always a no-op then sure | 17:07 |
| rogpeppe | bcsaller: i'm trying to think of a case where we'd want the MA kill the UA (and its container) and restart it. because there's no way the MA can restart the unit in general without doing that. | 17:08 |
| bcsaller | rogpeppe: there would have to be a coordination protocol around the stop/restart/upgrade of units, kapil has been thinking about that stuff for some time now | 17:09 |
| rogpeppe | bcsaller: do any of those (stop/restart/upgrade) imply that the unit's container needs to be ripped down, then recreated? | 17:11 |
| bcsaller | rogpeppe: upgrade might for some definition of ripped down, but generally no | 17:11 |
| bcsaller | rogpeppe: upgrading the agent itself | 17:12 |
| rogpeppe | bcsaller: upgrading the agent itself is ok i think, and can be done without affecting the container | 17:12 |
| bcsaller | rogpeppe: agreed | 17:13 |
| rogpeppe | TheMue: the difference between addLoggingCharm and addDummyCharm is that addDummyCharm is used a lot, so probably justifies the charm variables stored in StateSuite (avoiding the overhead of reading the charm directory each time), but i'm not sure that addLoggingCharm does... | 17:19 |
| rogpeppe | TheMue: on the other hand, maybe a more general mechanism might be good. i'll think about that. | 17:22 |
| niemeyer | rogpeppe: I can't think of a reason for tha MA to be killing the UA like that | 17:22 |
| Aram | gah. | 17:22 |
| Aram | managed to lock myself out of the apartment. | 17:23 |
| Aram | we have these dumb doors that lock themselves at all times. | 17:23 |
| niemeyer | rogpeppe: and +1 on having UAs handling their own charms | 17:23 |
| rogpeppe | Aram: so you're outside the door using your wi-fi? | 17:23 |
| Aram | rogpeppe: no, I had to wait for my girlfriend to come back. | 17:23 |
| rogpeppe | niemeyer: cool. | 17:23 |
| niemeyer | Aram: Oh, crap.. that was one of the first things I did when we moved | 17:23 |
| niemeyer | Aram: I mean, replacing the door handle by a reasonable one | 17:24 |
| Aram | heh, yeah. | 17:24 |
| niemeyer | If you want to lock the door from outside, you need the keys.. such a nice way to turn the issue into a non-existent problem | 17:24 |
| * rogpeppe has to go. happy evenings all. | 17:26 | |
| Aram | fare thee well. | 17:26 |
| rogpeppe | niemeyer: if you manage to look at those two CLs (ttps://codereview.appspot.com/6299073 and https://codereview.appspot.com/6299082) that would be marvellous, thanks. | 17:27 |
| niemeyer | rogpeppe: Certainly will, thanks | 17:27 |
| niemeyer | rogpeppe: Have a good time there | 17:27 |
| * TheMue has to leave too | 17:38 | |
| === samkottler1 is now known as samkottler | ||
| === hazmat is now known as kapilt | ||
| niemeyer | rogpeppe: If you have a spare moment in your evening by any chance, Dave's https://codereview.appspot.com/6307071/ is waiting for you | 19:48 |
| niemeyer | Okay, review queue is in good state finally.. three days to get it cleaned after holidays. Phew. | 20:05 |
| niemeyer | I'm having a break.. will come back later to polish some juju slides | 20:05 |
| === kapilt is now known as hazmat | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!