| davecheney | bugs, so many bugs | 00:05 |
|---|---|---|
| bac | hi bigjools | 00:40 |
| bigjools | o/ | 00:40 |
| bigjools | OTP | 00:40 |
| bac | np, just saying hello | 00:40 |
| bigjools | hello bac :) | 00:40 |
| bodie_ | I need some guidance here: http://bazaar.launchpad.net/~binary132/juju-core/charm-actions/view/head:/charm/actions.go | 01:05 |
| bodie_ | this bit in particular (line 58): actionsSpec.ActionSpecs[name].Params = map[string]interface{}{} | 01:05 |
| bodie_ | I'm trying to initialize the Params member with an empty interface (I've also tried make( map[string]interface{} ) with the same results but it's giving me the following error: | 01:06 |
| davecheney | looking for a review on this | 01:07 |
| davecheney | https://github.com/juju/errgo/pull/6 | 01:07 |
| davecheney | wallyworld_: nate, jimmiebtlr_ | 01:07 |
| bodie_ | cannot assign to actionsSpec.ActionSpecs[name].Params | 01:07 |
| davecheney | no, jam | 01:07 |
| bodie_ | presumably because Params is an uninitialized map | 01:07 |
| davecheney | bodie_: i don't think reflect is necessary | 01:08 |
| bodie_ | you can do equality on a potentially nested map without reflect? | 01:08 |
| davecheney | if actionSpec.ActionSpec[name].Params == nil { actionSpec.ActionSpec[name].Params = make(map[string]interface{}) | 01:08 |
| wallyworld_ | davecheney: lgtm, i like it | 01:08 |
| davecheney | } | 01:08 |
| bodie_ | I tried that, but it gives me the same error | 01:09 |
| bodie_ | it doesn't want me to assign values to that member... I'm not seeing why | 01:09 |
| davecheney | because maps are not addressible | 01:09 |
| davecheney | bodie_: why do you need to initalise that map | 01:10 |
| davecheney | line 60 will work fine with the zero value | 01:10 |
| bodie_ | well, I don't necessarily need to. what's happening is that I'm expecting an empty map in my test but getting nil | 01:10 |
| bodie_ | which got me thinking | 01:10 |
| davecheney | var actionsSpec *Actions | 01:11 |
| davecheney | if err := goyaml.Unmarshal(data, &actionsSpec); err != nil { | 01:11 |
| bodie_ | perhaps if the "params" key has no values, it should be defined as an empty value, rather than nil, that way it can be handled without nil checks | 01:11 |
| davecheney | ^ ??? | 01:11 |
| bodie_ | what's the question | 01:11 |
| davecheney | your passing **Action to Unmarshal | 01:11 |
| davecheney | shouldn't this be | 01:11 |
| bodie_ | one moment sorry | 01:11 |
| davecheney | var actionSpec Actions | 01:11 |
| bodie_ | i'll be back | 01:11 |
| davecheney | goyaml.Unmarshal(date &actionSpec) | 01:12 |
| bodie_ | okay, looking | 01:15 |
| bodie_ | child just awoke suddenly... | 01:15 |
| bodie_ | mh | 01:16 |
| bodie_ | hm* | 01:16 |
| axw | wallyworld_: hey. are you on mgz's box? | 01:18 |
| wallyworld_ | no | 01:18 |
| axw | k | 01:18 |
| davecheney | wallyworld_: follow up for ya https://code.launchpad.net/~dave-cheney/juju-core/178-update-errgo-and-loggo-dependencies/+merge/220340 | 01:19 |
| wallyworld_ | rightio | 01:19 |
| wallyworld_ | land that sucker | 01:21 |
| axw | wallyworld_: are you looking at the bzr error, or should I? | 01:21 |
| wallyworld_ | axw: i haven't looked yet | 01:22 |
| wallyworld_ | it did happen after my branch landed | 01:22 |
| axw | hrm ok | 01:23 |
| wallyworld_ | but off hand i can't see the causal link | 01:23 |
| axw | I wonder if it's due to the bzr config not being found in $HOME | 01:23 |
| wallyworld_ | leave it and doing another one if you want, likr the new mongo failure | 01:23 |
| axw | ok | 01:23 |
| wallyworld_ | maybe | 01:23 |
| wallyworld_ | axw: but it does pass here locally | 01:23 |
| axw | I see | 01:23 |
| wallyworld_ | so could be pythin version related according to curtis | 01:23 |
| bodie_ | davecheney, seems weird that it would work even though I had the error you pointed out | 01:25 |
| bodie_ | anyway, corrected that but it's still giving me the same thing | 01:25 |
| bodie_ | with make as well as the way I had it | 01:26 |
| wallyworld_ | axw: it does seem that in CI, that ensure admin user stuff is happy now | 01:27 |
| axw | hooray | 01:27 |
| wallyworld_ | but of course we have the new failures :-( | 01:27 |
| axw | these last couple have been a massive PITA | 01:27 |
| axw | yeah | 01:27 |
| davecheney | faaaaaaaaaaaaaaark | 01:27 |
| davecheney | https://code.launchpad.net/~dave-cheney/juju-core/178-update-errgo-and-loggo-dependencies/+merge/220340 | 01:27 |
| davecheney | this is really gettig me down | 01:27 |
| axw | never ending | 01:27 |
| wallyworld_ | yep :-( | 01:27 |
| davecheney | bodie_: can you paste the error | 01:27 |
| davecheney | please | 01:27 |
| bodie_ | sure thing | 01:28 |
| bodie_ | cannot assign to actionsSpec.ActionSpecs[name].Params | 01:28 |
| davecheney | bodie_: i need the code, the line number and the error | 01:28 |
| axw | davecheney: I think that means the bot needs manually updating | 01:28 |
| wallyworld_ | davecheney: i'm not a git expert so have no idea wtf that error means | 01:28 |
| davecheney | wallyworld_: what axw said | 01:29 |
| axw | wallyworld_: godeps -u doesn't actually pull new revs | 01:29 |
| davecheney | axw: nope, because godeps creates disconneted heads | 01:29 |
| wallyworld_ | so godeps needs fixing? | 01:29 |
| davecheney | which is a git word for, i dunno, branches that have no upstream | 01:29 |
| davecheney | wallyworld_: yup | 01:29 |
| wallyworld_ | i can manually update the bot | 01:29 |
| davecheney | wallyworld_: thanks | 01:29 |
| wallyworld_ | davecheney: can you file a bug for the godeps thing? | 01:30 |
| bodie_ | davecheney, charm/actions.go:58: cannot assign to actionsSpec.ActionSpecs[name].Params | 01:30 |
| bodie_ | http://bazaar.launchpad.net/~binary132/juju-core/charm-actions/view/head:/charm/actions.go | 01:30 |
| bodie_ | same error if I use a composite literal of map[string]interface{} | 01:32 |
| bodie_ | er... append {} | 01:33 |
| davecheney | bodie_: ok | 01:34 |
| davecheney | sorry | 01:34 |
| davecheney | there are two things going on | 01:34 |
| davecheney | 1. map values may not be assignable | 01:34 |
| davecheney | but | 01:34 |
| davecheney | 2. you probaly don't need to do all that defensive coding | 01:34 |
| * davecheney reads the code | 01:34 | |
| wallyworld_ | bot fixed, tests running | 01:34 |
| davecheney | wallyworld_: got another one for you https://code.launchpad.net/~dave-cheney/juju-core/179-remove-thirdparty-pbkdf2-dependency/+merge/220341 | 01:35 |
| wallyworld_ | ok | 01:35 |
| bodie_ | I don't necessarily want to assign anything except an empty but non-nil value | 01:35 |
| bodie_ | just so I don't have people stumbling on nil param sets if the action doesn't take any parameters | 01:36 |
| bodie_ | I had a devil of a time trying to figure this out earlier | 01:36 |
| bodie_ | but, I guess I can leave it nil -- I just don't want inability to be what prevents me from doing it right | 01:36 |
| bodie_ | frustrating | 01:36 |
| davecheney | bodie_: leave it as nil | 01:37 |
| davecheney | bodie_: i'm really not sure about all the use of reflect in that file | 01:38 |
| davecheney | it shouldn't be necessary | 01:38 |
| davecheney | in fact, why won't that struct just unmarshall directly ? | 01:39 |
| davecheney | bodie_: dont you just need to add some `yaml:...` tags to the ActionSpec struct | 01:39 |
| davecheney | and it should unmarshal directly | 01:39 |
| davecheney | without having to do it by hand | 01:39 |
| davecheney | i'm clearly missing something | 01:40 |
| bodie_ | I'm probably not fully understanding the difference between what you're suggesting and what I have here -- I'm not certain I can do it justice, but I've been over it with William several times and he thought it important to leave room for metadata that we specify, in addition to a more flexible Params map per action which only need conform to JSON-Schema | 01:42 |
| davecheney | bodie_: can you show me a sample actions.yaml file | 01:43 |
| bodie_ | the deep equality was because the compiler complained about the use of normal equality, and it seemed sensible since there could be nested maps | 01:43 |
| bodie_ | yeah, there's an example in my mr... let's see | 01:43 |
| bodie_ | https://codereview.appspot.com/94540044 | 01:44 |
| davecheney | bodie_: it feels like you are fighting the language | 01:44 |
| bodie_ | perhaps a bit :) | 01:44 |
| davecheney | and you'll loose, 'cos its really stubourn | 01:44 |
| davecheney | so i'd like to help find a better way to do what you want to do, rather than help you win a small battle with the compiler | 01:44 |
| bodie_ | that usually means I'm not properly understanding the way it wants me to do things :) | 01:44 |
| davecheney | so looking at that definition | 01:45 |
| davecheney | you wnt to do somethign like | 01:45 |
| davecheney | type ActionSpec struct { Description string ; Params map[string]interface{} } | 01:46 |
| davecheney | actionSpecs := make(map[string]ActionSpec) | 01:47 |
| davecheney | goyaml.Unmarshal(r, &actionSpecs) | 01:47 |
| davecheney | that should be pretty close | 01:47 |
| davecheney | then you'll need to go through the params of each ActionSpec to do the json validation | 01:48 |
| davecheney | but that should be a separate function | 01:48 |
| * davecheney doesn't even know what json validatoin means | 01:48 | |
| davecheney | it sounds like an oxymoron | 01:48 |
| bodie_ | json-schema is a subset of json which has to conform to a spec | 01:48 |
| bodie_ | it's to define the format of json objects which will be passed as the arguments to the action | 01:48 |
| bodie_ | invalid params will be rejected | 01:49 |
| bodie_ | and you can just check it against the JsonSchemaDoc to validate the parameters | 01:49 |
| bodie_ | this is the argument in favor of this | 01:49 |
| davecheney | sure | 01:49 |
| davecheney | but that doens't need to be wedded to the yaml decoding process | 01:50 |
| bodie_ | so, we're ensuring that this yaml matches the format we need for json-schema, since charm coders like yaml, or something | 01:50 |
| davecheney | it should just be a function that takes a map[string]interface{} and returns an error | 01:50 |
| wallyworld_ | arosales: did you end up meeting with the joyent folks? or is that yet to happen? i don't think i need to be there - hopefully the pull requests are self explanatory | 01:50 |
| bodie_ | I think the goyaml.Unmarshal bit is making the map, but I could be wrong | 01:52 |
| bodie_ | davecheney, we're just making sure at the time of unmarshaling that it's valid | 01:59 |
| bodie_ | the jsonschema bit just checks that, does nothing with the result | 01:59 |
| bodie_ | then, I'm going through each action in the list, making sure it has an OK name, and then checking to make sure its params do | 02:07 |
| bodie_ | that's really all | 02:07 |
| bodie_ | if the params map is nil, I'm trying to make it empty instead | 02:07 |
| * davecheney twiddles thumbs waiting for the bot | 02:21 | |
| axw | jam: you put up a branch fixing this issue before, right? but it didn't get proposed for trunk? #1320794 | 02:40 |
| _mup_ | Bug #1320794: After bootstrapping an OpenStack environment, juju tries to connect to the internal IP <addressability> <landscape> <openstack-provider> <juju-core:Triaged> <https://launchpad.net/bugs/1320794> | 02:40 |
| arosales | wallyworld: I haven't meet with them yet, but I think they are working on the pull requests | 04:46 |
| wallyworld | arosales: great, thanks, let me know if i can do anything | 04:47 |
| arosales | wallyworld: np will do | 04:47 |
| jam | axw: IIRC we put one together that made it so we didn't use networks marked MachineLocal, however, it is possible the auto-detection in Openstack is wrong. | 04:54 |
| jam | I don't think I did the fix | 04:54 |
| axw | jam: ok. thought you did something for ODS | 04:58 |
| axw | maybe that was it | 04:58 |
| jam | axw: ah the ODS thing was attached to a bug but never proposed because it didn't have any tests. | 04:58 |
| jam | axw: that was https://bugs.launchpad.net/juju-core/+bug/1308767 | 04:59 |
| _mup_ | Bug #1308767: juju client is not using the floating ip to connect to the state server <addressability> <juju-core:Triaged> <https://launchpad.net/bugs/1308767> | 04:59 |
| jam | it does have a branch associated, but without tests or someone at least trying it in the wild, I wasn't confident about it. | 05:00 |
| jam | you're welcome to pick it up and finish it off | 05:00 |
| jam | it *seems* correct to me | 05:00 |
| axw | jam: thanks, that's what I was thinking about. I will take a look after my current work | 05:00 |
| wallyworld | jam: do you have any idea what might cause "bzr add" to crash hard and print out "(PC=0x4143E6)" | 05:05 |
| jam | wallyworld: that looks like a stack trace, which would sound like a C sort of extension, which *sounds* like a plugin. | 05:06 |
| jam | try "bzr add --no-plugins" ? | 05:06 |
| jam | well s/stack trace/core dump/ | 05:06 |
| wallyworld | jam: this is on jenkins, in some of the tests | 05:06 |
| wallyworld | bug 1321282 | 05:07 |
| _mup_ | Bug #1321282: PANIC: branch_test in trusty <ppc64el> <regression> <test-failure> <juju-core:Triaged> <https://launchpad.net/bugs/1321282> | 05:07 |
| wallyworld | i guess we could modify the tests | 05:07 |
| wallyworld | to add the --no-plugins arg | 05:07 |
| jam | wallyworld: so the PC=... stuff is Go | 05:07 |
| jam | not bzr | 05:07 |
| wallyworld | i could have msiread the source code. it looked like when the cmd.exec failed it printed the failing cmd line and then the stdout | 05:08 |
| wallyworld | i'll look closer | 05:08 |
| jam | wallyworld: output is a []byte, so maybe it is giving the array rather than the string content? | 05:09 |
| wallyworld | seems strange that it's just the bzr commands, hence my suspicion based on no facts that bzr might be involved | 05:09 |
| wallyworld | could be yeah | 05:09 |
| jam | wallyworld: my guess is that the failure is actually that bzr isn't present | 05:09 |
| jam | note that we aren't checking "err" | 05:09 |
| jam | which could be saying "ENOENT" | 05:09 |
| wallyworld | yeah, true | 05:10 |
| wallyworld | i'll modify the test to give better output | 05:10 |
| wallyworld | i haven't really looked that closely | 05:10 |
| davecheney | does anyone know anything about a Tagger interface ? | 05:42 |
| davecheney | for things that have, uh, a tag ? | 05:42 |
| fwereade | davecheney, I think all state entities have one | 05:52 |
| fwereade | davecheney, the tag is the definitely-unambiguous identifier we use over the API | 05:53 |
| fwereade | davecheney, ie unit-mysql-3, machine-7-lxc-1, service-blah, relation-some-horrible-mess | 05:53 |
| fwereade | davecheney, except we don't always use it over the API, and sometimes for no explicable reason someone allows them to leak into the UI | 05:54 |
| davecheney | fwereade: there is no state.Tagger interface definition | 05:54 |
| davecheney | nm, i just made one up | 05:54 |
| davecheney | fwereade: i can easily move that definition into the state package for better feng shui | 05:56 |
| fwereade | davecheney, heh, it's state.Entity | 06:37 |
| davecheney | fwereade: right | 06:46 |
| davecheney | will fix | 06:46 |
| davecheney | or I can fix in a followup | 06:46 |
| TheMue | morning | 07:31 |
| dimitern | hey TheMue | 07:51 |
| axw | wallyworld: FYI, bzr tests are barfing on my laptop | 07:53 |
| wallyworld | oh great :-( | 07:53 |
| axw | wallyworld: I can take that next if you're not on top of it | 07:53 |
| wallyworld | axw: i've been caught up with other stuff o haven't yet got root cause sussed out. i'll take another look and we can discuss at standup | 07:54 |
| wallyworld | bug 1321009 is pretty important | 07:55 |
| axw | wallyworld: sure | 07:55 |
| axw | wallyworld: is that what you're looking at now? | 07:55 |
| axw | wallyworld: or I should take a look? | 07:55 |
| wallyworld | nah, gccgo stuff | 07:55 |
| axw | ok | 07:55 |
| wallyworld | feel free | 07:55 |
| voidspace | morning all | 08:00 |
| dimitern | morning voidspace | 08:03 |
| dimitern | (just how it sounds :) | 08:04 |
| wallyworld | axw: i just got the failing bzr tests as well although i could have sworn they passed before as i did a complete test run. go figure | 08:06 |
| fwereade | hey all | 08:06 |
| fwereade | I'd love a review of https://codereview.appspot.com/94540043/ | 08:07 |
| axw | wallyworld: me too, though I did a pull earlier... I'm pretty sure I had all of your changes already though | 08:07 |
| dimitern | fwereade, jam, vladk|offline, I'd really appreciate a second look on this goamz CL https://codereview.appspot.com/98430044/ | 08:11 |
| * dimitern is away for a while | 08:11 | |
| tasdomas | fwereade, hi | 08:29 |
| fwereade | tasdomas, heyhey | 08:30 |
| fwereade | dimitern, oops, looking | 08:30 |
| fwereade | tasdomas, (I can talk and review at the same time, at least to some degree) | 08:32 |
| tasdomas | fwereade, was thinking about the ensure-availability MP - should we display the Promote/Demote/Remove information always or just in verbose mode? | 08:32 |
| axw | wallyworld: I think it will be very difficult to backport that fix for 1.18 | 08:33 |
| axw | it relies on a bunch of existing changes in 1.19 that allow dialling multiple addresses | 08:33 |
| wallyworld | axw: ok, it seemed like something we would have wanted | 08:33 |
| fwereade | tasdomas, I think we should generally display feedback, and have a --quiet option; --verbose remains meaningful, it just means *loads* of output | 08:33 |
| wallyworld | axw: i think a 2nd opinion to mine as to how important it is would be desireable | 08:34 |
| axw | wallyworld: ideally yes, but I think it should probably just wait for 1.20 unless it's critical (which I assume it's not, since this is nothing new) | 08:34 |
| wallyworld | ok | 08:34 |
| tasdomas | fwereade, so would this sort of output look sane to you: http://paste.ubuntu.com/7496511/ ? | 08:43 |
| wallyworld | axw: FFS :-( https://codereview.appspot.com/100590048 | 08:46 |
| mgz | wallyworld: aha | 08:56 |
| mgz | wallyworld: what's the reason? | 08:57 |
| wallyworld | mgz: i was dumb | 08:57 |
| axw | mgz: bzr wants a home | 08:57 |
| axw | it tries to read /.bazaar if $HOME=="" | 08:57 |
| mgz | oh yeah, \bsse just removes the envvar | 08:58 |
| axw | wallyworld: mgz sorry to be a pain, going to be a little late again - gotta get my daughter to have a shower... back real soon | 08:58 |
| wallyworld | yep | 08:58 |
| wallyworld | ok, np | 08:58 |
| fwereade | tasdomas, sorry: I'd say --quiet can suppress all output, and it would be perfect if normal mode output the final list of expected sate servers | 09:09 |
| tasdomas | fwereade, ok - what do you mean by "final list of expected state servers" ? | 09:10 |
| fwereade | tasdomas, if we add a new one it isn't a state server *yet*, but we expect that in a couple of minutes it will be | 09:11 |
| axw | wallyworld mgz: back | 09:12 |
| tasdomas | fwereade, so instead of printing the promotion/demotion lists, I should just print a list of what state servers are expected to be operating once the ensure-availability process completes (i.e. collapse those lists)? | 09:15 |
| fwereade | tasdomas, yeah, I reckon the lists are maybe best for --verbose | 09:16 |
| fwereade | tasdomas, the thing most people care about is "which ones are now my state servers" | 09:17 |
| fwereade | tasdomas, with an option on "which ones used to be but aren't any more" | 09:17 |
| tasdomas | fwereade, ok - I will update the branch | 09:18 |
| dimitern | fwereade, how's the looking going? :) | 09:42 |
| * fwereade glances guiltily at dimitern, changes tabs | 09:43 | |
| dimitern | :) | 09:45 |
| dimitern | np | 09:45 |
| mgz | wallyworld: I wonder if we should switch over goose or something first, less active than core and may avoid some early pain | 09:45 |
| wallyworld | mgz: not a bad idea. but i guess we already have other things in github, but not the landing bot support. let's see how we go over the next day or so. at this stage i think we should just rip off the band aid | 09:47 |
| wallyworld | also, the guys getting store out of core are waiting for us | 09:47 |
| wallyworld | gotta run though, back later maybe | 09:48 |
| mgz | wallyworld: goose is on the landing bot atm I mean, so we need both and it's the same steps | 09:48 |
| mgz | just doing the smaller one first will give a day or whatever to find glaring issues | 09:48 |
| voidspace | router firmware update | 09:50 |
| voidspace | going offline briefly | 09:50 |
| fwereade | dimitern, so, it is looking pretty sane to me, but I'm worried I'm not the best person to review it -- jam, do you feel ~at home with the ec2 testing double? I've never really worked with it at all | 09:53 |
| fwereade | dimitern, all I really have is a whine that one of the methods is awfully big | 09:56 |
| dimitern | fwereade, it is indeed, and that's after splitting it up a bit | 09:57 |
| dimitern | fwereade, I welcome suggestions how to simplify it further though | 09:57 |
| fwereade | dimitern, I was thinking that at least the if len(ifaces) == 0 block could move out | 09:58 |
| fwereade | dimitern, it was less clear whether it'd be a win to extract that massive switch | 09:58 |
| dimitern | fwereade, ah, good point | 09:58 |
| dimitern | fwereade, it's really not that easy to parse that huge complicated form | 09:59 |
| fwereade | dimitern, I suspect that if you were to do that it'd be a matter of a new type that sort of accumulates state in its fields | 09:59 |
| dimitern | fwereade, hence the switch | 09:59 |
| dimitern | fwereade, expand a bit please? | 09:59 |
| fwereade | dimitern, some of those switch branches are pretty big themselves, so it could be good to extract them; but maybe it isn't even a type, it's just a func that takes a *iface and modifies it in place | 10:02 |
| fwereade | sorry forget anything I said about an extra type | 10:02 |
| fwereade | dimitern, just consider breaking the longer switch branches out so it's easier to see the structure of the whole method without getting lost | 10:03 |
| dimitern | fwereade, i kinda feel there is indeed a type waiting to emerge around parsing stuff in the test server, but it will require bigger refactoring | 10:03 |
| dimitern | fwereade, alright then, sgtm | 10:03 |
| fwereade | dimitern, I heartily support the "refactor first, so that the change you want to make becomes trivial" approach, so I'm not going to complain if you do do that, but if it's not clear what to do maybe best to skip it ;p :) | 10:04 |
| dimitern | fwereade, yeah, it will blow up the CL without much immediate gain, and I just want to get it landed so I can concentrate on juju work | 10:08 |
| fwereade | dimitern, +1 | 10:08 |
| === vladk|offline is now known as vladk | ||
| pindonga | hi, I'm having some issues trying to deploy charms on canonistack; I've filed 2 bugs in case someone can help me figure out what's wrong with this | 10:13 |
| pindonga | https://bugs.launchpad.net/juju-core/+bug/1321686 | 10:13 |
| _mup_ | Bug #1321686: initial deploy using local provider fails because template creation takes too long <juju-core:New> <https://launchpad.net/bugs/1321686> | 10:13 |
| pindonga | https://bugs.launchpad.net/juju-core/+bug/1321683 | 10:13 |
| _mup_ | Bug #1321683: juju takes a long time to deploy using the local provider <juju-core:New> <https://launchpad.net/bugs/1321683> | 10:13 |
| tasdomas | is `juju ensure-availability` supposed to work with the local provider? | 10:25 |
| jam | tasdomas: it is currently blocked from working there | 10:27 |
| jam | well, it doesn't work there | 10:27 |
| jam | enabling it properly is a big pile of work | 10:27 |
| voidspace | natefinch: https://codereview.appspot.com/94510043/ | 10:27 |
| jam | doing an approximation of it is just a matter of disabling some checks | 10:27 |
| tasdomas | jam, just to check that I understand this correctly - enabling it would mean changing the location of the state server in local provider (as I understand it, localhost is the state server in local provider) | 10:29 |
| jam | tasdomas: enabling it would mean that we would add a state server also in a container. To enable it properly that state server would then need something to talk to running on your host that could start more containers | 10:30 |
| jam | because from within a container, you cannot spawn other containers | 10:30 |
| tasdomas | jam, thanks | 10:31 |
| jam | fwereade: want to join our standup? | 10:48 |
| fwereade | jam, why not :) | 10:49 |
| jam | https://plus.google.com/hangouts/_/canonical.com/juju-sapphire | 10:49 |
| voidspace | natefinch: actually there was a review from fwereade that I missed - with a few changes needed on that mp | 11:01 |
| voidspace | natefinch: so switching to completing that | 11:01 |
| natefinch | ok cool | 11:02 |
| voidspace | fwereade: ping | 11:16 |
| fwereade | voidspace, pong | 11:16 |
| voidspace | fwereade: about your review comments on the rsyslog branch | 11:16 |
| voidspace | fwereade: specifically, dropping the Port field on RsyslogConfig | 11:17 |
| voidspace | fwereade: I did start down that road | 11:17 |
| fwereade | voidspace, oh yes? | 11:17 |
| voidspace | fwereade: it means having RsyslogWorker creation plus template rendering use HostPorts instead of the global Port | 11:17 |
| fwereade | voidspace, yeah, that was the plan in my mind | 11:18 |
| voidspace | unfortunately at bootstrap time we have a global port and not a set of HostPorts available | 11:18 |
| voidspace | so it means threading this api change through several layers of architecture in two places (initial creation plus in the worker handle) | 11:18 |
| voidspace | that was quite an invasive change (not *too bad*) | 11:18 |
| voidspace | https://code.launchpad.net/~mfoord/juju-core/ha-rsyslog-api/+merge/220087 | 11:18 |
| voidspace | I made the code changes, but that broke a metric crapload of tests *too* that would also need fixing | 11:19 |
| voidspace | (and I'm a little uncertain about what I've done with worker creation - but I can talk that through with someone) | 11:19 |
| voidspace | fwereade: so it seemed like quite a lot of extra work for something we *might* use in the future (switch away from a global port) | 11:20 |
| voidspace | fwereade: so in discussion with natefinch it seemed like defering that work until we actually needed this capability was more sensible | 11:20 |
| voidspace | fwereade: would you rather I spent the time on it now? | 11:20 |
| fwereade | voidspace, heh, I see -- tbh those changes do look like a win to me (although there's still the odd Port sitting around?), but let me think a sec | 11:20 |
| fwereade | voidspace, the main thing is that if we have HostPorts in the API but don't use it, there doesn't seem much point to that | 11:21 |
| voidspace | it's an initial step... | 11:21 |
| voidspace | it means when we do switch the GetRsyslogConfig already supports it | 11:22 |
| voidspace | the *API* already supports it | 11:22 |
| fwereade | voidspace, right, but unless that step is imminently followed by further steps in the same direction its value is somewhat limited | 11:22 |
| voidspace | well, it means when we do make the switch it doesn't require a backwards incompatible API change | 11:22 |
| fwereade | voidspace, versioning is coming along apace, thanks be to jam | 11:22 |
| voidspace | switching template rendering etc to use it - but still *actually* only storing and using a global port would be just as pointless | 11:23 |
| voidspace | but more work | 11:23 |
| voidspace | even with these changes (plus maybe pulling the Port out of syslogConfig altogether) wouldn't change the fact that we're only *really* using a global Port | 11:23 |
| voidspace | it would just mask it further | 11:24 |
| fwereade | voidspace, agree -- but each, in the absence of the other, is just a speculative change, and they rarely work out well | 11:24 |
| voidspace | so yes, fewer changes to make in the future - but no actual change in behaviour | 11:24 |
| fwereade | voidspace, getting the mask in place between the api server and the rest of the world is imo a good sort of masking | 11:24 |
| voidspace | fwereade: but even with this extra work, it's *still* a speculative change | 11:24 |
| voidspace | fwereade: right, yep | 11:25 |
| voidspace | that is true, it leaves the only place that thinks in terms of a global port being the internal database | 11:25 |
| fwereade | voidspace, it eliminates the assumption of a single port among the api clients | 11:25 |
| voidspace | fwereade: it sounds like you'd rather I did it | 11:25 |
| fwereade | voidspace, what happens under the covers is exposed to far fewer people ;) | 11:25 |
| fwereade | voidspace, I would, yeah | 11:26 |
| voidspace | ok | 11:26 |
| voidspace | np | 11:26 |
| fwereade | voidspace, tyvm | 11:26 |
| natefinch | sorry to throw a wrench in things, guys. It just sounded like more work was being added to support a feature that may or may not ever be implemented, and I wanted Michael to be able to move on to other projects. | 11:27 |
| voidspace | fwereade: natefinch: shall I do this as a separate CL - it's going to be quite a big diff against the existing branch | 11:35 |
| voidspace | so I can address the *other* comments from fwereade in the existing CL and then work on the new one | 11:35 |
| natefinch | voidspace: that sounds like a good idea | 11:36 |
| voidspace | fwereade: ah, there's a difficulty with eliminating use of Port altogether | 11:39 |
| voidspace | fwereade: when we start a new state server we need to pick a port to use | 11:39 |
| voidspace | fwereade: and the "State" needs to *know* which port we picked, because it needs to associate the state server with that port to return in RsylogConfig.HostPorts | 11:39 |
| fwereade | voidspace, does that happen at the same time as we generate the cert? | 11:40 |
| voidspace | fwereade: to be honest I'm not sure how we pick the initial port - let me check | 11:40 |
| voidspace | fwereade: hmmm... jujud/agent doesn't pass a port and the worker is started with a port of 0 | 11:43 |
| fwereade | voidspace, so, wait, we have an env config setting that's just ignored? | 11:43 |
| voidspace | ensureCertificates doesn't set a port | 11:44 |
| * fwereade wonders why it's env config in the first place then :/ | 11:44 | |
| voidspace | fwereade: we must be setting it somewhere - because HA does set the correct port | 11:45 |
| voidspace | I just haven't found it yet | 11:45 |
| voidspace | fwereade: hah | 11:46 |
| voidspace | fwereade: we have DefaultSyslogPort in environs/config/config.go | 11:46 |
| fwereade | voidspace, that syllable rarely preceds good news | 11:46 |
| voidspace | fwereade: what happens is that we use port 0 | 11:47 |
| voidspace | fwereade: when we set the cert it causes a rewrite of the config which requests the port from EnvironConfig | 11:47 |
| voidspace | fwereade: which returns the DefaultPort, which is what we use | 11:47 |
| voidspace | and port 6541 is what I always see used (the default) | 11:47 |
| voidspace | we have an api to change the port though | 11:47 |
| fwereade | voidspace, ah, ok, but someone might have set it to something different, and in that case it would be used? | 11:48 |
| voidspace | let me see where that's called from | 11:48 |
| voidspace | fwereade: yes, if it's changed we would (previously anyway!) see the change and rewrite | 11:48 |
| voidspace | fwereade: axw is certain that the port is in the config when the worker is created - so we should be safe (and indeed we will make the port immutable once set) | 11:48 |
| * fwereade is somewhat reassured | 11:49 | |
| voidspace | heh | 11:49 |
| voidspace | hmmm... no, we only have an api to set the cert | 11:50 |
| voidspace | setting the port has to be done through environconfig I believe | 11:50 |
| voidspace | fwereade: sigh, so I don't think it's set deliberately in our code - we always use the default | 11:52 |
| voidspace | fwereade: so the only way it would be set would be by users doing it - and we wouldn't pick up that change with our new code | 11:52 |
| voidspace | fwereade: so I need to talk to axw again about the motivation for allowing it to be changed | 11:52 |
| voidspace | fwereade: if there's a real use case then we need to still support it | 11:52 |
| voidspace | he said it was because previously it could pick a privileged port and rsyslog refuse to start | 11:53 |
| voidspace | but with our default of 6541 I can't see that being the case now | 11:53 |
| voidspace | it maybe that the initial starting of the rsyslog worker was picking a privileged port when we give it port 0 | 11:54 |
| voidspace | but as far as I can see we *always* overwrite that immediately - we don't set back the port the system gives us | 11:55 |
| wwitzel3 | hello | 11:56 |
| voidspace | wwitzel3: morning | 11:58 |
| voidspace | fwereade: what that means though is that we *don't* have a way to pick a port for rsyslog - we use the default or one the user has set | 11:59 |
| voidspace | fwereade: so a new state server coming online either has to have a new way of picking a port, and that needs to be stored associated with the server | 11:59 |
| voidspace | fwereade: or we still have to use a global one | 11:59 |
| voidspace | fwereade: meaning I can't eliminate it from the api client | 12:00 |
| voidspace | fwereade: we could call it "DefaultPort", and it would only be used by state servers not by units | 12:00 |
| voidspace | and setting the environ config would only change the default port for new state servers | 12:00 |
| fwereade | voidspace, that doesn't seem quite right either | 12:00 |
| fwereade | voidspace, although... there's something funny about having the same RsyslogConfig for servers and clients, isn't there? I'm not sure we should be getting the info from the same call | 12:02 |
| voidspace | fwereade: the part of the state server config that specifies the port to listen on is a separate part to the bit where we specify forwarding rules | 12:03 |
| voidspace | fwereade: so conceptually there is a difference between "the HostPorts to forward to" and "the port to listen on" | 12:03 |
| voidspace | and if we eliminate Port now, it's harder to communicate a *port change* which is a compatibility issue | 12:03 |
| fwereade | voidspace, ok, so, if we keep Port and document it as "the port this server should listen on"; and also have HostPorts meaning "where you should send your logs", we're essentially fine modulo the semantic drift, which probably doesn't hurt us much? | 12:04 |
| voidspace | fwereade: I concur | 12:05 |
| fwereade | voidspace, and again the fact that Port is always the same is something we *can* vary per-server if we want to , without changing anythying outside the API | 12:05 |
| fwereade | voidspace, ok, cool | 12:05 |
| fwereade | voidspace, keep Port but make it clear what it's for | 12:05 |
| voidspace | we can't really eliminate Port from the API client until we start properly storing HostPorts in the db, and have state servers pick a port and store that information | 12:05 |
| voidspace | yep, we can still do that without changing the api | 12:06 |
| fwereade | voidspace, ok, sgtm, thanks | 12:06 |
| voidspace | wwitzel3: I'm addressing William's comments on our CL | 12:08 |
| wwitzel3 | voidspace: got my coffee, reading the review and chat here | 12:13 |
| voidspace | I'm going on lunch | 12:24 |
| voidspace | wwitzel3: I'll sync up with you when I'm back | 12:24 |
| wwitzel3 | voidspace: sounds good | 12:25 |
| axw | voidspace: the use case was purely for supporting upgrading existing environments to TLS without triggering the privileged port/drop-to-root race in rsyslog | 12:25 |
| axw | voidspace: we don't need to do this anymore, as of 1.19.x | 12:26 |
| axw | voidspace: I think let's just make it immutable again | 12:26 |
| axw | (it used to be) | 12:26 |
| axw | voidspace: also, the default used to be a privileged port | 12:27 |
| voidspace | dammit, dammit | 12:42 |
| voidspace | I mean, what axw said is *great* | 12:42 |
| voidspace | but I forgot I have a dentist appointment today | 12:42 |
| voidspace | so I'm back off lunch, but will be going out shortly and not back until later | 12:43 |
| wwitzel3 | gotta keep those teeth healthy | 12:43 |
| voidspace | heh, yeah | 12:43 |
| perrito666 | voidspace: uh, you reminded me | 12:43 |
| voidspace | perrito666: sorry :-) | 12:43 |
| voidspace | I completely forgot about it until now | 12:43 |
| * perrito666 changes his dentist appointment by one week bc he accidentally let himself without health care for a month | 12:44 | |
| hazmat | anyone know the reason add-unit does a tools lookup instead of using the env existing tools? | 12:52 |
| natefinch | I'm sure someone knows | 12:53 |
| natefinch | but not me | 12:53 |
| * hazmat files a bug | 12:55 | |
| voidspace | fwereade: if we no longer support setting rsyslog port at all, should I remove upgrades/rsyslogport.go? | 12:55 |
| voidspace | fwereade: it would only ever set the port to the DefaultSyslogPort anyway | 12:55 |
| voidspace | fwereade: the use case was for an older version of juju anyway | 12:56 |
| fwereade | voidspace, I would prefer not to drop anything from upgrades until I can no longer think of any way anyone could possibly be running an old enough version to qualify | 13:00 |
| voidspace | fwereade: well, it's just that if the port is immutable then that code can't succeed | 13:01 |
| voidspace | fwereade: unless we make it "immutable unless switching to the default" | 13:01 |
| voidspace | fwereade: but we'd like to be able to guarantee that the port can't change | 13:01 |
| voidspace | fwereade: so the watcher can ignore environ config altogether | 13:01 |
| fwereade | voidspace, or immutable unless switching from unset (and being sure that it's always set on bootstrap ofor newer envs) | 13:02 |
| fwereade | voidspace, wait, possibly that made no sense | 13:02 |
| voidspace | fwereade: we *always* use default, except on an existing system where it may have been changed | 13:02 |
| voidspace | fwereade: upgraded from 1.16 to 1.18 will switch to default | 13:02 |
| voidspace | fwereade: and we don't support 1.16 to 1.20 directly I don't believe | 13:03 |
| fwereade | voidspace, but it's in env config, won't we use whatever was already there? | 13:03 |
| voidspace | so 1.20 doesn't have to consider a *change* | 13:03 |
| voidspace | we will use whatever is already there | 13:03 |
| voidspace | the question is - do we have to watch for changes | 13:03 |
| voidspace | the use case for permitting change was for the upgrade | 13:04 |
| voidspace | there's no such value as "unset" really I believe - just Default or non-default. If we allow changing away from default *at all*, then we need extra complexity in the watcher to handle the port changing | 13:05 |
| voidspace | and post 1.18 there's no actual use case for that | 13:05 |
| voidspace | is the "upgrades/*" code used by the *client*? If a 1.20 client might run against an earlier server then maybe we still need that code. | 13:07 |
| voidspace | But that code should fail against a 1.20 server. | 13:07 |
| fwereade | voidspace, the upgrades code should not be used by the client, no, I think we're ok there | 13:09 |
| fwereade | voidspace, (sorry, workman interruption, reloading state) | 13:10 |
| fwereade | voidspace, IIRC you don't have to watch for changes anyway | 13:10 |
| fwereade | voidspace, the watcher will always fire once when the agent bounces, and it'll always bounce on upgrade | 13:10 |
| voidspace | fwereade: right, but it's not the upgrade that's the problem - it's if we don't make it totally immutable it could change at *other* times | 13:11 |
| voidspace | and *then* we'd have to watch for changes | 13:11 |
| voidspace | unless I can make it "immutable except during upgrade" | 13:12 |
| voidspace | there's no "unset" value I don't think | 13:12 |
| fwereade | voidspace, well, before it existed it would simply not have been there, right? | 13:13 |
| fwereade | voidspace, ah, ok, but it's inserted automatically when we read a config without it | 13:14 |
| fwereade | voidspace, hmm, in which case, yeah, I don't see the point of that upgrade step at all | 13:15 |
| fwereade | voidspace, I think you can drop it, but get axw to review it as well | 13:16 |
| fwereade | voidspace, on a separate note | 13:16 |
| fwereade | voidspace, jobs in agent config vs jobs from the API | 13:16 |
| voidspace | ok | 13:16 |
| fwereade | voidspace, which we happen to use at any given time seems pretty random | 13:16 |
| fwereade | voidspace, is there a pattern I'm not seeing? | 13:16 |
| voidspace | fwereade: what do you mean by "jobs"? :-) | 13:17 |
| fwereade | voidspace, entity.Jobs() vs agentConfig.Jobs() | 13:18 |
| natefinch | fwereade: if there's two sources of truth, a programmer will use whichever one he stumbles on first | 13:18 |
| fwereade | voidspace, quite so | 13:18 |
| voidspace | fwereade: I don't see either used in our CL | 13:18 |
| fwereade | voidspace, I had a vague recollection that you were doing something around that area in your first sprint, but maybe you were just in the conversation | 13:19 |
| voidspace | fwereade: ah, not something I recall I'm afraid | 13:19 |
| fwereade | voidspace, no worries | 13:19 |
| fwereade | dimitern, does the Jobs() thing above ring a bell for you? | 13:19 |
| * dimitern reads scrollback | 13:20 | |
| natefinch | fwereade: I've faced that dilemma - where to get it. is there a right place and a wrong place? If we have two places, can we consolidate them, or at least hide one so we don't confuse ourselves? | 13:20 |
| natefinch | fwereade: I was talking about this with perrito666 because he was doing work in APIWorker to only connect to localhost for the API if we're on a state server, but it seemed like we needed to connect to the API first to determine our jobs. Maybe we missed where the jobs were held in the agent config | 13:21 |
| fwereade | natefinch, well, the jobs from the state server should be authoritative | 13:21 |
| dimitern | fwereade, it's not random at all - Jobs from agent config is used only before we have an api connection, then we use API Jobs | 13:21 |
| dimitern | voidspace, ^^ | 13:22 |
| fwereade | dimitern, but we don't update config jobs based on api results, right? | 13:22 |
| fwereade | dimitern, mainly because jobs don;t change -- yet -- I guess | 13:22 |
| dimitern | fwereade, no, they're not updated | 13:22 |
| * perrito666 feels summoned | 13:22 | |
| fwereade | dimitern, do you remember why we need jobs before we have an api connection? | 13:23 |
| fwereade | natefinch, fwiw there's a Jobs method on agent config | 13:23 |
| natefinch | fwereade: well, in the example I was talking about, we were using jobs to determine which API to connect to | 13:23 |
| dimitern | fwereade, to know what workers to start in the agent? | 13:23 |
| fwereade | dimitern, what workers need to be started before we're connected, that can't be determined trivially by looking for whether we have api-connect info and/or state-connect info? | 13:24 |
| fwereade | dimitern, natefinch: ie, always connect to the api; and if we can connect to state, do so | 13:24 |
| voidspace | dimitern: thanks | 13:27 |
| dimitern | fwereade, i'm not saying it's not shit, it is :) | 13:27 |
| voidspace | ok, folks - off to the dentist | 13:27 |
| voidspace | it's in the next town over, so I'll be a while | 13:27 |
| dimitern | fwereade, we should not use agent config Jobs or other "cached" stuff for that matter before we connect to the api | 13:28 |
| wwitzel3 | voidspace: gl :) | 13:28 |
| fwereade | dimitern, I'm not quibbling there, I'm really just trying to determine the diet that led to the substance in question | 13:29 |
| perrito666 | voidspace: beautiful, across town travel with half face asleep | 13:29 |
| fwereade | voidspace, take care | 13:29 |
| TheMue | jam: you discussed about the charm implications of the network model. that’s not in the agenda table. add? | 13:30 |
| dimitern | fwereade, for agent config Jobs() existence specifically, I'm to blame, but I just moved them from the Values map to a config field | 13:35 |
| fwereade | dimitern, no worries, I'm happier having them there than in Values | 13:36 |
| dimitern | fwereade, you can probably find the reason following the changes around adding the jobs to the map in the first place | 13:36 |
| fwereade | dimitern, just trying to unpick jujud a little bit, it's triped all my needs-fixing flags :) | 13:36 |
| dimitern | fwereade, but i really am not the best person to ask why do we have them | 13:36 |
| fwereade | dimitern, not to worry, thanks for the nuggets you supplied :) | 13:37 |
| dimitern | fwereade, thumper / rog know more most likely | 13:37 |
| dimitern | fwereade, ;) | 13:37 |
| fwereade | dimitern, hmm, I did not actualy mean nuggets of shit there, sorry | 13:37 |
| * dimitern hears for that type of nugget for the first time :) | 13:37 | |
| natefinch | lol when you have kids, you get to know lots of types of nuggets | 13:40 |
| natefinch | and shit | 13:40 |
| jcw4 | fwereade: regarding the CL https://codereview.appspot.com/98260043/ ... you'd mentioned cloning the Unit. I thought that would be at least the same db impact as what I'm doing by checking the u.doc.Life in mongo directly? | 13:41 |
| fwereade | jcw4, the trick is only hitting the db after you've failed a transaction, and doing that is often a bit tedious to arrange cleanly | 13:45 |
| fwereade | jcw4, "if i != 0" is about as nice as it gets iirc | 13:45 |
| fwereade | jcw4, and that still has a little ickiness about it | 13:46 |
| jcw4 | fwereade: I see. Other than that and punting on the contention simulation I think everything else is ready to submit... | 13:46 |
| jcw4 | fwereade: I was kinda waiting for your cleanup refactor to land to introduce actions cleanup | 13:47 |
| fwereade | jcw4, excellent, I'll take a quick look and approve in a bit | 13:47 |
| jcw4 | fwereade: tx | 13:47 |
| fwereade | jcw4, good point | 13:48 |
| fwereade | natefinch, dimitern: either of you free to review https://codereview.appspot.com/94540043/ ? | 13:48 |
| jcw4 | fwereade: mgz may be interested too? | 13:49 |
| fwereade | jcw4, good point, yeah; mgz ^^ | 13:49 |
| dimitern | fwereade, i can look a bit later if not too late | 13:50 |
| natefinch | fwereade: sure | 13:50 |
| jcw4 | forgot to lbox propose it again... | 13:50 |
| jcw4 | okay done | 13:53 |
| fwereade | oh WTF | 13:55 |
| dimitern | fwereade, vladk, https://codereview.appspot.com/98430044/ updated - final look while i'm waiting for gustavo? | 13:56 |
| fwereade | we don't even use the jobs in agent config, except in the StateWorker, which we only start if we have StateServingInfo available in the first place | 13:56 |
| fwereade | and while it might indeed be nice to separate jobs that connect to state from jobs that cause state to exist, we don't have a distinction there at all currently | 13:57 |
| fwereade | next does-anyone-know question: (1) we still have minunitsworker running against state, does anyone have any idea why? (2) we have all this "singular" crap in stateworker, does anyone know why? | 13:59 |
| natefinch | fwereade: the singular stuff is to ensure we only have one of them running in the environment, in case of HA where we have multiple state servers | 14:00 |
| fwereade | natefinch, right, but those are all tasks that are meant to work with multiples | 14:00 |
| wwitzel3 | natefinch: standup? | 14:02 |
| natefinch | wwitzel3: thanks for reminding me | 14:02 |
| wwitzel3 | np :) | 14:02 |
| fwereade | natefinch, it's quite nice to have minunitsworker singular I guess | 14:02 |
| fwereade | natefinch, but resumer should certainly be running everywhere | 14:02 |
| fwereade | natefinch, and cleaner should also be fine, I think | 14:03 |
| fwereade | (grumble grumble, why is resumer only started after upgrade?) | 14:04 |
| natefinch | fwereade: voidspace did that work with roger I think. | 14:08 |
| fwereade | natefinch, cool, I'll see what he remembers when he gets back | 14:09 |
| dimitern | niemeyer, hey hey | 14:23 |
| dimitern | niemeyer, i've updated again https://codereview.appspot.com/98430044/ - included more tests and simplified the code a bit, can I get an LGTM? :) | 14:23 |
| niemeyer | dimitern: Will have a look | 14:24 |
| dimitern | niemeyer, thanks! | 14:25 |
| niemeyer | dimitern: Have you found anyone with good knowledge on those areas of EC2 to have a look over it? | 14:25 |
| dimitern | niemeyer, it seems only vladk has more experience with ec2/vpc/nic stuff, I asked for a review | 14:26 |
| dimitern | niemeyer, but in any case, i have a parallel juju branch that depends on those changes and it does seem to work for what we need | 14:27 |
| niemeyer | dimitern: Can I nitpick a bit about the comment? It'll probably be useful in future code.. | 14:27 |
| niemeyer | 404 // If default-vpc were provided, create them and their | 14:27 |
| niemeyer | 405 // subnet. | 14:27 |
| niemeyer | This is right after | 14:28 |
| dimitern | niemeyer, yes? | 14:28 |
| niemeyer | 403 if attrName == "default-vpc" { | 14:28 |
| niemeyer | dimitern: At this point, it's not "if".. the attribute *was* provided | 14:28 |
| niemeyer | dimitern: and "them" is context-less there.. you are not creating attributes, I assume | 14:28 |
| dimitern | niemeyer, "them" refers to the default vpc | 14:29 |
| niemeyer | dimitern: That's not what the first part of the sentence refers to | 14:29 |
| niemeyer | dimitern: It's talking about the defalut-vpc attribute | 14:29 |
| natefinch | fwereade: looking at the review you mentioned about setting departed on dying units' relations... what does it mean for a relation to be in scope? | 14:29 |
| niemeyer | dimitern: I'd suggest something along the lines of | 14:29 |
| dimitern | niemeyer, well, it's both the attribute name and what it signifies | 14:29 |
| niemeyer | dimitern: // The default-vpc attribute was provided, so create the respective VPCs and their subnets. | 14:29 |
| niemeyer | dimitern: No, it's not | 14:30 |
| dimitern | niemeyer, sgtm, will change it | 14:30 |
| niemeyer | dimitern: It's the attribute that was provided | 14:30 |
| niemeyer | dimitern: You cannot provide the VPC object | 14:30 |
| niemeyer | dimitern: Either way, that's FWIW really.. | 14:31 |
| dimitern | niemeyer, that's correct, you just provide it's id in the values slice of the attribute named "default-vpc" | 14:31 |
| dimitern | niemeyer, I see how it's confusing and will rephrase it as you suggest | 14:31 |
| perrito666 | ´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´ | 14:32 |
| perrito666 | ´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´´' | 14:32 |
| perrito666 | ouch sorry dropped my external kb over the laptop | 14:32 |
| niemeyer | dimitern: Thanks.. I'd not bikeshed for this instance alone.. the comment might even go away entirely since the code is clear enough, but keeping that kind of thing in mind has a chance of improving other comments in future code | 14:32 |
| fwereade | natefinch, I haven't written that bit up yet: quick hangout? | 14:33 |
| natefinch | fwereade: sure, cool | 14:33 |
| === Ursinha is now known as Ursinha-afk | ||
| niemeyer | perrito666: I thought it was a big quote for an upcoming text file! | 14:33 |
| fwereade | natefinch, popping into moonstone | 14:33 |
| niemeyer | perrito666: Glad it wasn't ;) | 14:33 |
| niemeyer | dimitern: The trick of having the comment inside the if block is a good one I learned not too long ago | 14:34 |
| niemeyer | dimitern: It enables phrasing such as "We have a foo, do bar." | 14:34 |
| dimitern | niemeyer, +1 good point | 14:35 |
| niemeyer | dimitern: Instead of "If the world is in the proper conditions then we'll have a foo, and be able to do bar." | 14:35 |
| wwitzel3 | lol .. when I read fwereade cooment to natefinch I added mentally replaced that second p with an o .. now I can't stop laughing. | 14:35 |
| * wwitzel3 is mature. | 14:35 | |
| * dimitern is afk for some time | 14:36 | |
| fwereade | haha | 14:41 |
| natefinch | wwitzel3: lol | 14:41 |
| === Ursinha-afk is now known as Ursinha | ||
| TheMue | *rofl* | 14:52 |
| niemeyer | dimitern: I still don't get the logic around this: | 14:55 |
| niemeyer | if len(ifaceToCreate.PrivateIPs) > 0 { | 14:55 |
| niemeyer | primaryIP = ifaceToCreate.PrivateIPs[0].Address | 14:55 |
| niemeyer | The new comment you added says | 14:56 |
| niemeyer | 732 // To simulate that, we need to set PrimaryIPAddress of the | 14:56 |
| niemeyer | 733 // NIC and empty the PrivateIPs when there is only one address | 14:56 |
| niemeyer | 734 // specified there. | 14:56 |
| niemeyer | But nowhere does it check that there is a single entry in that list | 14:56 |
| niemeyer | It just picks up the first one blindly and considers it as the primary ip | 14:56 |
| niemeyer | At the other end of it, we see | 14:57 |
| niemeyer | 614 iface.PrivateIPs = append(iface.PrivateIPs, ec2.PrivateIP{ | 14:57 |
| niemeyer | dimitern: I must be missing something | 14:58 |
| dimitern | niemeyer, just a sec | 14:59 |
| dimitern | niemeyer, it my tests AWS behaves like this: if you specify explicit NICs and private IPs in RunInstances, when you get the instance details, the NetworkInterfaces[x].PrivateIPs is populated as you specified | 15:00 |
| dimitern | niemeyer, OTOH if you don't specify NICs, AWS creates one for you with a single private ip assigned, which is the primary, and sets NetworksInterface.PrivateIPAddress and PrivateDNSName, but leaves the PrivateIPs slice empty, i.e. the single ip is not duplicated in the slice and the field | 15:01 |
| dimitern | niemeyer, i forgot to mention that in the first case, PrivateIPAddress is not set | 15:02 |
| niemeyer | dimitern: and what prevents that slice from having 10 entries? | 15:03 |
| niemeyer | dimitern: and why would the first one be primary in that case? | 15:03 |
| dimitern | niemeyer, the first one is the primary, when there's anything in the slice at all - that's my observation | 15:04 |
| dimitern | niemeyer, the slice can have many entries | 15:05 |
| niemeyer | dimitern: Sorry, but I don't get the logic | 15:05 |
| niemeyer | dimitern: There's a: case "PrivateIpAddresses": | 15:05 |
| dimitern | niemeyer, i don't get it either - it's just what I've seen AWS respond with | 15:05 |
| niemeyer | dimitern: In your code | 15:05 |
| niemeyer | dimitern: That adds entries into the iface.PrivateIPs | 15:06 |
| dimitern | niemeyer, yes, that's where PrivateIPs gets deserialized from | 15:06 |
| niemeyer | dimitern: slice | 15:06 |
| niemeyer | dimitern: and has a privateIP.IsPrimary = val | 15:06 |
| niemeyer | dimitern: Assignment | 15:06 |
| niemeyer | dimitern: Based on a "Primary" field | 15:06 |
| dimitern | niemeyer, yes | 15:06 |
| niemeyer | dimitern: There's zero consideration of ordering in that logic | 15:06 |
| niemeyer | dimitern: Why on earth a few instructions afterwards the first one would magically be the primary? | 15:07 |
| dimitern | niemeyer, sorry, I don't understand that question | 15:07 |
| niemeyer | dimitern: It | 15:07 |
| niemeyer | dimitern: It's the only one I've been asking so far :) | 15:07 |
| niemeyer | dimitern: On line 607 there's logic appending to PrivateIPs and handling a Primary boolean | 15:08 |
| dimitern | niemeyer, AWS, when there is more than one private ip, returns a populated PrivateIPs, and usually the first one in there is the primary | 15:09 |
| niemeyer | dimitern: On line 736 there's logic that considers the first IP on PrivateIPs to be the primary, whatever happened on line 607 | 15:09 |
| niemeyer | dimitern: You've said that many times, but it's completely irrelevant | 15:09 |
| dimitern | niemeyer, these are two different things | 15:10 |
| niemeyer | dimitern: Okay, phew.. can you explain how? | 15:10 |
| dimitern | niemeyer, in parseRunNetworkInterfaces, I'm taking AWS request and I'm parsing is as best as I can, because that's what the ec2 provider in juju will be using | 15:10 |
| niemeyer | dimitern: Okay? | 15:11 |
| dimitern | niemeyer, in createNICsOnRun I'm using that to create fake NICs for the instance, so when I get the instance info in juju it will have a NIC | 15:11 |
| niemeyer | dimitern: Well, yes, you've described what both of these functions do | 15:12 |
| niemeyer | dimitern: per their documentation | 15:12 |
| niemeyer | dimitern: Now, what gives them a free pass to create completely bogus data? | 15:12 |
| niemeyer | dimitern: Based on blind assumptions? | 15:12 |
| dimitern | niemeyer, why bogus? | 15:12 |
| niemeyer | dimitern: because parse takes a list of interfaces which explicitly define what is primary.. and create ignores that information entirely and picks the first entry out of the slice | 15:13 |
| niemeyer | dimitern: Again, this is the same point I've been making over and over | 15:13 |
| niemeyer | dimitern: and so far there's no good answer for why that'd be okay | 15:13 |
| dimitern | niemeyer, well, because i'm trying to accommodate an arbitrary user-sent RunInstances + NICs request as AWS would do (all quirks included) | 15:14 |
| niemeyer | dimitern: What quirks? Do you mean Amazon ignores a Primary: true definition for an interface? | 15:15 |
| niemeyer | dimitern: THat'd be *very* surprising | 15:15 |
| dimitern | niemeyer, and in case the user didn't give any NICs, create one as if the user did, and in that case i control what's in PrivateIPs | 15:15 |
| dimitern | niemeyer, that part of AWS API is unnecessary complicated i think - why have both PrivateIpAddress as a separate field, and a list of PrivateIpAddresses, which can be primary or secondary | 15:16 |
| dimitern | niemeyer, you can even combine both in the same request up to point | 15:16 |
| niemeyer | dimitern: Okay, we've been talking about this for half an hour, and I'm very unconvinced | 15:16 |
| niemeyer | dimitern: I'm copy & pasting that conversation somewhere we can refer to, and will mark the issue as not lgtm | 15:17 |
| dimitern | niemeyer, ok, so to summarize - you suggest to change the code in createNICs to either use the stand-alone PrivateIPAddress as primary, or scan PrivateIPs to find it | 15:17 |
| niemeyer | dimitern: Yes, I suggest doing something minimally sensible and internally consistent.. | 15:18 |
| dimitern | niemeyer, will that be ok? | 15:18 |
| dimitern | niemeyer, ok, will do it then and repropose | 15:18 |
| dimitern | niemeyer, in the mean time I'd appreciate other suggestions on the rest of the CL, if you have them | 15:19 |
| niemeyer | dimitern: Sent | 15:25 |
| dimitern | niemeyer, cheers | 15:26 |
| perrito666 | wwitzel3: if you have a moment let me know if that mail I sent is accurate, so I can actually write docs with it | 15:27 |
| perrito666 | wwitzel3: I am a bit rust at mecanography, at least in english | 15:28 |
| wwitzel3 | perrito666: sure, looking now | 15:29 |
| dimitern | niemeyer, updated https://codereview.appspot.com/98430044/ again - this time the code around PrivateIPs in createNICs should be straightforward; also fixed the comment and the error message | 15:49 |
| dimitern | hazmat, ping | 15:49 |
| hazmat | dimitern, pong | 15:50 |
| dimitern | hazmat, quick question: are you comfortable enough with EC2 networking API for VPC to review this? https://codereview.appspot.com/98430044/ | 15:50 |
| hazmat | dimitern, looking. | 15:50 |
| dimitern | hazmat, the changes are mostly in the ec2test testing server, improving it to behave closer to the real AWS wrt running instances with/without specified NICs | 15:51 |
| dimitern | hazmat, thanks! | 15:51 |
| bodie_ | anyone know offhand what tz mgz is in? | 15:52 |
| hazmat | dimitern, fwiw. default vpc on an acct starts with subnets in several azs | 15:52 |
| hazmat | bodie_, gmt | 15:52 |
| bodie_ | thanks | 15:52 |
| dimitern | hazmat, yes, a subnet per AZ | 15:52 |
| hazmat | dimitern, btw. the aws unified cli is a great tool | 15:52 |
| dimitern | hazmat, the ec2-xxx one? | 15:52 |
| hazmat | can do raw request return, and has wide api coverage | 15:52 |
| hazmat | dimitern, no.. the python one. | 15:52 |
| hazmat | dimitern, https://aws.amazon.com/cli/ | 15:53 |
| dimitern | hazmat, haven't used any aws cli that much - will check it out, thanks | 15:53 |
| hazmat | dimitern, they used to random hodgepodge by each team in different languages, they rewrote based on json description of the api with python frontend, much nicer to work with, but also easy to determine exact params on apis | 15:54 |
| dimitern | hazmat, that is indeed useful, would've saved me a few hours of wireshark usage :) | 15:55 |
| hazmat | dimitern, default-vpc doesn't have a name | 15:56 |
| dimitern | hazmat, name? i don't think any one has | 15:57 |
| mgz | bodie_: gmt+1 atm | 16:01 |
| hazmat | dimitern, nevermind.. i got thrown by 'default-vpc' == Attributes[0].Name .. but that's just describe account attributes resposne. | 16:02 |
| dimitern | hazmat, ah :) | 16:03 |
| bodie_ | mgz, hua | 16:03 |
| hazmat | dimitern, re awscli fwiw.. http://pastebin.ubuntu.com/7497943/ | 16:03 |
| dimitern | hazmat, interesting - the api docs do not describe most of these | 16:06 |
| hazmat | dimitern, these commands also have builtin detailed parameter help | 16:06 |
| * dimitern is going now, might be back later | 16:16 | |
| bodie_ | how does the frontend talk to the client api? | 16:20 |
| bodie_ | is there an http service or does it go through sockets? | 16:20 |
| bodie_ | I'm familiar with the CLI client api interface | 16:21 |
| bodie_ | I suppose that info will be in doc/api.txt :) | 16:21 |
| hazmat | bodie_, via an api client that uses websockets | 16:23 |
| hazmat | bodie_, parts of the api are over https (charm upload, log download), but most goes over the websocket | 16:23 |
| bodie_ | so via cmd/*? | 16:23 |
| hazmat | bodie_, yes.. cmd/* are cli commands that use state/api client | 16:24 |
| hazmat | bodie_, did you guys know go before you started this project? | 16:24 |
| bodie_ | I just want to make sure I'm understanding you right, the web frontend also uses that command client? | 16:24 |
| bodie_ | I'd experimented with and explored it a bit but I wouldn't say I was quite intermediate even really :) | 16:24 |
| jcw4 | hazmat: some, but this is my first production code Go project | 16:26 |
| bodie_ | I'm from a C / Java background so it's been a pretty natural fit outside of some of the RPC and interface{} bits | 16:26 |
| bodie_ | why do you ask? | 16:28 |
| hazmat | bodie_, the webfront end uses the a javascript implementation of the same | 16:30 |
| hazmat | bodie_, the webfront talks via the websocket protocol using a js client implementation of the api.. there's also a python-jujuclient that exposes the same client api in python. | 16:31 |
| hazmat | bodie_, ie. api not cli implementation | 16:31 |
| hazmat | bodie_, jcw4 just curious about ramp-up time | 16:31 |
| bodie_ | I see | 16:32 |
| bodie_ | so we expose the stateservice endpoints and the js client will have methods written to map to them, I suppose | 16:33 |
| jcw4 | hazmat: for me the big ramp up time is learning juju, not so much Go | 16:34 |
| jcw4 | hazmat: although to be fair I do need to learn and internalize some of the Go idioms better | 16:35 |
| hazmat | bodie_, yes | 16:37 |
| jcw4 | <fwereade> natefinch, dimitern: either of you free to review https://codereview.appspot.com/94540043/ ? | 17:29 |
| jcw4 | mgz? :-D | 17:29 |
| mgz | jcw4: on it | 17:31 |
| jcw4 | mgz: ta | 17:32 |
| mgz | (thanks for the poke :) | 17:32 |
| jcw4 | :) | 17:32 |
| jcw4 | mgz: any luck on that? :) | 19:15 |
| mgz | jcw4: need to hit m, will do shortly | 19:16 |
| jcw4 | mgz: yay :) tx | 19:16 |
| jcw4 | mgz: looking forward to more fun comments ;) | 19:16 |
| hatch | hi dpritchett :) | 19:53 |
| dpritchett | hi there | 19:53 |
| fwereade | jcw4, bah, I thought you were looking for a review for yourself, and I jumped straight on it, then...hmm | 20:02 |
| jcw4_ | lol | 20:03 |
| jcw4 | mgz: is almost done | 20:03 |
| mgz | yup | 20:03 |
| mgz | only nits really, just got distracted earlier | 20:03 |
| jcw4 | I like nits | 20:03 |
| jcw4 | better than "what is this shyte?" | 20:03 |
| jcw4 | fwereade: any eta on your cleanup refactor landing? | 20:04 |
| jcw4 | fwereade: should I just make a dependent branch on that one? | 20:04 |
| fwereade | jcw4, isn't that the one you just ask mgz for a review of? it's rather in his hands ;p | 20:05 |
| jcw4 | I thought he was reviewing the one you just finished reviewing of mine? | 20:06 |
| jcw4 | fwereade: I'm referring to https://code.launchpad.net/~fwereade/juju-core/use-prepare-leave-scope | 20:08 |
| jcw4 | fwereade: I *think* mgz is reviewing https://code.launchpad.net/~johnweldon4/juju-core/action | 20:09 |
| fwereade | jcw4, ehh, who knows, but just above you quoted me asking for a review on mine | 20:09 |
| jcw4 | fwereade: doh... I just saw the last few digits of the number and assumed... | 20:09 |
| fwereade | jcw4, there is definitely something funny about codereview.appspot.com, "043" comes up alarmingly often | 20:10 |
| jcw4 | fwereade: I'm actually glad... I want that one in soon! 42 +1? | 20:10 |
| mgz | I can do both! | 20:11 |
| jcw4 | woo hoo | 20:11 |
| fwereade | haha | 20:11 |
| * fwereade cheers | 20:11 | |
| mgz | fwereade: yours seems to have some overlap with one I already reviewed? | 20:11 |
| mgz | or I'm getting confused? | 20:11 |
| jcw4 | :) | 20:11 |
| fwereade | mgz, yeah, it's the followup to that one | 20:11 |
| jcw4 | mgz: mine is https://codereview.appspot.com/98260043/ | 20:11 |
| mgz | k | 20:11 |
| fwereade | mgz, it uses it and renames a bunch of stuff just to keep your life interesting | 20:12 |
| === vladk is now known as vladk|offline | ||
| mgz | jcw4: will payload change type or remain map[string]interface{}? | 20:21 |
| jcw4 | afaik it will remain | 20:21 |
| jcw4 | mgz: the actual definition is configurable per action, per charm so that's the strongest typing we can achieve imo | 20:22 |
| bodie_ | mgz, that's where the json-schema comes into play, so we don't know how the charmer will define his or her params spec | 20:31 |
| bodie_ | I'm working through some potential edge cases right now in my tests | 20:31 |
| bodie_ | the map[string]interface{} represents a list of arguments, but those arguments could have complex values (is my understanding) | 20:32 |
| mgz | bodie_: yup | 20:35 |
| jcw4 | tx mgz | 20:36 |
| jcw4 | so mgz : I should add that ErrMatches and you want me to comment Name and Payload in the actionDoc for now? | 20:40 |
| mgz | yeah, that would be nice | 20:40 |
| jcw4 | Okay. No issues with ErrMatches, just cautious about Name/Payload comment, but since we're not using it now and as you suggest we may want to clarify our thinking on it I'll do that and re-submit | 20:41 |
| bodie_ | does anyone know the default behavior of json-schema if the "optional" or "required" keys are omitted? | 20:47 |
| perrito666 | hey what is the criteria for naming things such as addresser | 20:53 |
| mgz | no erers | 20:53 |
| perrito666 | and I am mostly curious about the "er" | 20:53 |
| perrito666 | mgz: @more | 20:53 |
| mgz | try not to make it too weird | 20:54 |
| perrito666 | ok | 20:54 |
| bodie_ | lol | 20:58 |
| bodie_ | uniter is pretty weird | 20:58 |
| bodie_ | at least there's no errer | 20:59 |
| jcw4_ | lol | 21:02 |
| jcw4 | mgz: done | 21:05 |
| mgz | jcw4: I think you can go ahead and land then, fwereade any reason not to? | 21:05 |
| jcw4 | mgz: pending fwereade approval, do I do something to land that branch? | 21:06 |
| mgz | jcw4: basically mark the proposal on launchpad as approved and set the commit message (to something similar to the description) | 21:07 |
| jcw4 | mgz: +1 I'll do that once fwereade approves. Tx again | 21:07 |
| bodie_ | woot, I think I have a good MR here. next stop will be the Validate function so I'm sure there will be more interesting stuff to come | 21:09 |
| jcw4 | bodie_: yay | 21:09 |
| bodie_ | https://codereview.appspot.com/94540044 | 21:09 |
| bodie_ | brb | 21:09 |
| * jcw4 leaves for a couple hours | 21:10 | |
| fwereade | jcw4, can't figure out why name/payload are commented; otherwise looks fine | 21:13 |
| fwereade | mgz, thanks for the review, good points | 21:13 |
| jcw4 | fwereade: per mgz | 21:14 |
| jcw4 | fwereade: maybe I misunderstood mgz ... | 21:15 |
| fwereade | jcw4, I think he meant "describe them with comments" rather than "comment them out" | 21:15 |
| jcw4 | ROFL | 21:15 |
| jcw4 | seriously | 21:15 |
| jcw4 | okay getting up again | 21:15 |
| jcw4 | doh | 21:15 |
| fwereade | jcw4, but to be fair there should probably also be tests that what we getout matches what we put in | 21:16 |
| fwereade | jcw4, I might actually recommend getting some sleep ;p | 21:16 |
| jcw4 | I'll uncomment and then comment and then add tests and then resubmit | 21:16 |
| jcw4 | ;) | 21:16 |
| fwereade | jcw4, cheers | 21:16 |
| fwereade | jcw4, I think I'll be in bed by then, I'm afraid | 21:17 |
| jcw4 | no worries. I'll just let it wait til your morning fwereade , thanks | 21:17 |
| * jcw4 mutters to himself... I thought commenting out was a little weird | 21:18 | |
| mgz | jcw4: doh, yeah I did, sorry, not clear enough | 21:18 |
| jcw4 | :) | 21:18 |
| * jcw4 leaves for real this time | 21:19 | |
| fwereade | bodie_, https://codereview.appspot.com/94540044/ reviewed | 21:34 |
| * fwereade goes for a ciggie, might go straight to bed afterwards, but ping me if you need me in case I don'y | 21:34 | |
| wallyworld | sinzui: hi, did you have time for a quick catch up? | 21:54 |
| sinzui | I will in 6 minutes | 21:54 |
| wallyworld | ok | 21:55 |
| sinzui | wallyworld, I am ready | 22:03 |
| wallyworld | sinzui: https://plus.google.com/hangouts/_/gtg7auay67vchcqri3to3a6g4qa | 22:03 |
| sinzui | wallyworld, it that the full url? I am told the party is over | 22:04 |
| wallyworld | sigh, let me try again | 22:04 |
| wallyworld | https://plus.google.com/hangouts/_/gwbfwjemvxm5p7u4jhegwu2aima | 22:04 |
| sinzui | wallyworld, try inviting me. Google doesn't want me you interrupt your party | 22:05 |
| wallyworld | ok, invite sent | 22:06 |
| === jcsackett_ is now known as jcsackett | ||
| === dpritchett_ is now known as dpritchett | ||
| === _mup__ is now known as _mup_ | ||
| waigani | So Actions in juju. What is that all about? what are 'Actions'? | 23:04 |
| rick_h_ | waigani: the idea of an action is a prepackged script that can be run on demand. The primary example is a mongodb "backup" action that can be triggered and will perform a mongodump and place the backup at a location that might be configurable in the action call. | 23:06 |
| rick_h_ | waigani: so actions can take some pre-defined input as if script --params | 23:06 |
| waigani | rick_h_: thanks. What would trigger the backup (to go with your analogy)? | 23:07 |
| waigani | i.e. is this pub/sub or command line calls or ... | 23:08 |
| rick_h_ | waigani: a command. So you'd juju xxxx mongodb backup --output=/tmp/xxxx.back | 23:08 |
| rick_h_ | waigani: so command line calls, but available over the api so the GUI will report and allow users to submit them from there as well | 23:09 |
| rick_h_ | waigani: in the future there might be a place for automating actions based on some external controls | 23:09 |
| waigani | rick_h_: right, cool. | 23:09 |
| wallyworld | waigani: you were looking at this problem, right? cannot share a state between two dummy environs; old "dummyenv"; new "dummyenv" | 23:51 |
| waigani | wallyworld: I fixed that one. I had to dummy.Reset() in TearDownTest | 23:52 |
| wallyworld | waigani: it's still appearing on the bot | 23:52 |
| waigani | hmph | 23:52 |
| wallyworld | might be in a different place | 23:52 |
| wallyworld | waigani: can you take aquick peak and see if it is failing in the place you fixed? http://juju-ci.vapour.ws:8080/job/walk-unit-tests-amd64-trusty/317/console | 23:53 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!