pjfanning opened a new pull request, #3676:
URL: https://github.com/apache/pekko/pull/3676
### Motivation
When two Artery systems share a system uid, the system that detects the
collision (the receiver) rejects the
colliding peer's handshake, but the colliding peer can still deliver
messages to the receiver's actors (#3672):
- The colliding peer completes its own outbound handshake when it receives
the receiver's `HandshakeReq`
(receiving a `HandshakeReq` completes the handshake in the reverse
direction).
- The Artery envelope header identifies the sender only by its uid. The
receiver's decoder resolves that uid to the
existing healthy association and `InboundHandshake.isKnownOrigin` accepts
the message. Inbound connections are
merged before decoding, so the receiver cannot tell which peer sent it.
- Replies go to the colliding peer's address, whose handshake never
completes, so they are not delivered.
On collision, `ArteryTransport.completeHandshake` called
`quarantine(peer.address, Some(peer.uid), ...)`. That
is a no-op: the association to the colliding address has no unique remote
address yet, so it only logs
"Quarantine of [...] ignored because handshake not completed".
The issue lists two directions. Putting the sender's address in the
handshake or envelope header changes the wire
format. This PR takes the other one: the receiver tells the colliding peer
to stop, using only existing messages.
### Modification
On `UidCollisionException`, `ArteryTransport.completeHandshake` now sends
the existing `Quarantined(localAddress, peer)`
control message to the colliding peer's address, replacing the no-op
quarantine call.
How it works:
1. The receiver detects the collision when handling a `HandshakeReq` or
`HandshakeRsp` from the colliding peer. As
before, it logs the `UID collision old [...] new [...]` warning and does
not send a `HandshakeRsp`.
2. It sends `Quarantined(receiver, collidingPeer)` with
`sendControl(peer.address, ...)`. `sendControl` injects the
message through `OutboundControlJunction`, which comes **after**
`OutboundHandshake` in the outbound control
stream. The message is not held back by the unfinished outbound handshake
to the colliding peer, just as a
`HandshakeRsp` is sent before the reverse handshake has completed. (The
issue assumed it would be held back.)
3. The colliding peer has already received the receiver's `HandshakeReq`
(that is what completed its outbound
handshake), so it knows the receiver's uid and its `InboundHandshake`
accepts the message. The existing
`Quarantined(from, to) if to == localAddress` handling then logs `Other
node [...] quarantined this node.` and
publishes `ThisActorSystemQuarantinedEvent`.
4. In a cluster, the Split Brain Resolver reacts to
`ThisActorSystemQuarantinedEvent` with
`DownSelfQuarantinedByRemote`, provided the receiver is a member in the
colliding node's view. This takes the
colliding node out of the cluster, so its messages stop.
5. The receiver sends `Quarantined` each time it detects the collision, i.e.
on each handshake retry or liveness
probe from the colliding peer. If the first one arrives before the
colliding peer has handled the receiver's
`HandshakeReq`, the peer drops it as coming from an unknown uid, and a
later one gets through. This is the same
kind of unthrottled send that `InboundQuarantineCheck` already does for
quarantined peers.
The healthy association that owns the uid is not touched.
### Result
A peer whose uid collides with an existing association is now told that it
has been quarantined, instead of the
collision only being visible on the receiver. Operators see a warning on
both systems, and cluster nodes using the
Split Brain Resolver down the colliding node. Wire format is unchanged.
Remaining limitation: without a cluster (plain remoting), the colliding peer
only logs the warning and publishes
the event; nothing stops it sending. The receiver still cannot tell its
messages from those of the healthy peer
with the same uid. Fixing that fully needs the sender's address on the wire,
which is the wire-format direction
in #3672 and needs a rolling-upgrade plan of its own.
### Rolling updates with older Pekko versions
No new message, manifest or field is added: `Quarantined` is an existing
Artery control message, serialized by
`ArteryMessageSerializer` with the existing manifest. Mixed-version clusters
behave as follows.
**New receiver, older colliding peer (Pekko 1.x or 2.0.0).**
All Artery versions handle an inbound `Quarantined(from, to)` addressed to
them in the same way, with the warning
and `ThisActorSystemQuarantinedEvent`. I checked the `1.0.x`, `1.1.x` and
`1.7.x` branches. The Split Brain Resolver
subscribes to that event and downs the node on all of them. So an older
colliding node gets the full benefit of
this change.
**Older receiver, new colliding peer.**
No change from today. Older receivers (1.x) throw
`IllegalArgumentException("UID collision ...")` from
`AssociationRegistry.setUID` and do not send `Quarantined`. 2.0.0 receivers
with #3652 reject the handshake but
only make the no-op quarantine call. Either way, the colliding peer is not
told. The behaviour only improves once
the system that *detects* the collision runs a version with this change.
**Both new.**
As described above.
**Collision likelihood in mixed clusters.**
Nodes from before #3652 (and classic remoting) use a random 64-bit uid, so
collisions with them stay as unlikely as
before. This change does not affect how uids are generated.
**Classic remoting.**
Not affected. The change is only in the Artery transport.
**Binary compatibility.**
Internal change in `ArteryTransport.completeHandshake`; no signatures
changed.
### Tests
- `sbt "remote/testOnly
org.apache.pekko.remote.artery.RemoteConnectionSpec"`: both uid collision tests
(connections
TO and FROM two systems sharing a uid) now also assert that the colliding
system receives
`ThisActorSystemQuarantinedEvent` from the receiver. With the
`ArteryTransport` change reverted, both collision
tests fail. With it, all 4 tests pass.
- `sbt "remote/testOnly org.apache.pekko.remote.artery.RemoteConnectionSpec
org.apache.pekko.remote.artery.SystemUidSpec
org.apache.pekko.remote.artery.HandshakeRetrySpec
org.apache.pekko.remote.artery.InboundHandshakeSpec
org.apache.pekko.remote.artery.OutboundHandshakeSpec"`:
20 passed.
- `scalafmt` run on the changed files.
- MiMa not run: no signatures changed.
### References
Refs #3672 - addresses the "make the colliding peer drop its association"
direction without a wire format change;
the plain-remoting case still needs the wire-format direction.
Follows up #3652.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
