Open
Conversation
Db.peers was only used as a cache for reporting peers to ddm-api clients calling GET /peers, and the contents of the cache were a bit misleading. For example, there is no such thing as a peer state in DDM. DDM FSMs have states, but they are instantiated per interface not per peer. Not only does the state reported by GET /peers not actually map to a peer, but it doesn't even directly map to the FSM of the peer's interface. Instead, that state was populated by manual db updates scattered throughout the FSM implementation. This removes Db.peers and introduces a new shared InterfaceState struct that consolidates peer/FSM info for a given interface. GET /peers now reads from each InterfaceState and passes along any peers it finds along with interface state. Also fixes a pre-existing bug in oxstats where interface names were always empty due to reading from a stale SmContext clone.
After adding PeerIdentity, the peer_address field of SessionStats is now redundant (and also isn't a stat) so this removes it. The peer address is tracked in PeerIdentity, so we can just rely on that field behind a single mutex instead of locking/juggling both stats and identity. Also adds a couple helper methods for transitioning FSM state and updating the peer state.
taspelund
added
ddm
Contributor
Author
|
I've manually tested this in a4x2 and proven that everything still works as expected. I've also manually verified that The open question that I still have here: is |
Contributor
Author
|
One other thing: I am also happy to add an FSM history buffer for DDM that mirrors what we have in BGP, especially if we don't believe that this PR fully addresses the visibility issue described in #54 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
At its heart, this is a re-implementation of #180 which has been updated for the modern codebase and offers some cleanup in addition to timestamps.
First and foremost, this PR adds duration tracking for DDM FSM states. It does so by updating the FSM state enum variants to hold a std::time::Instant representing the timestamp when the FSM entered that state, and calculating a Duration using .elapsed() when the FSM state is queried (i.e. when
ddmadm get-peersis called).Part of this work was to cleanup the implementation of peer tracking: namely getting rid of the Db.peers table and instead tracking a peer in each FSM. Since the FSMs are instantiated per interface, this naturally gives us the interface-to-peer mapping structure so we don't actually need to maintain a separate table for the peers. Instead, we update/query the peers directly from the FSMs.
There are also a couple quality of life improvements: putting the state duration in a separate column and sorting the output of
ddmadm get-peers.Fixes: #54