=== dfc is now known as Guest82651
jamhey wallyworld_, how's it going?05:05
wallyworld_ell thanks05:05
jamwallyworld_: any reviews, etc, I can help you out with?05:05
jam(My review email box sometimes has 400 things in it from just a couple days. Unforunately, I'm in too many groups on LP :)05:05
wallyworld_i've landed everything, am working on some stuff. i'd be happy to fill you in via mumble since i will not be at the meeting05:06
jamwallyworld_: sure05:06
rogpeppeif anyone has some space to look at this, a review would be very much appreciated: https://codereview.appspot.com/7178044/10:08
dimiternwhat's the idea with panicWrite() -> writeCertAndKey called unexpectedly in the bootstrap tests?10:32
dimiternit seems it'll fail always in the normal case10:32
dimiternrogpeppe: can you help here? ^^10:34
rogpeppedimitern: hmm, i haven't seen that. looking.10:35
dimiternrogpeppe: so there's that callback in environs.Bootstrap(Env, uploadTools, writeCertAndKey())10:35
dimiternrogpeppe: which in the tests is always panicWrite instead10:36
rogpeppedimitern: yeah. that will only happen if the environment config doesn't have a CA cert and key10:36
rogpeppedimitern: so if that's happening in the openstack tests, i'd guess you've got a config without those in10:36
dimiternrogpeppe: so for the tests what I'm supposed to do - remove the ca/key from ~/.juju ?10:36
rogpeppedimitern: no, pass the testing CA cert and key in the env config10:37
dimiternrogpeppe: missing? ahaa ok10:37
rogpeppedimitern: i *think* that's right. give me a few moments to check.10:37
rogpeppedimitern: yeah, that seems about right.10:38
rogpeppedimitern: grep for CACert in environs/ec2/*.go10:38
dimiternrogpeppe: I saw it - authorized-keys as well perhaps?10:39
rogpeppedimitern: maybe10:39
rogpeppedimitern: not for live tests10:40
rogpeppedimitern: you actually want it to use the authorized keys from your home directory for live tests, so you can ssh to the started instances10:40
dimiternrogpeppe: I see, ok - now I added these conf keys and it passed10:41
rogpeppedimitern: cool10:41
rogpeppearam: hiya10:56
dimiternaram: hey11:00
dimiternrogpeppe: so after the bootstrap is done, we're trying to open the state and connect to the bootstrap machine - how is this simulated in the local live tests?11:01
dimiternand why is mongo on my machine defaulting to 27017 and juju expects it to be at :37017 ?11:15
rogpeppedimitern: we don't simulate that in the local live tests AFAIR11:16
rogpeppedimitern: we decided that trying to simulate the cloud-init stuff was going too far11:17
dimiternrogpeppe: but I can see things like: [LOG] 18.27607 JUJU environs/openstack: started instance "8cd71439-c5f6-47bd-94e1-f7693d47f11a"11:17
dimiternopening connection11:17
dimitern[LOG] 23.11676 JUJU environs/openstack: waiting for DNS name(s) of state server instances [8cd71439-c5f6-47bd-94e1-f7693d47f11a]11:17
dimitern[LOG] 23.11794 JUJU state: opening state; mongo addresses: ["localhost:27017"]; entity ""11:17
dimitern[LOG] 23.11974 JUJU state: connecting to
rogpeppedimitern: does the connection succeed?11:18
dimiternI changed 37017 to 27017 to match my running mongo and StateInfo to return localhost11:18
dimiternI'm lost.. what has to happen at this step in BootstrapAndDeploy ?11:18
dimiternit's still a test (live, if local)11:18
rogpeppedimitern: if you look at the ec2 live tests, you'll find that LiveTests.CanOpenState is false for the local "live" tests11:19
dimiternrogpeppe: got you! right this should work then :)11:19
rogpeppedimitern: and BootstrapAndDeploy looks at CanOpenState to see if it can connect to the state11:19
rogpeppedimitern: BootstrapAndDeploy can't work unless the relevant agents are started too11:19
dimiternrogpeppe: so it's only a truly live test then?11:20
rogpeppedimitern: which we don't do in the local tests - we test them independently11:20
rogpeppedimitern: yes11:20
rogpeppedimitern: although it does test some stuff, i guess11:20
dimiternrogpeppe: I see11:20
rogpeppedimitern: (in the local case)11:21
dimiternrogpeppe: and HasProvisioner = false as well for local tests?11:21
rogpeppedimitern: yeah11:22
rogpeppedimitern: i did once suggest that we could have the local tests start the agents too, but it was considered not worth it.11:22
jamdimitern: I'm pretty sure Mongo gets set up directly by the test infrastructure. Which is why it isn't on the port you expected.11:26
dimiternjam: it seems something like that, anyway I'm skipping these now11:26
jamdimitern: cloudinit_test.go spawns it with "--port 37017"11:27
dimiternjam: yeah, I saw this11:27
dimiternjam: but was still confusing at first11:27
niemeyerGood morning11:27
dimiternniemeyer: morning11:28
dimiternjam: shall we start?11:29
jamdimitern: I'm on11:29
jammgz: poke11:29
jamwallyworld won't be making it, movie night11:29
mgzdimitern: the other part (talking to the metadata service) is in juju.unit.address.OpenStackUnitAddress11:39
dimiternfinally, openstack bootstrap - https://codereview.appspot.com/7181046/11:57
rogpeppeniemeyer: hiya11:59
niemeyerrogpeppe: Just finished a pass on https://codereview.appspot.com/7178044/12:02
rogpeppeniemeyer: thanks12:02
rogpeppeniemeyer: i'm not entirely sure of the best approach to the two-error thing12:09
rogpeppeniemeyer: if we get ErrDead and some other error, which error should we act on?12:09
rogpeppeniemeyer: and which error should we return from Run?12:10
niemeyerrogpeppe: Both are good questions.. "whatever" is probably not the right answer. :-)12:11
niemeyerrogpeppe: I'll step out to grab some coffee and ponder meanwhile12:11
rogpeppeniemeyer: runTasks returns the first error and logs the others12:11
rogpeppeniemeyer: ok12:11
niemeyerrogpeppe: Think about how both are handled meanwhile12:11
niemeyerrogpeppe: So we can talk12:11
niemeyerrogpeppe: SO, how do we handle ErrDead vs. other errors?12:34
rogpeppeniemeyer: i think we could make an importance hierarchy of errors, perhaps12:35
rogpeppeniemeyer: ErrDead > UpgradedError > * > nil12:35
niemeyerrogpeppe: How do we handle ErrDead vs. other errors today in that context?12:35
rogpeppeniemeyer: we just choose the first one, except that UpgradedError gets priority12:35
niemeyerrogpeppe: You mean we never compare an error to ErrDead?12:36
niemeyerrogpeppe: Why do we even have it?12:36
rogpeppeniemeyer: we do, in order to return nil from Run if the error is ErrDead12:36
rogpeppeniemeyer: because if the entity has died, we just exit normally12:37
niemeyerrogpeppe: Yes, and that means we actually exit from the process12:37
rogpeppeniemeyer: and in that case, we probably don't care if someone wants to upgrade us, or other errors.12:37
rogpeppeniemeyer: yup12:37
niemeyerrogpeppe: I think we do care we've seen an error, to log it if nothing else.. but indeed ErrDead should not be dropped silently12:38
rogpeppeniemeyer: i've been toying with the idea of letting the tomb package have some sort of control over error precedence, rather than just discarding later erorrs12:38
rogpeppeniemeyer: for instance, one might give it an error-precedence comparison function.12:39
niemeyerrogpeppe: Seems like unnecessary complexity12:39
rogpeppeniemeyer: you're probably right12:39
niemeyerrogpeppe: Let's just solve the problem at hand12:39
rogpeppeniemeyer: there are two places we'd want to solve it in: runTasks and the machine agent top level. both are really a similar thing.12:40
rogpeppeniemeyer: and there's also the issue of the "configuration does not have state server cert/key" fatal error.12:42
rogpeppeniemeyer: i wonder if we could define func IsFatal(err error) bool12:43
rogpeppeniemeyer: then we  wouldn't have to kill the tomb in the above place12:43
rogpeppeniemeyer: which means we don't run the risk of squashing those errors.12:44
niemeyerrogpeppe: Isn't that unrelated to the actual problem?12:44
niemeyerrogpeppe: ErrDead must not be dropped silently.. there are two possible errors, and one of them is being dropped silently. That sounds rather easy.12:44
rogpeppeniemeyer: actually there are n possible errors, one for each task (if you look at runTasks)12:45
rogpeppeniemeyer: we don't want to drop UpgradedError either12:45
niemeyerrogpeppe: I'm talking about that loop12:46
niemeyerrogpeppe: There are only two possible errors there12:46
rogpeppeniemeyer: the loop in MachineAgent.Run ?12:46
niemeyerrogpeppe: The one where the review comment was made12:46
rogpeppeniemeyer: it's possible that we can drop ErrDead silently in runTasks too12:49
rogpeppeniemeyer: but maybe we don't care that much.12:49
niemeyerrogpeppe: There's already logic there to pick a proper error12:50
niemeyerrogpeppe: If it's broken, it should be fixed12:50
niemeyerrogpeppe: If it isn't..12:50
rogpeppeniemeyer: it just chooses the first error (unless something returned an UpgradedError)12:50
rogpeppeniemeyer: tbh dropping ErrDead in favour of UpgradedError isn't a problem12:51
rogpeppeniemeyer: it'll just mean it'll restart only to die with ErrDead12:51
niemeyerrogpeppe: It's also cheap to do it right, but that's really unrelated to the problem at hand12:52
rogpeppeniemeyer: the loop in MachineAgent.Run is the moral equivalent of the loop in runTasks IMHO - both loops are multiplexing more than one task and returning the result12:52
rogpeppeniemeyer: which is why i'm thinking about both of them here12:53
niemeyerrogpeppe: Sure, whatever works12:53
rogpeppeniemeyer: i think i know a decent way to solve the problem.12:54
rogpeppeniemeyer: it's kinda funny that we'll end up ignoring the tomb.Err() value, but i think we've gone beyond its capabilities here.12:55
niemeyerrogpeppe: tomb.Err should be whatever error took precedence12:56
rogpeppeniemeyer: tomb.Err is always going to be the first error that happened to kill the tomb12:56
niemeyerrogpeppe: Sure, and that's great12:56
rogpeppeniemeyer: that's not necessarily the error we're going to use though12:57
niemeyerrogpeppe: If it was killed ahead of time12:57
niemeyerrogpeppe: All errors are used12:57
niemeyerrogpeppe: Some of them are logged, others returned12:57
rogpeppeniemeyer: sure. tomb.Err won't be the error that took precedence though.12:59
niemeyerrogpeppe: Yes, nevermind.. I think we both understand the context12:59
rogpeppeniemeyer: yeah.13:00
dimiternare we having the weekly meeting?13:01
dimiternit seems no one but me is there.. at least using the link in the calendar13:01
rogpeppedimitern: i guess so.13:01
rogpeppedimitern: i'll join now13:01
aramwhat is the url?13:02
rogpeppearam: https://plus.google.com/hangouts/_/33acfc2c8af8792568274fa110371db956f9fde713:02
rogpeppeniemeyer: meeting?13:03
niemeyerYep, let's do it13:04
rogpeppemramm: meeting?13:14
arammramm: https://plus.google.com/hangouts/_/33acfc2c8af8792568274fa110371db956f9fde713:14
rogpeppeniemeyer: PTAL https://codereview.appspot.com/7178044/14:44
* rogpeppe goes for some lunch14:44
niemeyerrogpeppe: LGTM14:50
niemeyerrogpeppe: Thanks for the changes14:50
=== slank_away is now known as slank
rogpeppeniemeyer: cool, thanks for the prompt review14:57
rogpeppeniemeyer: ping15:25
niemeyerrogpeppe: Hey15:44
rogpeppeniemeyer: i just wanted to run an idea past you15:44
niemeyerrogpeppe: Of course15:44
rogpeppeniemeyer: i find myself replacing state.Info with a pair of (state.Info, api.Info) everywhere, and i'm wondering if it would be bad to just add a state.Info member to api.Info, to be removed when we can use the API on its own.15:46
niemeyerrogpeppe: I think we covered that before15:46
rogpeppeniemeyer: that was the connection itself15:46
rogpeppeniemeyer: this is just the info, which i think is more reasonable15:46
rogpeppeniemeyer: as it's got no methods, and i wouldn't embed it15:47
niemeyerrogpeppe: I think the reasoning is the same15:47
rogpeppeniemeyer: could you remind me of the reason this is a bad idea15:48
niemeyerrogpeppe: Sure: http://pastebin.ubuntu.com/1559918/15:51
rogpeppeniemeyer: i think there's a significant difference between Info and State here.15:52
niemeyerrogpeppe: The idea is exactly the same15:52
rogpeppeniemeyer: we're going to need the state.Info around until the very last remnant goes15:52
rogpeppeniemeyer: because it's used in setting up the thing that everything talks to15:52
niemeyerrogpeppe: Disagree.. you only need it in places where the API doesn't cover15:52
rogpeppeniemeyer: you need it in the top-level places where the state is connected to - in juju.Conn and at the top level in jujud. those places are used by everything.15:53
rogpeppeniemeyer: so as long as there's something using it, you'll still need it, AFAICS15:54
niemeyerrogpeppe: The top level in jujud is "used by everything"?15:54
rogpeppeniemeyer: the top level in jujud creates a *state.Info, which is then passed to all the tasks.15:55
rogpeppecreates a *state.State, of course15:55
rogpeppeniemeyer: as long as we have one task that needs a *state.State, we'll need the *state.Info.15:55
niemeyerrogpeppe: That's totally fine?15:56
rogpeppeniemeyer: but at that point we could just remove the StateInfo field from api.Info15:56
rogpeppeniemeyer: with considerably less disruption to the APIs15:56
niemeyerrogpeppe: That will mean that every single entity in the system that has a StateInfo will have the api.Info, even if it doesn't care about it15:56
rogpeppeniemeyer: no15:56
niemeyerrogpeppe: It's exactly the same motivation described before15:56
niemeyerDec 12 09:55:31 <niemeyer>rogpeppe: It's pretty much impossible to tell if things you're passing apiState in actually are ported or not, without investigating all calls15:57
niemeyerDec 12 09:56:09 <niemeyer>rogpeppe: In general, when something is ported we can actually *port* it15:57
rogpeppeniemeyer: every single entity in the system that has a api.Info will have state.Info15:57
rogpeppeniemeyer: but not vice versa15:57
niemeyerrogpeppe: Sorry, my opinion is the same.. if you want to talk to William and get agreement on something else, please feel free to move on with it15:58
niemeyerrogpeppe: I would just be explicit myself15:58
rogpeppeniemeyer: ok, fair enough.15:58
rogpeppeniemeyer: in which case, does the following change to Environ make sense to you? http://paste.ubuntu.com/1559933/15:58
rogpeppeniemeyer: i was considering having two entry points, StateInfo and APIInfo, but they duplicate almost all of their work.16:00
rogpeppeniemeyer: and we'll only want one eventually16:00
niemeyerrogpeppe: It is kind of weird.. but that method always was16:00
rogpeppeniemeyer: StartInstance?16:00
niemeyerrogpeppe: I think eventually it should be returning something environ-specific16:00
niemeyerrogpeppe: StateInfo16:00
rogpeppeniemeyer: then the environ would connect to the state?16:01
niemeyerrogpeppe: Same thing about StartInstance16:01
niemeyerrogpeppe: But I think this change looks fine for the moment, yeah16:01
niemeyerrogpeppe: Those other things are things to think in the future16:02
niemeyerrogpeppe: Why does it need both, btw?16:02
rogpeppeniemeyer: how else does a new instance know how to connect to the API?16:03
niemeyerrogpeppe: Supposedly there's a lot of duplication in them.. what's the information that is unique to each?16:03
rogpeppeniemeyer: the port16:03
rogpeppeniemeyer: and, currently, api.Info holds a single address, not a list16:04
niemeyerrogpeppe: Hmm.. any particular reason?16:04
rogpeppeniemeyer: just that it was easy to get going, and i'm not quite sure how we'll make use of an address list as an API client.16:05
rogpeppeniemeyer: perhaps just choose one at random16:05
niemeyerrogpeppe: I'm not sure either, but it's quite clear that this will happen16:06
rogpeppeniemeyer: but given that nothing has more than one address currently, it seems a bit like overkill16:06
niemeyerrogpeppe: Yes, probably random16:06
rogpeppeniemeyer: it'll be easy enough to change when HA comes in16:06
niemeyerrogpeppe: I'd not do thta16:06
niemeyerrogpeppe: It's easy to overlook it16:06
niemeyerrogpeppe: For example, the data file in Amazon holds a list16:07
niemeyerrogpeppe: In S3, that is16:07
niemeyerrogpeppe: Changing to multiple addresses is just a matter of adding more of them, and that won't break previous clients16:07
rogpeppeniemeyer: i'd never intended to change that, just the Addr field in api.Info16:07
niemeyerrogpeppe: My point is that it's easy to overlook stuff up if you hide the fact this is *intended* to be a list16:08
rogpeppeniemeyer: but if you think it should be changed now, i'll do that16:08
niemeyerrogpeppe: Using the single item in a list is easy.. backporting data files and making sure previous clients continue to work correctly when you provide a list isn't16:09
niemeyerI'm going to step out to take Otávio to his ear check-up16:15
niemeyerBack in a bit16:15
hazmatniemeyer, ping16:26
* niemeyer is back17:33
rogpeppeniemeyer: i'm presuming the same reasoning applies to juju.Conn too, so i'm making juju.APIConn which is the same as juju.Conn but holds api.State17:44
niemeyerrogpeppe: Uh?17:44
rogpeppeniemeyer: the alternative is to have juju.Conn hold both *state.State and *api.State17:45
niemeyerrogpeppe: I didn't imagine we'd be forking Conn17:45
rogpeppeniemeyer: your reasoning applies there too, i think17:45
niemeyerrogpeppe: I don't see how..17:45
niemeyerrogpeppe: Conn is what we want to port..17:46
rogpeppeniemeyer: if we're just passing around juju.Conn, with both connections in it, how can we know easily what's left to port?17:46
niemeyerrogpeppe: Whatever uses juju.Conn will continue using juju.Conn17:47
rogpeppeniemeyer: ok, what about things that use Conn.State directly?17:47
niemeyerrogpeppe: Either return both, or add an APIState method17:47
rogpeppeniemeyer: it's a field17:47
niemeyerrogpeppe: The latter is probably better17:47
niemeyerrogpeppe: Sure, whatever17:48
rogpeppeniemeyer: i actually started doing it that way, but thought you'd prefer it separate. and actually, the code *is* quite a bit cleaner when it's kept separate.17:49
niemeyerrogpeppe: and then duplicate all the methods of Conn, and all the tests?17:50
rogpeppeniemeyer: each method of Conn will need to be ported individually17:50
bigjoolshas go got an x509 library?17:50
rogpeppeniemeyer: because most of them have state-related types in the signatures17:50
niemeyerrogpeppe: Yes, and it can be ported on the same Conn17:50
niemeyerbigjools: Yep17:51
bigjoolscool, ta17:51
niemeyerbigjools: In the standard library.. have a look under crypto17:51
bigjoolssounds good, ta17:51
niemeyerbigjools: http://golang.org/pkg/crypto/17:51
rogpeppebigjools: we do a fair amount of x509 hackery in juju-core - grep for x50917:52
niemeyerrogpeppe: Still.. the idea is being able to port one at a time17:52
niemeyerrogpeppe: But, I really don't mind too much, to be honest..17:52
bigjoolsya, we need to use it for Azure17:52
niemeyerrogpeppe: It feels like a lot of work to be introducing a second juju.Conn and duplicating all the tests and porting all the client sites17:52
niemeyerrogpeppe: If you feel like that's easier..17:52
rogpeppeniemeyer: i think it may be17:53
rogpeppeniemeyer: i'll see how it goes.17:54
rogpeppeniemeyer: currently it certainly seems cleaner than interleaving the code,  but i may well change my mind!17:54
niemeyerrogpeppe: If there are two methods, you'll be unable to port one of them at a time, for example..17:54
niemeyerrogpeppe: But you have better context than I do, so go for it17:55
rogpeppeniemeyer: it would be awkward the other way around too - e.g. AddService returns *state.Service, which AddUnits expects as an argument17:55
niemeyerrogpeppe: Yeah, and maybe the API is not that big anyway17:56
rogpeppeniemeyer: i think so. we'll see17:57
rogpeppeniemeyer: i've gotta go now. good progress made today, BTW.17:57
rogpeppeg'night all17:57
niemeyerrogpeppe: Great to hear!17:59
niemeyerrogpeppe: It looks like things are progressing very well indeed17:59
niemeyerrogpeppe: Have a good evening there17:59
bigjoolsWhat are the requirements of provider storage now?  What needs to be stored and what key is used to access it?20:28

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