[#1017] Recognise a replication server by the address it names, not by the one its session came from - #1020
Conversation
f0acce5 to
44f98ce
Compare
|
@maximthomas rebased on master after #935 and #976 landed; the branch had gone The one conflict was in Re-run on the rebased branch: the 12 classes from the description plus |
|
@maximthomas one more commit, Checking the rebase for overlap with #935 turned up one in prose rather than code: the comment in The duplicate server id check itself is not touched: |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The change sits where the bug is, and one predicate serves every reader of the address.
- The three call sites the issue and the description name all go through one predicate:
ReplicationServer.runConnect:612,ReplicationServerDomain.isAlreadyConnectedToRS:1431andstopReplicationServers:1080all end inReplicationServerHandler.isServerAt(). - The comparison is the one the data-server side already uses for this question,
ReplicationBroker.isSameReplicationServerUrl()→HostPort.isEquivalentTo(), andgetConnectedRSAddresses()is gone rather than widened. MultiHomedPeerTestasserts its precondition (getServerAddressURL()= the loopback address,getServerURL()= the named one) before the case, so a handshake that ends any other way fails on the precondition, not beside it.
issue (blocking): A peer that names a host this server cannot resolve is still reported as ERR_DUPLICATE_REPLICATION_SERVER_ID, where the rewritten connect() comment says its second session is dropped as a cross connect.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:783, :60-64, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:758-767, opendj-server-legacy/src/main/java/org/opends/server/types/HostPort.java:489-490, :525-532
isServerAt() decides only through HostPort.isEquivalentTo(), which calls InetAddress.getAllByName() on both sides and returns false on UnknownHostException — a name that does not resolve from this server is equivalent to nothing, not even to the identical string. The addresses javadoc calls such a name "an ordinary thing for a peer to name" (the setServerURL() fallback of a peer whose configured address is not local to it — the NAT peer of #1017). For that peer, inbound handler [H:p, A2:p], outbound [H:p, A1:p]: isSameServerAs() compares H/H (throws → false), H/A2, A1/H, A1/A2 — all false — and isAlreadyConnectedToRS() throws the duplicate-id message, once per dial, at the #935 blacklist rate, plus the ERR_COULD_NOT_SOLVE_HOSTNAME line each outbound handler's HostPort.valueOf(H) logs. Not a regression, and the multi-homed peer (names the address it is configured under) is fixed; but the comment at ReplicationServer.java:765 promises the cross connect for exactly this peer, and the code does not deliver it. HostPort.equals() compares normalizedHost, which is the raw name when resolution failed (HostPort.java:363-364), so one short circuit closes it:
boolean isServerAt(HostPort address)
{
for (HostPort known : addresses)
{
// equals() matches a name neither side can resolve; isEquivalentTo() answers false for it.
if (address.equals(known) || address.isEquivalentTo(known))
{
return true;
}
}
return false;
}Pin: a MultiHomedPeerTest case whose fake peer names nonexistent.invalid:ports[1] (RFC 6761, resolvers answer NXDOMAIN) and dials the server at it; assert the second session is dropped with no ERR_DUPLICATE_REPLICATION_SERVER_ID record — red at the head, green with the short circuit.
issue (non-blocking): The runConnect() call site is pinned by no case: restoring the base branch's skip at :612 survives 4/4 (measured), while the description says the first case pins that site.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:126, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:612-619
aPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress asserts domain.isConnectedToServerAt(peerAddress) directly and never observes the connect thread; the run the description reports mutated the comparison inside the predicate, which the case reaches by calling it, not through runConnect(). Mutant at :612, isConnectedToServerAt() untouched: if (domain.getConnectedRSs().values().stream().map(h -> HostPort.valueOf(h.getServerAddressURL())).anyMatch(rsAddress::equals)) — survives 4/4 (bytecode checked). Under it the connect thread dials 192.0.2.1:p1 on every pass — the symptom the class javadoc names first — and nothing in the class moves: cases 2–4 never read the connect-failure records, and errorLogRecordsOfHandshakeWith() filters to the duplicate-id text.
final ReplicationServerHandler registered = waitForRegistration(domain);
// Two passes of the connect thread after the registration: the already connected
// branch reports a session (ReplicationServer.java:619) and dials nothing.
rs.waitConnections();
rs.waitConnections();
assertThat(connectFailureRecordsFor(peerAddress, afterRegistration))
.as("a registered peer must not be dialled again")
.isEmpty();Pin: at the head the already-connected branch takes reportConnectionRestored(rsAddress, baseDN, true) and never dials; under the mutant every pass dials and the first failure is logged after the registration timestamp.
suggestion (non-blocking): The host half of isServerAt() is pinned by no case: a port-only comparison survives 4/4 (measured).
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:201, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:783
The one negative case, twoServersSharingAServerIdAreStillReported, names the second server at 198.51.100.1:ports[2] while every positive case uses ports[1], so isEquivalentTo() answers false on the port before any host is read. Mutant if (address.getPort() == known.getPort()) in place of address.isEquivalentTo(known) — 4/4 green (javap: no isEquivalentTo in the class).
// Same port as the registered peer: only the host tells the two apart.
final HostPort otherAddress = documentationAddress(OTHER_ADDRESS, ports[1]);Pin: nothing binds ports[2]; at the head isEquivalentTo(198.51.100.1:p1, 192.0.2.1:p1) is false (distinct literals) and vs 127.0.0.1:p1 false (one local, one not), so the case still reports; the port-only mutant drops the session in silence and isNotEmpty fails.
suggestion (non-blocking): The connected-address arm of addresses is pinned by no case: singletonList(namedAddress) survives 4/4 (measured).
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:123, :769-773, opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:112, :159, :210, :250
Every case has the fake peer name the address the server is configured with, so every match lands on the named arm, and the getServerAddressURL() preconditions read serverAddressURL, which the mutant still sets. The javadoc of isServerAt() calls the connected arm "the only one known of a server which names an address this configuration does not use"; that road is asserted nowhere. Mutant addresses = Collections.singletonList(namedAddress); — 4/4 green.
// The peer is configured at the address it dials from and names one this
// configuration does not use: only the connected arm can match it.
final HostPort peerAddress = loopbackAt(ports[1]);
final HostPort namedAddress = documentationAddress(OTHER_ADDRESS, ports[1]);
rs = startServerWithPeerConfiguredAt(ports[0], "multiHomedPeerConnectedArmDb", peerAddress);
inbound = registerPeerFrom(ports[0], namedAddress, baseDN);
...
assertThat(domain.isConnectedToServerAt(peerAddress)).isTrue();Pin: at the head isConnectedToServerAt(127.0.0.1:p1) matches on the connected arm; the mutant's isEquivalentTo(127.0.0.1:p1, 198.51.100.1:p1) is false (one local, one not) and the case goes red.
suggestion (non-blocking): The resolve-once design is pinned by nothing: every address the cases hand in is an IP literal.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:60-64, opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:423-430
The reason given for building the two HostPorts once is the ERR_COULD_NOT_SOLVE_HOSTNAME line HostPort logs per construction from a name it cannot resolve; documentationAddress() and loopbackAt() build literals only, for which normalizeHost() logs nothing, and no case reads that message. A mutant rebuilding HostPort.valueOf(serverURL) inside isServerAt() on every call re-logs per connect pass and stays green (traced, not run).
Pin: the same nonexistent.invalid:ports[1] case as the blocking issue — assert exactly one ERR_COULD_NOT_SOLVE_HOSTNAME record for that name over two waitConnections() passes after registration.
suggestion (non-blocking): The choice of isEquivalentTo() over equals() is unpinned by construction.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:430, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:783
The two differ only on a name with several A records (a hosts-file fixture) or on an unresolvable name — where equals() is the one that answers true, see the blocking issue. On the literals the cases use they agree, so address.equals(known) alone is green on every case. Recorded once; nothing portable to ask for.
suggestion (non-blocking): ERR_DUPLICATE_REPLICATION_SERVER_ID prints the two connected addresses, while the decision is now made on the named ones too.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1444-1447, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:122
serverAddressURL is the connected address only. A pair reported at the head differs in what it names as well as where it came from; the line shows the latter, which for two peers behind one gateway is the same string twice. Pre-existing shape; twoServersSharingAServerIdAreStillReported pins the current text, so this is a test change too.
LocalizableMessage message = ERR_DUPLICATE_REPLICATION_SERVER_ID.get(
localReplicationServer.getMonitorInstanceName(),
oldRsHandler.getServerURL() + " (" + oldRsHandler.getServerAddressURL() + ")",
rsHandler.getServerURL() + " (" + rsHandler.getServerAddressURL() + ")",
rsHandler.getServerId());nitpick (non-blocking): The addresses javadoc and the description say the addresses are "resolved once … rather than on each comparison"; the comparison resolves per call.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:60-64, opendj-server-legacy/src/main/java/org/opends/server/types/HostPort.java:489-490
isEquivalentTo() calls getAllByName() on both hosts on every call; what is once is the construction — normalizeHost() and its ERR line. The stated outcome (no ERR line per pass) holds, but because isEquivalentTo() traces its UnknownHostException instead of logging it; the connect thread does resolve every connected handler's named host per pass, per configured address with a matching port, bounded by the JVM resolver cache (10 s negative TTL).
/**
* The addresses the remote replication server is known by, built once, when its start
* message names it: {@link HostPort} logs a name it cannot resolve each time it is built
* from one -- which the fall back of {@code ReplicationServer.setServerURL()} to the host
* name of the machine makes an ordinary thing for a peer to name -- while the comparison
* the connect thread makes on every pass reports nothing above trace.
*/nitpick (non-blocking): "Nothing the tests do can leave the machine" / "which nothing routes" — the server under test dials 192.0.2.1:P.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:72, :427, :270-272, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:711
The helper's own javadoc says the connect thread "dials it, fails, and comes back to it". Any pass that finds the domain with the peer not yet registered runs socket.connect(192.0.2.1:p1, 5000) under connectThreadLock; a host stack does not drop a SYN to TEST-NET-1, it sends it to the default route. A race (pass 1 usually precedes the domain), fails nothing, costs up to 5 s of connect-thread wall time and a thread that outlives remove(rs) by up to the timeout (shutdown() interrupts only, socket.connect is not interruptible). The claim to keep is "nothing answers there", not "nothing leaves".
nitpick (non-blocking): "Two servers which name different addresses are still reported" (description, "What this gives up") is not so for two peers behind one gateway on the same port.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:753-761, :120-121
Two peers sharing a server id, dialling in from one source S on the same listen port, carry S:port on the connected arm of both handlers (connectedAddress takes the named port); isSameServerAs() matches S/S and drops the second in silence, and stopReplicationServers(S:port) stops both. Same as the base (string compare on S:port); the code's own javadoc ("either of the two addresses … identifies it") is consistent with it — the over-promise is only in the description.
nitpick (non-blocking): ReplicationServerConnectFailureTest still documents the pre-#1020 identity, and both test javadocs misname the old method.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/ReplicationServerConnectFailureTest.java:312-330, :345-347, opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:56
The javadoc of aPeerRegisteredUnderAnotherAddressStillClosesItsOutage says "This is the multi homed peer", names toServerAddressURL() and says the already-connected branch "compares the configured address against one which never matches it". At the head the method is gone from src/main, a multi-homed peer is matched by its named address, and the fixture (127.0.0.1:ports[2] vs configured ports[1]) is the port-mismatch peer of the rewritten connect() comment — it reaches ERR_DUPLICATE_REPLICATION_SERVER_ID only because the port differs. The second commit rewrote the two connect() paragraphs that said the same and left this one. Also, ServerHandler.toServerAddressURL() (both javadocs, the description) was a private method of ReplicationServerHandler.
note (non-blocking): Not run.
- The three mutants against the 13 classes the description lists: measured against
MultiHomedPeerTestonly (all three survive there); the sibling suites' replication servers namelocalhost:port, whichisEquivalentTo()answers on the named arm, so they do not reach the arms in question (reasoned, not run). - Cases 2 and 4 red on the base code with
isConnectedToServerAt()shimmed: taken from the description's own run, not re-run.
|
@maximthomas round 2, issue (blocking) -- the peer whose named host this server cannot resolve. Taken. Two things the change carries, and the description now says both:
issue (non-blocking) -- the The pin as proposed does not hold, and not only under the mutant. What the case asserts instead is the other side of that same branch: an outage recorded from the suggestion -- the host half of suggestion -- the connected arm. Taken, as suggestion -- the resolve once design. Taken inside the unresolvable case: three suggestion -- suggestion -- nitpick -- "resolved once". Taken: built once. The javadoc now says what the building buys nitpick -- "nothing can leave the machine". Taken: nothing answers there. The class javadoc nitpick -- "two servers which name different addresses are still reported". Taken in the nitpick -- the stale javadoc. Taken: One more from the same seam, recorded in Runs, on the head of this round:
|
maximthomas
left a comment
There was a problem hiding this comment.
praise: The fix sits where the bug is, and its trade-off is written down beside it.
ReplicationServerHandler.isServerAt():795—address.equals(known) || address.isEquivalentTo(known): the unresolvable name thesetServerURL()fallback produces now matches itself, andaPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerIdgoes red without theequals()arm.ReplicationServerHandler.java:776-785— the javadoc says what theequals()arm gives up (two servers sharing an id and one unresolvable name on one port), so the next reader does not rediscover it.aPeerWhichNamesAnAddressThisConfigurationDoesNotUseIsFoundByTheOneItDialledFrom— the connected arm ofaddresseshas its own case, and the description's mutant table names which case kills which mutant.
issue (non-blocking): stopReplicationServers() drops a healthy peer which names a loopback address when this server's own entry is removed from ds-cfg-replication-server.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1083-1093, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServer.java:2017-2027, :625
HostPort.normalizeHost() folds every address local to this machine to localhost, so HostPort.valueOf("<own hostname>:8989").equals(HostPort.valueOf("localhost:8989")) is true (measured on the compiled class, unchanged by this PR). A peer whose configuration lists localhost:P or 127.0.0.1:P for itself names exactly that in its start message (setServerURL() takes the first configured entry which isLocalAddress() on its port), and on this server that named address is this server's own configured entry on the same port. disconnectRemovedReplicationServers() puts every removed entry into serversToDisconnect, this server's own included — the connect road skips self at :625, the disconnect road does not — so removing this server's own entry from its configuration (:604-609 say a list without self is supported) has isServerAt(hostA:P) answer true on that peer's handler and stopServer() its session once. At the base the comparison read the connected address, the peer's real IP, which never folds. Cost: one spurious drop and a reconnect on the peer's next pass; the precondition (a loopback self-entry on the peer, one port, removal of the own entry) is why this is non-blocking.
// ReplicationServer.disconnectRemovedReplicationServers()
final Set<HostPort> newRSAddresses = getConfiguredRSAddresses();
// This server holds no session with itself, and a peer naming a loopback address on this
// port reads as this server under HostPort.equals(): skip self, as runConnect() does.
final HostPort localAddress = HostPort.localAddress(getReplicationPort());
for (HostPort oldRSAddress : oldRSAddresses)
{
if (!newRSAddresses.contains(oldRSAddress) && !oldRSAddress.equals(localAddress))
{
serversToDisconnect.add(oldRSAddress);
}
}Pin: the fold needs one port on two machines, which the in-JVM fixture cannot give; a case whose fake peer names the server's own address (loopbackAt(ports[0])) and whose removal list holds only that entry asserts the registered handler survives — it pins the guard, not the fold.
suggestion (non-blocking): The equals()-arm pin turns on the resolver answering NXDOMAIN for nonexistent.invalid, and nothing in the case checks that premise.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:284, :301-306
On a resolver that synthesises an A record for NXDOMAIN (consumer ISPs, captive portals, some corporate DNS) the name resolves to one address on both handlers: HostPort.normalizeHost() never logs, so the isEmpty at :306 holds vacuously, and isEquivalentTo() answers true on its own, so the equals()-dropped mutant survives while the case stays green. The record which proves the premise — ERR_COULD_NOT_SOLVE_HOSTNAME, logged once when the address is built at registration — lands before ERROR_TEXT_WRITER.clear() at :301 and is asserted nowhere. The kill in the mutant table is a property of the box it ran on; the case never goes red, so this is an environment-dependent pin, not a flake.
// before TestCaseUtils.ERROR_TEXT_WRITER.clear() at :301
assertThat(recordsContaining(TestCaseUtils.ERROR_TEXT_WRITER.getMessages(),
ERR_COULD_NOT_SOLVE_HOSTNAME.get(UNRESOLVABLE_HOST).toString()))
.as("the resolver of this box must not resolve " + UNRESOLVABLE_HOST)
.isNotEmpty();Or: a SkipException on a resolving name; the assertion is the one that says which network is wrong, and it doubles as the positive control the :306 isEmpty lacks.
suggestion (non-blocking): The connected-arm match of isSameServerAs() — two peers behind one gateway read as one — is pinned by no case.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:755-763
isSameServerAs() walks both of the other handler's addresses through isServerAt(). Every case where isAlreadyConnectedToRS() must answer true has the two handlers naming the same address (192.0.2.1:P twice, or nonexistent.invalid:P twice), and twoServersSharingAServerIdAreStillReported differs on both arms, so a mutant that iterates only the named arm of other.addresses answers the same on all six cases (reasoned from the inputs, not run). The description names the road as inherited from the base and pins its reporting counterpart; whether the silent drop on the connected arm is intended is readable from no test. The singletonList(namedAddress) row of the mutant table is killed through isConnectedToServerAt(), which leaves this call site's arm unmeasured.
// after registerPeerFrom(ports[0], peerAddress, baseDN) and waitForRegistration(domain):
// a second session from the address the peer is connected on, naming another address
final List<String> records =
errorLogRecordsOfHandshakeWith(rs, baseDN, loopbackAt(ports[1]), otherAddress);
assertThat(duplicateServerIdRecords(records, rs, loopbackAt(ports[1]), otherAddress))
.as("a second session from the address a peer is connected on is that peer")
.isEmpty();Pin: the named-arm-only mutant goes red on it — 198.51.100.1:P against 192.0.2.1:P and 127.0.0.1:P is false under both equals() and isEquivalentTo(), and only the connected arm carries 127.0.0.1:P.
suggestion (non-blocking): isAlreadyConnectedToRS() resolves the other handler's named host under the domain lock on the road where the two handlers name different hosts.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:185-219, :326-330, opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1440
lockDomainNoTimeout() at :185 → processStartFromRemote() → isAlreadyConnectedToRS() → isSameServerAs() → isEquivalentTo() → InetAddress.getAllByName() on both named hosts, per pair, whenever equals() fails on a pair; the accept road holds the lock at :328 for :330 the same way. Only RS-RS handshakes resolve there (DataServerHandler holds no such call); DS handshakes on the domain wait on the lock. On a genuine duplicate id, or a round-robin name whose first address differed at the two constructions, the old handler's name is cold once per JVM TTL, and a slow or dead resolver then holds the domain lock for the resolver timeout: lockDomainWithTimeout() callers on that domain fail with WARN_TIMEOUT_WHEN_CROSS_CONNECTION (3-8 s), lockDomainNoTimeout() callers wait. The base had the same shape (toServerAddressURL() resolved the peer's named host under this lock), so this widens it by one name on a rarer road rather than adding a class; the description states the per-call resolve and its log cost, not that the handshake road does it under the lock. No code change asked — a clause in the isServerAt() javadoc ("resolves under the domain lock on the handshake road") is the cheap fix.
nitpick (non-blocking): The isConnectedToServerAt() javadoc bounds the hide narrower than the code: the fallback host name a peer names matches whatever it resolves to on this server, no address configured twice needed.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerDomain.java:1051-1056
setServerURL() falls back to InetAddress.getLocalHost().getHostName() for the NAT peer this PR is for (ReplicationServer.java:1884-1887); setServerAddresses() builds that name here, and isServerAt() matches whatever this server's resolver gives it — a clone host name, split DNS or a stale /etc/hosts line pointing at another RS's address hides that RS with nothing configured twice. Suggested: "so that takes a peer whose named address resolves here to one this configuration gives to another replication server: two of them configured at one address, or the host name setServerURL() falls back to mapped there by this server's resolver".
nitpick (non-blocking): "as two servers which name the same resolvable address already are" reads as base behaviour; the base compared connected addresses only.
opendj-server-legacy/src/main/java/org/opends/server/replication/server/ReplicationServerHandler.java:782-784
At the base isAlreadyConnectedToRS() compared getServerAddressURL() strings over socket-derived URLs, so two servers naming one address were told apart by their source addresses; "already" is true of the isEquivalentTo() arm beside it, which is this PR's, and the description says "are now read as one". Suggested: "as two servers which name the same resolvable address are under the other arm".
nitpick (non-blocking): The class javadoc says the connect thread dials the documentation address "once per pass until the peer has registered"; the blacklist makes it one dial per six passes.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:72-74, :417-418
ReplicationServer.runConnect() blacklists a failed address for six tickets (:632, :640): a dial at ticket T is skipped at T+1..T+5 and repeated at T+6. Suggested at :74: "once per six passes -- a failed dial is blacklisted for the five after it -- until the peer has registered"; at :417-418: "comes back to it six passes later".
nitpick (non-blocking): The class javadoc names ReplicationServerHandler.toServerAddressURL() in the present tense; this PR removes it.
opendj-server-legacy/src/test/java/org/opends/server/replication/server/MultiHomedPeerTest.java:56-57
git grep toServerAddressURL at the head returns this line alone; the method is replaced by setServerAddresses() (ReplicationServerHandler.java:118-126). The paragraph narrates the base, but the tense says it is current. Suggested: "ReplicationServerHandler.toServerAddressURL(), which this change removes, took the host of a handler from ...".
…ress it names, not by the one its session came from A remote replication server was identified by the address the socket of one of its sessions happened to carry, which `ServerHandler.toServerAddressURL()` takes from `session.getRemoteAddress()`. That address is the interface a connection used rather than an identity, so a peer reachable at more than one address -- a multi homed host, or a NAT where the address a peer connects from is not the address it is configured as -- was never recognised as the peer configured at its own address. A remote server is now known by both of the addresses it comes with, the one it names in its start message and the one its session came from, and either identifies it. They are resolved once, when the start message names them, because the connect thread compares them on every one of its passes and `HostPort` logs a name it cannot resolve each time one is built from it. Three places read the address, and all three were wrong for such a peer: * `runConnect()` skipped a configured peer only when a handler was registered under that exact address, so it dialled a peer it was already connected to about once a second, and every one of those handshakes aborted and logged `ERR_DUPLICATE_REPLICATION_SERVER_ID` or `ERR_RS_DISCONNECTED_DURING_HANDSHAKE` with no throttle, for as long as the peer stayed where it was; * `isAlreadyConnectedToRS()` compared the address URLs of the two handlers as strings, so a second session with a peer already connected read as two replication servers sharing a server id, which is a misconfiguration this topology does not have; * `stopReplicationServers()` compared the addresses removed from `ds-cfg-replication-server` against the registered one, so a peer an administrator took out of the topology kept its session. The comparison is `HostPort.isEquivalentTo()`, which is what the data server side already uses for the same question in `ReplicationBroker`: `HostPort.equals()` resolves only the first address a name maps to, and the multi homing that leaves it blind to is noted in a FIXME of its own in `normalizeHost()`. What this gives up is the peer whose configuration was copied whole: two servers which share a server id and name the same address are now read as one and the second session is dropped in silence, where the source addresses used to tell them apart. Two servers which name different addresses are still reported, which is every duplicate id an administrator can act on.
…never sees connected now costs The comment OpenIdentityPlatform#935 left in ReplicationServer.connect() justified closing an outage on the connection alone with the multi homed peer runConnect() could never match, and described that peer by what the old comparison did with it: registered under the source address of its session, never matched, its handshake aborted on a duplicate server id. Both addresses now identify it, so what is left of that reading is the peer which names an address this configuration does not use, and its handshake is resolved as a cross connect rather than reported as a duplicate. The last paragraph said the pass after a cross connect says what is true, which holds for the peer runConnect() can match and not for this one.
…can resolve, and pin each call site The comparison a remote replication server is recognised by answers false for a host name this server cannot resolve: InetAddress.getAllByName() throws for it and HostPort.isEquivalentTo() reads that as "not the same server", even against the same name. So the peer of the rewritten connect() comment -- the one which falls back to naming the host name of its machine, which the rest of the topology has no reason to resolve -- was still reported as two servers sharing a server id, once per dial. It is now compared as the name it is, which is what HostPort.equals() does with it. What that gives up is two servers which share a server id and both name one unresolvable name on one port: they are read as one, as two which name the same resolvable address already were. Each of the three call sites is now pinned by a case of its own: * runConnect(): the first case records an outage for the configured address and asserts NOTE_REPLICATION_SERVER_CONNECT_RESTORED, which the already connected branch writes and nothing else does -- a second dial writes nothing at all, ConnectFailureReporter recording a peer as DOWN once and connect() having no debug branch under it; * the host half of isServerAt(): the second server of the negative case moved to the port of the first, so the host is what tells the two apart; * the connected arm of the addresses: a case whose peer names an address this configuration does not use and is found by the one it dialled from. Also the javadoc: "built once" rather than "resolved once", and what the comparison resolves on every pass; what the connect thread does with a documentation address, which is a SYN to the default route rather than nothing leaving the machine; the fixture of ReplicationServerConnectFailureTest, which is the peer whose two addresses the configured one matches neither of rather than the multi homed peer; the name of ReplicationServerHandler.toServerAddressURL(); and what trusting the address a peer names costs, in ReplicationServerDomain.isConnectedToServerAt().
…disconnect list, and pin the arms no case read HostPort normalises every address local to this machine to localhost, so the entry of this server and the loopback address a peer names for itself are one address on one port: a peer whose own configuration lists localhost:P for itself names exactly that in its start messages, and this server reads that name as its own entry. disconnectRemovedReplicationServers() handed that entry to the domain like any other, so removing it -- a configuration which does not list this server is supported -- stopped the session of a healthy peer, which then had to dial again. At the base the comparison read the connected address, the real address of the peer, which never normalises that way. The entry of this server is now skipped there, as runConnect() skips it before it dials. Three roads the cases reached without reading are pinned: * removingTheEntryOfThisServerStopsNoPeer registers two peers, one naming the address this server listens on and one naming its own entry, and removes both entries in a single applyConfigurationChange(): the guard alone tells the two apart, so the case is red without it and the peer whose own entry went is the positive control of the road. * aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeer answers a handshake from the address the registered peer is connected on while naming another address, which only the connected arm of isSameServerAs() can match. That arm carries what this change gives up -- two servers behind one gateway read as one -- and no case stated it. * The unresolvable case now asserts the ERR_COULD_NOT_SOLVE_HOSTNAME record its premise rests on before it clears the log. A resolver which answers a name which does not exist would have isEquivalentTo() carry that case on its own, leaving the equals() arm unread and the count of that record vacuous, with nothing red to say so. The javadoc says three things it did not: isServerAt() resolves under the domain lock on the handshake road, whose cost is the domain rather than one comparison; isConnectedToServerAt() is hidden from by any named address which resolves here to another replication server, the fall back host name of setServerURL() included, not only by two servers configured at one address; and the pair the equals() arm reads as one is read as one under the other arm too, which is this change rather than the base. In the test, toServerAddressURL() is named in the past tense, and a dial of an address nothing answers at is one per six passes, the blacklist of OpenIdentityPlatform#935 holding the five in between.
9e51b42 to
cde2f96
Compare
|
@maximthomas round 3, issue (non-blocking) -- The port it reads is the new one, which is the right reading of the one case where the two Pinned by suggestion -- the Measured: with suggestion -- the connected arm of suggestion -- nitpick -- the nitpick -- "as two servers which name the same resolvable address already are". Taken: "are nitpick -- the class javadoc says the connect thread dials once per pass. Taken in both nitpick -- One correction to the table of round two. Runs, on the head of this round:
Rebased on the current Still not taken, and unchanged from round two: the addresses |
maximthomas
left a comment
There was a problem hiding this comment.
praise: Every new pin was measured red against the mutant it is there for.
ReplicationServer.disconnectRemovedReplicationServers():2049: removing the guard turnsremovingTheEntryOfThisServerStopsNoPeerred ("Expecting [] to contain [8252]"). That case removes both entries in one change, so the guard alone tells the two peers apart.aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeerkills the named-onlyisSameServerAs()mutant andaddresses = singletonList(namedAddress). Restoring the base skip inrunConnect()now turnsaPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddressred. Both mutants survived 4/4 at 58c7218.MultiHomedPeerTest:309-313: the unresolvable case now asserts its own premise before it clears the log.
Fixes #1017.
What was wrong
A remote replication server was identified by the address the socket of one of its sessions happened to carry:
ServerHandler.toServerAddressURL()takes the host of a handler fromsession.getRemoteAddress()and its port from the start message that handler received. That address is the interface a connection used, not an identity, so a peer reachable at more than one address — a multi homed host, or a NAT where the address a peer connects from is not the address it is configured as — is registered under an address nothing in the configuration ever names.Three places read that address, and all three were wrong for such a peer:
runConnect()skipped a configured peer only when a handler was registered under that exact address, so it dialled a peer it was already connected to on every pass, about once a second, for as long as that peer stayed where it was;isAlreadyConnectedToRS()compared the address URLs of the two handlers as strings, so the second session with a peer already connected read as two replication servers sharing a server id — a misconfiguration this topology does not have;stopReplicationServers()compared the addresses removed fromds-cfg-replication-serveragainst the registered one, so a peer an administrator took out of the topology kept its session and went on replicating. This one is not in the issue, which is about the log volume; it is the same defect at a third call site and it is a functional one, so it is fixed here rather than left to be found again.Which message the flood carries depends on the peer, and the issue names only one of the two. The inbound path checks
isAlreadyConnectedToRS()before it answers (ReplicationServerHandler.startFromRemoteRS(), ahead of itssendStartToRemote()), so a peer which already holds a handler for this server never answers the start message it is offered: the dialling side then reads EOF and logsERR_RS_DISCONNECTED_DURING_HANDSHAKE(theIOExceptionbranch ofReplicationServerHandler.connect()), whileERR_DUPLICATE_REPLICATION_SERVER_IDis logged by whichever end has the mismatched pair of addresses. Both areERR, neither is throttled, and both are one line per pass.The fix
A remote server is now known by both of the addresses it comes with — the one it names in its start message, which is the address it is configured under, and the one its session came from — and either of them identifies it.
ReplicationServer.setServerURL()builds what a peer names from the configured address which is local to it, so the named address is the one the rest of the topology configures that peer at.The addresses are built once, when the start message names them, rather than on each comparison: the connect thread compares them on every one of its passes, and
HostPortlogsERR_COULD_NOT_SOLVE_HOSTNAMEeach time one is built from a name it cannot resolve — which the fall back ofsetServerURL()to the host name of the machine makes an ordinary thing for a peer to name. Building them per pass would have traded one unthrottled line per second for another. The comparison itself does resolve, on every call, and reports nothing above trace when it cannot.The comparison is
HostPort.isEquivalentTo(), which is what the data server side already uses for this question inReplicationBroker.isSameReplicationServerUrl(), andHostPort.equals()beside it. Both are needed, and for opposite reasons.isEquivalentTo()resolves both hosts, which is what sees through a name with several A records:equals()compares the normalized hosts, which hold the first address a name maps to, and the multi homing that leaves it blind to is written down in a FIXME of its own innormalizeHost(). But a host name neither end can resolve makesInetAddress.getAllByName()throw, andisEquivalentTo()reads the exception as “not the same server” — the same name is not equivalent to itself. That name is whatsetServerURL()falls back to when none of the addresses a peer is configured with is local to it, which is the peer of this change, andnormalizedHostholds it as the name it is where resolution failed. SoisServerAt()answers on either:address.equals(known) || address.isEquivalentTo(known).What this gives up
The peer whose configuration was copied whole: two servers which share a server id and name the same address are now read as one, and the second session is dropped in silence where the source addresses used to tell them apart. The
equals()arm carries that to the name which resolves nowhere: two servers which share a server id and both name one unresolvable name, on one port, are read as one as well.Two servers which name different addresses are still reported — with one exception, and it is the behaviour of the base rather than of this change: two peers behind one gateway, dialling in on one port, carry one and the same address on the connected arm of both handlers, and the second is dropped in silence, exactly as it was when that address was compared as a string.
twoServersSharingAServerIdAreStillReportedpins the reporting of the pair an administrator can act on, andaSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeerpins the silent drop, so what is given up here is readable from a test rather than from this paragraph alone.And the address a peer names is what that peer says of itself, which now decides
runConnect()as well: a peer which names an address this configuration gives to another replication server hides that one from the connect thread, which then dials nothing for it and reports it connected while it is down. It takes two servers configured at one address — a configuration copied whole, or two sites whose private ranges overlap — and the peer it hides is still the one which dials this server.ReplicationServerDomain.isConnectedToServerAt()says so.Left out on purpose
abortStart().logger.error(reason)inServerHandler.abortStart()is still unthrottled. [#911] Report the replication connections which used to be dropped in silence #935, which this branch now sits on, is whereFailureLogThrottlelives, and it also hasconnect()blacklist a peer whose handshake aborted for six connect passes, where an abort used to count as a connection; putting a second throttle here would collide with it. This change removes the mis-dialling that produced the volume, rather than bounding the volume.ERR_DUPLICATE_REPLICATION_SERVER_IDprints. The line names the two connected addresses, while the decision is now made on the named ones as well. After theequals()arm above, no pair it reports carries one address twice, so what is left is a line which does not show everything the decision read — a change of the message and of the case which pins its text, and one for a change of its own.runConnect()can never skip a peer whose configuration names it by something this server's configuration does not use. What that costs is bounded by the blacklist of [#911] Report the replication connections which used to be dropped in silence #935, not by this change.The test
MultiHomedPeerTestdrives the mechanism rather than a stand in for it. The fake peer dials the server under test over the loopback interface and names a documentation address (RFC 5737) in its start message, which is what a peer behind a NAT looks like to the server it dials: it registers under127.0.0.1:Pwhile it is configured, and names itself, as192.0.2.1:P. The handshake the server then offers it runs on a session which reports the address it dialled, which is what reaching that peer at its configured address gives. The addresses named are documentation addresses (RFC 5737): nothing answers at them, which is what the connect thread of the server under test finds when it dials one — a host stack sends that SYN to its default route, and the connect fails, once per pass until the peer has registered.The registration under the other address is asserted as a precondition rather than assumed, because a handshake which ends any other way registers nothing at all and would leave the cases asserting something else.
Without the fix, three of the cases fail, and on the mechanism rather than beside it (measured on the first commit, so the line numbers are the ones it had):
Round two of review: a mutant per road
The run above reaches the predicate and the three call sites together, which left each of them pinned by the same thing. The cases now stand apart, and each one is measured against a mutant of its own — every mutant run against the whole class, all six cases:
address.equals(known)dropped fromisServerAt()aPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId(the duplicate id line)address.getPort() == known.getPort()in place of the comparisontwoServersSharingAServerIdAreStillReportedaddresses = singletonList(namedAddress)aPeerWhichNamesAnAddressThisConfigurationDoesNotUseIsFoundByTheOneItDialledFromHostPorts rebuilt insideisServerAt()on every callaPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerId(theERR_COULD_NOT_SOLVE_HOSTNAMEline)ReplicationServer.java:612,isConnectedToServerAt()untouchedaPeerRegisteredUnderTheAddressItDialledFromIsFoundByItsConfiguredAddress(the connect thread line)The last of those is why the first case now records an outage for the configured address (
rs.connect(peerAddress, baseDN)) and waits forNOTE_REPLICATION_SERVER_CONNECT_RESTORED: that record is written by the already connected branch ofrunConnect()and by nothing else, while a second dial writes nothing at all —ConnectFailureReporterrecords a peer as DOWN once, andconnect()has no debug branch under it.Round three of review: the entry of this server, and the arms no case read
HostPort.normalizeHost()folds every address local to this machine tolocalhost, so the entry of this server and the loopback address a peer names for itself are one address on one port: a peer whose own configuration listslocalhost:Pfor itself names exactly that in its start messages, and this server reads that name as its own entry.disconnectRemovedReplicationServers()handed that entry to the domain like any other, so removing it — a configuration which does not list this server is supported, andrunConnect()says so — stopped the session of a healthy peer, which then had to dial again. At the base the comparison read the connected address of that peer, its real one, which never folds that way. The entry of this server is now skipped there asrunConnect()skips it before it dials, and by the same test:!oldRSAddress.equals(HostPort.localAddress(getReplicationPort())), which covers every address local to this machine on the port this server listens on rather than the literal entry.Three roads the cases reached without reading are pinned, one mutant each:
disconnectRemovedReplicationServers()removingTheEntryOfThisServerStopsNoPeerisSameServerAs()walking the named arm of the other handler onlyaSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeernonexistent.invalidreplaced by a name which resolvesaPeerWhichNamesAnUnresolvableHostIsNotReportedAsADuplicateServerIdremovingTheEntryOfThisServerStopsNoPeerregisters two peers — one naming127.0.0.1:ports[0], the address this server listens on, and one naming its own entry at127.0.0.1:ports[1]— and removes both entries in a singleapplyConfigurationChange(), so the guard alone tells the two apart: without it the first peer loses its session, while the second, which does go, is what says the road reachedstopReplicationServers()rather than returning early.aSecondSessionFromTheAddressAPeerIsConnectedOnIsReadAsThatPeeranswers a handshake from the address the registered peer is connected on while naming another address, which only the connected arm ofisSameServerAs()can match — the arm which carries what "What this gives up" says of two peers behind one gateway. And the unresolvable case now asserts theERR_COULD_NOT_SOLVE_HOSTNAMErecord its premise rests on before it clears the log: a resolver which answers a name which does not exist would haveisEquivalentTo()carry that case on its own, leaving theequals()arm unread and the count of that record vacuous, with nothing red to say so.One row of the table of round two moves with them:
addresses = singletonList(namedAddress)now kills two cases, its own and the new connected arm one, which reads the same road from the other side.The javadoc says three things it did not.
isServerAt()resolves a pair the names alone do not answer on every call, and both handshake roads hold the domain lock acrossisAlreadyConnectedToRS(), so what a resolver which does not answer holds is the domain rather than one comparison — and only replication servers reach it, a data server handshake making no such comparison.isConnectedToServerAt()is hidden from by any named address which resolves here to one this configuration gives to another replication server, the fall back host name ofsetServerURL()included, not only by two servers configured at one address. And the pair theequals()arm reads as one is read as one under the other arm as well, which is this change rather than the base. In the test,toServerAddressURL()is named in the past tense as the method this change removes, and a dial of an address nothing answers at is one per six passes, thedomainTicket + 6blacklist of #935 holding the five in between.The branch was rebased on the current
masterbefore this round, so the round commit carries the round and nothing else: the three commits replayed with no conflict, andgit range-diffreports=for all three. Two of the commits which landed since do touch these files — #1019, which reports aReplicaOfflineMsgas forwarded only to a peer which can decode it, and #987, which interrupts the listen thread after those messages are forwarded — and neither touches what this change owns.Rebased on master after #935 and #976
The one conflict was in
runConnect(): #935 hoistedgetConfiguredRSAddresses()into a local it reads again further down, forconnectFailures.retainAll(), while this change replaced theconnectedRSAddressesset next to it. The resolution keeps that local and putsdomain.isConnectedToServerAt()whereconnectedRSAddresses.contains()was, which is the same two-line change as before against the new base;getConnectedRSAddresses()is gone as before. Nothing else #935 or #976 touched overlaps with this change in code: the skip branch ofrunConnect()now also reports the peer as reachable again, andconnect()now returns whether the handshake completed, and both are as right for a peer recognised by the address it names as for one recognised by the address it came from.What did overlap is a comment. #935 justifies closing an outage on the connection alone, in
ReplicationServer.connect(), with the multi homed peerrunConnect()could never match, and it describes that peer by what the old comparison did with it -- registered under the source address of its session, never matched, its handshake aborted on a duplicate server id. The second commit rewrites the two paragraphs which said that: what is left of that reading is the peer of "A complete skip test" above, the one which names an address this configuration does not use, and its handshake is resolved as a cross connect rather than reported as a duplicate. The last paragraph said the pass after a cross connect says what is true, which holds for the peerrunConnect()can match and not for this one: nothing more is said of it where no outage was reported, the one warning stays where one was, and the blacklist bounds the dialling.Verified locally, on the branch rebased on master
MultiHomedPeerTest, the classes [#917] Wait for every peer replication server to forward the ReplicaOfflineMsg #947 rewrote, and the one [#911] Report the replication connections which used to be dropped in silence #935 added for the connect thread, whose skip branch this change decides — 91 tests, no failures (80 before the rebase, the currentmastercarrying more of them). Run as three invocations ofmvn -Pprecommit -pl opendj-server-legacy verify -Dit.test=…over four, four and five classes rather than one, because this machine was building other branches at the same time and the single invocation was killed for memory before any test ran.mvn -pl opendj-server-legacy package -Dmaven.test.skip=true— theattach-javadocsexecution, withdoclint=all,-missingandfailOnWarnings=true, passes, the new{@link ReplicationServerDomain#isAlreadyConnectedToRS(ReplicationServerHandler)}included.singletonList(namedAddress)killing two, as the round three section says. Two of those runs failed insetUpon binding a port of the embedded server, another JVM on this machine holding0.0.0.0:65528and0.0.0.0:65534, and were green when re-run.ReplicationServerDomain.isConnectedToServerAt()present but still comparing whatgetConnectedRSAddresses()used to compare — so the cases fail on the comparison rather than on a missing method. Each of them reaches one of the three call sites and no other, so that run pins all three: the first the one inrunConnect(), the second the one inisAlreadyConnectedToRS(), the third the one instopReplicationServers().