| thumper | axw: hey | 02:21 |
|---|---|---|
| thumper | axw: fwereade was considering staying up to chat to you but when he realized what time you started he decided not to :) | 02:22 |
| axw | thumper: yo | 02:22 |
| axw | hehe | 02:22 |
| thumper | axw: probably about the precheck branch | 02:22 |
| thumper | I've not read is response other than to realize that he made one | 02:22 |
| axw | thumper: I think about the null provider actually | 02:22 |
| * thumper is busy with kvm | 02:22 | |
| thumper | ah | 02:22 |
| axw | but yes, precheck needs work too | 02:22 |
| thumper | expect a ping from him when he wakes | 02:22 |
| axw | yup, will do | 02:23 |
| axw | thanks | 02:23 |
| thumper | np | 02:23 |
| thumper | axw: when a manually provided machine is added, the agents still run as root, right? | 02:26 |
| axw | thumper: yes | 02:26 |
| thumper | cool | 02:26 |
| thumper | just checking | 02:26 |
| fwereade | axw, heyhey | 05:53 |
| axw | fwereade: heya | 05:53 |
| fwereade | axw, so, sorry I got confused about the time -- I hope I didn't leave you hanging too badly | 05:54 |
| axw | fwereade: nope, not at all. thumper let me know you realised later about the time :) | 05:54 |
| axw | I would feel quite guilty if you got up in the middle of the night to talk to me ;) | 05:55 |
| thumper | axw: you get over that :) | 05:55 |
| axw | heh | 05:55 |
| fwereade | axw, haha | 05:55 |
| axw | I guess it's inevitable | 05:55 |
| fwereade | axw, ok, just catching up on your responses | 05:56 |
| fwereade | axw, I *think* that series is maybe still a useful thing to have | 05:56 |
| fwereade | axw, because it's the environ that's more or less in charge of making tools, images, etc available to juju | 05:57 |
| thumper | also... for future container checks | 05:57 |
| thumper | we probably want the instance id | 05:57 |
| thumper | so we can get the machine | 05:57 |
| thumper | and see if it said kvm was ok | 05:58 |
| thumper | fyi, on ec2, no kvm | 05:58 |
| fwereade | axw, and at least a check for "do we have an image available" depends on seriess + type | 05:58 |
| axw | thumper: ah yes, I forgot you mentioned kvm-ok | 05:58 |
| axw | fwereade: fair enough | 05:58 |
| thumper | I plan to have the machine agent run it on start up | 05:58 |
| fwereade | thumper, AFAIK Prechecker will be somehow used inside state | 05:58 |
| fwereade | thumper, so we'll already know the machine | 05:58 |
| thumper | and if kvm-ok fails, then we should record in state that the host machine can't do kvm | 05:58 |
| fwereade | thumper, definitely | 05:59 |
| thumper | fwereade: but is it passed through? | 05:59 |
| thumper | we probably should | 05:59 |
| fwereade | thumper, not yet | 05:59 |
| axw | thumper: passing through is in a followup | 05:59 |
| fwereade | thumper, we need tofigure out how to get this code where we need it without making baby jesus cry | 05:59 |
| thumper | that's fine | 05:59 |
| axw | hehe | 05:59 |
| thumper | fwereade: I'm struggling getting the damn kvm code to actually run | 06:00 |
| thumper | constantly fails for me | 06:00 |
| fwereade | :( | 06:00 |
| thumper | I'm writing the interfaces and mocks | 06:00 |
| thumper | and broker | 06:00 |
| thumper | and stuff | 06:00 |
| thumper | hoping that we can plug working bits in later | 06:00 |
| fwereade | thumper, cool, but if the stuff you'remocking seem flaky... | 06:00 |
| fwereade | thumper, yeah | 06:00 |
| thumper | it is what it is | 06:00 |
| thumper | we can work with it for now | 06:01 |
| fwereade | quite so | 06:01 |
| thumper | we know basicly what it will take | 06:01 |
| thumper | so can mock out that | 06:01 |
| thumper | it won't be perfect | 06:01 |
| thumper | until the utility code actually works | 06:01 |
| thumper | so I'm mocking out what I think we'll need | 06:01 |
| thumper | as far as params go | 06:01 |
| thumper | the function calls are known though | 06:01 |
| thumper | start/stop/list | 06:01 |
| thumper | and that's about it | 06:01 |
| thumper | except for the initialize | 06:01 |
| thumper | go get me an image type code | 06:02 |
| fwereade | thumper, ok, great | 06:02 |
| axw | fwereade, thumper: so I'll put instance.Id in since it'll be used for kvm-ok, and I'll put series in for checking image availability | 06:02 |
| axw | (into PrecheckContainer params) | 06:02 |
| fwereade | axw, ah, ok, clearly I'm missing something -- why's the instance id needed there? | 06:03 |
| fwereade | axw, thumper: surely what will happen is the machine agent will run and check, and record in state whether kvm is a viable option | 06:04 |
| fwereade | axw, eliminating the need to ask the env? | 06:04 |
| fwereade | axw, or am I ignorant about something? | 06:04 |
| axw | ah yes of course | 06:04 |
| fwereade | (I am surely ignorant about many things ofc) | 06:04 |
| axw | sorry, I'm still getting my head around what's known in state etc. | 06:05 |
| axw | that makes sense | 06:05 |
| fwereade | axw, no worries | 06:05 |
| axw | nope, I'm ignorant :) | 06:05 |
| axw | ok. just series then | 06:05 |
| fwereade | axw, that's not known yet anyway | 06:05 |
| fwereade | axw, cool, thanks | 06:05 |
| axw | fwereade: what's not known? | 06:05 |
| fwereade | axw, sorry -- the can-we-run-kvm code doe not *yet* write anything into state to record the results | 06:06 |
| axw | right | 06:06 |
| fwereade | axw, but it should/will, so we're good with series + contianer kind | 06:06 |
| axw | fwereade: thanks, I'll update it after proposing my httpstorage auth branch | 06:07 |
| fwereade | axw, brilliant, thanks | 06:07 |
| fwereade | axw, bah, it looks like I may be off shortly... the power company has scheduled maintenance, apparently | 06:07 |
| fwereade | axw, so it I disappear that's where I'll be | 06:08 |
| axw | fwereade: no worries, thanks for coming on early to chat. | 06:08 |
| fwereade | axw, and thanks for addressing all those things, those CLs look solid, I'll give them a closer look in a bit | 06:10 |
| axw | cool | 06:10 |
| * fwereade is always a bit worried about searching for his power company | 06:11 | |
| axw | bad reviews? :) | 06:11 |
| * fwereade fears the consequence of a fat-fingered search for, yes, "enemalta" | 06:11 | |
| axw | haha | 06:11 |
| fwereade | it's particularly alarming seeing branded fuel tankers driving around the airport | 06:12 |
| fwereade | aaanyway | 06:12 |
| axw | speaking of alarming branding, I saw this in a parenting magazine yesterday: http://www.juju.com.au/ | 06:13 |
| fwereade | haha | 06:16 |
| davecheney | oh dear | 06:26 |
| rogpeppe | mornin' all | 07:04 |
| rogpeppe | fwereade: hiya | 07:06 |
| fwereade | rogpeppe, heyhey | 07:06 |
| rogpeppe | fwereade: martin reviewed this, but suggested another look might be good, so i wondered if you could take a glance before i approve it. https://codereview.appspot.com/13778046/ | 07:06 |
| fwereade | rogpeppe, ah, sure | 07:06 |
| fwereade | rogpeppe, btw I will disappear for an unknown period at some point today, power company doing some maintenance | 07:07 |
| rogpeppe | fwereade: ok, thanks for the heads up | 07:07 |
| TheMue | morning | 07:35 |
| rogpeppe | TheMue: yo! | 08:13 |
| TheMue | rogpeppe: btw, the chrome plugin you twittered is nice - but i dropped chrome | 08:14 |
| rogpeppe | TheMue: what do you use now? | 08:15 |
| TheMue | rogpeppe: to much memory and cpu consumption | 08:15 |
| TheMue | rogpeppe: now back to the good old ff | 08:15 |
| dimitern | TheMue, I have exactly the opposite experience with ff and chrome - the latter works faster and its more lightweight on my machine :) | 08:16 |
| TheMue | rogpeppe: imho chrome becomes more and more an own os, only missing a go and a dart ide integrated ;) | 08:16 |
| TheMue | rogpeppe: funny | 08:16 |
| TheMue | rogpeppe: some of my friends switched too and feel better now | 08:16 |
| TheMue | rogpeppe: may depend on the exact plugins one is using | 08:17 |
| === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: TheMue | Bugs: 12 Critical, 135 High - https://bugs.launchpad.net/juju-core/ | ||
| TheMue | hmm, the number of bugs is only increasing. we need a feature freeze to simply handle those bugs | 08:18 |
| jam | wallyworld_: I'm looking at the fetch code right now, and I see it failing to find sjson in 3 places before it even tries to look for .json: http://paste.ubuntu.com/6149206/ | 08:40 |
| jam | that looks like private-bucket, then ? then tools-url | 08:40 |
| jam | wallyworld_: I thought the goal was to search for sjson then json in private, then fallback to site-tools, then user config? | 08:40 |
| jam | wallyworld_: the issue is that the signed/unsigned flag is being done at a higher level | 08:42 |
| jam | with "requireSigned" | 08:42 |
| jam | environs/tools/simplestreams.go always calls "GetMaybeSignedMetadata(...,requireSigned=true)"before calling it with something else if it hasn't found anything | 08:43 |
| jam | doesn't that mean when we have juju.canonical.com or even just signed mirror data, it *won't* ever use user-specified unsigned metadata ? | 08:43 |
| raywang | jamespage, ping | 08:56 |
| jam | wallyworld_: I'm also surprised you decided to take our "implicit" URLs before user-explicit URLs (you check the custom urls before "tools-url") | 08:56 |
| jam | how would a user override an explicit setting in the env? Only by using their private bucket? | 08:56 |
| wallyworld_ | jam: i had no choice for the 1.15 release, because for in HP Cloud (for example), the tools url was set automatically and this hid the uploaded tools when using a dev release | 09:12 |
| jam | wallyworld_: how is tools-url set automatically ? | 09:13 |
| wallyworld_ | jam: see certifiedclouds.go | 09:14 |
| wallyworld_ | it sets up the tools url in config validation | 09:14 |
| wallyworld_ | we won't need this once the repository comes online | 09:15 |
| jam | wallyworld_: so... that indicates to me that our layering is probably wrong. Can't we put it into the CustomSources instead of overriding user-config ? | 09:15 |
| jam | wallyworld_: we could call GetCertified... in openstack/provider.go GetToolsSources, couldn't we ? | 09:16 |
| wallyworld_ | maybe. it was done later friday night because i was smoke testing for the release and found the issue. but we didn't release 1.15 so it was for nought | 09:16 |
| wallyworld_ | i was just trying to get something done so the release would be ok | 09:17 |
| wallyworld_ | it's on my list to revisit | 09:17 |
| jam | sure, did you see my point about always searching for DefaultBaseURL+ .sjson *before* searching the private-bucket for .json ? | 09:17 |
| jam | that will also cause problems when we have "official" locations online | 09:17 |
| wallyworld_ | i'm reading it now | 09:18 |
| wallyworld_ | the thinking way back when was that signed metadata should take precedence but this could cause issues as you say | 09:19 |
| wallyworld_ | jam: the hp cloud url was done in config because i needed to convert the deprecated public-bucket-url into a tools url and that deprecation code lives in config, but i think it can be split out into custom sources | 09:22 |
| wallyworld_ | i was initially looking to keep the code dealing with deprecated config values together | 09:23 |
| jam | wallyworld_: sure. It just feels like it should be "user-config" then "private" then "default tools-for-cloud" then "juju.canonical.com" | 09:23 |
| jam | *maybe* private then user-config | 09:24 |
| jam | but right now we have "default-tools-for-cloud" before user-config | 09:24 |
| wallyworld_ | sure. but a few short hours before when i thought the release was being done i needed a quick fix | 09:24 |
| jam | np | 09:24 |
| wallyworld_ | tomorrow i should get to do mirrors and hence was making tomorrow my "deal with simplestreams" stuff day | 09:25 |
| wallyworld_ | stuff = where to find things | 09:25 |
| jam | np | 09:25 |
| jam | wallyworld_: I only noticed because I had to touch those bits, and it caused conflicts when merging turnk | 09:26 |
| jam | trunk | 09:26 |
| wallyworld_ | np. i hadn't appreciated/property though about the sjon vs json thing so i'm glad you asked | 09:26 |
| rogpeppe | jam, mgz, dimitern, TheMue, axw, natefinch: a small cleanup to the environs.Environ interface: https://codereview.appspot.com/13587046 | 09:48 |
| TheMue | rogpeppe: looking | 09:49 |
| rogpeppe | TheMue: thanks | 09:49 |
| TheMue | rogpeppe: reviewed | 09:55 |
| rogpeppe | TheMue: ta! | 09:55 |
| dimitern | rogpeppe, hey he provisioner is ready for review :) https://codereview.appspot.com/13720051/ | 10:17 |
| rogpeppe | dimitern: cool. i'll take a look soon. | 10:19 |
| jamespage | raywang, hello | 10:45 |
| raywang | Hi jamespage, just want to check with you that after deploy ceph-radosgw, anything extra works need to be done for testing s3 or swift compatibility? | 10:46 |
| === teknico1 is now known as teknico | ||
| jam | standup time | 10:47 |
| jam | mgz: dimitern https://plus.google.com/hangouts/_/239d0ac12a07a73dd5a83cc2b9d8bb4047ce20b4 | 10:47 |
| jamespage | raywang, #juju | 10:50 |
| raywang | jamespage, ok | 10:50 |
| TheMue | here's my CL for review: https://codereview.appspot.com/13430044/ | 11:08 |
| * TheMue => lunch | 11:18 | |
| natefinch | jam - one thing I forgot to mention is that once the maas tags bugs are fixed, I'll need another project to work on, if you have ideas. | 11:22 |
| jam | natefinch: I would look closely at supporting the work from the weekly agenda https://docs.google.com/a/canonical.com/document/d/1eeHzbtyt_4dlKQMof-vRfplMWMrClBx32k6BFI-77MI/edit# | 11:22 |
| jam | whether that is doing code reviews | 11:22 |
| jam | or working with Martin on Amazon + VPC stuff | 11:23 |
| jam | natefinch: also, if you have MaaS up and running | 11:23 |
| jam | there is something I'm concerned about with our container strategy | 11:23 |
| jam | in that we fire up a container and it grabs an address from the DHCP server. | 11:23 |
| jam | I'm concerned that the MaaS infrastructure itself thinks that these are new machines that it is going to want to PXE boot. | 11:24 |
| natefinch | ahh yeah, I remember you mentioned that | 11:24 |
| natefinch | jam I can do some testing on that | 11:24 |
| jam | natefinch: essentially I think "juju deploy --constraints container=lxc" and then see what changes on the MaaS master node | 11:25 |
| natefinch | jam: I can do that. So that'll deploy a service to a container on one of the nodes, and we just want to make sure maas doesn't think you've added a new node for it to manage, right? | 11:26 |
| jam | natefinch: right. | 11:26 |
| jam | or at least document what happens so we can open bugs about how we want to fix it | 11:26 |
| natefinch | jam: cool. I'll test that out once I get my maas in working order again. | 11:28 |
| natefinch | smoser: you around? | 11:29 |
| rogpeppe | wallyworld_: i've got a few remarks on https://codereview.appspot.com/13842044 if you can hold off approval for a few minutes | 11:34 |
| wallyworld_ | ok | 11:35 |
| === Guest47103 is now known as ehw | ||
| rogpeppe | wallyworld_: you have a review | 11:48 |
| wallyworld_ | thanks, looking | 11:48 |
| mgz | sorry for missing standup, had to step out | 11:51 |
| wallyworld_ | rogpeppe: thanks for the Go tips, will implement those tomorrow before landing | 11:52 |
| dimitern | fwereade, hey | 11:54 |
| fwereade | dimitern, hey dude | 11:54 |
| dimitern | fwereade, provisioner review? https://codereview.appspot.com/13720051/ | 11:54 |
| fwereade | dimitern, that took longer than expected | 11:54 |
| fwereade | dimitern, twould be a pleasure | 11:54 |
| dimitern | fwereade, I was wondering where are you | 11:54 |
| fwereade | dimitern, power cut | 11:55 |
| fwereade | dimitern, was apparently scheduled, I found out just today, wasn't expecting it to be like 4 hours though | 11:55 |
| dimitern | fwereade, aw.. well at least it's just 4 hours :) | 11:56 |
| jam | fwereade: welcome back | 11:57 |
| rogpeppe | dimitern: quick question: why do non-global provisioners need the environ config at all? | 12:05 |
| dimitern | rogpeppe, afaik they need to know the environ config is saved to state already | 12:12 |
| rogpeppe | dimitern: why so? just asking, because in future we won't want them to be able to get access to the env config, so it'll save us work if we can avoid them accessing it now. | 12:12 |
| jam | rogpeppe: so today the code is structured just that we wait for environ config changes and instantiate an "Environ" object | 12:13 |
| jam | which needs an EnvironConfig | 12:14 |
| jam | they then later on do stuff with that Environ | 12:14 |
| jam | but the two parts of the code are pretty far apart | 12:14 |
| rogpeppe | jam: what does the local provisioner do with an Environ? | 12:14 |
| jam | rogpeppe: I'm not 100% sure how it all hangs together.From what I can tell it uses "getBroker" which returns either the Environ itself, or an LXCBrokecr | 12:15 |
| jam | Broker | 12:15 |
| jam | rogpeppe: and the Config stuff ends up getting handed off to a lot of places | 12:16 |
| dimitern | rogpeppe, so in short, it seems the lxc one does not need it at all, but that's how the code is written | 12:16 |
| rogpeppe | dimitern: I'd much prefer it if the local provisioner never called WatchEnvironConfig | 12:17 |
| jam | so what comes to mind is having an API that returns a minimialistic EnvironConfig, which we later ask another API for the credentials needed for the environment broker | 12:17 |
| rogpeppe | dimitern: (i mean WatchForEnvironConfigChanges of course) | 12:18 |
| rogpeppe | dimitern: couldn't you only set p.environ if p.pt == ENVIRON ? | 12:18 |
| jam | looking at the code, it does feel like we have a bunch of switch statements that might look better overall if we just had 2 types | 12:18 |
| jam | an LXCProvisioner | 12:18 |
| jam | vs an EnvironProvisioner | 12:18 |
| fwereade | dimitern, ha, I was just coming to start the above conversation | 12:20 |
| fwereade | dimitern, rogpeppe: so, yeah, the environ stuff is tied in too tightly there | 12:21 |
| rogpeppe | ah, i see why | 12:21 |
| jam | fwereade: so I think landing what we have to guarantee API-based provisioner in 13.10 and *then* trying to split things up would be our best way forward | 12:21 |
| rogpeppe | i don't think the change would be difficult at all, at first glance anyway | 12:21 |
| fwereade | dimitern, rogpeppe: in particular, it look like an lxc provisioner is waiting for a valid environ config | 12:21 |
| fwereade | dimitern, rogpeppe: else how can it set p.environ | 12:21 |
| fwereade | dimitern, rogpeppe: and that mean we're pooing secrets onto every machine again | 12:22 |
| fwereade | dimitern, rogpeppe, jam: in the name of expediency I could put up with a hack-for-landing that had a p.environ field that was always nil for non-environ provisioners, for example | 12:23 |
| fwereade | dimitern, rogpeppe, jam: but it'd really have to be addressed very soon afterwards | 12:23 |
| rogpeppe | the problem is that ContainerManager.StartContainer wants a config.Config | 12:24 |
| rogpeppe | ahhh | 12:25 |
| rogpeppe | so we actually do want a significant portion of the environment config there | 12:25 |
| rogpeppe | just not the secret bits | 12:25 |
| fwereade | rogpeppe, ha, yes, anotherslice through environ config with its own special idiosyncracies | 12:25 |
| rogpeppe | because we need to call FinishMachineConfig, which needs things like authorized keys, | 12:25 |
| fwereade | rogpeppe, what does FinishMachineConfig use for non-managers aside from the auth-keys? | 12:26 |
| fwereade | rogpeppe, AIUI it is in fact *only* the authkeys we need | 12:26 |
| rogpeppe | fwereade: provider type | 12:27 |
| fwereade | rogpeppe, aw, ffs | 12:28 |
| rogpeppe | fwereade: and it uses cfg.AdminSecret, which seems wrong | 12:28 |
| rogpeppe | fwereade: and StatePort and APIPort | 12:28 |
| fwereade | rogpeppe, nah, that's all after "if !mcfg.StateServer{ return nil}" | 12:29 |
| rogpeppe | fwereade: ah! missed that, thanks. | 12:29 |
| fwereade | rogpeppe, I guess provider type i not so bad | 12:30 |
| jam | fwereade: we *are* putting environ creds onto every machine, but we know that, and while it lets you start and stop instances, it isn't the same as giving you root on all the machines. | 12:30 |
| rogpeppe | fwereade: for lxc, i presume it's always lxc | 12:30 |
| jam | For example, you can't actually add ssh keys to machines just with provider creds (only startup ssh keys, etc) | 12:30 |
| fwereade | rogpeppe, but I think there's a pretty good case for a narrower FinishBasicConfig which is called by FinishBootstrapConfig | 12:31 |
| rogpeppe | yeah, after reflection, i think i'm with jam that we should land this now and work towards eliminating environ config later | 12:31 |
| fwereade | jam, it lets an attacker spend a user's money | 12:31 |
| fwereade | jam, keeping those particular credential secret i one of the major points of this | 12:31 |
| jam | fwereade: I agree 100% that we want to get away from this. But I think getting to the point where "root on machine-a doesn't give root on all other machines" is a very useful step forward. | 12:32 |
| rogpeppe | jam: +1 | 12:32 |
| rogpeppe | fwereade: i think that this CL is progress | 12:32 |
| rogpeppe | fwereade: and it's nice to see it when not too much logic has changed | 12:32 |
| jam | fwereade: the main problem is that we have a "config.Config" and we honestly don't know without a lot of deep inspection what we need out of that. | 12:33 |
| fwereade | rogpeppe, jam, dimitern: I don't care if the provisioner API does something ludicrously dirty like look up SecretAttrs and crap in NO-NOT-YOURS values over what they return | 12:33 |
| jam | so I think incremental steps. But having an EnvironProvider and an LXCProvider will actually clean up the code a bit as well as make it clearer what bits we actually need. | 12:33 |
| fwereade | rogpeppe, jam, dimitern: but we really must not put provider cred on every machine | 12:33 |
| rogpeppe | fwereade: agreed | 12:33 |
| rogpeppe | fwereade: but that doesn't have to happen in this CL | 12:33 |
| dimitern | exactly | 12:34 |
| rogpeppe | fwereade: we don't have to do it all at once | 12:34 |
| rogpeppe | fwereade: "progress not perfection" | 12:34 |
| fwereade | rogpeppe: sorry, but this is one of the main point of this work | 12:34 |
| rogpeppe | fwereade: sure | 12:34 |
| rogpeppe | fwereade: but it doesn't have to happen in *this* CL | 12:34 |
| rogpeppe | fwereade: which is already big enough | 12:34 |
| rogpeppe | fwereade: and gives us behaviour that's better than we had before | 12:35 |
| jam | fwereade, dimitern: so something like line 158 of state/apiserver/provisioner/provisioner.go after we grab Config.AllAttrs() we could do a "JobHostsStateServer" check and if that isn't part of the unit making the request, we nuke whatever secrets we can ? | 12:35 |
| jam | fwereade: aren't those specific to the provider itself (openstack has different secrets than ec2 than azure) | 12:36 |
| jam | or can we just nuke all secret attrs ? | 12:36 |
| fwereade | jam, yes they are, but we don't need to construct an actual environ to get secrets, that' on EnvironProvider | 12:36 |
| rogpeppe | jam: yeah, we can do that (only if the provisioner isn't the global one though, of course) | 12:37 |
| fwereade | jam, but we will probably need to dick around and figure out the types of the secret attrs so we can overwrite them usefully,and in the general case overwriting them usefully may not even be possible | 12:37 |
| dimitern | so do we need an Environ (and its config) in a lxc provisioner at all? | 12:37 |
| fwereade | dimitern, we *need* two values out of the config | 12:37 |
| dimitern | fwereade, which ones? | 12:38 |
| === natefinch is now known as natefinch-afk | ||
| fwereade | dimitern, provider type and authorized keys | 12:38 |
| dimitern | fwereade, why? | 12:38 |
| fwereade | dimitern, they're the ones actually used by FinishMachineConfig for a non-state-server | 12:38 |
| jam | fwereade: so the Environ.SecretAttrs seems to be the only place that knows what the private attributes are | 12:38 |
| fwereade | dimitern, but IIRC we agreed that authorized-keys should come from the machine anyway? | 12:38 |
| jam | state.EnvironConfig doesn't combine sections or anything | 12:39 |
| fwereade | jam, yeah | 12:39 |
| fwereade | jam, afraid not | 12:39 |
| dimitern | fwereade, how about provider type? | 12:39 |
| jam | fwereade: so I would be fine with a quick hack, but I think if we can have LXCProvisioner than it doesn't need anything like EnvironConfig, it can just do "GetMyProvisionerConfig" sort of call. | 12:39 |
| jam | dimitern: I would guess that we don't actually need environ for LXC provisioner | 12:40 |
| jam | dimitern: but the current code layering | 12:40 |
| dimitern | jam, yeah | 12:40 |
| jam | means a lot of line-by-line auditing of "can I get here in an LXC provisioner" | 12:40 |
| rogpeppe | personally I think we should move towards having known attributes in the state in a well known form separate from environ config. | 12:41 |
| fwereade | rogpeppe, yeah, I'm definitely keen on migrating stuff into sensible buckets | 12:41 |
| fwereade | dimitern, I'm trying to figure out why we need provider type | 12:41 |
| dimitern | fwereade, we can do ProviderType() (string, error) api call, like we did with the uniter | 12:41 |
| rogpeppe | fwereade: we'd have to be careful around juju set-environment | 12:41 |
| fwereade | dimitern, yeah, indeed | 12:42 |
| jam | dimitern: moving towards that also gets us away from "NewSimpleAuthenticator" running on the LXCProvisioner, which makes it easier to say "this does not actually directly touch Mongo" | 12:42 |
| fwereade | rogpeppe, yeah, definitely | 12:42 |
| dimitern | jam, we don't have to change that - we can change the api not to set mongo passwords | 12:42 |
| fwereade | rogpeppe, but, eh, that stuff's all fatally broken anyway,it only works by luck | 12:42 |
| jam | dimitern: this is more about auditability | 12:43 |
| jam | by sharing the code | 12:43 |
| jam | you have to look at switches, etc | 12:43 |
| jam | oh, this isn't enabled in this configuration | 12:44 |
| jam | if we split them apart | 12:44 |
| jam | then there just isn't a state connection object anywhere in the LXCProvisioner | 12:44 |
| dimitern | fwereade, so how about a follow-up that introduces ProviderType() and apiprovisioner.Machine.GetAuthorizedKeys() API calls, and uses that in the lxc provisioner? | 12:44 |
| jam | dimitern: I don't tihnk we need the api to get the ProvisionerType, as cmd/jujud/machine.go is the only place where we ever call NewProvisioner | 12:44 |
| jam | and if we *want*, we can make Provisioner just an interface{} | 12:45 |
| jam | and then implement 2 of them | 12:45 |
| dimitern | fwereade, GetAuthorizedKeys() needs to read the env config internally and return a slice of what? | 12:45 |
| jam | and NewProvisioner returns one or the other based on the flag you passed in. | 12:45 |
| dimitern | jam, it currently does that | 12:45 |
| fwereade | dimitern, GetAuthorizedKeys I think takes an []tag, and retur an []string | 12:45 |
| dimitern | jam, but not as an interface | 12:46 |
| dimitern | fwereade, machine tag ok, and which env config keys should it return? | 12:46 |
| jam | dimitern: no, it currently returns a Provisioner that has an internal flag set. vs a separate type/struct/whatever | 12:47 |
| jam | dimitern: I guess my point is, we shared a bunch of the code because they look "similar" but honestly there are a *lot* of different bits, and I'd like to split them further apart. | 12:47 |
| fwereade | dimitern, it just returns the valueof authorized-keysin config | 12:47 |
| fwereade | jam, yeah, they should unquestionably be split | 12:47 |
| dimitern | fwereade, ok, one per machine | 12:47 |
| fwereade | jam, that was always the original plan | 12:48 |
| jam | and have 2 implementations that share an interface, rather than 1 concrete type that has a bunch of switch statements. | 12:48 |
| fwereade | jam, but the authkeys-in-config scotched it | 12:48 |
| dimitern | fwereade, and then a follow-up that splits the 2 provisioners into separate types implementing an interface | 12:48 |
| fwereade | dimitern, do you know offhand why there's an environ on p? | 12:48 |
| jam | fwereade: GetAuthorizedKeys returns a []{tag: Name, keys []string} doesn't it? | 12:48 |
| dimitern | fwereade, and a third CL, which binds the previous 2 together | 12:49 |
| jam | well, []{tag: Name, keys []string, Error} | 12:49 |
| dimitern | fwereade, it's an instance broker | 12:49 |
| fwereade | jam, authkey is just a string in the format one would expect in .ssh | 12:49 |
| rogpeppe | jam: we current represent a set of authorized keys as a single string | 12:49 |
| fwereade | dimitern, wtf, why's it called environ then? | 12:49 |
| jam | fwereade: []{tag: Name, keys string, Error} ? | 12:49 |
| dimitern | jam, not really - we can co with just StringsResults - no name neede | 12:49 |
| fwereade | dimitern, +1 | 12:49 |
| fwereade | dimitern, ok, I need to think and read more code a little | 12:50 |
| dimitern | fwereade, because it implements what InstanceBroker's interface was extracted from | 12:50 |
| jam | dimitern: I personally *really* like APIs that return "result + context you requested it" | 12:50 |
| rogpeppe | jam: too late :-) | 12:50 |
| dimitern | jam, we need to unnecessary change all calls | 12:50 |
| dimitern | jam, and it's working well already | 12:51 |
| rogpeppe | jam: also, it's really not necessary - adds more network traffic and more error conditions to check | 12:51 |
| jam | rogpeppe: slightly more traffic vs being able to actually look at a response and instantly understand it.... | 12:51 |
| jam | xml/json are reasonably self documenting over ASN.1 for a reason | 12:52 |
| dimitern | jam, you can look at the request and understand the response | 12:52 |
| jam | dimitern: still takes 2 bits of information, when it could be all together | 12:52 |
| rogpeppe | dimitern: +1. without the request, you're pretty much out of luck anyway | 12:52 |
| dimitern | jam, i really don't see this as a drawback | 12:52 |
| jam | dimitern: so if you take this array of things, and sort of squint at it enough, you can line it up with that array of things. | 12:53 |
| rogpeppe | jam: i could potentially change the rpc package so it always included all the request data with the response | 12:53 |
| jam | *or* you could write it down as an array of tuples | 12:53 |
| jam | and immediately see the pairirng | 12:53 |
| rogpeppe | jam: we never pass more than one thing anyway - the whole point is moot | 12:53 |
| dimitern | jam, and send twice as much over the wire | 12:54 |
| rogpeppe | jam: we might possibly one day have two uses for bulk calls | 12:54 |
| jam | dimitern: one is a tag that is 5-10 bytes, one is an authorized-keys string that is 1000bytes, that is not 2x over the wire | 12:54 |
| fwereade | dimitern, ok, so, that environConfig goes on a loong adventure between the getBroker call and the final FinishMachineConfig, but that's the only place it happens | 12:54 |
| dimitern | jam, rogpeppe: if anything, logging api requests and responses can be improved - like print the request data in the response log | 12:55 |
| fwereade | dimitern, so resolving that in this CL is indeed definitely not an option | 12:55 |
| rogpeppe | fwereade: +1 | 12:55 |
| fwereade | dimitern, however I remain unrepentantly opposed to putting secrets,via the API, anywhere that is not known to be authorized to read those | 12:55 |
| rogpeppe | fwereade: me too | 12:56 |
| dimitern | rogpeppe, and we need to get rid of these discard * call signature debug logs from the rpc | 12:56 |
| rogpeppe | dimitern: they're gone | 12:56 |
| dimitern | rogpeppe, sweet! | 12:56 |
| dimitern | fwereade, ok, it's just a temporary state | 12:56 |
| rogpeppe | dimitern: https://codereview.appspot.com/13249054/ | 12:56 |
| dimitern | fwereade, it'll be fixed in the next 3 cls | 12:56 |
| rogpeppe | dimitern, fwereade: i'd still like a second review of this (martin's request) BTW: https://codereview.appspot.com/13778046/ | 12:57 |
| fwereade | rogpeppe, bugger, I thought I texted you -- I'd just finished reading it when the power went out | 12:57 |
| fwereade | rogpeppe, I said "LGTM" :) | 12:58 |
| rogpeppe | fwereade: ah, you did! my phone was muted | 12:58 |
| rogpeppe | fwereade: i sent you an email, expecting your "from-the-future toy" response... | 12:58 |
| rogpeppe | fwereade: thanks | 12:59 |
| fwereade | dimitern, can we have a quick g+ in 5 please? | 12:59 |
| rogpeppe | fwereade: ha, looks like a merged it anyway! | 12:59 |
| rogpeppe | s/a merged/i merged/ | 13:00 |
| dimitern | fwereade, sure, let me start one | 13:00 |
| dimitern | fwereade, https://plus.google.com/hangouts/_/96f599fd9a86362a79be136169847367eaaf5539 | 13:01 |
| jam | dimitern: http://paste.ubuntu.com/6150016 | 13:02 |
| jam | fwereade: ^^ | 13:02 |
| jam | that is in EnvironConfig | 13:03 |
| jam | we already have the AuthEnvironManager | 13:03 |
| jam | to handle stuff like WatchEnvironMachines | 13:03 |
| jam | so we can just use it here to hide all secrets if we aren't an environ manager | 13:03 |
| dimitern | jam, and we need to restrict WatchForEnvironConfigChanges to the environ manager as well as EnvironConfig() | 13:03 |
| dimitern | jam, then we won't need to delete any attrs | 13:04 |
| dimitern | fwereade, i'm waiting | 13:04 |
| jam | dimitern: so I think we need an entirely new structure for non environ provisioners, but if we just want "don't leak secrets to those machines" that should be enough, I think. | 13:04 |
| jam | as in, we can do a bit of testing and land that in 2 hours | 13:05 |
| jam | well hours not days | 13:05 |
| fwereade | jam, ok, dimitern will be closing the API hole such that the current structure will work without secrets, and in the meantime I'll be finishing the review and trying to come up with a way forward | 13:25 |
| fwereade | jam, well, without *real* secrets anyway | 13:26 |
| TheMue | fwereade: I proposed the latest CL after the review changes again. would you mind to take a look there too? | 13:29 |
| fwereade | TheMue, will do | 13:29 |
| TheMue | fwereade: great, thx | 13:30 |
| rogpeppe | fwereade, mgz, jam, TheMue, dimitern: small review if anyone wants to look: https://codereview.appspot.com/13839046/ | 13:31 |
| TheMue | rogpeppe: already reviewing it | 13:32 |
| rogpeppe | TheMue: ta! | 13:32 |
| TheMue | rogpeppe: done | 13:34 |
| rogpeppe | TheMue: thanks | 13:34 |
| fwereade | dimitern, wait, shit -- that SimpleAuthenticator thing needs a valid environ | 13:36 |
| dimitern | fwereade, to call StateInfo() on it, yeah | 13:38 |
| fwereade | dimitern, so we can't plug the hole until we've handled that | 13:38 |
| fwereade | dimitern, it's easy to be fair becaue I'm pretty sure the provisioner is started with an agent config anyway, so it can clone its own | 13:38 |
| dimitern | fwereade, so should I stop doing the masking of the secret attrs then? | 13:39 |
| fwereade | dimitern, that's still necessary, I'm just trying to figure out the ordering | 13:39 |
| dimitern | fwereade, ah, ok | 13:39 |
| fwereade | dimitern, we can land secret masking right now with no impact | 13:39 |
| fwereade | dimitern, but with secret masking in place, we can't land the provisioner until the StateInfo requirement is fixed | 13:40 |
| fwereade | dimitern, but similarly, we can land provisioner with no impact, but then can't land secret masking until StateInfo is dropped | 13:40 |
| dimitern | fwereade, hmm | 13:42 |
| dimitern | fwereade, but the secretattrs that get masked is only one it seems - "secret", and it's needed by the stateInfo only, we don't need to use it, right? | 13:44 |
| fwereade | dimitern, in practice the ec2 secret-key will be masked | 13:46 |
| fwereade | dimitern, preventing us, I think, from reading the instance from storage and figuring out it address | 13:46 |
| fwereade | dimitern, hey | 13:46 |
| fwereade | dimitern, don't we already have API code for saying what the API addresses are? | 13:47 |
| fwereade | dimitern, I guess maybe we don't | 13:47 |
| dimitern | fwereade, we have one for the deployer | 13:49 |
| fwereade | dimitern, yay! then we should surely reuse that | 13:49 |
| dimitern | fwereade, so we get rid of environ.StateInfo() and call the APIAddresses() instead | 13:50 |
| fwereade | dimitern, yeah, I think so, and if it turns out that some annoying client depend on state.Info being meaningful we can just fill it with nonsense in order to move forward | 13:51 |
| fwereade | dimitern, it's nice that we have that auth interface for easy swapping out, innit ;) | 13:51 |
| dimitern | fwereade, the manual provisioner uses the SimpleAuthenticator as well | 13:52 |
| fwereade | dimitern, that's fine, it has legitimate access to the environ | 13:52 |
| fwereade | dimitern has a power cut too | 13:55 |
| dimitern | hmm it seems it was only my e-meter that tripped off | 14:02 |
| dimitern | fwereade, back | 14:03 |
| fwereade | dimitern, ah cool | 14:04 |
| fwereade | dimitern, https://codereview.appspot.com/13720051/ just reviewed | 14:04 |
| dimitern | fwereade, cheers | 14:05 |
| rogpeppe | dimitern: a few comments from me too that i forgot to publish earlier | 14:06 |
| dimitern | fwereade, you mean all api watchers should return tags? | 14:06 |
| dimitern | fwereade, none of them do | 14:06 |
| dimitern | rogpeppe, thanks | 14:06 |
| === BradCrittenden is now known as bac | ||
| rogpeppe | dimitern: i hadn't noticed that, but for consistency all the watchers should really return tags not ids, i guess | 14:08 |
| dimitern | rogpeppe, you'd have surely noticed if you did a state-to-api migration of existing code :) | 14:09 |
| rogpeppe | dimitern: surely i would :) | 14:10 |
| fwereade | dimitern, part of the point of the relation tag changes was so that the watcher could convert them to tags, iirc,did we forget to do that bit? | 14:10 |
| fwereade | dimitern, when we noticed we knew we were stuck with deployer's watchers but I think the others were fine | 14:11 |
| fwereade | dimitern, ie not yet implemented | 14:11 |
| fwereade | dimitern, eh,we both dropped the ball there I guess | 14:11 |
| fwereade | dimitern, what else have we released with non-tag-based watchers | 14:12 |
| fwereade | ? | 14:12 |
| dimitern | fwereade, all of them | 14:12 |
| dimitern | fwereade, but I need to check which ones exactly | 14:13 |
| fwereade | dimitern, ok, let's not sweat it now | 14:14 |
| rogpeppe | all tests pass | 14:14 |
| dimitern | fwereade, so machiner, deployer, provisioner and uniter all have stringswatchers that need fixing | 14:14 |
| rogpeppe | yay! | 14:14 |
| * rogpeppe says, a little wearily | 14:14 | |
| fwereade | dimitern, tech-debt bug pointing out that all the StringsWatcher use a silly format but it's two-way transformable without context so nbd really | 14:14 |
| fwereade | dimitern, we'll fix it in our Copious Free Time, but at this stage we may as well be consistent and fix them all together | 14:15 |
| dimitern | fwereade, added bug 1229755 | 14:15 |
| _mup_ | Bug #1229755: Watchers returned from the API should report tags, not ids as changes <tech-debt> <juju-core:Triaged by dimitern> <https://launchpad.net/bugs/1229755> | 14:15 |
| fwereade | dimitern, cheers | 14:15 |
| rogpeppe | fwereade: perhas we could change the client code now to accept ids or tags, and transform them to tags if it gets ids. that way it's backward compatible when we move to returning tags from the API | 14:15 |
| dimitern | rogpeppe, this is how all the client (well agent really) code works | 14:17 |
| fwereade | rogpeppe, dimitern, hmm: maybe we could do that in the api stringsWatcher anyway... we can tell what kind a name/id has by inspection, can't we? | 14:17 |
| fwereade | except wait, better yet | 14:18 |
| dimitern | fwereade, except for the relation units watcher | 14:18 |
| rogpeppe | fwereade: i'm not keen on doing that if it's still called "stringsWatcher". "tagsWatcher" would be a better name in that case. | 14:18 |
| fwereade | rogpeppe, actually I had it the wrong way round anyway | 14:19 |
| dimitern | rogpeppe, tags are strings | 14:19 |
| fwereade | rogpeppe, stringsWatcher -- *if* it is indeed actually a names/ids/tags/whatever watcher will still want to produce non-tag things | 14:19 |
| rogpeppe | dimitern: yes, but strings aren't tags | 14:19 |
| dimitern | rogpeppe, i'm -1 on tagsWatcher | 14:20 |
| dimitern | rogpeppe, perhaps entityTagsWatcher | 14:20 |
| rogpeppe | dimitern: sure | 14:20 |
| rogpeppe | dimitern: just i'm -1 on anything tag-related inside something that calls itself "stringsWatcher", implying that it's just a bunch of strings with no other attached semantics | 14:21 |
| fwereade | rogpeppe, dimitern: ok, so, we can in that case wrap the existing stringswatchers with wrappers that untag the strings as they land, if they are in fact tags | 14:21 |
| dimitern | fwereade, why so complicated? | 14:22 |
| dimitern | fwereade, we just assume we always get tags | 14:22 |
| jamespage | fwereade, who is looking at juju memory consumption on service units aka bootstrap node won't run on a tiny | 14:23 |
| fwereade | dimitern, because that's the low-impact change we can make today that gives us forward compatibility with anapi that returns tags from watchers, I think? | 14:23 |
| dimitern | fwereade, changing the stringsWatcher to return tags at client-side api seems low-impact to me too | 14:24 |
| fwereade | jamespage, I think I will have to force some space to look at that myself, I'm afraid :( | 14:24 |
| fwereade | dimitern, except that all the existing code is expecting non-tags | 14:24 |
| rogpeppe | jamespage: various of us have been tangientally involved | 14:25 |
| dimitern | fwereade, the needed changes to start expecting tags from these watchers is minimal | 14:25 |
| fwereade | jamespage, I know jam has been looking at it to dome degree too | 14:25 |
| mgz | jamespage: what we're really missing is some expertise from the other side | 14:25 |
| fwereade | dimitern, but there is code out there in the wild that we have released using ids, and changing to tags will mess with them | 14:25 |
| jamespage | mgz: from the other side? | 14:25 |
| mgz | it seems pretty clear something changed on canonistack, but no idea wgat | 14:25 |
| mgz | *what | 14:26 |
| jamespage | mgz, hmm - well I see exactly the same thing on serverstack | 14:26 |
| dimitern | fwereade, I think we messed up pretty much by being lenient about version upgrades | 14:26 |
| fwereade | dimitern, by implementing clients that can handle tags when they land in the future we can avoid changing the API today | 14:26 |
| dimitern | fwereade, and now we lock ourselves in a vicious circle every time we need to change the api in any way | 14:26 |
| jamespage | mgz, and I think I'm seeing a related issue with the mysql charm | 14:26 |
| mgz | well, or possibly in cloudinit or the images | 14:26 |
| mgz | we went from being able to run on tiny, to not, one afternoon without the juju code being changed (old versions now also exhibit the problem) | 14:27 |
| mgz | so, there's stuff juju can do to make this work, some of which is documented in the bug, but would be good to understand what exactly broke | 14:28 |
| fwereade | dimitern, yeah, I should have raged and screamed at the first version of the API and insisted on explicit version handling from the beginning | 14:28 |
| fwereade | dimitern, but the questions of compatibility just shift there really | 14:28 |
| dimitern | fwereade, but fair enough, I see your point with decorating the stringsWatchers at client-side to untag stuff it gets | 14:28 |
| fwereade | dimitern, and honestly, I don't think even that is really worth doing at this point | 14:29 |
| dimitern | fwereade, then we need to change a bunch of watchers in state to return tags directly | 14:29 |
| fwereade | dimitern, hmm, not so sure, I was thinking that tags were really an API language | 14:29 |
| fwereade | dimitern, the first cut at them did I think leak into state, which is a shame | 14:30 |
| fwereade | dimitern, but the transform necessary from id to tag is not hard to do at Next() time | 14:30 |
| rogpeppe | dimitern: it's easy enough to change the API in a backwardly compatible way if we want - just return both tags *and* ids for the time being, then deprecate the ids field later. | 14:30 |
| fwereade | rogpeppe, indeed | 14:30 |
| dimitern | fwereade, so we don't need to do anything for bug 122975 for now | 14:30 |
| _mup_ | Bug #122975: thunderbird-bin crashed with SIGSEGV in raise() after click in email to show it in the preview pane <mt-needtestcase> <mt-waitdup> <thunderbird (Ubuntu):Invalid by mozilla-bugs> <https://launchpad.net/bugs/122975> | 14:30 |
| fwereade | dimitern, that's a relief | 14:30 |
| fwereade | ;p | 14:30 |
| dimitern | oops bug 1229755 | 14:31 |
| _mup_ | Bug #1229755: Watchers returned from the API should report tags, not ids as changes <tech-debt> <juju-core:Triaged by dimitern> <https://launchpad.net/bugs/1229755> | 14:31 |
| fwereade | dimitern, but yeah we don't need to do anything for bug 1229755 either | 14:31 |
| _mup_ | Bug #1229755: Watchers returned from the API should report tags, not ids as changes <tech-debt> <juju-core:Triaged by dimitern> <https://launchpad.net/bugs/1229755> | 14:31 |
| dimitern | :) | 14:31 |
| dimitern | fwereade, I didn't quite get this https://codereview.appspot.com/13720051/diff/1/cmd/jujud/machine.go#newcode201 | 14:34 |
| fwereade | dimitern, jujud houldn't know about differences between providers | 14:34 |
| === hatch_ is now known as hatch | ||
| dimitern | fwereade, you're asking to add a tech-dept bug, which says something like "We need a MachineJob called JobHostLXCContainers" ? | 14:35 |
| fwereade | dimitern, yeah, we're running LXC based on kooky inferences | 14:35 |
| fwereade | dimitern, we should be getting what to try to run from state, I think | 14:36 |
| dimitern | fwereade, and who should decide whether the machine has that job or not? | 14:36 |
| fwereade | dimitern, the provider | 14:36 |
| dimitern | fwereade, and cmd/juju stuff that adds a machine? | 14:36 |
| fwereade | dimitern, don't think so, no | 14:37 |
| fwereade | hey wait sorry wrong job | 14:37 |
| fwereade | dimitern, ok there is more context mixed in the linked CL | 14:37 |
| fwereade | dimitern, I *think* that we figure out whether we can run containers by asking the environment | 14:38 |
| fwereade | dimitern, but we should talk about this with axw tomorrow morning, I think | 14:38 |
| dimitern | fwereade, ok | 14:38 |
| dimitern | fwereade, morning being? :) | 14:38 |
| fwereade | dimitern, the important thing is that you don't need to take action now but you do need to assign corrective action to yourself and we'll figure out exactly what it should be tomorrow | 14:39 |
| dimitern | fwereade, 10pm our time? | 14:39 |
| fwereade | dimitern, nah, morning our time, I think he's still around then usually | 14:39 |
| dimitern | fwereade, ok, so no bug to file until then | 14:39 |
| fwereade | dimitern, I was going to wait up last night but I don't think he actually get in until 3am or so | 14:39 |
| fwereade | dimitern, I guess the bug is something like "MachineAgent.APIWorker uses witchcraft to start lxc provisioner" | 14:43 |
| axw | fwereade, dimitern: I'm awake for the time being. What's up? | 14:43 |
| fwereade | dimitern, but yeah, we can charactierise it in context with axw tomorrow | 14:43 |
| fwereade | axw, heyhey! | 14:43 |
| axw | :) | 14:43 |
| === natefinch-afk is now known as natefinch | ||
| fwereade | axw, more layering violations of the kind I was bitching about in the null provider review | 14:44 |
| axw | fwereade: is this the LXC provisioner worker thing in the same area? | 14:44 |
| fwereade | axw, machine agent deciding whether to do things based on provider/container type | 14:44 |
| fwereade | axw, close but not directly touching | 14:44 |
| fwereade | axw, the common thread is that machine agents are depending on environment info that's at the wrong level of abstraction | 14:45 |
| fwereade | axw, and that we have a perfectly good mechanism for telling agent what to do already, which is jobs | 14:45 |
| axw | right, as in they check the provider name, rather than checking what capabilities they have | 14:45 |
| fwereade | axw, exactly | 14:46 |
| fwereade | axw, and I thought that since I am telling both of yu that things must be fixed, and could perhap be fixed by defining new jobs, i thought it would be good for us all to be in sync | 14:46 |
| rogpeppe | fwereade, mgz, TheMue, dimitern, axw, natefinch, jam: i'd love it if someone could take a look at this: https://codereview.appspot.com/13573046 | 14:46 |
| axw | fwereade: righto | 14:46 |
| rogpeppe | it's unfortunately big, but not really splittable | 14:46 |
| axw | fwereade: so, I was looking at a TODO you left in bootstrap.go regarding customising jobs | 14:46 |
| fwereade | axw, yeah, that's the one :) | 14:47 |
| axw | fwereade: I think that's going to be necessary here | 14:47 |
| axw | and that leads to the question I emailed you regarding upgrade | 14:47 |
| mgz | rogpeppe: okay, as it's so small :) | 14:47 |
| fwereade | axw, yeah, it's a little fiddly, and in your case involves sending special instructions via cloud-init, I think | 14:47 |
| * rogpeppe blows mgz a kiss | 14:47 | |
| axw | fwereade: for local-storage, yes | 14:47 |
| axw | dimitern: you can reference 1229507 if you like | 14:49 |
| axw | once one's done, it should be simple to do a second (or third, as is the case) | 14:49 |
| dimitern | axw, ok, mine will be similar | 14:49 |
| * rogpeppe goes back to the branch for which i made these changes... 46 branches previously | 14:50 | |
| fwereade | axw, ok, updating jobs should be pretty tolerable, but there is currently no code that does this IIRC | 14:51 |
| fwereade | axw, I will expand on that in the response | 14:53 |
| TheMue | rogpeppe: I like your large ones | 14:53 |
| axw | fwereade: to my naive mind, this sounds like it might require generic upgrade support, which could also be used for state schema upgrades | 14:53 |
| fwereade | dimitern, I will cc you in | 14:53 |
| rogpeppe | TheMue: ha ha! | 14:53 |
| fwereade | axw, yeah, and at least the actual upgrading is now behind the api server and could thus perhaps be managed with a bit more finesse | 14:54 |
| * rogpeppe likes a good double entendre | 14:54 | |
| * fwereade considers that to be barely a single entendre | 14:54 | |
| jamespage | mgz, for context the mysql charms attempt to configure mysql with 80% of total memory on a service unit | 14:57 |
| jamespage | this borkes fairly frequently now | 14:57 |
| mgz | interesting | 14:57 |
| jamespage | mgz, different openstack deployment but I see the same issue with the bootstrap node on a m1.tiny | 14:58 |
| mgz | so, the main issue we were seeing was on bootstrap, starting jujud during cloudinit setup failing due to being short on memory | 14:58 |
| TheMue | rogpeppe: and please change NewMem() to NewMemory() (or at least NewDisk() to NewDsk(), just to make it similar *smile*) | 14:58 |
| mgz | mysql charm issues is a later point, and there's no mongo install happening | 14:58 |
| rogpeppe | TheMue: somehow NewMemory doesn't do quite the right thing for me as NewMem. I'm not quite sure why. | 14:59 |
| rogpeppe | TheMue: and Mem is a very commonly used abbreviation (e.g. memcached, etc) | 14:59 |
| mgz | jamespage: do you have any clues on what could have changed? | 14:59 |
| mgz | we were presumably close to the limit previously, but something made everyone start hitting this issue... | 15:00 |
| rogpeppe | TheMue: i'm not sure it's worth bikeshedding over | 15:00 |
| jamespage | mgz, poking again right now | 15:00 |
| TheMue | rogpeppe: still NewMem reads strange ;) | 15:00 |
| rogpeppe | TheMue: to do it right, it would probably be NewMemoryBasedConfigStorage | 15:01 |
| rogpeppe | TheMue: but i'm actually reasonably happy with NewMem | 15:01 |
| TheMue | rogpeppe: yeah, java style | 15:01 |
| TheMue | rogpeppe: ok, can live with it | 15:01 |
| TheMue | rogpeppe: NewMBCS() | 15:02 |
| TheMue | ;) | 15:02 |
| rogpeppe | TheMue: :) | 15:02 |
| TheMue | rogpeppe: reviewed | 15:04 |
| rogpeppe | TheMue: that was quick! thanks. | 15:04 |
| jamespage | mgz, doh! I already forced the dataset-size smaller for my config | 15:05 |
| * jamespage pushed the car back up the hill to see if it crashes next time | 15:05 | |
| TheMue | rogpeppe: most have been changes using the NewMem as additional arg, so here the reading has been simple | 15:05 |
| rogpeppe | TheMue: cool | 15:05 |
| rogpeppe | mgz: were you reviewing it; if not, i'll approve it now. | 15:05 |
| mgz | nearly there | 15:06 |
| mgz | nothing substantive | 15:06 |
| mgz | I have some sympathy with TheMue's complaint about NewMem now he's emntioned it | 15:06 |
| TheMue | mgz: also want a change from NewMem to NewMemory? ;) | 15:06 |
| fwereade | TheMue, lgtm with a couple of comments | 15:06 |
| TheMue | mgz: h5 | 15:06 |
| TheMue | fwereade: ta | 15:06 |
| fwereade | rogpeppe, onto yours in a few minutes :) | 15:07 |
| mgz | rogpeppe: posted | 15:07 |
| rogpeppe | fwereade: cool, thanks | 15:07 |
| rogpeppe | mgz: you're dead right about TestDestroyEnvironmentCommandConfirmation; i'll split it. | 15:08 |
| mgz | there are still so many little aspects of the go language that trip me up... | 15:09 |
| mgz | return statements being one of the more trivial... | 15:09 |
| TheMue | fwereade: the hook name export has indeed an error, overlooked that i'm using it in the same package | 15:12 |
| natefinch | mgz: what about return statements trips you up? | 15:13 |
| mgz | there are so many possible variations to how they'll look in a function | 15:18 |
| natefinch | mgz: I think as long as people avoid bare returns (which I think are an anti-pattern, and I wish they'd do away with them), then it's not so different than other languages... except for the multiple returns | 15:21 |
| rogpeppe | fwereade: are you still planning to look at that branch? | 15:52 |
| fwereade | rogpeppe, sorry, the first file is still open | 16:14 |
| rogpeppe | fwereade: np; i'm still working on a fix for TestBootstrapWithDefaultSeries live test as it happens | 16:14 |
| rogpeppe | fwereade: do you know what the deal is with tools/juju-1.15.0-raring-amd64.tgz versus tools/releases/juju-1.15.0-raring-amd64.tgz ? | 16:15 |
| rogpeppe | fwereade: it seems to be uploading to the latter, but StorageName is returning the former | 16:16 |
| fwereade | rogpeppe, huh | 16:16 |
| fwereade | rogpeppe, there is some horrible patching function | 16:16 |
| rogpeppe | fwereade: any clues as to where it might be? | 16:17 |
| * fwereade meditates | 16:17 | |
| fwereade | rogpeppe, environs/tool/storage.go, right at the top | 16:18 |
| fwereade | rogpeppe, I would guess there's something screwy about the patching/unpatching | 16:18 |
| fwereade | rogpeppe, *but* I cannot recall why it was needed in the first place | 16:18 |
| rogpeppe | WTF!?!?! | 16:18 |
| rogpeppe | we set global variables for testing purposes, but SetTestPrefix is bonkers | 16:19 |
| rogpeppe | it means that SyncTools is fundamentally not thread-safe | 16:20 |
| * rogpeppe feels slightly queasy | 16:21 | |
| rogpeppe | fwereade: ^ | 16:22 |
| fwereade | rogpeppe, I do recall seeing it and being scared before, but being on the trail of bigger game, so your surmise is probably correct | 16:23 |
| fwereade | rogpeppe, so we call that outside a testing context? | 16:23 |
| fwereade | rogpeppe, or just that the tests are screwy? | 16:23 |
| rogpeppe | fwereade: we call it in ReadList | 16:24 |
| fwereade | rogpeppe, uhh? | 16:24 |
| rogpeppe | fwereade: and in copyOneToolsPackage | 16:24 |
| rogpeppe | fwereade: which is called by SyncTools | 16:24 |
| rogpeppe | fwereade: and Upload calls it too | 16:25 |
| rogpeppe | fwereade: as does SyncTools itself | 16:25 |
| fwereade | rogpeppe, well, while it *might* coincidentally be safe, yeah, that's crack | 16:26 |
| rogpeppe | fwereade: it's just random side-effects unrelated to the function | 16:26 |
| rogpeppe | fwereade: ReadList, for example just changes the global tool prefix if it gets ErrNoTools | 16:27 |
| rogpeppe | fwereade: oh actually it doesn't, sorry | 16:27 |
| fwereade | rogpeppe, yeah, I think it always cleans up after itself | 16:27 |
| fwereade | rogpeppe, but it's totally unaware of concurrency | 16:27 |
| rogpeppe | fwereade: even if it was, it would be totally wrong | 16:28 |
| rogpeppe | fwereade: you could protect it with a mutex and it would be just as wrong | 16:28 |
| fwereade | rogpeppe, yeah, it's a pretty fundamentally insane operation, that stuff should be passed around, no question | 16:28 |
| rogpeppe | fwereade: why did the tools need to move, BTW? | 16:30 |
| fwereade | rogpeppe, I have absolutely no recollection of that, I'm afraid | 16:30 |
| rogpeppe | fwereade: because we *could* put in lots of extra context and change quite a bit of code that assumes that names are independent of storage context, *or* we could just leave the names as they are | 16:31 |
| rogpeppe | wallyworld: ping | 16:31 |
| rogpeppe | fwereade: well, i think i'll leave that test as broken as is, as it's currently broken on trunk | 16:35 |
| rogpeppe | fwereade: and i'll file a bug | 16:36 |
| rogpeppe | fwereade: and i will approve my branch, unless you want to have a look first. | 16:36 |
| fwereade | rogpeppe, I'm reading through it, if I find any reason to panic I will be sure t oinform you | 16:38 |
| rogpeppe | fwereade: i'm sure you will :-) | 16:39 |
| rogpeppe | fwereade: if you think you'll finish tonight, i'll hold off | 16:39 |
| rogpeppe | fwereade: https://bugs.launchpad.net/juju-core/+bug/1229839 | 16:46 |
| _mup_ | Bug #1229839: provider/ec2: LiveTests.TestBootstrapWithDefaultSeries is broken <juju-core:New for wallyworld> <https://launchpad.net/bugs/1229839> | 16:46 |
| fwereade | rogpeppe, nice, thanks | 16:47 |
| fwereade | rogpeppe, LGTM, that's great | 16:59 |
| rogpeppe | fwereade: cool, thanks | 16:59 |
| fwereade | rogpeppe, am I sane re: https://codereview.appspot.com/13573046/diff/5001/environs/open.go#newcode101 ? | 17:00 |
| rogpeppe | fwereade: yeah, i feel quite good about the direction we're going | 17:00 |
| rogpeppe | fwereade: i've been thinking about this issue | 17:01 |
| rogpeppe | fwereade: short answer: yes, i was planning on just storing a diff | 17:02 |
| fwereade | aww | 17:02 |
| fwereade | it'll be big and smelly and redundant | 17:02 |
| rogpeppe | fwereade: because otherwise the attributes may well have changed between the time you call Prepare and the time you call Open | 17:02 |
| fwereade | loads of auto-inserted defaults | 17:02 |
| rogpeppe | fwereade: that's true, but at least you'll have one single place to see all the settings that go into an environment | 17:03 |
| rogpeppe | fwereade: that smelliness is actually our genuine stink | 17:03 |
| fwereade | rogpeppe, well, it's still two places if you consider what's in environment.yaml ;) | 17:04 |
| rogpeppe | fwereade: eventually, i hope that environments.yaml will actually be replaced by a network call to fetch actual config values | 17:04 |
| fwereade | rogpeppe, I do agree there's a foul smell to the whole thing | 17:04 |
| fwereade | rogpeppe, eh, the CLI should hardly ever need to know them in the first place | 17:05 |
| fwereade | rogpeppe, bootstrap is, I think, the special case | 17:05 |
| rogpeppe | fwereade: that's true | 17:05 |
| rogpeppe | fwereade: and in most cases, the CLI will never see any attributes at all | 17:05 |
| rogpeppe | fwereade: they're only relevant for the bootstrapper | 17:06 |
| rogpeppe | fwereade: i think that, for an admin, knowing that they can bootstrap, then copy *only* environments.yaml and environments/name.xxx (ext TBD!) to another machine, is a really useful property | 17:07 |
| rogpeppe | fwereade: without needing to remember to copy across any number of provider-specific env vars | 17:08 |
| rogpeppe | too | 17:08 |
| rogpeppe | fwereade: how about we go with "just a diff" to begin with, and if we think it looks too smelly, we can trim it down later. | 17:08 |
| rogpeppe | ? | 17:09 |
| rogpeppe | fwereade: i've got to go | 17:19 |
| fwereade | rogpeppe, sorry, got called away myself | 17:19 |
| fwereade | rogpeppe, I thought it'd be 5 seconds... | 17:19 |
| fwereade | rogpeppe, the env vars are a very good point, but my heart still yearns for making the provider -- that knows about that -- responsible for recording those | 17:20 |
| fwereade | rogpeppe, I'm not against the env file + environments.yaml, I think that's good, but I don't think it needs defaults inserted | 17:20 |
| fwereade | rogpeppe, values read from other files, probably, yes, though | 17:21 |
| fwereade | rogpeppe, anyway I must return | 17:21 |
| rogpeppe | fwereade: we'll chat about this tomorrow | 17:21 |
| rogpeppe | fwereade: g'night! | 17:21 |
| rogpeppe | fwereade: BTW, only provider defaults will go in, as it's currently structured. | 17:23 |
| rogpeppe | fwereade: and it's perhaps actually nice to see what those are | 17:23 |
| rogpeppe | fwereade: by my logic above though, we'd should put in authorized keys and all the other defaults, making them concrete. | 17:24 |
| * rogpeppe is really gone now | 17:24 | |
| natefinch | mgz, fwereade. jam: if there's anyone left online: https://codereview.appspot.com/13802045 | 17:39 |
| mgz | looking | 17:39 |
| mgz | ah, and it's one I really should review too | 17:40 |
| mgz | tas is stored as a list in Constraints I take it? | 17:41 |
| mgz | hence the change to IsEmpty checks rather than compare to {} | 17:41 |
| mgz | *tags | 17:41 |
| * mgz j-s it up | 17:41 | |
| natefinch | mgz: yeah, I added a comment to that effect after I realized it wasn't going to be obvious at first | 17:50 |
| mgz | I'm still not sure about that vs just storing a space-seperated string | 17:53 |
| mgz | (for other reasons, not because it would save an Isempty function, that's fine) | 17:53 |
| natefinch | in general I really like to store lists of strings as lists of strings... then you let each consumer reformat (or not) as necessary | 17:53 |
| natefinch | then it's really clear "hey, there's multiple values here" and you don't have to go look at the implementation to know how to separate the values | 17:54 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!