Jt/82 webrtcinrustping #84

John-LittleBearLabs · 2022-11-29T22:33:09Z

Finally appears to be working, with proviso below.

Connection established! PeerId("12D3KooWNfGZwVxzv4Eu2RvmCrLKQibdp2U57E93WH36rwygYk8Y")=Dialer { address: "/ip4/16.13.0.2/udp/1234/webrtc/certhash/uEiBLxsx_9DM981WqZCG8H0LDEcHRzJxJM8zgkpqXhVH95Q", role_override: Dialer }

Outcome: success

Before this is merged in we'd want to undo a couple things that I have to short-circuit to quickly test this:

The default value for the transport testparam must be tcp, not webrtc
Need to re-enable the older versions in rust.toml

…ustping checkpoint

…nd default is set to webrtc for my own sake ATM

mxinden

Direction here looks good to me.

@John-LittleBearLabs does a Testground run with these changes succeed for you locally?

Would you mind resolving the merge conflicts and marking it ready for review? That way we can test your changes on our CI.

ping/rust/src/lib.rs

…nge on a port constant - the motivation for the change is gone anyhow.

p-shahi · 2022-12-05T05:59:24Z

Approved and ran the CI workflow, I think it'll be good to add you guys as libp2p contributors in github-mgmt

jxs

Hi John, this already looks very good! Left some minor nits. Btw @mxinden should we add a composition (manually for now) testing webrtc to webrtc transport?

ping/rust/Dockerfile

ping/rust/src/lib.rs

ping/rust/src/bin/testplan_0440.rs

ping/rust/src/bin/testplan_0500.rs

jxs

LGTM, thanks John! @thomaseizinger maybe you also wanna take a look.

ping/rust/src/lib.rs

thomaseizinger · 2022-12-07T06:36:58Z

LGTM, thanks John! @thomaseizinger maybe you also wanna take a look.

Thanks for the ping, I'll take a look tomorrow!

thomaseizinger

Thanks! Looks good. A couple of improvement suggestions :)

ping/rust/src/bin/testplan_0500.rs

thomaseizinger · 2022-12-07T23:21:06Z

ping/rust/src/bin/testplan_0510.rs

+                Some(SwarmEvent::ConnectionEstablished {
+                    peer_id, endpoint, ..
+                }) => {
+                    info!("Connection established! {:?}={:?}", &peer_id, &endpoint);


PeerId supports Display so we don't need to use Debug here. Would also be a nicer log message if we get a multi-address out of endpoint and Display log that one.

Suggested change

info!("Connection established! {:?}={:?}", &peer_id, &endpoint);

info!("Connection established! {}={:?}", &peer_id, &endpoint);

thomaseizinger · 2022-12-07T23:21:19Z

ping/rust/src/bin/testplan_0510.rs

+                    info!("Connection established! {:?}={:?}", &peer_id, &endpoint);
+                    connected.insert(peer_id);
+                }
+                Some(event) => info!("Received event {:?}", &event), //This is useful, because it sometimes logs error messages


Again, info is likely way too noisy.

thomaseizinger · 2022-12-07T23:22:02Z

ping/rust/src/lib.rs

+    let local_addr = match transport.as_str() {
+        "tcp" => format!("/ip4/{local_ip_addr}/tcp/{LISTENING_PORT}"),
+        "webrtc" => format!("/ip4/{local_ip_addr}/udp/{LISTENING_PORT}/webrtc"),
+        unhandled => unimplemented!("Transport unhandled in test: '{}'", unhandled),


Instead of panicking, we could just return an error with anyhow::bail!().

thomaseizinger

Two more comments.

thomaseizinger · 2022-12-12T05:23:34Z

ping/rust/src/bin/testplan_0440.rs

+                                                  peer,
+                                                  result: Ok(ping::Success::Ping { .. }),
+                                              })) = self.0.next().await


Can we undo these formatting changes please?

This was resolved with a whole pass of cargo fmt

thomaseizinger · 2022-12-12T05:25:27Z

ping/rust/src/lib.rs

@@ -70,19 +73,20 @@ where
        // two peers dialling each other at the same time).
        //
        // We can do this because sync service pubsub is ordered.
-        .take_while(|a| ready(a != &local_addr));
+        .take_while(|a| ready(a != &local_addr && a != &dialable_multiaddr));


I think we can now simplify this.

Suggested change

.take_while(|a| ready(a != &local_addr && a != &dialable_multiaddr));

.take_while(|a| ready(a != &dialable_multiaddr));

thomaseizinger

Thanks! The final step is to remove the client parameter again as we no longer need it now outside the PingSwarm.

thomaseizinger · 2022-12-12T21:29:07Z

ping/rust/src/bin/testplan_0500.rs

+    let client = testground::client::Client::new_and_init()
+        .await
+        .expect("Unable to init testground cient.");


We no longer need the client here now so I'd like to move its creation back into PingSwarm.

Confirmed this has been removed.

thomaseizinger · 2022-12-12T21:30:02Z

ping/rust/src/bin/testplan_0500.rs

+                    debug!(
+                        "NewListenAddr event: listener_id={:?}, address={:?}",
+                        &listener_id, &address
+                    );


Unless you deem the listener ID very important, we could move this log to the ping swarm so all tests benefit from it.

thomaseizinger

Thanks! All looks good to me. We are not enforcing it in here but can you make sure that everything is formatted with cargo fmt?

thomaseizinger · 2022-12-13T21:10:30Z

ping/rust/src/lib.rs

        .into_iter()
        .find(|iface| iface.name == "eth1")
-        .unwrap()
+        .ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Can't find iface eth1"))?


Suggested change

.ok_or_else(|| io::Error::new(io::ErrorKind::Other, "Can't find iface eth1"))?

.context("Can't find iface eth1"))?

We are using anyhow so this would work too!

Thanks for the suggestion! This has been updated.

thomaseizinger · 2022-12-13T21:11:25Z

ping/rust/src/lib.rs

-        .unwrap()
+    let client = testground::client::Client::new_and_init()
+        .await
+        .expect("Unable to init testground cient.");


Suggested change

.expect("Unable to init testground cient.");

.context("Unable to init testground client.")?;

ddimaria · 2022-12-14T21:04:43Z

Thanks! All looks good to me. We are not enforcing it in here but can you make sure that everything is formatted with cargo fmt?

This is done. I'll run clippy as well shortly. Did we want to add any fmt/lint to CI @thomaseizinger?

thomaseizinger · 2022-12-14T21:08:14Z

Thanks! All looks good to me. We are not enforcing it in here but can you make sure that everything is formatted with cargo fmt?

This is done. I'll run clippy as well shortly. Did we want to add any fmt/lint to CI @thomaseizinger?

That would be great! If you are up for it, please send a separate PR, we don't need to block this on that :)

thomaseizinger · 2022-12-14T21:39:50Z

ping/rust/src/lib.rs

+    client
+        .run_parameters()
+        .test_instance_params
+        .get("transport").cloned()


Pretty sure rustfmt would want that on a separate line now 🙃

Ha, you're too fast for me @thomaseizinger 😉

That, and rust analyzer needed a restart.

ddimaria · 2022-12-14T22:01:54Z

Thanks for the comments and suggestions @thomaseizinger! I believe all of them have been resolved. Please let me know if there anything else you'd like done here. I left the unwraps alone that were in existing code...I'm not familiar with this codebase but figured there was a reason they were in there 🤷. Did you want those done in a separate PR?

thomaseizinger · 2022-12-15T03:00:11Z

Thanks for the comments and suggestions @thomaseizinger! I believe all of them have been resolved. Please let me know if there anything else you'd like done here. I left the unwraps alone that were in existing code...I'm not familiar with this codebase but figured there was a reason they were in there 🤷. Did you want those done in a separate PR?

It doesn't matter much because either will abort the test program and report a failure. Just a matter of consistency really but nothing too important :)

thomaseizinger

A few more suggestions, not pressing. Otherwise looks good to me! :)

thomaseizinger · 2022-12-15T03:01:25Z

ping/rust/src/lib.rs

+        .map_err(|e| anyhow::anyhow!(e.to_string()))
+        .context("Unable to init testground cient.")?;


Suggested change

.map_err(|e| anyhow::anyhow!(e.to_string()))

.context("Unable to init testground cient.")?;

.context("Unable to init testground cient.")?;

Double mapping is a bit useless :)

I'll look at this in the morning (diff TZ than you), but the suggestion caused compilation errors:

error[E0599]: the method `context` exists for enum `Result<Client, Box<dyn std::error::Error>>`, but its trait bounds were not satisfied --> src/lib.rs:41:10 | 41 | .context("Unable to init testground cient.")?; | ^^^^^^^ method cannot be called on `Result<Client, Box<dyn std::error::Error>>` due to unsatisfied trait bounds | ::: .rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:504:1 | 504 | pub enum Result<T, E> { | --------------------- doesn't satisfy `_: anyhow::Context<Client, Box<dyn std::error::Error>>` | ::: .rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/alloc/src/boxed.rs:194:1 | 194 | / pub struct Box< 195 | | T: ?Sized, 196 | | #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global, 197 | | >(Unique<T>, A); | | - | | | | | doesn't satisfy `Box<dyn std::error::Error>: Sync` | |_doesn't satisfy `Box<dyn std::error::Error>: std::marker::Send` | doesn't satisfy `_: anyhow::context::ext::StdError` | = note: the following trait bounds were not satisfied: `Box<dyn std::error::Error>: anyhow::context::ext::StdError` which is required by `Result<Client, Box<dyn std::error::Error>>: anyhow::Context<Client, Box<dyn std::error::Error>>` `Box<dyn std::error::Error>: std::marker::Send` which is required by `Result<Client, Box<dyn std::error::Error>>: anyhow::Context<Client, Box<dyn std::error::Error>>` `Box<dyn std::error::Error>: Sync` which is required by `Result<Client, Box<dyn std::error::Error>>: anyhow::Context<Client, Box<dyn std::error::Error>>`

Ah damn, that trait object is not Send + 'static! I think it would work otherwise. That is a shame. Let's leave it then!

thomaseizinger · 2022-12-15T03:01:42Z

ping/rust/src/lib.rs

+    let client = testground::client::Client::new_and_init()
+        .await
+        .map_err(|e| anyhow::anyhow!(e.to_string()))
+        .context("Unable to init testground cient.")?;


Suggested change

.context("Unable to init testground cient.")?;

.context("Unable to init testground client")?;

Typo.

thomaseizinger · 2022-12-15T03:03:40Z

ping/rust/src/lib.rs

+        .cloned()
+        .unwrap_or_else(|| "tcp".to_owned())


Suggested change

.cloned()

.unwrap_or_else(|| "tcp".to_owned())

.as_deref()

.unwrap_or("tcp")

Just curious whether this would work? (Together with &str as a return type).

.get("transport") returns an Option<&String> and .as_deref() has no effect on the return type. From the docs:

Converts from Option<T> (or &Option<T>) to Option<&T::Target>.

If we're looking to avoid the extra allocation, we could .remove() (instead of .get()) to pull the value out of the hashmap if the value is no longer needed?

.get("transport") returns an Option<&String> and .as_deref() has no effect on the return type. From the docs:

Converts from Option<T> (or &Option<T>) to Option<&T::Target>.

Hmm, the intention was to to convert &String to &str which should work because String implements Deref<Target = str>. Seems like we are missing something. It is not important though, I was just curious :)

thomaseizinger · 2022-12-15T03:06:39Z

ping/rust/src/lib.rs

@@ -94,7 +102,7 @@ where
    swarm
        .await_connections(client.run_parameters().test_instance_count as usize - 1)
        .await;
-
+    info!("Connections awaited.");


Suggested change

info!("Connections awaited.");

info!("Connected to {} peers", client.run_parameters().test_instance_count - 1);

Good call, much more meaningful log output!

ddimaria · 2022-12-15T19:24:27Z

CI is failing on run-ping-all: https://github.com/John-LittleBearLabs/tg-libp2p/actions/runs/3706321649/jobs/6282598377

LLVM ERROR: IO failure on output stream: No space left on device

It's running for 49 mins before running out, that's a long running test suite.

ddimaria · 2022-12-22T18:05:51Z

@thomaseizinger do you have opinions about the failing CI? More specifically, do you think tackling that is in the scope of this PR? If so, how has PL handled this in the past (disk cleanup, hosted runners, ..etc.)? There are other options to investigate, but I wanted to check in with you first.

galargh · 2022-12-23T09:00:31Z

@thomaseizinger do you have opinions about the failing CI? More specifically, do you think tackling that is in the scope of this PR? If so, how has PL handled this in the past (disk cleanup, hosted runners, ..etc.)? There are other options to investigate, but I wanted to check in with you first.

Thanks to https://github.com/pl-strflt/tf-aws-gh-runner we can set up self-hosted runners in no time. Once we clear this repo to use PL self-hosted runners and we prepare a runner that has all the deps testground needs, all it should require is changing labels here.

I know @laurentsenta also wanted to set up a long running testground daemon on EC2. For this, the only difference would be setting a testground daemon url that the CLI should communicate with.

The latter is a tad bit more work. I think I'd be able to get the former ready today. Worth noting, the two are not mutually exclusive. What's more, the AMI we prepare for self-hosted runners will surely be useful for the long-running daemon machine too so we won't be duplicating work in either case. Finally, since the changes required are so minimal, we're not locking ourselves in in any given solution.

I'll update once self-hosted runners are ready for use here. I'll propose the changes in a separate PR.

thomaseizinger · 2022-12-23T10:47:24Z

@thomaseizinger do you have opinions about the failing CI? More specifically, do you think tackling that is in the scope of this PR? If so, how has PL handled this in the past (disk cleanup, hosted runners, ..etc.)? There are other options to investigate, but I wanted to check in with you first.

Thanks for the ping and sorry for the late reply.

GH-hosted runners give us 14GB of disk space. The fact that we are exceeding this is in my opinion another pointer that the current architecture of these tests simply does not scale.

It is completely unnecessary that we rebuild all of these tests on every test run. I am of the strong opinion that these docker containers should be an artifact that is produced once (1x), uploaded to a registry and then only pulled for each test run.

It is also unnecessary that we maintain them in parallel. Old versions of the test should never need to change. The test should be co-located with the library code over at https://github.com/libp2p/rust-libp2p and versioned along side with it, producing a new docker container for every released version.

That is the proper solution in my opinion. I opened an issue about this in testground a while ago: testground/testground#1522

I have no opinion on how we fix this in the short term. Happy to throw more disk-space at the problem for now.

ddimaria · 2022-12-23T16:33:48Z

Thanks for the reply @thomaseizinger! It looks like @galargh will take care of CI in a separate PR, so I'll close this one out, or can wait for that to land before merging? I agree with the pre-built binaries/images approach. I'll read through that Testground issue. A possible challenge might be features in Rust, though pulling the features in this PR and moving to a rust-libp2p CI build-image job (specifically for Testground?) seems sensible.

galargh · 2022-12-23T23:26:57Z

Quick update: I didn't manage to get to fully setting up/testing self-hosted runners for this repo today but it's the first item on my TODO list for next week.

galargh · 2022-12-30T14:46:12Z

Quick update: I didn't manage to get to fully setting up/testing self-hosted runners for this repo today but it's the first item on my TODO list for next week.

Ready 🥳 #92 🎉

ddimaria · 2023-01-12T00:09:11Z

@p-shahi I wanted to make sure this PR close was intentional? I was waiting for #92 to merge in order to validate this work before merging this PR.

jxs · 2023-01-12T01:38:16Z

Hi @ddimaria, this PR targets the old dir which has been superseeded by the new one introduced with #97. I started working on introducing WebRTC support for the Rust "test-plans" there, see here, is that ok with you folks?

ddimaria · 2023-01-12T14:32:12Z

Thanks for the info @jxs! This sounds good to me, I just didn't want any work to go to waste unintentionally.

John-LittleBearLabs added 5 commits November 28, 2022 17:13

starting

766c909

Merge remote-tracking branch 'up/master' into jt/82_webrtcinrustping

ad93609

squash! Merge remote-tracking branch 'up/master' into jt/82_webrtcinr…

65117e1

…ustping checkpoint

compiles...

54466dd

unknown protocol string: webrtc

2653436

John-LittleBearLabs mentioned this pull request Nov 29, 2022

add webrtc to logic of ping/rust #82

Closed

John-LittleBearLabs added 2 commits November 30, 2022 11:05

Update Cargo.toml to pull in new release

a9a4f18

hold

79b4441

p-shahi linked an issue Dec 1, 2022 that may be closed by this pull request

add webrtc to logic of ping/rust #82

Closed

John-LittleBearLabs added 4 commits December 2, 2022 00:51

Restricting to webrtc-enabled versions to verify that.

b114212

switch to test_param. Currently taking its value only from default, a…

2cc28e0

…nd default is set to webrtc for my own sake ATM

removing some temporary stuff

2d0bc4a

oops

31666f5

mxinden reviewed Dec 2, 2022

View reviewed changes

ping/rust/src/lib.rs Outdated Show resolved Hide resolved

John-LittleBearLabs added 2 commits December 2, 2022 10:49

Change to optional return value

31e8197

Merge remote-tracking branch 'up/master' into jt/82_webrtcinrustping

76b9283

John-LittleBearLabs marked this pull request as ready for review December 2, 2022 16:07

John-LittleBearLabs added 3 commits December 2, 2022 11:12

Post-merge duplicate group IDs, also reverting inappropriate type cha…

8991625

…nge on a port constant - the motivation for the change is gone anyhow.

express it in code

89ce9f1

Porting updates to testplan_0510

35869fe

jxs reviewed Dec 5, 2022

View reviewed changes

Some PR comments

e9c0ba4

jxs approved these changes Dec 5, 2022

View reviewed changes

ping/rust/src/lib.rs Outdated Show resolved Hide resolved

expect

5cae933

thomaseizinger reviewed Dec 7, 2022

View reviewed changes

John-LittleBearLabs added 2 commits December 9, 2022 01:16

logging verbosity

afe3a77

listen_on now returns an address across all versions.

24c1f23

thomaseizinger reviewed Dec 12, 2022

View reviewed changes

updates

a8fefc2

thomaseizinger reviewed Dec 12, 2022

View reviewed changes

Positive consequences of previous shifts.

c3207d2

thomaseizinger approved these changes Dec 13, 2022

View reviewed changes

cargo fmt

8696244

fix clippy lints

c916d4b

thomaseizinger reviewed Dec 14, 2022

View reviewed changes

prefer Anyhow's context

3dab17b

thomaseizinger approved these changes Dec 15, 2022

View reviewed changes

ddimaria added 2 commits December 14, 2022 21:19

Improve logging, fix typo

26f311a

Merge branch 'libp2p:master' into jt/82_webrtcinrustping

753cd1b

p-shahi closed this Jan 10, 2023

p-shahi mentioned this pull request Jan 10, 2023

[tracking issue] Interop test-plans for all existing/developing libp2p transports #61

Open

28 tasks

	info!("Connection established! {:?}={:?}", &peer_id, &endpoint);
	info!("Connection established! {}={:?}", &peer_id, &endpoint);

	.take_while(\|a\| ready(a != &local_addr && a != &dialable_multiaddr));
	.take_while(\|a\| ready(a != &dialable_multiaddr));

	.ok_or_else(\|\| io::Error::new(io::ErrorKind::Other, "Can't find iface eth1"))?
	.context("Can't find iface eth1"))?

	.expect("Unable to init testground cient.");
	.context("Unable to init testground client.")?;

		.map_err(\|e\| anyhow::anyhow!(e.to_string()))
		.context("Unable to init testground cient.")?;

	info!("Connections awaited.");
	info!("Connected to {} peers", client.run_parameters().test_instance_count - 1);

Jt/82 webrtcinrustping #84

Jt/82 webrtcinrustping #84

Conversation

John-LittleBearLabs commented Nov 29, 2022 • edited Loading

mxinden left a comment

Choose a reason for hiding this comment

p-shahi commented Dec 5, 2022

jxs left a comment

Choose a reason for hiding this comment

jxs left a comment

Choose a reason for hiding this comment

thomaseizinger commented Dec 7, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddimaria commented Dec 14, 2022

thomaseizinger commented Dec 14, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddimaria commented Dec 14, 2022

thomaseizinger commented Dec 15, 2022

thomaseizinger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddimaria commented Dec 15, 2022 • edited Loading

ddimaria commented Dec 22, 2022

galargh commented Dec 23, 2022 • edited Loading

thomaseizinger commented Dec 23, 2022 • edited Loading

ddimaria commented Dec 23, 2022

galargh commented Dec 23, 2022

galargh commented Dec 30, 2022

ddimaria commented Jan 12, 2023

jxs commented Jan 12, 2023

ddimaria commented Jan 12, 2023

John-LittleBearLabs commented Nov 29, 2022 •

edited

Loading

thomaseizinger commented Dec 14, 2022 •

edited

Loading

ddimaria commented Dec 15, 2022 •

edited

Loading

galargh commented Dec 23, 2022 •

edited

Loading

thomaseizinger commented Dec 23, 2022 •

edited

Loading