clean up tempfiles when downloading artifacts #1447
Conversation
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
looks good. one minor thing to consider.
sled-agent/src/updates.rs
Outdated
| file.sync_all().await.map_err(|err| Error::Io { | ||
| message: "sync temp file".to_string(), | ||
| err, | ||
| })?; |
There was a problem hiding this comment.
-
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.
-
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.
There was a problem hiding this comment.
... I think I read that and my brain latched onto sync_all in the docs. will swap that out
Crucible changes:
Make crutest use BlockIO trait instead of a Guest (#1452)
Use new syncfs syscall (#1427)
Increment write backpressure before deferred encryption (#1444)
Use RAII handles for backpressure (#1443)
Fine-tuning backpressure clamping, and API cleanups (#1442)
Fix outdated comment (#1447)
Update Rust crate rusqlite to 0.32 (#1418)
Fix write reordering bug (#1448)
Remove `history_file` (#1446)
Remove optionality for `BlockRes` in `DeferredWrite` (#1441)
Propolis changes
instance spec rework: tighten up component naming (#761)
instance spec rework: remove most dependencies on `InstanceSpecV0` from propolis-server (#757)
fix new 1.81.0 warning and clippy error (#760)
standalone: be more helpful with bad block device configs (#758)
Crucible changes:
Make crutest use BlockIO trait instead of a Guest (#1452)
Use new syncfs syscall (#1427)
Increment write backpressure before deferred encryption (#1444)
Use RAII handles for backpressure (#1443)
Fine-tuning backpressure clamping, and API cleanups (#1442)
Fix outdated comment (#1447)
Update Rust crate rusqlite to 0.32 (#1418)
Fix write reordering bug (#1448)
Remove `history_file` (#1446)
Remove optionality for `BlockRes` in `DeferredWrite` (#1441)
Propolis changes
instance spec rework: tighten up component naming (#761)
instance spec rework: remove most dependencies on `InstanceSpecV0` from
propolis-server (#757)
fix new 1.81.0 warning and clippy error (#760)
standalone: be more helpful with bad block device configs (#758)
Co-authored-by: Alan Hanson <alan@oxide.computer>
re: #1442 (comment) -- uses the tempfile crate to clean up the temporary file we're writing to in case we run into an error before persisting it to the final path.