[22:12] <fwereade> waigani___, heyhey
[22:12] <waigani_____> fwereade: hello :)
[22:13] <fwereade> waigani___, how's it going?
[22:13] <waigani_____> getting there
[22:13] <fwereade> waigani___, sorry I haven't really said hi properly since you joined :(
[22:13] <waigani_____> a LOT of code to get my head around!
[22:13] <fwereade> waigani___, ha, yes indeed
[22:13] <waigani_____> no problem, totally understandable
[22:13] <waigani_____> ditto, sorry for not saying hi :)
[22:14] <waigani_____> so, I'm getting racy hey?
[22:14] <fwereade> waigani___, I'm kinda going to bed shortly but I just wanted to chat briefly about the set environ config stuff
[22:14] <waigani_____> sure
[22:17] <waigani____> fwereade: hangout or irc?
[22:17] <fwereade> waigani___, the issue is mainly that I don't really want it to be possible to set an invalid environ config, and I am not convinced that it's possible to guarantee the sanity of a given change without (1) validating the change from the previous version --which is fine, you do it-- and (2) asserting in state that the previous version, from which the change is known to be valid, still applies
[22:17] <fwereade> waigani____, I think I can irc it, I will be off to bed in a mo
[22:18] <waigani____> ok
[22:18] <fwereade> waigani____, does that make sense at first reading or shall I expand a little? ;)
[22:19] <waigani____> right, so the real issue is the last one
[22:19] <waigani____> how do you ensure the oldconfig is itslef valid
[22:19] <fwereade> waigani____, if you can guarantee every change is valid, I think yu can guarantee that the previous version was
[22:20] <fwereade> waigani____, because you cannot bootstrap but with a valid config
[22:20] <waigani____> can we?
[22:20] <waigani____> right
[22:20] <waigani____> in which case the current method works then?
[22:21] <waigani____> we can rely on an oldconfig
[22:21] <waigani____> oohhhh
[22:21] <waigani____> lol
[22:21] <waigani____> i see
[22:21] <waigani____> THAT is why you don't want to allow setconfig without validating
[22:22] <fwereade> waigani____, the current method is (at least in theory) crack, because the *actual* change we end up making is pretty random -- see state/state.go:264
[22:23] <waigani____> so this is the race condition you mentioned?
[22:24] <fwereade> waigani____, in *practice* it's usually ok, but that's because we don't get many concurrent updates to environ config
[22:24] <fwereade> waigani____, yeah, I think so
[22:24] <fwereade> waigani____, I have a tendency to froth about a wide range of possible race conditions
[22:24] <waigani____> heh
[22:25] <fwereade> waigani____, ah, but, yes, that's the one
[22:25] <fwereade> waigani____, so what's interesting is that axw is doing some work that involves using an Environ inside state
[22:25] <waigani____> what does axw mean when he says The settings changes are actually applied as a delta to what's on disk
[22:26] <waigani____> "applied as a delta"?
[22:26] <fwereade> waigani____, there's a Settings type
[22:26] <fwereade> waigani____, it was written in python for zookeeper and ported straight without much consideration for whether it still makes sense
[22:26] <fwereade> waigani____, hint: it doesn't
[22:26] <waigani____> lol, okay...
[22:27] <fwereade> waigani____, I am struggling to remember the *exact* details
[22:27] <fwereade> waigani____, but it is itself fundamentally racy
[22:27] <waigani____> don't worry, I'll follow up
[22:27] <fwereade> waigani____, it reads existing state, calculates delta, applies delta
[22:27] <waigani____> now I know the direction to look in
[22:28] <fwereade> waigani____, pretty sure it doesn't assert the base state still holds
[22:28] <waigani____> now state == mongodb?
[22:28] <fwereade> waigani____, yeah, take a look at that for a bit of context
[22:28] <fwereade> waigani____, yep
[22:28] <waigani____> i.e. config stored in mdb
[22:29] <fwereade> waigani____, but have a word with axw, because *he* is doing some work that involves using an Environ inside state to check sanity of AddMachine ops by checking with the provider
[22:29] <fwereade> waigani____, and *you* are doing something that involves checking sanity of applying changes to Environ (configs)
[22:30] <waigani____> will do.
[22:30] <waigani____> would you like to see the ApplyAndValidate method I've refactored out, added back into SetEnvironConfig?
[22:31] <waigani____> Such that you cannot set an environ config without validating it?
[22:31] <fwereade> waigani____, I think that is at least part of it, but be suspicious of what actually happens when you try to use that settings type
[22:32] <waigani____> fwereade: be suspicious of SetEnvironConfig?
[22:32] <fwereade> waigani____, it may be that you're best off using entirely different code that just happens to use the same data format
[22:32] <fwereade> waigani____, more of state.Settings
[22:33] <fwereade> waigani____, ...or, heh, maybe converting it to use a new data format
[22:33] <fwereade> waigani____, I'm also pretty sure that the fields are dumped straight into the mongo document
[22:33] <waigani____> fwereade: just saw your comment: TODO(fwereade) state.Settings is itself really problematic in just about every use case
[22:33] <fwereade> waigani____, and by sheer conincidence magic fields like txn-revno happen not to collide with actual env settings
[22:34] <fwereade> waigani____, I think what I'm saying is that you have stumbled into a rabbit hole
[22:34] <fwereade> waigani____, do not feel obliged to fix everything
[22:34] <waigani____> I was just about to say the same!
[22:35] <fwereade> waigani____, but be careful to understand what you're fixing and what you're not, and please document it clearly
[22:35] <fwereade> waigani____, take heart: you're unlikely to make that specific code *worse*
[22:35] <waigani____> obviously my understanding is cursory, so I'll follow up with axw and look into state.Settings
[22:35] <fwereade> waigani____, and I'll be happy if it just gets a little bit better
[22:36] <fwereade> waigani____, awesome, thanks
[22:36] <waigani____> are there any other hits for me?
[22:36] <waigani____> bits of code I should look at?
[22:36] <fwereade> waigani____, for this particular stuff it's quite localised
[22:37] <fwereade> waigani____, there are a couple of other Settings clients
[22:37] <fwereade> waigani____, one day we'll get rid of them all
[22:37] <waigani____> okay
[22:37] <fwereade> waigani____, but it's the env config that really upsets me
[22:38] <fwereade> waigani____, because it's the most potentially horrible one ;)
[22:38] <fwereade> waigani____, anyway I should get some sleep
[22:38] <waigani____> just before you go .... :D
[22:38] <fwereade> waigani____, I might be a little late tomorrow, but dimitern is staying here, so if you see him and want to talk to me tell him to come shout at me :)
[22:39] <fwereade> waigani____, I'm still here :)
[22:39] <waigani____> could you give me one example of the race conditon?
[22:39] <waigani____> two clients trying to set the config at the same time? Is that right?
[22:39] <fwereade> waigani____, yeah
[22:40] <waigani____> clients = machine agent, juju cli, ...
[22:40] <fwereade> waigani____, ah!
[22:40] <fwereade> waigani____, there *is* a replaceSettingsOp func
[22:40] <fwereade> waigani____, that I think is not racy
[22:40] <waigani____> oh
[22:40] <fwereade> waigani____, it may need to be retried
[22:40] <fwereade> waigani____, but if you use it the txn will abort
[22:41] <waigani____> okay, lots to learn ... I'll let you sleep
[22:41] <fwereade> waigani____, so, *if* you can construct a known-valid config, you can use replaceSettingsOp to be sure it actually lands in state
[22:41] <waigani____> thanks for taking the time to walk me through the rabbit whole, a little ;)
[22:41] <fwereade> waigani____, but to construct a known-valid one inside state, you'll need to talk to axw
[22:41] <waigani____> *hole
[22:41] <fwereade> waigani____, because
[22:41] <fwereade> (you're sitting down, right)
[22:42] <waigani____> hehe yep
[22:42] <fwereade> waigani____, just because it's a valid config.Config
[22:42] <fwereade> waigani____, it is *not* necessarily a valid config for any given provider
[22:42] <waigani____> ugh
[22:42] <fwereade> waigani____, let alone a valid change for that specific environ
[22:42] <waigani____> what?
[22:43] <waigani____> I get that it might not be valid for other providers
[22:43] <fwereade> waigani____, simplest case: some fields are immutable, or should be
[22:43] <fwereade> waigani____, change an env's name from foo to bar, bad things will happenb
[22:43] <fwereade> waigani____, constructing an env, and setting the new config, *should* catch all those cases
[22:43] <waigani____> so that should be caught as invalid by the environ provider validator?
[22:44] <waigani____> right
[22:44] <fwereade> waigani____, *but* the environs package uses the state package, so you can't directly reference an Environ as such from code inside state
[22:44] <fwereade> waigani____, axw is dealing with *exactly* the same problem, creating environs inside state to validate machine addition
[22:45] <fwereade> waigani____, so it would be good if your solutions were roughly aligned :)
[22:45] <fwereade> waigani____, was that a bit more helpful?
[22:46] <waigani____> yes, you've set me in the right direction
[22:46] <fwereade> (ok, it's not *exactly* the same problem, but you can surely talk productively)
[22:46] <waigani____>  I'll go annoy axw now  ;)
[22:46] <fwereade> waigani____, cool, cheers
[22:47] <waigani____> thanks again, sleep well
[22:47]  * fwereade disappears