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