| TheMue | Good morning. | 07:04 |
|---|---|---|
| fwereade | TheMue, heyhey | 07:09 |
| TheMue | fwereade: Hiya | 07:09 |
| === TheMue_ is now known as TheMue | ||
| davecheney | good morning gentle ment | 07:32 |
| davecheney | gentlemen | 07:32 |
| fwereade | davecheney, heyhey | 07:32 |
| fwereade | TheMue, ok, I am looking in detail at the "firewaller bug" | 07:33 |
| fwereade | TheMue, have you played around with that at all yourself? | 07:33 |
| TheMue | davecheney: Good mroning | 07:33 |
| TheMue | fwereade: No, only discussed it so far with Aram. | 07:34 |
| fwereade | TheMue, can you recall anything specific about how he was characterising it? | 07:34 |
| davecheney | https://codereview.appspot.com/6856120/ | 07:34 |
| davecheney | ^ version III of cross series bootstapping | 07:34 |
| TheMue | fwereade: It only happened after has change of the firewaller. So maybe it's better to wait until Monday to talk with him about those changes. | 07:34 |
| fwereade | TheMue, so the answer to the question I asked is "no"? :) | 07:35 |
| TheMue | fwereade: He talked about a kind of race situation, but only in global mode. | 07:35 |
| fwereade | TheMue, ah, hmm, ok, it looks like I'm stuck on something different then | 07:36 |
| fwereade | TheMue, I think I fixed that one | 07:36 |
| TheMue | fwereade: Hey, not so fast. I need my time to write here in the bottom line while you're asking new questions above. ;) | 07:36 |
| fwereade | TheMue, sorry :) | 07:36 |
| TheMue | fwereade: NP, had a smiley at the end. :D | 07:37 |
| TheMue | fwereade: His change after the watcher changed has been to extract a larger part of the firewallers loop code into an own method. But there it works differently, because the lifecycle states of the units returned by the watcher has to be checked. | 07:38 |
| fwereade | TheMue, ok, I don't *think* that is currently an issue | 07:39 |
| fwereade | TheMue, I'm pretty certain I have tweaked the firewaller such that it's responding to global mode events nicely | 07:39 |
| fwereade | TheMue, now I'm confused that a unit watch appears not to be firing when it should | 07:39 |
| TheMue | fwereade: Is it already submitted or a CL? | 07:39 |
| fwereade | TheMue, thank you for the context though | 07:40 |
| fwereade | TheMue, it's Arams 120 CL | 07:40 |
| fwereade | TheMue, that niemeer asked me to finish off and land | 07:40 |
| TheMue | fwereade: You're welcome, I'm interested in it too as the Firewaller has initially been my baby. ;) | 07:40 |
| TheMue | fwereade: Do you please have the URL for me, to quickly jump in? | 07:41 |
| fwereade | TheMue, I haven't even pushed it yet, just a mo | 07:41 |
| TheMue | fwereade: OK | 07:41 |
| fwereade | TheMue, hm, sorry, it'll be inconvenient to push right now | 07:42 |
| TheMue | fwereade: OK, I think I get a notification when you'll push it, so then I can jump in again. | 07:43 |
| fwereade | TheMue, I'm just pushed to lp:~fwereade/juju-core/aram-firewaller-sketch if you want to take a look (and, ideally, paste me the output of TestGlobalModeRestartPorts on your machine?) | 07:45 |
| fwereade | TheMue, it has a load of random debug prints pooed in there | 07:45 |
| TheMue | fwereade: Yes, one moment. | 07:46 |
| fwereade | TheMue, heyyyyyyyyy I know what this looks like | 08:03 |
| fwereade | TheMue, maybe | 08:03 |
| fwereade | TheMue, if a client asks for a watch on a channel and isn't receiving, it just blocks the whole watcher | 08:04 |
| TheMue | fwereade: So, after the mgo/mongodb update test is running and TestGlobalModeRestartPorts passes w/o any messages. Strange. | 08:16 |
| fwereade | TheMue, gaah TestGlobalModeRestartPortCounts | 08:16 |
| fwereade | TheMue, sorry | 08:16 |
| TheMue | fwereade: Passes too, also sorry. ;) | 08:17 |
| fwereade | TheMue, ok, that's weird :) | 08:19 |
| TheMue | fwereade: Indeed. | 08:20 |
| rogpeppe | davecheney, fwereade, TheMue: morning! | 08:51 |
| TheMue | rogpeppe: Hi | 08:51 |
| fwereade | rogpeppe, heyhey | 08:51 |
| davecheney | morning all | 08:52 |
| rogpeppe | davecheney: glad to hear the TLS update worked for you... | 08:54 |
| davecheney | rogpeppe: it's fn' awesome | 08:57 |
| davecheney | so much faster than ssh | 08:57 |
| rogpeppe | davecheney: i'm a bit surprised by that actually | 08:58 |
| rogpeppe | davecheney: the network latency should be approximately similar | 08:58 |
| davecheney | much less round trips | 08:58 |
| rogpeppe | davecheney: to set up the connection? | 08:58 |
| rogpeppe | davecheney: i didn't realise ssh was inefficient that way | 08:59 |
| davecheney | 3 round trips for tcp handshake, then a dozen for ssh handshake, then 2 for tcp channel, then the mgo setup on top of that | 09:05 |
| davecheney | i'd bet pesos to dollars that tls handshaking is more effective than ssh tunneling | 09:06 |
| rogpeppe | davecheney: TLS handshake is standard diffie hellman, i think, which is one round trip. there may be more too though, to negotiate the crypto. | 09:08 |
| davecheney | depends on the size of the cert | 09:09 |
| davecheney | but it is much more efficient | 09:09 |
| davecheney | so, on high latency the difference is remarkable | 09:09 |
| davecheney | also, shit: https://code.google.com/p/go/source/detail?r=697f36fec52ceaabc2208d28918fc34787b617bb | 09:10 |
| davecheney | spent three evenings working on this one | 09:10 |
| rogpeppe | davecheney: ah bugger. what a pity. have you worked out what the problem was yet? | 09:23 |
| davecheney | issue 599 | 09:25 |
| davecheney | 64bit atomics need to be 8 byte aligned | 09:25 |
| davecheney | i should have remembered earlier | 09:26 |
| fwereade | rogpeppe, TheMue: I had believed that the watcher would panic if you tried to unwatch something you hadn't watched... is that not the case? | 09:34 |
| rogpeppe | fwereade: you're talking about state/watcher? | 09:35 |
| fwereade | rogpeppe, yeah | 09:35 |
| rogpeppe | fwereade: doesn't look like it from a brief glance at the source | 09:36 |
| rogpeppe | fwereade: looks like it's idempotent | 09:36 |
| fwereade | rogpeppe, indeed, I just could have sworn it was like that once | 09:36 |
| fwereade | rogpeppe, probably just fever dreams | 09:36 |
| rogpeppe | fwereade: it would be an easy change to make | 09:36 |
| fwereade | rogpeppe, indeed, I may mention it to niemeyer | 09:37 |
| TheMue | fwereade: One moment, daughter is here, will be back in a few seconds. | 09:38 |
| fwereade | rogpeppe, TheMue: could I get a trivial +1 on https://codereview.appspot.com/6856122 please? | 09:38 |
| fwereade | rogpeppe, that panicing would have made it *much* quicker to find that issue, I think | 09:39 |
| TheMue | fwereade: *click* | 09:39 |
| rogpeppe | fwereade: it's a pity that a document id is an interface{} | 09:41 |
| rogpeppe | fwereade: in fact, where do we use a document id that's not a string? | 09:42 |
| TheMue | fwereade: Has the Unwatch() changed to take an id instead of a Life. | 09:43 |
| rogpeppe | fwereade: ha, the answer is "nowhere" | 09:44 |
| rogpeppe | fwereade: probably that's only since you've changed machine id to string | 09:44 |
| rogpeppe | fwereade: that would've caught the problem too | 09:45 |
| rogpeppe | fwereade: no, it was just a mistake | 09:45 |
| rogpeppe | s/fwereade/TheMue/ | 09:45 |
| TheMue | rogpeppe: OK | 09:45 |
| fwereade | rogpeppe, also, relations have an "Id" field that we use internally but a mgo "_id" called Key | 09:47 |
| fwereade | rogpeppe, so, yes from the watcher POV we could still use strings | 09:47 |
| fwereade | rogpeppe, but it's muddy | 09:48 |
| rogpeppe | fwereade: all the tests run fine with strings | 09:48 |
| fwereade | TheMue, the Unwatch never needed a life | 09:48 |
| rogpeppe | fwereade: but i see what you mean | 09:48 |
| fwereade | rogpeppe, cool | 09:48 |
| rogpeppe | fwereade: could you make a test for this? | 09:48 |
| fwereade | rogpeppe, I broached the subject with niemeyer, he seemed -1 on changing it | 09:48 |
| rogpeppe | fwereade: fairy nuff | 09:48 |
| TheMue | fwereade: Yep, I took a look and saw that it only hasn't been catched by the compiler due to the interface{}. | 09:49 |
| fwereade | rogpeppe, that's an interesting philosophical question | 09:49 |
| fwereade | rogpeppe, if I were to write a test for this I would also feel obliged to write a similar one for every watcher | 09:49 |
| rogpeppe | fwereade: ha | 09:49 |
| rogpeppe | fwereade: maybe you should | 09:50 |
| rogpeppe | fwereade: maybe they're broken similarly... | 09:50 |
| davecheney | rogpeppe: https://codereview.appspot.com/6856120/ | 09:56 |
| rogpeppe | davecheney: looking | 09:58 |
| rogpeppe | davecheney: is configTest.series ever going to be set to something other than version.Current.Series? | 10:00 |
| rogpeppe | davecheney: (or "") | 10:00 |
| davecheney | rogpeppe: yes | 10:00 |
| rogpeppe | davecheney: how so? | 10:01 |
| davecheney | bootstrapping from quantal -> precise | 10:01 |
| rogpeppe | davecheney: so is the plan to add an argument to config.New? | 10:02 |
| rogpeppe | davecheney: thinking about it, i'm not convinced that default-series should default to anything | 10:03 |
| rogpeppe | davecheney: but i'm open to contrary arguments | 10:04 |
| davecheney | rogpeppe: id' say it should always default to precise | 10:04 |
| davecheney | (insert current LTS release) | 10:04 |
| davecheney | this is all in aide of https://codereview.appspot.com/6851081/ | 10:04 |
| rogpeppe | davecheney: ignoring version.Current.Series? | 10:04 |
| davecheney | after a very long talk with gustavo | 10:04 |
| davecheney | i still have no idea how to test this | 10:04 |
| davecheney | other than the sledgehammer approaches I have previously suggested | 10:04 |
| rogpeppe | davecheney: i don't think the configTest.series parameter is necessary. you can just test against whatever the default should be, in configTest.check. | 10:06 |
| rogpeppe | s/parameter/field/ | 10:06 |
| rogpeppe | davecheney: i'll have a think about how you might want to test cross-series bootstrapping | 10:07 |
| davecheney | thanks rogpeppe, i'm really stuck | 10:07 |
| fwereade | brb | 10:21 |
| dimitern | hey, can I depend on deterministic ordering in slices? (when testing building a slice by appending) | 10:22 |
| dimitern | and what's the best way to test this, given []Type as a result and having another - what type of assert is best? | 10:23 |
| davecheney | dimitern: yes, slice are ordered | 10:25 |
| rogpeppe | dimitern: yes | 10:25 |
| rogpeppe | and DeepEquals | 10:26 |
| rogpeppe | dimitern: assuming the types are not opaque | 10:26 |
| dimitern | rogpeppe, davecheney: ok, 0x | 10:26 |
| rogpeppe | dimitern: have you read this article? http://research.swtch.com/godata | 10:27 |
| dimitern | rogpeppe: what do you mean opaque? no I'll read it now | 10:29 |
| rogpeppe | dimitern: i mean you shouldn't use DeepEqual if the values have unexported fields | 10:29 |
| dimitern | rogpeppe: right, that's the case | 10:30 |
| dimitern | rogpeppe: but does it matter, since the're all in the same package? | 10:30 |
| rogpeppe | dimitern: it depends what kind of equality you're interested in | 10:30 |
| rogpeppe | dimitern: what are you actually trying to assert? | 10:31 |
| dimitern | rogpeppe: well, I have AllFlavors() call, which returns []Flavor and error, and I called AddFlavor() twice with fl1, fl2, and now it seems []Flavor{fl1,fl2} != the result, but maybe I'm returning them in reverse order | 10:32 |
| davecheney | dimitern: if your flavors live in a map | 10:33 |
| davecheney | possibly | 10:33 |
| rogpeppe | dimitern: i didn't realise a Flavor had unexported fields | 10:33 |
| dimitern | davecheney: yes, I have map[string]Flavor | 10:33 |
| davecheney | so if you're doing something lie | 10:33 |
| davecheney | for k, v := range flavors { if k == "something" { append(...) } } | 10:34 |
| davecheney | then the order your iterate over flavors is random | 10:34 |
| dimitern | rogpeppe: it's a local type, Flavor has entity *nova.Entity and detail *nova.FlavorDetail - one or both can be specified | 10:34 |
| dimitern | davecheney: ok, is there something similar to pythons's dict.values(), without iterating over the map? | 10:35 |
| rogpeppe | dimitern: it sounds like DeepEqual might be appropriate in this particular case. | 10:35 |
| davecheney | rogpeppe: isn't there a SliceEquals or something ? | 10:35 |
| davecheney | for set equality ? | 10:36 |
| dimitern | rogpeppe: tried DeepEquals, but does not work if the order is different | 10:36 |
| dimitern | davecheney: not sure, I'll try | 10:36 |
| rogpeppe | davecheney: i don't think so | 10:36 |
| davecheney | shitter | 10:37 |
| rogpeppe | dimitern: one conventional way of doing this is to sort the slices before comparing them | 10:37 |
| dimitern | rogpeppe: how? | 10:37 |
| dimitern | davecheney: no, SliceEquals does not exists | 10:37 |
| rogpeppe | dimitern: look at the sort package, define Less, Swap and Len methods on a new type sortedFlavors []Flavor | 10:37 |
| rogpeppe | dimitern: (one line each) | 10:38 |
| dimitern | rogpeppe: ok, I'll check it out | 10:38 |
| rogpeppe | dimitern: then sort each slice, by type-converting it to sortedFlavors, then use whatever equality operation you feel is appropriate | 10:38 |
| dimitern | rogpeppe: that seems like a lot of work to compare 2 slices - how about just iterating over them and checking each | 10:40 |
| rogpeppe | dimitern: it's probably more work to do that, tbh | 10:41 |
| rogpeppe | dimitern: defining the sort methods is mechanical and easy | 10:42 |
| rogpeppe | dimitern: because you don't just want to declare two slices | 10:42 |
| rogpeppe | dimitern: you want to make sure they're exactly the same | 10:42 |
| dimitern | rogpeppe: I'm looking at the sort pkg now | 10:43 |
| rogpeppe | dimitern: regardless of order | 10:43 |
| rogpeppe | dimitern: tbh, if you're adding your own flavors and checking them, it's probably overkill to check the whole Flavor | 10:44 |
| dimitern | rogpeppe: the docs for Soft says: The sort is not guaranteed to be stable. - what's that? | 10:44 |
| rogpeppe | dimitern: it means that equal items won't necessarily end up in the original order | 10:44 |
| rogpeppe | dimitern: you could just check some aspect of it that you know about | 10:44 |
| rogpeppe | dimitern: e.g. a name | 10:44 |
| dimitern | rogpeppe: exactly, I have only 2 items and they can have at most 2 orderings | 10:45 |
| rogpeppe | dimitern: in which case, you could just compare and swap, then DeepEqual | 10:45 |
| rogpeppe | dimitern: same difference | 10:45 |
| dimitern | rogpeppe: yep, that seems the easiest | 10:45 |
| rogpeppe | dimitern: if even, just compare the first element of each slice, then swap one if they're different, then compare both slices | 10:46 |
| rogpeppe | s/if even/or even/ | 10:46 |
| dimitern | rogpeppe: yeah, this works and since it's a simple case I'll leave it to this | 10:46 |
| dimitern | mgz: or w7z: ? | 11:00 |
| mgz | hey dimitern | 11:01 |
| dimitern | mgz: hey :) i was not sure which one you're watching - mumble? | 11:01 |
| mgz | nearly there, left my headset somewhere silly... | 11:02 |
| dimitern | wallyworld: ^^ | 11:05 |
| wallyworld | ok | 11:05 |
| fwereade | rogpeppe, TheMue: I've updated https://codereview.appspot.com/6856122 -- it should still be pretty trivial | 11:17 |
| TheMue | fwereade: Already lookin' ;) | 11:17 |
| fwereade | rogpeppe, TheMue: the watcher package change means that the MUW tests fail reasonably comprehensibly when the dont-unwatch-life-values fix | 11:17 |
| fwereade | ...is not there | 11:18 |
| TheMue | fwereade: Aaaaah, that's what missing, just wanted to ask. | 11:19 |
| rogpeppe | fwereade: LGTM | 11:21 |
| fwereade | rogpeppe, awesome, thanks | 11:22 |
| fwereade | rogpeppe, I'm printing the channels because it really helped me to be able to just print suspicious-looking channels when I watched, and that was very useful in the early stages when I was still zeroing in on a fix | 11:24 |
| fwereade | rogpeppe, it may not be so useful now, i guess | 11:24 |
| fwereade | rogpeppe, thanks | 11:24 |
| rogpeppe | fwereade: i think that if you're debugging, you'll add prints where necessary. an isolated channel pointer print isn't much help really. | 11:24 |
| fwereade | rogpeppe, the isolated panic is kinda handy though, because you can just put your prints in state/watcher and not have to worry about understanding the watcher package | 11:26 |
| rogpeppe | fwereade: true, the channel is an external thing, isn't it. fair enough, why not leave it? | 11:26 |
| fwereade | rogpeppe, cheers | 11:27 |
| fwereade | TheMue, any comments? | 11:28 |
| TheMue | fwereade: You'll get your LGTM in a few moments. ;) | 11:28 |
| fwereade | TheMue, <3 | 11:29 |
| TheMue | fwereade: You've got it. | 11:31 |
| fwereade | TheMue, the channel pointer print is the difference between (1) debugging by sprinking a few prints in state/watcher.go (or whatever the client is) and (2) opening up and coming to understand the watcher package well enough to determine that you need to print the channels around the pointw where I panic | 11:35 |
| fwereade | TheMue, and (2) also includes all of (1) | 11:35 |
| fwereade | TheMue, as rogpeppe pointed out, the channel is a watch param just like coll and id | 11:35 |
| fwereade | TheMue, revno is not interesting here, so I didn't print that | 11:36 |
| fwereade | TheMue, but the specific channel *is* interesting, because the watcher can have N watches for a given key | 11:36 |
| fwereade | TheMue, and the challenge becomes figuring out which watch is unmatched | 11:36 |
| fwereade | TheMue, do you still object to panicing with the chan? | 11:37 |
| fwereade | niemeyer, heyhey | 11:38 |
| niemeyer | Good morning! | 11:39 |
| TheMue | fwereade: OK, reasonable. | 11:39 |
| fwereade | TheMue, cheers | 11:39 |
| TheMue | niemeyer: Hiya. | 11:39 |
| rogpeppe | niemeyer: morning! | 11:40 |
| niemeyer | fwereade, TheMue, rogpeppe: Heyas! | 11:40 |
| jam | mgz, dimitern, wallyworld: Something landed recently in trunk broke the build: https://pastebin.canonical.com/79484/ | 11:54 |
| jam | wallyworld: I think it was your change to return 'nil' for objects when there has been an error. | 11:54 |
| wallyworld | yes :-( | 11:55 |
| jam | If we want to keep 'nil', we need to change the return to "*Object" rather than "Object" | 11:55 |
| jam | since pointers can be nil | 11:55 |
| jam | but a struct cannot. | 11:55 |
| wallyworld | hmmm. i guess that's the correct thing to do | 11:55 |
| jam | wallyworld: but also note, I'm not sure that adding the 'if' statement *improves* the code. | 11:55 |
| jam | wallyworld: I tried sending an email this morning, and tried again just now. | 11:55 |
| wallyworld | i was doing what the code review called for | 11:55 |
| wallyworld | let me check, i missed the email | 11:56 |
| jam | wallyworld: Oh, I understand that, I'm disagreeing with rogpeppe not you. I approved your change, but questioned the motive. | 11:56 |
| wallyworld | maybe i need to return an empty struct? | 11:56 |
| jam | even with that, we should be careful to run 'go test ./...' before committing on trunk, since we don't have a bot yet. | 11:56 |
| wallyworld | yeah, sorry | 11:56 |
| wallyworld | i thought i had but clearly not | 11:56 |
| jam | wallyworld: IMO, returning an empty struct is just as bad as a partial one. Since they have an object that is 'valid'. | 11:57 |
| jam | In which case, just return what you have, and set err like you were doing. | 11:57 |
| jam | Or, we change everything to pointers. | 11:57 |
| jam | anyway, i'm not really working today | 11:57 |
| jam | :L) | 11:57 |
| wallyworld | your preference? | 11:57 |
| jam | :) | 11:57 |
| rogpeppe | jam: was that the named-return-variable discussion? | 11:57 |
| jam | wallyworld: *mine* is to not repeat the if err != nil, check in the function, that the callers are going to have to do anyway. | 11:57 |
| jam | rogpeppe: sorry, I do want to discuss this, but my son needs to play Lego Batman... :) | 11:58 |
| jam | Can we chat maybe later, or on Monday/Tues? | 11:58 |
| rogpeppe | jam: nananananananana batman! | 11:58 |
| rogpeppe | jam: sure | 11:58 |
| wallyworld | jam: i'll fix the build and we can discuss next week | 11:58 |
| niemeyer | wallyworld: If in doubt, I'd suggest returning a pointer | 12:15 |
| niemeyer | wallyworld: That's appropriate in most cases | 12:16 |
| niemeyer | wallyworld: Of course, and returning nil consistently when err != nil | 12:16 |
| wallyworld | niemeyer: for now, i'm just reverting the changes to fix the breakages, and will tackle properly next week once we have consensus | 12:16 |
| wallyworld | since it's late here now and i'm tired | 12:16 |
| niemeyer | wallyworld: Sure, I'm just saying this is a well known concept | 12:16 |
| niemeyer | wallyworld: If you go over existing code, both ours and upstream, you'll see a clear pattern | 12:17 |
| wallyworld | jam makes a good point though about the functions doing their own if err != nil checks | 12:17 |
| rogpeppe | wallyworld: it's definitely conventional to return a zero value when err != nil, pointer or not. that was the reason for my comments. | 12:17 |
| wallyworld | agree pointer is best | 12:17 |
| niemeyer | Yes, and returning partial data when err != nil is not okay | 12:18 |
| wallyworld | i wonder, if err != nil, why does the returned value need to be zero? | 12:18 |
| wallyworld | wouldn't it be ignored anyway? | 12:18 |
| niemeyer | There are very rare exceptions to this rule, which confirms the rule.. Those are related to cases where it's important to observe the partial values, and the partial values are deterministic in such cases. | 12:19 |
| wallyworld | it seems like it's just adding complexity | 12:19 |
| wallyworld | lots of apis in other languages say, if there's an error, such and uch is undefined | 12:19 |
| niemeyer | wallyworld: It's one more layer to prevent bad usage | 12:19 |
| niemeyer | wallyworld: yes, that's not the case here | 12:20 |
| niemeyer | wallyworld: We consistently return a zero value when errors are found | 12:20 |
| wallyworld | ok. it costs to do that though | 12:20 |
| niemeyer | wallyworld: I haven't spent much so far :) | 12:20 |
| wallyworld | well, each api call needs repetitive blocks of code "if err != nil ..." | 12:21 |
| wallyworld | makes it messy, especially if the caller also does "if err != nul..." | 12:21 |
| niemeyer | wallyworld: We do err != nil all the time | 12:21 |
| niemeyer | wallyworld: It's pretty clear and cheap | 12:21 |
| wallyworld | so we have 2 places doing the error checking - the caller and the callee | 12:21 |
| rogpeppe | wallyworld: you only need that if you're returning something that may not be zero | 12:22 |
| niemeyer | wallyworld: We have tons of places doing error checking | 12:22 |
| rogpeppe | wallyworld: in the case we're talking about, we were returning a value that may or may not have been partially filled in | 12:22 |
| wallyworld | what if the semantics of the call say - "if there's an error, don't use the returned vsalue" | 12:22 |
| niemeyer | wallyworld: That's not even on the list of things that actually burned my own time over the past few years | 12:22 |
| niemeyer | wallyworld: That still wouldn't change the fact that there's a strong convention that is useful as it prevents crack from flowing through | 12:23 |
| wallyworld | ok, no problem. it does seem like code duplication for little gain, but that's just IMO :-) | 12:24 |
| wallyworld | s/code duplication/excessive boiler plate | 12:24 |
| niemeyer | wallyworld: I'd appreciate seeing an example | 12:24 |
| niemeyer | wallyworld: Of what you claim is code duplication | 12:24 |
| wallyworld | ok, i'll do a pastebin | 12:24 |
| niemeyer | wallyworld: Doing if err != nil { return nil, err }, or doing if err != nil { return foo, err } | 12:25 |
| niemeyer | wallyworld: Doesn't really cost much, and the former one is both less error prone, and more clear in intention | 12:25 |
| rogpeppe | niemeyer: in this case, i think it was between: if err != nil { return nil, err }; return foo, nil; and a plain return. | 12:26 |
| wallyworld | https://pastebin.canonical.com/79488/ | 12:27 |
| wallyworld | see how the caller and calle both do the if err != nil | 12:27 |
| wallyworld | the function has no need to | 12:27 |
| wallyworld | if just clutters the code | 12:27 |
| wallyworld | why not just return data, err | 12:27 |
| wallyworld | who cares if data is partially populated, err will tell that something is wrong and not to use it | 12:28 |
| niemeyer | wallyworld: pastebin.ubuntu.com is a better one for that | 12:28 |
| niemeyer | wallyworld: As it's public | 12:28 |
| wallyworld | ok, old habits | 12:28 |
| niemeyer | wallyworld: Let me grab my second factor auth key :) | 12:28 |
| wallyworld | sorry | 12:28 |
| niemeyer | wallyworld: No worries | 12:28 |
| wallyworld | the private one is in my browser history | 12:29 |
| wallyworld | so, my view is that normally, the function semantics of such things normally are such that if an exception is raised, or an error returned, disregard any result data | 12:29 |
| niemeyer | wallyworld: It's hard to follow that example, as it contains completely invalid code | 12:30 |
| wallyworld | sorry, it's pseudo code cut and pasted from real code | 12:30 |
| niemeyer | wallyworld: Yeah, but it's completely broken, badly intended, and referring to variables that don't even exist | 12:30 |
| wallyworld | sure, but that doesn'tmatter for the sake of the argument | 12:31 |
| niemeyer | wallyworld: You won't convince me about how I'm wrong about things being error prone and trivial with such an example :-) | 12:31 |
| wallyworld | that take awaya is | 12:31 |
| wallyworld | if err != nil { | 12:31 |
| wallyworld | return nil, err | 12:31 |
| wallyworld | } | 12:31 |
| wallyworld | return resp.Images, nil | 12:31 |
| wallyworld | the above is from the function | 12:31 |
| niemeyer | resp doesn't exist on that context | 12:31 |
| wallyworld | why does it need to do the if err != nil | 12:31 |
| wallyworld | why not just return resp.IMages, err | 12:32 |
| wallyworld | and let the client do if err != nil | 12:32 |
| niemeyer | wallyworld: Sorry, I honestly can't suggest anything.. the example is completely broken. I'm arguing that this is trivial to write, and that it makes code less error prone. Hard to even make the point if the example is completely crackful. | 12:32 |
| wallyworld | here's the full function, i was hoping you could just see the intent from the pseudo code, http://pastebin.ubuntu.com/1398973/ | 12:33 |
| wallyworld | imagine a module with dozens of such functions, all doing if err != nil unnecessarily, sure adds a lot of bloat and boilerplate | 12:34 |
| wallyworld | when the caller just goes ahead and does the err != nil dance anyway | 12:34 |
| rogpeppe | wallyworld: that's a pretty unusual case actually | 12:34 |
| rogpeppe | wallyworld: because it's not returning something that something else returned. | 12:35 |
| rogpeppe | wallyworld: it's returning something that something else might or might not have filled in | 12:35 |
| wallyworld | yes true | 12:36 |
| wallyworld | and the err tells us if the fill in worked | 12:36 |
| rogpeppe | wallyworld: agreed. we *could* just allow arbitrary broken return values when err != nil | 12:36 |
| niemeyer | wallyworld: That's fine, as long as there's a promise from the underlying function to never touch the resp value if any errors are returned | 12:37 |
| wallyworld | that's a pretty standard semantic | 12:37 |
| rogpeppe | wallyworld: but i think it's nicer if we do things more predictably | 12:37 |
| niemeyer | wallyworld: I'd be slightly surprised if that promise is in place, though | 12:37 |
| wallyworld | i agree it's nicer perhaps, but at the cost of a fair bit of bloat | 12:37 |
| niemeyer | wallyworld: Because it means you cannot return any errors after resp has been touched | 12:37 |
| niemeyer | wallyworld: That seems the heart of your concern | 12:38 |
| rogpeppe | wallyworld: tbh, i think the "bloat" makes the code easier to read | 12:38 |
| niemeyer | wallyworld: And honestly bloat to me is something else.. | 12:38 |
| wallyworld | i guess i see cut and paste boilerplate as bloat | 12:38 |
| rogpeppe | wallyworld: it means that i *know* instantly the set of possible values returned by ListImagesDetail | 12:38 |
| niemeyer | wallyworld: I see logic that is hard to read, poorly engineered, hard to maintain, as bloat | 12:39 |
| niemeyer | wallyworld: Error prone as well.. totally bloatful | 12:39 |
| wallyworld | sure, there's more than one type of bloat | 12:39 |
| wallyworld | i'll point jam to the scrollback when he is next in the office, since he also shared my concerns | 12:40 |
| niemeyer | wallyworld: That trivial line, makes things lighter to process.. I know I don't have to worry about the return value having bad data, and I know any callers couldn't *possibly* use a partial value coming out of it. It empties brain space rather than filling it up. | 12:40 |
| wallyworld | s/line/lines :-P | 12:41 |
| wallyworld | thanks for the discussion though | 12:42 |
| wallyworld | interesting points of view | 12:42 |
| niemeyer | wallyworld: Exactly my point.. 10 lines of code may be easier to read than 1. | 12:42 |
| wallyworld | maybe, depends on the reader i guess | 12:43 |
| niemeyer | wallyworld: Depends on the lines of code | 12:43 |
| wallyworld | or more so one's unerlying expections about the semantcs of the call | 12:44 |
| wallyworld | as a caller, i would never dream of using returned data if err != nil | 12:44 |
| niemeyer | wallyworld: I'm glad to hear that. | 12:44 |
| wallyworld | so it never would enter my head to expect the function to have to do anything about it | 12:44 |
| niemeyer | wallyworld: That still doesn't mean that you won't do that anyway, by mistake. | 12:45 |
| niemeyer | wallyworld: Either way, if the point isn't easy to agree with, I ask that we at least sustain that strong convention that we use both internally, and externally in the core Go development team. | 12:45 |
| wallyworld | sure, no problem | 12:46 |
| wallyworld | i was trying to understand the underlying rationale | 12:46 |
| niemeyer | var foo []Foo | 12:46 |
| niemeyer | foo, err = Func() | 12:46 |
| niemeyer | if err == ErrSomething { | 12:46 |
| niemeyer | // Ah, okay, that error is expected here. | 12:47 |
| niemeyer | } | 12:47 |
| niemeyer | Oops.. foo has bad data now. | 12:47 |
| wallyworld | yes, and so don't use it | 12:47 |
| niemeyer | wallyworld: Yeah, you're very smart and will never do that. I'm kind of dumb, and tend to make mistakes. | 12:47 |
| wallyworld | hey, i didn't mean it like that | 12:48 |
| wallyworld | tests are also part of the solution too | 12:48 |
| niemeyer | wallyworld: They surely are. Not rare we assert the fact we get a zero value, for example. | 12:50 |
| fwereade | niemeyer, aram's firewaller change is up again at https://codereview.appspot.com/6843128 | 12:51 |
| wallyworld | anyways, i'll make the necessary changes to use pointers | 12:51 |
| niemeyer | fwereade: I'll get on it right now | 12:51 |
| fwereade | niemeyer, the firewaller bug was a MachineUnitsWatcher bug, fixed this morning | 12:51 |
| wallyworld | so the pattern will work as intended | 12:51 |
| niemeyer | fwereade: Oh, what was it about? | 12:51 |
| fwereade | niemeyer, we were unwatching the wrong key | 12:51 |
| niemeyer | wallyworld: Thanks a lot | 12:51 |
| niemeyer | wallyworld: and thanks for explaining your POV | 12:51 |
| wallyworld | np, thanks for the discussion | 12:51 |
| fwereade | niemeyer, state/watcher now panics if you try any funny business like that | 12:51 |
| niemeyer | fwereade: Holy crap, the wrong key huh | 12:52 |
| fwereade | niemeyer, `for _, unit := range map[string]Life{...} {` | 12:53 |
| niemeyer | fwereade: Woah | 12:53 |
| fwereade | niemeyer, quite so | 12:53 |
| niemeyer | fwereade: I think I did that before too.. will pay more attention to ranges | 12:53 |
| fwereade | niemeyer, took a while to track it down, it doesn't exactly leap out at you when the type is defined elsewhere | 12:53 |
| TheMue | lunchtime, biab | 12:54 |
| niemeyer | fwereade: It didn't help that Unwatch takes an interface to support keys of multiple types | 12:56 |
| niemeyer | TheMue: Enjoy | 12:56 |
| fwereade | niemeyer, yeah, indeed | 12:56 |
| fwereade | niemeyer, rogpeppe pointed out that all our _ids are actually strings now | 12:56 |
| fwereade | niemeyer, although I'm still really bothered by id/_id on Relation | 12:57 |
| niemeyer | fwereade: Well, we could have saved ourselves the trouble if the watcher was at least verifying basic typing | 12:57 |
| niemeyer | fwereade: string/int/int64... | 12:57 |
| niemeyer | fwereade: Do you have an idea to workaround it? | 12:58 |
| fwereade | niemeyer, only that we could change it to demand string ids right now and I think everything would still work | 12:58 |
| fwereade | niemeyer, but tbh the panicing seemed to do the trick well enough | 12:59 |
| niemeyer | fwereade: Have you gone through the firewaller logic, or just reinstated the original branch? | 12:59 |
| niemeyer | fwereade: I don't understand how that would solve the id/_id case | 12:59 |
| fwereade | niemeyer, I have made slight enhancements to what existed but have not 100% verified everything from the ground up -- but 3 of us have already done so AIUI | 12:59 |
| fwereade | niemeyer, oh, it wouldn't at all | 12:59 |
| fwereade | niemeyer, it's just that I'm reluctant to start making pronouncements about "id"s wen I am still regularly tripped up by the relation id/key thing | 13:00 |
| niemeyer | fwereade: You could invert the relationship again by doing an extra lookup | 13:01 |
| fwereade | niemeyer, the id/key one? sorry, I don;t follow | 13:02 |
| niemeyer | fwereade: That said, how to guarantee uniqueness would still bother | 13:02 |
| niemeyer | fwereade: Yes | 13:02 |
| niemeyer | fwereade: I'm talking about the reason why we have key in _id | 13:02 |
| fwereade | niemeyer, ah, ok, I see -- we can't enforce uniqueness on additional fields then? | 13:03 |
| niemeyer | fwereade: We can create additional unique indexes on another field | 13:03 |
| niemeyer | fwereade: But it's an extra index, and the txn package works against _id | 13:03 |
| niemeyer | fwereade: We could disable the auto-index creation, create an additional unique index on key, and change the txn package to support other fields | 13:04 |
| niemeyer | fwereade: But, where's the gold at the other side of the rainbow? | 13:05 |
| fwereade | niemeyer, I'm sorry, I don't see where the txn package changes come in | 13:05 |
| niemeyer | fwereade: The Id field is mapped on _id | 13:06 |
| niemeyer | fwereade: All of the transactioned operations, inserts, updates, removes, are done against _id | 13:06 |
| fwereade | niemeyer, ok -- but both "id" and "_id" would still have to be unique, so surely we could still do the same against _ids that looked like "0" or "198" just as easily as ones that look like "wordpress:db mysql:server", couldn't we? | 13:07 |
| fwereade | niemeyer, and we look up by both key and id, so we really ought to be indexing on key anyway | 13:08 |
| fwereade | niemeyer, sorry, "id" | 13:08 |
| rogpeppe | niemeyer, fwereade, TheMue: i'm going for lunch now. i will be working this afternoon, but maybe not online all the time as i'll be on the road. | 13:08 |
| niemeyer | rogpeppe: Cool, have a good one | 13:09 |
| niemeyer | fwereade: Uniqueness against 0 or 198 don't mean anything.. we create those numbers to be unique | 13:09 |
| fwereade | niemeyer, the only other change I can think of is in the check-by-prefix in ServiceRelationsWatcher, and that's just a field switch... right? | 13:09 |
| niemeyer | s/don't/doesn't | 13:09 |
| fwereade | niemeyer, will the txn package somehow be able to insert docs with duplicate keys in the face of uniqueness constraints? | 13:10 |
| niemeyer | fwereade: Well, the underlying db doesn't allow it | 13:12 |
| fwereade | niemeyer, ah! but it will break txn.DocMissing? | 13:13 |
| niemeyer | fwereade: DocExists, DocMissing, yes | 13:14 |
| * fwereade sees now | 13:15 | |
| fwereade | niemeyer, yeah, probably worth the localised confusion then :) | 13:15 |
| niemeyer | fwereade: Hah, funny.. the fictitious err-related example I mentioned above using a zero-value on err != nil exists on this branch. :-) | 13:19 |
| fwereade | niemeyer, oh, poo, what did I do? | 13:25 |
| niemeyer | fwereade: Uh, nothing bad? :) | 13:25 |
| fwereade | niemeyer, ah? sorry, I thought you were pointing out a mistake | 13:25 |
| niemeyer | fwereade: No, I was pointing out earlier that it's important to be consistent on the zero-value on errors convention | 13:26 |
| fwereade | niemeyer, definitely -- and I guess aram or I did so? :) | 13:26 |
| niemeyer | fwereade: and as one example I used a snippet, (HH:46, above) | 13:26 |
| niemeyer | fwereade: That snippet exists almost as-is in the branch | 13:26 |
| niemeyer | fwereade: It would be a bug if the convention wasn't respected | 13:27 |
| fwereade | niemeyer, ah, yes, indeed | 13:27 |
| niemeyer | fwereade: Very nice | 13:30 |
| fwereade | niemeyer, cheers :) | 13:31 |
| niemeyer | fwereade: Thanks a lot | 13:31 |
| fwereade | niemeyer, np at all | 13:31 |
| fwereade | niemeyer, ah HA: there is *also*, it appears, a firewaller bug, that I had hitherto failed to repro | 13:41 |
| fwereade | niemeyer, I shall meditate upon this | 13:41 |
| niemeyer | fwereade: Hm | 13:41 |
| niemeyer | fwereade: How did you figure? | 13:41 |
| fwereade | niemeyer, I saw a test failure :/ | 13:42 |
| niemeyer | fwereade: Oh | 13:42 |
| fwereade | niemeyer, in TestGlobalModeRestartPortCount, as flagged :( | 13:42 |
| fwereade | niemeyer, I'm just about to investigate after I propose a trivial fix to UnitsWatcher -- I mistakenlythought I didn't want to notify Dead units in the initial set, but I was wrong | 13:44 |
| niemeyer | fwereade: Oh? | 13:44 |
| niemeyer | fwereade: Ah, I think I see | 13:45 |
| fwereade | niemeyer, they get lost from view if they're dead when we start up | 13:45 |
| fwereade | niemeyer, generally we need to handle dead ones | 13:45 |
| fwereade | niemeyer, I *could* do it by reconciling against what's installed but htat actually just feels like needless complexity | 13:45 |
| niemeyer | fwereade: Agreed, I think it's fine to report | 13:46 |
| fwereade | niemeyer, we can do it differently if you don;t like Deployer (which I will get to one day ;p) | 13:46 |
| fwereade | niemeyer, cool | 13:46 |
| niemeyer | fwereade: We can make it respect the Dead-is-the-last-thing-seen idea | 13:46 |
| fwereade | niemeyer, ah, I can't reconcile anyway | 13:46 |
| fwereade | niemeyer, exactly, it is still respected | 13:46 |
| fwereade | niemeyer, https://codereview.appspot.com/6846133 | 13:48 |
| niemeyer | fwereade: We don't send dead statuses, strictly speaking | 13:49 |
| * niemeyer thinks | 13:50 | |
| niemeyer | fwereade: "Once a unit observed as Dead has been reported" perhaps | 13:50 |
| fwereade | niemeyer, SGTM, thanks | 13:51 |
| niemeyer | fwereade: LGTM, cheers | 13:53 |
| fwereade | niemeyer, "as Dead or removed"? | 13:54 |
| niemeyer | fwereade: +1 | 13:55 |
| fwereade | niemeyer, cheers | 13:55 |
| * niemeyer => lunch | 14:05 | |
| TheMue | niemeyer: Enjoy | 14:09 |
| TheMue | *: Any experiences with OAuth packages around? | 14:14 |
| rogpeppe | TheMue: which version? | 14:17 |
| TheMue | *: Will start with goauth2 by Andrew. | 14:17 |
| rogpeppe | TheMue: istr that oauth2 is quite different from oauth, but otherwise i know very little | 14:17 |
| TheMue | rogpeppe: Ah, good hint. The Py version uses a package named oauth. Will take a look which version it supports. | 14:18 |
| TheMue | rogpeppe: Seems to be v1. | 14:21 |
| fwereade | TheMue, may I share your wisdom re firewaller? | 14:50 |
| fwereade | TheMue, I can't quite figure out whether the bug is in the FW or the test | 14:51 |
| TheMue | fwereade: I'm listening. | 14:51 |
| fwereade | TheMue, ok, so, the test is problematic | 14:52 |
| fwereade | TheMue, in that it starts a new FW, and asserts that the opened ports are the same as before | 14:52 |
| TheMue | fwereade: Which test do you refer to? | 14:53 |
| fwereade | TheMue, and so that test actually finds the expected state before the FW has even had a chance to open or close anything | 14:53 |
| fwereade | TheMue, TestGlobalModeRestartPortCount | 14:53 |
| fwereade | TheMue, the problematic one | 14:53 |
| TheMue | fwereade: OK, open it in the editor. | 14:53 |
| fwereade | TheMue, ok, so the last 4 blocks are a problem | 14:54 |
| fwereade | TheMue, the assert in the first one is likely to pass regardless of state | 14:54 |
| fwereade | TheMue, and when the test subsequently passes, the actual timing of the changes is a bit suspect -- initial events are still being handled after port 8080 is closed | 14:57 |
| fwereade | TheMue, and *at the moment* this is passing reliably for me, butthis is more likely a happy accident of logging than anything else | 14:57 |
| fwereade | TheMue, I can make the test 100% reliable, with event handling happening at the times I expect, by opening another port somewhere before starting the FW; and waiting for *that* port to be opened | 14:58 |
| TheMue | fwereade: I'm gong through it step by step now. | 14:59 |
| fwereade | TheMue, http://paste.ubuntu.com/1399817/ and http://paste.ubuntu.com/1399818/ might help you to visualize | 15:01 |
| * rogpeppe is looking at a beautiful frosty sunset while cruising down the A1. the joys of mobile connectivity. | 15:01 | |
| fwereade | TheMue, === signals event in the main loop; +++ signals port refcount ++, --- signals ports refcount -- | 15:01 |
| TheMue | fwereade: Ah, good. | 15:02 |
| fwereade | TheMue, the first is what we get normally; the second is what we get if I force the test to wait for the correct ports | 15:02 |
| fwereade | TheMue, I *think* it should always look like the second | 15:03 |
| fwereade | TheMue, *but* I don't see how that's possible in general, because (as you can see) the events coming in willy-nilly seem to cause surprising refcounts in the first | 15:05 |
| fwereade | TheMue, ie we never see a refcount of 2 for tcp:80 in the first paste, and ISTM that we should | 15:07 |
| TheMue | fwereade: Yes, I'm wondering. | 15:07 |
| TheMue | fwereade: That's what Aram called the race situation of overlaying changes incrementing and decrementing the refcount at once. | 15:10 |
| fwereade | TheMue, I would have expected the initial state to be built from initial events, so I'm having a little trouble following how the state built up in initGlobalPorts fits in with the watchery state | 15:10 |
| TheMue | fwereade: The initial one is build by taking a look at the state. | 15:11 |
| fwereade | TheMue, yeah, that is clear -- but AFAICT it doesn't necessarily correspond with the state picture that will be built up by the watchers | 15:12 |
| fwereade | TheMue, and I can't see how you reconcile them | 15:12 |
| TheMue | fwereade: One moment, I have to look on my own how we've done it. | 15:14 |
| TheMue | fwereade: It's the first filter part of flushGlobalPorts() | 15:18 |
| TheMue | fwereade: Here toOpen and toClose are only set if they are not only opened or closed. | 15:18 |
| TheMue | fwereade: And in closing, when the refcount is 0, it will be closed. | 15:19 |
| TheMue | fwereade: One has to look exactly when globalPortOpen (a map of port to bool) and globalPortRef (a map of port to int) is used. | 15:20 |
| fwereade | TheMue, ok, but globalPortOpen is initialized in initGlobalPorts while globalPortsRef is not | 15:24 |
| TheMue | fwereade: Yes, because otherwise each port would be counted twice, once by the state reading, once by the intial events. | 15:25 |
| fwereade | TheMue, ok -- and it's not ever possible to get a machine thinking it should close a port that was never open? | 15:26 |
| fwereade | TheMue, (according to globalPortsRef, that is) | 15:27 |
| TheMue | fwereade: So while the initGlobalPorts() just ensures that the ports aren't closed and immediatelly reopened the initial events ensure the correct counting (so far the idea). | 15:27 |
| fwereade | TheMue, ok, so there's no way an initial event can have the effect of closing a port? but that can't be true | 15:29 |
| TheMue | fwereade: "not ever possible" are big words. ;) Have to check it. But closing in global mode is done when ref == 0 and a close event is raised. | 15:29 |
| fwereade | TheMue, sure, I can see that | 15:29 |
| fwereade | TheMue, ok, thought experiment | 15:30 |
| TheMue | fwereade: I'm thinking about enqued event while the state scanning is in progress. | 15:31 |
| fwereade | TheMue, doing that would make me much more comfortable | 15:31 |
| fwereade | TheMue, but let's see: 1 machine, with 2 units on it | 15:31 |
| fwereade | TheMue, one of the units has port 80 open | 15:32 |
| fwereade | TheMue, a FW starts up and scans this state correctly | 15:32 |
| fwereade | TheMue, port 80 is true in globalPortsOpen, and is 0 in globalPortRef | 15:33 |
| fwereade | TheMue, and the FW then starts up its watches | 15:33 |
| TheMue | fwereade: Exactly, and now the initial watcher event is retrieved. | 15:33 |
| fwereade | TheMue, it gets an event for the machine, and starts a watch for its units | 15:34 |
| fwereade | TheMue, it then gets an event with the two units in | 15:34 |
| fwereade | TheMue, and starts port watches for each | 15:34 |
| fwereade | TheMue, at this precise moment, the unitds think they have no open ports, right? | 15:35 |
| fwereade | TheMue, so what happens when we get the first port event depends on which port watch happens to fire first | 15:36 |
| TheMue | fwereade: Important is, what the fw thinks. And it thinks, hey, I have the port open, but I don't know how often. But it gets the number by the incoming events. | 15:37 |
| TheMue | fwereade: But you're right, the watcher for the machines units is started after the machine event. And so the state could have been changed inbetween. | 15:37 |
| fwereade | TheMue, ok, actually, it can be simpler I think | 15:38 |
| TheMue | fwereade: The machine watcher is started before (!) the initial state scanning, so here it's better, | 15:38 |
| fwereade | TheMue, exactly: if the unit with 80 open closes it after the inital scan and before the watch, port 80 will never be mentioned and never be closed | 15:38 |
| fwereade | TheMue, it'll be open forever with refcount 0 | 15:39 |
| TheMue | fwereade: As a first thought I would say you're right. | 15:40 |
| TheMue | fwereade: The delay until the machined is started ... | 15:40 |
| TheMue | fwereade: It seems like initGlobalMode() should start the needed machined immediately. | 15:41 |
| fwereade | TheMue, if we could do that, that would be great | 15:41 |
| fwereade | TheMue, it could be fiddly though | 15:42 |
| TheMue | fwereade: And the initial event of the machines watcher has to compare if the started ones are still correct. | 15:42 |
| TheMue | fwereade: Yes, not trivial. | 15:43 |
| fwereade | TheMue, I don't see any way for it to be correct without doing so though | 15:43 |
| TheMue | fwereade: Based on our current analysis I have to agree, yes. | 15:43 |
| fwereade | niemeyer, ok, I think TheMue and I are in agreement that there is a fundamental race in the firewaller, which is not trivial to resolve | 15:47 |
| niemeyer | fwereade: Oh | 15:47 |
| fwereade | niemeyer, I am not intrinsically opposed to diving in and fixing it, but I am reluctant to allow it to delay subordinates | 15:48 |
| niemeyer | fwereade: What's it? | 15:48 |
| fwereade | niemeyer, basically it (1) builds up a bunch of reference state and (2) starts a bunch of watches to fill in the details of that state | 15:49 |
| fwereade | niemeyer, but neglects to take account of possible changes in between (1) and (2) | 15:49 |
| niemeyer | fwereade: Ok | 15:49 |
| niemeyer | fwereade: How could this take place? | 15:49 |
| fwereade | niemeyer, well, initGlobalMode just does a straight scan of state; the main loop then starts building up a huge tree of watchers, starting from the machines and adding units and services as events dictate | 15:51 |
| niemeyer | fwereade: Okay | 15:51 |
| fwereade | niemeyer, I don't think it'll be *toooo* hard to do it right, but it will definitely be fiddly | 15:52 |
| niemeyer | fwereade: Sorry, I still don't perceive the issue | 15:52 |
| niemeyer | fwereade: Can you walk through the error happening? | 15:52 |
| fwereade | niemeyer, one machine with one unit; port 80 is open on the unit and in the environment | 15:53 |
| fwereade | niemeyer, FW does initial scan | 15:53 |
| fwereade | niemeyer, unit closes port 80 | 15:53 |
| niemeyer | fwereade: Ok | 15:53 |
| niemeyer | fwereade: Ok | 15:53 |
| fwereade | niemeyer, Fw gets its initial machines event, and gets/watches unit state | 15:54 |
| niemeyer | fwereade: Okay | 15:54 |
| fwereade | niemeyer, the initial state reported is "no ports open" | 15:54 |
| niemeyer | Hmm | 15:54 |
| * niemeyer looks at the code | 15:54 | |
| fwereade | niemeyer, we're watching a unit we got via the machine's units event, not the one we originally looked at to determine that 80 was open | 15:55 |
| niemeyer | fwereade: The set of ports that are actually opened is obtained within the unit watch, when the even fires, right? | 15:56 |
| niemeyer | Oh, no | 15:56 |
| fwereade | niemeyer, ok, what happens is: | 15:56 |
| fwereade | niemeyer, the firewaller never sees a change to port 80, and hence never closes it | 15:56 |
| fwereade | niemeyer, ...that's it | 15:57 |
| fwereade | niemeyer, there's a refcounting mechanism that works fine (I think) when state doesn't change during that critical window | 15:59 |
| niemeyer | fwereade: Where is the window in the code? | 16:00 |
| fwereade | niemeyer, from the end of fw.initGlobalMode() to the end of the last event on the ports channel that is traceable to the initial machines event | 16:01 |
| fwereade | niemeyer, I think | 16:01 |
| fwereade | niemeyer, ok, they all are | 16:01 |
| fwereade | niemeyer, the last initial event that is itself a conseqence only of initial events, sharing the ultimate ancestor of the first machines event | 16:02 |
| fwereade | niemeyer, I don;t think I made that any clearer, did I? | 16:02 |
| niemeyer | fwereade: No, but I blame my question | 16:03 |
| fwereade | niemeyer, ok, looking just at the main fw loop | 16:03 |
| fwereade | niemeyer, first of all we set up initial state, and then we start paying attention to a pre-started global machines watch | 16:04 |
| niemeyer | fwereade: Perhaps I should try to point out how I originally imagined this would work | 16:04 |
| fwereade | niemeyer, changes on that are safe | 16:04 |
| niemeyer | fwereade: So you can fix my assumption with reality | 16:04 |
| fwereade | niemeyer, ok, go for it | 16:04 |
| niemeyer | fwereade: There's an initial pass at the beginning which verifies what is *actually* open in the environment | 16:06 |
| fwereade | niemeyer, ie according to the provider? | 16:06 |
| niemeyer | fwereade: Yes | 16:06 |
| fwereade | niemeyer, ok, ty, carry on, this is accurate | 16:07 |
| niemeyer | fwereade: Okay, I think I see the problem actually | 16:08 |
| niemeyer | fwereade: No, wait, okay | 16:09 |
| niemeyer | fwereade: Nevermind, I do see the issue.. closing is indeed a problem because the initial state isn't connected to the initial referenced state | 16:10 |
| niemeyer | fwereade: Open works under the same circumstances, though | 16:10 |
| niemeyer | fwereade: Makes sense? | 16:10 |
| fwereade | niemeyer, agreed, open is safe | 16:10 |
| fwereade | niemeyer, except, I *think* there is another issue | 16:11 |
| fwereade | niemeyer, but I need to think it through a bit | 16:11 |
| fwereade | niemeyer, gaah it seems not to be flowing | 16:12 |
| fwereade | niemeyer, the trouble is fundamentally that (1) changes can come in and change state both before and during the watch-scan, and this is confusing and (2) there is a failing test that demonstrates weird refcounts while this is happening, even if it doesn't always fail, and which is fixed by delaying the external changes until we;re sure the FW has finished what it's doing | 16:14 |
| niemeyer | fwereade: I think the issue is rather simple, actually | 16:15 |
| fwereade | niemeyer, I am sadly not able to actually hold the whole thing in my head atm, so I can't come up with a good explanation of why the test sometimes fails: adding logging seems to have fixed it | 16:15 |
| fwereade | niemeyer, I am talking here in a very narrow context | 16:16 |
| * TheMue is playing with the idea of always start the watcher goroutines based on the initial state before retrieving the first machines watcher events. | 16:16 | |
| niemeyer | fwereade: I suggest trying to address the actual issue we do know about first | 16:16 |
| fwereade | niemeyer, that issue only came to light because of the more fuzzily specified one I just mentioned | 16:16 |
| fwereade | niemeyer, there's a test that fails sometimes | 16:17 |
| niemeyer | fwereade: Cool, I'm glad you did look at this | 16:17 |
| fwereade | niemeyer, there's an issue in the watcher | 16:17 |
| niemeyer | fwereade: Hm? | 16:17 |
| fwereade | niemeyer, the two may be connected | 16:17 |
| fwereade | niemeyer, I am not equals to the task of concretely demonstrating such a connection without handwaving | 16:17 |
| fwereade | niemeyer, well, I tried to repro it, and I could, and I found the MUW unwatch bug | 16:18 |
| niemeyer | fwereade: That's fine, I'll try to interpret the hand-wavy frequency :) | 16:18 |
| fwereade | niemeyer, in the following test runs, the firewaller appeared to be fine | 16:18 |
| fwereade | niemeyer, some time after submitting, I saw the test fail | 16:18 |
| fwereade | niemeyer, it is irritatingly elusive, and currently appears to be passing 100% for me with no changes otherthan logging | 16:19 |
| fwereade | niemeyer, but sometimes it fails every 2 or 3 goes | 16:19 |
| niemeyer | fwereade: Okay, but what's the failure about? | 16:19 |
| fwereade | niemeyer, the failure is about whether a port gets closed in the environment or not | 16:20 |
| niemeyer | fwereade: Isn't that the bug we're discussing above? | 16:20 |
| fwereade | niemeyer, not exactly -- the test doesn't exercise that precise situation | 16:21 |
| fwereade | niemeyer, this is a port that should close but sometimes doesn't | 16:21 |
| niemeyer | fwereade: That's exactly the case we're talking about above | 16:21 |
| fwereade | niemeyer, ...ok, they are more closely connected than I thought :/ | 16:22 |
| fwereade | niemeyer, I'd approached the conclusion from 2 separate directions, didn't realise they were the same place | 16:23 |
| fwereade | niemeyer, so | 16:23 |
| fwereade | niemeyer, I am comfortable that this is the bug | 16:23 |
| fwereade | niemeyer, I have a patch for the test, to cause it to avoid exercising the bug | 16:24 |
| fwereade | niemeyer, I am not *really* comfortable taking on "fix the firewaller" at this point, especially because I don't feel I know it as well as others | 16:24 |
| niemeyer | fwereade: I suspect the issue is simple to solve | 16:25 |
| niemeyer | fwereade: and to describe | 16:25 |
| fwereade | niemeyer, yeah, it may be | 16:26 |
| niemeyer | fwereade: machined.ports starts empty | 16:26 |
| niemeyer | fwereade: That's it really | 16:26 |
| fwereade | niemeyer, I'm a bit suspicious of unitd.ports starting empty too | 16:27 |
| niemeyer | fwereade: All the deltas are potentially wrong because of that | 16:27 |
| fwereade | niemeyer, yeah, that does sound simpler to deal with | 16:28 |
| niemeyer | fwereade: I don't think that's a problem | 16:28 |
| niemeyer | fwereade: unitd.ports reflects what we learned from the state itself | 16:28 |
| niemeyer | fwereade: Which is always right | 16:28 |
| fwereade | niemeyer, hold on, where does machined.ports come from if not from unitds' ports? | 16:28 |
| fwereade | niemeyer, nah, it starts out empty | 16:28 |
| niemeyer | fwereade: Exactly | 16:28 |
| niemeyer | fwereade: We compute what we want machined.ports to be out of unitd.ports, which we learn from the state | 16:29 |
| fwereade | niemeyer, ok, but | 16:29 |
| fwereade | niemeyer, when a machine has >1 unit | 16:29 |
| niemeyer | fwereade: and diff against the previous value of machined.ports to know what to apply to the env | 16:29 |
| fwereade | niemeyer, we cannot calculate the machined's ports without knowing all its unitds' ports, right? | 16:30 |
| niemeyer | fwereade: We know all unitd ports.. that's why we cache it | 16:31 |
| fwereade | niemeyer, but we don't actually set it when we create it | 16:31 |
| fwereade | niemeyer, it sits around empty until the main loop first handles a ports event for that unit | 16:31 |
| niemeyer | fwereade: Ah, interesting | 16:32 |
| niemeyer | me looks | 16:32 |
| fwereade | niemeyer, so when a machine has 2 units, and we flush the machine's ports based on the first unit's ports only (because we haven't yet handled the second one's initial event) we have a problem | 16:32 |
| fwereade | niemeyer, if I promise I will take a deeper look at it, can I patch the proximate problem by tweaking the test to skirt the problem? | 16:34 |
| niemeyer | fwereade: You can do that either way | 16:34 |
| niemeyer | fwereade: The problem is there and isn't going away until someone fixes it | 16:34 |
| niemeyer | fwereade: Your branch is orthogonal | 16:34 |
| fwereade | niemeyer, true, but we don't want test failures dirtying up our lives | 16:35 |
| fwereade | niemeyer, it was this bug, I think, that originally blocked it | 16:36 |
| fwereade | niemeyer, ok, let me put it differently | 16:36 |
| niemeyer | fwereade: Yes, but the point of blocking is that no one could tell what the bug actually was | 16:36 |
| fwereade | niemeyer, ah! and we now have progress? | 16:36 |
| niemeyer | fwereade: So whether it was orthogonal or not was at stake | 16:36 |
| fwereade | niemeyer, that makes sense | 16:36 |
| fwereade | niemeyer, got you | 16:36 |
| niemeyer | TheMue: Are you following? | 16:37 |
| TheMue | niemeyer: Yes, the whole time. | 16:37 |
| niemeyer | TheMue: Awesome, thanks | 16:37 |
| niemeyer | TheMue: Can we have a fix for the two issues next week? | 16:37 |
| TheMue | niemeyer: I think so, as it's now clearer. | 16:38 |
| fwereade | niemeyer, TheMue: well, awesome :) | 16:38 |
| niemeyer | TheMue: Two different branches: 1) machined.ports must be properly initialized; that must be done differently for the two modes | 16:39 |
| niemeyer | TheMue: 2) Creating new machined's must ensure all unitd's within it are initialized before returning to the main loop | 16:39 |
| niemeyer | fwereade: Makes sense? Anything to add? | 16:39 |
| fwereade | niemeyer, I think that covers it | 16:40 |
| niemeyer | TheMue: Questions? | 16:41 |
| * TheMue feels comfortable with it too. | 16:41 | |
| fwereade | niemeyer, I will propose a trivial .Skip on that test for now | 16:42 |
| fwereade | niemeyer, I think it's more valuable to have it preserved in a state that more-or-less exposes the issues than it is to hide them away by dodging the problem | 16:42 |
| niemeyer | fwereade: +1 | 16:42 |
| * rogpeppe2 needs to do some navigation now. i may not return today. if not, have a great weekend everyone! | 16:48 | |
| TheMue | rogpeppe2: Enjoy your weekend | 16:51 |
| rogpeppe2 | TheMue: thanks. lots of obscure carols being sung this weekend - we're going to a Festival of Village Carols... | 16:52 |
| * TheMue will step out in a few moments too. Tomorrow our little Vanessa has her next archery tournament. I'm excited. | 16:52 | |
| TheMue | rogpeppe2: That's pre-christmas time … :D | 16:53 |
| rogpeppe2 | TheMue: it's the 1st tomorrow! | 16:53 |
| rogpeppe2 | TheMue: just scrapes into advent | 16:53 |
| fwereade | niemeyer, https://codereview.appspot.com/6849126 | 16:53 |
| fwereade | niemeyer, for form's sake :) | 16:53 |
| TheMue | rogpeppe2: Yep, have been to our local Christmas market yesterday. Has been quite good. | 16:54 |
| niemeyer | rogpeppe2: Have fun there | 17:00 |
| niemeyer | fwereade: +1 | 17:00 |
| TheMue | So, I'm stepping out. Have a nice weekend everyone. | 17:43 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!