Skip to content

clean up tempfiles when downloading artifacts #1447

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.

Sign up for GitHub

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

Merged
iliana merged 2 commits into from
Jul 16, 2022
Merged

clean up tempfiles when downloading artifacts #1447

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad5f525
clean up tempfiles when downloading artifacts
iliana Jul 16, 2022
8b39bd5
flush instead of sync
iliana Jul 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 21 additions & 23 deletions sled-agent/src/updates.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use omicron_common::api::internal::nexus::{
UpdateArtifact, UpdateArtifactKind,
};
use std::path::PathBuf;
use tempfile::NamedTempFile;
use tokio::io::AsyncWriteExt;

#[derive(thiserror::Error, Debug)]
Expand Down Expand Up @@ -39,11 +40,16 @@ pub async fn download_artifact(
}
})?;

// We download the file to a location named "<artifact-name>-<version>".
// We then rename it to "<artifact-name>" after it has successfully
// downloaded, to signify that it is ready for usage.
let tmp_path = directory
.join(format!("{}-{}", artifact.name, artifact.version));
// We download the file to a temporary file. We then rename it to
// "<artifact-name>" after it has successfully downloaded, to
// signify that it is ready for usage.
let (file, temp_path) = NamedTempFile::new_in(&directory)
.map_err(|err| Error::Io {
message: "create temp file".to_string(),
err,
})?
.into_parts();
let mut file = tokio::fs::File::from_std(file);

// Fetch the artifact and write to the file in its entirety,
// replacing it if it exists.
Expand All @@ -57,16 +63,6 @@ pub async fn download_artifact(
.await
.map_err(Error::Response)?;

let mut file =
tokio::fs::File::create(&tmp_path).await.map_err(|err| {
Error::Io {
message: format!(
"create {}",
tmp_path.to_string_lossy()
),
err,
}
})?;
let mut stream = response.into_inner_stream();
while let Some(chunk) = stream
.try_next()
Expand All @@ -80,17 +76,19 @@ pub async fn download_artifact(
})
.await?;
}
file.sync_all().await.map_err(|err| Error::Io {
message: "sync temp file".to_string(),
err,
})?;
Copy link
Contributor

@ahl ahl Jul 16, 2022

Choose a reason for hiding this comment

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

  1. is it important to sync? This seems like it might be expensive, but I don't know how it's implemented on illumos. Is it important that it reaches persistent storage before the HTTP call returns? It seems like it's probably fine if it's still just in memory, but I don't claim to know for sure.

  2. Do we need to flush()? It seems like we do if there might be other threads/processes/HTTP queries reading the file.

From https://docs.rs/tokio/latest/tokio/fs/struct.File.html

A file will not be closed immediately when it goes out of scope if there are any IO operations that have not yet completed. To ensure that a file is closed immediately when it is dropped, you should call flush before dropping it. Note that this does not ensure that the file has been fully written to disk; the operating system might keep the changes around in an in-memory buffer. See the sync_all method for telling the OS to write the data to disk.

Copy link
Contributor Author

@iliana iliana Jul 16, 2022

Choose a reason for hiding this comment

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

... I think I read that and my brain latched onto sync_all in the docs. will swap that out

drop(file);

// Move the file to its final path.
let destination = directory.join(artifact.name);
tokio::fs::rename(&tmp_path, &destination).await.map_err(
|err| Error::Io {
message: format!(
"Renaming {tmp_path:?} to {destination:?}"
),
err,
},
)?;
temp_path.persist(&destination).map_err(|err| Error::Io {
message: format!("renaming {:?} to {destination:?}", err.path),
err: err.error,
})?;

Ok(())
}
}
Expand Down