Do InvalidateRoutes when a forwarded packet updates a MAC cache entry #1570
Conversation
When we get a forwarded packet, we put its source MAC into the MAC cache, overriding any existing entry. But we could have a flow which says to forward packets destined to that MAC based on the old entry. Such a flow could be prolonged indefinitely by attempts to send to the MAC. This is obviously bad, so do InvalidateRoutes in such cases.
|
A while back I asked why we didn't update the MAC cache for relayed packets and you gave a (convincing) rationale against doing so. It's not clear to me that it extends to cover the cache invalidation case though - thoughts? |
This PR addresses a discrepancy between the MAC cache and the flows which can persist indefinitely. By discrepancy I mean a case where the MAC cache says "packets for this MAC go to this peer", but the flows don't actually correspond to that. When an outgoing flow is created (one taking a packet from the Bridge to the Overlay) in handleCapturedPacket, it reflects the state of the MAC cache entry for the destination MAC at the time is was created. A discrepancy can arise if the MAC cache entry then changes. Relayed packets don't update the MAC cache, and so don't create such a discrepancy. It's certainly possible to imagine using information from relayed packets to update the MAC cache. But we don't do that. I detect some concern that changes such as this one seem rather ad-hoc, and perhaps that it is hard to be confident that they are addressing the whole problem. I'd agree. The relationship between the MAC cache (the dest MAC -> dest peer function), the forwarding map (the dest peer -> next hop function), and the peer <-> short ID relation is the weakest area of the fastdp design. (Even if one is optimistic about the correctness of the code, it is unpleasant that it invalidates all the flows whenever there is a possibility that we have to invalidate any flow.) I have some ideas for a comprehensive solution which reframes the core of the router as a composition of those functions, and might be able to encompass bridging too. But I'm not sure there is the appetite for router changes on that scale, or that it can be justified on the basis of any conspicuous consequences of the current design. |
Indeed - that was the subject of my original inquiry. However, assuming that such a discrepancy arises anyway, what is the harm in using the source of relayed traffic to detect and take corrective action? Note I'm not suggesting that we insert new entries based on relayed traffic - I accept the argument you advanced before for not doing that. However, this is different - we're talking about the situation where a) this host does already have an entry for the MAC and b) it is stale. In that case, can we not delete the entry and purge the flows? |
I'm always willing to discuss the pros and cons of changes, but I don't think this is the right place to be discussing the change you are proposing. While I feel that the changes in this PR are of very low risk, I have not submitted it on the basis of "what could be the harm". It's too close to the 1.2 release for that. Rather, I have a specific concern about a specific weakness in the code. So do you have any comment on the changes in this PR, or their rationale? < 8000 /td> |
I suspect then that I have failed to articulate properly what I mean. There's nothing at all wrong with this PR, or the motivation; I was merely observing that there is an additional source of information regarding MAC address relocation that we could also take advantage of. Alternatively, perhaps I have missed some subtlety - lets talk about it in person. In any case, it would be an incremental change on top of this PR so I will merge it now. |
Do InvalidateRoutes when a forwarded packet updates a MAC cache entry
When we get a forwarded packet, we put its source MAC into the MAC cache,
overriding any existing entry. But we could have a flow which says
to forward packets destined to that MAC based on the old entry. Such
a flow could be prolonged indefinitely by attempts to send to the MAC.
This is obviously bad, so do InvalidateRoutes in such cases.