| davecheney | wallyworld: yeah, i plan to day | 00:22 |
|---|---|---|
| davecheney | just waiting for a smoke test build to pass | 00:23 |
| wallyworld | davecheney: thumper and i had a couple of fixes | 00:23 |
| wallyworld | is it too late? | 00:23 |
| davecheney | wallyworld: it is not too late | 00:24 |
| davecheney | remember my tagging score card | 00:24 |
| davecheney | 0 for 2 | 00:24 |
| wallyworld | davecheney: it would be great if you could +1 this then. you're the only person besides thumper who is around to ask sorry. https://codereview.appspot.com/11493043/ | 00:25 |
| davecheney | no wories | 00:25 |
| wallyworld | thanks. do you have a link to the release notes? | 00:26 |
| davecheney | https://docs.google.com/a/canonical.com/document/d/1ZBV6m0D1cfJQGoHW7EzJEc2qZeJFR38teHM_4OiYkeM/edit# | 00:26 |
| wallyworld | i'll add some shit | 00:26 |
| davecheney | capital! | 00:26 |
| davecheney | environs/azure isn't in environs/all | 00:29 |
| davecheney | but it is leaking into jujud somewhere | 00:29 |
| davecheney | i need to fix that quickly | 00:29 |
| thumper | davecheney: in my trunk, azure is in all | 00:52 |
| davecheney | yeah, for some reason I couldn't see it last night | 00:52 |
| davecheney | i'll have to submit a branch to remove it | 00:52 |
| thumper | +1 trivial from me | 00:52 |
| thumper | but I'll do it when you have it up | 00:53 |
| davecheney | wallyworld: LGTM on your branch | 00:53 |
| davecheney | ooops, sam is up | 00:53 |
| davecheney | afk for a few mins | 00:53 |
| wallyworld | davecheney: thanks, will land | 00:53 |
| sidnei | thumper: should i flip that mp to approved? iiuc there's a release about to be cut so maybe wait until after that? | 01:09 |
| davecheney | https://codereview.appspot.com/11557043 | 01:11 |
| davecheney | wallyworld: thumper could I give some love for this change ? | 01:12 |
| wallyworld | sure, looking | 01:12 |
| davecheney | sidnei: sure, check it in | 01:12 |
| sidnei | davecheney: oh, apparently i don't have perms. do the honors? https://code.launchpad.net/~sidnei/juju-core/mongo-2.4-compat/+merge/175653 | 01:13 |
| davecheney | sidnei: screw that, i'll give you perms | 01:13 |
| sidnei | /o\ | 01:13 |
| sidnei | ok :) | 01:13 |
| davecheney | try now | 01:14 |
| sidnei | done | 01:14 |
| sidnei | or. slow launchpad is slow. | 01:15 |
| sidnei | now for realz. | 01:15 |
| bigjools | davecheney: I had a patch from agl to handle tls renegs... can you take a look please? | 01:17 |
| davecheney | bigjools: FUCKING A | 01:17 |
| bigjools | you can say that if it works | 01:17 |
| bigjools | check your email | 01:18 |
| davecheney | bigjools: please send | 01:18 |
| davecheney | got it | 01:18 |
| davecheney | bigjools: speaking of azure signup | 01:18 |
| davecheney | did some dude try to call your to welcome you to the azure failmy | 01:19 |
| davecheney | family | 01:19 |
| bigjools | using what number? *cough* | 01:19 |
| thumper | wallyworld: hangout? | 01:20 |
| davecheney | i had to give a mobile number so they send me the activation code | 01:20 |
| wallyworld | ok | 01:20 |
| davecheney | i guess that is how they got my phone number | 01:20 |
| davecheney | i did use my @canonical email | 01:20 |
| davecheney | maybe this is VIP treatment | 01:20 |
| bigjools | ummmm maybe I did that | 01:25 |
| bigjools | never got called | 01:25 |
| bigjools | I can't remember what I did yesterday let alone 2 months ago | 01:25 |
| davecheney | good point | 01:25 |
| davecheney | bigjools: patch didn't apply cleanly, but i'll hack it in manually | 01:26 |
| bigjools | ok | 01:26 |
| bigjools | that's interesting | 01:26 |
| bigjools | what is he hacking on... | 01:26 |
| wallyworld | thumper: url? | 01:27 |
| thumper | https://plus.google.com/hangouts/_/e105bdb56dcdc0f23d38c9cdafc18eea12967111?hl=en | 01:27 |
| davecheney | no idea, the tls package hasn't received any updates for a looooong time | 01:27 |
| davecheney | I do know that google have their own internal version of Go | 01:28 |
| davecheney | but this isn't a fork | 01:28 |
| davecheney | it's just the way they managage external source into the big mega google/ repo | 01:28 |
| davecheney | so possibly he was working from that copy | 01:28 |
| davecheney | anyhoo | 01:28 |
| bigjools | davecheney: notice he said it's not suitable for upstream, so we still have a problem | 01:30 |
| davecheney | yup | 01:30 |
| bigjools | if they are that against it, fixing go-curl is the only realistic option | 01:31 |
| davecheney | sidnei: thouest forget the commit message | 01:31 |
| davecheney | bigjools: one elephant at a time | 01:31 |
| davecheney | ok, tagging time | 02:21 |
| bigjools | davecheney: did you get it working? | 02:27 |
| davecheney | bigjools: not yet | 02:29 |
| davecheney | it's next on my list after tagging 1.11.3 | 02:29 |
| bigjools | kk | 02:29 |
| wallyworld | davecheney: after a spurious bit failure, my last branch for release finally merged | 02:29 |
| davecheney | wallyworld: what is the push location for the _real_ trunk now ? | 02:29 |
| wallyworld | s/it/bot | 02:29 |
| wallyworld | ~gobot/juju-core of something i think | 02:30 |
| davecheney | lucky(~/devel/juju-core) % bzr push lp:~gobot/juju-core | 02:30 |
| davecheney | bzr: ERROR: Invalid url supplied to transport: "lp:~gobot/juju-core": No such person or team: gobot | 02:30 |
| davecheney | ahh, hyphen | 02:31 |
| wallyworld | go-ot sorry | 02:31 |
| wallyworld | i think you also need to add credentials | 02:31 |
| wallyworld | not sure | 02:31 |
| davecheney | lucky(~/devel/juju-core) % bzr push lp:~go-bot/juju-core/trunk | 02:31 |
| wallyworld | there's an email somewhere perhaps, i'll try and find it | 02:31 |
| davecheney | bzr: ERROR: Cannot lock LockDir(chroot-76960720:///~go-bot/juju-core/trunk/.bzr/branch/lock): Transport operation not possible: readonly transport | 02:31 |
| wallyworld | yeah, you need credentials | 02:32 |
| davecheney | jam said that my key shuld be there | 02:32 |
| davecheney | tey fook | 02:32 |
| wallyworld | no, the push needs the correct form of the url | 02:32 |
| wallyworld | bzr+ssh://.... or something i think | 02:32 |
| davecheney | lucky(~/devel/juju-core) % bzr push bzr+ssh://bazaar.launchpad.net/~go-bot/juju-core/trunk | 02:33 |
| davecheney | bzr: ERROR: Cannot lock LockDir(chroot-92566480:///~go-bot/juju-core/trunk/.bzr/branch/lock): Transport operation not possible: readonly transport | 02:33 |
| wallyworld | davecheney: bzr+ssh://go-bot@bazaar.launchpad.net/~go-bot/juju-core/trunk | 02:34 |
| wallyworld | i think | 02:34 |
| bigjools | lp:juju-core should work | 02:34 |
| wallyworld | bigjools: it's protected | 02:34 |
| bigjools | oh you need to use go-butt | 02:35 |
| davecheney | bigjools: is that like a ro-butt ? | 02:35 |
| bigjools | you could bzr lp-login first then | 02:35 |
| bigjools | then push to lp:juju-core | 02:36 |
| wallyworld | i didn't know you could do that | 02:36 |
| davecheney | lucky(~/devel/juju-core) % bzr lp-login | 02:36 |
| davecheney | dave-cheney | 02:36 |
| davecheney | lucky(~/devel/juju-core) % bzr push bzr+ssh://bazaar.launchpad.net/~go-bot/juju-core/trunk | 02:36 |
| davecheney | bzr: ERROR: Cannot lock LockDir(chroot-67593168:///~go-bot/juju-core/trunk/.bzr/branch/lock): Transport operation not possible: readonly transport | 02:36 |
| davecheney | lucky(~/devel/juju-core) % bzr push lp:juju-core | 02:36 |
| davecheney | bzr: ERROR: Cannot lock LockDir(chroot-92087248:///%2Bbranch/juju-core/.bzr/branch/lock): Transport operation not possible: readonly transport | 02:36 |
| davecheney | oh, do I have to lp-login as go-but ? | 02:36 |
| wallyworld | davecheney: did you try my suggestion above? | 02:36 |
| wallyworld | add the go-bot@ | 02:37 |
| davecheney | wallyworld: yes i tried your suggestion | 02:37 |
| davecheney | oh, not that one | 02:37 |
| davecheney | << tit | 02:37 |
| davecheney | lucky(~/devel/juju-core) % bzr push bzr+ssh://go-bot@bazaar.launchpad.net/~go-bot/juju-core/trunk | 02:38 |
| davecheney | Permission denied (publickey). | 02:38 |
| davecheney | very close now | 02:38 |
| davecheney | ConnectionReset reading response for 'BzrDir.open_2.1', retrying | 02:38 |
| davecheney | key isn't actually there | 02:38 |
| wallyworld | oh, your key is not registered? | 02:38 |
| wallyworld | no, it's not, i just checked | 02:39 |
| davecheney | wallyworld: i don't know how to answer that | 02:40 |
| davecheney | nothing did not work before | 02:40 |
| wallyworld | davecheney: see https://launchpad.net/~go-bot | 02:40 |
| wallyworld | i can't see your key | 02:40 |
| davecheney | ok, i'll fix it | 02:42 |
| davecheney | ok, how do I fix it ? | 02:42 |
| davecheney | ie, nothing didn't work before hand | 02:42 |
| davecheney | looks like I need to login as go-bot and rummage around | 02:43 |
| wallyworld | i don't think i have perms to add your key | 02:57 |
| wallyworld | maybe ask in -ops | 02:58 |
| thumper | davecheney: If I use go.build package, build.ImportPath(".", 0) and use that in a test, will it always import the directory I think it should? | 02:58 |
| davecheney | thumper: not 100% sure | 02:59 |
| davecheney | never tried that | 02:59 |
| davecheney | by rights it shuld use whatever the value of $(pwd) is | 02:59 |
| davecheney | but that can sometimes be surprising | 02:59 |
| thumper | hmm... | 03:03 |
| thumper | I want to write a test to make sure people don't mistakenly expand the dependencies of a package | 03:03 |
| thumper | so I want to get the list of imports for the current package | 03:04 |
| thumper | it seems to work | 03:04 |
| thumper | but the "." makes me hessitant | 03:04 |
| thumper | also annoying if I want to put the test in a helper package | 03:04 |
| thumper | in python there is __file__ | 03:04 |
| thumper | anything like that in go? | 03:04 |
| davecheney | what about this | 03:06 |
| davecheney | go list -f '{{ .Imports }}' $PKG | 03:07 |
| davecheney | thumper: you want build.ImportPath("launchpad.net/juju-core/cmd/juju", 0) or something | 03:08 |
| thumper | that doesn't work | 03:09 |
| thumper | &os.PathError{Op:"open", Path:"launchpad.net/juju-core/agent", Err:0x2} ("open launchpad.net/juju-core/agent: no such file or directory") | 03:09 |
| davecheney | ok | 03:10 |
| davecheney | you are right to be concerned about ".' | 03:10 |
| thumper | it seems to work, but I'm not convinced it is always going to work | 03:11 |
| davecheney | rightly so | 03:12 |
| thumper | hangon, | 03:13 |
| thumper | think I have it | 03:13 |
| thumper | davecheney: | 03:39 |
| thumper | func (*deps) TestPackageDependencies(c *gc.C) { | 03:39 |
| thumper | c.Assert( | 03:39 |
| thumper | FindJujuCoreImports(c, "launchpad.net/juju-core/agent"), | 03:39 |
| thumper | gc.DeepEquals, | 03:39 |
| thumper | []string{"log", "state", "version"}) | 03:39 |
| thumper | } | 03:39 |
| thumper | whaddaya think? | 03:39 |
| * thumper proposes | 03:43 | |
| bigjools | wallyworld: is that cost and constraint evaluation done in the core code or each provider? | 04:26 |
| bigjools | (as opposed to defining the structs I mean) | 04:27 |
| wallyworld | umm. core code i think, let me check | 04:27 |
| bigjools | that's cool if so | 04:27 |
| wallyworld | yeah, core code | 04:28 |
| wallyworld | there's glue in each provider to plug in the data | 04:28 |
| wallyworld | and the core code does the rest | 04:28 |
| wallyworld | so the common logic is all outside the providers | 04:28 |
| bigjools | nice | 04:29 |
| bigjools | so we just define the structs and Robert's your father's brother | 04:29 |
| wallyworld | that's the idea more or less | 04:30 |
| wallyworld | the openstack and ec2 implementations give some idea of what to do | 04:31 |
| bigjools | yeah | 04:37 |
| bigjools | wallyworld: oh, does cpupower have any meaning outside of ec2? | 04:37 |
| wallyworld | bigjools: nope | 04:38 |
| bigjools | wallyworld: what should it be set to, anything? | 04:39 |
| bigjools | as long as it's the same for each type I guess | 04:39 |
| wallyworld | no, don't set it. it will be ignored | 04:39 |
| bigjools | ah ok | 04:39 |
| bigjools | I used a ¢ symbol in a comment and now I get this "illegal UTF-8 encoding" compile error, what do I do? | 05:22 |
| === tasdomas_afk is now known as tasdomas | ||
| davecheney | bigjools: delete that character ? | 06:03 |
| davecheney | did your editor put a BOM at the top fo the file ? | 06:03 |
| bigjools | not that I can tell | 06:03 |
| davecheney | like this ? http://play.golang.org/p/MHE29ZM7Ln | 06:04 |
| bigjools | it was working ok until I did a go fmt | 06:04 |
| davecheney | oooh interesting | 06:04 |
| davecheney | is the \c all mangled as well ? | 06:04 |
| bigjools | it now looks like: ¢ | 06:05 |
| bigjools | if I remove the  Go bitches at me | 06:05 |
| davecheney | bigjools: can you provoke the problem in another editor ? | 06:07 |
| * bigjools tries again | 06:07 | |
| davecheney | or can you send me the file pre fmt ? | 06:07 |
| bigjools | I used vim to make a basic file | 06:09 |
| bigjools | it works | 06:09 |
| bigjools | post format it works | 06:09 |
| dimitern | ¢ looks like latin1 encoding interpreted as utf-8 | 06:10 |
| bigjools | except it added that  | 06:10 |
| bigjools | hmmm vim adds it later | 06:11 |
| bigjools | with no fmt | 06:11 |
| * bigjools sighs heavily | 06:11 | |
| jtv | dimitern: the other way around, no? | 06:14 |
| dimitern | jtv: ah, yes right | 06:14 |
| dimitern | if it was interpreted as utf there will be <?> things inside | 06:15 |
| dimitern | fwereade_: ping | 07:02 |
| dimitern | rogpeppe: hey | 07:04 |
| rogpeppe | dimitern: yo! | 07:04 |
| dimitern | rogpeppe: so about SetAgentAlive again | 07:05 |
| rogpeppe | dimitern: okay... | 07:05 |
| dimitern | rogpeppe: did we make a way to expose the same functionality through the api connection? | 07:06 |
| dimitern | rogpeppe: i think that was what we decided last | 07:06 |
| rogpeppe | dimitern: i can't entirely remember if we decided to make it explicit or implicit | 07:06 |
| rogpeppe | dimitern: both have advantages and disadvantages | 07:06 |
| dimitern | rogpeppe: it was something like auto start a pinger on connect | 07:06 |
| rogpeppe | dimitern: yeah, that's the implicit approach | 07:07 |
| rogpeppe | dimitern: how do we currently deal with the machine nonce? | 07:07 |
| dimitern | rogpeppe: we set it at provisioning time and can verify it in the agent | 07:08 |
| rogpeppe | dimitern: yeah, but do we deal with it in the API at all yet? | 07:08 |
| dimitern | rogpeppe: i don't think so, let me check | 07:08 |
| dimitern | rogpeppe: no | 07:09 |
| dimitern | rogpeppe: why do we need that? | 07:09 |
| dimitern | rogpeppe: i was thinking to do the api migration in steps | 07:10 |
| dimitern | rogpeppe: machiner doesn't need it, only the MA and provisioner | 07:10 |
| rogpeppe | dimitern: that's the thing that ensures we don't have two machine agents connected at the same time | 07:10 |
| dimitern | rogpeppe: how so? | 07:10 |
| rogpeppe | dimitern: isn't that the whole reason for the nonce? | 07:11 |
| rogpeppe | dimitern: my suggestion is that we integrate the nonce with the login request, and start the pinger then. | 07:12 |
| dimitern | rogpeppe: expand please | 07:12 |
| rogpeppe | dimitern: currently when you log in, you provide an entity tag and password, right? | 07:13 |
| dimitern | rogpeppe: right | 07:13 |
| rogpeppe | dimitern: so it's only at that stage that the API server knows who's at the other end | 07:14 |
| rogpeppe | dimitern: we need to know who's at the other end if we're to start a pinger | 07:14 |
| rogpeppe | dimitern: but we don't want to start a pinger if an agent comes up with the wrong nonce (because then they'll immediately go away again) | 07:15 |
| rogpeppe | dimitern: so we could add the nonce to the login request, so the login will fail without the correct nonce. | 07:15 |
| rogpeppe | dimitern: (for machine agents only) | 07:15 |
| TheMue | morning | 07:15 |
| rogpeppe | dimitern: and then start the pinger on a successful login request (and stop it when the connection goes away) | 07:16 |
| dimitern | rogpeppe: hmm | 07:16 |
| dimitern | rogpeppe: so add an optional field to apiinfo? | 07:16 |
| dimitern | rogpeppe: that's alright, but how can juju status get this to show it (i.e. AgentAlive) ? | 07:17 |
| rogpeppe | dimitern: we can still have an API AgentAlive call | 07:17 |
| dimitern | rogpeppe: but for the CLI/clients only | 07:18 |
| rogpeppe | dimitern: probably, yes | 07:18 |
| dimitern | rogpeppe: and assuming we can start the same pinger out-of-order to get the already running one, should be fine | 07:18 |
| rogpeppe | dimitern: i don't understand that | 07:19 |
| dimitern | rogpeppe: i mean the pinger we start on MA login will be running, and then AgentAlive tries to find whether it's running, but it will be in another process perhaps | 07:20 |
| rogpeppe | dimitern: that's always been the case | 07:20 |
| dimitern | rogpeppe: so it's fine then | 07:20 |
| rogpeppe | dimitern: yeah | 07:20 |
| rogpeppe | dimitern: you don't even need a Pinger to ask if something's alive | 07:21 |
| dimitern | rogpeppe: ok then, so the machiner can be simplified a bit | 07:22 |
| dimitern | rogpeppe: sorry, the MA actually | 07:23 |
| rogpeppe | dimitern: yeah | 07:23 |
| dimitern | rogpeppe: no need to call CheckProvisioned - if it's not we won't be able to connect | 07:23 |
| rogpeppe | dimitern: yup | 07:23 |
| dimitern | rogpeppe: but the error was to be a different one from "login failed", so it won't retry | 07:23 |
| rogpeppe | dimitern: good point, yes | 07:24 |
| dimitern | rogpeppe: I think I can see it now, the CL I mean to enable all that | 07:25 |
| rogpeppe | dimitern: great | 07:25 |
| dimitern | rogpeppe: if you don't mind I'll try it out and propose it for discussion | 07:25 |
| rogpeppe | dimitern: sounds good | 07:26 |
| dimitern | rogpeppe: as a first step towards api -> machiner | 07:26 |
| rogpeppe | dimitern: i'm not sure there's any problem with LongWait being 10 seconds BTW | 07:29 |
| rogpeppe | dimitern: can you think of one? | 07:29 |
| dimitern | rogpeppe: why so long? | 07:29 |
| dimitern | rogpeppe: why not 1h then? :) | 07:29 |
| rogpeppe | dimitern: because some it's important in some cases as a worst case; and it's not long to wait for a failed test. | 07:30 |
| rogpeppe | dimitern: it will never happen when tests pass, so it doesn't impact anything in the usual case | 07:30 |
| dimitern | rogpeppe: as long it's only a failure timeout then ok | 07:31 |
| rogpeppe | dimitern: that's the whole point of LongWait, isn't it? | 07:31 |
| dimitern | rogpeppe: i assumed so yes, but in some places it seemed less clear | 07:31 |
| rogpeppe | dimitern: anywhere it's part of the test-passing path, i'd like to know | 07:32 |
| dimitern | rogpeppe: it should be easy enough to spot - running go test before and after the branch changes a few times and comparing times | 07:33 |
| rogpeppe | dimitern: yeah, change it to 1 minute and see which tests take longer | 07:33 |
| dimitern | rogpeppe: have you tried that yourself? | 07:34 |
| rogpeppe | dimitern: no - i'll do it now | 07:34 |
| * TheMue hates import cycles | 07:40 | |
| rogpeppe | dimitern: useful test - thanks for the suggestion. it found two places we were using LongWait inappropriately | 07:51 |
| dimitern | rogpeppe: nice! | 07:52 |
| rogpeppe | dimitern: make that three places - there were two related tests in state that were doing it wrong | 07:57 |
| dimitern | rogpeppe: i'm glad i asked you to do this test then | 07:58 |
| rogpeppe | dimitern: me too | 07:58 |
| rvba | Hi guys, would someone be available for a second review of https://codereview.appspot.com/11524044/ ? | 08:02 |
| dimitern | rvba: I'll take a look, but it doesn't seem there is a first review there | 08:06 |
| rvba | dimitern: yeah, the first review is here https://codereview.appspot.com/11433044/. You know, I've got this problem that each time I run lbox propose it creates another MP. | 08:08 |
| rvba | dimitern: thanks! | 08:08 |
| dimitern | rvba: you could probably just do bzr push and publish your comments manually from rietveld | 08:09 |
| dimitern | rvba: the only drawback is you won't see the updated diff on rietveld i guess | 08:10 |
| rvba | dimitern: well, I think I'll live with the creation of an additional MP each time I run "lbox propose" :) | 08:10 |
| dimitern | rvba: if you rebuild lbox from tip using go 1.1.2 (or whatever the latest release is) I think you should be fine - that's what I did and it works. | 08:14 |
| rvba | dimitern: okay, I'll try that. | 08:14 |
| dimitern | rvba: but just in case it doesn't better keep a copy of the lbox binary around as backup | 08:15 |
| rvba | sure | 08:15 |
| dimitern | rvba: and also set a separate $GOPATH for go 1.1.2, so it'll be easier to switch back to your current go version | 08:15 |
| rvba | Well, because of the go-curl problem I'm sort of stuck with the old version for now. | 08:16 |
| dimitern | rvba: ah.. | 08:16 |
| dimitern | rvba: reviewed | 08:17 |
| rvba | ta | 08:17 |
| === racedo` is now known as racedo | ||
| mgz | mornin' | 09:11 |
| dimitern | mgz: moin | 09:11 |
| dimitern | rogpeppe: so far so good - changed OpenAPIAs and all relevent code to use machine nonce for MA logins, tests pass, only need to add the pinger and will propose | 09:13 |
| rogpeppe | dimitern: cool | 09:15 |
| rogpeppe | dimitern: final look? https://codereview.appspot.com/11529043/ | 09:21 |
| dimitern | rogpeppe: sure | 09:22 |
| * TheMue removed cyclic dependency, but our packages are sometime really tight entangled | 09:22 | |
| * fwereade_ out for breakfast, bbiab | 09:24 | |
| rogpeppe | TheMue: what was the issue in this case? | 09:28 |
| dimitern | rogpeppe: reviewed | 09:31 |
| TheMue | rogpeppe: I'm trying to extract the sync tools so that they are better reusable | 09:32 |
| TheMue | rogpeppe: and my wish has been environs/tools | 09:33 |
| TheMue | rogpeppe: sadly I'm using the EC2 HTTP storage reader, and environs/ec2 uses environs/tools. *rofl* | 09:34 |
| rogpeppe | dimitern: replied | 09:37 |
| rogpeppe | dimitern: (and i'm going to submit it) | 09:37 |
| rogpeppe | dimitern: did jam tell you anything about his plans around upgrade worker (in particular the role of his upgrade handler stuff)? | 09:40 |
| rogpeppe | fwereade_: ping | 09:45 |
| dimitern | rogpeppe: not much that I remember off hand | 09:55 |
| rogpeppe | dimitern: ok, i'll figure it out | 09:56 |
| dimitern | rogpeppe: it model is similar to notifyWorker | 09:56 |
| rogpeppe | dimitern: hmm, that seems a bit odd - isn't notifyWorker structured like that because it's generic? | 09:56 |
| rogpeppe | dimitern: i think the upgrader is the same in every case, isn't it? | 09:57 |
| rogpeppe | dimitern: i'm a bit concerned that we're taking time to refactor significant logic that really doesn't need refactoring at this stage | 09:57 |
| rogpeppe | dimitern: when i tried to estimate the time to implement the API, i did not include time to refactor all the client code too | 09:58 |
| dimitern | rogpeppe: i'm not sure, but i think the idea is to refactor the upgrader so it can use the notifyworker, like the machiner | 09:59 |
| rogpeppe | dimitern: that's a different thing. | 10:00 |
| fwereade_ | rogpeppe, pong | 10:01 |
| rogpeppe | fwereade_: i was just wondering why params.AgentTools flattens out all the version fields | 10:02 |
| fwereade_ | rogpeppe, I'd be +1 on changing that | 10:02 |
| fwereade_ | I asked jam, he said it basically seemed like a good idea at the time but didn't have strong feelings either way | 10:03 |
| fwereade_ | dimitern, rogpeppe: fwiw I am -1 on spending time making upgrader a handler-style worker | 10:03 |
| rogpeppe | fwereade_: it seems like an embedded version.BinaryVersion would be more obvious | 10:03 |
| fwereade_ | dimitern, rogpeppe: it's got 2 things to handle | 10:03 |
| rogpeppe | fwereade_: me too | 10:03 |
| rogpeppe | fwereade_: i'm not even sure it'll work well using NotifyWorker | 10:04 |
| fwereade_ | rogpeppe, ah, we're on go 1.1 now, so marshalling embedded fields works properly, right | 10:04 |
| fwereade_ | rogpeppe, hey wait | 10:04 |
| rogpeppe | fwereade_: no we're not | 10:04 |
| fwereade_ | rogpeppe, Upgrader... *is* that watching 2 things? oh yes it does the download stuff | 10:04 |
| fwereade_ | rogpeppe, we're not? | 10:04 |
| fwereade_ | rogpeppe, I thought we had everything building with go1.1 | 10:05 |
| rogpeppe | fwereade_: oh, have i missed something? | 10:05 |
| fwereade_ | rogpeppe, I *think* so | 10:05 |
| rogpeppe | fwereade_: is 1.1 backported to precise etc? | 10:05 |
| fwereade_ | rogpeppe, I suspect mgz will be able to give you the precise (heh) details | 10:05 |
| rogpeppe | fwereade_: i thought tarmac was still running 1.0.2 | 10:06 |
| rogpeppe | fwereade_: if we're really 1.1 throughout, then \o/ | 10:06 |
| fwereade_ | rogpeppe, honestly for the tools I'd just as soon send a string | 10:06 |
| rogpeppe | fwereade_: don't we need the version *and* the URL? | 10:06 |
| fwereade_ | rogpeppe, sure, sorry, I mean send the version as a string | 10:07 |
| rogpeppe | fwereade_: ah, you mean for AgentTools | 10:07 |
| rogpeppe | fwereade_: version.BinaryVersion marshals as a string anyway AFAIR | 10:07 |
| fwereade_ | rogpeppe, ah, that's nice | 10:07 |
| rogpeppe | fwereade_: ah, i misremembered - it only does that in mongo (it has SetBSON etc methods) | 10:09 |
| fwereade_ | dimitern, rogpeppe: the other option is to drop the un- or poorly-tested download-cancelling stuff, and then upgrader *would* make a nice NotifyWorker, I think | 10:09 |
| rogpeppe | fwereade_: it looks like versions aren't currently used in the API yet, so we can still do it for JSON | 10:09 |
| fwereade_ | rogpeppe, sgtm | 10:10 |
| rogpeppe | fwereade_: the important thing, for the time being, is that the upgrader waits around to complete an upgrade for a while after it's killed (because of the one-kills-all logic in the machine agent) | 10:11 |
| rogpeppe | fwereade_: when we fix things so that we can detect non-fatal errors, we can rethink that | 10:11 |
| fwereade_ | rogpeppe, I thought we weren't doing that on the api side? | 10:11 |
| rogpeppe | fwereade_: we are currently. we need to have a hard think about error handling before we change that. | 10:13 |
| fwereade_ | rogpeppe, I don't think it's worth worrying about preserving that, so long as we just defer landing a NotifyWorker version until it's going via the api? | 10:13 |
| fwereade_ | rogpeppe, isn't the only reason for the delayed stop to work around the one-error-kills-everything behaviour? | 10:13 |
| rogpeppe | fwereade_: not entirely | 10:14 |
| fwereade_ | rogpeppe, go on | 10:14 |
| rogpeppe | fwereade_: even if there is a fatal error due to the state going away, i think we want to allow the upgrade to proceed | 10:14 |
| rogpeppe | fwereade_: because we're not downloading the tools from the state | 10:15 |
| fwereade_ | rogpeppe, ok, but that just involves dropping the download-cancelling cleverness, right? if we just do a blocking download and return the error when it's done we're dorted | 10:16 |
| rogpeppe | fwereade_: i think we should probably leave existing logic as is. i know it's tempting to do drive-by refactoring of everything, but it all eats into the critical path | 10:16 |
| dimitern | fwereade_, rogpeppe: https://codereview.appspot.com/11424044 please take a look | 10:16 |
| rogpeppe | fwereade_: i'm dorted anyway, dunno about you :-) | 10:16 |
| fwereade_ | rogpeppe, I think that'd hold more water if upgrader were properly tested... as it is the tested bits needed changing, and if we dropped the untested bits we could also drop the somewhat astonishing delayed stop | 10:18 |
| fwereade_ | rogpeppe, which is at least tested but appears not to be delivering much value now that it's only there to support the untested bits | 10:18 |
| * fwereade_ gets "dorted" now, haha :) | 10:20 | |
| dimitern | fwereade_, rogpeppe: ^^ that CL implements the first step towards ditching SetAgentAlive | 10:21 |
| rogpeppe | fwereade_: the delayed stop isn't just there to support the download cancellation | 10:21 |
| rogpeppe | fwereade_: it's there to make the system more robust in the face of stupid errors. | 10:21 |
| fwereade_ | dimitern, cool, I probably won't get to it until later today I'm afraid | 10:21 |
| rogpeppe | fwereade_: (and the download cancellation doesn't work anyway AFAIR because we don't provide a means the abort the http download) | 10:22 |
| fwereade_ | rogpeppe, those errors being ones in parallel workers, right? we still depend on the outer agent mechanisms working right | 10:23 |
| dimitern | fwereade_: when you can, np, i have a couple more coming | 10:23 |
| rogpeppe | fwereade_: well yes, it has to get into the upgrader logic | 10:24 |
| fwereade_ | rogpeppe, so... if it starts an upgrader correctly (which we depend on anyway) it won't stop the upgrader unless there's an API problem, right? if there *is* an api problem, no upgrades; if not, we'll get the upgrade, and if we ignore the tomb once we've started downloading there's no further need for delay... is there? | 10:30 |
| fwereade_ | rogpeppe, the only thing that'll kill the upgrader is ErrTerminateAgent, and that's a potential stupid error the delayed stop doesn't defend against anyway | 10:31 |
| rogpeppe | fwereade_: first: i'm not sure we want to ignore the tomb when we're downloading - a download can take a long time. | 10:33 |
| rogpeppe | fwereade_: second: it will stop the upgrader if any API-based agent returns any error | 10:33 |
| fwereade_ | rogpeppe, first, var upgraderKillDelay = 5 * time.Minute | 10:34 |
| fwereade_ | rogpeppe, second: I thought the whole idea was that it *wouldn't* do that? | 10:34 |
| rogpeppe | fwereade_: yes - that happens only for the very first download | 10:34 |
| rogpeppe | fwereade_: in fact it doesn't happen if we come up successfully and the version is as expected | 10:35 |
| rogpeppe | fwereade_: AFAIR... | 10:35 |
| fwereade_ | rogpeppe, yeah, we do indeed usually not wait that long | 10:35 |
| fwereade_ | rogpeppe, but if it's acceptable for the upgrader to block everything for 5 mins while it's figuring stuff out, I think it's also ok for it to wait for the duration of a download | 10:36 |
| rogpeppe | fwereade_: the 5 minute wait only happens in a very specific circumstance | 10:37 |
| fwereade_ | rogpeppe, likewise the wait for a download | 10:38 |
| rogpeppe | fwereade_: it might actually be better structured as a statement outside the main upgrader loop actually | 10:38 |
| rogpeppe | fwereade_: in my experience, a network download can take lots more than 5 minutes to time out | 10:39 |
| fwereade_ | rogpeppe, let's try a different approach -- describe a bug that the delayed stop will actually defend against that *isn't* a bug in the mechanism that starts the upgrader anyway | 10:40 |
| rogpeppe | fwereade_: say there's a compatibility problem in the unit agent, for example - it encounters an "impossible" state and bombs out | 10:41 |
| rogpeppe | s/unit agent/uniter/ | 10:41 |
| fwereade_ | rogpeppe, then the uniter waits 5s and gets restarted | 10:41 |
| fwereade_ | rogpeppe, the upgrader continues to toddle along just fine | 10:42 |
| rogpeppe | fwereade_: except that as things are *currently*, the upgrader is killed too | 10:42 |
| fwereade_ | rogpeppe, we have been over that | 10:42 |
| rogpeppe | fwereade_: because we have no way of distinguishing temporary from permanent errors | 10:42 |
| rogpeppe | fwereade_: i'd like to fix the upgrader logic *after* we've added that error logic | 10:43 |
| fwereade_ | rogpeppe, the upgrader is killed too because isFatal always returns true; but when it's an api worker, it won't kill everything | 10:43 |
| fwereade_ | rogpeppe, we write a simple upgrader, and defer landing it until we can also land it as an api worker instead of a state worker | 10:43 |
| rogpeppe | fwereade_: we don't use isFatal for the APIWorker runner | 10:43 |
| rogpeppe | runner := worker.NewRunner(allFatal, moreImportant) | 10:43 |
| fwereade_ | rogpeppe, WTF | 10:44 |
| rogpeppe | fwereade_: it has to be like that | 10:44 |
| rogpeppe | fwereade_: because we can't currently know when an API connection is borked | 10:44 |
| rogpeppe | fwereade_: we do need to do that, but we haven't done it yet | 10:45 |
| fwereade_ | rogpeppe, so maybe it's not hooked up yet, because we seem tohave some pathology where we still somehow write reams of code and never integrate it | 10:45 |
| rogpeppe | fwereade_: what's not hooked up yet? | 10:46 |
| fwereade_ | rogpeppe, but "does Ping return an error is I think adequate" | 10:46 |
| rogpeppe | fwereade_: i don't believe it is | 10:46 |
| rogpeppe | fwereade_: what happens if the underlying state connection that the API server is talking to goes away? | 10:46 |
| fwereade_ | rogpeppe, then handling that is the api server's job | 10:47 |
| rogpeppe | fwereade_: yes, and we don't know how to do that yet | 10:47 |
| fwereade_ | rogpeppe, it is *not* the agent's concern though | 10:47 |
| rogpeppe | fwereade_: it is currently - that's the only way we can recover from.... oh, in fact we never do recover. we're stuffed. | 10:48 |
| fwereade_ | rogpeppe, the answer to possible crapness in one component is *not* to smear workarounds across the rest of the system, it's to fix the problematic component | 10:48 |
| rogpeppe | fwereade_: i agree. but i don't want to remove our current workaround until we've fixed that problematic component | 10:49 |
| fwereade_ | rogpeppe, using allFatal just makes the agent crap as well | 10:50 |
| rogpeppe | fwereade_: so... it seems to me that there are two fixes we really need | 10:52 |
| rogpeppe | fwereade_: 1) add another worker that watches api.State.Broken and exits with a fatal error if it is. | 10:53 |
| rogpeppe | fwereade_: 2) change the api server so that if it sees an error that implies the mongo state connection is broken, it quits | 10:53 |
| fwereade_ | rogpeppe, yeah, both good things | 10:54 |
| fwereade_ | rogpeppe, more important than implementing the API workers how I asked more than a month ago? very much not so | 10:54 |
| rogpeppe | fwereade_: i wasn't planning on rewriting all the workers to change them to use the API, and i don't think it's necessary | 10:55 |
| rogpeppe | fwereade_: that's what i'm resisting | 10:56 |
| * TheMue smiles | 10:56 | |
| TheMue | tests are passing | 10:56 |
| fwereade_ | rogpeppe, we *have* to rewrite the upgrader, because you implemented it so that we have to send secrets out to every single agent | 10:56 |
| fwereade_ | rogpeppe, since we're doing it I would prefer to do it right | 10:57 |
| rogpeppe | fwereade_: we don't have to change anything other than change it to read on a notify channel and read the tools when we get a notification | 10:57 |
| rogpeppe | fwereade_: that's a fairly trivial change, i think | 10:57 |
| rogpeppe | fwereade_: definitely not a rewrite | 10:57 |
| fwereade_ | rogpeppe, apart from all the magic untested complex code in there | 10:58 |
| rogpeppe | fwereade_: that's a totally orthogonal issue | 10:58 |
| rogpeppe | fwereade_: it may be the case, but we don't need to fix everything now | 10:58 |
| fwereade_ | rogpeppe, you know we could have written a NotifyWatcher implementation in the time you've been kicking up dust over this issue? | 10:59 |
| rogpeppe | fwereade_: but it would probably be wrong in some subtle way. i like not rewriting things when possible. | 11:00 |
| fwereade_ | rogpeppe, you are free to think I'm wrong about things | 11:02 |
| fwereade_ | rogpeppe, you are not free to persistently go off and implement WTF you want to in the face of my instructions | 11:02 |
| rogpeppe | fwereade_: no, i think you have good points, | 11:03 |
| rogpeppe | fwereade_: and i think we want to go in the same direction | 11:03 |
| rogpeppe | fwereade_: to get things straight here: your instructions here are to rewrite the upgrader? | 11:06 |
| fwereade_ | rogpeppe, to (1) do the API runner as originally requested, so rogue tasks don't take the others down; to (2) implement a simpler upgrader as a NotifyWorker against the API, that does a blocking download and returns the appropriate upgrade error when done; and to (3) unhook the state one and activate the machine one, for the machine agent only | 11:09 |
| fwereade_ | s/the machine one/the api one/ | 11:10 |
| rogpeppe | fwereade_: ok, thanks | 11:10 |
| dimitern | rogpeppe: ping | 11:19 |
| rogpeppe | dimitern: pong | 11:20 |
| dimitern | rogpeppe: how about that review? | 11:20 |
| dimitern | ;) | 11:20 |
| rogpeppe | dimitern: reviewed | 11:20 |
| dimitern | rogpeppe: thanks | 11:20 |
| jtv | Would any of the Go experts here know how I'd go about debugging this crash? I went through it with gdb but it seems to be happening somewhere pretty deep down in the malloc logic... http://paste.ubuntu.com/5890497/ | 11:30 |
| jtv | I don't even know why it'd list two goroutines, since as far as I know this code doesn't even use goroutines. | 11:31 |
| dimitern | fwereade_: standup? | 11:34 |
| jtv | Phew. Go 1.0.2 will produce a proper traceback. The 1.1.1 from the PPA we're using is a dev version, not a released one, right? | 11:40 |
| rvba | Anyone up for two really tiny reviews? https://codereview.appspot.com/11404044/ / https://codereview.appspot.com/11511043/ | 12:50 |
| rogpeppe | rvba: will look after lunch | 12:59 |
| * rogpeppe goes for lunch | 12:59 | |
| rvba | Thanks. | 12:59 |
| * TheMue steps out and continues later | 13:10 | |
| hazmat | rogpeppe, is there some sort of cache around the api? | 13:28 |
| hazmat | rogpeppe, i can do actions, like deploy/add_units/add_relation, then do an all watcher immediately after and not get the results back | 13:29 |
| hazmat | ie. the api is behaving as though its eventual consistent. | 13:29 |
| hazmat | hmm.. nevermind maybe user error | 13:32 |
| dimitern | looking for a second review on https://codereview.appspot.com/11424044/ and reviews on https://codereview.appspot.com/11572043/ | 13:52 |
| dimitern | fwereade_: when you can as well ^^ | 13:54 |
| === tasdomas is now known as tasdomas_afk | ||
| === tasdomas_afk is now known as tasdomas | ||
| rogpeppe | hazmat: the events coming from the allwatcher are only delivered when the global polling goroutine gets around to checking for changes | 13:58 |
| rogpeppe | hazmat: the poll interval is 5s | 13:59 |
| rogpeppe | hazmat: so that's your worst case delay | 13:59 |
| rogpeppe | hazmat: although... | 13:59 |
| rogpeppe | hazmat: the first result should come immediately | 13:59 |
| hazmat | rogpeppe, immediately, but not nesc. reflective of current reality. | 14:01 |
| fwereade_ | anybody else unable to connect to irc.canonical.com? | 14:01 |
| rogpeppe | hazmat: no, it should be reflective of current reality | 14:01 |
| hazmat | rogpeppe, because the status api call wasn't fully implemented i did status emulation on the watcher | 14:01 |
| hazmat | using the initial watch response value | 14:02 |
| hazmat | but that doesn't always seem to reflect the last ops done | 14:02 |
| hazmat | running into this against some unit tests | 14:02 |
| === tasdomas is now known as tasdomas_afk | ||
| hazmat | rogpeppe, here's an example.. http://pastebin.ubuntu.com/ | 14:05 |
| hazmat | whoops | 14:05 |
| hazmat | http://pastebin.ubuntu.com/5890883/ | 14:05 |
| hazmat | i deploy two services, add a relation, add a unit.. then do a status immediately after, it reflects only the new machines, a watcher initial result doesn't even show the machines. | 14:05 |
| hazmat | ideally these should both show the services, units, relations, and machines of previous api ops | 14:06 |
| hazmat | for this simple sync usage on the client, watches here use a separate client connection (env.status is a watch translator due to lack of details in rpc status) | 14:07 |
| rogpeppe1 | hazmat: what does wait_for_units do? | 14:11 |
| hazmat | rogpeppe, in this case nothing, because its waiting for units to be in the 'started' state, but it can't see any units, so its basically a no-op here. | 14:12 |
| rogpeppe1 | hazmat: does it create a new watcher or use an existing one? | 14:12 |
| hazmat | rogpeppe, normally its a watch against units agent-state to reach started | 14:12 |
| hazmat | rogpeppe, new watcher | 14:12 |
| rogpeppe1 | hazmat: it seems odd that the first event isn't producing the current status, because it gets the contents of all collections before producing it | 14:14 |
| rogpeppe1 | hazmat: could you show me the whole code? (or just a trace of the API messages would be even better) | 14:14 |
| hazmat | rogpeppe1, hence the question is there some sort of cache behavior? | 14:14 |
| hazmat | rogpeppe1, okay.. | 14:14 |
| rogpeppe1 | hazmat: ah... | 14:14 |
| rogpeppe1 | hazmat: i've just realised | 14:15 |
| rogpeppe1 | hazmat: yes, of course, there *is* some sort of cache behaviour! | 14:15 |
| rogpeppe1 | hazmat: because the allwatcher is shared between all clients | 14:15 |
| rogpeppe1 | hazmat: and we keep a single copy of all the data | 14:15 |
| hazmat | rogpeppe1, i thought each client had independent pointer into that data | 14:15 |
| rogpeppe1 | hazmat: yes, each client does | 14:16 |
| rogpeppe1 | hazmat: but the underlying data is a cache that's only updated when new events arrive (by that 5s poll interval) | 14:16 |
| rogpeppe1 | hazmat: sorry for the mis-info! | 14:16 |
| hazmat | rogpeppe1, okay.. so i think the critical fix here then for would be for status op to report status instead of just machines. | 14:16 |
| rogpeppe1 | hazmat: i think the status op should provide the whole status, yes | 14:17 |
| hazmat | otherwise, there's no way a client can see its mods in th eenv. | 14:17 |
| rogpeppe1 | hazmat: and then we can make the status command use that | 14:17 |
| rogpeppe1 | hazmat: mgz is working on some stuff to enable that | 14:17 |
| hazmat | mgz, ping :-) | 14:18 |
| mgz | hazmat: hey | 14:18 |
| hazmat | mgz, rogpeppe1 mentioned you might be working on cli using api? | 14:20 |
| rogpeppe1 | hazmat: no, that's not quite it | 14:20 |
| mgz | well, mostly as a side effect | 14:20 |
| hazmat | specifically i was wondering about status support, since the client api is hard to use/verify ops without status working given 5s delay on watcher. | 14:20 |
| rogpeppe1 | hazmat: the work mgz is doing makes it possible to get all the status info over the api, but not the status call itself | 14:20 |
| mgz | hazmat: if I read the log correctly, your issue is that just using watcher apis you don't get all the status info you could with manual `juju status` calls? | 14:21 |
| rogpeppe1 | hazmat: you can get info on individual machines i think | 14:21 |
| hazmat | rogpeppe1, that's not useful when i'm verifying services, units, relations.. | 14:21 |
| rogpeppe1 | hazmat: one possibility would be to add a Sync call to the API | 14:22 |
| hazmat | rogpeppe1, that would help.. but i'm not sure its something we want in the api | 14:22 |
| rogpeppe1 | hazmat: to sync any watchers | 14:22 |
| rogpeppe1 | hazmat: but yeah, i think you may be right | 14:23 |
| hazmat | rogpeppe1, the watchers are already sync'd, its the a dos api against updating the stream. | 14:23 |
| rogpeppe1 | hazmat: i don't know how much dos could be done this way that couldn't be done through other api calls. | 14:24 |
| hazmat | which to combat would just introduce the delay elsewhere... what this usage needs is non-cached access. hence status | 14:24 |
| rogpeppe1 | hazmat: yeah | 14:24 |
| hazmat | rogpeppe1, well we have fetch service info, no fetch unit info, or machine afaik | 14:24 |
| rogpeppe1 | hazmat: ironically my original plan was to do the status API call as the first actual use of the API | 14:25 |
| dimitern | rogpeppe1: https://codereview.appspot.com/11572043/ ? | 14:26 |
| rogpeppe1 | dimitern: i don't think NewAgentAPI is the right place to start the pinger | 14:29 |
| hazmat | mgz, no.. the watch apis do give all the status, but their cached, so its eventual consistent behavior, which is causing issues with unit tests, the issue would be solved if the rpc status call actually returned status info, instead of just a partial machine info | 14:29 |
| dimitern | rogpeppe1: why? | 14:29 |
| rogpeppe1 | dimitern: because it's called once for each machiner API call | 14:29 |
| rogpeppe1 | dimitern: so you're quickly going to have many many pingers :-) | 14:29 |
| dimitern | rogpeppe1: isn't that the singleton root? | 14:30 |
| dimitern | rogpeppe1: for the machiner | 14:30 |
| rogpeppe1 | dimitern: nope | 14:30 |
| dimitern | rogpeppe1: aaah | 14:30 |
| dimitern | rogpeppe1: got you! | 14:30 |
| dimitern | rogpeppe1: it's the entry point, but for every call | 14:30 |
| rogpeppe1 | dimitern: yes | 14:30 |
| dimitern | rogpeppe1: so then after the CheckProvisioned in admin then | 14:31 |
| rogpeppe1 | dimitern: in srvAdmin.Login, you mean? | 14:31 |
| dimitern | rogpeppe1: yeah? | 14:32 |
| rogpeppe1 | dimitern: yes, i think that's the right place | 14:32 |
| dimitern | rogpeppe1: ok, will move it there | 14:32 |
| mgz | hazmat: I see. seems fixable, but not really what I'm poking right now. | 14:32 |
| rogpeppe1 | dimitern: or perhaps in apiRootForEntity | 14:33 |
| dimitern | rogpeppe1: I've go another CL for you as well :) https://codereview.appspot.com/11574044/ | 14:33 |
| dimitern | rogpeppe1: in apiRootForEntity with the idea that once it becomes a "root factory" it will be more appropriate? | 14:34 |
| rogpeppe1 | dimitern: yeah | 14:34 |
| dimitern | rogpeppe1: good point | 14:34 |
| rogpeppe1 | dimitern: i've still got that CL around somewhere | 14:34 |
| dimitern | rogpeppe1: it got rejected i think | 14:35 |
| rogpeppe1 | dimitern: i don't think it was reviewed actually | 14:35 |
| dimitern | rogpeppe1: ah, it might be | 14:35 |
| rogpeppe1 | dimitern: yeah, no reviews: https://codereview.appspot.com/10684044 | 14:36 |
| dimitern | rogpeppe1: I think fwereade mentioned something he didn't like there, but can't remember details | 14:36 |
| rogpeppe1 | dimitern: oh, i'd like to know if there was | 14:37 |
| rogpeppe1 | dimitern: this was the crux of it. your NewPinger code would go into the NewMachineRoot function. | 14:39 |
| rogpeppe1 | https://codereview.appspot.com/10684044/diff/1015/state/apiserver/facades/machine.go | 14:39 |
| dimitern | rogpeppe1: yeah, well we can still do it next week | 14:39 |
| dimitern | rogpeppe1: haven't look into it in detail myself | 14:40 |
| rogpeppe1 | dimitern: reviewed | 14:45 |
| mattyw | I just did an lbox propose but it didn't make a Rietveld code review for me? | 15:05 |
| rvba | Anyone up for a review? https://codereview.appspot.com/11404044/ | 15:07 |
| fwereade | hey guys | 15:07 |
| fwereade | I'm about to go through the reviews from the top | 15:07 |
| dimitern | rogpeppe1: thanks | 15:08 |
| fwereade | rogpeppe1, I thought https://codereview.appspot.com/10684044 looked cool but hit the nice-to-have buttons not the must-have buttons, so I didn't want anyone to take it over | 15:08 |
| rogpeppe1 | fwereade: yeah, i appreciate that | 15:08 |
| fwereade | rogpeppe1, but since it's here, and you're here to respond, I'm on it now (it's at the top after all :)) | 15:09 |
| rogpeppe1 | fwereade: that's appreciated. it wasn't complete though - in particular i hadn't added any more tests | 15:09 |
| fwereade | rogpeppe1, ok, give me 5 mins to reload my state on it just in case there was something I didn't like, but I think I'm happy for you to continue with it, just make it a side task not the main one please ;) | 15:11 |
| rogpeppe1 | fwereade: ta | 15:11 |
| mattyw | fwereade, I just submitted a small proposoal but lbox propose didn't create a Rietveld code review for me? | 15:11 |
| rogpeppe1 | fwereade: i haven't touched it since i got back BTW | 15:11 |
| fwereade | mattyw, huh, weird | 15:11 |
| mattyw | fwereade, I take it we're still using Rietveld | 15:11 |
| rogpeppe1 | fwereade: a proposal against juju-core? | 15:11 |
| fwereade | mattyw, I use +activereviews as my task list anyway so rest assured I'll see it | 15:11 |
| rogpeppe1 | s/fwereade/mattyw/ | 15:12 |
| mattyw | fwereade, ok, shall I try to make a Rietveld review for it or just ignore it? | 15:12 |
| mattyw | rogpeppe1, yeah juju-core | 15:12 |
| rogpeppe1 | mattyw: hmm, weird. can you run lbox propose --debug and paste the result? | 15:12 |
| rogpeppe1 | mattyw: fwiw, rvba has been having some perhaps similar issues with lbox | 15:13 |
| mattyw | rogpeppe1, hmm, running debug tells me it can't login - I guess that's the problem ;) | 15:15 |
| fwereade | rogpeppe1, yeah, direction looks sane, thanks :) | 15:15 |
| rogpeppe1 | mattyw: try removing ~/.lpad_oauth* and trying again | 15:16 |
| fwereade | oh, incidentally, did everyone see http://foaas.herokuapp.com/ | 15:16 |
| rogpeppe1 | fwereade: no | 15:16 |
| fwereade | rogpeppe1, it made me smile | 15:16 |
| rogpeppe1 | fwereade: me too | 15:16 |
| === rogpeppe1 is now known as rogpeppe | ||
| dimitern | LOL | 15:17 |
| * fwereade didn't really have a proper lunch and is off to have one now, reviews will have to wait a bit | 15:18 | |
| mattyw | rogpeppe, got it working without having to resort to removing .lpad_oauth | 15:18 |
| * fwereade will probably just have a sandwich at his desk though, so don;t fret ;p | 15:18 | |
| fwereade | jtv, ping | 15:40 |
| jtv | Hi fwereade | 15:40 |
| fwereade | jtv, heyhey, it looks like you have 2 LGTMs on "Drop startInstanceParams" | 15:40 |
| jtv | Ah great, thanks | 15:41 |
| fwereade | jtv, unless you particularly want to, please don't feel you need to wait for my review | 15:41 |
| jtv | Thans! | 15:41 |
| jtv | *Thanks | 15:42 |
| fwereade | jtv, thank *you*, I'm really appreciating what you're doing | 15:42 |
| dimitern | rogpeppe: all the other client-side facades for workers use the worker name as the facade type name | 15:42 |
| dimitern | rogpeppe: and i think it's reasonable as well, for agent facades (currently MA only) State seems more appropriate | 15:42 |
| jtv | hi-ho, hi-ho, to tarmac-land we go | 15:43 |
| rogpeppe | dimitern: i think they should all use State | 15:43 |
| dimitern | rogpeppe: i don't agree | 15:43 |
| rogpeppe | dimitern: that was my original plan, agreed with fwereade AFAIR | 15:43 |
| dimitern | rogpeppe: why? | 15:43 |
| dimitern | fwereade: ? | 15:43 |
| rogpeppe | dimitern: because the Machiner state API facade is not itself a Machiner | 15:43 |
| rogpeppe | dimitern: it is actually the state | 15:43 |
| rogpeppe | dimitern: with a facade in front of it | 15:44 |
| dimitern | rogpeppe: it's not state either | 15:44 |
| rogpeppe | dimitern: so calling it State in all those places seems like a Good Thing to me | 15:44 |
| dimitern | rogpeppe: it's api | 15:44 |
| fwereade | rogpeppe, dimitern: I don't *love* state but IMO it's at least consistent with existing usage and as a name for the source of truth | 15:44 |
| rogpeppe | dimitern: the API is just a front-end for the state | 15:44 |
| dimitern | rogpeppe: so MachinerAPI then | 15:45 |
| rogpeppe | dimitern: and naming it State means we don't need to rename all the variables in every piece of client code | 15:45 |
| dimitern | rogpeppe: better than State | 15:45 |
| dimitern | rogpeppe: that's not a big deal imo | 15:45 |
| dimitern | rogpeppe: we don't have to rename them either way | 15:45 |
| rogpeppe | dimitern: it means that it's potentially easy to move code from one worker to another | 15:45 |
| rogpeppe | dimitern: or factor it out | 15:46 |
| fwereade | rogpeppe, dimitern: I'm ok sticking with state to reduce churn, personally, but I don't have a strong opinion either way | 15:46 |
| dimitern | rogpeppe: i don't see how this should be something to consider | 15:46 |
| dimitern | rogpeppe: facades are specifically done for a worker/agent | 15:46 |
| rogpeppe | dimitern: sure. but from the point of view of that agent, it's talking to the state | 15:46 |
| rogpeppe | dimitern: so having it called, say machiner.State, seems like it works well to me | 15:47 |
| dimitern | rogpeppe: i think it's misleading | 15:47 |
| rogpeppe | dimitern: it means that when you read the code, it's fairly obvious that all these pieces of code are talking to the same underlying thing | 15:47 |
| dimitern | rogpeppe: maybe not Machiner then, MachinerAPI is actually closer to the intent | 15:47 |
| dimitern | rogpeppe: when it has API in its name it's pretty obvious | 15:48 |
| rogpeppe | dimitern: i don't see a significant benefit from machiner.API (over machiner.State) (and i'm not keen on machiner.MachinerAPI | 15:48 |
| rogpeppe | ) | 15:48 |
| rogpeppe | dimitern: as fwereade says, it reduces churn, and that seems like a good thing | 15:49 |
| rogpeppe | dimitern: i'd like to be able to change all the client code with minimal fuss | 15:49 |
| dimitern | rogpeppe: who's going to change all the other facades then? | 15:49 |
| dimitern | rogpeppe: which are already using these names | 15:50 |
| rogpeppe | dimitern: i'm happy to do it. i've already done the upgrader one. | 15:50 |
| dimitern | rogpeppe: ok then | 15:50 |
| rogpeppe | dimitern: given that nothing is using them yet, it's not much of an issue | 15:50 |
| dimitern | rogpeppe: i *still* don't agree | 15:50 |
| dimitern | rogpeppe: but will do it | 15:50 |
| dimitern | rogpeppe: 2:1 against :) | 15:50 |
| fwereade | rogpeppe, did you assign yourself to the time.Format bug? | 15:50 |
| rogpeppe | dimitern: thanks a lot. i do think it makes sense though, if only from a grandfathered-in p.o.v. | 15:51 |
| rogpeppe | fwereade: no | 15:51 |
| rogpeppe | fwereade: but i have merged a fix | 15:51 |
| rogpeppe | fwereade: so should mark the bug fix committed | 15:51 |
| rogpeppe | fwereade: have you got the link to hand? | 15:51 |
| fwereade | rogpeppe, not offhand -- it's just that sidnei also fixed it but a bit late | 15:52 |
| rogpeppe | fwereade: i think mark assigned the bug to me actually | 15:52 |
| hazmat | mgz, gotcha.. could you clarify what your working on?.. from the kanban its not relatable at all? goamz for extra ip.. (which is vpc support incidentally) and lxc container addressability. | 15:53 |
| fwereade | rogpeppe, oh, double-bad luck, sidnei found and fixed it completely independently | 15:53 |
| hazmat | not sure how either of relates to the api bits | 15:53 |
| hazmat | i did a branch on that one too (time.format), but couldn't ever the get the full test suite to pass.. | 15:54 |
| mgz | hazmat: specifically I need a mechanism for keeping machine address details updated in state | 15:54 |
| fwereade | rogpeppe, I've marked it fix committed | 15:54 |
| rogpeppe | fwereade: thanks | 15:54 |
| mgz | which seems easiest to do by monitoring machines in the process of coming up and polling over the cloud's api for changes | 15:55 |
| dimitern | rogpeppe: i've updated this as well https://codereview.appspot.com/11572043/ | 15:56 |
| rogpeppe | dimitern: looking | 15:56 |
| mgz | then everything else can just watch state for address stuff, rather than having to go run something provider specific at the unit level | 15:56 |
| rogpeppe | dimitern: reviewed | 16:00 |
| dimitern | rogpeppe: thanks\ | 16:00 |
| === flaviamissi is now known as flaviamissi_ | ||
| fwereade | rvba, a question about https://codereview.appspot.com/11578043/ | 16:06 |
| fwereade | mattyw, I think that one's a trivial, go ahead and merge it | 16:08 |
| fwereade | mattyw, tyvm | 16:08 |
| mattyw | fwereade, trivial is my middle name | 16:09 |
| fwereade | mattyw, haha | 16:09 |
| mattyw | fwereade, I don't think I can merge it, I only have the option to say it's already merged in lp | 16:09 |
| fwereade | mattyw, ok, I've approved it for you | 16:10 |
| fwereade | mattyw, keep half an eye on it just in case the bot doesn't like something | 16:10 |
| mattyw | fwereade, thanks :) | 16:10 |
| jtv | Oh no. | 16:19 |
| jtv | Our branches are spending too long in review. Getting really hard conflicts popping up in the meantime. | 16:20 |
| fwereade | dimitern, https://codereview.appspot.com/11424044/ LGTM, but ponder my comments before you merge | 16:25 |
| dimitern | fwereade: thanks; looking | 16:29 |
| rogpeppe | fwereade, dimitern: https://codereview.appspot.com/11586043 | 16:43 |
| dimitern | fwereade: let's rename it to Nonce when we have UA connecting the same way? | 16:43 |
| rogpeppe | dimitern: will we need a nonce for the unit agent? | 16:43 |
| dimitern | rogpeppe: fwereade mentioned that | 16:43 |
| dimitern | rogpeppe: looking | 16:44 |
| rogpeppe | fwereade: +1 to omitempty - i nearly mentioned that | 16:44 |
| dimitern | rogpeppe: will do | 16:45 |
| fwereade | dimitern, let's rename it *now* if we're going to | 16:48 |
| fwereade | dimitern, it may be that we never will | 16:48 |
| fwereade | dimitern, but I suspect that one day units will be able to move from one machine to another | 16:49 |
| dimitern | fwereade: will we? | 16:49 |
| dimitern | fwereade: I prefer not to complicate more that CL renaming all MachineNonce instances to Nonce | 16:49 |
| fwereade | dimitern, I remember thinking they might need it but I can't remember why | 16:50 |
| dimitern | fwereade: but can do a follow up | 16:50 |
| fwereade | dimitern, ok, so long as we don't release with MachineNonce in the api, or we'll never get away from it ;p | 16:50 |
| dimitern | fwereade: why? | 16:50 |
| dimitern | fwereade: it's just an argument | 16:50 |
| dimitern | a field even | 16:50 |
| fwereade | dimitern, that some clients will use one version of, and other clients will use another version of | 16:52 |
| dimitern | fwereade: ok, will do a follow-up after the third branch has landed | 17:00 |
| fwereade | dimitern, cool | 17:01 |
| dimitern | fwereade: and rename *all* MachineNonce to Nonce (even at top-level in agent.Conf) | 17:01 |
| dimitern | fwereade: or you meant only the one in API Info actually? | 17:01 |
| rogpeppe | dimitern, fwereade: fairly trivial: https://codereview.appspot.com/11589043/ | 17:01 |
| fwereade | dimitern, I really just meant the one in api info | 17:02 |
| fwereade | dimitern, agent.conf is alredy compatibility-infected, best not touch it unless we have to | 17:02 |
| dimitern | fwereade: ok | 17:03 |
| dimitern | fwereade: if it's only that one, then I guess it's no bigger change to do it in the current CL | 17:03 |
| fwereade | dimitern, that'd be great | 17:03 |
| fwereade | dimitern, tyvm | 17:03 |
| dimitern | rogpeppe: how will that make the api nicer? | 17:03 |
| fwereade | rogpeppe, LGTM | 17:04 |
| TheMue | rogpeppe: first review ;) | 17:04 |
| fwereade | dimitern, check out params.AgentTools | 17:04 |
| dimitern | rogpeppe: i really don't get that CL | 17:04 |
| dimitern | sorry | 17:04 |
| rogpeppe | dimitern: a version will be encoded as a nice string rather than several fields. params.AgentTools can also just use version.Binary rather than explicitly redeclaring all the version fields. | 17:05 |
| fwereade | dimitern, it lets us just have a version.Binary in a params struct, and get it sent nicely over the wire, rather than as like 7 distinct fields | 17:05 |
| rogpeppe | fwereade: thanks | 17:05 |
| dimitern | rogpeppe, fwereade: ah! I see, thanks | 17:06 |
| fwereade | dimitern, https://codereview.appspot.com/11572043/ also LGTM with one tweak | 17:08 |
| fwereade | dimitern, and https://codereview.appspot.com/11574044/ LGTM too | 17:10 |
| dimitern | fwereade: tyvm | 17:11 |
| fwereade | right, that's 6pm: I've survived the week and I'm going to lie down in a park for a bit | 17:11 |
| fwereade | I'm on holiday mon-weds but will probably be around mon am a bit | 17:11 |
| rogpeppe | fwereade: enjoy | 17:12 |
| fwereade | so I can talk to whoever's here about where we're going with environment config | 17:12 |
| rogpeppe | fwereade: i'm also off now | 17:12 |
| fwereade | happy weekends all! | 17:12 |
| rogpeppe | fwereade: i've done the upgrader but not tested it... | 17:12 |
| fwereade | rogpeppe, sweet, tyvm | 17:12 |
| rogpeppe | g'night all | 17:12 |
| dimitern | fwereade: why the need to test is the pinger a resource? | 17:14 |
| TheMue | fwereade: enjoy your holiday | 17:14 |
| dimitern | fwereade: I can add a test that after disconnecting it's no longer alive - will that be enought? | 17:14 |
| dimitern | fwereade: ah, sorry didn't see you're previous msgs | 17:14 |
| TheMue | so, time for me to step out too | 17:26 |
| TheMue | enjoy your weekend, dimitern | 17:26 |
| dimitern | TheMue: thanks, you too! | 17:26 |
| TheMue | dimitern: will do so, tomorrow invented to a breakfast and later to a bbq ;) | 17:27 |
| TheMue | invited | 17:27 |
| benji | gary_poster: I'll take one | 17:54 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!