thumperoh ffs00:12
thumperlbox choked on this: https://code.launchpad.net/~thumper/juju-core/consolidate-bootstrap-state/+merge/17270100:13
thumperbigjools: you like go, care to review?00:13
bigjoolsthumper: I LOVE go00:14
bigjoolsthumper: but I might have to pass, I'm not really working today00:14
thumperbigjools: oh?00:14
thumperneither is wallyworld00:15
bigjoolsit's easier to ask for forgiveness than to block ;)00:15
thumperbigjools: ha, not blocked00:28
thumperdavecheney: what does it mean for a struct to embed an interface?00:44
davecheneythumper: type S struct { io.Reader }00:45
thumperyeah, or similar00:45
thumperin this particular instance it is environs.Environ00:45
davecheneyit's a shorthand for having to type out all the methods in the io.Reader interface00:46
thumperand it only defines some methods00:46
thumperwhat about the methods it doesn't define?00:46
thumperyou don't normally type out methods inside a struct00:46
thumperI know about embedding an interface in an interface00:46
davecheneythat is correct00:46
thumperbut this is different00:46
davecheneyyeah, interface inside interface really just is short hand00:46
davecheneyso, by embedding an io.Reader inside S this method is automatically crated00:47
davecheneytype (s *S) Read(...) { return s.Reader.Read(...) }00:47
davecheneythere actually is not forwarding method00:47
davecheneybut the world behaves as it is is00:48
thumperso we can implement an interface00:49
thumperwithout implementing all the interface00:49
thumperI get it now00:49
bigjoolswhat happens when an unimplemented method is called?00:50
davecheneywell ... yues00:50
davecheneyS.Reader is nil00:50
bigjoolsso, boom?00:50
davecheneyso S.Read will blow up unless something has been assigned to S.Reader00:50
bigjoolsisn't this dangerous?00:55
davecheneyi'm not really sure what you're trying to do00:56
thumperdavecheney: it isn't me, it is me understanding our existing code00:57
thumperwe have a test that is testing bootstrap and has a fake environment00:57
thumperit only implements the functions it cares about00:57
thumperand passes the test structure through as an environs.Environ00:57
thumperseems to work00:58
=== fdehay_ is now known as fdehay
bigjoolsdavecheney: any luck working out go-curl with Go1?01:22
davecheneybigjools: we need at least version 1.0.2 to make it work01:38
* davecheney finds issue01:38
davecheneybigjools: https://bugs.launchpad.net/gwacl/+bug/119681101:39
thumperwe are going to have some work cut out with HA state01:48
thumperthere is a lot of code that assumes one and only one state server01:48
* thumper passes that problem on to a future self01:48
thumperdavecheney: what's the practicle difference between path.Join and filepath.Join?04:02
thumperfreaking import loops04:07
davecheneythumper: path is an ideal path, filepath is OS specific04:47
thumperdavecheney: so which should we be using? filepath?04:47
davecheneyif it is a filesystem path, filepath04:48
davecheneyif it is an ideal path, a url, use path04:48
jamthumper: I'm looking at your branch, btw.05:27
thumperjam: oh, ok05:28
thumperjam: there are two05:28
jamthumper: pratical matters, 'filepath' will create '\' in names on Windows, "path" never will05:28
jamI would point you towards using 'path' as a default unless there are strong reasons not to.05:28
jamI had to audit a bunch of places recently to get a client on win32 to work05:28
jamthumper: so while I agree with dfc's comment, I would add to it "if in doubt, use path"05:29
jamas those generally still work on all platforms.05:29
jamThe only things I've found that need '\' are: dir (because it uses / to indicate argument flags), and the actual executable that you are spawning05:30
jamthumper: for https://code.launchpad.net/~thumper/juju-core/consolidate-bootstrap-state/+merge/17270105:35
jamI'm a little unsure about having a LoadProviderState function that returns a list of instances05:36
jam'state' sounds like an object to me05:36
thumperjam: sorry, afk now and not wanting to think too deeply.  We could change it to be LoadProviderInstances05:52
thumperit never was a "state" type thing05:52
thumperjust one instance id05:52
thumperand later more than one05:52
=== rvba` is now known as rvba
=== tasdomas_afk is now known as tasdomas
jammgz_: I did try manually merging and just running the ec2 tests on go-bot, and it failed for me.08:03
jamI'm running them now to make sure it runs fine without your patch.08:03
jamMy guess is that the patch introduces a bad setup/teardown which causes a test to hang08:04
jamand thus gets killed after 5min08:04
jamWith -gocheck.v I see this: http://paste.ubuntu.com/5839627/08:04
jamwithout the merge, I see this: http://paste.ubuntu.com/5839628/08:05
jamSo I'm pretty sure BootstrapMultiple is the culprit08:05
jamexcept running just "go test -gocheck.v -gocheck.vv -gocheck.f BootstrapMultiple" passes the test.08:07
jammakes me wonder if there is a timing related issue08:07
jamone test not cleaning up in time for the other test to have a clean slate.08:07
jamhmmm... picked the wrong test.08:10
jamI saw the one that was skipped, but we have 2 skipped.08:10
jamI need the one after BootstrapWithDefaultSeries08:10
=== allenap` is now known as allenap
jamjtv: poke08:30
jamnot sure if you are still around08:30
jtvHi jam08:30
jamI'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
jtvAbout to go into my standup!08:30
jamspecifically, lets test this against all environs, so we all do the same thing.08:30
jammgz_: so the full run without your patch is: http://paste.ubuntu.com/5839699/08:41
jamthat would mean TestDestroy is what is failing.08:42
jamrunning it by itself with -gocheck.vv passes, though with: http://paste.ubuntu.com/5839700/08:43
jamjtv: when you're back, I think I've reviewed all your branches.09:07
jam(my personal queue has emptied)09:07
jtvjam: 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
jtv(My working machine is an i386 installation, so can't run all of the tests)09:08
jtvfwereade: is https://codereview.appspot.com/10869043/ to your satisfaction with the explanation & added comment?09:09
jamjtv: I didn't actually run the tests for your patches. I'm going over a failing submission from mgz.09:09
jamjtv: but I certainly can on request :)09:09
jtvWell, I'm trying to run tests.  :)09:09
jammgz_: 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:09
jamjtv: and the bot will run all the tests when it goes to land09:10
jtvjam: 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
jtvoh nm, my current code can wait.09:10
jamjtv: do you use a lightweight checkout or cobzr?09:11
jambzr commit/bzr shelve; bzr switch works pretty well for me.09:11
jamjtv: as for the original question, yes you could do it in a followup.09:12
jtvjam: 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
jamfwereade: just a mechanical rename change: https://codereview.appspot.com/10906043/09:14
jamjtv: if you are dealing with goroutines09:14
jamyou might need a sleep09:15
jambecause they are not preemptive09:15
jamand if the timeout is too short, it will just return "false" immediately09:15
jtvI was told even time.Sleep(0) would yield.09:15
jamand never give the other goroutine a slice to do its init/etc.09:15
jamjtv: I looked at AttemptStrategy09:15
jamit always retruns 1 time without sleeping09:15
jamand then if now + delay < end it won't ever sleep09:15
jamjtv: http://paste.ubuntu.com/5839772/09:16
jamif I havent set end yet, just return true09:16
jamonce it is set09:16
jamif now + delay > end return false09:16
jamno sleep09:16
fwereadejam, a suggestion -- NotifyWatcher is an interface, EntityWatcher is an implementation of that interface? don't actually need to export entityWatcher09:16
jamso we could set Total up09:16
jamand Delay to 909:17
jamsorry, to 009:17
jtvSo this makes our tests invisibly timing-dependent?  Bummer.09:17
jamjtv: well, the current codebase is far more timing dependent than I would like09:17
* jtv sad09:17
jamfwereade: that would be possible, but does it benefit us to call the public interface something else in its private implementation ?09:18
jamfwereade: seems more like a case of "so what watchers do I have, oh this entity thing..."09:18
fwereadejam, I think so, because eg CleanupWatcher has Changes() <-chan struct{}09:18
jamfwereade: 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:19
fwereadejam, it might be a dumb idea -- but ISTM that the document watching is very much an implementation detail09:20
fwereadejam, we could go either way, I think, either have N types for N different watchers -- MachineWatcher, UnitWatcher, ServiceWatcher, blah, blah09:21
fwereadejam, or just categorize them all by what sort of data they send and save a load of boilerplate at the API level09:22
fwereadejam, the middle ground where we categorize by implementation seems a bit off though09:22
jamfwereade: if they don't have a functional difference, I'd like to go with the NotifyWatcher at API level.09:22
fwereadejam, yeah, that's what I'm thinking09:22
fwereadejam, but it may as well be what we expose in state as well09:24
jtvjam: 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:25
jamjtv: there was a discussion about what was 'right', I think we want to go with 10*time.Millisecond rather than 0.01e909:26
jamwe've used it in most of the other places09:26
jamthough the discussion at the time was "I perfectly understand that the field is in Nanoseconds and find the shorter scientific notation more readable"09:26
jamjtv: I don't know if it was clear, you don't have to fix DummyProvider in your patch09:27
jamI was just offering ways in which it might have been hanging.09:27
jtvAh yes, the if-normal-people-can-read-this-I-won't-be-elite argument...  :/09:27
jtvI hadn't gotten to that point yet — still documenting the timing issue.  :)09:27
jtvBecause 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
jtvExactly so that this conversation (or worse, ill-advised code change) doesn't have to happen again.  :-)09:28
jamjtv: 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-Delay09:29
jtvThese 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:30
jtvBut 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
jamjtv: yeah, and is the statement meaning "I want to wait no more than X" or "I want to wait at least X"09:31
jtvGood one.09:31
jamfwereade: also, you didn't LGTM the upgrader-api branch09:32
jamI think I responded to your requests09:32
jtvMost 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:33
jamfwereade: 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
fwereadejam, yeah, I'm just doing a last pass on the upgrader now09:34
fwereadejam, and, yeah, if API code is copy-pasting state code someone's doing it wrong09:35
fwereadejam, which bits in particular?09:35
jamfwereade: state/api/watcher.go09:37
jamlooks very cut & pasted09:37
jamwith newEntityWatcher et al09:37
fwereadejam, yeah, I agree that should be using the interface09:39
fwereadejam, but it's not quite cut and pasted, it's more a translation layer09:40
fwereadejam, which inevitably bears some resemblance, it is true09:40
jamfwereade: 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
jamfwereade: note that there are no *callers* of newEntityWatcher in API09:41
jamwhich is how I know it is a C&P09:41
fwereadejam, I think it was more not-actually-deleted than C+P09:41
fwereadejam, however it does rather imply that nobody got around to finishing the tests for stuff like the machiner api09:42
fwereadejam, which would/should be using that code for the machine watch09:43
fwereadejam, well, never got round to actually finishing the client side of the api, actually09:43
fwereadejam, nothing to do with the tests09:43
jamfwereade: much of the client side is definitely unimplemented :)09:44
jamor :'( depending on your POV09:44
fwereadejam, heh09:44
fwereadejam, so anyway AFAIK in that file entityWatcher is good, lifecycleWatcher is crack (but easily fixable) and environConfigWatcher should never have been implemented09:45
fwereadejam, (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:46
jamfwereade: how does it drop changes?09:48
jambecause it switching what "out" to use?09:48
jamit looks like it is coallescing09:48
jamor is it that it just ends up with "Latest"09:48
jameven if it got a set/unset/set ?09:49
fwereadejam, LW reads from in and overwrites changes09:49
fwereadejam, if it hasn't already been sent on to out09:49
fwereadejam, switching outs is great09:50
jamfwereade: "ids, err = w.merge(ids, ch);"09:50
jamfwereade: isn't that merging rather than overriding?09:50
fwereadejam, state/api/watcher.go?09:50
jamfwereade: ah, state/watcher.go09:50
fwereadejam, AFAIK state.LifecycleWatcher is ok (although I do need to land those branches)09:51
jamespagejam, fwereade: hey guys09:51
jamhi jam09:51
jamjamespage: ^^ :)09:51
fwereadejamespage, heyhey09:51
jamespageso  - golang versions09:51
jamespageright now we have version 1 in 12.0409:51
jamespage1.0.2 quantal->raring09:52
jamespageand 1.1.1 sometime in saucy09:52
jamjamespage: sure. and we *really* want something >= 1.0.2 in P09:52
jambut even the patched 1.0.2 isn't great09:52
jamespageso the version 1 in 12.04 is no good anyway you look at it - why?09:53
fwereadejam, jamespage: yeah, if we're stuck with 1.0.x we'd really like 1.0.309:53
Davieyjam / fwereade: Can you pinpoint exactly why 1 isn't suitable?09:53
davecheneyjamespage: the version in P won't build the gwacl (azure) support package that julian is working on09:54
* davecheney scrobbles for issue09:54
jamdavecheney: and I think your SSL connection problems are also related to the 1.0.2+backported changes.09:54
jamjamespage: ISTR that go1 has other problems with HTTP support.09:55
DavieyDidn't we have a work around for the go-curl issue?09:55
jamI'm 70% sure that it just won't pass the juju-core test suite for even juju-core 1.1009:55
fwereadejam, jamespage, davecheney: yeah, in practice we see the tools bucket response hang up if we use 1.0.2, right?09:56
jamDaviey: IIRC, the original problem is that go SSL support internally doesn't support renegotiating the connection. Which the workaround was to depend on go-curl09:56
jamwhich has a different cgo related failure09:56
DavieyThe 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
davecheneyfwereade: jam i think we had this problem as early as Altanta but didn't have the time to investigate properly09:56
jamDaviey: "pushback on newer golang" even for 1.0.3 ?09:57
DavieyWell, we might attack that differently TBH09:57
jamAFAIK, 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 branch09:57
jam(so there will never be a go 1.0.4)09:58
fwereadeDaviey, 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
davecheneyjam: and there are bugs in 1.0.309:58
davecheneyloooots of ugs09:58
davecheneybugs in the http library09:58
Daviey1.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 know09:58
jamdavecheney: fewer than in go 1.0 :)09:58
Davieyfwereade: We /can/, but it would piss some people off.09:58
davecheneyDaviey: i can attest to the commitment of the Go authors to backwards compatibilty09:59
jamDaviey: 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 P09:59
davecheneywe 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.009:59
fwereadeDaviey, 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...09:59
Davieydavecheney: 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
fwereadeDaviey, 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
Davieyunless the workarounds are known to also work with a known good version?10:00
davecheneyDaviey: that is fair10:00
jamespagefwereade, thats some of the challenge: saucy will get 1.1.1 soon-ish making that immediately inconsistent with earlier releases10:00
davecheneyi do not have reliable stats on the various install paths for Go10:00
davecheneymany many people were using Gustavo's PPA versions10:00
davecheneythey have been quite surprised that they disappeared10:00
DavieyWhich is a good reason PPA's are dangerous to depend upon.  The primary archive we are duty bound to support by promise.10:01
davecheneyDaviey: certainly, i am in no way arguing the rightness or wrongness of that approach10:01
davecheneyonly offering a data point that main was not the only way people get Go on P10:02
DavieyIf 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
DavieySoo. if we can evaluate feasibility of dual support, it gives us more merrit eithetr way10:02
jamDaviey: I know you can set GOPATH and compile with more than 1 go binary10:03
jamespagesupport for 1.1.x and 1.0.x10:03
DavieyIf it's going to be super hard, and lots of time - we have documented reasons why backporting the newer version is Superior10:03
jam(I've done it from time to time)10:03
jamI'm not sure how you turn that into builtin support for more than one version.10:03
Davieyjamespage: Yeah, i am hoping we can build for 1.0 and 1.X concurrently10:03
jamespageso client tooling works for 12.04 -> current dev release right10:04
Davieythen is backporting a newer golang a theoretical problem?10:04
Davieysolve a  theoretical problem, rather?10:05
jamjamespage: "client tooling works" ?10:05
jamespagesorry - that was misleading10:05
jamespageso that the juju-core package is either building against lastest 1.0.x golang or latest 1.1.x golang10:06
jamespagein the distro backports pocket which is how we are proposing to distribute this officially for precise10:06
jamjamespage: today we can build with 1.0.3 and 1.1.110:06
jamespageOK - Daviey: I think the right thing is to get the 1.0.x version consistent across 12.04->13.0410:06
jamespagewe are only 1 release away from next LTS at which point most people will start to use 14.04 anyway10:07
Davieyjamespage: Are you suggesting we pursue an SRU, or the backport track for 1.0.x?10:09
DavieyTo confirm, everyone thinks we do indeed need a newer golang available in 12.04?10:09
jamDaviey: to build Main package with a tool, doesn't that tool need to be an SRU?10:09
Davieyjam: it's not main, so that isn't an issue10:09
davecheneyDaviey: yes, precise is essentially the only release that matters for Juju until we reach the next LTS10:10
fwereadeDaviey, yes, we do10:10
Davieyjam: There is likely going to be some push back on the prospect of backporting a toolchain package.10:10
jamfwereade: https://codereview.appspot.com/10906043/ updated to make NotifyWatcher an interface{} which is implemented by entityWatcher.10:11
Davieyfwereade / 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 this10:11
jamwell, once lbox propose finishes10:11
davecheneyDaviey: 1 ? you mean the current version in P ?10:11
davecheneyin addition to the problems of http bugs that affects P, Q and R10:12
davecheneyit also contains bugs that prevents our windows azure support from compiling10:12
Davieydavecheney: Do you have commits to hand that fixe these issues?10:12
davecheneyDaviey: to fix https://bugs.launchpad.net/juju-core/+bug/1196811 ?10:13
Daviey(or bugs)10:13
davecheneythere is no bug for the http issues with juju 1.11.210:13
* davecheney creates one10:13
jamespageDaviey, re SRU or backport - probably SRU10:13
jamespageand we should probably try to get a MRE for golang at some point in time10:14
Davieydavecheney: Do you know how this was fixed in golang >=1.0.2?10:14
davecheneyi'm sure it's already been discussed, but there is an open issue where the builders do not use packages from backports10:14
Davieydavecheney: Yep, that isn't a blocker10:14
davecheneyDaviey: no, i only reproduced the problem this afternoon10:14
davecheneythe 1.11.2 breakage is not reproducable using the upstream go  1.0.2 release10:15
jamdavecheney: 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
davecheneyonly our version of 1.0.2 shipped in Q and R10:15
davecheneyjam: see just above10:15
jamdavecheney: yeah, you hit enter before me :)10:15
Davieyjamespage: 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:16
davecheneyDaviey: https://bugs.launchpad.net/juju-core/+bug/119732610:18
_mup_Bug #1197326: environs/ec2: juju 1.11.2 cannot bootstrap when built with go 1.0.2-2 <juju-core:New> <https://launchpad.net/bugs/1197326>10:18
davecheneyI am looking for older bugs, we've had this problem before but didn't pick the the cause before now10:18
jamfwereade: "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:22
jammgz_: are you around yet? https://codereview.appspot.com/10906043/ could use your rubber stamp10:24
fwereadejam, ha. yes, JujuConnSuite is a monster (and actively dangerous in some ways... it sets up both client-only and server-only state)10:26
jamfwereade: 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
jam(Upgrader test code)10:27
jamfwereade: 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
jamfwereade: 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
fwereadejam, no worries, it's pretty clearly going to be useful imminently, I'm not complaining about it10:28
jamHowever, when we start StateWorker, it just starts jobs immediately.10:28
jamIs it supposed to be looking at the list?10:29
jamI'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 today10:29
jamfwereade: also, what needs to be done to land  your 'remove TxnRevno' patch?10:33
jamfwereade: I need to tweak the Upgrader watcher, and I'd like to depend on your change.10:33
jamdavecheney: 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:34
davecheneyjam: the only thing I can suggest is whatever ships in the series the bot is running10:35
fwereadejam, sorry, phone10:37
fwereadejam, 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 today10:38
fwereadejam, 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:39
fwereadejam, I think that the list is just about figuring out when we no longer need a state connection10:40
jamfwereade: well, roge mentioned something about asking the API for what jobs to run10:47
jamfwereade: as for 2 LGTMs... when we are 4 people down, it is really hard to get enough reviews to maintain velocity.10:48
fwereadejam, ah, yes, that does make sense -- we should always start an API connection and get jobs from that10:48
fwereadejam, and figure out workers from jobs10:48
jamfwereade: right, it seems to start the API connectio, get a list of jobs, and then does nothing but start a StateWorker.10:48
jamwhich hard-codes the workers it runs.10:48
fwereadejam, but this is just dickery that pushes back starting just one freaking job with an api connection10:49
fwereadejam, which is all I have cared about for like th last month10:49
fwereadejam, and which somehow has not happened10:49
fwereadejam, and which is why we have all these half done workers10:50
jamjtv: 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
jamfwereade: acked10:50
fwereadejam, once we have that tracer bullet landed I thinkit will be much easier to trim away the ther bits10:51
jamfwereade: 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
fwereadejam, ha, at least JujuConnSuite's a bit less awful than I feared ;p10:54
jamfwereade: api client discussion. Should state/api/upgrader/* take objects that are 'state/*' objects, or should they be "state/api/params/*' objects?11:05
jamI know we have to use state/api/params on the wire to the apiserver11:05
jambut what should the client interface be?11:05
jamstate/* so that the API code looks like the existing raw state code?11:05
jamor state/api/params code so it looks closer to what will actually be sent to the APIServer ?11:06
fwereadejam, params please11:06
jamfwereade: k11:06
fwereadejam, before too long, hopefully, the only thing that knows about state will be the api11:06
jamfwereade: 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
fwereadejam, is that not the case today?11:07
fwereadejam, it's certainly the intent11:07
jamfwereade: well, it wasn't true as I was implementing Pinger, but I'm not sure about the existing code.11:08
jamand I'm realizing I was wrong there, so it could have just been me.11:08
jamUpgrader does11:08
jamand the Machiner code I've looked at does11:08
fwereadejam, does touch the DB?11:09
fwereadejam, or does hide it? :)11:09
jamfwereade: Upgrader and Machine* seem to use state calls, *not* DB calls11:09
fwereadejam, good-oh :)11:10
jamPinger as I was writing it was copying presence and using DB calls, but I was slowly working that out.11:10
fwereadejam, ah, got you, cheers11:10
jamfwereade: also, state/api/upgrader/* presents a single-object interface?11:11
fwereadejam, but the existing SetAgentAlives should give you Pingers already, right?11:11
jamrather than the multi-way at state/apiserver/upgrader/* ?11:11
fwereadejam, I am agnostic there -- if you're rewriting enough of the upgrader worker I'm fine using direct API calls11:12
jamfwereade: there is no API for SetAgentAlive11:12
fwereadejam, the bigger the worker the more concerned I am that we maintain the appearance/semantics of the state api11:12
jamthere is pinger_test.go, but it just tests that a disconnect can be noticed, which is true, but not quite reality.11:12
jamfwereade: I'd like the api/* code to be reasonably consistent. If we want single-object or multi-object there, now is the time to decide11:13
jamI'd personally like to push multi-object as far down as we can.11:13
jamBut I think api/machine/* is all single-object11:13
fwereadejam, if you're arguing consistency I think we have to go single11:13
fwereadejam, because we do not want to rewrite all the workers11:13
fwereadejam, long-term I would kinda like to move away from that11:13
jtvjam: ah thanks — I wasn't aware of the third argument, or had forgotten.11:14
fwereadejam, but the current plan is to wrap them in a thin layer to minimise reimplementation burden of the original workers11:14
jamjtv: anyway, it is a hint, not a requirement.11:14
jtvIt does make it a lot easier.  Thanks again.11:15
jamfwereade: machiner.SetStatus() takes an args that is an array11:17
jamah stupid, I'm in apiserver11:17
jamof course it does11:17
fwereadejam, yeah, the idea is just to fix what we send over the wire so it's a bit more flexible but to maintain the interface11:20
fwereadejam, on the client side11:20
fwereadejam, when it's really no more costly to fix the worker I have no qualms about doing so early11:21
fwereadejam, at least the various facades are split up so we can drop the domain object style code one facade at a time11:21
fwereadejam, sane?11:21
jamfwereade: seems ok to me11:22
fwereadejam, hell, cath popped out and is not back yet, and laura's getting a bit screamy, I might have to skip standup today11:25
jamjtv: you need to import "time" at the top of polling_test.go11:49
jtvYes I know, thanks11:49
jamjtv: 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
jtvNo it's fine, really.  I appreciate it.11:50
jtvIt's just that I happened to be already on it.  :)11:50
jtvjam: if people aren't noticing their landing failures, that may mean they've got too much work in-flight, which in turn might mean that the process needs tweaking.11:54
jtvFor example, if branches take too long to complete when the "real" work is already done, people  will work around the blockage by taking on additional work.11:56
fwereadejam, I think I'm going to land https://codereview.appspot.com/10798043, would you just cast a quick eye over it on general principles please?12:44
jamfwereade: Upgrader.Tools currently returns the desired Tools for a given agent. But we don't have an API to get the *actual* current tools for an agent.12:44
fwereadejam, do we need that in the API?12:45
jamfwereade: I don't know. I know I expected it as part of testing12:45
fwereadejam, state has Unit and Machine.AgentTools12:45
fwereadejam, I don't think it's necessary tbh -- all your testing of what-actually-happened should probably be against independent state instances regardless, right?12:46
jamfwereade: offhand Upgrader.Tools sounds like something that could just return the desired tools. Vs a Machine.Tools that should return the current running tools.12:47
jamso we're probably fine.12:47
jamIt was just a moment of surprise12:47
fwereadejam, cool12:47
jamfwereade: still LGTM12:49
jamfwereade: ran into something interesting. It seems you cannot embed types in api/params12:52
jamif you do, they don't get serialized back properly by rpc.jsoncodec12:52
jamdirect server tests don't notice,because you test the object being returned12:52
jambut client-side tests notice12:52
jamthe data isn't on the wire12:52
fwereadejam, aww, wtf12:52
jamfwereade: http://paste.ubuntu.com/5840230/12:55
jamWithout that change to api/params/params.go I was getting Error == null, but no JSON content for the embedded AgentTools fields.12:55
fwereadejam, that's really unhelpful :/13:01
fwereadejam, ah, apparently it's fixed in 1.113:02
fwereadejam, although that is of limited value to us :/13:02
jamfwereade: depends how the conversation goes :)13:02
jamfwereade, mgz_: If you have the time this is the changes to get client-side Upgrader up and running: https://codereview.appspot.com/10711044/13:08
=== mgz_ is now known as mgz
fwereadejam, cool, just a mo13:09
jammgz: fwereade: I'm off for today, but I'd love to get feedback before I start work tomorrow.13:10
fwereadejam, np13:10
jamI might even stop by tonight.13:11
=== wedgwood_away is now known as wedgwood
=== tasdomas is now known as tasdomas_afk
=== wedgwood is now known as wedgwood_away
=== wedgwood_away is now known as wedgwood
arosalesIs it correct to state the only valid option to add-unit in juju-core  is "-n"15:27
arosales"add-unit -c" or "add-unit --count"  to look to be supported15:28
fwereadearosales, -n/--num-units are all we implemented it seems -- if we should have --count as well, would you open a bug please?15:32
* fwereade needs to head out a bit early today, will see you all later15:33
arosalesfwereade, thanks for the info and will do re: --count15:33
mgzif its just an alias, do we actually want that versus just a deprecation in python/unsupported notice in juju-core?15:49
mgzpotentially there are scripts that use it, but we need some paths for updating the commandline interface15:49
jammgz: did you get a chance to look at upgrader-api-client?16:04
mgzjam, yeah, commenting now16:04
mgzjam: lgtmed16:07
=== wedgwood is now known as wedgwood_away
=== wedgwood_away is now known as wedgwood
=== wedgwood is now known as wedgwood_away
=== wedgwood_away is now known as wedgwood
thumpermorning folks20:55
thumpersmall victory21:29
* thumper chucks some stuff away22:08
thumperharder to get it merged than I care about22:08
thumpernot worth the effort right now22:08
* thumper feels like he is missing a step somewhere23:21
* thumper is unblocked23:43
thumperI hate, *hate* having to guess why something works...23:44
* thumper adds some doc23:44

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!