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

Feat: impl db snap logic for k8s/native #180

Merged
merged 8 commits into from
Mar 5, 2024
Merged

Conversation

pepoviola
Copy link
Collaborator

Add:

  • Logic to use db_snapshot in both native/k8s providers
  • Small fix on chain_spec_path (for both native/k8s)
  • New example db_snapshot

@pepoviola pepoviola requested a review from l0r1s March 4, 2024 12:18
Copy link
Contributor

@l0r1s l0r1s left a comment

Choose a reason for hiding this comment

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

👍

@@ -156,7 +156,8 @@ where
.map(|var| (var.name.clone(), var.value.clone())),
)
.injected_files(files_to_inject)
.created_paths(created_paths);
.created_paths(created_paths)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove this, there is no need to use created_paths now that it's created on send_file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We still need this for native, we should abstract in some way for both providers in a follow up pr.

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, we should probably at it here (as the Kubernetes provider):

Comment on lines +348 to +374
async fn upload_to_fileserver(&self, location: &Path) -> Result<Url, ProviderError> {
let data = self.filesystem.read(location).await?;
let hashed_path = hex::encode(sha2::Sha256::digest(&data));
let req = self
.http_client
.head(format!(
"http://{}/{hashed_path}",
self.file_server_local_host().await?
))
.build()
.map_err(|err| {
ProviderError::UploadFile(location.to_string_lossy().to_string(), err.into())
})?;

let url = req.url().clone();
let res = self.http_client.execute(req).await.map_err(|err| {
ProviderError::UploadFile(location.to_string_lossy().to_string(), err.into())
})?;

if res.status() != reqwest::StatusCode::OK {
// we need to upload the file
self.http_client
.post(url.as_ref())
.body(data)
.send()
.await
.map_err(|err| {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the difference with send_file below ? You want to update without copying to the pod ? Then maybe split the logic below in 2 parts and have a function used where we need ? wdyt ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, sounds good. We should split the send_file (upload/download) since we don't always want to just copy to one pod.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We also hashed the file content here since we want to dedup the upload phase of the process. I will open an issue to track this refactor in a followup pr.

@pepoviola
Copy link
Collaborator Author

Follow up issue #182, merging now.

@pepoviola pepoviola merged commit 4292e97 into main Mar 5, 2024
5 of 6 checks passed
@pepoviola pepoviola deleted the feat-impl-db_snap branch March 5, 2024 15:06
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