From 22d09372d9b8d7224244f1ecb2f876b56bb2a0bc Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Sat, 21 Mar 2026 03:44:32 +0000 Subject: [PATCH 1/9] Add design doc: replace docker exec gnoi_client with native gRPC calls Proposes replacing subprocess-based gnoi_client invocations in gnoi_shutdown_daemon with direct Python gRPC calls. Documents the gnoi_client output format, a parsing bug in RebootStatus polling, and the difficulty of diagnosing RPC failures through Go panic stack traces. Ref: sonic-net/sonic-host-services#360 Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 306 +++++++++++++++++++++++++++++++++ 1 file changed, 306 insertions(+) create mode 100644 doc/gnoi-native-grpc-design.md diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md new file mode 100644 index 00000000..8e7fbb5f --- /dev/null +++ b/doc/gnoi-native-grpc-design.md @@ -0,0 +1,306 @@ +# Design: Replace `docker exec gnoi_client` with Native gRPC Calls + +## 1. Background + +The `gnoi_shutdown_daemon` on SmartSwitch NPU orchestrates graceful DPU shutdown by issuing gNOI `System.Reboot(HALT)` and polling `System.RebootStatus`. Today it does this by shelling out: + +``` +docker exec gnmi gnoi_client -target=: -notls -module System -rpc Reboot ... +docker exec gnmi gnoi_client -target=: -notls -module System -rpc RebootStatus +``` + +This has several problems: + +| Problem | Impact | +|---------|--------| +| Requires the `gnmi` container to be running and healthy | If gnmi container is restarting or unhealthy, DPU shutdown fails silently | +| Subprocess overhead per RPC call | Extra process creation, Docker CLI round-trip, stdout parsing | +| Fragile output parsing | `"reboot complete" in out_s.lower()` breaks on any output format change | +| No structured error handling | gRPC status codes are lost; only `rc != 0` is checked | +| Error output is a Go panic stack trace | Extremely painful to diagnose failures (see §1.1) | +| Security surface | Shell-out through Docker CLI is a wider attack surface than a direct socket | + +### 1.1 gnoi_client Output Format Analysis + +The `gnoi_client` binary in sonic-gnmi is a Go CLI tool. Understanding its output format reveals why the current approach is fragile: + +**Reboot RPC (`-rpc Reboot`):** +- On **success**: prints `"System Reboot\n"` to stdout, exits 0. No structured output. +- On **failure**: calls `panic(err.Error())`, which dumps a **Go panic stack trace** to stderr and exits with a non-zero code. The daemon only checks `rc != 0` — the actual gRPC error code, message, and details are buried in a multi-line panic dump that is not parsed. + +**RebootStatus RPC (`-rpc RebootStatus`):** +- On **success**: prints `"System RebootStatus\n"` header followed by JSON-marshaled `RebootStatusResponse`, e.g.: + ```json + System RebootStatus + {"active":false,"status":{"status":"STATUS_SUCCESS","message":"..."}} + ``` +- On **failure**: same `panic(err.Error())` — Go stack trace, non-zero exit. + +**The parsing bug:** The daemon currently checks: +```python +if rc_s == 0 and out_s and ("reboot complete" in out_s.lower()): + return True +``` +But the actual protobuf `RebootStatusResponse` serialized to JSON contains fields like `"active":false` and `"status":"STATUS_SUCCESS"` — the string `"reboot complete"` never appears in the output. This means the poll loop **always times out** regardless of whether the DPU successfully halted, and the daemon proceeds purely on the timeout path. + +**Why this matters for error diagnosis:** When a gNOI RPC fails (DPU unreachable, TLS mismatch, auth failure, server-side error), the only signal is a Go panic: +``` +panic: rpc error: code = Unavailable desc = connection error: ... + +goroutine 1 [running]: +main.main() + /sonic/gnoi_client/gnoi_client.go:42 +... +``` +The daemon captures this in `err` (stderr) but never logs or inspects it — it just logs `"Reboot command failed"` with no context. Diagnosing production failures requires SSHing into the switch, manually running the docker exec command, and reading Go stack traces. + +## 2. Goal + +Replace the subprocess-based `gnoi_client` invocations with direct Python gRPC calls using generated protobuf stubs for the [OpenConfig gNOI System service](https://github.com/openconfig/gnoi/blob/main/system/system.proto). + +## 3. Scope + +### In Scope +- Generate or vendor Python gRPC stubs for `gnoi.system.System` (Reboot, RebootStatus RPCs) +- Create a lightweight `GnoiClient` wrapper class +- Refactor `GnoiRebootHandler._send_reboot_command()` and `_poll_reboot_status()` to use native gRPC +- Remove `execute_command()` helper (becomes unused) +- Update unit tests to mock at the gRPC stub level +- Add `grpcio` and `protobuf` to package dependencies + +### Out of Scope +- TLS/mTLS on the midplane channel (future work; midplane is trusted today) +- Refactoring the daemon's main loop or config DB subscription logic +- Other gNOI services beyond `System` +- Changes to how DPU IP/port are discovered from CONFIG_DB + +## 4. Design + +### 4.1 Phase 1 — Proto Stubs + +Vendor pre-generated Python stubs from the gNOI `system.proto` definition. + +**Files to add:** +``` +host_modules/gnoi/ +├── __init__.py +├── system_pb2.py # generated message classes +└── system_pb2_grpc.py # generated service stubs +``` + +The stubs are generated from: +- https://github.com/openconfig/gnoi/blob/main/system/system.proto +- https://github.com/openconfig/gnoi/blob/main/types/types.proto (dependency) + +Generation command (for reference / CI reproducibility): +```bash +python -m grpc_tools.protoc \ + -I./proto \ + --python_out=host_modules/gnoi \ + --grpc_python_out=host_modules/gnoi \ + system/system.proto types/types.proto +``` + +**Why vendor instead of build-time generation?** +- sonic-host-services has no existing proto compilation infrastructure +- The gNOI System proto is stable (no changes in years) +- Keeps the build simple; can migrate to build-time generation later if more protos are needed + +### 4.2 Phase 2 — GnoiClient Wrapper + +A thin wrapper providing the two RPCs we need: + +```python +# host_modules/gnoi/client.py + +import grpc +from . import system_pb2, system_pb2_grpc + +class GnoiClient: + """Lightweight gNOI System service client for DPU communication.""" + + def __init__(self, target: str, timeout: int = 30): + """ + Args: + target: gRPC target in "host:port" format + timeout: Default RPC timeout in seconds + """ + self._channel = grpc.insecure_channel(target) + self._stub = system_pb2_grpc.SystemStub(self._channel) + self._timeout = timeout + + def reboot(self, method: int = 3, message: str = "") -> None: + """ + Send System.Reboot RPC. + + Args: + method: RebootMethod enum value (3 = HALT) + message: Human-readable reason string + + Raises: + grpc.RpcError: on any gRPC failure + """ + request = system_pb2.RebootRequest( + method=method, + message=message, + ) + self._stub.Reboot(request, timeout=self._timeout) + + def reboot_status(self) -> system_pb2.RebootStatusResponse: + """ + Poll System.RebootStatus RPC. + + Returns: + RebootStatusResponse with .active and .wait fields + + Raises: + grpc.RpcError: on any gRPC failure + """ + request = system_pb2.RebootStatusRequest() + return self._stub.RebootStatus(request, timeout=self._timeout) + + def close(self): + """Close the underlying gRPC channel.""" + if self._channel: + self._channel.close() + + def __enter__(self): + return self + + def __exit__(self, *args): + self.close() +``` + +### 4.3 Phase 3 — Refactor gnoi_shutdown_daemon + +Replace the two subprocess call sites in `GnoiRebootHandler`: + +#### `_send_reboot_command` (before) +```python +def _send_reboot_command(self, dpu_name, dpu_ip, port): + reboot_cmd = ["docker", "exec", "gnmi", "gnoi_client", ...] + rc, out, err = execute_command(reboot_cmd, ...) + return rc == 0 +``` + +#### `_send_reboot_command` (after) +```python +def _send_reboot_command(self, dpu_name, dpu_ip, port): + try: + with GnoiClient(f"{dpu_ip}:{port}", timeout=REBOOT_RPC_TIMEOUT_SEC) as client: + client.reboot( + method=REBOOT_METHOD_HALT, + message="Triggered by SmartSwitch graceful shutdown" + ) + return True + except grpc.RpcError as e: + logger.log_error(f"{dpu_name}: gNOI Reboot failed: {e.code()} {e.details()}") + return False +``` + +#### `_poll_reboot_status` (before) +```python +def _poll_reboot_status(self, dpu_name, dpu_ip, port): + status_cmd = ["docker", "exec", "gnmi", "gnoi_client", ...] + while time.monotonic() < deadline: + rc_s, out_s, _ = execute_command(status_cmd, ...) + if rc_s == 0 and "reboot complete" in out_s.lower(): + return True +``` + +#### `_poll_reboot_status` (after) +```python +def _poll_reboot_status(self, dpu_name, dpu_ip, port): + deadline = time.monotonic() + _get_halt_timeout() + with GnoiClient(f"{dpu_ip}:{port}", timeout=STATUS_RPC_TIMEOUT_SEC) as client: + while time.monotonic() < deadline: + try: + resp = client.reboot_status() + if not resp.active: + status_str = system_pb2.RebootStatus.Status.Name(resp.status.status) + logger.log_notice(f"{dpu_name}: RebootStatus complete: {status_str} - {resp.status.message}") + return resp.status.status == system_pb2.RebootStatus.Status.STATUS_SUCCESS + except grpc.RpcError as e: + logger.log_warning( + f"{dpu_name}: RebootStatus poll error: code={e.code()} details={e.details()}" + ) + time.sleep(STATUS_POLL_INTERVAL_SEC) + return False +``` + +**Key improvements over the subprocess approach:** +- **Fixes the parsing bug**: checks `resp.active == False` directly instead of the broken `"reboot complete" in stdout` match that never triggers +- **Distinguishes success from failure**: inspects `resp.status.status` enum (`STATUS_SUCCESS` vs `STATUS_FAILURE` vs `STATUS_RETRIABLE_FAILURE`) +- **Actionable error logs**: gRPC errors include status code and details (e.g., `code=UNAVAILABLE details=connection refused`) instead of opaque "command failed" + +#### Removals +- `execute_command()` function — no longer needed +- `import subprocess` — no longer needed + +#### Additions +- `import grpc` +- `from host_modules.gnoi.client import GnoiClient` + +### 4.4 Phase 4 — Update Tests + +Current tests mock `execute_command` and check return codes. New tests mock at the gRPC level: + +```python +@mock.patch('gnoi_shutdown_daemon.GnoiClient') +def test_send_reboot_command_success(self, MockClient): + mock_client = MockClient.return_value.__enter__.return_value + # reboot() returns None on success + mock_client.reboot.return_value = None + + result = handler._send_reboot_command("DPU0", "10.0.0.1", "8080") + assert result is True + mock_client.reboot.assert_called_once() + +@mock.patch('gnoi_shutdown_daemon.GnoiClient') +def test_send_reboot_command_failure(self, MockClient): + mock_client = MockClient.return_value.__enter__.return_value + mock_client.reboot.side_effect = grpc.RpcError() + + result = handler._send_reboot_command("DPU0", "10.0.0.1", "8080") + assert result is False +``` + +### 4.5 Dependencies + +| Package | Version | Notes | +|---------|---------|-------| +| `grpcio` | >=1.51.0 | Already in SONiC build environment | +| `protobuf` | >=4.21.0 | Already in SONiC build environment | + +Verify these are available in the sonic-host-services build context. If not, add to `setup.py` `install_requires`. + +## 5. Implementation Plan + +| Phase | Description | PR | +|-------|-------------|----| +| 1 | Vendor gNOI System proto stubs | PR #1 | +| 2 | Add `GnoiClient` wrapper + unit tests | PR #1 (same) | +| 3 | Refactor `gnoi_shutdown_daemon` to use `GnoiClient` | PR #1 (same) | +| 4 | Update existing daemon tests | PR #1 (same) | + +All phases can ship as a single PR since they form one atomic change — the old subprocess path is fully replaced. + +## 6. Testing + +- **Unit tests**: Mock gRPC stubs, verify correct protobuf messages are sent, verify error handling for various `grpc.StatusCode` values +- **Integration test**: On a SmartSwitch testbed, trigger `config chassis modules shutdown DPU0` and verify gNOI HALT is sent and RebootStatus is polled successfully via syslog +- **Regression**: Existing CI pipeline covers the daemon; updated mocks ensure no regressions + +## 7. Risks & Mitigations + +| Risk | Mitigation | +|------|------------| +| gRPC/protobuf not available in host environment | Verify during build; these are already used by other SONiC components | +| Proto stub drift from upstream gnoi | Pin to a specific gnoi commit; stubs are stable | +| Insecure channel on midplane | Same trust model as today's `gnoi_client -notls`; TLS is future work | + +## 8. Future Work + +- **TLS support**: Add optional mTLS when midplane security is hardened +- **Build-time proto generation**: If more gNOI/gNMI services are needed, add a proto compilation step +- **Connection pooling**: Reuse gRPC channels across polls instead of creating per-call (minor optimization) From 90df6107e0f154dbe43695d85d5a1d0a150f2807 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Sat, 21 Mar 2026 04:05:12 +0000 Subject: [PATCH 2/9] design doc: add reference code for gnoi_client and openconfig system proto Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 226 ++++++++++++++++++++++++++++++++- 1 file changed, 225 insertions(+), 1 deletion(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index 8e7fbb5f..d65ce94c 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -299,7 +299,231 @@ All phases can ship as a single PR since they form one atomic change — the old | Proto stub drift from upstream gnoi | Pin to a specific gnoi commit; stubs are stable | | Insecure channel on midplane | Same trust model as today's `gnoi_client -notls`; TLS is future work | -## 8. Future Work +## 8. Reference Code + +This section provides the upstream source code that this design replaces and builds upon, so the document is self-contained. + +### 8.1 Current `gnoi_client` Entry Point + +**Source:** [`sonic-gnmi/gnoi_client/gnoi_client.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/gnoi_client.go) + +```go +package main + +import ( + "context" + "os" + "os/signal" + + "github.com/google/gnxi/utils/credentials" + "github.com/sonic-net/sonic-gnmi/gnoi_client/config" + "github.com/sonic-net/sonic-gnmi/gnoi_client/system" + "google.golang.org/grpc" +) + +func main() { + config.ParseFlag() + opts := credentials.ClientCredentials(*config.TargetName) + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + c := make(chan os.Signal, 1) + signal.Notify(c, os.Interrupt) + <-c + cancel() + }() + conn, err := grpc.Dial(*config.Target, opts...) + if err != nil { + panic(err.Error()) + } + + switch *config.Module { + case "System": + switch *config.Rpc { + case "Reboot": + system.Reboot(conn, ctx) + case "RebootStatus": + system.RebootStatus(conn, ctx) + case "SetPackage": + system.SetPackage(conn, ctx) + // ... other RPCs omitted for brevity + } + // ... other modules omitted + } +} +``` + +### 8.2 `gnoi_client/system/reboot.go` — Reboot & RebootStatus Implementation + +**Source:** [`sonic-gnmi/gnoi_client/system/reboot.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go) + +```go +package system + +import ( + "context" + "encoding/json" + "fmt" + pb "github.com/openconfig/gnoi/system" + "github.com/sonic-net/sonic-gnmi/gnoi_client/config" + "github.com/sonic-net/sonic-gnmi/gnoi_client/utils" + "google.golang.org/grpc" +) + +func Reboot(conn *grpc.ClientConn, ctx context.Context) { + fmt.Println("System Reboot") + ctx = utils.SetUserCreds(ctx) + sc := pb.NewSystemClient(conn) + req := &pb.RebootRequest{} + json.Unmarshal([]byte(*config.Args), req) + _, err := sc.Reboot(ctx, req) + if err != nil { + panic(err.Error()) // ← Error is lost in a Go panic stack trace + } +} + +func RebootStatus(conn *grpc.ClientConn, ctx context.Context) { + fmt.Println("System RebootStatus") + ctx = utils.SetUserCreds(ctx) + sc := pb.NewSystemClient(conn) + req := &pb.RebootStatusRequest{} + resp, err := sc.RebootStatus(ctx, req) + if err != nil { + panic(err.Error()) + } + respstr, err := json.Marshal(resp) + if err != nil { + panic(err.Error()) + } + fmt.Println(string(respstr)) // ← Output that daemon tries to parse +} +``` + +**Key observations:** +- `Reboot()` prints `"System Reboot\n"` on success, panics on failure — no structured output +- `RebootStatus()` prints JSON-serialized `RebootStatusResponse` — the daemon searches for `"reboot complete"` which never appears in this JSON +- Errors use `panic()` which produces Go stack traces instead of parseable error output + +### 8.3 `gnoi_client/system/set_package.go` — SetPackage Implementation + +**Source:** [`sonic-gnmi/gnoi_client/system/set_package.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/set_package.go) + +```go +func SetPackage(conn *grpc.ClientConn, ctx context.Context) { + ctx = utils.SetUserCreds(ctx) + sc := newSystemClient(conn) + + download := &common.RemoteDownload{Path: *url} + pkg := &system.Package{ + Filename: *filename, + Version: *version, + Activate: *activate, + RemoteDownload: download, + } + + req := &system.SetPackageRequest{ + Request: &system.SetPackageRequest_Package{Package: pkg}, + } + + stream, err := sc.SetPackage(ctx) + if err != nil { + return fmt.Errorf("error creating stream: %v", err) + } + stream.Send(req) + stream.CloseSend() + resp, err := stream.CloseAndRecv() + // ... +} +``` + +This is the RPC path that triggered the `too_many_pings` issue fixed in PR #620 — the streaming `SetPackage` call is long-lived and sensitive to keepalive misconfiguration. + +### 8.4 OpenConfig gNOI System Proto Definition + +**Source:** [`openconfig/gnoi/system/system.proto`](https://github.com/openconfig/gnoi/blob/main/system/system.proto) (vendored at `sonic-gnmi/vendor/github.com/openconfig/gnoi/system/system.proto`) + +```protobuf +syntax = "proto3"; +package gnoi.system; + +service System { + rpc Reboot(RebootRequest) returns (RebootResponse) {} + rpc RebootStatus(RebootStatusRequest) returns (RebootStatusResponse) {} + rpc CancelReboot(CancelRebootRequest) returns (CancelRebootResponse) {} + rpc SetPackage(stream SetPackageRequest) returns (SetPackageResponse) {} + rpc KillProcess(KillProcessRequest) returns (KillProcessResponse) {} + rpc Time(TimeRequest) returns (TimeResponse) {} + // ... Ping, Traceroute, SwitchControlProcessor omitted +} + +message RebootRequest { + RebootMethod method = 1; + uint64 delay = 2; // Delay in nanoseconds + string message = 3; // Informational reason + repeated types.Path subcomponents = 4; + bool force = 5; +} + +message RebootResponse {} + +enum RebootMethod { + UNKNOWN = 0; + COLD = 1; // Shutdown and restart OS and all hardware + POWERDOWN = 2; // Halt and power down + HALT = 3; // Halt (used for DPU shutdown) + WARM = 4; // Reload configuration only + NSF = 5; // Non-stop-forwarding reboot + POWERUP = 7; // Apply power +} + +message RebootStatusRequest { + repeated types.Path subcomponents = 1; +} + +message RebootStatusResponse { + bool active = 1; // If reboot is active + uint64 wait = 2; // Time left until reboot (ns) + uint64 when = 3; // Reboot time (ns since epoch) + string reason = 4; + uint32 count = 5; + RebootMethod method = 6; + RebootStatus status = 7; // Only meaningful when active = false +} + +message RebootStatus { + enum Status { + STATUS_UNKNOWN = 0; + STATUS_SUCCESS = 1; + STATUS_RETRIABLE_FAILURE = 2; + STATUS_FAILURE = 3; + } + Status status = 1; + string message = 2; +} + +// SetPackage — streaming RPC for software packages +message SetPackageRequest { + oneof request { + Package package = 1; + bytes contents = 2; + types.HashType hash = 3; + } +} + +message Package { + string filename = 1; + string version = 4; + bool activate = 5; + common.RemoteDownload remote_download = 6; +} +``` + +**Key proto details for the Python wrapper:** +- `RebootMethod.HALT = 3` — the method used for DPU graceful shutdown +- `RebootStatusResponse.active == false` with `status.status == STATUS_SUCCESS` indicates successful halt completion +- `SetPackage` is a client-streaming RPC — the only streaming call in our scope + +## 9. Future Work - **TLS support**: Add optional mTLS when midplane security is hardened - **Build-time proto generation**: If more gNOI/gNMI services are needed, add a proto compilation step From 8bbf237a5f12b4e9836df5892d37910b12b54042 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Sat, 21 Mar 2026 04:06:42 +0000 Subject: [PATCH 3/9] design doc: remove unrelated SetPackage references from reference section Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 57 ++-------------------------------- 1 file changed, 2 insertions(+), 55 deletions(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index d65ce94c..1c95ba7d 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -344,8 +344,6 @@ func main() { system.Reboot(conn, ctx) case "RebootStatus": system.RebootStatus(conn, ctx) - case "SetPackage": - system.SetPackage(conn, ctx) // ... other RPCs omitted for brevity } // ... other modules omitted @@ -404,41 +402,7 @@ func RebootStatus(conn *grpc.ClientConn, ctx context.Context) { - `RebootStatus()` prints JSON-serialized `RebootStatusResponse` — the daemon searches for `"reboot complete"` which never appears in this JSON - Errors use `panic()` which produces Go stack traces instead of parseable error output -### 8.3 `gnoi_client/system/set_package.go` — SetPackage Implementation - -**Source:** [`sonic-gnmi/gnoi_client/system/set_package.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/set_package.go) - -```go -func SetPackage(conn *grpc.ClientConn, ctx context.Context) { - ctx = utils.SetUserCreds(ctx) - sc := newSystemClient(conn) - - download := &common.RemoteDownload{Path: *url} - pkg := &system.Package{ - Filename: *filename, - Version: *version, - Activate: *activate, - RemoteDownload: download, - } - - req := &system.SetPackageRequest{ - Request: &system.SetPackageRequest_Package{Package: pkg}, - } - - stream, err := sc.SetPackage(ctx) - if err != nil { - return fmt.Errorf("error creating stream: %v", err) - } - stream.Send(req) - stream.CloseSend() - resp, err := stream.CloseAndRecv() - // ... -} -``` - -This is the RPC path that triggered the `too_many_pings` issue fixed in PR #620 — the streaming `SetPackage` call is long-lived and sensitive to keepalive misconfiguration. - -### 8.4 OpenConfig gNOI System Proto Definition +### 8.3 OpenConfig gNOI System Proto Definition **Source:** [`openconfig/gnoi/system/system.proto`](https://github.com/openconfig/gnoi/blob/main/system/system.proto) (vendored at `sonic-gnmi/vendor/github.com/openconfig/gnoi/system/system.proto`) @@ -450,10 +414,9 @@ service System { rpc Reboot(RebootRequest) returns (RebootResponse) {} rpc RebootStatus(RebootStatusRequest) returns (RebootStatusResponse) {} rpc CancelReboot(CancelRebootRequest) returns (CancelRebootResponse) {} - rpc SetPackage(stream SetPackageRequest) returns (SetPackageResponse) {} rpc KillProcess(KillProcessRequest) returns (KillProcessResponse) {} rpc Time(TimeRequest) returns (TimeResponse) {} - // ... Ping, Traceroute, SwitchControlProcessor omitted + // ... Ping, Traceroute, SwitchControlProcessor, SetPackage omitted } message RebootRequest { @@ -501,27 +464,11 @@ message RebootStatus { string message = 2; } -// SetPackage — streaming RPC for software packages -message SetPackageRequest { - oneof request { - Package package = 1; - bytes contents = 2; - types.HashType hash = 3; - } -} - -message Package { - string filename = 1; - string version = 4; - bool activate = 5; - common.RemoteDownload remote_download = 6; -} ``` **Key proto details for the Python wrapper:** - `RebootMethod.HALT = 3` — the method used for DPU graceful shutdown - `RebootStatusResponse.active == false` with `status.status == STATUS_SUCCESS` indicates successful halt completion -- `SetPackage` is a client-streaming RPC — the only streaming call in our scope ## 9. Future Work From 0bf9f219b81a6b504e371b24d8791dad5ee04d1a Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Sat, 21 Mar 2026 04:09:19 +0000 Subject: [PATCH 4/9] design doc: address Copilot review comments - Add types_pb2.py to vendored stubs file list (types.proto dependency) - Fix RebootStatus enum access pattern for protobuf Python codegen - Fix test mock to implement grpc.Call interface (code()/details()) Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index 1c95ba7d..1622cba0 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -85,7 +85,9 @@ Vendor pre-generated Python stubs from the gNOI `system.proto` definition. host_modules/gnoi/ ├── __init__.py ├── system_pb2.py # generated message classes -└── system_pb2_grpc.py # generated service stubs +├── system_pb2_grpc.py # generated service stubs +├── types_pb2.py # generated types (dependency of system_pb2) +└── types_pb2_grpc.py # generated (empty, no services in types.proto) ``` The stubs are generated from: @@ -217,9 +219,10 @@ def _poll_reboot_status(self, dpu_name, dpu_ip, port): try: resp = client.reboot_status() if not resp.active: - status_str = system_pb2.RebootStatus.Status.Name(resp.status.status) + status_enum = resp.status.status + status_str = system_pb2.RebootStatus.Status.Name(status_enum) logger.log_notice(f"{dpu_name}: RebootStatus complete: {status_str} - {resp.status.message}") - return resp.status.status == system_pb2.RebootStatus.Status.STATUS_SUCCESS + return status_enum == system_pb2.RebootStatus.STATUS_SUCCESS except grpc.RpcError as e: logger.log_warning( f"{dpu_name}: RebootStatus poll error: code={e.code()} details={e.details()}" @@ -259,7 +262,10 @@ def test_send_reboot_command_success(self, MockClient): @mock.patch('gnoi_shutdown_daemon.GnoiClient') def test_send_reboot_command_failure(self, MockClient): mock_client = MockClient.return_value.__enter__.return_value - mock_client.reboot.side_effect = grpc.RpcError() + error = mock.create_autospec(grpc.RpcError) + error.code.return_value = grpc.StatusCode.UNAVAILABLE + error.details.return_value = "connection refused" + mock_client.reboot.side_effect = error result = handler._send_reboot_command("DPU0", "10.0.0.1", "8080") assert result is False From bf4c41cb2389ee01e60b10bd38fdf06c123c1c65 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Sat, 21 Mar 2026 04:58:58 +0000 Subject: [PATCH 5/9] Address Copilot review: fix stderr description, add packaging note - Correct stderr statement: _send_reboot_command uses suppress_stderr=True, so panic output is suppressed, not captured - Add packaging note: setup.py must include host_modules.gnoi in packages list for vendored stubs to be installed Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index 1622cba0..1c2b2f69 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -52,7 +52,7 @@ main.main() /sonic/gnoi_client/gnoi_client.go:42 ... ``` -The daemon captures this in `err` (stderr) but never logs or inspects it — it just logs `"Reboot command failed"` with no context. Diagnosing production failures requires SSHing into the switch, manually running the docker exec command, and reading Go stack traces. +For the Reboot call, `_send_reboot_command()` invokes `execute_command(..., suppress_stderr=True)`, so this panic output on stderr is suppressed rather than logged or inspected — the daemon just logs `"Reboot command failed"` with no actionable context. Diagnosing production failures requires SSHing into the switch, manually running the docker exec command, and reading Go stack traces. ## 2. Goal @@ -103,6 +103,8 @@ python -m grpc_tools.protoc \ system/system.proto types/types.proto ``` +**Packaging note:** `setup.py` currently lists packages explicitly (`['host_modules', 'utils']`). The implementation must add `'host_modules.gnoi'` to the `packages` list and corresponding `package_dir` entry, otherwise the vendored stubs won't be installed and imports will fail in deployed environments. + **Why vendor instead of build-time generation?** - sonic-host-services has no existing proto compilation infrastructure - The gNOI System proto is stable (no changes in years) From 72eb52a4888d284396dbf2637ea74368ba5206b3 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Sat, 21 Mar 2026 05:12:39 +0000 Subject: [PATCH 6/9] Replace inline reference code with links to source files Section 8 now links to gnoi_client, reboot.go, and system.proto instead of inlining full source code. Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 172 +-------------------------------- 1 file changed, 4 insertions(+), 168 deletions(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index 1c2b2f69..b5919de6 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -309,174 +309,10 @@ All phases can ship as a single PR since they form one atomic change — the old ## 8. Reference Code -This section provides the upstream source code that this design replaces and builds upon, so the document is self-contained. - -### 8.1 Current `gnoi_client` Entry Point - -**Source:** [`sonic-gnmi/gnoi_client/gnoi_client.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/gnoi_client.go) - -```go -package main - -import ( - "context" - "os" - "os/signal" - - "github.com/google/gnxi/utils/credentials" - "github.com/sonic-net/sonic-gnmi/gnoi_client/config" - "github.com/sonic-net/sonic-gnmi/gnoi_client/system" - "google.golang.org/grpc" -) - -func main() { - config.ParseFlag() - opts := credentials.ClientCredentials(*config.TargetName) - - ctx, cancel := context.WithCancel(context.Background()) - go func() { - c := make(chan os.Signal, 1) - signal.Notify(c, os.Interrupt) - <-c - cancel() - }() - conn, err := grpc.Dial(*config.Target, opts...) - if err != nil { - panic(err.Error()) - } - - switch *config.Module { - case "System": - switch *config.Rpc { - case "Reboot": - system.Reboot(conn, ctx) - case "RebootStatus": - system.RebootStatus(conn, ctx) - // ... other RPCs omitted for brevity - } - // ... other modules omitted - } -} -``` - -### 8.2 `gnoi_client/system/reboot.go` — Reboot & RebootStatus Implementation - -**Source:** [`sonic-gnmi/gnoi_client/system/reboot.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go) - -```go -package system - -import ( - "context" - "encoding/json" - "fmt" - pb "github.com/openconfig/gnoi/system" - "github.com/sonic-net/sonic-gnmi/gnoi_client/config" - "github.com/sonic-net/sonic-gnmi/gnoi_client/utils" - "google.golang.org/grpc" -) - -func Reboot(conn *grpc.ClientConn, ctx context.Context) { - fmt.Println("System Reboot") - ctx = utils.SetUserCreds(ctx) - sc := pb.NewSystemClient(conn) - req := &pb.RebootRequest{} - json.Unmarshal([]byte(*config.Args), req) - _, err := sc.Reboot(ctx, req) - if err != nil { - panic(err.Error()) // ← Error is lost in a Go panic stack trace - } -} - -func RebootStatus(conn *grpc.ClientConn, ctx context.Context) { - fmt.Println("System RebootStatus") - ctx = utils.SetUserCreds(ctx) - sc := pb.NewSystemClient(conn) - req := &pb.RebootStatusRequest{} - resp, err := sc.RebootStatus(ctx, req) - if err != nil { - panic(err.Error()) - } - respstr, err := json.Marshal(resp) - if err != nil { - panic(err.Error()) - } - fmt.Println(string(respstr)) // ← Output that daemon tries to parse -} -``` - -**Key observations:** -- `Reboot()` prints `"System Reboot\n"` on success, panics on failure — no structured output -- `RebootStatus()` prints JSON-serialized `RebootStatusResponse` — the daemon searches for `"reboot complete"` which never appears in this JSON -- Errors use `panic()` which produces Go stack traces instead of parseable error output - -### 8.3 OpenConfig gNOI System Proto Definition - -**Source:** [`openconfig/gnoi/system/system.proto`](https://github.com/openconfig/gnoi/blob/main/system/system.proto) (vendored at `sonic-gnmi/vendor/github.com/openconfig/gnoi/system/system.proto`) - -```protobuf -syntax = "proto3"; -package gnoi.system; - -service System { - rpc Reboot(RebootRequest) returns (RebootResponse) {} - rpc RebootStatus(RebootStatusRequest) returns (RebootStatusResponse) {} - rpc CancelReboot(CancelRebootRequest) returns (CancelRebootResponse) {} - rpc KillProcess(KillProcessRequest) returns (KillProcessResponse) {} - rpc Time(TimeRequest) returns (TimeResponse) {} - // ... Ping, Traceroute, SwitchControlProcessor, SetPackage omitted -} - -message RebootRequest { - RebootMethod method = 1; - uint64 delay = 2; // Delay in nanoseconds - string message = 3; // Informational reason - repeated types.Path subcomponents = 4; - bool force = 5; -} - -message RebootResponse {} - -enum RebootMethod { - UNKNOWN = 0; - COLD = 1; // Shutdown and restart OS and all hardware - POWERDOWN = 2; // Halt and power down - HALT = 3; // Halt (used for DPU shutdown) - WARM = 4; // Reload configuration only - NSF = 5; // Non-stop-forwarding reboot - POWERUP = 7; // Apply power -} - -message RebootStatusRequest { - repeated types.Path subcomponents = 1; -} - -message RebootStatusResponse { - bool active = 1; // If reboot is active - uint64 wait = 2; // Time left until reboot (ns) - uint64 when = 3; // Reboot time (ns since epoch) - string reason = 4; - uint32 count = 5; - RebootMethod method = 6; - RebootStatus status = 7; // Only meaningful when active = false -} - -message RebootStatus { - enum Status { - STATUS_UNKNOWN = 0; - STATUS_SUCCESS = 1; - STATUS_RETRIABLE_FAILURE = 2; - STATUS_FAILURE = 3; - } - Status status = 1; - string message = 2; -} - -``` - -**Key proto details for the Python wrapper:** -- `RebootMethod.HALT = 3` — the method used for DPU graceful shutdown -- `RebootStatusResponse.active == false` with `status.status == STATUS_SUCCESS` indicates successful halt completion +- **`gnoi_client` entry point:** [`sonic-gnmi/gnoi_client/gnoi_client.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/gnoi_client.go) — dispatches to per-module handlers; errors use `panic()` producing Go stack traces +- **Reboot/RebootStatus implementation:** [`sonic-gnmi/gnoi_client/system/reboot.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go) — `Reboot()` prints `"System Reboot\n"` on success; `RebootStatus()` prints JSON-serialized `RebootStatusResponse` (the output the daemon tries to parse with `"reboot complete"`) +- **gNOI System proto:** [`openconfig/gnoi/system/system.proto`](https://github.com/openconfig/gnoi/blob/main/system/system.proto) — defines `RebootMethod.HALT = 3`, `RebootStatusResponse.active`, and `RebootStatus.Status` enum (`STATUS_SUCCESS = 1`) +- **gNOI types proto (dependency):** [`openconfig/gnoi/types/types.proto`](https://github.com/openconfig/gnoi/blob/main/types/types.proto) ## 9. Future Work From f47c59609d47b9d97a409854b81f8a7a476ac8b5 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Mon, 23 Mar 2026 16:23:10 +0000 Subject: [PATCH 7/9] =?UTF-8?q?design=20doc:=20trim=20for=20human=20readab?= =?UTF-8?q?ility=20=E2=80=94=20lead=20with=20the=20bug,=20cut=20boilerplat?= =?UTF-8?q?e?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 302 +++++---------------------------- 1 file changed, 41 insertions(+), 261 deletions(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index b5919de6..0b132f91 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -1,218 +1,63 @@ # Design: Replace `docker exec gnoi_client` with Native gRPC Calls -## 1. Background +## TL;DR -The `gnoi_shutdown_daemon` on SmartSwitch NPU orchestrates graceful DPU shutdown by issuing gNOI `System.Reboot(HALT)` and polling `System.RebootStatus`. Today it does this by shelling out: +`gnoi_shutdown_daemon` polls DPU reboot status by checking for `"reboot complete"` in `gnoi_client` stdout — but that string never appears in the output. Every DPU shutdown poll **times out unconditionally**. The fix: replace the subprocess calls with direct Python gRPC, which also eliminates the gnmi container dependency and gives us real error messages. -``` -docker exec gnmi gnoi_client -target=: -notls -module System -rpc Reboot ... -docker exec gnmi gnoi_client -target=: -notls -module System -rpc RebootStatus -``` - -This has several problems: - -| Problem | Impact | -|---------|--------| -| Requires the `gnmi` container to be running and healthy | If gnmi container is restarting or unhealthy, DPU shutdown fails silently | -| Subprocess overhead per RPC call | Extra process creation, Docker CLI round-trip, stdout parsing | -| Fragile output parsing | `"reboot complete" in out_s.lower()` breaks on any output format change | -| No structured error handling | gRPC status codes are lost; only `rc != 0` is checked | -| Error output is a Go panic stack trace | Extremely painful to diagnose failures (see §1.1) | -| Security surface | Shell-out through Docker CLI is a wider attack surface than a direct socket | - -### 1.1 gnoi_client Output Format Analysis - -The `gnoi_client` binary in sonic-gnmi is a Go CLI tool. Understanding its output format reveals why the current approach is fragile: - -**Reboot RPC (`-rpc Reboot`):** -- On **success**: prints `"System Reboot\n"` to stdout, exits 0. No structured output. -- On **failure**: calls `panic(err.Error())`, which dumps a **Go panic stack trace** to stderr and exits with a non-zero code. The daemon only checks `rc != 0` — the actual gRPC error code, message, and details are buried in a multi-line panic dump that is not parsed. +## The Bug -**RebootStatus RPC (`-rpc RebootStatus`):** -- On **success**: prints `"System RebootStatus\n"` header followed by JSON-marshaled `RebootStatusResponse`, e.g.: - ```json - System RebootStatus - {"active":false,"status":{"status":"STATUS_SUCCESS","message":"..."}} - ``` -- On **failure**: same `panic(err.Error())` — Go stack trace, non-zero exit. +`_poll_reboot_status()` in `scripts/gnoi_shutdown_daemon.py`: -**The parsing bug:** The daemon currently checks: ```python if rc_s == 0 and out_s and ("reboot complete" in out_s.lower()): return True ``` -But the actual protobuf `RebootStatusResponse` serialized to JSON contains fields like `"active":false` and `"status":"STATUS_SUCCESS"` — the string `"reboot complete"` never appears in the output. This means the poll loop **always times out** regardless of whether the DPU successfully halted, and the daemon proceeds purely on the timeout path. -**Why this matters for error diagnosis:** When a gNOI RPC fails (DPU unreachable, TLS mismatch, auth failure, server-side error), the only signal is a Go panic: -``` -panic: rpc error: code = Unavailable desc = connection error: ... +Actual `gnoi_client -rpc RebootStatus` output: -goroutine 1 [running]: -main.main() - /sonic/gnoi_client/gnoi_client.go:42 -... ``` -For the Reboot call, `_send_reboot_command()` invokes `execute_command(..., suppress_stderr=True)`, so this panic output on stderr is suppressed rather than logged or inspected — the daemon just logs `"Reboot command failed"` with no actionable context. Diagnosing production failures requires SSHing into the switch, manually running the docker exec command, and reading Go stack traces. - -## 2. Goal - -Replace the subprocess-based `gnoi_client` invocations with direct Python gRPC calls using generated protobuf stubs for the [OpenConfig gNOI System service](https://github.com/openconfig/gnoi/blob/main/system/system.proto). - -## 3. Scope +System RebootStatus +{"active":false,"status":{"status":"STATUS_SUCCESS","message":"..."}} +``` -### In Scope -- Generate or vendor Python gRPC stubs for `gnoi.system.System` (Reboot, RebootStatus RPCs) -- Create a lightweight `GnoiClient` wrapper class -- Refactor `GnoiRebootHandler._send_reboot_command()` and `_poll_reboot_status()` to use native gRPC -- Remove `execute_command()` helper (becomes unused) -- Update unit tests to mock at the gRPC stub level -- Add `grpcio` and `protobuf` to package dependencies +The string `"reboot complete"` never appears. The poll always exhausts its timeout, then proceeds as if the DPU halted — whether it did or not. -### Out of Scope -- TLS/mTLS on the midplane channel (future work; midplane is trusted today) -- Refactoring the daemon's main loop or config DB subscription logic -- Other gNOI services beyond `System` -- Changes to how DPU IP/port are discovered from CONFIG_DB +A secondary problem: when the Reboot RPC fails, `gnoi_client` panics with a Go stack trace on stderr. The daemon calls `execute_command(..., suppress_stderr=True)`, so the error goes to `/dev/null`. The only log is `"Reboot command failed"` with zero context. -## 4. Design +## What Changes -### 4.1 Phase 1 — Proto Stubs +Replace `docker exec gnmi gnoi_client` subprocess calls with direct Python gRPC using vendored [gNOI System proto](https://github.com/openconfig/gnoi/blob/main/system/system.proto) stubs. -Vendor pre-generated Python stubs from the gNOI `system.proto` definition. +### New files -**Files to add:** ``` host_modules/gnoi/ ├── __init__.py -├── system_pb2.py # generated message classes -├── system_pb2_grpc.py # generated service stubs -├── types_pb2.py # generated types (dependency of system_pb2) -└── types_pb2_grpc.py # generated (empty, no services in types.proto) -``` - -The stubs are generated from: -- https://github.com/openconfig/gnoi/blob/main/system/system.proto -- https://github.com/openconfig/gnoi/blob/main/types/types.proto (dependency) - -Generation command (for reference / CI reproducibility): -```bash -python -m grpc_tools.protoc \ - -I./proto \ - --python_out=host_modules/gnoi \ - --grpc_python_out=host_modules/gnoi \ - system/system.proto types/types.proto +├── client.py # GnoiClient wrapper (reboot + reboot_status) +├── system_pb2.py # vendored proto stubs +├── system_pb2_grpc.py +├── types_pb2.py +└── types_pb2_grpc.py ``` -**Packaging note:** `setup.py` currently lists packages explicitly (`['host_modules', 'utils']`). The implementation must add `'host_modules.gnoi'` to the `packages` list and corresponding `package_dir` entry, otherwise the vendored stubs won't be installed and imports will fail in deployed environments. - -**Why vendor instead of build-time generation?** -- sonic-host-services has no existing proto compilation infrastructure -- The gNOI System proto is stable (no changes in years) -- Keeps the build simple; can migrate to build-time generation later if more protos are needed +### Modified files -### 4.2 Phase 2 — GnoiClient Wrapper - -A thin wrapper providing the two RPCs we need: +**`scripts/gnoi_shutdown_daemon.py`** — the two RPC call sites change: -```python -# host_modules/gnoi/client.py - -import grpc -from . import system_pb2, system_pb2_grpc - -class GnoiClient: - """Lightweight gNOI System service client for DPU communication.""" - - def __init__(self, target: str, timeout: int = 30): - """ - Args: - target: gRPC target in "host:port" format - timeout: Default RPC timeout in seconds - """ - self._channel = grpc.insecure_channel(target) - self._stub = system_pb2_grpc.SystemStub(self._channel) - self._timeout = timeout - - def reboot(self, method: int = 3, message: str = "") -> None: - """ - Send System.Reboot RPC. - - Args: - method: RebootMethod enum value (3 = HALT) - message: Human-readable reason string - - Raises: - grpc.RpcError: on any gRPC failure - """ - request = system_pb2.RebootRequest( - method=method, - message=message, - ) - self._stub.Reboot(request, timeout=self._timeout) - - def reboot_status(self) -> system_pb2.RebootStatusResponse: - """ - Poll System.RebootStatus RPC. - - Returns: - RebootStatusResponse with .active and .wait fields - - Raises: - grpc.RpcError: on any gRPC failure - """ - request = system_pb2.RebootStatusRequest() - return self._stub.RebootStatus(request, timeout=self._timeout) - - def close(self): - """Close the underlying gRPC channel.""" - if self._channel: - self._channel.close() - - def __enter__(self): - return self - - def __exit__(self, *args): - self.close() -``` - -### 4.3 Phase 3 — Refactor gnoi_shutdown_daemon - -Replace the two subprocess call sites in `GnoiRebootHandler`: - -#### `_send_reboot_command` (before) -```python -def _send_reboot_command(self, dpu_name, dpu_ip, port): - reboot_cmd = ["docker", "exec", "gnmi", "gnoi_client", ...] - rc, out, err = execute_command(reboot_cmd, ...) - return rc == 0 -``` - -#### `_send_reboot_command` (after) +`_send_reboot_command` becomes: ```python def _send_reboot_command(self, dpu_name, dpu_ip, port): try: with GnoiClient(f"{dpu_ip}:{port}", timeout=REBOOT_RPC_TIMEOUT_SEC) as client: - client.reboot( - method=REBOOT_METHOD_HALT, - message="Triggered by SmartSwitch graceful shutdown" - ) + client.reboot(method=REBOOT_METHOD_HALT, + message="Triggered by SmartSwitch graceful shutdown") return True except grpc.RpcError as e: logger.log_error(f"{dpu_name}: gNOI Reboot failed: {e.code()} {e.details()}") return False ``` -#### `_poll_reboot_status` (before) -```python -def _poll_reboot_status(self, dpu_name, dpu_ip, port): - status_cmd = ["docker", "exec", "gnmi", "gnoi_client", ...] - while time.monotonic() < deadline: - rc_s, out_s, _ = execute_command(status_cmd, ...) - if rc_s == 0 and "reboot complete" in out_s.lower(): - return True -``` - -#### `_poll_reboot_status` (after) +`_poll_reboot_status` becomes: ```python def _poll_reboot_status(self, dpu_name, dpu_ip, port): deadline = time.monotonic() + _get_halt_timeout() @@ -221,101 +66,36 @@ def _poll_reboot_status(self, dpu_name, dpu_ip, port): try: resp = client.reboot_status() if not resp.active: - status_enum = resp.status.status - status_str = system_pb2.RebootStatus.Status.Name(status_enum) - logger.log_notice(f"{dpu_name}: RebootStatus complete: {status_str} - {resp.status.message}") - return status_enum == system_pb2.RebootStatus.STATUS_SUCCESS + return resp.status.status == system_pb2.RebootStatus.STATUS_SUCCESS except grpc.RpcError as e: - logger.log_warning( - f"{dpu_name}: RebootStatus poll error: code={e.code()} details={e.details()}" - ) + logger.log_warning(f"{dpu_name}: RebootStatus poll error: {e.code()} {e.details()}") time.sleep(STATUS_POLL_INTERVAL_SEC) return False ``` -**Key improvements over the subprocess approach:** -- **Fixes the parsing bug**: checks `resp.active == False` directly instead of the broken `"reboot complete" in stdout` match that never triggers -- **Distinguishes success from failure**: inspects `resp.status.status` enum (`STATUS_SUCCESS` vs `STATUS_FAILURE` vs `STATUS_RETRIABLE_FAILURE`) -- **Actionable error logs**: gRPC errors include status code and details (e.g., `code=UNAVAILABLE details=connection refused`) instead of opaque "command failed" - -#### Removals -- `execute_command()` function — no longer needed -- `import subprocess` — no longer needed - -#### Additions -- `import grpc` -- `from host_modules.gnoi.client import GnoiClient` - -### 4.4 Phase 4 — Update Tests - -Current tests mock `execute_command` and check return codes. New tests mock at the gRPC level: - -```python -@mock.patch('gnoi_shutdown_daemon.GnoiClient') -def test_send_reboot_command_success(self, MockClient): - mock_client = MockClient.return_value.__enter__.return_value - # reboot() returns None on success - mock_client.reboot.return_value = None - - result = handler._send_reboot_command("DPU0", "10.0.0.1", "8080") - assert result is True - mock_client.reboot.assert_called_once() - -@mock.patch('gnoi_shutdown_daemon.GnoiClient') -def test_send_reboot_command_failure(self, MockClient): - mock_client = MockClient.return_value.__enter__.return_value - error = mock.create_autospec(grpc.RpcError) - error.code.return_value = grpc.StatusCode.UNAVAILABLE - error.details.return_value = "connection refused" - mock_client.reboot.side_effect = error - - result = handler._send_reboot_command("DPU0", "10.0.0.1", "8080") - assert result is False -``` - -### 4.5 Dependencies - -| Package | Version | Notes | -|---------|---------|-------| -| `grpcio` | >=1.51.0 | Already in SONiC build environment | -| `protobuf` | >=4.21.0 | Already in SONiC build environment | - -Verify these are available in the sonic-host-services build context. If not, add to `setup.py` `install_requires`. +`execute_command()` and `import subprocess` are removed. -## 5. Implementation Plan +**`setup.py`** — add `host_modules.gnoi` to packages list. -| Phase | Description | PR | -|-------|-------------|----| -| 1 | Vendor gNOI System proto stubs | PR #1 | -| 2 | Add `GnoiClient` wrapper + unit tests | PR #1 (same) | -| 3 | Refactor `gnoi_shutdown_daemon` to use `GnoiClient` | PR #1 (same) | -| 4 | Update existing daemon tests | PR #1 (same) | +**`tests/gnoi_shutdown_daemon_test.py`** — mocks move from `execute_command` to `GnoiClient`. -All phases can ship as a single PR since they form one atomic change — the old subprocess path is fully replaced. +### What stays the same -## 6. Testing +Main loop, CONFIG_DB subscription, DPU IP/port discovery, halt flag handling, threading model — all unchanged. -- **Unit tests**: Mock gRPC stubs, verify correct protobuf messages are sent, verify error handling for various `grpc.StatusCode` values -- **Integration test**: On a SmartSwitch testbed, trigger `config chassis modules shutdown DPU0` and verify gNOI HALT is sent and RebootStatus is polled successfully via syslog -- **Regression**: Existing CI pipeline covers the daemon; updated mocks ensure no regressions +## Why vendor stubs instead of build-time generation? -## 7. Risks & Mitigations +sonic-host-services has no proto compilation infra. The gNOI System proto hasn't changed in years. We can migrate to build-time generation later if more protos are needed. -| Risk | Mitigation | -|------|------------| -| gRPC/protobuf not available in host environment | Verify during build; these are already used by other SONiC components | -| Proto stub drift from upstream gnoi | Pin to a specific gnoi commit; stubs are stable | -| Insecure channel on midplane | Same trust model as today's `gnoi_client -notls`; TLS is future work | +## Risks -## 8. Reference Code +- **grpcio/protobuf availability**: both are already in the SONiC build environment. +- **Proto drift**: pin to a specific gnoi commit; the System service is stable. +- **Insecure channel**: same trust model as today's `-notls` flag on midplane. TLS is future work. -- **`gnoi_client` entry point:** [`sonic-gnmi/gnoi_client/gnoi_client.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/gnoi_client.go) — dispatches to per-module handlers; errors use `panic()` producing Go stack traces -- **Reboot/RebootStatus implementation:** [`sonic-gnmi/gnoi_client/system/reboot.go`](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go) — `Reboot()` prints `"System Reboot\n"` on success; `RebootStatus()` prints JSON-serialized `RebootStatusResponse` (the output the daemon tries to parse with `"reboot complete"`) -- **gNOI System proto:** [`openconfig/gnoi/system/system.proto`](https://github.com/openconfig/gnoi/blob/main/system/system.proto) — defines `RebootMethod.HALT = 3`, `RebootStatusResponse.active`, and `RebootStatus.Status` enum (`STATUS_SUCCESS = 1`) -- **gNOI types proto (dependency):** [`openconfig/gnoi/types/types.proto`](https://github.com/openconfig/gnoi/blob/main/types/types.proto) +## Appendix: gnoi_client output format -## 9. Future Work +For readers who want to verify the bug claim — here's what `gnoi_client` actually does ([source](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go)): -- **TLS support**: Add optional mTLS when midplane security is hardened -- **Build-time proto generation**: If more gNOI/gNMI services are needed, add a proto compilation step -- **Connection pooling**: Reuse gRPC channels across polls instead of creating per-call (minor optimization) +- **Reboot**: prints `"System Reboot\n"` on success, `panic(err.Error())` on failure (Go stack trace to stderr). +- **RebootStatus**: prints `"System RebootStatus\n"` + `json.Marshal(resp)` on success, same panic on failure. The JSON is protobuf-serialized `RebootStatusResponse` with fields `active`, `wait`, `status.status`, `status.message`. From 4940fab8f391b33ad07238a1565a4b503a726240 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Mon, 23 Mar 2026 16:33:39 +0000 Subject: [PATCH 8/9] design doc: reframe as footgun pattern, trim implementation details Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 117 +++++++++++++++------------------ 1 file changed, 53 insertions(+), 64 deletions(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index 0b132f91..6af3e2f2 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -2,100 +2,89 @@ ## TL;DR -`gnoi_shutdown_daemon` polls DPU reboot status by checking for `"reboot complete"` in `gnoi_client` stdout — but that string never appears in the output. Every DPU shutdown poll **times out unconditionally**. The fix: replace the subprocess calls with direct Python gRPC, which also eliminates the gnmi container dependency and gives us real error messages. +`gnoi_shutdown_daemon` issues gNOI RPCs by shelling out to `docker exec gnmi gnoi_client`. This is fragile, opaque, and already causing silent failures. Replace with direct Python gRPC calls using vendored proto stubs. -## The Bug +## 1. Why This Pattern Is Bad -`_poll_reboot_status()` in `scripts/gnoi_shutdown_daemon.py`: +| Problem | Impact | +|---------|--------| +| Requires `gnmi` container running and healthy | DPU shutdown silently fails if gnmi is restarting | +| Subprocess + Docker CLI overhead per RPC | Extra process creation, Docker round-trip, stdout capture | +| Output is unstructured text with a header line | Any format change in `gnoi_client` breaks parsing | +| gRPC status codes are lost | Caller only sees `rc != 0` — no code, no details | +| Errors are Go `panic()` stack traces on stderr | Production diagnosis requires SSH + manual docker exec | +| `suppress_stderr=True` discards those panics | Error output goes to `/dev/null`, logs say only "command failed" | +| String matching for completion detection | `"reboot complete" in out_s.lower()` doesn't match actual output (see §2) | +| Tight coupling to CLI flag interface | `-module System -rpc Reboot -jsonin '{...}'` is a serialization layer we don't need | +| Security surface | Shell-out through Docker CLI is wider than a direct gRPC socket | + +## 2. Already Broken: RebootStatus Parsing + +The poll loop checks: ```python if rc_s == 0 and out_s and ("reboot complete" in out_s.lower()): return True ``` -Actual `gnoi_client -rpc RebootStatus` output: +Actual `gnoi_client` output ([source](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go)): ``` System RebootStatus {"active":false,"status":{"status":"STATUS_SUCCESS","message":"..."}} ``` -The string `"reboot complete"` never appears. The poll always exhausts its timeout, then proceeds as if the DPU halted — whether it did or not. - -A secondary problem: when the Reboot RPC fails, `gnoi_client` panics with a Go stack trace on stderr. The daemon calls `execute_command(..., suppress_stderr=True)`, so the error goes to `/dev/null`. The only log is `"Reboot command failed"` with zero context. +`"reboot complete"` never appears → poll **always times out** regardless of DPU state. -## What Changes +## 3. Proposed Change -Replace `docker exec gnmi gnoi_client` subprocess calls with direct Python gRPC using vendored [gNOI System proto](https://github.com/openconfig/gnoi/blob/main/system/system.proto) stubs. - -### New files +Replace subprocess calls with a thin Python gRPC client using vendored [gNOI System proto](https://github.com/openconfig/gnoi/blob/main/system/system.proto) stubs. +**Before** (subprocess): ``` -host_modules/gnoi/ -├── __init__.py -├── client.py # GnoiClient wrapper (reboot + reboot_status) -├── system_pb2.py # vendored proto stubs -├── system_pb2_grpc.py -├── types_pb2.py -└── types_pb2_grpc.py +docker exec gnmi gnoi_client -target=: -notls -module System -rpc Reboot -jsonin '{"method":3}' ``` -### Modified files - -**`scripts/gnoi_shutdown_daemon.py`** — the two RPC call sites change: - -`_send_reboot_command` becomes: +**After** (direct gRPC): ```python -def _send_reboot_command(self, dpu_name, dpu_ip, port): - try: - with GnoiClient(f"{dpu_ip}:{port}", timeout=REBOOT_RPC_TIMEOUT_SEC) as client: - client.reboot(method=REBOOT_METHOD_HALT, - message="Triggered by SmartSwitch graceful shutdown") - return True - except grpc.RpcError as e: - logger.log_error(f"{dpu_name}: gNOI Reboot failed: {e.code()} {e.details()}") - return False +with GnoiClient(f"{dpu_ip}:{port}") as client: + client.reboot(method=REBOOT_METHOD_HALT, message="graceful shutdown") ``` -`_poll_reboot_status` becomes: +For RebootStatus, check the protobuf response directly instead of string matching: ```python -def _poll_reboot_status(self, dpu_name, dpu_ip, port): - deadline = time.monotonic() + _get_halt_timeout() - with GnoiClient(f"{dpu_ip}:{port}", timeout=STATUS_RPC_TIMEOUT_SEC) as client: - while time.monotonic() < deadline: - try: - resp = client.reboot_status() - if not resp.active: - return resp.status.status == system_pb2.RebootStatus.STATUS_SUCCESS - except grpc.RpcError as e: - logger.log_warning(f"{dpu_name}: RebootStatus poll error: {e.code()} {e.details()}") - time.sleep(STATUS_POLL_INTERVAL_SEC) - return False +resp = client.reboot_status() +if not resp.active and resp.status.status == STATUS_SUCCESS: + return True ``` -`execute_command()` and `import subprocess` are removed. - -**`setup.py`** — add `host_modules.gnoi` to packages list. - -**`tests/gnoi_shutdown_daemon_test.py`** — mocks move from `execute_command` to `GnoiClient`. - -### What stays the same - -Main loop, CONFIG_DB subscription, DPU IP/port discovery, halt flag handling, threading model — all unchanged. +### What this gives us +- **Structured errors**: `grpc.RpcError` with status code + details instead of opaque exit codes +- **Correct completion check**: inspect `resp.active` and `resp.status.status` directly +- **No container dependency**: gRPC goes straight to the DPU, gnmi container health is irrelevant +- **No parsing**: protobuf deserialization, not string matching on CLI output -## Why vendor stubs instead of build-time generation? +## 4. Scope -sonic-host-services has no proto compilation infra. The gNOI System proto hasn't changed in years. We can migrate to build-time generation later if more protos are needed. +### In scope +- Vendor Python gRPC stubs for `gnoi.system.System` (Reboot, RebootStatus) +- Lightweight `GnoiClient` wrapper +- Refactor the two RPC call sites in `GnoiRebootHandler` +- Update unit tests -## Risks +### Out of scope +- TLS/mTLS on midplane (future work; midplane is trusted today) +- Main loop, config DB subscription, halt flag handling — unchanged +- Other gNOI services beyond System -- **grpcio/protobuf availability**: both are already in the SONiC build environment. -- **Proto drift**: pin to a specific gnoi commit; the System service is stable. -- **Insecure channel**: same trust model as today's `-notls` flag on midplane. TLS is future work. +## 5. Why Vendor Stubs? -## Appendix: gnoi_client output format +sonic-host-services has no proto compilation infra. The gNOI System proto is stable (no changes in years). Vendoring keeps the build simple; can migrate to build-time generation later if more protos are needed. -For readers who want to verify the bug claim — here's what `gnoi_client` actually does ([source](https://github.com/sonic-net/sonic-gnmi/blob/master/gnoi_client/system/reboot.go)): +## 6. Risks -- **Reboot**: prints `"System Reboot\n"` on success, `panic(err.Error())` on failure (Go stack trace to stderr). -- **RebootStatus**: prints `"System RebootStatus\n"` + `json.Marshal(resp)` on success, same panic on failure. The JSON is protobuf-serialized `RebootStatusResponse` with fields `active`, `wait`, `status.status`, `status.message`. +| Risk | Mitigation | +|------|------------| +| grpcio/protobuf not in host environment | Already used by other SONiC components | +| Proto drift from upstream gnoi | Pin to a specific commit; System service is stable | +| Insecure channel on midplane | Same trust model as today's `-notls`; TLS is future work | From eb0cf402ca08dc81f5d240b30d4c70ccdc3c44e6 Mon Sep 17 00:00:00 2001 From: Dawei Huang Date: Mon, 23 Mar 2026 16:35:44 +0000 Subject: [PATCH 9/9] design doc: soften language, use neutral framing Signed-off-by: Dawei Huang --- doc/gnoi-native-grpc-design.md | 89 ++++++++++++++++++++++++++-------- 1 file changed, 70 insertions(+), 19 deletions(-) diff --git a/doc/gnoi-native-grpc-design.md b/doc/gnoi-native-grpc-design.md index 6af3e2f2..e534824a 100644 --- a/doc/gnoi-native-grpc-design.md +++ b/doc/gnoi-native-grpc-design.md @@ -2,23 +2,23 @@ ## TL;DR -`gnoi_shutdown_daemon` issues gNOI RPCs by shelling out to `docker exec gnmi gnoi_client`. This is fragile, opaque, and already causing silent failures. Replace with direct Python gRPC calls using vendored proto stubs. +`gnoi_shutdown_daemon` issues gNOI RPCs by shelling out to `docker exec gnmi gnoi_client`. This introduces several layers of indirection that make failures hard to diagnose and completion detection unreliable. This document proposes replacing the subprocess path with direct Python gRPC calls using vendored proto stubs. -## 1. Why This Pattern Is Bad +## 1. Limitations of the Current Approach -| Problem | Impact | -|---------|--------| -| Requires `gnmi` container running and healthy | DPU shutdown silently fails if gnmi is restarting | -| Subprocess + Docker CLI overhead per RPC | Extra process creation, Docker round-trip, stdout capture | -| Output is unstructured text with a header line | Any format change in `gnoi_client` breaks parsing | -| gRPC status codes are lost | Caller only sees `rc != 0` — no code, no details | -| Errors are Go `panic()` stack traces on stderr | Production diagnosis requires SSH + manual docker exec | -| `suppress_stderr=True` discards those panics | Error output goes to `/dev/null`, logs say only "command failed" | -| String matching for completion detection | `"reboot complete" in out_s.lower()` doesn't match actual output (see §2) | -| Tight coupling to CLI flag interface | `-module System -rpc Reboot -jsonin '{...}'` is a serialization layer we don't need | -| Security surface | Shell-out through Docker CLI is wider than a direct gRPC socket | +| Observation | Consequence | +|-------------|-------------| +| Requires NPU `gnmi` container running and healthy | DPU shutdown depends on an unrelated NPU container's availability, even though the RPC target is the DPU's own gnmi server | +| Subprocess + Docker CLI overhead per RPC | Extra process creation, Docker round-trip, stdout capture on each call | +| Output is unstructured text with a header line | Parsing is coupled to `gnoi_client`'s print format, which has no stability guarantee | +| gRPC status codes are not propagated | Caller only sees `rc != 0` — no status code, no error details | +| Errors surface as Go `panic()` stack traces on stderr | Diagnosing RPC failures requires SSH + manual docker exec | +| `suppress_stderr=True` on the Reboot call | Panic output is discarded; logs show only "command failed" | +| Completion check uses string matching | `"reboot complete" in out_s.lower()` does not match actual output format (see §2) | +| Tight coupling to CLI flag interface | `-module System -rpc Reboot -jsonin '{...}'` adds a serialization layer between caller and protobuf | +| Broader privilege surface | Shell-out through Docker CLI vs. a direct gRPC socket | -## 2. Already Broken: RebootStatus Parsing +## 2. Existing Issue: RebootStatus Completion Detection The poll loop checks: @@ -34,7 +34,7 @@ System RebootStatus {"active":false,"status":{"status":"STATUS_SUCCESS","message":"..."}} ``` -`"reboot complete"` never appears → poll **always times out** regardless of DPU state. +`"reboot complete"` does not appear in this output → the poll always exhausts its timeout regardless of DPU state. ## 3. Proposed Change @@ -59,10 +59,42 @@ if not resp.active and resp.status.status == STATUS_SUCCESS: ``` ### What this gives us -- **Structured errors**: `grpc.RpcError` with status code + details instead of opaque exit codes -- **Correct completion check**: inspect `resp.active` and `resp.status.status` directly -- **No container dependency**: gRPC goes straight to the DPU, gnmi container health is irrelevant -- **No parsing**: protobuf deserialization, not string matching on CLI output + +**Better error detection** — gRPC errors carry status codes and details natively: +```python +except grpc.RpcError as e: + logger.log_error(f"{dpu_name}: Reboot failed: {e.code()} {e.details()}") + # e.g. "UNAVAILABLE: connection refused" vs today's "command failed" +``` + +**Better testing** — mocks operate on typed protobuf objects instead of crafting subprocess stdout strings: +```python +# Today: mock must reproduce gnoi_client's exact text output +mock_execute.return_value = (0, "reboot complete", "") # this doesn't even match reality + +# After: mock returns a typed response +mock_client.reboot_status.return_value = RebootStatusResponse( + active=False, status=RebootStatus(status=STATUS_SUCCESS)) +``` + +**Correct completion check** — inspect `resp.active` and `resp.status.status` directly instead of string matching. + +**Removes unnecessary NPU gnmi container dependency** — the current approach shells into the NPU's `gnmi` container to run `gnoi_client`, but there's no reason the NPU daemon needs the NPU gnmi container as an intermediary. The DPU's own gnmi server is the actual RPC endpoint; direct gRPC connects to it without involving the NPU container. + +**Scales to future RPCs** — the same pattern extends to any gNOI or gNMI call without adding more subprocess wrappers: +```python +# Adding a new gNOI RPC is just another method on the client +class GnoiClient: + def reboot(self, ...): ... + def reboot_status(self, ...): ... + def cancel_reboot(self, ...): ... # future + def system_time(self, ...): ... # future + +# Or a gNMI client alongside it +with GnmiClient(f"{dpu_ip}:{port}") as client: + client.get(path="/system/state/...") +``` +Each new RPC is a typed method with protobuf request/response — no new shell commands, no new output formats to parse. ## 4. Scope @@ -72,6 +104,25 @@ if not resp.active and resp.status.status == STATUS_SUCCESS: - Refactor the two RPC call sites in `GnoiRebootHandler` - Update unit tests +New directory structure: +``` +host_modules/gnoi/ +├── __init__.py +├── client.py # GnoiClient: reboot(), reboot_status(), context manager +├── system_pb2.py # vendored from openconfig/gnoi system.proto +├── system_pb2_grpc.py +├── types_pb2.py # dependency of system.proto +└── types_pb2_grpc.py +``` + +The daemon change is essentially replacing `execute_command(["docker", "exec", ...])` with: +```python +with GnoiClient(f"{dpu_ip}:{port}") as client: + client.reboot(method=REBOOT_METHOD_HALT, ...) + # ... + resp = client.reboot_status() +``` + ### Out of scope - TLS/mTLS on midplane (future work; midplane is trusted today) - Main loop, config DB subscription, halt flag handling — unchanged