[MGS] Reject SP reset requests to our own sled by jgallagher · Pull Request #8830 · oxidecomputer/omicron

jgallagher · 2025-08-12T18:33:34Z

Fixes #8768 by implementing "option 1" from that issue's proposed solutions:

Extends MGS's config file slightly to specify which sled it's running on (once it determines which scrimlet position it's in).
Adds a config option to disallow SP reset requests to the local scrimlet.
Adds a custom dropshot error type for sp_component_reset() that can cleanly report this particular kind of failure.

This is definitely the first custom dropshot error in MGS (although we need some others: #8013, #8014, #8350), and maybe the first in omicron more generally? The server side is pretty straightforward, I think, but the client side ended up being a little messy. I'm not sure how much of that is because of the rework I had to do to try_all_serially() vs the inherent changes (e.g., the fact that we have a different progenitor error type for this one endpoint).

I'll definitely put this on a racklette before merging, but this should be close enough to review, and I'm very interested in feedback about integrating the custom error type.

hawkw

Nice to see the first use of structured dropshot errors in practice! I left a couple ltitle notes, but this looks good to me --- perhaps worth waiting to hear from Dave or Karen though for the meat of it.

hawkw · 2025-08-12T20:14:28Z

+/// Helper trait for error types we can get from various gateway_client API
+/// calls.
+///
+/// For each error type, we need to know whether we should retry the request (by
+/// sending it to the next MGS instance).
+pub(super) trait RetryableMgsError: fmt::Display {
+    fn should_try_next_mgs(&self) -> bool;


Take it or leave it: I wonder if this ought to be in gateway-client? There's other code in Nexus that has similar behavior of attempting a request through both MGSes that could also use this...

I can see that, although I worry slightly that this is already a little update-specific. If it were in gateway-client I'd be tempted to call it err.is_retryable(), except the real driver for this at all is to be able to say "if you get back SpComponentResetError::ResetSpOfLocalSled, you must send the request to the other MGS". So it's not really "retryable" so much as "you might have success if you tried the other MGS instead of this one". Maybe the name should reflect that more precisely, and then it would be fine to put it in gateway-client?

karencfv

Thanks for setting these safeguards! I have a few minor questions

karencfv · 2025-08-13T06:02:31Z

+    fn status_code(&self) -> ErrorStatusCode {
+        match self {
+            SpComponentResetError::ResetSpOfLocalSled => {
+                ErrorStatusCode::BAD_REQUEST


I'm on the fence about this status code. Would a 403 not be more suitable? A 400 may imply that the request is malformed?

Ehh, maybe? I think of 403 as being for auth problems; I used 400 because this is a malformed request semantically (calling "reset sled 14's SP" is not a legal request to the MGS on switch 0). But I also think this doesn't really matter, so if 403 seems like a better fit I'm happy to change it.

I used 400 because this is a malformed request semantically (calling "reset sled 14's SP" is not a legal request to the MGS on switch 0)

Fair enough! I leave it to you to go with either one

karencfv · 2025-08-13T06:25:41Z

+allow_local_sled_sp_reset = true
+
+[[switch.location.description]]
+name = "switch1"
+local_sled = 1
+allow_local_sled_sp_reset = true


This is the config file that is used for systems like a4x2. Would we not want it to emulate a behaviour that is closer to the real thing to avoid potential bugs? Or, more concretely, in which cases do we anticipate wanting to reset an sp-sim on it's own local sled here?

The thing I was mainly concerned about was any unit or integration tests where we're doing simulated SP resets today. For any of those, I disabling resets if the fake SP thinks it's connected to a particular sled is just a headache with no benefit.

I'm a lot less sure about a4x2, but also it just doesn't really work anyway? Resetting an SP in a4x2 doesn't do anything, because it's not hooked up to the "sleds" in any meaningful way. Or in other words - I think for this to matter we'd need to be in an environment where we're testing upgrades, and we can't do that in a4x2 without a lot of work. I defaulted to true here in case there were any unit tests that happened to use this config file. I could change it to false and see if anything breaks, though?

I'm a lot less sure about a4x2, but also it just doesn't really work anyway? Resetting an SP in a4x2 doesn't do anything, because it's not hooked up to the "sleds" in any meaningful way. Or in other words - I think for this to matter we'd need to be in an environment where we're testing upgrades, and we can't do that in a4x2 without a lot of work.

Fair 🤔. I think I was mostly concerned about someone developing something against an a4x2 only to find that it's buggy when testing against a racklette. It'd be nice to catch something like this beforehand.

I could change it to false and see if anything breaks, though?

That'd be nice! But also I guess I don't feel sufficiently strongly to push for it. I leave it up to you whether we change this or not!

…-local-sled-sp

jgallagher · 2025-08-13T14:44:44Z

Testing on dublin looks good: from switch zone 0, I can reset the other sleds/psc/switches, but trying to reset sled 14 fails:

root@oxz_switch0:~# curl -v -X POST http://[::1]:12225/sp/sled/14/component/sp/reset
*   Trying [::1]:12225...
* Connected to ::1 (::1) port 12225
* using HTTP/1.x
> POST /sp/sled/14/component/sp/reset HTTP/1.1
> Host: [::1]:12225
> User-Agent: curl/8.14.0
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 400 Bad Request
< content-type: application/json
< x-request-id: dac778bc-3b7b-4786-858e-13126a77c034
< content-length: 34
< date: Wed, 13 Aug 2025 13:53:54 GMT
<
* Connection #0 to host ::1 left intact
{"state":"reset_sp_of_local_sled"}

And similarly, from switch zone 1, I can reset everything except its own local sled:

root@oxz_switch1:~# curl -v -X POST http://[::1]:12225/sp/sled/16/component/sp/reset
*   Trying [::1]:12225...
* Connected to ::1 (::1) port 12225
* using HTTP/1.x
> POST /sp/sled/16/component/sp/reset HTTP/1.1
> Host: [::1]:12225
> User-Agent: curl/8.14.0
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 400 Bad Request
< content-type: application/json
< x-request-id: ae0386a1-1228-41ed-858d-b7b6961fd540
< content-length: 34
< date: Wed, 13 Aug 2025 13:52:42 GMT
<
* Connection #0 to host ::1 left intact
{"state":"reset_sp_of_local_sled"}

karencfv

Looks good!

karencfv · 2025-08-14T02:58:01Z

+allow_local_sled_sp_reset = true
+
+[[switch.location.description]]
+name = "switch1"
+local_sled = 1
+allow_local_sled_sp_reset = true


I'm a lot less sure about a4x2, but also it just doesn't really work anyway? Resetting an SP in a4x2 doesn't do anything, because it's not hooked up to the "sleds" in any meaningful way. Or in other words - I think for this to matter we'd need to be in an environment where we're testing upgrades, and we can't do that in a4x2 without a lot of work.

Fair 🤔. I think I was mostly concerned about someone developing something against an a4x2 only to find that it's buggy when testing against a racklette. It'd be nice to catch something like this beforehand.

I could change it to false and see if anything breaks, though?

That'd be nice! But also I guess I don't feel sufficiently strongly to push for it. I leave it up to you whether we change this or not!

karencfv · 2025-08-14T02:59:21Z

+    fn status_code(&self) -> ErrorStatusCode {
+        match self {
+            SpComponentResetError::ResetSpOfLocalSled => {
+                ErrorStatusCode::BAD_REQUEST


I used 400 because this is a malformed request semantically (calling "reset sled 14's SP" is not a legal request to the MGS on switch 0)

Fair enough! I leave it to you to go with either one

jgallagher added 8 commits August 12, 2025 12:42

MGS: add new config options to describe local sled

b57c428

add new fields to LocationMap

b636a80

plumb rejecting "reset local sled" requests

ae1f3c0

use custom error type for component reset

74b8f3b

openapi and update clients

70d0f5a

clean up weird nested result, now that we have a way to

aac04f8

rustdoc

e644015

self editing

9dc8ec0

jgallagher requested review from davepacheco, hawkw and karencfv August 12, 2025 18:33

re-export to fix rustdoc link

a802b76

hawkw reviewed Aug 12, 2025

View reviewed changes

karencfv reviewed Aug 13, 2025

View reviewed changes

jgallagher added 3 commits August 13, 2025 10:28

report values of duplicates instead of just "there were duplicates"

67da4f8

note expected value in production

548412a

Merge remote-tracking branch 'origin/main' into john/mgs-reject-reset…

acd83ab

…-local-sled-sp

karencfv approved these changes Aug 14, 2025

View reviewed changes

jgallagher merged commit a62fc86 into main Aug 14, 2025
18 checks passed

jgallagher deleted the john/mgs-reject-reset-local-sled-sp branch August 14, 2025 15:27

Uh oh!

Conversation

jgallagher commented Aug 12, 2025

Uh oh!

hawkw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgallagher commented Aug 13, 2025

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

karencfv Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

karencfv Aug 14, 2025 •

edited

Loading

karencfv Aug 14, 2025 •

edited

Loading