[doc] HLD: BMC-initiated graceful shutdown and restart of the switch-host - #2500
Open
william8545 wants to merge 6 commits into
Open
[doc] HLD: BMC-initiated graceful shutdown and restart of the switch-host#2500william8545 wants to merge 6 commits into
william8545 wants to merge 6 commits into
Conversation
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
william8545
force-pushed
the
bmc-switch-host-graceful-shutdown
branch
from
August 12, 2026 15:34
bdc43b2 to
e8f757b
Compare
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
…host On a chassis where SONiC runs on both the BMC and the switch-host, the BMC removes switch-host power without telling the host, so the host cannot flush state, run any platform ordering it needs, or record why it went down. This design adds a bounded handshake before the cut: the BMC asks the host over gNOI System.Reboot(HALT), waits up to graceful_shutdown_timeout, and removes power either way. The host side is the existing reboot -p path, which runs the normal reboot teardown and exits with the OS still running, so the host reports completion in band on System.RebootStatus and no new platform API or hardware readiness signal is needed. It also adds a GRACEFUL_RESTART command composed from power off, a pause and power on, so a critical leak can be re-checked before power is raised. No SAI change, no new daemon, container, database table, CLI command or gNOI method. A platform opts in through a platform.json capability after passing the qualification checks in the document; every other platform keeps today's behaviour. Signed-off-by: William Tsai <willtsai@nvidia.com>
Two points reviewers are likely to ask about were implied but never said: - A failed pre-shutdown is not repaired. There is no rollback and no retry; the host reports FAILURE and stops, and the BMC responds exactly as it does to a timeout by removing power and recording the operation as forced. Says why: once a teardown has begun the only useful end states are powered off or rebooted by the watchdog. Also notes that when a pre-check refuses before the teardown starts, the cut is simply no gentler than today's. - Where vendor differences belong. scripts/reboot already branches on platform attributes, and the switch-host pre-shutdown runs that same body, so a vendor needing to stop an extra service or reorder something adds its branch there and nothing in this design changes. Names the two constraints on such a branch: it must not stop the services that carry the result back to the BMC, and it must be bounded because it spends the BMC's timeout. Signed-off-by: William Tsai <willtsai@nvidia.com>
william8545
force-pushed
the
bmc-switch-host-graceful-shutdown
branch
from
August 12, 2026 22:09
86c2702 to
cfb8871
Compare
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
Contributor
|
Really solid HLD — the safety invariant (power is removed on every path; the host's confirmation only changes how the op is recorded) reads consistently throughout, and the failure model and open-items are refreshingly honest. A few clarifying questions to make sure I'm reading the design right:
|
…leak Make result attribution positive instead of inferred, and state the predicate completely: graceful requires a tag that extracts exactly and matches, a terminal report, method HALT and status SUCCESS. Anything else is forced, and a tagged terminal FAILURE is check_failed. The tag rides EVERY terminal report, appended to the existing result string, which is what keeps a pre-shutdown that refuses early fast — an untagged report cannot end the wait, because acting on one is what the discarded inference rule did. Name the property that actually carries freshness: the host replaces the published report synchronously before it returns its acceptance, so the request id need only be unique within a bmcctld run. Correct the reporting path. It is database, gnmi, sysmgr and sonic-hostservice, which owns the published report; swss is not on it and the pre-shutdown stops swss itself on an ASAN image. Only gnmi declares any relation to syncd.service, an After=; the other three declare none. PartOf=syncd.service does occur on a platform-specific unit today, so absence on the reporting path stays a per-platform opt-in assertion. Bound the leak response against the class, not one instance. A critical leak is the highest priority, but preemption displaces a wait, not a call already running, and three such calls exist: do_power_cycle(), the set_admin_state() that raises power on a restart, and the gNOI RPC under its own deadline. GRACEFUL_RESTART's window is the shortest, not absent. Say which timeout gets frozen, why the CLI's upper limit can only be a coarse bound, and that freezing a non-zero value makes the deferred leak-triggered graceful_shutdown a real delay. Add the poll interval, the missing forced-reason codes and the near-match test cases, and correct the platform.json key count. Signed-off-by: William Tsai <willtsai@nvidia.com>
An internal-consistency sweep over the whole document, separate from the review response so the two are legible apart. None of these came from the review; all predate it. Timing had one bound where it needed two. The host's completion check is bounded by dpu_halt_services_timeout, 60 s when absent and polled every 5 s, and that key is read on any HALT rather than only on a DPU — so it governs the switch-host path and it, not the BMC's timeout, decides whether success can ever be reported. Rule 1 now states both bounds as one ordering. Rule 2 states the watchdog inequality outright, because the BMC owns its left side and the host reads its right, which is why two local checks exist rather than one. Figure 3 wrote the reboot cause two steps before the completion check, against both requirement 5 and 7.7. The cause write moves after the check, where the design puts it. 7.6 now says the existing guard has to give way: a power off is skipped today, and reported successful, when the recorded state is already POWERING_OFF or GRACEFUL_SHUTTING_DOWN — which is the state a graceful shutdown writes before it waits, so a preemptor's power off would be swallowed and the leak logged as served while power stayed on. Scope claimed more delta than it adds and then claimed none at all four lines later: the critical-leak check on POWER_ON already ships on all three entry points, only POWER_CYCLE is ungated, and the three power commands do not keep their behaviour unchanged because their oper-status verification becomes interruptible. Requirement 8 is qualified the same way. 7.8 overstated the watchdog backup: the arm sits near the end of the pre-shutdown and an existing exit path leaves the script after the teardown but before it, so a single orderly failure — not only a double one — can leave a torn-down host with no watchdog. Also: name what the reboot-cause rule keys on and why it has to be a distinct major cause, add determine-reboot-cause to the change table, align 7.9's opt-in list with the corrected reporting path, note the DPU-named key that governs this path, and separate the two senses of abort. Signed-off-by: William Tsai <willtsai@nvidia.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
…uthz scope Five areas, all found by checking the document's claims against the trees it cites rather than by new design. Timing. The document had the host's completion-check timeout containing the pre-shutdown. It does not: the host runs 'reboot -p' to completion in a blocking call and only then starts its own clock, so the BMC's graceful_shutdown_timeout is the binding bound. It serves two requirements and is stated as two -- success reportable is the precondition and carries a poll allowance; failure reportable carries the whole residual bound, decides only whether a failed pre-shutdown is diagnosable, and is therefore not an enablement gate. Above both sits a ceiling nobody had named: the reboot backend forwards RebootStatus to the host only while it treats the halt as in progress, and gives up after a fixed 260s wait for a platform that never halts, which on this path is every platform. Neither that ceiling nor the watchdog is readable from the BMC, and the two clocks share no origin, so the BMC sets its deadline before sending Reboot. Leak severity. The deferral of a leak-triggered graceful shutdown was written without a severity, so it read as excluding every level. Severity-specific fields exist with their own defaults and dispatch, and below critical a graceful shutdown is the intended setting. Two independent fields carry a critical trigger, not one. Priority is derived where a request is admitted and carried on it, because the trigger's severity is not otherwise available to the consumer; within the critical tier a direct power off displaces a graceful wait. The existing skip guard has three conditions and two callers, and the exemption is stated in the guard's own terms: a critical-leak action is exempt from the two recorded transitions and not from the live offline read, so a displacing power off runs while a genuine repeat is still absorbed. Attribution. The request id is a freshly generated UUID: the previous argument observed the wrong acceptance, since the BMC holds the reboot backend's, returned as soon as it has spawned its worker, while the host replaces its report later inside the call that worker makes. Any answer the backend substitutes keeps the tag, so the predicate gains a fifth condition -- the host always leaves the status message empty. Reason codes are an ordered first-match table. Authorization. Authorization is not per gNOI module: nearly every call site uses one 'gnoi' target matched by role prefix, so a single role spans the whole gNOI surface and most of gNSI. A certificate whose roles match none of them is refused only on writes, and the read-classified set includes file put and remove, OS install, factory reset and the gNSI rotations -- so what that gap admits is unauthorized mutation rather than read breadth. One Healthz handler performs no authorization at all. Platform contract. platform.json gains a second key because the switch-host's residual bound has to be sized for this host rather than inherited from a DPU setting, whose only in-tree value leaves too little of the ceiling for anything else and cannot be lowered without retiming the DPU legs. Two guarantees are scoped rather than restated, because the design does not deliver them absolutely, and every restatement is scoped with them. Power removal covers every outcome bmcctld handles; the daemon's own death abandons the operation instead, and that is a stated restriction. Requirement 7 refuses a power raise on a critical leak present in published state, since the startup power-on on an air-cooled chassis can precede any publisher -- a window section 7.9 records as accepted rather than covered. Everything else changes only how an operation is recorded. Signed-off-by: William Tsai <willtsai@nvidia.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
oleksandrivantsiv
previously approved these changes
Aug 21, 2026
…s prose 7.2 argued that the gNOI reporting path survives the teardown from two layered reasons - the teardown never stops syncd.service as a unit, with the absence of Requires=/BindsTo=/PartOf= as a fallback. The layering was wrong: the second reason carried the case, and once nothing on the path declares such a dependency it stops mattering how syncd ended. Trying to state that carefully is what the rest of this change stops doing. A static reading of unit files cannot establish a runtime property here, and this document already says why: drop-ins and generator-created .requires links change the effective graph, a stop-propagating edge can be declared on syncd.service rather than on the unit it affects, and swss is coupled to syncd by a container watch that appears in no unit file at all. So the assertion moves to where it can be met. 7.9 asserts a property of the rendered image - the reporting path serving after syncd ends, effective graph and scripted watches included, database named because gnmi and sysmgr require it - and 13.2 exercises it on hardware, injecting a clean syncd process exit where the teardown does not produce one and an outright unit stop, asserting the path alive through both. 7.2 keeps only the mechanism: nothing on the path is coupled to syncd in the current tree, gnmi's sole relation is an After=, and swss is the worked example of why that reading is not the guarantee. It no longer enumerates systemd directives, argues from dependency atoms, restates 7.9's per-platform scope and vendor prohibition, or repeats that the watchdog reboots a host left powered. swss was also answered wrongly before and is corrected here. swss.service declares nothing about syncd.service, but swss.sh wait - its ExecStart - blocks on a container watch over syncd, so a syncd container that goes away can end swss. The teardown does not take that container away on either branch, so swss runs on to the cut; whether an ASIC is still beneath it depends on whether the platform's teardown shut one down. Also 7.3 no longer claims a backend exit leaves the status message empty when the backend itself is stopping. Production installs no stop handler, the serve loop exits after HandleDone, and an active HALT status request is forwarded to the host, so no poller observes that state. Signed-off-by: William Tsai <willtsai@nvidia.com>
Collaborator
|
/azp run |
|
No pipelines are associated with this pull request. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
On a chassis where SONiC runs on both the BMC and the switch-host, the BMC removes switch-host power without telling the host, so the host cannot flush state, run any platform ordering it needs, or record why it went down.
This design adds a bounded handshake before the cut: the BMC asks the host over gNOI System.Reboot(HALT), waits up to graceful_shutdown_timeout, and removes power either way. The host side is the existing reboot -p path, which runs the normal reboot teardown and exits with the OS still running, so the host reports completion in band on System.RebootStatus and no new platform API or hardware readiness signal is needed. It also adds a GRACEFUL_RESTART command composed from power off, a pause and power on, so a critical leak can be re-checked before power is raised.
No SAI change, no new daemon, container, database table, CLI command or gNOI method. A platform opts in through a platform.json capability after passing the qualification checks in the document; every other platform keeps today's behaviour.