Add zombienet integration #307

pepoviola · 2024-05-10T16:13:25Z

Does not require a CHANGELOG entry

Hi 👋 , this pr add a very small smoke test as PoC of using zombienet-sdk with the runtimes repo. The idea is to explore the integration and add more assertions (metrics, logs, etc) and complex scenarios in the future (e.g: test upgrades, mix node's versions, etc).

This test use a set of env vars that allow you to specify the images and the provider to use:
POLKADOT_IMAGE (default: docker.io/parity/polkadot:latest)
CUMULUS_IMAGE (default: docker.io/parity/polkadot-parachain:latest)
ZOMBIE_PROVIDER(default: Docker)

And needs to have the chain-spec-generator binary as part of your PATH, any feedback is welcome :)

Also worth notice that the chain-spec-generator tool is compiling all the runtimes, and would be good to only compile the needed ones (polkadot in this case), we can add some feature-gate logic or we can also move _ presets_ to runtimes (ping @michalkucharczyk ).

Thanks!

michalkucharczyk · 2024-05-10T19:50:17Z

or we can also move _ presets_ to runtimes

With this we could use generic chain-spec-builder to build chain-specs.
But there is still open question how to handle the rest of chain-spec (e.g. chain-name, id, tokenDecimals). Providing them in command line every time the tool is used will be inconvenient. As trivial/naive solution bunch of utility scripts could be provided. Any thoughts on this? Unfortunately I don't have good ideas on this. We would also need a way to provide production specs.

Anyway I think that as an intermediate step we could move presets to the runtimes, and keep chain-spec-builder in this rep:

without any dependencies on runtime crates
and maybe reusing this little lib which is part of generic tool.

As reference, some example work of moving presets from binary to runtime is provided here: paritytech/polkadot-sdk#3996

bkchr

Left only nitpicks.

For a first integration it is good, but also doesn't do that much :D

integration-tests/zombienet/src/lib.rs

bkchr · 2024-05-14T10:14:57Z

integration-tests/zombienet/src/lib.rs

+					log::trace!("{:?}", e);
+					if let subxt::Error::Rpc(subxt::error::RpcError::ClientError(inner)) = e {
+						log::trace!("inner: {}", inner.to_string());
+						if inner.to_string().contains("i/o error") {


This is also quite hacky :P

Yes, I'm not super happy with this. Maybe @jsdw can help with a better approach to check if the inner error is related to i/o.

Thx!

Because somebody can plug in an arbitrary RPC client, we end up with this Box<dyn std::error::Error + Send + Sync + 'static> type to store errors from them. If you know which RPC client is used, you can try to cast that into the relevant error.

In the usual case, the error will be a jsonrpsee_core::ClientError, so in that case you could do something like:

if let Ok(client_error) = inner.downcast::<jsonrpsee_core::ClientError> { if matches!(client_error, ClientError::Transport(_)) { // ... } }

It's not much less ugly though in the end!

Not sure if this applies for you, but in Subxt the way that we wait for the node to be ready for connections is to wait for it to log that it's listening for them, ie https://github.com/paritytech/subxt/blob/ec7cea0acb59cdffb281aacd86a62c33514331e6/testing/substrate-runner/src/lib.rs#L191-L198 (that waits for a couple of "legacy" strings that nodes don't emit now too, which we can prob remove at this point).

integration-tests/zombienet/src/lib.rs

.github/workflows/test.yml

integration-tests/zombienet/Cargo.toml

bkchr · 2024-05-14T11:22:26Z

integration-tests/zombienet/tests/smoke.rs

+	let config = small_network().unwrap();
+
+	// spawn
+	let now = Instant::now();
+	let network = spawn_fn(config).await.unwrap();
+	let elapsed = now.elapsed();
+	log::info!("🚀🚀🚀🚀 network deployed in {:.2?}", elapsed);
+
+	let alice = network.get_node("alice").unwrap();


Where can I find the names of the nodes? How do I know if a node belongs to a parachain or the relay chain?

The config came from small_network and I placed there for reuse in other tests, but could be better to each test have it's own config in the same file to easily find the nodes/paras. I can change if you are agree.

Thx!

I mean I saw where it came from, but even there I don't see the names of the nodes :D

Ahh oks, in the sdk you have to set the name of the node with a like like this

.with_node(|node| node.with_name("alice"))

but we can make more explicit as const to be easily find with a quick look. wdyt?

.with_node(|node| node.with_name("alice"))

But you don't do this here anywhere in the code? Or am I blind?

Fuck I'm blind 🤦 🤦

One thing that is weird is that you call it with_node and for the parachain with_collator, while for the relay chain the node is probably a validator?

Thanks for the feedback! 🙌

Yes, the naming could be confusing. Both method with_node and with_collator return a NodeConfigBuilder that by default is validator (this is also confusing because for nodes of a parachain means that we pass the flag --collator).
Maybe is better to just have with_node with the default _role_ set to ROLE::AUTHORITY and you can change to full with node.role(ROLE::FULL)

So, this way is more clear and use the same wording you see in the logs of the client. wdyt?

Thx!

So, this way is more clear and use the same wording you see in the logs of the client. wdyt?

Or just call them with_validator, with_node and with_collator. Probably the least confusing one. Otherwise I would go with with_node and require that you make it a validator/collator (but the argument against this being that you almost never need a normal full node in the tests)

Yes, I think is more explicit and will cause less confusion have those 3 methods 👍.
Thanks again for the feedback!
update: tracking here

integration-tests/zombienet/tests/smoke.rs

Co-authored-by: Bastian Köcher <git@kchr.de>

integration-tests/zombienet/Cargo.toml

pepoviola · 2024-05-21T07:55:48Z

ping @eskimor / @ordian, can you also please review this one (we need 3 rank 3 reviews).
Thanks!!

ordian · 2024-05-21T08:47:14Z

Cargo.lock

@@ -562,7 +595,7 @@ dependencies = [
 "frame-support",
 "kusama-emulated-chain",
 "parachains-common",
- "sp-core",
+ "sp-core 29.0.0",


@pepoviola is it possible to deduplicate the dependencies so that we don't have 2 versions of polkadot-sdk crates?

or is it coming from subxt?

Yes, we can align the version of sp-core. There is any reason why we are using 29.0.0 here since the latest release is 32.0.0?

Thanks!!

it's probably fine to leave it as is for now. the versions will be upgraded when we update the runtumes to the next polkadot-sdk release

Yea its fine as long as we dont have duplicated versions in the runtimes themselves.

pepoviola · 2024-05-21T11:28:54Z

Ping @bkchr, can we merge this one? Thx!

bkchr · 2024-05-22T13:02:08Z

integration-tests/zombienet/Cargo.lock

Why do we still have one extra Cargo.lock file?

We shouldn't, let me delete this one. Thanks for the feedback.

bkchr · 2024-05-22T13:55:34Z

/merge

fellowship-merge-bot · 2024-05-22T13:59:28Z

Enabled auto-merge in Pull Request

Available commands

/merge: Enables auto-merge for Pull Request
/merge cancel: Cancels auto-merge for Pull Request
/merge help: Shows this menu

For more information see the documentation

pepoviola added 4 commits May 10, 2024 16:08

Add zombienet integration, smoke test as PoC

21d5f3d

remove comments

c5fe286

update cumulus default image

592fda1

small clean up

02b5858

fmt

63369e3

bkchr approved these changes May 14, 2024

View reviewed changes

pepoviola and others added 8 commits May 14, 2024 15:06

add zombienet to the workspace

affa662

Update .github/workflows/test.yml

73753f4

Co-authored-by: Bastian Köcher <git@kchr.de>

changes from feedback

ab11095

fmt

261b153

feedback for deps versions

a4b2423

Merge branch 'main' into add-zombienet-smoke

5a3dec4

add more logging

b9de589

use fast-runtime and add more time

154a190

davxy approved these changes May 20, 2024

View reviewed changes

integration-tests/zombienet/Cargo.toml Outdated Show resolved Hide resolved

integration-tests/zombienet/Cargo.toml Outdated Show resolved Hide resolved

pepoviola added 5 commits May 20, 2024 13:10

resolve and merge main

00db351

changes from feedback

b38ca9d

bump log version, feedback from zepter

3a87e20

bump tokio version, feedback from zepter

daf4287

apply zepter feedback

7f9dbfb

ordian reviewed May 21, 2024

View reviewed changes

ordian approved these changes May 21, 2024

View reviewed changes

bkchr reviewed May 22, 2024

View reviewed changes

remove old lock file

d2cc593

bkchr approved these changes May 22, 2024

View reviewed changes

Merge branch 'main' into add-zombienet-smoke

146179b

fellowship-merge-bot bot enabled auto-merge (squash) May 22, 2024 13:59

fellowship-merge-bot bot merged commit 65e23d8 into polkadot-fellows:main May 22, 2024
37 checks passed

michalkucharczyk mentioned this pull request Jun 12, 2024

Update to SDK 1.13 #322

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add zombienet integration #307

Add zombienet integration #307

pepoviola commented May 10, 2024

michalkucharczyk commented May 10, 2024 •

edited

Loading

bkchr left a comment

bkchr May 14, 2024

pepoviola May 15, 2024

jsdw May 15, 2024

bkchr May 14, 2024

pepoviola May 14, 2024

bkchr May 14, 2024

pepoviola May 14, 2024

bkchr May 15, 2024

bkchr May 15, 2024

bkchr May 15, 2024

pepoviola May 16, 2024

bkchr May 16, 2024

pepoviola May 16, 2024 •

edited

Loading

pepoviola commented May 21, 2024

ordian May 21, 2024

ordian May 21, 2024

pepoviola May 21, 2024

ordian May 21, 2024

ggwpez May 22, 2024

pepoviola commented May 21, 2024

bkchr May 22, 2024

pepoviola May 22, 2024

bkchr commented May 22, 2024

fellowship-merge-bot bot commented May 22, 2024

Add zombienet integration #307

Add zombienet integration #307

Conversation

pepoviola commented May 10, 2024

michalkucharczyk commented May 10, 2024 • edited Loading

bkchr 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepoviola May 16, 2024 • edited Loading

Choose a reason for hiding this comment

pepoviola commented May 21, 2024

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

pepoviola commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkchr commented May 22, 2024

fellowship-merge-bot bot commented May 22, 2024

michalkucharczyk commented May 10, 2024 •

edited

Loading

pepoviola May 16, 2024 •

edited

Loading