ts_control*, ts_runtime, etc: handle PeerChange, OnlineStatus, PeerSeen messages#185
ts_control*, ts_runtime, etc: handle PeerChange, OnlineStatus, PeerSeen messages#185dylan-tailscale wants to merge 1 commit into
Conversation
dylan-tailscale
force-pushed
the
dylan/peer-patch
branch
from
cd95736 to
9dfc064
Compare
npry
left a comment
npry
left a comment
There was a problem hiding this comment.
minor nits and questions, nonblocking — lgtm!
| /// do not take into account differences between the control plane clock and local device clock. See | ||
| /// each variant for more information. | ||
| #[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)] | ||
| pub enum NodeLastSeen { |
There was a problem hiding this comment.
Nonblocking: why an enum rather than a struct? Mightn't we want to know both pieces of info?
There was a problem hiding this comment.
Hmm not sure I follow; I view NodeLastSeen::Control as definitive/correct, and NodeLastSeen::Estimated as a fallback for when the control plane doesn't tell us when it last saw the node (because it's using MapResponse::peer_seen_change to report it). In other words, I only see NodeLastSeen::Estimated as a rare last resort to give us something, albeit probably pretty inaccurate compared to Nodes with a populated NodeLastSeen::Control variant.
I can see us always calculating/storing an estimated timestamp when we receive an online/offline update for a Node just so we have a rough timestamp that's synced with the local device clock, although that timestamp won't take into account clock skew between the control plane/local device, nor transit delay for the message. This would at least give callers the ability to always compare NodeLastSeen values between Nodes using estimated timestamps...I guess it depends on the use case whether or not that's acceptable?
Lmk if I misunderstood - happy to change this to a struct if it's useful! Just want to make sure I understand the use case so I can properly document any caveats.
There was a problem hiding this comment.
I think I see it the other way, since NodeLastSeen::Control is intentionally obfuscated, so if I'm a user trying to implement some logic around when another peer was last reported online, it's kind of useless — I want to know the local point-in-time when we last saw control change the node's online status. Unless the point is that control delays those updates to 10m boundaries also? I assumed not but if so then this makes sense
…ndle PeerChange, OnlineStatus, PeerSeen messages
Applies peer updates from MapResponse::{peers_changed_patch, peer_seen_change,
online_change} to the peer tracker's `Node` state. Introduces a number of
types to ease handling of patch/delta updates, especially concerning online
and last-seen timestamps. Updates order that delta updates are applied to match
the Golang codebase.
Signed-off-by: Dylan Bargatze <dylan@tailscale.com>
dylan-tailscale
force-pushed
the
dylan/peer-patch
branch
from
9dfc064 to
3cb2b02
Compare
2 participants
Applies peer updates from
MapResponse::{peers_changed_patch, peer_seen_change, online_change}to the peer tracker'sNodestate. Introduces a number of types to ease handling of patch/delta updates, especially concerning online and last-seen timestamps. Updates order that delta changes are applied to match the Golang codebase.Closes #41.