[sonic-py-common] Add gRPC framework with gNOI client#27760
Open
hdwhdw wants to merge 16 commits into
Open
Conversation
Add sonic_py_common.grpc — a reusable gRPC framework for SONiC Python
components, starting with gNOI support.
Structure:
sonic_py_common/
grpc/ # gRPC framework root
__init__.py # future: gnmi, gribi sub-packages
gnoi/ # gNOI services
__init__.py
client.py # GnoiClient - channel manager + service stubs
system_pb2.py # vendored proto stubs (openconfig/gnoi)
system_pb2_grpc.py
types_pb2.py
types_pb2_grpc.py
common_pb2.py
common_pb2_grpc.py
GnoiClient is service-agnostic: stubs accessed via properties
(client.system, with scaffolding for healthz/cert/file/os).
The grpc/ namespace leaves room for gnmi/ and gribi/ sub-packages.
Dependencies: grpcio, protobuf (already used by other SONiC components).
Signed-off-by: sigabrtv1-ui <sig.abrt.v1@gmail.com>
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Add sonic_py_common.grpc.gnoi.testing with:
- FakeSystemStub: controllable System service stub with injectable
responses, side_effects (including sequences for polling scenarios),
and call history for assertions
- FakeGnoiClient: drop-in replacement for GnoiClient with no real
gRPC connections, same context manager protocol
Consumers can write tests like:
fake = FakeGnoiClient()
fake.system.set_reboot_status(active=False, status=STATUS_SUCCESS)
with patch('my_module.GnoiClient', return_value=fake):
assert my_reboot_function() == True
assert len(fake.system.reboot_calls) == 1
12 new tests for the fake itself.
Signed-off-by: sigabrtv1-ui <sig.abrt.v1@gmail.com>
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Replace mock-based testing with a real gRPC fake server: - FakeGnoiServer: starts a real gRPC server on localhost with controllable service implementations. Tests use the real GnoiClient connecting over a real channel — no mocking of gRPC internals. - FakeSystemServicer: configurable System service with: - set_reboot_response() / set_reboot_status() for single responses - set_reboot_status_sequence() for polling scenarios (active→done) - Error injection via grpc.StatusCode - Call history for assertions (reboot_calls, etc.) - reset() to clear state between tests - Rewrote all client tests to use FakeGnoiServer (no sys.modules mocking) 19 tests, all using real gRPC. Zero mocks. Signed-off-by: sigabrtv1-ui <sig.abrt.v1@gmail.com> Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
The generated protobuf stubs were created with protobuf 6.31.1 which uses 'runtime_version' import not available in the build image's protobuf 4.25.9. Regenerated using grpcio-tools 1.60.1 + protobuf 4.25.9 for compatibility with the SONiC build environment. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new sonic_py_common.grpc.gnoi package to provide a reusable gNOI gRPC client framework (plus vendored proto stubs) for SONiC Python components, along with a local fake gRPC server to enable integration-style unit tests without mocking.
Changes:
- Added
GnoiClient(context-managed gRPC channel + gNOI System service stub access). - Vendored generated gNOI proto/gRPC stubs (
*_pb2.py,*_pb2_grpc.py) for System/Common/Types. - Added
FakeGnoiServer/FakeSystemServicerand new tests exercising real gRPC server/client flows; updatedsetup.pyto include dependencies and packages.
Reviewed changes
Copilot reviewed 10 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sonic-py-common/tests/test_gnoi_testing.py | Adds integration-style tests for the fake server + servicer behavior. |
| src/sonic-py-common/tests/test_gnoi_client.py | Adds tests for GnoiClient channel lifecycle and basic RPC flows. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/types_pb2.py | Vendored generated protobuf stubs for gNOI types. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/types_pb2_grpc.py | Vendored generated gRPC module placeholder for types (no services). |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/testing.py | Adds FakeGnoiServer + configurable FakeSystemServicer for tests. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/system_pb2.py | Vendored generated protobuf stubs for gNOI System. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/system_pb2_grpc.py | Vendored generated gRPC stubs/servicer base for gNOI System. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/common_pb2.py | Vendored generated protobuf stubs for gNOI Common. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/common_pb2_grpc.py | Vendored generated gRPC module placeholder for common (no services). |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/client.py | Implements GnoiClient and exposes system stub property. |
| src/sonic-py-common/sonic_py_common/grpc/gnoi/init.py | Defines package exports for the gNOI framework. |
| src/sonic-py-common/sonic_py_common/grpc/init.py | Introduces sonic_py_common.grpc namespace for future gRPC clients. |
| src/sonic-py-common/setup.py | Adds grpcio/protobuf dependencies and includes new packages. |
Files not reviewed (3)
- src/sonic-py-common/sonic_py_common/grpc/gnoi/common_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/system_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/types_pb2.py: Language not supported
Comment on lines
+62
to
+69
| @property | ||
| def system(self): | ||
| """gNOI System service stub (gnoi.system.System). | ||
|
|
||
| Provides: Reboot, RebootStatus, CancelReboot, Time, Ping, | ||
| Traceroute, SwitchControlProcessor, etc. | ||
| """ | ||
| return system_pb2_grpc.SystemStub(self._channel) |
Comment on lines
+181
to
+184
| @property | ||
| def target(self): | ||
| """gRPC target string, e.g. 'localhost:50051'.""" | ||
| return f"localhost:{self._port}" |
Comment on lines
+39
to
+42
| install_requires=dependencies + [ | ||
| 'grpcio', | ||
| 'protobuf', | ||
| ], |
Comment on lines
43
to
47
| packages=[ | ||
| 'sonic_py_common', | ||
| 'sonic_py_common.grpc', | ||
| 'sonic_py_common.grpc.gnoi', | ||
| ], |
Comment on lines
+15
to
+18
| # Configure responses | ||
| server.system.set_reboot_response() # default success | ||
| server.system.set_reboot_status(active=False, status=STATUS_SUCCESS) | ||
|
|
Move grpcio and protobuf from install_requires to extras_require['gnoi']. Every SONiC container imports sonic_py_common, so making grpcio a hard runtime dependency would silently add roughly 30 MB of native code to each container image, including ones that will never speak gNOI (database, snmp, lldp, dhcp_relay, etc.). Containers that need the gNOI client opt in by either: pip install sonic-py-common[gnoi] or by adding the underlying packages to their own Dockerfile, e.g. 'pip3 install grpcio protobuf' or apt-installing python3-grpcio / python3-protobuf. The Python sources for sonic_py_common.grpc.gnoi still ship in the wheel; only the gRPC runtime is decoupled, so 'import grpc' will surface a clear ImportError on containers that forgot to opt in. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…ire" This reverts commit ed4a89c. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 7 comments.
Files not reviewed (3)
- src/sonic-py-common/sonic_py_common/grpc/gnoi/common_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/system_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/types_pb2.py: Language not supported
| import unittest | ||
| import grpc | ||
|
|
||
| from sonic_py_common.grpc.gnoi.testing import FakeGnoiServer, FakeSystemServicer |
Comment on lines
+135
to
+144
| def RebootStatus(self, request, context): | ||
| self.reboot_status_calls.append(request) | ||
| if self._reboot_status_responses: | ||
| item = self._reboot_status_responses.pop(0) | ||
| if isinstance(item, tuple): | ||
| context.abort(item[0], item[1]) | ||
| return item | ||
| if self._reboot_status_error: | ||
| context.abort(self._reboot_status_error[0], self._reboot_status_error[1]) | ||
| return self._reboot_status_response |
Comment on lines
+194
to
+199
| def stop(self, grace=0): | ||
| """Stop the server.""" | ||
| if self._server: | ||
| self._server.stop(grace) | ||
| self._server = None | ||
|
|
Comment on lines
+63
to
+69
| def system(self): | ||
| """gNOI System service stub (gnoi.system.System). | ||
|
|
||
| Provides: Reboot, RebootStatus, CancelReboot, Time, Ping, | ||
| Traceroute, SwitchControlProcessor, etc. | ||
| """ | ||
| return system_pb2_grpc.SystemStub(self._channel) |
Comment on lines
+39
to
+42
| install_requires=dependencies + [ | ||
| 'grpcio', | ||
| 'protobuf', | ||
| ], |
Comment on lines
+1
to
+5
| # -*- coding: utf-8 -*- | ||
| # Generated by the protocol buffer compiler. DO NOT EDIT! | ||
| # source: github.com/openconfig/gnoi/types/types.proto | ||
| # Protobuf Python Version: 4.25.0 | ||
| """Generated protocol buffer code.""" |
Comment on lines
+44
to
+46
| 'sonic_py_common', | ||
| 'sonic_py_common.grpc', | ||
| 'sonic_py_common.grpc.gnoi', |
GnoiClient previously hard-coded grpc.insecure_channel(), so it could
only reach plaintext targets (FakeGnoiServer, localhost helpers). The
SONiC telemetry/gNOI server requires mTLS, which the framework could
not address without callers reaching past the wrapper into raw grpc.
Add an optional credentials parameter on GnoiClient.__init__. When
None (default), keep the existing insecure_channel behavior — fully
backward compatible. When a grpc.ChannelCredentials is supplied, open
a secure_channel instead. Callers build credentials with the standard
grpc.ssl_channel_credentials(...) helper.
Verified end-to-end against the running telemetry/gNOI server on
vlab-01 from inside the gnmi container, calling gnoi.system.System/Time
over mTLS:
creds = grpc.ssl_channel_credentials(
root_certificates=open(server_cert).read(),
private_key=open(client_key).read(),
certificate_chain=open(client_cert).read(),
)
options = (("grpc.ssl_target_name_override", server_cn),)
with GnoiClient(target, credentials=creds, options=options) as c:
resp = c.system.Time(system_pb2.TimeRequest(), timeout=5)
Adds two tests covering the dispatch:
- credentials=None -> grpc.insecure_channel
- credentials=<creds> -> grpc.secure_channel(target, creds, ...)
All 21 tests in test_gnoi_client.py + test_gnoi_testing.py pass.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
gRPC's insecure_channel already accepts the unix:// target scheme, so
GnoiClient supports Unix-domain-socket targets out of the box — no
TLS, no extra code path. This is the natural transport for talking to
the local gNMI/gNOI server's /var/run/gnmi/gnmi.sock from a sibling
container that has the socket bind-mounted.
Verified end-to-end on vlab-01: gnoi.system.System/Time over
unix:///var/run/gnmi/gnmi.sock from inside the gnmi container against
the same container's gNMI server, via:
with GnoiClient("unix:///var/run/gnmi/gnmi.sock") as c:
resp = c.system.Time(system_pb2.TimeRequest(), timeout=5)
Updates the module/class docstrings to document this third transport
alongside insecure TCP and mTLS, and adds a unit test pinning the
unix:// pass-through so a future refactor doesn't silently break it.
23 of 23 tests pass.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 13 changed files in this pull request and generated 6 comments.
Files not reviewed (3)
- src/sonic-py-common/sonic_py_common/grpc/gnoi/common_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/system_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/types_pb2.py: Language not supported
Comment on lines
39
to
47
| install_requires=dependencies + [ | ||
| 'grpcio', | ||
| 'protobuf', | ||
| ], | ||
| packages=[ | ||
| 'sonic_py_common', | ||
| 'sonic_py_common.grpc', | ||
| 'sonic_py_common.grpc.gnoi', | ||
| ], |
Comment on lines
+44
to
+46
| 'sonic_py_common', | ||
| 'sonic_py_common.grpc', | ||
| 'sonic_py_common.grpc.gnoi', |
Comment on lines
+108
to
+115
| @property | ||
| def system(self): | ||
| """gNOI System service stub (gnoi.system.System). | ||
|
|
||
| Provides: Reboot, RebootStatus, CancelReboot, Time, Ping, | ||
| Traceroute, SwitchControlProcessor, etc. | ||
| """ | ||
| return system_pb2_grpc.SystemStub(self._channel) |
Comment on lines
+186
to
+191
| def start(self): | ||
| """Start the fake gRPC server on a random port.""" | ||
| self._server = grpc.server(futures.ThreadPoolExecutor(max_workers=self._max_workers)) | ||
| system_pb2_grpc.add_SystemServicer_to_server(self.system, self._server) | ||
| self._port = self._server.add_insecure_port("localhost:0") | ||
| self._server.start() |
Comment on lines
+194
to
+199
| def stop(self, grace=0): | ||
| """Stop the server.""" | ||
| if self._server: | ||
| self._server.stop(grace) | ||
| self._server = None | ||
|
|
Comment on lines
+99
to
+105
| if active is not None: | ||
| self._reboot_status_response.active = active | ||
| if status is not None: | ||
| self._reboot_status_response.status.status = status | ||
| if message: | ||
| self._reboot_status_response.status.message = message | ||
| self._reboot_status_error = (error_code, error_message) if error_code else None |
Bind-mount /var/run/gnmi into every runtime container so any process
inside any container can reach the local gNMI/gNOI server through its
Unix domain socket without TLS — the natural transport for in-box
control-plane RPCs.
The gnmi container (which owns the server and creates the socket)
already has it as :rw via rules/docker-gnmi.mk. Mount it :ro
everywhere else: client containers only need to connect(2) to the
existing socket; they have no business creating, deleting, or
replacing files in the directory.
This pairs with the new sonic_py_common.grpc.gnoi.GnoiClient — once a
container has both the wheel (via SONIC_PY_COMMON_PY3 propagation
through docker-config-engine-trixie) and this mount, the in-process
gNOI call is two lines:
with GnoiClient("unix:///var/run/gnmi/gnmi.sock") as c:
c.system.Time(system_pb2.TimeRequest(), timeout=5)
Verified end-to-end on vlab-01 from the gnmi container (which already
has the mount in stock images) calling gnoi.system.System/Time over
the UDS — see the matching unit test
test_unix_socket_target_uses_insecure_channel.
Affected runtime containers (29):
bmp, bmp-watchdog, dash-ha, database, eventd, fpm-frr,
gnmi-sidecar, gnmi-watchdog, iccpd, lldp, mux, nat, orchagent,
otel, p4rt, platform-monitor, restapi, restapi-sidecar,
restapi-watchdog, router-advertiser, sflow, snmp,
sonic-mgmt-framework, stp, sysmgr, teamd, telemetry,
telemetry-sidecar, telemetry-watchdog.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
…ors, fix edge cases Address Copilot reviewer findings on PR sonic-net#27760: * setup.py: gate grpcio/protobuf install_requires AND the sonic_py_common.grpc{,.gnoi} package entries on Python 3. Both grpcio and protobuf dropped Py2 wheels long ago, and the new code (testing.py, client.py) uses f-strings and other Py3-only syntax; without gating, ENABLE_PY2_MODULES=y builds would break trying to package code that won't import on Py2 anyway. * client.py: add _require_channel() guard so accessing a service stub before __enter__() (or after close()) raises an explicit RuntimeError pointing at the misuse, instead of an opaque AttributeError from inside the generated stub. * testing.py: - FakeGnoiServer.target now raises RuntimeError if accessed before start() — previously returned 'localhost:None' which surfaced as a confusing connection error. - FakeGnoiServer.start() checks add_insecure_port()'s return value and raises on bind failure (0 = failure per the gRPC contract). - FakeGnoiServer.stop() waits on the threading.Event returned by grpc.Server.stop() with a 5s cap, then clears _port. Prevents leaking server threads / FDs across many start/stop iterations and ensures .target stops pointing at a defunct server. - FakeSystemServicer.RebootStatus distinguishes 'sequence not configured' from 'sequence exhausted' so the documented fall-back-to-single-response semantics hold after exhaustion. - FakeSystemServicer.set_reboot_status: use 'message is not None' so callers can intentionally clear the status message to ''. - Fix docstring example: STATUS_SUCCESS is a nested enum value, not a bare name — use system_pb2.RebootStatus.Status.STATUS_SUCCESS. * tests: - Drop unused FakeSystemServicer import from test_gnoi_testing.py. - Add test_service_stub_before_open_raises + test_service_stub_after_close_raises in test_gnoi_client.py. - Add test_target_before_start_raises, test_target_after_stop_raises, test_stop_waits_for_termination, sequence-exhaustion-fallback test, and empty-message-clearing test in test_gnoi_testing.py. 29 of 29 unit tests pass. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
…quirements
The vendored *_pb2.py files in sonic_py_common/grpc/gnoi were
generated with protoc-gen 4.25.0 (visible in the 'Protobuf Python
Version' header at the top of each file). The runtime-version check
embedded in those stubs by protobuf 4.21+ refuses to import under
protobuf < 4.21, surfacing as a descriptor-pool mismatch on first
import.
Modern SONiC base images already satisfy this:
- bookworm: Debian python3-protobuf 3.21.12 (still too old, will
fail import)
- trixie: Debian python3-protobuf >= 5.x (works)
By pinning protobuf>=4.21 in install_requires, the metadata contract
matches reality: pip-driven installs error out fast on bullseye
images (rules/protobuf.mk builds protobuf 3.21.12 locally), and any
ambiguity for downstream consumers is gone. On the trixie target
where the FR cares about (where docker-config-engine-trixie installs
SONIC_PY_COMMON_PY3 into every container), the constraint is
trivially satisfied.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Comment on lines
+83
to
+85
| def set_reboot_status(self, active=None, status=None, message="", | ||
| response=None, error_code=None, error_message=""): | ||
| """Set RebootStatus single response. |
Comment on lines
+6
to
+9
| import unittest | ||
| from unittest import mock | ||
| import grpc | ||
|
|
Comment on lines
+6
to
+8
| import unittest | ||
| import grpc | ||
|
|
Comment on lines
62
to
69
| setup_requires= [ | ||
| 'pytest-runner', | ||
| 'wheel' | ||
| ], | ||
| tests_require=[ | ||
| 'pytest', | ||
| 'mock==3.0.5' # For python 2. Version >=4.0.0 drops support for py2 | ||
| ], |
…ing extra, message default
* tests/test_gnoi_{client,testing}.py: add module-level
`raise unittest.SkipTest` guarded on `sys.version_info[0] < 3`, so
ENABLE_PY2_MODULES=y wheel builds skip the file under Py2 (where
`import grpc` and `import unittest.mock` would fail anyway). Both
unittest discovery (used by `python setup.py test` on the legacy
bullseye path) and pytest collection (bookworm/trixie path) honor
`raise SkipTest` at module level.
* setup.py: define `extras_require['testing']` so the bookworm/trixie
wheel build's `pip install ".[testing]"` step (slave.mk:1101)
doesn't fail with "package has no extra 'testing'".
* sonic_py_common/grpc/gnoi/testing.py: switch
`FakeSystemServicer.set_reboot_status` default from `message=""`
to `message=None`. With the old default, calls like
`set_reboot_status(active=True)` silently cleared any previously
configured status message; with the explicit-None default, the
message is only changed when the caller passes one, and
`message=""` remains a meaningful intentional clear (still covered
by the existing
test_set_reboot_status_message_can_be_cleared_to_empty).
* tests/test_gnoi_testing.py: add
test_set_reboot_status_message_default_does_not_clobber to pin the
new no-clobber contract.
30 of 30 tests pass.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Comment on lines
+95
to
+99
| def close(self): | ||
| """Close the gRPC channel.""" | ||
| if self._channel: | ||
| self._channel.close() | ||
| self._channel = None |
Comment on lines
+197
to
+200
| def start(self): | ||
| """Start the fake gRPC server on a random port.""" | ||
| self._server = grpc.server(futures.ThreadPoolExecutor(max_workers=self._max_workers)) | ||
| system_pb2_grpc.add_SystemServicer_to_server(self.system, self._server) |
Comment on lines
+219
to
+226
| if self._server: | ||
| stopped = self._server.stop(grace) | ||
| # Bounded wait: grace=0 means "abort in-flight RPCs immediately", | ||
| # so the Event fires almost instantly. Cap the wait so a broken | ||
| # gRPC build can't hang a test run indefinitely. | ||
| stopped.wait(timeout=5) | ||
| self._server = None | ||
| self._port = None |
Comment on lines
+34
to
+35
| # For gNMI/gNOI Unix Domain Socket (local access without TLS) | ||
| $(DOCKER_TELEMETRY)_RUN_OPT += -v /var/run/gnmi:/var/run/gnmi:ro |
…, double-start guard, surface stop() timeout
* client.py: `close()` now checks `self._channel is not None` rather
than relying on truthiness. A future channel-like object with a
custom `__bool__`/`__len__` could otherwise look falsy and silently
skip the `.close()` call, leaking the underlying connection.
* testing.py:
- `FakeGnoiServer.start()` now raises if called on an already-started
server instead of silently overwriting `_server` and `_port` and
leaking the first server's thread + listening socket.
- `FakeGnoiServer.stop()` now checks the return value of
`Event.wait(timeout=5)`. If the gRPC server failed to terminate in
the budget, raise instead of silently clearing `_server`/`_port`,
so callers (typically tests) fail loudly rather than appearing to
succeed while leaking resources.
* tests/test_gnoi_testing.py: add `test_double_start_raises` to pin
the new double-start guard.
31 of 31 tests pass.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Previously this PR added 27 nearly-identical `_RUN_OPT += -v
/var/run/gnmi:/var/run/gnmi:ro` lines, one per `rules/docker-*.mk`.
That works but has two problems:
1. Every new SONiC container added in the future would need a fresh
edit here to gain access to the local gNOI server. Easy to forget.
2. The intent (every container is a potential gNOI client) is spread
across 27 files instead of being stated once.
`files/build_templates/docker_image_ctl.j2` is the central template
that emits the `docker create` invocation for every container exactly
once. Adding the bind-mount there covers all current containers and
all future ones automatically.
The gnmi container itself owns the server and must mount the directory
:rw (it creates the socket); rules/docker-gnmi.mk keeps that line.
This template skips gnmi to avoid a duplicate-mount error from docker.
Revert the 27 per-rule edits; add a single conditional block in the
template. Net: 54 lines removed, 8 added.
Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 14 changed files in this pull request and generated 4 comments.
Files not reviewed (3)
- src/sonic-py-common/sonic_py_common/grpc/gnoi/common_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/system_pb2.py: Language not supported
- src/sonic-py-common/sonic_py_common/grpc/gnoi/types_pb2.py: Language not supported
Comment on lines
+80
to
+89
| def __enter__(self): | ||
| if self._credentials is None: | ||
| self._channel = grpc.insecure_channel( | ||
| self._target, options=self._options | ||
| ) | ||
| else: | ||
| self._channel = grpc.secure_channel( | ||
| self._target, self._credentials, options=self._options | ||
| ) | ||
| return self |
Comment on lines
+177
to
+181
| def __init__(self, max_workers=2): | ||
| self._max_workers = max_workers | ||
| self._server = None | ||
| self._port = None | ||
| self.system = FakeSystemServicer() |
Comment on lines
+210
to
+221
| self._server = grpc.server(futures.ThreadPoolExecutor(max_workers=self._max_workers)) | ||
| system_pb2_grpc.add_SystemServicer_to_server(self.system, self._server) | ||
| port = self._server.add_insecure_port("localhost:0") | ||
| if port == 0: | ||
| # add_insecure_port returns 0 on bind failure. | ||
| self._server = None | ||
| raise RuntimeError( | ||
| "FakeGnoiServer.start(): add_insecure_port('localhost:0') failed" | ||
| ) | ||
| self._port = port | ||
| self._server.start() | ||
| return self |
Comment on lines
+234
to
+248
| if self._server: | ||
| stopped = self._server.stop(grace) | ||
| # Bounded wait: grace=0 means "abort in-flight RPCs immediately", | ||
| # so the Event fires almost instantly. Cap the wait so a broken | ||
| # gRPC build can't hang a test run indefinitely. | ||
| if not stopped.wait(timeout=5): | ||
| # Don't silently clear state — the server is leaking. Surface | ||
| # it so the caller (typically a test) can fail loudly. | ||
| raise RuntimeError( | ||
| "FakeGnoiServer.stop(): gRPC server did not terminate " | ||
| "within 5s; the server is leaking threads / sockets." | ||
| ) | ||
| self._server = None | ||
| self._port = None | ||
|
|
Previous revision mounted /var/run/gnmi into every container except gnmi. That's a wide attack surface: any compromised container (snmp, restapi, bgp, dhcp_relay, ...) could drive the full gNOI surface (Reboot, SetPackage, File.Put, OS.Install) on the local server. Switch to an explicit allow-list. Start with the three in-box infrastructure containers that are the natural first consumers of an in-box gNOI client: swss, syncd, pmon. They're internal (no network endpoint of their own) so the threat model is roughly "a local SAI / swss / sensor bug becomes a privileged in-box RPC". That's a real risk but bounded; additional containers should be added by explicit request, not by default. The gnmi container itself continues to get the directory :rw via rules/docker-gnmi.mk (it owns the server and creates the socket). Trim the template comment too. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
Collaborator
|
/azp run Azure.sonic-buildimage |
|
Azure Pipelines successfully started running 1 pipeline(s). |
hdwhdw
added a commit
to hdwhdw/sonic-buildimage
that referenced
this pull request
Jul 10, 2026
The SONiC bookworm/trixie wheel build rule runs `pytest` on each wheel's source unless <pkg>_TEST=n. sonic-grpc had no tests, so pytest collected 0 items and the wheel target failed. Add the gNOI client and FakeGnoiServer unit tests (31 tests, lifted from the framework in sonic-net#27760) under src/sonic-grpc/tests/, and declare a `testing` extra so the build's `pip install ".[testing]"` step is satisfied. Verified: `target/python-wheels/bookworm/sonic_grpc-1.0-py3-none-any.whl` now builds with `31 passed` in the test step. Signed-off-by: Dawei Huang <daweihuang@microsoft.com>
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
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.
What this PR adds
A reusable Python gNOI client framework for in-box SONiC components, plus the plumbing to make it usable from any container.
1.
sonic_py_common.grpc.gnoi— gNOI client frameworkGnoiClient— a thin context-managed wrapper around a gRPC channel that exposes gNOI service stubs as properties (client.system.Time(...),client.system.Reboot(...), etc.). Service-agnostic: new gNOI services (Healthz, Cert, File, OS, …) plug in as new properties without touching existing code.GnoiClient("host:port"), intended forFakeGnoiServerand localhost helpers.GnoiClient("unix:///var/run/gnmi/gnmi.sock"), the natural transport for sibling-container IPC. No TLS, no extra code path — gRPC handlesunix://natively throughinsecure_channel.GnoiClient(target, credentials=creds, options=...)with credentials built viagrpc.ssl_channel_credentials(...), intended for the production telemetry/gNOI server on:50052.system_pb2,types_pb2,common_pb2and matching_grpcmodules) regenerated against protobuf 4.x / 5.x. No runtime dependency ongnoi.protofiles at install time.FakeGnoiServer+FakeSystemServicertest doubles (insonic_py_common.grpc.gnoi.testing) for unit tests of code that drives gNOI — replays scripted responses, records call history, programmable per-method errors.2. Wheel propagation — available in every container
SONIC_PY_COMMON_PY3already feeds into every trixie-based container viadocker-config-engine-trixie. Addinggrpcioandprotobuftosetup.py'sinstall_requiresis enough to get the framework, and its runtime deps, onto every container that shipssonic_py_common.3. UDS bind-mount (allow-list)
Bind-mount
/var/run/gnmi:/var/run/gnmi:rointo a small allow-list of in-box infrastructure containers —swss,syncd,pmon— so processes in those containers can reach the local gNMI/gNOI server through its Unix domain socket without TLS. Clients only need toconnect(2)the existing socket;:roblocks creating, deleting, or replacing files in the directory.The
gnmicontainer (which owns the server and creates the socket) already has it as:rwviarules/docker-gnmi.mk— no change there.Implemented in
files/build_templates/docker_image_ctl.j2, the central template that emits thedocker createinvocation for every container. The conditional adds the mount only for the allow-listed container names; future additions are one-line edits to the same block.Why allow-list, not blanket. Even with
:ro, the mount only governs filesystem operations; any container with the mount canconnect(2)to the socket and drive the full gNOI surface (Reboot, SetPackage, File.Put, OS.Install). Adding the mount to network-facing containers (snmp, restapi, bgp, dhcp_relay, ...) would convert a compromise of any of those into device takeover. Additional containers should be added by explicit request, not by default.End-to-end validation
Verified live on a vlab-01 KVM testbed by manually dropping the framework + grpcio runtime into the
gnmicontainer (which already has the UDS mount in stock images) and exercising all three transports against the running gNMI/gNOI server in the same container:The mTLS demo connects to the live
/usr/sbin/telemetryprocess on:50052using thestreamingtelemetryserver.cer/dsmsroot.{cer,key}cert bundle from/etc/sonic/telemetry/. Thessl_target_name_overrideis required because the server cert is CN-only (no SAN); recent gRPC stacks reject pure CN validation by default.The UDS demo talks to the same gNOI server through
/var/run/gnmi/gnmi.sock— no certs, no TLS, just the bind-mount. Once this PR merges, the allow-listed containers (swss,syncd,pmon) get that bind-mount in stock images and can run the same demo unchanged.Same demo from a different container (
pmon, TCP mTLS)Confirmed the framework works unchanged from a non-
gnmiallow-listed container.pmonships the gRPC SDK in the stock image, so the only thing this PR contributes at runtime is thesonic_py_common.grpc.gnoisubpackage (carried in the existingSONIC_PY_COMMON_PY3wheel that all containers already install).Evidence that
grpcio/protobufare preinstalled in stockpmon:(
Required-byempty → grpcio is installed at the base-image layer, not pulled in by any sonic wheel.)Running the mTLS demo from
pmonagainst the live telemetry server on:50052(pmon shares the host network, so127.0.0.1:50052reaches the same/usr/sbin/telemetryprocess thegnmicontainer runs):Test coverage