/srv/irclogs.ubuntu.com/2012/07/18/#juju-dev.txt

TheMueMorning06:18
fwereade_morning TheMue, davecheney, rog06:39
davecheneyhowdy06:39
* fwereade_ looks back at davecheney's conversation06:42
fwereade_davecheney, interesting, sorry to lead you astray on /initialized06:43
davecheneyfwereade_: no harm done06:43
fwereade_davecheney, I always saw it as meaning "if this node does not exist, do not trust the state, because it is wrong"06:43
davecheneytwo commits today, so ain't nothing going to bring me down :)06:43
fwereade_davecheney, sweet :)06:43
* TheMue would like to take his whole family with him to Lisbon. It's raining cats and dogs here today.06:55
TheMueMaybe I should try a summer2012 *= -106:56
rogTheMue, fwereade_: mornin'!07:25
TheMuerog: Heya07:27
fwereade_rog, heyhey07:39
davecheneyrog: can I ask you a question about gnu flags ?09:14
rogdavecheney: ask away09:14
davecheneyi'll describe what I want to do, not what I am currently doing09:14
davecheneyto support multple output formatters09:14
davecheneyI have a map[string]func(io.Writer)09:14
davecheneywhich maps format names to a function that will format the output09:14
rogdavecheney: ok09:15
davecheneybut i'm having trouble satistfying the interface for whatever fs.Var takes09:16
davecheneywhich needs Set() and String()09:16
davecheneyideally i'd like to avoid having to make a struct { name string render func() } for each map entry09:16
rogdavecheney: you want a flag that selects the format?09:16
davecheneyjust so we knew it's name09:16
davecheneyyes09:16
davecheneythat defaults to yaml09:17
davecheneyrog: please hold, pasting09:17
davecheneyrog: http://paste.ubuntu.com/1097972/09:17
davecheneythis may not work exactly, i've been hacking on it09:17
Arammoin.09:18
* davecheney waves09:18
rogdavecheney: so your difficulty is that you don't know how to define the String method on the formatterVar?09:19
davecheneyrog: not without having to pass a struct with the name of the formatter around09:20
davecheneyI was trying to keep it lean using only a function09:20
rogdavecheney: why not just do: type formatterVar string; and then index into the formatter map at a later stage?09:20
rogdavecheney: or have another method on formatterVar that returns the function.09:21
davecheneythe latter might be good09:21
rogdavecheney: by looking up into the map.09:21
fwereade_davecheney, take a look at cmd/jujuc/server/output.go09:21
rogdavecheney: all this stuff is identical to the standard flag package BTW09:21
davecheneyyeah, i've been looking in cmd/cmd and cmd/jujud for hints09:21
davecheneygive me a sec to play09:22
davecheneythnks for your suggestions09:22
fwereade_davecheney, the file I suggested should have ~exactly what you need already implemented09:22
davecheneyfwereade_: sweeeeeeeeeeeet09:22
rogfwereade_: i *thought* i'd seen it before!09:22
fwereade_:D09:23
davecheneyfwereade_: that is _exactly_ what I want09:23
* davecheney smells refactoring09:23
fwereade_davecheney, awesome :)09:23
TheMueAram: Moin09:24
davecheneyfwereade_: can I move this to cmd/ ?09:24
fwereade_davecheney, please do :)09:24
davecheneyas it will be used by several cmds ?09:24
fwereade_davecheney, SGTM, you may want to write a couple of explicit tests for it09:25
davecheneyfwereade_: will do09:25
fwereade_davecheney, beware the --test flag, which we probably don;t want on juju status09:27
fwereade_davecheney, but you should probably keep --output, pointless though it may be, in the service of interface stability09:28
davecheneyfwereade_: in that case, may I move c.out.testmode out of the formatter09:36
davecheneyas test mode is only used by the jujuc/server types09:37
Aramfwereade_: there's a problem with multiple testing suites in mstate :).09:39
Aramstarting a replicated mongod takes 30 seconds, so tests take 200 seconds just to bootstrap!09:40
AramI tried to make mongod start faster but ti can't be done.09:40
fwereade_Aram, whoa :)09:40
fwereade_davecheney, SGTM09:40
fwereade_Aram, you *should* be able to do something like we do in testing09:41
fwereade_Aram, so at least you share a mongo between tests in every package09:41
Aramthat's the plan I had in mind.09:41
davecheneylbox propose over celular connection will probably be a fail09:41
fwereade_Aram, and surely you can run most tests without a replicated mongod?09:41
Aramthat's another plan I had in mind.09:42
fwereade_Aram, STM that running most of them without replication, but the actual mstate suite with, would be pretty plausible09:43
davecheneyfwereade_: rog: https://codereview.appspot.com/642604309:45
fwereade_davecheney, could we not have testMode on ClientContext please?09:46
davecheneyactaully, please hold, still massaging09:46
davecheneyfwereade_: indeed, i moved it there09:47
fwereade_davecheney, emphasis on "not"09:47
davecheneyit's in server.go, i'll move it into context.go09:47
fwereade_davecheney, the commands do embed a ClientContext but I don't think the ClientContext is "commandy"09:47
davecheneyfwereade_: ahh, my mistake, i misinterprited you09:48
fwereade_davecheney, and I *think* that not every ClientContext-embedder actually wants that flag09:48
davecheneyonly config-get and unit-get care about this09:48
davecheneyi'll add it explicitly to them09:48
fwereade_davecheney, yeah, there will be more but you don't need to worry about them yet09:48
fwereade_davecheney, I'm worrying about how I'm going to make all these things work enough for everybody ;p09:49
davecheneyfwereade_: PTAL09:54
fwereade_davecheney, LGTM modulo tests :)09:57
davecheneyadding some testage to TestCommand now09:58
davecheneyfwereade_: rog aram: comments most welcome on tests "{\"Juju\":1,\"Puppet\":false}\n"10:45
davecheneyerr, "{\"Juju\":1,\"Puppet\":false}\n"10:45
davecheneyhttps://codereview.appspot.com/642604310:45
rogdavecheney: does that test pass consistently?10:48
davecheneyrog: yup10:48
davecheneyi use a struct for the test data so I don't get screwed by map ordering10:48
rogdavecheney: oh yeah, it's struct not a map, duh10:48
davecheneyrog: don't worry, i made that mistake10:48
davecheneyi added a comment to warn othrs10:49
davecheneyothers10:49
rogdavecheney: i wonder whether instead of specifying the entire error output, it might be better to make a regexp that just includes the most important bits.10:50
davecheneyrog: yeah, i'll make that change10:51
rogdavecheney: e.g. `usage.*invalid value \"xml\" for flag.*`10:51
davecheneythat was overkill10:51
rogdavecheney: actually i think it may have to be (.|\n)*10:51
davecheneyAssert( Equals ) is already a regex right ?)10:52
rogdavecheney: also, perhaps use a more-obviously unrecognised name than "xml"?10:52
rogdavecheney: nope, use Assert(Matches)10:52
rogdavecheney: it's just plausible we may add xml support in the future, i guess10:52
davecheneyrog: i can invent a more implausible output format10:53
rogdavecheney: i'm sure you can :-)10:53
rogdavecheney: you've got a review11:06
davecheneythanks rog11:08
davecheneyrog: Assert Matche is having trouble with a multi line message11:13
davecheneymatches11:13
rogdavecheney: did you use (.|\n)* ?11:14
davecheneyno ...11:14
rogdavecheney: i think you need to, as . doesn't match \n11:14
rogTheMue: i wonder if another way of testing the internals of the firewaller might be to hook into the logger.11:15
davecheneythat go it11:15
davecheneythanks11:15
rogTheMue: you could make a little type with an Output method that grepped for certain strings.11:16
rogTheMue: then there would be no need to have unsafe access to firewaller internals11:16
davecheneyrog: holy crap11:26
davecheneyif you include a CL url in the body of lbox commit message11:26
davecheneyit appends it to the old review11:27
davecheneyfarq!11:27
rogdavecheney: you mean lbox submit ?11:27
davecheneyno, look what happened here11:27
davecheneyhttps://codereview.appspot.com/6405046#msg1411:27
rog*sigh* random emails are going missing, and i don't know why11:28
rogdavecheney: in particular, i never saw the Submitted email, so i thought i was replying to an active review11:29
davecheneyyeah, that happens a lot11:29
rogdavecheney: i wonder why that message came out yellow11:29
davecheneybut I use my gmail for reviews11:30
davecheneywhich is more reliable11:30
rogdavecheney: me too11:30
rogdavecheney: i use gmail for everything11:30
rogdavecheney: and it worked fine before11:30
rogdavecheney: i verified that it wasn't any of my filters misbehaving, and it's not going into Spam.11:30
davecheneyrog: https://codereview.appspot.com/6416044/11:30
davecheneyrog: email totally crapped out last week, thursday/friday11:31
davecheney24+ hours delays11:31
davecheneymaybe this is a continuation11:31
rogdavecheney: email from launchpad was always very slow, but it got there in the end.11:31
rogdavecheney: these days, i'm not sure i'm seeing any email from launchpad at all.11:31
davecheneyweird11:33
TheMuerog: Instead of logging I also could add a chan of interface{} and send testing data to a goroutine where it's simply added to a slice. Additionally I can send funcs to that goroutine which iterate and type switch over the collected data.11:50
rogTheMue: i like the logging approach because it doesn't make the core code do anything unusual11:57
rogTheMue: there's no need to be sending to a chan in the normal code flow11:57
rogTheMue: and having a log.Debugf call would look quite normal11:58
rogTheMue: you could parse the messages trivially, with fmt.Scanf11:58
TheMuerog: Have to think about. I'm not such a big fan of string-stream-data-fetching. ;)12:02
TheMuerog: Thx for your review. How do we handle ZK outages in other parts?12:28
rogTheMue: i think some tests close the underlying zk and test the behaviour12:29
rogTheMue: or perhaps the underlying state.12:29
rogTheMue: it's not easy, but i think it's worth doing as much of the logic is about the error paths, so it's important that we get it right.12:29
TheMuerog: Btw, the better handling of a dying machine units watcher is in the next branch (goes in today). It hasn't been complete so far.12:30
rogTheMue: i'm thinking about the handling of a dying machine watcher in this case12:31
TheMuerog: I can follow your motivation but I'm not sure how you would do it.12:31
rogTheMue: here's a thought: currently the firewaller opens its own state. how about we change the signature to NewFirewaller(*state.State)?12:32
rogTheMue: then we can pass in a state that we have a handle to, and can close that and see what happens12:33
rogTheMue: in fact, that enables both the PA and the firewaller to use the same state object, which is probably a good thing.12:33
TheMuerog: Would be ok for me, yes.12:34
TheMuerog: Could you add this as a review comment so that Gustavo could take a look at that idea too?12:35
rogTheMue: done12:36
TheMuerog: thx12:36
TheMuerog: I'll now finalize the follow-up branch as proposal and then return to the initial one12:37
rogTheMue: sounds good12:37
rogTheMue: here's one way of doing the log-based tests. i think it works quite naturally, but it may be crack! http://paste.ubuntu.com/1098250/12:57
rogfwereade: what do you think of the above approach?12:57
* fwereade reads12:57
* fwereade isn't sure13:00
fwereaderog, I think it's decent scaffolding but bad testing if you see what I mean13:01
fwereaderog, it will help us verify behaviour as we build up the firewaller13:01
rogfwereade: that's the idea13:01
fwereaderog, but I think that once we can test the whole thing in terms of dummy Operations we should trash it13:01
rogfwereade: i agree13:01
fwereaderog, I think we're on the same page then :)13:02
rogfwereade: i just wanted something that was unintrusive but reliable13:02
rogfwereade: the current approach of inspecting the firewaller's locals from another goroutine seems icky to me13:02
rogTheMue: hey, i just saw your CloseState method again13:03
rogTheMue: that approach would be fine, i think.13:03
fwereaderog, yep, definitely13:04
rogTheMue: maybe we don't need more error checking currently in fact13:05
TheMuerog: Eh, sorry, somehow I'm lost.13:06
rogTheMue: lost where?13:07
TheMuerog: From the paste above (which I'm reading now and so maybe answered too quick) and the last statement about CloseState and "don't need more".13:07
rogTheMue: the paste above is orthogonal to the error testing issue that i was just referring to13:08
rogTheMue: sorry, two things without sufficient disambiguation!13:09
rogTheMue: i'm suggesting the paste above as a way to avoid the need for the thread-unsafe AllMachines methods.13:09
TheMuerog: ah, ok13:09
rogTheMue: it's the kind of thing i was referring to earlier when i talked about hooking into the log messages13:10
TheMuerog: yes, ic, your approach is nice. i only have to add some more debug statements then.13:13
rogTheMue: exactly13:13
rogTheMue: cool, glad you like it13:13
rogTheMue: if you like, i'll submit it as another CL after yours is done.13:14
rogTheMue: you could leave a "TODO: thread safe testing" or somesuch comment lying around13:14
TheMuerog: can do so, but I can also add it myself and update the proposal13:15
rogTheMue: ok, that's probably easier tbh13:15
rogbiab13:16
TheMuerog: I'm just testing the follow-up branch, then I can start with it13:16
TheMueniemeyer: morning13:16
niemeyerGooood mornings13:19
rogniemeyer: yo!13:19
niemeyerrog: Heya!13:21
rogniemeyer: in https://bugs.launchpad.net/bugs/1017732, you say "The two first parameters are only useful for InferCharm" but there's not currently a function by that name. did you mean InferURL?13:40
niemeyerrog: Yeah13:42
rogniemeyer: ok. next branch up, i'll propose a fix. i want to make the argument to juju.Conn.Deploy a charm.URL13:43
rogniemeyer: which means i want InferRepository as you suggest it13:44
niemeyerrog: Sounds great, cheers13:44
rogfwereade: sound ok to you? i'll assign the bug to me if you've not already got a fix in progress.13:45
niemeyerTheMue: ping13:47
fwereaderog, sorry, was having a bite to eat, forgot lunch, reading back13:53
fwereadeniemeyer, heyhey13:53
rogfwereade: np13:53
fwereaderog, go for it :)13:54
rogfwereade: done13:54
* fwereade cheers13:55
TheMueniemeyer: pong13:55
TheMueniemeyer: seen your +1 an will re-propose it later13:55
niemeyerTheMue: I'm just sending a few other notes13:55
niemeyerTheMue: Can you please see if you'd like to talk about something there?13:55
TheMueniemeyer: thx13:55
niemeyerfwereade: Heya!13:56
fwereadeniemeyer, heyhey13:58
fwereadeniemeyer, how's it going?13:58
niemeyerfwereade: Smoothly!14:01
niemeyer:)14:01
fwereadeniemeyer, sweet :D14:01
TheMueniemeyer: Only one question regarding your first comment. You would prefer a stopping of the firewall using the typical way?14:03
niemeyerTheMue: Hmm14:04
niemeyerTheMue: You know what, I think the implementation is right14:04
niemeyerTheMue: It's the comment that is wrong14:04
niemeyerTheMue: "can't stop machine tracker: %v"14:05
niemeyerTheMue: The machine tracker *was* stopped, if we got there14:05
niemeyerTheMue: Ah, and it should also delete the machine from the map14:06
TheMueniemeyer: this is done below with delete. the error is the the result of a stopping error. that's why I log "can't stop the tracker". the machine has been removed, but the tracker showed an error during stopping.14:10
TheMueniemeyer: maybe better "stopping machine tracker %d showed errer %v"14:11
niemeyerTheMue: See the comment in the CL14:11
TheMues/errer/error/14:11
niemeyerTheMue: If you 'continue', the delete is never reached14:11
niemeyerTheMue: Yes, but "can't stop" is incorrect14:12
niemeyerTheMue: It won't get there unless it is stopped14:12
TheMueniemeyer: iiirks, the continue, you're right14:13
TheMueniemeyer: any good idea for the logging message? or shall i take that "stopping machine tracker %d: %v" thingy?14:15
niemeyerTheMue: Have you seen the CL comments?14:16
niemeyerTheMue: There's a suggestion there14:16
TheMueniemeyer: didn't refreshed screen while we're typing here14:17
TheMueniemeyer: rietveld needs auto-refresh. ;)14:18
niemeyerTheMue: Yeah :)14:18
niemeyerTheMue: What about the channel closing?14:20
TheMueniemeyer: you've seen rogs comment? it's the firewallers channel, only passed to the tracker. I could move it up. it can only be closed by the firewaller. so my fw.finish() has to take care to first stop the machine trackers befor closing that channel.14:22
niemeyerTheMue: Hold on tight there14:23
niemeyer:)14:23
niemeyerTheMue: The question was pretty simple14:23
niemeyerTheMue: What happens if the channel is closed?14:23
niemeyerTheMue: The sentence should start with "When it is closed, ..." :-)14:23
TheMueniemeyer: "When it is closed, no tracker should be running."14:24
niemeyerTheMue: Okay, that sounds perfect, thanks14:24
TheMueniemeyer: ;)14:24
niemeyerTheMue: We need to have that idea clear in there14:25
TheMueniemeyer: Yes, and I have an idea how to ensure it, based on rogs suggestion14:26
TheMueniemeyer: the next CL will have it, it's a simple one14:27
TheMueniemeyer: btw, maybe you could help me14:27
niemeyerTheMue: Right now it's a puzzle that is being carefully put together, not easy to assess whether it is right, and very easy to screw up in the next change14:27
niemeyerTheMue: If nothing else, we must document how to not screw up14:27
TheMueniemeyer: I know change sets and their acronym CS, but what is the L in CL?14:27
niemeyerTheMue: change list, I don't know where it comes from, to be honest14:28
TheMueniemeyer: ah, thought so but not have been sure14:28
niemeyerTheMue: In fact, I think we have a problem in there14:29
TheMueniemeyer: I think the next propose will it make more clear14:29
TheMueniemeyer: fw.loop() has a defer fw.finish()14:30
TheMueniemeyer: there first all trackers are stopped14:30
niemeyerTheMue: Ah, no, the problem I imagined just now doesn't exist14:30
TheMueniemeyer: and then the channels closed (possible by making them to firewaller fields).14:30
niemeyerTheMue: Btw, why do we have to wait for the firewaller.tomb.Dying within the machineTracker?14:32
TheMueniemeyer: I think that's a clear tear down. and by referencing fw.xyzChan in the trackers it is more clear that the change is sent back to be handled there14:32
niemeyerTheMue: Ah, yes, that sounds good14:33
niemeyerTheMue: You mean removing the defers and moving them into the finish method, right?14:33
TheMueniemeyer: yes14:33
niemeyerTheMue:+1!14:33
TheMueniemeyer: which line you're referring to with the wait?14:34
niemeyerTheMue: Hmm..14:34
niemeyerTheMue: So why do we even close the machineUnitsChanges channel?14:34
niemeyerTheMue: I don't think we have to close it.. there's no benefit, apparently14:35
TheMueniemeyer: you mean it's ok to let the GC close them?14:35
niemeyerTheMue: It's just introducing a race we have to be aware of14:35
niemeyerTheMue: No, the GC doesn't close channels14:35
niemeyerTheMue: Ever14:35
niemeyerTheMue: The GC just collects them, once they are unused14:35
niemeyerTheMue: It's fine to leave a channel unclosed14:36
niemeyerTheMue: If it makes sense for the intended workflow14:36
TheMueniemeyer: yeah, thought a bit about it.14:37
TheMueniemeyer: most important is a proper closing of the trackers bottom up.14:38
niemeyerTheMue: So, +1 on moving stuff onto finish, +1 on dropping the unnecessary close of the channel too14:38
niemeyerTheMue: So, going back a bit, the line I was referring to is 132 and 14014:39
fwereadeniemeyer, quickly, please confirm: a hook is allowed to set the running unit's relation state for any relation the unit is in... right?14:43
fwereadeniemeyer, s/running/local/ is perhaps clearer14:43
niemeyerfwereade: Yes14:44
fwereadeniemeyer, jolly good, thanks :)14:44
niemeyerfwereade: There are defaults, but yes, it is allowed14:44
fwereadeniemeyer, yep14:44
TheMueniemeyer: thinking about it. don't know anymore *argh*14:44
fwereadeniemeyer, just wanted to check I was following the python correctly14:44
TheMueniemeyer: currently see now reason14:44
niemeyerfwereade: Now that I think of it, I'm not sure if Python ever did this correctly, since we figured those details as we went through it15:00
niemeyerfwereade: I've recently seen bug reports addressing precisely this issue, IIRC15:01
niemeyerfwereade: May have been fixed by now15:01
fwereadeniemeyer, hmmmmm :/15:01
fwereadeniemeyer, ok, I'll take a look :)15:01
niemeyerfwereade: Nothing to worry about, I think.. it was just not something we focused a lot on as we got the basic semantics right in the first go15:01
fwereadeniemeyer, ah, jolly good15:02
niemeyerfwereade: Your feeling of "all hooks should be able to see and touch this" is the right one15:02
fwereadeniemeyer, still worth double checking what went wrong so I don;t make the same mistake15:02
niemeyerfwereade: Good point15:02
fwereadeniemeyer, all my mistakes will be new and fresh :)15:02
niemeyerfwereade: LOL15:03
niemeyerfwereade: Fresh mistakes just leaving the oven are always so tasty, though!15:03
* fwereade runs his tummy15:04
fwereadeer, rubs15:04
fwereadeniemeyer, cool, nothing looks likely to be a serious problem15:06
TheMueSo, switching branches15:07
rogniemeyer: https://codereview.appspot.com/643004415:28
rogniemeyer: should be quite trivial15:29
niemeyerrog: Clean and sweet15:35
rogniemeyer: thanks!15:37
niemeyerrog: Btw, if you add -bug to lbox propose, it will link the bug to the proposal15:37
rogniemeyer: ah, thank you for reminding of that.15:38
niemeyerrog: and assign to you, and put it in progress, but hopefully those two should be done ahead of time (or with -wip)15:38
rogniemeyer: presumably then lbox submit should mark the bug as "fix committed" ?15:41
niemeyerThat's lunch time!15:58
rogfwereade, niemeyer: i'm thinking of a signature something like this for juju.Conn.Deploy: http://paste.ubuntu.com/1098537/16:07
rogfwereade: what d'ya think?16:07
fwereaderog, looks reasonable, I think16:09
fwereaderog, the charm is the potentially-fuzzy charm name from the command line, yes?16:09
rogfwereade: yeah16:09
fwereaderog, yeah, that looks right, I think16:09
rogfwereade: i was wondering about using charm.URL, but i think better to keep the interface simpler, i think.16:10
rogfwereade: i don't think it loses any generality16:10
fwereaderog, I don't think you could even if you wanted, could you?16:11
rogfwereade: no?16:11
fwereaderog, we don't necessarily know the charm URL until we've messed around using potentially both state's default-series and the LocalRepoPath16:11
rogfwereade: i was going to assume that the caller would invoke charm.InferURL16:12
rogfwereade: (for which you don't need LocalRepoPath, incidentally)16:12
fwereaderog, good point; but they *do* need State16:13
rogfwereade: really?16:13
rogfwereade: i didn't think InferURL used State16:13
fwereaderog, where are you going to get default-series from?16:13
rogfwereade: ah!16:13
rogfwereade: an excellent point sir16:14
* fwereade bows graciously16:14
TheMuerog: integrated your log testing, works like a charm16:16
rogTheMue: cool. i hoped you'd like it!16:16
TheMuerog: definitely16:16
rogTheMue: see, string-stream-data-fetching can be nice sometimes... :-)16:17
fwereaderog, I have a naming problem16:17
rogfwereade: god, me too16:17
fwereaderog, I'm writing a HookQueue16:17
rog:-)16:17
TheMuerog: well said16:17
rogfwereade: ok16:18
fwereaderog, and I basically want to pop hook executions in 2 stages16:18
TheMuerog: the test-by-string-stram-data-fetching-and-comparing-idiom16:18
fwereaderog, I'd like to call the operations Peek and Pop, except Peek is the thing that returns something and we should panic if we Pop without havng Peeked16:19
fwereaderog, that I want to do this at all *may* be evidence of total insanity on my part in the first place, but I think it'll all work out rather nicely16:19
rogfwereade: you could make the Pop operation work on the thing just peeked i suppose16:19
fwereaderog, it actually has to16:20
rogfwereade: why do you need a peek operation BTW?16:20
rogfwereade: perhaps you could paste a brief outline of your basic design intentions here16:20
fwereaderog, I'll give it a god16:20
rogfwereade: i've got a spare god if you need one16:21
fwereaderog, wait, balls, thought of a problem, need to think a sec16:21
fwereaderog, ah no it's ok16:22
fwereaderog, OK16:22
fwereaderog, the idea is that the HookQueue is a magical change-collapsing queue16:22
fwereaderog, we feed it RelationUnitsChange events16:22
rogfwereade: yeah, i figured something of the kind16:22
fwereaderog, and it decomposes them into individual hook executions16:23
rogfwereade: i still don't quite see how Peek fits in though16:23
fwereaderog, of which there should only be one of joined/changed/departed per unit in the queue at a time16:23
fwereaderog, the context is kinda necessary I think16:23
rogfwereade: also, Get is probably a better name for a queue than Pop, which implies a stack to me.16:23
fwereaderog, yeah, good point, bad connotations16:24
fwereaderog, ok, anyway16:24
fwereaderog, we always can collapse two potential hooks16:24
fwereaderog, if we get a changed on top of a changed, we can just ignore it16:25
rogfwereade: yup16:25
fwereaderog, if we get a departed on top of a changed, we drop the changed and append the departed, etc16:25
rogfwereade: uh huh16:25
fwereaderog, so, this is nice16:25
rogfwereade: ah, i think i see the need for Peek now16:26
fwereaderog, but I would like to be able to maintain all the hook queue state in one place16:26
fwereaderog, ie the HookQueue saves its state after every change, so it can reload itself if we restart16:26
rogfwereade: is that actually necessary?16:27
fwereaderog, I think so...16:27
fwereaderog, something needs to know what state we think we're in16:27
TheMuerog: propose is in again16:28
rogfwereade: so your HookQueue kinda corresponds to my "intentions" except that you save all the intentions at every step16:28
fwereaderog, and I think the HookQueue has basically all of it that we need16:28
fwereaderog, kind of; but an intention is not all we need to save, is it?16:28
rogfwereade: i'm not sure. what else do we need to save?16:29
fwereaderog, we need to keep track of relation membership and settings versions so far as the unit currently knows16:29
rogfwereade: ok, yes, in my original sketch, the intention was saved along with the version that corresponded to that intention16:31
fwereaderog, where do you keep the versions that don't correspond to a queued intention?16:31
rogfwereade: do they matter?16:31
fwereaderog, yes, we don't want to run a change hook for every unit whenever we bounce the process16:32
fwereaderog, how else do we reconcile latest state with saved state and know which changes are worth hooking?16:32
rogfwereade: we've already run hooks for those units, right?16:33
rogfwereade: so there will be an intention stored in the log with a version number16:33
fwereaderog, ah, ok, the intentions log grows without bound?16:33
rogfwereade: (it will be also be tagged as "done" in the log)16:33
rogfwereade: not without bound - we can collapse it occasionally16:34
rogfwereade: like a log-structured filesystem16:34
rogfwereade: we'd probably collapse it when we restart and if it grows bigger than some bound16:34
fwereaderog, hmm, how do we collapse the intentions we don't actually need to run?16:35
rogfwereade: each intention has some tag associated with it (originally i thought the zk path, but we don't have access to that) - we collapse intentions with the same tag.16:36
rogfwereade: it's quite possible that this scheme is entirely crackful, but i'd like to have a go to see if it is possible, because i *think* it could work nicely.16:37
fwereaderog, I don't see how we do all this collapsing without writing it all the time, though16:37
rogfwereade: it also potentially scales reasonably well too.16:37
fwereaderog, not to mention marking them as done16:38
rogfwereade: we *are* writing it all the time16:38
fwereaderog, ah, I thought you were claiming there was something different about what we were doing, and that my approach was a problem because I was writing all the time16:39
rogfwereade: the collapsing happens occasionally, by reading through the entire log16:39
rogfwereade: no16:39
rogfwereade: the problem i have is that your approach writes *everything* all the time16:39
rogfwereade: if the system grows quite big, there are going to be many many units and many many changes...16:40
fwereaderog, ...and a vast log file to scan through and write "done" in the middle of...?16:40
rogfwereade: we never write into the middle of the log file16:41
rogfwereade: it's append-only16:41
fwereaderog, ah, ok, that does make more sense16:41
rogfwereade: presumably in your approach you'd do something like alternate two files, writing the HookQueue alternately to each one?16:43
fwereaderog, basically yeah16:43
fwereaderog, I'm still trying to figure out how to do the constant event collapsing with the log approach16:43
fwereaderog, if we're lagging at all behind "now", which I imagine we will in a big system, how do we strip unwanted intentions from the log?16:44
rogfwereade: the hook queue event collapsing?16:45
fwereaderog, we add both "done"s and "whoops-don't-bother"s?16:45
rogfwereade: or the log collapsing?16:45
fwereaderog, the hook queue16:45
fwereaderog, the log collapsing doesn't sound too challenging16:45
rogfwereade: i'm thinking that the hook queue doesn't have any persistence at all16:45
rogfwereade: s/have/need/16:45
fwereaderog, so we just run loads more hooks?16:45
rogfwereade: no, the hook queue does event collapsing16:46
rogfwereade: but i don't think it needs to save its state16:46
fwereaderog, so it collapses an event, writes an intention, and then when that intention is no longer valid it...16:46
fwereaderog, or it doesn't write the intention until it's actually about to run the hook?16:47
rogfwereade: the way i'd thought about it is that the hook queue is running in a separate goroutine. it receives events, collapses them into a queue, and tries to send hooks down a channel to the hook executor16:47
rogfwereade: the hook executor is the thing that writes the intentions, when it's read the hooks from the hook queue16:48
fwereaderog, so the hook queue needs to read the intentions as well to know how to collapse?16:48
fwereaderog, how does it cancel a "joined" intention for example?16:49
fwereaderog, I think the second bit is the more interesting question, sorryt16:49
rogfwereade: the hook queue starts with our current knowledge of the state of the world16:49
fwereaderog, sorry, side question: how many not-done intentions will be in the log file at a time?16:50
rogfwereade: we've found that out by reading the log file at startup16:50
rogfwereade: one or two16:50
rogfwereade: any sequence of actions that corresponds to a single state change (i.e. version change)16:51
rogfwereade: so the first two intentions would be "install" and "start", presumably16:51
fwereaderog, ok, and this intention-storing is per-relation?16:51
fwereadewho ok no16:51
fwereadewhoa^^16:51
fwereaderog, ok, we have a constant stream of hooks coming from a bunch of places in separate streams, some of which need to be paused for arbitrary chunks of time16:53
rogfwereade: the intention storing is per hook execution - we're storing the "intention" to execute a hook, based on some database change16:53
fwereaderog, the relationship between database changes and hook executions is not simple16:53
fwereaderog, your hook executor will need ot know basicaly everything about the state of the system, won't it?16:53
rogfwereade: ah, well that may well be a killer for this approach :-)16:54
rogfwereade: i was kinda assuming that a single database change triggers a single hook execution16:54
fwereaderog, 0-2 in general, I think, and the changes come from lots of different places16:54
rogfwereade: ok... let's enumerate16:55
rogfwereade: 0?16:55
fwereaderog, and 0s may look like 1s without reference to past state, and 1s may look like 0s without reference to future state16:55
fwereaderog, change on change16:55
fwereaderog, the first change shouldn't actually happen16:56
rogfwereade: ah, i don't care about the number of hook executions a db change triggers16:56
rogfwereade: but...16:56
fwereaderog, what hooks needs to be executed depend on future state16:56
rogfwereade: is there ever a time when a hook execution depends on *more than one* db change?16:56
fwereaderog, in terms of deciding whether or not to run it, yes16:57
rogfwereade: how do you mean?16:57
fwereaderog, we can have a joined queued and then decide not to bother because we got a departed16:57
rogfwereade: ok, i see that. but i don't *think* that matters.16:58
fwereaderog, the fact that the system state changed does not automatically imply that a hook should be run, but we don't know the truth of that until later16:58
rogfwereade: the idea is that everything in the hook queue is "fuzzy might wanna do it stuff". but when you get something *out* of the queue, you are really going to try and execute it regardless16:59
rogfwereade: at some point, we have to commit to an intention16:59
rogfwereade: which is what the intentions log is supposed to mirror16:59
fwereaderog, hmm, ok, I might be able to see how it could work if we had one per relation17:00
rogfwereade: why one per relation?17:01
fwereaderog, what about the hooks we run and need to store the context of for an arbitrarily long time until we run them again?17:01
fwereaderog, while running other hooks in the interim?17:01
rogfwereade: how do you mean?17:01
fwereaderog, resolved17:01
rogfwereade: how does resolved work?17:02
fwereaderog, hook runs and errors; we pause the hook queue for that relation until the user resolves it, and may or may not want to retry the hook17:02
rogfwereade: do any extra hooks get run as a result of this?17:02
fwereaderog, you have a stored intention for the changed, after the joined that just failed17:03
fwereaderog, you cannot run that hook until the joined is resolved one way or another17:03
rogfwereade: how does the user decide whether or not to retry the hook?17:03
fwereaderog, --retry-hook or something17:03
rogfwereade: well, you don't mark an intention as done until it's completed without error.17:04
fwereaderog, ok, so intentions execute out of order?17:04
fwereaderog, or, alternatively, you have an intention log per relation, and one for the whole unit17:05
rogfwereade: hmm. i see, i thought a hook execution error paused execution of *all* hooks17:05
fwereaderog, just because one relation is screwed doesn't mean we should take *everything* down17:05
rogfwereade: hmm, seems slightly odd to me - the screwed-uppedness could be to do with the central working of the charm17:06
rogfwereade: but anyway, that's how it is17:06
fwereaderog, in that case, all the relations fall over, but they do so independently :)17:06
niemeyer<rog> fwereade, niemeyer: i'm thinking of a signature something like this for juju.Conn.Deploy: http://paste.ubuntu.com/1098537/17:06
niemeyerrog: That's mixing concerns17:06
niemeyerrog: Deploy is about, well, deploying17:06
rogniemeyer: ok, that's why i was askin'!17:06
fwereaderog, I have to go I'm afrad17:06
niemeyerrog: And I'm answering!17:07
rogfwereade: me too. talk tomorrow. i'll keep thinking about it.17:07
niemeyer!? :-)17:07
rogniemeyer: thanks!17:07
rogniemeyer: what concerns is it mixing?17:07
niemeyerrog: Deploy is about deploying.. it shouldnt' be resolving URLs IMO17:07
rogniemeyer: fwereade made a good point earlier17:07
rogniemeyer: i don't think we can resolve the url without a state17:08
rogniemeyer: i guess we could add juju.Conn.InferURL17:08
niemeyerrog: Yep.. that's going to be an issue with other methods too17:09
fwereaderog, hmm, really gtgt, but: 3 operations really -- PublishCharm, AddService, AddUnits17:09
rogfwereade: yes, that sounds reasonable.17:10
rogfwereade: i'm kinda trying to avoid mirroring all the state types in juju. perhaps juju should be a more transparent layering.17:11
niemeyerrog: I'm not sure if InferURL is even the best interface in that specific case.. let me paste you the original point in the original CL17:12
rogfwereade: func Deploy(*state.State, *charm.URL) *state.Service17:12
niemeyerrog: http://pastebin.ubuntu.com/1098633/17:12
rogniemeyer: how do we get the charm URL without getting the default series from the state?17:13
niemeyerrog: I don't understand why you'd want to do that17:14
niemeyerrog: What's the problem with getting default-series from the state?17:14
rogniemeyer: it's an argument to InferURL17:14
niemeyerrog: That's where it lives..17:14
niemeyerrog: Yes.. let's use it.. ? :)17:14
rogniemeyer: right, so juju.Conn should expose its State field17:15
rogniemeyer: which i actually think is a good idea17:15
niemeyerrog: Why?17:15
rogniemeyer: otherwise we'll have to open another state just to get the default series, no?17:15
niemeyerrog: Not that I think it is a big problem.. it's already exposed, but I don't really get why exposing it or not is relevant in this case17:15
niemeyerrog: I'm pretty lost17:16
niemeyerrog: Conn has a State.. PutCharm is in Conn.. Deploy is in Conn17:16
rogniemeyer: PutCharm takes a charm.URL as an argument17:16
niemeyerrog: Ah, sure.. we can have InferURL in Conn too.. is that what you're concerned with?17:16
rogniemeyer: the canonical way of getting a charm.URL is by calling charm.InferURL17:17
rogniemeyer: yes!17:17
niemeyerrog: Phew.. ok..17:17
rog[18:08:26] <rog> niemeyer: i guess we could add juju.Conn.InferURL17:17
niemeyerrog: Either way sounds fine.. Conn.State exists today.. Conn.InferURL sounds fine too17:17
rogniemeyer: to be honest, i prefer the idea of the juju package as a relatively transparent layer atop Environ and State17:17
niemeyerrog: i'd prefer to not change what the juju package is today17:18
rogniemeyer: i don't intend to do that17:18
niemeyerrog: Conn looks great as it is17:18
niemeyerrog: It's working, implemented, and tested17:18
niemeyerrog: Let's solve the problem we have at hand and move on, IMO17:18
rogniemeyer: just that it means that we don't necessarily have to provide a totally complete layer on top of state17:18
niemeyerrog: We're not doing that17:19
rogniemeyer: so we can allow clients to, for example, get the default series from the state17:19
niemeyerrog: Look at the state interface, and look at the Conn interface17:19
niemeyerrog: They are pretty different, and seem to be going exactly in the direction we designed them to be17:19
rogniemeyer: yeah, but what i'm worried about is having types in juju mirroring the ones in state, e.g. juju.Service, juju.Relation etc17:19
niemeyerrog: We don't have that today, and I don't think we should have that.. why would we?17:20
rogniemeyer: i'm wondering how juju.Conn.SetRelation would work, for example17:20
rogniemeyer: how do we phrase the arguments to it?17:20
niemeyerrog: SetRelation?  What's that?17:20
rogniemeyer: sorry, RelationSet17:21
niemeyerrog: What's that?17:21
rogniemeyer: erk17:21
niemeyerrog: AddRelation?17:21
rogniemeyer: ok, Set17:21
niemeyerrog: Sorry, I'm lost.. those names don't mean anything to me17:22
rogsorry17:22
rogniemeyer: the juju set command17:22
rogniemeyer: equivalent in juju pkg would probably be juju.Conn.Set17:22
niemeyerrog: Okay, thanks17:23
roghmm, i guess we can just use the svc name17:23
niemeyerrog: Yeah, or a *state.Service17:23
rogniemeyer: i was wondering if we'd return a *state.Service from juju.Conn.Deploy; then juju.Conn.Set could take a service17:24
rogniemeyer: yeah17:24
rogniemeyer: that's the kind of thing i mean by a "more transparent layer" - we're not entirely hiding the fact that state is under the surface17:24
niemeyerrog: I never thought of Conn like that17:25
rogniemeyer: cool17:25
rogniemeyer: then i'm happy17:25
niemeyerrog: We have State in there, after all17:25
rogniemeyer: yeah... i guess i'd thought of that mainly for testing purposes17:25
niemeyerrog: It's an API that allows us to poke into both the environment and the state together.. a facade if you will17:25
rogniemeyer: yeah, sounds good17:26
rogniemeyer: a collection of higher level operations17:26
niemeyerYeah17:26
rogniemeyer: ok, so Deploy(*charm.URL) it is. that's lovely.17:27
rogniemeyer: and it's funny we both came up with *almost* exactly the same sig for Deploy. (i'd totally forgotten about your remark in that CL!)17:27
niemeyerrog: I think the suggested interface solves the issue better, but you'll probably figure as you implement it17:29
rogniemeyer: what do you see as the main differences, besides the URL resolving stuff?17:30
niemeyerrog: Deploying a charm may be done many times, with charms that are already in the state17:30
niemeyerrog: That does not involve any kind of URL resolution or inferring17:31
niemeyerrog: Putting a charm in the state can also be done by itself, independently17:31
rogniemeyer: yeah, i'm with you. i was wondering about other differences.17:31
rogniemeyer: (hence "besides" the URL resolving stuff)17:32
rogniemeyer: i've gotta go, but i'll see what you write, later.17:32
rogniemeyer: have a great rest-of-day and evening17:32
niemeyerrog: That's really it.. it seems to make more sense to deploy a *state.Charm than a *charm.URL, in Conn17:32
niemeyerrog: Cheers17:33
rogniemeyer: ok cool17:33
rogniemeyer: guess i'll see you in lisbon next!17:33
niemeyerrog: For you too17:33
rogniemeyer: hope your interminable travel time isn't too bad17:33
niemeyerrog: Generally not an issue.. there's so much to do with quiet time :)17:33
niemeyerrog: Thanks, though17:34
niemeyerrog: Have a good time yourself17:34
niemeyer"Are you uncertain, or hoping to convince me through Socratic dialogue?"18:59
niemeyerfwereade_: LOL18:59
niemeyerGiving a ride to Ale.. back in 1519:43
niemeyerfwereade_: ping20:39
hazmatyes20:51
niemeyerhazmat: I like your thinking21:05
hazmatecontext wrong channel21:08
niemeyerhazmat: The positiveness is welcome no matter what :-)21:09
hazmatawesome21:09
davecheneyniemeyer: thanks for the reviews22:22
davecheneyat some point today the workmen in my house will need to turn off the power22:23
davecheneyso i'll go into the city and work from there22:23
niemeyerdavecheney: np, and thanks for the note22:56
fwereade_niemeyer, pong22:57
niemeyerfwereade_: Heya22:57
fwereade_niemeyer, heyhey22:57
niemeyerI think you've got it all in your inbox already :)22:57
fwereade_niemeyer, cool :)22:58
fwereade_niemeyer, I see nothing on watch-presence-children?22:59
fwereade_niemeyer, thank you for the others though, and I think I like the RelationHandler suggestion23:00
fwereade_niemeyer, need a little time to absorb it though :)23:01
fwereade_niemeyer, I should probably do it while I sleep, though :)23:01
fwereade_niemeyer, nn23:01
niemeyerfwereade_: Sounds like a good plan! :-)23:03
niemeyerfwereade_: Have a good night23:03
fwereade_niemeyer, and you :)23:03
fwereade_niemeyer, hm, I have just had a thought about RelationHandler23:07
fwereade_niemeyer, no, it's not well-formed23:08
fwereade_niemeyer, really going now ;)23:08
niemeyerfwereade_: Cheers! ;)23:08

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