-
Notifications
You must be signed in to change notification settings - Fork 91
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
Conversation
Will this change break existing database snapshots? Seems that the top-most directory contained in |
@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 |
# Conflicts: # javascript/packages/orchestrator/src/paras.ts
@pepoviola @michalkucharczyk sorry i missed this pr had conflicts. They have been resolved now |
@pepoviola @michalkucharczyk is there something blocking this review? Just worried I might have missed something |
@pepoviola can you look into this please? |
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) => { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
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!! |
@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! |
@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!! |
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 ( Thanks!! |
@pepoviola awesome! I have got k8 installed and just prepping images. Will pull your fix once its committed and test locally too |
@pepoviola I had a go on removing the need for root volumes. Let me know if that wors |
javascript/packages/orchestrator/src/providers/native/nativeClient.ts
Outdated
Show resolved
Hide resolved
javascript/packages/orchestrator/src/providers/podman/podmanClient.ts
Outdated
Show resolved
Hide resolved
Hi @samelamin, looks good and works! I just made an small change in the command to exec. Then is good to merge. Thanks!! |
Hi @pepoviola
I noticed a few bugs while trying to write a test for cumulus so I thought I would push some fixes in 😄
Fix for spinning up k8 provider with correct paths.
the bug is explained here
Snapshots for collators were not supported, detailed here
Adding parachain and relaychain specs. Providing both Relay and Parachain chainspec fails #701