[06:09] fwereade: thank you for your review [06:10] davecheney, np, the tweaks should be straightforward :) [06:10] fwereade: already done [06:11] davecheney, cool [06:19] fwereade: could I press you to expand on this statement [06:19] FWIW, I maintain that user-config is not the same thing as internal-config, and [06:19] trying to pretend that they are causes us nothing but pain. Not sure that topic [06:19] is really open for discussion, though... [06:19] i too feel the sting of too many battles over configuration [06:21] davecheney, I was trying to argue for it when we were originally having the config, and lost... but I'm still not quite sure *why* I lost, and so I can't figure out whether this situation qualifies as a reason to reopen the discussion (and indeed I'm not really sure whether I even want to...) [06:21] fwereade: so this is a diffrence between the environments configuration (passwords and shit that the agents need) [06:22] vs the pure minimum of config as a connection string [06:25] davecheney, I hadn't looked at it exactly that way -- in my eyes the problem is simply that we're trying to make a single parser do different things in different contexts [06:27] fwereade: well, IMO the wrinkle is not in the parser [06:28] but the way BootstrapConfig fucks with the config, then tries to parse it again [06:31] morning [06:31] hello [06:31] davecheney, ISTM that the only reason we're messing around with the config is so that we can pretend that the two situations are the same [06:31] * davecheney nods [06:31] davecheney, and that that is the cause of the ugliness you reference [06:32] davecheney, so yeah, not exactly the parser itself, but a decision made in the parser's intellectual scope, if you like [06:32] TheMue, heyhey [06:32] fwereade: heya, just reading your review [06:32] TheMue, probably all these things were covered in a prereq, but I didn't see that I'm afraid [06:33] fwereade: no, they are not [06:34] fwereade: one simply is a typo (c'n'p error) - the "dummy" in the ec2 config. i'll change it [06:36] fwereade: the "default" is still in discussion and may be changed later. but i still prefer the word "default" so that one explicitely state in the config, that he wants the default behavior instead by leaving it empty [06:37] fwereade: could you please describe your question regarding the change of the mode more? do you mean a change at runtime? [06:37] TheMue, feels wrong to me to have 2 names for the same state, but mileages may reasonably vary I guess [06:37] TheMue, yeah [06:39] TheMue, there's handling somewhere for things like attempts to change the env type, we can probably just do something similar there [06:39] fwereade: we don't have two names, we only have, if somebody writes nothing, a default mode. but how would you otherwise explicit define that you want the default mode? [06:39] TheMue, honestly I don't think this should be user-configurable at all [06:39] TheMue, it feels like we're repeating the placement mistake [06:40] fwereade: environments like ec2 have different modes, later maybe also a third one (one group per service). how to control that? [06:41] TheMue, ask the environ what it prefers? [06:41] fwereade: the environment prefers nothing, it depends on your cloud size and services [06:43] TheMue, in that case, surely, it shouldn't have a default value: it's is an important decision that we should require an explicit value for [06:43] TheMue, but... well, look, we'll be dealing with a whole bunch of environments soon enough [06:44] TheMue, some of them have no concept of security groups in the first place, eg MAAS (as far as I know, not very up to date with progress) [06:45] TheMue, for those envs we will actually have to implement a local firewaller in every machine agent anyway, I think [06:45] fwereade: yes, that's why niemeyer wanted a default, because you today can't tell if instance and global are ok for each environment. default is start, more modes to come. [06:45] fwereade: exactly [06:46] fwereade: but more important is your thought about changing it at runtime [06:46] TheMue, ok, if we implement local per-machine firewalling with iptables or something [06:47] TheMue, why would we need "global" or "instance" modes at all? [06:47] TheMue, ok, if we implement local per-machine firewalling with iptables or something [06:47] TheMue, why would we need "global" or "instance" modes at all? [06:48] TheMue, envs like MAAS will require that we add local firewalling, I think [06:48] TheMue_ ^^ [06:48] fwereade: yes, but other envs maybe not [06:49] TheMue_, if we have local firewalling implemented for just one env we can just do that everywhere [06:49] fwereade: niemeyer wanted to keep it more abstract, so we later can support multiple envs with the same configuration machanism [06:49] TheMue_, and just have a thin layer of the global handling in the environments that need it on top of that [06:49] fwereade: you now change the whole firewalling concept [06:49] fwereade: please let's do it step by step [06:50] TheMue_, in that situation global-firewalling is just Wrong === TheMue_ is now known as TheMue [06:50] TheMue_, how so? AIUI I merely bring up what everyone has always known had to be done from the beginning [06:50] TheMue, I'm not demanding specific action :) [06:51] TheMue, but I am worried that we're adding a piece of totally transitory dev-relevant-only config [06:51] TheMue, that we will end up releasing and have to go through all the tediousness of deprecating [06:51] fwereade: what's wrong with security groups for the ec2 env? [06:53] fwereade: especially, if the user configures abstract modes like instace or global, the change from sec groups to iptables is transparent for the user [06:53] TheMue, they scale like shit, and ISTM that the global firewaller provides so flimsy a veil of kinda-security that , while it may allow us to scale, will not be used in any serious deployments [06:53] TheMue, what is the purpose of the global firewaller if not to paper over the security group issues? [06:54] fwereade: using them today lets us sart while a change to a different implementation later is transparent for the user. so we ca release early with today demands but still have a comfortable solution for the future [06:55] TheMue, today's demands are "match python"; I though python had no global firewaller [06:55] TheMue, the most transaprent thing is to do the best we can for the user given our capabilities [06:56] TheMue, if we want to target, right, now, users who want either scalability or security, but not both, then I guess we have no choice [06:56] TheMue, fwereade, davecheney: mornin'! [06:56] fwereade: afaik it's just a simple temporary solution which later is easy to change. how fast could you set up iptables inclusive testing? [06:56] Guest2629: morning === Guest2629 is now known as rogpeppe [06:57] TheMue, but if we're asking them to make a choice like *that* I think we should be upfront about it and have no default [06:57] rogpeppe, heyhey [06:58] fwereade: could you add this with your reasons to the review so that we could discuss it with niemeyer later. i'm not good as a man in the middle. [06:58] TheMue, clearly the decision to write the global firewaller has already been made, but when I review the code I lack the context behind the decisions and need to ask questions [06:59] fwereade: yes, for sure [06:59] TheMue, I will expend my review, thanks [06:59] fwereade: but i'm not very good in repeating your questions later to niemeyer. it's better when you ask him too [06:59] er extend [07:00] TheMue, absolutely, np :) [07:00] fwereade: as long as this topic is in move changes are more simple than later [07:01] davecheney: ping [07:01] rogpeppe: ack [07:02] davecheney: i see your issue with my admin-secret fix, but i think that moving the admin-secret check earlier in the Bootstrap function would fix that. [07:02] rogpeppe: fair call [07:02] what you go, ie the blowup at cloud init level [07:02] davecheney: admin-secret can't be a required field in the config, unfortunately AFAICS [07:02] was where i started with [07:02] i got most of the way with making admin-secret rquired [07:02] but BoostrapConfig screwed me [07:03] davecheney: the fundamental issue is that we don't want to push admin-secret into the state. [07:03] rogpeppe: what about just overwriting the secret with 'your secret isn't here' [07:03] config.New would be happy [07:04] and we _know_ we're never putting the real value into the state [07:04] davecheney: i don't see how that's better than just not requiring the admin secret. [07:04] rogpeppe: i would argue the opposite [07:04] but not streniously [07:06] davecheney: it's like we're saying, "yeah it's required, but only means something in some cases" [07:06] rogpeppe: can't we leave it out of config/config [07:06] and apply it at environs/ec2/config.go [07:06] it is already in there [07:06] but specfied as schema.Omit [07:06] davecheney: no [07:07] davecheney: because we need to be able to create an environment without admin-secret, i think [07:08] davecheney: or... maybe that is a possibility, let me think [07:08] rogpeppe: yeah, can't we do that just by changing environs/ec2/config.go ? [07:08] rogpeppe: it would make admin-secret an environ specific value [07:08] unknownattrs, or whatever [07:08] davecheney: no, we do need to be able to create an environment without admin-secret [07:09] rogpeppe: i don't understand that sentance [07:09] an environment == ec2 ? [07:09] davecheney: environs == environs.Environ [07:09] s/environs/environment/ [07:09] yeah, that is easy [07:10] remove the admin-secret atom from environs/config/config.go [07:11] davecheney: but can we then open an ec2 environment with that config? [07:11] rogpeppe: depends if it is a valid ec2 config [07:12] davecheney: i thought you were suggesting that the ec2 config required admin-secret [07:12] rogpeppe: indeed i am [07:12] davecheney: so when an agent opens the environment, it must make up an admin secret? [07:13] rogpeppe: NFI [07:13] i dont know how it works ont he agents [07:13] davecheney: it works ok. agents don't need admin-secret [07:14] davecheney: only the client has the admin secret [07:14] yeah, they would use environs/config/config.go [07:14] which only requires a generic config [07:14] so removing admin-secret from e/c/config.go is a valid suggestion [07:14] davecheney: they've still got to get it through environs/ec2/config.go,m no? [07:15] rogpeppe: don't thye do that by getting the providor, then the provider can validate the config ? [07:15] davecheney: i'm not sure i understand. where are you suggesting we make the check for admin-secret's presence? [07:16] environs/ec2/config.go [07:16] it is already there [07:16] but is optional atm [07:16] davecheney: so what happens when we don't have an admin secret? [07:16] not a valid config [07:17] davecheney: then how can an agent create an Environ? [07:17] well, you got me there [07:17] i guess they are screwed [07:17] and we are back to sq one [07:17] davecheney: that's the reason i decided that the best place to put the check was where admin-secret is actually needed - in Bootstrap [07:18] rogpeppe: fair enough [07:18] best of a bad situation [07:18] oh [07:18] davecheney: yeah. i'm very happy to entertain other possibilities, but i haven't seen a better one yet [07:19] what about if admin secret in ec2 worked the same way as authorised-keys in e/c/config.go ? [07:19] ie, if it were blank, they values are sourced from somewhere else [07:19] (else == whereever the agents stash their secret value) [07:19] davecheney: so you can't stash your secret in environments.yaml? [07:20] rogpeppe: i believe authorised-keys has a fallback mech [07:20] yaml first, disk second [07:20] davecheney: yeah it does. [07:20] davecheney: but... how does this help? [07:20] does it solve the problem of the e/ec2/config.go being able to require admin-secret, but allow it to come from several locations [07:20] davecheney: i think we've got a fundamental issue here - in some places in the system, we don't *have* an admin secret [07:21] agreed [07:21] davecheney: ... where it's coming from another location or not [07:21] i think this it the point fwereade says he told us so [07:21] davecheney: i agree with fwereade's point, but i don't mind this solution *too* much [07:22] rogpeppe: althoght, it does bail out after writing the data to the s3 store [07:22] which means you have to use juju destroy-environment before trying again [07:23] davecheney: yeah, that's a bug [07:23] davecheney: as it suggested earlier, the check should come further up [07:23] s/it/i [07:24] davecheney: i'll fix that [07:24] sweet [07:24] afk for a while [07:24] davecheney: afk? [07:28] rogpeppe, away from keyboard [07:28] fwereade: thanks [07:59] fwereade: thx for your notes. both sound very good to me. [07:59] TheMue, cool, cheers :) [08:32] moin. [08:33] Aram: moin moin [08:37] *: Always the same question. *sigh* When having a chain of CLs which shall bei updated to trunk, then after merge only commit and push or also lbox propose of already submitted pre-requesites? [09:13] rogpeppe, fwereade: any hint ^^? [09:13] TheMue, I generally don't repropose unless there were conflicts [09:15] fwereade: so only commit and push? i twice made it wrong in the beginning and had too much code in the reviews [09:15] TheMue, it seems to work ok for me [09:16] TheMue, I only get too much code when I forget a -req [09:16] fwereade: thx, i'll make a note to keep it in mind next time. ;) [10:10] TheMue: no need to repropose before submitting. [10:10] just make sure you merge trunk. [10:10] so you merge the prereq [12:08] Morning jujuers! [12:13] niemeyer, heyhey [12:13] fwereade: Heya [12:13] fwereade: Good weekend? [12:13] niemeyer, very nice thanks -- saw cath's cousins and their new baby :) [12:14] fwereade: Oh, nice [12:14] niemeyer, and also stacked up a few reviews which will, I hope, lead to interesting conversations if not necessarily immediate progress [12:14] fwereade: /me is all sensitive about "new babies" :-) [12:15] niemeyer, he's about 4 months old and smiley and dribbly and cute [12:15] fwereade: LOL [12:22] niemeyer: morning and thx for your comment [12:23] TheMue: Heya, np [12:23] * TheMue 's small baby - she is now 16 yrs old ;) - has her first archery tournament this afternoon. we'll all watch her. [12:27] TheMue: Wow, very nice. Good luck there! [12:28] niemeyer: morning :-) [12:35] fss: Heya [12:36] niemeyer: thank you, I will tell her when she's back from school. just had to watch our older one leaving home with her driving teacher. one of the last lessons before driving test. [12:48] late lunch, bbiab [12:49] fwereade: Enjoy [13:15] fwereade: https://codereview.appspot.com/6651060/ is waiting for you [13:38] niemeyer, cheers [14:11] Aram: ping [14:22] niemeyer: ping [14:22] fss: hi [14:23] did you see that iamtest cl? [14:24] fss: Still churning CLs pushed over the holiday and/or weekend [14:27] niemeyer: oh, right :-) please let me know if I can help with anything [14:31] fss: Cheers! [14:33] fwereade_: ping [14:34] niemeyer, pong [14:34] fwereade_: Yo [14:34] fwereade_: Just started looking at relation-lifecycles [14:35] niemeyer, oh yes [14:35] fwereade_: The N=100k indeed feels troublesome there [14:36] niemeyer, does my wolly justification for an end-run around txn hold any water? [14:36] s/wolly/wooly/ [14:36] fwereade_: I'm thinking too.. in principle it sounds doable [14:37] fwereade_: But even better would be to have these settings collected in a more distributed way [14:37] fwereade_: Even if we do RemoveAll, 100k is still a lot [14:38] niemeyer, not really sure where it makes sense to distribute it to -- a CA that just does a bit of work every few seconds might do the trick I guess :) [14:39] fwereade_: Yeah, but that sounds worryingly hand-wavy [14:40] fwereade_: We've decided to leave settings around, and that seems to work well, but is there any real point where we can say settings for a unit leaving aren't relevant anymore? [14:41] niemeyer, I don't really think we can at the unit level, can we? We could make subordinate relations be cleaned up incrementally by their own units [14:41] niemeyer, but that doesn't help with the big ugly case [14:42] fwereade_: Right [14:45] niemeyer, the CA case doesn't seem too bad to me though -- it's just one extra op to create a document with the relation's ID (or I guess just directly its scope key prefix) [14:45] niemeyer, the CA then just removes a few matching docs at a time until it runs out, and then deletes the "droppings" doc [14:46] fwereade_: The extra doc for? [14:46] niemeyer, or adds it into an existing doc, sorry, haven't considered tradeoffs here [14:47] fwereade_: Ah, I see [14:47] niemeyer, brb phone [14:47] fwereade_: Cool, thinking meanwhile [14:54] fwereade_: Yeah, you're right.. the CA may be a good idea after all [14:55] niemeyer, however I'd prefer not to switch focus to that just now [14:56] niemeyer, do you think that an interim solution of either monster-transaction, or remove-all-settings-before-transaction, would be plausible? [15:01] fwereade_: Not greatly comfortably with either [15:02] * fwereade_ is crestfallen [15:02] * niemeyer looks in the dictionary to see what fwereade_ is [15:03] niemeyer, how about deleting no settings, but creating a droppings doc for future use? [15:03] * niemeyer laughs at the "-- said of a horse." part of the explanation [15:04] * fwereade_ doesn't know which dictionary niemeyer has [15:09] fwereade_: That was from gcide [15:10] niemeyer, ah nice :) [15:10] fwereade_: Sounds good regarding injecting doc into cleanups collection [15:11] niemeyer, great [15:11] niemeyer, and that's a nicer name than droppings [15:12] fwereade_: {Id: bson.ObjectId, Type: "settings", Prefix: "r#..."} [15:12] niemeyer, LGTM [15:12] fwereade_: /Type/Kind/ perhaps [15:12] niemeyer, +1 [15:12] fwereade_: We'll need the CA soon enough anyway [15:12] fwereade_: To recover lost transactions [15:13] niemeyer, indeed [15:14] I'll step out for lunch.. back in a bit to continue on reviews [16:24] wrtp, Aram, niemeyer: is http://paste.ubuntu.com/1281389/ expected/familiar? just merged... [16:25] fwereade_: i haven't seen that before [16:25] fwereade_: ISTM that's probably a consequence of recent fw changes, renaming "default" to "" [16:27] wrtp, yeah, I guess we will see a fix tomorrow [16:31] I guess no cloud consistency call today.. [16:31] fwereade_: Can you paste the link again please? [16:31] niemeyer, http://paste.ubuntu.com/1281389/ [16:31] fwereade_: Cheers [16:32] fwereade_: I have a try at fixing it quickly [16:32] So we don't stick with a broken trunk [16:35] robbiew: Cloud consistency call today? [16:35] niemeyer: no idea...I know I won't be attending (at ODS) [16:36] niemeyer: I would say "no" [16:36] ...as I don't know who will be around to attend [16:36] robbiew: Cheers [16:36] ;) [17:21] i'm off for the day. see y'all tomorrow. [17:28] wrtp: Enjoy the evening [17:28] fwereade: https://codereview.appspot.com/6699044 .. nothing exciting, luckily === andrewsmedina is now known as almanegra [20:15] niemeyer, ping [20:17] niemeyer, re service-lifecycles EnterScope txns -- I suspect the logic *is* faulty, but the mistake is not, I think, in the inability of a unit to re-enter a scope -- I think that is a feature [20:17] niemeyer, thoughts? [20:21] fwereade_: Yo [20:21] niemeyer, heyhey [20:22] fwereade_: In principle I don't see a great reason to allow it, but at the same time it feels bad to have faulty logic that doesn't really bring any benefit and misbehaves in such cases [20:22] niemeyer, to expand: when a unit leaves the scope, it has run the relation-broken hook and represented to the charm that its participation in the relation is entirely done [20:23] fwereade_: Sure, but in theory it could happily re-enter the scope by reporting to the charm that it's entering again [20:24] niemeyer, I am 100% with you on the faulty logic, I will rework and explain nicely, unless you feel there is something deeply wrong with the approach -- I think that it is possible to determine the various situations cleanly [20:24] niemeyer, which feeds into ErrScopeDying [20:25] niemeyer, which should maybe be ErrScopeClosed -- indicating that either the unit has already left and can't come back, or just that the scope is not open to new members... [20:26] niemeyer, ...but in either case, the correct response for the uniter is to ignore the error and all further references to that relation [20:26] fwereade: Is there anything that misbehaves today if we enter the scope again, as far as the the state/watchers/etc are concerned? [20:27] niemeyer, I don't think so, no [20:28] fwereade: So I don't see yet the motivation to be actively preventing the unit from entering the scope. It feels like a property we can easily explore for good reasons in the future, and don't have to think much right now [20:29] niemeyer, the intent is that a unit "re-entering" a scope should basically do nothing except possibly update the settings if the private address has changed [20:30] niemeyer, that's from the API perspective [20:30] fwereade: Yep, sounds fine [20:30] fwereade: I'm talking about leaving and entering, though [20:31] fwereade: I don't see a reason to actively disallow it [20:31] niemeyer, gaah, sorry [20:31] niemeyer, I saw/said: [20:31] fwereade: Yep, sounds fine [20:31] niemeyer, from the model perspective I think it is sensible to make this part of the model agree with the part which thinks that relation-broken is the dividing line between "in the relation" and "left, never ever coming back" [20:33] niemeyer, I would favour consistency for the time being and consider experimentation later [20:33] fwereade> niemeyer, that's from the API perspective [20:33] fwereade: Yep, sounds fine [20:33] fwereade: I'm talking about leaving and entering, though [20:33] fwereade: I don't see a reason to actively disallow it [20:33] fwereade_: I don't see the reason to building onto the model that "left, never ever coming back" [20:34] niemeyer, I understand that to be the meaning of the relation-broken hook [20:34] niemeyer, I had a feeling we did state somewhere that it would be called once per relation and it would be the last thing called [20:35] niemeyer, if that is the case (er, I'll see if I can find anywhere...) then I think it's best to be consistent [20:36] fwereade_: We're getting into shady areas [20:36] fwereade_: We never discussed what happens when you start a unit in a different machine, for example [20:36] fwereade_: Do we Leave and then Join from the other machine? Etc [20:36] fwereade_: I'm not suggesting we should specify any of that right now, though [20:37] fwereade_: I'm just suggesting we don't start hardcoding unnecessary behavior that closes doors we haven't talked about yet [20:38] niemeyer, I shall meditate upon this :) [20:38] fwereade_: Hehe :) [20:39] niemeyer, hm, actually, on https://juju.ubuntu.com/docs/charm.html it says "Runs when a relation which had at least one other relation hook run for it (successfully or not) is now unavailable" [20:39] niemeyer, that is not really a good description IMO but it does if anythig carry connotations of potentially being rerunnable [20:39] fwereade_: Overall, the lifecycle branches look like going in a pretty good direction.. the leave scope one has a few things we'll have to discuss too to make sure we understand what's up, but it's all getting together nicely [20:40] niemeyer, yeah, I thought there'd probably be a bit of discussion for each of them [20:40] niemeyer, the one that sits on top of them all does make me happy, though [20:40] fwereade_: It looks lke surprisingly small amount of logic, thankfully [20:41] niemeyer, yeah, it's not too bad [20:42] niemeyer, there may be a simplification waiting to get out but I haven't got round to implementing it against that tree yet so it could yet surprise me [20:43] fwereade_: Cool [20:44] fwereade_: I'll step out to do some end-of-afternoon errands.. back later