Skip to content
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

Always restart container chain before starting collation #307

Merged
merged 5 commits into from
Nov 7, 2023

Conversation

tmpolaczyk
Copy link
Contributor

@tmpolaczyk tmpolaczyk commented Oct 31, 2023

Container chains can start in "sync mode", which uses random ports, or in "collation mode" which uses the ports provided by the user through CLI. To ensure that the container chain that is currently collating uses the correct ports, we restart it right when it is time to start collation.

The user can specify p2p, rpc, and prometheus ports using the CLI,
and since there can be 2 container chains running at the same time,
the port used may be the one specified by the user, or that +1
Copy link
Contributor

github-actions bot commented Oct 31, 2023

Master coverage: 71.35%
Coverage generated "Thu Nov 2 10:14:34 UTC 2023":
https://d3gkbsry1ehhqi.cloudfront.net/tanssi-coverage/pulls/307/index.html
Pull coverage: 70.59%

@tmpolaczyk
Copy link
Contributor Author

tmpolaczyk commented Oct 31, 2023

Check edge case of going from (2000, 2001) to (2001, 2000) Fixed

@@ -89,14 +89,16 @@ pub struct ContainerChainSpawnerState {

pub struct ContainerChainState {
/// Async callback that enables collation on this container chain
// We don't use it since we are always restarting container chains
#[allow(unused)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting should we remove it already then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could, but it will make it a bit harder to revert this change in the future

Copy link
Collaborator

Choose a reason for hiding this comment

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

no worries then

if !start_collation {
log::info!("This is a syncing container chain, using random ports");
// Use random ports to avoid conflicts with the other running container chain
let random_ports = [23456, 23457, 23458];
Copy link
Collaborator

Choose a reason for hiding this comment

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

wont it automatically use random ports even if we dont assign any?, in other words, can we use None here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We maybe can use [0, 0, 0], but the reason I selected the random ports myself is that if we actually use random ports there is a small probability of occupying a port from the collating container. The way it is implemented now that will only happen if this fixed random port is already taken, which can happen in zombienet because all of the collators try to use the same ports. But in production where every collator runs in isolation, I think it is better to use these fixed random ports instead of actually random.

}

if need_to_restart_next {
// Handle edge case of going from (2000, 2001) to (2001, 2000). In that case we must restart both chains,
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the second case, it restarts with random ports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we stop both of them, wait 10 seconds, and then start the new "syncing" with random ports, and the new "collating" with the good ports (in either order).

@tmpolaczyk tmpolaczyk merged commit e1a9ead into master Nov 7, 2023
23 checks passed
@tmpolaczyk tmpolaczyk deleted the tomasz-ports-fix-2 branch November 7, 2023 12:58
girazoki pushed a commit that referenced this pull request Nov 16, 2023
* Increment ports of "odd" parachains to avoid port conflicts

The user can specify p2p, rpc, and prometheus ports using the CLI,
and since there can be 2 container chains running at the same time,
the port used may be the one specified by the user, or that +1

* Always restart container chain before starting collation

* Fix wrong condition and add keep_db to stop signal

* Fix unit tests

* Fix edge case where we need to restart the next chain
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants