mwhudsondavechen1y: just a peanut gallery comment but does that ppc64 crash happen with GOGC=off?00:51
davechen1ymwhudson: let me check01:15
davechen1yi already checked stack copying01:15
davechen1yand adjusted the min stack size to be higher01:15
davechen1ywhat it looks like to me is the receiver of the method is not being carried over in the case that there are no arguments01:15
davechen1ywhich is weird01:15
davechen1ybut that'ts the case01:16
mwhudsondoes it happen all the time?01:16
mwhudsoni could guess that autogenerated methods might screw up SP manipulation but that seems like the sort of thing that ought to break hard all the time01:17
davechen1ymwhudson: 100% of the time01:19
mwhudsonwell that shooooooooouuuuuuuuuuuuulllllllllllllld make it easier to fix01:19
davechen1ygithub.com/juju/juju/cmd/envcmd.(*SysCommandBase).ConnectionInfo(0x478420, 0x0, 0x0, 0x0, 0x0) /home/ubuntu/src/github.com/juju/juju/cmd/envcmd/systemcommand.go:130 +0xec fp=0xc820484c98 sp=0xc820484c2001:19
davechen1ygithub.com/juju/juju/cmd/envcmd.(*SysCommandBase).ConnectionEndpoint(0x478420, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...) /home/ubuntu/src/github.com/juju/juju/cmd/envcmd/systemcommand.go:116 +0x5c fp=0xc820484df8 sp=0xc820484c9801:19
davechen1yreceiver is 0x0 in the autogenerated method01:19
davechen1ythen bonkers in the next frame down01:20
davechen1ygithub.com/juju/juju/cmd/envcmd.(*SysCommandBase).ConnectionEndpoint(0x478420, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...) /home/ubuntu/src/github.com/juju/juju/cmd/envcmd/systemcommand.go:116 +0x5c fp=0xc820484df8 sp=0xc820484c9801:20
davechen1ygithub.com/juju/juju/cmd/juju/user.(*AddCommand).ConnectionEndpoint(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...) <autogenerated>:17 +0x30 fp=0xc820484e00 sp=0xc820484df801:20
davechen1ythis, not the other one01:20
mwhudsonis a nil receiver plausible?01:20
davechen1ytheorerically yes01:21
davechen1ycalling through an interface, no01:21
davechen1yand in this case01:21
davechen1ythe receiver was not nil01:21
davechen1ythe weird thing is genwrapper is only called from two places in the compiler01:22
davechen1yit is called a _lot_01:22
mwhudsonright, and you haven't been able to reduce it?01:22
davechen1yit takes minutes every run01:22
davechen1yi am trying to reduce it01:22
davechen1ybut everything on ppc takes so long01:23
davechen1yalso, you cannot breakpoint an autogenerated wrapper01:23
davechen1ywhich is fucked01:23
davechen1yif you could01:23
davechen1ythis would be easy01:23
mwhudsongdb and go, two great tastes etc01:23
mwhudsondavechen1y: you can break point on an address btw01:24
mwhudsonbr *0xdeadbeef01:24
davechen1ygithub.com/juju/juju/cmd/envcmd.(*sysCommandWrapper).Run(0xc82023e580, 0xc82040b590, 0x0, 0x0) <autogenerated>:61 +0x9c fp=0xc820485300 sp=0xc8204852b801:24
davechen1ywhat's the address ?01:24
mwhudsondoes "p 'github.com/juju/juju/cmd/envcmd.(*sysCommandWrapper).Run'" show anything?01:25
mwhudsondavechen1y: lolz01:49
mwhudson(vivid-ppc64el)mwh@kelsey01:~$ ~/go/bin/go get github.com/juju/juju/...01:49
mwhudson# cd /home/mwh/gopath/src/github.com/juju/juju; git show-ref01:49
mwhudsonpackage github.com/juju/juju/...: signal: segmentation fault01:49
davechen1ymwhudson: it prints something03:53
davechen1ybut it's not that useful03:53
davechen1yi have some ideas for a repro now03:53
davechen1yi'll keep trying03:53
wallyworldaxw: did you have 5 minutes? in standup hangout?04:13
axwwallyworld: be there in a sec04:13
mupBug #1483492 opened: worker/storageprovisioner: machine agents attempting to attach environ-scoped volumes <juju-core:Triaged by axwalk> <https://launchpad.net/bugs/1483492>04:30
aisraelaxw: building with your latest pull request05:12
davechen1ymwhudson: Thuper: got it!05:27
davechen1yi have a repro05:27
davechen1ywhich doesn't involve any juju code05:28
mupBug #1483525 opened: Document action-set quoting <juju-core:New> <https://launchpad.net/bugs/1483525>06:00
axwaisrael: thanks for the update. the other things are known issues. to destroy with persistent storage, you currently have to use --force, or destroy the machines that the volumes are attached to. I'll send a PR soon that makes this unnecessary06:38
davechen1ymwhudson: i know what it is06:41
mupBug #1236859 changed: errors reported in agent-state can contain too much information (traceback, etc) <logging> <traceback> <juju-core:Fix Released> <https://launchpad.net/bugs/1236859>07:00
voidspacedimitern: ping07:07
voidspacedimitern: unping07:09
dimiternvoidspace, (un)pong? :)07:11
voidspacedimitern: hah07:11
voidspacedimitern: I had a question, but I figured it out...07:11
dimiternvoidspace, awesome!07:11
voidspacehah :-)07:12
voidspacenot sure that counts as awesome, but thanks07:12
voidspaceyou must have had your coffee already this morning07:12
mupBug #1298819 changed: Mechanism for retrieving relation names <feature> <relations> <juju-core:Won't Fix> <https://launchpad.net/bugs/1298819>07:12
dimiternvoidspace, well, you know at least it's a great start of the morning07:13
dimiternvoidspace, :)07:13
voidspaceapparently giving your smilies noses, the - in :-) , is a sign that you're old07:14
voidspaceI just can't get used to :) though07:14
voidspacehis eyes are too close to his mouth07:14
dimiternI like :o) sometimes or just ;] when lazy07:16
voidspace:o) looks like a clown07:18
anastasiamacvoidspace: really? it has something pig-ish to my eyes .. ")07:19
voidspaceanastasiamac: heh, maybe07:20
voidspaceclown pig07:20
voidspacedimitern: soooo....07:20
voidspacedimitern: we have an issue with address allocation for containers07:21
voidspacedimitern: we get the subnet from the provider07:21
voidspacedimitern: and create it in state if necessary07:21
voidspacedimitern: and now subnets in state have space information07:21
voidspacedimitern: but address allocation for containers cares nothing for spaces yet07:21
voidspacedimitern: so if the subnet it discovers from the provider doesn't exist in state it will create it in the default space07:22
voidspacedimitern: I might just ignore this for my PR and I think we need a new card for it07:22
voidspacedimitern: because fixing that is quite a bit of thinking07:22
voidspaceanastasiamac: good evening07:23
voidspacedimitern: what it ought to do is check the space constraints for the container, iterate over all subnets until it finds one that matches and use that07:24
mupBug #1298819 opened: Mechanism for retrieving relation names <feature> <relations> <juju-core:Won't Fix> <https://launchpad.net/bugs/1298819>07:24
anastasiamacvoidspace: evening :D07:25
dimiternvoidspace, it does need a new card, yes07:30
mupBug #1298819 changed: Mechanism for retrieving relation names <feature> <relations> <juju-core:Won't Fix> <https://launchpad.net/bugs/1298819>07:30
dimiternvoidspace, but it needs spaces-in-constraints support first07:30
voidspacedimitern: fair enough07:31
voidspacedimitern: maybe it's time to start telling containers which subnet(s) to pick an address from instead of just picking an arbitrary one07:31
voidspacedimitern: at the moment we discover subnets and create them in state when we use them07:32
voidspaceI'll add a card for us to estimate07:32
voidspacedimitern: I added a card for updating AddSpace/CreateSpace to allow spaces with 0 subnets07:35
voidspacedimitern: not much work, but some work07:35
dimiternvoidspace, cheers07:36
dimiternvoidspace, as soon as we finish the mvp stuff, we should make sure the address allocation still works and fix it as needed07:37
voidspacedimitern: why does subnetsAPI define AllSpaces?07:38
voidspaceand define it differently to the spacesAPI07:38
dimiternvoidspace, to verify the spaces07:38
dimiternvoidspace, is this the backing interface or?07:39
voidspacedimitern: apiserver/subnets/subnets.go07:39
voidspacedimitern: and it returns a list of space tags, whereas AllSpaces as used by "space list" returns full space information07:40
dimiternvoidspace, well, we only need the tags for the CLI, but I guess there's no harm do get all the information if it's easier07:43
voidspacedimitern: ah, it's called ListSpaces on the spacesAPI07:44
voidspacedimitern: but the backing call is AllSpaces07:44
voidspacedimitern: slightly confusing but not actually wrong07:44
dimiternvoidspace, these are one of the earliest attempts - it was expected for the various cli, api, apiserver, and backing interfaces to change, but get some sort of convergence at the backing side eventually07:49
mwhudsondavechen1y: nice test case reduction07:57
mwhudsondavechen1y: ooooh i think i know08:06
davechen1ymwhudson: do tell08:16
davechen1yit's something about the setup for duff copy08:16
davechen1yi'll see what russ says overnight08:16
mwhudsondavechen1y: well duffzero is entered by bl08:16
davechen1ythat' won't help08:17
mwhudsonand the function exits by b to the method08:17
davechen1yi've hit that before in risc compilers08:17
mwhudsonwhich blr's out...08:17
davechen1yand back to the callers caller08:17
davechen1yhang on08:18
mwhudsonback to the generated method i think08:18
davechen1yi don't think that is it08:18
mwhudsoni was clued in by a breakpoint being hit an impossible number of times08:18
davechen1yso it's going around and around until it rolls off the end of the stack ?08:18
mwhudsoner not sure08:19
davechen1yi think you're close08:19
mwhudsoni think the stack pointer is wrong08:19
davechen1ybut that isn't the whole story08:19
davechen1ythe reciver is zeroed out08:19
mwhudsonso the load from the stack at the end of the function is not reading the right bit of stack08:19
mwhudsonthis is all very familiar feeling, did we fix this bug in arm64 already or something?08:19
davechen1yi think you're right with the tail call thing08:20
davechen1ywrapper methods are designed to return to their caller08:20
davechen1yanyway, i know how to fix juju08:20
davechen1yeven if it means a small patch08:20
mwhudsondisable duffzero?08:21
mwhudsonoh, or a juju change?08:21
davechen1yjust disable duffzero08:22
davechen1ywe'll live08:22
mwhudsoni wonder if this fixes the GOARCH=amd64 go build runtime thing08:23
mwhudsonthat's probably a bit optimistic08:23
davechen1yprobaby a long shot08:23
davechen1ymwhudson: anyway, not looking at it any more tonight08:26
mwhudsondavechen1y: fair enough08:26
davechen1ythanks for the sugestions, i'll have a look tomorrow08:26
mwhudsoni'll add a quick comment08:27
dimiterndooferlad, go fmt is sad: cmd/juju/commands/main.go08:27
davechen1ymwhudson: thanks for that comment08:35
davechen1yi think you're right08:35
davechen1ythere was something similar a while back with somethign in the arm32 runtime08:35
davechen1yi don't rememver exactly08:35
davechen1yit was something like the kernel atomic helper08:36
davechen1yor something like that08:36
davechen1yit was the gettls helper08:36
davechen1ywhich was sort of the same thing08:36
davechen1yneeds a kernel helper to get the tls offset08:36
mwhudsonah yes08:36
davechen1yblah blah08:36
davechen1ytail call didn't call no more08:36
mwhudsonthat tickles faint bells08:36
davechen1ymwhudson: b377c9c6a9b720d0897d298652bebd3887ceeb4608:38
davechen1yfor your ref08:38
davechen1yi think08:38
davechen1yi might have fixed a similar problem08:39
davechen1ybut i cannot find the ref08:39
davechen1yit's the same smell08:39
davechen1yarch's with link registers get tripped up by these shennaigans08:39
mwhudsoni can't for the life of me find the code that compiles the tail call08:39
mwhudsoni mean the liblink bit08:42
davechen1yi don't think it's there08:42
mwhudsonsorry, i mean the bit that creats the b $foo instruction08:43
mwhudsoni found it, then realized i was looking at arm6408:43
davechen1yi think it's08:43
mwhudsoncase 11 in asm9.go08:44
davechen1yoh god08:44
davechen1yplease don't make me look at asmN.go08:45
mwhudsonheh ok08:45
davechen1ythat's the worst08:45
davechen1yhmm, this doesn't look to magic08:46
davechen1yi don't think that's the right place to look08:46
davechen1yi see, ADUFFCOPY is implemened in case 1108:47
davechen1yarm and arm64 will be in the same boat08:48
davechen1ywhat are they doing I wonder08:48
mwhudsonyeah i wonder how come they don't break08:48
mwhudsonarm64 reloads the link register from the stack before the tail call08:53
davechen1yare you looking at zerorange in arm64 ?09:13
davechen1yor is that in asm7 ?09:13
davechen1ymwhudson: here's some math for ya09:24
mwhudsondavechen1y: seems like that could do with some constant folding09:24
mwhudsondavechen1y: i think i've found the ppc64le/arm64 point of difference09:25
mwhudsonthe ARET case in preprocess09:25
davechen1yi think it could be soethign else09:27
davechen1yhave a look at zerorange in arm6409:27
davechen1yand zerorange in ppc6409:27
davechen1yarm64 laosd the SP into rt1 then adds the size to zero to it09:27
davechen1yppc64 does not load the sp09:28
davechen1yyes it does09:28
davechen1yit sets the source reg to be regsp09:28
davechen1yand the dest reg to rt1 with an add09:28
davechen1ywe cannot do the same for arm64 because you cannot use Sp as the source or something09:29
davechen1ythat's not it09:29
davechen1yi'm sure you're right about the tail call not reloading sp09:29
mwhudsondavechen1y: disabling duffzero seems like the right fix09:42
mwhudsonfor the short term09:42
mwhudsonand to a general lack of surprise it doesn't fix the "can't compile amd64 code" thing09:52
voidspacedimitern: we used to have space validation that would check all specified subnets have a valid CIDR and exist10:32
voidspacedimitern: I replaced the validation logic with a check that they exist in the transaction10:32
voidspacedimitern: so an invalid subnet still fails (because it doesn't exist in state)10:33
voidspacedimitern: the place to check for an invalid cidr would seem to be when adding a subnet10:33
voidspacedimitern: so I'm going to remove the failing test I have as a consequence10:33
voidspacedimitern: specifying an invalid cidr when adding a space still fails10:33
voidspacedimitern: but now it fails with subnet not found10:33
voidspaceand there's a test for that already10:34
dimiternvoidspace, hmm.. I have to take a look to remind myself..10:37
voidspacedimitern: TestAddSpaceInvalidCIDR is gone10:38
voidspacedimitern: see https://github.com/juju/juju/compare/net-cli...voidspace:spaces-list-state?expand=110:38
voidspacedimitern: due to the removal of Space.validate and changes to AddSpace10:39
voidspacedimitern: so the only difference is that for an invalid CIDR in AddSpace we don't validate first, but we do ensure the subnet exists (it won't) and fail with a sensible error message (subnet not found)10:40
dimiternvoidspace, hmm..10:40
dimiternvoidspace, so invalid CIDR I think should be validated before looking it up10:41
voidspacedimitern: why?10:41
voidspacedimitern: that's only sensible if we will be putting invalid CIDRs in state10:41
voidspacedimitern: which we really shouldn't!10:41
dimiternvoidspace, well, it how about ";drop tables foo" :)10:41
voidspacedimitern: you're saying we have an injection vulnerability...10:42
voidspacedimitern: I really don't think so10:42
dimiternvoidspace, but seriously, we shouldn't put invalid cidrs in state indeed10:42
dimiternvoidspace, jk10:42
dimiternvoidspace, so you're saying adding a space with an empty list of subnets is now allowed?10:43
voidspacedimitern: yes10:43
dimiternvoidspace, but ISTM non-empty list with possibly invalid cidrs leads to erraborted on addspace10:43
voidspacedimitern: yep10:43
voidspacedimitern: and there's a check for that10:43
voidspacedimitern: if we get ErrAborted that isn't due to the space already existing we check to see which subnet is missing10:44
voidspacedimitern: so we can report the error correctly10:44
voidspacedimitern: and there's a test for it, which passes10:44
voidspacewell, missing subnet10:44
voidspacewhich is exactly the same thing (as an invalid cidr will always be missing)10:45
dimiternvoidspace, indeed, sounds reasonable10:45
voidspacecool, thanks10:45
dimiternTheMue, you have review - sorry it took so long11:11
TheMuedimitern: great, thanks.11:11
perrito666morning people11:15
mupBug #1483672 opened: Allow charms to associate structured data with status <cloud-installer> <landscape> <juju-core:Triaged> <https://launchpad.net/bugs/1483672>11:40
TheMuedimitern: the mentioned trick is, because State.DeadIPAddresses() returns a []*state.IPAddress, but in the mock I want a []StateIPAddress. a direct type cast of a slice isn't possible, only in a loop one by one12:16
TheMuedimitern: any more intelligent way is welcome12:17
dimiternTheMue, I see, ok12:18
TheMuedimitern: heyCompilerImNotDumbSoCast([]myType, mySubInterfaceSlice)12:18
dimiternTheMue, :) I'd suggest adding a comment like // Convert to addresses to []StateIPAddress12:27
TheMuedimitern: yeah, makes it more clear.12:27
dimiternTheMue, uhm.. s/to[1]//12:30
dimiternjust Convert addresses..12:30
TheMuedimitern: already thought so12:31
frankbandpb1: ping12:34
TheMuedimitern: voidspace: do you have a moment to review http://reviews.vapour.ws/r/2324/ ?12:55
voidspaceTheMue: after the MAAS call maybe12:55
dimiternTheMue, yeap12:56
TheMuegreat, thx12:56
bogdanteleagaalexisb, katco, perrito666: there's also this as a dependency for the centos PR, it doesn't seem to show up on rbt because the request didn't get automatically created13:42
voidspacedooferlad: dimitern: if you have time http://reviews.vapour.ws/r/2344/13:59
voidspacedimitern: hmmm... "space add" won't initially be supported by ec2 will it. So I'm not sure why we've implemented it (with the same semantics as create it seems)14:01
voidspaceah well14:01
voidspaceI'm not even entirely convinced we need both anyway...14:02
voidspacewe should just have add, if it already exists on the provider that one should be used - if it doesn't (and creating is supported) then it should be created14:03
voidspacepossibly the same for subnets14:03
dpb1hi frankban14:05
voidspaceadded as a comment to the model doc14:06
frankbandpb1: hi, could you please either give me write access to lp:juju-deployer or take a look, merge and release https://code.launchpad.net/~frankban/juju-deployer/guiserver-bundle-v4/+merge/267540 ?14:06
ericsnowdooferlad: would you mind adding a link to the PR to bug #1468349?14:09
mupBug #1468349: discoverySuite.TestDiscoverServiceLocalHost: invalid series for wily and vivid <centos> <test-failure> <unit-tests> <vivid> <wily> <juju-core:Fix Committed by dooferlad> <https://launchpad.net/bugs/1468349>14:09
dooferladericsnow: done14:13
ericsnowdooferlad: thanks!14:13
dimiternvoidspace, well, I think technically speaking, juju space add ... is a great way to "import" spaces from previous deployments14:16
voidspacedimitern: sure, and it would remain so14:16
voidspacedimitern: however, if there is no previous deployment then it would create them14:16
voidspacedimitern: I'm not suggesting any change in functionality, merely merging add and create14:17
dimiternvoidspace, because it allows you to discover tagged subnets as part of a space, and .. hmm.. although overwriting the list of subnets seems dangerous14:17
voidspacedimitern: doesn't add already have this problem14:18
voidspacedimitern: if you want to add an existing space then don't specify any subnets14:18
voidspacedimitern: the user will always want to either add or create, but they only need a single entry point to do it14:18
voidspacedimitern: whichever we do under the hood, from the user perspective they are adding a space to the current environment14:18
dimiternvoidspace, and, in case of AWS if you pre-tag subnets you effectively create spaces you can immediately use for bootstrapping14:19
dimiternvoidspace, so "add" only makes sense to take a name, yeah14:19
dimiternvoidspace, perhaps it can optionally take CIDRs and verify if they match, or if not - create it..14:20
TheMuevoidspace: thx for review. "failled" is no typo, the more l, the more it failed ;)14:21
voidspaceTheMue: hehe14:21
voidspacedimitern: we don't need to take CIDRs - let's not14:22
voidspacedimitern: I've added a comment to the model doc14:22
voidspacedimitern: I'd prefer we just merged add and create14:22
dimiternvoidspace, or if they match (or are a subset of) the actual tagged subnets just ignores them and adds all subnets14:22
voidspacedimitern: all existing subnets should be discovered14:22
voidspacedimitern: in fact - we probably don't want add at all14:23
voidspacedimitern: we probably want "space discover"14:23
dimiternvoidspace, not really14:23
voidspacedimitern: find and add all subnets and spaces14:23
dimiternvoidspace, I think the original idea was that "create" is an admin command14:23
voidspacedimitern: in case you only want a subset of available spaces and subnets?14:23
voidspacedimitern: it's not in the doc...14:23
dimiternvoidspace, while "add" can be used to "subclass" a bigger space14:24
voidspacedimitern: MVP is ec2 only, which doesn't support add at all14:24
dimiternvoidspace, :) it's in one of the other docs or numerous notes, I'm sure14:24
voidspaceand "space discover" would still be useful - find and add all spaces and subnets14:24
dimiternvoidspace, or space list --all14:24
voidspacedimitern: and I bet that would be all most users would ever use... with manually adding a subset being an esoteric and rarely used option14:25
dimiternvoidspace, or you mean more like "space import --existing"14:25
voidspace"space import"14:25
voidspacethe --existing flag is redundant14:25
voidspace"space list --all" would be cool too (later)14:26
dimiternyeah, it could instead be --from file.yaml14:26
dimiternvoidspace, agreed, I'll fix this to be reflected by the doc14:26
dimiternvoidspace, it should be easy to drop the parsing/tests around add with CIDRs14:27
voidspacewe don't need it at all for ec2, but yes14:28
dimiternvoidspace, and we could then just omit it from the MVP14:28
dimiternvoidspace, I'd like to keep it in net-cli, as soon we should be able to discover and add maas spaces14:29
TheMuedimitern: will you find a moment for a hopefully final review too?14:45
dimiternTheMue, did you see my comment about the EnvironWatcher ?14:54
TheMuedimitern: yeah, removed it14:55
TheMuedimitern: now everything is even smaller and more simple14:55
dimiternTheMue, hmm well I still see it in api/14:56
TheMuedimitern: oh, will take a look, thought I got them all14:57
perrito666ericsnow: wwitzel3 any of you around?14:57
ericsnowperrito666: sure14:57
wwitzel3perrito666: heyo14:57
perrito666ericsnow: a quick question (hopefully) I see you guys implemented a basic disk abstraction14:57
perrito666and that you have only types for persistent and scratch14:58
TheMuedimitern: sh*t, yes, killed it on server side but missed client, one moment14:58
ericsnowperrito666: right14:58
ericsnowperrito666: those are needed during provisioning14:58
perrito666what do I get with persistent, standard persistent or ssd persistent?14:58
ericsnowperrito666: I don't really remember (the GCE docs can tell you though)14:59
perrito666mmm I see that goes for an attached disk, Ill look more into creating un attached disks15:01
perrito666tx guys15:01
TheMuedimitern: I don't know which EnvironWatcher you mean. *cough* *cough*15:01
ericsnowperrito666: np15:01
wwitzel3perrito666: pd-standard15:05
wwitzel3perrito666: we don't explictly set the diskType in the initializeParams for the attachDisk call, so it defaults to pd-standard15:05
wwitzel3perrito666: if that was something you wanted to specify, you'd do so in the initializeParams on line 92 in gce/google/disk.go15:06
wwitzel3perrito666: you would just extend the DiskSpec to capture that information15:07
voidspacedooferlad: I replied to your review comment15:11
voidspacedooferlad: and that change was discussed with dimitern :-)15:11
voidspacewwitzel3: o/15:11
voidspacewwitzel3: I'm just off to exercise, so just waving as I go...15:12
dooferladvoidspace: cool. Fine by me!15:12
voidspacedooferlad: and thanks15:12
wwitzel3voidspace: o/15:13
perrito666wwitzel3: thanks man :) I am seeing how to code it properly to match the other volume implementation yet :)15:30
dimiternTheMue, you have a review15:53
TheMuedimitern: thx15:54
voidspaceright, bye all15:57
=== xwwt is now known as xwwt-afk
TheMuedimitern: in case of a remove error we currently bail out directly with the according error, no ErrTryAgain16:07
TheMuedimitern: that's why I removed the "retry" in the log16:08
TheMuedimitern: could change it to handle it like release errors before16:08
dimiternTheMue, why kill the worker if it can't remove an address?16:09
TheMuedimitern: good question, but then err = errors.Annotatef(err, "removing IP address %q", ipAddress.Value()) of last review is wrong16:10
TheMuedimitern: logging and returning ErrTryAgain is better then16:11
dimiternTheMue, although.. killing it with a some sort of ErrTryAgain with decreasing number of retries, finally killing it; with that on both release or remove errors, the runner can restart the worker, re-trigger the watcher and we have automatic-retry16:11
TheMuedimitern: eh, today the worker isn't killed when the returned error is an ErrTryAgain16:12
dimiternTheMue, there was something rogpeppe did about giving a worker and/or runner a callback to decide if an error is more important or not (i.e. ErrTryAgain can be less important than ErrTryAgain{retries-1} or {retries=0}16:13
TheMuedimitern: it simply waits until the next trigger and tries again. in case of other errors the worker will be stopped and restarted16:13
dimiternTheMue, yeah, it isn't, but doesn't that sound like a good way to simplify things like retrying?16:14
TheMuedimitern: it sounds to me more like complicating16:14
dimiternTheMue, so, if you have this you don't need to use a timer to trigger it16:15
TheMuedimitern: imho there's no advantage when restarting this worker16:15
dimiternTheMue, when restarted, it will call watch again and get all dead addresses16:15
TheMuedimitern: you mean because restarting always starts to clean?16:16
dimiternTheMue, yes, it's as good, if not better than calling Handle() yourself out-of-order to the watcher16:16
TheMuedimitern: ok, so in this case we don't need ErrTryAgain. we simply die when releasing or removing fails16:17
TheMuedimitern: at least we don't have to separate on worker side.16:18
TheMuedimitern: if cleanup fails we restart the worker16:18
dimiternTheMue, that's the simplest thing, but remember - if we don't manage to run handle() without an error for a long time it's just sitting there eating resources and log space16:18
TheMuedimitern: yes, optimally cleanup returns no error and e'thing is fine16:19
dimiternTheMue, that's why I'm suggesting to add a field or method on ErrTryAgain or ErrLimitedRetry if you prefer16:19
dimiternTheMue, which "grants you" a retry when called (e.g. decrementing the num retries at construction) - like a semaphore lock, but doesn't have to be16:20
dimiternTheMue, if retires run out it returns the last error instead of no error or an "acquired" ErrLimitedRetry16:22
dimiternTheMue, have you looked at moreImportant (or was it errMoreImportant)..16:22
TheMuedimitern: hmmm, so the worker manages this number? because the api is stateless16:22
dimiternTheMue, ok, to let's keep it simpler16:23
dimiternTheMue, I've looked at the more important stuff in detail and, although it can be useful, it's purely an optimization of the runner16:24
TheMuedimitern: ok16:27
dimiternTheMue, so, assuming Remove() doesn't return NotFound error or we ignore it (possibly logging it), it's fine like this I think16:28
TheMuedimitern: I would like to change it, that in case of a not found it's fine and in case of any other error we continue like at release errors (logging and setting the retry flag). so we at least try all and not bail out at the first error16:30
dimiternTheMue, I saw a few places gc.IsNil checks for errors - please, use jc.ErrorIsNil - the only places where it might not work is if something like [*]params.Error{} is returned instead of *params.Error(nil)16:33
dimiternespecially true for client-side apis16:33
TheMuedimitern: that should be the only place where I used it16:33
TheMuedimitern: at least I tried so, will check again16:34
dimiternTheMue, sure, I had issues with some cases like this - that's why we have things like .OneError() on ErrorResults16:37
TheMuedimitern: sound like an even better change then16:38
dimiternTheMue, cheers16:49
TheMuedimitern: your comment regarding waitForReleaseOp(), here I lost you.16:54
natefinchericsnow: you around?17:11
ericsnownatefinch: yep17:11
=== xwwt-afk is now known as xwwt
dimiternTheMue, which one? :)17:29
TheMuedimitern: for worker/addresser/worker_test.go line 10717:30
dimiternTheMue, there are two cases - waiting to happen and waiting not to happen17:30
dimiternTheMue, without creating a chan to listen for it, you can't guarantee it happened or not17:32
TheMuedimitern: not sure if we talk about the same. the comment is "There should be at least one check in the tests below where we do a similar loop, but for a shortAttempt, asserting it wasn't called."17:37
dimiternTheMue, yes, me too - the loop there waits for a ReleaseAddressOp on the dummy channel17:41
TheMuedimitern: it has no loop, it's right now a select with timeout17:42
dimiternTheMue, for the places where we know ReleaseAddress should not be called (or it should fail), we need a similar attempt, waiting for an op to make sure it wasn't called17:43
dimiternTheMue, oh, I see17:43
dimiternTheMue, that's due to RB's somewhat confusing folding diffs17:43
TheMuedimitern: ah, getting closer, maybe only had my troubles with the location of the comment.17:43
TheMuedimitern: hehe, exactly17:44
dimiternTheMue, this comment is actually mean for the loop below17:44
dimiternTheMue, in TestWorkerReleasesAlreadyDead17:45
TheMuedimitern: ah, and I see it here at the function declaration17:45
TheMuedimitern: that's what made me wonder17:45
perrito666mm, I wish my editor had a "make this struct satisfy that interface" shortcut17:46
TheMueperrito666: oh, cool idea for my plugin17:46
perrito666TheMue: tell me your plugin is for vi17:47
TheMueperrito666: I've once built one for  vi but right now it's for BBEdit17:47
perrito666ah osx, that explains17:49
perrito666aghh I think Ill have to write it myself and propose it17:49
TheMueperrito666: hehe17:49
perrito666is there a go tool that returns the methods for a given interface?17:50
dimiternTheMue, yeah it's a bit surprising17:50
* dimitern at eod17:50
TheMuedimitern: what? os x?17:50
natefinchquery katco19:01
natefinchkatco: I had the wrong day for my daughter's doctor's appointment... it's on thursday overlapping with our 1:1... can we move the 1:1?19:01
katconatefinch: sure, let me find a time19:01
katconatefinch: would you mind reviewing http://reviews.vapour.ws/r/2199/ and http://reviews.vapour.ws/r/2318/ ?19:22
katconatefinch: they're both windows/powershell specific, so you seem like the right person :)19:23
natefinchkatco: will do19:24
katconatefinch: ty19:24
katconatefinch: also, how's the unregister method coming?19:28
natefinchkatco: written but needs tests. I'm going to be working late tonight due to the late start this morning, too.  So, hopefully wrapped up today.19:33
katconatefinch: cool beans, thanks for the update19:33
natefinchkatco: I may be the best person on the Juju team to review those patches, but that doesn't mean I'm qualified.  I've never used powershell.  I'll do what I can, but I could easily miss simple things in the powershell code.19:43
katconatefinch: no worries, just do your best. PS is in many ways like C#19:43
katconatefinch: give a +1 instead of a +2 if you have to19:43
natefinchkatco: yeah, definitely reading it like it's bizzaro C# helps19:44
mupBug #1392786 changed: charm has no way to report error state <charms> <feature> <hooks> <juju-core:Fix Released> <https://launchpad.net/bugs/1392786>19:44
mupBug #1483879 opened: MAAS provider: terminate-machine --force or destroy-environment (without --force) don't DHCP release container IPs <landscape> <juju-core:New> <https://launchpad.net/bugs/1483879>20:14
mupBug #1483889 opened: OSX elcapitan support <juju-core:New> <https://launchpad.net/bugs/1483889>21:15
perrito666ericsnow: still around?21:27
ericsnowperrito666: yep21:27
perrito666ericsnow: I have read https://godoc.org/google.golang.org/api/compute/v1 but I cannot find a way to create a disk not attached to an instance, did you by any chance saw something like that?21:29
perrito666its like it is possible to create the attached one, and to use one already created21:29
perrito666but not to create a volume independent of an instance21:29
ericsnowperrito666: check out https://cloud.google.com/compute/docs/reference/latest/21:30
ericsnowperrito666: the "disks" section probably21:30
perrito666oh I see they seem to lack a nice implementation for that, how convenient ...not21:32
thumpermramm2: around?21:39
thumperor mramm21:42
* thumper suspects auto-reconnection21:42
davecheneymwhudson: i found the prolog stuff21:58
davecheneyit's quite hairy21:58
davecheneyso I sent https://go-review.googlesource.com/13570 instead21:58
mupBug #1324318 changed: build and host osx binaries  and update docs to point to them <feature> <osx> <juju-core:Fix Released by sinzui> <https://launchpad.net/bugs/1324318>22:00
mupBug #1483889 changed: OSX elcapitan support <osx> <juju-core:New> <https://launchpad.net/bugs/1483889>22:00
mupBug #1324318 opened: build and host osx binaries  and update docs to point to them <feature> <osx> <juju-core:Fix Released by sinzui> <https://launchpad.net/bugs/1324318>22:12
mupBug #1483889 opened: OSX elcapitan support <osx> <juju-core:New> <https://launchpad.net/bugs/1483889>22:12
mupBug #1324318 changed: build and host osx binaries  and update docs to point to them <feature> <osx> <juju-core:Fix Released by sinzui> <https://launchpad.net/bugs/1324318>22:15
mupBug #1483889 changed: OSX elcapitan support <osx> <juju-core:New> <https://launchpad.net/bugs/1483889>22:15
davecheneysuddenly -- netspilts!22:31
perrito666odd, I just saw the disconnects22:41
perrito666not the netsplit warning22:41
perrito666axw: let me know if you prefer to have the talk before or after22:42
perrito666since it is just you and I22:42
axwperrito666: hey, want to get started now?23:09
perrito666as you wish23:09
menn0thumper: can you take another look at http://reviews.vapour.ws/r/2328/ please?23:12
menn0thumper: http://reviews.vapour.ws/r/2328/diff/1-2/ is probably what you want23:14
thumperack, otp23:14
mwhudson_davecheney: yeah, v hairy23:21
mwhudson_i was looking at it because it needs to change for shared libraries on power (i think)23:21
mwhudson_mucho mucho hair23:21
perrito666thumper: tx for the reviews and follow up23:46

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