[00:00] or make StreamsVersion private [00:00] either way [00:00] once it's public people will starting using it for things [00:00] think of the version.Version cluster fuck === Ursinha is now known as Ursinha-afk [00:06] davecheney: I did [00:06] wait [00:06] davecheney: expand that [00:06] perrito666: i cannot see any comments on reviewboard [00:07] this may not be your fault [00:07] it could be RB [00:07] davecheney: which one, I sent you two emails [00:07] perrito666: did you place any comments on http://reviews.vapour.ws/r/18/ [00:07] no, I just LGTMd, I see you added a comment, I am checking what I missed now [00:07] I did add several comments to 33 though [00:08] perrito666: cool [00:08] to restate, this is not a critisism [00:08] just checking that i'm not missing things recorded in reviewboard [00:08] i find it's interface as comfortable as riding a tractor [00:08] davecheney: that is ok, I just went back to take a look just in case, I a a bit lost in RB still [00:09] perrito666: 33 is NOT LGTM [00:09] davecheney: last tractor I rode was waaay better than my car [00:10] davecheney: odd, dimitern said that this change was suggested by you [00:10] yup, he's fixed some todos [00:10] which is awesome [00:11] but changed the tags inside some structures which i'm super protective of [00:11] davecheney: re the tag changes - it was making writing tests unwieldy and dave already commented in a few places we need to fix this [00:11] he got carried away with changes [00:12] perrito666: the reason we have to use a type assertion is the interface those types conforms to requries the Tag() method return a names.Tag, [00:12] that can be fixed [00:12] but the type stored inside the structure must be the concrete tag type that matches the thing it's tagged [00:12] or in other wods [00:12] type Machince struct { tag names.MachineTag } [00:12] ^ anything else is going backwards [00:13] makes sense [00:20] davecheney: thanks for the explanation though [00:21] thumper: another one: http://reviews.vapour.ws/r/25/ [00:22] davecheney: btw, do you see the comments I did there? [00:24] perrito666: nope [00:24] but i didn't read the whole thing [00:25] just rage quit when i saw the change to the tags [00:25] mm, I seem to need to do some changes so others can see those [00:25] gotta push the 'review' button [00:25] which actually means 'post review' [00:25] the [00:26] the 'review' button is actually labeled 'view diff' [00:26] I did, odd [00:26] thumper: with RB, if you're happy with a change you need to click "Ship it" or tick the option (depending on which screen you're on) [00:26] thumper: I've just done it for #30 since you were happy with that one. [00:26] * perrito666 looks for a way to links to his comments and fails.. and burns the food in the stove [00:27] kk [00:28] thumper: i have to go out in 60 mins [00:29] any chance of a chat before then ? [00:29] davecheney: sure, how about now? [00:29] thumper: i'll see you in the standup channel [00:29] kk [00:46] megawatcher tests are passing === Ursinha-afk is now known as Ursinha [00:57] wallyworld: is there any precedent for passing in partially filled results structs as filters/tempates for an API List* or Get* method in Juju? [00:58] jcw4: do you have a concrete example? [00:59] wallyworld: well for actions, I want to be able to List the actions queued up for a Service and/or a Unit, and possibly filter by Status (pending, running, finished, failed, etc.) [00:59] s%and/or%or% [01:00] you'd define a Filter struct to hold the query parameters? [01:00] wallyworld: I was thinking of using the return type struct and partially filling out the fields...? [01:00] yuk :-) [01:00] :) [01:00] IMHO [01:01] (that's why I asked) [01:01] :-) [01:01] np [01:01] I know mongo does something like that [01:02] I'm pulled between keeping type explosion down vs. simple API [01:02] it does? i didn't know that. I've just encountered Find(blah).One(&result) type stuff [01:02] sometimes simple != intuitive [01:02] wallyworld: I interpreted the blah as a 'template' [01:03] eg [01:03] s.db.C("managedStoredResources").Find(bson.D{{"path", "environs/env" + path}}).One(&mrDoc) [01:03] blah = a list of filter conditions [01:04] wallyworld: okay.. so instead of one Get method, maybe GetActionsOn(service names.Tag), GetFailedActionsOn(service names.Tag), etc... [01:04] or GetActionsByStatus(entity names.Tag, status string) etc. [01:05] the latter would be preferable [01:05] and, modified to be all bulk calls of course [01:05] i would imagine under the covers it would call GetActions(filter) [01:05] wallyworld: yes. [01:06] okay, so slightly more verbose API would be preferable [01:06] that works for me personally. i like a wide API at the level of the caller, and a narrow api over the wire [01:07] wallyworld: hence the GetActions(filter) under the covers I suppose? [01:08] yep [01:09] a narrow api over the wire is more robust against change [01:09] eg if a new status were added [01:09] makes sense [01:50] axw: hi there, i booted an aws environment from trunk using upload-tools, and when I try and add-machine, I get [01:50] "1": [01:50] agent-state-info: no matching tools available [01:50] :/ [01:50] have you seen that? [01:51] nope [01:51] ok, i'll poke around [02:06] wallyworld: what's this placement/ensure availabiity stuff? [02:06] wallyworld: you guys going to give me a manual api back? [02:06] rick_h_: the landscape guys want to use ensure-availability and be able to nominate what machines should become the new HA servers [02:06] we already have this for bootstrap [02:07] wallyworld: what about UI guys that want to have ensure-availability by default, but allow people to override if they want to? [02:07] it's extending that to ensure-availability [02:07] wallyworld: ah ok, so not what I'm hoping for [02:07] what do you mean ensure-availability by default? [02:08] so that is on by default on azure [02:08] which is cool, I like safe defaults and all that [02:08] but that means by default, no user can specify placement [02:08] they have to rebootstrap without the ensure-availability and then don't get any help [02:09] yes, that is an azure limitation :-( [02:09] right [02:09] this change is primarily for maas [02:09] but my understanding is that it's only a limitation because we made it that way [02:09] i don't think so, axw would know more [02:10] no, it was made that way to work around azure's restrictive nature [02:10] there's no way to setup 'I don't care if these are HA' services manually placed, log a log aggregator or *cough* jenkins *cough* install [02:11] rick_h_: the way azure does HA is very different to other cloud providers, which means we can't support both at the same time [02:11] axw: right, but I'm suggestions, or I guess asking, if there's room for some things to not be HA in an env [02:11] /suggestions/suggestion [02:12] rick_h_: yes, with some work we could probably do it on a per-service basis [02:13] I don't think it's a small amount of work though [02:13] axw: yea, in brussels I'll probably ask about it. Especially with the planned work for more auto placement, it kind of breaks the whole chunk of work we've done in machine view [02:13] axw: on the one hand we're looking at trying to enable colocating and collapsing down, but if we start auto placing and turning off overrides, then we break that story for smaller/getting started users [02:14] and even stuff like the orange box setup where almost every node there has multiple services on it [02:14] axw: no need to worry about the versioning stuff for juju run .. I spoke to william and he completely extended the scope of my work and added some rather large, non-trivial things to that PR ;) [02:14] wwitzel3: lol [02:14] axw: good news is, it will be versioned :) [02:14] axw: wallyworld ok, saw a branch go by with an interesting little title and had to ask what was up. [02:15] rick_h_: I understand, we do need to do something about it [02:15] rick_h_: np at all, ask anytime [02:15] it's a bit crappy having to choose [02:15] at such a coarse level anyway [02:15] axw: I'm just making a new, smaller, focused facade for the run part of the client, instead of leaving it all rolled up under client. [02:15] wwitzel3: cool, SGTM, just like what wallyworld did for ensure-availability [02:16] axw: yea, my feeling on it. I think it's good stuff, just a bit :( since it's by default and the answer is "once you realize you DO want to colocate something, rebootstrap" [02:16] and eric for Backup, and also previously for KeyManager, UserManager etc [02:17] wallyworld: I hate private image metadata [02:17] :( [02:17] sadly we need it :0( [02:17] :-( [02:17] axw: yep, and the usermanager facade [02:17] it's messing up my nice abstractions [02:17] wwitzel3: and keymanager and backup [02:18] axw: did what we discuss last night about adding it in to the current CustomMetadata work flow turn out? [02:19] wallyworld: I've got an API client-based one jammed in there. feels a bit wrong to be calling the API from there though... [02:19] also, going to need to do something else to read the image metadata at bootstrap time [02:19] previously we'd upload to cloud storage before bootstrapping, don't have that luxury now [02:20] axw: we should use a bespoke interface and inject in an implementation [02:20] for retrieving image metadata [02:20] the interface lives in provider [02:20] there's then an api cient adaptor [02:21] or something like that [02:21] wallyworld: it's too error prone... we'll end up missing a call to set the source [02:21] that's what tests are for :-) [02:21] tests where? *everything* uses an Environ, and we add it all the time [02:21] and we can have the thing which constructs the provider even read stuff from state, and pass in static data behind that interface [02:22] environs.New(*config.Config) [02:22] hmmm [02:23] maybe it needs to be done on the provider [02:23] cause that's where startinstance is [02:23] no it's not, that's on Environ [02:23] ah, yeah [02:23] EnvironProvider just opens/prepares [02:24] more or less [02:24] it's at that point we could pass in the metadata [02:25] this is all handwavy without looking back at the code [02:30] i'm getting the following error: /usr/lib/go/pkg/tool/linux_amd64/6l: conflicting definitions for @"github.com/juju/juju/state".State [02:32] is it safe to delete /usr/lib/go/pkg/ ? [02:32] I deleted ~/go/pkg but it didn't help [02:32] davecheney: ^? [02:33] waigani: I don't think you'd want to remove /usr/lib/go/pkg, unless you installed a package there with sudo [02:33] axw: no I didn't [02:34] axw: I'm using the go that comes with 14.04 [02:34] wallyworld: I'm going to investigate another approach of having a datasource registry in environs/imagemetadata [02:34] ok [02:35] waigani: ok, dunno what that error is, but removing that dir sounds wrong [02:35] yeah, felt wrong, that's why I asked. thanks [02:35] actually its referring to a bunch of stuff in tmp [02:35] that I can delete [02:35] e.g /tmp/go-build964096169/github.com/juju/juju/state/_test/github.com/juju/juju/state_test.a(_go_.6): [02:36] wallyworld: the registry will be keyed on environ. may make it possible to remove the SupportsCustomSources interfaces [02:37] worth a spike [02:37] except that its already been deleted... hmm [02:41] fixed it, that was a new one [02:43] wallyworld: do you need a review for this? http://reviews.vapour.ws/r/38 [02:43] menn0: yes please [02:43] wallyworld: doing it now [02:44] ty [02:56] menn0: i pushed a small fix to the github pull request, what's the command to update the reviewboard diff? just "rbt post" ? [02:57] wallyworld: "rbt post -u" might do it... or else "rbt post -r " [02:57] ta [03:16] axw: found the tools problem. i had an older compiled jujud in my path (1.20.8) and it found and uploaded that one and then when that 1.20.8 jujud went to find tools, it looked for 1.20.8 and there were none as juju recorded tools for 1.21 [03:17] wallyworld: ah, ok. [03:18] that's a problem with upload-tools, it just assumes whatever it finds is the same as the CLI version [03:18] yeah, so i found out :-( [03:18] i didn't even realise i had a binary, and i didn't notice the log message n amongst all the noise [03:26] wallyworld: just finished reviewing http://reviews.vapour.ws/r/38/. thumper can you do a meta-review please? [03:26] thanks menn0 [03:27] * thumper looks [03:31] menn0: I can't see your comments on the main diff [03:32] ahh... [03:32] because the comment is on diff one [03:32] thumper: ah... wallyworld uploaded a new diff while I was reviewing... [03:33] thumper: if you're looking at my review comments, you can click the title (where the filename is) to jump to the code. [03:33] menn0: thumper: i am going to back out that change i think - the issue i had from from using an old jujud. i thought i messed up the api versioning as i just guessed based on reading the code, but it turns out i was right after all [03:34] but i will keep the extra test [03:37] menn0: thumper: although, I do like the fact that the new implementation gets the version directly off the ha client. but it means possibly opening a ha client and closing it again if the version is wrong. what implementation do you guys prefer? [03:38] the first one gets the facade version by asking for it by name [03:38] the second one gets a client and asks the client [03:39] menn0, wallyworld: why has noone updated the branch in the .reviewboardrc? [03:40] nfi [03:41] thumper: I think we needed to establish whether everyone has the "upstream" remote set up in the same way [03:42] menn0: if they followed the instructions they should have [03:42] menn0: if not, they need to fix it [03:42] menn0: why make the rest of us suffer? [03:42] thumper: yeah.. and no-one said anything when I mentioned it on the list [03:42] thumper: let's fix it [03:43] thumper: I've been running rbt post with --tracking-branch=upstream/master in the mean time [03:43] * thumper nods [03:43] * thumper submits his first review board review [03:45] http://reviews.vapour.ws/r/39/diff/ [03:45] thumper: will take a look soon [03:46] ta [04:16] jam: hi, i'd like your input on determining API version. http://reviews.vapour.ws/r/38/ The first diff did this: root.BestFacadeVersion("HighAvailability"), the second diff does this after instantiating a client: client.BestAPIVersion(). Both work. The first way doesn't require creating a client only to potentially not use it if the API doesn;t exist, but does require knowing the facade by name. What would you prefer? [04:17] this is in cmd/juju/ensureavailability.go [04:17] wallyworld: so, I haven't looked at the whole diff, but is the idea that you're moving EA into another location? [04:18] so IMO, you would have an api/ha/client.go that would try to use the new location if it existed, else fall back to the old location [04:18] in which case, you'd still be wanting the client [04:18] it would just be using the right API call underneath [04:19] jam: the facade name is different [04:19] i'm moving methods off the original big client [04:19] wallyworld: that doesn't mean you need a different client side type [04:19] and onto a whole new facade [04:20] wallyworld: sure, but the api/highavailability/client.go can still call APICall("Client", 0, "", "EnsureAvailability", ...) [04:21] oh,i see [04:21] that sounds nicer [04:21] jam: i'll implement that and get you to +1 if that's ok [04:21] wallyworld: so it depends *what* we can get away with, but my idea is that we should push the compatibility layer as low as we can [04:21] yep [04:21] agreed [04:22] i didn't know how to do that [04:22] wallyworld: so if we can hide compat underneath api/foo.go then do so [04:22] +1 [04:22] sometimes it has to be done in client code like cmd/juju level code [04:22] but for this, I think you can do it lower. [04:22] that would be good [04:23] wallyworld: fwiw, I would rather it not be called apiserver/highavailability/client.go [04:23] "client" here seems confusing. [04:23] wallyworld: api/highavailability/highavailability.go seems slightly better (if a bit longer) [04:23] fair point, i cargo culted that from somewhere [04:23] ill fix [04:24] wallyworld: apiserver/client/client.go I'd imagine [04:24] could be [04:40] jam: i've pushed the changes to rb if you would be kind enough to look [04:40] thumper: wow, such diffs, very long [04:40] thumper: almost done [04:41] menn0: ack, and ta [04:46] thumper: done [04:50] thumper: thanks for the chat earlier [04:50] i've had a good mull over it [04:50] davechen1y: np [04:51] i think the multiwatcher should only accept types defined in its package [04:51] i won't say it is the only sane way to do it [04:51] but it feels more sane than what we have at the moment [04:59] holy shit [04:59] thumper: [04:59] type EntityId struct { Kind string Id interface{} [04:59] } [04:59] look, it's a tag!! [05:00] davechen1y: yeah, I've been looking at that information now in the watcher... [05:00] thumper: i keep getting confused between state/megawatcher, and state/multiwatcher [05:01] me too [05:04] thumper: i'll be spending 1/2 of tomorrow and all of friday on arm stuff [05:04] ack [05:04] now we have a machine I suspect aram's output will increase quite a lot [05:18] wallyworld: morning ... is there anything that you'd like to discuss today on a regular catchup? [05:18] urulama: hey, morning. [05:22] thumper: i'm thinking that params.EntityId should be the state versino of a tag [05:22] william has said many times he doesn't want tags stored in the db [05:22] but it feels like there is a case of a tag like think [05:22] and entity id may be that thing [05:22] it's an identified and a type bound together [05:22] hmm... interesting [05:22] honestly entitydi could be [05:23] type MachineEntity interface{} [05:23] well maybe, that might be a bad idea [05:23] but that sort of thing [05:23] it's a typed thing [05:23] and that is _exactly_ what an interface is [05:23] * thumper nods [05:23] sounds reasonable... [05:24] i haven't quited figured out what entity id's do yet [05:24] they seem to be a way of informing watchers what _type_ of entity changed [05:24] I was just looking at the entity stuff as a wip review of jesse's look at adding the env-uuid to the service collection [05:24] I'm wanting the id to be environment specific [05:24] so the actual document ID is an opaque transformation of the entity id [05:24] oh yeah, they completely fail to capture that [05:24] they are at best a tupple { collection, row } [05:25] * thumper nods [05:25] it could be looked at like that [05:25] so, the question is, do we extend the tupple to be [05:25] but I think it would be good to work out the real intent [05:25] { collection, env-uuid, row } [05:25] to work out how best to model this [05:25] I don't think that bit is necessary, as the state instance has an implicit env-uuid [05:25] or stick with { collection, row} and accept that GetAll does not return the complete set of documents in the collection [05:25] which is why I think the mega watcher needs to be environment specific [05:26] the later of what you said [05:26] I think [05:26] i need to figure out where that data is begin used [05:26] i'm guessing it's somethinglike [05:26] yeah, I agree [05:26] switch collection { [05:26] case "machines": state.GetMachine(id) [05:27] case "unit"L: state.GetUnit(id) [05:27] and so forth [05:27] which is so like tags it's not funny [05:32] thumper: why do all the backingNNN methods in megawatcher.go return an error [05:32] yet they are all hard coded to return nil [05:32] ? [05:32] NFI [05:32] store.Update(info) [05:32] return nil [05:32] } [05:32] * davechen1y feels a branch coming on [05:32] because someone thought it was a good idea? [05:33] davechen1y: it would almost be "state.FindEntity(tag)" for the switch above [05:33] i wonder if they have to fit some interface [05:34] ./megawatcher.go:458: cannot use (*backingRelation)(nil) (type *backingRelation) as type backingEntityDoc in assignment: [05:34] hmmm [05:48] menn0: so when I update this review, do I squish locally first? [05:48] what is the normal practice? [05:49] thumper: you can do whatever you like. review board will just upload the diff of your branch to upstream/master, regardless of how many commits it's in [05:49] ok [05:50] menn0: is there a keyboard shortcut to take me to the next review comment? [05:50] that'd be handy [05:51] I haven't looked but I'd like to know too if there is one [05:52] * thumper looks for the rbt command to update [05:54] * thumper is off now. [06:07] wallyworld: thumper is EOD so could you do a meta-review for this please? http://reviews.vapour.ws/r/34/ [06:07] sure [06:07] wallyworld: otherwise ericsnow will have to wait longer to get his review [06:07] and we don't want that :-) [06:08] in case we need to ask for a reviewboard fix :-) [06:21] wallyworld: :) [06:21] wallyworld: cheers [06:21] menn0: 3/4 of the way through [06:27] wallyworld: so your patch doesn't actually get the backwards compatibility stuff quite right. [06:27] ok [06:28] wallyworld: you mention "regression tested on AWS", does that mean you did "juju-1.20 bootstrap" and "juju-1.21 ensure-availabilty" ? [06:29] jam: yes [06:29] but if i got the backwards compat stuff wrong, i will retest [06:30] wallyworld: so you don't actually fall back to calling the Client API [06:30] AFAICT [06:30] HA didn't exist in 1.20, right? [06:31] jam: i think you are right, i tested before i did the last lot of changes after talking to you [06:31] wallyworld: AFAICT, your legacy test is only patching the client side, so if it makes the new API call, the server still would accept it. [06:32] yeah, i'll have to rework it i think and retest [06:32] so you really want a server that doesn't think it has the new functions, so that you know the client isn't requesting them [06:32] TheMue should have some examples he's putting together [06:33] or i could patch out the client and check the facade called [06:33] wallyworld: I feel like that still gets you into a place where you have to be careful to patch everything that might be touched [06:33] you could [06:33] sure, yeah, i'll run up one end-end test [06:34] i'll look for the doc [06:34] thanks [06:35] jam: for some reason, reviewboard doesn't show the client.go files as renamed; the github pr does [07:09] morning [07:23] jam, hey, would you like to have a look at my http://reviews.vapour.ws/r/33/ please? [07:33] dimitern: looking [07:33] jam, thanks [07:37] jam, in particular I'm having some trouble understanding why "s.firewaller.Machine(s.machines[0].Tag())" is worse than "s.firewaller.Machine(s.machines[0].Tag().(names.MachineTag))" [07:38] dimitern: I'm not personally very sure why MachineTag is better/worse than just Tag, for that you'd have to have a discussion with davecheney [07:39] (I certainly thought the point of Tag was that it was the abstract representation, so having type-specific versions of it seems odd) [07:40] dimitern: oh, and *IMO* ever doing a plain type assertion: .(names.MachineTag) in production code is asking for a panic to bring your system down [07:40] panic() ==> the whole thing dies [07:40] jam, strictly speaking, using names.Tag internally is less safe than names.MachineTag, but the apiserver facade validates all tags it gets, so there's no way to pass a unit tag to a method allowing only machine tags and not get an error [07:40] error() => we return "sorry can't do that" [07:41] jam, ok so why state.Machine.Tag() returns names.Tag rather than names.MachineTag ? [07:41] jam, I think in the API we should follow the same logic [07:41] dimitern: *If* we are supposed to be using names.MachineTag, then it seems like it should [07:42] dimitern: as stated, *I* don't really understand why [07:42] jam, oh, ok - so we're in the same boat on this :) [07:42] dimitern: my point was just that a bare type assertion seems to mostly be the wrong thing to do [07:43] where are thumper and davecheney when you need them :) [07:43] dimitern: certainly IMO MachineTag should be an implementation detail of Tag [07:44] dimitern: also note that NewUnitTag was something that exists, and dave has stated "it is there for tests" but we don't have another way to go from "unit/0" to a UnitTag [07:44] jam, I agree unchecked type assertions are bad, but if state.Machine.Tag() returns anything else than a names.MachineTag wrapped into a names.Tag interface, I'd rather panic than let it slip :) [07:44] (it also used to panic if you gave it bad data, which meant that charms could crash the API server) [07:45] dimitern: panic if developers are writing bad assumptions, but don't panic if you were given bad data [07:46] jam, I think I understand their point - the internal tag filed of the Machine struct should be names.MachineTag, but firewaller.Machine() should take names.Tag and possibly ParseMachineTag and verify internally [07:46] s/filed/field/ [07:46] dimitern: so NewFacadeCallerForVersion seems like a bit of a "we don't really want to do it this way", but I suppose the point is to allow the server to go to a new version before the client is ready to handle it? But we can just do that by not incrementing the best-known version in api/facadeversions.go [07:46] though I guess the test suite will tell you off for that... [07:47] jam, no, it's not about the client [07:47] jam, it's a way to implement a new server and client version of the api, test it and only switch over to it once it's user (i.e. the worker) is ready [07:49] jam, I thought a lot about how to do it nicely, and I'm keen on your thoughts about my approach (both server-side and client) [07:50] dimitern: so this is actually because we can't abstract the API difference, and we need 2 different workers, thus we are forced to explicitly chose which version the worker is talking to [07:50] ? [07:51] jam, in this case, having old and new versions of the worker using v0 and v1 is the easiest way, until we can drop the old worker and rely on having apiserver supporting v1 [07:52] jam, in reality, once we have apiv1 in place and the upgrade step done, I think it will probably be safe to drop the v0-only worker, isn't it? [07:54] jam, the only problematic case is when a MA running v1 firewaller worker is talking to a v0 apiserver, but since upgrades are done in lock-step, can we have this case at all? [07:54] dimitern: you mean, we are unlikely to be talking to a v0 API server if the Agent is V1 ? [07:54] I do believe that is true [07:54] dimitern: well, if they "finished" the upgrade path wrt HA upgrades [07:54] I'm not sure if menno has landed all of the pieces there [07:54] jam, yeah, so the old v0 worker will be kept only until upgrade is done [07:55] morning all [07:56]