[00:12] oh ffs [00:13] lbox choked on this: https://code.launchpad.net/~thumper/juju-core/consolidate-bootstrap-state/+merge/172701 [00:13] bigjools: you like go, care to review? [00:14] thumper: I LOVE go [00:14] thumper: but I might have to pass, I'm not really working today [00:14] bigjools: oh? [00:15] :( [00:15] neither is wallyworld [00:15] it's easier to ask for forgiveness than to block ;) [00:28] bigjools: ha, not blocked [00:44] davecheney: what does it mean for a struct to embed an interface? [00:45] thumper: type S struct { io.Reader } [00:45] ^ [00:45] yeah, or similar [00:45] in this particular instance it is environs.Environ [00:46] it's a shorthand for having to type out all the methods in the io.Reader interface [00:46] and it only defines some methods [00:46] what about the methods it doesn't define? [00:46] you don't normally type out methods inside a struct [00:46] I know about embedding an interface in an interface [00:46] that is correct [00:46] but this is different [00:46] yeah, interface inside interface really just is short hand [00:47] so, by embedding an io.Reader inside S this method is automatically crated [00:47] type (s *S) Read(...) { return s.Reader.Read(...) } [00:47] there actually is not forwarding method [00:48] but the world behaves as it is is [00:49] ... [00:49] oh [00:49] so we can implement an interface [00:49] without implementing all the interface [00:49] I get it now [00:50] what happens when an unimplemented method is called? [00:50] well ... yues [00:50] S.Reader is nil [00:50] so, boom? [00:50] so S.Read will blow up unless something has been assigned to S.Reader [00:55] isn't this dangerous? [00:56] i'm not really sure what you're trying to do [00:56] thumper [00:57] davecheney: it isn't me, it is me understanding our existing code [00:57] ok [00:57] we have a test that is testing bootstrap and has a fake environment [00:57] it only implements the functions it cares about [00:57] and passes the test structure through as an environs.Environ [00:58] seems to work [00:58] yup === fdehay_ is now known as fdehay [01:22] davecheney: any luck working out go-curl with Go1? [01:38] bigjools: we need at least version 1.0.2 to make it work [01:38] * davecheney finds issue [01:39] bigjools: https://bugs.launchpad.net/gwacl/+bug/1196811 [01:39] k [01:48] omg... [01:48] we are going to have some work cut out with HA state [01:48] there is a lot of code that assumes one and only one state server [01:48] * thumper passes that problem on to a future self [04:02] davecheney: what's the practicle difference between path.Join and filepath.Join? [04:02] practical [04:07] grrr [04:07] freaking import loops [04:47] thumper: path is an ideal path, filepath is OS specific [04:47] davecheney: so which should we be using? filepath? [04:48] if it is a filesystem path, filepath [04:48] if it is an ideal path, a url, use path [05:27] thumper: I'm looking at your branch, btw. [05:28] jam: oh, ok [05:28] jam: there are two [05:28] thumper: pratical matters, 'filepath' will create '\' in names on Windows, "path" never will [05:28] I would point you towards using 'path' as a default unless there are strong reasons not to. [05:28] I had to audit a bunch of places recently to get a client on win32 to work [05:29] thumper: so while I agree with dfc's comment, I would add to it "if in doubt, use path" [05:29] as those generally still work on all platforms. [05:30] The only things I've found that need '\' are: dir (because it uses / to indicate argument flags), and the actual executable that you are spawning [05:35] thumper: for https://code.launchpad.net/~thumper/juju-core/consolidate-bootstrap-state/+merge/172701 [05:36] I'm a little unsure about having a LoadProviderState function that returns a list of instances [05:36] 'state' sounds like an object to me [05:52] jam: sorry, afk now and not wanting to think too deeply. We could change it to be LoadProviderInstances [05:52] it never was a "state" type thing [05:52] just one instance id [05:52] and later more than one === rvba` is now known as rvba === tasdomas_afk is now known as tasdomas [08:03] mgz_: I did try manually merging and just running the ec2 tests on go-bot, and it failed for me. [08:03] I'm running them now to make sure it runs fine without your patch. [08:04] My guess is that the patch introduces a bad setup/teardown which causes a test to hang [08:04] and thus gets killed after 5min [08:04] With -gocheck.v I see this: http://paste.ubuntu.com/5839627/ [08:05] without the merge, I see this: http://paste.ubuntu.com/5839628/ [08:05] So I'm pretty sure BootstrapMultiple is the culprit [08:07] except running just "go test -gocheck.v -gocheck.vv -gocheck.f BootstrapMultiple" passes the test. [08:07] makes me wonder if there is a timing related issue [08:07] one test not cleaning up in time for the other test to have a clean slate. [08:10] hmmm... picked the wrong test. [08:10] I saw the one that was skipped, but we have 2 skipped. [08:10] I need the one after BootstrapWithDefaultSeries === allenap` is now known as allenap [08:30] jtv: poke [08:30] not sure if you are still around [08:30] Hi jam [08:30] I'd like to see one more test for https://codereview.appspot.com/10905043/, but I can help you add if if you need. [08:30] About to go into my standup! [08:30] specifically, lets test this against all environs, so we all do the same thing. [08:41] mgz_: so the full run without your patch is: http://paste.ubuntu.com/5839699/ [08:42] that would mean TestDestroy is what is failing. [08:43] running it by itself with -gocheck.vv passes, though with: http://paste.ubuntu.com/5839700/ [09:07] jtv: when you're back, I think I've reviewed all your branches. [09:07] (my personal queue has emptied) [09:08] jam: just out of the call & looking at your review. Thanks for running the tests as well; it's been taking me ages to get a cloud instance set up for that. [09:08] (My working machine is an i386 installation, so can't run all of the tests) [09:09] fwereade: is https://codereview.appspot.com/10869043/ to your satisfaction with the explanation & added comment? [09:09] jtv: I didn't actually run the tests for your patches. I'm going over a failing submission from mgz. [09:09] jtv: but I certainly can on request :) [09:09] oh [09:09] Well, I'm trying to run tests. :) [09:09] mgz_: mgz_: http://paste.ubuntu.com/5839754/ is the test suite with danilos' patch. It almost looks like CheckEnvironmentsOnConnect is running too fast. So it is starting up and stopping and then another thread gets stuck waiting for stuff to come up. [09:10] jtv: and the bot will run all the tests when it goes to land [09:10] jam: I wonder if the review changes to my branch could be handled in a separate branch… I've already started on a new branch and with colocation this sort of thing is very awkward. I think it's been costing us a lot. [09:10] oh nm, my current code can wait. [09:11] jtv: do you use a lightweight checkout or cobzr? [09:11] bzr commit/bzr shelve; bzr switch works pretty well for me. [09:12] Yes. [09:12] jtv: as for the original question, yes you could do it in a followup. [09:12] Thanks. [09:14] jam: I did wonder why 10ns is too short a timeout — why wait at all? Really the only reason I didn't set zero was that that number might have a special meaning. [09:14] fwereade: just a mechanical rename change: https://codereview.appspot.com/10906043/ [09:14] jtv: if you are dealing with goroutines [09:15] you might need a sleep [09:15] because they are not preemptive [09:15] and if the timeout is too short, it will just return "false" immediately [09:15] I was told even time.Sleep(0) would yield. [09:15] and never give the other goroutine a slice to do its init/etc. [09:15] jtv: I looked at AttemptStrategy [09:15] it always retruns 1 time without sleeping [09:15] and then if now + delay < end it won't ever sleep [09:16] jtv: http://paste.ubuntu.com/5839772/ [09:16] if I havent set end yet, just return true [09:16] once it is set [09:16] if now + delay > end return false [09:16] no sleep [09:16] jam, a suggestion -- NotifyWatcher is an interface, EntityWatcher is an implementation of that interface? don't actually need to export entityWatcher [09:16] so we could set Total up [09:17] and Delay to 9 [09:17] sorry, to 0 [09:17] So this makes our tests invisibly timing-dependent? Bummer. [09:17] jtv: well, the current codebase is far more timing dependent than I would like [09:17] * jtv sad [09:18] fwereade: that would be possible, but does it benefit us to call the public interface something else in its private implementation ? [09:18] fwereade: seems more like a case of "so what watchers do I have, oh this entity thing..." [09:18] jam, I think so, because eg CleanupWatcher has Changes() <-chan struct{} [09:19] fwereade: so I guess it depends on what your thoughts on why some things should be a NotifyWatcher (significant detail is that it has a Changes that returns no content), and entityWatcher whose significant detail is that it watches a document. [09:20] jam, it might be a dumb idea -- but ISTM that the document watching is very much an implementation detail [09:21] jam, we could go either way, I think, either have N types for N different watchers -- MachineWatcher, UnitWatcher, ServiceWatcher, blah, blah [09:22] jam, or just categorize them all by what sort of data they send and save a load of boilerplate at the API level [09:22] jam, the middle ground where we categorize by implementation seems a bit off though [09:22] fwereade: if they don't have a functional difference, I'd like to go with the NotifyWatcher at API level. [09:22] jam, yeah, that's what I'm thinking [09:24] jam, but it may as well be what we expose in state as well [09:25] jam: thanks for explaining the timing issue by the way. The code I based this on just set the timings to 0.01e9 and 0.25e9, without documentation — with this knowledge looked more like a deliberate trap than proper software engineering. [09:26] jtv: there was a discussion about what was 'right', I think we want to go with 10*time.Millisecond rather than 0.01e9 [09:26] we've used it in most of the other places [09:26] though the discussion at the time was "I perfectly understand that the field is in Nanoseconds and find the shorter scientific notation more readable" [09:27] jtv: I don't know if it was clear, you don't have to fix DummyProvider in your patch [09:27] I was just offering ways in which it might have been hanging. [09:27] Ah yes, the if-normal-people-can-read-this-I-won't-be-elite argument... :/ [09:27] I hadn't gotten to that point yet — still documenting the timing issue. :) [09:28] Because really, I don't care a hoot what the timing should be, just that it be clear _why_ it is what it is. [09:28] Exactly so that this conversation (or worse, ill-advised code change) doesn't have to happen again. :-) [09:29] jtv: yeah, I personally find the attempt strategy's logic a bit odd. So that the actual time you sleep isn't >= Total, but instead >= Total-Delay [09:30] These things do get hairy if you want to get them right... sometimes the accounting gets a bit weird if you also want it to include the time spent doing the accounting itself and such. [09:31] But I'm merrily staying out of that for now, just documenting what the crucial factors are that should urgently have been documented originally. [09:31] jtv: yeah, and is the statement meaning "I want to wait no more than X" or "I want to wait at least X" [09:31] Good one. [09:32] fwereade: also, you didn't LGTM the upgrader-api branch [09:32] I think I responded to your requests [09:33] Most of these things most readers won't need or want to care about — the arrogance of assuming that the reader will want to spend their time grokking the author's life work and deep thoughts is just stunning. Writing "0.01e9" for "integer numer of nanoseconds" is only clear to the author, or someone who went to all that trouble. [09:34] fwereade: there is also some confusion as bits of the API code just copy & pasted the state code, which isn't quite what we want. I think there having an interface for NotifyWatcher might make it clearer that API probably wants to share the interface, but not the implementation. [09:34] jam, yeah, I'm just doing a last pass on the upgrader now [09:35] jam, and, yeah, if API code is copy-pasting state code someone's doing it wrong [09:35] jam, which bits in particular? [09:37] fwereade: state/api/watcher.go [09:37] looks very cut & pasted [09:37] with newEntityWatcher et al [09:39] jam, yeah, I agree that should be using the interface [09:40] jam, but it's not quite cut and pasted, it's more a translation layer [09:40] jam, which inevitably bears some resemblance, it is true [09:41] fwereade: well, I think it started out as a C&P and then was a bit adapted. However, I think a lot of the internals aren't actually relevant. [09:41] fwereade: note that there are no *callers* of newEntityWatcher in API [09:41] which is how I know it is a C&P [09:41] jam, I think it was more not-actually-deleted than C+P [09:42] jam, however it does rather imply that nobody got around to finishing the tests for stuff like the machiner api [09:43] jam, which would/should be using that code for the machine watch [09:43] jam, well, never got round to actually finishing the client side of the api, actually [09:43] jam, nothing to do with the tests [09:44] fwereade: much of the client side is definitely unimplemented :) [09:44] or :'( depending on your POV [09:44] jam, heh [09:45] jam, so anyway AFAIK in that file entityWatcher is good, lifecycleWatcher is crack (but easily fixable) and environConfigWatcher should never have been implemented [09:46] jam, (LW drops events -- we could fix it either by coalescing (hard work) or flip-flopping between reads from in and writes to out (easier, probably smarter too)) [09:48] fwereade: how does it drop changes? [09:48] because it switching what "out" to use? [09:48] it looks like it is coallescing [09:48] or is it that it just ends up with "Latest" [09:49] even if it got a set/unset/set ? [09:49] jam, LW reads from in and overwrites changes [09:49] jam, if it hasn't already been sent on to out [09:50] jam, switching outs is great [09:50] fwereade: "ids, err = w.merge(ids, ch);" [09:50] fwereade: isn't that merging rather than overriding? [09:50] jam, state/api/watcher.go? [09:50] fwereade: ah, state/watcher.go [09:51] jam, AFAIK state.LifecycleWatcher is ok (although I do need to land those branches) [09:51] jam, fwereade: hey guys [09:51] hi jam [09:51] jamespage: ^^ :) [09:51] lol [09:51] jamespage, heyhey [09:51] so - golang versions [09:51] right now we have version 1 in 12.04 [09:52] 1.0.2 quantal->raring [09:52] and 1.1.1 sometime in saucy [09:52] jamespage: sure. and we *really* want something >= 1.0.2 in P [09:52] but even the patched 1.0.2 isn't great [09:53] so the version 1 in 12.04 is no good anyway you look at it - why? [09:53] jam, jamespage: yeah, if we're stuck with 1.0.x we'd really like 1.0.3 [09:53] jam / fwereade: Can you pinpoint exactly why 1 isn't suitable? [09:54] jamespage: the version in P won't build the gwacl (azure) support package that julian is working on [09:54] * davecheney scrobbles for issue [09:54] https://bugs.launchpad.net/gwacl/+bug/1196811 [09:54] davecheney: and I think your SSL connection problems are also related to the 1.0.2+backported changes. [09:55] jamespage: ISTR that go1 has other problems with HTTP support. [09:55] Didn't we have a work around for the go-curl issue? [09:55] I'm 70% sure that it just won't pass the juju-core test suite for even juju-core 1.10 [09:56] jam, jamespage, davecheney: yeah, in practice we see the tools bucket response hang up if we use 1.0.2, right? [09:56] Daviey: IIRC, the original problem is that go SSL support internally doesn't support renegotiating the connection. Which the workaround was to depend on go-curl [09:56] which has a different cgo related failure [09:56] The reason jamespage and myself are exploring this, is that we may have pushback and other complexities bringing back a newer golang.. So we want to be certain it's our best effort. [09:56] fwereade: jam i think we had this problem as early as Altanta but didn't have the time to investigate properly [09:57] Daviey: "pushback on newer golang" even for 1.0.3 ? [09:57] Well, we might attack that differently TBH [09:57] AFAIK, we could live with 1.0.3 because that is what we are ensuring support for. We would like 1.1+ because go authors have themselves stopped support for the 1.0 branch [09:58] (so there will never be a go 1.0.4) [09:58] Daviey, and just to confirm (I guess this is a stupid question) there's no way we can build it for P without tools distributed in P? [09:58] jam: and there are bugs in 1.0.3 [09:58] loooots of ugs [09:58] bugs in the http library [09:58] 1.0.3 might be fit for a point release SRU to precise, but considering it seems that much iternal stuff is changing.. personally (as an SRU member), i am uncomfortable with that based on what i currently know [09:58] davecheney: fewer than in go 1.0 :) [09:58] fwereade: We /can/, but it would piss some people off. [09:59] Daviey: i can attest to the commitment of the Go authors to backwards compatibilty [09:59] Daviey: the SRU idea is that we don' want to break people that are using the existing thing, or introduce regression in what is working for them. I *think* 1.0.3 is a sufficiently-better-than-1.0 for parties that are using golang on P [09:59] we have a tool which runs as part of every single CI build that valdiates we never depricate or change a symbol that existed at Go 1.0 [09:59] Daviey, more or less than changing the golang version in P? because being able to use the same golang version across the board would be very handy... [10:00] davecheney: Well, anyone else using Go in 12.04 has had to work their app based on known issues that we are talkign about now. The thing that worries me is that we change the known broken behaviour, and it regresses their stuff. [10:00] Daviey, and if we piss people off either way I'd prefer to devote our efforts towards the one that works better for us... [10:00] unless the workarounds are known to also work with a known good version? [10:00] Daviey: that is fair [10:00] fwereade, thats some of the challenge: saucy will get 1.1.1 soon-ish making that immediately inconsistent with earlier releases [10:00] i do not have reliable stats on the various install paths for Go [10:00] many many people were using Gustavo's PPA versions [10:00] they have been quite surprised that they disappeared [10:01] Which is a good reason PPA's are dangerous to depend upon. The primary archive we are duty bound to support by promise. [10:01] Anyway... [10:01] Daviey: certainly, i am in no way arguing the rightness or wrongness of that approach [10:02] only offering a data point that main was not the only way people get Go on P [10:02] If it takes a few hours to have dual toolchain support in Go, that is better time spent than jamespage and myself fighting with the rest of the distro team to force something in. [10:02] Soo. if we can evaluate feasibility of dual support, it gives us more merrit eithetr way [10:03] Daviey: I know you can set GOPATH and compile with more than 1 go binary [10:03] support for 1.1.x and 1.0.x [10:03] ? [10:03] If it's going to be super hard, and lots of time - we have documented reasons why backporting the newer version is Superior [10:03] (I've done it from time to time) [10:03] I'm not sure how you turn that into builtin support for more than one version. [10:03] jamespage: Yeah, i am hoping we can build for 1.0 and 1.X concurrently [10:04] so client tooling works for 12.04 -> current dev release right [10:04] Oh! [10:04] then is backporting a newer golang a theoretical problem? [10:05] solve a theoretical problem, rather? [10:05] jamespage: "client tooling works" ? [10:05] sorry - that was misleading [10:06] so that the juju-core package is either building against lastest 1.0.x golang or latest 1.1.x golang [10:06] in the distro backports pocket which is how we are proposing to distribute this officially for precise [10:06] jamespage: today we can build with 1.0.3 and 1.1.1 [10:06] OK - Daviey: I think the right thing is to get the 1.0.x version consistent across 12.04->13.04 [10:07] we are only 1 release away from next LTS at which point most people will start to use 14.04 anyway [10:09] jamespage: Are you suggesting we pursue an SRU, or the backport track for 1.0.x? [10:09] To confirm, everyone thinks we do indeed need a newer golang available in 12.04? [10:09] Daviey: to build Main package with a tool, doesn't that tool need to be an SRU? [10:09] jam: it's not main, so that isn't an issue [10:10] Daviey: yes, precise is essentially the only release that matters for Juju until we reach the next LTS [10:10] Daviey, yes, we do [10:10] jam: There is likely going to be some push back on the prospect of backporting a toolchain package. [10:11] fwereade: https://codereview.appspot.com/10906043/ updated to make NotifyWatcher an interface{} which is implemented by entityWatcher. [10:11] fwereade / davecheney: Right.. Would you mind outlining the specific reasons why "1" isn't suitabl.. just so jamespage and I have some things we can use to support this [10:11] well, once lbox propose finishes [10:11] Daviey: 1 ? you mean the current version in P ? [10:12] Yeah [10:12] in addition to the problems of http bugs that affects P, Q and R [10:12] it also contains bugs that prevents our windows azure support from compiling [10:12] davecheney: Do you have commits to hand that fixe these issues? [10:13] Daviey: to fix https://bugs.launchpad.net/juju-core/+bug/1196811 ? [10:13] (or bugs) [10:13] there is no bug for the http issues with juju 1.11.2 [10:13] * davecheney creates one [10:13] yeah [10:13] Daviey, re SRU or backport - probably SRU [10:14] and we should probably try to get a MRE for golang at some point in time [10:14] davecheney: Do you know how this was fixed in golang >=1.0.2? [10:14] i'm sure it's already been discussed, but there is an open issue where the builders do not use packages from backports [10:14] davecheney: Yep, that isn't a blocker [10:14] Daviey: no, i only reproduced the problem this afternoon [10:15] the 1.11.2 breakage is not reproducable using the upstream go 1.0.2 release [10:15] davecheney: btw, have you tried building 1.0.2 from source? One argument I read in the past was that the Ubuntu package of 1.0.2 backports a change that actually broke things. [10:15] only our version of 1.0.2 shipped in Q and R [10:15] jam: see just above [10:15] davecheney: yeah, you hit enter before me :) [10:16] jamespage: Before being confident in processing an SRU, it would probably have to be raised with the TB first - "intent to MRE, with a trial SRU first".. and Some decent assurance we won't screw people using the current version. [10:18] Daviey: https://bugs.launchpad.net/juju-core/+bug/1197326 [10:18] <_mup_> Bug #1197326: environs/ec2: juju 1.11.2 cannot bootstrap when built with go 1.0.2-2 [10:18] I am looking for older bugs, we've had this problem before but didn't pick the the cause before now [10:22] fwereade: "no idea how this charm". It is part of JujuConnSuite, *way* too much stuff gets added there. :). Though all that code is going to get overhauled anyway as the client-side API is finished up. [10:24] mgz_: are you around yet? https://codereview.appspot.com/10906043/ could use your rubber stamp [10:26] jam, ha. yes, JujuConnSuite is a monster (and actively dangerous in some ways... it sets up both client-only and server-only state) [10:27] fwereade: a lot of that Upgrader code existed because I thought I needed a Unit to test, and to have a unit you need a service and a service needs a charm, etc. [10:27] (Upgrader test code) [10:28] fwereade: fwiw, I side-tracked for NotifyWatcher, but I'll be on Upgrader-client-API and Upgrader-Worker as the next things in the pipe. [10:28] fwereade: as a question there. rog made it so that we start the state worker if we query the api and see a job that requires it. [10:28] jam, no worries, it's pretty clearly going to be useful imminently, I'm not complaining about it [10:28] However, when we start StateWorker, it just starts jobs immediately. [10:29] Is it supposed to be looking at the list? [10:29] I'm trying to sort out how I switch UpgradeWorker from state to API and whether I'm missing something vs just starting it like it is today [10:33] fwereade: also, what needs to be done to land your 'remove TxnRevno' patch? [10:33] fwereade: I need to tweak the Upgrader watcher, and I'd like to depend on your change. [10:34] davecheney: what package would you like me to run on go-bot? I don't want to upgrade to 1.1.1 until we know we can get it into P, but we certainly need >go1 (go-bot runs on P) [10:35] jam: the only thing I can suggest is whatever ships in the series the bot is running [10:37] jam, sorry, phone [10:38] jam, I just need to remove certain appendages from certain other parts of myself and maybe get a second LGTM on the prereq -- that will happen today [10:39] jam, I think you need to remove Upgrader from the stateWorkers list, not start as a state worker, and start it as a api worker -- but I guess you'reasking for somethingmore? [10:40] jam, I think that the list is just about figuring out when we no longer need a state connection [10:47] fwereade: well, roge mentioned something about asking the API for what jobs to run [10:48] fwereade: as for 2 LGTMs... when we are 4 people down, it is really hard to get enough reviews to maintain velocity. [10:48] jam, ah, yes, that does make sense -- we should always start an API connection and get jobs from that [10:48] jam, and figure out workers from jobs [10:48] fwereade: right, it seems to start the API connectio, get a list of jobs, and then does nothing but start a StateWorker. [10:48] which hard-codes the workers it runs. [10:49] jam, but this is just dickery that pushes back starting just one freaking job with an api connection [10:49] jam, which is all I have cared about for like th last month [10:49] jam, and which somehow has not happened [10:50] jam, and which is why we have all these half done workers [10:50] jtv: point of clarification: "make([]foo, 0, 10)" creates a slice with len(0) and cap(10). So you don't have 'nil's on the end. [10:50] fwereade: acked [10:51] jam, once we have that tracer bullet landed I thinkit will be much easier to trim away the ther bits [10:54] fwereade: btw, bot tells me you were right about the charm not being there. I had copied it from somewhere else, apparently I wasn't running what I thought I was... [10:54] jam, ha, at least JujuConnSuite's a bit less awful than I feared ;p [11:05] fwereade: api client discussion. Should state/api/upgrader/* take objects that are 'state/*' objects, or should they be "state/api/params/*' objects? [11:05] I know we have to use state/api/params on the wire to the apiserver [11:05] but what should the client interface be? [11:05] state/* so that the API code looks like the existing raw state code? [11:06] or state/api/params code so it looks closer to what will actually be sent to the APIServer ? [11:06] jam, params please [11:06] fwereade: k [11:06] jam, before too long, hopefully, the only thing that knows about state will be the api [11:07] fwereade: so one thing I was thinking about. Shouldn't state/* be the only things that touch the db, and state/apiserver/* be using the state/* apis? [11:07] jam, is that not the case today? [11:07] jam, it's certainly the intent [11:08] fwereade: well, it wasn't true as I was implementing Pinger, but I'm not sure about the existing code. [11:08] and I'm realizing I was wrong there, so it could have just been me. [11:08] Upgrader does [11:08] and the Machiner code I've looked at does [11:09] jam, does touch the DB? [11:09] jam, or does hide it? :) [11:09] fwereade: Upgrader and Machine* seem to use state calls, *not* DB calls [11:10] jam, good-oh :) [11:10] Pinger as I was writing it was copying presence and using DB calls, but I was slowly working that out. [11:10] jam, ah, got you, cheers [11:11] fwereade: also, state/api/upgrader/* presents a single-object interface? [11:11] jam, but the existing SetAgentAlives should give you Pingers already, right? [11:11] rather than the multi-way at state/apiserver/upgrader/* ? [11:12] jam, I am agnostic there -- if you're rewriting enough of the upgrader worker I'm fine using direct API calls [11:12] fwereade: there is no API for SetAgentAlive [11:12] jam, the bigger the worker the more concerned I am that we maintain the appearance/semantics of the state api [11:12] there is pinger_test.go, but it just tests that a disconnect can be noticed, which is true, but not quite reality. [11:13] fwereade: I'd like the api/* code to be reasonably consistent. If we want single-object or multi-object there, now is the time to decide [11:13] I'd personally like to push multi-object as far down as we can. [11:13] But I think api/machine/* is all single-object [11:13] jam, if you're arguing consistency I think we have to go single [11:13] jam, because we do not want to rewrite all the workers [11:13] jam, long-term I would kinda like to move away from that [11:14] jam: ah thanks — I wasn't aware of the third argument, or had forgotten. [11:14] jam, but the current plan is to wrap them in a thin layer to minimise reimplementation burden of the original workers [11:14] jtv: anyway, it is a hint, not a requirement. [11:15] It does make it a lot easier. Thanks again. [11:17] fwereade: machiner.SetStatus() takes an args that is an array [11:17] ah stupid, I'm in apiserver [11:17] of course it does [11:20] jam, yeah, the idea is just to fix what we send over the wire so it's a bit more flexible but to maintain the interface [11:20] jam, on the client side [11:21] jam, when it's really no more costly to fix the worker I have no qualms about doing so early [11:21] jam, at least the various facades are split up so we can drop the domain object style code one facade at a time [11:21] jam, sane? [11:22] fwereade: seems ok to me [11:25] jam, hell, cath popped out and is not back yet, and laura's getting a bit screamy, I might have to skip standup today [11:26] k [11:49] jtv: you need to import "time" at the top of polling_test.go [11:49] Yes I know, thanks [11:50] jtv: sorry if I came across as bugging. I just try to inform people about bot results since they don't seem to poll it themselves. [11:50] No it's fine, really. I appreciate it. [11:50] It's just that I happened to be already on it. :) [11:54]