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

Restore snapshots to parachains #673

Merged
merged 16 commits into from
Feb 2, 2023

Conversation

samelamin
Copy link
Contributor

@samelamin samelamin commented Jan 4, 2023

Hi @pepoviola

I noticed a few bugs while trying to write a test for cumulus so I thought I would push some fixes in 😄

  1. Fix for spinning up k8 provider with correct paths.
    the bug is explained here

  2. Snapshots for collators were not supported, detailed here

  3. Adding parachain and relaychain specs. Providing both Relay and Parachain chainspec fails #701

@michalkucharczyk
Copy link
Contributor

Will this change break existing database snapshots? Seems that the top-most directory contained in tgz is now different.

@samelamin
Copy link
Contributor Author

@michalkucharczyk it does yes, because we are restoring into the root.

We need to if we want to support collators because the data for collators is split between the relay chain and parachain folders. the current implementation restores directly to the chains folder which is fine for the relay data but wont restore a parachains relay data

@samelamin
Copy link
Contributor Author

@michalkucharczyk @pepoviola ping

@samelamin
Copy link
Contributor Author

@pepoviola @michalkucharczyk sorry i missed this pr had conflicts. They have been resolved now

@samelamin samelamin changed the title fix for k8 provider Restore snapshots to parachains Jan 26, 2023
@samelamin
Copy link
Contributor Author

@pepoviola @michalkucharczyk ping

@samelamin
Copy link
Contributor Author

@pepoviola @michalkucharczyk is there something blocking this review? Just worried I might have missed something

@bkchr
Copy link
Member

bkchr commented Jan 31, 2023

@pepoviola can you look into this please?

@pepoviola
Copy link
Collaborator

Yes, sorry about the delay. I will review today. I checked the pr and I think we can simplify the code and not use another volume.

Thanks and sorry again for the delay.

@@ -261,6 +261,38 @@ export async function start(
const chainSpecContent = require(chainSpecFullPathPlain);
client.chainId = chainSpecContent.id;

const parachainFilesPromiseGenerator = async (parachain: Parachain) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is just a reorder in the code, or I'm missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its a reorder because we need to generate the parachain files, and since the relay chain we provided isnt raw, these codeblock does not run and the cumulus parachains fail to get created

@pepoviola
Copy link
Collaborator

Hi @samelamin, looks good. Just a couple of small comments. Can you also run:

npm run lint:write

Since the CI is failing because of the linter.

Thanks!!

@samelamin
Copy link
Contributor Author

@pepoviola ping

@pepoviola
Copy link
Collaborator

@pepoviola ping

Hi @samelamin, all test are failing. I have to review the cause but I think could be because of the new volume (root).

Did you manage to spawn a network with k8s with this changes?

Thanks!

@samelamin
Copy link
Contributor Author

@pepoviola Im on an M1 so I was not able to no, I shall try spinning up a new machine to see if i can do some tests

@pepoviola
Copy link
Collaborator

@pepoviola Im on an M1 so I was not able to no, I shall try spinning up a new machine to see if i can do some tests

I will try in my end today, thanks!!

@pepoviola
Copy link
Collaborator

Hi @samelamin, I tested locally and I can reproduce the issue with k8s. I will extract your fix and change to not use the root (/) volume that is causing the issue.

Thanks!!

@samelamin
Copy link
Contributor Author

@pepoviola awesome! I have got k8 installed and just prepping images. Will pull your fix once its committed and test locally too

@samelamin
Copy link
Contributor Author

@pepoviola I had a go on removing the need for root volumes. Let me know if that wors

@pepoviola
Copy link
Collaborator

Hi @samelamin, looks good and works! I just made an small change in the command to exec. Then is good to merge.

Thanks!!

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.

4 participants