[support bundle] Ensure that the transfer buffers are flushed #8708
Conversation
| @@ -592,11 +592,12 @@ impl BundleCollection { | |||
| })?; | |||
|
|
|||
| // Only stream at most "transfer_chunk_size" bytes at once | |||
| let remaining = std::cmp::min( | |||
| let chunk_size = std::cmp::min( | |||
There was a problem hiding this comment.
This is just for clarify; it's not the bug fix
| })?; | ||
|
|
||
| offset += self.transfer_chunk_size.get(); | ||
| offset += chunk_size; |
There was a problem hiding this comment.
This technically wasn't "wrong" before, because we didn't use the offset after the very last call, but over-shooting the actual length of the support bundle still felt fishy, so I changed this.
(The resulting behavior should be identical)
| // It is possible, although uncommon, for us to write to this file, | ||
| // drop the handle to it, and for it to have not been fully written to | ||
| // storage. | ||
| tmp_file.flush().await?; |
There was a problem hiding this comment.
This is the real fix of this issue
| tmp_file | ||
| .seek(tokio::io::SeekFrom::Current(i64::try_from(offset)?)) | ||
| .await?; | ||
| tmp_file.seek(tokio::io::SeekFrom::Start(offset)).await?; |
There was a problem hiding this comment.
Again, this isn't wrong, because we just opened the file, so it should be equivalent. However, IMO this version is less sketchy.
There was a problem hiding this comment.
Thanks, @smklein, great job running this down. There was one change you made that I don't quite understand, but otherwise this looks good.
| @@ -743,7 +758,7 @@ impl<'a> SupportBundleManager<'a> { | |||
| info!(log, "Bundle written successfully"); | |||
| let metadata = SupportBundleMetadata { | |||
| support_bundle_id, | |||
| state: SupportBundleState::Complete, | |||
| state: SupportBundleState::Incomplete, | |||
There was a problem hiding this comment.
Can you explain this change?
There was a problem hiding this comment.
TL;DR this is basically an informational change that results in no behavioral changes, but it was a code smell.
- Before we chunked support bundles, the status would return "has the bundle been created, and is it ready for usage"
- We used "Complete" to indicate "you're good to use the bundle" and "incomplete" to mean "the bundle has not been fully written to durable storage yet"
- When we switched to chunking behavior, this protocol changes a little bit. Nexus used the new "transfer + finalize" APIs, and would never infer that the bundle was ready from a call to "Transfer" alone
- However, we didn't change that returned state from transfer. Even though bundles are never ready-to-use before finalize, we returned "Complete"
- ... but this didn't matter, because Nexus was ignoring the returned state anyway.
So: It's more accurate to say "The bundle has not been finalized" during this transfer. Arguably we could remove this state from the internal transfer API altogether. But "Incomplete" is more accurate than "Complete"
| @@ -743,7 +758,7 @@ impl<'a> SupportBundleManager<'a> { | |||
| info!(log, "Bundle written successfully"); | |||
| let metadata = SupportBundleMetadata { | |||
| support_bundle_id, | |||
| state: SupportBundleState::Complete, | |||
| state: SupportBundleState::Incomplete, | |||
Fixes #8681