Remove ReqwestBlockIO and VolumeConstructionRequest::Url #1293
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As far as I can tell, nothing uses this anymore. With the exception of a
pantry integration test. That test is not interested in the Reqwest
part, and is just interested in a read-only parent with random data in
it, so I changed that to use a tempfile read-only parent.
Propolis will need to be updated. It has exactly one reference to
`VolumeConstructionRequest::Url`:
bin/propolis-server/src/lib/initializer.rs
385: VolumeConstructionRequest::Url { id, .. } => id.to_string(),
This is in some code that's extracting the UUIDs out of all the VCR
types, and also doesn't care about the Reqwest functionality.
jmpesp
reviewed
May 14, 2024
| @@ -18,11 +18,6 @@ pub enum VolumeConstructionRequest { | |||
| sub_volumes: Vec<VolumeConstructionRequest>, | |||
| read_only_parent: Option<Box<VolumeConstructionRequest>>, | |||
| }, | |||
| Url { | |||
There was a problem hiding this comment.
Unfortunately, I think we need to confirm that there's no existing VolumeConstructionRequest in the wild with this variant - removing this means that record won't deserialize.
There was a problem hiding this comment.
yeah agreed. i think its unlikely but we've still got to check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As far as I can tell, nothing uses this anymore. With the exception of a pantry integration test. That test is not interested in the Reqwest part, and is just interested in a read-only parent with random data in it, so I changed that to use a tempfile read-only parent.
Propolis will need to be updated. It has exactly one reference to
VolumeConstructionRequest::Url:That reference is in some code that's extracting the UUIDs out of all the VCR types, and also doesn't care about the Reqwest functionality.
This change still leaves in the pantry functionality to import from a URL. If we did at some point bring back support for booting a disk from an arbitrary URL, that feels like a better way to do it to me. If we do requests on-demand to the URL (the way ReqwestBlockIO does) we're subject to latency problems, the web server not supporting range requests, the web server straight up dying. whereas, while importing to pantry still has the tricky problem of making sure the URL is one we'd accept, it's much less spicy during VM operation.