-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CVPN-1818 Inside Packet Codec #161
base: main
Are you sure you want to change the base?
Conversation
Code coverage summary for 69482af:
✅ Region coverage 54% passes |
958b759
to
3de9228
Compare
aaf7e86
to
eb7088e
Compare
// Ignore invalid inside packet | ||
} | ||
Err(err) => { | ||
// Propagate fatal error up |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will drop all the remaining packets in the current vector.
We will need a metric to monitor this usage
27a2532
to
8231bb6
Compare
6e84723
to
e499d14
Compare
7d183f5
to
f5bc40a
Compare
b169417
to
2fe9714
Compare
Interface for encoding and decoding inside packets in lightway-core. Inside packets can be accumulated and encoded, which allows error correction code to be integrated to lightway-core.
These new types of wire frame works like Data and DataFrag. The sole purpose is to determines whether the Data payload is encoded or not.
Connection optionally holds an encoder and an decoder for inside packets. Encoder can either accumulates the inside packets and encode it before it is sent to WolfSSL, or skip the packet and Connection will directly pass it to WoflSSL. Decoder accumulates all encoded packets and decode it before it is sent to TUN.
Packet Codec is supplied by lw-server to lw-core. The packets in the encoder might have sit in the encoder for too long if inside packets are not frequently received. To avoid having a high packet delay, lw-server triggers a flush when the encoder is ready. There could be stale states in the decoder. The decoder's stale state is cleaned periodically. lw-server tracks the list of encoders and decoders for each connection. Instead of accesing the codec via locking the connection, codec is access directly instead. In this way, the number of locking to Connection is minimized.
Packet Codec is supplied by lw-client to lw-core. The packets in the encoder might have sit in the encoder for too long if inside packets are not frequently received. To avoid having a high packet delay, lw-server triggers a flush when the encoder is ready. There could be stale states in the decoder. The decoder's stale state is cleaned periodically.
By default, inside packet encoding is disabled. The client now can toggle encoding in both directions in the following work-flow: 1. Client sends a ToggleEncoding packet to the server 2. Server toggles encoding, and reply with the same packet 3. Client receives the packet and toggles encoding TODO: under heavy packet loss, the above packets could be lost. We may need to add a re-transmission mechanism to make this more robust.
Client now has two ways to toggle encoding: 1. -e flag in CLI: automatically sends a toggle encoding packet to the server when lightway-core's state changes to Connected. 2. send a signal to the toggle_encode_tx in main.rs to toggle encoding at runtime. The client may enable/disable in runtime without disrupting the connection. TODO: create an appropriate interface to send a toggle encoding signal via CLI?
When flushing the encoder, more than one packets are sent to WolfSSL. If any of the send operation returns a fatal error, all subsequent inside packets are dropped. Added a metric to track the number of such packets.
/// | ||
/// Returns a [`CodecStatus`]. | ||
/// If the status is [`CodecStatus::ReadyToFlush`], encoded packets are ready to be retrieved. | ||
/// If the status is [`CodecStatus::SkipPacket`], the packet should not be added to the encoder. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not this mean `the packet is skipped by the encoder and lightway can send it as normal Wire::Data packet.
fn cleanup_stale_states(&mut self); | ||
} | ||
|
||
/// Indicates whether the [`PacketEncoder`] or [`PacketDecoder`] is ready to be flushed or not |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indicates the status of [PacketEncoder
] or [PacketDecoder
] after storing the current packet
@@ -137,6 +138,9 @@ async fn main() -> Result<()> { | |||
key_update_interval: config.key_update_interval.into(), | |||
inside_plugins: Default::default(), | |||
outside_plugins: Default::default(), | |||
inside_pkt_codec: None, | |||
pkt_encoder_flush_interval: Duration::from_secs_f64(0.0001), | |||
pkt_decoder_clean_up_interval: Duration::from_secs_f64(0.5), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make this values as cli args ?
} else { | ||
metrics::tun_rejected_packet_no_connection(); | ||
} | ||
} | ||
}); | ||
|
||
tokio::spawn(pkt_encoder_flush( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This two tasks should be created only if config.inside_pkt_codec
is not None, should not this ?
@@ -97,6 +99,9 @@ async fn main() -> Result<()> { | |||
server: config.server, | |||
inside_plugins: Default::default(), | |||
outside_plugins: Default::default(), | |||
inside_pkt_codec: None, | |||
pkt_encoder_flush_interval: Duration::from_secs_f64(0.0001), | |||
pkt_decoder_clean_up_interval: Duration::from_secs_f64(0.5), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as server, better this be an cli config
// encoder is not set. Awaits forever... | ||
// Not returning immediately to avoid triggering an exit of the | ||
// entire client program. | ||
tracing::debug!("Pkt encoder flush task is freezed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, tokio::select supports enabling the branch only on valid condition (valid packet codec)
You might use it to disable this task check instead of this.
|
||
/// Signal for toggling inside packet encoding | ||
#[educe(Debug(ignore))] | ||
pub toggle_encoding_signal: tokio::sync::mpsc::Receiver<bool>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not force apps to provide this value. Better to make this as option similar to inside_pkt_codec
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or should we create a new PacketEncoderConfig and add all these variables inside it and make it as an option.
It reduces the burden for apps to skip this, if codec is not needed
Description
Added inside packet codec interface for Lightway UDP:
EncodedData
andEncodedDataFrag
ToggleEncoding
that allows the client to enable encoding on both sides-e
flag in CLI args to automatically send aToggleEncoding
packet to the server once lightway-core changes state toConnected
Motivation and Context
FEC code can be integrated to inside packets. Throughput and latency can be reduced under high packet loss network conditions.
How Has This Been Tested?
iperf3
andping 8.8.8.8
Integration with C servers
With the network namespace scripts, running the latest version of lightway C UDP server: Verified that sending wire frames with the new ids does not break the connection with the server.
Types of changes
Checklist:
main