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

Add cli-flag to keep db after changing assignments #280

Merged
merged 18 commits into from
Oct 30, 2023

Conversation

Agusrodri
Copy link
Contributor

This PR adds a simple keep_db flag to ContainerChainRunCmd struct that allows to keep container-chain databases after changing collator assignments.

@Agusrodri Agusrodri requested a review from tmpolaczyk October 5, 2023 13:18
@github-actions
Copy link
Contributor

github-actions bot commented Oct 5, 2023

Master coverage: 70.97%
Coverage generated "Mon Oct 23 15:17:09 UTC 2023":
https://d3gkbsry1ehhqi.cloudfront.net/tanssi-coverage/pulls/280/index.html
Pull coverage: 70.62%

@Agusrodri
Copy link
Contributor Author

One of the warp sync tests is failing because it seems the container chain db for Collator2000-02 is not being deleted after the new configuration. I'm investigating the reason of the issue.

@tmpolaczyk
Copy link
Contributor

One of the warp sync tests is failing because it seems the container chain db for Collator2000-02 is not being deleted after the new configuration. I'm investigating the reason of the issue.

That was because it was panicking on this line: state.assigned_para_id.unwrap(), you could see it in the node logs. assigned_para_id can be None if the collator is not assigned to any chain.

I pushed a commit fixing that by using a different kind of channel to signal shutdown, now we can distinguish between "stopping because someone called .stop()" and "stopping because the node process is stopping". We will only delete the db in the first case.

@Agusrodri
Copy link
Contributor Author

@tmpolaczyk thank you so much for the clarification! I was hesitating if that piece of code was in the correct place.

.latest_block_number(orchestrator_chain_info.best_hash, container_chain_para_id)
.unwrap_or_default();

let max_block_diff_allowed = 10u32;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let max_block_diff_allowed = 10u32;
let max_block_diff_allowed = 100u32;

@@ -226,7 +254,7 @@ impl ContainerChainSpawner {
container_chain_cli_config.database.set_path(&db_path);

// Delete existing database if running as collator
if validator {
if validator && !container_chain_cli.base.keep_db && warp_sync {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am hesitating about this. What happens if:

  • I have the keep_db flag to true
  • but I detect that the genesis-hash of the chain-spec I am running is different (genesis-hash different)
  • in that case I would be stuck (the node tells me to reboot and that I need to use warp sync, all the time, but I need all these 3 flags to be true to delete the db and hence be able to do warp sync)

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should immediatly delete the container-chain-db if the spec is differnt I believe

Copy link
Collaborator

Choose a reason for hiding this comment

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

there is no reason to maintain a DB that does not match with the specs right?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right

node/src/container_chain_spawner.rs Outdated Show resolved Hide resolved
@girazoki
Copy link
Collaborator

I like the way it looks now, we can unitest the delete db cases quite easily!

Copy link
Contributor

@tmpolaczyk tmpolaczyk left a comment

Choose a reason for hiding this comment

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

self-approving

@tmpolaczyk tmpolaczyk merged commit 6a30da3 into master Oct 30, 2023
23 checks passed
@tmpolaczyk tmpolaczyk deleted the agustin-keepdb-cli-flag branch October 30, 2023 13:38
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.

3 participants