Allow one way partitionings #187
Conversation
src/top.rs
Outdated
| @@ -332,14 +368,20 @@ impl Link { | |||
| let delay = self.delay(global_config.latency(), rand); | |||
| DeliveryStatus::DeliverAfter(self.now + delay) | |||
| } | |||
| State::ExplicitPartitionAToB if src.ip() > dst.ip() => { | |||
There was a problem hiding this comment.
This is a bit tricky to reason about as written. I assume this delivers in the non-partitioned link direction. Consider adding a comment on these branches.
There was a problem hiding this comment.
I agree. I'll see if I can make it clearer.
There was a problem hiding this comment.
I've tried to make it clearer.
src/top.rs
Outdated
| @@ -155,7 +155,15 @@ enum State { | |||
| Healthy, | |||
|
|
|||
| /// The link was explicitly partitioned. | |||
| ExplicitPartition, | |||
There was a problem hiding this comment.
This implementation strategy was chosen because it was the easiest to implement.
The disadvantage is that making oneway partitionings a specific type of link status means that it can't easily be combined with 'Hold' or 'RandPartition'. For my particular use case this is fine. I'm unsure of how common the need to combine 'Hold' or 'RandPartition' with one-way partitioning is.
There was a problem hiding this comment.
I haven't seen a need for this yet, but one idea to achieve this is to model a link with two direction states and allow setting either one. This also might make the A,B -> src,dst stuff cleaner. wdyt?
There was a problem hiding this comment.
I think it would be a lot cleaner if the 'links'-map in the 'Topology'-struct didn't deduplicate links. I.e, if the network topology between any two nodes 'a' and 'b' was described by two links, 'a'->'b' and 'b'->'a', instead of having one multidirectional link.
However, that feels like a rather big change, and it might also have other downsides.
We could just split 'state' in struct 'Link' into 'state_a_to_b' and 'state_b_to_a'. I worry that the tricky A,B->src,dst stuff would basically remain the same though?
There was a problem hiding this comment.
I agree we should hold off on a larger refactor. We've been discussing a larger restructuring of the network to support different ip versions and a more realistic lo for some time now. We can tackle that then.
This idea doesn't avoid complexity around the directions, but it does make it really clear what is happening in each direction. I think its worth doing that as part of this change.
struct Link {
a_b: State,
b_a: State,
}There was a problem hiding this comment.
I implemented your suggestion, as I understood it, and I think it was a big improvement. Thanks for the idea!
src/top.rs
Outdated
| @@ -332,14 +368,20 @@ impl Link { | |||
| let delay = self.delay(global_config.latency(), rand); | |||
| DeliveryStatus::DeliverAfter(self.now + delay) | |||
| } | |||
| State::ExplicitPartitionAToB if src.ip() > dst.ip() => { | |||
There was a problem hiding this comment.
I agree. I'll see if I can make it clearer.
src/top.rs
Outdated
| @@ -155,7 +155,15 @@ enum State { | |||
| Healthy, | |||
|
|
|||
| /// The link was explicitly partitioned. | |||
| ExplicitPartition, | |||
There was a problem hiding this comment.
I haven't seen a need for this yet, but one idea to achieve this is to model a link with two direction states and allow setting either one. This also might make the A,B -> src,dst stuff cleaner. wdyt?
There was a problem hiding this comment.
Mainly just one minor comment about arg names. Looks like you also have a couple clippy violations: https://github.com/tokio-rs/turmoil/actions/runs/11164962511/job/31053944943?pr=187
| @@ -251,10 +259,38 @@ impl Topology { | |||
| self.links[&Pair::new(a, b)].explicit_partition(); | |||
| } | |||
|
|
|||
| pub(crate) fn partition_oneway(&mut self, a: IpAddr, b: IpAddr) { | |||
There was a problem hiding this comment.
I think the best way to define this is from: IpAddr, to: IpAddr and then the a to b sorting is entirely internal. wdyt?
There was a problem hiding this comment.
Yeah, that's obviously much better!
There was a problem hiding this comment.
I have implemented your suggestion (I hope :-) ).
|
Looks like this needs a rebase and was hoping we could get |
I rebased, squashed into a single commiit, and renamed the parameter 'a' and 'b' to 'from' and 'to'. |
minor doc update
This is an attempt to solve #167 .