ping/rust: adapt to rust-sdk patch, remove unused dependencies #42

elenaf9 · 2022-09-17T15:06:23Z

01d0b18 adapts to an breaking api change in the rust-sdk (src/client: Make subscribe stream capacity explicit testground/sdk-rust#36) which causes the build to fail after running cargo update.
e9d601b removes unused dependencies. Was there a reason why they were added in the first place?
76a9ee2: fix instructions for importing the testplans in testground. Without the --from the testground CLI did not accept the current instruction described in the README. Does it work for other folks / did I do something wrong?

Builds on #41.

See release of v0.48.0 in libp2p/rust-libp2p#2869. Master bump to v0.49.0 in libp2p/rust-libp2p#2813

elenaf9 · 2022-09-18T11:33:53Z

ping/rust/src/main.rs

@@ -85,7 +88,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
    }

    let mut address_stream = client
-        .subscribe("peers")
+        .subscribe("peers", 1)


Should we set a higher capacity here? I am not familiar with how the background task works but the description in testground/sdk-rust#36 indicates that there is a footgun related to this. Concretely I am not sure if its an issue that we first create the stream, then publish our own message (and maybe it's blocking here?) before we start polling the stream.
CI is failing currently, and this is the only change that I can think of that is causing this.
On the other hand, before this patch we were using an older version of the rust-sdk were also capacity 1 was used: https://github.com/testground/sdk-rust/blob/b6aa70a32cdb31d27ee14a73b9e6fd325cf5fe1f/src/client.rs#L110.

Alternatively, could we just publish our own message before subscribing?

CC @mxinden since you were the author of testground/sdk-rust#36.

@elenaf9 Sorry for the typo in the README and thanks a lot for beta-testing and fixing the workflow 🙏

Regarding the failure, just a note in case that helps: CI is failing because the run is reaching the 6 minutes timeout.
https://github.com/libp2p/test-plans/actions/runs/3076811088/jobs/4971230622 (from my experience it takes between 1 and 2 minutes).

I can't tell where in the loop that happens, we'd need more logs, but it looks like there's a 10s hang between some iterations:

Sep 18 10:53:21.455380 INFO 211.7877s ERROR << 0.48.0[000] (760771) >> [2022-09-18T10:53:21Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWF1r3xcd5BiaLoCZjcfknWBnZE9gCyzd8mSuSRSiyGaJJ"), result: Ok(Pong) }) Sep 18 10:53:21.455461 INFO 211.7878s ERROR << v0.44.0[000] (3ad09f) >> [2022-09-18T10:53:21Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWNndM73hpCtFFYHwerYsAYYoAe7iqW2vcnUVHimmjL7PE"), result: Ok(Ping { rtt: 601.899µs }) }) # note the time jumping from 211 seconds to 221 seconds. Sep 18 10:53:31.436076 INFO 221.7684s ERROR << v0.45.1[000] (a2d41f) >> [2022-09-18T10:53:31Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWF1r3xcd5BiaLoCZjcfknWBnZE9gCyzd8mSuSRSiyGaJJ"), result: Ok(Pong) }) Sep 18 10:53:31.436141 INFO 221.7685s ERROR << v0.45.1[000] (a2d41f) >> [2022-09-18T10:53:31Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWF1r3xcd5BiaLoCZjcfknWBnZE9gCyzd8mSuSRSiyGaJJ"), result: Ok(Ping { rtt: 516.899µs }) }) Sep 18 10:53:31.436172 INFO 221.7685s ERROR << v0.46.0[000] (155fcf) >> [2022-09-18T10:53:31Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWNndM73hpCtFFYHwerYsAYYoAe7iqW2vcnUVHimmjL7PE"), result: Ok(Pong) }) ... Sep 18 10:53:31.455713 INFO 221.7880s ERROR << v0.46.0[000] (155fcf) >> [2022-09-18T10:53:31Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWDbRZhiWMUKmhLY7e7jPkvyf9eECmFCUvzjMN3cGszRuU"), result: Ok(Ping { rtt: 262.2µs }) }) Sep 18 10:53:31.455726 INFO 221.7880s ERROR << v0.44.0[000] (3ad09f) >> [2022-09-18T10:53:31Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWNndM73hpCtFFYHwerYsAYYoAe7iqW2vcnUVHimmjL7PE"), result: Ok(Ping { rtt: 311.099µs }) }) # again Sep 18 10:53:41.436572 INFO 231.7689s ERROR << v0.46.0[000] (155fcf) >> [2022-09-18T10:53:41Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWNndM73hpCtFFYHwerYsAYYoAe7iqW2vcnUVHimmjL7PE"), result: Ok(Pong) }) Sep 18 10:53:41.436588 INFO 231.7689s ERROR << v0.45.1[000] (a2d41f) >> [2022-09-18T10:53:41Z INFO testplan] Event: Behaviour(Event { peer: PeerId("12D3KooWF1r3xcd5BiaLoCZjcfknWBnZE9gCyzd8mSuSRSiyGaJJ"), result: Ok(Pong) })

Suggested change

.subscribe("peers", 1)

.subscribe("peers", client.run_parameters().test_instance_count as usize)

I suggest setting it to the amount of messages we expect to receive on the channel.

Alternatively, could we just publish our own message before subscribing?

I think we could do that. In other words I think the sync service allows us to still retrieve all the addresses.

~~Did both changes because imo they each make sense independently of the other.~~

Edit: It seems like the sync service does not allow publishing a message before subscribing. Thus I left the existing order and just cahnged the channel capacity like you suggested it.

mxinden

Thanks for the patch @elenaf9!

For the sake of small atomic changes, I suggest we try to get #41 merged by itself and then proceed here.

mxinden · 2022-09-21T12:20:49Z

ping/rust/src/main.rs

@@ -85,7 +88,7 @@ async fn main() -> Result<(), Box<dyn std::error::Error>> {
    }

    let mut address_stream = client
-        .subscribe("peers")
+        .subscribe("peers", 1)


Suggested change

.subscribe("peers", 1)

.subscribe("peers", client.run_parameters().test_instance_count as usize)

I suggest setting it to the amount of messages we expect to receive on the channel.

Alternatively, could we just publish our own message before subscribing?

I think we could do that. In other words I think the sync service allows us to still retrieve all the addresses.

mxinden · 2022-09-25T18:31:41Z

@elenaf9 #41 is merged now. Can you resolve the merge conflicts?

Revert changes from commit 210007f.

elenaf9 · 2022-09-28T02:23:05Z

Any idea why the CI is still failing @mxinden @laurentsenta?

laurentsenta · 2022-09-28T14:54:03Z

@elenaf9 have you had a chance to run the tests locally?
In the CI this is what I can see:

Tue, 27 Sep 2022 11:28:56 GMT
Sep 27 11:28:56.207712	INFO	1.9102s      ERROR << v0.46.0[000] (b39142) >> [2022-09-27T11:28:56Z INFO  testplan] Connected,peer=12D3KooWKCdiq4FWSFUhWiukx2VyLGUXH2nWLzLyZdU128evxqkh
Tue, 27 Sep 2022 11:28:56 GMT
Sep 27 11:28:56.207724	INFO	1.9102s      ERROR << v0.46.0[000] (b39142) >> [2022-09-27T11:28:56Z INFO  testplan] Connected,peer=12D3KooWJ1rcDMqmSLPawkWMgPQH1KMqpFiRy83Jz2MqcFQEjJzx
Tue, 27 Sep 2022 11:35:06 GMT
Error: The action has timed out.

It looks like the test hangs for ~6 minutes. I haven't had time to debug this yet, but I get a similar timeout on my machine.

laurentsenta · 2022-09-28T14:54:57Z

ping/rust/Cargo.toml

 serde_json = "1"
 soketto = "0.7.1"
-testground = {git = "https://github.com/testground/sdk-rust", rev = "94a9a72796f94cc7ca786a5f019d07f328c76d4b", version = "0.4.0"}
+testground = {git = "https://github.com/testground/sdk-rust", branch = "master", version = "0.4.0"}


Should we pin this?
If tomorrow, rust-sdk publish a new version this will break the build for every interop test.

I released v0.4.0 yesterday. I suggest we use the released version instead.

https://github.com/testground/sdk-rust/releases/tag/v0.4.0

I released v0.4.0 yesterday. I suggest we use the released version instead.

Of course you are maintaining that too 😂

mxinden · 2022-09-28T18:16:15Z

If I am not mistaken, none of the nodes are able to connect to the node based on master. Still investigating.

mxinden · 2022-09-29T10:44:30Z

I suspect libp2p/rust-libp2p#2954 will fix the failures here.

mxinden · 2022-09-29T12:16:53Z

With libp2p/rust-libp2p#2954 merged, this should go green now. Triggering CI.

mxinden · 2022-09-30T17:38:51Z

@elenaf9 can you merge latest master? Somehow I can not do so via the UI.

elenaf9 · 2022-09-30T19:07:37Z

🎉 CI is green. Thanks for the help @mxinden!

mxinden and others added 4 commits September 10, 2022 19:41

ping/: Update to rust-libp2p release v0.48.0 and master v0.49.0

bca429b

See release of v0.48.0 in libp2p/rust-libp2p#2869. Master bump to v0.49.0 in libp2p/rust-libp2p#2813

ping/rust: adapt to testground/sdk-rust#36

01d0b18

ping/rust: remove unused dependencies

e9d601b

README: fix import instructions

76a9ee2

elenaf9 commented Sep 18, 2022

View reviewed changes

mxinden reviewed Sep 21, 2022

View reviewed changes

mxinden mentioned this pull request Sep 21, 2022

ping/: Update to rust-libp2p release v0.48.0 and master v0.49.0 #41

Merged

elenaf9 mentioned this pull request Sep 27, 2022

ping/rust: remove the master dependency #45

Merged

elenaf9 added 4 commits September 27, 2022 12:38

Merge branch 'master' of github.com:libp2p/test-plans into patch-1

9c25b26

ping/rust: set channel capacity to # of instances

92d2073

ping/rust: publish own message before subscribing

210007f

ping/rust: revert 210007f

371f544

Revert changes from commit 210007f.

laurentsenta reviewed Sep 28, 2022

View reviewed changes

mxinden mentioned this pull request Sep 28, 2022

protocols/identify: Revise symbol naming libp2p/rust-libp2p#2927

Merged

4 tasks

elenaf9 added 3 commits September 30, 2022 20:25

Merge branch 'master' of github.com:libp2p/test-plans into patch-1

f0865bd

ping/rust: use testground-v0.4.0 from crates.io

fb8f83a

ping/rust: run cargo update, fix warnings

a75d25d

mxinden approved these changes Oct 2, 2022

View reviewed changes

mxinden merged commit c71602e into libp2p:master Oct 4, 2022

elenaf9 deleted the patch-1 branch October 4, 2022 15:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ping/rust: adapt to rust-sdk patch, remove unused dependencies #42

ping/rust: adapt to rust-sdk patch, remove unused dependencies #42

elenaf9 commented Sep 17, 2022 •

edited

Loading

elenaf9 Sep 18, 2022 •

edited

Loading

laurentsenta Sep 19, 2022

mxinden Sep 21, 2022

elenaf9 Sep 27, 2022 •

edited

Loading

mxinden left a comment

mxinden Sep 21, 2022

mxinden commented Sep 25, 2022

elenaf9 commented Sep 28, 2022

laurentsenta commented Sep 28, 2022

laurentsenta Sep 28, 2022 •

edited

Loading

mxinden Sep 28, 2022

thomaseizinger Sep 29, 2022

mxinden commented Sep 28, 2022

mxinden commented Sep 29, 2022

mxinden commented Sep 29, 2022

mxinden commented Sep 30, 2022

elenaf9 commented Sep 30, 2022

	.subscribe("peers", 1)
	.subscribe("peers", client.run_parameters().test_instance_count as usize)

ping/rust: adapt to rust-sdk patch, remove unused dependencies #42

ping/rust: adapt to rust-sdk patch, remove unused dependencies #42

Conversation

elenaf9 commented Sep 17, 2022 • edited Loading

elenaf9 Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

laurentsenta Sep 19, 2022

Choose a reason for hiding this comment

mxinden Sep 21, 2022

Choose a reason for hiding this comment

elenaf9 Sep 27, 2022 • edited Loading

Choose a reason for hiding this comment

mxinden left a comment

Choose a reason for hiding this comment

mxinden Sep 21, 2022

Choose a reason for hiding this comment

mxinden commented Sep 25, 2022

elenaf9 commented Sep 28, 2022

laurentsenta commented Sep 28, 2022

laurentsenta Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

mxinden Sep 28, 2022

Choose a reason for hiding this comment

thomaseizinger Sep 29, 2022

Choose a reason for hiding this comment

mxinden commented Sep 28, 2022

mxinden commented Sep 29, 2022

mxinden commented Sep 29, 2022

mxinden commented Sep 30, 2022

elenaf9 commented Sep 30, 2022

elenaf9 commented Sep 17, 2022 •

edited

Loading

elenaf9 Sep 18, 2022 •

edited

Loading

elenaf9 Sep 27, 2022 •

edited

Loading

laurentsenta Sep 28, 2022 •

edited

Loading