Skip to content

Commit a9f8d44

Browse files
committed
design doc: soften language, use neutral framing
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
1 parent 4940fab commit a9f8d44

1 file changed

Lines changed: 15 additions & 15 deletions

File tree

doc/gnoi-native-grpc-design.md

Lines changed: 15 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,23 @@
22

33
## TL;DR
44

5-
`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.
5+
`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.
66

7-
## 1. Why This Pattern Is Bad
7+
## 1. Limitations of the Current Approach
88

9-
| Problem | Impact |
10-
|---------|--------|
11-
| Requires `gnmi` container running and healthy | DPU shutdown silently fails if gnmi is restarting |
12-
| Subprocess + Docker CLI overhead per RPC | Extra process creation, Docker round-trip, stdout capture |
13-
| Output is unstructured text with a header line | Any format change in `gnoi_client` breaks parsing |
14-
| gRPC status codes are lost | Caller only sees `rc != 0` — no code, no details |
15-
| Errors are Go `panic()` stack traces on stderr | Production diagnosis requires SSH + manual docker exec |
16-
| `suppress_stderr=True` discards those panics | Error output goes to `/dev/null`, logs say only "command failed" |
17-
| String matching for completion detection | `"reboot complete" in out_s.lower()` doesn't match actual output (see §2) |
18-
| Tight coupling to CLI flag interface | `-module System -rpc Reboot -jsonin '{...}'` is a serialization layer we don't need |
19-
| Security surface | Shell-out through Docker CLI is wider than a direct gRPC socket |
9+
| Observation | Consequence |
10+
|-------------|-------------|
11+
| Requires `gnmi` container running and healthy | DPU shutdown depends on an unrelated container's availability |
12+
| Subprocess + Docker CLI overhead per RPC | Extra process creation, Docker round-trip, stdout capture on each call |
13+
| Output is unstructured text with a header line | Parsing is coupled to `gnoi_client`'s print format, which has no stability guarantee |
14+
| gRPC status codes are not propagated | Caller only sees `rc != 0` — no status code, no error details |
15+
| Errors surface as Go `panic()` stack traces on stderr | Diagnosing RPC failures requires SSH + manual docker exec |
16+
| `suppress_stderr=True` on the Reboot call | Panic output is discarded; logs show only "command failed" |
17+
| Completion check uses string matching | `"reboot complete" in out_s.lower()` does not match actual output format (see §2) |
18+
| Tight coupling to CLI flag interface | `-module System -rpc Reboot -jsonin '{...}'` adds a serialization layer between caller and protobuf |
19+
| Broader privilege surface | Shell-out through Docker CLI vs. a direct gRPC socket |
2020

21-
## 2. Already Broken: RebootStatus Parsing
21+
## 2. Existing Issue: RebootStatus Completion Detection
2222

2323
The poll loop checks:
2424

@@ -34,7 +34,7 @@ System RebootStatus
3434
{"active":false,"status":{"status":"STATUS_SUCCESS","message":"..."}}
3535
```
3636

37-
`"reboot complete"` never appears → poll **always times out** regardless of DPU state.
37+
`"reboot complete"` does not appear in this output → the poll always exhausts its timeout regardless of DPU state.
3838

3939
## 3. Proposed Change
4040

0 commit comments

Comments
 (0)