Skip to content

Commit 9d2a2ea

Browse files
committed
server: return better HTTP errors when not ensured
Currently, if the `propolis-server` APIs that require a running instance are called when the server has not received an `InstanceEnsure` request, we return a 404. Sled-agent and Nexus handle this status code by assuming *they* have done something wrong, such as attempting to call an API route that the server doesn't support. This results in issues where sled-agent assumes that it's the one that made a mistake when it tries to call APIs on a `propolis-server` that has lost its VM instance due to being restarted after a panic. To make it easier for callers distinguish between errors that indicate "this server doesn't know about a running instance" and errors that indicate "you attempted to call an API route that I don't know about", I've changed the status code returned by `propolis-server`s that haven't created an instance to HTTP [424 Failed Dependency]. The best thing about returning this status code is that it's "not 404", but it does arguably have the most correct semantics for this specific error: > The HTTP 424 Failed Dependency client error response code indicates > that the method could not be performed on the resource because the > requested action depended on another action, and that action failed. On the other hand, this is kind of a weird status code that's mostly used by WebDAV servers, so I'm open to using something else. We could alternatively return 410 Gone in these cases, but that status is already returned by servers whose VM instance has been deleted. I thought it might be nice to be able to distinguish between "not ensured" and "ensured, and then deleted"...but we could switch to 410 Gone here, too. [424 Failed Dependency]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/424
1 parent 1b34075 commit 9d2a2ea

File tree

1 file changed

+18
-18
lines changed

1 file changed

+18
-18
lines changed

bin/propolis-server/src/lib/server.rs

+18-18
Original file line numberDiff line numberDiff line change
@@ -263,12 +263,7 @@ impl DropshotEndpointContext {
263263
self.services.vm.lock().await,
264264
VmControllerState::as_controller,
265265
)
266-
.map_err(|_| {
267-
HttpError::for_not_found(
268-
None,
269-
"Server not initialized (no instance)".to_string(),
270-
)
271-
})
266+
.map_err(|_| no_instance_error())
272267
}
273268
}
274269

@@ -647,10 +642,7 @@ async fn instance_get_common(
647642
) -> Result<(api::Instance, VersionedInstanceSpec), HttpError> {
648643
let ctx = rqctx.context();
649644
match &*ctx.services.vm.lock().await {
650-
VmControllerState::NotCreated => Err(HttpError::for_not_found(
651-
None,
652-
"Server not initialized (no instance)".to_string(),
653-
)),
645+
VmControllerState::NotCreated => Err(no_instance_error()),
654646
VmControllerState::Created(vm) => {
655647
Ok((
656648
api::Instance {
@@ -724,10 +716,7 @@ async fn instance_state_monitor(
724716
let vm_state = ctx.services.vm.lock().await;
725717
match &*vm_state {
726718
VmControllerState::NotCreated => {
727-
return Err(HttpError::for_not_found(
728-
None,
729-
"Server not initialized (no instance)".to_string(),
730-
));
719+
return Err(no_instance_error());
731720
}
732721
VmControllerState::Created(vm) => vm.state_watcher().clone(),
733722
VmControllerState::Destroyed { state_watcher, .. } => {
@@ -747,12 +736,13 @@ async fn instance_state_monitor(
747736
// means it will never reach the number the client wants it to reach.
748737
// Inform the client of this condition so it doesn't wait forever.
749738
state_watcher.changed().await.map_err(|_| {
750-
HttpError::for_status(
751-
Some(format!(
739+
HttpError::for_client_error(
740+
Some(NO_INSTANCE.to_string()),
741+
http::status::StatusCode::GONE,
742+
format!(
752743
"No instance present; will never reach generation {}",
753744
gen
754-
)),
755-
http::status::StatusCode::GONE,
745+
),
756746
)
757747
})?;
758748
}
@@ -1065,6 +1055,16 @@ pub fn api() -> ApiDescription<Arc<DropshotEndpointContext>> {
10651055
api
10661056
}
10671057

1058+
const NO_INSTANCE: &str = "NO_INSTANCE";
1059+
1060+
fn no_instance_error() -> HttpError {
1061+
HttpError::for_client_error(
1062+
Some(NO_INSTANCE.to_string()),
1063+
http::StatusCode::FAILED_DEPENDENCY,
1064+
"Server not initialized (no instance)".to_string(),
1065+
)
1066+
}
1067+
10681068
#[cfg(test)]
10691069
mod tests {
10701070
#[test]

0 commit comments

Comments
 (0)