Skip to content

feat: Add TTL connection param to multicast #1842

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

Conversation

jmonticelli
Copy link
Contributor

@jmonticelli jmonticelli commented Mar 18, 2025

Hi, I am using Zenoh for a project and I use a multicast listen endpoint in peer mode.

Using a multicast endpoint for multicast communication works fine on flat networks, but once a router is involved, the multicast traffic is lost, due to the default TTL of 1 in multicast connections in the Tokio library.

See also: https://docs.rs/tokio/latest/tokio/net/struct.UdpSocket.html#method.set_multicast_ttl_v4

Note I have minimal rust programming experience, and used online resources and the code surrounding this addition to help me craft this PR, so feel free to make changes if I did something suboptimally or wrong.

I was able to build zenoh and run the zenoh pub client like so:

./target/release/examples/z_pub -m peer -l 'udp/239.1.3.37:9001#iface=myinterface;ttl=32' -k 'testkey' -p 'Hello from beyond the router' --no-multicast-scouting

I confirmed that the outgoing multicast TTL is set to what I specify it to, and that the traffic is no longer dropped on the other end of the network.

This is a critical feature when handling multicast traffic that is routed.

P.S., very nice library :)

Copy link

PR missing one of the required labels: {'breaking-change', 'enhancement', 'new feature', 'documentation', 'dependencies', 'bug', 'internal'}

@jmonticelli jmonticelli force-pushed the jmonticelli/add-multicast-ttl-connection-parameter branch from 6a95955 to 7c4a8e4 Compare March 18, 2025 20:52
Copy link

PR missing one of the required labels: {'enhancement', 'documentation', 'internal', 'breaking-change', 'new feature', 'bug', 'dependencies'}

@jmonticelli jmonticelli force-pushed the jmonticelli/add-multicast-ttl-connection-parameter branch from 7c4a8e4 to 83bb94c Compare March 18, 2025 21:01
Copy link

PR missing one of the required labels: {'bug', 'breaking-change', 'documentation', 'dependencies', 'internal', 'new feature', 'enhancement'}

@jmonticelli jmonticelli force-pushed the jmonticelli/add-multicast-ttl-connection-parameter branch from 83bb94c to 113e3a2 Compare March 18, 2025 21:27
Copy link

PR missing one of the required labels: {'enhancement', 'breaking-change', 'internal', 'new feature', 'bug', 'dependencies', 'documentation'}

@gabrik gabrik added the enhancement Existing things could work better label Mar 19, 2025
Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.

Project coverage is 70.84%. Comparing base (a89b501) to head (fa3aa05).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
io/zenoh-links/zenoh-link-udp/src/multicast.rs 18.18% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1842      +/-   ##
==========================================
- Coverage   70.99%   70.84%   -0.16%     
==========================================
  Files         360      360              
  Lines       65175    65186      +11     
==========================================
- Hits        46272    46180      -92     
- Misses      18903    19006     +103     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -321,6 +321,34 @@ impl LinkManagerMulticastUdp {
// https://docs.rs/tokio/latest/tokio/net/struct.UdpSocket.html#notes
mcast_sock.set_nonblocking(true)?;

// If TTL is specified, add set the socket's TTL
if let Some(ttl_str) = config.get(UDP_MULTICAST_TTL) {
let ttl = match ttl_str.parse::<u32>() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IPv6 hop limit is not yet supported in the Rust library.
I've opened rust-lang/rust#138744 to address the issue.

In the meanwhile, I suggest to simply log a warning in the case of an IPv6 socket.

        // If TTL is specified, add set the socket's TTL
        if let Some(ttl_str) = config.get(UDP_MULTICAST_TTL) {
            match &local_addr {
                IpAddr::V4(_) => {
                    let ttl = match ttl_str.parse::<u32>() {
                        Ok(ttl) => ttl,
                        Err(e) => bail!("Can not parse TTL '{}' to a u32: {}", ttl_str, e),
                    };

                    ucast_sock.set_multicast_ttl_v4(ttl).map_err(|e| {
                        zerror!("Can not set multicast TTL {} on {}: {}", ttl, mcast_addr, e)
                    })?;
                }
                IpAddr::V6(_) => {
                    tracing::warn!(
                            "UDP multicast hop limit not supported for v6 socket: {}. See https://github.com/rust-lang/rust/pull/138744.",
                            mcast_addr
                        );
                }
            }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wasn't sure how tokio handled this under the hood, I just saw is said it "may not have any effect" and figured it was a shot in the dark to attempt to set the TTL.

Sounds great, I will make this change. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have taken your changes, feel free to re-review that they were taken correctly.
It passed a local build for me.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, thanks!

jmonticelli added a commit to jmonticelli/zenoh that referenced this pull request Mar 20, 2025
@jmonticelli jmonticelli force-pushed the jmonticelli/add-multicast-ttl-connection-parameter branch from 3f31068 to 8f2d13e Compare March 20, 2025 18:13
@Mallets Mallets merged commit a90f7f9 into eclipse-zenoh:main Mar 21, 2025
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Existing things could work better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants