[jgallagher]

Attempting to reset a sidecar a SP to perform an update that includes a new FPGA image through the MGS on that sidecar's scrimlet cannot succeed:

1. MGS issues the sidecar reset with the SP reset watchdog armed
2. The new SP image boots; loading the new FPGA image causes the sidecar to reset
3. The sidecar resetting causes the switch zone to restart
4. The switch zone restarting tears down the MGS that issued the reset, meaning it won't be around to disarm the watchdog
5. The sidecar will (after ~90 seconds) roll itself back due, which causes the switch zone to bounce again and undoes the update that initiated this whole process

This matches the symptoms we see in #11512, if we assume that we eventually broke out of this loop by getting lucky and issuing the reset via the other scrimlet's MGS. It exactly matches behavior @labbott induced on london while trying to reproduce the dogfood issue.

This change means we'll always reject sidecar SP resets from that sidecar's local MGS. This is overly cautious, given we only need to reject them when there's a new FPGA image, and it means a reconfigurator-driven update now explicitly requires two healthy scrimlets/sidecars. (Based on the description above, we believe it already implicitly required that anyway!)

[hawkw]

The fix looks good to me!

[davepacheco]

This seems fine. I guess the official procedure for doing an update with a broken sidecar will be to use faux-mgs?

[jgallagher]

Laura noted in testing that the error we were sending back was wrong:

[null]root@oxz_switch1:~# curl -H 'api-version: 5.0.0' -X POST http://[::1]:12225/sp/switch/1/component/sp/reset
{"state":"reset_sp_of_local_sled"}

e15ea1b fixes that, and 9dfc079 adds an integration test. These made the diff quite a bit larger than it was when initially reviewed, but I think are pretty uncontroversial. I'll leave this open for a bit if @hawkw or @davepacheco want to give the latest changes a skim, but don't feel obligated to.

[jgallagher]

@claude review

[hawkw]

Still looks good to me, though I wonder if we could move more of the logic out of the API endpoint?

[hawkw]

take it or leave it: it seems to me that we could have all of this logic remain encapsulated behind ManagementSwitch::allowed_to_reset_sp, and include both the "is the SP local" and "are we allowed to reset our local sled/switch SP?" checks there, if we changed that function to return a Result? The error type would have to be something that bundled together both the ``SpComponentResetErrorandSpLookupError`, though, which might make it a bit messier. So, I dunno if it's actually worth it or not, just a thought.

[jgallagher]

Yeah, I started down that road and

The error type would have to be something that bundled together both the SpComponentResetError and SpLookupError, though, which might make it a bit messier

is exactly why I punted on it. I do think it would be nicer, though; I guess it's fine to have a SpResetValidityError that combines the two. I'll give that a whirl.

[jgallagher]

Mmkay it's less code but more awkward - didn't need a new error type at all, at the expense of a "I'm sorry for my past crimes" comment:

        // Error conversion here is a little awkward. If we don't yet have our
        // location map, we can convert that error into an
        // `SpComponentResetError` by travelling through `HttpError`, which
        // turns into `SpComponentResetError::Other { .. }`.
        let location_map = self.location_map().map_err(HttpError::from)?;

[hawkw]

Hmm, well, if this feels too awkward to you, I'd be fine with putting it back the way it was before, but I do also kinda like having everything in one function. Up to you.

[jgallagher]

Nah, I like it better in one place too. I can live with the out-of-place HttpError.