[01:17] niemeyer: you make a good point [01:17] For context, [01:17] http://codereview.appspot.com/6497057/ is my proposal [01:17] I suspect it's a bit more involved than that [01:17] As the comment suggests, we should only push secrets when they are not yet set, and should push *only* secrets [01:17] should we take this to the chhanel ? [01:17] Sure [01:17] niemeyer: what happenes currently is AllAttrs - SecretAttrs are pushed on bootstrap [01:17] so, with 6497057 [01:18] the set of attrs not pushed would only be the secrets [01:18] but this is only by coincidence, not design [01:18] davechen1y: Hmm.. I'm not sure I understand what that means [01:18] davechen1y: That's what have been aiming at [01:18] * davechen1y finds code [01:19] davechen1y: Also, it's not true that we currently push AllAttrs-SecretAttrs.. it will be so once wrtp's branch goes in with the fix, though [01:19] niemeyer: hmm, there are unit tests that check this [01:20] certainly up to the point that they are yaml encoded and passed to cloudinit.Config [01:20] davechen1y: If we have unittests for this, they're broken or not being run [01:20] davechen1y: https://codereview.appspot.com/6500052/diff/2001/environs/ec2/ec2.go [01:20] davechen1y: Line 132 of the left-hand side [01:24] niemeyer: http://codereview.appspot.com/6458161/patch/17001/15007 [01:30] davechen1y: Sure, it works because updateSecrets is broken, right? [01:30] davechen1y: The configuration being tested there is after updateSecrets has set everything over agian [01:30] sent [01:31] niemeyer: yes, you are correct [01:32] niemeyer: http://codereview.appspot.com/6458161/diff/17001/state/state_test.go?column_width=80, rhs, like ~ 88 [01:32] that doesn't go through the conn [01:32] so the problem must be the handoff to jujud bootstrap-state via cloudinit [01:32] davechen1y: I'm not entirely sure about what you're trying to show me.. the test contains the full configuration.. [01:33] davechen1y: The problem is in the exact line I've shown you above, and wrtp's branch fixes it [01:34] cool, i'll wait for it to lang [01:34] land [01:36] davechen1y: You can also use it as a pre-req if you'd like to build on it [01:36] davechen1y: It's already approved [01:36] sweet, will do [02:11] OK: 1 passed [02:11] PASS [02:11] ok launchpad.net/juju-core/mstate/presence 1.111s [02:11] !!!!! [02:11] On that note, I'm heading to bed as tomorrow will be an early day [02:11] * davecheney opens the champaign [02:11] davecheney: I wish I could enjoy it with you :) [02:12] TO LISBON! [02:12] davecheney: Oh yeah! [02:12] MOAR PORT WINE [02:12] SO TRUE [02:12] 8) [02:12] 'davecheney: Have a great working day, and will see you in a bit! [02:12] will do [02:12] * niemeyer takes off [03:02] common LP, you can do it [03:02] % lbox propose [03:02] error: Failed to load data for project "juju-core": Get https://api.launchpad.net/devel/juju-core: unexpected EOF [04:59] davecheney: mornin' [05:15] wrtp: hody! [05:15] can you commit your bootstrap branch [05:15] so I can unfuck my cross branch that requiresit [05:15] * davecheney hates prereqs so much [05:16] davecheney: sorry for stepping on your toes with the provisioner fix - i'd done the fix before i checked the bug and found it was InProgress [05:16] davecheney: am just about to submit [05:16] wrtp: no appology needed [05:16] don't care, it's fixed :) [05:16] :-) [05:16] i have a secrets only branch to follow [05:16] davecheney: it was great that you'd added the last "happens in the wild" comment to the bug report [05:17] davecheney: 'cos otherwise i wouldn't have found the other firewaller bug [05:17] wrtp: also, nice use of the watcher inside the provisioner test [05:18] davecheney: it was a little more involved than perhaps it should be, but i hate fuckin' timeouts [05:19] davecheney: we have far too many tests that rely on waiting for arbitrary time periods [05:20] wrtp: 110% agree, see my whinge about https://bugs.launchpad.net/juju-core/+bug/1037421 [05:20] davecheney: yeah. [05:20] davecheney: i wonder if we should standardise on using debug log messages to determine "actively doing nothing" [05:21] davecheney: because that's usually what we're testing when we timeout [05:21] wrtp: not sure, like it says in the ticket, i haven't looked into _why_ this happens at all [05:21] davecheney: presence may be different [05:22] davecheney: i looked at the timing of the presence tests BTW, and it's not that there's one test that's slow - there are many slow tests there. [05:22] wrtp: 0% cpu usage while it is waiting, i smell timeout [05:22] davecheney: definitely [05:27] davecheney: just doing final test after merging trunk, then will submit [05:27] kk [05:27] davecheney: the other thing to make tests faster: it would be great if all the tests could use the same state server. [05:29] wrtp: i cant' see how we could do that in zk, but with mongo probably [05:30] davecheney: apparently it is possible in zk (you can do the equivalent of chroot) but mongo is more important now. [05:30] davecheney: submitted [05:31] wrtp: i guess with mgo it would be some sort of unique prefix on the table names [05:32] davecheney: i guess. [05:32] davecheney: BTW do you think that https://codereview.appspot.com/6499056/ is superficial enough to submit without niemeyer's LGTM? [05:33] wrtp: you have my permission to throw me under the bus if niemeyer objects [05:33] i've been submitting build fixes without waiting [05:34] davecheney: i didn't see any buses in lisbon [05:34] you never say LONGO BUS ? [05:34] davecheney: oh yeah, one thing: there was a test in firewaller_test that looked like a debugging remnant. see line 81 of https://codereview.appspot.com/6499056/diff/5001/worker/firewaller/firewaller_test.go [05:35] davecheney: i left it in, but was it there for a particular reason? [05:35] wrtp: dunno, i didn't write it (i think) [05:35] davecheney: actually i came from the airport on a bus. you may be in danger. [05:35] davecheney: frank thought you did actually [05:35] s/say/saw [05:35] davecheney: that bit of it anyway [05:36] wrtp: fuck it, it's called refactoring [05:36] davecheney: yeah. i didn't want to leave out tests that actually checked functionality. [05:36] davecheney: but that just tests that the instance has been started, which we test elsewhere [05:38] # launchpad.net/juju-core/juju [05:38] ./conn.go:95: env.Get undefined (type *config.Config has no field or method Get) [05:38] ./conn.go:96: env.Set undefined (type *config.Config has no field or method Set) [05:38] ./conn.go:99: env.Write undefined (type *config.Config has no field or method Write) [05:38] sadface [05:38] i was using those [05:44] davecheney: we could add Get. [05:45] davecheney: i don't think the other two are appropriate though [05:45] wrtp: what CL changed this [05:45] i'll get my learn on [05:45] davecheney: in fact, i wonder if the lack of Write is a problem [05:45] davecheney: it's been going to happen for ages [05:45] davecheney: frank did it at niemeyer's request [05:49] davecheney: right, you're officially at risk [05:52] wrtp: jolly good [06:12] wrtp: any idea why this happens ? [06:12] ubuntu@server-15347:~/go/src/pkg/net/http$ godoc launchpad.net/juju-core/environs/ec2 [06:12] PACKAGE [06:12] package ec2 import "launchpad.net/juju-core/environs/ec2" [06:12] interesting [06:12] tested from a clean checkout [06:13] davecheney: yes, happens for me too [06:14] davecheney: but works fine on dummy, for example [06:14] how odd [06:15] * wrtp refuses to get distracted :-) [06:17] wrtp: i'll lot a bug to figure out WTF that is happening [06:28] can I test the waters with this CL, http://codereview.appspot.com/6492065/ [06:29] I was playing on canonistack today [06:29] it is very possible to do a three line install of juju from source from scratch [06:34] has anyone else run into this issue btw, https://bugs.launchpad.net/juju-core/+bug/1044164 [06:43] davecheney: i think that's a good idea [06:43] davecheney: i think it's a pity that go get doesn't have a way of asking for testing deps too [06:44] wrtp: it's kind of a hack [06:44] but it works [06:44] and is reasonably self contained [06:45] davecheney: given the current limitations of go get, i think it's reasonable. [06:45] davecheney: i've fixed the issue you had above BTW [06:45] davecheney: (it happens all the time for me) [06:45] godoc ? [06:45] or AWS [06:45] davecheney: sig mismatch [06:46] do I need to update goamz ? [06:46] davecheney: i can't remember if i submitted or not [06:47] davecheney: hmm, maybe i never even proposed it [06:48] davecheney: ha, i never even committed the change! [06:48] davecheney: short story: goamz is totally broken currently [06:48] davecheney: fix: add / to the urls [06:48] * davecheney facepalm [06:49] ah yes [06:49] bucket.s3.amazon.com is a host [06:49] bucket.s3.amazon.com/ is a url [06:49] or something like that [06:50] davecheney: it signs the path, which should be "/" but is "" [06:50] right [06:50] the root of a URL is always tricky [06:50] well it's not tricky [06:50] it's very simple [06:50] but somehow always catches things out [06:51] davecheney: URLs are fundamentally broken [06:51] wrtp: no argument there [06:51] davecheney: we just hobble around the brokenness [06:51] davecheney: any sequence of slashes should be a separator [06:51] davecheney: like in unix paths [06:52] davecheney: mind you, even in unix it's a bit broken: i think cat /etc/passwd/ should work [06:52] wrtp: not sure I can follow you there [06:52] is a collection [06:53] /index is a resource [06:53] / is a collection [06:53] davecheney: i don't understand that [06:53] put it this way [06:54] / is a dir [06:54] actually never mind [06:54] iw as talking cack [06:55] davecheney: the model becomes really simple if trailing slash (or several) make no semantic difference. [06:55] davecheney: with root being a special case of course, so that we get relative paths. [06:57] wrtp: what does cat /etc/ do ? [06:58] davecheney: depends on the system [06:58] :) [06:58] you've used solaris [06:58] davecheney: once upon a time it gave you the contents of the dir in binary format [06:58] davecheney: (it still does in plan 9, and i think that's ok) [06:58] wrtp: i kinda agree [06:58] davecheney: but the point is {operation /etc/} is the same as {operation /etc} [06:59] i'm not sure if I want /etc and /etc/ to *be* the same thing [06:59] davecheney: they are [06:59] well currently /etc/ isn't, it's an error [06:59] davecheney: no it's not - it's just fine [06:59] davecheney: ls /etc/ [06:59] % ls /etc/ | md5sum [07:00] 910833b8b809292bd85de24a9536df74 - [07:00] % ls /etc | md5sum [07:00] 910833b8b809292bd85de24a9536df74 - [07:00] but then we get to the issue of ls /etc/passwd/ [07:00] davecheney: /etc/passwd/ is just an error because something in the kernel is trying to be clever [07:00] i try not to be clever, to many things have gone wrong [07:00] davecheney: or probably because of the way the namec works internally [07:00] s/the/that/ [07:01] hurry up ec2 [07:01] if it's still called namec any more [07:01] i wanna propose and get to my friends leaving drinks [07:01] namec is the function ? [07:01] in plan 9 you could do cat /etc/passwd/ if /etc/passwd existed :-) [07:02] davecheney: yeah, it's the kernel name lookup function, or used to be [07:02] davecheney: probably not under linux [07:02] damn open sores [07:02] i may well be remembering very badly :-) [07:08] davecheney: my memory is indeed faulty. it's namei. http://minnie.tuhs.org/cgi-bin/utree.pl?file=V7/usr/sys/sys/nami.c [07:09] later lads [07:09] davecheney: cheerio [08:32] morning [08:45] moin. [09:09] Aram: hi [09:31] Aram: how are the watchers doing? [09:31] I ma fighting bugs. [09:33] Aram: if i can help you let me know. [09:33] Aram: i'm interested in how they are done now. so any paste is welcome. [09:33] ok, let me bzr push [09:34] Aram, TheMue, fwereade__: morning [09:34] yo. [09:34] wrtp, hey [09:34] wrtp: good morning, too [09:34] hey everyone else too :) [09:34] fwereade__: and also you ;) [09:34] i've actually been around for over 4 hours this morning, just forgot to say hello earlier! === wrtp is now known as rogpeppe [09:35] TheMue: lp:~aramh/juju-core/62-mstate-watchers-mux [09:35] look in watchserver.go [09:35] there are bugs, but that's the general idea [09:35] Aram: great, thx [09:39] some types will change, e.g. will use a chan []genericDoc instead of chan genericDoc, some loops will change from m x n to n x m etc. [09:39] but now I am debugging the thing first. [09:40] ah, there will be e generic unmarshalling function using reflection [09:42] rogpeppe: I forgot how can I run a single test. [09:42] Aram: -gocheck.f pattern [09:42] rogpeppe: thanks, how do I find this information if I need it again? [09:43] Aram: run the test binary with -help [09:43] ah yes, I was running go test itself with -help [09:44] for standard tests my editor scans the file and provides a shortcut to a menu where all or individual tests can be run. that's nice. [09:49] TheMue: do you remember talking about the possibility of using a single watcher for the EnvironConfig for both provisioner and firewaller? [09:49] TheMue: it looks like it should be ok to me, but i'm not entirely sure. [09:49] btw, in the new watcher implementation, there is a single real watcher. a watch server demultiplexer, so to speak. [09:49] it's only one of those per session. [09:50] rogpeppe: yeah, we once talked about it. i don't remember anymore why we chosen watchers for each. [09:50] Aram: that sounds like a good move [09:50] it gets the data and distributes it to interested parties [09:50] Aram: i was hoping for something like that [09:50] Aram: in this case though, it's more about saving code than watchers per se [09:50] rogpeppe: they should reference the same environ, so that one SetConfig() should be enought. [09:50] TheMue: exactly [09:50] rogpeppe: but there are TWO "should" in that sentence ;) [09:51] TheMue: i think i'll do it. shouldn't be hard. and it removes some moving parts (and hence tests) from the firewaller and the provisioner. [09:52] rogpeppe: +1 [09:52] func NewProvisioner(environ environs.Environ, st *state.State) *Provisioner [09:52] rogpeppe: it simply sounds more logical [09:52] fwereade__, Aram: does that sound reasonable to you? [09:53] * fwereade__ reads back for context [09:53] fwereade__, Aram: using a single shared environs.Environ for the various workers, and having a separate process watching it and updating it when necessary. [09:53] rogpeppe, +1 [09:53] lgtm [10:00] Aram: i'm no friend of the reflection based unmarshal idea. ok, it may save a few lines, but we only talk about four internal docs, so your actual way seems fine to me. [10:29] yes! the fix I dreamt last night was correct! [10:29] dreaming fixes has happened before, but not in a long time. [10:30] I tried it in the morning, and thought it didn't work, but my fix was wrong because the zero value of interface{} is nil, not 0. [10:30] but what I have dreamt was correct :). [10:31] Aram, nice :) [10:33] "panic: interface conversion: interface is int, not mstate.Life" [10:33] TheMue: ^ ^ this is the reason I want to use reflection, not because it produces less code. [10:34] but because the code is necessarily correct and doesn't break when you change the data model. [10:34] conversely, I just spent all frickin' morning delving deeeeep into the guts of the charm manager, trying to figure out what's going on, and I *just* realised it was a poorly merged test :( [10:38] fwereade__: my fault? [10:39] rogpeppe, nope, 100% mine [10:39] fwereade__: my commiserations [10:39] fwereade__: i hate it when that happens [10:39] YES! everything works. [10:39] fwereade__: the bright side is you usually come out with a better understanding of everything goingon [10:39] fixed the damn thing. [10:39] fwereade__: and probably made some fixes while you were doing it [10:40] rogpeppe, sadly in this case all I have done is come up with a series of ever-more-batshit theories about how git works, and I fear they may pollute my brain for a while ;) [10:40] fwereade__: oh well [10:40] TheMue: you might want to pull again, to see working watchers. [10:40] tests pass now [10:41] * fwereade__ cheers at Aram [10:41] it's by far the most complex piece of machinery in mstate, sadly. [10:42] but I am happy with it from many perspectives. [10:52] Aram: great [10:52] Aram: and yes, the server is a little monster ;) [10:52] Aram: which piece is this? [10:52] rogpeppe: lp:~aramh/juju-core/62-mstate-watchers-mux [10:53] Aram: i will have a look :-) [10:53] watchserver.go is the real thing, watcher.go is just a quick hack to be able to validate the design. [10:53] environ watchers ripped out of firewaller and provisioner. all tests pass (some new tests needed though). am happy. [10:54] Aram: btw, I'm just doing the EnvironConfig change for mstate [10:54] TheMue: what change? [10:54] rogpeppe: great., i hope https://codereview.appspot.com/6488057/ will then still run too [10:55] Aram: state.EnvironConfig now returns an environs/config.Config and there's an additional SetEnvironConfig [10:55] ok. [10:55] Aram: test is already working here [11:01] have we got a meeting now? [11:02] waiting on gustavo and mramm [11:02] davecheney: just a quick query [11:02] I'm on my way [11:02] Morning all [11:02] Do we have invites sent yet? [11:02] not yet [11:02] that i see [11:02] rogpeppe:mmm [11:02] inviting everybody now [11:03] davecheney: i decided that it would be a good thing if the provisioner didn't listen for environ config changes itself [11:03] davecheney: so something else listens and changes the environ; everyone uses that environ [11:03] davecheney: does that sound reasonable to you? [11:03] rogpeppe: why the reason for the change ? [11:04] davecheney: i was going to have to have exactly the same code in three places [11:04] it sounds like my beloved proxy environ idea :) [11:04] davecheney: no need for a proxy i think [11:04] https://plus.google.com/hangouts/_/4ca5fa037ece34849caa79686808bc9e75cfae19 [11:04] ah, meeting today? [11:05] Aram: yup [11:06] https://launchpad.net/juju-core/+milestone/1.3 [11:06] fwereade__: meeting? [11:06] rogpeppe, whoops, ty === Aram2 is now known as Aram [11:48] Aram: https://wiki.canonical.com/UbuntuEngineering/Sprints/JujuCoreIntegration [11:58] * Aram goes to lunch [12:00] lunchtime [12:06] case change, ok := <-fw.environWatcher.Changes(): [12:06] err := fw.environ.SetConfig(change) [12:07] niemeyer: http://paste.ubuntu.com/1177699/ === ChanServ changed the topic of #juju-dev to: Milestone goals: 1) mstate in, state out 2) uniter working with simple charms; 3) upgrade-juju working [12:22] night guys [12:35] rogpeppe: http://pastebin.ubuntu.com/1177736/ [12:46] rogpeppe: fw.environ = environ [12:46] rogpeppe: worker.SetEnvironConfig(fw.environ, config) [12:49] rogpeppe: https://codereview.appspot.com/6488057/diff/1/worker/provisioner/provisioner_test.go [12:52] rogpeppe: case fw.environTest <- fw.environ: // do nothing [12:53] rogpeppe: someworker.SetEnvironWatcher [12:54] rogpeppe: someworker.SetEnvironObserver [12:54] rogpeppe: someworker.SetEnvironObserver(ch) [12:54] rogpeppe: case fw.environObserver <- fw.environ: // do nothing [13:05] niemeyer: http://paste.ubuntu.com/1177767/ [13:11] niemeyer: perhaps you could mention to TheMue the thoughts you had on the worker tests, re: the above provisioner_test.go CL. [13:11] rogpeppe: Will do [13:14] Will get some breakfast first, though! [13:49] niemeyer: are we concerned about what might happen if an upgrade is done concurrently with an environment setting change? [13:49] rogpeppe: In which sense? [13:49] niemeyer: would it be acceptable for the upgrade request to be lost, for example? [13:49] (in that case) [13:51] rogpeppe: Are you alluding to the race in SetEnvironConfig? [13:51] niemeyer: yeah. [13:51] rogpeppe: Not a problem we have to worry for a while, IMO [13:51] niemeyer: well, the race that happens when you must do: EnvironConfig followed by SetEnvironConfig. [13:51] rogpeppe: yeah [13:51] niemeyer: ok, i thought so, but just wanted to check explicitly [13:52] niemeyer: thanks [13:52] rogpeppe: We should fix that at some point, probably by having a method that enables applying deltas safely, but this is a nice problem for us to have when things actually work [13:52] rogpeppe: In practice, fiddling with the env is rare enough that it won't be an issue anytime soon [13:53] niemeyer: definite agreement there. and fiddling with the env *concurrently* is even rarer. [13:53] rogpeppe: Yeha [14:00] niemeyer, before I polish this properly, can I get another pre-review sanity check on the charm upgrades please? https://codereview.appspot.com/6488062 [14:02] fwereade__: Sure [14:02] fwereade__: I'll just finish something and will be with you [14:02] niemeyer, cheers [14:21] Flights booked [14:29] fwereade__: Haven't reviewed it entirely, but sent some pre-review comments [14:29] niemeyer, thanks, that's what I was after -- and sorry to keep bugging you with this one [14:30] fwereade__: It still looks like you're fighting with the awkwardness of having the charm manager handling responsibilities which ought to be elsewhere [14:31] niemeyer, it seems excessive to me to have to keep *another* state file around to allow the uniter to keep track of what operation it's performing [14:32] niemeyer, it feels to me that being able to determine restart state purely from charm state and hook state is a good thing [14:32] niemeyer, charm state + hook state + uniter state feels wrong -- especially since IMO charm state and hook state are useful simplifying features [14:32] niemeyer, but once we're dealing with uniter state as well they actually become redundant and make life more complex [14:33] niemeyer, or have I misunderstod something? [14:34] niemeyer, whereas just making sure that the charm op and the hook op overlap STM like the clearest way to ensure that the appropriate ops are followed by appropriate hooks [14:34] / charm directory, it returns ErrConflict. cbSuccess will be called before [14:34] / status is set from st back to Deployed, to give clients the opportunity [14:34] / to complete dependent actions. [14:34] fwereade__: This is a hack [14:35] fwereade__: This is a charm.Manager.. [14:35] fwereade__: It has deployed the charm [14:35] fwereade__: There are no "dependent actions" [14:35] fwereade__: It has done it [14:35] fwereade__: No conflicts, no problems.. it's finished its job [14:35] fwereade__: It doesn't depend on anything else to say it has finished its job either [14:36] fwereade__: We can talk about different ways in which we can solve that, but first thing is to understand what we're doing and why [14:37] niemeyer, ok -- what I am trying to do is ensure that a charm operation is always followed by the apropriate hook [14:38] niemeyer, ISTM that the two ways to do that are to overlap the operations, so the charm op ends after the hook op has begun; or to add *another* layer of state that's specific to the uniter, which feels redundant [14:39] fwereade__: So what you mean is that you'd rather do the whole upgrade again than acknowledge the fact we can tell the merging has been done? [14:40] niemeyer, er, I don't think that's what I'm saying at all [14:40] fwereade__: That's what's in the code [14:40] fwereade__: If cbSuccess fails, we never set the status to deployed, despite the fact it *is deployed* [14:41] niemeyer, so what? same thing happens if writing "Deployed" fails [14:41] fwereade__: So what you mean is that you'd rather do the whole upgrade again than acknowledge the fact we can tell the merging has been done? [14:42] niemeyer, are yu telling me I should be parsing git logs or something? [14:42] fwereade__: No.. I'm telling that we shouldn't have a cbSuccess that prevents us from acknowledging the merge has been done, for purposes that are completely external to the concerns of the Manager [14:43] fwereade__: Divide and conquer.. the Manager is doing a lot [14:43] fwereade__: We should have a pleasant API to work with, that makes sense by itself [14:43] fwereade__: I don't buy that we should be breaking abstractions and injecting that kind of weirdness because we can save a file from being written to disk elsewhere [14:45] niemeyer, I am trying, but I don't see the middle ground between what I suggest and dropping independent charm and hook state entirely [14:47] niemeyer, if I've got a super-state that tells me how to interpret charm and hook state, doesn't it end up noticeably simpler to simply write every operation out in just one place? ...but then we lose many of the benefits of separating out /charm and /hook [14:47] niemeyer, or...... hum, I think I see [14:48] niemeyer, the uniter state is basically independent of hook state and exists purely to manage the transition from charm op to associated hook? [14:49] fwereade__: Possibly. I'm not even entirely sure (yet) we need an extra state, to be honest [14:50] niemeyer, ok -- the problem I am trying to avoid is: write-charm; set-deployed; whoops-we-crashed-before-run-hook [14:50] fwereade__: If the uniter decides it needs to upgrade, what happens within the uniter itself? [14:50] fwereade__: Yeah, I kind of see it.. I just don't have as good a view on the details as you do [14:51] fwereade__: The basic semantics I imagine for the manager, in isolation is: write-charm-and-url, set-deployed [14:52] fwereade__: If there is a crash, the uniter should have a mechanism that tells it was going to over a charm upgrade.. it will try to write the charm again, and the manager will go "Ah, don't worry, that's what I have" [14:53]