| mwhudson | davechen1y: just a peanut gallery comment but does that ppc64 crash happen with GOGC=off? | 00:51 |
|---|---|---|
| davechen1y | mwhudson: let me check | 01:15 |
| davechen1y | i already checked stack copying | 01:15 |
| davechen1y | and adjusted the min stack size to be higher | 01:15 |
| davechen1y | what it looks like to me is the receiver of the method is not being carried over in the case that there are no arguments | 01:15 |
| davechen1y | which is weird | 01:15 |
| davechen1y | but that'ts the case | 01:16 |
| mwhudson | does it happen all the time? | 01:16 |
| mwhudson | i 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 time | 01:17 |
| davechen1y | mwhudson: 100% of the time | 01:19 |
| mwhudson | oh | 01:19 |
| mwhudson | well that shooooooooouuuuuuuuuuuuulllllllllllllld make it easier to fix | 01:19 |
| davechen1y | github.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=0xc820484c20 | 01:19 |
| davechen1y | github.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=0xc820484c98 | 01:19 |
| davechen1y | receiver is 0x0 in the autogenerated method | 01:19 |
| davechen1y | then bonkers in the next frame down | 01:20 |
| davechen1y | opps | 01:20 |
| davechen1y | github.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=0xc820484c98 | 01:20 |
| davechen1y | github.com/juju/juju/cmd/juju/user.(*AddCommand).ConnectionEndpoint(0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...) <autogenerated>:17 +0x30 fp=0xc820484e00 sp=0xc820484df8 | 01:20 |
| davechen1y | this, not the other one | 01:20 |
| mwhudson | is a nil receiver plausible? | 01:20 |
| davechen1y | theorerically yes | 01:21 |
| davechen1y | calling through an interface, no | 01:21 |
| davechen1y | and in this case | 01:21 |
| davechen1y | the receiver was not nil | 01:21 |
| davechen1y | the weird thing is genwrapper is only called from two places in the compiler | 01:22 |
| davechen1y | it is called a _lot_ | 01:22 |
| mwhudson | right, and you haven't been able to reduce it? | 01:22 |
| davechen1y | it takes minutes every run | 01:22 |
| davechen1y | i am trying to reduce it | 01:22 |
| davechen1y | but everything on ppc takes so long | 01:23 |
| davechen1y | also, you cannot breakpoint an autogenerated wrapper | 01:23 |
| davechen1y | which is fucked | 01:23 |
| davechen1y | if you could | 01:23 |
| davechen1y | this would be easy | 01:23 |
| mwhudson | eh? | 01:23 |
| mwhudson | gdb and go, two great tastes etc | 01:23 |
| mwhudson | davechen1y: you can break point on an address btw | 01:24 |
| mwhudson | br *0xdeadbeef | 01:24 |
| davechen1y | github.com/juju/juju/cmd/envcmd.(*sysCommandWrapper).Run(0xc82023e580, 0xc82040b590, 0x0, 0x0) <autogenerated>:61 +0x9c fp=0xc820485300 sp=0xc8204852b8 | 01:24 |
| davechen1y | what's the address ? | 01:24 |
| mwhudson | does "p 'github.com/juju/juju/cmd/envcmd.(*sysCommandWrapper).Run'" show anything? | 01:25 |
| mwhudson | davechen1y: lolz | 01: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-ref | 01:49 |
| mwhudson | package github.com/juju/juju/...: signal: segmentation fault | 01:49 |
| davechen1y | /away | 03:53 |
| davechen1y | mwhudson: it prints something | 03:53 |
| davechen1y | but it's not that useful | 03:53 |
| davechen1y | i have some ideas for a repro now | 03:53 |
| davechen1y | i'll keep trying | 03:53 |
| wallyworld | axw: did you have 5 minutes? in standup hangout? | 04:13 |
| axw | wallyworld: be there in a sec | 04:13 |
| mup | Bug #1483492 opened: worker/storageprovisioner: machine agents attempting to attach environ-scoped volumes <juju-core:Triaged by axwalk> <https://launchpad.net/bugs/1483492> | 04:30 |
| aisrael | axw: building with your latest pull request | 05:12 |
| davechen1y | mwhudson: Thuper: got it! | 05:27 |
| davechen1y | i have a repro | 05:27 |
| davechen1y | which doesn't involve any juju code | 05:28 |
| davechen1y | https://github.com/golang/go/issues/12108 | 05:38 |
| mup | Bug #1483525 opened: Document action-set quoting <juju-core:New> <https://launchpad.net/bugs/1483525> | 06:00 |
| axw | aisrael: 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 unnecessary | 06:38 |
| davechen1y | mwhudson: i know what it is | 06:41 |
| mup | Bug #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 |
| voidspace | dimitern: ping | 07:07 |
| voidspace | dimitern: unping | 07:09 |
| dimitern | voidspace, (un)pong? :) | 07:11 |
| voidspace | dimitern: hah | 07:11 |
| voidspace | dimitern: I had a question, but I figured it out... | 07:11 |
| dimitern | voidspace, awesome! | 07:11 |
| voidspace | hah :-) | 07:12 |
| voidspace | not sure that counts as awesome, but thanks | 07:12 |
| voidspace | you must have had your coffee already this morning | 07:12 |
| mup | Bug #1298819 changed: Mechanism for retrieving relation names <feature> <relations> <juju-core:Won't Fix> <https://launchpad.net/bugs/1298819> | 07:12 |
| dimitern | voidspace, well, you know at least it's a great start of the morning | 07:13 |
| dimitern | voidspace, :) | 07:13 |
| voidspace | :-) | 07:14 |
| voidspace | apparently giving your smilies noses, the - in :-) , is a sign that you're old | 07:14 |
| voidspace | I just can't get used to :) though | 07:14 |
| voidspace | his eyes are too close to his mouth | 07:14 |
| dimitern | hehe | 07:16 |
| dimitern | I like :o) sometimes or just ;] when lazy | 07:16 |
| voidspace | :o) looks like a clown | 07:18 |
| anastasiamac | voidspace: really? it has something pig-ish to my eyes .. ") | 07:19 |
| voidspace | anastasiamac: heh, maybe | 07:20 |
| voidspace | clown pig | 07:20 |
| voidspace | dimitern: soooo.... | 07:20 |
| voidspace | dimitern: we have an issue with address allocation for containers | 07:21 |
| voidspace | dimitern: we get the subnet from the provider | 07:21 |
| voidspace | dimitern: and create it in state if necessary | 07:21 |
| voidspace | dimitern: and now subnets in state have space information | 07:21 |
| voidspace | dimitern: but address allocation for containers cares nothing for spaces yet | 07:21 |
| voidspace | dimitern: so if the subnet it discovers from the provider doesn't exist in state it will create it in the default space | 07:22 |
| voidspace | dimitern: I might just ignore this for my PR and I think we need a new card for it | 07:22 |
| voidspace | dimitern: because fixing that is quite a bit of thinking | 07:22 |
| voidspace | anastasiamac: good evening | 07:23 |
| voidspace | dimitern: 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 that | 07:24 |
| mup | Bug #1298819 opened: Mechanism for retrieving relation names <feature> <relations> <juju-core:Won't Fix> <https://launchpad.net/bugs/1298819> | 07:24 |
| anastasiamac | voidspace: evening :D | 07:25 |
| dimitern | voidspace, it does need a new card, yes | 07:30 |
| mup | Bug #1298819 changed: Mechanism for retrieving relation names <feature> <relations> <juju-core:Won't Fix> <https://launchpad.net/bugs/1298819> | 07:30 |
| dimitern | voidspace, but it needs spaces-in-constraints support first | 07:30 |
| voidspace | dimitern: fair enough | 07:31 |
| voidspace | dimitern: maybe it's time to start telling containers which subnet(s) to pick an address from instead of just picking an arbitrary one | 07:31 |
| voidspace | dimitern: at the moment we discover subnets and create them in state when we use them | 07:32 |
| voidspace | anyway | 07:32 |
| voidspace | I'll add a card for us to estimate | 07:32 |
| voidspace | dimitern: I added a card for updating AddSpace/CreateSpace to allow spaces with 0 subnets | 07:35 |
| voidspace | dimitern: not much work, but some work | 07:35 |
| dimitern | voidspace, cheers | 07:36 |
| dimitern | voidspace, as soon as we finish the mvp stuff, we should make sure the address allocation still works and fix it as needed | 07:37 |
| voidspace | dimitern: why does subnetsAPI define AllSpaces? | 07:38 |
| voidspace | and define it differently to the spacesAPI | 07:38 |
| dimitern | voidspace, to verify the spaces | 07:38 |
| dimitern | voidspace, is this the backing interface or? | 07:39 |
| voidspace | dimitern: apiserver/subnets/subnets.go | 07:39 |
| voidspace | dimitern: and it returns a list of space tags, whereas AllSpaces as used by "space list" returns full space information | 07:40 |
| dimitern | voidspace, well, we only need the tags for the CLI, but I guess there's no harm do get all the information if it's easier | 07:43 |
| voidspace | dimitern: ah, it's called ListSpaces on the spacesAPI | 07:44 |
| voidspace | dimitern: but the backing call is AllSpaces | 07:44 |
| voidspace | dimitern: slightly confusing but not actually wrong | 07:44 |
| dimitern | voidspace, 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 eventually | 07:49 |
| voidspace | ok | 07:49 |
| mwhudson | davechen1y: nice test case reduction | 07:57 |
| mwhudson | davechen1y: ooooh i think i know | 08:06 |
| davechen1y | mwhudson: do tell | 08:16 |
| davechen1y | it's something about the setup for duff copy | 08:16 |
| davechen1y | i'll see what russ says overnight | 08:16 |
| mwhudson | davechen1y: well duffzero is entered by bl | 08:16 |
| davechen1y | OH | 08:17 |
| mwhudson | yeah | 08:17 |
| davechen1y | that' won't help | 08:17 |
| mwhudson | and the function exits by b to the method | 08:17 |
| davechen1y | yup | 08:17 |
| davechen1y | i've hit that before in risc compilers | 08:17 |
| mwhudson | which blr's out... | 08:17 |
| davechen1y | and back to the callers caller | 08:17 |
| davechen1y | no | 08:18 |
| davechen1y | hang on | 08:18 |
| mwhudson | back to the generated method i think | 08:18 |
| davechen1y | hmm | 08:18 |
| davechen1y | i don't think that is it | 08:18 |
| mwhudson | i was clued in by a breakpoint being hit an impossible number of times | 08:18 |
| davechen1y | oh | 08:18 |
| davechen1y | so it's going around and around until it rolls off the end of the stack ? | 08:18 |
| mwhudson | er not sure | 08:19 |
| davechen1y | i think you're close | 08:19 |
| mwhudson | i think the stack pointer is wrong | 08:19 |
| davechen1y | but that isn't the whole story | 08:19 |
| davechen1y | the reciver is zeroed out | 08:19 |
| davechen1y | anyway | 08:19 |
| mwhudson | so the load from the stack at the end of the function is not reading the right bit of stack | 08:19 |
| mwhudson | this is all very familiar feeling, did we fix this bug in arm64 already or something? | 08:19 |
| davechen1y | i think you're right with the tail call thing | 08:20 |
| davechen1y | wrapper methods are designed to return to their caller | 08:20 |
| davechen1y | anyway, i know how to fix juju | 08:20 |
| davechen1y | even if it means a small patch | 08:20 |
| mwhudson | disable duffzero? | 08:21 |
| mwhudson | oh, or a juju change? | 08:21 |
| davechen1y | just disable duffzero | 08:22 |
| davechen1y | we'll live | 08:22 |
| mwhudson | i wonder if this fixes the GOARCH=amd64 go build runtime thing | 08:23 |
| mwhudson | that's probably a bit optimistic | 08:23 |
| davechen1y | probaby a long shot | 08:23 |
| davechen1y | mwhudson: anyway, not looking at it any more tonight | 08:26 |
| mwhudson | davechen1y: fair enough | 08:26 |
| davechen1y | thanks for the sugestions, i'll have a look tomorrow | 08:26 |
| mwhudson | i'll add a quick comment | 08:27 |
| dimitern | dooferlad, go fmt is sad: cmd/juju/commands/main.go | 08:27 |
| davechen1y | mwhudson: thanks for that comment | 08:35 |
| davechen1y | i think you're right | 08:35 |
| davechen1y | there was something similar a while back with somethign in the arm32 runtime | 08:35 |
| davechen1y | i don't rememver exactly | 08:35 |
| davechen1y | it was something like the kernel atomic helper | 08:36 |
| davechen1y | or something like that | 08:36 |
| davechen1y | NO | 08:36 |
| davechen1y | it was the gettls helper | 08:36 |
| davechen1y | which was sort of the same thing | 08:36 |
| davechen1y | needs a kernel helper to get the tls offset | 08:36 |
| mwhudson | ah yes | 08:36 |
| davechen1y | blah blah | 08:36 |
| davechen1y | tail call didn't call no more | 08:36 |
| mwhudson | that tickles faint bells | 08:36 |
| davechen1y | mwhudson: b377c9c6a9b720d0897d298652bebd3887ceeb46 | 08:38 |
| davechen1y | for your ref | 08:38 |
| davechen1y | i think | 08:38 |
| davechen1y | i might have fixed a similar problem | 08:39 |
| davechen1y | but i cannot find the ref | 08:39 |
| davechen1y | anyway | 08:39 |
| davechen1y | it's the same smell | 08:39 |
| davechen1y | arch's with link registers get tripped up by these shennaigans | 08:39 |
| mwhudson | i can't for the life of me find the code that compiles the tail call | 08:39 |
| davechen1y | genwrapper | 08:42 |
| mwhudson | i mean the liblink bit | 08:42 |
| davechen1y | i don't think it's there | 08:42 |
| mwhudson | sorry, i mean the bit that creats the b $foo instruction | 08:43 |
| mwhudson | i found it, then realized i was looking at arm64 | 08:43 |
| davechen1y | i think it's | 08:43 |
| davechen1y | compile/internal/gc/subr.go | 08:44 |
| davechen1y | generapper | 08:44 |
| mwhudson | case 11 in asm9.go | 08:44 |
| davechen1y | oh god | 08:44 |
| davechen1y | please don't make me look at asmN.go | 08:45 |
| mwhudson | heh ok | 08:45 |
| davechen1y | that's the worst | 08:45 |
| davechen1y | hmm, this doesn't look to magic | 08:46 |
| davechen1y | i don't think that's the right place to look | 08:46 |
| davechen1y | ok | 08:47 |
| davechen1y | i see, ADUFFCOPY is implemened in case 11 | 08:47 |
| davechen1y | so | 08:48 |
| davechen1y | arm and arm64 will be in the same boat | 08:48 |
| davechen1y | what are they doing I wonder | 08:48 |
| mwhudson | yeah i wonder how come they don't break | 08:48 |
| mwhudson | arm64 reloads the link register from the stack before the tail call | 08:53 |
| davechen1y | are you looking at zerorange in arm64 ? | 09:13 |
| davechen1y | or is that in asm7 ? | 09:13 |
| davechen1y | mwhudson: here's some math for ya | 09:24 |
| davechen1y | 8+frame+lo-8 | 09:24 |
| mwhudson | davechen1y: seems like that could do with some constant folding | 09:24 |
| mwhudson | davechen1y: i think i've found the ppc64le/arm64 point of difference | 09:25 |
| mwhudson | the ARET case in preprocess | 09:25 |
| davechen1y | i think it could be soethign else | 09:27 |
| davechen1y | have a look at zerorange in arm64 | 09:27 |
| davechen1y | and zerorange in ppc64 | 09:27 |
| davechen1y | arm64 laosd the SP into rt1 then adds the size to zero to it | 09:27 |
| davechen1y | ppc64 does not load the sp | 09:28 |
| davechen1y | oh | 09:28 |
| davechen1y | sorry | 09:28 |
| davechen1y | yes it does | 09:28 |
| davechen1y | it sets the source reg to be regsp | 09:28 |
| davechen1y | and the dest reg to rt1 with an add | 09:28 |
| davechen1y | we cannot do the same for arm64 because you cannot use Sp as the source or something | 09:29 |
| davechen1y | anyway | 09:29 |
| davechen1y | that's not it | 09:29 |
| davechen1y | i'm sure you're right about the tail call not reloading sp | 09:29 |
| mwhudson | davechen1y: disabling duffzero seems like the right fix | 09:42 |
| mwhudson | for the short term | 09:42 |
| mwhudson | and to a general lack of surprise it doesn't fix the "can't compile amd64 code" thing | 09:52 |
| voidspace | dimitern: we used to have space validation that would check all specified subnets have a valid CIDR and exist | 10:32 |
| voidspace | dimitern: I replaced the validation logic with a check that they exist in the transaction | 10:32 |
| voidspace | dimitern: so an invalid subnet still fails (because it doesn't exist in state) | 10:33 |
| voidspace | dimitern: the place to check for an invalid cidr would seem to be when adding a subnet | 10:33 |
| voidspace | dimitern: so I'm going to remove the failing test I have as a consequence | 10:33 |
| voidspace | dimitern: specifying an invalid cidr when adding a space still fails | 10:33 |
| voidspace | dimitern: but now it fails with subnet not found | 10:33 |
| voidspace | and there's a test for that already | 10:34 |
| dimitern | voidspace, hmm.. I have to take a look to remind myself.. | 10:37 |
| voidspace | dimitern: TestAddSpaceInvalidCIDR is gone | 10:38 |
| voidspace | dimitern: see https://github.com/juju/juju/compare/net-cli...voidspace:spaces-list-state?expand=1 | 10:38 |
| voidspace | dimitern: due to the removal of Space.validate and changes to AddSpace | 10:39 |
| voidspace | dimitern: 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 |
| dimitern | voidspace, hmm.. | 10:40 |
| dimitern | voidspace, so invalid CIDR I think should be validated before looking it up | 10:41 |
| voidspace | dimitern: why? | 10:41 |
| voidspace | dimitern: that's only sensible if we will be putting invalid CIDRs in state | 10:41 |
| voidspace | dimitern: which we really shouldn't! | 10:41 |
| dimitern | voidspace, well, it how about ";drop tables foo" :) | 10:41 |
| voidspace | dimitern: you're saying we have an injection vulnerability... | 10:42 |
| voidspace | dimitern: I really don't think so | 10:42 |
| dimitern | voidspace, but seriously, we shouldn't put invalid cidrs in state indeed | 10:42 |
| dimitern | voidspace, jk | 10:42 |
| voidspace | :-p | 10:42 |
| dimitern | voidspace, so you're saying adding a space with an empty list of subnets is now allowed? | 10:43 |
| voidspace | dimitern: yes | 10:43 |
| dimitern | voidspace, but ISTM non-empty list with possibly invalid cidrs leads to erraborted on addspace | 10:43 |
| voidspace | dimitern: yep | 10:43 |
| voidspace | dimitern: and there's a check for that | 10:43 |
| voidspace | dimitern: if we get ErrAborted that isn't due to the space already existing we check to see which subnet is missing | 10:44 |
| voidspace | dimitern: so we can report the error correctly | 10:44 |
| voidspace | dimitern: and there's a test for it, which passes | 10:44 |
| voidspace | well, missing subnet | 10:44 |
| voidspace | which is exactly the same thing (as an invalid cidr will always be missing) | 10:45 |
| dimitern | voidspace, indeed, sounds reasonable | 10:45 |
| voidspace | cool, thanks | 10:45 |
| dimitern | TheMue, you have review - sorry it took so long | 11:11 |
| TheMue | dimitern: great, thanks. | 11:11 |
| perrito666 | morning people | 11:15 |
| mup | Bug #1483672 opened: Allow charms to associate structured data with status <cloud-installer> <landscape> <juju-core:Triaged> <https://launchpad.net/bugs/1483672> | 11:40 |
| TheMue | dimitern: 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 one | 12:16 |
| TheMue | dimitern: any more intelligent way is welcome | 12:17 |
| dimitern | TheMue, I see, ok | 12:18 |
| TheMue | dimitern: heyCompilerImNotDumbSoCast([]myType, mySubInterfaceSlice) | 12:18 |
| dimitern | TheMue, :) I'd suggest adding a comment like // Convert to addresses to []StateIPAddress | 12:27 |
| TheMue | dimitern: yeah, makes it more clear. | 12:27 |
| dimitern | TheMue, uhm.. s/to[1]// | 12:30 |
| dimitern | just Convert addresses.. | 12:30 |
| TheMue | dimitern: already thought so | 12:31 |
| frankban | dpb1: ping | 12:34 |
| TheMue | dimitern: voidspace: do you have a moment to review http://reviews.vapour.ws/r/2324/ ? | 12:55 |
| voidspace | TheMue: after the MAAS call maybe | 12:55 |
| dimitern | TheMue, yeap | 12:56 |
| TheMue | great, thx | 12:56 |
| bogdanteleaga | alexisb, 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 created | 13:42 |
| voidspace | dooferlad: dimitern: if you have time http://reviews.vapour.ws/r/2344/ | 13:59 |
| voidspace | dimitern: 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 |
| voidspace | ah well | 14:01 |
| voidspace | I'm not even entirely convinced we need both anyway... | 14:02 |
| voidspace | we 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 created | 14:03 |
| voidspace | possibly the same for subnets | 14:03 |
| dpb1 | hi frankban | 14:05 |
| voidspace | added as a comment to the model doc | 14:06 |
| frankban | dpb1: 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 |
| ericsnow | dooferlad: would you mind adding a link to the PR to bug #1468349? | 14:09 |
| mup | Bug #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 |
| dooferlad | ericsnow: done | 14:13 |
| ericsnow | dooferlad: thanks! | 14:13 |
| dimitern | voidspace, well, I think technically speaking, juju space add ... is a great way to "import" spaces from previous deployments | 14:16 |
| voidspace | dimitern: sure, and it would remain so | 14:16 |
| voidspace | dimitern: however, if there is no previous deployment then it would create them | 14:16 |
| voidspace | dimitern: I'm not suggesting any change in functionality, merely merging add and create | 14:17 |
| dimitern | voidspace, because it allows you to discover tagged subnets as part of a space, and .. hmm.. although overwriting the list of subnets seems dangerous | 14:17 |
| voidspace | dimitern: doesn't add already have this problem | 14:18 |
| voidspace | dimitern: if you want to add an existing space then don't specify any subnets | 14:18 |
| voidspace | dimitern: the user will always want to either add or create, but they only need a single entry point to do it | 14:18 |
| voidspace | dimitern: whichever we do under the hood, from the user perspective they are adding a space to the current environment | 14:18 |
| dimitern | voidspace, and, in case of AWS if you pre-tag subnets you effectively create spaces you can immediately use for bootstrapping | 14:19 |
| dimitern | voidspace, so "add" only makes sense to take a name, yeah | 14:19 |
| dimitern | voidspace, perhaps it can optionally take CIDRs and verify if they match, or if not - create it.. | 14:20 |
| TheMue | voidspace: thx for review. "failled" is no typo, the more l, the more it failed ;) | 14:21 |
| voidspace | TheMue: hehe | 14:21 |
| voidspace | failllllllled | 14:21 |
| voidspace | dimitern: we don't need to take CIDRs - let's not | 14:22 |
| voidspace | dimitern: I've added a comment to the model doc | 14:22 |
| voidspace | dimitern: I'd prefer we just merged add and create | 14:22 |
| dimitern | voidspace, or if they match (or are a subset of) the actual tagged subnets just ignores them and adds all subnets | 14:22 |
| voidspace | dimitern: all existing subnets should be discovered | 14:22 |
| voidspace | dimitern: in fact - we probably don't want add at all | 14:23 |
| voidspace | dimitern: we probably want "space discover" | 14:23 |
| dimitern | voidspace, not really | 14:23 |
| voidspace | dimitern: find and add all subnets and spaces | 14:23 |
| dimitern | voidspace, I think the original idea was that "create" is an admin command | 14:23 |
| voidspace | dimitern: in case you only want a subset of available spaces and subnets? | 14:23 |
| voidspace | dimitern: it's not in the doc... | 14:23 |
| dimitern | voidspace, while "add" can be used to "subclass" a bigger space | 14:24 |
| voidspace | dimitern: MVP is ec2 only, which doesn't support add at all | 14:24 |
| dimitern | voidspace, :) it's in one of the other docs or numerous notes, I'm sure | 14:24 |
| voidspace | and "space discover" would still be useful - find and add all spaces and subnets | 14:24 |
| dimitern | voidspace, or space list --all | 14:24 |
| voidspace | dimitern: and I bet that would be all most users would ever use... with manually adding a subset being an esoteric and rarely used option | 14:25 |
| dimitern | voidspace, or you mean more like "space import --existing" | 14:25 |
| voidspace | "space import" | 14:25 |
| voidspace | the --existing flag is redundant | 14:25 |
| voidspace | "space list --all" would be cool too (later) | 14:26 |
| dimitern | yeah, it could instead be --from file.yaml | 14:26 |
| voidspace | bbiab | 14:26 |
| voidspace | hah | 14:26 |
| dimitern | voidspace, agreed, I'll fix this to be reflected by the doc | 14:26 |
| dimitern | voidspace, it should be easy to drop the parsing/tests around add with CIDRs | 14:27 |
| voidspace | we don't need it at all for ec2, but yes | 14:28 |
| dimitern | voidspace, and we could then just omit it from the MVP | 14:28 |
| dimitern | voidspace, I'd like to keep it in net-cli, as soon we should be able to discover and add maas spaces | 14:29 |
| TheMue | dimitern: will you find a moment for a hopefully final review too? | 14:45 |
| dimitern | TheMue, did you see my comment about the EnvironWatcher ? | 14:54 |
| TheMue | dimitern: yeah, removed it | 14:55 |
| TheMue | dimitern: now everything is even smaller and more simple | 14:55 |
| dimitern | TheMue, hmm well I still see it in api/ | 14:56 |
| TheMue | dimitern: oh, will take a look, thought I got them all | 14:57 |
| perrito666 | ericsnow: wwitzel3 any of you around? | 14:57 |
| ericsnow | perrito666: sure | 14:57 |
| wwitzel3 | perrito666: heyo | 14:57 |
| perrito666 | ericsnow: a quick question (hopefully) I see you guys implemented a basic disk abstraction | 14:57 |
| perrito666 | and that you have only types for persistent and scratch | 14:58 |
| TheMue | dimitern: sh*t, yes, killed it on server side but missed client, one moment | 14:58 |
| ericsnow | perrito666: right | 14:58 |
| ericsnow | perrito666: those are needed during provisioning | 14:58 |
| perrito666 | what do I get with persistent, standard persistent or ssd persistent? | 14:58 |
| ericsnow | perrito666: I don't really remember (the GCE docs can tell you though) | 14:59 |
| perrito666 | mmm I see that goes for an attached disk, Ill look more into creating un attached disks | 15:01 |
| perrito666 | tx guys | 15:01 |
| TheMue | dimitern: I don't know which EnvironWatcher you mean. *cough* *cough* | 15:01 |
| TheMue | :D | 15:01 |
| ericsnow | perrito666: np | 15:01 |
| wwitzel3 | perrito666: pd-standard | 15:05 |
| wwitzel3 | perrito666: we don't explictly set the diskType in the initializeParams for the attachDisk call, so it defaults to pd-standard | 15:05 |
| wwitzel3 | perrito666: if that was something you wanted to specify, you'd do so in the initializeParams on line 92 in gce/google/disk.go | 15:06 |
| wwitzel3 | perrito666: you would just extend the DiskSpec to capture that information | 15:07 |
| voidspace | dooferlad: I replied to your review comment | 15:11 |
| voidspace | dooferlad: and that change was discussed with dimitern :-) | 15:11 |
| voidspace | wwitzel3: o/ | 15:11 |
| voidspace | wwitzel3: I'm just off to exercise, so just waving as I go... | 15:12 |
| dooferlad | voidspace: cool. Fine by me! | 15:12 |
| voidspace | great | 15:12 |
| voidspace | dooferlad: and thanks | 15:12 |
| wwitzel3 | voidspace: o/ | 15:13 |
| perrito666 | wwitzel3: thanks man :) I am seeing how to code it properly to match the other volume implementation yet :) | 15:30 |
| dimitern | TheMue, you have a review | 15:53 |
| TheMue | dimitern: thx | 15:54 |
| voidspace | right, bye all | 15:57 |
| === xwwt is now known as xwwt-afk | ||
| TheMue | dimitern: in case of a remove error we currently bail out directly with the according error, no ErrTryAgain | 16:07 |
| TheMue | dimitern: that's why I removed the "retry" in the log | 16:08 |
| TheMue | dimitern: could change it to handle it like release errors before | 16:08 |
| dimitern | TheMue, why kill the worker if it can't remove an address? | 16:09 |
| TheMue | dimitern: good question, but then err = errors.Annotatef(err, "removing IP address %q", ipAddress.Value()) of last review is wrong | 16:10 |
| TheMue | dimitern: logging and returning ErrTryAgain is better then | 16:11 |
| dimitern | TheMue, 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-retry | 16:11 |
| TheMue | dimitern: eh, today the worker isn't killed when the returned error is an ErrTryAgain | 16:12 |
| dimitern | TheMue, 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 |
| TheMue | dimitern: it simply waits until the next trigger and tries again. in case of other errors the worker will be stopped and restarted | 16:13 |
| dimitern | TheMue, yeah, it isn't, but doesn't that sound like a good way to simplify things like retrying? | 16:14 |
| TheMue | dimitern: it sounds to me more like complicating | 16:14 |
| dimitern | TheMue, so, if you have this you don't need to use a timer to trigger it | 16:15 |
| TheMue | dimitern: imho there's no advantage when restarting this worker | 16:15 |
| dimitern | TheMue, when restarted, it will call watch again and get all dead addresses | 16:15 |
| TheMue | dimitern: you mean because restarting always starts to clean? | 16:16 |
| dimitern | TheMue, yes, it's as good, if not better than calling Handle() yourself out-of-order to the watcher | 16:16 |
| TheMue | dimitern: ok, so in this case we don't need ErrTryAgain. we simply die when releasing or removing fails | 16:17 |
| TheMue | dimitern: at least we don't have to separate on worker side. | 16:18 |
| TheMue | dimitern: if cleanup fails we restart the worker | 16:18 |
| dimitern | TheMue, 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 space | 16:18 |
| TheMue | dimitern: yes, optimally cleanup returns no error and e'thing is fine | 16:19 |
| dimitern | TheMue, that's why I'm suggesting to add a field or method on ErrTryAgain or ErrLimitedRetry if you prefer | 16:19 |
| dimitern | TheMue, which "grants you" a retry when called (e.g. decrementing the num retries at construction) - like a semaphore lock, but doesn't have to be | 16:20 |
| dimitern | TheMue, if retires run out it returns the last error instead of no error or an "acquired" ErrLimitedRetry | 16:22 |
| dimitern | TheMue, have you looked at moreImportant (or was it errMoreImportant).. | 16:22 |
| TheMue | dimitern: hmmm, so the worker manages this number? because the api is stateless | 16:22 |
| dimitern | TheMue, ok, to let's keep it simpler | 16:23 |
| dimitern | TheMue, I've looked at the more important stuff in detail and, although it can be useful, it's purely an optimization of the runner | 16:24 |
| TheMue | dimitern: ok | 16:27 |
| dimitern | TheMue, so, assuming Remove() doesn't return NotFound error or we ignore it (possibly logging it), it's fine like this I think | 16:28 |
| TheMue | dimitern: 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 error | 16:30 |
| dimitern | TheMue, 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 |
| dimitern | especially true for client-side apis | 16:33 |
| TheMue | dimitern: that should be the only place where I used it | 16:33 |
| TheMue | dimitern: at least I tried so, will check again | 16:34 |
| dimitern | TheMue, sure, I had issues with some cases like this - that's why we have things like .OneError() on ErrorResults | 16:37 |
| TheMue | dimitern: sound like an even better change then | 16:38 |
| dimitern | TheMue, cheers | 16:49 |
| TheMue | dimitern: your comment regarding waitForReleaseOp(), here I lost you. | 16:54 |
| natefinch | ericsnow: you around? | 17:11 |
| ericsnow | natefinch: yep | 17:11 |
| === xwwt-afk is now known as xwwt | ||
| dimitern | TheMue, which one? :) | 17:29 |
| TheMue | dimitern: for worker/addresser/worker_test.go line 107 | 17:30 |
| dimitern | TheMue, there are two cases - waiting to happen and waiting not to happen | 17:30 |
| dimitern | TheMue, without creating a chan to listen for it, you can't guarantee it happened or not | 17:32 |
| TheMue | dimitern: 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 |
| dimitern | TheMue, yes, me too - the loop there waits for a ReleaseAddressOp on the dummy channel | 17:41 |
| TheMue | dimitern: it has no loop, it's right now a select with timeout | 17:42 |
| dimitern | TheMue, 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 called | 17:43 |
| dimitern | TheMue, oh, I see | 17:43 |
| dimitern | TheMue, that's due to RB's somewhat confusing folding diffs | 17:43 |
| TheMue | dimitern: ah, getting closer, maybe only had my troubles with the location of the comment. | 17:43 |
| TheMue | dimitern: hehe, exactly | 17:44 |
| dimitern | TheMue, this comment is actually mean for the loop below | 17:44 |
| dimitern | TheMue, in TestWorkerReleasesAlreadyDead | 17:45 |
| TheMue | dimitern: ah, and I see it here at the function declaration | 17:45 |
| TheMue | dimitern: that's what made me wonder | 17:45 |
| perrito666 | mm, I wish my editor had a "make this struct satisfy that interface" shortcut | 17:46 |
| TheMue | perrito666: oh, cool idea for my plugin | 17:46 |
| perrito666 | TheMue: tell me your plugin is for vi | 17:47 |
| TheMue | perrito666: I've once built one for vi but right now it's for BBEdit | 17:47 |
| perrito666 | ah osx, that explains | 17:49 |
| perrito666 | aghh I think Ill have to write it myself and propose it | 17:49 |
| TheMue | perrito666: hehe | 17:49 |
| perrito666 | is there a go tool that returns the methods for a given interface? | 17:50 |
| dimitern | TheMue, yeah it's a bit surprising | 17:50 |
| * dimitern at eod | 17:50 | |
| TheMue | dimitern: what? os x? | 17:50 |
| natefinch | query katco | 19:01 |
| natefinch | lol | 19:01 |
| katco | ? | 19:01 |
| natefinch | katco: 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 |
| katco | natefinch: sure, let me find a time | 19:01 |
| katco | natefinch: would you mind reviewing http://reviews.vapour.ws/r/2199/ and http://reviews.vapour.ws/r/2318/ ? | 19:22 |
| katco | natefinch: they're both windows/powershell specific, so you seem like the right person :) | 19:23 |
| natefinch | katco: will do | 19:24 |
| katco | natefinch: ty | 19:24 |
| katco | natefinch: also, how's the unregister method coming? | 19:28 |
| natefinch | katco: 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 |
| katco | natefinch: cool beans, thanks for the update | 19:33 |
| natefinch | katco: 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 |
| katco | natefinch: no worries, just do your best. PS is in many ways like C# | 19:43 |
| katco | natefinch: give a +1 instead of a +2 if you have to | 19:43 |
| natefinch | katco: yeah, definitely reading it like it's bizzaro C# helps | 19:44 |
| mup | Bug #1392786 changed: charm has no way to report error state <charms> <feature> <hooks> <juju-core:Fix Released> <https://launchpad.net/bugs/1392786> | 19:44 |
| mup | Bug #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 |
| mup | Bug #1483889 opened: OSX elcapitan support <juju-core:New> <https://launchpad.net/bugs/1483889> | 21:15 |
| perrito666 | ericsnow: still around? | 21:27 |
| ericsnow | perrito666: yep | 21:27 |
| perrito666 | ericsnow: 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 |
| perrito666 | its like it is possible to create the attached one, and to use one already created | 21:29 |
| perrito666 | but not to create a volume independent of an instance | 21:29 |
| ericsnow | perrito666: check out https://cloud.google.com/compute/docs/reference/latest/ | 21:30 |
| ericsnow | perrito666: the "disks" section probably | 21:30 |
| perrito666 | oh I see they seem to lack a nice implementation for that, how convenient ...not | 21:32 |
| thumper | mramm2: around? | 21:39 |
| thumper | or mramm | 21:42 |
| * thumper suspects auto-reconnection | 21:42 | |
| davecheney | mwhudson: i found the prolog stuff | 21:58 |
| davecheney | it's quite hairy | 21:58 |
| davecheney | so I sent https://go-review.googlesource.com/13570 instead | 21:58 |
| mup | Bug #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 |
| mup | Bug #1483889 changed: OSX elcapitan support <osx> <juju-core:New> <https://launchpad.net/bugs/1483889> | 22:00 |
| mup | Bug #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 |
| mup | Bug #1483889 opened: OSX elcapitan support <osx> <juju-core:New> <https://launchpad.net/bugs/1483889> | 22:12 |
| mup | Bug #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 |
| mup | Bug #1483889 changed: OSX elcapitan support <osx> <juju-core:New> <https://launchpad.net/bugs/1483889> | 22:15 |
| davecheney | suddenly -- netspilts! | 22:31 |
| perrito666 | odd, I just saw the disconnects | 22:41 |
| perrito666 | not the netsplit warning | 22:41 |
| perrito666 | axw: let me know if you prefer to have the talk before or after | 22:42 |
| perrito666 | since it is just you and I | 22:42 |
| axw | perrito666: hey, want to get started now? | 23:09 |
| perrito666 | as you wish | 23:09 |
| menn0 | thumper: can you take another look at http://reviews.vapour.ws/r/2328/ please? | 23:12 |
| menn0 | thumper: http://reviews.vapour.ws/r/2328/diff/1-2/ is probably what you want | 23:14 |
| thumper | ack, otp | 23:14 |
| mwhudson_ | davecheney: yeah, v hairy | 23:21 |
| mwhudson_ | i was looking at it because it needs to change for shared libraries on power (i think) | 23:21 |
| mwhudson_ | mucho mucho hair | 23:21 |
| perrito666 | thumper: tx for the reviews and follow up | 23:46 |
| perrito666 | bbl | 23:54 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!