| davecheney | thumper: re last nights discussion of pacakging | 00:35 |
|---|---|---|
| davecheney | can you outline the concerns of the packager's ? | 00:36 |
| thumper | primarily it seemed to be that we weren't following company standards | 00:36 |
| davecheney | of that I have no doubt | 00:36 |
| davecheney | but that isn't very actionable | 00:37 |
| davecheney | i'm guessing the lack of build reproducability is a red flag | 00:38 |
| thumper | I'm going to email people concerned to see if we can get help from people who do know the standards | 00:38 |
| davecheney | awesome | 00:38 |
| thumper | one guy is even in sydney, so pairing might be good if you are interested | 00:38 |
| davecheney | sure | 00:39 |
| thumper | I'll keep you in the email loop | 00:39 |
| thumper | davecheney: can I get you to merge that packaging branch of mine? then I can move the card to done | 02:14 |
| bigjools | thumper: how was your foray into packaging? | 02:22 |
| thumper | bigjools: brief | 02:22 |
| davecheney | thumper: i have some questions about that change | 02:25 |
| thumper | davecheney: shoot | 02:26 |
| davecheney | see review | 02:26 |
| thumper | kk | 02:27 |
| thumper | davecheney: replied | 02:39 |
| * thumper is EODing early due to overly long meeting last night | 02:39 | |
| * thumper will look back in later | 02:41 | |
| davecheney | kk | 02:41 |
| davecheney | thumper: fair enough, that makes sense | 02:42 |
| jtv | Anyone else getting this build error? go/src/launchpad.net/juju-core/cmd/juju/constraints.go:66: undefined: results | 03:25 |
| jtv | Looks like a :=/= typo. | 03:25 |
| * thumper tries | 03:25 | |
| davecheney | jtv: which revno ? | 03:25 |
| jtv | The latest. | 03:26 |
| jtv | I'm also getting this one, which I guess is good news: | 03:26 |
| jtv | constraints_test.go:283: | 03:26 |
| jtv | // A failure here indicates that goyaml bug lp:1132537 is fixed; please | 03:26 |
| jtv | // delete this test and uncomment the flagged constraintsRoundtripTests. | 03:26 |
| davecheney | +1 for being unhelpful :) | 03:26 |
| thumper | +1 for getting that | 03:26 |
| thumper | davecheney: I get it with tip | 03:26 |
| thumper | jtv: yes | 03:26 |
| davecheney | confirmed, # launchpad.net/juju-core/cmd/juju | 03:26 |
| davecheney | cmd/juju/constraints.go:66: undefined: results | 03:26 |
| davecheney | cmd/juju/constraints.go:66: cannot assign to results | 03:26 |
| davecheney | cmd/juju/constraints.go:67: undefined: results | 03:26 |
| davecheney | will submit a fix | 03:26 |
| jtv | Ah thanks. | 03:26 |
| thumper | plz fix | 03:26 |
| davecheney | what the fuck people | 03:26 |
| thumper | we just talked about this last night | 03:27 |
| thumper | it worked on my commit | 03:27 |
| thumper | yep, last commit added that | 03:27 |
| * davecheney sends a shittyram | 03:27 | |
| jtv | gram? | 03:28 |
| thumper | jtv: perhaps he is sending it to mramm | 03:28 |
| thumper | or he is sending a pooey male sheep | 03:28 |
| jtv | thumper: that's why I asked :-P | 03:28 |
| jtv | Confucius say, man-sheep eat from curry tree, he be shitty ram for a day | 03:29 |
| davecheney | sorry, type | 03:29 |
| davecheney | typo | 03:29 |
| davecheney | i meant shittyHAM | 03:29 |
| jtv | Oh! | 03:29 |
| jtv | Why didn't you SAY so | 03:29 |
| davecheney | it's my accent | 03:29 |
| thumper | davecheney: I think the whole = / := thing is horrible | 03:29 |
| thumper | and needs a rethink | 03:29 |
| thumper | so many problems come about from it | 03:29 |
| jtv | FWIW I guess you _can_ create this kind of bug just from concurrent commits. | 03:30 |
| jtv | The shadowing is really horrible though. Makes you wonder what's so horrible about a mere unused variable. | 03:30 |
| jtv | foo, err := bar() | 03:30 |
| jtv | if err != nil { panic(err) } | 03:30 |
| jtv | if foo { | 03:31 |
| jtv | sklurg, err := szot() | 03:31 |
| jtv | Oops now I've created a second err. | 03:31 |
| thumper | jtv: I don't think you have | 03:31 |
| jtv | That's what I thought. | 03:32 |
| jtv | It'll shadow quietly. | 03:32 |
| thumper | it isn't shadowed | 03:32 |
| thumper | it is using the same err | 03:32 |
| thumper | a new sklurg is created though | 03:32 |
| thumper | hmm, inside the if though, new scope and all... | 03:33 |
| thumper | I've not tested... | 03:33 |
| thumper | I don't actually know what is happening there | 03:33 |
| thumper | perhaps davecheney does | 03:33 |
| davecheney | davecheney: knows what ? | 03:33 |
| jtv | thumper: I do know _this_ happens: http://play.golang.org/p/uV__rKVATq | 03:34 |
| jtv | Woo! Test suite completed with just 1 compile error and 1 test failure. | 03:35 |
| thumper | davecheney: you have a local packaging branch...? | 03:35 |
| thumper | davecheney: https://code.launchpad.net/~thumper/juju-core/package/+merge/157020 has instructions at the top | 03:35 |
| thumper | davecheney: bzr merge ... | 03:35 |
| thumper | davecheney: bzr commit... bzr push | 03:35 |
| thumper | davecheney: done | 03:35 |
| davecheney | thumper: oh, ok, it's that simple | 03:35 |
| * davecheney wonders what lbox does ... | 03:35 | |
| thumper | davecheney: previously was referring to jtv's comment above about errors | 03:35 |
| thumper | davecheney: lbox does that too, but slowly | 03:36 |
| thumper | and with extra checking | 03:36 |
| thumper | and many round trips | 03:36 |
| davecheney | thumper: are you talking about shadowing ? | 03:38 |
| thumper | davecheney: yeah, is the err a shadow or using the one from the previous scope? | 03:38 |
| davecheney | it is what it is | 03:38 |
| davecheney | it can't be changed now, it's in the spec | 03:38 |
| jtv | thumper: just tried the sklurg/err thing and yes, it creates a new sklurg *and* a new err. | 03:41 |
| davecheney | looks like others feel the same, https://twitter.com/GolangSucks/status/319999909866651648 | 03:47 |
| thumper | jtv: is that you @GolangSucks ? | 03:48 |
| jtv | thumper: nope | 03:59 |
| jtv | Bit strong for me... You know what I'm like. I'm trying to see the good things. :) | 03:59 |
| jtv | But I do notice a surprising similarity. "for item := range x counts x. Unless it's a channel in which case it returns values." Pretty much a literal match with something I wrote. | 04:01 |
| jtv | The unnecessary choices are something I mentioned as well. Lots of make-work. | 04:01 |
| bigjools | Getting a compile error from trunk: http://paste.ubuntu.com/5678748/ | 05:00 |
| davecheney | bigjools: probably got versoin skew with openstack | 05:01 |
| bigjools | davecheney: oh I assumed a go get -u would update that? | 05:01 |
| davecheney | try go get -u launchpad.net/goose/... | 05:01 |
| davecheney | it might | 05:01 |
| bigjools | as in, I did a get -u on juju-core | 05:02 |
| davecheney | ohhh, i wouldn't do that | 05:02 |
| davecheney | it'll totally screw your bzr checkout | 05:02 |
| bigjools | wha? | 05:02 |
| davecheney | go get -u doesn't understand cobzr branches | 05:03 |
| bigjools | I don't use cobzr | 05:03 |
| davecheney | fair enough | 05:03 |
| bigjools | that's the least of its problems anyway, I aliased my branch command so it doesn't use trees | 05:03 |
| bigjools | which breaks go get | 05:04 |
| davecheney | bigjools: probably easier to skip it and just to a bzr pull | 05:04 |
| bigjools | yup! | 05:04 |
| bigjools | anyway - still got that compile error | 05:04 |
| bigjools | after goose updated | 05:04 |
| bigjools | I am on raring if that makes any difference | 05:05 |
| davecheney | bigjools: pls hold | 05:07 |
| bigjools | davecheney: hlding :) | 05:07 |
| bigjools | and holding | 05:07 |
| davecheney | bigjools: fastest way to a fix | 05:08 |
| davecheney | rm -rf $GOPATH/src/launchpad.net/goose | 05:09 |
| davecheney | go get launcpad.net/goose/... | 05:09 |
| davecheney | or whatever method you want to use to manage your source | 05:09 |
| bigjools | ok | 05:09 |
| bigjools | oh did it move? | 05:09 |
| davecheney | it did not move | 05:10 |
| davecheney | actually | 05:10 |
| davecheney | that isn't strictly true | 05:10 |
| bigjools | quickest way to fix that is: | 05:10 |
| bigjools | bzr pull --remember lp:goose | 05:10 |
| bigjools | aaaand done. Loads of updates, crikey. | 05:11 |
| davecheney | someone fucked a lot of things up when the kicked us all out of the ~gophers gropu | 05:11 |
| bigjools | ok new errors | 05:11 |
| bigjools | canonical/GO/src/launchpad.net/juju-core/cmd/juju/constraints.go:66: undefined: results | 05:11 |
| davecheney | fix proposed | 05:12 |
| bigjools | ah is this what you were talking about jtv? | 05:12 |
| davecheney | https://codereview.appspot.com/8370047/ | 05:13 |
| davecheney | yup | 05:13 |
| bigjools | davecheney: given that it's blocking everyone, are you going to land it right away or do you have to wait for 2 pairs of eyes? | 05:15 |
| davecheney | bigjools: that is all the enchouragement I need :) | 05:16 |
| bigjools | davecheney: that was the intention :) | 05:16 |
| bigjools | *cough* | 05:16 |
| davecheney | comitted | 05:20 |
| bigjools | davecheney: compiles \o/ | 05:21 |
| bigjools | and the raring mongo seems to have ssl support \o/ | 05:52 |
| davecheney | bigjools: where is the lp project for mongo ? | 05:54 |
| davecheney | ~mongodb looks stale | 05:54 |
| bigjools | davecheney: not sure there is one - I am just using the latest package, I think jamespage packaged it | 05:54 |
| bigjools | now, I am getting loads of test failures :/ | 05:54 |
| bigjools | [LOG] 92.72463 ERROR api: error receiving request: read tcp 127.0.0.1:38922: use of closed network connection | 05:55 |
| bigjools | all over | 05:55 |
| davecheney | bigjools: id suggest that ssl doesn't work | 05:55 |
| bigjools | are you on raring? | 05:56 |
| davecheney | nope | 05:57 |
| bigjools | davecheney: I think raring's mongo (2.2.4?) is incompatible with the current juju | 06:21 |
| bigjools | it definitely has ssl support | 06:22 |
| bigjools | thumper: you're on raring? | 06:24 |
| bigjools | hmm I have it working on a fresh raring VM. I wonder what's up with my local env... | 06:31 |
| rogpeppe | mornin' all! | 06:45 |
| bigjools | evening | 06:46 |
| TheMue | morning | 06:47 |
| davecheney | error: Failed to load data for branch bzr+ssh://bazaar.launchpad.net/~dave-cheney/juju-core/111-add-go-build-check-to-lbox/: Server returned 502 and body: <!DOCTYPE html> | 07:17 |
| davecheney | <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en"> | 07:17 |
| davecheney | thanks for coming in today, launchpad | 07:17 |
| * fwereade_ -> shops, bbiab | 07:33 | |
| dimitern | rogpeppe: ping | 09:07 |
| rogpeppe | dimitern: pong | 09:07 |
| * fwereade_ slept badly, and is more than usually stupid today, and is going to lie down for a bit | 09:07 | |
| dimitern | rogpeppe: hey, let me pass an idea by | 09:08 |
| rogpeppe | dimitern: ok | 09:08 |
| dimitern | rogpeppe: how about adding Id() to AgentState, so the jujud/agent can find out which entity id it's associated with and check if it's sane, and then set the password? | 09:08 |
| * rogpeppe goes to look | 09:10 | |
| rogpeppe | dimitern: i'm not sure i understand the motivation. | 09:11 |
| dimitern | rogpeppe: it seems all implementations, except the upgrader are backed by a state object - machine or unit | 09:11 |
| rogpeppe | dimitern: the code that sets the password first gets the entity | 09:11 |
| rogpeppe | dimitern: there's already a method, Entity, to do that | 09:11 |
| dimitern | rogpeppe: ah! | 09:11 |
| rogpeppe | dimitern: if we make the Entity code return an error if something about it is bogus, then you've got what you want, no? | 09:12 |
| rogpeppe | dimitern: that's what i was suggesting last night | 09:12 |
| dimitern | rogpeppe: exactly | 09:12 |
| dimitern | rogpeppe: for example how is a machine taken as an entity? | 09:13 |
| rogpeppe | dimitern: i'm not sure i understand the question | 09:13 |
| dimitern | rogpeppe: trying to find the Entity interface | 09:14 |
| rogpeppe | dimitern: there's no Entity interface | 09:14 |
| rogpeppe | dimitern: Entity returns an AgentState interface value | 09:14 |
| dimitern | rogpeppe: ok, but where's the Id()? | 09:14 |
| dimitern | rogpeppe: it's not in the AgentState | 09:14 |
| rogpeppe | dimitern: Entity is implemented by the agent in question. | 09:15 |
| dimitern | rogpeppe: and all I get back is an AgentState - should I cast it to something to get its Id? | 09:15 |
| rogpeppe | dimitern: so we have MachineAgent.Entity and UnitAgent.Entity | 09:15 |
| rogpeppe | dimitern: each of those knows the id | 09:15 |
| dimitern | rogpeppe: ok, got you - you mean only return the entity if it's sane | 09:15 |
| rogpeppe | dimitern: for instance, look at the implementation of MachineAgent.Entity | 09:15 |
| rogpeppe | dimitern: yes | 09:16 |
| rogpeppe | dimitern: there's already an error return | 09:16 |
| dimitern | rogpeppe: that should work, cheers | 09:16 |
| rogpeppe | dimitern: cool | 09:16 |
| rogpeppe | dimitern: you might find this quite cool: i've been trying to isolate a go runtime bug that occurs when running the uniter tests against go tip. i wanted to eliminate the possibility that it was a problem with our use of unsafe and cgo (goyaml uses cgo), so i mocked up goyaml to make rpc calls instead. here's the "mock" goyaml implementation: lp:~rogpeppe/+junk/yamlserver | 09:26 |
| rogpeppe | dimitern: it worked - i got uniter tests passing when using it, and i'm still seeing the runtime crashes. | 09:26 |
| dimitern | rogpeppe: cool, I'll take a look | 09:27 |
| rogpeppe | dimitern: i think it's nice that i can swap out an entire implementation like that and almost nothing needed to change in any of the other code (i just needed to change the uniter tests to register the expected types) | 09:28 |
| dimitern | rogpeppe: so it uses goyaml on the rpc server | 09:28 |
| rogpeppe | dimitern: yeah | 09:29 |
| dimitern | rogpeppe: and the crash in uniter tests is no longer there? | 09:29 |
| rogpeppe | dimitern: no, it still crashes | 09:29 |
| rogpeppe | dimitern: which is what i was hoping | 09:29 |
| dimitern | rogpeppe: so it's not our code then | 09:29 |
| rogpeppe | dimitern: because it means it's (almost certainly) not a problem with our code | 09:29 |
| rogpeppe | dimitern: yeah | 09:29 |
| dimitern | rogpeppe: good to hear, but any idea is it in goyaml or is it go-tip related bug? | 09:30 |
| rogpeppe | dimitern: it's a go-tip related bug. probably the new garbage collector. | 09:30 |
| rogpeppe | dimitern: but perhaps the new scheduler. | 09:31 |
| rogpeppe | dimitern: http://code.google.com/p/go/issues/detail?id=5193 | 09:31 |
| dimitern | rogpeppe: hmm.. it seems the closer we get to 1.1 release more nasty bugs appear | 09:31 |
| rogpeppe | dimitern: they've shoved a lot of stuff in quite recently. | 09:31 |
| rogpeppe | dimitern: i'm very concerned too. | 09:31 |
| dimitern | rogpeppe: it seems at least they're looking into it, it's not just collecting dust in the issues list | 09:34 |
| rogpeppe | dimitern: yeah, they're concerned too | 09:34 |
| dimitern | rogpeppe: well, there are a lot of smart people there, so there's hope of fixing it | 09:40 |
| rogpeppe | dimitern: yeah. it would be nice if i could replicate the problem with less than x00,000 lines of source code :-) | 09:40 |
| dimitern | rogpeppe: definitely :) | 09:41 |
| rogpeppe | dimitern: unfortunately the problem is so intermittent, that's not easy | 09:42 |
| dimitern | rogpeppe: how often can you reproduce it? | 09:47 |
| rogpeppe | dimitern: about once in every 3 or 4 runs | 09:47 |
| rogpeppe | dimitern: it dies in quite a few different ways | 09:48 |
| dimitern | rogpeppe: bugger | 09:49 |
| rogpeppe | dimitern: see the collection of panics in my last message on the above issue | 09:50 |
| dimitern | rogpeppe: in random places of the code or at least these are often the same? | 09:50 |
| dimitern | rogpeppe: ah, ok | 09:50 |
| dimitern | rogpeppe: nasty.. they're in at least 3 different places | 09:54 |
| rogpeppe | dimitern: yeah. i'm pretty sure they're all symptomatic of something else (for instance a race or other bug in the garbage collector) | 09:55 |
| rogpeppe | dimitern: i see other problems too which aren't panics, but are symptomatic of memory corruption | 09:55 |
| dimitern | rogpeppe: so you nailed it down to r16008 | 09:56 |
| dimitern | (the link is broken there btw) | 09:56 |
| rogpeppe | dimitern: i'm not sure. | 09:57 |
| rogpeppe | dimitern: that might just be the change that made the garbage collector bug show up | 09:57 |
| rogpeppe | dimitern: because 16008 is when the new scheduler was introduced | 09:57 |
| dimitern | rogpeppe: in can be in either the new scheduler or the GC, both nasty internals | 09:59 |
| dimitern | rogpeppe: oh well, we'll see how it goes | 09:59 |
| rogpeppe | dimitern: yeah | 09:59 |
| dimitern | rogpeppe: so if a machine doesn't have instid, is it correct to call EnsureDead() before returning it from the Entity()? That way it'll be detected in openState and the worker will die properly | 10:08 |
| fwereade_ | dimitern, hell no :) | 10:09 |
| dimitern | fwereade_: why? | 10:09 |
| fwereade_ | dimitern, we don't want the machine to die -- there'll be another agent along soon to do the right thing | 10:09 |
| fwereade_ | dimitern, we just want the bad agent to go away and not come back :) | 10:09 |
| rogpeppe | fwereade_: exactly | 10:10 |
| dimitern | fwereade_: so we want the instance dead, not the machine | 10:10 |
| fwereade_ | dimitern, yeah, exactly | 10:10 |
| rogpeppe | dimitern: and the agent will die immediately anyway | 10:10 |
| dimitern | rogpeppe: not really | 10:10 |
| fwereade_ | dimitern, sorry about the terminology, it is annoying ;) | 10:10 |
| dimitern | rogpeppe: it'll die (now) iff the machine is missing or dead | 10:10 |
| rogpeppe | dimitern: because the run loop will terminate | 10:10 |
| rogpeppe | dimitern: ah, you're right - it'll keep on trying | 10:11 |
| fwereade_ | dimitern, rogpeppe: how would you feel about just writing a flag somewhere local saying "stopped" or something | 10:11 |
| dimitern | rogpeppe: I'll have to introduce a specific ErrUnprovisioned for example, and use that to amend the cases in which the worker dies | 10:11 |
| rogpeppe | fwereade_: i really don't care about this case | 10:11 |
| rogpeppe | fwereade_: it's a ghost machine | 10:12 |
| rogpeppe | fwereade_: i don't care if it carries on trying | 10:12 |
| rogpeppe | a ghost *instance*, rather | 10:12 |
| dimitern | rogpeppe: but it'll be nicer to finish off the rouge MA as early as possible in this case, isn't it? | 10:12 |
| fwereade_ | rogpeppe, so long as we return nil from Run it'll only retry once per restart anyway I guess | 10:12 |
| fwereade_ | rogpeppe, but wait | 10:12 |
| rogpeppe | dimitern: i don't want to introduce more logic for this case which is actually vanishingly unlikely | 10:13 |
| fwereade_ | rogpeppe, if we *don't* mark it stopped, then a badly-timed restart of its instance can fuck up the other machine, I think | 10:13 |
| rogpeppe | fwereade_: how so? | 10:13 |
| dimitern | fwereade_, rogpeppe: how about that specific error (ErrUnprovisioned) or something? | 10:14 |
| dimitern | (returned from MA.Entity) | 10:14 |
| dimitern | or maybe (nastier) return NotFoundf(), although it's clearly a lie, but it'll only affect the agent | 10:15 |
| fwereade_ | rogpeppe, agreed vanshingly unlikely, but: (1) bad machine logs in with old password (2) provisioner sets password for good machine and starts an instance (3) bad machine changes password and starts running | 10:15 |
| fwereade_ | rogpeppe, (4) provisioner takes down bad machine (5) good machine can't run | 10:15 |
| dimitern | fwereade_: why is this at all so unlikely? | 10:16 |
| fwereade_ | dimitern, it requires pathologically bad timing | 10:16 |
| dimitern | fwereade_: given infinite time and scenarios, everything is possible :) | 10:16 |
| fwereade_ | dimitern, exactly so | 10:17 |
| fwereade_ | dimitern, that is why I would prefer a little tweak to the agent running | 10:17 |
| fwereade_ | dimitern, first thing an agent should do is check its conf for a stopped flag; last thing it should do if it's returning nil from Run is to set the stopped flag | 10:18 |
| rogpeppe | fwereade_: why would the bad machine ever get as far as changing the password? | 10:18 |
| dimitern | rogpeppe: this happens first thing after running the MA, right? | 10:18 |
| fwereade_ | rogpeppe, because the provisioner set one at the worst possible time... say during a 5-second t1.micro sleep on the bad machine | 10:18 |
| rogpeppe | fwereade_: the bad machine doesn't care - it looks at the instance id and sees that it doesn't match its own instance | 10:19 |
| rogpeppe | fwereade_: i don't see that there's any way it can get past that barrier | 10:19 |
| fwereade_ | rogpeppe, how's it meant to find that out? we're planning to drop InstanceId from EnvironProvider | 10:19 |
| dimitern | rogpeppe: it cannot know it's own instance id, only the machine id | 10:19 |
| dimitern | rogpeppe: it can check is it "" or not | 10:19 |
| rogpeppe | fwereade_: orly? | 10:20 |
| fwereade_ | rogpeppe, yesrly, it's caused problems for both maas and openstack | 10:20 |
| rogpeppe | fwereade_: so how do we deal with the "provisioner unprovisions itself" problem? | 10:20 |
| fwereade_ | rogpeppe, and there's no reason for it except to prevent the provisioner from screwing itself | 10:20 |
| fwereade_ | rogpeppe, so, we special-case in the provisioner instead | 10:21 |
| dimitern | fwereade_: I think MAAS guys figured it out, and in openstack it can be done through the storage | 10:21 |
| fwereade_ | dimitern, I know it can be done | 10:21 |
| rogpeppe | fwereade_: how does that work? | 10:21 |
| fwereade_ | dimitern, but there is a different shitty wa of doing it in each case | 10:21 |
| fwereade_ | rogpeppe, if there's only one machine in state, grab the instance id from provider state... done. right? | 10:22 |
| fwereade_ | dimitern, note that the put-it-in-environ-state works for the bootstrap machine only -- it won't let us get an InstanceId on an arbitrary machine | 10:23 |
| * dimitern keeps waiting for an answer on errUnprovisioned (locally in jujud only) | 10:23 | |
| rogpeppe | fwereade_: assuming the machine doesn't come up really fast, i suppose :-) | 10:23 |
| rogpeppe | dimitern: we're getting there :-) | 10:24 |
| dimitern | :) | 10:24 |
| fwereade_ | rogpeppe, even if it does, we fail out and get restarted in 5s | 10:24 |
| rogpeppe | fwereade_: no, if it does, we unprovision ourselves | 10:24 |
| rogpeppe | fwereade_: or... no, i see | 10:24 |
| fwereade_ | rogpeppe, this would happen before we do anything else, so I think it's safe | 10:25 |
| rogpeppe | fwereade_: i'm not sure you can guarantee it happens before we do anything else | 10:25 |
| rogpeppe | fwereade_: the provisioner is just another API client, right? | 10:26 |
| fwereade_ | rogpeppe, what's affected by this other than the provisioner? | 10:26 |
| rogpeppe | fwereade_: someone external could jump in before the provisioner and add another machine | 10:26 |
| fwereade_ | rogpeppe, ah! | 10:26 |
| fwereade_ | rogpeppe, ok, so we special-case it to be machine 0 then ;p | 10:26 |
| dimitern | fwereade_: how about HA state then? | 10:27 |
| rogpeppe | dimitern: HA state is fine - we only have this problem at bootstrap | 10:27 |
| fwereade_ | dimitern, HA state is orthogonal, I still plan to bootstrap one machine and add HA later | 10:28 |
| dimitern | ah, ok then | 10:28 |
| fwereade_ | dimitern, even if it's part of jujud bootstrap itself to add N more machines to state directly | 10:28 |
| rogpeppe | fwereade_: ah, i've got an idea | 10:28 |
| rogpeppe | fwereade_: the bootstrap init code marks machine 0 with an instance-id "pending" | 10:29 |
| rogpeppe | fwereade_: when the provisioner comes up, if it sees machine 0 with "pending" instance id, it fetches it from the provider | 10:29 |
| rogpeppe | fwereade_: otherwise i don't see how the machine-0 special casing works | 10:30 |
| fwereade_ | rogpeppe, not sure about a magic value in that field, I'd kinda rather it be separate | 10:31 |
| * dimitern still doesn't quite get why we need to special-case machine-0 | 10:32 | |
| rogpeppe | dimitern: because machine-0 is the *only* place that haven't got the state to write the new instance id to | 10:32 |
| fwereade_ | dimitern, we can't always know machine 0's instance id before we provision it | 10:32 |
| fwereade_ | dimitern, and we can't write it to state just after we provisioned it, because there's no state yet | 10:32 |
| rogpeppe | fwereade_: yeah, an extra field in Machine would work as well | 10:33 |
| dimitern | oh, I see now | 10:33 |
| fwereade_ | rogpeppe, I'm wondering whether it's actually a property of the environment | 10:33 |
| rogpeppe | fwereade_: oh, doh! | 10:34 |
| fwereade_ | rogpeppe, Environment.BootstrapComplete() (bool, error) ? | 10:34 |
| rogpeppe | fwereade_: it's something that can be done by bootstrap-init | 10:34 |
| rogpeppe | fwereade_: the provisioner doesn't need to know anything about it | 10:34 |
| fwereade_ | rogpeppe, hmm | 10:35 |
| rogpeppe | fwereade_: i mean bootstrap-state, of course | 10:35 |
| fwereade_ | rogpeppe, yeah, that's right | 10:35 |
| fwereade_ | rogpeppe, oh no wait | 10:35 |
| fwereade_ | rogpeppe, we need the user to have connected once before we have the keys that let us find out the instance id | 10:36 |
| * dimitern goes for a run until the argument settles, bbi30m | 10:36 | |
| fwereade_ | rogpeppe, that doesn't matter now | 10:36 |
| dimitern | :) | 10:36 |
| rogpeppe | fwereade_: hmm, i suppose so. unless the provider can make it available to anyone | 10:37 |
| fwereade_ | rogpeppe, but might it if the CLI were doing an API connection? | 10:37 |
| fwereade_ | rogpeppe, I think it's ok to put it in the provisioner anyway -- the locality of reference would help, I think | 10:38 |
| rogpeppe | fwereade_: the code would be much simpler if it could go in bootstrap-state, but i'd forgotten about the provisioner keys issue | 10:38 |
| fwereade_ | rogpeppe, I know it would :( | 10:39 |
| fwereade_ | rogpeppe, but I honestly think it's not too bad | 10:39 |
| fwereade_ | rogpeppe, it exists to fix the provisioner | 10:39 |
| rogpeppe | fwereade_: yeah, but the muck spreads into quite a few places | 10:39 |
| rogpeppe | fwereade_: perhaps we change Machine.InstanceId to return, instead of a bool, one of InstanceIdNotSet, InstanceIdOk or InstanceIdPending, rather than add another bool. | 10:40 |
| rogpeppe | fwereade_: then if the provisioner finds machine 0 with InstanceIdPending, it knows where to look | 10:41 |
| rogpeppe | fwereade_: and it can then fill in the instance id and carry on as normal | 10:41 |
| fwereade_ | rogpeppe, it feels quite localized to me anyway though | 10:42 |
| rogpeppe | fwereade_: well, it affects the provisioners and the state as well as the provisioner, so not *very* localized :-) | 10:42 |
| rogpeppe | s/provisioners/providers/ | 10:43 |
| fwereade_ | rogpeppe, I don't think it even has to affect the providers | 10:43 |
| rogpeppe | fwereade_: don't they have to be able to return the instance id for machine 0 ? | 10:43 |
| fwereade_ | rogpeppe, they all have to list instances on demand, and those instances have an InstanceId method | 10:44 |
| mattyw | fwereade_, rogpeppe sorry to interupt, I'm still having problems connecting to the api https://pastebin.canonical.com/88504 | 10:44 |
| rogpeppe | fwereade_: that doesn't help | 10:44 |
| fwereade_ | rogpeppe, meaning that we only have to worry about a single mechanism for determining instance id | 10:44 |
| rogpeppe | fwereade_: we need to know which of those instances corresponds to machine 0's instance | 10:45 |
| rogpeppe | mattyw: OpenID discovery error: HTTP Response status from identity URL host is not 200. Got status 503 | 10:45 |
| fwereade_ | rogpeppe, how are those other instances going to get started without a provisioner? :) | 10:45 |
| rogpeppe | fwereade_: bootstrap | 10:45 |
| rogpeppe | fwereade_: and possibly by a previous environment | 10:45 |
| rogpeppe | fwereade_: if we could assume that we'd only ever get one instance after bootstrap, the problem would be much easier | 10:47 |
| fwereade_ | rogpeppe, do you mean a previous environment that has not been properly shut down, or just an eventual-consistency phantom? | 10:47 |
| rogpeppe | fwereade_: the former. | 10:47 |
| fwereade_ | rogpeppe, screw that, user error | 10:47 |
| fwereade_ | rogpeppe, if destroy-environment failed, run it again until it succeeds | 10:47 |
| rogpeppe | fwereade_: but eventual consistency could make a properly shut down environment show the same symptom | 10:48 |
| TheMue | aaaaaargh (facepalm) | 10:48 |
| TheMue | found the failure, only had to move three loc a bit. didn't seen which value differs otherwise in all that debugging output. (hmpf) | 10:50 |
| rogpeppe | fwereade_: the scenario that concerns me is when, somehow, the token that signifies that an environment has been bootstrapped has gone (perhaps someone removed all their buckets), so we bootstrap and find all the previous instances. | 10:52 |
| fwereade_ | rogpeppe, to be utterly explicit, this is when the env reports one terminated machine to be running and one running machine not to exist, right? | 10:52 |
| rogpeppe | fwereade_: no, it's when the env reports more than one running instance, only one of which is the bootstrapped instance. | 10:53 |
| mattyw | fwereade_, rogpeppe https://pastebin.canonical.com/88504/ should be working again now - it's trouble connecting to the api | 10:53 |
| fwereade_ | rogpeppe, meh, fail out when len(instances) != 1 | 10:53 |
| rogpeppe | fwereade_: that is a possibility, yeah. we could mark the bootstrap machine with an error status in that case. | 10:54 |
| fwereade_ | rogpeppe, not even that | 10:55 |
| fwereade_ | rogpeppe, just try again later | 10:55 |
| rogpeppe | fwereade_: the user needs at least some feedback, otherwise they'd have no idea why "juju deploy" isn't working. | 10:56 |
| rogpeppe | fwereade_: or... just don't unprovision any machines in that case, i guess | 10:56 |
| fwereade_ | rogpeppe, but wait I'm being stupid | 10:57 |
| fwereade_ | rogpeppe, we *always* set the instance id in the provider storage anyway | 10:57 |
| rogpeppe | fwereade_: yes | 10:57 |
| fwereade_ | rogpeppe, provisioner, as soon as it has keys, calls ensureBootstrapComplete() | 10:57 |
| rogpeppe | fwereade_: well, in the existing providers | 10:57 |
| rogpeppe | fwereade_: we don't make it available though | 10:58 |
| fwereade_ | rogpeppe, agreed that without storage we will have a hard time | 10:58 |
| rogpeppe | fwereade_: we had plans to move away from that storage in ec2 | 10:58 |
| rogpeppe | fwereade_: and use instance tags instead | 10:58 |
| rogpeppe | fwereade_: ah! | 10:58 |
| fwereade_ | rogpeppe, well, that would be an alternative solution | 10:58 |
| rogpeppe | fwereade_: this is a good reason why generating the env UUID on the client side would be good | 10:59 |
| fwereade_ | rogpeppe, tagging instances with uuid? yeah | 10:59 |
| rogpeppe | fwereade_: yeah | 10:59 |
| fwereade_ | rogpeppe, it feels to me like storage is the tool we have available now though | 10:59 |
| rogpeppe | fwereade_: that's true. but i'd like our solution to work with tags too. | 11:00 |
| rogpeppe | mattyw: looking | 11:00 |
| fwereade_ | rogpeppe, something funny about `invalid entity name ""` given the `info.Tag = "user-admin"` | 11:01 |
| rogpeppe | fwereade_: that's what i'm thinking | 11:01 |
| rogpeppe | mattyw: is this on go trunk? | 11:01 |
| rogpeppe | mattyw: i mean juju-core tip! | 11:02 |
| mattyw | rogpeppe, no, it's 2 or 3 days old | 11:03 |
| rogpeppe | mattyw: what revid? | 11:03 |
| mattyw | rogpeppe, 1060 I think *double checks* | 11:03 |
| mattyw | rogpeppe, just to make things worse, it's actually of our own branch https://code.launchpad.net/~cloud-green/juju-core/trunk so we had a stationary target to work on | 11:04 |
| rogpeppe | mattyw: that's ok - i'll branch that and have a look | 11:05 |
| fwereade_ | rogpeppe, anyway, for maximum paranoia we can wait until there is a running instance with an id that matches the one in storage | 11:07 |
| rogpeppe | fwereade_: i think just st.Machine("0").SetInstanceId(environ.InstanceIdOfBootstrapMachine()) should be fine | 11:08 |
| fwereade_ | rogpeppe, ok, sgtm :) | 11:09 |
| rogpeppe | mattyw: ok, looks like you're on 1091. have you made any of your own changes to that branch? | 11:11 |
| rogpeppe | mattyw: (if not, i'm not quite sure why you merged trunk rather than just pulling it) | 11:11 |
| fwereade_ | rogpeppe, btw did the terminated-flag-in-conf idea seem to hold water? | 11:12 |
| rogpeppe | fwereade_: oh yes, i was meaning to get back to that | 11:12 |
| rogpeppe | fwereade_: i'm not sure it eliminates the race entirely | 11:12 |
| fwereade_ | rogpeppe, hmm. "if there's an instance ID but an independent attempt to log in with my original credentials fails, it's not my instance id, so bomb out with errTerminating; otherwise go ahead and change it"? | 11:16 |
| rogpeppe | fwereade_: sounds plausible | 11:17 |
| rogpeppe | fwereade_: although a bit more heavyweight than i'd like | 11:18 |
| rogpeppe | fwereade_: a simple nonce might be better | 11:19 |
| fwereade_ | rogpeppe, chosen by provisioner, set on machine, passed through StartInstance? | 11:20 |
| rogpeppe | fwereade_: yup | 11:20 |
| fwereade_ | rogpeppe, nice | 11:20 |
| rogpeppe | fwereade_: perhaps have it returned by Machine.SetInstanceId | 11:21 |
| rogpeppe | fwereade_: which would increment it | 11:21 |
| fwereade_ | rogpeppe, we need to pick a nonce before we start the machine | 11:21 |
| rogpeppe | fwereade_: doh! | 11:21 |
| rogpeppe | fwereade_: just use a random number. | 11:23 |
| rogpeppe | fwereade_: if we get the provisioner to pick a random number n when it starts, then n.seqno actually provides useful information about which provisioner (in an HA world) provisioned the machine. | 11:25 |
| rogpeppe | fwereade_: and it's a nice consistent link between new machine and the state. i *think* i like it. | 11:27 |
| rogpeppe | s/new machine/new instance/ of course | 11:27 |
| fwereade_ | rogpeppe, I don't follow the "seqno" bit, but if you mean $(provisioner-id)-$(random-number) I'm +1 | 11:28 |
| rogpeppe | fwereade_: hmm, mind you, the initial password is a pretty good link too | 11:28 |
| fwereade_ | rogpeppe, a prefix on the random password? that feels a bit off to me | 11:29 |
| rogpeppe | fwereade_: i mean $(random-provisioner-id)-$(sequentially-incrementing-number) | 11:29 |
| rogpeppe | fwereade_: no, i mean that if you manage to log in with the initial password, you've a pretty good idea that you're talking to the right state. | 11:29 |
| fwereade_ | rogpeppe, damn lunch | 11:32 |
| rogpeppe | fwereade_: enjoy | 11:32 |
| * dimitern is back | 11:33 | |
| mattyw | rogpeppe, no changes, the only reason I haven't pulled is my inexperience with bzr | 11:33 |
| rogpeppe | mattyw: sorry, i'll take another look in a mo | 11:34 |
| rogpeppe | dimitern: there's already a way of telling the run loop to terminate | 11:36 |
| rogpeppe | dimitern: see isFatal in agent.go | 11:36 |
| rogpeppe | dimitern: if you return errUnprovisioned and add errUnprovisioned to the isFatal cases, it should all just work. | 11:37 |
| rogpeppe | dimitern: ha! | 11:37 |
| rogpeppe | dimitern: in fact, just return &fatalError{"machine was not provisioned"} | 11:38 |
| * rogpeppe feels a little bit smug | 11:38 | |
| rogpeppe | mattyw: are you bootstrapping with the same version as you're compiling your client against? | 11:39 |
| dimitern | just finished reading through the log | 11:39 |
| dimitern | rogpeppe: sgtm, nicer | 11:40 |
| dimitern | rogpeppe: unfortunately it doesn't work | 11:41 |
| rogpeppe | dimitern: oh? | 11:42 |
| dimitern | rogpeppe: isFatal is checked after we're in runLoop | 11:42 |
| rogpeppe | dimitern: and? | 11:43 |
| dimitern | rogpeppe: so it won't solve the issue about messing up the password by a bad machine | 11:43 |
| rogpeppe | dimitern: the password is changed inside runOnce | 11:43 |
| rogpeppe | dimitern: erm... i think :-) | 11:43 |
| rogpeppe | dimitern: yeah | 11:44 |
| dimitern | rogpeppe: istm it's in openState:SetMongoPassword | 11:44 |
| rogpeppe | dimitern: yup | 11:44 |
| rogpeppe | dimitern: and what's openState called by? | 11:44 |
| dimitern | rogpeppe: by RunLoop's callback in runagentloop | 11:44 |
| rogpeppe | dimitern: which is called? | 11:45 |
| dimitern | rogpeppe: so it's too late then | 11:45 |
| rogpeppe | dimitern: i don't think so | 11:45 |
| rogpeppe | dimitern: runOnce calls openState | 11:45 |
| dimitern | rogpeppe: why not just have Entity return a fatalError instead and add it to the conditions? | 11:45 |
| rogpeppe | dimitern: what conditions? | 11:46 |
| dimitern | if state.IsNotFound(err) || err == nil && entity.Life() == state.Dead { err = worker.ErrDead } | 11:46 |
| dimitern | in openState | 11:47 |
| dimitern | make that || isFatal(err) || ... and all done | 11:47 |
| rogpeppe | dimitern: there's no point in doing that | 11:47 |
| rogpeppe | dimitern: if the err is not nil, it returns in | 11:48 |
| rogpeppe | s/in/it | 11:48 |
| rogpeppe | dimitern: on the next line | 11:48 |
| dimitern | rogpeppe: that's ater | 11:48 |
| dimitern | rogpeppe: yeah, the first if is what i'm aiming for | 11:48 |
| rogpeppe | dimitern: you want openState to return ErrDead? | 11:48 |
| dimitern | rogpeppe: it already does, doesn't it? | 11:49 |
| rogpeppe | dimitern: yeah, or a fatalError if Entity returns that | 11:49 |
| dimitern | rogpeppe: ok, it seems I'm not explaining myself well enough | 11:49 |
| dimitern | rogpeppe: err = worker.ErrDead is what I want to be the case, just like the other conditions, where the machine is missing or dead | 11:50 |
| dimitern | rogpeppe: this causes the next if to return ErrDead | 11:50 |
| dimitern | rogpeppe: and kills the worker before setting the password or anything bad happens | 11:50 |
| rogpeppe | dimitern: even if you return a fatalError, the worker will be killed before that | 11:50 |
| rogpeppe | dimitern: but... | 11:51 |
| rogpeppe | dimitern: there's something to be said for returning ErrDead, because then the agent exits and never restarts | 11:51 |
| dimitern | rogpeppe: yeah, actually.. but the test fails | 11:51 |
| dimitern | rogpeppe: look at this http://paste.ubuntu.com/5679464/ | 11:52 |
| rogpeppe | dimitern: are you saying you're returning a *fatalError from Entity and the agent isn't exiting? | 11:52 |
| dimitern | rogpeppe: this test succeeds when I used errUnprovisioned and checked for it in the first if, with fatalError it fails, saying it's still running | 11:52 |
| rogpeppe | dimitern: could you paste your Entity implementation, please? | 11:52 |
| dimitern | rogpeppe: http://paste.ubuntu.com/5679472/ | 11:54 |
| dimitern | rogpeppe: and now I'm getting "timed out waiting for agent to finish; stop error: <nil>" in the assert after runAgentTimeout | 11:55 |
| mattyw | rogpeppe, no, I'm not bootstrapping using the same version | 11:55 |
| rogpeppe | dimitern: hmm, that's weird. | 11:55 |
| rogpeppe | dimitern: it *should* work. could you push your branch? | 11:56 |
| dimitern | rogpeppe: ok, just a sec | 11:56 |
| rogpeppe | mattyw: are you bootstrapping with a newer version, by any chance? | 11:56 |
| rogpeppe | mattyw: the API has recently changed in a backwardly incompatible way | 11:56 |
| mattyw | rogpeppe, I don't think so. My juju command is 1.9.12-precise-amd64 | 11:57 |
| rogpeppe | mattyw: in general, while we're developing stuff fast, you need to run exactly the same version of client and server. | 11:57 |
| mattyw | rogpeppe, ok. I'll try that | 11:57 |
| rogpeppe | mattyw: you can use juju bootstrap --upload-tools | 11:57 |
| rogpeppe | mattyw: but currently any charms you deploy have to be from the same series as your local machine | 11:58 |
| mattyw | rogpeppe, I forget, I've had a lot of problems trying to bootstrap at all recently | 11:58 |
| rogpeppe | mattyw: every time you see a problem, add a bug! | 11:58 |
| mattyw | rogpeppe, they're the same series | 11:58 |
| mattyw | rogpeppe, I do :) | 11:58 |
| dimitern | rogpeppe: lp:~dimitern/+junk/machine-agent-status | 11:59 |
| rogpeppe | mattyw: (or add to an existing bug if it seems the same) | 11:59 |
| rogpeppe | dimitern: ta | 11:59 |
| mattyw | rogpeppe, is there a version of the repo you'd recomend running against at the moment? | 12:00 |
| rogpeppe | mattyw: i'd try and keep in sync with tip, though it's a pain | 12:02 |
| dimitern | rogpeppe: hmm.. for some reason the old change got pushed, not the one with fatalError | 12:03 |
| rogpeppe | dimitern: perhaps you didn't commit | 12:03 |
| dimitern | rogpeppe: ouch :( haven't saved it in emacs | 12:03 |
| dimitern | rogpeppe: sorry, let me try again | 12:03 |
| rogpeppe | dimitern: ah, that might have something to do with it! | 12:04 |
| dimitern | rogpeppe: now it works with fatalError, sorry for the noise | 12:05 |
| rogpeppe | dimitern: np. i was a bit confused :-) | 12:05 |
| rogpeppe | dimitern: but... | 12:05 |
| dimitern | rogpeppe: the only change is I needed to change the assert to check for the error after runAgentTimeout | 12:05 |
| rogpeppe | dimitern: having said that... | 12:05 |
| rogpeppe | dimitern: i'm wondering if you should return ErrDead anyway | 12:06 |
| dimitern | rogpeppe: maybe upstart will mess it up if it's not ErrDead and restart it? | 12:06 |
| rogpeppe | dimitern: exactly | 12:06 |
| fwereade_ | rogpeppe, mattyw: FWIW I landed --fake-series yesterday | 12:07 |
| fwereade_ | rogpeppe, mattyw: it seemed to work for me | 12:07 |
| rogpeppe | dimitern: in fact, that's precisely the behaviour we *want* when upgrading | 12:07 |
| rogpeppe | fwereade_: \o/ | 12:07 |
| dimitern | rogpeppe: ok, ErrDead it is then - directly from the Entity() | 12:07 |
| fwereade_ | rogpeppe, mattyw: `juju bootstrap --upload-tools --fake-series precise,quantal` | 12:07 |
| dimitern | rogpeppe: or fatalError + isFatal() in the if -> ErrDead? | 12:07 |
| rogpeppe | dimitern: i think so. although it's kind of a lie. | 12:08 |
| fwereade_ | rogpeppe, I'm -1 on reusing ErrDead | 12:08 |
| rogpeppe | dimitern: nononono | 12:08 |
| fwereade_ | rogpeppe, it means something specific | 12:08 |
| mattyw | rogpeppe, ok thanks, what does --upload-tools actually do? | 12:08 |
| rogpeppe | fwereade_: yes | 12:08 |
| dimitern | so we're back on errUnprovisioned :) | 12:08 |
| rogpeppe | fwereade_: i think we're just using ErrDead as a convenience | 12:09 |
| rogpeppe | fwereade_: i think we can change all instances of ErrDead to ErrDeadOrInvalid | 12:09 |
| fwereade_ | rogpeppe, dimitern: what's wrong with errTerminating? the top-level Run can set the conf to terminated and return nil; other conditions that indicate we should stop, like ErrDead, can do the same | 12:09 |
| fwereade_ | rogpeppe, call it ErrTerminateAgent and I'm fine | 12:10 |
| rogpeppe | fwereade_: what do you mean by "set the conf to terminated"? | 12:10 |
| * dimitern an there goes another 20m for 2 lines of code ;) | 12:10 | |
| fwereade_ | rogpeppe, I mean, set a flag meaning "don't even try" that we can check on startup | 12:11 |
| rogpeppe | fwereade_: why bother? | 12:11 |
| dimitern | I'm fine to call it errTerminated instead of errUnprovisioned, although the former seems not quite right - do you intend to reuse it for something else? | 12:11 |
| rogpeppe | dimitern: i think i'd have type terminalError {err string} | 12:12 |
| rogpeppe | dimitern: then the message printed by the agent when exiting can be accurate | 12:12 |
| dimitern | rogpeppe: lethalCase{} ? :D | 12:12 |
| fwereade_ | dimitern, I'm saying that Run at the top level should have a single error that it treats as "return nil, causing upstart to stop bothering" | 12:12 |
| rogpeppe | fwereade_: agreed. currently that's just ErrDead. | 12:13 |
| dimitern | fwereade_: we don't even have to get to run | 12:13 |
| fwereade_ | rogpeppe, maybe we don't, let me reload state | 12:13 |
| rogpeppe | fwereade_: but we'd change that to check for *terminalError | 12:13 |
| rogpeppe | fwereade_: ah! it does need to check for ErrDead too | 12:14 |
| rogpeppe | fwereade_: because that's what the workers return if the machine is dead. | 12:15 |
| fwereade_ | rogpeppe, yeah, exactly | 12:15 |
| rogpeppe | fwereade_: but tbh why not just reuse it? | 12:15 |
| rogpeppe | fwereade_: we're adding more code for a marginal case | 12:15 |
| fwereade_ | rogpeppe, I'm fine having the workers who need to return ErrTerminateAgent, I'm not so fine having an unprovisioned machine return ErrDead | 12:15 |
| rogpeppe | fwereade_: the machine might as well be dead as far as the agent is concerned | 12:16 |
| rogpeppe | fwereade_: ok, i'd be happy with that | 12:16 |
| * rogpeppe has lunch | 12:16 | |
| fwereade_ | rogpeppe, cool, cheers | 12:16 |
| dimitern | fwereade_: how about having a isDeadOrTerminated(err) checking for both ErrDead and ErrTerminated and is it in place of err == ErrDead checks? | 12:18 |
| fwereade_ | dimitern, I'm just saying s/ErrDead/ErrTerminateAgent/ throughout | 12:20 |
| dimitern | fwereade_: actually, I can just add ErrTerminateAgent and return in from Entity, make it work like ErrDead wrt upstart and draw the line there, changing all the workers can be a follow-up | 12:21 |
| fwereade_ | dimitern, forget ErrDead | 12:21 |
| fwereade_ | dimitern, a single magic value meaning "stop and don't come back" is all I'm really thinking about here | 12:21 |
| fwereade_ | dimitern, ErrDead exists, and is perfect, except it has the wrong name | 12:21 |
| dimitern | fwereade_: i'm not comfortable changing something without knowing the repercussions | 12:21 |
| fwereade_ | dimitern, fix the name and we're fine :) | 12:22 |
| dimitern | fwereade_: what is ErrDead used for - in all cases? | 12:22 |
| dimitern | (for workers) | 12:22 |
| fwereade_ | dimitern, it is used, in all cases, to indicate that the running agent should exit 0 | 12:22 |
| dimitern | fwereade_: well, if it's only that, the renaming makes sens | 12:23 |
| dimitern | sense | 12:23 |
| fwereade_ | dimitern, cool | 12:24 |
| fwereade_ | sorry, I definitely didn't communicate correctly | 12:24 |
| dimitern | fwereade_: I'll do it as a prereq though, since it's trivial | 12:24 |
| fwereade_ | dimitern, sgtm -- but just to sync up, can we have a quick hangout? | 12:25 |
| mattyw | rogpeppe, that seems to be working now thanks :) | 12:28 |
| rogpeppe | mattyw: cool. sorry about that - it's been a problem for loads of people, because we're changing things in a backwardly incompatible way all the time without changing the major version number | 12:29 |
| rogpeppe | mattyw: but that's dev versions for ya! | 12:29 |
| mattyw | rogpeppe, one quick odd question | 12:36 |
| rogpeppe | mattyw: ok | 12:36 |
| mattyw | when a unit gets removed we get a unitinfo back with all the status about that unit? Is the plan to stay like that? The reason I ask is that in the pyjuju 'api' when a unit is removed we only get told the unit name of the removed unit | 12:37 |
| rogpeppe | mattyw: yeah, it should stay like that. | 12:38 |
| rogpeppe | mattyw: although it's kind of an unintended consequence of the way things are implemented | 12:39 |
| mattyw | rogpeppe, ok thanks, some of my favourite consequences are the unintended ones | 12:39 |
| mattyw | rogpeppe, once the api ships I guess any change we'll get told about? | 12:40 |
| rogpeppe | mattyw: yup | 12:40 |
| mattyw | rogpeppe, that's fine for me. When vds and I have a few more tests It would be good if you could take a look, will probably be next week though | 12:40 |
| === jam1 is now known as jam | ||
| rogpeppe | mattyw: sure | 12:42 |
| === BradCrittenden is now known as bac | ||
| rogpeppe | fwereade_: do you know the PPA/name of the SSL-capable mongod package, by any chance? | 13:44 |
| fwereade_ | rogpeppe, sorry, I'm afraid not -- I always just grabbed it from the bucket and stuck in on $PATH | 13:45 |
| rogpeppe | fwereade_: yeah, i did too. i'm trying to put some instructions together so that people can try to reproduce the go-runtime errors i've been seeing | 13:46 |
| rogpeppe | fwereade_: and i seemed to remember someone had packaged mongod | 13:46 |
| rogpeppe | fwereade_: i guess i'll just say "grab the bucket contents" | 13:46 |
| rogpeppe | niemeyer: hiya | 13:46 |
| dimitern | fwereade_, rogpeppe: ErrDead -> ErrTerminateAgent https://codereview.appspot.com/8416043 | 13:49 |
| fwereade_ | dimitern, LGTM trivial | 14:00 |
| dimitern | fwereade_: cheers | 14:01 |
| rogpeppe | niemeyer: https://codereview.appspot.com/8367044 (not that it helped, in fact, but worth doing anyway) | 14:07 |
| teknico | rogpeppe, you're possibly looking for https://launchpad.net/~julian-edwards/+archive/mongodb/+packages | 14:40 |
| rogpeppe | teknico: thanks | 14:40 |
| rogpeppe | teknico: though i've just gone with directing people to the public bucket for now | 14:40 |
| rogpeppe | teknico: (with an accompanying sha1 sum) | 14:40 |
| niemeyer | rogpeppe: Rob spent a good while last night looking at the issue | 14:55 |
| niemeyer | rogpeppe: I've helped them setting up a juju environment so they could reproduce the issue | 14:55 |
| niemeyer | rogpeppe: He wrote down notes by the end of his day, and I suppose Dmitry moved it on | 14:56 |
| rogpeppe | niemeyer: thanks for that | 15:03 |
| niemeyer | rogpeppe: np, let's hope good stuff comes out of it | 15:05 |
| * rogpeppe crosses his fingers | 15:05 | |
| dimitern | cloudinit tests are a proper pain to change | 15:07 |
| rogpeppe | dimitern: if you can think of a better way... | 15:07 |
| rogpeppe | dimitern: they're better than they used to be | 15:07 |
| rogpeppe | dimitern: a little script could probably help actually | 15:08 |
| dimitern | :) i'm not sticking my finger in there any more than I need to for now | 15:09 |
| rogpeppe | mgz: hey mr on-call :-) a quick trivial branch for you (code move only) https://codereview.appspot.com/8422043 | 15:16 |
| mgz | looking | 15:17 |
| mgz | mega over usage :) | 15:18 |
| mgz | are file-level comments a thing in go? | 15:18 |
| rogpeppe | mgz: well, yeah | 15:18 |
| rogpeppe | mgz: yes, we can do that though we tend not to | 15:18 |
| mgz | like, triple quote at top of module saying roughly what it's about? | 15:18 |
| mgz | or is the pre-function /// | 15:19 |
| rogpeppe | mgz: usually the type declaration at the top gives a good idea | 15:19 |
| mgz | // funcName | 15:19 |
| mgz | the only special magic that the doc generation picks up on? | 15:19 |
| rogpeppe | mgz: file-level comments are ignored by the doc tools | 15:19 |
| mgz | thanks | 15:19 |
| rogpeppe | mgz: that and comments just before the package decl | 15:20 |
| rogpeppe | mgz: the doc tool picks up comments attached directly to top level declarations, essentially | 15:20 |
| mgz | lgtm'd | 15:21 |
| rogpeppe | mgz: ta! | 15:22 |
| mgz | dimitern: ^if you have a moment and if rog wants two reviews | 15:22 |
| rogpeppe | mgz: i'm not sure that i can say much in a file-level comment that the type decl and doc comment at the top don't already say | 15:23 |
| rogpeppe | mgz: i think i'll leave it as is if that's ok | 15:24 |
| mgz | that's a fair enough answer for one-struct files I think | 15:24 |
| mgz | which this isn't quite, but nearly :) | 15:25 |
| dimitern | mgz: oh, the reviewer showed up ;) | 15:26 |
| mgz | I was expecting pokes | 15:27 |
| dimitern | let me read through the log | 15:27 |
| mgz | browsing randomly for stuff to do is painful with rietveld... | 15:27 |
| mgz | dimitern: just click the codereview link, it's pretty clear | 15:27 |
| dimitern | mgz: it is, but fortunately fwereade is keeping +activereviews neat now | 15:28 |
| dimitern | looking, but it's submitted already | 15:29 |
| mgz | that answers clause #2 then :) | 15:29 |
| dimitern | what's clause #2? | 15:29 |
| mgz | if rog wanted another review | 15:30 |
| rogpeppe | mgz, dimitern: here's another fairly trivial one: https://codereview.appspot.com/8294044/ | 15:31 |
| dimitern | rogpeppe: just opened it while you're writing :) | 15:32 |
| mgz | gaba? | 15:32 |
| mgz | leading underscore? | 15:32 |
| rogpeppe | mgz: python-style :-) | 15:33 |
| mgz | so, as a hint internal to the package past the caps-vs-nocaps exposure? | 15:33 |
| rogpeppe | mgz: yeah | 15:33 |
| mgz | I guess that seems reasonable | 15:34 |
| rogpeppe | fwereade: what do you think? | 15:34 |
| dimitern | rogpeppe: why do you need this? | 15:34 |
| fwereade | rogpeppe, that sounds sensible to me | 15:34 |
| fwereade | dimitern, because package-level-only encapsulation is scary as hell once you're past... say... 1 file per package ;) | 15:35 |
| rogpeppe | dimitern: because i'm essentially exposing the allInfo type as an API to the backing type. some methods are not supposed to be called by the backing. | 15:35 |
| dimitern | rogpeppe: sure, but what will this solve? | 15:35 |
| rogpeppe | dimitern: it just makes it easy to verify that the backing is only calling expected methods | 15:36 |
| rogpeppe | dimitern: and if you call a _method from outside the type itself, you'll probably think twice | 15:37 |
| dimitern | rogpeppe: how about putting really internal stuff in a subpackage? | 15:37 |
| rogpeppe | dimitern: i'm not sure. it seems very intertwined with the allWatcher implementation | 15:39 |
| rogpeppe | dimitern: and that need to be internal to state, i *think*. | 15:39 |
| dimitern | rogpeppe: this *could* indicate code smell, but otherwise I'm fine with it | 15:40 |
| rogpeppe | dimitern: actually, i think you're perhaps right - the allWatcher can trivially be moved into its own package. | 15:44 |
| rogpeppe | dimitern: while the backing implementation remains inside state | 15:44 |
| dimitern | rogpeppe: sgtm | 15:44 |
| dimitern | fwereade, rogpeppe: machine nonce introduced https://codereview.appspot.com/8247045/ | 16:04 |
| rogpeppe | dimitern: cool; will look soon. | 16:04 |
| rogpeppe | dimitern: actually, i'm not sure, peer reviews need doing by the end of day | 16:04 |
| rogpeppe | dimitern: and i haven't done any yet | 16:05 |
| dimitern | rogpeppe: what do you mean? | 16:05 |
| rogpeppe | dimitern: the allhands stuff | 16:05 |
| dimitern | rogpeppe: ah, yeah - I've done mine on the day | 16:06 |
| rogpeppe | dimitern: v sensible! | 16:06 |
| dimitern | rogpeppe: I know myself how forgetful I can get, so I try to cheat and do it right away, so I can forget it comfortably :) | 16:06 |
| rogpeppe | dimitern: that's not cheating, that's the right thing to do! | 16:07 |
| dimitern | rogpeppe: yeah, i meant cheating the lazyness :) | 16:08 |
| === deryck is now known as deryck[lunch] | ||
| fwereade | TheMue, reviewed | 16:53 |
| fwereade | dimitern, why is MachineNonce required in Conf? | 16:58 |
| dimitern | fwereade: that's what I asked earlier - should it be or not | 16:58 |
| dimitern | fwereade: but I think it's better to be required and add tests for it | 16:59 |
| dimitern | next one coming - state support for nonce provisioning - https://codereview.appspot.com/8429044/ | 16:59 |
| fwereade | dimitern, sorry, I thought you meant in state | 16:59 |
| fwereade | dimitern, but it shouldn't be required for agent.Conf -- that has to work for unit agents too | 17:00 |
| dimitern | fwereade: oops, I mean *environs* | 17:00 |
| fwereade | dimitern, cool | 17:00 |
| dimitern | fwereade: so no tests then? only in cloudinit perhaps\ | 17:00 |
| fwereade | dimitern, in cloudinit sounds good | 17:01 |
| dimitern | fwereade: this will simplify things a lot | 17:01 |
| fwereade | dimitern, and you should at least check that a Conf can be read/written both with and without a MachineNonce | 17:01 |
| dimitern | fwereade: wasn't quite sure it'll be enough, but it's better to cut than to add ;) | 17:01 |
| dimitern | fwereade: you mean add a no-error test with machinenonce in? | 17:02 |
| dimitern | fwereade: and one without | 17:02 |
| fwereade | dimitern, yeah, so really leave the rest and just add a single test that includes it I think | 17:03 |
| dimitern | fwereade: deal | 17:04 |
| dimitern | fwereade: i'm starting on state now | 17:04 |
| rogpeppe | dimitern: allwatcher now factored into its own package. the tests weren't entirely trivial, but altogether fairly painless | 17:08 |
| dimitern | rogpeppe: great to hear! | 17:09 |
| niemeyer | rogpeppe: You mentioned yesterday that you reproduced the bug on socket.Acquire even after the fix was made, right? | 17:11 |
| rogpeppe | niemeyer: yes | 17:12 |
| niemeyer | rogpeppe: Okay, cool | 17:12 |
| niemeyer | rogpeppe: Just want to make sure I'm not on crac | 17:12 |
| rogpeppe | niemeyer: all the panics in my last post on that issue were with that fix made | 17:12 |
| niemeyer | k | 17:12 |
| rogpeppe | niemeyer: yeah, i've spent a fair amount of time trying to convince myself of the same thing | 17:12 |
| niemeyer | rogpeppe: Oh, might be worth mentioning it in that thread | 17:12 |
| niemeyer | rogpeppe: Were the tracebacks made with the rpc trick too? | 17:15 |
| rogpeppe | niemeyer: yes | 17:16 |
| rogpeppe | niemeyer: (i did mention that actually) | 17:16 |
| niemeyer | rogpeppe: Yeah, it was Rob that seemed unaware in the thread | 17:16 |
| fwereade | dimitern, reviewed | 17:21 |
| dimitern | fwereade: tyvm | 17:26 |
| niemeyer | fwereade: FYI, juju publish will be up for review in a few | 17:37 |
| fwereade | niemeyer, awesome, sorry I've been lagging a bit on your reviews | 17:37 |
| niemeyer | fwereade: It's fine.. I have all reviewed right now | 17:37 |
| niemeyer | fwereade: Just have to submit | 17:37 |
| niemeyer | fwereade: And the main piece is coming up in a moment | 17:37 |
| fwereade | niemeyer, ah, fantastic :) | 17:37 |
| niemeyer | fwereade: I still want to add a few features, but I want to get the core in before that | 17:38 |
| fwereade | niemeyer, +1 :) | 17:38 |
| dimitern | fwereade: why should CheckProvisioned(nonce) return an err ever? m.doc.Nonce == nonce should be enough, I think | 17:39 |
| fwereade | dimitern, ha, yes, I'm on crack | 17:39 |
| dimitern | fwereade: and it's because it can't be ever changed it'll either be that or emtpy | 17:39 |
| dimitern | fwereade: so, perhaps m.doc.Nonce == nonce && nonce != "" | 17:40 |
| fwereade | dimitern, but do check that InstanceId != "" as well, or make sure there's no way for it not to be (did we agree to panic on SetProvisioned with "" id?) | 17:40 |
| dimitern | fwereade: yeah, already taken care of in SetProvisioned | 17:40 |
| fwereade | dimitern, but you're absolutely right that we don't need to hit the network for that, cheers | 17:41 |
| dimitern | fwereade: notTheSame := D{{"instanceid", D{{"$nin", []InstanceId{"", id}}}}} | 17:41 |
| dimitern | fwereade: that's the extra assert in SetProvisioned | 17:41 |
| dimitern | fwereade: and it has corresponding bunch of errAborted handling (i'm proud :) ) | 17:42 |
| fwereade | dimitern, I look forward to seeing it :) | 17:42 |
| fwereade | although, everybody, I'm off for a bit now | 17:42 |
| fwereade | I'll probably be back at some stage, I'm up to my elbows in environs again, but I'm out of go-juice ATM | 17:42 |
| fwereade | happy weekends to those I miss | 17:43 |
| dimitern | fwereade: you too! | 17:43 |
| === deryck[lunch] is now known as deryck | ||
| rogpeppe | it's that time of day. happy weekend to all and sundry | 17:47 |
| TheMue | fwereade, rogpeppe: have a nice weekend | 17:48 |
| TheMue | fwereade: will propose the latest changes later | 17:48 |
| dimitern | rogpeppe, TheMue: both of you too ;) | 17:48 |
| TheMue | dimitern: enjoy the spare time to recreate | 17:49 |
| dimitern | TheMue: oh, i do, especially lately | 17:58 |
| TheMue | so, final propose is in, time for the weekend. bye | 19:28 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!