[09:44] <fwereade_> wrtp, are you free for a vague rambling discussion about hook contexts?
[09:45]  * wrtp enjoys vague rambling discussions.
[09:45] <wrtp> fwereade_: certainly
[09:45] <wrtp> mornin' BTW
[09:46] <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 there
[09:46] <fwereade_> wrtp, mornin' :)
[09:47] <wrtp> fwereade_: 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 cool
[09:47] <wrtp> fwereade_: ah, the Context that was called Env, right?
[09:47] <fwereade_> wrtp, yeah
[09:48] <wrtp> fwereade_: 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, yeah
[09:49] <fwereade_> wrtp, it seems that the way to do that is to have fields for local unit, unit relation, relation members, and remote unit
[09:49] <wrtp> i'm not sure i understand
[09:49] <fwereade_> wrtp, and simply to error sensibly when someone asks for a capability we cannot provide
[09:50] <wrtp> oh, i see, i think.
[09:50] <wrtp> you 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 .relation
[09:51] <fwereade_> wrtp, it sounds like that's on the roadmap
[09:51] <fwereade_> wrtp, and thinking about it from that perspective leads to IMO a nicer implementation right now
[09:52] <wrtp> in which case, yeah, i could see that working. each field determines the behaviour of one or more callback methods.
[09:52] <fwereade_> wrtp, exactly
[09:53] <fwereade_> wrtp, ...hm ok I think I now know what I need to do in the exec branch
[09:53] <wrtp> fwereade_: 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 context
[09:53] <fwereade_> wrtp, I think it works out nicer if they are though
[09:53] <wrtp> fwereade_: you can still use your approach of "remote command execution" if you want to.
[09:54] <fwereade_> wrtp, yep, that's almost entirely orthogonal
[09:54] <wrtp> fwereade_: yeah
[09:54] <wrtp> fwereade_: 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 in
[09:55] <fwereade_> wrtp, and we need to be able to optionally override which state we're working with on the command line
[09:56] <wrtp> fwereade_: so the agent proactively fills in those fields rather than wait for the callback before reacting?
[09:56] <wrtp> i'm not sure i understand your last remark
[09:57] <fwereade_> wrtp, sorry, it's somewhat speculative
[09:57] <wrtp> what'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-bar
[09:57] <fwereade_> wrtp, state in that case is $SOME_RELATION
[09:58] <fwereade_> wrtp, foo=bar^^
[09:59] <wrtp> hmm, still at sea
[09: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 need
[09:59] <fwereade_> wrtp, sorry, I'll try to build it up a bit more logically
[09:59] <wrtp> fwereade_: i'd like to understand your speculation...
[10:01] <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 it
[10:02] <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 handling
[10:04] <wrtp> fwereade_: 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 Contexts
[10:04] <fwereade_> wrtp, yes
[10:05] <wrtp> hrmm
[10:06] <fwereade_> wrtp, that's not something I'm offering a *plan* for at the moment
[10:06] <wrtp> i don't see why they'd need to do that.
[10:06] <fwereade_> wrtp, it's just a door I don't want to close
[10:06] <wrtp> fwereade_: can't you ensure that there's always a context for a command?
[10:07] <fwereade_> wrtp, so that relation-set $SOME_RELATION can work out-of-band
[10:07] <wrtp> fwereade_: even out-of-band commands may have a context, no?
[10:08] <wrtp> fwereade_: (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 deal
[10:09] <fwereade_> wrtp, but I don't think we can know what relation relation-set wants to affect before it's called
[10:09] <wrtp> fwereade_: i think that is a big deal when we allow multiple unit agents in the same container
[10:10] <fwereade_> wrtp, huh, yeah, true
[10:10] <fwereade_> bah
[10:10] <wrtp> fwereade_: so given that, i think we can always assume JUJU_CONTEXT_ID, and hence some context.
[10:11] <wrtp> niemeyer: hiya
[10:11] <fwereade_> wrtp, I think they're different problems
[10:12] <niemeyer> Heya!
[10:12] <niemeyer> Mornings
[10:12] <fwereade_> wrtp, out-of-band execution means we *can't* depend on JUJU_CONTEXT_ID
[10:12] <fwereade_> morning niemeyer
[10:12] <wrtp> fwereade_: i don't think it does
[10:12] <wrtp> fwereade_: environment variables get exported
[10:12] <wrtp> fwereade_: out-of-band doesn't mean no-relation-to.
[10:14] <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 environment
[10:14] <wrtp> fwereade_: 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] <wrtp> s/make the/map the/
[10:14] <wrtp> fwereade_: i don't think that can possibly work
[10:15] <fwereade_> wrtp, I don't see how that's any different to just not having a context id
[10:15] <wrtp> fwereade_: maybe it's not. JUJU_AGENT_SOCKET is the important thing. but...
[10:16] <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 elegant
[10:16] <wrtp> fwereade_: 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 behaviour
[10:17] <niemeyer> fwereade_: <fwereade_> wrtp, out-of-band execution means we *can't* depend on JUJU_CONTEXT_ID
[10:17] <niemeyer> fwereade_: Kind of.. JUJU_CONTEXT_ID is what says it's out-of-band in the first place, right?
[10:18] <fwereade_> niemeyer, lack thereof, yes
[10:18] <wrtp> niemeyer: i think there are two kinds of out-of-band here
[10:18] <wrtp> niemeyer: 1) called after the hook has returned
[10:18] <wrtp> niemeyer: 2) called from an independent environment
[10:19] <wrtp> niemeyer: 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 out
[10:19] <wrtp> in 1), you've always got a JUJU_CONTEXT_ID
[10:19] <fwereade_> wrtp, ...which maps to a context with a potentially out-of-date member set
[10:19] <wrtp> fwereade_: at least we know when the context is wrong, and can do something appropriate
[10:20] <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 with
[10:21] <wrtp> fwereade_: because i imagine that the behaviour might differ depending on the original hook context.
[10:21] <fwereade_> wrtp, why?
[10:21] <niemeyer> wrtp: I don't understand what you mean by that
[10:21] <niemeyer> wrtp: 1 or 2
[10:22] <niemeyer> fwereade_: Agree regarding the out-of-date member set
[10:22] <niemeyer> fwereade_: But, it's a bit of a tricky situation..
[10:22] <wrtp> ok... here's a tangential question: which hook callbacks are not ok to call in which hooks?
[10:23] <niemeyer> fwereade_: 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 hook
[10:23] <wrtp> fwereade_: ok, but we'd change that, right?
[10:24] <wrtp> fwereade_: 'cos it's silly if we allow them out-of-band but not in certain hooks
[10: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] <wrtp> niemeyer: that's an easy answer - because the command spawned in the background by something else can't know what unit it's executing in
[10: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_SOCKET
[10:25] <wrtp> fwereade_: exactly
[10:25] <wrtp> fwereade_: and relation-get returns something different depending on what unit it's in
[10:25] <niemeyer> wrtp: A command spawned in the background isn't necessarily executing in any unit..
[10:25] <wrtp> niemeyer: no?
[10:25] <fwereade_> wrtp, indeed
[10:25] <niemeyer> wrtp: Sorry, bad terminology of my part
[10:26] <niemeyer> wrtp: 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 instance
[10:26] <wrtp> niemeyer: yeah, that seems fine.
[10:27] <niemeyer> It shouldn't blow up just because it was started within a hook rather than by the init scripts
[10:27] <niemeyer> In that sense, it's out-of-band as well, and it should work
[10:27] <wrtp> niemeyer: 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] <wrtp> niemeyer: and all callbacks should be valid all the time.
[10:28] <fwereade_> wrtp, exactly so
[10:28] <wrtp> except...
[10:28] <fwereade_> wrtp, I hadn't picked up on this until I spoke to niemeyer ?yesterday?
[10:28] <wrtp> that what about races
[10:28] <wrtp> ?
[10:28] <wrtp> i.e. within a hook, you know that relation-get is consistent
[10:29] <wrtp> but outside a hook, it might be constantly changing.
[10:29] <wrtp> ha
[10:30] <wrtp> maybe there could be some way of *asking* for a context id
[10:30] <niemeyer> wrtp: 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] <wrtp> JUJU_CONTEXT_ID=$(get-context-id)
[10:30] <fwereade_> wrtp, yeah, was just pondering same
[10:31] <wrtp> niemeyer: 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 details
[10:31] <niemeyer> Might be intersting to marry context id and the socket
[10:31] <niemeyer> fwereade_: ^
[10:32] <fwereade_> niemeyer, hm, yeah
[10:32] <niemeyer> fwereade_: While we still have time :)
[10:32] <wrtp> niemeyer: i was thinking the same
[10:32] <fwereade_> niemeyer, I don't think that door is closed
[10:33] <niemeyer> fwereade_: JUJU_AGENT_SOCKET=<path>[@<n>]
[10:33] <fwereade_> niemeyer, neither context id nor agent socket are actually intrinsic to the context, I think
[10: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 convenience
[10:34] <fwereade_> niemeyer, (...when constructing the env vars for a hook)
[10:34] <niemeyer> fwereade_: Right
[10:34] <wrtp> JUJU_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 itself
[10:34] <niemeyer> fwereade_: It's also provably not necessary right now, which means we can make it optional
[10:35] <niemeyer> fwereade_: Actually, now that I think of it, it's only optional because we don't have anything else but context-rich execution
[10: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 comes
[10:35] <niemeyer> fwereade_: So once we introduce out-of-band commands, context-rich execution needs the context info
[10:36] <wrtp> niemeyer: yes
[10:36] <niemeyer> fwereade_: I suggest taking the context stuff out
[10:36] <niemeyer> fwereade_: I mean, the context id, more specifically
[10:36] <niemeyer> fwereade_: Unless you're already making use of it in a way that moves us closer to these ideas we're talking about
[10:37] <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] <niemeyer> fwereade_: 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 easier
[10:38] <wrtp> don't we need a context id to stop inappropriate execution of hook commands after the hook has finished?
[10:38] <niemeyer> fwereade_: I was just suggesting dropping context id from the context, until we have a reason to hold it
[10:38] <fwereade_> niemeyer, OTOH having them on the context potentially encourages people to use them inappropriately
[10:38] <fwereade_> wrtp, we need it but it doesn't have to live on the context
[10:38] <niemeyer> fwereade_: What's "context" in this sentence.. it feels like we're talking about different things?
[10:40] <fwereade_> niemeyer, the type which (1) ultimately supplies env vars to Exec and (2) exposes useful methods to the hookcommand implementations
[10:40] <fwereade_> niemeyer, the actual execution environment the hook runs in needs a context id ATM, no argument there
[10:40] <niemeyer> fwereade_: Those feel like different things..
[10:41] <fwereade_> niemeyer, well, I thought so too, hence separation between Env and Context
[10:41] <fwereade_> niemeyer, but I'm starting to think otherwise
[10:41] <niemeyer> fwereade_: 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 looks
[10:42] <niemeyer> fwereade_: Cool
[10:43] <wrtp> fwereade_: hmm, i have to say i was quite convinced by our earlier discussion
[10:43] <niemeyer> fwereade_: 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 moment
[10:43] <wrtp> fwereade_: i think Context works well combined with Env
[10:43] <wrtp> type Context struct {
[10:43] <wrtp> 	Id int
[10:43] <wrtp> 	Vars map[string]string
[10:43] <wrtp> 	Relations map[string]string
[10:43] <wrtp> 	Unit *Unit
[10:43] <wrtp> }
[10:43] <wrtp> or whatever
[10:44] <wrtp> but... yeah
[10:44] <fwereade_> niemeyer, but it's true that as you suggested everything in Env can be derived from stuff we have in the context
[10:44] <wrtp> the Env is independent, because we might have a context without running a hook
[10:44] <fwereade_> niemeyer, and there's not much justification for a separate type/interface
[10:44] <niemeyer> fwereade_: Right
[10:44] <fwereade_> niemeyer, except in that testing becomes a bit easier
[10:44] <niemeyer> fwereade_: I see two things, but they're not the same two things that were in the original branch
[10:45] <wrtp> fwereade_: that's an excellent point though
[10:46] <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 conversation
[10:46] <niemeyer> fwereade_: 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:47] <niemeyer> fwereade_: 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 even
[10:47] <fwereade_> niemeyer, definitely not perfect, our discussion the other day modified it a certain amount, but I think the broad strokes are similar enough
[10:48] <niemeyer> fwereade_: 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 interface
[10:49] <fwereade_> niemeyer, I'm actually not so sure about that: I think we just need Context to have `Vars() []string` and leave it at that
[10:50] <niemeyer> fwereade_: I can easily buy into that too
[10:50] <fwereade_> niemeyer, whether we ultimately pass a Context or an []string to Exec depends on what makes sense in the final context
[10:51] <niemeyer> fwereade_: I don't think it makes sense to pass a context to Exec..
[10:53] <fwereade_> niemeyer, well, it needs Vars and a way to get to the actual hook, which IMO it's sensible to calculate from Context.CharmDir
[10:53] <wrtp> fwereade_: 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 elsewhere
[10:55] <niemeyer> fwereade_: Agreed. This whole design may actually change a bit once we see the rest of what's necessary
[10:55] <niemeyer> fwereade_: It's strange to have CharmDir in Context, for example.
[10:56] <fwereade_> niemeyer, agreed, now you mention it :)
[10:56] <niemeyer> fwereade_: Many of those properties aren't properties of a context.. they are properties of a unit
[10:57] <niemeyer> fwereade_: So it feels a bit like we're trying to reverse engineer how the bottom looks like by looking at the tip
[10:57] <niemeyer> fwereade_: WHich IMO is why we're a bit lost in that back and forth debate
[10:57] <fwereade_> niemeyer, true... but I'm not sure there's such a thing as a context without a unit, even when out-of-band
[10:57] <niemeyer> fwereade_: There's no context without a unit.. but a unit has multiple contexts
[10:57] <fwereade_> niemeyer, true
[10:58] <wrtp> hmm, i wonder what happened there
[10:58] <wrtp> fwereade_: 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)
 fwereade_: Agreed. This whole design may actually change a bit once we see the rest of what's necessary
[10:59] <fwereade_>  fwereade_: It's strange to have CharmDir in Context, for example.
 niemeyer, agreed, now you mention it :)
 fwereade_: Many of those properties aren't properties of a context.. they are properties of a unit
[10:59] <fwereade_> <-- wrtp has quit (Read error: Operation timed out)
 fwereade_: So it feels a bit like we're trying to reverse engineer how the bottom looks like by looking at the tip
[10:59] <fwereade_>  fwereade_: WHich IMO is why we're a bit lost in that back and forth debate
 niemeyer, true... but I'm not sure there's such a thing as a context without a unit, even when out-of-band
 fwereade_: There's no context without a unit.. but a unit has multiple contexts
 niemeyer, true
[10:59] <niemeyer> fwereade_: 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] <wrtp> fwereade_: thanks
[11:00] <fwereade_> niemeyer, sounds good to me
[11:00] <niemeyer> fwereade_: I suspect the correct thing to do will be a lot more obvious once we reach these fundamental layers
[11:00] <fwereade_> niemeyer, yeah
[11:00] <wrtp> vars map[string]string, presumably
[11:00] <niemeyer> fwereade_: 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 concepts
[11:01] <niemeyer> wrtp: That'd work, yeah
[11:03] <fwereade_> wrtp, not sure it matters much either way, not unreasonable to pass vars around in os.Environ style
[11:03] <fwereade_> wrtp, once they're fixed anyway
[11:03] <wrtp> fwereade_: i think os.Environ might be a map now...
[11:03] <fwereade_> wrtp, ofc they're easier to manipulate as map[string]string
[11:03] <wrtp> fwereade_: no, it's not
[11:04] <wrtp> fwereade_: there was *something* like that :-)
[11:05] <fwereade_> niemeyer, wrtp: I'm just going to try to flesh out my current sketch a little and see if any new enlightenment dawns
[11:06] <fwereade_> niemeyer, wrtp: well *actually* it seems I'm going to eat lunch
[11:06] <wrtp> lol
[11:06] <fwereade_> ttyl :)
[11:06] <wrtp> fwereade_: luncheon enlightenment?
[11:06] <fwereade_> haha
[11:06] <fwereade_> by light alone
[11:06] <wrtp> fwereade_: you read that yet?
[11:08] <niemeyer> fwereade_: Lunch.. hmm.. still several hours to go here, how unfortunate :)
[11:08] <niemeyer> fwereade_: Enjoy :)
[11:09] <wrtp> niemeyer: []string is better actually. that's the type of the Env field in exec.Cmd.
[11:13] <niemeyer> wrtp: Right, but it may be easily assembled within Exec.. your suggestion is likely a better interface to work with from within the package
[11:14] <wrtp> niemeyer: it's possible. but i suspect that in practice all the calls will look like: Exec([]string{"FOO="+foo, "BAR="+bar}, ...)
[11:14] <wrtp> niemeyer: that is, i wonder how much we'll actually "work with" the environment variables.
[11:15] <niemeyer> wrtp: That looks like a map to me
[11:15] <wrtp> niemeyer: no more than os.Environ or exec.Cmd.Env :-)
[11:15] <niemeyer> wrtp: Never built one of those by hand.
[11:16] <wrtp> niemeyer: i'm happy to minimise impedance mismatch where it doesn't make much difference.
[11:17] <wrtp> niemeyer: but i don't mind too much (i did suggest a map originally, after all)
[11:17] <niemeyer> wrtp: :-)
[11:18] <TheMue> wrtp: i would uses an own type based on map[string]string here, having methds to pass it as []string
[11:19] <TheMue> wrtp: so the usage is simple and passing the content to Exec() too
[11:19] <wrtp> niemeyer: 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:20] <niemeyer> wrtp: m["ONE_VAR"] = ...
[11:21] <wrtp> niemeyer: that doesn't work if you don't want the map to be changed for everyone
[11:21] <wrtp> niemeyer: you'd have to copy the map first
[11:21] <niemeyer> wrtp: That's not how exec works..
[11:21] <TheMue> wrtp: could be done with my type too. even more comfortable in a method myEnv.WithNewVariable(k, v string) []string
[11:21] <TheMue> wrtp: w/o changing it, using the append internally
[11:22] <wrtp> i think all this is overthinking it. i think in practice the usage will be ultra simple.
[11:22] <niemeyer> Let's please move on. Whatever fwereade_ picks is fine.
[11:22] <wrtp> agreed
[11:22] <niemeyer> wrtp: Yes, and you've been redefining how to work with a map above..
[11:22] <wrtp> niemeyer: ?
[11:22] <niemeyer> wrtp: !
[11:23] <wrtp> [11:21] <niemeyer> wrtp: That's not how exec works..
[11:23] <wrtp> depends whether the map is used in a concurrent context
[11:24] <wrtp> anyway, let's leave it.
[11:24] <niemeyer> wrtp: Both structures are mutable.. a process environment isn't mutable.
[11:26] <wrtp> niemeyer: 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] <niemeyer> wrtp: So will append..
[11:26] <niemeyer> fwereade_: Please use a map.. let's see how it looks like.
[11:27] <wrtp> niemeyer: not if the slice has cap == len. but perhaps that's too fragile.
[11:27] <niemeyer> wrtp: Indeed it is. append mutates the slice.
[11:27] <wrtp> niemeyer: only cap!=len
[11:27] <TheMue> wrtp: take a look at http://paste.ubuntu.com/883111/
[11:28] <niemeyer> wrtp: Indeed it is fragile.
[11:28] <TheMue> wrtp: untested quick hack
[11:28] <niemeyer> wrtp: Gosh..
[11:28] <niemeyer> wrtp: These arguments really consume our time for no good reason.
[11:28] <wrtp> yeah
[11:28] <wrtp> let's not
[11:29] <wrtp> sorry, it's my fault!
[11:30]  * TheMue sighs
[12:13] <TheMue> lunchtime
[12:14] <fwereade_> niemeyer, I think that Exec does actually need a Context... we'll need to flush settings writes once the hook's complete
[12:25] <wrtp> fwereade_: can't that be done by the caller of Exec?
[12:27] <niemeyer> fwereade_: Agree with wrtp's questoin
[12:28] <niemeyer> fwereade_: Also, who calls Exec
[12: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 more
[12:28] <wrtp> niemeyer, fwereade_: is it ok if i leave the log prints in the live test until we hook up logging to the tests?
[12:28] <niemeyer> fwereade_: 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 isolated
[12:28] <wrtp> fwereade_: we had too long a discussion about this earlier. go with what you feel like :-)
[12:29] <fwereade_> wrtp, I question the value of logging those tests at all, really, but I don't feel all that strongly about it
[12:29] <fwereade_> niemeyer, wrtp: cool, cheers
[12:29] <wrtp> fwereade_: the logging only appears in verbose mode. sometimes it'll hang forever and it's useful to know where.,
[12:29] <niemeyer> wrtp: Hooking up logging to the tests is done by log.Target = c in SetUpTest
[12:29] <niemeyer> wrtp: We already do that in some suites
[12:30] <fwereade_> wrtp, ok, cool, that makes sense then
[12:30] <niemeyer> wrtp: I'm happy with the branch to move forward as it is, though, and have that in a separate one
[12:31] <wrtp> niemeyer: i can do that, but i'd need to add log.Printf calls too. i'd prefer to do it in a separate branch
[12:31] <wrtp> yeah
[12:35] <niemeyer> fwereade_: Just as a nice realization, everything in that branch *is* a thing wrapper on top of Exec..
[12:35] <niemeyer> s/thing/thin
[12:37] <fwereade_> niemeyer, agreed, but imo it's sane for "hook.Exec" to know about the particular idiosyncracies of hook execution
[12:37] <fwereade_> niemeyer, it'd be a different matter if it were "util.Exec" or something
[12:37] <niemeyer> fwereade_: Agreed
[12:38] <niemeyer> fwereade_: But there are none there, yet..
[12:39] <niemeyer> fwereade_: I see your point, though.. it may well make sense to have a context there
[12:40] <wrtp> fwereade_, niemeyer: submitted! thanks. 144 commits it took :-)
[12:40] <fwereade_> wrtp, that's gross
[12:40]  * fwereade_ tries to keep a straight face
[12:40] <niemeyer> wrtp: 2 of those are mine! ;-)
[12:41] <wrtp> niemeyer: 2 of your commits say "committer: Roger Peppe" ?
[12:42] <niemeyer> wrtp: Nah, nevermind
[12:42] <wrtp> anyway, i'm very happy to have got there in the end!
[12:43] <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 pun
[12:47]  * wrtp lets the pun sail way over his head.
[12:49]  * wrtp lunches
[12:58] <niemeyer> fwereade_: So, turns out I lied to you.. proposing again with lbox won't mark the branch as Needs Review
[12:59] <niemeyer> fwereade_: I'm fixing that now, but just wanted to mention in case you reproposed some branch before that I'm not seeing!
[13:23] <fwereade_> niemeyer, ah-ha, I suspect I have a branch or two languishing then, let me check
[13:24] <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 though
[13:24] <fwereade_> niemeyer, thanks
[13:32] <andrewsmedina> good morning
[13:33] <niemeyer> andrewsmedina: Heya
[13:33] <niemeyer> fwereade_: np
[13:34] <niemeyer> fwereade_: Already fixing it, so hopefully won't bite in the future
[13:34] <fwereade_> niemeyer, sweet, tyvm
[13:34] <fwereade_> andrewsmedina, good morning
[13:34] <andrewsmedina> niemeyer: I saw the reviews made for you and rog
[13:34] <niemeyer> fwereade_: I hope to fix pre-req right after
[13:35] <fwereade_> niemeyer, <3
[13:35] <niemeyer> fwereade_: *that* will be a real win :)
[13:35] <fwereade_> niemeyer, maybe even <4
[13:35] <niemeyer> andrewsmedina: Super.. sorry for some confusion there
[13:35] <niemeyer> fwereade_: LOL
[13:35] <niemeyer> fwereade_: We should totally start using <4 as a convention :)
[13:36] <fwereade_> niemeyer, I have a vague feeling I stole it from kingdom of loathing
[13:36] <andrewsmedina> niemeyer: I realy need parse the lxc error output?
[13:37] <niemeyer> andrewsmedina: 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 there
[13:37] <niemeyer> andrewsmedina: so
[13:37] <niemeyer> output, err := ...
[13:38] <andrewsmedina> niemeyer: I'm already doing it
[13:38] <niemeyer> if i := bytes.Index(output, []byte("\nUsage: ")); i > 0 {
[13:38] <niemeyer>     output = output[:i]
[13:38] <niemeyer> }
[13:38] <niemeyer> andrewsmedina: That's all really
[13:39] <andrewsmedina> niemeyer: I will do it now
[13:43] <niemeyer> andrewsmedina: Thanks a lot
[13:43] <niemeyer> I promise I won't ask why setStatus is its own method in a merge_proposal object, while everything else is just changing fields
[13:56] <niemeyer> fwereade_, wrtp: I'll also invert the meaning of -same-log, and make it the default behavior, as suggested by wrtp before.
[13:56] <niemeyer> wrtp: 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:57] <fwereade_> niemeyer, I update the log (er, sometimes)
[13:57] <wrtp> niemeyer: ECONTEXT
[13:57] <fwereade_> niemeyer, but I can bear to type a few more chars when I need to ;)
[13:58] <niemeyer> wrtp: Fixing lbox
[13:58]  * wrtp can't remember what command -same-log is a flag to...
[13:59] <wrtp> ah, i remember now
[13:59] <niemeyer> fwereade_: It will continue to be shown on submit too, so that's always a good hooking point to get the log right
[13:59] <wrtp> log==description
[13:59] <fwereade_> niemeyer, excellent
[14:00] <wrtp> i've said this before, but i think it's a pity that so much functionality is shoehorned into "lbox propose"
[14:01] <wrtp> niemeyer: 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:02] <niemeyer> wrtp: Yes
[14:03] <wrtp> niemeyer: 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] <wrtp> each change generates about 3 emails.
[14:04] <niemeyer> wrtp: 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] <wrtp> niemeyer: there's also the two emails that are sent when you actually send the comments on the codereview page...
[14:05] <niemeyer> wrtp: Exactly.. each change generates two emails.
[14:06] <wrtp> niemeyer: it means i can't respond to some comments with some changed code without generating 4 emails.
[14:06] <niemeyer> wrtp: I apologize for the trouble. I don't know how to solve the issue.
[14:06] <wrtp> niemeyer: yeah, i know. maybe i should just filter out all emails that simply say "please take a look"
[14:07] <wrtp> niemeyer: because they're often noise, and the real ones are those sent direct from codereview
[14:07] <niemeyer> wrtp: They're not noise for me, but whatever works for you is certainly fine for me.
[14:08] <niemeyer> wrtp: Many times people don't answer the feedback, and simply do an "lbox propose". You'd miss those.
[14:08] <wrtp> niemeyer: if there was a way of saying "please upload these changes *and* publish my codereview comments", that would be nice...
[14:08] <niemeyer> wrtp: That's already what it does..
[14:08] <wrtp> niemeyer: yeah, i know.
[14:09] <wrtp> niemeyer: really?
[14:09] <niemeyer> wrtp: I believe..
[14:09] <niemeyer> wrtp: Yep.. try it out and let me know :)
[14:09] <wrtp> niemeyer: so lbox propose publishes pending codereview comments, you think?
[14:10] <wrtp> hmm, that would be good, if slightly unexpected.
[14:11] <wrtp> if i could upload changes without sending mail (and also without marking the branch as "work in progress") that would work too.
[14:11] <wrtp> along the lines of go's hg upload.
[14:12] <niemeyer> wrtp: Probably not, actually
[14:12] <niemeyer> wrtp: But it might be doable
[14:12] <niemeyer> wrtp: I have message_only set to true
[14:12] <wrtp> i 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:13] <niemeyer> wrtp: There's no reason for people to be uploading changes to Rietveld when they don't intend those changes to be reviewed
[14:14] <niemeyer> wrtp: If all you want is to push the branch, just "bzr push"
[14:14] <niemeyer> wrtp: lbox propose will.. well.. propose
[14:14] <wrtp> niemeyer: 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:15] <wrtp> niemeyer: or lbox mail :-)
[14:15] <wrtp> if it existed.
[14:15] <niemeyer> wrtp: I'll reserve lbox mail to the moment when lbox becomes an email client.
[14:17] <wrtp> niemeyer: lbox announce, maybe
[14:20] <niemeyer> wrtp: What you want already exists.. lbox propose -prep..
[14:20] <wrtp> niemeyer: but -prep marks the branch as "work in progress"
[14:21] <wrtp> niemeyer: and taking it out of -prep mode sends a mail.
[14:21] <niemeyer> wrtp: Which seems to be exactly what you want.. if you're reviewing your own changes, it's not yet ready for review
[14:24] <wrtp> niemeyer: the problem is that it's not possible to publish codereview remarks and upload code ready for review without sending 4 emails.
[14:24] <wrtp> niemeyer: (i just verified that lbox propose does not submit pending codereview comments)
[14:24] <niemeyer> wrtp: I can fix that easily
[14:24] <wrtp> niemeyer: and also that the default should be to not announce.
[14:25] <wrtp> niemeyer: that way it's harder to make unnecessary noise.
[14:25] <niemeyer> wrtp: We already talked about that. The command "lbox propose" should propose. This won't change.
[14:26] <wrtp> niemeyer: it is editing the proposal. it doesn't have to announce that edit.
[14:26] <wrtp> niemeyer: 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 IMHO
[14:27] <wrtp> niemeyer: it would be great to fix the codereview comments issue BTW
[14:43] <niemeyer> andrewsmedina: Splitting the container names on spaces as you are doing is fine. Please once you're happy with the error stuff, just lbox propose again
[14:44] <andrewsmedina> niemeyer: I'll finish it in time for lunch
[14:46] <niemeyer> andrewsmedina: Thank you
[14:47] <andrewsmedina> niemeyer: Do you have any deadline for the release of the juju Go port?
[14:47] <wrtp> niemeyer: indeed it's a bug in lxc-create. i wondered where that 'b' directory had come from:
[14:47] <wrtp> /usr/bin/lxc-create:123
[14:47] <wrtp> mkdir -p $lxc_path/$lxc_name
[14:47]  * wrtp wishes that the bourne shell had got quoting right.
[14:48] <niemeyer> andrewsmedina: We have some plans, but they're not firm yet
[15:15]  * fwereade_ looks owlishly at allhands.canonical.com and tries to resist the temptation to add himself as a peer reviewer
[15:17]  * wrtp can't log in to allhands.canonical.com
[15:30] <wrtp> fwereade_: 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 plane
[15:31] <hazmat> niemeyer, i did work out how to get py juju to handle session expiration without restart, impl in progress
[15:31] <hazmat> basically tracking watches and ephemerals with the existing expiration trap handlers
[15:31] <fwereade_> wrtp, it strikes me as kinda tedious to do different things on different errors after the fact
[15:32] <wrtp> fwereade_: 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 errors
[15:34] <wrtp> fwereade_: depends how expensive the setup is, i guess
[15:34] <fwereade_> wrtp, hmm, I guess I trust the Stat error to be relevant but not the Run one, which is maybe irrational
[15:34] <wrtp> fwereade_: it is - it works just fine (assuming the path has a separator in)
[15:35] <wrtp> fwereade_: http://paste.ubuntu.com/883400/
[15:35] <fwereade_> wrtp, the path passed to Command?
[15:35] <niemeyer> hazmat: Expiration trap handlers?
[15:35] <wrtp> fwereade_: yeah
[15:35] <niemeyer> hazmat: Greetings, btw :)
[15:35] <niemeyer> hazmat: Back from PyCon?
[15:35] <hazmat> niemeyer, greetings! good to be back
[15:35] <wrtp> fwereade_: (it's documented in exec.Command)
[15:35] <hazmat> niemeyer, indeed. i took the red eye, just got back in a few minutes ago
[15:36] <fwereade_> wrtp, ah, ok; and I guess we get sensible errors for non-executability too?
[15:36] <wrtp> fwereade_: yeah, you'll get "permission denied" which seems fine to me.
[15:36] <fwereade_> wrtp, cool, should be a never-happen anyway I think
[15:37] <wrtp> fwereade_: although you can test with os.IsPermission too if you want
[15:37] <fwereade_> wrtp, we check hook permissions on charms anyway IIRC
[15:37] <wrtp> right
[15:37] <hazmat> niemeyer, 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:38] <wrtp> fwereade_: also, isn't most of the setup done *before* calling Exec?
[15:38] <hazmat> in the latter case we re-establish a session and the prior ephemeral nodes/watches and allow the op to retry
[15:41] <hazmat> i think i still need to work out some details on the last one for ops made during bad connection state
[15:41] <niemeyer> hazmat: You mean recreating watch/ephemeral/etc state without telling the call sites about what is actually going on behind the scenes?
[15:42] <hazmat> niemeyer, yes
[15:42] <niemeyer> hazmat: I'm a bit skeptical about being able to do that in a reliable manner
[15:42] <hazmat> fair enough
[15:43] <hazmat> niemeyer, effectively the call sites will see a watch fire, with the current state, so they will be informed about the current view
[15:43] <hazmat> er. state
[15:44] <niemeyer> hazmat: Yeah, but the whole world may have changed.. every single watch will have to fire somehow
[15:44] <niemeyer> hazmat: Since the event they were waiting for may have occurred meanwhile..
[15:44] <hazmat> niemeyer, indeed
[15:44] <niemeyer> hazmat: Or it may not..
[15:44] <hazmat> niemeyer, we can fire the watch when its restablished
[15:45] <niemeyer> hazmat: 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 state
[15:45] <hazmat> niemeyer, we've already done the latter
[15:46] <hazmat> but you've adovocated that we can do better
[15:46] <niemeyer> hazmat: Kind of..
[15:46] <fwereade_> wrtp, admittedly there's not much setup
[15:47] <niemeyer> hazmat: The unit agent having to reestablish a connection with zookeeper doesn't mean it's fine to disturb the actual software that is running
[15:47] <niemeyer> hazmat: This is the tricky part
[15:50] <niemeyer> hazmat: 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] <hazmat> niemeyer, 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 state
[15:50] <hazmat> niemeyer, yes
[15:50] <hazmat> niemeyer, we can also robustly die ;-) and restart
[15:51] <hazmat> which is what i was planning on.. its a quite a bit simpler
[15:51] <hazmat> but i think this might approach has some merit as well
[15:51] <hazmat> er..
[15:52] <hazmat> s/might/
[15:53] <niemeyer> hazmat: I hope we can robustly die, and restart, and not disturb the charm.
[15:53] <niemeyer> hazmat: and by not disturbing I mean not marking the unit as offline to the relation peers, etc
[15:55] <niemeyer> hazmat: That said, you certainly have freedom for experimentation on that area..
[15:56] <niemeyer> hazmat: 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:57] <hazmat> okay.. 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 inconsistencies
[15:59] <hazmat> niemeyer, 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 though
[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.
[16:00] <wrtp> fwereade_: although it's probably premature optimisation; i can't see this being a bottleneck.
[16:01] <niemeyer> hazmat: Yeah, it's tight
[16:02] <TheMue> fwereade_: 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] <niemeyer> Okay.. lunch time here. Will be back in a bit.
[16:03] <fwereade_> wrtp, fair enough, it doesn't look too bad as it is
[16:03] <fwereade_> TheMue, hum, this is somewhat disturbing
[16:03] <fwereade_> TheMue, you're definitely using the right gozk?
[16:04] <TheMue> fwereade_: which is "the right" gozk?
[16:04] <TheMue> fwereade_: or better, which is the "wrong" one?
[16:04] <fwereade_> TheMue, the current one should be fine, AFAIK
[16:04] <fwereade_> TheMue, the wrong one is any version before rog's locking fixes
[16:05] <TheMue> fwereade_: i updated it together with weekly …03-13
[16:06] <TheMue> fwereade_: now done a fresh go get -u and i still have the same problem
[16:06] <wrtp> fwereade_, TheMue: i may need to do the tagging
[16:06] <wrtp> fwereade_: hold on
[16:06] <wrtp> TheMue: hold on
[16:07]  * TheMue holds on
[16:08] <TheMue> wrtp: btw, seen my little env proposal at http://paste.ubuntu.com/883111/ ?
[16:09] <wrtp> TheMue: yeah, i thought it was a bit overkill tbh. i'm happy going with the []string as in fwereade_'s proposal.
[16:09] <TheMue> wrtp: 13 lines are overkill?
[16:09] <wrtp> TheMue: any additional abstraction can be overkill :-)
[16:10] <TheMue> wrtp: now we got such a fine language to abstract data types w/o much trouble and he calls it overkill ;)
[16:10] <wrtp> TheMue: abstraction is abstraction, and the less of it the better :-)
[16:10] <TheMue> wrtp: so why do you wanted an extra type for the retry values?
[16:11] <TheMue> wrtp: and less it not always better. if it leads to more code duplication and possible failures
[16:11] <wrtp> TheMue: 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 handle
[16:12] <wrtp> fwereade_: my perspective is that the caller of Exec can have sole responsibility for the context.
[16:12] <TheMue> wrtp: is the tagging done?
[16:12] <wrtp> TheMue: yeah, i think so
[16:12] <fwereade_> wrtp, ie that contexts aren't really anything to do with hook execution?
[16:13] <wrtp> fwereade_: that too. the Exec function doesn't care about the context, other than to flush it.
[16:13] <TheMue> wrtp: yep, thx, no ore error
[16:13] <wrtp> TheMue: cool. i must make a script that automatically does the tagging after a submit.
[16:14]  * TheMue still like abstraction that helps. maybe it's my smalltalk history
[16:14] <wrtp> TheMue: 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 persisted
[16:14] <fwereade_> wrtp, I don't think I agree with that
[16:15] <TheMue> wrtp: and here it is the right place. an env is anv, not only a slice of strings
[16:15] <wrtp> TheMue: for exec.Cmd and os, it's a slice of strings.
[16:15] <TheMue> fwereade_: if i'm doing an Alive() on a non-existing node i expect a false. am i wrong here?
[16:15] <wrtp> TheMue: and i'm happy to go along with that view unless it's making things harder
[16:16] <fwereade_> TheMue, no, you're perfectly correct there
[16:16] <wrtp> fwereade_: i'm not saying that
[16:16] <TheMue> wrtp: 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:17] <TheMue> fwereade_: strange, because i'm getting a true
[16:17] <fwereade_> wrtp, that it can just kinda half do it?
[16:17] <wrtp> fwereade_: no, Exec executes the hook. when the hook returns, we flush its context.
[16:17] <fwereade_> TheMue, is this in a test?
[16:17] <wrtp> fwereade_: but the caller of Exec can create the context, call Exec, then flush it
[16:17] <TheMue> fwereade_: in my test. ;) but pls wait, will test it with helpful print statements, not that i'm testing the wrong node
[16:17] <fwereade_> wrtp, in my view context flushing is part of execution -- it's the completion of any relation-sets that happen to be called
[16:18] <fwereade_> TheMue, are you sure you're cleaning zk up properly between tests?
[16:18] <wrtp> fwereade_: schematic: http://paste.ubuntu.com/883487/
[16:18] <fwereade_> TheMue, we don;t seem to have a proper nuke-everything-in-zk function anywhere
[16:19] <wrtp> fwereade_: 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 state
[16:19] <wrtp> fwereade_: yeah, true. it could easily do the flush only if Exec didn't return an error.
[16:20] <fwereade_> wrtp, I still don't see the benefit of putting the flush somewhere else
[16:20] <wrtp> fwereade_: i don't see the benefit of passing a callback into Exec when it's trivial to do the call directly.
[16:21] <wrtp> fwereade_: it makes for a better separation of concerns, i think
[16:21] <wrtp> fwereade_: Exec is solely responsible for running the hook
[16:22] <wrtp> fwereade_: the caller of Exec is responsible for the context management
[16:23] <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:24] <wrtp> fwereade_: 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] <TheMue> fwereade_: even w/o nuke-everything i'm talking here about only one node which get's nuked. and my tests before worked.
[16:24] <wrtp> fwereade_: which, as i think we've agreed, are managed by the caller of Exec
[16:24] <wrtp> fwereade_: we don't flush local machine state...
[16:25] <fwereade_> wrtp, no, because we don't need to do it ourselves
[16:25] <wrtp> fwereade_: and it's flushing that we're concerned with here
[16: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't
[16:26] <fwereade_> wrtp, ...aren't you?
[16:27]  * wrtp is thinking
[16:28] <wrtp> fwereade_: i'd say that the context is flushed *after* the hook returns
[16:29] <wrtp> fwereade_: the relation-set is "done" before exec returns, but relation-set doesn't actually change the relation until the hook returns.
[16:30] <fwereade_> wrtp, in the sense of "after the hook-as-written-by-the-user exits", that's true
[16:30] <fwereade_> wrtp, well, it does, as far as the hook author knows
[16:30] <wrtp> fwereade_: 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 thing
[16:31] <wrtp> fwereade_: 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 locally
[16:31] <wrtp> well, not the first set anyway
[16:31] <wrtp> fwereade_: sure
[16:32] <wrtp> fwereade_: so i think it's logical to have Exec mirror the "hook as written by the author" executing.
[16:33] <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 persisted
[16:34] <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't
[16:35] <fwereade_> wrtp, if you can explain why that line's not arbitrary you may convince me
[16:38] <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 hook
[16:38] <wrtp> fwereade_: it doesn't matter - Exec can be entirely context-agnostic, and that's a good thing
[16:38] <wrtp> fwereade_: here's how i imagine Exec: http://paste.ubuntu.com/883519/
[16:39] <wrtp> fwereade_: a very very simple wrapper around exec.Command
[16:39] <fwereade_> wrtp, ctx.Vars?
[16:39] <wrtp> fwereade_: oh yeah, forgot that bit, one mo
[16:40] <wrtp> fwereade_: http://paste.ubuntu.com/883521/
[16:40] <fwereade_> wrtp, all you're doing is smearing responsibility for... hook execution... into different places
[16:40] <wrtp> fwereade_: i really don't think so
[16:41] <wrtp> fwereade_: Exec *executes the hook*. something else *creates and flushes the context*.
[16:41] <wrtp> fwereade_: no need for a context interface
[16:42] <wrtp> fwereade_: 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] <wrtp> s/folk/fold/
[16:43] <wrtp> fwereade_: presumably the context becomes invalid after the hook has finished execution?
[16:43] <fwereade_> wrtp, why?
[16:43] <fwereade_> wrtp, we reuse contexts in python
[16:44] <wrtp> fwereade_: what happens if some background process uses it later?
[16:44] <wrtp> fwereade_: it'll get the context for a different hook
[16:44] <fwereade_> wrtp, I don't see how that's a danger
[16:45] <fwereade_> wrtp, sometimes we know we want to run 2 hooks immediately after one another
[16:45] <fwereade_> wrtp, and so the context gets reused
[16:45] <fwereade_> wrtp, flushing doesn't invalidate a context
[16:45] <fwereade_> wrtp, but contexts don't live beyond the time they're relevant either
[16:46] <wrtp> fwereade_: 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] <wrtp> fwereade_: we might want to delay flushing until after the second hook has run too...
[16:47] <wrtp> fwereade_: i'm not saying it should.
[16:47] <wrtp> fwereade_: it's outside the purview of Exec
[16:47] <fwereade_> wrtp, so an error in one hook should affect the results of an earlier hook? surely not
[16:48] <wrtp> fwereade_: erm, i think you'd have a problem with that anyway
[16:48] <wrtp> fwereade_: if you reuse a context
[16:48] <wrtp> fwereade_: then the dodgy settings from the failed hook will continue on into the next hook
[16:49] <fwereade_> wrtp, if the hook fails we won't continue on to the next one
[16:49] <fwereade_> wrtp, you just suggested we should delay the flush
[16:49] <fwereade_> wrtp, so you get hook 1 writing state, hook 2 failing, and hook 1's apparent success not really sticking
[16:49] <wrtp> fwereade_: ah, i see.
[16:50] <fwereade_> wrtp, you seemed to be arguing from a position in which flushing and creation were linked somehow -- did I misunderstand that?
[16:50] <wrtp> fwereade_: no, i'm arguing that Exec shouldn't concern itself with the context at all
[16:50] <wrtp> fwereade_: because it's trivial for Exec's caller to do that.
[16:51] <wrtp> fwereade_: it makes Exec simpler at no cost, AFAICS
[16:52] <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 call
[16:53] <wrtp> fwereade_: it's more than just moving code - it removes code (the ExecContext type and that argument to Exec)
[16:54] <fwereade_> wrtp, it's 1 less param and 1 fewer lines in a not-very-big function, in exchange for an arbitrary division of responsibilities
[16:54] <fwereade_> wrtp, ExecContext is only there because Context is not yet implemented
[16:55] <fwereade_> wrtp, I *do* kinda want to keep it around beyond this branch, for simplicity of testing, but that's not critical really
[16:56] <fwereade_> wrtp, and that one saved line has to be called somewhere else anyway
[16:56] <wrtp> fwereade_: 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:58] <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] <wrtp> fwereade_: 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 returns
[16:58] <wrtp> fwereade_: sure. we'll be checking the error anyway
[16:59] <fwereade_> wrtp, and no it's not just dependent on whether there's an error returned
[16:59] <wrtp> fwereade_: no?
[16:59] <fwereade_> wrtp, if the hook doesn't exist we don;t bother to flush
[16:59] <fwereade_> wrtp, but that's not an error
[17:00] <wrtp> fwereade_: interesting point.
[17:00] <fwereade_> wrtp, ok, we *could* just always flush, it's basically be a no-op if no state changed
[17:00] <wrtp> fwereade_: it's a performance issue
[17:00] <wrtp> fwereade_: but if that *is* an issue, then yes, it's a good reason for flush to stay inside Exec.
[17:00] <wrtp> fwereade_: although...
[17:01] <fwereade_> wrtp, I doubt it's significant, I imagine ConfigNode is smart enough not to round-trip if there have been no changes
[17:01] <wrtp> fwereade_: 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 that
[17:02] <fwereade_> wrtp, why make the rest of the code care about hook existence?
[17:02] <TheMue> fwereade_: 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:03] <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-checking
[17:03] <wrtp> [16:00] <wrtp> fwereade_: although it's probably premature optimisation; i can't see this being a bottleneck.
[17:04] <fwereade_> wrtp, still, why does it matteratall to anything otherthan the hook executor whetheror not the hook exists?
[17:04] <wrtp> fwereade_: anyway, i think Exec still serves a useful role - it sets up environment variables and it knows where hooks live &c
[17:05] <wrtp> fwereade_: just a matter of efficiency, i think.
[17:05] <wrtp> fwereade_: maybe Exec should be a method on a Charm type.
[17:05] <niemeyer> fwereade_: I suggest dropping Context from that branch entirely, so we can submit it, and then bring on another branch
[17:05] <wrtp> fwereade_: then the Charm type could do some initial introspection when created.
[17:06] <wrtp> niemeyer: +1
[17:06] <fwereade_> wrtp, but you've also moved responsibility for env var setup outside exec in your last paste
[17:06] <wrtp> fwereade_: all the stuff in ExecInfo.Vars would still be around
[17:06] <niemeyer> fwereade_: 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 context
[17:07] <niemeyer> Erm.. overuse of "context" there, but you see what I mean
[17:07] <wrtp> fwereade_: it's just that ExecInfo would also have ExtraVars (say) giving env vars that aren't inferrable from ExecInfo
[17:07] <wrtp> it'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:08] <wrtp> fwereade_: isn't that what ExecContext.Vars did?
[17:08] <niemeyer> wrtp: 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] <wrtp> niemeyer: yeah, that sounds right.
[17:08] <niemeyer> fwereade_: 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:10] <fwereade_> niemeyer, ok, I'll try to find some way to do so sanely :/
[17:11] <fwereade_> I still just do not get this insistence that Exec shouldn't actually finish hook execution
[17:11] <niemeyer> fwereade_: Exec in that branch is a self contained problem, very well isoalted.
[17:12] <niemeyer> fwereade_: 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:13] <niemeyer> fwereade_: 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] <niemeyer> fwereade_: Bring it back in the next branch, if needed be
[17:14] <fwereade_> niemeyer, ok then
[17:14] <Pikkachu> what's juju?
[17:15] <wrtp> Pikkachu: https://juju.ubuntu.com/
[17:16] <Pikkachu> ah ok nevermind
[17:19] <TheMue> fwereade_: got my question regarding StartPinger() above?
[17:19] <fwereade_> TheMue, whoops, sorry
[17:20] <TheMue> fwereade_: i didn't expected the ZNONODE
[17:20] <fwereade_> TheMue, StartPinger doesn;t necessarily create a node
[17:20] <fwereade_> TheMue, but, yes, it should if it doesn't exist
[17:20] <TheMue> fwereade_: yes, that's what i expected when reading your code
[17:21] <fwereade_> TheMue, and, yes, it depends on the parent existing
[17:22] <TheMue> fwereade_: the parent exists, but the node itself not, yes
[17:23] <fwereade_> TheMue, but there aren't any errors?
[17:24] <TheMue> fwereade_: 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:25] <fwereade_> TheMue, ZNONODE back from where?
[17:26] <TheMue> fwereade_: from StartPinger()
[17:27] <fwereade_> TheMue, it *looks* like the only place that could be coming from is the Create in changeNode.change, can you confirm that?
[17:28] <wrtp> anyone interested in helping me discuss a problem i've got with the ec2test server?
[17:28] <TheMue> fwereade_: just tested explicitely, the parent exists
[17: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:31] <TheMue> fwereade_: will have to add some print statements, one mom pls
[17:38] <fwereade_> wrtp, niemeyer: btw, https://codereview.appspot.com/5753057 is now context-free
[17:41] <TheMue> fwereade_: 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] <niemeyer> fwereade_: LGTM, sorry for the pain on this one.
[17:42] <fwereade_> niemeyer, no worries, there's clearly something I haven't managed to communicate but I still can't quite figure out what ;)
[17:43] <niemeyer> fwereade_: We're moving forward.. it's all good
[17:43] <fwereade_> niemeyer, yeah :)
[17:43] <wrtp> fwereade_: LGTM, likewise on the pain.
[17:43] <fwereade_> wrtp, cheers :)
[17:44] <wrtp> niemeyer: i've come up against a difficult issue doing the delayed ec2test update stuff.
[17:44] <niemeyer> fwereade_:
[17:44] <niemeyer> % lbox submit -adopt
[17:44] <niemeyer> Branches: lp:~fwereade/juju/go-tweak-supercommand => lp:juju/go
[17:44] <niemeyer> Requires: lp:~fwereade/juju/go-add-cmd-context
[17:44] <niemeyer> Proposal: https://code.launchpad.net/~fwereade/juju/go-tweak-supercommand/+merge/96136
[17:44] <niemeyer> error: Pre-requisite lp:~fwereade/juju/go-add-cmd-context not yet merged.
[17:44] <niemeyer> fwereade_: One side is done.. :)
[17:44] <fwereade_> niemeyer, yay!
[17:44] <wrtp> yay! +1
[17:44] <andrewsmedina> niemeyer: I'm having a problem with lbox
[17:44] <niemeyer> wrtp: What's up?
[17:45] <niemeyer> andrewsmedina: What's up?
[17:45] <niemeyer> :)
[17:45] <wrtp> niemeyer: here's a summary: http://paste.ubuntu.com/883633/
[17:45] <andrewsmedina> niemeyer: http://dpaste.org/eEmUR/
[17:45] <wrtp> niemeyer: basically it's hard to know how eventual consistency works
[17:45] <wrtp> niemeyer: and i'm not sure that my simplistic idea for modelling it is sufficient
[17:46] <niemeyer> andrewsmedina: Authentication is failing for some reason
[17:47] <niemeyer> andrewsmedina: I suggest removing ~/.lpad_oauth and ~/.goetveld_* and trying again
[17:48] <niemeyer> wrtp: What's that text reflecting, more precisely?
[17:48] <TheMue> fwereade_: 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] <niemeyer> wrtp: Assumptions, experiments, ..?
[17:48] <wrtp> niemeyer: a sequence of actions.
[17:49] <niemeyer> wrtp: I can tell that part.. but it's still a number of bulleted points.. what's the introduction to those actions.
[17:49] <wrtp> niemeyer: difficult to experiment when we're concerned with non-deterministic behaviour
[17:49] <niemeyer> wrtp: I'm just trying to reverse engineer what you're showing me.. :)
[17:50] <wrtp> niemeyer: 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] <wrtp> niemeyer: "transaction" isn't the right word there
[17:51] <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 possibility
[17:51] <wrtp> niemeyer: 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] <andrewsmedina> niemeyer: same problem
[17:52] <niemeyer> wrtp: 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] <niemeyer> andrewsmedina: You've reauthenticated, and it's failing even then?
[17:52]  * wrtp refers niemeyer to the last paragraph.
[17:52] <andrewsmedina> niemeyer: yes :(
[17:52] <fwereade_> TheMue, you should certainly be explicitly stopping/killing the pinger if you don't want it to hang around
[17:53] <wrtp> niemeyer: i can't see how i can make something which can reconcile both views - one fully consistent and the other not.
[17:53] <wrtp> niemeyer: (with just a simple queue)
[17:53] <niemeyer> wrtp: Let's do this.. let's ignore this for the moment
[17:53] <niemeyer> wrtp: and let's move on..
[17:53] <wrtp> niemeyer: ignore this set of changes to ec2test?
[17:54] <wrtp> niemeyer: i'm happy to do that. it seems... difficult.
[17:54] <wrtp> niemeyer: i can shelve what i've done for now.
[17:54] <niemeyer> wrtp: 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] <wrtp> niemeyer: yeah, i think so.
[17:54] <wrtp> niemeyer: and i think that's why i've been finding it difficult to make good progress with it recently.
[17:55] <niemeyer> wrtp: 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] <wrtp> niemeyer: agreed.
[17:55] <wrtp> niemeyer: in that case, i think my next, smallish change will be to add better error messages to the zookeeper package.
[17:55] <TheMue> so, off for today, bye
[17:56] <wrtp> TheMue: night night
[17:56] <niemeyer> TheMue: Have a good evening!
[17:56] <niemeyer> wrtp: Sounds fantastic
[17:56] <wrtp> niemeyer: i thought about renaming Error to ErrorCode and having type Error struct {Code ErrorCode; Path string}
[17:56] <niemeyer> andrewsmedina: Ok.. hmm
[17:56] <niemeyer> andrewsmedina: What's the "date" command telling you right now?
[17:57] <andrewsmedina> I'm using the last weekly
[17:57] <andrewsmedina> release yesterday
[17:57] <andrewsmedina> *released
[17:57] <niemeyer> wrtp: Sounds reasonable
[18:04] <wrtp> fwereade_: you missed this minor... // Exec executes the named hook in the environment defined by ctx and info.
[18:05] <fwereade_> wrtp, blast, sorry
[18:05] <wrtp> fwereade_: 's fine... next time.
[18:05] <fwereade_> wrtp, cheers :)
[18:07] <fwereade_> gents, I'm off for the night, take care
[18:08] <wrtp> fwereade_: g'night
[18:12] <niemeyer> fwereade_: Have a good one indeed
 andrewsmedina: What's the "date" command telling you right now?
[18:13] <andrewsmedina> niemeyer: Wed Mar 14 14:07:51 BRT 2012
[18:13] <niemeyer> andrewsmedina: The error message is right.. your system time is bogus
[18:16] <andrewsmedina> :(
[18:16] <niemeyer> andrewsmedina: Just fix it.. should be easy :)
[18:17] <andrewsmedina> niemeyer: yes
[18:32] <niemeyer> Breaking for a moment
[18:36] <wrtp> i'm away for the evening.
[18:37] <wrtp> see y'all tomorrow
[18:56] <niemeyer> wrtp: Apparently the submit-with-comments is actually working with current lbox: https://codereview.appspot.com/5753057/
[18:56] <niemeyer> wrtp: Erm, propose-with-comments
[18:57] <niemeyer> Oh, or maybe not.. this is a different code path
[18:58] <niemeyer> Ah, not really.. it's the same mechanism.. should be working indeed
[19:23] <andrewsmedina> niemeyer: https://codereview.appspot.com/5764043/
[19:35] <niemeyer> andrewsmedina: Looks great, thank you
[19:36] <niemeyer> andrewsmedina: I suspect "usage" there needs a capital "U"
[19:36] <andrewsmedina> niemeyer: tonight I will start a local environment
[19:36] <andrewsmedina> niemeyer: from lxc no
[19:36] <niemeyer> andrewsmedina:
[19:36] <niemeyer> % lxc-start --blah
[19:36] <niemeyer> lxc-start: unrecognized option '--blah'
[19:36] <niemeyer> Usage: lxc-start --name=NAME -- COMMAND
[19:36] <andrewsmedina> niemeyer: http://dpaste.org/kKOdM/
[19:37] <niemeyer> andrewsmedina: Ouch
[19:39] <niemeyer> andrewsmedina: Looks like we'll need to parse for both..
[19:39] <niemeyer> andrewsmedina: Also, Output() there won't work, as it's dropping stderr
[19:41] <niemeyer> andrewsmedina: You can use CombinedOutput() instead
[19:41] <andrewsmedina> niemeyer: I can use Run and set stderr an stdout
[19:41] <niemeyer> andrewsmedina: CombinedOutput does that for you
[19:41] <andrewsmedina> niemeyer: nice!
[19:44] <andrewsmedina> niemeyer: can You run "lxc-version" and send me the output
[19:44] <andrewsmedina> ?
[19:55] <niemeyer> andrewsmedina: 0.7.5
[19:56] <andrewsmedina> niemeyer: also here
[19:56] <andrewsmedina> but I'm using centos
[19:57] <niemeyer> andrewsmedina: The issue is that the usage message isn't consistent
[19:58] <niemeyer> andrewsmedina: I also have both locally
[19:59] <andrewsmedina> yep