vacp2p / wakurtosis

4 stars 3 forks source link

Nomos distributed testing kurtosis module #70

Closed bacv closed 1 year ago

bacv commented 1 year ago

A draft pull request for nomos kurtosis code as a place for discussions and ideas.

AlbertoSoutullo commented 1 year ago

So far looks in a good direction to me.

Still, in order to look forward in the future, in main.star we should do an abstraction when we interconnect nodes. The code was like waku.interconnect_waku_nodes because I didn't really know how nomos would work. Now that I see this, I think the correct approach would be do something like node.interconnect_nodes, and inside we call waku or nomos interconnection with some dispatcher to have this methods abstracted of the node type.

In a similar way when we build the services. We just look in the topology which node are we building, and we use a node_builder for that. In the same way, we should look at which node we are interconnecting, and we would call the correspondant method.

So in this way, we would have something like node.interconnect_nodes instead of waku.interconnect_waku_nodes and nomos.interconnect_nomos_nodes.

I think the current structure makes it fairly easy to implement it this way (I hope), what do you think?

bacv commented 1 year ago

So in this way, we would have something like node.interconnect_nodes instead of waku.interconnect_waku_nodes and nomos.interconnect_nomos_nodes.

I think the current structure makes it fairly easy to implement it this way (I hope), what do you think?

I think that's great idea, the implementation would be very similar to nodes.instantiate_services(plan, waku_topology, same_toml_configuration). I can make this change as a part of this PR.

I'll convert this from draft to normal PR once WSL module starts working with nomos node, if there are any other ideas, please let me know.

bacv commented 1 year ago

Marking this PR as ready for review. The main goal was to introduce nomos node as an option alongside waku nodes.

Starlark code was modified to use different methods for initializing and interconnecting nodes based on the topology configuration (more accurately, by the docker image name).

At this point there are two different python scripts for waku and nomos WSL. We might decide to combine these scripts, but I'd suggest to make such changes in a separate PR.

AlbertoSoutullo commented 1 year ago

You might want to take a look at #90

As I mentioned in the PR, there are a lot of files changed, mainly because of some refactoring. The core functionality was changed to allow several nodes per container, so take a look specially in:

I don't really think you have to change a lot of stuff to be honest. I tried to decouple even more the starlark code, so it is easier to add things without interfere. When you refactor your code to match it with my brach structure, add me as reviewer and I will do it propperly. And ofc if you have any question just ping me!

AlbertoSoutullo commented 1 year ago

Despite working with Starlark that it is pretty limited, I am very happy on how the code structure of wakurtosis is going tbh. I don't know if you need to do more changes to this, I have a couple of picky comments to say, so when you are ready add me for review and I will test it.

How do you find the new structure? It was messy to refactor the last changes?

bacv commented 1 year ago

With the last commit I've rebased this PR on the master, so it's ready for another review.

How do you find the new structure? It was messy to refactor the last changes?

The updates were easy to understand, the tricky part was to deal with small naming changes. I believe nomos code shouldn't interfere with the work for waku and once merged both projects will benefit from the updates related to starting containers at scale.

bacv commented 1 year ago

Fine! It also would be nice to add tests for nomos.

Maybe these tests could be added in another PR?

AlbertoSoutullo commented 1 year ago

Fine! It also would be nice to add tests for nomos.

Maybe these tests could be added in another PR?

Yes for sure. I ment that in my previous message but maybe it was not clear enough!