| jam | axw: I sent a first set of review comments on your PR. I'm going to try and finish reviewing it today, but i realize that it would have been useful to submit the feedback I had earlier so you could respond | 05:00 |
|---|---|---|
| axw | jam: thanks | 05:02 |
| === frankban|afk is now known as frankban | ||
| axw | jam: in one of your comments, it sounds like you're leaning back towards the original approach of mapping IDs to addresses not stored in the raft config, is that right? | 09:56 |
| jam | axw: so I wanted to discuss it at least. I thought you were doing separate mapping on our side, not part of the raft config | 09:57 |
| jam | you were saying that you needed to publish the alternative config and track it somewhere | 09:57 |
| jam | which was my big concern | 09:57 |
| axw | jam: originally I had (in the raft config) the server ID and address both equal to machine-tag | 09:58 |
| axw | jam: then the transport code would map the "address" (machine tag) to the real API server hostport | 09:58 |
| jam | axw: ah, that's probably the part I didn't want. I'd prefer not to have the "address" be a tag | 09:58 |
| axw | jam: it eliminates the split-brain issue, since you have a stable address in the raft config (no need to remove/add). what's your concern exactly? about consistency? | 09:59 |
| axw | jam: consistency/ordering isn't ideal for name resolution, you always want what's current? | 10:00 |
| jam | axw: my main concern is that the raft config is no longer self contained. you need to look up somewhere else that you then need to keep consistent | 10:04 |
| axw | jam: ok. I agree that it would be nice to have it self-contained. it's unfortunate that there's no atomic config change op | 10:05 |
| jam | axw: so it separates the idea of what IP address a given node has, but it doesn't let you change the address of the node without removing it? | 10:05 |
| jam | axw: that seems weird given the separation | 10:05 |
| axw | jam: yup | 10:05 |
| axw | jam: unless docs lie... | 10:07 |
| axw | I see code that looks like it *might* allow an address change, which is not what the docs say | 10:08 |
| jam | axw: looking at the code in configuration.go | 10:08 |
| axw | yep, same | 10:08 |
| jam | looks like you can just AddVoter(existingId, newAddress) | 10:08 |
| jam | and it will overried | 10:08 |
| jam | if server.Sufferage == Voter ; Servers[i].Address = serverAddress | 10:09 |
| jam | axw: ^^ | 10:09 |
| axw | jam: yep, I just saw the same. I'll test it now. yay for misleading doc comments | 10:09 |
| axw | "If the server is already in the cluster as a voter, this does nothing." | 10:09 |
| jam | axw: yeah, there definitely is a discrepency between some of the code comments and the "design" vs the "implementation" | 10:11 |
| jam | things like "AddVoter" actually adds it as staging, but promotion actually does nothing which means it actually adds it as voter | 10:11 |
| axw | jam: ok, it works fine - I'll update the code to get rid of the intermediate remove op, and I'll add in a check to stop it from removing a server if it could lead to split-brain | 10:22 |
| jam | wpk: I looked at PR 8323 | 10:23 |
| jam | axw: that changes your code also to handle the "localhost" to "public IP" case, doesn't it? | 10:28 |
| axw | jam: sorry, don't follow - which case is that? you mean changing the bootstrap address (juju.localhost)? | 10:28 |
| jam | axw: you have comments around "if len(X) < 3)" and some comments in the test suite about handling when we have 1 entry vs 3 entries | 10:29 |
| jam | axw: can we simplify that code/ | 10:29 |
| jam | ? | 10:29 |
| axw | jam: yes, as we speak :) I'll just make sure that it doesn't remove any servers if that would lead to an even-sized configuration | 10:30 |
| axw | add or remove... | 10:31 |
| jam | axw: i'm less concerned about an even-sized config, because often it is necessary to go via that to get to what you want. I'd like to move our existing ensure-ha to not mandate even-always | 10:33 |
| axw | jam: OK, I'll leave that for now then | 10:33 |
| axw | I'll just drop the length check | 10:33 |
| jam | axw: sgtm. (specifically, if there is a breakage on one of your machines, you'd really like to just kill that machine, and then bring it back into rotation, and our code around "protect you from yourself" usually gets in the way of people actually fixing stuff) | 10:34 |
| * axw nods | 10:34 | |
| jam | axw: I updated my comments on 8308 | 10:37 |
| axw | jam: I've pushed updates | 10:46 |
| axw | jam: I have no strong feeling regarding Internal vs. HA, so let me know if you'd like me to change it. also please see my reply regarding Debug vs. Info for cluster config updates | 11:01 |
| === frankban is now known as frankban|afk | ||
| balloons | how is the ami chosen when creating a unit? | 19:07 |
| balloons | wallyworld, ^^? | 19:21 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!