/srv/irclogs.ubuntu.com/2012/03/14/#juju-dev.txt

=== TheMue_ is now known as TheMue
=== asavu_ is now known as asavu
=== asavu_ is now known as asavu
=== asavu_ is now known as asavu
=== asavu_ is now known as asavu
fwereade_wrtp, are you free for a vague rambling discussion about hook contexts?09:44
* wrtp enjoys vague rambling discussions.09:45
wrtpfwereade_: certainly09:45
wrtpmornin' BTW09:45
fwereade_wrtp, ok: I think I've figured out how Context should look, but I'm not sure what path I should take to get there09:46
fwereade_wrtp, mornin' :)09:46
wrtpfwereade_: what general direction are you aiming in?09:47
fwereade_wrtp, I'm pretty sure that we only need a single `type Context struct`, with methods that map 1:1 to hook commands, which is cool09:47
wrtpfwereade_: ah, the Context that was called Env, right?09:47
fwereade_wrtp, yeah09:47
wrtpfwereade_: so this is from the agent's perspective?09:48
fwereade_wrtp, (I retain a feeling that it's not insane to have an Env interface and a Context interface, just for clarity of testing, but that's by the by_09:48
fwereade_wrtp, yeah09:48
fwereade_wrtp, it seems that the way to do that is to have fields for local unit, unit relation, relation members, and remote unit09:49
wrtpi'm not sure i understand09:49
fwereade_wrtp, and simply to error sensibly when someone asks for a capability we cannot provide09:49
wrtpoh, i see, i think.09:50
wrtpyou think it's possible to make all hook commands context insensitive.09:50
fwereade_wrtp, so RelationSet, for example, just won't work if we don't have a .relation09:50
fwereade_wrtp, it sounds like that's on the roadmap09:51
fwereade_wrtp, and thinking about it from that perspective leads to IMO a nicer implementation right now09:51
wrtpin which case, yeah, i could see that working. each field determines the behaviour of one or more callback methods.09:52
fwereade_wrtp, exactly09:52
fwereade_wrtp, ...hm ok I think I now know what I need to do in the exec branch09:53
wrtpfwereade_: in fact, the callback methods need not be defined on the Context.09:53
fwereade_wrtp, my original plan was not to have them on the context09:53
fwereade_wrtp, I think it works out nicer if they are though09:53
wrtpfwereade_: you can still use your approach of "remote command execution" if you want to.09:53
fwereade_wrtp, yep, that's almost entirely orthogonal09:54
wrtpfwereade_: yeah09:54
wrtpfwereade_: but i think it would look nicer if they were there, from an implementation perspective. they wouldn't need to be exported.09:54
fwereade_wrtp, the only difference is that JUJU_CONTEXT_ID maps back to a context which already has some of those fields filled in09:54
fwereade_wrtp, and we need to be able to optionally override which state we're working with on the command line09:55
wrtpfwereade_: so the agent proactively fills in those fields rather than wait for the callback before reacting?09:56
wrtpi'm not sure i understand your last remark09:56
fwereade_wrtp, sorry, it's somewhat speculative09:57
wrtpwhat's "state" here?09:57
fwereade_wrtp, for now it's not necessary: it's just so we have a path to `relation-set $SOME_RELATION` foo-bar09:57
fwereade_wrtp, state in that case is $SOME_RELATION09:57
fwereade_wrtp, foo=bar^^09:58
wrtphmm, still at sea09:59
fwereade_wrtp, it's far enough in the future that I can't predict exactly how it will look: for now, pre-filled contexts are exactly what we need09:59
fwereade_wrtp, sorry, I'll try to build it up a bit more logically09:59
wrtpfwereade_: i'd like to understand your speculation...09:59
fwereade_wrtp, proximate goal: given a known, er "platonic" context for a given hook, we can construct a Context that knows how to get to the correct state.Unit/Service/UnitRelation in order to extract or manipulate it10:01
fwereade_wrtp, we can then implement Commands that just have the appropriate Context set on creation, and which can manipulate the context directly, concerning themselves only with input/output handling10:02
wrtpfwereade_: the Commands are agent-side?10:04
fwereade_wrtp, ultimate goal: make the Commands capable of handling additional args that allow them to construct their own Contexts10:04
fwereade_wrtp, yes10:04
wrtphrmm10:05
fwereade_wrtp, that's not something I'm offering a *plan* for at the moment10:06
wrtpi don't see why they'd need to do that.10:06
fwereade_wrtp, it's just a door I don't want to close10:06
wrtpfwereade_: can't you ensure that there's always a context for a command?10:06
fwereade_wrtp, so that relation-set $SOME_RELATION can work out-of-band10:07
wrtpfwereade_: even out-of-band commands may have a context, no?10:07
wrtpfwereade_: (they've got to know how to talk to the agent, for a start)10:08
fwereade_wrtp, they do, but that context is essentially "what unit/relation are we working with here?"10:08
fwereade_wrtp, yep, so JUJU_AGENT_SOCKET may end up having to be global, IMO that's not an exceptionally big deal10:08
fwereade_wrtp, but I don't think we can know what relation relation-set wants to affect before it's called10:09
wrtpfwereade_: i think that is a big deal when we allow multiple unit agents in the same container10:09
fwereade_wrtp, huh, yeah, true10:10
fwereade_bah10:10
wrtpfwereade_: so given that, i think we can always assume JUJU_CONTEXT_ID, and hence some context.10:10
wrtpniemeyer: hiya10:11
fwereade_wrtp, I think they're different problems10:11
niemeyerHeya!10:12
niemeyerMornings10:12
fwereade_wrtp, out-of-band execution means we *can't* depend on JUJU_CONTEXT_ID10:12
fwereade_morning niemeyer10:12
wrtpfwereade_: i don't think it does10:12
wrtpfwereade_: environment variables get exported10:12
wrtpfwereade_: out-of-band doesn't mean no-relation-to.10:12
fwereade_wrtp, get exported to where? my understanding is that we eventually intend `relation-set foo bar=baz` to work even when called in response to some action taken by the service itself in an entirely unpredictable environment10:14
wrtpfwereade_: once a hook context has "expired" (when the hook has finished executing), we could make the old context id to a generic "out of band" context.10:14
wrtps/make the/map the/10:14
wrtpfwereade_: i don't think that can possibly work10:14
fwereade_wrtp, I don't see how that's any different to just not having a context id10:15
wrtpfwereade_: maybe it's not. JUJU_AGENT_SOCKET is the important thing. but...10:15
fwereade_wrtp, there's the how-do-we-talk-to-the-right-agent problem but that's probably soluble even if it doesn't turn out especially elegant10:16
wrtpfwereade_: perhaps we might want to make callbacks within the context of a given hook (even if the hook has completed), still have some hook-specific behaviour10:16
niemeyerfwereade_: <fwereade_> wrtp, out-of-band execution means we *can't* depend on JUJU_CONTEXT_ID10:17
niemeyerfwereade_: Kind of.. JUJU_CONTEXT_ID is what says it's out-of-band in the first place, right?10:17
fwereade_niemeyer, lack thereof, yes10:18
wrtpniemeyer: i think there are two kinds of out-of-band here10:18
wrtpniemeyer: 1) called after the hook has returned10:18
wrtpniemeyer: 2) called from an independent environment10:18
wrtpniemeyer: i'm thinking about 1), and i think we can rule out 2)10:19
fwereade_wrtp, in (1), the old hook context is potentially *wrong*, so I think that's the case we should rule out10:19
wrtpin 1), you've always got a JUJU_CONTEXT_ID10:19
fwereade_wrtp, ...which maps to a context with a potentially out-of-date member set10:19
wrtpfwereade_: at least we know when the context is wrong, and can do something appropriate10:19
fwereade_wrtp, how's thatany different to just not having a context and knowing we have to "do something appropriate"? ;)10:20
fwereade_wrtp, ie somehow obtain the correct context to work with10:20
wrtpfwereade_: because i imagine that the behaviour might differ depending on the original hook context.10:21
fwereade_wrtp, why?10:21
niemeyerwrtp: I don't understand what you mean by that10:21
niemeyerwrtp: 1 or 210:21
niemeyerfwereade_: Agree regarding the out-of-date member set10:22
niemeyerfwereade_: But, it's a bit of a tricky situation..10:22
wrtpok... here's a tangential question: which hook callbacks are not ok to call in which hooks?10:22
niemeyerfwereade_: Why is a command spawned in the background by a hook different from a command spawned in the background by something else?10:23
fwereade_wrtp, *at the moment* you can't do relation-set,-get,-list outside a relation hook10:23
wrtpfwereade_: ok, but we'd change that, right?10:23
wrtpfwereade_: 'cos it's silly if we allow them out-of-band but not in certain hooks10:24
fwereade_wrtp, and attempting a relation-set in a departed hook will error out (which I think we should also change -- no reason not to be able to set, even if nobody's going to read it)10:24
wrtpniemeyer: that's an easy answer - because the command spawned in the background by something else can't know what unit it's executing in10:24
fwereade_wrtp, yep, which is why this is a bitof a digression ;)10:24
fwereade_wrtp, but why should it care?10:24
fwereade_wrtp, ok, it should care because AGENT_SOCKET10:24
wrtpfwereade_: exactly10:25
wrtpfwereade_: and relation-get returns something different depending on what unit it's in10:25
niemeyerwrtp: A command spawned in the background isn't necessarily executing in any unit..10:25
wrtpniemeyer: no?10:25
fwereade_wrtp, indeed10:25
niemeyerwrtp: Sorry, bad terminology of my part10:25
niemeyerwrtp: What I mean is that it's not necessarily executing in any relation.. you can spawn a server, and have it executing relation-set reactively, for instance10:26
wrtpniemeyer: yeah, that seems fine.10:26
niemeyerIt shouldn't blow up just because it was started within a hook rather than by the init scripts10:27
niemeyerIn that sense, it's out-of-band as well, and it should work10:27
wrtpniemeyer: i think we're leading towards the idea that all the callbacks should work independently of whether they're called inside a hook or not.10:27
wrtpniemeyer: and all callbacks should be valid all the time.10:27
fwereade_wrtp, exactly so10:28
wrtpexcept...10:28
fwereade_wrtp, I hadn't picked up on this until I spoke to niemeyer ?yesterday?10:28
wrtpthat what about races10:28
wrtp?10:28
wrtpi.e. within a hook, you know that relation-get is consistent10:28
wrtpbut outside a hook, it might be constantly changing.10:29
wrtpha10:29
wrtpmaybe there could be some way of *asking* for a context id10:30
niemeyerwrtp: Indeed. We can allow the creation of contexts at some point, for providing the same guarantees a hook has, but that's a distant future. Commands don't even work out of hooks yet.10:30
wrtpJUJU_CONTEXT_ID=$(get-context-id)10:30
fwereade_wrtp, yeah, was just pondering same10:30
wrtpniemeyer: yeah, this all goes back to some current design decisions which were being influenced by this speculation...10:31
fwereade_wrtp, we'd need some way to retire/discard them as well, but details details10:31
niemeyerMight be intersting to marry context id and the socket10:31
niemeyerfwereade_: ^10:31
fwereade_niemeyer, hm, yeah10:32
niemeyerfwereade_: While we still have time :)10:32
wrtpniemeyer: i was thinking the same10:32
fwereade_niemeyer, I don't think that door is closed10:32
niemeyerfwereade_: JUJU_AGENT_SOCKET=<path>[@<n>]10:33
fwereade_niemeyer, neither context id nor agent socket are actually intrinsic to the context, I think10:33
fwereade_niemeyer, context id is needed to look up a context, but I think the only reason it's stored in the context is for convenience10:33
fwereade_niemeyer, (...when constructing the env vars for a hook)10:34
niemeyerfwereade_: Right10:34
wrtpJUJU_CONTEXT=<n>:path ?10:34
fwereade_niemeyer, similarly for the agent socket: they'll both need to be controlled by something other than the context itself10:34
niemeyerfwereade_: It's also provably not necessary right now, which means we can make it optional10:34
niemeyerfwereade_: Actually, now that I think of it, it's only optional because we don't have anything else but context-rich execution10:35
fwereade_niemeyer, yeah, I don;t want this to affect the implementation right now, but I don;t want to make assumptions that will make it *harder* to implement this when the time comes10:35
niemeyerfwereade_: So once we introduce out-of-band commands, context-rich execution needs the context info10:35
wrtpniemeyer: yes10:36
niemeyerfwereade_: I suggest taking the context stuff out10:36
niemeyerfwereade_: I mean, the context id, more specifically10:36
niemeyerfwereade_: Unless you're already making use of it in a way that moves us closer to these ideas we're talking about10:36
fwereade_niemeyer, so Exec takes a context id and an agent socket in addition to the context itself, which doesn't itself know about them?10:37
niemeyerfwereade_: Hmm.. context id and the agent socket are part of the context right now..10:37
fwereade_niemeyer, feels like a bit of a speculative interface complication to me... not sure it makes the putative changes any easier10:37
wrtpdon't we need a context id to stop inappropriate execution of hook commands after the hook has finished?10:38
niemeyerfwereade_: I was just suggesting dropping context id from the context, until we have a reason to hold it10:38
fwereade_niemeyer, OTOH having them on the context potentially encourages people to use them inappropriately10:38
fwereade_wrtp, we need it but it doesn't have to live on the context10:38
niemeyerfwereade_: What's "context" in this sentence.. it feels like we're talking about different things?10:38
fwereade_niemeyer, the type which (1) ultimately supplies env vars to Exec and (2) exposes useful methods to the hookcommand implementations10:40
fwereade_niemeyer, the actual execution environment the hook runs in needs a context id ATM, no argument there10:40
niemeyerfwereade_: Those feel like different things..10:40
fwereade_niemeyer, well, I thought so too, hence separation between Env and Context10:41
fwereade_niemeyer, but I'm starting to think otherwise10:41
niemeyerfwereade_: Regarding your second sentence above, ok, so let's keep ContextId in Context (perhaps calling it Id, though?)10:41
fwereade_niemeyer, yeah, that's how my current latest sketch looks10:41
niemeyerfwereade_: Cool10:42
wrtpfwereade_: hmm, i have to say i was quite convinced by our earlier discussion10:43
niemeyerfwereade_: Exec needs a type that it can use to execute a command.. this feels isolated from the aspect of hook commands themselves, at least as far as I can foresee at the moment10:43
wrtpfwereade_: i think Context works well combined with Env10:43
wrtptype Context struct {10:43
wrtpId int10:43
wrtpVars map[string]string10:43
wrtpRelations map[string]string10:43
wrtpUnit *Unit10:43
wrtp}10:43
wrtpor whatever10:43
wrtpbut... yeah10:44
fwereade_niemeyer, but it's true that as you suggested everything in Env can be derived from stuff we have in the context10:44
wrtpthe Env is independent, because we might have a context without running a hook10:44
fwereade_niemeyer, and there's not much justification for a separate type/interface10:44
niemeyerfwereade_: Right10:44
fwereade_niemeyer, except in that testing becomes a bit easier10:44
niemeyerfwereade_: I see two things, but they're not the same two things that were in the original branch10:44
wrtpfwereade_: that's an excellent point though10:45
fwereade_niemeyer, heh, I can't precisely remember the original branch now... I thought I had a Context that exposed Env()... and the Commands() thing is no longer really relevant to the conversation10:46
niemeyerfwereade_: It's quite possible as well that you had a perfect grand vision, and I just assumed things incorrectly from looking into part of it.10:46
niemeyerfwereade_: Context was lost in that branch. It may make sense for another concept, but it doesn't belong into that branch, or in that file even10:47
fwereade_niemeyer, definitely not perfect, our discussion the other day modified it a certain amount, but I think the broad strokes are similar enough10:47
niemeyerfwereade_: What we need is Exec and ExecVars (your Env, just avoiding the name conflict)10:48
fwereade_niemeyer, ah yes, it was just Env which I was semi-expecting the eventual Context type to implement, but wasn't yet sure about: hence just an easy-to-independently-test interface10:48
fwereade_niemeyer, I'm actually not so sure about that: I think we just need Context to have `Vars() []string` and leave it at that10:49
niemeyerfwereade_: I can easily buy into that too10:50
fwereade_niemeyer, whether we ultimately pass a Context or an []string to Exec depends on what makes sense in the final context10:50
niemeyerfwereade_: I don't think it makes sense to pass a context to Exec..10:51
fwereade_niemeyer, well, it needs Vars and a way to get to the actual hook, which IMO it's sensible to calculate from Context.CharmDir10:53
wrtpfwereade_: i think that both Context and Env can be concrete types.10:53
fwereade_niemeyer, we could ofc just have context take (vars []string, hook string) where hook is an absolute path claculated elsewhere10:53
niemeyerfwereade_: Agreed. This whole design may actually change a bit once we see the rest of what's necessary10:55
niemeyerfwereade_: It's strange to have CharmDir in Context, for example.10:55
fwereade_niemeyer, agreed, now you mention it :)10:56
niemeyerfwereade_: Many of those properties aren't properties of a context.. they are properties of a unit10:56
niemeyerfwereade_: So it feels a bit like we're trying to reverse engineer how the bottom looks like by looking at the tip10:57
niemeyerfwereade_: WHich IMO is why we're a bit lost in that back and forth debate10:57
fwereade_niemeyer, true... but I'm not sure there's such a thing as a context without a unit, even when out-of-band10:57
niemeyerfwereade_: There's no context without a unit.. but a unit has multiple contexts10:57
fwereade_niemeyer, true10:57
wrtphmm, i wonder what happened there10:58
wrtpfwereade_: i missed everything after "niemeyer, we could ofc just have context take (vars []string, hook string) where hook is an absolute path claculated elsewhere"10:58
wrtp(if there was anything)10:58
fwereade_<niemeyer> fwereade_: Agreed. This whole design may actually change a bit once we see the rest of what's necessary10:59
fwereade_ fwereade_: It's strange to have CharmDir in Context, for example.10:59
fwereade_<fwereade_> niemeyer, agreed, now you mention it :)10:59
fwereade_<niemeyer> fwereade_: Many of those properties aren't properties of a context.. they are properties of a unit10:59
fwereade_<-- wrtp has quit (Read error: Operation timed out)10:59
fwereade_<niemeyer> fwereade_: So it feels a bit like we're trying to reverse engineer how the bottom looks like by looking at the tip10:59
fwereade_ fwereade_: WHich IMO is why we're a bit lost in that back and forth debate10:59
fwereade_<fwereade_> niemeyer, true... but I'm not sure there's such a thing as a context without a unit, even when out-of-band10:59
fwereade_<niemeyer> fwereade_: There's no context without a unit.. but a unit has multiple contexts10:59
fwereade_<fwereade_> niemeyer, true10:59
niemeyerfwereade_: To solve the current debate and make some progress, I suggest we move on with you suggestion. Let's make Exec trivial.. Exec(vars []string, path) as you suggested.10:59
wrtpfwereade_: thanks10:59
fwereade_niemeyer, sounds good to me11:00
niemeyerfwereade_: I suspect the correct thing to do will be a lot more obvious once we reach these fundamental layers11:00
fwereade_niemeyer, yeah11:00
wrtpvars map[string]string, presumably11:00
niemeyerfwereade_: I also suggest sleeping on the idea of how a context relate to a unit, and what's the proper way to organize the relationship between those concepts11:00
niemeyerwrtp: That'd work, yeah11:01
fwereade_wrtp, not sure it matters much either way, not unreasonable to pass vars around in os.Environ style11:03
fwereade_wrtp, once they're fixed anyway11:03
wrtpfwereade_: i think os.Environ might be a map now...11:03
fwereade_wrtp, ofc they're easier to manipulate as map[string]string11:03
wrtpfwereade_: no, it's not11:03
wrtpfwereade_: there was *something* like that :-)11:04
fwereade_niemeyer, wrtp: I'm just going to try to flesh out my current sketch a little and see if any new enlightenment dawns11:05
fwereade_niemeyer, wrtp: well *actually* it seems I'm going to eat lunch11:06
wrtplol11:06
fwereade_ttyl :)11:06
wrtpfwereade_: luncheon enlightenment?11:06
fwereade_haha11:06
fwereade_by light alone11:06
wrtpfwereade_: you read that yet?11:06
niemeyerfwereade_: Lunch.. hmm.. still several hours to go here, how unfortunate :)11:08
niemeyerfwereade_: Enjoy :)11:08
wrtpniemeyer: []string is better actually. that's the type of the Env field in exec.Cmd.11:09
niemeyerwrtp: Right, but it may be easily assembled within Exec.. your suggestion is likely a better interface to work with from within the package11:13
wrtpniemeyer: it's possible. but i suspect that in practice all the calls will look like: Exec([]string{"FOO="+foo, "BAR="+bar}, ...)11:14
wrtpniemeyer: that is, i wonder how much we'll actually "work with" the environment variables.11:14
niemeyerwrtp: That looks like a map to me11:15
wrtpniemeyer: no more than os.Environ or exec.Cmd.Env :-)11:15
niemeyerwrtp: Never built one of those by hand.11:15
wrtpniemeyer: i'm happy to minimise impedance mismatch where it doesn't make much difference.11:16
wrtpniemeyer: but i don't mind too much (i did suggest a map originally, after all)11:17
niemeyerwrtp: :-)11:17
TheMuewrtp: i would uses an own type based on map[string]string here, having methds to pass it as []string11:18
TheMuewrtp: so the usage is simple and passing the content to Exec() too11:19
wrtpniemeyer: one possible place where it's actually easier to work with a slice is if your env vars are mostly constant, with one changing. then you can do Exec(append(constantVars, "ONE_VAR="+...))11:19
niemeyerwrtp: m["ONE_VAR"] = ...11:20
wrtpniemeyer: that doesn't work if you don't want the map to be changed for everyone11:21
wrtpniemeyer: you'd have to copy the map first11:21
niemeyerwrtp: That's not how exec works..11:21
TheMuewrtp: could be done with my type too. even more comfortable in a method myEnv.WithNewVariable(k, v string) []string11:21
TheMuewrtp: w/o changing it, using the append internally11:21
wrtpi think all this is overthinking it. i think in practice the usage will be ultra simple.11:22
niemeyerLet's please move on. Whatever fwereade_ picks is fine.11:22
wrtpagreed11:22
niemeyerwrtp: Yes, and you've been redefining how to work with a map above..11:22
wrtpniemeyer: ?11:22
niemeyerwrtp: !11:22
wrtp[11:21] <niemeyer> wrtp: That's not how exec works..11:23
wrtpdepends whether the map is used in a concurrent context11:23
wrtpanyway, let's leave it.11:24
niemeyerwrtp: Both structures are mutable.. a process environment isn't mutable.11:24
wrtpniemeyer: i didn't say it was. i was referring to m["ONE_VAR"] which will change m for everyone unless you create it every time. it's not a big issue.11:26
niemeyerwrtp: So will append..11:26
niemeyerfwereade_: Please use a map.. let's see how it looks like.11:26
wrtpniemeyer: not if the slice has cap == len. but perhaps that's too fragile.11:27
niemeyerwrtp: Indeed it is. append mutates the slice.11:27
wrtpniemeyer: only cap!=len11:27
TheMuewrtp: take a look at http://paste.ubuntu.com/883111/11:27
niemeyerwrtp: Indeed it is fragile.11:28
TheMuewrtp: untested quick hack11:28
niemeyerwrtp: Gosh..11:28
niemeyerwrtp: These arguments really consume our time for no good reason.11:28
wrtpyeah11:28
wrtplet's not11:28
wrtpsorry, it's my fault!11:29
* TheMue sighs11:30
TheMuelunchtime12:13
fwereade_niemeyer, I think that Exec does actually need a Context... we'll need to flush settings writes once the hook's complete12:14
wrtpfwereade_: can't that be done by the caller of Exec?12:25
niemeyerfwereade_: Agree with wrtp's questoin12:27
niemeyerfwereade_: Also, who calls Exec12:28
fwereade_wrtp, well, it could; but `Exec(vars map[string]string, path, dir string)` is actually so trivial a wrapper around os/exec that I'm not sure it's meaningful any more12:28
wrtpniemeyer, fwereade_: is it ok if i leave the log prints in the live test until we hook up logging to the tests?12:28
niemeyerfwereade_: I don't have the full picture, so don't really have any strong recommendations at this point.. I'd just try to work out a way to keep the concepts nicely isolated12:28
wrtpfwereade_: we had too long a discussion about this earlier. go with what you feel like :-)12:28
fwereade_wrtp, I question the value of logging those tests at all, really, but I don't feel all that strongly about it12:29
fwereade_niemeyer, wrtp: cool, cheers12:29
wrtpfwereade_: the logging only appears in verbose mode. sometimes it'll hang forever and it's useful to know where.,12:29
niemeyerwrtp: Hooking up logging to the tests is done by log.Target = c in SetUpTest12:29
niemeyerwrtp: We already do that in some suites12:29
fwereade_wrtp, ok, cool, that makes sense then12:30
niemeyerwrtp: I'm happy with the branch to move forward as it is, though, and have that in a separate one12:30
wrtpniemeyer: i can do that, but i'd need to add log.Printf calls too. i'd prefer to do it in a separate branch12:31
wrtpyeah12:31
niemeyerfwereade_: Just as a nice realization, everything in that branch *is* a thing wrapper on top of Exec..12:35
niemeyers/thing/thin12:35
fwereade_niemeyer, agreed, but imo it's sane for "hook.Exec" to know about the particular idiosyncracies of hook execution12:37
fwereade_niemeyer, it'd be a different matter if it were "util.Exec" or something12:37
niemeyerfwereade_: Agreed12:37
niemeyerfwereade_: But there are none there, yet..12:38
niemeyerfwereade_: I see your point, though.. it may well make sense to have a context there12:39
wrtpfwereade_, niemeyer: submitted! thanks. 144 commits it took :-)12:40
fwereade_wrtp, that's gross12:40
* fwereade_ tries to keep a straight face12:40
niemeyerwrtp: 2 of those are mine! ;-)12:40
wrtpniemeyer: 2 of your commits say "committer: Roger Peppe" ?12:41
niemeyerwrtp: Nah, nevermind12:42
wrtpanyway, i'm very happy to have got there in the end!12:42
fwereade_aww, I was hoping for at least a groan of pain from someone, it's rare to get such a perfect opportunity for a really ugly pun12:43
* wrtp lets the pun sail way over his head.12:47
* wrtp lunches12:49
niemeyerfwereade_: So, turns out I lied to you.. proposing again with lbox won't mark the branch as Needs Review12:58
niemeyerfwereade_: I'm fixing that now, but just wanted to mention in case you reproposed some branch before that I'm not seeing!12:59
=== marrusl_ is now known as marrusl
fwereade_niemeyer, ah-ha, I suspect I have a branch or two languishing then, let me check13:23
fwereade_niemeyer, nope, go-add-cmd-context is still NR (but then I don't think I ever WIPed it); I'll bear it in mind though13:24
fwereade_niemeyer, thanks13:24
andrewsmedinagood morning13:32
niemeyerandrewsmedina: Heya13:33
niemeyerfwereade_: np13:33
niemeyerfwereade_: Already fixing it, so hopefully won't bite in the future13:34
fwereade_niemeyer, sweet, tyvm13:34
fwereade_andrewsmedina, good morning13:34
andrewsmedinaniemeyer: I saw the reviews made for you and rog13:34
niemeyerfwereade_: I hope to fix pre-req right after13:34
fwereade_niemeyer, <313:35
niemeyerfwereade_: *that* will be a real win :)13:35
fwereade_niemeyer, maybe even <413:35
niemeyerandrewsmedina: Super.. sorry for some confusion there13:35
niemeyerfwereade_: LOL13:35
niemeyerfwereade_: We should totally start using <4 as a convention :)13:35
fwereade_niemeyer, I have a vague feeling I stole it from kingdom of loathing13:36
andrewsmedinaniemeyer: I realy need parse the lxc error output?13:36
niemeyerandrewsmedina: Not really parsing.. the basic idea is to include it in the error.. but it'd be nice to strip out the usage message in case it's there13:37
niemeyerandrewsmedina: so13:37
niemeyeroutput, err := ...13:37
andrewsmedinaniemeyer: I'm already doing it13:38
niemeyerif i := bytes.Index(output, []byte("\nUsage: ")); i > 0 {13:38
niemeyer    output = output[:i]13:38
niemeyer}13:38
niemeyerandrewsmedina: That's all really13:38
andrewsmedinaniemeyer: I will do it now13:39
niemeyerandrewsmedina: Thanks a lot13:43
niemeyerI promise I won't ask why setStatus is its own method in a merge_proposal object, while everything else is just changing fields13:43
niemeyerfwereade_, wrtp: I'll also invert the meaning of -same-log, and make it the default behavior, as suggested by wrtp before.13:56
niemeyerwrtp: You were right.. showing the logs in all cases is boring, and people don't update the log even if it's in their face.13:56
fwereade_niemeyer, I update the log (er, sometimes)13:57
wrtpniemeyer: ECONTEXT13:57
fwereade_niemeyer, but I can bear to type a few more chars when I need to ;)13:57
niemeyerwrtp: Fixing lbox13:58
* wrtp can't remember what command -same-log is a flag to...13:58
wrtpah, i remember now13:59
niemeyerfwereade_: It will continue to be shown on submit too, so that's always a good hooking point to get the log right13:59
wrtplog==description13:59
fwereade_niemeyer, excellent13:59
wrtpi've said this before, but i think it's a pity that so much functionality is shoehorned into "lbox propose"14:00
wrtpniemeyer: i know the conception of lbox is as a general interface to launchpad, but will it actually end up as any more than propose+submit?14:01
niemeyerwrtp: Yes14:02
wrtpniemeyer: BTW it would also be great if we could somehow reduce the volume of emails that are sent as a result of lbox.14:03
wrtpeach change generates about 3 emails.14:03
niemeyerwrtp: Each change generates two emails, one from codereview, one from Launchpad. I don't know of any way to avoid the email from Launchpad without also losing the history in the merge proposal itself.14:04
wrtpniemeyer: there's also the two emails that are sent when you actually send the comments on the codereview page...14:04
niemeyerwrtp: Exactly.. each change generates two emails.14:05
wrtpniemeyer: it means i can't respond to some comments with some changed code without generating 4 emails.14:06
niemeyerwrtp: I apologize for the trouble. I don't know how to solve the issue.14:06
wrtpniemeyer: yeah, i know. maybe i should just filter out all emails that simply say "please take a look"14:06
wrtpniemeyer: because they're often noise, and the real ones are those sent direct from codereview14:07
niemeyerwrtp: They're not noise for me, but whatever works for you is certainly fine for me.14:07
niemeyerwrtp: Many times people don't answer the feedback, and simply do an "lbox propose". You'd miss those.14:08
wrtpniemeyer: if there was a way of saying "please upload these changes *and* publish my codereview comments", that would be nice...14:08
niemeyerwrtp: That's already what it does..14:08
wrtpniemeyer: yeah, i know.14:08
wrtpniemeyer: really?14:09
niemeyerwrtp: I believe..14:09
niemeyerwrtp: Yep.. try it out and let me know :)14:09
wrtpniemeyer: so lbox propose publishes pending codereview comments, you think?14:09
wrtphmm, that would be good, if slightly unexpected.14:10
wrtpif i could upload changes without sending mail (and also without marking the branch as "work in progress") that would work too.14:11
wrtpalong the lines of go's hg upload.14:11
niemeyerwrtp: Probably not, actually14:12
niemeyerwrtp: But it might be doable14:12
niemeyerwrtp: I have message_only set to true14:12
wrtpi think a lot of the noise is because the default behaviour is to mail, where i think that sending mail should be a considered action, executed deliberately.14:12
niemeyerwrtp: There's no reason for people to be uploading changes to Rietveld when they don't intend those changes to be reviewed14:13
niemeyerwrtp: If all you want is to push the branch, just "bzr push"14:14
niemeyerwrtp: lbox propose will.. well.. propose14:14
wrtpniemeyer: i usually do want those changes to be reviewed, but i'll usually push them and have a quick check, before clicking Publish+Mail.14:14
wrtpniemeyer: or lbox mail :-)14:15
wrtpif it existed.14:15
niemeyerwrtp: I'll reserve lbox mail to the moment when lbox becomes an email client.14:15
wrtpniemeyer: lbox announce, maybe14:17
niemeyerwrtp: What you want already exists.. lbox propose -prep..14:20
wrtpniemeyer: but -prep marks the branch as "work in progress"14:20
wrtpniemeyer: and taking it out of -prep mode sends a mail.14:21
niemeyerwrtp: Which seems to be exactly what you want.. if you're reviewing your own changes, it's not yet ready for review14:21
wrtpniemeyer: the problem is that it's not possible to publish codereview remarks and upload code ready for review without sending 4 emails.14:24
wrtpniemeyer: (i just verified that lbox propose does not submit pending codereview comments)14:24
niemeyerwrtp: I can fix that easily14:24
wrtpniemeyer: and also that the default should be to not announce.14:24
wrtpniemeyer: that way it's harder to make unnecessary noise.14:25
niemeyerwrtp: We already talked about that. The command "lbox propose" should propose. This won't change.14:25
wrtpniemeyer: it is editing the proposal. it doesn't have to announce that edit.14:26
wrtpniemeyer: it should always be good practice to check your proposal before sending mail to people about it, and the default usage of the tool should reflect that IMHO14:26
wrtpniemeyer: it would be great to fix the codereview comments issue BTW14:27
niemeyerandrewsmedina: Splitting the container names on spaces as you are doing is fine. Please once you're happy with the error stuff, just lbox propose again14:43
andrewsmedinaniemeyer: I'll finish it in time for lunch14:44
niemeyerandrewsmedina: Thank you14:46
andrewsmedinaniemeyer: Do you have any deadline for the release of the juju Go port?14:47
wrtpniemeyer: indeed it's a bug in lxc-create. i wondered where that 'b' directory had come from:14:47
wrtp/usr/bin/lxc-create:12314:47
wrtpmkdir -p $lxc_path/$lxc_name14:47
* wrtp wishes that the bourne shell had got quoting right.14:47
niemeyerandrewsmedina: We have some plans, but they're not firm yet14:48
* fwereade_ looks owlishly at allhands.canonical.com and tries to resist the temptation to add himself as a peer reviewer15:15
* wrtp can't log in to allhands.canonical.com15:17
wrtpfwereade_: you can definitely use the error from Cmd.Run, BTW, rather than doing the separate Stat call.15:30
* hazmat wishes he could have slept on the plane15:30
hazmatniemeyer, i did work out how to get py juju to handle session expiration without restart, impl in progress15:31
hazmatbasically tracking watches and ephemerals with the existing expiration trap handlers15:31
fwereade_wrtp, it strikes me as kinda tedious to do different things on different errors after the fact15:31
wrtpfwereade_: you're doing just that on the Stat result, no?15:32
fwereade_wrtp, and I'm not sure why we should bother setting up and running a process if we can guarantee it's not going to work and provide tailor-made errors15:32
wrtpfwereade_: depends how expensive the setup is, i guess15:34
fwereade_wrtp, hmm, I guess I trust the Stat error to be relevant but not the Run one, which is maybe irrational15:34
wrtpfwereade_: it is - it works just fine (assuming the path has a separator in)15:34
wrtpfwereade_: http://paste.ubuntu.com/883400/15:35
fwereade_wrtp, the path passed to Command?15:35
niemeyerhazmat: Expiration trap handlers?15:35
wrtpfwereade_: yeah15:35
niemeyerhazmat: Greetings, btw :)15:35
niemeyerhazmat: Back from PyCon?15:35
hazmatniemeyer, greetings! good to be back15:35
wrtpfwereade_: (it's documented in exec.Command)15:35
hazmatniemeyer, indeed. i took the red eye, just got back in a few minutes ago15:35
fwereade_wrtp, ah, ok; and I guess we get sensible errors for non-executability too?15:36
wrtpfwereade_: yeah, you'll get "permission denied" which seems fine to me.15:36
fwereade_wrtp, cool, should be a never-happen anyway I think15:36
wrtpfwereade_: although you can test with os.IsPermission too if you want15:37
fwereade_wrtp, we check hook permissions on charms anyway IIRC15:37
wrtpright15:37
hazmatniemeyer, there's a session expiration callback, tracking ephemeral nodes and extant watches, allow us to recreate them on session expiration without disturbing the watch callbacks. we can also trap/cb  on connectionloss/sessionexpired errors for operations that are attempted while this connection state exists (expired session)15:37
wrtpfwereade_: also, isn't most of the setup done *before* calling Exec?15:38
hazmatin the latter case we re-establish a session and the prior ephemeral nodes/watches and allow the op to retry15:38
hazmati think i still need to work out some details on the last one for ops made during bad connection state15:41
niemeyerhazmat: You mean recreating watch/ephemeral/etc state without telling the call sites about what is actually going on behind the scenes?15:41
hazmatniemeyer, yes15:42
niemeyerhazmat: I'm a bit skeptical about being able to do that in a reliable manner15:42
hazmatfair enough15:42
hazmatniemeyer, effectively the call sites will see a watch fire, with the current state, so they will be informed about the current view15:43
hazmater. state15:43
niemeyerhazmat: Yeah, but the whole world may have changed.. every single watch will have to fire somehow15:44
niemeyerhazmat: Since the event they were waiting for may have occurred meanwhile..15:44
hazmatniemeyer, indeed15:44
niemeyerhazmat: Or it may not..15:44
hazmatniemeyer, we can fire the watch when its restablished15:44
niemeyerhazmat: I know.. we can do all sorts of things to try to hide the fact that, indeed, the connection has died.. or, we can prepare the application for having the application dying on its face and reestablishing state15:45
hazmatniemeyer, we've already done the latter15:45
hazmatbut you've adovocated that we can do better15:46
niemeyerhazmat: Kind of..15:46
fwereade_wrtp, admittedly there's not much setup15:46
niemeyerhazmat: The unit agent having to reestablish a connection with zookeeper doesn't mean it's fine to disturb the actual software that is running15:47
niemeyerhazmat: This is the tricky part15:47
niemeyerhazmat: As I understand it, and please correct me if I'm wrong, what the approach you suggest would do to work around this is to preserve state in memory so that the charm isn't disturbed. Do I get it right?15:50
hazmatniemeyer, well the world may have changed by the time we re-establish, we have no way of knowing, triggering the watch gives the application a chance to reconsider the global state in the context of its last knowledge of the current state15:50
hazmatniemeyer, yes15:50
hazmatniemeyer, we can also robustly die ;-) and restart15:50
hazmatwhich is what i was planning on.. its a quite a bit simpler15:51
hazmatbut i think this might approach has some merit as well15:51
hazmater..15:51
hazmats/might/15:52
niemeyerhazmat: I hope we can robustly die, and restart, and not disturb the charm.15:53
niemeyerhazmat: and by not disturbing I mean not marking the unit as offline to the relation peers, etc15:53
niemeyerhazmat: That said, you certainly have freedom for experimentation on that area..15:55
niemeyerhazmat: I'm just starting to wonder a bit how it'll all fit in time for 12.04, as per the mail, but no please-mom in this case, as you say.15:56
hazmatokay.. that's fine as well re die without affecting the charm.. it has some of its own edge cases as the failure to record the transition, may attempt to write zk state, which will also fail.. needing zk/disk state reconcilliation but that's needed anyways to prevent a termination at the moment between those two writes from causing inconsistencies15:57
hazmatniemeyer, gotcha, i have some concerns as well, but more around the bugs than the features. i think the subordinate work was in good progress. the constraints work should be okay. we need to yank some of the ghetto constraints out of the env file.. i guess we have more time for bug fixes though15:59
wrtpfwereade_: if there's a place for the Stat, i think it's in the code that initially analyses the charm and can decide which hooks exist, once only.16:00
wrtpfwereade_: although it's probably premature optimisation; i can't see this being a bottleneck.16:00
niemeyerhazmat: Yeah, it's tight16:01
TheMuefwereade_: i've got two probs with the presence package. a standand alone test crashes and the usage of Alive() for a non-existent path leads to true.16:02
niemeyerOkay.. lunch time here. Will be back in a bit.16:02
fwereade_wrtp, fair enough, it doesn't look too bad as it is16:03
fwereade_TheMue, hum, this is somewhat disturbing16:03
fwereade_TheMue, you're definitely using the right gozk?16:03
TheMuefwereade_: which is "the right" gozk?16:04
TheMuefwereade_: or better, which is the "wrong" one?16:04
fwereade_TheMue, the current one should be fine, AFAIK16:04
fwereade_TheMue, the wrong one is any version before rog's locking fixes16:04
TheMuefwereade_: i updated it together with weekly …03-1316:05
TheMuefwereade_: now done a fresh go get -u and i still have the same problem16:06
wrtpfwereade_, TheMue: i may need to do the tagging16:06
wrtpfwereade_: hold on16:06
wrtpTheMue: hold on16:06
* TheMue holds on16:07
TheMuewrtp: btw, seen my little env proposal at http://paste.ubuntu.com/883111/ ?16:08
wrtpTheMue: yeah, i thought it was a bit overkill tbh. i'm happy going with the []string as in fwereade_'s proposal.16:09
TheMuewrtp: 13 lines are overkill?16:09
wrtpTheMue: any additional abstraction can be overkill :-)16:09
TheMuewrtp: now we got such a fine language to abstract data types w/o much trouble and he calls it overkill ;)16:10
wrtpTheMue: abstraction is abstraction, and the less of it the better :-)16:10
TheMuewrtp: so why do you wanted an extra type for the retry values?16:10
TheMuewrtp: and less it not always better. if it leads to more code duplication and possible failures16:11
wrtpTheMue: that abstraction already existed, it just didn't have a name :-)16:11
fwereade_wrtp, I'm not quite sure I see where you're coming from on Flush... you seem to be arguing that we should do as little as possible in Exec, while my perspective is that hook.Exec should do not leave hook execution incomplete for some putative future client to handle16:11
wrtpfwereade_: my perspective is that the caller of Exec can have sole responsibility for the context.16:12
TheMuewrtp: is the tagging done?16:12
wrtpTheMue: yeah, i think so16:12
fwereade_wrtp, ie that contexts aren't really anything to do with hook execution?16:12
wrtpfwereade_: that too. the Exec function doesn't care about the context, other than to flush it.16:13
TheMuewrtp: yep, thx, no ore error16:13
wrtpTheMue: cool. i must make a script that automatically does the tagging after a submit.16:13
* TheMue still like abstraction that helps. maybe it's my smalltalk history16:14
wrtpTheMue: oh yeah, don't get me wrong, i love a good abstraction in the right place.16:14
fwereade_wrtp, you seem to be saying that hook execution can be considered to be complete before its changed state has been persisted16:14
fwereade_wrtp, I don't think I agree with that16:14
TheMuewrtp: and here it is the right place. an env is anv, not only a slice of strings16:15
wrtpTheMue: for exec.Cmd and os, it's a slice of strings.16:15
TheMuefwereade_: if i'm doing an Alive() on a non-existing node i expect a false. am i wrong here?16:15
wrtpTheMue: and i'm happy to go along with that view unless it's making things harder16:15
fwereade_TheMue, no, you're perfectly correct there16:16
wrtpfwereade_: i'm not saying that16:16
TheMuewrtp: yeah, sadly, but that doesn't make it better. time is also only a number, but thankfully they changed it to a better type.16:16
fwereade_wrtp, then... that Exec should not be responsible for executing the hook?16:16
TheMuefwereade_: strange, because i'm getting a true16:17
fwereade_wrtp, that it can just kinda half do it?16:17
wrtpfwereade_: no, Exec executes the hook. when the hook returns, we flush its context.16:17
fwereade_TheMue, is this in a test?16:17
wrtpfwereade_: but the caller of Exec can create the context, call Exec, then flush it16:17
TheMuefwereade_: in my test. ;) but pls wait, will test it with helpful print statements, not that i'm testing the wrong node16:17
fwereade_wrtp, in my view context flushing is part of execution -- it's the completion of any relation-sets that happen to be called16:17
fwereade_TheMue, are you sure you're cleaning zk up properly between tests?16:18
wrtpfwereade_: schematic: http://paste.ubuntu.com/883487/16:18
fwereade_TheMue, we don;t seem to have a proper nuke-everything-in-zk function anywhere16:18
wrtpfwereade_: except of course, the context id would be passed to Exec.16:19
fwereade_wrtp, why defer Flush()16:19
fwereade_wrtp, I'm pretty sure that a broken hook shouldn't mess with state16:19
wrtpfwereade_: yeah, true. it could easily do the flush only if Exec didn't return an error.16:19
fwereade_wrtp, I still don't see the benefit of putting the flush somewhere else16:20
wrtpfwereade_: i don't see the benefit of passing a callback into Exec when it's trivial to do the call directly.16:20
wrtpfwereade_: it makes for a better separation of concerns, i think16:21
wrtpfwereade_: Exec is solely responsible for running the hook16:21
wrtpfwereade_: the caller of Exec is responsible for the context management16:22
fwereade_wrtp, running a hook will often change state somewhere, right? what's so special about relation-set that it should be handled outside the hook execution?16:23
wrtpfwereade_: running a hook changes state via the context callbacks only, right?16:24
fwereade_wrtp, running a hook can do anything it wants to local machine state...16:24
TheMuefwereade_: even w/o nuke-everything i'm talking here about only one node which get's nuked. and my tests before worked.16:24
wrtpfwereade_: which, as i think we've agreed, are managed by the caller of Exec16:24
wrtpfwereade_: we don't flush local machine state...16:24
fwereade_wrtp, no, because we don't need to do it ourselves16:25
wrtpfwereade_: and it's flushing that we're concerned with here16:25
fwereade_wrtp, you're still saying that some of the things done in a hook should be handled before exec returns and some shouldn't16:25
fwereade_wrtp, ...aren't you?16:26
* wrtp is thinking16:27
wrtpfwereade_: i'd say that the context is flushed *after* the hook returns16:28
wrtpfwereade_: the relation-set is "done" before exec returns, but relation-set doesn't actually change the relation until the hook returns.16:29
fwereade_wrtp, in the sense of "after the hook-as-written-by-the-user exits", that's true16:30
fwereade_wrtp, well, it does, as far as the hook author knows16:30
wrtpfwereade_: well, that wouldn't change. (actually it's probably possible to observe it in fact)16:30
fwereade_wrtp, relation-set followed by relation-get should do the apparently sane thing16:30
wrtpfwereade_: relation-set; sleep 10; relation-set; wouldn't result in any changes visible in another unit...16:31
fwereade_wrtp, no, but they should be visible locally16:31
wrtpwell, not the first set anyway16:31
wrtpfwereade_: sure16:31
wrtpfwereade_: so i think it's logical to have Exec mirror the "hook as written by the author" executing.16:32
fwereade_wrtp, and I think it's logical to have Exec-having-returned mean that the exec is complete and all necessary changes have been persisted16:33
fwereade_wrtp, I don't think it's reasonable to draw an arbitrary line which says that some direct consequences of hook execution are part of exec and some aren't16:34
fwereade_wrtp, if you can explain why that line's not arbitrary you may convince me16:35
fwereade_wrtp, just a thought: why do you assume the caller of exec will necessarily be the creator of the context?16:38
fwereade_wrtp, in python, some contexts are reused for more than one hook16:38
wrtpfwereade_: it doesn't matter - Exec can be entirely context-agnostic, and that's a good thing16:38
wrtpfwereade_: here's how i imagine Exec: http://paste.ubuntu.com/883519/16:38
wrtpfwereade_: a very very simple wrapper around exec.Command16:39
fwereade_wrtp, ctx.Vars?16:39
wrtpfwereade_: oh yeah, forgot that bit, one mo16:39
wrtpfwereade_: http://paste.ubuntu.com/883521/16:40
fwereade_wrtp, all you're doing is smearing responsibility for... hook execution... into different places16:40
wrtpfwereade_: i really don't think so16:40
wrtpfwereade_: Exec *executes the hook*. something else *creates and flushes the context*.16:41
wrtpfwereade_: no need for a context interface16:41
wrtpfwereade_: of course, it might be that we want to folk context creation and flushing *into* the Exec call. i'd be good with that.16:42
fwereade_wrtp, what's the connection between creation and flushing?16:42
wrtps/folk/fold/16:42
wrtpfwereade_: presumably the context becomes invalid after the hook has finished execution?16:43
fwereade_wrtp, why?16:43
fwereade_wrtp, we reuse contexts in python16:43
wrtpfwereade_: what happens if some background process uses it later?16:44
wrtpfwereade_: it'll get the context for a different hook16:44
fwereade_wrtp, I don't see how that's a danger16:44
fwereade_wrtp, sometimes we know we want to run 2 hooks immediately after one another16:45
fwereade_wrtp, and so the context gets reused16:45
fwereade_wrtp, flushing doesn't invalidate a context16:45
fwereade_wrtp, but contexts don't live beyond the time they're relevant either16:45
wrtpfwereade_: that's all fine. if we don't want to create the context each time, fine.16:46
fwereade_wrtp, so why is there a connection between creation and flushing? why should the same thing be responsible for both?16:46
wrtpfwereade_: we might want to delay flushing until after the second hook has run too...16:46
wrtpfwereade_: i'm not saying it should.16:47
wrtpfwereade_: it's outside the purview of Exec16:47
fwereade_wrtp, so an error in one hook should affect the results of an earlier hook? surely not16:47
wrtpfwereade_: erm, i think you'd have a problem with that anyway16:48
wrtpfwereade_: if you reuse a context16:48
wrtpfwereade_: then the dodgy settings from the failed hook will continue on into the next hook16:48
fwereade_wrtp, if the hook fails we won't continue on to the next one16:49
fwereade_wrtp, you just suggested we should delay the flush16:49
fwereade_wrtp, so you get hook 1 writing state, hook 2 failing, and hook 1's apparent success not really sticking16:49
wrtpfwereade_: ah, i see.16:49
fwereade_wrtp, you seemed to be arguing from a position in which flushing and creation were linked somehow -- did I misunderstand that?16:50
wrtpfwereade_: no, i'm arguing that Exec shouldn't concern itself with the context at all16:50
wrtpfwereade_: because it's trivial for Exec's caller to do that.16:50
wrtpfwereade_: it makes Exec simpler at no cost, AFAICS16:51
fwereade_wrtp, it just moves code around, at the cost of having an Exec that smears the effect of any relation-set calls out beyond the lifetime of that call16:52
wrtpfwereade_: it's more than just moving code - it removes code (the ExecContext type and that argument to Exec)16:53
fwereade_wrtp, it's 1 less param and 1 fewer lines in a not-very-big function, in exchange for an arbitrary division of responsibilities16:54
fwereade_wrtp, ExecContext is only there because Context is not yet implemented16:54
fwereade_wrtp, I *do* kinda want to keep it around beyond this branch, for simplicity of testing, but that's not critical really16:55
fwereade_wrtp, and that one saved line has to be called somewhere else anyway16:56
wrtpfwereade_: i guess i really don't see it as an arbitrary division of responsibilities. i think it makes a lot of sense to have Exec as a simple wrapper around Command, and the context stuff dealt with elsewhere. then you don't *need* the ExecContext interface for testing, because Exec doesn't use it.16:56
fwereade_wrtp, ok, why have an Exec function at all? if you just want a trivial wrapper around Command, why not inline the whole thing at the call site (whatever that may be)?16:58
wrtpfwereade_: and if you look at the code without considering the hook abstraction, why are we passing in the context? it's so that it can be called when Exec returns. we know how to do that - we call Exec, then call Flush...16:58
fwereade_wrtp, so thatit can *sometimes* be called when hook returns16:58
wrtpfwereade_: sure. we'll be checking the error anyway16:58
fwereade_wrtp, and no it's not just dependent on whether there's an error returned16:59
wrtpfwereade_: no?16:59
fwereade_wrtp, if the hook doesn't exist we don;t bother to flush16:59
fwereade_wrtp, but that's not an error16:59
wrtpfwereade_: interesting point.17:00
fwereade_wrtp, ok, we *could* just always flush, it's basically be a no-op if no state changed17:00
wrtpfwereade_: it's a performance issue17:00
wrtpfwereade_: but if that *is* an issue, then yes, it's a good reason for flush to stay inside Exec.17:00
wrtpfwereade_: although...17:00
fwereade_wrtp, I doubt it's significant, I imagine ConfigNode is smart enough not to round-trip if there have been no changes17:01
wrtpfwereade_: as i said earlier, i think that we shouldn't be calling Exec at all if the hook doesn't exist.17:01
fwereade_wrtp, ah, sorry, I missed that17:01
fwereade_wrtp, why make the rest of the code care about hook existence?17:02
TheMuefwereade_: if i get it right StartPinger() creates a non-existent node (if the parent exists). i'm wondering, because i get a -101 (no node)17:02
wrtp[16:00] <wrtp> fwereade_: if there's a place for the Stat, i think it's in the code that initially analyses the charm and can decide which hooks exist, once only.17:03
fwereade_wrtp, yeah, I've already dropped the pre-checking17:03
wrtp[16:00] <wrtp> fwereade_: although it's probably premature optimisation; i can't see this being a bottleneck.17:03
fwereade_wrtp, still, why does it matteratall to anything otherthan the hook executor whetheror not the hook exists?17:04
wrtpfwereade_: anyway, i think Exec still serves a useful role - it sets up environment variables and it knows where hooks live &c17:04
wrtpfwereade_: just a matter of efficiency, i think.17:05
wrtpfwereade_: maybe Exec should be a method on a Charm type.17:05
niemeyerfwereade_: I suggest dropping Context from that branch entirely, so we can submit it, and then bring on another branch17:05
wrtpfwereade_: then the Charm type could do some initial introspection when created.17:05
wrtpniemeyer: +117:06
fwereade_wrtp, but you've also moved responsibility for env var setup outside exec in your last paste17:06
wrtpfwereade_: all the stuff in ExecInfo.Vars would still be around17:06
niemeyerfwereade_: I'm not suggesting it doesn't make sense.. if you still think it makes sense by the time Context is introduced, just add the argument again and let's evaluate it with more context17:06
niemeyerErm.. overuse of "context" there, but you see what I mean17:07
wrtpfwereade_: it's just that ExecInfo would also have ExtraVars (say) giving env vars that aren't inferrable from ExecInfo17:07
wrtpit's true, we are discussing without knowing what the surroundings are like.17:07
fwereade_wrtp, as advised against on "data-bag" grounds by niemeyer...17:07
wrtpfwereade_: isn't that what ExecContext.Vars did?17:08
niemeyerwrtp: ExtraVars isn't necessary.. anything that would be put in ExtraVars can be introduced via another field in ExecInfo.. after all, it's sole reason of existence is as a holder for those vars.17:08
wrtpniemeyer: yeah, that sounds right.17:08
niemeyerfwereade_: But really, just drop context and let's move on.. you know more about what's coming than we do, so you may be right and we just don't know it.17:08
fwereade_niemeyer, ok, I'll try to find some way to do so sanely :/17:10
fwereade_I still just do not get this insistence that Exec shouldn't actually finish hook execution17:11
niemeyerfwereade_: Exec in that branch is a self contained problem, very well isoalted.17:11
niemeyerfwereade_: There's an interface being provided, that is part of something else that doesn't yet exist. We don't have visibility yet onto the caller of Exec either. It's quite reasonable to have Flush being called by whoever calls Exec, after all, that's not about *executing* anything.17:12
niemeyerfwereade_: That said, I'm just trying to satisfy your curiosty. I'm not even suggesting yet that this is the right approach. I just think we've had enough discussion, and dropping Context from that branch is a trivial way to move forward.17:13
niemeyerfwereade_: Bring it back in the next branch, if needed be17:13
fwereade_niemeyer, ok then17:14
Pikkachuwhat's juju?17:14
wrtpPikkachu: https://juju.ubuntu.com/17:15
Pikkachuah ok nevermind17:16
TheMuefwereade_: got my question regarding StartPinger() above?17:19
fwereade_TheMue, whoops, sorry17:19
TheMuefwereade_: i didn't expected the ZNONODE17:20
fwereade_TheMue, StartPinger doesn;t necessarily create a node17:20
fwereade_TheMue, but, yes, it should if it doesn't exist17:20
TheMuefwereade_: yes, that's what i expected when reading your code17:20
fwereade_TheMue, and, yes, it depends on the parent existing17:21
TheMuefwereade_: the parent exists, but the node itself not, yes17:22
fwereade_TheMue, but there aren't any errors?17:23
TheMuefwereade_: i only get the ZNONODE back, i'll now take a look if the parent really exists. but it should, there's only so few code. ;)17:24
fwereade_TheMue, ZNONODE back from where?17:25
TheMuefwereade_: from StartPinger()17:26
fwereade_TheMue, it *looks* like the only place that could be coming from is the Create in changeNode.change, can you confirm that?17:27
wrtpanyone interested in helping me discuss a problem i've got with the ec2test server?17:28
TheMuefwereade_: just tested explicitely, the parent exists17:28
wrtp(a theoretical problem - i'm not sure what to do...)17:28
fwereade_TheMue, is the Create the source of the error?17:28
TheMuefwereade_: will have to add some print statements, one mom pls17:31
fwereade_wrtp, niemeyer: btw, https://codereview.appspot.com/5753057 is now context-free17:38
TheMuefwereade_: it's at the create. it's called twice and the first time w/o an error, the second time with "no node"17:41
niemeyerfwereade_: LGTM, sorry for the pain on this one.17:41
fwereade_niemeyer, no worries, there's clearly something I haven't managed to communicate but I still can't quite figure out what ;)17:42
niemeyerfwereade_: We're moving forward.. it's all good17:43
fwereade_niemeyer, yeah :)17:43
wrtpfwereade_: LGTM, likewise on the pain.17:43
fwereade_wrtp, cheers :)17:43
wrtpniemeyer: i've come up against a difficult issue doing the delayed ec2test update stuff.17:44
niemeyerfwereade_:17:44
niemeyer% lbox submit -adopt17:44
niemeyerBranches: lp:~fwereade/juju/go-tweak-supercommand => lp:juju/go17:44
niemeyerRequires: lp:~fwereade/juju/go-add-cmd-context17:44
niemeyerProposal: https://code.launchpad.net/~fwereade/juju/go-tweak-supercommand/+merge/9613617:44
niemeyererror: Pre-requisite lp:~fwereade/juju/go-add-cmd-context not yet merged.17:44
niemeyerfwereade_: One side is done.. :)17:44
fwereade_niemeyer, yay!17:44
wrtpyay! +117:44
andrewsmedinaniemeyer: I'm having a problem with lbox17:44
niemeyerwrtp: What's up?17:44
niemeyerandrewsmedina: What's up?17:45
niemeyer:)17:45
wrtpniemeyer: here's a summary: http://paste.ubuntu.com/883633/17:45
andrewsmedinaniemeyer: http://dpaste.org/eEmUR/17:45
wrtpniemeyer: basically it's hard to know how eventual consistency works17:45
=== Leseb_ is now known as Leseb
wrtpniemeyer: and i'm not sure that my simplistic idea for modelling it is sufficient17:45
niemeyerandrewsmedina: Authentication is failing for some reason17:46
niemeyerandrewsmedina: I suggest removing ~/.lpad_oauth and ~/.goetveld_* and trying again17:47
niemeyerwrtp: What's that text reflecting, more precisely?17:48
TheMuefwereade_: ah, changing the test order helped, the first time it works, then it fails. does the Pinger has to be stopped explicitely before you can start another one on the same node?17:48
niemeyerwrtp: Assumptions, experiments, ..?17:48
wrtpniemeyer: a sequence of actions.17:48
niemeyerwrtp: I can tell that part.. but it's still a number of bulleted points.. what's the introduction to those actions.17:49
wrtpniemeyer: difficult to experiment when we're concerned with non-deterministic behaviour17:49
niemeyerwrtp: I'm just trying to reverse engineer what you're showing me.. :)17:49
wrtpniemeyer: ok, i'm implementing the transaction stuff - instead of acting directly on the server state, each action looks at the current server state, but creates a delta which will be applied some time later.17:50
wrtpniemeyer: "transaction" isn't the right word there17:50
fwereade_TheMue, hmm, I don't *think* it *should* be a problem to have more than one pinger on a node, but I never really considered the possibility17:51
wrtpniemeyer: there's a queue of deltas, and one gets applied on each server action, so the visible state of the server always lags behind the state the actions have "modified"17:51
andrewsmedinaniemeyer: same problem17:51
niemeyerwrtp: Ok, I don't think this is reasonable: "client: create g2 referring to g1 (succeeds because g1 exists in the visible version)17:52
niemeyer"17:52
niemeyerandrewsmedina: You've reauthenticated, and it's failing even then?17:52
* wrtp refers niemeyer to the last paragraph.17:52
andrewsmedinaniemeyer: yes :(17:52
fwereade_TheMue, you should certainly be explicitly stopping/killing the pinger if you don't want it to hang around17:52
wrtpniemeyer: i can't see how i can make something which can reconcile both views - one fully consistent and the other not.17:53
wrtpniemeyer: (with just a simple queue)17:53
niemeyerwrtp: Let's do this.. let's ignore this for the moment17:53
niemeyerwrtp: and let's move on..17:53
wrtpniemeyer: ignore this set of changes to ec2test?17:53
wrtpniemeyer: i'm happy to do that. it seems... difficult.17:54
wrtpniemeyer: i can shelve what i've done for now.17:54
niemeyerwrtp: This is a *huge* rabbit hole.. trying to simulate an interface that is arbitrarily broken in ways we can't really predict is not likely to yield anything good.17:54
wrtpniemeyer: yeah, i think so.17:54
wrtpniemeyer: and i think that's why i've been finding it difficult to make good progress with it recently.17:54
niemeyerwrtp: Our live tests are more likely to be relevant than anything we come up with, and this will save us countless hours of pain.17:55
wrtpniemeyer: agreed.17:55
wrtpniemeyer: in that case, i think my next, smallish change will be to add better error messages to the zookeeper package.17:55
TheMueso, off for today, bye17:55
wrtpTheMue: night night17:56
niemeyerTheMue: Have a good evening!17:56
niemeyerwrtp: Sounds fantastic17:56
wrtpniemeyer: i thought about renaming Error to ErrorCode and having type Error struct {Code ErrorCode; Path string}17:56
niemeyerandrewsmedina: Ok.. hmm17:56
niemeyerandrewsmedina: What's the "date" command telling you right now?17:56
andrewsmedinaI'm using the last weekly17:57
andrewsmedinarelease yesterday17:57
andrewsmedina*released17:57
niemeyerwrtp: Sounds reasonable17:57
wrtpfwereade_: you missed this minor... // Exec executes the named hook in the environment defined by ctx and info.18:04
fwereade_wrtp, blast, sorry18:05
wrtpfwereade_: 's fine... next time.18:05
fwereade_wrtp, cheers :)18:05
fwereade_gents, I'm off for the night, take care18:07
wrtpfwereade_: g'night18:08
niemeyerfwereade_: Have a good one indeed18:12
niemeyer<niemeyer> andrewsmedina: What's the "date" command telling you right now?18:12
andrewsmedinaniemeyer: Wed Mar 14 14:07:51 BRT 201218:13
niemeyerandrewsmedina: The error message is right.. your system time is bogus18:13
andrewsmedina:(18:16
niemeyerandrewsmedina: Just fix it.. should be easy :)18:16
andrewsmedinaniemeyer: yes18:17
niemeyerBreaking for a moment18:32
wrtpi'm away for the evening.18:36
wrtpsee y'all tomorrow18:37
niemeyerwrtp: Apparently the submit-with-comments is actually working with current lbox: https://codereview.appspot.com/5753057/18:56
niemeyerwrtp: Erm, propose-with-comments18:56
niemeyerOh, or maybe not.. this is a different code path18:57
niemeyerAh, not really.. it's the same mechanism.. should be working indeed18:58
andrewsmedinaniemeyer: https://codereview.appspot.com/5764043/19:23
niemeyerandrewsmedina: Looks great, thank you19:35
niemeyerandrewsmedina: I suspect "usage" there needs a capital "U"19:36
andrewsmedinaniemeyer: tonight I will start a local environment19:36
andrewsmedinaniemeyer: from lxc no19:36
niemeyerandrewsmedina:19:36
niemeyer% lxc-start --blah19:36
niemeyerlxc-start: unrecognized option '--blah'19:36
niemeyerUsage: lxc-start --name=NAME -- COMMAND19:36
andrewsmedinaniemeyer: http://dpaste.org/kKOdM/19:36
niemeyerandrewsmedina: Ouch19:37
niemeyerandrewsmedina: Looks like we'll need to parse for both..19:39
niemeyerandrewsmedina: Also, Output() there won't work, as it's dropping stderr19:39
niemeyerandrewsmedina: You can use CombinedOutput() instead19:41
andrewsmedinaniemeyer: I can use Run and set stderr an stdout19:41
niemeyerandrewsmedina: CombinedOutput does that for you19:41
andrewsmedinaniemeyer: nice!19:41
andrewsmedinaniemeyer: can You run "lxc-version" and send me the output19:44
andrewsmedina?19:44
niemeyerandrewsmedina: 0.7.519:55
andrewsmedinaniemeyer: also here19:56
andrewsmedinabut I'm using centos19:56
niemeyerandrewsmedina: The issue is that the usage message isn't consistent19:57
niemeyerandrewsmedina: I also have both locally19:58
andrewsmedinayep19:59

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