Fixes #1852 - Unmap addresses of a neighbour router when the inter-ro…#1853
Fixes #1852 - Unmap addresses of a neighbour router when the inter-ro…#1853gabordozsa wants to merge 1 commit intoskupperproject:mainfrom
Conversation
…n the inter-router link is lost
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
There was a problem hiding this comment.
As noted in the comment, this change unmaps a former-neighbor router's addresses when the router may still be reachable in the topology.
Furthermore, this doesn't address the general problem from the linked issue. For example, in a three-node network:
A -- B -- C
If the link between A and B is lost, addresses on C will not be unmapped by this PR and will suffer the long delay in notification.
| node = self.nodes[node_id] | ||
| node.remove_link() | ||
| if self.link_state.del_peer(node_id): | ||
| node.unmap_all_addresses() |
There was a problem hiding this comment.
This is not the right place to do this. The fact that the link has dropped does not mean that the (formerly) peer router has become unreachable. It's possible that the router and all of its addresses are still reachable via a different path.
There was a problem hiding this comment.
I understand. My assumption is that the most common reason for the link drop is that the peer router got stopped. This would be an optimisation for that common case.
I also think that this is a low risk change : if the peer is still alive and accessible then the router will ask for the addresses again. But again, this only makes sense if link failures are due to the peer being stopped most of the time.
I see this change as a trade-off: it only covers a peer router. If the router went down then multi-key listeners can switch to alternative addresses fast. If the (former) peer is still up then it will be asked eventually to send the addresses again. The addresses cannot be used in the interim period anyway. Regarding addresses on C, we could try to unmap all addresses from every (not peer) routers which used to be connected via the lost link. However, that may trigger lots of potentially unnecessary traffic if we have a lot of routers and they are still accessible via alternate links. |
|
Copying here @ted-ross example that explains why this change would not be correct: Proper soluiion is proposed in #1854 Closing. |
3 participants
…uter link is lost.
Fixes #1852
If the inter-router link is lost then likely the neighbour is down. If we unmap all of its addresses then multi-key listeners get a chance to switch to alternative addresses immediately. Otherwise, the addresses get unmapped when the
RouterNodeis deleted which happens if no hello message is received for a period ofremoteLsMaxAgeSeconds. The default is 60s.