fwereade_TheMue, heyhey06:44
TheMuefwereade_: heya06:45
rogfwereade_, TheMue: hiya07:20
fwereade_rog, heyhey07:20
TheMuerog: hi07:21
TheMuerog: impl of environ config is - almost - done, i'm currently change the testing. here Config behaves different than ConfigNode07:21
rogTheMue: great! thanks for doing it.07:22
TheMuerog: one question here07:23
TheMuerog: is SetEnvironConfig intended to update or to replace the config?07:24
rogTheMue: yes, i believe so07:24
rogTheMue: for State anyway07:24
rogTheMue: it's not intended to replace ConfigNode in general, i think07:24
TheMuerog: no, only this one method. so the passed config replaces an existing one, ok.07:26
rogTheMue: oops, sorry i answered the wrong question07:26
rogTheMue: i *think* it should replace the config07:27
rogTheMue: as it's usually set all at once. but i'm not so sure07:27
rogTheMue: interesting question.07:27
TheMuerog: i think so too, otherwise by updating you only can add more values but never remove them07:27
rogTheMue: the issue i'm thinking is that if one client updates the environment settings and the other does an upgrade (setting the proposed version), then one or other of the changes may be lost.07:29
TheMuerog: cfg(a, b, c).update(cfg(b, c, d)) => cfg(a, b', c', d)07:29
TheMuerog: oh, yes, would be an issue too07:29
TheMuerog: but the same would be in case of an update by two clients07:30
rogTheMue: i'm not sure07:30
rogTheMue: what i'm worried about is: c := cfg(a=1); {c.set(a=2)} in parallel with {c.set(b=1)}07:31
TheMuerog: client a thinks b has to be b' and updates, a millisecond later client b thinks b (the old value he remembers) has to be b'' and updates too. the first update is lost.07:31
rogTheMue: that's true, but in this case we're dealing with disjoint sets of attributes.07:32
TheMue_shit, network agaiin07:33
TheMue_rog: ok, as long as keys are different an update mixing the value sets is ok. but updating the same key is still a problem.07:33
rogTheMue: the environ config that we set when changing provider attributes doesn't hold the proposed version attribute.07:33
rogTheMue_: of course, but that's inevitable07:34
rogAram: hiya07:34
TheMue_Aram: moin ;)07:34
rogTheMue_: it's reasonable that someone might want to do an upgrade at the same time as updating the provider attrinbutes07:34
rogTheMue_: i'm becoming less convinced that it's appropriate to lump the proposed version in with the provider attributes07:35
TheMue_rog: a kind of txn.exec(func(cfg)) would be nice, so the whole change based on the current config executes in a txn07:36
=== TheMue_ is now known as TheMue
rogTheMue_: actually, we do have that kind of thing. i think we might be ok. let me check.07:36
TheMuerog: i have to leave for some time in a few minutes, so you've time to rethink it07:37
rogTheMue: ok07:37
rogTheMue: i think it depends on how the API to changing environment settings works07:39
rogfwereade: do you know what mechanism the user will use to change provider settings in the state? (i'm presuming that the client is the only thing that will change them - is that right?)07:42
TheMuerog: let's talk about it later07:42
* TheMue is now afk, bbl07:42
rogTheMue: k07:42
fwereaderog, um, I think there's an env-set command planned08:00
fwereaderog, constraints will interact somehow; python already has set-constraints which allows for env-level settings08:01
rogfwereade: that's good. i don't think the user would expect attributes they don't provide on the command line to be deleted.08:01
fwereaderog, damn straight :)08:01
rogfwereade: but the SetEnvironConfig method kind of implies that08:02
fwereaderog, what's the justification for that method?08:02
fwereaderog, surely all we ever want is some form of update?08:02
rogfwereade: i *think* we already have something similar. but maybe not.08:03
rogfwereade: i agree.08:03
fwereaderog, what we originally had was just a ConfigNode, but I haven't been following the updates very closely08:03
rogfwereade: yeah.08:04
rogfwereade: we're going to replace that with a config.Config08:04
fwereaderog, ok, indeed, that's a good thing to get08:04
rogfwereade: but there's no way of setting attributes on a config.Config, so the interface will have to be SetEnvironConfig(*config.Config)08:04
fwereaderog, my heart still says we want an UpdateEnvironConfig method08:05
rogfwereade: and it seems perhaps a little surprising that if you do that, then no attributes will ever be deleted.08:05
fwereaderog, what attributes might we want to delete?08:05
rogfwereade: signature?08:05
rogfwereade: i don't think we do08:05
fwereaderog, hold on, a bell is ringing08:06
fwereaderog, maybe SetEnvironConfig *is* ok, if we can trust ourselves to build a provider-valid config before we call it08:06
rogfwereade: it's not ok if it does a replace not an update08:07
fwereaderog, ha, concurrent updates08:07
rogfwereade: yup08:07
fwereaderog, hm, it could still use a confignode internally and work right08:08
rogfwereade: not really. because we don't know what attributes to delete.08:08
fwereaderog, what's the use case for deleting attributes?08:08
rogfwereade: i think we should have UpdateEnvironConfig(*config.Config) and document that attributes are never deleted.08:09
rogfwereade: 'cos yeah i don't see a use case for deleting attributes.08:09
fwereaderog, sounds sane to me08:09
rogfwereade: are you planning to add more methods to UniterSuite?11:21
rogfwereade: more Test methods, that is11:21
fwereaderog, no, I think I'll just be adding table entries11:21
rogfwereade: currently we've got TestUniter only, but SetUpSuite writes stuff to VarDir. unfortunately JujuConnSuite sets up environs.VarDir on a per-test basis, not a per-suite basis11:22
rogfwereade: actually, perhaps i'll just do the tools building in SetUpTest instead of SetUpSuite.11:22
rogfwereade: it makes me wish for more flexible fixtures...11:23
fwereaderog, yeah, I know the feeling11:23
fwereaderog, I have a vague recollection of being aware of that and deciding it didn't matter -- I guess it does for what you're doing?11:23
rogfwereade: i'm wondering if fixtures should all conform to interface{SetUp(c *gocheck.C); TearDown(c *gocheck.C); Reset(c *gocheck.C)}11:24
rogfwereade: JujuConnSuite did not set environs.VarDir before11:24
TheMuerog: i've just read that UpdateEnvironConfig() makes more sense? would be fine for me.11:24
fwereaderog, ah, ok11:24
rogTheMue: cool11:24
fwereaderog, I do rather like that potential fixture style, yes11:25
rogfwereade: i might run the idea past niemeyer some time11:25
rogfwereade: actually, it seems that the above interface would not be sufficient for the use case in UniterSuite. the JujuConnSuite is reset for every test in the table, but that makes a new VarDir; whereas we want to keep the same jujuc executable around for all the tests.11:44
rogfwereade: so actually i think that suite really does want to set VarDir itself.11:44
rogfwereade: (took me a while to realise what was going on!)11:45
rognote for the future: this is not a good idea: c.Logf("testlog: %q", c.GetTestLog())11:57
rogi wondered why i was seeing sequences of 255 backslashes11:58
rog256 presumably11:58
fwereaderog, huh, did I do that?12:01
rogfwereade: no, i did, trying to debug a failing UniterSuite test12:01
fwereaderog, ah, good, because I'm pretty sure I felt the temptation to the other day, was wondering if I'd blacked out and checked it in or something ;p12:02
rogfwereade: i'm sure the cause is something simple, but currently i can't see the wood for trees12:02
rogfwereade: do you have an instant gut feeling for what kind of thing might be causing this failure? http://paste.ubuntu.com/1173708/12:11
Aramlsr | grep test | grep -v internal | grep -v confignode | grep -v life | xargs grep '(Update(Id)?)|(Find(Id)?)|(Insert(Id)?)|(Upsert(Id)?)'12:11
Aramwrong focus12:11
rogfwereade: it's almost certainly to do with something in the environment that hasn't been set up correctly.12:12
TheMuerog: so, implementation and test in state are done, now check for not yet covered dependencies. had to fight with the schema for test ;)12:14
rogTheMue: cool. i'm struggling trying to merge trunk into a LGTM'd branch :-(12:26
TheMuerog: *lol* yeah, sometimes hard12:26
rogTheMue: the uniter tests are broken somehow, and it's too subtle for me to see yet12:27
TheMuerog: strange. no useful hints?12:27
AramTheMue: thanks for the work you did on mstate, I merged all txn stuff and pushed it.12:28
rogTheMue: no clue yet. am delving to see what's actually going on in the tests.12:28
TheMueAram: yeah, took already a look. it's doing great steps into the right direction.12:28
fwereaderog, sorry, I was having lunch12:53
niemeyerGooood morning12:53
rogfwereade: np12:53
rogniemeyer: hiya12:53
fwereaderog, and, hum, I'd usually expect to see rather more in the logs than just that, looks almost like there's no uniter at all12:53
fwereadeniemeyer, heyhey12:54
fwereademan, I think there's a thunderstorm on its way, I'm feeling ridiculously flat12:54
rogfwereade: it's failing in startupError, doing waitUnit for hook failed just after createUniter{}12:54
fwereaderog, yeah, I saw that much... and I didn't see any of the uniter logging at all in that paste12:55
rogfwereade: i changed the tests so that everything gets run with this function; that way i can see substeps:12:55
rogfunc step(c *C, ctx *context, s stepper) {12:55
rogc.Logf("%#v", s)12:55
rogs.step(c, ctx)12:55
fwereaderog, ISTM like the uniter is failing before it even hits a mode12:56
rogfwereade: hmm, maybe the LoggingSuite isn't being hooked in right12:56
fwereaderog, otherwise I'd expect an "examining charm state..."12:56
fwereaderog, possibly something is up with the tools?12:57
rogfwereade: entirely possible12:57
fwereaderog, NewUniter calls esnureFs which calls EnsureTools12:57
fwereaderog, except hmm *that* surely ought to be caught :/12:57
fwereaderog, yeah, startUniter checks that :/12:58
rogfwereade: the logger seems to be hooked in ok12:58
rogfwereade: here's the full test output BTW: http://paste.ubuntu.com/1173763/12:59
fwereaderog, all I can say is that something is very up, because test 3 at *least* ought to be giving you log output13:00
rogfwereade: what log messages should i be seeing?13:00
* rog has an idea13:01
TheMuerog: is there a way to push an invalid value (not matching the schema) into config.Config?13:02
rogTheMue: which schema?13:02
TheMuerog: i have to invalidate it for a test13:02
fwereaderog, something more like http://paste.ubuntu.com/1173766/13:03
TheMuerog: environ config13:03
rogfwereade: the log hooking is being disabled after Reset!13:03
fwereaderog, ahhhhh13:03
TheMuerog: i want to set name = 1, the provider test has done it before to see how the provider reacts13:03
fwereaderog, then that would definitely explain it :)13:03
TheMuerog: and the old usage of the config node allowed it to be so mean ;)13:04
rogTheMue: you'll have to go behind the scenes, i'm afraid13:04
TheMuerog: *sigh*13:04
rogTheMue: kinda the point of config.Config is that it always matches the schema13:04
TheMuerog: expected this answer, but hoped for an easier one :|13:05
rogfwereade: all tests pass. phew. thanks for your help.13:05
TheMuerog: need a kind of config.Evil(attrs) *Config, err13:06
fwereaderog, phew indeed :)13:06
rogfwereade: i'd let an extra line creep in when merging.13:06
rogfwereade: which happened to be LoggingSuite.TearDownTest13:06
niemeyerTheMue: Yo13:06
fwereaderog, ouch :)13:06
niemeyerTheMue: How's the EnvironConfig stuff?13:06
TheMueniemeyer: implemented in state and tested there, now i'm changing the dependencies13:06
TheMueniemeyer: hi btw13:07
TheMueniemeyer: and for the provider test i have to write an invalid config13:07
niemeyerTheMue: Superb13:08
TheMueniemeyer: i think i have to go directly to ZK here *sigh*13:09
Aramniemeyer: TheMue: I want to get rid completely of mgo in mstate_test. Tests should only rely on the public API and not implementation details. for test that HAVE to peek in the DB, I'd rather move them to a internal_test.go file or something.13:11
niemeyerAram: +113:11
TheMueAram: +113:11
niemeyerTheMue: FWIW, it's a good thing that you're finding it difficult to write down a broken config ;)13:12
TheMueniemeyer: yeah, indeed *lol*13:12
TheMueniemeyer: the usage of config.Config with coercing a schema makes it hard, and that's a good feature13:14
rogfwereade: could you have a glance at the uniter test changes in this CL, please, before i submit: https://codereview.appspot.com/648405113:16
fwereaderog, they look fine to me :)13:26
rogfwereade: thanks13:26
fwereaderog, I'm feeling rather uncomfortable about writing tests which check git output directly, but I can't think of another way to get, say, the status/log of a git repo13:28
fwereaderog, am I missing something obvious? :/13:28
rogfwereade: i presume that's the only way, but i'm not familiar with git13:29
rogfwereade: there may be some API13:29
fwereaderog, hmm, seems that such things exist, but they look like a hassle13:31
niemeyerAram: How's the watcher going?13:33
Aramniemeyer: working on it, have a good model drawn on paper, expecting very good results.13:33
niemeyerAram: Was there any issue with the proposal?13:34
Aramat least I haven't found any.13:35
niemeyerAram: Cool, was just wondering if the model on paper was different13:35
rogniemeyer: can you think of any reason to retain the AgentToolsWatcher?14:11
niemeyerrog: No, I think EnvironConfig watcher should cover it14:12
rogniemeyer: yeah, i think so. i was wondering if we might have reason to watch the current tools of an agent, but i don't think so. just checking.14:12
niemeyerrog: I don't think so as well.. it's mostly informational14:13
fwereade_niemeyer, I'm wiped out and need a break; if you have a moment, can I get a really casual pre-review on https://codereview.appspot.com/6488051 please? there's something not quite right about it:14:45
fwereade_niemeyer, I'm starting to think I should drop charm.Manager entirely and move its functionality into Uniter itself14:46
niemeyerfwereade_: Interesting, what's the motivation that makes you feel that way?14:46
fwereade_niemeyer, (also the tests are screwed up, but I'm hoping fresh eyes after a lie down will fix that, so you shouldn't have to worry too much there...14:46
fwereade_niemeyer, I can't quite put my finger on it... I'm sort of hoping you'll take one look and say "why are you doing X" and I'll say "er, I don't know", and we'll all live happily ever after14:48
niemeyerfwereade_: LOL14:48
fwereade_niemeyer, don't spend too much time on it, though14:49
niemeyerfwereade_: Cool, will check it out14:50
fwereade_niemeyer, cheers14:50
TheMueniemeyer: most stuff for environ config is done, but provisioner (and here especially the tests) are a problem. dave also tested invalid environments. his impl today is relative friendly and retries based on the changes delivered by the watcher. but the new watcher stops on invalid environments, because config.New() thankfully doesn't except them.15:21
niemeyerTheMue: There is an important distinction between an invalid config.Config and a config.Config considered invalid by a provider15:22
niemeyerTheMue: The logic in the provisioner, and in fact in other workers too, should still consider a broken env and retry15:23
TheMueniemeyer: config.New() checks the schema and returns an error if it is invalid15:23
niemeyerTheMue: The firewaller should do that too, for example15:23
niemeyer<niemeyer> TheMue: There is an important distinction between an invalid config.Config and a config.Config considered invalid by a provider15:23
TheMueniemeyer: what should the watcher return in this case? a struct containing the config and an error, so that the receiver can check? would be easy, but leads to more changes in other places.15:25
niemeyerTheMue: If the watcher receives an invalid *config.Config*, it can drop it, and wait for the next change15:25
niemeyerTheMue: That said, the firewaller and the provisioner must still check for a config.Config considered invalid by the provider15:26
TheMueniemeyer: ah, good idea15:26
niemeyerTheMue: Because a valid config.Config is not necessarily a valid configuration for a specific provider15:26
TheMueniemeyer: yeah, i don't change that check15:26
TheMueso, it seems there's only one failing assert left. have to check why15:36
TheMueah, landed, only one well known fail due to my locale (compared bzr message of store is wrong)15:48
niemeyerfwereade_: I think I spotted some of your dislike there15:57
niemeyerTheMue: Enjoy!15:57
fwereade_niemeyer, oh yes?16:03
niemeyerfwereade_: Yeah, I've sent a review16:05
fwereade_niemeyer, sweet, tyvm16:05
niemeyerfwereade_: np!16:06
* niemeyer steps out for lunch16:06
fwereade_niemeyer, yeah, good food for thought -- thanks :)16:07
rogniemeyer: ping17:37
rogfwereade_: ping17:37
niemeyerrog: Pongus17:50
rogniemeyer: just wanted to have a word about the strictness of provider config attributes17:50
niemeyerrog: Ok17:51
rogniemeyer: you said yesterday that they shouldn't be strict, but we had tests specifically checking for strictness. just wanted to check that it's ok to relax the checks.17:51
rogniemeyer: i've proposed the CL anyway.17:51
niemeyerrog: I'm not sure about what this is about17:51
rogniemeyer: EnvironConfig17:51
niemeyerrog: I don't recall saying anything about strictness17:51
rog[16:05:25] <niemeyer> rog: No provider should break if config.COnfig has an attribute that it doesn't know about17:52
niemeyerrog: Yeah, that's still the case17:53
rogniemeyer: but at least one provider *was* breaking when it saw an unknown attribute17:53
rogniemeyer: (and we had a test to check it)17:53
niemeyerrog: I can't imagine how that would have happened.. they don't validate the config.Config internals17:53
rogniemeyer: environs.Provider.SetConfig17:54
niemeyerrog: ?17:54
rogniemeyer: the provider sees all the attributes in the config17:54
rogniemeyer: including (now) agent-version17:54
niemeyerrog: I said something else17:55
niemeyerrog: They don't validate the config.Config internals17:55
niemeyerrog: What is breaking, more precisely/17:55
rogniemeyer: your comment didn't mention internals.17:56
niemeyerrog: config.New validates itself17:56
niemeyer<niemeyer> rog: I can't imagine how that would have happened.. they don't validate the config.Config internals17:56
rogniemeyer: they validate the attributes they get from the config17:56
rogniemeyer: SetConfig was failing17:56
niemeyerrog: What is breaking, more precisely?17:56
niemeyerrog: file and line, please17:56
rogniemeyer: well, the symptom i saw was that the provisioner failed to make a valid environment17:57
rogniemeyer: the cause was:17:57
rogniemeyer: environs/dummy/environs.go, checker.Coerce17:58
rogniemeyer: (in Validate)17:59
niemeyerrog: There are no general attributes in there17:59
niemeyerrog: The file and line where an environment validates the general config.Config attributes, please?17:59
rogniemeyer: ah, should config.Config know about agent-version?17:59
niemeyerrog: Yes17:59
rogniemeyer: ok17:59
rogniemeyer: that makes sense.18:00
niemeyerrog: Sweet18:00
rogniemeyer: ok, thanks. i'll mark that branch as WIP18:01
niemeyerrog: np18:01
rogniemeyer: gotta go now. see you tomorrow.18:02
niemeyerrog: have a good time18:02
rogniemeyer: have fun18:02
niemeyerrog: Thanks18:02

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